| Message ID | 20180620035453.7721-1-baijiaju1990@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
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.
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.
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.
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. ;-)
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.
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);
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(-)