linux-kernel - Re: Convert type of 'struct dmi_system_id -> driver_data' from 'void *' to kernel_ulong

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <4a767d48-fa5d-17be-373d-b49522910285@redhat.com>
Date:   Wed, 19 Jan 2022 21:45:21 +0100
From:   Hans de Goede <hdegoede@...hat.com>
To:     Bjorn Helgaas <helgaas@...nel.org>
Cc:     Kai-Heng Feng <kai.heng.feng@...onical.com>,
        LKML <linux-kernel@...r.kernel.org>, jkosina@...e.cz,
        Jason Gunthorpe <jgg@...pe.ca>,
        Bjorn Helgaas <bhelgaas@...gle.com>, mgurtovoy@...dia.com,
        linux@...ssschuh.net, Arnd Bergmann <arnd@...db.de>,
        stephan@...hold.net, "Rafael J. Wysocki" <rafael@...nel.org>
Subject: Re: Convert type of 'struct dmi_system_id -> driver_data' from 'void
 *' to kernel_ulong_t?

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