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

[Vernon Mauery]

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.

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.

>> +/* 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.

>> +#define RTL_SIGNATURE (('L'<<24)|('T'<<16)|('R'<<8)|'_')
>> +
>> +#define ERROR(A, B...) printk(KERN_ERR "ibm-rtl: " A, ##B )
>> +#define WARNING(A, B...) printk(KERN_WARNING "ibm-rtl: " A, ##B )
>> +#define DEBUG(A, B...) do { \
>> +     if (debug) \
>> +             printk(KERN_INFO "ibm-rtl: " A, ##B ); \
>> +} while (0)
>
> We already have wrappers for these, no need to define your own.
> Please just use dev_{err,warn,dbg} or pr_{err,warning,debug}.

Ack.

>> +static DEFINE_MUTEX(rtl_lock);
>> +static struct ibm_rtl_table __iomem *rtl_table = NULL;
>> +static void __iomem *ebda_map;
>> +static void __iomem *rtl_cmd_iomem_addr = NULL;
>> +static u32 rtl_cmd_port_addr;
>> +static enum rtl_addr_type rtl_cmd_type;
>> +static u8 rtl_cmd_width;
>
> This is somewhat inconsistent, some of these are implicitly initialized,
> others have an explicit "= NULL". I would recommend leaving out the
> initialization, which is the historic way to do this in the kernel.

The variables that have an explicit initialization are the variables
that I read before I write.  It just looks funny to me to read a
variable that hasn't been initialized.  But I can axe the
initializations for the sake of consistency.

> Some people find it cleaner to define a structure containing all the
> driver specific data. Since there can be only one of these devices
> in your case, it's probably not important.
>
>> +                     if (rtl_cmd_type == RTL_ADDR_TYPE_MMIO)
>> +                             iowrite8((u8)cmd_port_val, rtl_cmd_iomem_addr);
>> +                     else
>> +                             outb((u8)cmd_port_val, rtl_cmd_port_addr);
>
> ioread/iowrite already has the capability to use both mmio and pio
> addresses. You can use ioport_map() to create an __iomem token that
> corresponds to your rtl_cmd_port_addr and get rid of the rtl_cmd_type
> variable.

Thank you for that tip.  I will look into it and roll out another version.

--Vernon

>        Arnd
> --
> 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/
>
--
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/