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

[Jim Quinlan <james.quinlan@broadcom.com>]

On Thu, Nov 11, 2021 at 6:38 PM Krzysztof Wilczyński <kw@linux.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
>