DWC2 may be paired with a full-speed PHY which is not capable of
high-speed operation. Report this correctly to the gadget core by
setting max_speed from the core parameters.
Prior to commit 5324bad66f09f ("usb: dwc2: gadget: implement
udc_set_speed()") this didn't cause the hardware to be configured
incorrectly, although the speed may have been reported incorrectly. But
after that commit params.speed is updated based on a value passed in by
the gadget core which may set it to a faster speed than is supported by
the hardware. Initialising the max_speed parameter ensures the speed
passed to dwc2_gadget_set_speed() will be one supported by the hardware.
Fixes: 5324bad66f09f ("usb: dwc2: gadget: implement udc_set_speed()")
Signed-off-by: John Keeping <[email protected]>
---
drivers/usb/dwc2/gadget.c | 13 ++++++++++++-
1 file changed, 12 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
index b884a83b26a6e..2bc03f41c70ad 100644
--- a/drivers/usb/dwc2/gadget.c
+++ b/drivers/usb/dwc2/gadget.c
@@ -4974,7 +4974,18 @@ int dwc2_gadget_init(struct dwc2_hsotg *hsotg)
hsotg->params.g_np_tx_fifo_size);
dev_dbg(dev, "RXFIFO size: %d\n", hsotg->params.g_rx_fifo_size);
- hsotg->gadget.max_speed = USB_SPEED_HIGH;
+ switch (hsotg->params.speed) {
+ case DWC2_SPEED_PARAM_LOW:
+ hsotg->gadget.max_speed = USB_SPEED_LOW;
+ break;
+ case DWC2_SPEED_PARAM_FULL:
+ hsotg->gadget.max_speed = USB_SPEED_FULL;
+ break;
+ default:
+ hsotg->gadget.max_speed = USB_SPEED_HIGH;
+ break;
+ }
+
hsotg->gadget.ops = &dwc2_hsotg_gadget_ops;
hsotg->gadget.name = dev_name(dev);
hsotg->gadget.otg_caps = &hsotg->params.otg_caps;
--
2.34.1
Hi John,
On 12/2/2021 9:17 PM, John Keeping wrote:
> DWC2 may be paired with a full-speed PHY which is not capable of
> high-speed operation. Report this correctly to the gadget core by
> setting max_speed from the core parameters.
>
> Prior to commit 5324bad66f09f ("usb: dwc2: gadget: implement
> udc_set_speed()") this didn't cause the hardware to be configured
> incorrectly, although the speed may have been reported incorrectly. But
> after that commit params.speed is updated based on a value passed in by
> the gadget core which may set it to a faster speed than is supported by
> the hardware. Initialising the max_speed parameter ensures the speed
> passed to dwc2_gadget_set_speed() will be one supported by the hardware.
>
> Fixes: 5324bad66f09f ("usb: dwc2: gadget: implement udc_set_speed()")
> Signed-off-by: John Keeping <[email protected]>
> ---
> drivers/usb/dwc2/gadget.c | 13 ++++++++++++-
> 1 file changed, 12 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
> index b884a83b26a6e..2bc03f41c70ad 100644
> --- a/drivers/usb/dwc2/gadget.c
> +++ b/drivers/usb/dwc2/gadget.c
> @@ -4974,7 +4974,18 @@ int dwc2_gadget_init(struct dwc2_hsotg *hsotg)
> hsotg->params.g_np_tx_fifo_size);
> dev_dbg(dev, "RXFIFO size: %d\n", hsotg->params.g_rx_fifo_size);
>
> - hsotg->gadget.max_speed = USB_SPEED_HIGH;
> + switch (hsotg->params.speed) {
> + case DWC2_SPEED_PARAM_LOW:
> + hsotg->gadget.max_speed = USB_SPEED_LOW;
> + break;
> + case DWC2_SPEED_PARAM_FULL:
> + hsotg->gadget.max_speed = USB_SPEED_FULL;
> + break;
> + default:
> + hsotg->gadget.max_speed = USB_SPEED_HIGH;
> + break;
> + }
> +
> hsotg->gadget.ops = &dwc2_hsotg_gadget_ops;
> hsotg->gadget.name = dev_name(dev);
> hsotg->gadget.otg_caps = &hsotg->params.otg_caps;
>
Just setting gadget's max_speed doesn't resolve described issue. After
setting gadget's max_speed in your patch, it doesn't used in driver at
all. Per me, you need also fix dwc2_gadget_set_speed() function by
checking requested speed with max_speed: if requested speed higher than
max_speed then set speed to max_speed.
Thanks,
Minas
Hi Minas,
On Fri, Dec 03, 2021 at 05:49:36AM +0000, Minas Harutyunyan wrote:
> On 12/2/2021 9:17 PM, John Keeping wrote:
> > DWC2 may be paired with a full-speed PHY which is not capable of
> > high-speed operation. Report this correctly to the gadget core by
> > setting max_speed from the core parameters.
> >
> > Prior to commit 5324bad66f09f ("usb: dwc2: gadget: implement
> > udc_set_speed()") this didn't cause the hardware to be configured
> > incorrectly, although the speed may have been reported incorrectly. But
> > after that commit params.speed is updated based on a value passed in by
> > the gadget core which may set it to a faster speed than is supported by
> > the hardware. Initialising the max_speed parameter ensures the speed
> > passed to dwc2_gadget_set_speed() will be one supported by the hardware.
> >
> > Fixes: 5324bad66f09f ("usb: dwc2: gadget: implement udc_set_speed()")
> > Signed-off-by: John Keeping <[email protected]>
> > ---
> > drivers/usb/dwc2/gadget.c | 13 ++++++++++++-
> > 1 file changed, 12 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
> > index b884a83b26a6e..2bc03f41c70ad 100644
> > --- a/drivers/usb/dwc2/gadget.c
> > +++ b/drivers/usb/dwc2/gadget.c
> > @@ -4974,7 +4974,18 @@ int dwc2_gadget_init(struct dwc2_hsotg *hsotg)
> > hsotg->params.g_np_tx_fifo_size);
> > dev_dbg(dev, "RXFIFO size: %d\n", hsotg->params.g_rx_fifo_size);
> >
> > - hsotg->gadget.max_speed = USB_SPEED_HIGH;
> > + switch (hsotg->params.speed) {
> > + case DWC2_SPEED_PARAM_LOW:
> > + hsotg->gadget.max_speed = USB_SPEED_LOW;
> > + break;
> > + case DWC2_SPEED_PARAM_FULL:
> > + hsotg->gadget.max_speed = USB_SPEED_FULL;
> > + break;
> > + default:
> > + hsotg->gadget.max_speed = USB_SPEED_HIGH;
> > + break;
> > + }
> > +
> > hsotg->gadget.ops = &dwc2_hsotg_gadget_ops;
> > hsotg->gadget.name = dev_name(dev);
> > hsotg->gadget.otg_caps = &hsotg->params.otg_caps;
> >
>
> Just setting gadget's max_speed doesn't resolve described issue. After
> setting gadget's max_speed in your patch, it doesn't used in driver at
> all. Per me, you need also fix dwc2_gadget_set_speed() function by
> checking requested speed with max_speed: if requested speed higher than
> max_speed then set speed to max_speed.
Setting the max_speed here restricts what the gadget core will pass in
to the .udc_set_speed hook, see usb_gadget_udc_set_speed():
s = min(speed, udc->gadget->max_speed);
udc->gadget->ops->udc_set_speed(udc->gadget, s);
We can't add a check in dwc2_gadget_set_speed() because the original
capability may have been lost, for example if a high-speed capable
device is configured for full-speed operation then hsotg->params.speed
is set to full-speed and there's nothing that says the device is capable
of high-speed when setting the speed back again.
I believe this patch is sufficient because of the guarantees by the
gadget core that we will never be called with a speed that is faster
than we support.
Regards,
John
On 12/3/2021 3:36 PM, John Keeping wrote:
> Hi Minas,
>
> On Fri, Dec 03, 2021 at 05:49:36AM +0000, Minas Harutyunyan wrote:
>> On 12/2/2021 9:17 PM, John Keeping wrote:
>>> DWC2 may be paired with a full-speed PHY which is not capable of
>>> high-speed operation. Report this correctly to the gadget core by
>>> setting max_speed from the core parameters.
>>>
>>> Prior to commit 5324bad66f09f ("usb: dwc2: gadget: implement
>>> udc_set_speed()") this didn't cause the hardware to be configured
>>> incorrectly, although the speed may have been reported incorrectly. But
>>> after that commit params.speed is updated based on a value passed in by
>>> the gadget core which may set it to a faster speed than is supported by
>>> the hardware. Initialising the max_speed parameter ensures the speed
>>> passed to dwc2_gadget_set_speed() will be one supported by the hardware.
>>>
>>> Fixes: 5324bad66f09f ("usb: dwc2: gadget: implement udc_set_speed()")
>>> Signed-off-by: John Keeping <[email protected]>
Acked-by: Minas Harutyunyan <[email protected]>
>>> ---
>>> drivers/usb/dwc2/gadget.c | 13 ++++++++++++-
>>> 1 file changed, 12 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
>>> index b884a83b26a6e..2bc03f41c70ad 100644
>>> --- a/drivers/usb/dwc2/gadget.c
>>> +++ b/drivers/usb/dwc2/gadget.c
>>> @@ -4974,7 +4974,18 @@ int dwc2_gadget_init(struct dwc2_hsotg *hsotg)
>>> hsotg->params.g_np_tx_fifo_size);
>>> dev_dbg(dev, "RXFIFO size: %d\n", hsotg->params.g_rx_fifo_size);
>>>
>>> - hsotg->gadget.max_speed = USB_SPEED_HIGH;
>>> + switch (hsotg->params.speed) {
>>> + case DWC2_SPEED_PARAM_LOW:
>>> + hsotg->gadget.max_speed = USB_SPEED_LOW;
>>> + break;
>>> + case DWC2_SPEED_PARAM_FULL:
>>> + hsotg->gadget.max_speed = USB_SPEED_FULL;
>>> + break;
>>> + default:
>>> + hsotg->gadget.max_speed = USB_SPEED_HIGH;
>>> + break;
>>> + }
>>> +
>>> hsotg->gadget.ops = &dwc2_hsotg_gadget_ops;
>>> hsotg->gadget.name = dev_name(dev);
>>> hsotg->gadget.otg_caps = &hsotg->params.otg_caps;
>>>
>>
>> Just setting gadget's max_speed doesn't resolve described issue. After
>> setting gadget's max_speed in your patch, it doesn't used in driver at
>> all. Per me, you need also fix dwc2_gadget_set_speed() function by
>> checking requested speed with max_speed: if requested speed higher than
>> max_speed then set speed to max_speed.
>
> Setting the max_speed here restricts what the gadget core will pass in
> to the .udc_set_speed hook, see usb_gadget_udc_set_speed():
>
> s = min(speed, udc->gadget->max_speed);
> udc->gadget->ops->udc_set_speed(udc->gadget, s);
>
> We can't add a check in dwc2_gadget_set_speed() because the original
> capability may have been lost, for example if a high-speed capable
> device is configured for full-speed operation then hsotg->params.speed
> is set to full-speed and there's nothing that says the device is capable
> of high-speed when setting the speed back again.
>
> I believe this patch is sufficient because of the guarantees by the
> gadget core that we will never be called with a speed that is faster
> than we support.
>
>
> Regards,
> John
>