RFC: Add USBDEVFS_TRY_DISCONNECT ioctl

PATCH Alan Stern 3 participants 6 messages Aug 24, 2011 1 message not in the archive

Subsystems (4)

SCSI USB USB mass storage driver SCSI SG driver

lore

Share & tools

lore

A message that is not in the archive (sent off-list or not yet received): [email protected]

Alan Stern

author maintainer

2011-08-24 20:32 UTC #1 lore raw

Quoted 12 lines · Greg KH

Quoted 10 lines · Alan Stern
Quoted 4 lines
It's of course racey for userspace to check
whether a device is busy and then disconnect the driver, but the "try disconnect" ioctl could cause the driver to disconnect itself. In the end there wasn't a very good solution to this problem.
In principle it's quite doable. The question is whether people will stand for it. Greg KH has already come out against the idea, although perhaps he could be persuaded. It doesn't help that this new mechanism would be used only for USB; if other subsystems could benefit from it too then it would be easier to sell.

Quoted 7 lines

I still don't think this would really work that well. But it wouldn't be just USB devices that need it, in the future we will have lots of detatched storage devices that guests want access to in "native" mode (Thunderbolt being one good example.)
But feel free to change my mind with a patch showing just how this all would work :)

Okay, here's a sample patch. Actually it's three patches, listed one after another, but people can apply it like a single patch.

1. Introduce the USBDEVFS_TRY_DISCONNECT ioctl and the check_busy
callback it uses. Implement the callback in the usbfs driver; this gives a way for programs to unbind kernel drivers without unbinding other userspace drivers.

2. Implement device-file reference tracking in the SCSI layer,
and the device_open and device_close callbacks it uses.

3. Implement both check_busy and device_{open,close} in
usb-storage.

Attached is a sample program that demonstrates how to use this. It will unbind usb-storage from a USB interface if the storage device isn't open or mounted, otherwise it will fail with a "device busy" error. It will also unbind any other USB kernel driver from an interface -- but it won't unbind a usbfs userspace driver that has claimed an interface.

Alan Stern

Patch 1: USBDEVFS_TRY_DISCONNECT

 drivers/usb/core/devio.c     |   13 +++++++++++++
 include/linux/usb.h          |    5 +++++
 include/linux/usbdevice_fs.h |    1 +
 3 files changed, 19 insertions(+)

include/linux/usbdevice_fs.h +1 -0

Index: usb-3.1/include/linux/usbdevice_fs.h
===================================================================
@@ -204,4 +204,5 @@ struct usbdevfs_ioctl32 {
 #define USBDEVFS_CONNECT           _IO('U', 23)
 #define USBDEVFS_CLAIM_PORT        _IOR('U', 24, unsigned int)
 #define USBDEVFS_RELEASE_PORT      _IOR('U', 25, unsigned int)
+#define USBDEVFS_TRY_DISCONNECT    _IO('U', 26)
 #endif /* _LINUX_USBDEVICE_FS_H */

include/linux/usb.h +5 -0

Index: usb-3.1/include/linux/usb.h
===================================================================
@@ -812,6 +812,9 @@ struct usbdrv_wrap {
  *	post_reset method is called.
  * @post_reset: Called by usb_reset_device() after the device
  *	has been reset
+ * @check_busy: Called to see whether the device is currently
+ *	busy before doing a user-initiated unbind.  The driver must not
+ *	allow the device to become busy after this routine returns 0.
  * @id_table: USB drivers use ID table to support hotplugging.
  *	Export this with MODULE_DEVICE_TABLE(usb,...).  This must be set
  *	or your driver's probe function will never get called.

@@ -858,6 +861,8 @@ struct usb_driver {
 	int (*pre_reset)(struct usb_interface *intf);
 	int (*post_reset)(struct usb_interface *intf);
 
+	int (*check_busy)(struct usb_interface *intf);
+
 	const struct usb_device_id *id_table;
 
 	struct usb_dynids dynids;

drivers/usb/core/devio.c +13 -0

Index: usb-3.1/drivers/usb/core/devio.c
===================================================================
@@ -516,12 +516,18 @@ static int driver_resume(struct usb_inte
 	return 0;
 }
 
+static int driver_check_busy(struct usb_interface *intf)
+{
+	return -EBUSY;
+}
+
 struct usb_driver usbfs_driver = {
 	.name =		"usbfs",
 	.probe =	driver_probe,
 	.disconnect =	driver_disconnect,
 	.suspend =	driver_suspend,
 	.resume =	driver_resume,
+	.check_busy =	driver_check_busy,
 };
 
 static int claimintf(struct dev_state *ps, unsigned int ifnum)

@@ -1647,9 +1653,16 @@ static int proc_ioctl(struct dev_state *
 	else switch (ctl->ioctl_code) {
 
 	/* disconnect kernel driver from interface */
+	case USBDEVFS_TRY_DISCONNECT:
 	case USBDEVFS_DISCONNECT:
 		if (intf->dev.driver) {
 			driver = to_usb_driver(intf->dev.driver);
+			if (ctl->ioctl_code == USBDEVFS_TRY_DISCONNECT &&
+					driver->check_busy) {
+				retval = driver->check_busy(intf);
+				if (retval)
+					break;
+			}
 			dev_dbg(&intf->dev, "disconnect by usbfs\n");
 			usb_driver_release_interface(driver, intf);
 		} else

Patch 2: SCSI device file open reference tracking

 drivers/scsi/hosts.c       |   32 ++++++++++++++++++++++++++++++++
 drivers/scsi/sd.c          |    7 +++++++
 drivers/scsi/sg.c          |    8 +++++++-
 drivers/scsi/sr.c          |    3 ++-
 include/scsi/scsi_device.h |    2 ++
 include/scsi/scsi_host.h   |   19 +++++++++++++++++++
 6 files changed, 69 insertions(+), 2 deletions(-)

include/scsi/scsi_host.h +19 -0

Index: usb-3.1/include/scsi/scsi_host.h
===================================================================
@@ -356,6 +356,25 @@ struct scsi_host_template {
 	enum blk_eh_timer_return (*eh_timed_out)(struct scsi_cmnd *);
 
 	/*
+	 * This function is called when a device file is about to be opened;
+	 * if the function returns nonzero then the open fails.  The low
+	 * level driver can use this to track the number of open device file
+	 * references (including mounts), and thereby know when it is safe to
+	 * unregister the host without disrupting mounted filesystems or
+	 * the like.
+	 *
+	 * Status: OPTIONAL
+	 */
+	int (*device_open)(struct Scsi_Host *, struct scsi_device *);
+
+	/*
+	 * This function is called after a device file is closed.
+	 *
+	 * Status: OPTIONAL
+	 */
+	void (*device_close)(struct Scsi_Host *, struct scsi_device *);
+
+	/*
 	 * Name of proc directory
 	 */
 	const char *proc_name;

include/scsi/scsi_device.h +2 -0

Index: usb-3.1/include/scsi/scsi_device.h
===================================================================
@@ -361,6 +361,8 @@ extern struct scsi_event *sdev_evt_alloc
 extern void sdev_evt_send(struct scsi_device *sdev, struct scsi_event *evt);
 extern void sdev_evt_send_simple(struct scsi_device *sdev,
 			  enum scsi_device_event evt_type, gfp_t gfpflags);
+extern int scsi_device_open(struct scsi_device *sdev);
+extern void scsi_device_close(struct scsi_device *sdev);
 extern int scsi_device_quiesce(struct scsi_device *sdev);
 extern void scsi_device_resume(struct scsi_device *sdev);
 extern void scsi_target_quiesce(struct scsi_target *);

drivers/scsi/hosts.c +32 -0

Index: usb-3.1/drivers/scsi/hosts.c
===================================================================
@@ -577,3 +577,35 @@ void scsi_flush_work(struct Scsi_Host *s
 	flush_workqueue(shost->work_q);
 }
 EXPORT_SYMBOL_GPL(scsi_flush_work);
+
+/**
+ * scsi_device_open - Check whether it's okay to open a device file
+ * @sdev:	Device whose file will be opened.
+ *
+ * Return value:
+ *	0 if open is allowed; negative errno otherwise
+ **/
+int scsi_device_open(struct scsi_device *sdev)
+{
+	struct Scsi_Host *shost = sdev->host;
+	struct scsi_host_template *sht = shost->hostt;
+
+	if (!sht->device_open)
+		return 0;
+	return sht->device_open(shost, sdev);
+}
+EXPORT_SYMBOL_GPL(scsi_device_open);
+
+/**
+ * scsi_device_close - Announce that a device file has been closed
+ * @sdev:	Device whose file has been closed.
+ **/
+void scsi_device_close(struct scsi_device *sdev)
+{
+	struct Scsi_Host *shost = sdev->host;
+	struct scsi_host_template *sht = shost->hostt;
+
+	if (sht->device_close)
+		sht->device_close(shost, sdev);
+}
+EXPORT_SYMBOL_GPL(scsi_device_close);

drivers/scsi/sd.c +7 -0

Index: usb-3.1/drivers/scsi/sd.c
===================================================================
@@ -935,6 +935,10 @@ static int sd_open(struct block_device *
 
 	sdev = sdkp->device;
 
+	retval = scsi_device_open(sdev);
+	if (retval)
+		goto error_open;
+
 	retval = scsi_autopm_get_device(sdev);
 	if (retval)
 		goto error_autopm;

@@ -985,6 +989,8 @@ static int sd_open(struct block_device *
 error_out:
 	scsi_autopm_put_device(sdev);
 error_autopm:
+	scsi_device_close(sdev);
+error_open:
 	scsi_disk_put(sdkp);
 	return retval;	
 }

@@ -1020,6 +1026,7 @@ static int sd_release(struct gendisk *di
 	 */
 
 	scsi_autopm_put_device(sdev);
+	scsi_device_close(sdev);
 	scsi_disk_put(sdkp);
 	return 0;
 }

drivers/scsi/sr.c +2 -1

Index: usb-3.1/drivers/scsi/sr.c
===================================================================
@@ -623,7 +623,7 @@ static int sr_open(struct cdrom_device_i
 	if (!scsi_block_when_processing_errors(sdev))
 		goto error_out;
 
-	return 0;
+	retval = scsi_device_open(sdev);
 
 error_out:
 	return retval;

@@ -636,6 +636,7 @@ static void sr_release(struct cdrom_devi
 	if (cd->device->sector_size > 2048)
 		sr_set_blocklength(cd, 2048);
 
+	scsi_device_close(cd->device);
 }
 
 static int sr_probe(struct device *dev)

drivers/scsi/sg.c +7 -1

Index: usb-3.1/drivers/scsi/sg.c
===================================================================
@@ -247,10 +247,14 @@ sg_open(struct inode *inode, struct file
 	if (retval)
 		goto sg_put;
 
-	retval = scsi_autopm_get_device(sdp->device);
+	retval = scsi_device_open(sdp->device);
 	if (retval)
 		goto sdp_put;
 
+	retval = scsi_autopm_get_device(sdp->device);
+	if (retval)
+		goto scsi_device_close;
+
 	if (!((flags & O_NONBLOCK) ||
 	      scsi_block_when_processing_errors(sdp->device))) {
 		retval = -ENXIO;

@@ -310,6 +314,8 @@ sg_open(struct inode *inode, struct file
 error_out:
 	if (retval) {
 		scsi_autopm_put_device(sdp->device);
+scsi_device_close:
+		scsi_device_close(sdp->device);
 sdp_put:
 		scsi_device_put(sdp->device);
 	}

Patch 3: implementation for usb-storage

 drivers/usb/storage/scsiglue.c |   30 ++++++++++++++++++++++++++++++
 drivers/usb/storage/usb.c      |   21 +++++++++++++++++++++
 drivers/usb/storage/usb.h      |    1 +
 3 files changed, 52 insertions(+)

drivers/usb/storage/usb.h +1 -0

Index: usb-3.1/drivers/usb/storage/usb.h
===================================================================
@@ -135,6 +135,7 @@ struct us_data {
 	struct scsi_cmnd	*srb;		 /* current srb		*/
 	unsigned int		tag;		 /* current dCBWTag	*/
 	char			scsi_name[32];	 /* scsi_host name	*/
+	int			open_count;	 /* open file refs	*/
 
 	/* control and bulk communications data */
 	struct urb		*current_urb;	 /* USB requests	 */

drivers/usb/storage/usb.c +21 -0

Index: usb-3.1/drivers/usb/storage/usb.c
===================================================================
@@ -217,6 +217,26 @@ int usb_stor_post_reset(struct usb_inter
 }
 EXPORT_SYMBOL_GPL(usb_stor_post_reset);
 
+int usb_stor_check_busy(struct usb_interface *iface)
+{
+	struct us_data *us = usb_get_intfdata(iface);
+	struct Scsi_Host *host = us_to_host(us);
+	int rc = -EBUSY;
+
+	US_DEBUGP("%s\n", __func__);
+
+	/* If there are no open file references then we aren't busy */
+	scsi_lock(host);
+	if (us->open_count <= 0) {
+		us->open_count = -1;	/* No more opens allowed */
+		rc = 0;
+	}
+	scsi_unlock(host);
+
+	return rc;
+}
+EXPORT_SYMBOL_GPL(usb_stor_check_busy);
+
 /*
  * fill_inquiry_response takes an unsigned char array (which must
  * be at least 36 characters) and populates the vendor name,

@@ -1060,6 +1080,7 @@ static struct usb_driver usb_storage_dri
 	.reset_resume =	usb_stor_reset_resume,
 	.pre_reset =	usb_stor_pre_reset,
 	.post_reset =	usb_stor_post_reset,
+	.check_busy =	usb_stor_check_busy,
 	.id_table =	usb_storage_usb_ids,
 	.supports_autosuspend = 1,
 	.soft_unbind =	1,

drivers/usb/storage/scsiglue.c +30 -0

Index: usb-3.1/drivers/usb/storage/scsiglue.c
===================================================================
@@ -411,6 +411,32 @@ void usb_stor_report_bus_reset(struct us
 	scsi_unlock(host);
 }
 
+/* Check whether to allow a device file to be opened. */
+static int device_open(struct Scsi_Host *host, struct scsi_device *sdev)
+{
+	struct us_data *us = host_to_us(host);
+	int rc = -EIO;
+
+	/* Allow the open if a disconnect hasn't been requested */
+	scsi_lock(host);
+	if (us->open_count >= 0) {
+		++us->open_count;
+		rc = 0;
+	}
+	scsi_unlock(host);
+	return rc;
+}
+
+/* A device file has been closed. */
+static void device_close(struct Scsi_Host *host, struct scsi_device *sdev)
+{
+	struct us_data *us = host_to_us(host);
+
+	scsi_lock(host);
+		--us->open_count;
+	scsi_unlock(host);
+}
+
 /***********************************************************************
  * /proc/scsi/ functions
  ***********************************************************************/

@@ -537,6 +563,10 @@ struct scsi_host_template usb_stor_host_
 	.eh_device_reset_handler =	device_reset,
 	.eh_bus_reset_handler =		bus_reset,
 
+	/* open file reference notifiers */
+	.device_open =			device_open,
+	.device_close =			device_close,
+
 	/* queue commands only, only one command per LUN */
 	.can_queue =			1,
 	.cmd_per_lun =			1,

/* usb-try-discon -- try to claim a USB mass-storage interface */

#include <stdio.h>
#include <unistd.h>
#include <stdlib.h>
#include <fcntl.h>
#include <errno.h>
#include <sys/ioctl.h>

#include <linux/usbdevice_fs.h>
#define USBDEVFS_TRY_DISCONNECT _IO('U', 26)

int main(int argc, char **argv)

{
	const char *filename;
	unsigned intf;
	int fd;
	int rc;
	int c;
	struct usbdevfs_ioctl uioctl;

	if (argc != 3) {
		fprintf(stderr, "Usage: usb-try-discon devpath intf-num\n");
		return 1;
	}
	filename = argv[1];
	intf = atoi(argv[2]);

	fd = open(filename, O_WRONLY);
	if (fd < 0) {
		perror("Error opening device file");
		return 1;
	}

	printf("Trying to disconnect driver of %s interface %d\n",
			filename, intf);
	uioctl.ifno = intf;
	uioctl.ioctl_code = USBDEVFS_TRY_DISCONNECT;
	uioctl.data = NULL;
	rc = ioctl(fd, USBDEVFS_IOCTL, &uioctl);
	if (rc < 0 && errno == ENODATA) {
		printf("No driver was connected.\n");
	} else if (rc < 0) {
		perror("Error in ioctl");
		return 1;
	} else {
		printf("Disconnect successful\n");
	}

	printf("Trying to claim the interface\n");
	rc = ioctl(fd, USBDEVFS_CLAIMINTERFACE, &intf);
	if (rc < 0) {
		perror("Error in ioctl");
		return 1;
	}
	printf("Claim successful\n");

	printf("Press Enter to continue...");
	do {
		c = getc(stdin);
	} while (c != '\n' && c != EOF);

	printf("Releasing interface\n");
	rc = ioctl(fd, USBDEVFS_RELEASEINTERFACE, &intf);
	if (rc < 0) {
		perror("Error in ioctl");
		return 1;
	}
	printf("Release successful\n");

	close(fd);
	return 0;
}

Greg KH

2011-08-24 20:45 UTC #2 lore raw

Quoted 10 lines · Alan Stern

Okay, here's a sample patch. Actually it's three patches, listed one after another, but people can apply it like a single patch.
1. Introduce the USBDEVFS_TRY_DISCONNECT ioctl and the check_busy
callback it uses. Implement the callback in the usbfs driver; this gives a way for programs to unbind kernel drivers without unbinding other userspace drivers.
2. Implement device-file reference tracking in the SCSI layer,
and the device_open and device_close callbacks it uses.

Does this handle if the filesystem is being created or fscked, as it's not mounted at that time.

Quoted 4 lines

drivers/usb/core/devio.c +0 -0

@@ -1647,9 +1653,16 @@ static int proc_ioctl(struct dev_state *
 	else switch (ctl->ioctl_code) {
 
 	/* disconnect kernel driver from interface */

drivers/usb/core/devio.c static int proc_ioctl(struct dev_state * · line 1665

Quoted 10 lines

drivers/usb/core/devio.c +7 -0

+	case USBDEVFS_TRY_DISCONNECT:
 	case USBDEVFS_DISCONNECT:
 		if (intf->dev.driver) {
 			driver = to_usb_driver(intf->dev.driver);
+			if (ctl->ioctl_code == USBDEVFS_TRY_DISCONNECT &&
+					driver->check_busy) {
+				retval = driver->check_busy(intf);
+				if (retval)
+					break;
+			}

I don't like the fact that if a driver doesn't contain check_busy() then it will automatically fall back to looking like it was a DISCONNECT call, which could give userspace a false sense of "everything was fine" when trying this out.

Why not fail if that callback is not present?

I can't comment on the scsi layer, but what about devices that don't use scsi? Like "raw" block drivers?

thanks,

greg k-h

Hans de Goede

2011-08-24 21:01 UTC #3 lore raw

Hi,

First of all: A big thanks to Alan for working on this. From my POV the code looks good, but I'm only familiar with the usb parts and not with the scsi parts.

One generic concern is the use of scsi_lock(host) in the scsi_device_open / close callbacks. We need to be sure non of the callers of these can already be holding the lock.

Quoted 18 lines · Greg KH

Quoted 10 lines · Alan Stern
Okay, here's a sample patch. Actually it's three patches, listed one after another, but people can apply it like a single patch.
1. Introduce the USBDEVFS_TRY_DISCONNECT ioctl and the check_busy
callback it uses. Implement the callback in the usbfs driver; this gives a way for programs to unbind kernel drivers without unbinding other userspace drivers.
2. Implement device-file reference tracking in the SCSI layer,
and the device_open and device_close callbacks it uses.
Does this handle if the filesystem is being created or fscked, as it's not mounted at that time.
Quoted 3 lines
drivers/usb/core/devio.c +0 -0
@@ -1647,9 +1653,16 @@ static int proc_ioctl(struct dev_state *
  	else switch (ctl->ioctl_code) {
 

Quoted 16 lines

drivers/usb/core/devio.c static int proc_ioctl(struct dev_state * · line 1665
Quoted 11 lines
drivers/usb/core/devio.c +7 -0
  	/* disconnect kernel driver from interface */
+	case USBDEVFS_TRY_DISCONNECT:
  	case USBDEVFS_DISCONNECT:
  		if (intf->dev.driver) {
  			driver = to_usb_driver(intf->dev.driver);
+			if (ctl->ioctl_code == USBDEVFS_TRY_DISCONNECT&&
+					driver->check_busy) {
+				retval = driver->check_busy(intf);
+				if (retval)
+					break;
+			}
I don't like the fact that if a driver doesn't contain check_busy() then it will automatically fall back to looking like it was a DISCONNECT call, which could give userspace a false sense of "everything was fine" when trying this out.

I've to agree with Greg here, what to do in case of a driver not implementing check_busy and thus not really offering USBDEVFS_TRY_DISCONNECT is policy and thus should be left to userspace. I suggest we just return -ENOTTY in case of USBDEVFS_TRY_DISCONNECT and the bound driver does not have checkbusy, then userspace can decide wether to fallback to a regular disconnect, or to give up.

Regards,

Hans

Alan Stern

author maintainer

2011-08-24 21:23 UTC #4 lore raw

Quoted 9 lines · Hans de Goede

Hi,
First of all: A big thanks to Alan for working on this. From my POV the code looks good, but I'm only familiar with the usb parts and not with the scsi parts.
One generic concern is the use of scsi_lock(host) in the scsi_device_open / close callbacks. We need to be sure non of the callers of these can already be holding the lock.

The callers always runs in process context (because file open/close always uses process context) and the lock is a spinlock. So no, the lock can't be held at the wrong time.

Quoted 7 lines · Greg KH

Quoted 5 lines
I don't like the fact that if a driver doesn't contain check_busy() then it will automatically fall back to looking like it was a DISCONNECT call, which could give userspace a false sense of "everything was fine" when trying this out.

Quoted 6 lines

I've to agree with Greg here, what to do in case of a driver not implementing check_busy and thus not really offering USBDEVFS_TRY_DISCONNECT is policy and thus should be left to userspace. I suggest we just return -ENOTTY in case of USBDEVFS_TRY_DISCONNECT and the bound driver does not have checkbusy, then userspace can decide wether to fallback to a regular disconnect, or to give up.

Okay, I'll change the patch. But the real question is whether the basic idea is acceptable.

Alan Stern

author maintainer

2011-08-24 21:18 UTC #5 lore raw

In reply to Greg KH

Quoted 9 lines · Greg KH

Quoted 8 lines · Alan Stern
Okay, here's a sample patch. Actually it's three patches, listed one after another, but people can apply it like a single patch.
1. Introduce the USBDEVFS_TRY_DISCONNECT ioctl and the check_busy
callback it uses. Implement the callback in the usbfs driver; this gives a way for programs to unbind kernel drivers without unbinding other userspace drivers.

Quoted 5 lines

Quoted 2 lines
2. Implement device-file reference tracking in the SCSI layer,
and the device_open and device_close callbacks it uses.
Does this handle if the filesystem is being created or fscked, as it's not mounted at that time.

Yes, because the device file is held open by mkfs or fsck. You can test this easily enough, in a nondestructive way, by using this little shell script:

echo -n 'Press RETURN to continue... '
read </dev/tty

Stick that in a file, and run the file with input redirected to the appropriate /dev/sd? file.

Quoted 20 lines

Quoted 4 lines
drivers/usb/core/devio.c +0 -0
@@ -1647,9 +1653,16 @@ static int proc_ioctl(struct dev_state *
 	else switch (ctl->ioctl_code) {
 
 	/* disconnect kernel driver from interface */
drivers/usb/core/devio.c static int proc_ioctl(struct dev_state * · line 1665
Quoted 10 lines
drivers/usb/core/devio.c +7 -0
+	case USBDEVFS_TRY_DISCONNECT:
 	case USBDEVFS_DISCONNECT:
 		if (intf->dev.driver) {
 			driver = to_usb_driver(intf->dev.driver);
+			if (ctl->ioctl_code == USBDEVFS_TRY_DISCONNECT &&
+					driver->check_busy) {
+				retval = driver->check_busy(intf);
+				if (retval)
+					break;
+			}
I don't like the fact that if a driver doesn't contain check_busy() then it will automatically fall back to looking like it was a DISCONNECT call, which could give userspace a false sense of "everything was fine" when trying this out.

Quoted 1 line

Why not fail if that callback is not present?

It could be made to work that way. I had to choose, so I chose to make TRY_DISCONNECT work like DISCONNECT when the callback was missing. Doing it as you suggest might be better though, because then the user program could decide what to do if the kernel driver doesn't support TRY_DISCONNECT.

What would be a good error code for that case? -EOPNOTSUPP? Or the traditional -ENOTTY?

Quoted 2 lines

I can't comment on the scsi layer, but what about devices that don't use scsi? Like "raw" block drivers?

You mean things like Pete Zaitcev's ub driver? They would need an equivalent change.

Remember, this was just a trial patch to demonstrate the idea. It wasn't intended to be complete. (Another thing I skipped was updating the usb_driver structures in all the little submodules in the drivers/usb/storage directory.)

Alan Stern

Greg KH

2011-08-24 21:35 UTC #6 lore raw

Quoted 21 lines · Alan Stern

Quoted 9 lines · Greg KH
Quoted 8 lines · Alan Stern
Okay, here's a sample patch. Actually it's three patches, listed one after another, but people can apply it like a single patch.
1. Introduce the USBDEVFS_TRY_DISCONNECT ioctl and the check_busy
callback it uses. Implement the callback in the usbfs driver; this gives a way for programs to unbind kernel drivers without unbinding other userspace drivers.
Quoted 5 lines
Quoted 2 lines
2. Implement device-file reference tracking in the SCSI layer,
and the device_open and device_close callbacks it uses.
Does this handle if the filesystem is being created or fscked, as it's not mounted at that time.
Yes, because the device file is held open by mkfs or fsck. You can test this easily enough, in a nondestructive way, by using this little shell script:

Quoted 5 lines

echo -n 'Press RETURN to continue... '
read </dev/tty
Stick that in a file, and run the file with input redirected to the appropriate /dev/sd? file.

Ok, good, just wondering.

Quoted 22 lines

Quoted 20 lines
Quoted 4 lines
drivers/usb/core/devio.c +0 -0
@@ -1647,9 +1653,16 @@ static int proc_ioctl(struct dev_state *
 	else switch (ctl->ioctl_code) {
 
 	/* disconnect kernel driver from interface */
drivers/usb/core/devio.c static int proc_ioctl(struct dev_state * · line 1665
Quoted 10 lines
drivers/usb/core/devio.c +7 -0
+	case USBDEVFS_TRY_DISCONNECT:
 	case USBDEVFS_DISCONNECT:
 		if (intf->dev.driver) {
 			driver = to_usb_driver(intf->dev.driver);
+			if (ctl->ioctl_code == USBDEVFS_TRY_DISCONNECT &&
+					driver->check_busy) {
+				retval = driver->check_busy(intf);
+				if (retval)
+					break;
+			}
I don't like the fact that if a driver doesn't contain check_busy() then it will automatically fall back to looking like it was a DISCONNECT call, which could give userspace a false sense of "everything was fine" when trying this out.
Quoted 1 line
Why not fail if that callback is not present?

Quoted 8 lines

It could be made to work that way. I had to choose, so I chose to make TRY_DISCONNECT work like DISCONNECT when the callback was missing. Doing it as you suggest might be better though, because then the user program could decide what to do if the kernel driver doesn't support TRY_DISCONNECT.
What would be a good error code for that case? -EOPNOTSUPP? Or the traditional -ENOTTY?

-ENOTTY is the correct thing here.

Quoted 5 lines

Quoted 2 lines
I can't comment on the scsi layer, but what about devices that don't use scsi? Like "raw" block drivers?
You mean things like Pete Zaitcev's ub driver? They would need an equivalent change.

Ok, and if we return the correct error code, as shown above, if the TRY_DISCONNECT was not there, then userspace could fall back on the "what do I do now?" logic.

If you can get the scsi people to accept that part, I'll take the usb portion, nice job.

greg k-h