Bug 24343 - Unintended side-effect when calling DriveDetach()
Unintended side-effect when calling DriveDetach()
Status: RESOLVED FIXED
Product: udisks
Classification: Unclassified
Component: detection
unspecified
Other All
: medium normal
Assigned To: David Zeuthen (not reading bugmail)
https://launchpad.net//bugs/404185
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2009-10-06 05:06 UTC by Martin Pitt
Modified: 2009-10-29 08:30 UTC (History)
3 users (show)

See Also:
i915 platform:
i915 features:


Attachments
dmidecode output (7.67 KB, text/plain)
2009-10-07 00:22 UTC, Martin Pitt
Details
lsusb -t (324 bytes, text/plain)
2009-10-29 02:39 UTC, Richard Hughes
Details
sudo lsusb -vvv (36.06 KB, text/plain)
2009-10-29 02:40 UTC, Richard Hughes
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Pitt 2009-10-06 05:06:01 UTC
Some laptops like the Dell Mini 10 have builtin SD card readers which are wired through an internal USB bus. The current behaviour is that if you click the eject button in nautilus, the card reader device gets ejected and disconnected, and cannot be reactivated ever, except by a reboot.

The cause of this is that DK-d's update_info_drive() sets "can_detach" for all USB devices currently, which causes devkit-disks-helper-drive-detach to be called on them.

So clearly this is too eager. Obviously there is no way to programmatically tell whether a particular USB device can actually be physically detached or whether it's sitting inside a case. So I see the following options right now:

 * If we can tell whether the device has removable media (like card readers), don't detach the reader device. Otherwise, continue to set can_detach. This still errs on the side of breakage, but at least would fix the behaviour with those card readers. I'm currently not sure whether this is feasible, I asked the original reporter for a dk-disks dump to check the properties.

 * Don't set can_detach for all USB devices, and just add udev rules which set ID_DRIVE_DETACHABLE for some known safe cases. 

  This would again introduce a whitelist, something which we hoped to get rid of with https://bugzilla.gnome.org/show_bug.cgi?id=576587.

So my question is, why was this current can_detach behaviour introduced? Are there some devices where not even eject is enough? Years ago, when I tested this with an iPod, umount wasn't sufficient, but eject made the "unsafe to remove blabla" warning go away.
Comment 1 David Zeuthen (not reading bugmail) 2009-10-06 08:33:37 UTC
(In reply to comment #0)
> Some laptops like the Dell Mini 10 have builtin SD card readers which are wired
> through an internal USB bus. The current behaviour is that if you click the
> eject button in nautilus, the card reader device gets ejected and disconnected,
> and cannot be reactivated ever, except by a reboot.

Huh, that's interesting, I never thought of that problem ;-). 

(Also, the fact that Dell is so cheap that they use USB-connected SD readers is interesting in it's own right.)

> The cause of this is that DK-d's update_info_drive() sets "can_detach" for all
> USB devices currently, which causes devkit-disks-helper-drive-detach to be
> called on them.
> 
> So clearly this is too eager. Obviously there is no way to programmatically
> tell whether a particular USB device can actually be physically detached or
> whether it's sitting inside a case. So I see the following options right now:
> 
>  * If we can tell whether the device has removable media (like card readers),
> don't detach the reader device. Otherwise, continue to set can_detach. This
> still errs on the side of breakage, but at least would fix the behaviour with
> those card readers. I'm currently not sure whether this is feasible, I asked
> the original reporter for a dk-disks dump to check the properties.
> 
>  * Don't set can_detach for all USB devices, and just add udev rules which set
> ID_DRIVE_DETACHABLE for some known safe cases. 
> 
>   This would again introduce a whitelist, something which we hoped to get rid
> of with https://bugzilla.gnome.org/show_bug.cgi?id=576587.
> 
> So my question is, why was this current can_detach behaviour introduced? Are
> there some devices where not even eject is enough? Years ago, when I tested
> this with an iPod, umount wasn't sufficient, but eject made the "unsafe to
> remove blabla" warning go away.

The different between DriveEject() and DriveDetach() is that the latter makes the whole device go away while the former only makes partitions go away. DriveEject() is sufficient on iPod and Kindle to make the "unsafe to remove blabla" warning go away.

Maybe we can look at the usb interface classes to avoid ejecting USB connected card readers? I also think the DMI data tells you whether a particular usb port has an external receptable - I remember Kay talking about that (adding to Cc). 

If this works for the Dell Mini maybe we can use this and vendors who are not properly populating DMI data will suffer (and we'd quirk this up via udev rules).

I don't know. Please also ask the original reporter for output of dmidecode and 'tree /sys'. Thanks.
Comment 2 Martin Pitt 2009-10-06 09:21:43 UTC
(In reply to comment #1)

> DriveEject() is sufficient on iPod and Kindle to make the "unsafe to remove
> blabla" warning go away.

Right, that's why I wondered why drive detaching was necessary. Just as a way to "clean up" and also reduce power consumption, or whether some devices really need it. (Just in case we need to disable can_detach right before a release, when time gets tight :) ).
 
> Maybe we can look at the usb interface classes to avoid ejecting USB connected
> card readers? I also think the DMI data tells you whether a particular usb port
> has an external receptable - I remember Kay talking about that (adding to Cc). 

That would be nice. The original report already has an udev dump (http://launchpadlibrarian.net/29486956/UdevDb.txt):

P: /devices/pci0000:00/0000:00:1d.7/usb1/1-3/1-3:1.0/host2/target2:0:0/2:0:0:0/block/sdb
E: ID_VENDOR=Generic-
E: ID_VENDOR_ENC=Generic-
E: ID_VENDOR_ID=0bda
E: ID_MODEL=Multi-Card
E: ID_MODEL_ENC=Multi-Card\x20\x20\x20\x20\x20\x20
E: ID_MODEL_ID=0158
E: ID_TYPE=disk
E: ID_INSTANCE=0:0
E: ID_BUS=usb
E: ID_USB_INTERFACES=:080650:

> Please also ask the original reporter for output of dmidecode and
> 'tree /sys'.

Done, will report back when data arrives. 

Thanks!
Comment 3 Martin Pitt 2009-10-06 09:31:55 UTC
> E: ID_USB_INTERFACES=:080650:

So class 08 is just "mass storage". According to http://www.usb.org/developers/devclass_docs/usb_msc_overview_1.2.pdf, subclass 06 is "SCSI transparent command set" (very helpful :-( ) and protocol 50 is "Bulk-only transport". So nothing specific for card readers here, I'm afraid. My USB stick is exactly the same.
Comment 4 David Zeuthen (not reading bugmail) 2009-10-06 11:39:48 UTC
(In reply to comment #2)
> (In reply to comment #1)
> 
> > DriveEject() is sufficient on iPod and Kindle to make the "unsafe to remove
> > blabla" warning go away.
> 
> Right, that's why I wondered why drive detaching was necessary. Just as a way
> to "clean up" and also reduce power consumption, or whether some devices really
> need it. (Just in case we need to disable can_detach right before a release,
> when time gets tight :) ).

Yeah. Btw, It turns out Windows actually powers down the port for this thing - I asked Alan Stern for a similar mechanism in Linux. See

http://thread.gmane.org/gmane.linux.usb.general/22706

> 
> > Maybe we can look at the usb interface classes to avoid ejecting USB connected
> > card readers? I also think the DMI data tells you whether a particular usb port
> > has an external receptable - I remember Kay talking about that (adding to Cc). 
> 
> That would be nice. 

I wonder how it works on Windows - e.g. is the SD Reader included in the "Safely Remove Hardware" list? And if so, does it require a reboot if you safely remove the device? 

If not, is this because of a .inf file (moral equivalent to udev rules - at least insofar that these files are used for quirks too) or do they parse the DMI data?

> > Please also ask the original reporter for output of dmidecode and
> > 'tree /sys'.
> 
> Done, will report back when data arrives.

Cool. We might end up needing to blacklist devices if the DMI data isn't helpful. I'd really like us to avoid that... 

Alternatively we can stop doing DriveDetach() automatically when the user selects Eject from the file manager menu (or presses the eject icon in the file manager siderbar).. and instead always offer a "Safely Remove Hardware" menu item. That would be a step back user-experience-wise though...
Comment 5 David Zeuthen (not reading bugmail) 2009-10-06 14:20:54 UTC
Richard, I was told you have hardware like this - any chance you can attach the output of dmidecode? And if you have Windows, any chance you can try out the thing I mentioned in comment 4? Thanks.
Comment 6 Martin Pitt 2009-10-07 00:22:37 UTC
Created attachment 30134 [details]
dmidecode output

Output of dmidecode. I'm not a guru with reading those, but it does not seem very useful to me. :-( There is no reference to "SD" or "card" or "storage", and just one USB related record which doesn't even say it's internal (well, the thing does have external USBs as well):

Handle 0x0008, DMI type 8, 9 bytes
Port Connector Information
        Internal Reference Designator: USB
        Internal Connector Type: None
        External Reference Designator: Not Specified
        External Connector Type: Access Bus (USB)
        Port Type: USB
Comment 7 Martin Pitt 2009-10-07 00:31:04 UTC
http://launchpadlibrarian.net/33181405/sys.txt.gz has an ls -lR /sys, http://launchpadlibrarian.net/33181491/dk-disks.txt is the devkit-disks --dump output.

The dk-d dump looks pretty much identical to an USB stick (they even both falsely claim "rotational media: 1" :-) ) except for the product name. So if nothing else helps, we could match on product == "*Card" to fix it for this particular model, but this does not seem to be very robust to me.

I'll ask the original reporter about trying Windows.
Comment 8 Richard Hughes 2009-10-07 02:27:53 UTC
(In reply to comment #5)
> Richard, I was told you have hardware like this - any chance you can attach the
> output of dmidecode? And if you have Windows, any chance you can try out the
> thing I mentioned in comment 4? Thanks.

I don't have Windows on it, sorry. I've got a copy of Windows XP home I could install on it if you wish, but it would mean nuking my current F11 install. I can do this if you really need the data.
Comment 9 David Zeuthen (not reading bugmail) 2009-10-08 05:32:28 UTC
(In reply to comment #8)
> (In reply to comment #5)
> > Richard, I was told you have hardware like this - any chance you can attach the
> > output of dmidecode? And if you have Windows, any chance you can try out the
> > thing I mentioned in comment 4? Thanks.
> 
> I don't have Windows on it, sorry. I've got a copy of Windows XP home I could
> install on it if you wish, but it would mean nuking my current F11 install. I
> can do this if you really need the data.

That's OK, no need to do this - I'm pretty sure they just use an .inf file. Thanks anyway.

Comment 10 David Zeuthen (not reading bugmail) 2009-10-08 05:37:46 UTC
(In reply to comment #6)
> Created an attachment (id=30134) [details]
> dmidecode output
> 
> Output of dmidecode. I'm not a guru with reading those, but it does not seem
> very useful to me. :-( There is no reference to "SD" or "card" or "storage",
> and just one USB related record which doesn't even say it's internal (well, the
> thing does have external USBs as well):
> 
> Handle 0x0008, DMI type 8, 9 bytes
> Port Connector Information
>         Internal Reference Designator: USB
>         Internal Connector Type: None
>         External Reference Designator: Not Specified
>         External Connector Type: Access Bus (USB)
>         Port Type: USB

Just for the record, see [0] from one of my boxes at home. 

Of course, I have no idea how to map USB0, USB1 to actual USB host controllers or even ports on the same host controller. It might be possible to get the kernel USB drivers to parse/interpret DMI data and then export some attributes like whether the actual USB port is external or internal.

But that's going to be a lot of work. And it might be imprecise because vendors don't properly populate DMI. And there's no guarantee the USB kernel folks are interested in this.

[0] :

Handle 0x0010, DMI type 8, 9 bytes
Port Connector Information
        Internal Reference Designator: USB0
        Internal Connector Type: None
        External Reference Designator: USB0
        External Connector Type: Access Bus (USB)
        Port Type: USB

Handle 0x0011, DMI type 8, 9 bytes
Port Connector Information
        Internal Reference Designator: USB1
        Internal Connector Type: None
        External Reference Designator: USB1
        External Connector Type: Access Bus (USB)
        Port Type: USB

Handle 0x0012, DMI type 8, 9 bytes
Port Connector Information
        Internal Reference Designator: USB2
        Internal Connector Type: None
        External Reference Designator: USB2
        External Connector Type: Access Bus (USB)
        Port Type: USB

Handle 0x0013, DMI type 8, 9 bytes
Port Connector Information
        Internal Reference Designator: USB3
        Internal Connector Type: None
        External Reference Designator: USB3
        External Connector Type: Access Bus (USB)
        Port Type: USB

[...]

Handle 0x0030, DMI type 8, 9 bytes
Port Connector Information
        Internal Reference Designator: JUSR2 - USB4/5
        Internal Connector Type: Other
        External Reference Designator: Not Specified
        External Connector Type: None
        Port Type: USB

Handle 0x0031, DMI type 8, 9 bytes
Port Connector Information
        Internal Reference Designator: JUSR3 - USB6/7
        Internal Connector Type: Other
        External Reference Designator: Not Specified
        External Connector Type: None
        Port Type: USB

Handle 0x0032, DMI type 8, 9 bytes
Port Connector Information
        Internal Reference Designator: USB8 - USB8
        Internal Connector Type: Access Bus (USB)
        External Reference Designator: Not Specified
        External Connector Type: None
        Port Type: USB

Handle 0x0033, DMI type 8, 9 bytes
Port Connector Information
        Internal Reference Designator: USB9 - USB9
        Internal Connector Type: Access Bus (USB)
        External Reference Designator: Not Specified
        External Connector Type: None
        Port Type: USB
Comment 11 David Zeuthen (not reading bugmail) 2009-10-08 05:42:15 UTC
So, based on all this I think the only viable approach is to

1. Add a note to the docs DriveDetach() saying that the drive, while
   detachable, may be located inside the box. E.g. basically recommend
   that policy agents only call DriveDetach() when the user specifically
   asked to "Safely Remove Hardware"

2. Fix up GVfs/Nautilus so we never call DriveDetach() when the user presses
   the "Eject" button - e.g. only when the user chooses the "Safely Remove
   Hardware", we call DriveDetach().

3. In the future we could add a DriveIsOnExternalPort boolean property that
   is set to TRUE if, and only if, we are 100% the port is external.

Thoughts?
Comment 12 David Zeuthen (not reading bugmail) 2009-10-08 17:38:03 UTC
Filed the GVfs bug with a patch

https://bugzilla.gnome.org/show_bug.cgi?id=597864

and committed the patch to master and gnome-2-28. It should be in the next GVfs point release for GNOME 2.28. Martin, if you can check the patch works as expect it would be great. Thanks.

I'm keeping this bug open until the docs for DriveDetach() are amended.
Comment 13 Martin Pitt 2009-10-08 23:34:37 UTC
> Martin, if you can check the patch works as expect it would be great.

It does seem to do the right thing here, thanks! I'll also ask the original reporters to triple-check.
Comment 14 David Zeuthen (not reading bugmail) 2009-10-09 07:20:45 UTC
Fixing up Summary for posterity
Comment 15 David Zeuthen (not reading bugmail) 2009-10-09 07:21:39 UTC
Added a note to the docs for DriveDetach():

http://cgit.freedesktop.org/DeviceKit/DeviceKit-disks/commit/?id=f72bc9e163df37984899d277525da579a0f4b197
Comment 16 David Zeuthen (not reading bugmail) 2009-10-28 21:30:42 UTC
Hey,

I was just looking through the USB2 spec and stumbled upon section 11.23.2.1 - the DeviceRemovable descriptor. Seems to work for, at least, my IBM UltraNav keyboard [0] - see [1] for lsusb snippets.

Can anyone with access to a Dell Mini (Richard?) please attach 'lsusb -vvv' and 'lsusb -t' output? Many thanks!

If it checks out for the Dell Mini, we should be able to figure out whether a given USB device really is removable or not (could be incorporated as a patch to udev's usb_id).

     David

[0] : http://www.geek.com/articles/chips/review-lenovo-thinkpad-ultranav-keyboard-20090325/

[1] :
lsusb -t:
[...]
    |-Dev#   3 Vendor 0x04b3 Product 0x3016
    | |-Dev#   5 Vendor 0x04b3 Product 0x3018
    | `-Dev#   6 Vendor 0x06cb Product 0x0009
[...]

lsusb -vvv:

[...]
Hub Descriptor:
  bLength               9
  bDescriptorType      41
  nNbrPorts             4
  wHubCharacteristic 0x0009
    Per-port power switching
    Per-port overcurrent protection
  bPwrOn2PwrGood       50 * 2 milli seconds
  bHubContrCurrent    100 milli Ampere
  DeviceRemovable    0x18
  PortPwrCtrlMask    0xff
 Hub Port Status:
   Port 1: 0000.0100 power
   Port 2: 0000.0100 power
   Port 3: 0000.0303 lowspeed power enable connect
   Port 4: 0000.0303 lowspeed power enable connect
[...]
Comment 17 Martin Pitt 2009-10-29 00:22:16 UTC
Oh, nice! I asked the original reporters for their data, too.
Comment 18 Richard Hughes 2009-10-29 02:39:50 UTC
Created attachment 30793 [details]
lsusb -t

(In reply to comment #16)
> Can anyone with access to a Dell Mini (Richard?) please attach 'lsusb -vvv' and
> 'lsusb -t' output? Many thanks!
Comment 19 Richard Hughes 2009-10-29 02:40:18 UTC
Created attachment 30794 [details]
sudo lsusb -vvv
Comment 20 Martin Pitt 2009-10-29 04:31:51 UTC
http://launchpadlibrarian.net/34592147/usb.txt is the output from Dell mini 10v. Unfortunately all 5 "Hub Descriptor"s have "DeviceRemovable 0x00"..
Comment 21 David Zeuthen (not reading bugmail) 2009-10-29 08:30:33 UTC
Hey, thanks for the output. Now that we know there is a mechanism for conveying this data, I'm tempted to

 1. Make udev's usb_id set a ID_USB_HOTPLUGGABLE property by analyzing
    the hub descriptors. Then take this property into account when
    calculating the CanDetach property.

 2. Fix/quirk broken hardware like the Dell Mini with simple udev rules
    that can override ID_USB_HOTPLUGGABLE.

    Also notify Dell (and others) of this problem so it won't happen
    in the future.