Re: [PATCH v2 1/7] PCI: Allow for indirecting capability registers

["Pali Rohár" <pali@kernel.org>]

On Thursday 10 November 2022 12:50:09 Jonathan Derrick wrote:
> Allow another driver to provide alternative operations when doing
> capability register reads and writes. The intention is to have
> pcie_bridge_emul provide alternate handlers for the Slot Capabilities, Slot
> Control, and Slot Status registers. Alternate handlers can return > 0 if
> unhandled, errno on error, or 0 on success. This could potentially be
> used to handle quirks in a different manner.

I think that this change should not be needed. Controller drivers
pci-mvebu.c and pci-aardvark.c already provides those alternative
operations when doing capability read and write, and it is working
without need to touch pci/access.c file. They directly register
pci_bridge_emul_init() device and then callbacks of this emulated bridge
either touches config space HW registers or provide some emulation layer
(for capabilities which are not provided by HW config space).

This approach (which is already in use) has one big advantage: There is
no need to touch common pci/access.c code, it only modifies code for
specific HW - controller driver which needs that bridge emulator. All
other HW platforms are unaffected / untouched. Whole emulator code is
separated from the core pci access code.

This is just my opinion, maybe Bjorn has different idea. I just wanted
to show how it is implemented in existing drivers.

> Signed-off-by: Jonathan Derrick <jonathan.derrick@linux.dev>
> ---
>  drivers/pci/access.c | 29 +++++++++++++++++++++++++++++
>  include/linux/pci.h  | 11 +++++++++++
>  2 files changed, 40 insertions(+)
> 
> diff --git a/drivers/pci/access.c b/drivers/pci/access.c
> index 708c7529647f..dbfea6824bd4 100644
> --- a/drivers/pci/access.c
> +++ b/drivers/pci/access.c
> @@ -424,6 +424,17 @@ int pcie_capability_read_word(struct pci_dev *dev, int pos, u16 *val)
>  		return ret;
>  	}
>  
> +	if (dev->caps_rw_ops) {
> +		u32 reg;
> +		ret = dev->caps_rw_ops->read(dev, pos, 4, &reg);
> +		if (!ret) {
> +			*val = reg & 0xffff;
> +			return ret;
> +		} else if (ret < 0) {
> +			return ret;
> +		}
> +	}
> +
>  	/*
>  	 * For Functions that do not implement the Slot Capabilities,
>  	 * Slot Status, and Slot Control registers, these spaces must
> @@ -459,6 +470,12 @@ int pcie_capability_read_dword(struct pci_dev *dev, int pos, u32 *val)
>  		return ret;
>  	}
>  
> +	if (dev->caps_rw_ops) {
> +		ret = dev->caps_rw_ops->read(dev, pos, 4, val);
> +		if (ret <= 0)
> +			return ret;
> +	}
> +
>  	if (pci_is_pcie(dev) && pcie_downstream_port(dev) &&
>  	    pos == PCI_EXP_SLTSTA)
>  		*val = PCI_EXP_SLTSTA_PDS;
> @@ -475,6 +492,12 @@ int pcie_capability_write_word(struct pci_dev *dev, int pos, u16 val)
>  	if (!pcie_capability_reg_implemented(dev, pos))
>  		return 0;
>  
> +	if (dev->caps_rw_ops) {
> +		int ret = dev->caps_rw_ops->write(dev, pos, 2, val);
> +		if (ret <= 0)
> +			return ret;
> +	}
> +
>  	return pci_write_config_word(dev, pci_pcie_cap(dev) + pos, val);
>  }
>  EXPORT_SYMBOL(pcie_capability_write_word);
> @@ -487,6 +510,12 @@ int pcie_capability_write_dword(struct pci_dev *dev, int pos, u32 val)
>  	if (!pcie_capability_reg_implemented(dev, pos))
>  		return 0;
>  
> +	if (dev->caps_rw_ops) {
> +		int ret = dev->caps_rw_ops->write(dev, pos, 4, val);
> +		if (ret <= 0)
> +			return ret;
> +	}
> +
>  	return pci_write_config_dword(dev, pci_pcie_cap(dev) + pos, val);
>  }
>  EXPORT_SYMBOL(pcie_capability_write_dword);
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 2bda4a4e47e8..ff47ef83ab38 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -311,6 +311,15 @@ struct pci_vpd {
>  	u8		cap;
>  };
>  
> +/*
> + * Capability reads/write redirect
> + * Returns 0, errno, or > 0 if unhandled
> + */
> +struct caps_rw_ops {
> +	int (*read)(struct pci_dev *dev, int pos, int len, u32 *val);
> +	int (*write)(struct pci_dev *dev, int pos, int len, u32 val);
> +};
> +
>  struct irq_affinity;
>  struct pcie_link_state;
>  struct pci_sriov;
> @@ -523,6 +532,8 @@ struct pci_dev {
>  
>  	/* These methods index pci_reset_fn_methods[] */
>  	u8 reset_methods[PCI_NUM_RESET_METHODS]; /* In priority order */
> +
> +	struct caps_rw_ops *caps_rw_ops;
>  };
>  
>  static inline struct pci_dev *pci_physfn(struct pci_dev *dev)
> -- 
> 2.30.2
>