linux-kernel - Re: [PATCH v2 4/8] PCI: Change the parent to correctly represent pcie hierarchy

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: <CAMRc=Mc3J_CRHSsU1ZowJxrx6V3Uici6iuJtTfR63Wt3xLrqAg@mail.gmail.com>
Date: Tue, 6 Aug 2024 22:06:00 +0200
From: Bartosz Golaszewski <brgl@...ev.pl>
To: Bjorn Helgaas <helgaas@...nel.org>
Cc: Krishna chaitanya chundru <quic_krichai@...cinc.com>, 
	Bartosz Golaszewski <bartosz.golaszewski@...aro.org>, Lorenzo Pieralisi <lpieralisi@...nel.org>, 
	Krzysztof Wilczyński <kw@...ux.com>, 
	Rob Herring <robh@...nel.org>, Bjorn Helgaas <bhelgaas@...gle.com>, 
	Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>, 
	Konrad Dybcio <konrad.dybcio@...aro.org>, cros-qcom-dts-watchers@...omium.org, 
	Jingoo Han <jingoohan1@...il.com>, 
	Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>, andersson@...nel.org, 
	quic_vbadigan@...cinc.com, linux-arm-msm@...r.kernel.org, 
	linux-pci@...r.kernel.org, devicetree@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 4/8] PCI: Change the parent to correctly represent pcie hierarchy

On Tue, Aug 6, 2024 at 9:07 PM Bjorn Helgaas <helgaas@...nel.org> wrote:
>
> On Sat, Aug 03, 2024 at 08:52:50AM +0530, Krishna chaitanya chundru wrote:
> > Currently the pwrctl driver is child of pci-pci bridge driver,
> > this will cause issue when suspend resume is introduced in the pwr
> > control driver. If the supply is removed to the endpoint in the
> > power control driver then the config space access by the
> > pci-pci bridge driver can cause issues like Timeouts.
>
> If "pci-pci bridge driver" refers to portdrv, please use "portdrv" to
> avoid confusion.
>
> Can you be a little more specific about config accesses by the bridge
> driver?  Generally portdrv wouldn't touch devices below the bridge.
> It sounds like you've tripped over something here, so you probably
> have an example of a timeout.
>
> s/pcie/PCIe/ in subject, although it'd be nice if the whole subject
> could be a little more specific.  I don't think pwrctl is directly
> part of the PCIe hierarchy, so I don't quite understand what you're
> saying there.
>
> > For this reason change the parent to controller from pci-pci bridge.
> >
> > Fixes: 4565d2652a37 ("PCI/pwrctl: Add PCI power control core code")
>
> Will need an ack from Bartosz, of course, since he added this.  Moved
> from cc: to to: list to make sure he sees this.
>

I would drop the Fixes tag altogether. This is a change in
implementation but it doesn't really fix a bug or regression.

Other than that: please feel free to add

Acked-by: Bartosz Golaszewski <bartosz.golaszewski@...aro.org>

I will also review the pwrctl part of the series shortly.

Bart

> > Signed-off-by: Krishna chaitanya chundru <quic_krichai@...cinc.com>
> > ---
> >  drivers/pci/bus.c         | 3 ++-
> >  drivers/pci/pwrctl/core.c | 9 ++++++++-
> >  2 files changed, 10 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c
> > index 55c853686051..15b42f0f588f 100644
> > --- a/drivers/pci/bus.c
> > +++ b/drivers/pci/bus.c
> > @@ -328,6 +328,7 @@ void __weak pcibios_bus_add_device(struct pci_dev *pdev) { }
> >   */
> >  void pci_bus_add_device(struct pci_dev *dev)
> >  {
> > +     struct pci_host_bridge *host = pci_find_host_bridge(dev->bus);
> >       struct device_node *dn = dev->dev.of_node;
> >       int retval;
> >
> > @@ -352,7 +353,7 @@ void pci_bus_add_device(struct pci_dev *dev)
> >
> >       if (dev_of_node(&dev->dev) && pci_is_bridge(dev)) {
> >               retval = of_platform_populate(dev_of_node(&dev->dev), NULL, NULL,
> > -                                           &dev->dev);
> > +                                           host->dev.parent);
>
> I'm not sure host->dev.parent is always valid.  There are
> pci_create_root_bus() callers that supply a NULL parent pointer.
>
> >               if (retval)
> >                       pci_err(dev, "failed to populate child OF nodes (%d)\n",
> >                               retval);
> > diff --git a/drivers/pci/pwrctl/core.c b/drivers/pci/pwrctl/core.c
> > index feca26ad2f6a..4f2ffa0b0a5f 100644
> > --- a/drivers/pci/pwrctl/core.c
> > +++ b/drivers/pci/pwrctl/core.c
> > @@ -11,6 +11,8 @@
> >  #include <linux/property.h>
> >  #include <linux/slab.h>
> >
> > +#include "../pci.h"
> > +
> >  static int pci_pwrctl_notify(struct notifier_block *nb, unsigned long action,
> >                            void *data)
> >  {
> > @@ -64,18 +66,23 @@ static int pci_pwrctl_notify(struct notifier_block *nb, unsigned long action,
> >   */
> >  int pci_pwrctl_device_set_ready(struct pci_pwrctl *pwrctl)
> >  {
> > +     struct pci_bus *bus;
> >       int ret;
> >
> >       if (!pwrctl->dev)
> >               return -ENODEV;
> >
> > +     bus = pci_find_bus(of_get_pci_domain_nr(pwrctl->dev->parent->of_node), 0);
> > +     if (!bus)
> > +             return -ENODEV;
> > +
> >       pwrctl->nb.notifier_call = pci_pwrctl_notify;
> >       ret = bus_register_notifier(&pci_bus_type, &pwrctl->nb);
> >       if (ret)
> >               return ret;
> >
> >       pci_lock_rescan_remove();
> > -     pci_rescan_bus(to_pci_dev(pwrctl->dev->parent)->bus);
> > +     pci_rescan_bus(bus);
> >       pci_unlock_rescan_remove();
> >
> >       return 0;
> >
> > --
> > 2.34.1
> >