Re: [PATCH resend 2/3] gpiolib: acpi: Rename honor_wakeup option to ignore_wake, add extra quirk

[Hans de Goede <hdegoede@redhat.com>]

Hi,

On 3/2/20 10:30 AM, Andy Shevchenko wrote:
> On Sat, Feb 29, 2020 at 09:57:52PM +0100, Hans de Goede wrote:
>> On 2/28/20 12:22 PM, Hans de Goede wrote:
>>> On 2/25/20 1:57 PM, Andy Shevchenko wrote:
>>>> On Tue, Feb 25, 2020 at 02:34:25PM +0200, Andy Shevchenko wrote:
>>>>> On Tue, Feb 25, 2020 at 12:26:04PM +0100, Hans de Goede wrote:
>>>>
>>>>> Let's do it as a list of pairs, but in slightly different format (I see some
>>>>> potential to derive a generic parser, based on users described in
>>>>> Documentation/admin-guide/kernel-parameters.txt), i.e.
>>>>>
>>>>>      ignore_wake=pin:controller[,pin:controller[,...]]
>>>>
>>>> Another possible format
>>>>
>>>>      ignore_wake=controller@pin[;controller@pin[;...]]
>>>
>>> I like this one, the other one with the pin first feels wrong, the pin is
>>> part of the controller, not the other way around.
>>>
>>> I will rework the patch series to use the ignore_wake=controller@pin format.
>>
>> Just a quick note. I've changed the separator from ; to , for some reason
>> grub, at least as used in Fedora with Fedora's grub2 BLS (boot loader spec)
>> implementation does not like it when there is a ; in the kernel commandline.
> 
> Hmm... I think it would be harder then to have less possible formats in the
> command line. Do you really need right now several pins to be listed?

Yes, the existing quirk for the HP X2 10 with Cherry Trail SoC + TI PMIC,
which currently ignores wakeups on all pins needs to ignore wakeup on 2 pins.

> If it's about testing, perhaps we may do it with other means.

Well it is possible to pass the ; by putting quotes around it, so we could
go with the ; if you insist, but it really makes life harder for

>> I will also send an email about this to Fedora grub maintainer, but for
>> now it is easiest to just avoid the problem.
> 
> It's definitely bug in Grub due to existing kernel users with such format.
> It means Grub is unable to support kernel command line in full.

So I discussed this with the Fedora Grub maintainer, he says the problem
exists in upstream grub2 too, grub2 uses a shell like command syntax
both in its config file and in interactive mode, so if you do e.g.:

linux /boot/vmlinuz root=/dev/sda1 gpiolib_acpi.ignore_wake=INT33FF:01@0;INT0002:00@2

Then grub will see the INT0002:00@2 as a new separate commaond, this should
work:

linux /boot/vmlinuz root=/dev/sda1 gpiolib_acpi.ignore_wake="INT33FF:01@0;INT0002:00@2"

But the recommended way to edit the cmdline is by editing /etc/default/grub and
then re-running grub2-mkconfig, which clears the quotes unless we escape them
and since grub2-mkconfig is shell script inside shell script inside shell script
I don't even want to think about how many times I need to escape the quotes.

TL;DR: Using ; in kernel commandline options makes life much harder for users
and as such is something which we should try to avoid.

I appreciate that you are trying to come up with a format for the option which
looks like existing options and I like the @ use, but using ; really is not a
good example to follow and IMHO that (not a good example / idea) trumps keeping
the syntax identical to an existing option.

Regards,

Hans