Re: [PATCH v7 1/2] platform/x86: dell-privacy: Add support for Dell hardware privacy

[Hans de Goede]

Hi Perry,

On 5/6/21 11:48 AM, Yuan, Perry wrote:
> Hi Hans.
> I changed the driver in V8 as your comments.
> Just one Kconfig change , It will cause some built error .

<snip>

>>> diff --git a/drivers/platform/x86/dell/Kconfig
>>> b/drivers/platform/x86/dell/Kconfig
>>> index e0a55337f51a..05d124442b25 100644
>>> --- a/drivers/platform/x86/dell/Kconfig
>>> +++ b/drivers/platform/x86/dell/Kconfig
>>> @@ -204,4 +204,18 @@ config DELL_WMI_SYSMAN
>>>  	  To compile this driver as a module, choose M here: the module will
>>>  	  be called dell-wmi-sysman.
>>>
>>> +config DELL_PRIVACY
>>> +	tristate "Dell Hardware Privacy Support"
>>> +	depends on ACPI
>>> +	depends on ACPI_WMI
>>> +	depends on INPUT
>>> +	depends on DELL_LAPTOP
>>> +	depends on LEDS_TRIGGER_AUDIO
>>> +	select DELL_WMI
>>
>> DELL_WMI is not a helper library which can be selected, please use depends
>> on here.
>>
>> More in general I'm a bit worried about the dependencies being added to dell-
>> laptop.c and dell-wmi.c on the new dell-privacy-wmi.ko module.
>>
>> What if e.g. dell-laptop.c gets builtin while dell-privacy-wmi.c is a module.
>>
>> Then we have dell-laptop.c depending on the dell_privacy_present linker-
>> symbol, but that symbol is in a module, so the main vmlinuz binary will fail to
>> link due to that missing symbol.
>>
>> To fix this you need to add:
>>
>> 	depends on DELL_PRIVACY || DELL_PRIVACY = n
>>
>> To the Kconfig sections for both DELL_WMI and DELL_LAPTOP
> 
> If I add "depends on DELL_PRIVACY || DELL_PRIVACY = n" to both DELL_WMI and DELL_LAPTOP
> The compile will report error "recursive dependency detected"
> I do not think the dell-laptop will be builtin option as we know.
> 
> I am confused that why the symbol will be failed to link like that ?
> because the compiler can find the dell_privacy_present which is defined in one common header file.

The issue is that e.g the dell-laptop code may be builtin into the kernel
(so part of the vmlinuz file) while the dell-privacy code could be build
as a module (so as a dell-privacy.ko file) in this case building the vmlinuz
file will fail at the linking stage since the dell_privacy_present() symbol
is not part of vmlinuz where as the dell-laptop code which needs that
symbol is part of vmlinuz.

The reason why these circular dependency issues trigger is because the
dell-privacy.ko module actually does not depend (at a symbol level) on
dell-laptop / dell-wmi at all. It is the other way around dell-laptop
and dell-wmi use symbols from dell-privacy, where as dell-privacy can
be loaded into the kernel without dell-wmi / dell-laptop being loaded
just fine.

Now there is a functional dependency where dell-privacy does not do much
when it is not called form the event handler in dell-wmi, but this is not
a code-level dependency.

I've been thinking a bit about this and I've come to the following
conclusions:

1. dell-privacy should not depend on dell-laptop at all, the only reason
dell-laptop calls into dell-privacy is to not register a mic-mute LED if
dell-laptop is not build at all then it will also not register the mic-mute
LED, so the dependency of dell-privacy on dell-laptop can be dropped.

2. Building dell-privacy without also building dell-wmi is a different story
this will work fine, but the dell-privacy module will the mostly just sit 
there (it will provide the sysfs files) without really doing anything.

Also since dell-wmi will depend on dell-privacy when it is enabled,
dell-privacy will always need to be loaded when dell-wmi is loaded.

The dell-privacy code really is an extension to / plugin of the dell-wmi
code; and since the 2 always need to be loaded together anyways, it would
be better to put the code in a single kernel-module (less overhead loading
modules that way) and this also neatly solves the builtin vs module
dependency issue.

This way we can simply make DELL_PRIVACY a boolean option which controls if
privacy support gets added to the dell-wmi module or not, but since it is
now built into the same module (if enabled) we can never have the case where
one part is built into the kernel and the other into a .ko file.

I'm currently preparing a set of changes which implements this, because
this is sort of hard to describe with words and I hope that providing a
patch implemeting the suggested change make things a bit more clear.

I'll send another email when the changes are ready.

Regards,

Hans