[PATCH 1/2] usb: gadget: r8a66597: Fix two possible sleep-in-atomic-context bugs in init_controller()

[PATCH 1/2] usb: gadget: r8a66597: Fix two possible sleep-in-atomic-context bugs in init_controller()

[Jia-Ju Bai]

The driver may sleep with holding a spinlock.
The function call paths (from bottom to top) in Linux-4.16.7 are:

[FUNC] msleep
drivers/usb/gadget/udc/r8a66597-udc.c, 839: 
		msleep in init_controller
drivers/usb/gadget/udc/r8a66597-udc.c, 96: 
		init_controller in r8a66597_usb_disconnect
drivers/usb/gadget/udc/r8a66597-udc.c, 93: 
		spin_lock in r8a66597_usb_disconnect

[FUNC] msleep
drivers/usb/gadget/udc/r8a66597-udc.c, 835: 
		msleep in init_controller
drivers/usb/gadget/udc/r8a66597-udc.c, 96: 
		init_controller in r8a66597_usb_disconnect
drivers/usb/gadget/udc/r8a66597-udc.c, 93: 
		spin_lock in r8a66597_usb_disconnect

To fix these bugs, msleep() is replaced with mdelay().

This bug is found by my static analysis tool (DSAC-2) and checked by
my code review.

Signed-off-by: Jia-Ju Bai <baijiaju1990@gmail.com>
---
 drivers/usb/gadget/udc/r8a66597-udc.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/gadget/udc/r8a66597-udc.c b/drivers/usb/gadget/udc/r8a66597-udc.c
index a3ecce62662b..24ee9867964b 100644
--- a/drivers/usb/gadget/udc/r8a66597-udc.c
+++ b/drivers/usb/gadget/udc/r8a66597-udc.c
@@ -832,11 +832,11 @@ static void init_controller(struct r8a66597 *r8a66597)
 
 		r8a66597_bset(r8a66597, XCKE, SYSCFG0);
 
-		msleep(3);
+		mdelay(3);
 
 		r8a66597_bset(r8a66597, PLLC, SYSCFG0);
 
-		msleep(1);
+		mdelay(1);
 
 		r8a66597_bset(r8a66597, SCKE, SYSCFG0);
 
-- 
2.17.0

Re: [PATCH 1/2] usb: gadget: r8a66597: Fix two possible sleep-in-atomic-context bugs in init_controller()

[Robert Jarzmik]

Jia-Ju Bai <baijiaju1990@gmail.com> writes:

> The driver may sleep with holding a spinlock.
> The function call paths (from bottom to top) in Linux-4.16.7 are:
>
> [FUNC] msleep
> drivers/usb/gadget/udc/r8a66597-udc.c, 839: 
> 		msleep in init_controller
> drivers/usb/gadget/udc/r8a66597-udc.c, 96: 
> 		init_controller in r8a66597_usb_disconnect
> drivers/usb/gadget/udc/r8a66597-udc.c, 93: 
> 		spin_lock in r8a66597_usb_disconnect

That should not happen...

If think the issue you have is that your usb_connect() and usb_disconnect() are
called from interrupt context. I think the proper fix, as what is done in most
udc phys, is to schedule a workqueue, see drivers/usb/phy/phy-gpio-vbus-usb.c,
gpio_vbus_data.vbus.

Cheers.

--
Robert

Re: [PATCH 1/2] usb: gadget: r8a66597: Fix two possible sleep-in-atomic-context bugs in init_controller()

[Felipe Balbi]

Hi,

Robert Jarzmik <robert.jarzmik@free.fr> writes:

> Jia-Ju Bai <baijiaju1990@gmail.com> writes:
>
>> The driver may sleep with holding a spinlock.
>> The function call paths (from bottom to top) in Linux-4.16.7 are:
>>
>> [FUNC] msleep
>> drivers/usb/gadget/udc/r8a66597-udc.c, 839: 
>> 		msleep in init_controller
>> drivers/usb/gadget/udc/r8a66597-udc.c, 96: 
>> 		init_controller in r8a66597_usb_disconnect
>> drivers/usb/gadget/udc/r8a66597-udc.c, 93: 
>> 		spin_lock in r8a66597_usb_disconnect
>
> That should not happen...
>
> If think the issue you have is that your usb_connect() and usb_disconnect() are
> called from interrupt context. I think the proper fix, as what is done in most
> udc phys, is to schedule a workqueue, see drivers/usb/phy/phy-gpio-vbus-usb.c,
> gpio_vbus_data.vbus.

argh, no. No workqueues needed here. Sorry

-- 
balbi

Re: [PATCH 1/2] usb: gadget: r8a66597: Fix two possible sleep-in-atomic-context bugs in init_controller()

[Robert Jarzmik]

Felipe Balbi <balbi@kernel.org> writes:

> Hi,
>
> Robert Jarzmik <robert.jarzmik@free.fr> writes:
>
>> Jia-Ju Bai <baijiaju1990@gmail.com> writes:
>>
>>> The driver may sleep with holding a spinlock.
>>> The function call paths (from bottom to top) in Linux-4.16.7 are:
>>>
>>> [FUNC] msleep
>>> drivers/usb/gadget/udc/r8a66597-udc.c, 839: 
>>> 		msleep in init_controller
>>> drivers/usb/gadget/udc/r8a66597-udc.c, 96: 
>>> 		init_controller in r8a66597_usb_disconnect
>>> drivers/usb/gadget/udc/r8a66597-udc.c, 93: 
>>> 		spin_lock in r8a66597_usb_disconnect
>>
>> That should not happen...
>>
>> If think the issue you have is that your usb_connect() and usb_disconnect() are
>> called from interrupt context. I think the proper fix, as what is done in most
>> udc phys, is to schedule a workqueue, see drivers/usb/phy/phy-gpio-vbus-usb.c,
>> gpio_vbus_data.vbus.
>
> argh, no. No workqueues needed here. Sorry
Technically why ?

And as bonus question, why is it better to have mdelay() calls in the driver ?

Cheers.

-- 
Robert

Re: [PATCH 1/2] usb: gadget: r8a66597: Fix two possible sleep-in-atomic-context bugs in init_controller()

[Felipe Balbi]

Hi,

Robert Jarzmik <robert.jarzmik@free.fr> writes:
>>>> The driver may sleep with holding a spinlock.
>>>> The function call paths (from bottom to top) in Linux-4.16.7 are:
>>>>
>>>> [FUNC] msleep
>>>> drivers/usb/gadget/udc/r8a66597-udc.c, 839: 
>>>> 		msleep in init_controller
>>>> drivers/usb/gadget/udc/r8a66597-udc.c, 96: 
>>>> 		init_controller in r8a66597_usb_disconnect
>>>> drivers/usb/gadget/udc/r8a66597-udc.c, 93: 
>>>> 		spin_lock in r8a66597_usb_disconnect
>>>
>>> That should not happen...
>>>
>>> If think the issue you have is that your usb_connect() and usb_disconnect() are
>>> called from interrupt context. I think the proper fix, as what is done in most
>>> udc phys, is to schedule a workqueue, see drivers/usb/phy/phy-gpio-vbus-usb.c,
>>> gpio_vbus_data.vbus.
>>
>> argh, no. No workqueues needed here. Sorry
> Technically why ?

well, strictly technically there's nothing wrong. But it opens a can of
worms. We've seen time and time again drivers growing into
unmaintainable mess because of workqueues being fired in several places.
>
> And as bonus question, why is it better to have mdelay() calls in the driver ?

As a bugfix, it's the smallest fix possible, right? Ideally, we wouldn't
need either of them. Perhaps there's a bit which can be polled instead?

Looking at the code again, it looks like that's messing with
controller's clock and PLL; seems like it should've been done with CCF
anyway.

-- 
balbi

Re: [PATCH 1/2] usb: gadget: r8a66597: Fix two possible sleep-in-atomic-context bugs in init_controller()

[Robert Jarzmik]

Felipe Balbi <balbi@kernel.org> writes:

> Hi,
>
> Robert Jarzmik <robert.jarzmik@free.fr> writes:
>>>>> The driver may sleep with holding a spinlock.
>>>>> The function call paths (from bottom to top) in Linux-4.16.7 are:
>>>>>
>>>>> [FUNC] msleep
>>>>> drivers/usb/gadget/udc/r8a66597-udc.c, 839: 
>>>>> 		msleep in init_controller
>>>>> drivers/usb/gadget/udc/r8a66597-udc.c, 96: 
>>>>> 		init_controller in r8a66597_usb_disconnect
>>>>> drivers/usb/gadget/udc/r8a66597-udc.c, 93: 
>>>>> 		spin_lock in r8a66597_usb_disconnect
>>>>
>>>> That should not happen...
>>>>
>>>> If think the issue you have is that your usb_connect() and usb_disconnect() are
>>>> called from interrupt context. I think the proper fix, as what is done in most
>>>> udc phys, is to schedule a workqueue, see drivers/usb/phy/phy-gpio-vbus-usb.c,
>>>> gpio_vbus_data.vbus.
>>>
>>> argh, no. No workqueues needed here. Sorry
>> Technically why ?
>
> well, strictly technically there's nothing wrong. But it opens a can of
> worms. We've seen time and time again drivers growing into
> unmaintainable mess because of workqueues being fired in several places.
I see.

>>
>> And as bonus question, why is it better to have mdelay() calls in the driver ?
>
> As a bugfix, it's the smallest fix possible, right? Ideally, we wouldn't
> need either of them. Perhaps there's a bit which can be polled instead?
Ideally yes. Do you remember if a "threaded interrupt" might use msleep() ? I
seem to remember that they can, so won't that be another alternative ?

Cheers.

-- 
Robert

Re: [PATCH 1/2] usb: gadget: r8a66597: Fix two possible sleep-in-atomic-context bugs in init_controller()

[Felipe Balbi]

Hi,

Robert Jarzmik <robert.jarzmik@free.fr> writes:
>>> And as bonus question, why is it better to have mdelay() calls in the driver ?
>>
>> As a bugfix, it's the smallest fix possible, right? Ideally, we wouldn't
>> need either of them. Perhaps there's a bit which can be polled instead?
> Ideally yes. Do you remember if a "threaded interrupt" might use msleep() ? I
> seem to remember that they can, so won't that be another alternative ?

yeah, unless, of course, you have a spinlock held. ;-)

-- 
balbi

Re: [PATCH 1/2] usb: gadget: r8a66597: Fix two possible sleep-in-atomic-context bugs in init_controller()

[Robert Jarzmik]

Felipe Balbi <balbi@kernel.org> writes:

> Hi,
>
> Robert Jarzmik <robert.jarzmik@free.fr> writes:
>>>> And as bonus question, why is it better to have mdelay() calls in the driver ?
>>>
>>> As a bugfix, it's the smallest fix possible, right? Ideally, we wouldn't
>>> need either of them. Perhaps there's a bit which can be polled instead?
>> Ideally yes. Do you remember if a "threaded interrupt" might use msleep() ? I
>> seem to remember that they can, so won't that be another alternative ?
>
> yeah, unless, of course, you have a spinlock held. ;-)
Ah yes, unless that :)

I would have proposed to call the disconnect out of the spinlock path, but
looking at the r8a66592_usb_disconnect(), with its spinlock flip-flop, I loose
heart ...

And even if I still think no mdelay() should be used, because of the kernel
stall (and global uniprocessor stall), I won't argue anymore. After all, if you
let in the mdelay(), perhaps the maintainers will agree to review their
architecture and drop the locks or sleeps in interrupt context in a follow-up
patch, who knows ...

Cheers.

-- 
Robert