| Message ID | 20240605081605.18769-9-pstanner@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | Make PCI's devres API more consistent | expand |
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. 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". > Signed-off-by: Philipp Stanner <pstanner@redhat.com> > --- > 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 >
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@redhat.com> > > --- > > 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 > > >
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 */
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. Signed-off-by: Philipp Stanner <pstanner@redhat.com> --- 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(-)