Linux-USB Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] usb: dwc2: Drop unlock/lock upon queueing a work item
@ 2019-11-20 10:15 Lukas Wunner
  2020-01-09 12:36 ` Lukas Wunner
  2020-01-15 13:11 ` Minas Harutyunyan
  0 siblings, 2 replies; 10+ messages in thread
From: Lukas Wunner @ 2019-11-20 10:15 UTC (permalink / raw)
  To: Minas Harutyunyan, Felipe Balbi; +Cc: linux-usb

The original dwc_otg driver used a DWC_WORKQ_SCHEDULE() wrapper to queue
work items.  Because that wrapper acquired the driver's global spinlock,
an unlock/lock dance was necessary whenever a work item was queued up
while the global spinlock was already held.

The dwc2 driver dropped DWC_WORKQ_SCHEDULE() in favor of a direct call
to queue_work(), but retained the (now gratuitous) unlock/lock dance in
dwc2_handle_conn_id_status_change_intr().  Drop it.

Signed-off-by: Lukas Wunner <lukas@wunner.de>
---
 drivers/usb/dwc2/core_intr.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/drivers/usb/dwc2/core_intr.c b/drivers/usb/dwc2/core_intr.c
index 6af6add3d4c0..876ff31261d5 100644
--- a/drivers/usb/dwc2/core_intr.c
+++ b/drivers/usb/dwc2/core_intr.c
@@ -288,14 +288,9 @@ static void dwc2_handle_conn_id_status_change_intr(struct dwc2_hsotg *hsotg)
 
 	/*
 	 * Need to schedule a work, as there are possible DELAY function calls.
-	 * Release lock before scheduling workq as it holds spinlock during
-	 * scheduling.
 	 */
-	if (hsotg->wq_otg) {
-		spin_unlock(&hsotg->lock);
+	if (hsotg->wq_otg)
 		queue_work(hsotg->wq_otg, &hsotg->wf_otg);
-		spin_lock(&hsotg->lock);
-	}
 }
 
 /**
-- 
2.24.0


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] usb: dwc2: Drop unlock/lock upon queueing a work item
  2019-11-20 10:15 [PATCH] usb: dwc2: Drop unlock/lock upon queueing a work item Lukas Wunner
@ 2020-01-09 12:36 ` Lukas Wunner
  2020-01-15  8:59   ` Felipe Balbi
  2020-01-15 13:11 ` Minas Harutyunyan
  1 sibling, 1 reply; 10+ messages in thread
From: Lukas Wunner @ 2020-01-09 12:36 UTC (permalink / raw)
  To: Minas Harutyunyan, Felipe Balbi; +Cc: linux-usb

Hi Felipe,

just a gentle ping:  The below patch was submitted more than 8 weeks ago
and I'm neither seeing it on one of your branches nor have there been
any comments on the list.  Are there objections to this patch?

Thanks,

Lukas

On Wed, Nov 20, 2019 at 11:15:15AM +0100, Lukas Wunner wrote:
> The original dwc_otg driver used a DWC_WORKQ_SCHEDULE() wrapper to queue
> work items.  Because that wrapper acquired the driver's global spinlock,
> an unlock/lock dance was necessary whenever a work item was queued up
> while the global spinlock was already held.
> 
> The dwc2 driver dropped DWC_WORKQ_SCHEDULE() in favor of a direct call
> to queue_work(), but retained the (now gratuitous) unlock/lock dance in
> dwc2_handle_conn_id_status_change_intr().  Drop it.
> 
> Signed-off-by: Lukas Wunner <lukas@wunner.de>
> ---
>  drivers/usb/dwc2/core_intr.c | 7 +------
>  1 file changed, 1 insertion(+), 6 deletions(-)
> 
> diff --git a/drivers/usb/dwc2/core_intr.c b/drivers/usb/dwc2/core_intr.c
> index 6af6add3d4c0..876ff31261d5 100644
> --- a/drivers/usb/dwc2/core_intr.c
> +++ b/drivers/usb/dwc2/core_intr.c
> @@ -288,14 +288,9 @@ static void dwc2_handle_conn_id_status_change_intr(struct dwc2_hsotg *hsotg)
>  
>  	/*
>  	 * Need to schedule a work, as there are possible DELAY function calls.
> -	 * Release lock before scheduling workq as it holds spinlock during
> -	 * scheduling.
>  	 */
> -	if (hsotg->wq_otg) {
> -		spin_unlock(&hsotg->lock);
> +	if (hsotg->wq_otg)
>  		queue_work(hsotg->wq_otg, &hsotg->wf_otg);
> -		spin_lock(&hsotg->lock);
> -	}
>  }
>  
>  /**
> -- 
> 2.24.0

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] usb: dwc2: Drop unlock/lock upon queueing a work item
  2020-01-09 12:36 ` Lukas Wunner
@ 2020-01-15  8:59   ` Felipe Balbi
  2020-01-15  9:25     ` Minas Harutyunyan
  0 siblings, 1 reply; 10+ messages in thread
From: Felipe Balbi @ 2020-01-15  8:59 UTC (permalink / raw)
  To: Lukas Wunner, Minas Harutyunyan, Felipe Balbi; +Cc: linux-usb

[-- Attachment #1: Type: text/plain, Size: 363 bytes --]


Hi,

Lukas Wunner <lukas@wunner.de> writes:
> Hi Felipe,
>
> just a gentle ping:  The below patch was submitted more than 8 weeks ago
> and I'm neither seeing it on one of your branches nor have there been
> any comments on the list.  Are there objections to this patch?

I don't see an Acked-by Minas, so I can't take the patch, sorry.

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] usb: dwc2: Drop unlock/lock upon queueing a work item
  2020-01-15  8:59   ` Felipe Balbi
@ 2020-01-15  9:25     ` Minas Harutyunyan
  2020-01-15 11:30       ` Lukas Wunner
  0 siblings, 1 reply; 10+ messages in thread
From: Minas Harutyunyan @ 2020-01-15  9:25 UTC (permalink / raw)
  To: Felipe Balbi, Lukas Wunner, Felipe Balbi; +Cc: linux-usb

Hi,

On 1/15/2020 12:59 PM, Felipe Balbi wrote:
> 
> Hi,
> 
> Lukas Wunner <lukas@wunner.de> writes:
>> Hi Felipe,
>>
>> just a gentle ping:  The below patch was submitted more than 8 weeks ago
>> and I'm neither seeing it on one of your branches nor have there been
>> any comments on the list.  Are there objections to this patch?
> 
> I don't see an Acked-by Minas, so I can't take the patch, sorry.
> 

But I didn't find original patch email in my inbox. I just got this ping.

Thanks,
Minas

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] usb: dwc2: Drop unlock/lock upon queueing a work item
  2020-01-15  9:25     ` Minas Harutyunyan
@ 2020-01-15 11:30       ` Lukas Wunner
  2020-01-15 11:44         ` Minas Harutyunyan
  0 siblings, 1 reply; 10+ messages in thread
From: Lukas Wunner @ 2020-01-15 11:30 UTC (permalink / raw)
  To: Minas Harutyunyan; +Cc: Felipe Balbi, Felipe Balbi, linux-usb

On Wed, Jan 15, 2020 at 09:25:30AM +0000, Minas Harutyunyan wrote:
> On 1/15/2020 12:59 PM, Felipe Balbi wrote:
> > Lukas Wunner <lukas@wunner.de> writes:
> > > just a gentle ping:  The below patch was submitted more than 8 weeks ago
> > > and I'm neither seeing it on one of your branches nor have there been
> > > any comments on the list.  Are there objections to this patch?
> > 
> > I don't see an Acked-by Minas, so I can't take the patch, sorry.
> 
> But I didn't find original patch email in my inbox. I just got this ping.

You were cc'ed on the patch and I didn't receive a bounce message.
So it must have been accepted by Synopsys' mail server.

I've just forwarded the e-mail to you once more.  Additionally you can
review the patch in the mailing list archive and, if there are no
objections, provide an Acked-by in reply to the present e-mail:

https://lore.kernel.org/linux-usb/77c07f00a6a9d94323c4a060a3c72817b0703b97.1574244795.git.lukas@wunner.de/

Thanks,

Lukas

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] usb: dwc2: Drop unlock/lock upon queueing a work item
  2020-01-15 11:30       ` Lukas Wunner
@ 2020-01-15 11:44         ` Minas Harutyunyan
  2020-01-15 11:57           ` Lukas Wunner
  0 siblings, 1 reply; 10+ messages in thread
From: Minas Harutyunyan @ 2020-01-15 11:44 UTC (permalink / raw)
  To: Lukas Wunner, Minas Harutyunyan; +Cc: Felipe Balbi, Felipe Balbi, linux-usb

Hi Lukas,

On 1/15/2020 3:30 PM, Lukas Wunner wrote:
> On Wed, Jan 15, 2020 at 09:25:30AM +0000, Minas Harutyunyan wrote:
>> On 1/15/2020 12:59 PM, Felipe Balbi wrote:
>>> Lukas Wunner <lukas@wunner.de> writes:
>>>> just a gentle ping:  The below patch was submitted more than 8 weeks ago
>>>> and I'm neither seeing it on one of your branches nor have there been
>>>> any comments on the list.  Are there objections to this patch?
>>>
>>> I don't see an Acked-by Minas, so I can't take the patch, sorry.
>>
>> But I didn't find original patch email in my inbox. I just got this ping.
> 
> You were cc'ed on the patch and I didn't receive a bounce message.
> So it must have been accepted by Synopsys' mail server.
> 
> I've just forwarded the e-mail to you once more.  Additionally you can
> review the patch in the mailing list archive and, if there are no
> objections, provide an Acked-by in reply to the present e-mail:

Ok. I just received your resubmitted patch.
> 
> https://urldefense.proofpoint.com/v2/url?u=https-3A__lore.kernel.org_linux-2Dusb_77c07f00a6a9d94323c4a060a3c72817b0703b97.1574244795.git.lukas-40wunner.de_&d=DwIBAg&c=DPL6_X_6JkXFx7AXWqB0tg&r=6z9Al9FrHR_ZqbbtSAsD16pvOL2S3XHxQnSzq8kusyI&m=RvUQvVjq5dj9CVUu3njmpdm88GS6B3rFv7iB9Rj8k4Y&s=p4AmQK1vx63kNa3BDdfxaOO1C80AvmgTQY5wtKJcXbc&e=
> 
> Thanks,
> 
> Lukas
> 
Actually I agree with you on unnecessary unlock/lock here. Currently I'm 
going to test your patch before Ack.
Just, want to check with you - did you see any issue in driver flow 
without this patch? or it just code cleanup?

Thanks,
Minas

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] usb: dwc2: Drop unlock/lock upon queueing a work item
  2020-01-15 11:44         ` Minas Harutyunyan
@ 2020-01-15 11:57           ` Lukas Wunner
  0 siblings, 0 replies; 10+ messages in thread
From: Lukas Wunner @ 2020-01-15 11:57 UTC (permalink / raw)
  To: Minas Harutyunyan; +Cc: Felipe Balbi, Felipe Balbi, linux-usb

On Wed, Jan 15, 2020 at 11:44:14AM +0000, Minas Harutyunyan wrote:
> On 1/15/2020 3:30 PM, Lukas Wunner wrote:
> > https://urldefense.proofpoint.com/v2/url?u=https-3A__lore.kernel.org_linux-2Dusb_77c07f00a6a9d94323c4a060a3c72817b0703b97.1574244795.git.lukas-40wunner.de_&d=DwIBAg&c=DPL6_X_6JkXFx7AXWqB0tg&r=6z9Al9FrHR_ZqbbtSAsD16pvOL2S3XHxQnSzq8kusyI&m=RvUQvVjq5dj9CVUu3njmpdm88GS6B3rFv7iB9Rj8k4Y&s=p4AmQK1vx63kNa3BDdfxaOO1C80AvmgTQY5wtKJcXbc&e=
> 
> Actually I agree with you on unnecessary unlock/lock here. Currently I'm 
> going to test your patch before Ack.
> Just, want to check with you - did you see any issue in driver flow 
> without this patch? or it just code cleanup?

Just a cleanup.  I was looking through dwc_otg (which is still used by the
Raspberry Pi Foundation's downstream tree) and checked to what extent the
antipatterns found there are still present in dwc2, thereby found this
occurrence.

Thanks,

Lukas

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] usb: dwc2: Drop unlock/lock upon queueing a work item
  2019-11-20 10:15 [PATCH] usb: dwc2: Drop unlock/lock upon queueing a work item Lukas Wunner
  2020-01-09 12:36 ` Lukas Wunner
@ 2020-01-15 13:11 ` Minas Harutyunyan
  2020-01-15 13:15   ` Felipe Balbi
  1 sibling, 1 reply; 10+ messages in thread
From: Minas Harutyunyan @ 2020-01-15 13:11 UTC (permalink / raw)
  To: Lukas Wunner, Felipe Balbi; +Cc: linux-usb



On 11/20/2019 2:15 PM, Lukas Wunner wrote:
> The original dwc_otg driver used a DWC_WORKQ_SCHEDULE() wrapper to queue
> work items.  Because that wrapper acquired the driver's global spinlock,
> an unlock/lock dance was necessary whenever a work item was queued up
> while the global spinlock was already held.
> 
> The dwc2 driver dropped DWC_WORKQ_SCHEDULE() in favor of a direct call
> to queue_work(), but retained the (now gratuitous) unlock/lock dance in
> dwc2_handle_conn_id_status_change_intr().  Drop it.
> 
> Signed-off-by: Lukas Wunner <lukas@wunner.de>
> ---

Acked-by: Minas Harutyunyan <hminas@synopsys.com>

>   drivers/usb/dwc2/core_intr.c | 7 +------
>   1 file changed, 1 insertion(+), 6 deletions(-)
> 
> diff --git a/drivers/usb/dwc2/core_intr.c b/drivers/usb/dwc2/core_intr.c
> index 6af6add3d4c0..876ff31261d5 100644
> --- a/drivers/usb/dwc2/core_intr.c
> +++ b/drivers/usb/dwc2/core_intr.c
> @@ -288,14 +288,9 @@ static void dwc2_handle_conn_id_status_change_intr(struct dwc2_hsotg *hsotg)
>   
>   	/*
>   	 * Need to schedule a work, as there are possible DELAY function calls.
> -	 * Release lock before scheduling workq as it holds spinlock during
> -	 * scheduling.
>   	 */
> -	if (hsotg->wq_otg) {
> -		spin_unlock(&hsotg->lock);
> +	if (hsotg->wq_otg)
>   		queue_work(hsotg->wq_otg, &hsotg->wf_otg);
> -		spin_lock(&hsotg->lock);
> -	}
>   }
>   
>   /**
> 

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] usb: dwc2: Drop unlock/lock upon queueing a work item
  2020-01-15 13:11 ` Minas Harutyunyan
@ 2020-01-15 13:15   ` Felipe Balbi
  2020-01-15 13:37     ` Greg Kroah-Hartman
  0 siblings, 1 reply; 10+ messages in thread
From: Felipe Balbi @ 2020-01-15 13:15 UTC (permalink / raw)
  To: Minas Harutyunyan, Lukas Wunner, Greg Kroah-Hartman; +Cc: linux-usb\

[-- Attachment #1: Type: text/plain, Size: 953 bytes --]


Hi Greg,

Minas Harutyunyan <Minas.Harutyunyan@synopsys.com> writes:

> On 11/20/2019 2:15 PM, Lukas Wunner wrote:
>> The original dwc_otg driver used a DWC_WORKQ_SCHEDULE() wrapper to queue
>> work items.  Because that wrapper acquired the driver's global spinlock,
>> an unlock/lock dance was necessary whenever a work item was queued up
>> while the global spinlock was already held.
>> 
>> The dwc2 driver dropped DWC_WORKQ_SCHEDULE() in favor of a direct call
>> to queue_work(), but retained the (now gratuitous) unlock/lock dance in
>> dwc2_handle_conn_id_status_change_intr().  Drop it.
>> 
>> Signed-off-by: Lukas Wunner <lukas@wunner.de>
>> ---
>
> Acked-by: Minas Harutyunyan <hminas@synopsys.com>

Do you mind picking this one up as a patch?

Acked-by: Felipe Balbi <balbi@kernel.org>

ps: if you don't have the patch anymore, I can dig it up and resend with
all appropriate acked-by tags.

cheers

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] usb: dwc2: Drop unlock/lock upon queueing a work item
  2020-01-15 13:15   ` Felipe Balbi
@ 2020-01-15 13:37     ` Greg Kroah-Hartman
  0 siblings, 0 replies; 10+ messages in thread
From: Greg Kroah-Hartman @ 2020-01-15 13:37 UTC (permalink / raw)
  To: Felipe Balbi; +Cc: Minas Harutyunyan, Lukas Wunner, linux-usb

On Wed, Jan 15, 2020 at 03:15:26PM +0200, Felipe Balbi wrote:
> 
> Hi Greg,
> 
> Minas Harutyunyan <Minas.Harutyunyan@synopsys.com> writes:
> 
> > On 11/20/2019 2:15 PM, Lukas Wunner wrote:
> >> The original dwc_otg driver used a DWC_WORKQ_SCHEDULE() wrapper to queue
> >> work items.  Because that wrapper acquired the driver's global spinlock,
> >> an unlock/lock dance was necessary whenever a work item was queued up
> >> while the global spinlock was already held.
> >> 
> >> The dwc2 driver dropped DWC_WORKQ_SCHEDULE() in favor of a direct call
> >> to queue_work(), but retained the (now gratuitous) unlock/lock dance in
> >> dwc2_handle_conn_id_status_change_intr().  Drop it.
> >> 
> >> Signed-off-by: Lukas Wunner <lukas@wunner.de>
> >> ---
> >
> > Acked-by: Minas Harutyunyan <hminas@synopsys.com>
> 
> Do you mind picking this one up as a patch?
> 
> Acked-by: Felipe Balbi <balbi@kernel.org>
> 
> ps: if you don't have the patch anymore, I can dig it up and resend with
> all appropriate acked-by tags.

I can take it, thanks.

greg k-h

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, back to index

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-11-20 10:15 [PATCH] usb: dwc2: Drop unlock/lock upon queueing a work item Lukas Wunner
2020-01-09 12:36 ` Lukas Wunner
2020-01-15  8:59   ` Felipe Balbi
2020-01-15  9:25     ` Minas Harutyunyan
2020-01-15 11:30       ` Lukas Wunner
2020-01-15 11:44         ` Minas Harutyunyan
2020-01-15 11:57           ` Lukas Wunner
2020-01-15 13:11 ` Minas Harutyunyan
2020-01-15 13:15   ` Felipe Balbi
2020-01-15 13:37     ` Greg Kroah-Hartman

Linux-USB Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-usb/0 linux-usb/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-usb linux-usb/ https://lore.kernel.org/linux-usb \
		linux-usb@vger.kernel.org
	public-inbox-index linux-usb

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-usb


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git