Re: [PATCH v1] usbip: Add USB_SPEED_SUPER_PLUS as valid arg

[Shuah Khan <skhan@linuxfoundation.org>]

On 7/10/24 04:50, Łukasz Bartosik wrote:
> On Tue, Jul 9, 2024 at 9:27 PM Shuah Khan <skhan@linuxfoundation.org> wrote:
>>
>> On 7/5/24 04:43, Łukasz Bartosik wrote:
>>> On Fri, Jul 5, 2024 at 11:40 AM Greg Kroah-Hartman
>>> <gregkh@linuxfoundation.org> wrote:
>>>>
>>>> On Fri, Jul 05, 2024 at 09:19:02AM +0000, Łukasz Bartosik wrote:
>>>>> Add USB_SPEED_SUPER_PLUS as valid argument to allow
>>>>> to attach USB SuperSpeed+ devices.
>>>>>
>>>>> Signed-off-by: Łukasz Bartosik <ukaszb@chromium.org>
>>>>> ---
>>>>>    drivers/usb/usbip/vhci_sysfs.c | 3 ++-
>>>>>    1 file changed, 2 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/usb/usbip/vhci_sysfs.c b/drivers/usb/usbip/vhci_sysfs.c
>>>>> index e2847cd3e6e3..d5865460e82d 100644
>>>>> --- a/drivers/usb/usbip/vhci_sysfs.c
>>>>> +++ b/drivers/usb/usbip/vhci_sysfs.c
>>>>> @@ -283,6 +283,7 @@ static int valid_args(__u32 *pdev_nr, __u32 *rhport,
>>>>>         case USB_SPEED_HIGH:
>>>>>         case USB_SPEED_WIRELESS:
>>>>>         case USB_SPEED_SUPER:
>>>>> +     case USB_SPEED_SUPER_PLUS:
>>>>>                 break;
>>>>>         default:
>>>>>                 pr_err("Failed attach request for unsupported USB speed: %s\n",
>>>>> @@ -349,7 +350,7 @@ static ssize_t attach_store(struct device *dev, struct device_attribute *attr,
>>>>>         vhci_hcd = hcd_to_vhci_hcd(hcd);
>>>>>         vhci = vhci_hcd->vhci;
>>>>>
>>>>> -     if (speed == USB_SPEED_SUPER)
>>>>> +     if (speed >= USB_SPEED_SUPER)
>>>>
>>>> It's an enum, are you sure this will work?
>>>>
>>>
>>> Gcc (gcc (Debian 13.2.0-13) 13.2.0) which I used to compile the patch
>>> does not complain about this change at all:
>>>     make
>>>     ...
>>>     CC [M]  drivers/usb/usbip/vhci_sysfs.o
>>>     LD [M]  drivers/usb/usbip/vhci-hcd.o
>>>
>>>
>>>
>>> Without the patch I was getting the following error when trying to
>>> attach a device:
>>> vhci_hcd: Failed attach request for unsupported USB speed: super-speed-plus
>>>
>>> With the patch USB SS+ device attaches successfully:
>>> [248223.654445] vhci_hcd vhci_hcd.0: pdev(0) rhport(0) sockfd(3)
>>> [248223.660701] vhci_hcd vhci_hcd.0: devid(65538) speed(6)
>>> speed_str(super-speed-plus)
>>> [248223.668540] vhci_hcd vhci_hcd.0: Device attached
>>> [248223.936363] usb 2-1: SetAddress Request (2) to port 0
>>> [248223.941698] usb 2-1: new SuperSpeed USB device number 2 using vhci_hcd
>>> [248224.138020] usb 2-1: LPM exit latency is zeroed, disabling LPM.
>>> [248224.331984] usb 2-1: New USB device found, idVendor=18d1,
>>> idProduct=0010, bcdDevice= 0.10
>>> [248224.340416] usb 2-1: New USB device strings: Mfr=1, Product=2,
>>> SerialNumber=3
>>> [248224.347805] usb 2-1: Product: Linux USB Debug Target
>>> [248224.352984] usb 2-1: Manufacturer: Linux Foundation
>>> [248224.358162] usb 2-1: SerialNumber: 0001
>>>
>>> I hope this will resolve your doubts.
>>>
>>
>> What about the other places that check for USB_SPEED_SUPER?
>> take a look at attach_store() that checks for USB_SPEED_SUPER
>> and picks the correct vdev.
>>
> 
> This patch already updates attach_store() to select the correct vdev.
> Please see lines:
> -     if (speed == USB_SPEED_SUPER)
> +     if (speed >= USB_SPEED_SUPER)
> 
> Also there are two other places where USB_SPEED_SUPER is used:
> 1) vhci_hcd.c:1158: hcd->self.root_hub->speed = USB_SPEED_SUPER - IMHO
> no need for a change as it is ok to attach USB3.1 device (taking into
> account it is backwards compatible) to USB3.0 HC

Let's update them all.

> 2) vudc_transfer.c:34: case USB_SPEED_SUPER - this seems to be
> unrelated to this patch

This isn't related - however feel free to send the patch if you are
bale to.

thanks,
-- Shuah