Re: [PATCH 15/17] ACPICA/ARM: ACPI 5.1: Update for GTDT table changes.

[Hanjun Guo]

Hi Timur,

oh, you already noticed this issue, sorry for the
noise in my previous email.

On 2015å05æ02æ 06:43, Timur Tabi wrote:
> On Tue, Jul 29, 2014 at 11:21 PM, Lv Zheng <lv.zheng@xxxxxxxxx> wrote:
> > From: Tomasz Nowicki <tomasz.nowicki@xxxxxxxxxx>
> >
> > New fields and new subtables. Tomasz Nowicki.
> > tomasz.nowicki@xxxxxxxxxx
> >
> > Signed-off-by: Tomasz Nowicki <tomasz.nowicki@xxxxxxxxxx>
> > Signed-off-by: Hanjun Guo <hanjun.guo@xxxxxxxxxx>
> > Signed-off-by: Bob Moore <robert.moore@xxxxxxxxx>
> > Signed-off-by: Lv Zheng <lv.zheng@xxxxxxxxx>
>
> Hi, I know this patch is old, but something confuses me about it:
>
> > +/* Common GTDT subtable header */
> > +
> > +struct acpi_gtdt_header {
> > +       u8 type;
> > +       u16 length;
> > +};
>
> I'm trying to write a function that parses the watchdog structure
> (acpi_gtdt_watchdog).  The first entry in that structure is
> acpi_gtdt_header.  Looking at the ACPI specification, I see that this
> is correct: the type is one byte, and the length is two bytes.
>
> However, this means that I cannot use acpi_parse_entries() to parse
> the watchdog subtable:
>
> int __init
> acpi_parse_entries(char *id, unsigned long table_size,
>          acpi_tbl_entry_handler handler,
>          struct acpi_table_header *table_header,
>          int entry_id, unsigned int max_entries)
>
> acpi_tbl_entry_handler takes an acpi_subtable_header as its first
> parameter.  However, that structure looks like this:
>
> struct acpi_subtable_header {
>      u8 type;
>      u8 length;
> };
>
> This is not compatible, so I'm confused now.  How do I properly parse
> the watchdog subtable, if I cannot use acpi_parse_entries?

This is really a pain. I noticed this when I was prototyping the code
for GTDT in ACPI 5.1, and I even sent an email to Charles to ask him if
there are mistakes here, but there are chances that more than 255 bytes
in that structure so we must use u16 for the length.

> For context, here is my patch:
>
> http://www.spinics.net/lists/linux-watchdog/msg06240.html
>
> Scroll down to function arm_sbsa_wdt_parse_gtdt().  The typecast in
> first line is invalid:
>
> +    struct acpi_gtdt_watchdog *wdg = (struct acpi_gtdt_watchdog *)header;
>
> because of the mismatch.  I don't know how to fix this.

Yes, you can't use acpi_parse_entries() anymore, in my opinion, you
can introduce a similar function as acpi_parse_entries(), you can
refer to some code that parsing the structures with u16 length, the
code I'm familiar it's the code for Intel DMAR (similar with ARM IORT),
for DMAR table parsing, it has the same head as GTDT watchdog timer:

/* DMAR subtable header */

struct acpi_dmar_header {
        u16 type;
        u16 length;
};

please refer to drivers/iommu/dmar.c and drivers/iommu/intel-iommu.c,
hope it helps.

Thanks
Hanjun


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