linux-kernel - Re: [PATCH 08/10] pinctrl: cs42l43: Add support for the cs42l43

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20230517101301.GV68926@ediswmail.ad.cirrus.com>
Date:   Wed, 17 May 2023 10:13:01 +0000
From:   Charles Keepax <ckeepax@...nsource.cirrus.com>
To:     Andy Shevchenko <andy.shevchenko@...il.com>
CC:     <broonie@...nel.org>, <lee@...nel.org>, <robh+dt@...nel.org>,
        <krzysztof.kozlowski+dt@...aro.org>, <conor+dt@...nel.org>,
        <tglx@...utronix.de>, <maz@...nel.org>, <linus.walleij@...aro.org>,
        <vkoul@...nel.org>, <lgirdwood@...il.com>,
        <yung-chuan.liao@...ux.intel.com>, <sanyog.r.kale@...el.com>,
        <pierre-louis.bossart@...ux.intel.com>,
        <alsa-devel@...a-project.org>, <patches@...nsource.cirrus.com>,
        <devicetree@...r.kernel.org>, <linux-gpio@...r.kernel.org>,
        <linux-spi@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 08/10] pinctrl: cs42l43: Add support for the cs42l43

On Tue, May 16, 2023 at 10:03:45PM +0300, Andy Shevchenko wrote:
> On Mon, May 15, 2023 at 1:13 PM Charles Keepax
> <ckeepax@...nsource.cirrus.com> wrote:
> > On Fri, May 12, 2023 at 10:19:14PM +0300, andy.shevchenko@...il.com wrote:
> > > Fri, May 12, 2023 at 01:28:36PM +0100, Charles Keepax kirjoitti:
> > > > +   if (!of_property_read_bool(dev_of_node(cs42l43->dev), "gpio-ranges")) {
> > > > +           ret = gpiochip_add_pin_range(&priv->gpio_chip, priv->gpio_chip.label,
> > > > +                                        0, 0, CS42L43_NUM_GPIOS);
> > > > +           if (ret) {
> > > > +                   dev_err(priv->dev, "Failed to add GPIO pin range: %d\n", ret);
> > > > +                   goto err_pm;
> > > > +           }
> > > > +   }
> > >
> > > Besides the fact that we have a callback for this, why GPIO library can't
> > > handle this for you already?
> >
> > Apologies but I am not quite sure I follow you, in the device
> > tree case this will be handled by the GPIO library. But for ACPI
> > this information does not exist so has to be called manually, the
> > library does not necessarily know which values to call with,
> > although admittedly our case is trivial but not all are.
> 
> Why can't the firmware provide this information? _DSD() is a part of
> ACPI v5.1 IIRC.
> 

I am very very far from confident we can guarantee that will be
present in the ACPI. The ACPI is typically made for and by the
Windows side.

> Although it might require moving some code from gpiolib-of.c to
> gpiolib.c with replacing OF APIs with agnostic ones.
> 

I really think if we want to start doing things that way on ACPI
platforms someone with a little more clout than us needs to start
doing it first. If Intel or someone was doing it that way it
might give us a little more levelage to push it as being the
"correct" way to do it.

I will switch to the callback, but really don't think we can rely
on this being in DSD yet.

> 
> > > > +static int cs42l43_pin_remove(struct platform_device *pdev)
> > > > +{
> > > > +   pm_runtime_disable(&pdev->dev);
> > >
> > > This is simply wrong order because it's a mix of non-devm_*() followed by
> > > devm_*() calls in the probe.
> > >
> >
> > I had missed there are now devm_pm_runtime calls, I will switch
> > to that. But I would like to understand the wrong order, remove
> > will be called before the devm bits are destroyed and it seems
> > reasonable to disable the pm_runtime before destroying the
> > pinctrl device. What exactly would run in the wrong order here?
> 
> At the ->remove() stage after this call an IRQ can be fired (or on SMP
> systems any other APIs can be called), for example. So, would it be a
> problem to service it with PM disabled?
> 
> But in any case the shuffling ordering like this is prone to subtle
> bugs. I prefer to have strict ordering if there is nothing preventing
> from doing that way.

Yeah happy enough to use devm_ here, just didn't know it existed
and wanted to better understand your concerns as I was having
difficulty seeing the issue.

Thanks,
Charles