usb: gadget: f_uac1: Convert use of __constant_cpu_to_le16 to cpu_to_le16

PATCH In review Vaishali Thakkar 3 participants 7 messages Aug 19, 2015 – Oct 6, 2015 USB

Share & tools

Vaishali Thakkar

author

2015-08-19 05:31 UTC #0 lore raw

PATCH 1/1 usb: gadget: f_uac1: Convert use of __constant_cpu_to_le16 to cpu_to_le16

In big endian cases, macro cpu_to_le16 unfolds to __swab16 which provides special case for constants. In little endian cases, __constant_cpu_to_le16 and cpu_to_le16 expand directly to the same expression. So, replace __constant_cpu_to_le16 with cpu_to_le16 with the goal of getting rid of the definition of __constant_cpu_to_le16 completely.

The semantic patch that performs this transformation is as follows:

@@expression x;@@

- __constant_cpu_to_le16(x)
+ cpu_to_le16(x)

Signed-off-by Vaishali Thakkar <[email protected]>

---
 drivers/usb/gadget/function/f_uac1.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

drivers/usb/gadget/function/f_uac1.c +3 -3

index 7856b33..5aa8d8a 100644
@@ -57,8 +57,8 @@ static struct uac1_ac_header_descriptor_1 ac_header_desc = {
 	.bLength =		UAC_DT_AC_HEADER_LENGTH,
 	.bDescriptorType =	USB_DT_CS_INTERFACE,
 	.bDescriptorSubtype =	UAC_HEADER,
-	.bcdADC =		__constant_cpu_to_le16(0x0100),
-	.wTotalLength =		__constant_cpu_to_le16(UAC_DT_TOTAL_LENGTH),
+	.bcdADC =		cpu_to_le16(0x0100),
+	.wTotalLength =		cpu_to_le16(UAC_DT_TOTAL_LENGTH),
 	.bInCollection =	F_AUDIO_NUM_INTERFACES,
 	.baInterfaceNr = {
 	/* Interface number of the first AudioStream interface */

@@ -186,7 +186,7 @@ static struct uac_iso_endpoint_descriptor as_iso_out_desc = {
 	.bDescriptorSubtype =	UAC_EP_GENERAL,
 	.bmAttributes = 	1,
 	.bLockDelayUnits =	1,
-	.wLockDelay =		__constant_cpu_to_le16(1),
+	.wLockDelay =		cpu_to_le16(1),
 };
 
 static struct usb_descriptor_header *f_audio_desc[] = {

Signature · 1 line

1.9.1

David Laight

2015-08-20 10:53 UTC #1 lore raw

From: Vaishali Thakkar

Quoted 29 lines

Sent: 19 August 2015 06:31
To: Felipe Balbi
Cc: Greg Kroah-Hartman; [email protected]; [email protected]
Subject: [PATCH] usb: gadget: f_uac1: Convert use of __constant_cpu_to_le16 to cpu_to_le16
In big endian cases, macro cpu_to_le16 unfolds to __swab16 which provides special case for constants. In little endian cases, __constant_cpu_to_le16 and cpu_to_le16 expand directly to the same expression. So, replace __constant_cpu_to_le16 with cpu_to_le16 with the goal of getting rid of the definition of __constant_cpu_to_le16 completely.
The semantic patch that performs this transformation is as follows:
@@expression x;@@
- __constant_cpu_to_le16(x)
+ cpu_to_le16(x)
Signed-off-by Vaishali Thakkar <[email protected]>
---
 drivers/usb/gadget/function/f_uac1.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
drivers/usb/gadget/function/f_uac1.c +0 -0
index 7856b33..5aa8d8a 100644

drivers/usb/gadget/function/f_uac1.c static struct uac1_ac_header_descriptor_1 ac_header_desc = { · line 61

Quoted 7 lines

drivers/usb/gadget/function/f_uac1.c +2 -2

@@ -57,8 +57,8 @@ static struct uac1_ac_header_descriptor_1 ac_header_desc = {
 	.bLength =		UAC_DT_AC_HEADER_LENGTH,
 	.bDescriptorType =	USB_DT_CS_INTERFACE,
 	.bDescriptorSubtype =	UAC_HEADER,
-	.bcdADC =		__constant_cpu_to_le16(0x0100),
-	.wTotalLength =		__constant_cpu_to_le16(UAC_DT_TOTAL_LENGTH),
+	.bcdADC =		cpu_to_le16(0x0100),
+	.wTotalLength =		cpu_to_le16(UAC_DT_TOTAL_LENGTH),

Have you test compiled this on a big-endian system? My gut feeling is that is fails.

David

Vaishali Thakkar

author

2015-08-22 01:57 UTC #2 lore raw

Quoted 25 lines · David Laight

From: Vaishali Thakkar
Quoted 24 lines
Sent: 19 August 2015 06:31
To: Felipe Balbi
Cc: Greg Kroah-Hartman; [email protected]; [email protected]
Subject: [PATCH] usb: gadget: f_uac1: Convert use of __constant_cpu_to_le16 to cpu_to_le16
In big endian cases, macro cpu_to_le16 unfolds to __swab16 which provides special case for constants. In little endian cases, __constant_cpu_to_le16 and cpu_to_le16 expand directly to the same expression. So, replace __constant_cpu_to_le16 with cpu_to_le16 with the goal of getting rid of the definition of __constant_cpu_to_le16 completely.
The semantic patch that performs this transformation is as follows:
@@expression x;@@
- __constant_cpu_to_le16(x)
+ cpu_to_le16(x)
Signed-off-by Vaishali Thakkar <[email protected]>
---
 drivers/usb/gadget/function/f_uac1.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Quoted 15 lines

drivers/usb/gadget/function/f_uac1.c static struct uac1_ac_header_descriptor_1 ac_header_desc = { · line 61
Quoted 12 lines
drivers/usb/gadget/function/f_uac1.c +2 -2
index 7856b33..5aa8d8a 100644
@@ -57,8 +57,8 @@ static struct uac1_ac_header_descriptor_1 ac_header_desc = {
      .bLength =              UAC_DT_AC_HEADER_LENGTH,
      .bDescriptorType =      USB_DT_CS_INTERFACE,
      .bDescriptorSubtype =   UAC_HEADER,
-     .bcdADC =               __constant_cpu_to_le16(0x0100),
-     .wTotalLength =         __constant_cpu_to_le16(UAC_DT_TOTAL_LENGTH),
+     .bcdADC =               cpu_to_le16(0x0100),
+     .wTotalLength =         cpu_to_le16(UAC_DT_TOTAL_LENGTH),
Have you test compiled this on a big-endian system? My gut feeling is that is fails.

No. I have tested it on little-endian system only. But I'll be really surprised if this will fail. Can you please tell me if I am missing something in this particular case or same applies for other cases because most of the cases like __constant_<foo> are already converted to <foo>?

As far as I know, if the argument is a constant the conversion happens at compile time. And unfolding both definitions returns to same expression. Still I am trying if someone can test it for me on big endian system.

Quoted 2 lines

David

Signature · 1 line

Vaishali

David Laight

2015-08-24 09:01 UTC #3 lore raw

From: Vaishali Thakkar [mailto:[email protected]]

Quoted 1 line

Sent: 22 August 2015 02:57

...

Quoted 14 lines

Quoted 7 lines
drivers/usb/gadget/function/f_uac1.c static struct uac1_ac_header_descriptor_1 ac_header_desc = { · line 61
Quoted 4 lines
drivers/usb/gadget/function/f_uac1.c +2 -2
-     .bcdADC =               __constant_cpu_to_le16(0x0100),
-     .wTotalLength =         __constant_cpu_to_le16(UAC_DT_TOTAL_LENGTH),
+     .bcdADC =               cpu_to_le16(0x0100),
+     .wTotalLength =         cpu_to_le16(UAC_DT_TOTAL_LENGTH),
Have you test compiled this on a big-endian system? My gut feeling is that is fails.
No. I have tested it on little-endian system only. But I'll be really surprised if this will fail. Can you please tell me if I am missing something in this particular case or same applies for other cases because most of the cases like __constant_<foo> are already converted to <foo>?

Quoted 4 lines

As far as I know, if the argument is a constant the conversion happens at compile time. And unfolding both definitions returns to same expression. Still I am trying if someone can test it for me on big endian system.

Flip one to cpu_to_be16() and see if it still compiles.

Static initialisers and case labels can be expressions, but the expression itself must only contain constants.
So it needs to be constant regardless of the value of any constants. If it contains 'a ? t : f' then both 't' and 'f' must be constant.

In code, if 'a' is constant the optimiser discards one of 't' or 'f'. I'm not sure what happens for non-static initialisers (they generate odd code at the best of times).

David

��{.n�+��+%��ݶ��w��{.n�+��{��G��{ay�ʇڙ�,j��f��h��z_�(�階�ݢj"��m��G��?��&��~��iO��z��v�^�m��?�I�

Vaishali Thakkar

author

2015-08-24 10:42 UTC #4 lore raw

Quoted 17 lines · David Laight

From: Vaishali Thakkar [mailto:[email protected]]
Quoted 1 line
Sent: 22 August 2015 02:57
...
Quoted 14 lines
Quoted 7 lines
drivers/usb/gadget/function/f_uac1.c static struct uac1_ac_header_descriptor_1 ac_header_desc = { · line 61
Quoted 4 lines
drivers/usb/gadget/function/f_uac1.c +2 -2
-     .bcdADC =               __constant_cpu_to_le16(0x0100),
-     .wTotalLength =         __constant_cpu_to_le16(UAC_DT_TOTAL_LENGTH),
+     .bcdADC =               cpu_to_le16(0x0100),
+     .wTotalLength =         cpu_to_le16(UAC_DT_TOTAL_LENGTH),
Have you test compiled this on a big-endian system? My gut feeling is that is fails.
No. I have tested it on little-endian system only. But I'll be really surprised if this will fail. Can you please tell me if I am missing something in this particular case or same applies for other cases because most of the cases like __constant_<foo> are already converted to <foo>?

Quoted 6 lines

Quoted 4 lines
As far as I know, if the argument is a constant the conversion happens at compile time. And unfolding both definitions returns to same expression. Still I am trying if someone can test it for me on big endian system.
Flip one to cpu_to_be16() and see if it still compiles.

Yes. It still compiles.

Quoted 11 lines

Static initialisers and case labels can be expressions, but the expression itself must only contain constants.
So it needs to be constant regardless of the value of any constants. If it contains 'a ? t : f' then both 't' and 'f' must be constant.
In code, if 'a' is constant the optimiser discards one of 't' or 'f'. I'm not sure what happens for non-static initialisers (they generate odd code at the best of times).
David

Signature · 1 line

Vaishali

Felipe Balbi

2015-10-05 23:29 UTC #5 lore raw

Quoted 18 lines · Vaishali Thakkar

Quoted 17 lines · David Laight
From: Vaishali Thakkar [mailto:[email protected]]
Quoted 1 line
Sent: 22 August 2015 02:57
...
Quoted 14 lines
Quoted 7 lines
drivers/usb/gadget/function/f_uac1.c static struct uac1_ac_header_descriptor_1 ac_header_desc = { · line 61
Quoted 4 lines
drivers/usb/gadget/function/f_uac1.c +2 -2
-     .bcdADC =               __constant_cpu_to_le16(0x0100),
-     .wTotalLength =         __constant_cpu_to_le16(UAC_DT_TOTAL_LENGTH),
+     .bcdADC =               cpu_to_le16(0x0100),
+     .wTotalLength =         cpu_to_le16(UAC_DT_TOTAL_LENGTH),
Have you test compiled this on a big-endian system? My gut feeling is that is fails.
No. I have tested it on little-endian system only. But I'll be really surprised if this will fail. Can you please tell me if I am missing something in this particular case or same applies for other cases because most of the cases like __constant_<foo> are already converted to <foo>?

Quoted 8 lines

Quoted 6 lines
Quoted 4 lines
As far as I know, if the argument is a constant the conversion happens at compile time. And unfolding both definitions returns to same expression. Still I am trying if someone can test it for me on big endian system.
Flip one to cpu_to_be16() and see if it still compiles.
Yes. It still compiles.

it's unclear to me if this is really safe to apply. Until then I'm dropping this from queue. Seems like, at a minimum, we need a better commit log

Signature · 1 line

balbi

Vaishali Thakkar

author

2015-10-06 01:38 UTC #6 lore raw

Quoted 25 lines · Felipe Balbi

Quoted 23 lines · Vaishali Thakkar
Quoted 22 lines · David Laight
From: Vaishali Thakkar [mailto:[email protected]]
Quoted 1 line
Sent: 22 August 2015 02:57
...
Quoted 18 lines
Quoted 7 lines
drivers/usb/gadget/function/f_uac1.c static struct uac1_ac_header_descriptor_1 ac_header_desc = { · line 61
Quoted 4 lines
drivers/usb/gadget/function/f_uac1.c +2 -2
-     .bcdADC =               __constant_cpu_to_le16(0x0100),
-     .wTotalLength =         __constant_cpu_to_le16(UAC_DT_TOTAL_LENGTH),
+     .bcdADC =               cpu_to_le16(0x0100),
+     .wTotalLength =         cpu_to_le16(UAC_DT_TOTAL_LENGTH),
Have you test compiled this on a big-endian system? My gut feeling is that is fails.
No. I have tested it on little-endian system only. But I'll be really surprised if this will fail. Can you please tell me if I am missing something in this particular case or same applies for other cases because most of the cases like __constant_<foo> are already converted to <foo>?
As far as I know, if the argument is a constant the conversion happens at compile time. And unfolding both definitions returns to same expression. Still I am trying if someone can test it for me on big endian system.

Quoted 7 lines

Quoted 3 lines
Quoted 1 line
Flip one to cpu_to_be16() and see if it still compiles.
Yes. It still compiles.
it's unclear to me if this is really safe to apply. Until then I'm dropping this from queue. Seems like, at a minimum, we need a better commit log

I compared both .s files [before the change and after the change] and I don't see any difference between instructions in them. I am not sure but it looks like if expression contains 'a ? t : f' then either 't' OR 'f' should
be constant not both.

Also, the code still complies on little endian machines after changing cpu_to_be16(). And on top of that fact is there are many patches applied in the kernel from years for the same and very less are remaining. In case, this fails then I think we need to change those cases as well. But I have never seen people reporting a bug for the same and changing <foo> to __constant_<foo> again.

Still I think probably it would be good if someone can test this patch on big endian machine [just to be sure about the change].

Quoted 2 lines

--
balbi

Signature · 1 line

Vaishali