From mboxrd@z Thu Jan 1 00:00:00 1970 From: Marek Szyprowski Subject: Re: [PATCH v2 10/10] usb: dwc2/gadget: rework suspend/resume code to correctly restore gadget state Date: Fri, 31 Oct 2014 11:08:02 +0100 Message-ID: <54535F82.7020406@samsung.com> References: <1413801940-31086-1-git-send-email-m.szyprowski@samsung.com> <1413801940-31086-11-git-send-email-m.szyprowski@samsung.com> <544DF1D6.3030003@samsung.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Return-path: Received: from mailout1.w1.samsung.com ([210.118.77.11]:47770 "EHLO mailout1.w1.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756393AbaJaKIE (ORCPT ); Fri, 31 Oct 2014 06:08:04 -0400 In-reply-to: <544DF1D6.3030003@samsung.com> Sender: linux-samsung-soc-owner@vger.kernel.org List-Id: linux-samsung-soc@vger.kernel.org To: Paul Zimmerman , "linux-usb@vger.kernel.org" , "linux-samsung-soc@vger.kernel.org" Cc: Felipe Balbi , Kyungmin Park , Robert Baldyga , Krzysztof Kozlowski Hello, On 2014-10-27 08:18, Marek Szyprowski wrote: > > On 2014-10-25 03:16, Paul Zimmerman wrote: >>> From: Marek Szyprowski [mailto:m.szyprowski@samsung.com] >>> Sent: Monday, October 20, 2014 3:46 AM >>> >>> Suspend/resume code assumed that the gadget was always enabled and >>> connected to usb bus. This means that the actual state of the gadget >>> (soft-enabled/disabled or connected/disconnected) was not correctly >>> preserved on suspend/resume cycle. This patch fixes this issue. >>> >>> Signed-off-by: Marek Szyprowski >>> --- >>> drivers/usb/dwc2/core.h | 4 +++- >>> drivers/usb/dwc2/gadget.c | 43 >>> +++++++++++++++++++++++++++---------------- >>> 2 files changed, 30 insertions(+), 17 deletions(-) >>> >>> diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h >>> index bf015ab3b44c..3648b76a18b4 100644 >>> --- a/drivers/usb/dwc2/core.h >>> +++ b/drivers/usb/dwc2/core.h >>> @@ -210,7 +210,9 @@ struct s3c_hsotg { >>> u8 ctrl_buff[8]; >>> >>> struct usb_gadget gadget; >>> - unsigned int setup; >>> + unsigned int setup:1; >>> + unsigned int connected:1; >>> + unsigned int enabled:1; >>> unsigned long last_rst; >>> struct s3c_hsotg_ep *eps; >>> }; >>> diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c >>> index 0d34cfc71bfb..c6c6cf982c90 100644 >>> --- a/drivers/usb/dwc2/gadget.c >>> +++ b/drivers/usb/dwc2/gadget.c >>> @@ -2925,6 +2925,8 @@ static int s3c_hsotg_udc_start(struct >>> usb_gadget *gadget, >>> spin_lock_irqsave(&hsotg->lock, flags); >>> s3c_hsotg_init(hsotg); >>> s3c_hsotg_core_init_disconnected(hsotg); >>> + hsotg->enabled = 1; >>> + hsotg->connected = 0; >>> spin_unlock_irqrestore(&hsotg->lock, flags); >>> >>> dev_info(hsotg->dev, "bound driver %s\n", driver->driver.name); >>> @@ -2961,6 +2963,8 @@ static int s3c_hsotg_udc_stop(struct >>> usb_gadget *gadget, >>> >>> hsotg->driver = NULL; >>> hsotg->gadget.speed = USB_SPEED_UNKNOWN; >>> + hsotg->enabled = 0; >>> + hsotg->connected = 0; >>> >>> spin_unlock_irqrestore(&hsotg->lock, flags); >>> >>> @@ -2999,11 +3003,14 @@ static int s3c_hsotg_pullup(struct >>> usb_gadget *gadget, int is_on) >>> dev_dbg(hsotg->dev, "%s: is_on: %d\n", __func__, is_on); >>> >>> spin_lock_irqsave(&hsotg->lock, flags); >>> + >>> if (is_on) { >>> clk_enable(hsotg->clk); >>> + hsotg->connected = 1; >>> s3c_hsotg_core_connect(hsotg); >>> } else { >>> s3c_hsotg_core_disconnect(hsotg); >>> + hsotg->connected = 0; >>> clk_disable(hsotg->clk); >>> } >>> >>> @@ -3652,16 +3659,18 @@ static int s3c_hsotg_suspend(struct >>> platform_device *pdev, pm_message_t state) >>> dev_info(hsotg->dev, "suspending usb gadget %s\n", >>> hsotg->driver->driver.name); >>> >>> - spin_lock_irqsave(&hsotg->lock, flags); >>> - s3c_hsotg_core_disconnect(hsotg); >>> - s3c_hsotg_disconnect(hsotg); >>> - hsotg->gadget.speed = USB_SPEED_UNKNOWN; >>> - spin_unlock_irqrestore(&hsotg->lock, flags); >>> + if (hsotg->enabled) { >> Hmm. Are you sure it's safe to check ->enabled outside of the spinlock? >> What happens if s3c_hsotg_udc_stop() runs right after this, before the >> spinlock is taken, and disables stuff? Sure, it's a tiny window, but >> still... > > This code is called only in system suspend path, when userspace has > been already frozen. > udc_stop() can be called only as a result of userspace action, so it > is imho safe to > assume that the above code doesn't need additional locking. However if you prefer the version with explicit locking, I will send v3 in a few minutes. Best regards -- Marek Szyprowski, PhD Samsung R&D Institute Poland