[RFC] usb: gadget: scheduling while atomic in pxa27x_udc, IS_ERR_OR_NULL test, duplicated definition

[Robert Jarzmik]

Petr Cvek <petr.cvek at tul.cz> writes:

Hi Petr,

> I found a few problems with the PXA27x UDC.
>
> 	usb_function_activate() in drivers/usb/gadget/composite.c
>
> does spin_lock_irqsave() and then calls 
>
> 	gadget->ops->pullup() in drivers/usb/gadget/udc/core.c
>
> which is set to pxa_udc_pullup(), which should be called not in interrupt
>
> 	/**
> 	 * pxa_udc_pullup - Offer manual D+ pullup control
> 	 * @_gadget: usb gadget using the control
> 	 * @is_active: 0 if disconnect, else connect D+ pullup resistor
> 	 * Context: !in_interrupt()
> 	 *
> 	 * Returns 0 if OK, -EOPNOTSUPP if udc driver doesn't handle D+ pullup
> 	 */
>
> This finally causes fail at
>
> 	udc_enable() in drivers/usb/gadget/udc/pxa27x_udc.c
> 	
> at code
>
> 	/*
> 	 * Caller must be able to sleep in order to cope with startup transients
> 	 */
> 	msleep(100);
>
> with a following error (with CONFIG_DEBUG_PREEMPT on):
>
> 	BUG: scheduling while atomic: v4l_id/360/0x00000002
>
> With the msleep changed to mdelay, the code (specified as !in_interrupt()) seems to work fine
> (after torture reloads). Can the caller (udc core) be changed to be able to
> sleep?

This question is for Felipe. From the several years back when I wrote this code
I think it was granted that the pullup() callback was not in interrupt context,
but Felipe knows better.

> Second bug was discovered by Robert Jarzmik during discussion in
Please go ahead and submit a patch.

> And as we talking about it, is this return correct?
I think so.

> 	if (of_have_populated_dt()) {
> 		udc->transceiver =
> 			devm_usb_get_phy_by_phandle(udc->dev, "phys", 0);
>
> 		if (IS_ERR(udc->transceiver))
> 			return PTR_ERR(udc->transceiver);
> 	} else {
> 		udc->transceiver = usb_get_phy(USB_PHY_TYPE_USB2);
> 	}
>
> One branch returns on error and second one is fine, udc->transceiver then can hold an error,
> but this is fine for the rest of driver (tested). Question is does it have to return from a first
> branch (e.g. my device does not have phy)?
In the devicetree context (first branch), even if you don't have a phy, you'll
instanciate a "nop" phy, ie. "usb-nop-xceiv". From a hardware perspective, you
have a phy, it's just that you don't have to do anything from a software
perspective to activate your phy.

In the platform_data context (second branch), an error is the common path, as
there is no phy in many old platforms, and pxa27x are old ... hence no check of
error.

> And finally it seems definitions from drivers/usb/gadget/udc/pxa27x_udc.c are duplicated:
>
> 	static void udc_enable(struct pxa_udc *udc);
> 	static void udc_disable(struct pxa_udc *udc);
>
> I will send patch series as soon as we agree on solutions.
Excellent.

Cheers.

-- 
Robert