diff mbox

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

Message ID 20161023194652.24335-2-hdegoede@redhat.com (mailing list archive)
State Superseded, archived
Headers show

Commit Message

Hans de Goede Oct. 23, 2016, 7:46 p.m. UTC
There are several cases where events handled in one of the dell-* drivers
need to be propagated to another dell-* driver.

This commits add 3 generic functions:
dell_smbios_register_notifier()
dell_smbios_unregister_notifier()
dell_smbios_call_notifier()

It currently only defines 1 action:
dell_smbios_kbd_backlight_brightness_changed

Which is intended to propagate kbd_backlight_brightness_changed wmi
events from dell-wmi to dell-laptop (which contains the actual kbd
backlight driver).

This is only somewhat dell smbios related, both dell-wmi and dell-laptop
use smbios functions and I do not want to put the notifier head in
either driver, as that will make the 2 drivers depend on each other.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v2:
-This is a new patch in v2 of this patch-set
---
 drivers/platform/x86/dell-smbios.c | 20 ++++++++++++++++++++
 drivers/platform/x86/dell-smbios.h | 11 +++++++++++
 2 files changed, 31 insertions(+)

Comments

Pali Rohár Oct. 24, 2016, 1:31 p.m. UTC | #1
On Sunday 23 October 2016 21:46:50 Hans de Goede wrote:
> There are several cases where events handled in one of the dell-* drivers
> need to be propagated to another dell-* driver.
> 
> This commits add 3 generic functions:
> dell_smbios_register_notifier()
> dell_smbios_unregister_notifier()
> dell_smbios_call_notifier()
> 
> It currently only defines 1 action:
> dell_smbios_kbd_backlight_brightness_changed
> 
> Which is intended to propagate kbd_backlight_brightness_changed wmi
> events from dell-wmi to dell-laptop (which contains the actual kbd
> backlight driver).
> 
> This is only somewhat dell smbios related, both dell-wmi and dell-laptop
> use smbios functions and I do not want to put the notifier head in
> either driver, as that will make the 2 drivers depend on each other.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> Changes in v2:
> -This is a new patch in v2 of this patch-set
> ---
>  drivers/platform/x86/dell-smbios.c | 20 ++++++++++++++++++++
>  drivers/platform/x86/dell-smbios.h | 11 +++++++++++
>  2 files changed, 31 insertions(+)
> 
> diff --git a/drivers/platform/x86/dell-smbios.c b/drivers/platform/x86/dell-smbios.c
> index d2412ab..8b96bdf 100644
> --- a/drivers/platform/x86/dell-smbios.c
> +++ b/drivers/platform/x86/dell-smbios.c
> @@ -105,6 +105,26 @@ struct calling_interface_token *dell_smbios_find_token(int tokenid)
>  }
>  EXPORT_SYMBOL_GPL(dell_smbios_find_token);
>  
> +static ATOMIC_NOTIFIER_HEAD(dell_smbios_chain_head);
> +
> +int dell_smbios_register_notifier(struct notifier_block *nb)
> +{
> +	return atomic_notifier_chain_register(&dell_smbios_chain_head, nb);
> +}
> +EXPORT_SYMBOL_GPL(dell_smbios_register_notifier);
> +
> +int dell_smbios_unregister_notifier(struct notifier_block *nb)
> +{
> +	return atomic_notifier_chain_unregister(&dell_smbios_chain_head, nb);
> +}
> +EXPORT_SYMBOL_GPL(dell_smbios_unregister_notifier);
> +
> +void dell_smbios_call_notifier(unsigned long action, void *data)
> +{
> +	atomic_notifier_call_chain(&dell_smbios_chain_head, action, data);
> +}
> +EXPORT_SYMBOL_GPL(dell_smbios_call_notifier);
> +
>  static void __init parse_da_table(const struct dmi_header *dm)
>  {
>  	/* Final token is a terminator, so we don't want to copy it */
> diff --git a/drivers/platform/x86/dell-smbios.h b/drivers/platform/x86/dell-smbios.h
> index ec7d40a..e91f13f 100644
> --- a/drivers/platform/x86/dell-smbios.h
> +++ b/drivers/platform/x86/dell-smbios.h
> @@ -16,6 +16,8 @@
>  #ifndef _DELL_SMBIOS_H_
>  #define _DELL_SMBIOS_H_
>  
> +struct notifier_block;
> +
>  /* This structure will be modified by the firmware when we enter
>   * system management mode, hence the volatiles */
>  
> @@ -43,4 +45,13 @@ void dell_smbios_release_buffer(void);
>  void dell_smbios_send_request(int class, int select);
>  
>  struct calling_interface_token *dell_smbios_find_token(int tokenid);
> +
> +enum dell_smbios_notifier_actions {
> +	dell_smbios_kbd_backlight_brightness_changed,
> +};
> +
> +int dell_smbios_register_notifier(struct notifier_block *nb);
> +int dell_smbios_unregister_notifier(struct notifier_block *nb);
> +void dell_smbios_call_notifier(unsigned long action, void *data);
> +
>  #endif

That is better but technically speaking those notifiers do not have
nothing common with dell-smbios :-( Driver dell-smbios.ko provides API
for sending SMBIOS requests via SMM. Maybe move those functions into
separate module (and built statically into kernel)? Just speculation, do
not have something useful in my mind.
Hans de Goede Oct. 24, 2016, 1:37 p.m. UTC | #2
Hi,

On 24-10-16 15:31, Pali Rohár wrote:
> On Sunday 23 October 2016 21:46:50 Hans de Goede wrote:
>> There are several cases where events handled in one of the dell-* drivers
>> need to be propagated to another dell-* driver.
>>
>> This commits add 3 generic functions:
>> dell_smbios_register_notifier()
>> dell_smbios_unregister_notifier()
>> dell_smbios_call_notifier()
>>
>> It currently only defines 1 action:
>> dell_smbios_kbd_backlight_brightness_changed
>>
>> Which is intended to propagate kbd_backlight_brightness_changed wmi
>> events from dell-wmi to dell-laptop (which contains the actual kbd
>> backlight driver).
>>
>> This is only somewhat dell smbios related, both dell-wmi and dell-laptop
>> use smbios functions and I do not want to put the notifier head in
>> either driver, as that will make the 2 drivers depend on each other.
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>> Changes in v2:
>> -This is a new patch in v2 of this patch-set
>> ---
>>  drivers/platform/x86/dell-smbios.c | 20 ++++++++++++++++++++
>>  drivers/platform/x86/dell-smbios.h | 11 +++++++++++
>>  2 files changed, 31 insertions(+)
>>
>> diff --git a/drivers/platform/x86/dell-smbios.c b/drivers/platform/x86/dell-smbios.c
>> index d2412ab..8b96bdf 100644
>> --- a/drivers/platform/x86/dell-smbios.c
>> +++ b/drivers/platform/x86/dell-smbios.c
>> @@ -105,6 +105,26 @@ struct calling_interface_token *dell_smbios_find_token(int tokenid)
>>  }
>>  EXPORT_SYMBOL_GPL(dell_smbios_find_token);
>>
>> +static ATOMIC_NOTIFIER_HEAD(dell_smbios_chain_head);
>> +
>> +int dell_smbios_register_notifier(struct notifier_block *nb)
>> +{
>> +	return atomic_notifier_chain_register(&dell_smbios_chain_head, nb);
>> +}
>> +EXPORT_SYMBOL_GPL(dell_smbios_register_notifier);
>> +
>> +int dell_smbios_unregister_notifier(struct notifier_block *nb)
>> +{
>> +	return atomic_notifier_chain_unregister(&dell_smbios_chain_head, nb);
>> +}
>> +EXPORT_SYMBOL_GPL(dell_smbios_unregister_notifier);
>> +
>> +void dell_smbios_call_notifier(unsigned long action, void *data)
>> +{
>> +	atomic_notifier_call_chain(&dell_smbios_chain_head, action, data);
>> +}
>> +EXPORT_SYMBOL_GPL(dell_smbios_call_notifier);
>> +
>>  static void __init parse_da_table(const struct dmi_header *dm)
>>  {
>>  	/* Final token is a terminator, so we don't want to copy it */
>> diff --git a/drivers/platform/x86/dell-smbios.h b/drivers/platform/x86/dell-smbios.h
>> index ec7d40a..e91f13f 100644
>> --- a/drivers/platform/x86/dell-smbios.h
>> +++ b/drivers/platform/x86/dell-smbios.h
>> @@ -16,6 +16,8 @@
>>  #ifndef _DELL_SMBIOS_H_
>>  #define _DELL_SMBIOS_H_
>>
>> +struct notifier_block;
>> +
>>  /* This structure will be modified by the firmware when we enter
>>   * system management mode, hence the volatiles */
>>
>> @@ -43,4 +45,13 @@ void dell_smbios_release_buffer(void);
>>  void dell_smbios_send_request(int class, int select);
>>
>>  struct calling_interface_token *dell_smbios_find_token(int tokenid);
>> +
>> +enum dell_smbios_notifier_actions {
>> +	dell_smbios_kbd_backlight_brightness_changed,
>> +};
>> +
>> +int dell_smbios_register_notifier(struct notifier_block *nb);
>> +int dell_smbios_unregister_notifier(struct notifier_block *nb);
>> +void dell_smbios_call_notifier(unsigned long action, void *data);
>> +
>>  #endif
>
> That is better but technically speaking those notifiers do not have
> nothing common with dell-smbios :-(

Well WMI events get enabled via a SMBIOS call, and dell-laptop uses
SMBIOS exclusively, 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.

Regards,

Hans
--
To unsubscribe from this list: send the line "unsubscribe platform-driver-x86" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Pali Rohár Oct. 24, 2016, 1:43 p.m. UTC | #3
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?
Hans de Goede Oct. 24, 2016, 1:45 p.m. UTC | #4
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.

Regards,

Hans
--
To unsubscribe from this list: send the line "unsubscribe platform-driver-x86" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Pali Rohár Oct. 27, 2016, 10:32 a.m. UTC | #5
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?
Hans de Goede Oct. 27, 2016, 12:45 p.m. UTC | #6
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).

Given that we already have dell-smbios as dell library functions
module, I think that adding a dell-common is a bit overkill.

I can rename the notifier, maybe use dell_laptop*notifier as names,
since dell-laptop is the main consumer of the notifications ?

Regards,

Hans

--
To unsubscribe from this list: send the line "unsubscribe platform-driver-x86" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Pali Rohár Oct. 27, 2016, 12:51 p.m. UTC | #7
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.

> 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!
Hans de Goede Oct. 27, 2016, 12:54 p.m. UTC | #8
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 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
--
To unsubscribe from this list: send the line "unsubscribe platform-driver-x86" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Pali Rohár Oct. 27, 2016, 12:57 p.m. UTC | #9
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
diff mbox

Patch

diff --git a/drivers/platform/x86/dell-smbios.c b/drivers/platform/x86/dell-smbios.c
index d2412ab..8b96bdf 100644
--- a/drivers/platform/x86/dell-smbios.c
+++ b/drivers/platform/x86/dell-smbios.c
@@ -105,6 +105,26 @@  struct calling_interface_token *dell_smbios_find_token(int tokenid)
 }
 EXPORT_SYMBOL_GPL(dell_smbios_find_token);
 
+static ATOMIC_NOTIFIER_HEAD(dell_smbios_chain_head);
+
+int dell_smbios_register_notifier(struct notifier_block *nb)
+{
+	return atomic_notifier_chain_register(&dell_smbios_chain_head, nb);
+}
+EXPORT_SYMBOL_GPL(dell_smbios_register_notifier);
+
+int dell_smbios_unregister_notifier(struct notifier_block *nb)
+{
+	return atomic_notifier_chain_unregister(&dell_smbios_chain_head, nb);
+}
+EXPORT_SYMBOL_GPL(dell_smbios_unregister_notifier);
+
+void dell_smbios_call_notifier(unsigned long action, void *data)
+{
+	atomic_notifier_call_chain(&dell_smbios_chain_head, action, data);
+}
+EXPORT_SYMBOL_GPL(dell_smbios_call_notifier);
+
 static void __init parse_da_table(const struct dmi_header *dm)
 {
 	/* Final token is a terminator, so we don't want to copy it */
diff --git a/drivers/platform/x86/dell-smbios.h b/drivers/platform/x86/dell-smbios.h
index ec7d40a..e91f13f 100644
--- a/drivers/platform/x86/dell-smbios.h
+++ b/drivers/platform/x86/dell-smbios.h
@@ -16,6 +16,8 @@ 
 #ifndef _DELL_SMBIOS_H_
 #define _DELL_SMBIOS_H_
 
+struct notifier_block;
+
 /* This structure will be modified by the firmware when we enter
  * system management mode, hence the volatiles */
 
@@ -43,4 +45,13 @@  void dell_smbios_release_buffer(void);
 void dell_smbios_send_request(int class, int select);
 
 struct calling_interface_token *dell_smbios_find_token(int tokenid);
+
+enum dell_smbios_notifier_actions {
+	dell_smbios_kbd_backlight_brightness_changed,
+};
+
+int dell_smbios_register_notifier(struct notifier_block *nb);
+int dell_smbios_unregister_notifier(struct notifier_block *nb);
+void dell_smbios_call_notifier(unsigned long action, void *data);
+
 #endif