Re: [PATCH V2 3/5] usb: gadget: pxa25x_udc: prepare/unprepare clocks in pxa-ssp

[Robert Jarzmik <robert.jarzmik@free.fr>]

Dmitry Eremin-Solenikov <dbaryshkov@gmail.com> writes:

> Change clk_enable/disable() calls to clk_prepare_enable() and
> clk_disable_unprepare().
>
> Signed-off-by: Dmitry Eremin-Solenikov <dbaryshkov@gmail.com>
> ---
>  drivers/usb/gadget/udc/pxa25x_udc.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/usb/gadget/udc/pxa25x_udc.c b/drivers/usb/gadget/udc/pxa25x_udc.c
> index 42f7eeb..e4964ee 100644
> --- a/drivers/usb/gadget/udc/pxa25x_udc.c
> +++ b/drivers/usb/gadget/udc/pxa25x_udc.c
> @@ -103,8 +103,8 @@ static const char ep0name [] = "ep0";
>  
>  /* IXP doesn't yet support <linux/clk.h> */
>  #define clk_get(dev,name)	NULL
> -#define clk_enable(clk)		do { } while (0)
> -#define clk_disable(clk)	do { } while (0)
> +#define clk_prepare_enable(clk)	do { } while (0)
> +#define clk_disable_unprepare(clk)	do { } while (0)
>  #define clk_put(clk)		do { } while (0)
>  
>  #endif
> @@ -932,7 +932,7 @@ static int pullup(struct pxa25x_udc *udc)
>  		if (!udc->active) {
>  			udc->active = 1;
>  			/* Enable clock for USB device */
> -			clk_enable(udc->clk);
> +			clk_prepare_enable(udc->clk);

Guess what, Russell's remark on i2c and serial made me cross-check.  And there
is a case where this will be called in irq context too ...

See :
-> do_IRQ
  -> lubbock_vbus_irq()
    -> pxa25x_udc_vbus_session()
      -> pullup()
        -> clk_prepare_enable()
          -> CRACK

Note that your patch is not really the faulty one, I think a threaded irq
instead of the "raw" irq should do the trick. And it is granted on UDC api that
vbus function is called in a "sleeping" context (Felipe correct me if I'm
wrong), so a patch to fix this before your current code would be fine.

Cheers.

--
Robert