Page MenuHomeFreeBSD

iichid: Fix layout of PNP ids
Needs ReviewPublic

Authored by ashafer_badland.io on Tue, Nov 5, 2:31 AM.
Tags
None
Referenced Files
F103148313: D47448.diff
Thu, Nov 21, 3:13 PM
Unknown Object (File)
Fri, Nov 8, 12:32 PM
Unknown Object (File)
Fri, Nov 8, 10:37 AM
Unknown Object (File)
Fri, Nov 8, 1:14 AM
Unknown Object (File)
Thu, Nov 7, 10:05 AM
Subscribers

Details

Reviewers
wulf
Summary

iichid uses IICBUS_ACPI_PNP_INFO to specify the supported ACPI ids,
however the format of this seems wrong. That uses ACPICOMPAT_PNP_INFO
under the hood, which uses a format of "Z:_HID". This expects a list of
strings, but we currently give it a list of struct iichid_id that
contain mixed pointers and ints. The other users of ACPICOMPAT_PNP_INFO
seem to use a list of char * pointers as well, iichid is the only one
that differs.

This change breaks out the struct array into a char * array and a
int array, and indexes into them instead of iterating through the
structs. This seems to fix the touchpad on my Legion 5 laptop.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Skipped
Unit
Tests Skipped
Build Status
Buildable 60392
Build 57276: arc lint + arc unit

Event Timeline

Curious your opinions on this. Maybe I'm misreading the pnp code but it looks like we have been handing it the wrong format and it thinks it's a list of strings when it isn't.

I view this as a step backwards. What's the advantage of doing parallel arrays that might not be updated correctly over having them in a structure?
The PNP stuff should be handled correctly because we also encode a table row length so that we ignore the remainder of the rows. If we can't use the 'stock' version of the macros, we should use the ones that let us set the row length instead (though I thought the stock ones did the right thing)

So the stock MODULE_PNP_INFOs do handle structures such as that, I guess the problem is we don't use them for ACPI things. from the manpage:

static struct my_pciids {
               uint32_t devid;
               const char *desc;
       } my_ids[] = {
               { 0x12345678, "Foo bar" },
               { 0x9abcdef0, "Baz fizz" },
       };

       MODULE_PNP_INFO("W32:vendor/device;D:#", pci, my_driver, my_ids,
           nitems(my_ids));

I think this is the descriptor string we would have to use for the array of structures that iichid_ids is currently, something like Z:_HID;W32:# to represent the set of { char *, int } structures in the list. Based on this example that's how I would expect iichid_ids to be handled.

Instead of that, ACPICOMPAT_PNP_INFO uses just one field in its descriptor string:

#define	ACPICOMPAT_PNP_INFO(t, busname)					\
	MODULE_PNP_INFO("Z:_HID", busname, t##hid, t, nitems(t)-1);	\
	MODULE_PNP_INFO("Z:_CID", busname, t##cid, t, nitems(t)-1);
#define	ACPI_PNP_INFO(t)	ACPICOMPAT_PNP_INFO(t, acpi)

Because of this to me it looks like ACPICOMPAT_PNP_INFO is treating our array as a list of char * pointers instead of the list of structures that you mentioned. The nitems(t) bit will also not be accurate in that case, since it will return the number of structs but the PNP processing won't actually index by the size of the structs due to the descriptor string not being accurate. But maybe I'm misunderstanding how these macros work?