Messages in this thread | | | Date | Wed, 19 Jan 2022 11:20:36 +0100 | Subject | Re: Convert type of 'struct dmi_system_id -> driver_data' from 'void *' to kernel_ulong_t? | From | 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
| |