diff mbox

usb: dwc3: make macros safe to expression arguments

Message ID 20170406101754.14585-1-felipe.balbi@linux.intel.com
State Accepted
Commit 8261bd4e91e26d12c1ade9f94dae51629157c18b
Headers show

Commit Message

Felipe Balbi April 6, 2017, 10:17 a.m. UTC 
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.

 drivers/usb/dwc3/core.h | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)

-- 
2.11.0.295.gd7dffce1ce

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

Comments

Sergei Shtylyov April 6, 2017, 10:24 a.m. UTC | #1 
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
Roger Quadros April 6, 2017, 10:26 a.m. UTC | #2 
+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
Felipe Balbi April 6, 2017, 10:45 a.m. UTC | #3 
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
Roger Quadros April 6, 2017, 11:21 a.m. UTC | #4 
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
Felipe Balbi April 6, 2017, 11:26 a.m. UTC | #5 
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 mbox

Patch

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