Re: [PATCH v3 1/2] usb: dwc2/gadget: add mutex to serialize init/deinit calls

2014-11-20 Thread Felipe Balbi 
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 m.szyprow...@samsung.com

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

2014-11-18 Thread Paul Zimmerman 
 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 m.szyprow...@samsung.com
 ---
  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 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;
  }
 

Acked-by: Paul Zimmerman pa...@synopsys.com

--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc 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

2014-11-14 Thread Marek Szyprowski 

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 m.szyprow...@samsung.com
---
   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 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

RE: [PATCH v3 1/2] usb: dwc2/gadget: add mutex to serialize init/deinit calls

2014-11-14 Thread Paul Zimmerman 
 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 m.szyprow...@samsung.com
  ---
 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 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

Re: [PATCH v3 1/2] usb: dwc2/gadget: add mutex to serialize init/deinit calls

2014-11-14 Thread Felipe Balbi 
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

2014-11-13 Thread Marek Szyprowski 

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 m.szyprow...@samsung.com
---
  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 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.


Best regards
--
Marek Szyprowski, PhD
Samsung RD Institute Poland

--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc 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

2014-11-13 Thread Paul Zimmerman 
 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 m.szyprow...@samsung.com
  ---
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 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

[PATCH v3 1/2] usb: dwc2/gadget: add mutex to serialize init/deinit calls

2014-10-31 Thread Marek Szyprowski 
This patch adds mutex, which protects initialization and
deinitialization procedures against suspend/resume methods.

Signed-off-by: Marek Szyprowski m.szyprow...@samsung.com
---
 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 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;
 }
 
-- 
1.9.2

--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc 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

2014-10-31 Thread Paul Zimmerman 
 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 m.szyprow...@samsung.com
 ---
  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 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?

-- 
Paul

--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html