Convert type of 'struct dmi_system_id -> driver_data' from 'void *' to kernel_ulong_t?

Convert type of 'struct dmi_system_id -> driver_data' from 'void *' to kernel_ulong_t?

[Kai-Heng Feng]

Hi,

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?

Kai-Heng

Re: Convert type of 'struct dmi_system_id -> driver_data' from 'void *' to kernel_ulong_t?

[Hans de Goede]

Hi Kai-Heng,

On 1/19/22 02:22, Kai-Heng Feng wrote:
> Hi,
> 
> 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?

I don't know if there is a technical reason for this, nor
why this choice was originally made.

But I do believe that changing this now will just lead to a lot
of unnecessary churn.

You can easily use the current void * for flags by doing;

#define FLAG1	BIT(0)
#define FLAG2	BIT(1)

...
		.driver_data = (void *)(FLAG1 | FLAG20,

...

	long quirks = (long)dmi_id->driver_data;


It is already used this way in many places. Have you done a grep
to see in how many places dmi_system_id is used? DMI based quirks
are used all over the place, changing this will cause a really
large amount of churn and for what?

So I just did a quick check and dmi_system_id is used in
*204* files in the kernel.

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.

Just a random example from: drivers/mmc/host/sdhci-pci.h

#define SDHCI_PCI_DEVICE(vend, dev, cfg) { \
        .vendor = _PCI_VEND(vend), .device = _PCI_DEV(vend, dev), \
        .subvendor = PCI_ANY_ID, .subdevice = PCI_ANY_ID, \
        .driver_data = (kernel_ulong_t)&(sdhci_##cfg) \
}

So here the unsigned long is actually not what we want and
we have to do a cast the other way around; and again the
kernel is full of these examples. So arguably if anything we
should change the other driver_data fields from
include/linux/mod_devicetable.h
to avoid the need for these kinda casts all over the kernel
(but again that would not be worth the churn IMHO).

TL;DR: lets just leave this all as it is please.

Regards,

Hans

Re: Convert type of 'struct dmi_system_id -> driver_data' from 'void *' to kernel_ulong_t?

[Bjorn Helgaas]

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.

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.

Bjorn

[1] https://git.kernel.org/pub/scm/linux/kernel/git/tglx/history.git/commit/?id=f45d069a5628

Re: Convert type of 'struct dmi_system_id -> driver_data' from 'void *' to kernel_ulong_t?

[Hans de Goede]

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.

Regards,

Hans

Re: Convert type of 'struct dmi_system_id -> driver_data' from 'void *' to kernel_ulong_t?

[Kai-Heng Feng]

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
>