On Mon, 9 Jan 2012, Sebastian Andrzej Siewior wrote:
> According to
> |git grep sg_tablesize drivers/usb/
> |drivers/usb/gadget/dummy_hcd.c: hcd->self.sg_tablesize = ~0;
> |drivers/usb/host/ehci-hcd.c: hcd->self.sg_tablesize = ~0;
> |drivers/usb/host/whci/hcd.c: usb_hcd->self.sg_tablesize = 2048; /* somewhat arbitrary */
> |drivers/usb/host/xhci.c: hcd->self.sg_tablesize = TRBS_PER_SEGMENT - 2;
>
> Minus drivers/usb/image/ and drivers/usb/storage/
> uhci is not here so usb_sg_init() does the wrong thing here. You want me
> send a patch initializes sg_tablesize for uhci?
I must have left it out when I added SG support to uhci-hcd. Yes,
please submit a patch to fix this.
> >You would also have to allocate a bunch of URBs using GFP_ATOMIC, which
> >is not desirable.
>
> I see. So this:
>
> From dd8a38268880a7141b5b0cc2b6c4168b96a3024a Mon Sep 17 00:00:00 2001
> From: Sebastian Andrzej Siewior <bigeasy@xxxxxxxxxxxxx>
> Date: Mon, 9 Jan 2012 18:33:04 +0100
> Subject: [PATCH] usb/uas: only bind if the hcd supports SG
>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@xxxxxxxxxxxxx>
> ---
> drivers/usb/storage/uas.c | 23 ++++++++++++++++++++---
> 1 files changed, 20 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
> index 9d40b98..d118e05 100644
> --- a/drivers/usb/storage/uas.c
> +++ b/drivers/usb/storage/uas.c
> @@ -13,6 +13,7 @@
> #include <linux/types.h>
> #include <linux/module.h>
> #include <linux/usb.h>
> +#include <linux/usb/hcd.h>
> #include <linux/usb/storage.h>
>
> #include <scsi/scsi.h>
> @@ -626,22 +627,38 @@ static int uas_is_interface(struct usb_host_interface *intf)
> intf->desc.bInterfaceProtocol == USB_PR_UAS);
> }
>
> +static int uas_isnt_supported(struct usb_device *udev)
> +{
> + struct usb_hcd *hcd = bus_to_hcd(udev->bus);
> +
> + pr_err("USB controller %s does not support sg. Can't use UAS.\n",
> + hcd->driver->description);
Use dev_warn, not pr_err.
Taking Sarah's remark into consideration, you could include a message
telling the user to plug the device into a different controller.
Also, you should spell out "scatter-gather"; normal people won't know
what "sg" means. :-)
(They probably won't know what "UAS" means either, but since that's the
driver's name they ought to be able to figure it out.)
> + return -EINVAL;
This should be -ENODEV. That way probing will continue, giving
usb-storage a chance to bind.
> +}
> +
> static int uas_switch_interface(struct usb_device *udev,
> struct usb_interface *intf)
> {
> int i;
> + int sg_supported = udev->bus->sg_tablesize != 0;
>
> - if (uas_is_interface(intf->cur_altsetting))
> - return 0;
> + if (uas_is_interface(intf->cur_altsetting)) {
> + if (sg_supported)
> + return 0;
> + return uas_isnt_supported(udev);
> + }
>
> for (i = 0; i < intf->num_altsetting; i++) {
> struct usb_host_interface *alt = &intf->altsetting[i];
> if (alt == intf->cur_altsetting)
> continue;
> - if (uas_is_interface(alt))
> + if (uas_is_interface(alt)) {
> + if (!sg_supported)
> + return uas_isnt_supported(udev);
> return usb_set_interface(udev,
> alt->desc.bInterfaceNumber,
> alt->desc.bAlternateSetting);
> + }
> }
>
> return -ENODEV;
The logic is a little convoluted. Here's what I'd do:
See if there's a UAS altsetting; if not return -ENODEV.
See if SG is supported; if not print the warning and
return -ENODEV.
If the UAS interface isn't the current one, do
usb_set_interface.
> or making a non-blocking version of usb_sg_wait()? Since it we should
> have BOT and UAS we could fallback to BOT, print a message and implement
> sg support if one complains.
For the time being we should be able to get along without a new version
of usb_sg_wait.
Alan Stern
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
