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.
Yes, yes, yes, you *can* use bitfields if you absolutely have to, but
then you have to sprinkle the end result with
#if defined(__BIG_ENDIAN_BITFIELD)
.. declare it in one order..
#else
.. declare it in the reverse order ..
and even then you need to just pray that the compiler packs things right.
See include/uapi/linux/cdrom.h for an example of where we do it.
Because people started using bitfields long ago, and never fixed it,
and it became part of the API interface and is even exposed to user
space. Ugh.
Please don't repeat that mistake. It's been repeated several times
because people think bitfields are convenient. And they *can* be
convenient, but only for pure software uses where the actual placement
of the bits doesn't matter.
Linus
