Re: [RFC PATCH V2 1/2] ACPI/PCI: Match PCI config space accessors against platfrom specific ECAM quirks
From: Tomasz Nowicki
Date: Tue Jun 14 2016 - 07:52:31 EST
On 14.06.2016 11:45, Dongdong Liu wrote:
Hi Duc
å 2016/6/14 17:00, Duc Dang åé:
On Mon, Jun 13, 2016 at 10:51 PM, Dongdong Liu
<liudongdong3@xxxxxxxxxx> wrote:
Hi Duc
å 2016/6/14 4:57, Duc Dang åé:
On Mon, Jun 13, 2016 at 8:47 AM, Christopher Covington
<cov@xxxxxxxxxxxxxx> wrote:
Hi Dongdong,
On 06/13/2016 09:02 AM, Dongdong Liu wrote:
diff --git a/drivers/acpi/pci_mcfg.c b/drivers/acpi/pci_mcfg.c
index d3c3e85..49612b3 100644
--- a/drivers/acpi/pci_mcfg.c
+++ b/drivers/acpi/pci_mcfg.c
@@ -22,6 +22,10 @@
#include <linux/kernel.h>
#include <linux/pci.h>
#include <linux/pci-acpi.h>
+#include <linux/pci-ecam.h>
+
+/* Root pointer to the mapped MCFG table */
+static struct acpi_table_mcfg *mcfg_table;
/* Structure to hold entries from the MCFG table */
struct mcfg_entry {
@@ -35,6 +39,38 @@ struct mcfg_entry {
/* List to save mcfg entries */
static LIST_HEAD(pci_mcfg_list);
+extern struct pci_cfg_fixup __start_acpi_mcfg_fixups[];
+extern struct pci_cfg_fixup __end_acpi_mcfg_fixups[];
+
+struct pci_ecam_ops *pci_mcfg_get_ops(struct acpi_pci_root *root)
+{
+ int bus_num = root->secondary.start;
+ int domain = root->segment;
+ struct pci_cfg_fixup *f;
+
+ if (!mcfg_table)
+ return &pci_generic_ecam_ops;
+
+ /*
+ * Match against platform specific quirks and return
corresponding
+ * CAM ops.
+ *
+ * First match against PCI topology <domain:bus> then use
OEM ID
and
+ * OEM revision from MCFG table standard header.
+ */
+ 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) &&
+ (!strncmp(f->oem_id, mcfg_table->header.oem_id,
+ ACPI_OEM_ID_SIZE)) &&
+ (!strncmp(f->oem_table_id,
mcfg_table->header.oem_table_id,
+ ACPI_OEM_TABLE_ID_SIZE)))
This would just be a small convenience, but if the character count
used
here were
min(strlen(f->oem_id), ACPI_OEM_ID_SIZE)
then the parameters to DECLARE_ACPI_MCFG_FIXUP macro could be
substrings
and
wouldn't need to be padded out to the full length.
+ return f->ops;
+ }
+ /* No quirks, use ECAM */
+ return &pci_generic_ecam_ops;
+}
diff --git a/include/linux/pci-acpi.h b/include/linux/pci-acpi.h
index 7d63a66..088a1da 100644
--- a/include/linux/pci-acpi.h
+++ b/include/linux/pci-acpi.h
@@ -25,6 +25,7 @@ static inline acpi_status
pci_acpi_remove_pm_notifier(struct acpi_device *dev)
extern phys_addr_t acpi_pci_root_get_mcfg_addr(acpi_handle
handle);
extern phys_addr_t pci_mcfg_lookup(u16 domain, struct resource
*bus_res);
+extern struct pci_ecam_ops *pci_mcfg_get_ops(struct acpi_pci_root
*root);
static inline acpi_handle acpi_find_root_bridge_handle(struct
pci_dev
*pdev)
{
@@ -72,6 +73,25 @@ struct acpi_pci_root_ops {
int (*prepare_resources)(struct acpi_pci_root_info *info);
};
+struct pci_cfg_fixup {
+ struct pci_ecam_ops *ops;
+ char *oem_id;
+ char *oem_table_id;
+ 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(ops, oem_id, oem_table_id, dom,
bus) \
+ static const struct
pci_cfg_fixup \
+
__mcfg_fixup_##oem_id##oem_table_id##dom##bus \
I'm not entirely sure that this is the right fix--I'm pretty blindly
following a GCC documentation suggestion [1]--but removing the
first two
preprocessor concatenation operators "##" solved the following build
error
for me.
include/linux/pci-acpi.h:90:2: error: pasting "__mcfg_fixup_" and
""QCOM"" does not give a valid preprocessing token
__mcfg_fixup_##oem_id##oem_table_id##dom##bus \
I think the problem is gcc is not happy with quoted string when
processing these tokens
(""QCOM"", the extra "" are added by gcc). So should we not concat
string tokens and
use the fixup definition in v1 of this RFC:
/* Designate a routine to fix up buggy MCFG */
#define DECLARE_ACPI_MCFG_FIXUP(ops, oem_id, rev, dom,
bus) \
static const struct pci_cfg_fixup
__mcfg_fixup_##system##dom##bus\
__used
__attribute__((__section__(".acpi_fixup_mcfg"), \
aligned((sizeof(void *)))))
= \
{ ops, oem_id, rev, dom, bus };
V1 fixup exist the redefinition error when compiling mutiple
DECLARE_ACPI_MCFG_FIXUP
with the same PCI_MCFG_DOMAIN_ANY and PCI_MCFG_BUS_ANY.
#define EFI_ACPI_HISI_OEM_ID "HISI"
#define EFI_ACPI_HISI_D02_OEM_TABLE_ID "HISI-D02"
#define EFI_ACPI_HISI_D03_OEM_TABLE_ID "HISI-D03"
DECLARE_ACPI_MCFG_FIXUP(&hisi_pcie_ecam_ops, EFI_ACPI_HISI_OEM_ID,
EFI_ACPI_HISI_D02_OEM_TABLE_ID, PCI_MCFG_DOMAIN_ANY,
PCI_MCFG_BUS_ANY);
DECLARE_ACPI_MCFG_FIXUP(&hisi_pcie_ecam_ops, EFI_ACPI_HISI_OEM_ID,
EFI_ACPI_HISI_D03_OEM_TABLE_ID, PCI_MCFG_DOMAIN_ANY,
PCI_MCFG_BUS_ANY);
In file included from drivers/pci/host/pcie-hisi-acpi.c:15:0:
include/linux/pci-acpi.h:98:43: error: redefinition of
'__mcfg_fixup_systemPCI_MCFG_DOMAIN_ANYPCI_MCFG_BUS_ANY'
static const struct pci_cfg_fixup
__mcfg_fixup_##system##dom##bus\
^
drivers/pci/host/pcie-hisi-acpi.c:215:1: note: in expansion of macro
'DECLARE_ACPI_MCFG_FIXUP'
DECLARE_ACPI_MCFG_FIXUP(&hisi_pcie_ecam_ops, EFI_ACPI_HISI_OEM_ID,
^
include/linux/pci-acpi.h:98:43: note: previous definition of
'__mcfg_fixup_systemPCI_MCFG_DOMAIN_ANYPCI_MCFG_BUS_ANY' was here
static const struct pci_cfg_fixup
__mcfg_fixup_##system##dom##bus\
^
drivers/pci/host/pcie-hisi-acpi.c:212:1: note: in expansion of macro
'DECLARE_ACPI_MCFG_FIXUP'
V2 fixup can resolve the redefinition error, but need to use macro.
We can see that the name of macro is not replace with it's value in
"__mcfg_fixup_EFI_ACPI_HISI_OEM_IDEFI_ACPI_HISI_D03_OEM_TABLE_IDPCI_MCFG_DOMAIN_ANYPCI_MCFG_BUS_ANY".
Any good idea is appreciated.
Hmmm.
I was testing # op and using min_t to get the min-len when doing
strncmp similar to Chris' suggestion (using min_t avoids type
warnings)
/* Designate a routine to fix up buggy MCFG */
#define DECLARE_ACPI_MCFG_FIXUP(ops, oem_id, oem_table_id, rev, dom,
bus) \
static const struct pci_cfg_fixup
__mcfg_fixup##oem_id##oem_table_id##rev##dom##bus\
__used
__attribute__((__section__(".acpi_fixup_mcfg"), \
aligned((sizeof(void *)))))
= \
{ ops, #oem_id, #oem_table_id, rev, dom, bus };
This should change to { ops, oem_id, oem_table_id, rev, dom, bus };
â#â is not need.
Both solutions are OK.
1. This works when we use macros as OEM ID and OEM table ID:
#define DECLARE_ACPI_MCFG_FIXUP(ops, oem_id, oem_table_id, rev, dom, bus)\
static const struct pci_cfg_fixup \
__mcfg_fixup_##oem_id##oem_table_id##dom##bus \
__used __attribute__((__section__(".acpi_fixup_mcfg"), \
aligned((sizeof(void *))))) = \
{ ops, oem_id, oem_table_id, rev, dom, bus };
#define OEM_ID "XXXXXX"
#define OEM_TABLE_ID "YYYYYYYY"
DECLARE_ACPI_MCFG_FIXUP(&quirk_ops, OEM_ID, OEM_TABLE_ID, 1,
PCI_MCFG_DOMAIN_ANY, PCI_MCFG_BUS_ANY);
2. This works w/o macro which means we need to define OEM ID and OEM as
string w/o quotation marks:
#define DECLARE_ACPI_MCFG_FIXUP(ops, oem_id, oem_table_id, rev, dom, bus)\
static const struct pci_cfg_fixup \
__mcfg_fixup_##oem_id##oem_table_id##dom##bus \
__used __attribute__((__section__(".acpi_fixup_mcfg"), \
aligned((sizeof(void *))))) = \
{ ops, #oem_id, #oem_table_id, rev, dom, bus };
DECLARE_ACPI_MCFG_FIXUP(&quirk_ops, XXXXXX, YYYYYYYY, 1,
PCI_MCFG_DOMAIN_ANY, PCI_MCFG_BUS_ANY);
Personally I think that (2) is better, no need for macro definitions.
Tomasz