On Wed, 2024-06-05 at 16:11 -0500, Bjorn Helgaas wrote: > On Wed, Jun 05, 2024 at 10:15:59AM +0200, Philipp Stanner wrote: > > The bit describing whether the PCI device is currently enabled is > > stored > > in struct pci_devres. Besides this struct being subject of a > > cleanup > > process, struct pci_device is in general the right place to store > > this > > information, since it is not devres-specific. > > > > Move the 'enabled' boolean bit to struct pci_dev. > > I think this (and the similar 'pinned' patch) appeared in v6. Yes. This patch and its brothers serve to remove members from struct pci_devres step by step, so it can ultimately be removed, so that we won't have a generic devres struct anymore, but actual resource-specific structs. > > It sounds plausible to have this in struct pci_dev, but it's > confusing > to have both: > > pci_dev.enabled > pci_dev.enable_cnt, used by pci_is_enabled() > > I haven't looked hard enough to see whether both are required. If > they are, I think we should rename "enabled" to something descriptive > enough to make it obviously different from "enable_cnt". I took a look at it and I think we can actually drop "enabled" and use "enable_cnt" for everything. That would even simplify things more, I'd say. Let me provide that in v8. P. > > > Signed-off-by: Philipp Stanner <pstanner@xxxxxxxxxx> > > --- > > drivers/pci/devres.c | 11 ++++------- > > drivers/pci/pci.c | 17 ++++++++++------- > > drivers/pci/pci.h | 1 - > > include/linux/pci.h | 1 + > > 4 files changed, 15 insertions(+), 15 deletions(-) > > > > diff --git a/drivers/pci/devres.c b/drivers/pci/devres.c > > index 572a4e193879..ea590caf8995 100644 > > --- a/drivers/pci/devres.c > > +++ b/drivers/pci/devres.c > > @@ -398,7 +398,7 @@ static void pcim_release(struct device *gendev, > > void *res) > > if (this->restore_intx) > > pci_intx(dev, this->orig_intx); > > > > - if (this->enabled && !this->pinned) > > + if (!this->pinned) > > pci_disable_device(dev); > > } > > > > @@ -441,14 +441,11 @@ int pcim_enable_device(struct pci_dev *pdev) > > dr = get_pci_dr(pdev); > > if (unlikely(!dr)) > > return -ENOMEM; > > - if (dr->enabled) > > - return 0; > > > > rc = pci_enable_device(pdev); > > - if (!rc) { > > + if (!rc) > > pdev->is_managed = 1; > > - dr->enabled = 1; > > - } > > + > > return rc; > > } > > EXPORT_SYMBOL(pcim_enable_device); > > @@ -466,7 +463,7 @@ void pcim_pin_device(struct pci_dev *pdev) > > struct pci_devres *dr; > > > > dr = find_pci_dr(pdev); > > - WARN_ON(!dr || !dr->enabled); > > + WARN_ON(!dr || !pdev->enabled); > > if (dr) > > dr->pinned = 1; > > } > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c > > index 8dd711b9a291..04accdfab7ce 100644 > > --- a/drivers/pci/pci.c > > +++ b/drivers/pci/pci.c > > @@ -2011,6 +2011,9 @@ static int do_pci_enable_device(struct > > pci_dev *dev, int bars) > > u16 cmd; > > u8 pin; > > > > + if (dev->enabled) > > + return 0; > > + > > err = pci_set_power_state(dev, PCI_D0); > > if (err < 0 && err != -EIO) > > return err; > > @@ -2025,7 +2028,7 @@ static int do_pci_enable_device(struct > > pci_dev *dev, int bars) > > pci_fixup_device(pci_fixup_enable, dev); > > > > if (dev->msi_enabled || dev->msix_enabled) > > - return 0; > > + goto success_out; > > > > pci_read_config_byte(dev, PCI_INTERRUPT_PIN, &pin); > > if (pin) { > > @@ -2035,6 +2038,8 @@ static int do_pci_enable_device(struct > > pci_dev *dev, int bars) > > cmd & > > ~PCI_COMMAND_INTX_DISABLE); > > } > > > > +success_out: > > + dev->enabled = true; > > return 0; > > } > > > > @@ -2193,6 +2198,9 @@ static void do_pci_disable_device(struct > > pci_dev *dev) > > { > > u16 pci_command; > > > > + if (!dev->enabled) > > + return; > > + > > pci_read_config_word(dev, PCI_COMMAND, &pci_command); > > if (pci_command & PCI_COMMAND_MASTER) { > > pci_command &= ~PCI_COMMAND_MASTER; > > @@ -2200,6 +2208,7 @@ static void do_pci_disable_device(struct > > pci_dev *dev) > > } > > > > pcibios_disable_device(dev); > > + dev->enabled = false; > > } > > > > /** > > @@ -2227,12 +2236,6 @@ void pci_disable_enabled_device(struct > > pci_dev *dev) > > */ > > void pci_disable_device(struct pci_dev *dev) > > { > > - struct pci_devres *dr; > > - > > - dr = find_pci_dr(dev); > > - if (dr) > > - dr->enabled = 0; > > - > > dev_WARN_ONCE(&dev->dev, atomic_read(&dev->enable_cnt) <= > > 0, > > "disabling already-disabled device"); > > > > diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h > > index 9fd50bc99e6b..e223e0f7dada 100644 > > --- a/drivers/pci/pci.h > > +++ b/drivers/pci/pci.h > > @@ -823,7 +823,6 @@ static inline pci_power_t > > mid_pci_get_power_state(struct pci_dev *pdev) > > * then remove them from here. > > */ > > struct pci_devres { > > - unsigned int enabled:1; > > unsigned int pinned:1; > > unsigned int orig_intx:1; > > unsigned int restore_intx:1; > > diff --git a/include/linux/pci.h b/include/linux/pci.h > > index 16493426a04f..110548f00b3b 100644 > > --- a/include/linux/pci.h > > +++ b/include/linux/pci.h > > @@ -367,6 +367,7 @@ struct pci_dev { > > this is D0-D3, D0 being > > fully > > functional, and D3 being > > off. */ > > u8 pm_cap; /* PM capability offset */ > > + unsigned int enabled:1; /* Whether this dev is > > enabled */ > > unsigned int imm_ready:1; /* Supports Immediate > > Readiness */ > > unsigned int pme_support:5; /* Bitmask of states from > > which PME# > > can be generated */ > > -- > > 2.45.0 > > >