Hi Andrzej,
On 2023/02/28 0:51, Andrzej Pietrasiewicz wrote:
> After all the discussion we've had, I agree that the initial approach in your
> patch to allow 0x100 as a legitimate value in the check performed in
> __ffs_do_os_desc_header() is correct.
>
> Whatever arrives through ep0 is in little endian format. So 0x0100 will be
> stored as 0x0001, but then u16 bcd_version = le16_to_cpu(desc->bcdVersion);
> will ensure bcd_version equals 0x0100 no matter what (and regardless of
> machine's endinanness) and we *should* be comparing it with 0x0100.
Thank you for your understanding. I'm sorry if my explanation was not clear.
> I would probably change the message generated by this portion:
>
> + pr_warn("bcdVersion is expected to be 0x100, but it was 0x%x. "
> + "Pass for compatibility, but fix your user space driver.\n",
> + bcd_version);
>
> to, say:
>
> pr_warn("bcdVersion must be 0x0100, stored in Little Endian order. "
> "Userspace driver should be fixed, accepting 0x0001 for compatibility.\n");
It helped me because I am not good at English. I would like to replace the
message with this in the v3 patch.
> and there's no need to format-print bcd_version given it is 0x1
> in this branch of the "if" statement.
Yes, indeed. Aside from the wording, the format specifier was not needed.
Regards,
Yuta Hayama
