| Message ID | 20200702144625.2533530-12-lee.jones@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fix a bunch of W=1 issues in USB | expand |
On Thu, Jul 02, 2020 at 03:46:06PM +0100, Lee Jones wrote: > Commit ec1f9d9f01384 ("usb: dwc2: gadget: parity fix in isochronous mode") moved > these checks to dwc2_hsotg_change_ep_iso_parity() back in 2015. The assigned > value hasn't been read back since. Let's remove the unnecessary H/W read. > > Fixes the following W=1 warning: > > drivers/usb/dwc2/gadget.c: In function ‘dwc2_hsotg_epint’: > drivers/usb/dwc2/gadget.c:2981:6: warning: variable ‘ctrl’ set but not used [-Wunused-but-set-variable] > 2981 | u32 ctrl; > | ^~~~ > > Cc: Minas Harutyunyan <hminas@synopsys.com> > Cc: Ben Dooks <ben@simtec.co.uk> > Signed-off-by: Lee Jones <lee.jones@linaro.org> > --- > drivers/usb/dwc2/gadget.c | 2 -- > 1 file changed, 2 deletions(-) > > diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c > index 116e6175c7a48..fa07e3fcb8841 100644 > --- a/drivers/usb/dwc2/gadget.c > +++ b/drivers/usb/dwc2/gadget.c > @@ -2975,10 +2975,8 @@ static void dwc2_hsotg_epint(struct dwc2_hsotg *hsotg, unsigned int idx, > u32 epctl_reg = dir_in ? DIEPCTL(idx) : DOEPCTL(idx); > u32 epsiz_reg = dir_in ? DIEPTSIZ(idx) : DOEPTSIZ(idx); > u32 ints; > - u32 ctrl; > > ints = dwc2_gadget_read_ep_interrupts(hsotg, idx, dir_in); > - ctrl = dwc2_readl(hsotg, epctl_reg); As you know, lots of hardware requires reads to happen to do things, so are you sure it is safe to remove this read call? thanks, greg k-h
Hi, On 7/3/2020 11:29 AM, Greg KH wrote: > On Thu, Jul 02, 2020 at 03:46:06PM +0100, Lee Jones wrote: >> Commit ec1f9d9f01384 ("usb: dwc2: gadget: parity fix in isochronous mode") moved >> these checks to dwc2_hsotg_change_ep_iso_parity() back in 2015. The assigned >> value hasn't been read back since. Let's remove the unnecessary H/W read. >> >> Fixes the following W=1 warning: >> >> drivers/usb/dwc2/gadget.c: In function ‘dwc2_hsotg_epint’: >> drivers/usb/dwc2/gadget.c:2981:6: warning: variable ‘ctrl’ set but not used [-Wunused-but-set-variable] >> 2981 | u32 ctrl; >> | ^~~~ >> >> Cc: Minas Harutyunyan <hminas@synopsys.com> >> Cc: Ben Dooks <ben@simtec.co.uk> >> Signed-off-by: Lee Jones <lee.jones@linaro.org> >> --- >> drivers/usb/dwc2/gadget.c | 2 -- >> 1 file changed, 2 deletions(-) >> >> diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c >> index 116e6175c7a48..fa07e3fcb8841 100644 >> --- a/drivers/usb/dwc2/gadget.c >> +++ b/drivers/usb/dwc2/gadget.c >> @@ -2975,10 +2975,8 @@ static void dwc2_hsotg_epint(struct dwc2_hsotg *hsotg, unsigned int idx, >> u32 epctl_reg = dir_in ? DIEPCTL(idx) : DOEPCTL(idx); >> u32 epsiz_reg = dir_in ? DIEPTSIZ(idx) : DOEPTSIZ(idx); >> u32 ints; >> - u32 ctrl; >> >> ints = dwc2_gadget_read_ep_interrupts(hsotg, idx, dir_in); >> - ctrl = dwc2_readl(hsotg, epctl_reg); > > As you know, lots of hardware requires reads to happen to do things, so > are you sure it is safe to remove this read call? > Greg, yes, it's Ok to remove this unnecessary read which remained from previous implementations. Lee, please add "Fixes:" tag and resubmit v2. Thanks, Minas > thanks, > > greg k-h >
On Fri, Jul 03, 2020 at 07:38:16AM +0000, Minas Harutyunyan wrote: > Hi, > > On 7/3/2020 11:29 AM, Greg KH wrote: > > On Thu, Jul 02, 2020 at 03:46:06PM +0100, Lee Jones wrote: > >> Commit ec1f9d9f01384 ("usb: dwc2: gadget: parity fix in isochronous mode") moved > >> these checks to dwc2_hsotg_change_ep_iso_parity() back in 2015. The assigned > >> value hasn't been read back since. Let's remove the unnecessary H/W read. > >> > >> Fixes the following W=1 warning: > >> > >> drivers/usb/dwc2/gadget.c: In function ‘dwc2_hsotg_epint’: > >> drivers/usb/dwc2/gadget.c:2981:6: warning: variable ‘ctrl’ set but not used [-Wunused-but-set-variable] > >> 2981 | u32 ctrl; > >> | ^~~~ > >> > >> Cc: Minas Harutyunyan <hminas@synopsys.com> > >> Cc: Ben Dooks <ben@simtec.co.uk> > >> Signed-off-by: Lee Jones <lee.jones@linaro.org> > >> --- > >> drivers/usb/dwc2/gadget.c | 2 -- > >> 1 file changed, 2 deletions(-) > >> > >> diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c > >> index 116e6175c7a48..fa07e3fcb8841 100644 > >> --- a/drivers/usb/dwc2/gadget.c > >> +++ b/drivers/usb/dwc2/gadget.c > >> @@ -2975,10 +2975,8 @@ static void dwc2_hsotg_epint(struct dwc2_hsotg *hsotg, unsigned int idx, > >> u32 epctl_reg = dir_in ? DIEPCTL(idx) : DOEPCTL(idx); > >> u32 epsiz_reg = dir_in ? DIEPTSIZ(idx) : DOEPTSIZ(idx); > >> u32 ints; > >> - u32 ctrl; > >> > >> ints = dwc2_gadget_read_ep_interrupts(hsotg, idx, dir_in); > >> - ctrl = dwc2_readl(hsotg, epctl_reg); > > > > As you know, lots of hardware requires reads to happen to do things, so > > are you sure it is safe to remove this read call? > > > > Greg, yes, it's Ok to remove this unnecessary read which remained from > previous implementations. Great, thanks for confirming! greg k-h
Hi, On 7/3/2020 11:43 AM, Greg KH wrote: > On Fri, Jul 03, 2020 at 07:38:16AM +0000, Minas Harutyunyan wrote: >> Hi, >> >> On 7/3/2020 11:29 AM, Greg KH wrote: >>> On Thu, Jul 02, 2020 at 03:46:06PM +0100, Lee Jones wrote: >>>> Commit ec1f9d9f01384 ("usb: dwc2: gadget: parity fix in isochronous mode") moved >>>> these checks to dwc2_hsotg_change_ep_iso_parity() back in 2015. The assigned >>>> value hasn't been read back since. Let's remove the unnecessary H/W read. >>>> >>>> Fixes the following W=1 warning: >>>> >>>> drivers/usb/dwc2/gadget.c: In function ‘dwc2_hsotg_epint’: >>>> drivers/usb/dwc2/gadget.c:2981:6: warning: variable ‘ctrl’ set but not used [-Wunused-but-set-variable] >>>> 2981 | u32 ctrl; >>>> | ^~~~ >>>> >>>> Cc: Minas Harutyunyan <hminas@synopsys.com> >>>> Cc: Ben Dooks <ben@simtec.co.uk> >>>> Signed-off-by: Lee Jones <lee.jones@linaro.org> >>>> --- >>>> drivers/usb/dwc2/gadget.c | 2 -- >>>> 1 file changed, 2 deletions(-) >>>> >>>> diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c >>>> index 116e6175c7a48..fa07e3fcb8841 100644 >>>> --- a/drivers/usb/dwc2/gadget.c >>>> +++ b/drivers/usb/dwc2/gadget.c >>>> @@ -2975,10 +2975,8 @@ static void dwc2_hsotg_epint(struct dwc2_hsotg *hsotg, unsigned int idx, >>>> u32 epctl_reg = dir_in ? DIEPCTL(idx) : DOEPCTL(idx); >>>> u32 epsiz_reg = dir_in ? DIEPTSIZ(idx) : DOEPTSIZ(idx); >>>> u32 ints; >>>> - u32 ctrl; >>>> >>>> ints = dwc2_gadget_read_ep_interrupts(hsotg, idx, dir_in); >>>> - ctrl = dwc2_readl(hsotg, epctl_reg); >>> >>> As you know, lots of hardware requires reads to happen to do things, so >>> are you sure it is safe to remove this read call? >>> >> >> Greg, yes, it's Ok to remove this unnecessary read which remained from >> previous implementations. > > Great, thanks for confirming! > Acked-by: Minas Harutyunyan <hminas@synopsys.com> > greg k-h >
On Fri, 03 Jul 2020, Minas Harutyunyan wrote: > Hi, > > On 7/3/2020 11:29 AM, Greg KH wrote: > > On Thu, Jul 02, 2020 at 03:46:06PM +0100, Lee Jones wrote: > >> Commit ec1f9d9f01384 ("usb: dwc2: gadget: parity fix in isochronous mode") moved > >> these checks to dwc2_hsotg_change_ep_iso_parity() back in 2015. The assigned > >> value hasn't been read back since. Let's remove the unnecessary H/W read. > >> > >> Fixes the following W=1 warning: > >> > >> drivers/usb/dwc2/gadget.c: In function ‘dwc2_hsotg_epint’: > >> drivers/usb/dwc2/gadget.c:2981:6: warning: variable ‘ctrl’ set but not used [-Wunused-but-set-variable] > >> 2981 | u32 ctrl; > >> | ^~~~ > >> > >> Cc: Minas Harutyunyan <hminas@synopsys.com> > >> Cc: Ben Dooks <ben@simtec.co.uk> > >> Signed-off-by: Lee Jones <lee.jones@linaro.org> > >> --- > >> drivers/usb/dwc2/gadget.c | 2 -- > >> 1 file changed, 2 deletions(-) > >> > >> diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c > >> index 116e6175c7a48..fa07e3fcb8841 100644 > >> --- a/drivers/usb/dwc2/gadget.c > >> +++ b/drivers/usb/dwc2/gadget.c > >> @@ -2975,10 +2975,8 @@ static void dwc2_hsotg_epint(struct dwc2_hsotg *hsotg, unsigned int idx, > >> u32 epctl_reg = dir_in ? DIEPCTL(idx) : DOEPCTL(idx); > >> u32 epsiz_reg = dir_in ? DIEPTSIZ(idx) : DOEPTSIZ(idx); > >> u32 ints; > >> - u32 ctrl; > >> > >> ints = dwc2_gadget_read_ep_interrupts(hsotg, idx, dir_in); > >> - ctrl = dwc2_readl(hsotg, epctl_reg); > > > > As you know, lots of hardware requires reads to happen to do things, so > > are you sure it is safe to remove this read call? > > > > Greg, yes, it's Ok to remove this unnecessary read which remained from > previous implementations. > > Lee, please add "Fixes:" tag and resubmit v2. Is this suitable for Stable Greg?
On Fri, Jul 03, 2020 at 06:16:26PM +0100, Lee Jones wrote: > On Fri, 03 Jul 2020, Minas Harutyunyan wrote: > > > Hi, > > > > On 7/3/2020 11:29 AM, Greg KH wrote: > > > On Thu, Jul 02, 2020 at 03:46:06PM +0100, Lee Jones wrote: > > >> Commit ec1f9d9f01384 ("usb: dwc2: gadget: parity fix in isochronous mode") moved > > >> these checks to dwc2_hsotg_change_ep_iso_parity() back in 2015. The assigned > > >> value hasn't been read back since. Let's remove the unnecessary H/W read. > > >> > > >> Fixes the following W=1 warning: > > >> > > >> drivers/usb/dwc2/gadget.c: In function ‘dwc2_hsotg_epint’: > > >> drivers/usb/dwc2/gadget.c:2981:6: warning: variable ‘ctrl’ set but not used [-Wunused-but-set-variable] > > >> 2981 | u32 ctrl; > > >> | ^~~~ > > >> > > >> Cc: Minas Harutyunyan <hminas@synopsys.com> > > >> Cc: Ben Dooks <ben@simtec.co.uk> > > >> Signed-off-by: Lee Jones <lee.jones@linaro.org> > > >> --- > > >> drivers/usb/dwc2/gadget.c | 2 -- > > >> 1 file changed, 2 deletions(-) > > >> > > >> diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c > > >> index 116e6175c7a48..fa07e3fcb8841 100644 > > >> --- a/drivers/usb/dwc2/gadget.c > > >> +++ b/drivers/usb/dwc2/gadget.c > > >> @@ -2975,10 +2975,8 @@ static void dwc2_hsotg_epint(struct dwc2_hsotg *hsotg, unsigned int idx, > > >> u32 epctl_reg = dir_in ? DIEPCTL(idx) : DOEPCTL(idx); > > >> u32 epsiz_reg = dir_in ? DIEPTSIZ(idx) : DOEPTSIZ(idx); > > >> u32 ints; > > >> - u32 ctrl; > > >> > > >> ints = dwc2_gadget_read_ep_interrupts(hsotg, idx, dir_in); > > >> - ctrl = dwc2_readl(hsotg, epctl_reg); > > > > > > As you know, lots of hardware requires reads to happen to do things, so > > > are you sure it is safe to remove this read call? > > > > > > > Greg, yes, it's Ok to remove this unnecessary read which remained from > > previous implementations. > > > > Lee, please add "Fixes:" tag and resubmit v2. > > Is this suitable for Stable Greg? Does it fix something that people have ever noticed or care about? Given the age of the bug, I doubt it :) thanks, greg k-h
diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c index 116e6175c7a48..fa07e3fcb8841 100644 --- a/drivers/usb/dwc2/gadget.c +++ b/drivers/usb/dwc2/gadget.c @@ -2975,10 +2975,8 @@ static void dwc2_hsotg_epint(struct dwc2_hsotg *hsotg, unsigned int idx, u32 epctl_reg = dir_in ? DIEPCTL(idx) : DOEPCTL(idx); u32 epsiz_reg = dir_in ? DIEPTSIZ(idx) : DOEPTSIZ(idx); u32 ints; - u32 ctrl; ints = dwc2_gadget_read_ep_interrupts(hsotg, idx, dir_in); - ctrl = dwc2_readl(hsotg, epctl_reg); /* Clear endpoint interrupts */ dwc2_writel(hsotg, ints, epint_reg);
Commit ec1f9d9f01384 ("usb: dwc2: gadget: parity fix in isochronous mode") moved these checks to dwc2_hsotg_change_ep_iso_parity() back in 2015. The assigned value hasn't been read back since. Let's remove the unnecessary H/W read. Fixes the following W=1 warning: drivers/usb/dwc2/gadget.c: In function ‘dwc2_hsotg_epint’: drivers/usb/dwc2/gadget.c:2981:6: warning: variable ‘ctrl’ set but not used [-Wunused-but-set-variable] 2981 | u32 ctrl; | ^~~~ Cc: Minas Harutyunyan <hminas@synopsys.com> Cc: Ben Dooks <ben@simtec.co.uk> Signed-off-by: Lee Jones <lee.jones@linaro.org> --- drivers/usb/dwc2/gadget.c | 2 -- 1 file changed, 2 deletions(-)