On Thu, Mar 17, 2022 at 12:11:59PM -0700, Linus Torvalds wrote:
> On Thu, Mar 17, 2022 at 9:17 AM Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> wrote:
> >
> > These warnings seem pretty bogus. Their meaning isn't entirely clear,
> > but the statements they complain about copy a 1-byte location to a
> > structure consisting of eight 1-bit fields (or the equivalent).
>
> bit field sizes are not well-defined by the standard.
>
> Eight 1-bit bitfields may well be an "int", with just 8 bits used (and
> it might be the high 8 bits of the 'int').
>
> The fact that you declare them with "char member:1" does *not* mean
> that the bitfield is encoded in a char. It might be. Or it might not
> be.
>
> "packed" may or may not help.
>
> The basic fact is that bitfields simply are not hugely well-specified.
> They are a convenience feature, not a "this is the layout in memory"
> feature.
>
> The fix for the warning itself would probably something along the lines of this:
>
> --- a/drivers/usb/storage/ene_ub6250.c
> +++ b/drivers/usb/storage/ene_ub6250.c
> @@ -2400,7 +2400,6 @@ static int ene_ub6250_resume(struct
> usb_interface *iface)
>
> static int ene_ub6250_reset_resume(struct usb_interface *iface)
> {
> - u8 tmp = 0;
> struct us_data *us = usb_get_intfdata(iface);
> struct ene_ub6250_info *info = (struct ene_ub6250_info
> *)(us->extra);
>
> @@ -2412,10 +2411,9 @@ static int ene_ub6250_reset_resume(struct
> usb_interface *iface)
> * the device
> */
> info->Power_IsResum = true;
> - /*info->SD_Status.Ready = 0; */
> - info->SD_Status = *(struct SD_STATUS *)&tmp;
> - info->MS_Status = *(struct MS_STATUS *)&tmp;
> - info->SM_Status = *(struct SM_STATUS *)&tmp;
> + info->SD_Status = (struct SD_STATUS) { .Ready = 0,};
> + info->MS_Status = (struct MS_STATUS) { };
> + info->SM_Status = (struct SM_STATUS) { };
>
> return 0;
> }
>
> but the fact is, using bitfields there is simply WRONG. Because this
> code that sets these fields from the hardware results:
>
> info->SD_Status = *(struct SD_STATUS *) bbuf;
>
> is fundamentally buggy. You are assuming little-endian bitfields. That
> is just not well-defined.
>
> Just don't do this. Use 'unsigned char' to specify that you want a
> byte, and use actual flags values to test the bits in that byte.
> Because that is actually well-defined and works.
Got it. Thanks for the advice; I'll create a suitable patch.
Alan Stern
(PS: Sorry about the messed up email address in the CC: list. I must
have left out the <> characters when typing it in by hand.)
