Re: [PATCH V3 20/21] pci, acpi: Match PCI config space accessors against platfrom specific quirks.

[Mark Salter]

On Wed, 2016-01-13 at 14:21 +0100, Tomasz Nowicki wrote:
> Some platforms may not be fully compliant with generic set of PCI config
> accessors. For these cases we implement the way to overwrite accessors
> set prior to PCI buses enumeration. Algorithm traverses available quirk
> list, matches against <platform ID (DMI), domain, bus number> tuple and
> returns corresponding accessors. All quirks can be defined using:
> DECLARE_ACPI_MCFG_FIXUP() and keep self contained. Example,
> 
> static const struct dmi_system_id foo_dmi[] = {
> ÂÂÂÂÂÂÂÂ{
> ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ.ident = "<Platform ident string>",
> ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ.callback = <handler>,
> ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ.matches = {
> ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂDMI_MATCH(DMI_SYS_VENDOR, "<system vendor>"),
> ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂDMI_MATCH(DMI_PRODUCT_NAME, "<product name>"),
> ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂDMI_MATCH(DMI_PRODUCT_VERSION, "product version"),
> ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ},
> ÂÂÂÂÂÂÂÂ},
> ÂÂÂÂÂÂÂÂ{ }
> };
> 
> static struct pci_ops foo_ecam_pci_ops = {
> ÂÂÂÂÂÂÂÂ.map_bus = pci_mcfg_dev_base,
> ÂÂÂÂÂÂÂÂ.read = foo_ecam_config_read,
> ÂÂÂÂÂÂÂÂ.write = foo_ecam_config_write,
> };
> DECLARE_ACPI_MCFG_FIXUP(foo_dmi, NULL, &foo_ecam_pci_ops, <domain_nr>, <bus_nr>);
> 
> More custom (non-DMI) matching can be done via an alternative call.
> Note that there is possibility to assign quirk related private data to
> root->sysdata which will be available along read/wriate accessor, example:
> 
> static int boo_match(struct pci_mcfg_fixup *fixup, struct acpi_pci_root *root)
> {
> ÂÂÂÂÂÂÂÂreturn [condition] ? 1 : 0;
> }
> 
> int boo_ecam_config_read(struct pci_bus *bus, unsigned int devfn,
> ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂint where, int size, u32 *val)
> {
> ÂÂÂÂÂÂÂÂstruct acpi_pci_root *root = bus->sysdata;
> ÂÂÂÂÂÂÂÂstruct boo_priv_data *boo_data = root->sysdata;
> 
> ÂÂÂÂÂÂÂÂ[..]
> }
> 
> static struct pci_ops boo_ecam_pci_ops = {
> 	.map_bus = pci_mcfg_dev_base,
> 	.read = boo_ecam_config_read,
> 	.write = boo_ecam_config_write,
> };
> DECLARE_ACPI_MCFG_FIXUP(NULL, boo_match, &boo_ecam_pci_ops, <domain_nr>, <bus_nr>);
> 
> Signed-off-by: Tomasz Nowicki <tn@xxxxxxxxxxxx>
> ---
> Âdrivers/acpi/mcfg.cÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ| 33 +++++++++++++++++++++++++++++++--
> Âinclude/acpi/acpi_bus.hÂÂÂÂÂÂÂÂÂÂÂ|ÂÂ1 +
> Âinclude/asm-generic/vmlinux.lds.h |ÂÂ7 +++++++
> Âinclude/linux/ecam.hÂÂÂÂÂÂÂÂÂÂÂÂÂÂ| 18 ++++++++++++++++++
> Â4 files changed, 57 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/acpi/mcfg.c b/drivers/acpi/mcfg.c
> index dfc2d14..ec5fe7b 100644
> --- a/drivers/acpi/mcfg.c
> +++ b/drivers/acpi/mcfg.c
> @@ -8,6 +8,7 @@
> Â */
> Â
> Â#include <linux/acpi.h>
> +#include <linux/dmi.h>
> Â#include <linux/ecam.h>
> Â#include <linux/pci.h>
> Â#include <linux/pci-acpi.h>
> @@ -34,6 +35,29 @@ int __weak raw_pci_write(unsigned int domain, unsigned int bus,
> Â	return PCIBIOS_DEVICE_NOT_FOUND;
> Â}
> Â
> +extern struct pci_mcfg_fixup __start_acpi_mcfg_fixups[];
> +extern struct pci_mcfg_fixup __end_acpi_mcfg_fixups[];
> +
> +static struct pci_ops *pci_mcfg_check_quirks(struct acpi_pci_root *root)
> +{
> +	struct pci_mcfg_fixup *f;
> +	int bus_num = root->secondary.start;
> +	int domain = root->segment;
> +
> +	/*
> +	Â* First match against PCI topology <domain:bus> then use DMI or
> +	Â* custom match handler.
> +	Â*/
> +	for (f = __start_acpi_mcfg_fixups; f < __end_acpi_mcfg_fixups; f++) {
> +		if ((f->domain == domain || f->domain == PCI_MCFG_DOMAIN_ANY) &&
> +		ÂÂÂÂ(f->bus_num == bus_num || f->bus_num == PCI_MCFG_BUS_ANY) &&
> +		ÂÂÂÂ(f->system ? dmi_check_system(f->system) : 0 ||
> +		ÂÂÂÂÂf->match ? f->match(f, root) : 0))
> +			return f->ops;

I think this would be better as:

		ÂÂÂÂ(f->system ? dmi_check_system(f->system) : 1 &&
		ÂÂÂÂÂf->match ? f->match(f, root) : 1))
			return f->ops;

Otherwise, one has to call dmi_check_system() from f->match() if
access to root is needed.
ÂÂ
> +	}
> +	return NULL;
> +}
> +
> Âvoid __iomem *
> Âpci_mcfg_dev_base(struct pci_bus *bus, unsigned int devfn, int offset)
> Â{
> @@ -56,10 +80,15 @@ static struct pci_ops default_pci_mcfg_ops = {
> Â
> Âstruct pci_ops *pci_mcfg_get_ops(struct acpi_pci_root *root)
> Â{
> +	struct pci_ops *pci_mcfg_ops_quirk;
> +
> Â	/*
> -	Â* TODO: Match against platform specific quirks and return
> -	Â* corresponding PCI config space accessor set.
> +	Â* Match against platform specific quirks and return corresponding
> +	Â* PCI config space accessor set.
> Â	Â*/
> +	pci_mcfg_ops_quirk = pci_mcfg_check_quirks(root);
> +	if (pci_mcfg_ops_quirk)
> +		return pci_mcfg_ops_quirk;
> Â
> Â	return &default_pci_mcfg_ops;
> Â}
> diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
> index ad0a5ff..ea4d2b5 100644
> --- a/include/acpi/acpi_bus.h
> +++ b/include/acpi/acpi_bus.h
> @@ -554,6 +554,7 @@ struct acpi_pci_root {
> Â	struct pci_bus *bus;
> Â	u16 segment;
> Â	struct resource secondary;	/* downstream bus range */
> +	void *sysdata;
> Â
> Â	u32 osc_support_set;	/* _OSC state of support bits */
> Â	u32 osc_control_set;	/* _OSC state of control bits */
> diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
> index c4bd0e2..c93fc97 100644
> --- a/include/asm-generic/vmlinux.lds.h
> +++ b/include/asm-generic/vmlinux.lds.h
> @@ -298,6 +298,13 @@
> Â		VMLINUX_SYMBOL(__end_pci_fixups_suspend_late) = .;	\
> Â	}								\
> Â									\
> +	/* ACPI MCFG quirks */						\
> +	.acpi_fixupÂÂÂÂÂÂÂÂ: AT(ADDR(.acpi_fixup) - LOAD_OFFSET) {	\
> +		VMLINUX_SYMBOL(__start_acpi_mcfg_fixups) = .;		\
> +		*(.acpi_fixup_mcfg)					\
> +		VMLINUX_SYMBOL(__end_acpi_mcfg_fixups) = .;		\
> +	}								\
> +									\
> Â	/* Built-in firmware blobs */					\
> Â	.builtin_fwÂÂÂÂÂÂÂÂ: AT(ADDR(.builtin_fw) - LOAD_OFFSET) {	\
> Â		VMLINUX_SYMBOL(__start_builtin_fw) = .;			\
> diff --git a/include/linux/ecam.h b/include/linux/ecam.h
> index e0f322e..21215be 100644
> --- a/include/linux/ecam.h
> +++ b/include/linux/ecam.h
> @@ -20,6 +20,24 @@ struct pci_mmcfg_region {
> Â	bool hot_added;
> Â};
> Â
> +struct pci_mcfg_fixup {
> +	const struct dmi_system_id *system;
> +	int (*match)(struct pci_mcfg_fixup *, struct acpi_pci_root *);
> +	struct pci_ops *ops;
> +	int domain;
> +	int bus_num;
> +};
> +
> +#define PCI_MCFG_DOMAIN_ANY	-1
> +#define PCI_MCFG_BUS_ANY	-1
> +
> +/* Designate a routine to fix up buggy MCFG */
> +#define DECLARE_ACPI_MCFG_FIXUP(system, match, ops, dom, bus)		\
> +	static const struct pci_mcfg_fixup __mcfg_fixup_##system##dom##bus\
> +	Â__used	__attribute__((__section__(".acpi_fixup_mcfg"),		\
> +				aligned((sizeof(void *))))) =		\
> +	{ system, match, ops, dom, bus };
> +
> Âstruct pci_mmcfg_region *pci_mmconfig_lookup(int segment, int bus);
> Âstruct pci_mmcfg_region *pci_mmconfig_alloc(int segment, int start,
> Â						ÂÂÂint end, u64 addr);