Hi Greg KH, > Why not use a union here as this is coming from the hardware, right? > I used a union in PATCH v1 (https://patchwork.kernel.org/project/linux-usb/patch/20220216081651.9089-1-3090101217@xxxxxxxxxx/), I compiled it to arm64 binary with GCC 11.2.1, the binary works properly. But "kernel test robot <lkp@xxxxxxxxx>" 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 the actual stream, so driver doesn't need to take care of these parameters. 3. If bVariableSize is 1, then dwBytesPerLine must be setted 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 a union like PATCH v1, 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" for the new PATCH. > > + /* 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 which use it, they can use "UVC_DT_FORMAT_UNCOMPRESSED_SIZE" to cover the modification. In addition, we don't need "copy all uncompressed code, rename uncompressed as framebased, and make a little change" to access framebased stream support. Thanks, Jing Leng
