Re: [PATCH] usb: gadget: uvc: add framebased stream support

Re: [PATCH] usb: gadget: uvc: add framebased stream support

[kernel test robot]

Hi,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on usb/usb-testing]
[also build test WARNING on linus/master v5.17-rc4 next-20220216]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/3090101217-zju-edu-cn/usb-gadget-uvc-add-framebased-stream-support/20220216-162037
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git usb-testing
config: arm-randconfig-r023-20220216 (https://download.01.org/0day-ci/archive/20220217/202202170211.van9U4Ha-lkp@intel.com/config)
compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project 0e628a783b935c70c80815db6c061ec84f884af5)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install arm cross compiling tool for clang build
        # apt-get install binutils-arm-linux-gnueabi
        # https://github.com/0day-ci/linux/commit/475fba6b60329d9270b699550907a5e077cd84b6
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review 3090101217-zju-edu-cn/usb-gadget-uvc-add-framebased-stream-support/20220216-162037
        git checkout 475fba6b60329d9270b699550907a5e077cd84b6
        # save the config file to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=arm SHELL=/bin/bash block// drivers/usb/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> drivers/usb/gadget/function/uvc_configfs.c:1091:3: warning: field  within 'struct uvcg_frame::(unnamed at drivers/usb/gadget/function/uvc_configfs.c:1068:2)' is less aligned than 'union uvcg_frame::(anonymous at drivers/usb/gadget/function/uvc_configfs.c:1091:3)' and is usually due to 'struct uvcg_frame::(unnamed at drivers/usb/gadget/function/uvc_configfs.c:1068:2)' being packed, which can lead to unaligned accesses [-Wunaligned-access]
                   union {
                   ^
   1 warning generated.


vim +1091 drivers/usb/gadget/function/uvc_configfs.c

  1060	
  1061	/* -----------------------------------------------------------------------------
  1062	 * streaming/<mode>/<format>/<NAME>
  1063	 */
  1064	
  1065	struct uvcg_frame {
  1066		struct config_item	item;
  1067		enum uvcg_format_type	fmt_type;
  1068		struct {
  1069			u8	b_length;
  1070			u8	b_descriptor_type;
  1071			u8	b_descriptor_subtype;
  1072			u8	b_frame_index;
  1073			u8	bm_capabilities;
  1074			u16	w_width;
  1075			u16	w_height;
  1076			u32	dw_min_bit_rate;
  1077			u32	dw_max_bit_rate;
  1078	
  1079			/*
  1080			 * The layout of last three members of framebased frame
  1081			 * is different from uncompressed frame.
  1082			 *   Last three members of uncompressed frame are:
  1083			 *     u32	dw_max_video_frame_buffer_size;
  1084			 *     u32	dw_default_frame_interval;
  1085			 *     u8	b_frame_interval_type;
  1086			 *   Last three members of framebased frame are:
  1087			 *     u32	dw_default_frame_interval;
  1088			 *     u8	b_frame_interval_type;
  1089			 *     u32	dw_bytes_perline;
  1090			 */
> 1091			union {
  1092				u32	dw_max_video_frame_buffer_size;
  1093				u32	dw_bytes_perline;
  1094			};
  1095			u32	dw_default_frame_interval;
  1096			u8	b_frame_interval_type;
  1097		} __attribute__((packed)) frame;
  1098		u32 *dw_frame_interval;
  1099	};
  1100	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

Re: Re: [PATCH] usb: gadget: uvc: add framebased stream support

[Jing Leng]

Hi Greg KH,

> Why not use a union here as this is coming from the hardware, right?
>

I used union in PATCH v1, I compiled it to arm64 binary with GCC 11.2.1, 
the binary works properly.
But "kernel test robot <lkp@intel.com>" reported a warnings:
 >> drivers/usb/gadget/function/uvc_configfs.c:1091:3: warning: 
 field  within 'struct uvcg_frame::(unnamed at drivers/usb/gadget/function/uvc_configfs.c:1068:2)' 
 is less aligned than 'union uvcg_frame::(anonymous at drivers/usb/gadget/function/uvc_configfs.c:1091:3)' 
 and is usually due to 'struct uvcg_frame::(unnamed at drivers/usb/gadget/function/uvc_configfs.c:1068:2)' 
 being packed, which can lead to unaligned accesses [-Wunaligned-access]
                   union {
                   ^
   1 warning generated.
So I use another way to handle the frame structure.

> Why is this writable, but the other variables are not?
> 

1. bFormatIndex is automatic auto calculated by the driver.
   So it is read-only.
2. I don't know why "b_aspect_ratio_x / b_aspect_ratio_y / bm_interface_flags"
   are read-only, Perhaps these parameters can be obtained directly from actual stream.
   so driver does not need to care about these parameters.
3. If bVariableSize is 1, then dwBytesPerLine must be set to zero (0).
   If bVariableSize is 0, then dwBytesPerLine can be setted to other value.
   So it is writable.

> > -		*size += sizeof(frm->frame);
> > +		*size += sizeof(frm->frame) - 4;
> 
> Where did "4" come from?
>

Uncompressed frame doesn't have "u32 dw_bytes_perline".
Framebased frame doesn't have "u32 dw_max_video_frame_buffer_size".
If we use union, there's no need to do this.
Maybe we can add "#define UVCG_SUB_FRAME_PAYLOAD_LENGTH 26", and use
"UVCG_SUB_FRAME_PAYLOAD_LENGTH" to replace "sizeof(frm->frame) - 4".

> > +	/* bVariableSize is only for framebased format. */
> > +	__u8  bVariableSize;
> 
> This just changed a user visable structure size.  What broke when doing
> this?  What tool uses this?
> 

As long as users use "UVC_DT_FORMAT_UNCOMPRESSED_SIZE" instead of
"sizeof(struct uvc_format_uncompressed)" to get the length, there is
no problem. So I have the following modifications:
    -			*size += sizeof(u->desc);
    +			*size += u->desc.bLength;

Currently this change does not break the kernel, and uvc stream APP
based on UVC gadget doesn't need to use "struct uvc_format_uncompressed".

There may be some tools that use it, They can use "UVC_DT_FORMAT_UNCOMPRESSED_SIZE"
to cover the modification.

We don't need "copy all uncompressed code, rename
uncompressed as framebased, make a little change" to access framebased
stream support.

Thanks,
Jing Leng