[06/10] pci: move pinned status bit to pci_dev struct
Commit Message
The bit describing whether the PCI device is currently pinned is stored
in the PCI devres struct. To clean up and simplify the pci-devres API,
it's better if this information is stored in the pci_dev struct, because
it allows for checking that device's pinned-status directly through the
device struct.
This will later permit simplifying pcim_enable_device().
Move the 'pinned' boolean bit to struct pci_dev.
Signed-off-by: Philipp Stanner <pstanner@redhat.com>
---
drivers/pci/devres.c | 14 ++++----------
drivers/pci/pci.h | 1 -
include/linux/pci.h | 1 +
3 files changed, 5 insertions(+), 11 deletions(-)
Comments
On Tue, 2024-01-16 at 23:34 +0200, andy.shevchenko@gmail.com wrote:
> Mon, Jan 15, 2024 at 03:46:17PM +0100, Philipp Stanner kirjoitti:
> > The bit describing whether the PCI device is currently pinned is
> > stored
> > in the PCI devres struct. To clean up and simplify the pci-devres
> > API,
>
> "PCI devres", 'pci-devres', ...
> Shouldn't these (and across entire series) be consistent terms?
> E.g., "PCI devres API".
Yes, we should agree on a canonical term probably.
Probably "PCI devres" is good.
>
> > it's better if this information is stored in the pci_dev struct,
> > because
>
> pci_dev struct --> struct pci_dev
>
> > it allows for checking that device's pinned-status directly through
> > the
> > device struct.
> > This will later permit simplifying pcim_enable_device().
>
> > Move the 'pinned' boolean bit to struct pci_dev.
>
> ...
>
> > u8 pm_cap; /* PM capability offset */
> > unsigned int enabled:1; /* Whether this dev is
> > enabled */
> > + unsigned int pinned:1; /* Whether this dev is
> > pinned */
> > unsigned int imm_ready:1; /* Supports Immediate
> > Readiness */
> > unsigned int pme_support:5; /* Bitmask of states from
> > which PME#
> > can be generated */
>
> First of all, I think it's better to group PM stuff, like
ACK
>
> u8 pm_cap; /* PM capability offset */
> unsigned int pme_support:5; /* Bitmask of states from
> which PME#
> can be generated */
> unsigned int imm_ready:1; /* Supports Immediate
> Readiness */
> unsigned int enabled:1; /* Whether this dev is
> enabled */
> unsigned int pinned:1; /* Whether this dev is pinned
> */
>
> Second, does this layout anyhow related to the HW layout? (For
> example,
> PME bits and their location in some HW register vs. these bitfields)
> If so, but not sure, it might be good to preserve (to some extent)
> the
> order.
As far as I know, one of the greatest weaknesses of C is that neither
Ritchie nor the standard ever said anything about the fields' order.
Hence, every field-order is purely implementation-defined and in no way
portable.
So I can't imagine a bitfield will ever directly mapp to a HW-layout?
The fields order is purely aesthetic / for the human reader.
P.
>
@@ -411,7 +411,7 @@ static void pcim_release(struct device *gendev, void *res)
if (this->restore_intx)
pci_intx(dev, this->orig_intx);
- if (!this->pinned)
+ if (!dev->pinned)
pci_disable_device(dev);
}
@@ -469,18 +469,12 @@ EXPORT_SYMBOL(pcim_enable_device);
* pcim_pin_device - Pin managed PCI device
* @pdev: PCI device to pin
*
- * Pin managed PCI device @pdev. Pinned device won't be disabled on
- * driver detach. @pdev must have been enabled with
- * pcim_enable_device().
+ * Pin managed PCI device @pdev. Pinned device won't be disabled on driver
+ * detach. @pdev must have been enabled with pcim_enable_device().
*/
void pcim_pin_device(struct pci_dev *pdev)
{
- struct pci_devres *dr;
-
- dr = find_pci_dr(pdev);
- WARN_ON(!dr || !pdev->enabled);
- if (dr)
- dr->pinned = 1;
+ pdev->pinned = true;
}
EXPORT_SYMBOL(pcim_pin_device);
@@ -809,7 +809,6 @@ static inline pci_power_t mid_pci_get_power_state(struct pci_dev *pdev)
* when a device is enabled using managed PCI device enable interface.
*/
struct pci_devres {
- unsigned int pinned:1;
unsigned int orig_intx:1;
unsigned int restore_intx:1;
unsigned int mwi:1;
@@ -368,6 +368,7 @@ struct pci_dev {
functional, and D3 being off. */
u8 pm_cap; /* PM capability offset */
unsigned int enabled:1; /* Whether this dev is enabled */
+ unsigned int pinned:1; /* Whether this dev is pinned */
unsigned int imm_ready:1; /* Supports Immediate Readiness */
unsigned int pme_support:5; /* Bitmask of states from which PME#
can be generated */