* [RFC 1/9] PCI: pci-bridge-emul: Update PCIe register behaviors
2020-02-07 23:59 [RFC 0/9] PCIe Hotplug Slot Emulation driver Jon Derrick
@ 2020-02-07 23:59 ` Jon Derrick
2020-02-08 9:55 ` Andy Shevchenko
2020-03-28 21:42 ` Bjorn Helgaas
2020-02-08 0:00 ` [RFC 2/9] PCI: pci-bridge-emul: Eliminate reserved member Jon Derrick
` (9 subsequent siblings)
10 siblings, 2 replies; 21+ messages in thread
From: Jon Derrick @ 2020-02-07 23:59 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: linux-pci, linux-kernel, Andy Shevchenko, Mika Westerberg,
Pawel Baldysiak, Sinan Kaya, Lorenzo Pieralisi, Keith Busch,
Alexandru Gagniuc, Christoph Hellwig, Jon Derrick
Update the PCIe register behaviors and comments for PCIe v5.0.
Replace the specific bit definitions with BIT and GENMASK to make
updating easier in the future.
Signed-off-by: Jon Derrick <jonathan.derrick@intel.com>
---
drivers/pci/pci-bridge-emul.c | 54 +++++++++++++++++++++----------------------
1 file changed, 27 insertions(+), 27 deletions(-)
diff --git a/drivers/pci/pci-bridge-emul.c b/drivers/pci/pci-bridge-emul.c
index fffa770..d065c2a 100644
--- a/drivers/pci/pci-bridge-emul.c
+++ b/drivers/pci/pci-bridge-emul.c
@@ -191,12 +191,12 @@ struct pci_bridge_reg_behavior {
.rw = GENMASK(15, 0),
/*
- * Device status register has 4 bits W1C, then 2 bits
- * RO, the rest is reserved
+ * Device status register has bits 6 and [3:0] W1C, [5:4] RO,
+ * the rest is reserved
*/
- .w1c = GENMASK(19, 16),
- .ro = GENMASK(20, 19),
- .rsvd = GENMASK(31, 21),
+ .w1c = (BIT(6) | GENMASK(3, 0)) << 16,
+ .ro = GENMASK(5, 4) << 16,
+ .rsvd = GENMASK(15, 7) << 16,
},
[PCI_EXP_LNKCAP / 4] = {
@@ -207,15 +207,16 @@ struct pci_bridge_reg_behavior {
[PCI_EXP_LNKCTL / 4] = {
/*
- * Link control has bits [1:0] and [11:3] RW, the
- * other bits are reserved.
- * Link status has bits [13:0] RO, and bits [14:15]
+ * Link control has bits [15:14], [11:3] and [1:0] RW, the
+ * rest is reserved.
+ *
+ * Link status has bits [13:0] RO, and bits [15:14]
* W1C.
*/
- .rw = GENMASK(11, 3) | GENMASK(1, 0),
+ .rw = GENMASK(15, 14) | GENMASK(11, 3) | GENMASK(1, 0),
.ro = GENMASK(13, 0) << 16,
.w1c = GENMASK(15, 14) << 16,
- .rsvd = GENMASK(15, 12) | BIT(2),
+ .rsvd = GENMASK(13, 12) | BIT(2),
},
[PCI_EXP_SLTCAP / 4] = {
@@ -224,19 +225,16 @@ struct pci_bridge_reg_behavior {
[PCI_EXP_SLTCTL / 4] = {
/*
- * Slot control has bits [12:0] RW, the rest is
+ * Slot control has bits [14:0] RW, the rest is
* reserved.
*
- * Slot status has a mix of W1C and RO bits, as well
- * as reserved bits.
+ * Slot status has bits 8 and [4:0] W1C, bits [7:5] RO, the
+ * rest is reserved.
*/
- .rw = GENMASK(12, 0),
- .w1c = (PCI_EXP_SLTSTA_ABP | PCI_EXP_SLTSTA_PFD |
- PCI_EXP_SLTSTA_MRLSC | PCI_EXP_SLTSTA_PDC |
- PCI_EXP_SLTSTA_CC | PCI_EXP_SLTSTA_DLLSC) << 16,
- .ro = (PCI_EXP_SLTSTA_MRLSS | PCI_EXP_SLTSTA_PDS |
- PCI_EXP_SLTSTA_EIS) << 16,
- .rsvd = GENMASK(15, 12) | (GENMASK(15, 9) << 16),
+ .rw = GENMASK(14, 0),
+ .w1c = (BIT(8) | GENMASK(4, 0)) << 16,
+ .ro = GENMASK(7, 5) << 16,
+ .rsvd = BIT(15) | (GENMASK(15, 9) << 16),
},
[PCI_EXP_RTCTL / 4] = {
@@ -244,18 +242,20 @@ struct pci_bridge_reg_behavior {
* Root control has bits [4:0] RW, the rest is
* reserved.
*
- * Root status has bit 0 RO, the rest is reserved.
+ * Root capabilities has bit 0 RO, the rest is reserved.
*/
- .rw = (PCI_EXP_RTCTL_SECEE | PCI_EXP_RTCTL_SENFEE |
- PCI_EXP_RTCTL_SEFEE | PCI_EXP_RTCTL_PMEIE |
- PCI_EXP_RTCTL_CRSSVE),
- .ro = PCI_EXP_RTCAP_CRSVIS << 16,
+ .rw = GENMASK(4, 0),
+ .ro = BIT(0) << 16,
.rsvd = GENMASK(15, 5) | (GENMASK(15, 1) << 16),
},
[PCI_EXP_RTSTA / 4] = {
- .ro = GENMASK(15, 0) | PCI_EXP_RTSTA_PENDING,
- .w1c = PCI_EXP_RTSTA_PME,
+ /*
+ * Root status has bits 17 and [15:0] RO, bit 16 W1C, the rest
+ * is reserved.
+ */
+ .ro = BIT(17) | GENMASK(15, 0),
+ .w1c = BIT(16),
.rsvd = GENMASK(31, 18),
},
};
--
1.8.3.1
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [RFC 1/9] PCI: pci-bridge-emul: Update PCIe register behaviors
2020-02-07 23:59 ` [RFC 1/9] PCI: pci-bridge-emul: Update PCIe register behaviors Jon Derrick
@ 2020-02-08 9:55 ` Andy Shevchenko
2020-03-28 21:42 ` Bjorn Helgaas
1 sibling, 0 replies; 21+ messages in thread
From: Andy Shevchenko @ 2020-02-08 9:55 UTC (permalink / raw)
To: Jon Derrick
Cc: Bjorn Helgaas, linux-pci, Linux Kernel Mailing List,
Andy Shevchenko, Mika Westerberg, Pawel Baldysiak, Sinan Kaya,
Lorenzo Pieralisi, Keith Busch, Alexandru Gagniuc,
Christoph Hellwig
On Sat, Feb 8, 2020 at 2:02 AM Jon Derrick <jonathan.derrick@intel.com> wrote:
>
> Update the PCIe register behaviors and comments for PCIe v5.0.
I think you may elaborate the changes here, like a summary.
> Replace the specific bit definitions with BIT and GENMASK to make
> updating easier in the future.
...
> + * Device status register has bits 6 and [3:0] W1C, [5:4] RO,
> + * the rest is reserved
Perhaps it makes sense to add '.' (period) to the end of sentence. And
the same for the rest comments.
...
> - .w1c = (PCI_EXP_SLTSTA_ABP | PCI_EXP_SLTSTA_PFD |
> - PCI_EXP_SLTSTA_MRLSC | PCI_EXP_SLTSTA_PDC |
> - PCI_EXP_SLTSTA_CC | PCI_EXP_SLTSTA_DLLSC) << 16,
> - .ro = (PCI_EXP_SLTSTA_MRLSS | PCI_EXP_SLTSTA_PDS |
> - PCI_EXP_SLTSTA_EIS) << 16,
> + .w1c = (BIT(8) | GENMASK(4, 0)) << 16,
> + .ro = GENMASK(7, 5) << 16,
I'm not sure the new one is better. Perhaps you need to add
description for the new bits and, if you consider it's needed, add a
picture of the bits in the comment, like XXXX rwX1 XXrX XwXX, where r
= ro, w = rw, 1 = w1c, X = rsvd. But it's up to Bjorn and you, I have
no strong opinion here.
Same for the rest similar changes.
...
> - .rw = (PCI_EXP_RTCTL_SECEE | PCI_EXP_RTCTL_SENFEE |
> - PCI_EXP_RTCTL_SEFEE | PCI_EXP_RTCTL_PMEIE |
> - PCI_EXP_RTCTL_CRSSVE),
> - .ro = PCI_EXP_RTCAP_CRSVIS << 16,
> + .rw = GENMASK(4, 0),
> + .ro = BIT(0) << 16,
Bit 0 in both rw and ro -- is it correct?
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [RFC 1/9] PCI: pci-bridge-emul: Update PCIe register behaviors
2020-02-07 23:59 ` [RFC 1/9] PCI: pci-bridge-emul: Update PCIe register behaviors Jon Derrick
2020-02-08 9:55 ` Andy Shevchenko
@ 2020-03-28 21:42 ` Bjorn Helgaas
1 sibling, 0 replies; 21+ messages in thread
From: Bjorn Helgaas @ 2020-03-28 21:42 UTC (permalink / raw)
To: Jon Derrick
Cc: linux-pci, linux-kernel, Andy Shevchenko, Mika Westerberg,
Pawel Baldysiak, Sinan Kaya, Lorenzo Pieralisi, Keith Busch,
Alexandru Gagniuc, Christoph Hellwig, Thomas Petazzoni,
Russell King
[+cc Thomas, Russell]
On Fri, Feb 07, 2020 at 04:59:59PM -0700, Jon Derrick wrote:
> Update the PCIe register behaviors and comments for PCIe v5.0.
> Replace the specific bit definitions with BIT and GENMASK to make
> updating easier in the future.
I think this patch makes sense on its own, independent of the rest of
the series. I *would* like to see it split into (a) a "no changes"
patch that converts to BIT/GENMASK, and (b) another patch that
contains only the PCIe r5.0 updates.
> Signed-off-by: Jon Derrick <jonathan.derrick@intel.com>
> ---
> drivers/pci/pci-bridge-emul.c | 54 +++++++++++++++++++++----------------------
> 1 file changed, 27 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/pci/pci-bridge-emul.c b/drivers/pci/pci-bridge-emul.c
> index fffa770..d065c2a 100644
> --- a/drivers/pci/pci-bridge-emul.c
> +++ b/drivers/pci/pci-bridge-emul.c
> @@ -191,12 +191,12 @@ struct pci_bridge_reg_behavior {
> .rw = GENMASK(15, 0),
>
> /*
> - * Device status register has 4 bits W1C, then 2 bits
> - * RO, the rest is reserved
> + * Device status register has bits 6 and [3:0] W1C, [5:4] RO,
> + * the rest is reserved
> */
> - .w1c = GENMASK(19, 16),
> - .ro = GENMASK(20, 19),
> - .rsvd = GENMASK(31, 21),
> + .w1c = (BIT(6) | GENMASK(3, 0)) << 16,
> + .ro = GENMASK(5, 4) << 16,
> + .rsvd = GENMASK(15, 7) << 16,
> },
>
> [PCI_EXP_LNKCAP / 4] = {
> @@ -207,15 +207,16 @@ struct pci_bridge_reg_behavior {
>
> [PCI_EXP_LNKCTL / 4] = {
> /*
> - * Link control has bits [1:0] and [11:3] RW, the
> - * other bits are reserved.
> - * Link status has bits [13:0] RO, and bits [14:15]
> + * Link control has bits [15:14], [11:3] and [1:0] RW, the
> + * rest is reserved.
> + *
> + * Link status has bits [13:0] RO, and bits [15:14]
> * W1C.
> */
> - .rw = GENMASK(11, 3) | GENMASK(1, 0),
> + .rw = GENMASK(15, 14) | GENMASK(11, 3) | GENMASK(1, 0),
> .ro = GENMASK(13, 0) << 16,
> .w1c = GENMASK(15, 14) << 16,
> - .rsvd = GENMASK(15, 12) | BIT(2),
> + .rsvd = GENMASK(13, 12) | BIT(2),
> },
>
> [PCI_EXP_SLTCAP / 4] = {
> @@ -224,19 +225,16 @@ struct pci_bridge_reg_behavior {
>
> [PCI_EXP_SLTCTL / 4] = {
> /*
> - * Slot control has bits [12:0] RW, the rest is
> + * Slot control has bits [14:0] RW, the rest is
> * reserved.
> *
> - * Slot status has a mix of W1C and RO bits, as well
> - * as reserved bits.
> + * Slot status has bits 8 and [4:0] W1C, bits [7:5] RO, the
> + * rest is reserved.
> */
> - .rw = GENMASK(12, 0),
> - .w1c = (PCI_EXP_SLTSTA_ABP | PCI_EXP_SLTSTA_PFD |
> - PCI_EXP_SLTSTA_MRLSC | PCI_EXP_SLTSTA_PDC |
> - PCI_EXP_SLTSTA_CC | PCI_EXP_SLTSTA_DLLSC) << 16,
> - .ro = (PCI_EXP_SLTSTA_MRLSS | PCI_EXP_SLTSTA_PDS |
> - PCI_EXP_SLTSTA_EIS) << 16,
> - .rsvd = GENMASK(15, 12) | (GENMASK(15, 9) << 16),
> + .rw = GENMASK(14, 0),
> + .w1c = (BIT(8) | GENMASK(4, 0)) << 16,
> + .ro = GENMASK(7, 5) << 16,
> + .rsvd = BIT(15) | (GENMASK(15, 9) << 16),
> },
>
> [PCI_EXP_RTCTL / 4] = {
> @@ -244,18 +242,20 @@ struct pci_bridge_reg_behavior {
> * Root control has bits [4:0] RW, the rest is
> * reserved.
> *
> - * Root status has bit 0 RO, the rest is reserved.
> + * Root capabilities has bit 0 RO, the rest is reserved.
> */
> - .rw = (PCI_EXP_RTCTL_SECEE | PCI_EXP_RTCTL_SENFEE |
> - PCI_EXP_RTCTL_SEFEE | PCI_EXP_RTCTL_PMEIE |
> - PCI_EXP_RTCTL_CRSSVE),
> - .ro = PCI_EXP_RTCAP_CRSVIS << 16,
> + .rw = GENMASK(4, 0),
> + .ro = BIT(0) << 16,
> .rsvd = GENMASK(15, 5) | (GENMASK(15, 1) << 16),
> },
>
> [PCI_EXP_RTSTA / 4] = {
> - .ro = GENMASK(15, 0) | PCI_EXP_RTSTA_PENDING,
> - .w1c = PCI_EXP_RTSTA_PME,
> + /*
> + * Root status has bits 17 and [15:0] RO, bit 16 W1C, the rest
> + * is reserved.
> + */
> + .ro = BIT(17) | GENMASK(15, 0),
> + .w1c = BIT(16),
> .rsvd = GENMASK(31, 18),
> },
> };
> --
> 1.8.3.1
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* [RFC 2/9] PCI: pci-bridge-emul: Eliminate reserved member
2020-02-07 23:59 [RFC 0/9] PCIe Hotplug Slot Emulation driver Jon Derrick
2020-02-07 23:59 ` [RFC 1/9] PCI: pci-bridge-emul: Update PCIe register behaviors Jon Derrick
@ 2020-02-08 0:00 ` Jon Derrick
2020-03-28 21:43 ` Bjorn Helgaas
2020-02-08 0:00 ` [RFC 3/9] PCI: pci-bridge-emul: Provide a helper to set behavior Jon Derrick
` (8 subsequent siblings)
10 siblings, 1 reply; 21+ messages in thread
From: Jon Derrick @ 2020-02-08 0:00 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: linux-pci, linux-kernel, Andy Shevchenko, Mika Westerberg,
Pawel Baldysiak, Sinan Kaya, Lorenzo Pieralisi, Keith Busch,
Alexandru Gagniuc, Christoph Hellwig, Jon Derrick
Assume any bit not in the Read-Only, Read-Write, or Write-1-to-Clear
behavior masks is a Reserved bit and should return 0 on reads.
Signed-off-by: Jon Derrick <jonathan.derrick@intel.com>
---
drivers/pci/pci-bridge-emul.c | 28 +++++++++++-----------------
1 file changed, 11 insertions(+), 17 deletions(-)
diff --git a/drivers/pci/pci-bridge-emul.c b/drivers/pci/pci-bridge-emul.c
index d065c2a..e0567ca 100644
--- a/drivers/pci/pci-bridge-emul.c
+++ b/drivers/pci/pci-bridge-emul.c
@@ -24,6 +24,15 @@
#define PCI_CAP_PCIE_START PCI_BRIDGE_CONF_END
#define PCI_CAP_PCIE_END (PCI_CAP_PCIE_START + PCI_EXP_SLTSTA2 + 2)
+/**
+ * struct pci_bridge_reg_behavior - register bits behaviors
+ * @ro: Read-Only bits
+ * @rw: Read-Write bits
+ * @w1c: Write-1-to-Clear bits
+ *
+ * Reads and Writes will be filtered by specified behavior. All other bits not
+ * declared are assumed 'Reserved' and will return 0 on reads.
+ */
struct pci_bridge_reg_behavior {
/* Read-only bits */
u32 ro;
@@ -33,9 +42,6 @@ struct pci_bridge_reg_behavior {
/* Write-1-to-clear bits */
u32 w1c;
-
- /* Reserved bits (hardwired to 0) */
- u32 rsvd;
};
static const struct pci_bridge_reg_behavior pci_regs_behavior[] = {
@@ -49,7 +55,6 @@ struct pci_bridge_reg_behavior {
PCI_COMMAND_FAST_BACK) |
(PCI_STATUS_CAP_LIST | PCI_STATUS_66MHZ |
PCI_STATUS_FAST_BACK | PCI_STATUS_DEVSEL_MASK) << 16),
- .rsvd = GENMASK(15, 10) | ((BIT(6) | GENMASK(3, 0)) << 16),
.w1c = (PCI_STATUS_PARITY |
PCI_STATUS_SIG_TARGET_ABORT |
PCI_STATUS_REC_TARGET_ABORT |
@@ -106,8 +111,6 @@ struct pci_bridge_reg_behavior {
PCI_STATUS_REC_MASTER_ABORT |
PCI_STATUS_SIG_SYSTEM_ERROR |
PCI_STATUS_DETECTED_PARITY) << 16,
-
- .rsvd = ((BIT(6) | GENMASK(4, 0)) << 16),
},
[PCI_MEMORY_BASE / 4] = {
@@ -140,12 +143,10 @@ struct pci_bridge_reg_behavior {
[PCI_CAPABILITY_LIST / 4] = {
.ro = GENMASK(7, 0),
- .rsvd = GENMASK(31, 8),
},
[PCI_ROM_ADDRESS1 / 4] = {
.rw = GENMASK(31, 11) | BIT(0),
- .rsvd = GENMASK(10, 1),
},
/*
@@ -168,8 +169,6 @@ struct pci_bridge_reg_behavior {
.ro = (GENMASK(15, 8) | ((PCI_BRIDGE_CTL_FAST_BACK) << 16)),
.w1c = BIT(10) << 16,
-
- .rsvd = (GENMASK(15, 12) | BIT(4)) << 16,
},
};
@@ -196,13 +195,11 @@ struct pci_bridge_reg_behavior {
*/
.w1c = (BIT(6) | GENMASK(3, 0)) << 16,
.ro = GENMASK(5, 4) << 16,
- .rsvd = GENMASK(15, 7) << 16,
},
[PCI_EXP_LNKCAP / 4] = {
/* All bits are RO, except bit 23 which is reserved */
.ro = lower_32_bits(~BIT(23)),
- .rsvd = BIT(23),
},
[PCI_EXP_LNKCTL / 4] = {
@@ -216,7 +213,6 @@ struct pci_bridge_reg_behavior {
.rw = GENMASK(15, 14) | GENMASK(11, 3) | GENMASK(1, 0),
.ro = GENMASK(13, 0) << 16,
.w1c = GENMASK(15, 14) << 16,
- .rsvd = GENMASK(13, 12) | BIT(2),
},
[PCI_EXP_SLTCAP / 4] = {
@@ -234,7 +230,6 @@ struct pci_bridge_reg_behavior {
.rw = GENMASK(14, 0),
.w1c = (BIT(8) | GENMASK(4, 0)) << 16,
.ro = GENMASK(7, 5) << 16,
- .rsvd = BIT(15) | (GENMASK(15, 9) << 16),
},
[PCI_EXP_RTCTL / 4] = {
@@ -246,7 +241,6 @@ struct pci_bridge_reg_behavior {
*/
.rw = GENMASK(4, 0),
.ro = BIT(0) << 16,
- .rsvd = GENMASK(15, 5) | (GENMASK(15, 1) << 16),
},
[PCI_EXP_RTSTA / 4] = {
@@ -256,7 +250,6 @@ struct pci_bridge_reg_behavior {
*/
.ro = BIT(17) | GENMASK(15, 0),
.w1c = BIT(16),
- .rsvd = GENMASK(31, 18),
},
};
@@ -364,7 +357,8 @@ int pci_bridge_emul_conf_read(struct pci_bridge_emul *bridge, int where,
* Make sure we never return any reserved bit with a value
* different from 0.
*/
- *value &= ~behavior[reg / 4].rsvd;
+ *value &= behavior[reg / 4].ro | behavior[reg / 4].rw |
+ behavior[reg / 4].w1c;
if (size == 1)
*value = (*value >> (8 * (where & 3))) & 0xff;
--
1.8.3.1
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [RFC 2/9] PCI: pci-bridge-emul: Eliminate reserved member
2020-02-08 0:00 ` [RFC 2/9] PCI: pci-bridge-emul: Eliminate reserved member Jon Derrick
@ 2020-03-28 21:43 ` Bjorn Helgaas
0 siblings, 0 replies; 21+ messages in thread
From: Bjorn Helgaas @ 2020-03-28 21:43 UTC (permalink / raw)
To: Jon Derrick
Cc: linux-pci, linux-kernel, Andy Shevchenko, Mika Westerberg,
Pawel Baldysiak, Sinan Kaya, Lorenzo Pieralisi, Keith Busch,
Alexandru Gagniuc, Christoph Hellwig, Thomas Petazzoni,
Russell King
[+cc Thomas, Russell]
On Fri, Feb 07, 2020 at 05:00:00PM -0700, Jon Derrick wrote:
> Assume any bit not in the Read-Only, Read-Write, or Write-1-to-Clear
> behavior masks is a Reserved bit and should return 0 on reads.
This also seems like it makes sense on its own, especially if you can
include a spec citation.
> Signed-off-by: Jon Derrick <jonathan.derrick@intel.com>
> ---
> drivers/pci/pci-bridge-emul.c | 28 +++++++++++-----------------
> 1 file changed, 11 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/pci/pci-bridge-emul.c b/drivers/pci/pci-bridge-emul.c
> index d065c2a..e0567ca 100644
> --- a/drivers/pci/pci-bridge-emul.c
> +++ b/drivers/pci/pci-bridge-emul.c
> @@ -24,6 +24,15 @@
> #define PCI_CAP_PCIE_START PCI_BRIDGE_CONF_END
> #define PCI_CAP_PCIE_END (PCI_CAP_PCIE_START + PCI_EXP_SLTSTA2 + 2)
>
> +/**
> + * struct pci_bridge_reg_behavior - register bits behaviors
> + * @ro: Read-Only bits
> + * @rw: Read-Write bits
> + * @w1c: Write-1-to-Clear bits
> + *
> + * Reads and Writes will be filtered by specified behavior. All other bits not
> + * declared are assumed 'Reserved' and will return 0 on reads.
> + */
> struct pci_bridge_reg_behavior {
> /* Read-only bits */
> u32 ro;
> @@ -33,9 +42,6 @@ struct pci_bridge_reg_behavior {
>
> /* Write-1-to-clear bits */
> u32 w1c;
> -
> - /* Reserved bits (hardwired to 0) */
> - u32 rsvd;
> };
>
> static const struct pci_bridge_reg_behavior pci_regs_behavior[] = {
> @@ -49,7 +55,6 @@ struct pci_bridge_reg_behavior {
> PCI_COMMAND_FAST_BACK) |
> (PCI_STATUS_CAP_LIST | PCI_STATUS_66MHZ |
> PCI_STATUS_FAST_BACK | PCI_STATUS_DEVSEL_MASK) << 16),
> - .rsvd = GENMASK(15, 10) | ((BIT(6) | GENMASK(3, 0)) << 16),
> .w1c = (PCI_STATUS_PARITY |
> PCI_STATUS_SIG_TARGET_ABORT |
> PCI_STATUS_REC_TARGET_ABORT |
> @@ -106,8 +111,6 @@ struct pci_bridge_reg_behavior {
> PCI_STATUS_REC_MASTER_ABORT |
> PCI_STATUS_SIG_SYSTEM_ERROR |
> PCI_STATUS_DETECTED_PARITY) << 16,
> -
> - .rsvd = ((BIT(6) | GENMASK(4, 0)) << 16),
> },
>
> [PCI_MEMORY_BASE / 4] = {
> @@ -140,12 +143,10 @@ struct pci_bridge_reg_behavior {
>
> [PCI_CAPABILITY_LIST / 4] = {
> .ro = GENMASK(7, 0),
> - .rsvd = GENMASK(31, 8),
> },
>
> [PCI_ROM_ADDRESS1 / 4] = {
> .rw = GENMASK(31, 11) | BIT(0),
> - .rsvd = GENMASK(10, 1),
> },
>
> /*
> @@ -168,8 +169,6 @@ struct pci_bridge_reg_behavior {
> .ro = (GENMASK(15, 8) | ((PCI_BRIDGE_CTL_FAST_BACK) << 16)),
>
> .w1c = BIT(10) << 16,
> -
> - .rsvd = (GENMASK(15, 12) | BIT(4)) << 16,
> },
> };
>
> @@ -196,13 +195,11 @@ struct pci_bridge_reg_behavior {
> */
> .w1c = (BIT(6) | GENMASK(3, 0)) << 16,
> .ro = GENMASK(5, 4) << 16,
> - .rsvd = GENMASK(15, 7) << 16,
> },
>
> [PCI_EXP_LNKCAP / 4] = {
> /* All bits are RO, except bit 23 which is reserved */
> .ro = lower_32_bits(~BIT(23)),
> - .rsvd = BIT(23),
> },
>
> [PCI_EXP_LNKCTL / 4] = {
> @@ -216,7 +213,6 @@ struct pci_bridge_reg_behavior {
> .rw = GENMASK(15, 14) | GENMASK(11, 3) | GENMASK(1, 0),
> .ro = GENMASK(13, 0) << 16,
> .w1c = GENMASK(15, 14) << 16,
> - .rsvd = GENMASK(13, 12) | BIT(2),
> },
>
> [PCI_EXP_SLTCAP / 4] = {
> @@ -234,7 +230,6 @@ struct pci_bridge_reg_behavior {
> .rw = GENMASK(14, 0),
> .w1c = (BIT(8) | GENMASK(4, 0)) << 16,
> .ro = GENMASK(7, 5) << 16,
> - .rsvd = BIT(15) | (GENMASK(15, 9) << 16),
> },
>
> [PCI_EXP_RTCTL / 4] = {
> @@ -246,7 +241,6 @@ struct pci_bridge_reg_behavior {
> */
> .rw = GENMASK(4, 0),
> .ro = BIT(0) << 16,
> - .rsvd = GENMASK(15, 5) | (GENMASK(15, 1) << 16),
> },
>
> [PCI_EXP_RTSTA / 4] = {
> @@ -256,7 +250,6 @@ struct pci_bridge_reg_behavior {
> */
> .ro = BIT(17) | GENMASK(15, 0),
> .w1c = BIT(16),
> - .rsvd = GENMASK(31, 18),
> },
> };
>
> @@ -364,7 +357,8 @@ int pci_bridge_emul_conf_read(struct pci_bridge_emul *bridge, int where,
> * Make sure we never return any reserved bit with a value
> * different from 0.
> */
> - *value &= ~behavior[reg / 4].rsvd;
> + *value &= behavior[reg / 4].ro | behavior[reg / 4].rw |
> + behavior[reg / 4].w1c;
>
> if (size == 1)
> *value = (*value >> (8 * (where & 3))) & 0xff;
> --
> 1.8.3.1
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* [RFC 3/9] PCI: pci-bridge-emul: Provide a helper to set behavior
2020-02-07 23:59 [RFC 0/9] PCIe Hotplug Slot Emulation driver Jon Derrick
2020-02-07 23:59 ` [RFC 1/9] PCI: pci-bridge-emul: Update PCIe register behaviors Jon Derrick
2020-02-08 0:00 ` [RFC 2/9] PCI: pci-bridge-emul: Eliminate reserved member Jon Derrick
@ 2020-02-08 0:00 ` Jon Derrick
2020-02-08 0:00 ` [RFC 4/9] PCI: pciehp: Indirect slot register operations Jon Derrick
` (7 subsequent siblings)
10 siblings, 0 replies; 21+ messages in thread
From: Jon Derrick @ 2020-02-08 0:00 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: linux-pci, linux-kernel, Andy Shevchenko, Mika Westerberg,
Pawel Baldysiak, Sinan Kaya, Lorenzo Pieralisi, Keith Busch,
Alexandru Gagniuc, Christoph Hellwig, Jon Derrick
Add a handler to set behavior of a PCI or PCIe register. Add the
appropriate enums to specify the register's Read-Only, Read-Write, and
Write-1-to-Clear behaviors.
Signed-off-by: Jon Derrick <jonathan.derrick@intel.com>
---
drivers/pci/pci-bridge-emul.c | 19 +++++++++++++++++++
drivers/pci/pci-bridge-emul.h | 10 ++++++++++
2 files changed, 29 insertions(+)
diff --git a/drivers/pci/pci-bridge-emul.c b/drivers/pci/pci-bridge-emul.c
index e0567ca..aa18091 100644
--- a/drivers/pci/pci-bridge-emul.c
+++ b/drivers/pci/pci-bridge-emul.c
@@ -44,6 +44,25 @@ struct pci_bridge_reg_behavior {
u32 w1c;
};
+void pci_bridge_emul_set_reg_behavior(struct pci_bridge_emul *bridge,
+ bool pcie, int reg, u32 val,
+ enum pci_bridge_emul_reg_behavior type)
+{
+ struct pci_bridge_reg_behavior *behavior;
+
+ if (pcie)
+ behavior = &bridge->pcie_cap_regs_behavior[reg / 4];
+ else
+ behavior = &bridge->pci_regs_behavior[reg / 4];
+
+ if (type == PCI_BRIDGE_EMUL_REG_BEHAVIOR_RO)
+ behavior->ro = val;
+ else if (type == PCI_BRIDGE_EMUL_REG_BEHAVIOR_RW)
+ behavior->rw = val;
+ else /* PCI_BRIDGE_EMUL_REG_BEHAVIOR_W1C */
+ behavior->w1c = val;
+}
+
static const struct pci_bridge_reg_behavior pci_regs_behavior[] = {
[PCI_VENDOR_ID / 4] = { .ro = ~0 },
[PCI_COMMAND / 4] = {
diff --git a/drivers/pci/pci-bridge-emul.h b/drivers/pci/pci-bridge-emul.h
index b318830..f7027e1 100644
--- a/drivers/pci/pci-bridge-emul.h
+++ b/drivers/pci/pci-bridge-emul.h
@@ -72,6 +72,12 @@ struct pci_bridge_emul_pcie_conf {
typedef enum { PCI_BRIDGE_EMUL_HANDLED,
PCI_BRIDGE_EMUL_NOT_HANDLED } pci_bridge_emul_read_status_t;
+enum pci_bridge_emul_reg_behavior {
+ PCI_BRIDGE_EMUL_REG_BEHAVIOR_RO,
+ PCI_BRIDGE_EMUL_REG_BEHAVIOR_RW,
+ PCI_BRIDGE_EMUL_REG_BEHAVIOR_W1C,
+};
+
struct pci_bridge_emul_ops {
/*
* Called when reading from the regular PCI bridge
@@ -132,4 +138,8 @@ int pci_bridge_emul_conf_read(struct pci_bridge_emul *bridge, int where,
int pci_bridge_emul_conf_write(struct pci_bridge_emul *bridge, int where,
int size, u32 value);
+void pci_bridge_emul_set_reg_behavior(struct pci_bridge_emul *bridge,
+ bool pcie, int reg, u32 val,
+ enum pci_bridge_emul_reg_behavior type);
+
#endif /* __PCI_BRIDGE_EMUL_H__ */
--
1.8.3.1
^ permalink raw reply related [flat|nested] 21+ messages in thread* [RFC 4/9] PCI: pciehp: Indirect slot register operations
2020-02-07 23:59 [RFC 0/9] PCIe Hotplug Slot Emulation driver Jon Derrick
` (2 preceding siblings ...)
2020-02-08 0:00 ` [RFC 3/9] PCI: pci-bridge-emul: Provide a helper to set behavior Jon Derrick
@ 2020-02-08 0:00 ` Jon Derrick
2020-02-08 0:00 ` [RFC 5/9] PCI: Add pcie_port_slot_emulated stub Jon Derrick
` (6 subsequent siblings)
10 siblings, 0 replies; 21+ messages in thread
From: Jon Derrick @ 2020-02-08 0:00 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: linux-pci, linux-kernel, Andy Shevchenko, Mika Westerberg,
Pawel Baldysiak, Sinan Kaya, Lorenzo Pieralisi, Keith Busch,
Alexandru Gagniuc, Christoph Hellwig, Jon Derrick
Allow another driver to provide alternative operations when doing
capability register reads and writes to the Slot Capabilities, Slot
Control, and Slot Status registers. Add handlers to pciehp to read/write
the slot registers.
Signed-off-by: Jon Derrick <jonathan.derrick@intel.com>
---
drivers/pci/hotplug/pciehp.h | 8 +++
drivers/pci/hotplug/pciehp_hpc.c | 116 ++++++++++++++++++++++++++++-----------
2 files changed, 93 insertions(+), 31 deletions(-)
diff --git a/drivers/pci/hotplug/pciehp.h b/drivers/pci/hotplug/pciehp.h
index aa61d4c..c898c75 100644
--- a/drivers/pci/hotplug/pciehp.h
+++ b/drivers/pci/hotplug/pciehp.h
@@ -28,6 +28,12 @@
extern bool pciehp_poll_mode;
extern int pciehp_poll_time;
+/* Capability reads/writes but only for SLTCAP, SLTCTL, and SLTSTA. */
+struct slot_ecap_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);
+};
+
/*
* Set CONFIG_DYNAMIC_DEBUG=y and boot with 'dyndbg="file pciehp* +p"' to
* enable debug messages.
@@ -76,6 +82,7 @@
* @request_result: result of last user request submitted to the IRQ thread
* @requester: wait queue to wake up on completion of user request,
* used for synchronous slot enable/disable request via sysfs
+ * @slot_ops: slot config registers read/write operations
*
* PCIe hotplug has a 1:1 relationship between controller and slot, hence
* unlike other drivers, the two aren't represented by separate structures.
@@ -105,6 +112,7 @@ struct controller {
unsigned int ist_running;
int request_result;
wait_queue_head_t requester;
+ struct slot_ecap_ops *slot_ops;
};
/**
diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
index 8a2cb17..ce1e8c7 100644
--- a/drivers/pci/hotplug/pciehp_hpc.c
+++ b/drivers/pci/hotplug/pciehp_hpc.c
@@ -35,6 +35,52 @@ static inline struct pci_dev *ctrl_dev(struct controller *ctrl)
static irqreturn_t pciehp_ist(int irq, void *dev_id);
static int pciehp_poll(void *data);
+static int pcie_read_slot_capabilities(struct controller *ctrl, u32 *sltcap)
+{
+ return ctrl->slot_ops->read(ctrl_dev(ctrl), PCI_EXP_SLTCAP,
+ sizeof(*sltcap), sltcap);
+}
+
+static int pcie_read_slot_control(struct controller *ctrl, u16 *sltctl)
+{
+ int ret;
+ u32 l;
+
+ ret = ctrl->slot_ops->read(ctrl_dev(ctrl), PCI_EXP_SLTCTL,
+ sizeof(*sltctl), &l);
+ if (ret)
+ return ret;
+
+ *sltctl = l;
+ return 0;
+}
+
+static int pcie_write_slot_control(struct controller *ctrl, u16 sltctl)
+{
+ return ctrl->slot_ops->write(ctrl_dev(ctrl), PCI_EXP_SLTCTL,
+ sizeof(sltctl), sltctl);
+}
+
+static int pcie_read_slot_status(struct controller *ctrl, u16 *sltsta)
+{
+ int ret;
+ u32 l;
+
+ ret = ctrl->slot_ops->read(ctrl_dev(ctrl), PCI_EXP_SLTSTA,
+ sizeof(*sltsta), &l);
+ if (ret)
+ return ret;
+
+ *sltsta = l;
+ return 0;
+}
+
+static int pcie_write_slot_status(struct controller *ctrl, u16 sltsta)
+{
+ return ctrl->slot_ops->write(ctrl_dev(ctrl), PCI_EXP_SLTSTA,
+ sizeof(sltsta), sltsta);
+}
+
static inline int pciehp_request_irq(struct controller *ctrl)
{
int retval, irq = ctrl->pcie->irq;
@@ -65,11 +111,10 @@ static inline void pciehp_free_irq(struct controller *ctrl)
static int pcie_poll_cmd(struct controller *ctrl, int timeout)
{
- struct pci_dev *pdev = ctrl_dev(ctrl);
u16 slot_status;
do {
- pcie_capability_read_word(pdev, PCI_EXP_SLTSTA, &slot_status);
+ pcie_read_slot_status(ctrl, &slot_status);
if (slot_status == (u16) ~0) {
ctrl_info(ctrl, "%s: no response from device\n",
__func__);
@@ -77,8 +122,7 @@ static int pcie_poll_cmd(struct controller *ctrl, int timeout)
}
if (slot_status & PCI_EXP_SLTSTA_CC) {
- pcie_capability_write_word(pdev, PCI_EXP_SLTSTA,
- PCI_EXP_SLTSTA_CC);
+ pcie_write_slot_status(ctrl, PCI_EXP_SLTSTA_CC);
return 1;
}
msleep(10);
@@ -145,7 +189,7 @@ static void pcie_do_write_cmd(struct controller *ctrl, u16 cmd,
*/
pcie_wait_cmd(ctrl);
- pcie_capability_read_word(pdev, PCI_EXP_SLTCTL, &slot_ctrl);
+ pcie_read_slot_control(ctrl, &slot_ctrl);
if (slot_ctrl == (u16) ~0) {
ctrl_info(ctrl, "%s: no response from device\n", __func__);
goto out;
@@ -157,7 +201,7 @@ static void pcie_do_write_cmd(struct controller *ctrl, u16 cmd,
ctrl->cmd_busy = 1;
smp_mb();
ctrl->slot_ctrl = slot_ctrl;
- pcie_capability_write_word(pdev, PCI_EXP_SLTCTL, slot_ctrl);
+ pcie_write_slot_control(ctrl, slot_ctrl);
ctrl->cmd_started = jiffies;
/*
@@ -316,7 +360,7 @@ int pciehp_get_raw_indicator_status(struct hotplug_slot *hotplug_slot,
u16 slot_ctrl;
pci_config_pm_runtime_get(pdev);
- pcie_capability_read_word(pdev, PCI_EXP_SLTCTL, &slot_ctrl);
+ pcie_read_slot_control(ctrl, &slot_ctrl);
pci_config_pm_runtime_put(pdev);
*status = (slot_ctrl & (PCI_EXP_SLTCTL_AIC | PCI_EXP_SLTCTL_PIC)) >> 6;
return 0;
@@ -329,7 +373,7 @@ int pciehp_get_attention_status(struct hotplug_slot *hotplug_slot, u8 *status)
u16 slot_ctrl;
pci_config_pm_runtime_get(pdev);
- pcie_capability_read_word(pdev, PCI_EXP_SLTCTL, &slot_ctrl);
+ pcie_read_slot_control(ctrl, &slot_ctrl);
pci_config_pm_runtime_put(pdev);
ctrl_dbg(ctrl, "%s: SLOTCTRL %x, value read %x\n", __func__,
pci_pcie_cap(ctrl->pcie->port) + PCI_EXP_SLTCTL, slot_ctrl);
@@ -354,10 +398,9 @@ int pciehp_get_attention_status(struct hotplug_slot *hotplug_slot, u8 *status)
void pciehp_get_power_status(struct controller *ctrl, u8 *status)
{
- struct pci_dev *pdev = ctrl_dev(ctrl);
u16 slot_ctrl;
- pcie_capability_read_word(pdev, PCI_EXP_SLTCTL, &slot_ctrl);
+ pcie_read_slot_control(ctrl, &slot_ctrl);
ctrl_dbg(ctrl, "%s: SLOTCTRL %x value read %x\n", __func__,
pci_pcie_cap(ctrl->pcie->port) + PCI_EXP_SLTCTL, slot_ctrl);
@@ -376,10 +419,9 @@ void pciehp_get_power_status(struct controller *ctrl, u8 *status)
void pciehp_get_latch_status(struct controller *ctrl, u8 *status)
{
- struct pci_dev *pdev = ctrl_dev(ctrl);
u16 slot_status;
- pcie_capability_read_word(pdev, PCI_EXP_SLTSTA, &slot_status);
+ pcie_read_slot_status(ctrl, &slot_status);
*status = !!(slot_status & PCI_EXP_SLTSTA_MRLSS);
}
@@ -397,11 +439,10 @@ void pciehp_get_latch_status(struct controller *ctrl, u8 *status)
*/
int pciehp_card_present(struct controller *ctrl)
{
- struct pci_dev *pdev = ctrl_dev(ctrl);
u16 slot_status;
int ret;
- ret = pcie_capability_read_word(pdev, PCI_EXP_SLTSTA, &slot_status);
+ ret = pcie_read_slot_status(ctrl, &slot_status);
if (ret == PCIBIOS_DEVICE_NOT_FOUND || slot_status == (u16)~0)
return -ENODEV;
@@ -433,10 +474,9 @@ int pciehp_card_present_or_link_active(struct controller *ctrl)
int pciehp_query_power_fault(struct controller *ctrl)
{
- struct pci_dev *pdev = ctrl_dev(ctrl);
u16 slot_status;
- pcie_capability_read_word(pdev, PCI_EXP_SLTSTA, &slot_status);
+ pcie_read_slot_status(ctrl, &slot_status);
return !!(slot_status & PCI_EXP_SLTSTA_PFD);
}
@@ -491,15 +531,13 @@ void pciehp_set_indicators(struct controller *ctrl, int pwr, int attn)
int pciehp_power_on_slot(struct controller *ctrl)
{
- struct pci_dev *pdev = ctrl_dev(ctrl);
u16 slot_status;
int retval;
/* Clear power-fault bit from previous power failures */
- pcie_capability_read_word(pdev, PCI_EXP_SLTSTA, &slot_status);
+ pcie_read_slot_status(ctrl, &slot_status);
if (slot_status & PCI_EXP_SLTSTA_PFD)
- pcie_capability_write_word(pdev, PCI_EXP_SLTSTA,
- PCI_EXP_SLTSTA_PFD);
+ pcie_write_slot_status(ctrl, PCI_EXP_SLTSTA_PFD);
ctrl->power_fault_detected = 0;
pcie_write_cmd(ctrl, PCI_EXP_SLTCTL_PWR_ON, PCI_EXP_SLTCTL_PCC);
@@ -552,7 +590,7 @@ static irqreturn_t pciehp_isr(int irq, void *dev_id)
}
}
- pcie_capability_read_word(pdev, PCI_EXP_SLTSTA, &status);
+ pcie_read_slot_status(ctrl, &status);
if (status == (u16) ~0) {
ctrl_info(ctrl, "%s: no response from device\n", __func__);
if (parent)
@@ -581,7 +619,7 @@ static irqreturn_t pciehp_isr(int irq, void *dev_id)
return IRQ_NONE;
}
- pcie_capability_write_word(pdev, PCI_EXP_SLTSTA, events);
+ pcie_write_slot_status(ctrl, events);
ctrl_dbg(ctrl, "pending interrupts %#06x from Slot Status\n", events);
if (parent)
pm_runtime_put(parent);
@@ -744,8 +782,7 @@ static void pcie_disable_notification(struct controller *ctrl)
void pcie_clear_hotplug_events(struct controller *ctrl)
{
- pcie_capability_write_word(ctrl_dev(ctrl), PCI_EXP_SLTSTA,
- PCI_EXP_SLTSTA_PDC | PCI_EXP_SLTSTA_DLLSC);
+ pcie_write_slot_status(ctrl, PCI_EXP_SLTSTA_PDC | PCI_EXP_SLTSTA_DLLSC);
}
void pcie_enable_interrupt(struct controller *ctrl)
@@ -782,7 +819,6 @@ void pcie_disable_interrupt(struct controller *ctrl)
int pciehp_reset_slot(struct hotplug_slot *hotplug_slot, int probe)
{
struct controller *ctrl = to_ctrl(hotplug_slot);
- struct pci_dev *pdev = ctrl_dev(ctrl);
u16 stat_mask = 0, ctrl_mask = 0;
int rc;
@@ -804,7 +840,7 @@ int pciehp_reset_slot(struct hotplug_slot *hotplug_slot, int probe)
rc = pci_bridge_secondary_bus_reset(ctrl->pcie->port);
- pcie_capability_write_word(pdev, PCI_EXP_SLTSTA, stat_mask);
+ pcie_write_slot_status(ctrl, stat_mask);
pcie_write_cmd_nowait(ctrl, ctrl_mask, ctrl_mask);
ctrl_dbg(ctrl, "%s: SLOTCTRL %x write cmd %x\n", __func__,
pci_pcie_cap(ctrl->pcie->port) + PCI_EXP_SLTCTL, ctrl_mask);
@@ -833,16 +869,32 @@ void pcie_shutdown_notification(struct controller *ctrl)
static inline void dbg_ctrl(struct controller *ctrl)
{
- struct pci_dev *pdev = ctrl->pcie->port;
u16 reg16;
ctrl_dbg(ctrl, "Slot Capabilities : 0x%08x\n", ctrl->slot_cap);
- pcie_capability_read_word(pdev, PCI_EXP_SLTSTA, ®16);
+ pcie_read_slot_status(ctrl, ®16);
ctrl_dbg(ctrl, "Slot Status : 0x%04x\n", reg16);
- pcie_capability_read_word(pdev, PCI_EXP_SLTCTL, ®16);
+ pcie_read_slot_control(ctrl, ®16);
ctrl_dbg(ctrl, "Slot Control : 0x%04x\n", reg16);
}
+static int pciehp_read_slot(struct pci_dev *dev, int pos, int len, u32 *val)
+{
+ return (len == 2) ? pcie_capability_read_word(dev, pos, (u16 *)val) :
+ pcie_capability_read_dword(dev, pos, val);
+}
+
+static int pciehp_write_slot(struct pci_dev *dev, int pos, int len, u32 val)
+{
+ return (len == 2) ? pcie_capability_write_word(dev, pos, val) :
+ pcie_capability_write_dword(dev, pos, val);
+}
+
+static struct slot_ecap_ops pciehp_default_slot_ops = {
+ .read = pciehp_read_slot,
+ .write = pciehp_write_slot,
+};
+
#define FLAG(x, y) (((x) & (y)) ? '+' : '-')
struct controller *pcie_init(struct pcie_device *dev)
@@ -858,7 +910,9 @@ struct controller *pcie_init(struct pcie_device *dev)
return NULL;
ctrl->pcie = dev;
- pcie_capability_read_dword(pdev, PCI_EXP_SLTCAP, &slot_cap);
+
+ ctrl->slot_ops = &pciehp_default_slot_ops;
+ pcie_read_slot_capabilities(ctrl, &slot_cap);
if (pdev->hotplug_user_indicators)
slot_cap &= ~(PCI_EXP_SLTCAP_AIP | PCI_EXP_SLTCAP_PIP);
@@ -887,7 +941,7 @@ struct controller *pcie_init(struct pcie_device *dev)
pcie_capability_read_dword(pdev, PCI_EXP_LNKCAP, &link_cap);
/* Clear all remaining event bits in Slot Status register. */
- pcie_capability_write_word(pdev, PCI_EXP_SLTSTA,
+ pcie_write_slot_status(ctrl,
PCI_EXP_SLTSTA_ABP | PCI_EXP_SLTSTA_PFD |
PCI_EXP_SLTSTA_MRLSC | PCI_EXP_SLTSTA_CC |
PCI_EXP_SLTSTA_DLLSC | PCI_EXP_SLTSTA_PDC);
--
1.8.3.1
^ permalink raw reply related [flat|nested] 21+ messages in thread* [RFC 5/9] PCI: Add pcie_port_slot_emulated stub
2020-02-07 23:59 [RFC 0/9] PCIe Hotplug Slot Emulation driver Jon Derrick
` (3 preceding siblings ...)
2020-02-08 0:00 ` [RFC 4/9] PCI: pciehp: Indirect slot register operations Jon Derrick
@ 2020-02-08 0:00 ` Jon Derrick
2020-02-08 0:00 ` [RFC 6/9] PCI: pciehp: Expose the poll loop to other drivers Jon Derrick
` (5 subsequent siblings)
10 siblings, 0 replies; 21+ messages in thread
From: Jon Derrick @ 2020-02-08 0:00 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: linux-pci, linux-kernel, Andy Shevchenko, Mika Westerberg,
Pawel Baldysiak, Sinan Kaya, Lorenzo Pieralisi, Keith Busch,
Alexandru Gagniuc, Christoph Hellwig, Jon Derrick
Add the checks to allow an emulated slot. An emulated slot will use
native Hotplug, AER, and PME services. It also needs to specify itself
as a hotplug bridge in order for bridge sizing to account for hotplug
reserved windows.
Signed-off-by: Jon Derrick <jonathan.derrick@intel.com>
---
drivers/pci/pci-acpi.c | 3 +++
drivers/pci/pcie/portdrv_core.c | 11 ++++++++---
drivers/pci/probe.c | 2 +-
include/linux/pci.h | 2 ++
4 files changed, 14 insertions(+), 4 deletions(-)
diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
index 0c02d50..b693e9f 100644
--- a/drivers/pci/pci-acpi.c
+++ b/drivers/pci/pci-acpi.c
@@ -796,6 +796,9 @@ bool pciehp_is_native(struct pci_dev *bridge)
if (!IS_ENABLED(CONFIG_HOTPLUG_PCI_PCIE))
return false;
+ if (pcie_port_slot_emulated(bridge))
+ return true;
+
pcie_capability_read_dword(bridge, PCI_EXP_SLTCAP, &slot_cap);
if (!(slot_cap & PCI_EXP_SLTCAP_HPC))
return false;
diff --git a/drivers/pci/pcie/portdrv_core.c b/drivers/pci/pcie/portdrv_core.c
index 5075cb9..5979bb7 100644
--- a/drivers/pci/pcie/portdrv_core.c
+++ b/drivers/pci/pcie/portdrv_core.c
@@ -193,6 +193,11 @@ static int pcie_init_service_irqs(struct pci_dev *dev, int *irqs, int mask)
return 0;
}
+static inline bool is_port_native_or_emulated(struct pci_dev *dev)
+{
+ return pcie_ports_native || pcie_port_slot_emulated(dev);
+}
+
/**
* get_port_device_capability - discover capabilities of a PCI Express port
* @dev: PCI Express port to examine
@@ -209,7 +214,7 @@ static int get_port_device_capability(struct pci_dev *dev)
int services = 0;
if (dev->is_hotplug_bridge &&
- (pcie_ports_native || host->native_pcie_hotplug)) {
+ (is_port_native_or_emulated(dev) || host->native_pcie_hotplug)) {
services |= PCIE_PORT_SERVICE_HP;
/*
@@ -222,7 +227,7 @@ static int get_port_device_capability(struct pci_dev *dev)
#ifdef CONFIG_PCIEAER
if (dev->aer_cap && pci_aer_available() &&
- (pcie_ports_native || host->native_aer)) {
+ (is_port_native_or_emulated(dev) || host->native_aer)) {
services |= PCIE_PORT_SERVICE_AER;
/*
@@ -239,7 +244,7 @@ static int get_port_device_capability(struct pci_dev *dev)
* those yet.
*/
if (pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT &&
- (pcie_ports_native || host->native_pme)) {
+ (is_port_native_or_emulated(dev) || host->native_pme)) {
services |= PCIE_PORT_SERVICE_PME;
/*
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 512cb43..b04b0c2 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -1477,7 +1477,7 @@ void set_pcie_hotplug_bridge(struct pci_dev *pdev)
u32 reg32;
pcie_capability_read_dword(pdev, PCI_EXP_SLTCAP, ®32);
- if (reg32 & PCI_EXP_SLTCAP_HPC)
+ if (reg32 & PCI_EXP_SLTCAP_HPC || pcie_port_slot_emulated(pdev))
pdev->is_hotplug_bridge = 1;
}
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 3840a54..0391e39 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1536,6 +1536,8 @@ static inline int pci_irqd_intx_xlate(struct irq_domain *d,
#define pcie_ports_native false
#endif
+#define pcie_port_slot_emulated(dev) false
+
#define PCIE_LINK_STATE_L0S BIT(0)
#define PCIE_LINK_STATE_L1 BIT(1)
#define PCIE_LINK_STATE_CLKPM BIT(2)
--
1.8.3.1
^ permalink raw reply related [flat|nested] 21+ messages in thread* [RFC 6/9] PCI: pciehp: Expose the poll loop to other drivers
2020-02-07 23:59 [RFC 0/9] PCIe Hotplug Slot Emulation driver Jon Derrick
` (4 preceding siblings ...)
2020-02-08 0:00 ` [RFC 5/9] PCI: Add pcie_port_slot_emulated stub Jon Derrick
@ 2020-02-08 0:00 ` Jon Derrick
2020-02-08 0:00 ` [RFC 7/9] PCI: Move pci_dev_str_match to search.c Jon Derrick
` (4 subsequent siblings)
10 siblings, 0 replies; 21+ messages in thread
From: Jon Derrick @ 2020-02-08 0:00 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: linux-pci, linux-kernel, Andy Shevchenko, Mika Westerberg,
Pawel Baldysiak, Sinan Kaya, Lorenzo Pieralisi, Keith Busch,
Alexandru Gagniuc, Christoph Hellwig, Jon Derrick
Abstract the poll loop into an 'events pending' check and a 'handle
events' method in order to allow another driver to call the polling
loop.
Signed-off-by: Jon Derrick <jonathan.derrick@intel.com>
---
drivers/pci/hotplug/pciehp.h | 2 ++
drivers/pci/hotplug/pciehp_hpc.c | 16 +++++++++++++---
2 files changed, 15 insertions(+), 3 deletions(-)
diff --git a/drivers/pci/hotplug/pciehp.h b/drivers/pci/hotplug/pciehp.h
index c898c75..48fdd62 100644
--- a/drivers/pci/hotplug/pciehp.h
+++ b/drivers/pci/hotplug/pciehp.h
@@ -160,6 +160,8 @@ struct controller {
#define NO_CMD_CMPL(ctrl) ((ctrl)->slot_cap & PCI_EXP_SLTCAP_NCCS)
#define PSN(ctrl) (((ctrl)->slot_cap & PCI_EXP_SLTCAP_PSN) >> 19)
+bool pciehp_events_pending(struct controller *ctrl);
+void pciehp_handle_events(struct controller *ctrl);
void pciehp_request(struct controller *ctrl, int action);
void pciehp_handle_button_press(struct controller *ctrl);
void pciehp_handle_disable_request(struct controller *ctrl);
diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
index ce1e8c7..e46f177 100644
--- a/drivers/pci/hotplug/pciehp_hpc.c
+++ b/drivers/pci/hotplug/pciehp_hpc.c
@@ -708,6 +708,17 @@ static irqreturn_t pciehp_ist(int irq, void *dev_id)
return IRQ_HANDLED;
}
+bool pciehp_events_pending(struct controller *ctrl)
+{
+ return pciehp_isr(IRQ_NOTCONNECTED, ctrl) == IRQ_WAKE_THREAD ||
+ atomic_read(&ctrl->pending_events);
+}
+
+void pciehp_handle_events(struct controller *ctrl)
+{
+ pciehp_ist(IRQ_NOTCONNECTED, ctrl);
+}
+
static int pciehp_poll(void *data)
{
struct controller *ctrl = data;
@@ -716,9 +727,8 @@ static int pciehp_poll(void *data)
while (!kthread_should_stop()) {
/* poll for interrupt events or user requests */
- while (pciehp_isr(IRQ_NOTCONNECTED, ctrl) == IRQ_WAKE_THREAD ||
- atomic_read(&ctrl->pending_events))
- pciehp_ist(IRQ_NOTCONNECTED, ctrl);
+ while (pciehp_events_pending(ctrl))
+ pciehp_handle_events(ctrl);
if (pciehp_poll_time <= 0 || pciehp_poll_time > 60)
pciehp_poll_time = 2; /* clamp to sane value */
--
1.8.3.1
^ permalink raw reply related [flat|nested] 21+ messages in thread* [RFC 7/9] PCI: Move pci_dev_str_match to search.c
2020-02-07 23:59 [RFC 0/9] PCIe Hotplug Slot Emulation driver Jon Derrick
` (5 preceding siblings ...)
2020-02-08 0:00 ` [RFC 6/9] PCI: pciehp: Expose the poll loop to other drivers Jon Derrick
@ 2020-02-08 0:00 ` Jon Derrick
2020-02-08 0:00 ` [RFC 8/9] PCI: pciehp: Add hotplug slot emulation driver Jon Derrick
` (3 subsequent siblings)
10 siblings, 0 replies; 21+ messages in thread
From: Jon Derrick @ 2020-02-08 0:00 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: linux-pci, linux-kernel, Andy Shevchenko, Mika Westerberg,
Pawel Baldysiak, Sinan Kaya, Lorenzo Pieralisi, Keith Busch,
Alexandru Gagniuc, Christoph Hellwig, Jon Derrick
The method which extracts a string descriptor of one or more PCI devices
and matches to a struct pci_dev is useful in general to other subsystems
needing to match parameters or sysfs strings. Move this function to
search.c for general use.
No functional changes.
Signed-off-by: Jon Derrick <jonathan.derrick@intel.com>
---
drivers/pci/pci.c | 163 ---------------------------------------------------
drivers/pci/search.c | 162 ++++++++++++++++++++++++++++++++++++++++++++++++++
include/linux/pci.h | 2 +
3 files changed, 164 insertions(+), 163 deletions(-)
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 3c30e72..dac1b08 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -203,169 +203,6 @@ void __iomem *pci_ioremap_wc_bar(struct pci_dev *pdev, int bar)
EXPORT_SYMBOL_GPL(pci_ioremap_wc_bar);
#endif
-/**
- * pci_dev_str_match_path - test if a path string matches a device
- * @dev: the PCI device to test
- * @path: string to match the device against
- * @endptr: pointer to the string after the match
- *
- * Test if a string (typically from a kernel parameter) formatted as a
- * path of device/function addresses matches a PCI device. The string must
- * be of the form:
- *
- * [<domain>:]<bus>:<device>.<func>[/<device>.<func>]*
- *
- * A path for a device can be obtained using 'lspci -t'. Using a path
- * is more robust against bus renumbering than using only a single bus,
- * device and function address.
- *
- * Returns 1 if the string matches the device, 0 if it does not and
- * a negative error code if it fails to parse the string.
- */
-static int pci_dev_str_match_path(struct pci_dev *dev, const char *path,
- const char **endptr)
-{
- int ret;
- int seg, bus, slot, func;
- char *wpath, *p;
- char end;
-
- *endptr = strchrnul(path, ';');
-
- wpath = kmemdup_nul(path, *endptr - path, GFP_KERNEL);
- if (!wpath)
- return -ENOMEM;
-
- while (1) {
- p = strrchr(wpath, '/');
- if (!p)
- break;
- ret = sscanf(p, "/%x.%x%c", &slot, &func, &end);
- if (ret != 2) {
- ret = -EINVAL;
- goto free_and_exit;
- }
-
- if (dev->devfn != PCI_DEVFN(slot, func)) {
- ret = 0;
- goto free_and_exit;
- }
-
- /*
- * Note: we don't need to get a reference to the upstream
- * bridge because we hold a reference to the top level
- * device which should hold a reference to the bridge,
- * and so on.
- */
- dev = pci_upstream_bridge(dev);
- if (!dev) {
- ret = 0;
- goto free_and_exit;
- }
-
- *p = 0;
- }
-
- ret = sscanf(wpath, "%x:%x:%x.%x%c", &seg, &bus, &slot,
- &func, &end);
- if (ret != 4) {
- seg = 0;
- ret = sscanf(wpath, "%x:%x.%x%c", &bus, &slot, &func, &end);
- if (ret != 3) {
- ret = -EINVAL;
- goto free_and_exit;
- }
- }
-
- ret = (seg == pci_domain_nr(dev->bus) &&
- bus == dev->bus->number &&
- dev->devfn == PCI_DEVFN(slot, func));
-
-free_and_exit:
- kfree(wpath);
- return ret;
-}
-
-/**
- * pci_dev_str_match - test if a string matches a device
- * @dev: the PCI device to test
- * @p: string to match the device against
- * @endptr: pointer to the string after the match
- *
- * Test if a string (typically from a kernel parameter) matches a specified
- * PCI device. The string may be of one of the following formats:
- *
- * [<domain>:]<bus>:<device>.<func>[/<device>.<func>]*
- * pci:<vendor>:<device>[:<subvendor>:<subdevice>]
- *
- * The first format specifies a PCI bus/device/function address which
- * may change if new hardware is inserted, if motherboard firmware changes,
- * or due to changes caused in kernel parameters. If the domain is
- * left unspecified, it is taken to be 0. In order to be robust against
- * bus renumbering issues, a path of PCI device/function numbers may be used
- * to address the specific device. The path for a device can be determined
- * through the use of 'lspci -t'.
- *
- * The second format matches devices using IDs in the configuration
- * space which may match multiple devices in the system. A value of 0
- * for any field will match all devices. (Note: this differs from
- * in-kernel code that uses PCI_ANY_ID which is ~0; this is for
- * legacy reasons and convenience so users don't have to specify
- * FFFFFFFFs on the command line.)
- *
- * Returns 1 if the string matches the device, 0 if it does not and
- * a negative error code if the string cannot be parsed.
- */
-static int pci_dev_str_match(struct pci_dev *dev, const char *p,
- const char **endptr)
-{
- int ret;
- int count;
- unsigned short vendor, device, subsystem_vendor, subsystem_device;
-
- if (strncmp(p, "pci:", 4) == 0) {
- /* PCI vendor/device (subvendor/subdevice) IDs are specified */
- p += 4;
- ret = sscanf(p, "%hx:%hx:%hx:%hx%n", &vendor, &device,
- &subsystem_vendor, &subsystem_device, &count);
- if (ret != 4) {
- ret = sscanf(p, "%hx:%hx%n", &vendor, &device, &count);
- if (ret != 2)
- return -EINVAL;
-
- subsystem_vendor = 0;
- subsystem_device = 0;
- }
-
- p += count;
-
- if ((!vendor || vendor == dev->vendor) &&
- (!device || device == dev->device) &&
- (!subsystem_vendor ||
- subsystem_vendor == dev->subsystem_vendor) &&
- (!subsystem_device ||
- subsystem_device == dev->subsystem_device))
- goto found;
- } else {
- /*
- * PCI Bus, Device, Function IDs are specified
- * (optionally, may include a path of devfns following it)
- */
- ret = pci_dev_str_match_path(dev, p, &p);
- if (ret < 0)
- return ret;
- else if (ret)
- goto found;
- }
-
- *endptr = p;
- return 0;
-
-found:
- *endptr = p;
- return 1;
-}
-
static int __pci_find_next_cap_ttl(struct pci_bus *bus, unsigned int devfn,
u8 pos, int cap, int *ttl)
{
diff --git a/drivers/pci/search.c b/drivers/pci/search.c
index 2061672..6821b70 100644
--- a/drivers/pci/search.c
+++ b/drivers/pci/search.c
@@ -394,3 +394,165 @@ int pci_dev_present(const struct pci_device_id *ids)
return 0;
}
EXPORT_SYMBOL(pci_dev_present);
+
+/**
+ * pci_dev_str_match_path - test if a path string matches a device
+ * @dev: the PCI device to test
+ * @path: string to match the device against
+ * @endptr: pointer to the string after the match
+ *
+ * Test if a string (typically from a kernel parameter) formatted as a
+ * path of device/function addresses matches a PCI device. The string must
+ * be of the form:
+ *
+ * [<domain>:]<bus>:<device>.<func>[/<device>.<func>]*
+ *
+ * A path for a device can be obtained using 'lspci -t'. Using a path
+ * is more robust against bus renumbering than using only a single bus,
+ * device and function address.
+ *
+ * Returns 1 if the string matches the device, 0 if it does not and
+ * a negative error code if it fails to parse the string.
+ */
+static int pci_dev_str_match_path(struct pci_dev *dev, const char *path,
+ const char **endptr)
+{
+ int ret;
+ int seg, bus, slot, func;
+ char *wpath, *p;
+ char end;
+
+ *endptr = strchrnul(path, ';');
+
+ wpath = kmemdup_nul(path, *endptr - path, GFP_KERNEL);
+ if (!wpath)
+ return -ENOMEM;
+
+ while (1) {
+ p = strrchr(wpath, '/');
+ if (!p)
+ break;
+ ret = sscanf(p, "/%x.%x%c", &slot, &func, &end);
+ if (ret != 2) {
+ ret = -EINVAL;
+ goto free_and_exit;
+ }
+
+ if (dev->devfn != PCI_DEVFN(slot, func)) {
+ ret = 0;
+ goto free_and_exit;
+ }
+
+ /*
+ * Note: we don't need to get a reference to the upstream
+ * bridge because we hold a reference to the top level
+ * device which should hold a reference to the bridge,
+ * and so on.
+ */
+ dev = pci_upstream_bridge(dev);
+ if (!dev) {
+ ret = 0;
+ goto free_and_exit;
+ }
+
+ *p = 0;
+ }
+
+ ret = sscanf(wpath, "%x:%x:%x.%x%c", &seg, &bus, &slot,
+ &func, &end);
+ if (ret != 4) {
+ seg = 0;
+ ret = sscanf(wpath, "%x:%x.%x%c", &bus, &slot, &func, &end);
+ if (ret != 3) {
+ ret = -EINVAL;
+ goto free_and_exit;
+ }
+ }
+
+ ret = (seg == pci_domain_nr(dev->bus) &&
+ bus == dev->bus->number &&
+ dev->devfn == PCI_DEVFN(slot, func));
+
+free_and_exit:
+ kfree(wpath);
+ return ret;
+}
+
+/**
+ * pci_dev_str_match - test if a string matches a device
+ * @dev: the PCI device to test
+ * @p: string to match the device against
+ * @endptr: pointer to the string after the match
+ *
+ * Test if a string (typically from a kernel parameter) matches a specified
+ * PCI device. The string may be of one of the following formats:
+ *
+ * [<domain>:]<bus>:<device>.<func>[/<device>.<func>]*
+ * pci:<vendor>:<device>[:<subvendor>:<subdevice>]
+ *
+ * The first format specifies a PCI bus/device/function address which
+ * may change if new hardware is inserted, if motherboard firmware changes,
+ * or due to changes caused in kernel parameters. If the domain is
+ * left unspecified, it is taken to be 0. In order to be robust against
+ * bus renumbering issues, a path of PCI device/function numbers may be used
+ * to address the specific device. The path for a device can be determined
+ * through the use of 'lspci -t'.
+ *
+ * The second format matches devices using IDs in the configuration
+ * space which may match multiple devices in the system. A value of 0
+ * for any field will match all devices. (Note: this differs from
+ * in-kernel code that uses PCI_ANY_ID which is ~0; this is for
+ * legacy reasons and convenience so users don't have to specify
+ * FFFFFFFFs on the command line.)
+ *
+ * Returns 1 if the string matches the device, 0 if it does not and
+ * a negative error code if the string cannot be parsed.
+ */
+int pci_dev_str_match(struct pci_dev *dev, const char *p, const char **endptr)
+{
+ int ret;
+ int count;
+ unsigned short vendor, device, subsystem_vendor, subsystem_device;
+
+ if (strncmp(p, "pci:", 4) == 0) {
+ /* PCI vendor/device (subvendor/subdevice) IDs are specified */
+ p += 4;
+ ret = sscanf(p, "%hx:%hx:%hx:%hx%n", &vendor, &device,
+ &subsystem_vendor, &subsystem_device, &count);
+ if (ret != 4) {
+ ret = sscanf(p, "%hx:%hx%n", &vendor, &device, &count);
+ if (ret != 2)
+ return -EINVAL;
+
+ subsystem_vendor = 0;
+ subsystem_device = 0;
+ }
+
+ p += count;
+
+ if ((!vendor || vendor == dev->vendor) &&
+ (!device || device == dev->device) &&
+ (!subsystem_vendor ||
+ subsystem_vendor == dev->subsystem_vendor) &&
+ (!subsystem_device ||
+ subsystem_device == dev->subsystem_device))
+ goto found;
+ } else {
+ /*
+ * PCI Bus, Device, Function IDs are specified
+ * (optionally, may include a path of devfns following it)
+ */
+ ret = pci_dev_str_match_path(dev, p, &p);
+ if (ret < 0)
+ return ret;
+ else if (ret)
+ goto found;
+ }
+
+ *endptr = p;
+ return 0;
+
+found:
+ *endptr = p;
+ return 1;
+}
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 0391e39..c5c9bb5 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1056,6 +1056,8 @@ struct pci_dev *pci_get_domain_bus_and_slot(int domain, unsigned int bus,
struct pci_dev *pci_get_class(unsigned int class, struct pci_dev *from);
int pci_dev_present(const struct pci_device_id *ids);
+int pci_dev_str_match(struct pci_dev *dev, const char *p, const char **endptr);
+
int pci_bus_read_config_byte(struct pci_bus *bus, unsigned int devfn,
int where, u8 *val);
int pci_bus_read_config_word(struct pci_bus *bus, unsigned int devfn,
--
1.8.3.1
^ permalink raw reply related [flat|nested] 21+ messages in thread* [RFC 8/9] PCI: pciehp: Add hotplug slot emulation driver
2020-02-07 23:59 [RFC 0/9] PCIe Hotplug Slot Emulation driver Jon Derrick
` (6 preceding siblings ...)
2020-02-08 0:00 ` [RFC 7/9] PCI: Move pci_dev_str_match to search.c Jon Derrick
@ 2020-02-08 0:00 ` Jon Derrick
2020-02-08 0:00 ` [RFC 9/9] PCI: pciehp: Wire up pcie_port_emulate_slot and pciehp_emul Jon Derrick
` (2 subsequent siblings)
10 siblings, 0 replies; 21+ messages in thread
From: Jon Derrick @ 2020-02-08 0:00 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: linux-pci, linux-kernel, Andy Shevchenko, Mika Westerberg,
Pawel Baldysiak, Sinan Kaya, Lorenzo Pieralisi, Keith Busch,
Alexandru Gagniuc, Christoph Hellwig, Jon Derrick
This adds the PCIe Hotplug emulation driver. There may be platforms with
specific configurations that can support hotplug but don't provide the
logical slot hotplug hardware. For instance, the platform may use an
electrically-tolerant interposer between the slot and the device.
This driver utilizes the pci-bridge-emul architecture to manage register
reads and writes. The underlying functionality of the hotplug emulation
driver uses the Data Link Layer Link Active Reporting mechanism in a
polling loop, but can tolerate other event sources such as AER or DPC.
When enabled and a slot is managed by the driver, all PCIe Port services
are managed by the kernel. This is done to ensure that firmware hotplug
and error architecture does not (correctly) machine check the system
when hotplug is performed on a non-hotplug slot.
The driver offers two active mode: auto and force.
auto: Bind to non-hotplug slots
force: Bind to all slots and overrides the slot's services
There are three kernel parameters:
pciehp.pciehp_emul_mode={off, auto, force}
pciehp.pciehp_emul_time=<msecs poll time> (def 1000, min 100, max 60000)
pciehp.pciehp_emul_ports=<PCI [S]BDF/ID format string>
The pciehp_emul_ports parameter takes a semi-colon tokenized string
representing PCI [S]BDFs and IDs. The pciehp_emul_mode will then be
applied to only those slots, leaving other slots unmanaged by
pciehp_emul.
The string follows the pci_dev_str_match() format:
[<domain>:]<bus>:<device>.<func>[/<device>.<func>]*
pci:<vendor>:<device>[:<subvendor>:<subdevice>]
When using the path format, the path for the device can be obtained
using 'lspci -t' and further specified using the upstream bridge and the
downstream port's device-function to be more robust against bus
renumbering.
When using the vendor-device format, a value of '0' in any field acts as
a wildcard for that field, matching all values.
The driver is enabled with CONFIG_HOTPLUG_PCI_PCIE_EMUL=y.
Signed-off-by: Jon Derrick <jonathan.derrick@intel.com>
---
drivers/pci/hotplug/Makefile | 4 +
drivers/pci/hotplug/pciehp_emul.c | 373 ++++++++++++++++++++++++++++++++++++++
drivers/pci/pcie/Kconfig | 14 ++
3 files changed, 391 insertions(+)
create mode 100644 drivers/pci/hotplug/pciehp_emul.c
diff --git a/drivers/pci/hotplug/Makefile b/drivers/pci/hotplug/Makefile
index 5196983..4c01eca 100644
--- a/drivers/pci/hotplug/Makefile
+++ b/drivers/pci/hotplug/Makefile
@@ -65,6 +65,10 @@ pciehp-objs := pciehp_core.o \
pciehp_pci.o \
pciehp_hpc.o
+ifdef CONFIG_HOTPLUG_PCI_PCIE_EMUL
+pciehp-objs += pciehp_emul.o
+endif
+
shpchp-objs := shpchp_core.o \
shpchp_ctrl.o \
shpchp_pci.o \
diff --git a/drivers/pci/hotplug/pciehp_emul.c b/drivers/pci/hotplug/pciehp_emul.c
new file mode 100644
index 0000000..1a1fb51
--- /dev/null
+++ b/drivers/pci/hotplug/pciehp_emul.c
@@ -0,0 +1,373 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * PCI Express PCI Hot Plug Slot Emulation Driver
+ * Author: Jon Derrick <jonathan.derrick@intel.com>
+ *
+ * Copyright (C) 2020 Intel Corp.
+ */
+
+#define dev_fmt(fmt) "pciehp: " fmt
+
+#include <linux/moduleparam.h>
+#include <linux/kthread.h>
+
+#include "../pci.h"
+#include "../pci-bridge-emul.h"
+#include "pciehp.h"
+
+static char *pciehp_emul_ports;
+module_param(pciehp_emul_ports, charp, 0444);
+MODULE_PARM_DESC(pciehp_emul_ports, "list of port BDF/IDs (optional)");
+
+static unsigned int pciehp_emul_time = 1000;
+module_param(pciehp_emul_time, uint, 0644);
+MODULE_PARM_DESC(pciehp_emul_time, "link polling delay in msecs");
+
+enum {
+ PCIEHP_EMUL_OFF,
+ PCIEHP_EMUL_AUTO,
+ PCIEHP_EMUL_FORCE,
+};
+
+static const char *const pciehp_emul_mode_strings[] = {
+ "off",
+ "auto",
+ "force",
+};
+
+static int pciehp_emul_mode;
+MODULE_PARM_DESC(pciehp_emul_mode, "slot emulation mode");
+
+static int pciehp_emul_mode_set(const char *str, const struct kernel_param *kp)
+{
+ int i;
+
+ i = match_string(pciehp_emul_mode_strings,
+ ARRAY_SIZE(pciehp_emul_mode_strings), str);
+ if (i < 0)
+ return i;
+
+ pciehp_emul_mode = i;
+ return 0;
+}
+
+static int pciehp_emul_mode_get(char *buffer, const struct kernel_param *kp)
+{
+ return sprintf(buffer, "%s", pciehp_emul_mode_strings[pciehp_emul_mode]);
+}
+
+static const struct kernel_param_ops pciehp_emul_mode_ops = {
+ .set = pciehp_emul_mode_set,
+ .get = pciehp_emul_mode_get,
+};
+
+module_param_cb(pciehp_emul_mode, &pciehp_emul_mode_ops, &pciehp_emul_mode, 0444);
+
+static LIST_HEAD(pciehp_emul_list);
+static DEFINE_MUTEX(list_lock);
+
+/**
+ * struct pciehp_emul - Emulated PCIe Hotplug Slot structure
+ * @node: List of PCIe Hotplug Emulated PCI devices.
+ * @ctrl: Slot's pciehp controller.
+ * @bridge: PCI Bridge Emul descriptor. Emulates the common register
+ * read/write behaviors.
+ * @old_ops: Default slot pcie capability read/write operations.
+ * This driver will override the slot ops and restore the
+ * ops on driver removal.
+ * @poll_thread: Per-slot Data Link Layer Link Active polling thread.
+ */
+struct pciehp_emul {
+ struct list_head node;
+ struct controller *ctrl;
+ struct pci_bridge_emul bridge;
+ struct slot_ecap_ops *old_ops;
+ struct task_struct *poll_thread;
+};
+
+/* Emul Bridge OPS */
+static struct pci_bridge_emul_ops pciehp_emul_pci_bridge_ops = {};
+
+static struct pciehp_emul *pci_dev_to_pciehp_emul(struct pci_dev *dev)
+{
+ struct pciehp_emul *emul;
+
+ list_for_each_entry(emul, &pciehp_emul_list, node)
+ if (emul->ctrl->pcie->port == dev)
+ return emul;
+
+ return NULL;
+}
+
+static int pciehp_emul_slot_rw(struct pci_dev *dev, int pos, int len,
+ u32 *val, int dir)
+{
+ struct pciehp_emul *emul;
+ int ret;
+
+ mutex_lock(&list_lock);
+ emul = pci_dev_to_pciehp_emul(dev);
+ if (!emul) {
+ mutex_unlock(&list_lock);
+ return PCIBIOS_DEVICE_NOT_FOUND;
+ }
+
+ if (dir == READ)
+ ret = pci_bridge_emul_conf_read(&emul->bridge,
+ pos + PCI_STD_HEADER_SIZEOF,
+ len, val);
+ else
+ ret = pci_bridge_emul_conf_write(&emul->bridge,
+ pos + PCI_STD_HEADER_SIZEOF,
+ len, *val);
+ mutex_unlock(&list_lock);
+
+ return ret;
+}
+
+static int pciehp_emul_slot_read(struct pci_dev *dev, int pos, int len, u32 *val)
+{
+ return pciehp_emul_slot_rw(dev, pos, len, val, READ);
+}
+
+static int pciehp_emul_slot_write(struct pci_dev *dev, int pos, int len, u32 val)
+{
+ return pciehp_emul_slot_rw(dev, pos, len, &val, WRITE);
+}
+
+static struct slot_ecap_ops pciehp_emul_slot_ops = {
+ .read = pciehp_emul_slot_read,
+ .write = pciehp_emul_slot_write,
+};
+
+static int pciehp_emul_thread(void *data)
+{
+ struct pciehp_emul *emul = data;
+ struct controller *ctrl = emul->ctrl;
+ struct pci_dev *dev = ctrl->pcie->port;
+ __le16 *emul_lnksta_p = &emul->bridge.pcie_conf.lnksta;
+ __le16 *emul_slotsta_p = &emul->bridge.pcie_conf.slotsta;
+ u16 emul_lnksta, port_lnksta;
+ int ret;
+
+ /* Store initial link status state */
+ ret = pcie_capability_read_word(dev, PCI_EXP_LNKSTA, &port_lnksta);
+ if (!ret && port_lnksta != (u16)~0)
+ *emul_lnksta_p = cpu_to_le16(port_lnksta);
+
+ /* Start with 1 sec delay */
+ schedule_timeout_idle(1 * HZ);
+
+ /*
+ * Data Link Layer Link Active event sets Data Link Layer State Changed
+ * and updates internal link status tracking.
+ */
+ set_current_state(TASK_INTERRUPTIBLE);
+ while (!kthread_should_stop()) {
+ ret = pcie_capability_read_word(dev, PCI_EXP_LNKSTA,
+ &port_lnksta);
+ if (ret || port_lnksta == (u16)~0)
+ goto cont;
+
+ emul_lnksta = le16_to_cpup(emul_lnksta_p);
+ if ((port_lnksta ^ emul_lnksta) & PCI_EXP_LNKSTA_DLLLA) {
+ ctrl_dbg(ctrl, "link change event (%04x->%04x)\n",
+ emul_lnksta, port_lnksta);
+ *emul_lnksta_p = cpu_to_le16(port_lnksta);
+ *emul_slotsta_p |= cpu_to_le16(PCI_EXP_SLTSTA_DLLSC);
+
+ while (pciehp_events_pending(ctrl))
+ pciehp_handle_events(ctrl);
+ }
+
+cont:
+ /* Clamp to sane value */
+ pciehp_emul_time = clamp(pciehp_emul_time, 100U, 60000U);
+ schedule_timeout_idle(msecs_to_jiffies(pciehp_emul_time));
+ }
+
+ return 0;
+}
+
+/**
+ * pciehp_emul_ports_check - Matches pci_dev against pciehp_emul_ports filter
+ * @dev: Struct pci_dev to match against string
+ *
+ * Matches a semi-colon tokenized string list of PCI addresses or vendor-device
+ * IDs to the struct pci_dev. Follows the pci_dev_str_match format in the form:
+ *
+ * [<domain>:]<bus>:<device>.<func>[/<device>.<func>]*
+ * pci:<vendor>:<device>[:<subvendor>:<subdevice>]
+ *
+ * When using the path format, the path for the device can be obtained using
+ * 'lspci -t' and further specified using the upstream bridge and the
+ * downstream port's device-function to be more robust against bus renumbering.
+ *
+ * When using the vendor-device format, a value of '0' in any field acts as a
+ * wildcard for that field, matching all values.
+ *
+ * Returns 0 on match or null/empty string.
+ */
+static int pciehp_emul_ports_check(struct pci_dev *dev)
+{
+ const char *p = pciehp_emul_ports;
+ int ret;
+
+ /* Null or empty string matches all devices */
+ if (!p || !*p)
+ return 0;
+
+ while (*p) {
+ ret = pci_dev_str_match(dev, p, &p);
+ if (ret < 0) {
+ pr_info_once("Can't parse pciehp_emul_ports parameter: %s\n",
+ pciehp_emul_ports);
+ return ret;
+ }
+
+ /* Found a match */
+ if (ret == 1)
+ return 0;
+
+ if (*p != ';' && *p != ',') {
+ /* End of param or invalid format */
+ return -ENODEV;
+ }
+ p++;
+ }
+
+ return -ENODEV;
+}
+
+bool pciehp_emul_will_manage(struct pci_dev *dev)
+{
+ u32 lnkcap;
+ u16 sltcap;
+ int ret;
+
+ if (pciehp_emul_mode == PCIEHP_EMUL_OFF)
+ return false;
+
+ /* A physical slot must be present for emulation to be useful. */
+ if (!(pcie_caps_reg(dev) & PCI_EXP_FLAGS_SLOT))
+ return false;
+
+ /* Match a port BDF/ID descriptor if given */
+ if (pciehp_emul_ports_check(dev))
+ return false;
+
+ /* Port must support Data Link Layer Link Active Reporting. */
+ ret = pcie_capability_read_dword(dev, PCI_EXP_LNKCAP, &lnkcap);
+ if (ret || lnkcap == (u32)~0)
+ return false;
+ if (!(lnkcap & PCI_EXP_LNKCAP_DLLLARC))
+ return false;
+
+ /* Only allow management of hotplug-capable ports in 'force' mode. */
+ ret = pcie_capability_read_word(dev, PCI_EXP_SLTCAP, &sltcap);
+ if (ret || sltcap == (u16)~0)
+ return false;
+ if (sltcap & PCI_EXP_SLTCAP_HPC && pciehp_emul_mode != PCIEHP_EMUL_FORCE)
+ return false;
+
+ return true;
+}
+
+int pciehp_emul_attach(struct controller *ctrl)
+{
+ struct pciehp_emul *emul;
+ struct pci_bridge_emul *bridge;
+ struct pci_dev *dev = ctrl->pcie->port;
+ u32 slotcap = 0;
+ int ret;
+
+ if (!pciehp_emul_will_manage(dev))
+ return 0;
+
+ emul = kzalloc(sizeof(*emul), GFP_KERNEL);
+ if (!emul)
+ return -ENOMEM;
+
+ ret = pcie_capability_read_dword(dev, PCI_EXP_SLTCAP, &slotcap);
+ if (ret)
+ goto free_emul;
+
+ emul->ctrl = ctrl;
+ bridge = &emul->bridge;
+ bridge->has_pcie = true;
+ bridge->data = emul;
+ bridge->ops = &pciehp_emul_pci_bridge_ops;
+ pci_bridge_emul_init(bridge, 0);
+
+ pci_bridge_emul_set_reg_behavior(bridge, true, PCI_EXP_SLTCTL,
+ ~PCI_EXP_SLTCTL_DLLSCE,
+ PCI_BRIDGE_EMUL_REG_BEHAVIOR_RO);
+
+ bridge->pcie_conf.slotcap = cpu_to_le32(PCI_EXP_SLTCAP_HPS |
+ PCI_EXP_SLTCAP_HPC |
+ PCI_EXP_SLTCAP_NCCS |
+ (slotcap & PCI_EXP_SLTCAP_PSN));
+ bridge->pcie_conf.slotsta = cpu_to_le16(PCI_EXP_SLTSTA_PDS);
+
+ mutex_lock(&list_lock);
+ list_add_tail(&emul->node, &pciehp_emul_list);
+ emul->old_ops = ctrl->slot_ops;
+ ctrl->slot_ops = &pciehp_emul_slot_ops;
+ mutex_unlock(&list_lock);
+
+ emul->poll_thread = kthread_run(&pciehp_emul_thread, emul,
+ "emul-%05x:%04x",
+ pci_domain_nr(dev->bus),
+ pci_dev_id(dev));
+ ret = PTR_ERR_OR_ZERO(emul->poll_thread);
+ if (ret)
+ goto del_node;
+
+ ctrl_info(ctrl, "PCIe Hotplug Emulation active -- use at own risk!\n");
+
+ return 0;
+
+del_node:
+ mutex_lock(&list_lock);
+ ctrl->slot_ops = emul->old_ops;
+ list_del(&emul->node);
+ mutex_unlock(&list_lock);
+free_emul:
+ kfree(emul);
+ return ret;
+}
+
+static void pciehp_emul_stop_child_threads(struct pci_dev *dev)
+{
+ struct pciehp_emul *emul;
+ struct pci_bus *bus = dev->subordinate;
+
+ if (!bus)
+ return;
+
+ list_for_each_entry(dev, &bus->devices, bus_list) {
+ pciehp_emul_stop_child_threads(dev);
+
+ emul = pci_dev_to_pciehp_emul(dev);
+ if (emul)
+ kthread_stop(emul->poll_thread);
+ }
+}
+
+void pciehp_emul_detach(struct controller *ctrl)
+{
+ struct pciehp_emul *emul;
+
+ mutex_lock(&list_lock);
+ pciehp_emul_stop_child_threads(ctrl->pcie->port);
+
+ emul = pci_dev_to_pciehp_emul(ctrl->pcie->port);
+ if (emul) {
+ kthread_stop(emul->poll_thread);
+ ctrl->slot_ops = emul->old_ops;
+ list_del(&emul->node);
+ }
+ mutex_unlock(&list_lock);
+
+ kfree(emul);
+}
diff --git a/drivers/pci/pcie/Kconfig b/drivers/pci/pcie/Kconfig
index 6e3c04b..2e4e5a3 100644
--- a/drivers/pci/pcie/Kconfig
+++ b/drivers/pci/pcie/Kconfig
@@ -21,6 +21,20 @@ config HOTPLUG_PCI_PCIE
When in doubt, say N.
+config HOTPLUG_PCI_PCIE_EMUL
+ bool "PCI Express Hotplug Emulation driver"
+ depends on HOTPLUG_PCI_PCIE
+ select PCI_BRIDGE_EMUL
+ select PCIEAER
+ help
+ This enables a PCI Express Native Hotplug emulation driver for
+ non-hotplug-capable PCI Express slots. Hotplug is determined using
+ link events and this driver can be disabled, or manage either only
+ non-hotplug-capable slots or both non-hotplug and hotplug slots.
+ Specific slots may also be targeted.
+
+ When in doubt, say N.
+
config PCIEAER
bool "PCI Express Advanced Error Reporting support"
depends on PCIEPORTBUS
--
1.8.3.1
^ permalink raw reply related [flat|nested] 21+ messages in thread* [RFC 9/9] PCI: pciehp: Wire up pcie_port_emulate_slot and pciehp_emul
2020-02-07 23:59 [RFC 0/9] PCIe Hotplug Slot Emulation driver Jon Derrick
` (7 preceding siblings ...)
2020-02-08 0:00 ` [RFC 8/9] PCI: pciehp: Add hotplug slot emulation driver Jon Derrick
@ 2020-02-08 0:00 ` Jon Derrick
2020-02-10 7:01 ` [RFC 0/9] PCIe Hotplug Slot Emulation driver Christoph Hellwig
2020-03-28 21:51 ` Bjorn Helgaas
10 siblings, 0 replies; 21+ messages in thread
From: Jon Derrick @ 2020-02-08 0:00 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: linux-pci, linux-kernel, Andy Shevchenko, Mika Westerberg,
Pawel Baldysiak, Sinan Kaya, Lorenzo Pieralisi, Keith Busch,
Alexandru Gagniuc, Christoph Hellwig, Jon Derrick
Add pcie_port_slot_emulated hook and wire it up to pciehp_emul. Add
pciehp_emul_{attach,detach} calls in pciehp and move the management
check into the attach caller.
Signed-off-by: Jon Derrick <jonathan.derrick@intel.com>
---
drivers/pci/hotplug/pciehp.h | 18 ++++++++++++++++++
drivers/pci/hotplug/pciehp_emul.c | 11 ++++++++---
drivers/pci/hotplug/pciehp_hpc.c | 4 ++++
drivers/pci/pcie/portdrv_core.c | 3 +++
include/linux/pci.h | 4 ++++
5 files changed, 37 insertions(+), 3 deletions(-)
diff --git a/drivers/pci/hotplug/pciehp.h b/drivers/pci/hotplug/pciehp.h
index 48fdd62..035d44d 100644
--- a/drivers/pci/hotplug/pciehp.h
+++ b/drivers/pci/hotplug/pciehp.h
@@ -197,6 +197,24 @@ struct controller {
int pciehp_set_raw_indicator_status(struct hotplug_slot *h_slot, u8 status);
int pciehp_get_raw_indicator_status(struct hotplug_slot *h_slot, u8 *status);
+#ifdef CONFIG_HOTPLUG_PCI_PCIE_EMUL
+bool pciehp_emul_will_manage(struct pci_dev *dev);
+int pciehp_emul_attach(struct controller *ctrl);
+void pciehp_emul_detach(struct controller *ctrl);
+#else
+static inline bool pciehp_emul_will_manage(struct pci_dev *dev)
+{
+ return false;
+};
+
+static inline int pciehp_emul_attach(struct controller *ctrl)
+{
+ return 0;
+};
+
+static inline void pciehp_emul_detach(struct controller *ctrl) {};
+#endif
+
static inline const char *slot_name(struct controller *ctrl)
{
return hotplug_slot_name(&ctrl->hotplug_slot);
diff --git a/drivers/pci/hotplug/pciehp_emul.c b/drivers/pci/hotplug/pciehp_emul.c
index 1a1fb51..b41bbae 100644
--- a/drivers/pci/hotplug/pciehp_emul.c
+++ b/drivers/pci/hotplug/pciehp_emul.c
@@ -281,9 +281,6 @@ int pciehp_emul_attach(struct controller *ctrl)
u32 slotcap = 0;
int ret;
- if (!pciehp_emul_will_manage(dev))
- return 0;
-
emul = kzalloc(sizeof(*emul), GFP_KERNEL);
if (!emul)
return -ENOMEM;
@@ -371,3 +368,11 @@ void pciehp_emul_detach(struct controller *ctrl)
kfree(emul);
}
+
+static int __init pciehp_emul_init(void)
+{
+ pcie_port_slot_emulated = &pciehp_emul_will_manage;
+
+ return 0;
+}
+postcore_initcall(pciehp_emul_init);
diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
index e46f177..e034ab0 100644
--- a/drivers/pci/hotplug/pciehp_hpc.c
+++ b/drivers/pci/hotplug/pciehp_hpc.c
@@ -922,6 +922,10 @@ struct controller *pcie_init(struct pcie_device *dev)
ctrl->pcie = dev;
ctrl->slot_ops = &pciehp_default_slot_ops;
+
+ if (pciehp_emul_will_manage(pdev) && pciehp_emul_attach(ctrl))
+ return NULL;
+
pcie_read_slot_capabilities(ctrl, &slot_cap);
if (pdev->hotplug_user_indicators)
diff --git a/drivers/pci/pcie/portdrv_core.c b/drivers/pci/pcie/portdrv_core.c
index 5979bb7..a1efaf7 100644
--- a/drivers/pci/pcie/portdrv_core.c
+++ b/drivers/pci/pcie/portdrv_core.c
@@ -19,6 +19,9 @@
#include "../pci.h"
#include "portdrv.h"
+/* Hook for hotplug emulation driver */
+bool (*pcie_port_slot_emulated)(struct pci_dev *dev);
+
struct portdrv_service_data {
struct pcie_port_service_driver *drv;
struct device *dev;
diff --git a/include/linux/pci.h b/include/linux/pci.h
index c5c9bb5..3b48968 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1538,7 +1538,11 @@ static inline int pci_irqd_intx_xlate(struct irq_domain *d,
#define pcie_ports_native false
#endif
+#ifdef CONFIG_HOTPLUG_PCI_PCIE_EMUL
+extern bool (*pcie_port_slot_emulated)(struct pci_dev *dev);
+#else
#define pcie_port_slot_emulated(dev) false
+#endif
#define PCIE_LINK_STATE_L0S BIT(0)
#define PCIE_LINK_STATE_L1 BIT(1)
--
1.8.3.1
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [RFC 0/9] PCIe Hotplug Slot Emulation driver
2020-02-07 23:59 [RFC 0/9] PCIe Hotplug Slot Emulation driver Jon Derrick
` (8 preceding siblings ...)
2020-02-08 0:00 ` [RFC 9/9] PCI: pciehp: Wire up pcie_port_emulate_slot and pciehp_emul Jon Derrick
@ 2020-02-10 7:01 ` Christoph Hellwig
2020-02-10 15:05 ` Derrick, Jonathan
2020-03-28 21:51 ` Bjorn Helgaas
10 siblings, 1 reply; 21+ messages in thread
From: Christoph Hellwig @ 2020-02-10 7:01 UTC (permalink / raw)
To: Jon Derrick
Cc: Bjorn Helgaas, linux-pci, linux-kernel, Andy Shevchenko,
Mika Westerberg, Pawel Baldysiak, Sinan Kaya, Lorenzo Pieralisi,
Keith Busch, Alexandru Gagniuc, Christoph Hellwig
On Fri, Feb 07, 2020 at 04:59:58PM -0700, Jon Derrick wrote:
> This set adds an emulation driver for PCIe Hotplug. There may be platforms with
> specific configurations that can support hotplug but don't provide the logical
> slot hotplug hardware. For instance, the platform may use an
> electrically-tolerant interposer between the slot and the device.
The code seems like one giant hack to me. What is the real life
use case for this? Another Intel chipset fuckup like vmd or the ahci
remapping?
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [RFC 0/9] PCIe Hotplug Slot Emulation driver
2020-02-10 7:01 ` [RFC 0/9] PCIe Hotplug Slot Emulation driver Christoph Hellwig
@ 2020-02-10 15:05 ` Derrick, Jonathan
2020-02-10 16:58 ` hch
0 siblings, 1 reply; 21+ messages in thread
From: Derrick, Jonathan @ 2020-02-10 15:05 UTC (permalink / raw)
To: hch
Cc: mr.nuke.me, Shevchenko, Andriy, lorenzo.pieralisi, linux-kernel,
mika.westerberg, helgaas, linux-pci, Baldysiak, Pawel, okaya,
kbusch
On Mon, 2020-02-10 at 08:01 +0100, Christoph Hellwig wrote:
> On Fri, Feb 07, 2020 at 04:59:58PM -0700, Jon Derrick wrote:
> > This set adds an emulation driver for PCIe Hotplug. There may be platforms with
> > specific configurations that can support hotplug but don't provide the logical
> > slot hotplug hardware. For instance, the platform may use an
> > electrically-tolerant interposer between the slot and the device.
>
> The code seems like one giant hack to me. What is the real life
> use case for this? Another Intel chipset fuckup like vmd or the ahci
> remapping?
>
Exactly as the cover letter describes. An interposer being used on a
non-hotplug slot.
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [RFC 0/9] PCIe Hotplug Slot Emulation driver
2020-02-10 15:05 ` Derrick, Jonathan
@ 2020-02-10 16:58 ` hch
2020-02-10 17:09 ` Derrick, Jonathan
0 siblings, 1 reply; 21+ messages in thread
From: hch @ 2020-02-10 16:58 UTC (permalink / raw)
To: Derrick, Jonathan
Cc: hch, mr.nuke.me, Shevchenko, Andriy, lorenzo.pieralisi,
linux-kernel, mika.westerberg, helgaas, linux-pci, Baldysiak,
Pawel, okaya, kbusch
On Mon, Feb 10, 2020 at 03:05:47PM +0000, Derrick, Jonathan wrote:
> > The code seems like one giant hack to me. What is the real life
> > use case for this? Another Intel chipset fuckup like vmd or the ahci
> > remapping?
> >
> Exactly as the cover letter describes. An interposer being used on a
> non-hotplug slot.
That isn't a use a case, that iѕ a description of the implementation.
Why would you want this code?
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [RFC 0/9] PCIe Hotplug Slot Emulation driver
2020-02-10 16:58 ` hch
@ 2020-02-10 17:09 ` Derrick, Jonathan
0 siblings, 0 replies; 21+ messages in thread
From: Derrick, Jonathan @ 2020-02-10 17:09 UTC (permalink / raw)
To: hch
Cc: Shevchenko, Andriy, lorenzo.pieralisi, linux-kernel,
mika.westerberg, Baldysiak, Pawel, helgaas, linux-pci,
mr.nuke.me, kbusch, okaya
On Mon, 2020-02-10 at 17:58 +0100, hch@lst.de wrote:
> On Mon, Feb 10, 2020 at 03:05:47PM +0000, Derrick, Jonathan wrote:
> > > The code seems like one giant hack to me. What is the real life
> > > use case for this? Another Intel chipset fuckup like vmd or the ahci
> > > remapping?
> > >
> > Exactly as the cover letter describes. An interposer being used on a
> > non-hotplug slot.
>
> That isn't a use a case, that iѕ a description of the implementation.
> Why would you want this code?
It allows non-hotplug slots to take advantage of the kernel's robust
hotplug ecosystem, if the platform configuration can tolerate the
events. This could also reduce BOM cost by eliminating some slot
controllers. Say you had something that only needed to be hotplugged
very infrequently, like RAIDed OS drives, versus something needing to
be hotplugged very frequently like data drives.
Granted it probably could be fit into pciehp_poll, but it seemed to
have a different objective (emulating slot)
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [RFC 0/9] PCIe Hotplug Slot Emulation driver
2020-02-07 23:59 [RFC 0/9] PCIe Hotplug Slot Emulation driver Jon Derrick
` (9 preceding siblings ...)
2020-02-10 7:01 ` [RFC 0/9] PCIe Hotplug Slot Emulation driver Christoph Hellwig
@ 2020-03-28 21:51 ` Bjorn Helgaas
2020-03-30 17:43 ` Derrick, Jonathan
10 siblings, 1 reply; 21+ messages in thread
From: Bjorn Helgaas @ 2020-03-28 21:51 UTC (permalink / raw)
To: Jon Derrick
Cc: linux-pci, linux-kernel, Andy Shevchenko, Mika Westerberg,
Pawel Baldysiak, Sinan Kaya, Lorenzo Pieralisi, Keith Busch,
Alexandru Gagniuc, Christoph Hellwig, Stuart Hayes, Lukas Wunner
[+cc Stuart, Lukas]
On Fri, Feb 07, 2020 at 04:59:58PM -0700, Jon Derrick wrote:
> This set adds an emulation driver for PCIe Hotplug. There may be platforms with
> specific configurations that can support hotplug but don't provide the logical
> slot hotplug hardware. For instance, the platform may use an
> electrically-tolerant interposer between the slot and the device.
>
> This driver utilizes the pci-bridge-emul architecture to manage register reads
> and writes. The underlying functionality of the hotplug emulation driver uses
> the Data Link Layer Link Active Reporting mechanism in a polling loop, but can
> tolerate other event sources such as AER or DPC.
>
> When enabled and a slot is managed by the driver, all port services are managed
> by the kernel. This is done to ensure that firmware hotplug and error
> architecture does not (correctly) halt/machine check the system when hotplug is
> performed on a non-hotplug slot.
>
> The driver offers two active mode: Auto and Force.
> auto: The driver will bind to non-hotplug slots
> force: The driver will bind to all slots and overrides the slot's services
>
> There are three kernel params:
> pciehp.pciehp_emul_mode={off, auto, force}
> pciehp.pciehp_emul_time=<msecs polling time> (def 1000, min 100, max 60000)
> pciehp.pciehp_emul_ports=<PCI [S]BDF/ID format string>
>
> The pciehp_emul_ports kernel parameter takes a semi-colon tokenized string
> representing PCI [S]BDFs and IDs. The pciehp_emul_mode will then be applied to
> only those slots, leaving other slots unmanaged by pciehp_emul.
>
> The string follows the pci_dev_str_match() format:
>
> [<domain>:]<bus>:<device>.<func>[/<device>.<func>]*
> pci:<vendor>:<device>[:<subvendor>:<subdevice>]
>
> When using the path format, the path for the device can be obtained
> using 'lspci -t' and further specified using the upstream bridge and the
> downstream port's device-function to be more robust against bus
> renumbering.
>
> When using the vendor-device format, a value of '0' in any field acts as
> a wildcard for that field, matching all values.
>
> The driver is enabled with CONFIG_HOTPLUG_PCI_PCIE_EMUL=y.
>
> The driver should be considered 'use at own risk' unless the platform/hardware
> vendor recommends this mode.
There's a lot of good work in here, and I don't claim to understand
the use case and all the benefits.
But it seems like quite a lot of additional code and complexity in an
area that's already pretty subtle, so I'm not yet convinced that it's
all worthwhile.
It seems like this would rely on Data Link Layer Link Active
Reporting. Is that something we could add to pciehp as a generic
feature without making a separate driver for it? I haven't looked at
this for a while, but I would assume that if we find out that a link
went down, pciehp could/should be smart enough to notice that even if
it didn't come via the usual pciehp Slot Status path.
> Jon Derrick (9):
> PCI: pci-bridge-emul: Update PCIe register behaviors
> PCI: pci-bridge-emul: Eliminate reserved member
> PCI: pci-bridge-emul: Provide a helper to set behavior
> PCI: pciehp: Indirect slot register operations
> PCI: Add pcie_port_slot_emulated stub
> PCI: pciehp: Expose the poll loop to other drivers
> PCI: Move pci_dev_str_match to search.c
> PCI: pciehp: Add hotplug slot emulation driver
> PCI: pciehp: Wire up pcie_port_emulate_slot and pciehp_emul
>
> drivers/pci/hotplug/Makefile | 4 +
> drivers/pci/hotplug/pciehp.h | 28 +++
> drivers/pci/hotplug/pciehp_emul.c | 378 ++++++++++++++++++++++++++++++++++++++
> drivers/pci/hotplug/pciehp_hpc.c | 136 ++++++++++----
> drivers/pci/pci-acpi.c | 3 +
> drivers/pci/pci-bridge-emul.c | 95 +++++-----
> drivers/pci/pci-bridge-emul.h | 10 +
> drivers/pci/pci.c | 163 ----------------
> drivers/pci/pcie/Kconfig | 14 ++
> drivers/pci/pcie/portdrv_core.c | 14 +-
> drivers/pci/probe.c | 2 +-
> drivers/pci/search.c | 162 ++++++++++++++++
> include/linux/pci.h | 8 +
> 13 files changed, 775 insertions(+), 242 deletions(-)
> create mode 100644 drivers/pci/hotplug/pciehp_emul.c
>
> --
> 1.8.3.1
>
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [RFC 0/9] PCIe Hotplug Slot Emulation driver
2020-03-28 21:51 ` Bjorn Helgaas
@ 2020-03-30 17:43 ` Derrick, Jonathan
2020-03-30 17:49 ` hch
2020-04-01 21:45 ` Bjorn Helgaas
0 siblings, 2 replies; 21+ messages in thread
From: Derrick, Jonathan @ 2020-03-30 17:43 UTC (permalink / raw)
To: helgaas
Cc: mr.nuke.me, hch, Shevchenko, Andriy, lorenzo.pieralisi,
linux-kernel, mika.westerberg, Baldysiak, Pawel, linux-pci,
lukas, okaya, kbusch, stuart.w.hayes
Hi Bjorn,
On Sat, 2020-03-28 at 16:51 -0500, Bjorn Helgaas wrote:
> [+cc Stuart, Lukas]
>
> On Fri, Feb 07, 2020 at 04:59:58PM -0700, Jon Derrick wrote:
> > This set adds an emulation driver for PCIe Hotplug. There may be platforms with
> > specific configurations that can support hotplug but don't provide the logical
> > slot hotplug hardware. For instance, the platform may use an
> > electrically-tolerant interposer between the slot and the device.
> >
> > This driver utilizes the pci-bridge-emul architecture to manage register reads
> > and writes. The underlying functionality of the hotplug emulation driver uses
> > the Data Link Layer Link Active Reporting mechanism in a polling loop, but can
> > tolerate other event sources such as AER or DPC.
> >
> > When enabled and a slot is managed by the driver, all port services are managed
> > by the kernel. This is done to ensure that firmware hotplug and error
> > architecture does not (correctly) halt/machine check the system when hotplug is
> > performed on a non-hotplug slot.
> >
> > The driver offers two active mode: Auto and Force.
> > auto: The driver will bind to non-hotplug slots
> > force: The driver will bind to all slots and overrides the slot's services
> >
> > There are three kernel params:
> > pciehp.pciehp_emul_mode={off, auto, force}
> > pciehp.pciehp_emul_time=<msecs polling time> (def 1000, min 100, max 60000)
> > pciehp.pciehp_emul_ports=<PCI [S]BDF/ID format string>
> >
> > The pciehp_emul_ports kernel parameter takes a semi-colon tokenized string
> > representing PCI [S]BDFs and IDs. The pciehp_emul_mode will then be applied to
> > only those slots, leaving other slots unmanaged by pciehp_emul.
> >
> > The string follows the pci_dev_str_match() format:
> >
> > [<domain>:]<bus>:<device>.<func>[/<device>.<func>]*
> > pci:<vendor>:<device>[:<subvendor>:<subdevice>]
> >
> > When using the path format, the path for the device can be obtained
> > using 'lspci -t' and further specified using the upstream bridge and the
> > downstream port's device-function to be more robust against bus
> > renumbering.
> >
> > When using the vendor-device format, a value of '0' in any field acts as
> > a wildcard for that field, matching all values.
> >
> > The driver is enabled with CONFIG_HOTPLUG_PCI_PCIE_EMUL=y.
> >
> > The driver should be considered 'use at own risk' unless the platform/hardware
> > vendor recommends this mode.
>
> There's a lot of good work in here, and I don't claim to understand
> the use case and all the benefits.
I've received more info that the customer use case is an AIC that
breaks out 1-4 M.2 cards which have been made hotplug tolerant.
>
> But it seems like quite a lot of additional code and complexity in an
> area that's already pretty subtle, so I'm not yet convinced that it's
> all worthwhile.
>
> It seems like this would rely on Data Link Layer Link Active
> Reporting. Is that something we could add to pciehp as a generic
> feature without making a separate driver for it? I haven't looked at
> this for a while, but I would assume that if we find out that a link
> went down, pciehp could/should be smart enough to notice that even if
> it didn't come via the usual pciehp Slot Status path.
I had a plan to do V2 by intercepting bus_ops rather than indirecting
slot_ops in pciehp. That should touch /a lot/ less code.
The problem I saw with adding DLLLA as a primary signal in pciehp is
that most of the pciehp boilerplate relies on valid Slot register
logic. I don't know how reliable pciehp will be if there's no backing
slot register logic, emulated or real. Consider how many slot
capability reads are in hpc.
I could add a non-slot flag check to each of those callers, but it
might be worse than the emulation alternative.
What do you think?
Thanks
>
> > Jon Derrick (9):
> > PCI: pci-bridge-emul: Update PCIe register behaviors
> > PCI: pci-bridge-emul: Eliminate reserved member
> > PCI: pci-bridge-emul: Provide a helper to set behavior
> > PCI: pciehp: Indirect slot register operations
> > PCI: Add pcie_port_slot_emulated stub
> > PCI: pciehp: Expose the poll loop to other drivers
> > PCI: Move pci_dev_str_match to search.c
> > PCI: pciehp: Add hotplug slot emulation driver
> > PCI: pciehp: Wire up pcie_port_emulate_slot and pciehp_emul
> >
> > drivers/pci/hotplug/Makefile | 4 +
> > drivers/pci/hotplug/pciehp.h | 28 +++
> > drivers/pci/hotplug/pciehp_emul.c | 378 ++++++++++++++++++++++++++++++++++++++
> > drivers/pci/hotplug/pciehp_hpc.c | 136 ++++++++++----
> > drivers/pci/pci-acpi.c | 3 +
> > drivers/pci/pci-bridge-emul.c | 95 +++++-----
> > drivers/pci/pci-bridge-emul.h | 10 +
> > drivers/pci/pci.c | 163 ----------------
> > drivers/pci/pcie/Kconfig | 14 ++
> > drivers/pci/pcie/portdrv_core.c | 14 +-
> > drivers/pci/probe.c | 2 +-
> > drivers/pci/search.c | 162 ++++++++++++++++
> > include/linux/pci.h | 8 +
> > 13 files changed, 775 insertions(+), 242 deletions(-)
> > create mode 100644 drivers/pci/hotplug/pciehp_emul.c
> >
> > --
> > 1.8.3.1
> >
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [RFC 0/9] PCIe Hotplug Slot Emulation driver
2020-03-30 17:43 ` Derrick, Jonathan
@ 2020-03-30 17:49 ` hch
2020-04-01 21:45 ` Bjorn Helgaas
1 sibling, 0 replies; 21+ messages in thread
From: hch @ 2020-03-30 17:49 UTC (permalink / raw)
To: Derrick, Jonathan
Cc: helgaas, mr.nuke.me, hch, Shevchenko, Andriy, lorenzo.pieralisi,
linux-kernel, mika.westerberg, Baldysiak, Pawel, linux-pci,
lukas, okaya, kbusch, stuart.w.hayes
On Mon, Mar 30, 2020 at 05:43:33PM +0000, Derrick, Jonathan wrote:
> > There's a lot of good work in here, and I don't claim to understand
> > the use case and all the benefits.
> I've received more info that the customer use case is an AIC that
> breaks out 1-4 M.2 cards which have been made hotplug tolerant.
Which sounds completely bogus. M.2 cards are eletrically not designed
for this at all, and neither is the AIC. If people want to support
similar use cases they need to get them standardized by PCI SIG
and handled by hotplug capable slots.
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [RFC 0/9] PCIe Hotplug Slot Emulation driver
2020-03-30 17:43 ` Derrick, Jonathan
2020-03-30 17:49 ` hch
@ 2020-04-01 21:45 ` Bjorn Helgaas
1 sibling, 0 replies; 21+ messages in thread
From: Bjorn Helgaas @ 2020-04-01 21:45 UTC (permalink / raw)
To: Derrick, Jonathan
Cc: helgaas, mr.nuke.me, hch, Shevchenko, Andriy, lorenzo.pieralisi,
linux-kernel, mika.westerberg, Baldysiak, Pawel, linux-pci,
lukas, okaya, kbusch, stuart.w.hayes
On Mon, Mar 30, 2020 at 12:43 PM Derrick, Jonathan
<jonathan.derrick@intel.com> wrote:
> On Sat, 2020-03-28 at 16:51 -0500, Bjorn Helgaas wrote:
> > On Fri, Feb 07, 2020 at 04:59:58PM -0700, Jon Derrick wrote:
> > > This set adds an emulation driver for PCIe Hotplug. There may be platforms with
> > > specific configurations that can support hotplug but don't provide the logical
> > > slot hotplug hardware. For instance, the platform may use an
> > > electrically-tolerant interposer between the slot and the device.
> > > ...
> > There's a lot of good work in here, and I don't claim to understand
> > the use case and all the benefits.
> I've received more info that the customer use case is an AIC that
> breaks out 1-4 M.2 cards which have been made hotplug tolerant.
>
> > But it seems like quite a lot of additional code and complexity in an
> > area that's already pretty subtle, so I'm not yet convinced that it's
> > all worthwhile.
> >
> > It seems like this would rely on Data Link Layer Link Active
> > Reporting. Is that something we could add to pciehp as a generic
> > feature without making a separate driver for it? I haven't looked at
> > this for a while, but I would assume that if we find out that a link
> > went down, pciehp could/should be smart enough to notice that even if
> > it didn't come via the usual pciehp Slot Status path.
> I had a plan to do V2 by intercepting bus_ops rather than indirecting
> slot_ops in pciehp. That should touch /a lot/ less code.
I assume this is something like pci_bus_set_ops() or
pci_bus_set_aer_ops()? Probably touches less code, but I'm not really
a fan of either of those current situations because they make things
magical -- there's a lot of stuff going on behind the curtain, and it
makes another thing to consider when we evaluate every pciehp change.
> The problem I saw with adding DLLLA as a primary signal in pciehp is
> that most of the pciehp boilerplate relies on valid Slot register
> logic. I don't know how reliable pciehp will be if there's no backing
> slot register logic, emulated or real. Consider how many slot
> capability reads are in hpc.
>
> I could add a non-slot flag check to each of those callers, but it
> might be worse than the emulation alternative.
I see what you mean -- there are quite a few reads of PCI_EXP_SLTSTA.
I'm not 100% sure all of those really need to exist. I would expect
that we'd read it once in the ISR and then operate on that value. So
maybe there's some middle ground of restructuring to remove some of
those reads and making the remaining few more generic.
All that being said, I'm also sympathetic to Christoph's concern about
cluttering pciehp to deal with non-standard topologies. At some point
if you need to do non-standard things you may have to write and
maintain your own drivers. I don't know where that point is yet.
Bjorn
^ permalink raw reply [flat|nested] 21+ messages in thread