Re: [PATCH v2 2/4] platform: x86: dell-smbios: Add a generic dell-smbios notifier chain

["Pali Rohár" <pali.rohar@gmail.com>]

On Thursday 27 October 2016 14:54:54 Hans de Goede wrote:
> Hi,
> 
> On 27-10-16 14:51, Pali Rohár wrote:
> >On Thursday 27 October 2016 14:45:20 Hans de Goede wrote:
> >>HI,
> >>
> >>On 27-10-16 12:32, Pali Rohár wrote:
> >>>On Monday 24 October 2016 15:45:02 Hans de Goede wrote:
> >>>>Hi,
> >>>>
> >>>>On 24-10-16 15:43, Pali Rohár wrote:
> >>>>>On Monday 24 October 2016 15:37:31 Hans de Goede wrote:
> >>>>>>Well WMI events get enabled via a SMBIOS call,
> >>>>>
> >>>>>This is truth only for few laptops and only for one WMI event.
> >>>>>Everything else is automatically enabled, no call is needed to issue.
> >>>>>
> >>>>>>and dell-laptop uses SMBIOS exclusively
> >>>>>
> >>>>>IIRC dell-led.ko uses also dell-smbio.ko, so it is not exclusive for
> >>>>>dell-laptop.
> >>>>>
> >>>>>>so it seems to fit. Basically this is a case of
> >>>>>>we have to put this somewhere and dell-smbios is the best fit IMHO.
> >>>>>
> >>>>>Agree, we need to put it somewhere...
> >>>>>
> >>>>>Basically we need to solve problem how (currently) 3 kernel modules can
> >>>>>communicate. Does not kernel support such "bus/event" mechanism for
> >>>>>this?
> >>>>
> >>>>Yes it does, that is exactly what notifiers are for, but we need to
> >>>>declare the bus somewhere. I still believe dell-smbios is the best
> >>>>place.
> >>>
> >>>But dell_smbios_register_notifier() name is totally confusing. It does
> >>>not register any notifier for SMBIOS. Nor it have nothing common with
> >>>SMBIOS API.
> >>>
> >>>Also there is absolutely no need that dell-rbtn.ko needs to depends on
> >>>dell-smbios.ko. dell-rbtn.ko is ACPI driver which does not use any of
> >>>SMBIOS API.
> >>>
> >>>Right now I'm not saying what is the best place for that notifier (as I
> >>>still do not have ideal candidate). I'm just saying that notifier is not
> >>>part of SMBIOS API and therefore dell-smbios.ko is not right place for
> >>>it.
> >>>
> >>>So currently we have these different APIs for dell notebook drivers:
> >>>* ACPI (used in dell-rbtn.ko and dell-smo8800.ko)
> >>>* WMI (used in dell-wmi.ko, dell-wmi-aio.ko, dell-led.ko)
> >>>* SMBIOS (used in dell-laptop.ko, dell-wmi.ko and dell-led.ko)
> >>>* some other platform code (used in dell-laptop.ko)
> >>>
> >>>And now notifier is needed for drivers:
> >>>* dell-laptop.ko
> >>>* dell-wmi.ko
> >>>* dell-rbtn.ko
> >>>
> >>>And if I look at above two sets, none of above drivers is good candidate
> >>>for central notifier functions... Maybe we should really introduce new
> >>>separate file where will central dell notifier live?
> >>
> >>We could put this in a dell-common or some such. My main reason for
> >>going with dell-smbios is that dell-smbios is a "library" module,
> >>loading it does not do anything other then make symbols available
> >>for use by other modules. So using it to store the notifier is safe
> >>(no side effects even if e.g. only dell-rbtn gets loaded).
> >
> >I understand your motivation.
> >
> >>Given that we already have dell-smbios as dell library functions
> >>module, I think that adding a dell-common is a bit overkill.
> >
> >New module is probably really overkill... But cannot we link add those
> >notifier functions statically? So it would not be new module.
> 
> No, we need to put the notifier_head somewhere, at which point
> making the actual notifier functions inline static is not really
> helpful.

I mean object file which will not be tristate, but only Y/N and selected
automatically when dell-laptop is Y or M. Not inline static functions.

> >>I can rename the notifier, maybe use dell_laptop*notifier as names,
> >>since dell-laptop is the main consumer of the notifications ?
> >
> >Yes, this is name is better!
> 
> Ok, I will change this for the next version.
> 
> Regards,
> 
> Hans

-- 
Pali Rohár
pali.rohar@gmail.com