Re: [PATCH v3 1/2] usb: dwc2/gadget: add mutex to serialize init/deinit calls
On Fri, Oct 31, 2014 at 11:12:33AM +0100, Marek Szyprowski wrote: > This patch adds mutex, which protects initialization and > deinitialization procedures against suspend/resume methods. > > Signed-off-by: Marek Szyprowski doesn't apply either: checking file drivers/usb/dwc2/core.h Hunk #1 FAILED at 187. 1 out of 1 hunk FAILED checking file drivers/usb/dwc2/gadget.c Hunk #2 succeeded at 2863 (offset -46 lines). Hunk #3 succeeded at 2889 (offset -46 lines). Hunk #4 succeeded at 2915 (offset -47 lines). Hunk #5 succeeded at 2934 (offset -47 lines). Hunk #6 succeeded at 2964 (offset -47 lines). Hunk #7 succeeded at 2976 (offset -47 lines). Hunk #8 FAILED at 3518. Hunk #9 succeeded at 3579 (offset -84 lines). Hunk #10 succeeded at 3603 with fuzz 1 (offset -84 lines). Hunk #11 succeeded at 3614 (offset -84 lines). Hunk #12 succeeded at 3632 with fuzz 1 (offset -84 lines). 1 out of 12 hunks FAILED please rebase on testing/next. Also, add Paul's Ack when doing so ;-) thanks -- balbi signature.asc Description: Digital signature
RE: [PATCH v3 1/2] usb: dwc2/gadget: add mutex to serialize init/deinit calls
> From: Marek Szyprowski [mailto:m.szyprow...@samsung.com]
> 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
> ---
> 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 mutexinit_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
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -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;
> }
>
Acked-by: Paul Zimmerman
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 1/2] usb: dwc2/gadget: add mutex to serialize init/deinit calls
Hi, On Fri, Nov 14, 2014 at 07:43:23PM +, Paul Zimmerman wrote: > > @@ -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? I can't think of a better way, I'm afraid :-( -- balbi signature.asc Description: Digital signature
RE: [PATCH v3 1/2] usb: dwc2/gadget: add mutex to serialize init/deinit calls
> From: Marek Szyprowski [mailto:m.szyprow...@samsung.com]
> Sent: Friday, November 14, 2014 1:19 AM
>
> On 2014-11-13 21:55, Paul Zimmerman wrote:
> >> From: Marek Szyprowski [mailto:m.szyprow...@samsung.com]
> >> Sent: Thursday, November 13, 2014 7:18 AM
> >>
> >> On 2014-10-31 19:46, Paul Zimmerman wrote:
> From: Marek Szyprowski [mailto:m.szyprow...@samsung.com]
> 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
> ---
> 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 mutexinit_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
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -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, "resumi
Re: [PATCH v3 1/2] usb: dwc2/gadget: add mutex to serialize init/deinit calls
Hello,
On 2014-11-13 21:55, Paul Zimmerman wrote:
From: Marek Szyprowski [mailto:m.szyprow...@samsung.com]
Sent: Thursday, November 13, 2014 7:18 AM
On 2014-10-31 19:46, Paul Zimmerman wrote:
From: Marek Szyprowski [mailto:m.szyprow...@samsung.com]
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
---
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 mutexinit_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
#include
#include
+#include
#include
#include
#include
@@ -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 calle
RE: [PATCH v3 1/2] usb: dwc2/gadget: add mutex to serialize init/deinit calls
> From: Marek Szyprowski [mailto:m.szyprow...@samsung.com]
> Sent: Thursday, November 13, 2014 7:18 AM
>
> On 2014-10-31 19:46, Paul Zimmerman wrote:
> >> From: Marek Szyprowski [mailto:m.szyprow...@samsung.com]
> >> 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
> >> ---
> >> 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 mutexinit_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
> >> #include
> >> #include
> >> +#include
> >> #include
> >> #include
> >> #include
> >> @@ -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 proble
Re: [PATCH v3 1/2] usb: dwc2/gadget: add mutex to serialize init/deinit calls
Hello,
On 2014-10-31 19:46, Paul Zimmerman wrote:
From: Marek Szyprowski [mailto:m.szyprow...@samsung.com]
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
---
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 mutexinit_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
#include
#include
+#include
#include
#include
#include
@@ -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.
Best regards
--
Marek Szyprowski, PhD
Samsung R&D Institute Poland
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v3 1/2] usb: dwc2/gadget: add mutex to serialize init/deinit calls
> From: Marek Szyprowski [mailto:m.szyprow...@samsung.com]
> 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
> ---
> 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 mutexinit_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
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -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?
--
Paul
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 1/2] usb: dwc2/gadget: add mutex to serialize init/deinit calls
This patch adds mutex, which protects initialization and
deinitialization procedures against suspend/resume methods.
Signed-off-by: Marek Szyprowski
---
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 mutexinit_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
#include
#include
+#include
#include
#include
#include
@@ -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;
}
--
1.9.2
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
