Re: [PATCH] efi: pstore: Fix pstore_disable parameter setting (if built as module)

["Guilherme G. Piccoli" <gpiccoli@igalia.com>]

Hi Ard, thanks for your prompt response!


>> [...]
>>  	if (pstore_disable)
>>  		return 0;
>>
>> +	if (efi_pstore_info.bufsize)
>> +		return 0;
>> +
> 
> Can we please address the root cause rather than paper over it here?
> 
> What is the point of having a) a pstore_disable= param on a module
> (which can be blacklisted or simply not loaded) and b) having it
> runtime writable so pstore-efi can be toggled on/off at will?
> 

Well ... despite we agree on that, I think removing pstore_disable is
kinda breaking "the contract" we're following since long time ago.

The thing is: unlike ramoops, or even maybe pstore/blk and other
potential backends, efi_pstore is a bit of a different beast IIUC.

It doesn't require any config, by having it loaded, it'll just go ahead
and ...get enabled as pstore backend (unless pstore.backend sets otherwise).

And not only that: efi_pstore is **usually built-in** - I guess in all
distros I've checked and by default config, it's built-in.

So, probably to avoid having to use {initcall,module}_blacklist
(depending on the build type), I guess it was decided to have this
parameter. The commit [0] that added that, >13y ago, mentions that since
it was enabled by default and some EFI implementations had issues when
filling its memory with oops logs, the option was to add such
parameter+Kconfig to force disabling efi_pstore. Good decision or not,
we should now live with that IMO.


> Also, the practice of controlling the functionality of a builtin
> diagnostic feature by poking something under /sys/module/foo/params
> is in rather poor taste IMNSHO.
> 

Not only poor taste, I would say super counter-intuitive. But allowing
the sysfs playing was added by myself, to enable users to change it
without requiring a reboot, for a very cheap cost (well...not so cheap,
since I added a bug and here am I now, trying to fix it heh).

If we require a reboot, we are in the same situation of kdump -
crashkernel must be set on command-line and requires a reboot to
properly reserve the memory. There is no technical reason for that in
case of efi_pstore, at least, none I can see.


So, with all that said, what is the path you suggest we follow?

a) This simple fix (or something similar)?

b) Drop entirely pstore_disable, risking a lot of users to just face
breakages in their setups?

c) Other ideas?


Appreciate your/others feedback, thanks in advance!
Cheers,

Guilherme


[0] ec0971ba5372 ("efivars: Add module parameter to disable use as a
pstore backend")