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 <[email protected]>
---
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
Jia-Ju Bai <[email protected]> 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
Felipe Balbi <[email protected]> writes:
> Hi,
>
> Robert Jarzmik <[email protected]> writes:
>
>> Jia-Ju Bai <[email protected]> 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
Hi,
Robert Jarzmik <[email protected]> writes:
> Jia-Ju Bai <[email protected]> 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
Hi,
Robert Jarzmik <[email protected]> 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
Felipe Balbi <[email protected]> writes:
> Hi,
>
> Robert Jarzmik <[email protected]> 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
Hi,
Robert Jarzmik <[email protected]> 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
Felipe Balbi <[email protected]> writes:
> Hi,
>
> Robert Jarzmik <[email protected]> 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