[PATCH] usb: dwc2: Drop unlock/lock upon queueing a work item

[PATCH] usb: dwc2: Drop unlock/lock upon queueing a work item

[Lukas Wunner]

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

Re: [PATCH] usb: dwc2: Drop unlock/lock upon queueing a work item

[Lukas Wunner]

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

Re: [PATCH] usb: dwc2: Drop unlock/lock upon queueing a work item

[Felipe Balbi]

[-- 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 --]

Re: [PATCH] usb: dwc2: Drop unlock/lock upon queueing a work item

[Minas Harutyunyan]

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

Re: [PATCH] usb: dwc2: Drop unlock/lock upon queueing a work item

[Lukas Wunner]

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

Re: [PATCH] usb: dwc2: Drop unlock/lock upon queueing a work item

[Minas Harutyunyan]

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

Re: [PATCH] usb: dwc2: Drop unlock/lock upon queueing a work item

[Lukas Wunner]

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

Re: [PATCH] usb: dwc2: Drop unlock/lock upon queueing a work item

[Minas Harutyunyan]

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);
> -	}
>   }
>   
>   /**
> 

Re: [PATCH] usb: dwc2: Drop unlock/lock upon queueing a work item

[Felipe Balbi]

[-- 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 --]

Re: [PATCH] usb: dwc2: Drop unlock/lock upon queueing a work item

[Greg Kroah-Hartman]

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