> From: Marek Szyprowski [mailto:m.szyprowski@xxxxxxxxxxx] > Sent: Friday, November 14, 2014 1:19 AM > > On 2014-11-13 21:55, Paul Zimmerman wrote: > >> From: Marek Szyprowski [mailto:m.szyprowski@xxxxxxxxxxx] > >> Sent: Thursday, November 13, 2014 7:18 AM > >> > >> On 2014-10-31 19:46, Paul Zimmerman wrote: > >>>> From: Marek Szyprowski [mailto:m.szyprowski@xxxxxxxxxxx] > >>>> Sent: Friday, October 31, 2014 3:13 AM > >>>> > >>>> This patch adds mutex, which protects initialization and > >>>> deinitialization procedures against suspend/resume methods. > >>>> > >>>> Signed-off-by: Marek Szyprowski <m.szyprowski@xxxxxxxxxxx> > >>>> --- > >>>> drivers/usb/dwc2/core.h | 1 + > >>>> drivers/usb/dwc2/gadget.c | 20 ++++++++++++++++++++ > >>>> 2 files changed, 21 insertions(+) > >>>> > >>>> diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h > >>>> index 9f77b4d1c5ff..58732a9a0019 100644 > >>>> --- a/drivers/usb/dwc2/core.h > >>>> +++ b/drivers/usb/dwc2/core.h > >>>> @@ -187,6 +187,7 @@ struct s3c_hsotg { > >>>> struct s3c_hsotg_plat *plat; > >>>> > >>>> spinlock_t lock; > >>>> + struct mutex init_mutex; > >>>> > >>>> void __iomem *regs; > >>>> int irq; > >>>> diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c > >>>> index d8dda39c9e16..a2e4272a904e 100644 > >>>> --- a/drivers/usb/dwc2/gadget.c > >>>> +++ b/drivers/usb/dwc2/gadget.c > >>>> @@ -21,6 +21,7 @@ > >>>> #include <linux/platform_device.h> > >>>> #include <linux/dma-mapping.h> > >>>> #include <linux/debugfs.h> > >>>> +#include <linux/mutex.h> > >>>> #include <linux/seq_file.h> > >>>> #include <linux/delay.h> > >>>> #include <linux/io.h> > >>>> @@ -2908,6 +2909,7 @@ static int s3c_hsotg_udc_start(struct usb_gadget *gadget, > >>>> return -EINVAL; > >>>> } > >>>> > >>>> + mutex_lock(&hsotg->init_mutex); > >>>> WARN_ON(hsotg->driver); > >>>> > >>>> driver->driver.bus = NULL; > >>>> @@ -2933,9 +2935,12 @@ static int s3c_hsotg_udc_start(struct usb_gadget *gadget, > >>>> > >>>> dev_info(hsotg->dev, "bound driver %s\n", driver->driver.name); > >>>> > >>>> + mutex_unlock(&hsotg->init_mutex); > >>>> + > >>>> return 0; > >>>> > >>>> err: > >>>> + mutex_unlock(&hsotg->init_mutex); > >>>> hsotg->driver = NULL; > >>>> return ret; > >>>> } > >>>> @@ -2957,6 +2962,8 @@ static int s3c_hsotg_udc_stop(struct usb_gadget *gadget, > >>>> if (!hsotg) > >>>> return -ENODEV; > >>>> > >>>> + mutex_lock(&hsotg->init_mutex); > >>>> + > >>>> /* all endpoints should be shutdown */ > >>>> for (ep = 1; ep < hsotg->num_of_eps; ep++) > >>>> s3c_hsotg_ep_disable(&hsotg->eps[ep].ep); > >>>> @@ -2974,6 +2981,8 @@ static int s3c_hsotg_udc_stop(struct usb_gadget *gadget, > >>>> > >>>> clk_disable(hsotg->clk); > >>>> > >>>> + mutex_unlock(&hsotg->init_mutex); > >>>> + > >>>> return 0; > >>>> } > >>>> > >>>> @@ -3002,6 +3011,7 @@ static int s3c_hsotg_pullup(struct usb_gadget *gadget, int is_on) > >>>> > >>>> dev_dbg(hsotg->dev, "%s: is_on: %d\n", __func__, is_on); > >>>> > >>>> + mutex_lock(&hsotg->init_mutex); > >>>> spin_lock_irqsave(&hsotg->lock, flags); > >>>> if (is_on) { > >>>> clk_enable(hsotg->clk); > >>>> @@ -3013,6 +3023,7 @@ static int s3c_hsotg_pullup(struct usb_gadget *gadget, int is_on) > >>>> > >>>> hsotg->gadget.speed = USB_SPEED_UNKNOWN; > >>>> spin_unlock_irqrestore(&hsotg->lock, flags); > >>>> + mutex_unlock(&hsotg->init_mutex); > >>>> > >>>> return 0; > >>>> } > >>>> @@ -3507,6 +3518,7 @@ static int s3c_hsotg_probe(struct platform_device *pdev) > >>>> } > >>>> > >>>> spin_lock_init(&hsotg->lock); > >>>> + mutex_init(&hsotg->init_mutex); > >>>> > >>>> hsotg->irq = ret; > >>>> > >>>> @@ -3652,6 +3664,8 @@ static int s3c_hsotg_suspend(struct platform_device *pdev, pm_message_t > >> state) > >>>> unsigned long flags; > >>>> int ret = 0; > >>>> > >>>> + mutex_lock(&hsotg->init_mutex); > >>>> + > >>>> if (hsotg->driver) > >>>> dev_info(hsotg->dev, "suspending usb gadget %s\n", > >>>> hsotg->driver->driver.name); > >>>> @@ -3674,6 +3688,8 @@ static int s3c_hsotg_suspend(struct platform_device *pdev, pm_message_t > >> state) > >>>> clk_disable(hsotg->clk); > >>>> } > >>>> > >>>> + mutex_unlock(&hsotg->init_mutex); > >>>> + > >>>> return ret; > >>>> } > >>>> > >>>> @@ -3683,7 +3699,9 @@ static int s3c_hsotg_resume(struct platform_device *pdev) > >>>> unsigned long flags; > >>>> int ret = 0; > >>>> > >>>> + mutex_lock(&hsotg->init_mutex); > >>>> if (hsotg->driver) { > >>>> + > >>>> dev_info(hsotg->dev, "resuming usb gadget %s\n", > >>>> hsotg->driver->driver.name); > >>>> > >>>> @@ -3699,6 +3717,8 @@ static int s3c_hsotg_resume(struct platform_device *pdev) > >>>> s3c_hsotg_core_connect(hsotg); > >>>> spin_unlock_irqrestore(&hsotg->lock, flags); > >>>> > >>>> + mutex_unlock(&hsotg->init_mutex); > >>>> + > >>>> return ret; > >>>> } > >>>> > >>> Hmm. I can't find any other UDC driver that uses a mutex in its > >>> suspend/resume functions. Can you explain why this is needed only > >>> for dwc2? > >> I've posted this version because I thought you were not convinced that > >> the patch > >> "usb: dwc2/gadget: rework suspend/resume code to correctly restore > >> gadget state" > >> can add code for initialization and deinitialization in suspend/resume > >> paths. > > My problem with that patch was that you were checking the ->enabled > > flag outside of the spinlock. To address that, you only need to move > > the check inside of the spinlock. I don't see why a mutex is needed. > > It is not that simple. I can add spin_lock() before checking enabled, > but then > I would need to spin_unlock() to call regulator_bulk_enable() and > phy_enable(), > because both cannot be called from atomic context. This means that the > spinlock > in such case will not protect anything and is simply useless. Ah, OK. So you're using the mutex instead of the ->enabled flag that you proposed in the "rework suspend/resume code" patch. So this patch is a replacement for that one. Somehow I was thinking this patch was on top of that one. So I guess this is OK, but I would like to get Felipe's opinion about it before we apply this. Felipe? -- Paul ��.n��������+%������w��{.n�����{���)�� jg��� �����ݢj����G���� ���j:+v���w�m������w�������h�����٥
