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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <f182e7e.aba73.17f0c847394.Coremail.3090101217@zju.edu.cn>
Date:   Fri, 18 Feb 2022 19:08:01 +0800 (GMT+08:00)
From:   "Jing Leng" <3090101217@....edu.cn>
To:     "kernel test robot" <lkp@...el.com>
Cc:     laurent.pinchart@...asonboard.com, balbi@...nel.org,
        gregkh@...uxfoundation.org, corbet@....net,
        mchehab+huawei@...nel.org, rdunlap@...radead.org, bilbao@...edu,
        pawell@...ence.com, llvm@...ts.linux.dev, kbuild-all@...ts.01.org,
        linux-kernel@...r.kernel.org, linux-usb@...r.kernel.org,
        "Jing Leng" <jleng@...arella.com>
Subject: Re: Re: [PATCH] usb: gadget: uvc: add framebased stream support

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@...el.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