| Message ID | 77c07f00a6a9d94323c4a060a3c72817b0703b97.1574244795.git.lukas@wunner.de (mailing list archive) |
|---|---|
| State | Mainlined |
| Commit | d8bc3bf8deede6d9c32f97b6a256264609ce2baa |
| Headers | show |
| Series | usb: dwc2: Drop unlock/lock upon queueing a work item | expand |
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
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.
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
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
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
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
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); > - } > } > > /** >
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
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
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); - } } /**
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(-)