Messages in this thread | | | Date | Tue, 4 Jul 2023 09:43:39 -0700 | Subject | Re: [PATCH 1/3] hwmon: (oxp-sensors): remove static board variable | From | Guenter Roeck <> |
| |
On 7/4/23 09:14, Greg Kroah-Hartman wrote: > On Tue, Jul 04, 2023 at 07:14:54AM -0700, Guenter Roeck wrote: >> On 7/4/23 06:44, Greg Kroah-Hartman wrote: >>> On Tue, Jul 04, 2023 at 06:39:07AM -0700, Guenter Roeck wrote: >>>> On 7/4/23 06:17, Greg Kroah-Hartman wrote: >>>>> Drivers should not have a single static variable for the type of device >>>>> they are bound to. While this driver is really going to only have one >>>>> device at a time in the system, remove the static variable and instead, >>>>> look up the device type when needed. >>>>> >>>> >>>> This is expensive. I think it would be much better to just move >>>> the board type detection into the init code and not instantiate >>>> the driver in the fist place if the board type is unknown. >>> >>> The board type detection is all over the place in the driver, it's not >>> just for "unknown" types, so how about just saving the board type at >>> probe time and using it then for all other places? >>> >> >> I must be missing something. The current code detects the board type >> only once, in the probe function. Otherwise the static variable is used. >> You are replacing it with repeated calls to get_board_type(). >> The whole point of the static variable is to avoid the cost of repeated >> calls to dmi_first_match(). > > Ah, ok, yes, I was refering to the fact that the driver relies on the > detection of the device type in lots of different places (and doesn't > ever error out from the detection call.) >
I am lost again. Current code:
dmi_entry = dmi_first_match(dmi_table); if (!dmi_entry || boot_cpu_data.x86_vendor != X86_VENDOR_AMD) return -ENODEV;
>>>> We can handle the static variable separately if it really bothers >>>> you that much. >>> >>> I did this change to make patch 2/3 more "obvious" what is happening >>> when the in_visible() callback happens, so that you don't have to worry >>> about the saved value or not. But this whole patch isn't really needed >>> if you don't mind the lookup just happening in the in_visible() callback >>> for the first time. >>> >> >> That would at least be a minimal change, and just add one extra lookup >> which is only called once (or zero, if it is used to save the board type). > > Ok, I'll switch it up, but really, it's just a simple table lookup loop, > and none of the detection calls are on a "hot path" that I can > determine. Or am I missing something? > >> As I said, my solution would be to move the board type detection >> into the init function and not instantiate the driver in the first >> place if the probe function would bail out anyway. > > That's not the case today, the only way the probe function would fail > today is if the registering of the sysfs files fail. It does not matter > if the board detection call passes or not. >
Again, dmi_entry = dmi_first_match(dmi_table); if (!dmi_entry || boot_cpu_data.x86_vendor != X86_VENDOR_AMD) return -ENODEV; ^^^^^^^^^^^^^^^
What am I missing ?
Guenter
| |