RFC: Add USBDEVFS_TRY_DISCONNECT ioctl

RFC: Add USBDEVFS_TRY_DISCONNECT ioctl

[Alan Stern]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 13689 bytes --]

On Sun, 21 Aug 2011, Greg KH wrote:

> On Fri, Aug 19, 2011 at 10:56:23PM -0400, Alan Stern wrote:
> > >  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.
> 
> 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(+)

Index: usb-3.1/include/linux/usbdevice_fs.h
===================================================================
--- usb-3.1.orig/include/linux/usbdevice_fs.h
+++ 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 */
Index: usb-3.1/include/linux/usb.h
===================================================================
--- usb-3.1.orig/include/linux/usb.h
+++ 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;
Index: usb-3.1/drivers/usb/core/devio.c
===================================================================
--- usb-3.1.orig/drivers/usb/core/devio.c
+++ 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(-)

Index: usb-3.1/include/scsi/scsi_host.h
===================================================================
--- usb-3.1.orig/include/scsi/scsi_host.h
+++ 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;
Index: usb-3.1/include/scsi/scsi_device.h
===================================================================
--- usb-3.1.orig/include/scsi/scsi_device.h
+++ 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 *);
Index: usb-3.1/drivers/scsi/hosts.c
===================================================================
--- usb-3.1.orig/drivers/scsi/hosts.c
+++ 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);
Index: usb-3.1/drivers/scsi/sd.c
===================================================================
--- usb-3.1.orig/drivers/scsi/sd.c
+++ 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;
 }
Index: usb-3.1/drivers/scsi/sr.c
===================================================================
--- usb-3.1.orig/drivers/scsi/sr.c
+++ 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)
Index: usb-3.1/drivers/scsi/sg.c
===================================================================
--- usb-3.1.orig/drivers/scsi/sg.c
+++ 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(+)

Index: usb-3.1/drivers/usb/storage/usb.h
===================================================================
--- usb-3.1.orig/drivers/usb/storage/usb.h
+++ 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	 */
Index: usb-3.1/drivers/usb/storage/usb.c
===================================================================
--- usb-3.1.orig/drivers/usb/storage/usb.c
+++ 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,
Index: usb-3.1/drivers/usb/storage/scsiglue.c
===================================================================
--- usb-3.1.orig/drivers/usb/storage/scsiglue.c
+++ 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,

[-- Attachment #2: Type: TEXT/plain, Size: 1604 bytes --]

/* 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;
}

Re: RFC: Add USBDEVFS_TRY_DISCONNECT ioctl

[Greg KH]

On Wed, Aug 24, 2011 at 04:32:31PM -0400, Alan Stern wrote:
> 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.

> @@ -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;
> +			}

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

Re: RFC: Add USBDEVFS_TRY_DISCONNECT ioctl

[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.

On 08/24/2011 10:45 PM, Greg KH wrote:
> On Wed, Aug 24, 2011 at 04:32:31PM -0400, Alan Stern wrote:
>> 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.
>
>> @@ -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;
>> +			}
>
> 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

Re: RFC: Add USBDEVFS_TRY_DISCONNECT ioctl

[Alan Stern]

On Wed, 24 Aug 2011, Greg KH wrote:

> On Wed, Aug 24, 2011 at 04:32:31PM -0400, Alan Stern wrote:
> > 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.

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.

> > @@ -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;
> > +			}
> 
> 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?

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?

> 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

Re: RFC: Add USBDEVFS_TRY_DISCONNECT ioctl

[Alan Stern]

On Wed, 24 Aug 2011, Hans de Goede wrote:

> 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.


> On 08/24/2011 10:45 PM, Greg KH wrote:

> > 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.

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

Alan Stern

Re: RFC: Add USBDEVFS_TRY_DISCONNECT ioctl

[Greg KH]

On Wed, Aug 24, 2011 at 05:18:57PM -0400, Alan Stern wrote:
> On Wed, 24 Aug 2011, Greg KH wrote:
> 
> > On Wed, Aug 24, 2011 at 04:32:31PM -0400, Alan Stern wrote:
> > > 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.
> 
> 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.

Ok, good, just wondering.

> > > @@ -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;
> > > +			}
> > 
> > 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?
> 
> 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.

> > 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