Skip to contentSkip to conversation

dvb: usb vendor_ids/product_ids are __le16

PATCH In review Harvey Harrison 6 participants 11 messages Nov 14, 2008 – Nov 15, 2008 Media (V4L/DVB)

lore
Share & tools
lore

Harvey Harrison

author
#0 lore raw

PATCH 1/1 dvb: usb vendor_ids/product_ids are __le16

Noticed by sparse:
drivers/media/dvb/dvb-usb/af9015.c:756:25: warning: restricted __le16 degrades to integer drivers/media/dvb/dvb-usb/af9015.c:744:28: warning: restricted __le16 degrades to integer

Signed-off-by Harvey Harrison <[email protected]>
---
 drivers/media/dvb/dvb-usb/af9015.c |   12 ++++++------
 1 files changed, 6 insertions(+), 6 deletions(-)
drivers/media/dvb/dvb-usb/af9015.c +6 -6
index 4b2af32..eb4495a 100644
@@ -742,7 +742,7 @@ static int af9015_read_config(struct usb_device *udev)
 			}
 		} else {
 			switch (udev->descriptor.idVendor) {
-			case USB_VID_LEADTEK:
+			case cpu_to_le16(USB_VID_LEADTEK):
 				af9015_properties[i].rc_key_map =
 				  af9015_rc_keys_leadtek;
 				af9015_properties[i].rc_key_map_size =

@@ -752,9 +752,9 @@ static int af9015_read_config(struct usb_device *udev)
 				af9015_config.ir_table_size =
 				  ARRAY_SIZE(af9015_ir_table_leadtek);
 				break;
-			case USB_VID_VISIONPLUS:
+			case cpu_to_le16(USB_VID_VISIONPLUS):
 				if (udev->descriptor.idProduct ==
-				USB_PID_AZUREWAVE_AD_TU700) {
+				cpu_to_le16(USB_PID_AZUREWAVE_AD_TU700)) {
 					af9015_properties[i].rc_key_map =
 					  af9015_rc_keys_twinhan;
 					af9015_properties[i].rc_key_map_size =

@@ -765,7 +765,7 @@ static int af9015_read_config(struct usb_device *udev)
 					  ARRAY_SIZE(af9015_ir_table_twinhan);
 				}
 				break;
-			case USB_VID_KWORLD_2:
+			case cpu_to_le16(USB_VID_KWORLD_2):
 				/* TODO: use correct rc keys */
 				af9015_properties[i].rc_key_map =
 				  af9015_rc_keys_twinhan;

@@ -778,7 +778,7 @@ static int af9015_read_config(struct usb_device *udev)
 			/* Check USB manufacturer and product strings and try
 			   to determine correct remote in case of chip vendor
 			   reference IDs are used. */
-			case USB_VID_AFATECH:
+			case cpu_to_le16(USB_VID_AFATECH):
 				memset(manufacturer, 0, sizeof(manufacturer));
 				usb_string(udev, udev->descriptor.iManufacturer,
 					manufacturer, sizeof(manufacturer));

@@ -806,7 +806,7 @@ static int af9015_read_config(struct usb_device *udev)
 					  ARRAY_SIZE(af9015_ir_table_msi);
 				}
 				break;
-			case USB_VID_AVERMEDIA:
+			case cpu_to_le16(USB_VID_AVERMEDIA):
 				af9015_properties[i].rc_key_map =
 				  af9015_rc_keys_avermedia;
 				af9015_properties[i].rc_key_map_size =
Signature · 1 line
1.6.0.4.879.g9d19a
#1 lore raw

Harvey, et al:

Quoted 15 lines · Harvey Harrison

Noticed by sparse:
drivers/media/dvb/dvb-usb/af9015.c:756:25: warning: restricted __le16 degrades to integer drivers/media/dvb/dvb-usb/af9015.c:744:28: warning: restricted __le16 degrades to integer

Signed-off-by Harvey Harrison <[email protected]>
---
 drivers/media/dvb/dvb-usb/af9015.c |   12 ++++++------
 1 files changed, 6 insertions(+), 6 deletions(-)
drivers/media/dvb/dvb-usb/af9015.c +0 -0
index 4b2af32..eb4495a 100644

drivers/media/dvb/dvb-usb/af9015.c static int af9015_read_config(struct usb_device *udev) · line 748

Quoted 8 lines
drivers/media/dvb/dvb-usb/af9015.c +1 -1
@@ -742,7 +742,7 @@ static int af9015_read_config(struct usb_device *udev)
 			}
 		} else {
 			switch (udev->descriptor.idVendor) {
-			case USB_VID_LEADTEK:
+			case cpu_to_le16(USB_VID_LEADTEK):
 				af9015_properties[i].rc_key_map =
 				  af9015_rc_keys_leadtek;
 				af9015_properties[i].rc_key_map_size =

Wouldn't it be nicer to just switch on cpu_to_le16(udev->descriptor.idVendor) ? This would be a 1-line change, compile to a smaller footprint, and be easier to read.

Personally, I prefer to try to avoid duplicating code in places where a single operation may occur centrally.

Regards,

Mike

author
#2 lore raw
Quoted 6 lines · Michael Krufky

Wouldn't it be nicer to just switch on cpu_to_le16(udev->descriptor.idVendor) ? This would be a 1-line change, compile to a smaller footprint, and be easier to read.

Personally, I prefer to try to avoid duplicating code in places where a single operation may occur centrally.

On a little-endian arch it makes no difference obviously, but on a big-endian arch it's the difference between compile-time and runtime byteswapping.

Harvey

#3 lore raw
Quoted 5 lines · Harvey Harrison
Quoted 4 lines · Michael Krufky

Wouldn't it be nicer to just switch on cpu_to_le16(udev->descriptor.idVendor) ? This would be a 1-line change, compile to a smaller footprint, and be easier to read.

Quoted 7 lines
Quoted 2 lines

Personally, I prefer to try to avoid duplicating code in places where a single operation may occur centrally.

On a little-endian arch it makes no difference obviously, but on a big-endian arch it's the difference between compile-time and runtime byteswapping.

Its not my driver, but I think that doing the operation on the switch is prettier than doing it on each case statement. Also, it would avoid future bugs, if somebody decides to add new cases to the switch block.

-Mike

author
#4 lore raw
Quoted 15 lines · Michael Krufky
Quoted 5 lines · Harvey Harrison
Quoted 4 lines · Michael Krufky

Wouldn't it be nicer to just switch on cpu_to_le16(udev->descriptor.idVendor) ? This would be a 1-line change, compile to a smaller footprint, and be easier to read.

Quoted 7 lines
Quoted 2 lines

Personally, I prefer to try to avoid duplicating code in places where a single operation may occur centrally.

On a little-endian arch it makes no difference obviously, but on a big-endian arch it's the difference between compile-time and runtime byteswapping.

Quoted 4 lines

Its not my driver, but I think that doing the operation on the switch is prettier than doing it on each case statement. Also, it would avoid future bugs, if somebody decides to add new cases to the switch block.

The alternative is to define the vendor ids as little-endian in the headers then you don't need the endian swap in the switch or the case, but that would require looking at the other uses first....or gently encourage more people to run sparse ;-)

Harvey

#5 lore raw
Quoted 21 lines · Harvey Harrison
Quoted 15 lines · Michael Krufky
Quoted 5 lines · Harvey Harrison
Quoted 4 lines · Michael Krufky

Wouldn't it be nicer to just switch on cpu_to_le16(udev->descriptor.idVendor) ? This would be a 1-line change, compile to a smaller footprint, and be easier to read.

Quoted 7 lines
Quoted 2 lines

Personally, I prefer to try to avoid duplicating code in places where a single operation may occur centrally.

On a little-endian arch it makes no difference obviously, but on a big-endian arch it's the difference between compile-time and runtime byteswapping.

Quoted 4 lines

Its not my driver, but I think that doing the operation on the switch is prettier than doing it on each case statement. Also, it would avoid future bugs, if somebody decides to add new cases to the switch block.

Quoted 5 lines

The alternative is to define the vendor ids as little-endian in the headers then you don't need the endian swap in the switch or the case, but that would require looking at the other uses first....or gently encourage more people to run sparse ;-)

Harvey

Harvey,

If I may offer my opinion, this is not remotely performance-critical code. That said, I would favor on the side of maintainability/reliability in this case, and mkrufky's approach seems to be better on both accounts.

Regards,

Devin

Signature · 3 lines
Devin J. Heitmueller
http://www.devinheitmueller.com
AIM: devinheitmueller
author
#6 lore raw
Quoted 4 lines · Devin Heitmueller
Quoted 4 lines

The alternative is to define the vendor ids as little-endian in the headers then you don't need the endian swap in the switch or the case, but that would require looking at the other uses first....or gently encourage more people to run sparse ;-)

Quoted 8 lines
Quoted 1 line

Harvey

Harvey,

If I may offer my opinion, this is not remotely performance-critical code. That said, I would favor on the side of maintainability/reliability in this case, and mkrufky's approach seems to be better on both accounts.

Well, I can see your point, and I don't even know what the chances are of even finding this hardware on a big-endian machine are. But I'd suggest that if you want to improve the maintainability of this code that someone in v4l-land start running sparse regularly and work on getting some of these _trivial_ parts annotated, then any new ones that get introduced will stick out like sore thumbs.

But, not my driver, not my decision. Someone else can implement Michael's suggestion if they want.

Harvey

#7 lore raw
Quoted 3 lines · Harvey Harrison

But I'd suggest
that if you want to improve the maintainability of this code that someone in v4l-land start running sparse regularly

For those who don't get the daily 'bot mail on the v4l-dvb-maintainer list, Hans Verkuil's v4l-dvb build 'bot runs sparse builds daily for all of v4l-dvb land:

http://linuxtv.org/pipermail/v4l-dvb-maintainer/2008-November/008595.html

Although developers won't see waring/error outputs from those runs until after their changes are merged in the main v4l-dvb repo.

The latest kernel this automatic sparse build compiles is currently 2.26.28-rc4. I've used it to monitor the cx18 driver for sparse build errors so I could address them - it's quite convenient.

Regards,
Andy

author
#8 lore raw
Quoted 10 lines · Andy Walls
Quoted 3 lines · Harvey Harrison

But I'd suggest
that if you want to improve the maintainability of this code that someone in v4l-land start running sparse regularly

For those who don't get the daily 'bot mail on the v4l-dvb-maintainer list, Hans Verkuil's v4l-dvb build 'bot runs sparse builds daily for all of v4l-dvb land:

Quoted 9 lines

http://linuxtv.org/pipermail/v4l-dvb-maintainer/2008-November/008595.html

Although developers won't see waring/error outputs from those runs until after their changes are merged in the main v4l-dvb repo.

The latest kernel this automatic sparse build compiles is currently 2.26.28-rc4. I've used it to monitor the cx18 driver for sparse build errors so I could address them - it's quite convenient.

Does Hans' build turn on __CHECK_ENDIAN__? That would help as well.

Harvey

#9 lore raw
Quoted 13 lines · On Friday 14 November 2008 22:08:45 Harvey Harrison wrote:
Quoted 12 lines · Andy Walls
Quoted 3 lines · Harvey Harrison

But I'd suggest
that if you want to improve the maintainability of this code that someone in v4l-land start running sparse regularly

For those who don't get the daily 'bot mail on the v4l-dvb-maintainer list, Hans Verkuil's v4l-dvb build 'bot runs sparse builds daily for all of v4l-dvb land:

http://linuxtv.org/pipermail/v4l-dvb-maintainer/2008-November/00859
5.html

Quoted 8 lines
Quoted 6 lines

Although developers won't see waring/error outputs from those runs until after their changes are merged in the main v4l-dvb repo.

The latest kernel this automatic sparse build compiles is currently 2.26.28-rc4. I've used it to monitor the cx18 driver for sparse build errors so I could address them - it's quite convenient.

Does Hans' build turn on __CHECK_ENDIAN__? That would help as well.

Now it does. Thanks for the tip!

Hans

#10 lore raw
In reply to Harvey Harrison
Quoted 2 lines · Harvey Harrison

But, not my driver, not my decision. Someone else can implement Michael's suggestion if they want.

A simpler solution could be to pass the struct usb_device_id *id to the function af9015_read_config. There, the idVendor and idProduct are host native...

Signature · 2 lines
Ken ar c'hentañ |             ** Breizh ha Linux atav! **
Jef             |               http://moinejf.free.fr/