> 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. -- Paul ��.n��������+%������w��{.n�����{���)�� jg��� �����ݢj����G���� ���j:+v���w�m������w�������h�����٥
