linux-kernel - Re: [PATCH v8 5/8] PCI/portdrv: add mechanism to turn on subdev regulators

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: <CA+-6iNzyduQskgfEkQMiU15N8f2-gRAoqx=eNfYtOEQiV38SAg@mail.gmail.com>
Date:   Mon, 15 Nov 2021 15:26:22 -0500
From:   Jim Quinlan <james.quinlan@...adcom.com>
To:     Krzysztof Wilczyński <kw@...ux.com>
Cc:     Jim Quinlan <jim2101024@...il.com>,
        "open list:PCI NATIVE HOST BRIDGE AND ENDPOINT DRIVERS" 
        <linux-pci@...r.kernel.org>, Bjorn Helgaas <bhelgaas@...gle.com>,
        Nicolas Saenz Julienne <nsaenz@...nel.org>,
        Rob Herring <robh@...nel.org>, Mark Brown <broonie@...nel.org>,
        "maintainer:BROADCOM BCM7XXX ARM ARCHITECTURE" 
        <bcm-kernel-feedback-list@...adcom.com>,
        Sean V Kelley <sean.v.kelley@...el.com>,
        Jonathan Cameron <Jonathan.Cameron@...wei.com>,
        Qiuxu Zhuo <qiuxu.zhuo@...el.com>,
        Keith Busch <kbusch@...nel.org>,
        open list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v8 5/8] PCI/portdrv: add mechanism to turn on subdev regulators

On Thu, Nov 11, 2021 at 6:38 PM Krzysztof Wilczyński <kw@...ux.com> wrote:
>
> Hi Jim,
>
> [...]
> > [1] These regulators typically govern the actual power supply to the
> >     endpoint chip.  Sometimes they may be a the official PCIe socket
>
> In the above, did you mean to say "be at the"?
Yep.
>
> > +static void *alloc_subdev_regulators(struct device *dev)
> > +{
> > +     static const char * const supplies[] = {
> > +             "vpcie3v3",
> > +             "vpcie3v3aux",
> > +             "vpcie12v",
> > +     };
> > +     const size_t size = sizeof(struct subdev_regulators)
> > +             + sizeof(struct regulator_bulk_data) * ARRAY_SIZE(supplies);
>
> [...]
> > +int pci_subdev_regulators_add_bus(struct pci_bus *bus)
> > +{
> > +     struct device *dev = &bus->dev;
> > +     struct subdev_regulators *sr;
> > +     int ret;
> > +
> > +     if (!pcie_is_port_dev(bus->self))
> > +             return 0;
> > +
> > +     if (WARN_ON(bus->dev.driver_data))
> > +             dev_err(dev, "multiple clients using dev.driver_data\n");
>
> I have to ask - is the WARN_ON() above adding value given the nature of the
> error?  Would dumping a stack be of interest to someone?
Hello Krzysztof,

It doesn't need to be a warning.  You are right, the backtrace will
not help anyone figure out how to fix the problem.

>
> Having said that, why do we even need to assert this?  Can there be some
> sort of a race condition with access happening here?
This commit-set is claiming the driver_data field of the PCIe port
device and I am concerned  that something else in the future would
unknowingly do the same.  It would not be a race, just two separate
pieces of code stomping on the same variable.  If I am over-worrying I
can use a dev_err or nothing at all.


>
> I am asking as pci_subdev_regulators_remove_bus() does not seem to be
> concerned about this sort of thing yet it also accesses the same driver
> data, and such.
Yes, but when pci_subdev_regulators_remove_bus() accesses the port
driver driver_data and it is non-NULL  it  does not  know whether it
is the expected pointer or something else.

>
> [...]
> > +/* forward declaration */
> > +static struct pci_driver pcie_portdriver;
>
> The comment above might not be needed as it's quite obvious what the code
> at this line is for, I believe.
Okay.
>
> [...]
> > @@ -131,6 +155,13 @@ static int pcie_portdrv_probe(struct pci_dev *dev,
> >       if (status)
> >               return status;
> >
> > +     if (dev->bus->ops &&
> > +         dev->bus->ops->add_bus &&
> > +         dev->bus->dev.driver_data) {
> > +             pcie_portdriver.resume = subdev_regulator_resume;
> > +             pcie_portdriver.suspend = subdev_regulator_suspend;
> > +     }
> > +
> >       pci_save_state(dev);
>
> [...]
> > @@ -237,6 +268,7 @@ static struct pci_driver pcie_portdriver = {
> >       .err_handler    = &pcie_portdrv_err_handler,
> >
> >       .driver.pm      = PCIE_PORTDRV_PM_OPS,
> > +     /* Note: suspend and resume may be set during probe */
>
> This comment here is for the "driver.pm" line above, correct?  If so, then
> I would move it above the statement.  It's a little bit confusing
> otherwise.
I'm planning to remove this comment and the code that sets
pcie_portdriver.{resume,suspend} and instead put this code into the
int pcie_port_device_{suspend,remove}() functions.

Regards,
Jim Quinlan
Broadcom STB

>
>         Krzysztof
>