[PATCH v1] usbip: Add USB_SPEED_SUPER_PLUS as valid arg

[PATCH v1] usbip: Add USB_SPEED_SUPER_PLUS as valid arg

[Łukasz Bartosik]

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)
 		vdev = &vhci->vhci_hcd_ss->vdev[rhport];
 	else
 		vdev = &vhci->vhci_hcd_hs->vdev[rhport];
-- 
2.45.2.803.g4e1b14247a-goog

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

[Greg Kroah-Hartman]

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?

thanks,

greg k-h

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

[Łukasz Bartosik]

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.

Thanks,
Lukasz

> thanks,
>
> greg k-h

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

[Shuah Khan]

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 change is incomplete and will break things.

thanks,
-- Shuah

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

[Łukasz Bartosik]

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
2) vudc_transfer.c:34: case USB_SPEED_SUPER - this seems to be
unrelated to this patch

Thanks,
Lukasz

> This change is incomplete and will break things.
>
> thanks,
> -- Shuah
>

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

[Shuah Khan]

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