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

[Moore, Robert]

Yes, it is unfortunate that different ACPI tables have different subtable headers, but that's the way it is. And, there are at least three different types, if not more.

Bob


> -----Original Message-----
> From: Hanjun Guo [mailto:hanjun.guo@xxxxxxxxxx]
> Sent: Saturday, May 02, 2015 7:02 AM
> To: Timur Tabi; Zheng, Lv
> Cc: Wysocki, Rafael J; Brown, Len; Lv Zheng; lkml; linux-
> acpi@xxxxxxxxxxxxxxx; Tomasz Nowicki; Moore, Robert
> Subject: Re: [PATCH 15/17] ACPICA/ARM: ACPI 5.1: Update for GTDT table
> changes.
> 
> 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
> 
> 
> >