Message ID | 20170406101754.14585-1-felipe.balbi@linux.intel.com |
---|---|
State | Accepted |
Commit | 8261bd4e91e26d12c1ade9f94dae51629157c18b |
Headers | show |
Hello. On 4/6/2017 1:17 PM, Felipe Balbi wrote: > From: Roger Quadros <rogerq@ti.com> > > We must make sure that our macros are safe against expressions passed > as arguments. We have see one problem where GTXFIFOSIZ(n) was failling "Seen" and "failing". > when passed the expression (epnum >> 1) as argument. The problem was > caused by operator precedence between >> and *. > > To make sure macros are safe, we just wrap argument with () when using > it. > > Signed-off-by: Roger Quadros <rogerq@ti.com> > Signed-off-by: Felipe Balbi <felipe.balbi@linux.intel.com> > --- > > As discussed over IRC, Roger figured out what was the problem > with "Bryan's patch". It turns out that GTXFIFOSIZ() macro wasn't > safe against expression arguments. > > Roger provided a patch, which I'm now sending to linux-usb in his > regard. > > drivers/usb/dwc3/core.h | 26 +++++++++++++------------- > 1 file changed, 13 insertions(+), 13 deletions(-) > > diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h > index 459ecf3afa48..8a0fbcd99638 100644 > --- a/drivers/usb/dwc3/core.h > +++ b/drivers/usb/dwc3/core.h [...] > @@ -460,7 +460,7 @@ > #define DWC3_DEPCMD_CMD(x) ((x) & 0xf) > > /* The EP number goes 0..31 so ep0 is always out and ep1 is always in */ > -#define DWC3_DALEPENA_EP(n) BIT(n) > +#define DWC3_DALEPENA_EP(n) BIT((n)) Why do we need double parens? [...] MBR, Sergei -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
+Bryan On 06/04/17 13:17, Felipe Balbi wrote: > From: Roger Quadros <rogerq@ti.com> > > We must make sure that our macros are safe against expressions passed > as arguments. We have see one problem where GTXFIFOSIZ(n) was failling > when passed the expression (epnum >> 1) as argument. The problem was > caused by operator precedence between >> and *. > > To make sure macros are safe, we just wrap argument with () when using > it. > > Signed-off-by: Roger Quadros <rogerq@ti.com> > Signed-off-by: Felipe Balbi <felipe.balbi@linux.intel.com> > --- > > As discussed over IRC, Roger figured out what was the problem > with "Bryan's patch". It turns out that GTXFIFOSIZ() macro wasn't > safe against expression arguments. > > Roger provided a patch, which I'm now sending to linux-usb in his > regard. Thanks Felipe. cheers, -roger > > drivers/usb/dwc3/core.h | 26 +++++++++++++------------- > 1 file changed, 13 insertions(+), 13 deletions(-) > > diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h > index 459ecf3afa48..8a0fbcd99638 100644 > --- a/drivers/usb/dwc3/core.h > +++ b/drivers/usb/dwc3/core.h > @@ -117,20 +117,20 @@ > #define DWC3_VER_NUMBER 0xc1a0 > #define DWC3_VER_TYPE 0xc1a4 > > -#define DWC3_GUSB2PHYCFG(n) (0xc200 + (n * 0x04)) > -#define DWC3_GUSB2I2CCTL(n) (0xc240 + (n * 0x04)) > +#define DWC3_GUSB2PHYCFG(n) (0xc200 + ((n) * 0x04)) > +#define DWC3_GUSB2I2CCTL(n) (0xc240 + ((n) * 0x04)) > > -#define DWC3_GUSB2PHYACC(n) (0xc280 + (n * 0x04)) > +#define DWC3_GUSB2PHYACC(n) (0xc280 + ((n) * 0x04)) > > -#define DWC3_GUSB3PIPECTL(n) (0xc2c0 + (n * 0x04)) > +#define DWC3_GUSB3PIPECTL(n) (0xc2c0 + ((n) * 0x04)) > > -#define DWC3_GTXFIFOSIZ(n) (0xc300 + (n * 0x04)) > -#define DWC3_GRXFIFOSIZ(n) (0xc380 + (n * 0x04)) > +#define DWC3_GTXFIFOSIZ(n) (0xc300 + ((n) * 0x04)) > +#define DWC3_GRXFIFOSIZ(n) (0xc380 + ((n) * 0x04)) > > -#define DWC3_GEVNTADRLO(n) (0xc400 + (n * 0x10)) > -#define DWC3_GEVNTADRHI(n) (0xc404 + (n * 0x10)) > -#define DWC3_GEVNTSIZ(n) (0xc408 + (n * 0x10)) > -#define DWC3_GEVNTCOUNT(n) (0xc40c + (n * 0x10)) > +#define DWC3_GEVNTADRLO(n) (0xc400 + ((n) * 0x10)) > +#define DWC3_GEVNTADRHI(n) (0xc404 + ((n) * 0x10)) > +#define DWC3_GEVNTSIZ(n) (0xc408 + ((n) * 0x10)) > +#define DWC3_GEVNTCOUNT(n) (0xc40c + ((n) * 0x10)) > > #define DWC3_GHWPARAMS8 0xc600 > #define DWC3_GFLADJ 0xc630 > @@ -144,13 +144,13 @@ > #define DWC3_DGCMD 0xc714 > #define DWC3_DALEPENA 0xc720 > > -#define DWC3_DEP_BASE(n) (0xc800 + (n * 0x10)) > +#define DWC3_DEP_BASE(n) (0xc800 + ((n) * 0x10)) > #define DWC3_DEPCMDPAR2 0x00 > #define DWC3_DEPCMDPAR1 0x04 > #define DWC3_DEPCMDPAR0 0x08 > #define DWC3_DEPCMD 0x0c > > -#define DWC3_DEV_IMOD(n) (0xca00 + (n * 0x4)) > +#define DWC3_DEV_IMOD(n) (0xca00 + ((n) * 0x4)) > > /* OTG Registers */ > #define DWC3_OCFG 0xcc00 > @@ -460,7 +460,7 @@ > #define DWC3_DEPCMD_CMD(x) ((x) & 0xf) > > /* The EP number goes 0..31 so ep0 is always out and ep1 is always in */ > -#define DWC3_DALEPENA_EP(n) BIT(n) > +#define DWC3_DALEPENA_EP(n) BIT((n)) > > #define DWC3_DEPCMD_TYPE_CONTROL 0 > #define DWC3_DEPCMD_TYPE_ISOC 1 > -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> writes: > Hello. > > On 4/6/2017 1:17 PM, Felipe Balbi wrote: > >> From: Roger Quadros <rogerq@ti.com> >> >> We must make sure that our macros are safe against expressions passed >> as arguments. We have see one problem where GTXFIFOSIZ(n) was failling > > "Seen" and "failing". > >> when passed the expression (epnum >> 1) as argument. The problem was >> caused by operator precedence between >> and *. >> >> To make sure macros are safe, we just wrap argument with () when using >> it. >> >> Signed-off-by: Roger Quadros <rogerq@ti.com> >> Signed-off-by: Felipe Balbi <felipe.balbi@linux.intel.com> >> --- >> >> As discussed over IRC, Roger figured out what was the problem >> with "Bryan's patch". It turns out that GTXFIFOSIZ() macro wasn't >> safe against expression arguments. >> >> Roger provided a patch, which I'm now sending to linux-usb in his >> regard. >> >> drivers/usb/dwc3/core.h | 26 +++++++++++++------------- >> 1 file changed, 13 insertions(+), 13 deletions(-) >> >> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h >> index 459ecf3afa48..8a0fbcd99638 100644 >> --- a/drivers/usb/dwc3/core.h >> +++ b/drivers/usb/dwc3/core.h > [...] >> @@ -460,7 +460,7 @@ >> #define DWC3_DEPCMD_CMD(x) ((x) & 0xf) >> >> /* The EP number goes 0..31 so ep0 is always out and ep1 is always in */ >> -#define DWC3_DALEPENA_EP(n) BIT(n) >> +#define DWC3_DALEPENA_EP(n) BIT((n)) > > Why do we need double parens? I don't know (i.e. don't wanna care) how BIT() is implemented. But I still want to be able to: DWC3_DALEPENA_EP(dwc->num_eps - (dep->epnum >> 1) + whatever_number_I_want); -- balbi
On 06/04/17 13:45, Felipe Balbi wrote: > > Hi, > > Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> writes: >> Hello. >> >> On 4/6/2017 1:17 PM, Felipe Balbi wrote: >> >>> From: Roger Quadros <rogerq@ti.com> >>> >>> We must make sure that our macros are safe against expressions passed >>> as arguments. We have see one problem where GTXFIFOSIZ(n) was failling >> >> "Seen" and "failing". >> >>> when passed the expression (epnum >> 1) as argument. The problem was >>> caused by operator precedence between >> and *. >>> >>> To make sure macros are safe, we just wrap argument with () when using >>> it. >>> >>> Signed-off-by: Roger Quadros <rogerq@ti.com> >>> Signed-off-by: Felipe Balbi <felipe.balbi@linux.intel.com> >>> --- >>> >>> As discussed over IRC, Roger figured out what was the problem >>> with "Bryan's patch". It turns out that GTXFIFOSIZ() macro wasn't >>> safe against expression arguments. >>> >>> Roger provided a patch, which I'm now sending to linux-usb in his >>> regard. >>> >>> drivers/usb/dwc3/core.h | 26 +++++++++++++------------- >>> 1 file changed, 13 insertions(+), 13 deletions(-) >>> >>> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h >>> index 459ecf3afa48..8a0fbcd99638 100644 >>> --- a/drivers/usb/dwc3/core.h >>> +++ b/drivers/usb/dwc3/core.h >> [...] >>> @@ -460,7 +460,7 @@ >>> #define DWC3_DEPCMD_CMD(x) ((x) & 0xf) >>> >>> /* The EP number goes 0..31 so ep0 is always out and ep1 is always in */ >>> -#define DWC3_DALEPENA_EP(n) BIT(n) >>> +#define DWC3_DALEPENA_EP(n) BIT((n)) >> >> Why do we need double parens? > > I don't know (i.e. don't wanna care) how BIT() is implemented. But I > still want to be able to: > > DWC3_DALEPENA_EP(dwc->num_eps - (dep->epnum >> 1) + whatever_number_I_want); > Caller needs to assume that macro is sane so I'd avoid the double parens. fyi. #define BIT(nr) (1UL << (nr)) cheers, -roger -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, Roger Quadros <rogerq@ti.com> writes: > On 06/04/17 13:45, Felipe Balbi wrote: >> >> Hi, >> >> Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> writes: >>> Hello. >>> >>> On 4/6/2017 1:17 PM, Felipe Balbi wrote: >>> >>>> From: Roger Quadros <rogerq@ti.com> >>>> >>>> We must make sure that our macros are safe against expressions passed >>>> as arguments. We have see one problem where GTXFIFOSIZ(n) was failling >>> >>> "Seen" and "failing". >>> >>>> when passed the expression (epnum >> 1) as argument. The problem was >>>> caused by operator precedence between >> and *. >>>> >>>> To make sure macros are safe, we just wrap argument with () when using >>>> it. >>>> >>>> Signed-off-by: Roger Quadros <rogerq@ti.com> >>>> Signed-off-by: Felipe Balbi <felipe.balbi@linux.intel.com> >>>> --- >>>> >>>> As discussed over IRC, Roger figured out what was the problem >>>> with "Bryan's patch". It turns out that GTXFIFOSIZ() macro wasn't >>>> safe against expression arguments. >>>> >>>> Roger provided a patch, which I'm now sending to linux-usb in his >>>> regard. >>>> >>>> drivers/usb/dwc3/core.h | 26 +++++++++++++------------- >>>> 1 file changed, 13 insertions(+), 13 deletions(-) >>>> >>>> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h >>>> index 459ecf3afa48..8a0fbcd99638 100644 >>>> --- a/drivers/usb/dwc3/core.h >>>> +++ b/drivers/usb/dwc3/core.h >>> [...] >>>> @@ -460,7 +460,7 @@ >>>> #define DWC3_DEPCMD_CMD(x) ((x) & 0xf) >>>> >>>> /* The EP number goes 0..31 so ep0 is always out and ep1 is always in */ >>>> -#define DWC3_DALEPENA_EP(n) BIT(n) >>>> +#define DWC3_DALEPENA_EP(n) BIT((n)) >>> >>> Why do we need double parens? >> >> I don't know (i.e. don't wanna care) how BIT() is implemented. But I >> still want to be able to: >> >> DWC3_DALEPENA_EP(dwc->num_eps - (dep->epnum >> 1) + whatever_number_I_want); >> > > Caller needs to assume that macro is sane so I'd avoid the double parens. > > fyi. > #define BIT(nr) (1UL << (nr)) fair enough, fixed on 'next' and 'testing/next' -- balbi -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h index 459ecf3afa48..8a0fbcd99638 100644 --- a/drivers/usb/dwc3/core.h +++ b/drivers/usb/dwc3/core.h @@ -117,20 +117,20 @@ #define DWC3_VER_NUMBER 0xc1a0 #define DWC3_VER_TYPE 0xc1a4 -#define DWC3_GUSB2PHYCFG(n) (0xc200 + (n * 0x04)) -#define DWC3_GUSB2I2CCTL(n) (0xc240 + (n * 0x04)) +#define DWC3_GUSB2PHYCFG(n) (0xc200 + ((n) * 0x04)) +#define DWC3_GUSB2I2CCTL(n) (0xc240 + ((n) * 0x04)) -#define DWC3_GUSB2PHYACC(n) (0xc280 + (n * 0x04)) +#define DWC3_GUSB2PHYACC(n) (0xc280 + ((n) * 0x04)) -#define DWC3_GUSB3PIPECTL(n) (0xc2c0 + (n * 0x04)) +#define DWC3_GUSB3PIPECTL(n) (0xc2c0 + ((n) * 0x04)) -#define DWC3_GTXFIFOSIZ(n) (0xc300 + (n * 0x04)) -#define DWC3_GRXFIFOSIZ(n) (0xc380 + (n * 0x04)) +#define DWC3_GTXFIFOSIZ(n) (0xc300 + ((n) * 0x04)) +#define DWC3_GRXFIFOSIZ(n) (0xc380 + ((n) * 0x04)) -#define DWC3_GEVNTADRLO(n) (0xc400 + (n * 0x10)) -#define DWC3_GEVNTADRHI(n) (0xc404 + (n * 0x10)) -#define DWC3_GEVNTSIZ(n) (0xc408 + (n * 0x10)) -#define DWC3_GEVNTCOUNT(n) (0xc40c + (n * 0x10)) +#define DWC3_GEVNTADRLO(n) (0xc400 + ((n) * 0x10)) +#define DWC3_GEVNTADRHI(n) (0xc404 + ((n) * 0x10)) +#define DWC3_GEVNTSIZ(n) (0xc408 + ((n) * 0x10)) +#define DWC3_GEVNTCOUNT(n) (0xc40c + ((n) * 0x10)) #define DWC3_GHWPARAMS8 0xc600 #define DWC3_GFLADJ 0xc630 @@ -144,13 +144,13 @@ #define DWC3_DGCMD 0xc714 #define DWC3_DALEPENA 0xc720 -#define DWC3_DEP_BASE(n) (0xc800 + (n * 0x10)) +#define DWC3_DEP_BASE(n) (0xc800 + ((n) * 0x10)) #define DWC3_DEPCMDPAR2 0x00 #define DWC3_DEPCMDPAR1 0x04 #define DWC3_DEPCMDPAR0 0x08 #define DWC3_DEPCMD 0x0c -#define DWC3_DEV_IMOD(n) (0xca00 + (n * 0x4)) +#define DWC3_DEV_IMOD(n) (0xca00 + ((n) * 0x4)) /* OTG Registers */ #define DWC3_OCFG 0xcc00 @@ -460,7 +460,7 @@ #define DWC3_DEPCMD_CMD(x) ((x) & 0xf) /* The EP number goes 0..31 so ep0 is always out and ep1 is always in */ -#define DWC3_DALEPENA_EP(n) BIT(n) +#define DWC3_DALEPENA_EP(n) BIT((n)) #define DWC3_DEPCMD_TYPE_CONTROL 0 #define DWC3_DEPCMD_TYPE_ISOC 1