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

[Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>]

On Mon, Mar 02, 2020 at 10:46:57AM +0100, Hans de Goede wrote:
> 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.

Ouch.

> > 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.

So, bug is still there...

> 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.

I see. Since we have to fix real problem, go ahead with comma, but please put
few words in the commit message why this format is being chosen.

-- 
With Best Regards,
Andy Shevchenko