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