Re: [linux-next:master] BUILD REGRESSION 8a11187eb62b8b910d2c5484e1f5d160e8b11eb4

[Alan Stern <stern@rowland.harvard.edu>]

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@rowland.harvard.edu> 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.)