On Tue, Jul 04, 2023 at 09:43:39AM -0700, Guenter Roeck wrote: > 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 ? Nothing, I'm missing it, sorry. Been a long day, let me redo this... greg k-h
