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

Message ID 1414750354-19571-1-git-send-email-m.szyprowski@samsung.com
State New
Headers show

Commit Message

Marek Szyprowski Oct. 31, 2014, 10:12 a.m.
This patch adds mutex, which protects initialization and
deinitialization procedures against suspend/resume methods.

Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
 drivers/usb/dwc2/core.h   |  1 +
 drivers/usb/dwc2/gadget.c | 20 ++++++++++++++++++++
 2 files changed, 21 insertions(+)

Comments

Paul Zimmerman Oct. 31, 2014, 6:46 p.m. | #1
> From: Marek Szyprowski [mailto:m.szyprowski@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.szyprowski@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 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?
Marek Szyprowski Nov. 13, 2014, 3:17 p.m. | #2
Hello,

On 2014-10-31 19:46, Paul Zimmerman wrote:
>> From: Marek Szyprowski [mailto:m.szyprowski@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.szyprowski@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 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.

Best regards
Paul Zimmerman Nov. 13, 2014, 8:55 p.m. | #3
> From: Marek Szyprowski [mailto:m.szyprowski@samsung.com]

> Sent: Thursday, November 13, 2014 7:18 AM

> 

> On 2014-10-31 19:46, Paul Zimmerman wrote:

> >> From: Marek Szyprowski [mailto:m.szyprowski@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.szyprowski@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 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
Marek Szyprowski Nov. 14, 2014, 9:19 a.m. | #4
Hello,

On 2014-11-13 21:55, Paul Zimmerman wrote:
>> From: Marek Szyprowski [mailto:m.szyprowski@samsung.com]
>> Sent: Thursday, November 13, 2014 7:18 AM
>>
>> On 2014-10-31 19:46, Paul Zimmerman wrote:
>>>> From: Marek Szyprowski [mailto:m.szyprowski@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.szyprowski@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 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.

Best regards
Paul Zimmerman Nov. 14, 2014, 7:43 p.m. | #5
PiBGcm9tOiBNYXJlayBTenlwcm93c2tpIFttYWlsdG86bS5zenlwcm93c2tpQHNhbXN1bmcuY29t
XQ0KPiBTZW50OiBGcmlkYXksIE5vdmVtYmVyIDE0LCAyMDE0IDE6MTkgQU0NCj4gDQo+IE9uIDIw
MTQtMTEtMTMgMjE6NTUsIFBhdWwgWmltbWVybWFuIHdyb3RlOg0KPiA+PiBGcm9tOiBNYXJlayBT
enlwcm93c2tpIFttYWlsdG86bS5zenlwcm93c2tpQHNhbXN1bmcuY29tXQ0KPiA+PiBTZW50OiBU
aHVyc2RheSwgTm92ZW1iZXIgMTMsIDIwMTQgNzoxOCBBTQ0KPiA+Pg0KPiA+PiBPbiAyMDE0LTEw
LTMxIDE5OjQ2LCBQYXVsIFppbW1lcm1hbiB3cm90ZToNCj4gPj4+PiBGcm9tOiBNYXJlayBTenlw
cm93c2tpIFttYWlsdG86bS5zenlwcm93c2tpQHNhbXN1bmcuY29tXQ0KPiA+Pj4+IFNlbnQ6IEZy
aWRheSwgT2N0b2JlciAzMSwgMjAxNCAzOjEzIEFNDQo+ID4+Pj4NCj4gPj4+PiBUaGlzIHBhdGNo
IGFkZHMgbXV0ZXgsIHdoaWNoIHByb3RlY3RzIGluaXRpYWxpemF0aW9uIGFuZA0KPiA+Pj4+IGRl
aW5pdGlhbGl6YXRpb24gcHJvY2VkdXJlcyBhZ2FpbnN0IHN1c3BlbmQvcmVzdW1lIG1ldGhvZHMu
DQo+ID4+Pj4NCj4gPj4+PiBTaWduZWQtb2ZmLWJ5OiBNYXJlayBTenlwcm93c2tpIDxtLnN6eXBy
b3dza2lAc2Ftc3VuZy5jb20+DQo+ID4+Pj4gLS0tDQo+ID4+Pj4gICAgZHJpdmVycy91c2IvZHdj
Mi9jb3JlLmggICB8ICAxICsNCj4gPj4+PiAgICBkcml2ZXJzL3VzYi9kd2MyL2dhZGdldC5jIHwg
MjAgKysrKysrKysrKysrKysrKysrKysNCj4gPj4+PiAgICAyIGZpbGVzIGNoYW5nZWQsIDIxIGlu
c2VydGlvbnMoKykNCj4gPj4+Pg0KPiA+Pj4+IGRpZmYgLS1naXQgYS9kcml2ZXJzL3VzYi9kd2My
L2NvcmUuaCBiL2RyaXZlcnMvdXNiL2R3YzIvY29yZS5oDQo+ID4+Pj4gaW5kZXggOWY3N2I0ZDFj
NWZmLi41ODczMmE5YTAwMTkgMTAwNjQ0DQo+ID4+Pj4gLS0tIGEvZHJpdmVycy91c2IvZHdjMi9j
b3JlLmgNCj4gPj4+PiArKysgYi9kcml2ZXJzL3VzYi9kd2MyL2NvcmUuaA0KPiA+Pj4+IEBAIC0x
ODcsNiArMTg3LDcgQEAgc3RydWN0IHMzY19oc290ZyB7DQo+ID4+Pj4gICAgCXN0cnVjdCBzM2Nf
aHNvdGdfcGxhdCAgICAqcGxhdDsNCj4gPj4+Pg0KPiA+Pj4+ICAgIAlzcGlubG9ja190ICAgICAg
ICAgICAgICBsb2NrOw0KPiA+Pj4+ICsJc3RydWN0IG11dGV4CQlpbml0X211dGV4Ow0KPiA+Pj4+
DQo+ID4+Pj4gICAgCXZvaWQgX19pb21lbSAgICAgICAgICAgICpyZWdzOw0KPiA+Pj4+ICAgIAlp
bnQgICAgICAgICAgICAgICAgICAgICBpcnE7DQo+ID4+Pj4gZGlmZiAtLWdpdCBhL2RyaXZlcnMv
dXNiL2R3YzIvZ2FkZ2V0LmMgYi9kcml2ZXJzL3VzYi9kd2MyL2dhZGdldC5jDQo+ID4+Pj4gaW5k
ZXggZDhkZGEzOWM5ZTE2Li5hMmU0MjcyYTkwNGUgMTAwNjQ0DQo+ID4+Pj4gLS0tIGEvZHJpdmVy
cy91c2IvZHdjMi9nYWRnZXQuYw0KPiA+Pj4+ICsrKyBiL2RyaXZlcnMvdXNiL2R3YzIvZ2FkZ2V0
LmMNCj4gPj4+PiBAQCAtMjEsNiArMjEsNyBAQA0KPiA+Pj4+ICAgICNpbmNsdWRlIDxsaW51eC9w
bGF0Zm9ybV9kZXZpY2UuaD4NCj4gPj4+PiAgICAjaW5jbHVkZSA8bGludXgvZG1hLW1hcHBpbmcu
aD4NCj4gPj4+PiAgICAjaW5jbHVkZSA8bGludXgvZGVidWdmcy5oPg0KPiA+Pj4+ICsjaW5jbHVk
ZSA8bGludXgvbXV0ZXguaD4NCj4gPj4+PiAgICAjaW5jbHVkZSA8bGludXgvc2VxX2ZpbGUuaD4N
Cj4gPj4+PiAgICAjaW5jbHVkZSA8bGludXgvZGVsYXkuaD4NCj4gPj4+PiAgICAjaW5jbHVkZSA8
bGludXgvaW8uaD4NCj4gPj4+PiBAQCAtMjkwOCw2ICsyOTA5LDcgQEAgc3RhdGljIGludCBzM2Nf
aHNvdGdfdWRjX3N0YXJ0KHN0cnVjdCB1c2JfZ2FkZ2V0ICpnYWRnZXQsDQo+ID4+Pj4gICAgCQly
ZXR1cm4gLUVJTlZBTDsNCj4gPj4+PiAgICAJfQ0KPiA+Pj4+DQo+ID4+Pj4gKwltdXRleF9sb2Nr
KCZoc290Zy0+aW5pdF9tdXRleCk7DQo+ID4+Pj4gICAgCVdBUk5fT04oaHNvdGctPmRyaXZlcik7
DQo+ID4+Pj4NCj4gPj4+PiAgICAJZHJpdmVyLT5kcml2ZXIuYnVzID0gTlVMTDsNCj4gPj4+PiBA
QCAtMjkzMyw5ICsyOTM1LDEyIEBAIHN0YXRpYyBpbnQgczNjX2hzb3RnX3VkY19zdGFydChzdHJ1
Y3QgdXNiX2dhZGdldCAqZ2FkZ2V0LA0KPiA+Pj4+DQo+ID4+Pj4gICAgCWRldl9pbmZvKGhzb3Rn
LT5kZXYsICJib3VuZCBkcml2ZXIgJXNcbiIsIGRyaXZlci0+ZHJpdmVyLm5hbWUpOw0KPiA+Pj4+
DQo+ID4+Pj4gKwltdXRleF91bmxvY2soJmhzb3RnLT5pbml0X211dGV4KTsNCj4gPj4+PiArDQo+
ID4+Pj4gICAgCXJldHVybiAwOw0KPiA+Pj4+DQo+ID4+Pj4gICAgZXJyOg0KPiA+Pj4+ICsJbXV0
ZXhfdW5sb2NrKCZoc290Zy0+aW5pdF9tdXRleCk7DQo+ID4+Pj4gICAgCWhzb3RnLT5kcml2ZXIg
PSBOVUxMOw0KPiA+Pj4+ICAgIAlyZXR1cm4gcmV0Ow0KPiA+Pj4+ICAgIH0NCj4gPj4+PiBAQCAt
Mjk1Nyw2ICsyOTYyLDggQEAgc3RhdGljIGludCBzM2NfaHNvdGdfdWRjX3N0b3Aoc3RydWN0IHVz
Yl9nYWRnZXQgKmdhZGdldCwNCj4gPj4+PiAgICAJaWYgKCFoc290ZykNCj4gPj4+PiAgICAJCXJl
dHVybiAtRU5PREVWOw0KPiA+Pj4+DQo+ID4+Pj4gKwltdXRleF9sb2NrKCZoc290Zy0+aW5pdF9t
dXRleCk7DQo+ID4+Pj4gKw0KPiA+Pj4+ICAgIAkvKiBhbGwgZW5kcG9pbnRzIHNob3VsZCBiZSBz
aHV0ZG93biAqLw0KPiA+Pj4+ICAgIAlmb3IgKGVwID0gMTsgZXAgPCBoc290Zy0+bnVtX29mX2Vw
czsgZXArKykNCj4gPj4+PiAgICAJCXMzY19oc290Z19lcF9kaXNhYmxlKCZoc290Zy0+ZXBzW2Vw
XS5lcCk7DQo+ID4+Pj4gQEAgLTI5NzQsNiArMjk4MSw4IEBAIHN0YXRpYyBpbnQgczNjX2hzb3Rn
X3VkY19zdG9wKHN0cnVjdCB1c2JfZ2FkZ2V0ICpnYWRnZXQsDQo+ID4+Pj4NCj4gPj4+PiAgICAJ
Y2xrX2Rpc2FibGUoaHNvdGctPmNsayk7DQo+ID4+Pj4NCj4gPj4+PiArCW11dGV4X3VubG9jaygm
aHNvdGctPmluaXRfbXV0ZXgpOw0KPiA+Pj4+ICsNCj4gPj4+PiAgICAJcmV0dXJuIDA7DQo+ID4+
Pj4gICAgfQ0KPiA+Pj4+DQo+ID4+Pj4gQEAgLTMwMDIsNiArMzAxMSw3IEBAIHN0YXRpYyBpbnQg
czNjX2hzb3RnX3B1bGx1cChzdHJ1Y3QgdXNiX2dhZGdldCAqZ2FkZ2V0LCBpbnQgaXNfb24pDQo+
ID4+Pj4NCj4gPj4+PiAgICAJZGV2X2RiZyhoc290Zy0+ZGV2LCAiJXM6IGlzX29uOiAlZFxuIiwg
X19mdW5jX18sIGlzX29uKTsNCj4gPj4+Pg0KPiA+Pj4+ICsJbXV0ZXhfbG9jaygmaHNvdGctPmlu
aXRfbXV0ZXgpOw0KPiA+Pj4+ICAgIAlzcGluX2xvY2tfaXJxc2F2ZSgmaHNvdGctPmxvY2ssIGZs
YWdzKTsNCj4gPj4+PiAgICAJaWYgKGlzX29uKSB7DQo+ID4+Pj4gICAgCQljbGtfZW5hYmxlKGhz
b3RnLT5jbGspOw0KPiA+Pj4+IEBAIC0zMDEzLDYgKzMwMjMsNyBAQCBzdGF0aWMgaW50IHMzY19o
c290Z19wdWxsdXAoc3RydWN0IHVzYl9nYWRnZXQgKmdhZGdldCwgaW50IGlzX29uKQ0KPiA+Pj4+
DQo+ID4+Pj4gICAgCWhzb3RnLT5nYWRnZXQuc3BlZWQgPSBVU0JfU1BFRURfVU5LTk9XTjsNCj4g
Pj4+PiAgICAJc3Bpbl91bmxvY2tfaXJxcmVzdG9yZSgmaHNvdGctPmxvY2ssIGZsYWdzKTsNCj4g
Pj4+PiArCW11dGV4X3VubG9jaygmaHNvdGctPmluaXRfbXV0ZXgpOw0KPiA+Pj4+DQo+ID4+Pj4g
ICAgCXJldHVybiAwOw0KPiA+Pj4+ICAgIH0NCj4gPj4+PiBAQCAtMzUwNyw2ICszNTE4LDcgQEAg
c3RhdGljIGludCBzM2NfaHNvdGdfcHJvYmUoc3RydWN0IHBsYXRmb3JtX2RldmljZSAqcGRldikN
Cj4gPj4+PiAgICAJfQ0KPiA+Pj4+DQo+ID4+Pj4gICAgCXNwaW5fbG9ja19pbml0KCZoc290Zy0+
bG9jayk7DQo+ID4+Pj4gKwltdXRleF9pbml0KCZoc290Zy0+aW5pdF9tdXRleCk7DQo+ID4+Pj4N
Cj4gPj4+PiAgICAJaHNvdGctPmlycSA9IHJldDsNCj4gPj4+Pg0KPiA+Pj4+IEBAIC0zNjUyLDYg
KzM2NjQsOCBAQCBzdGF0aWMgaW50IHMzY19oc290Z19zdXNwZW5kKHN0cnVjdCBwbGF0Zm9ybV9k
ZXZpY2UgKnBkZXYsIHBtX21lc3NhZ2VfdA0KPiA+PiBzdGF0ZSkNCj4gPj4+PiAgICAJdW5zaWdu
ZWQgbG9uZyBmbGFnczsNCj4gPj4+PiAgICAJaW50IHJldCA9IDA7DQo+ID4+Pj4NCj4gPj4+PiAr
CW11dGV4X2xvY2soJmhzb3RnLT5pbml0X211dGV4KTsNCj4gPj4+PiArDQo+ID4+Pj4gICAgCWlm
IChoc290Zy0+ZHJpdmVyKQ0KPiA+Pj4+ICAgIAkJZGV2X2luZm8oaHNvdGctPmRldiwgInN1c3Bl
bmRpbmcgdXNiIGdhZGdldCAlc1xuIiwNCj4gPj4+PiAgICAJCQkgaHNvdGctPmRyaXZlci0+ZHJp
dmVyLm5hbWUpOw0KPiA+Pj4+IEBAIC0zNjc0LDYgKzM2ODgsOCBAQCBzdGF0aWMgaW50IHMzY19o
c290Z19zdXNwZW5kKHN0cnVjdCBwbGF0Zm9ybV9kZXZpY2UgKnBkZXYsIHBtX21lc3NhZ2VfdA0K
PiA+PiBzdGF0ZSkNCj4gPj4+PiAgICAJCWNsa19kaXNhYmxlKGhzb3RnLT5jbGspOw0KPiA+Pj4+
ICAgIAl9DQo+ID4+Pj4NCj4gPj4+PiArCW11dGV4X3VubG9jaygmaHNvdGctPmluaXRfbXV0ZXgp
Ow0KPiA+Pj4+ICsNCj4gPj4+PiAgICAJcmV0dXJuIHJldDsNCj4gPj4+PiAgICB9DQo+ID4+Pj4N
Cj4gPj4+PiBAQCAtMzY4Myw3ICszNjk5LDkgQEAgc3RhdGljIGludCBzM2NfaHNvdGdfcmVzdW1l
KHN0cnVjdCBwbGF0Zm9ybV9kZXZpY2UgKnBkZXYpDQo+ID4+Pj4gICAgCXVuc2lnbmVkIGxvbmcg
ZmxhZ3M7DQo+ID4+Pj4gICAgCWludCByZXQgPSAwOw0KPiA+Pj4+DQo+ID4+Pj4gKwltdXRleF9s
b2NrKCZoc290Zy0+aW5pdF9tdXRleCk7DQo+ID4+Pj4gICAgCWlmIChoc290Zy0+ZHJpdmVyKSB7
DQo+ID4+Pj4gKw0KPiA+Pj4+ICAgIAkJZGV2X2luZm8oaHNvdGctPmRldiwgInJlc3VtaW5nIHVz
YiBnYWRnZXQgJXNcbiIsDQo+ID4+Pj4gICAgCQkJIGhzb3RnLT5kcml2ZXItPmRyaXZlci5uYW1l
KTsNCj4gPj4+Pg0KPiA+Pj4+IEBAIC0zNjk5LDYgKzM3MTcsOCBAQCBzdGF0aWMgaW50IHMzY19o
c290Z19yZXN1bWUoc3RydWN0IHBsYXRmb3JtX2RldmljZSAqcGRldikNCj4gPj4+PiAgICAJczNj
X2hzb3RnX2NvcmVfY29ubmVjdChoc290Zyk7DQo+ID4+Pj4gICAgCXNwaW5fdW5sb2NrX2lycXJl
c3RvcmUoJmhzb3RnLT5sb2NrLCBmbGFncyk7DQo+ID4+Pj4NCj4gPj4+PiArCW11dGV4X3VubG9j
aygmaHNvdGctPmluaXRfbXV0ZXgpOw0KPiA+Pj4+ICsNCj4gPj4+PiAgICAJcmV0dXJuIHJldDsN
Cj4gPj4+PiAgICB9DQo+ID4+Pj4NCj4gPj4+IEhtbS4gSSBjYW4ndCBmaW5kIGFueSBvdGhlciBV
REMgZHJpdmVyIHRoYXQgdXNlcyBhIG11dGV4IGluIGl0cw0KPiA+Pj4gc3VzcGVuZC9yZXN1bWUg
ZnVuY3Rpb25zLiBDYW4geW91IGV4cGxhaW4gd2h5IHRoaXMgaXMgbmVlZGVkIG9ubHkNCj4gPj4+
IGZvciBkd2MyPw0KPiA+PiBJJ3ZlIHBvc3RlZCB0aGlzIHZlcnNpb24gYmVjYXVzZSBJIHRob3Vn
aHQgeW91IHdlcmUgbm90IGNvbnZpbmNlZCB0aGF0DQo+ID4+IHRoZSBwYXRjaA0KPiA+PiAidXNi
OiBkd2MyL2dhZGdldDogcmV3b3JrIHN1c3BlbmQvcmVzdW1lIGNvZGUgdG8gY29ycmVjdGx5IHJl
c3RvcmUNCj4gPj4gZ2FkZ2V0IHN0YXRlIg0KPiA+PiBjYW4gYWRkIGNvZGUgZm9yIGluaXRpYWxp
emF0aW9uIGFuZCBkZWluaXRpYWxpemF0aW9uIGluIHN1c3BlbmQvcmVzdW1lDQo+ID4+IHBhdGhz
Lg0KPiA+IE15IHByb2JsZW0gd2l0aCB0aGF0IHBhdGNoIHdhcyB0aGF0IHlvdSB3ZXJlIGNoZWNr
aW5nIHRoZSAtPmVuYWJsZWQNCj4gPiBmbGFnIG91dHNpZGUgb2YgdGhlIHNwaW5sb2NrLiBUbyBh
ZGRyZXNzIHRoYXQsIHlvdSBvbmx5IG5lZWQgdG8gbW92ZQ0KPiA+IHRoZSBjaGVjayBpbnNpZGUg
b2YgdGhlIHNwaW5sb2NrLiBJIGRvbid0IHNlZSB3aHkgYSBtdXRleCBpcyBuZWVkZWQuDQo+IA0K
PiBJdCBpcyBub3QgdGhhdCBzaW1wbGUuIEkgY2FuIGFkZCBzcGluX2xvY2soKSBiZWZvcmUgY2hl
Y2tpbmcgZW5hYmxlZCwNCj4gYnV0IHRoZW4NCj4gSSB3b3VsZCBuZWVkIHRvIHNwaW5fdW5sb2Nr
KCkgdG8gY2FsbCByZWd1bGF0b3JfYnVsa19lbmFibGUoKSBhbmQNCj4gcGh5X2VuYWJsZSgpLA0K
PiBiZWNhdXNlIGJvdGggY2Fubm90IGJlIGNhbGxlZCBmcm9tIGF0b21pYyBjb250ZXh0LiBUaGlz
IG1lYW5zIHRoYXQgdGhlDQo+IHNwaW5sb2NrDQo+IGluIHN1Y2ggY2FzZSB3aWxsIG5vdCBwcm90
ZWN0IGFueXRoaW5nIGFuZCBpcyBzaW1wbHkgdXNlbGVzcy4NCg0KQWgsIE9LLiBTbyB5b3UncmUg
dXNpbmcgdGhlIG11dGV4IGluc3RlYWQgb2YgdGhlIC0+ZW5hYmxlZCBmbGFnIHRoYXQgeW91DQpw
cm9wb3NlZCBpbiB0aGUgInJld29yayBzdXNwZW5kL3Jlc3VtZSBjb2RlIiBwYXRjaC4gU28gdGhp
cyBwYXRjaCBpcyBhDQpyZXBsYWNlbWVudCBmb3IgdGhhdCBvbmUuIFNvbWVob3cgSSB3YXMgdGhp
bmtpbmcgdGhpcyBwYXRjaCB3YXMgb24gdG9wDQpvZiB0aGF0IG9uZS4NCg0KU28gSSBndWVzcyB0
aGlzIGlzIE9LLCBidXQgSSB3b3VsZCBsaWtlIHRvIGdldCBGZWxpcGUncyBvcGluaW9uIGFib3V0
DQppdCBiZWZvcmUgd2UgYXBwbHkgdGhpcy4NCg0KRmVsaXBlPw0KDQotLSANClBhdWwNCg0K
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Felipe Balbi Nov. 14, 2014, 7:51 p.m. | #6
Hi,

On Fri, Nov 14, 2014 at 07:43:23PM +0000, 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 :-(
Paul Zimmerman Nov. 18, 2014, 9 p.m. | #7
> From: Marek Szyprowski [mailto:m.szyprowski@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.szyprowski@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 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;
>  }
> 

Acked-by: Paul Zimmerman <paulz@synopsys.com>

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Felipe Balbi Nov. 20, 2014, 7:53 p.m. | #8
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.szyprowski@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

Patch

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;
 }