Skip to contentSkip to conversation

usb: gadget: uvc: cleanup UVCG_FRAME_ATTR macro

PATCH In review Dan Carpenter 3 participants 3 messages Jan 14, 2015 – Jan 18, 2015

Subsystems (3)
lore
Share & tools
lore
#0 lore raw

PATCH 6/6 usb: gadget: uvc: cleanup UVCG_FRAME_ATTR macro

1) Change "conv" an "vnoc" to "to_cpu_endian" to "to_little_endian".
2) No need to check the "limit" because that is already handled in kstrtoXX so delete that parameter along with the check.
3) By using a "bits" parameter, we can combine the "uxx" parameter and the "str2u" parameters.
4) The kstrtou##bits() conversion does not need to be done under the mutex so move it to the start of the function.
5) Change the name of "identity_conv" to "noop_conversion".

---
This file has a couple pages of Sparse endian warnings.
http://lwn.net/Articles/205624/
It's hard to review the static checker warnings when there are so many.

drivers/usb/gadget/function/uvc_configfs.c +18 -26
index a0443a2..87beb8c 100644
@@ -1032,7 +1032,7 @@ static struct configfs_item_operations uvcg_frame_item_ops = {
 	.store_attribute	= uvcg_frame_attr_store,
 };
 
-#define UVCG_FRAME_ATTR(cname, aname, conv, str2u, uxx, vnoc, limit)	\
+#define UVCG_FRAME_ATTR(cname, aname, to_cpu_endian, to_little_endian, bits) \
 static ssize_t uvcg_frame_##cname##_show(struct uvcg_frame *f, char *page)\
 {									\
 	struct f_uvc_opts *opts;					\
@@ -1046,7 +1046,7 @@ static ssize_t uvcg_frame_##cname##_show(struct uvcg_frame *f, char *page)\
 	opts = to_f_uvc_opts(opts_item);				\
 									\
 	mutex_lock(&opts->lock);					\
-	result = sprintf(page, "%d\n", conv(f->frame.cname));		\
+	result = sprintf(page, "%d\n", to_cpu_endian(f->frame.cname));	\
 	mutex_unlock(&opts->lock);					\
 									\
 	mutex_unlock(su_mutex);						\
@@ -1061,7 +1061,11 @@ static ssize_t  uvcg_frame_##cname##_store(struct uvcg_frame *f,	\
 	struct uvcg_format *fmt;					\
 	struct mutex *su_mutex = &f->item.ci_group->cg_subsys->su_mutex;\
 	int ret;							\
-	uxx num;							\
+	u##bits num;							\
+									\
+	ret = kstrtou##bits(page, 0, &num);				\
+	if (ret)							\
+		return ret;						\
 									\
 	mutex_lock(su_mutex); /* for navigating configfs hierarchy */	\
 									\
@@ -1075,15 +1079,7 @@ static ssize_t  uvcg_frame_##cname##_store(struct uvcg_frame *f,	\
 		goto end;						\
 	}								\
 									\
-	ret = str2u(page, 0, &num);					\
-	if (ret)							\
-		goto end;						\
-									\
-	if (num > limit) {						\
-		ret = -EINVAL;						\
-		goto end;						\
-	}								\
-	f->frame.cname = vnoc(num);					\
+	f->frame.cname = to_little_endian(num);				\
 	ret = len;							\
 end:									\
 	mutex_unlock(&opts->lock);					\
@@ -1097,24 +1093,20 @@ static struct uvcg_frame_attribute					\
 			uvcg_frame_##cname##_show,			\
 			uvcg_frame_##cname##_store)
 
-#define identity_conv(x) (x)
+#define noop_conversion(x) (x)
 
-UVCG_FRAME_ATTR(bm_capabilities, bmCapabilities, identity_conv, kstrtou8, u8,
-		identity_conv, 0xFF);
-UVCG_FRAME_ATTR(w_width, wWidth, le16_to_cpu, kstrtou16, u16, cpu_to_le16,
-		0xFFFF);
-UVCG_FRAME_ATTR(w_height, wHeight, le16_to_cpu, kstrtou16, u16, cpu_to_le16,
-		0xFFFF);
-UVCG_FRAME_ATTR(dw_min_bit_rate, dwMinBitRate, le32_to_cpu, kstrtou32, u32,
-		cpu_to_le32, 0xFFFFFFFF);
-UVCG_FRAME_ATTR(dw_max_bit_rate, dwMaxBitRate, le32_to_cpu, kstrtou32, u32,
-		cpu_to_le32, 0xFFFFFFFF);
+UVCG_FRAME_ATTR(bm_capabilities, bmCapabilities, noop_conversion,
+		noop_conversion, 8);
+UVCG_FRAME_ATTR(w_width, wWidth, le16_to_cpu, cpu_to_le16, 16);
+UVCG_FRAME_ATTR(w_height, wHeight, le16_to_cpu, cpu_to_le16, 16);
+UVCG_FRAME_ATTR(dw_min_bit_rate, dwMinBitRate, le32_to_cpu, cpu_to_le32, 32);
+UVCG_FRAME_ATTR(dw_max_bit_rate, dwMaxBitRate, le32_to_cpu, cpu_to_le32, 32);
 UVCG_FRAME_ATTR(dw_max_video_frame_buffer_size, dwMaxVideoFrameBufferSize,
-		le32_to_cpu, kstrtou32, u32, cpu_to_le32, 0xFFFFFFFF);
+		le32_to_cpu, cpu_to_le32, 32);
 UVCG_FRAME_ATTR(dw_default_frame_interval, dwDefaultFrameInterval,
-		le32_to_cpu, kstrtou32, u32, cpu_to_le32, 0xFFFFFFFF);
+		le32_to_cpu, cpu_to_le32, 32);
 
-#undef identity_conv
+#undef noop_conversion
 
 #undef UVCG_FRAME_ATTR
 
Acked-by on v1 6/6
#1 lore raw

W dniu 14.01.2015 o 22:06, Dan Carpenter pisze:

Quoted 11 lines

1) Change "conv" an "vnoc" to "to_cpu_endian" to "to_little_endian".
2) No need to check the "limit" because that is already handled in kstrtoXX so delete that parameter along with the check.
3) By using a "bits" parameter, we can combine the "uxx" parameter and the "str2u" parameters.
4) The kstrtou##bits() conversion does not need to be done under the mutex so move it to the start of the function.
5) Change the name of "identity_conv" to "noop_conversion".

---

Quoted 96 lines

This file has a couple pages of Sparse endian warnings.
http://lwn.net/Articles/205624/
It's hard to review the static checker warnings when there are so many.

drivers/usb/gadget/function/uvc_configfs.c +18 -27
index a0443a2..87beb8c 100644
@@ -1032,7 +1032,7 @@ static struct configfs_item_operations uvcg_frame_item_ops = {
  	.store_attribute	= uvcg_frame_attr_store,
  };
 
-#define UVCG_FRAME_ATTR(cname, aname, conv, str2u, uxx, vnoc, limit)	\
+#define UVCG_FRAME_ATTR(cname, aname, to_cpu_endian, to_little_endian, bits) \
  static ssize_t uvcg_frame_##cname##_show(struct uvcg_frame *f, char *page)\
  {									\
  	struct f_uvc_opts *opts;					\
@@ -1046,7 +1046,7 @@ static ssize_t uvcg_frame_##cname##_show(struct uvcg_frame *f, char *page)\
  	opts = to_f_uvc_opts(opts_item);				\
  									\
  	mutex_lock(&opts->lock);					\
-	result = sprintf(page, "%d\n", conv(f->frame.cname));		\
+	result = sprintf(page, "%d\n", to_cpu_endian(f->frame.cname));	\
  	mutex_unlock(&opts->lock);					\
  									\
  	mutex_unlock(su_mutex);						\
@@ -1061,7 +1061,11 @@ static ssize_t  uvcg_frame_##cname##_store(struct uvcg_frame *f,	\
  	struct uvcg_format *fmt;					\
  	struct mutex *su_mutex = &f->item.ci_group->cg_subsys->su_mutex;\
  	int ret;							\
-	uxx num;							\
+	u##bits num;							\
+									\
+	ret = kstrtou##bits(page, 0, &num);				\
+	if (ret)							\
+		return ret;						\
  									\
  	mutex_lock(su_mutex); /* for navigating configfs hierarchy */	\
  									\
@@ -1075,15 +1079,7 @@ static ssize_t  uvcg_frame_##cname##_store(struct uvcg_frame *f,	\
  		goto end;						\
  	}								\
  									\
-	ret = str2u(page, 0, &num);					\
-	if (ret)							\
-		goto end;						\
-									\
-	if (num > limit) {						\
-		ret = -EINVAL;						\
-		goto end;						\
-	}								\
-	f->frame.cname = vnoc(num);					\
+	f->frame.cname = to_little_endian(num);				\
  	ret = len;							\
  end:									\
  	mutex_unlock(&opts->lock);					\
@@ -1097,24 +1093,20 @@ static struct uvcg_frame_attribute					\
  			uvcg_frame_##cname##_show,			\
  			uvcg_frame_##cname##_store)
 
-#define identity_conv(x) (x)
+#define noop_conversion(x) (x)
 
-UVCG_FRAME_ATTR(bm_capabilities, bmCapabilities, identity_conv, kstrtou8, u8,
-		identity_conv, 0xFF);
-UVCG_FRAME_ATTR(w_width, wWidth, le16_to_cpu, kstrtou16, u16, cpu_to_le16,
-		0xFFFF);
-UVCG_FRAME_ATTR(w_height, wHeight, le16_to_cpu, kstrtou16, u16, cpu_to_le16,
-		0xFFFF);
-UVCG_FRAME_ATTR(dw_min_bit_rate, dwMinBitRate, le32_to_cpu, kstrtou32, u32,
-		cpu_to_le32, 0xFFFFFFFF);
-UVCG_FRAME_ATTR(dw_max_bit_rate, dwMaxBitRate, le32_to_cpu, kstrtou32, u32,
-		cpu_to_le32, 0xFFFFFFFF);
+UVCG_FRAME_ATTR(bm_capabilities, bmCapabilities, noop_conversion,
+		noop_conversion, 8);
+UVCG_FRAME_ATTR(w_width, wWidth, le16_to_cpu, cpu_to_le16, 16);
+UVCG_FRAME_ATTR(w_height, wHeight, le16_to_cpu, cpu_to_le16, 16);
+UVCG_FRAME_ATTR(dw_min_bit_rate, dwMinBitRate, le32_to_cpu, cpu_to_le32, 32);
+UVCG_FRAME_ATTR(dw_max_bit_rate, dwMaxBitRate, le32_to_cpu, cpu_to_le32, 32);
  UVCG_FRAME_ATTR(dw_max_video_frame_buffer_size, dwMaxVideoFrameBufferSize,
-		le32_to_cpu, kstrtou32, u32, cpu_to_le32, 0xFFFFFFFF);
+		le32_to_cpu, cpu_to_le32, 32);
  UVCG_FRAME_ATTR(dw_default_frame_interval, dwDefaultFrameInterval,
-		le32_to_cpu, kstrtou32, u32, cpu_to_le32, 0xFFFFFFFF);
+		le32_to_cpu, cpu_to_le32, 32);
 
-#undef identity_conv
+#undef noop_conversion
 
  #undef UVCG_FRAME_ATTR
 
--

To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Acked-by on v1 6/6
#2 lore raw
In reply to Dan Carpenter

Hi Dan,

Thank you for the patch.

Quoted 10 lines · On Thursday 15 January 2015 00:06:35 Dan Carpenter wrote:

1) Change "conv" an "vnoc" to "to_cpu_endian" to "to_little_endian".
2) No need to check the "limit" because that is already handled in kstrtoXX so delete that parameter along with the check.
3) By using a "bits" parameter, we can combine the "uxx" parameter and the "str2u" parameters.
4) The kstrtou##bits() conversion does not need to be done under the mutex so move it to the start of the function.
5) Change the name of "identity_conv" to "noop_conversion".

drivers/usb/gadget/function/uvc_configfs.c static struct uvcg_frame_attribute

Quoted 58 lines

---
This file has a couple pages of Sparse endian warnings.
http://lwn.net/Articles/205624/
It's hard to review the static checker warnings when there are so many.

(quoted diff) +0 -0

b/drivers/usb/gadget/function/uvc_configfs.c index a0443a2..87beb8c 100644

drivers/usb/gadget/function/uvc_configfs.c +1 -1
@@ -1032,7 +1032,7 @@ static struct configfs_item_operations
 uvcg_frame_item_ops = { .store_attribute	= uvcg_frame_attr_store,
 };
 
-#define UVCG_FRAME_ATTR(cname, aname, conv, str2u, uxx, vnoc, limit)	\
+#define UVCG_FRAME_ATTR(cname, aname, to_cpu_endian, to_little_endian,

bits) \ static ssize_t uvcg_frame_##cname##_show(struct uvcg_frame *f, char
*page)\ { \

drivers/usb/gadget/function/uvc_configfs.c +7 -11
 	struct f_uvc_opts *opts;					\
@@ -1046,7 +1046,7 @@ static ssize_t uvcg_frame_##cname##_show(struct
 uvcg_frame *f, char *page)\ opts = to_f_uvc_opts(opts_item);				\
 									\
 	mutex_lock(&opts->lock);					\
-	result = sprintf(page, "%d\n", conv(f->frame.cname));		\
+	result = sprintf(page, "%d\n", to_cpu_endian(f->frame.cname));	\
 	mutex_unlock(&opts->lock);					\
 									\
 	mutex_unlock(su_mutex);						\
@@ -1061,7 +1061,11 @@ static ssize_t  uvcg_frame_##cname##_store(struct
 uvcg_frame *f,	\ struct uvcg_format *fmt;					\
 	struct mutex *su_mutex = &f->item.ci_group->cg_subsys->su_mutex;\
 	int ret;							\
-	uxx num;							\
+	u##bits num;							\
+									\
+	ret = kstrtou##bits(page, 0, &num);				\
+	if (ret)							\
+		return ret;						\
 									\
 	mutex_lock(su_mutex); /* for navigating configfs hierarchy */	\
 									\
@@ -1075,15 +1079,7 @@ static ssize_t  uvcg_frame_##cname##_store(struct
 uvcg_frame *f,	\ goto end;						\
 	}								\
 									\
-	ret = str2u(page, 0, &num);					\
-	if (ret)							\
-		goto end;						\
-									\
-	if (num > limit) {						\
-		ret = -EINVAL;						\
-		goto end;						\
-	}								\
-	f->frame.cname = vnoc(num);					\
+	f->frame.cname = to_little_endian(num);				\
 	ret = len;							\
 end:									\
 	mutex_unlock(&opts->lock);					\
@@ -1097,24 +1093,20 @@ static struct uvcg_frame_attribute					

\

Quoted 34 lines
drivers/usb/gadget/function/uvc_configfs.c +9 -11
 			uvcg_frame_##cname##_show,			\
 			uvcg_frame_##cname##_store)
 
-#define identity_conv(x) (x)
+#define noop_conversion(x) (x)
 
-UVCG_FRAME_ATTR(bm_capabilities, bmCapabilities, identity_conv, kstrtou8,
 u8, -		identity_conv, 0xFF);
-UVCG_FRAME_ATTR(w_width, wWidth, le16_to_cpu, kstrtou16, u16, cpu_to_le16,
-		0xFFFF);
-UVCG_FRAME_ATTR(w_height, wHeight, le16_to_cpu, kstrtou16, u16,
 cpu_to_le16, -		0xFFFF);
-UVCG_FRAME_ATTR(dw_min_bit_rate, dwMinBitRate, le32_to_cpu, kstrtou32, u32,
-		cpu_to_le32, 0xFFFFFFFF);
-UVCG_FRAME_ATTR(dw_max_bit_rate, dwMaxBitRate, le32_to_cpu, kstrtou32, u32,
-		cpu_to_le32, 0xFFFFFFFF);
+UVCG_FRAME_ATTR(bm_capabilities, bmCapabilities, noop_conversion,
+		noop_conversion, 8);
+UVCG_FRAME_ATTR(w_width, wWidth, le16_to_cpu, cpu_to_le16, 16);
+UVCG_FRAME_ATTR(w_height, wHeight, le16_to_cpu, cpu_to_le16, 16);
+UVCG_FRAME_ATTR(dw_min_bit_rate, dwMinBitRate, le32_to_cpu, cpu_to_le32,
 32); +UVCG_FRAME_ATTR(dw_max_bit_rate, dwMaxBitRate, le32_to_cpu,
 cpu_to_le32, 32); UVCG_FRAME_ATTR(dw_max_video_frame_buffer_size,
 dwMaxVideoFrameBufferSize, -		le32_to_cpu, kstrtou32, u32, cpu_to_le32,
 0xFFFFFFFF);
+		le32_to_cpu, cpu_to_le32, 32);
 UVCG_FRAME_ATTR(dw_default_frame_interval, dwDefaultFrameInterval,
-		le32_to_cpu, kstrtou32, u32, cpu_to_le32, 0xFFFFFFFF);
+		le32_to_cpu, cpu_to_le32, 32);
 
-#undef identity_conv
+#undef noop_conversion
 
 #undef UVCG_FRAME_ATTR
Signature · 3 lines
Regards,

Laurent Pinchart