| Message ID | 20211202171748.3035874-1-john@metanate.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | usb: dwc2: gadget: initialize max_speed from params | expand |
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 <john@metanate.com> > --- > 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 <john@metanate.com> > > --- > > 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 <john@metanate.com> Acked-by: Minas Harutyunyan <Minas.Harutyunyan@synopsys.com> >>> --- >>> 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 >
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;
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 <john@metanate.com> --- drivers/usb/dwc2/gadget.c | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-)