Messages in this thread | | | From | Kai-Heng Feng <> | Date | Thu, 20 Jan 2022 09:14:59 +0800 | Subject | Re: Convert type of 'struct dmi_system_id -> driver_data' from 'void *' to kernel_ulong_t? |
| |
Hi Hans an Bjorn,
On Thu, Jan 20, 2022 at 4:45 AM Hans de Goede <hdegoede@redhat.com> wrote: > > Hi, > > On 1/19/22 21:27, Bjorn Helgaas wrote: > > On Wed, Jan 19, 2022 at 11:20:36AM +0100, Hans de Goede wrote: > >> On 1/19/22 02:22, Kai-Heng Feng wrote: > >>> I wonder if there's any reason to use 'void *' instead of > >>> kernel_ulong_t for 'driver_data' in 'struct dmi_system_id'? > >>> > >>> I'd like to use the driver_data for applying quirk flags, and I found > >>> out unlike most other struct *_id, the dmi variant is using 'void *' > >>> for driver_data. Is there any technical reason for this? > >>> ... > > > >> You are asking for a technical reason why "void *" was used, > >> but lets turn that around, why do you believe that "unsigned long" > >> is inherently a better type here ? > >> > >> driver_data in most places in the kernel (like data for > >> all sort of callback functions) actually typically is a void * > >> already, because often people want to pass more data then what > >> fits in a single long and this also applies to driver-id attached > >> data. > > > > FWIW, "egrep "context;|data;|info;" include/linux/mod_devicetable.h" > > says 4 of the ~40 instances use a void *; the others use > > kernel_ulong_t. > > Right inside mod_devicetable.h kernel_ulong_t is the norm, but outside > e.g. inside struct device and with dev_set_drvdata/dev_get_drvdata > and all their many derratives using void * is the norm. > > So looking at the kernel as a whole using kernel_ulong_t seems > to be the exception. But maybe that indeed has something to > do with: > > > f45d069a5628 ("PCI dynids - documentation fixes, id_table NULL check") > > [1] (from the tglx history tree) added the original hint for > > pci_device_id that: > > > > Best practice for use of driver_data is to use it as an index into a > > static list of equivalant device types, not to use it as a pointer. > > > > I don't know the background of that, but I could imagine that using an > > index rather than a pointer makes things like /sys/bus/pci/.../new_id > > easier and safer. > > Right, interesting. > > OTOH we have: > > const void *device_get_match_data(struct device *dev); > > Which is a wrapper to easily get the driver_data for popular firmware > based matches (ACPI/of), which also returns a void *... > > Actually the rule seems to be that firmware-id matching, > including WMI GUID matching uses void * where as hw-id > (e.g prod:vend matching) uses kernel_ulong_t with acpi_device_id > being the exception since it is a fwid using kernel_ulong_t, > which then gets "fixed" by acpi_device_get_match_data turning > it into a void * for the caller. > > As DMI matching is closer to firmware compatible/id matching then > to actual hw-id matching, it seems that it actually follows > the pattern of fw-id matches using void * where as hw-id > matches using void * . > > TBH I don't care much either way, but I also really don't see > strong reasons to spend a lot of time on changing any of this.
Thanks for all the info. I think they are great justifications that I should avoid all the hustles to convert the type, and stick to the pointer/integer casting.
Kai-Heng
> > Regards, > > Hans >
| |