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

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

[Petr Cvek]

Hi,

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
	...
	Preemption disabled at:
	[<bf43be34>] usb_function_activate+0x20/0xa4 [libcomposite]
	CPU: 0 PID: 360 Comm: v4l_id Not tainted 4.10.0-rc5-magician+ #2
	Hardware name: HTC Magician
	[<c000f540>] (unwind_backtrace) from [<c000d3e0>] (show_stack+0x10/0x14)
	[<c000d3e0>] (show_stack) from [<c00363b8>] (__schedule_bug+0x98/0xcc)
	[<c00363b8>] (__schedule_bug) from [<c035d94c>] (__schedule+0x58/0x444)
	[<c035d94c>] (__schedule) from [<c035dde0>] (schedule+0xa8/0xc8)
	[<c035dde0>] (schedule) from [<c0361558>] (schedule_timeout+0x1c8/0x1ec)
	[<c0361558>] (schedule_timeout) from [<c005312c>] (msleep+0x1c/0x20)
	[<c005312c>] (msleep) from [<bf4322e4>] (udc_enable+0x170/0x1d4 [pxa27x_udc])
	[<bf4322e4>] (udc_enable [pxa27x_udc]) from [<bf432560>] (pxa_udc_pullup+0x40/0x68 [pxa27x_udc])
	[<bf432560>] (pxa_udc_pullup [pxa27x_udc]) from [<bf4271c8>] (usb_gadget_connect+0x3c/0x5c [udc_core])
	[<bf4271c8>] (usb_gadget_connect [udc_core]) from [<bf43beac>] (usb_function_activate+0x98/0xa4 [libcomposite])
	[<bf43beac>] (usb_function_activate [libcomposite]) from [<bf44df08>] (uvc_function_connect+0x14/0x38 [usb_f_uvc])
	[<bf44df08>] (uvc_function_connect [usb_f_uvc]) from [<bf44e944>] (uvc_v4l2_open+0x4c/0x60 [usb_f_uvc])
	[<bf44e944>] (uvc_v4l2_open [usb_f_uvc]) from [<bf156420>] (v4l2_open+0x7c/0xc0 [videodev])
	[<bf156420>] (v4l2_open [videodev]) from [<c00b4010>] (chrdev_open+0x198/0x1b0)
	[<c00b4010>] (chrdev_open) from [<c00ad314>] (do_dentry_open.constprop.3+0x1b0/0x2ec)
	[<c00ad314>] (do_dentry_open.constprop.3) from [<c00bdbf0>] (path_openat+0xc10/0xdd4)
	[<c00bdbf0>] (path_openat) from [<c00bdde4>] (do_filp_open+0x30/0x78)
	[<c00bdde4>] (do_filp_open) from [<c00ae340>] (do_sys_open+0xf0/0x1b0)
	[<c00ae340>] (do_sys_open) from [<c000a320>] (ret_fast_syscall+0x0/0x38)

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?
	
Second bug was discovered by Robert Jarzmik during discussion in

	[BUG] usb: gadget: Kernel oops with UVC USB gadget and configfs

A call to usb_put_phy(udc->transceiver) in pxa_udc_remove() can be called with  variable
containing error pointer or NULL. So it should be moved to a previous call like this (modified
original suggestion):

	if (!IS_ERR_OR_NULL(udc->transceiver)) {
		usb_unregister_notifier(udc->transceiver, &pxa27x_udc_phy);
		usb_put_phy(udc->transceiver);
	}

And as we talking about it, is this return correct?

	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)?

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.

best regards,
Petr

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

[Robert Jarzmik]

Petr Cvek <petr.cvek@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

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

[Petr Cvek]

Dne 22.2.2017 v 21:46 Robert Jarzmik napsal(a):
> Petr Cvek <petr.cvek@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.
> 

Well now it is definitely inside spin_lock_irqsave()

	http://lxr.free-electrons.com/source/drivers/usb/gadget/composite.c#L374


>> 	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.
> 

OK so no problem there.

>> 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.

OK mdelay/msleep problem will take longer, so I will send that two patches now.

Petr