Re: [RFC][Patch] IBM Real-Time "SMI Free" mode driver -v4

[Randy Dunlap]

On Fri, 24 Sep 2010 07:14:27 -0700 Vernon Mauery wrote:

> On Fri, Sep 24, 2010 at 6:12 AM, Arnd Bergmann <arnd@xxxxxxxx> wrote:
> > On Friday 24 September 2010, Vernon Mauery wrote:
> >> +enum rtl_addr_type {
> >> +     RTL_ADDR_TYPE_IO = 1,
> >> +     RTL_ADDR_TYPE_MMIO,
> >> +} __attribute__((packed));
> >> +
> >> +enum rtl_cmd_type {
> >> +     RTL_CMD_NOP = 0,
> >> +     RTL_CMD_ENTER_PRTM,
> >> +     RTL_CMD_EXIT_PRTM,
> >> +} __attribute__((packed));
> >
> > You didn't reply to Randy's comment about the packed attribute.
> > I think it's rather confusing here.

thanks.

> Sorry.  I missed that comment.  The packed attribute on an enum forces
> it into the smallest int type it can be.  enums are normally the size
> of an int, but this enum in the RTL table must fit in an 8-bit int.

I think you would be better off making 'command' below be a u8 then.
Otherwise someone else might add an enum value that is > 255 and the
struct size will change.  I.e., the current way is error-prone.
Just using u8 for command's type is safer.


> >> +/* The RTL table as presented by the EBDA: */
> >> +struct ibm_rtl_table {
> >> +     char signature[5];
> >> +     u8 version;
> >> +     u8 rt_status;
> >> +     enum rtl_cmd_type command;
> >> +     u8 command_status;
> >> +     enum rtl_addr_type cmd_address_type;
> >> +     u8 cmd_granularity;
> >> +     u8 cmd_offset;
> >> +     u16 reserve1;
> >> +     u8 cmd_port_address[4]; /* platform dependent address */
> >> +     u8 cmd_port_value[4];   /* platform dependent value */
> >> +};
> >
> > I would recommend marking the member in this structure as packed instead,
> > not the enum.
> 
> It does not have the same effect.  Without the packed attribute on the
> enums, they end up to be the wrong size and then we would be reading
> from the wrong location in memory.

Thanks for the explanation.

---
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/