* [PATCH v1] igc: Reinstate IGC_REMOVED logic and implement it properly
@ 2022-06-02 15:58 ` Lennert Buytenhek
0 siblings, 0 replies; 6+ messages in thread
From: Lennert Buytenhek @ 2022-06-02 15:58 UTC (permalink / raw)
To: intel-wired-lan, Tony Nguyen, Jesse Brandeburg
Cc: netdev, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Vitaly Lifshits, Sasha Neftin, Aaron Brown,
Jeff Kirsher
The initially merged version of the igc driver code (via commit
146740f9abc4, "igc: Add support for PF") contained the following
IGC_REMOVED checks in the igc_rd32/wr32() MMIO accessors:
u32 igc_rd32(struct igc_hw *hw, u32 reg)
{
u8 __iomem *hw_addr = READ_ONCE(hw->hw_addr);
u32 value = 0;
if (IGC_REMOVED(hw_addr))
return ~value;
value = readl(&hw_addr[reg]);
/* reads should not return all F's */
if (!(~value) && (!reg || !(~readl(hw_addr))))
hw->hw_addr = NULL;
return value;
}
And:
#define wr32(reg, val) \
do { \
u8 __iomem *hw_addr = READ_ONCE((hw)->hw_addr); \
if (!IGC_REMOVED(hw_addr)) \
writel((val), &hw_addr[(reg)]); \
} while (0)
E.g. igb has similar checks in its MMIO accessors, and has a similar
macro E1000_REMOVED, which is implemented as follows:
#define E1000_REMOVED(h) unlikely(!(h))
These checks serve to detect and take note of an 0xffffffff MMIO read
return from the device, which can be caused by a PCIe link flap or some
other kind of PCI bus error, and to avoid performing MMIO reads and
writes from that point onwards.
However, the IGC_REMOVED macro was not originally implemented:
#ifndef IGC_REMOVED
#define IGC_REMOVED(a) (0)
#endif /* IGC_REMOVED */
This led to the IGC_REMOVED logic to be removed entirely in a
subsequent commit (commit 3c215fb18e70, "igc: remove IGC_REMOVED
function"), with the rationale that such checks matter only for
virtualization and that igc does not support virtualization -- but a
PCIe device can become detached even without virtualization being in
use, and without proper checks, a PCIe bus error affecting an igc
adapter will lead to various NULL pointer dereferences, as the first
access after the error will set hw->hw_addr to NULL, and subsequent
accesses will blindly dereference this now-NULL pointer.
This patch reinstates the IGC_REMOVED checks in igc_rd32/wr32(), and
implements IGC_REMOVED the way it is done for igb, by checking for the
unlikely() case of hw_addr being NULL. This change prevents the oopses
seen when a PCIe link flap occurs on an igc adapter.
Fixes: 146740f9abc4 ("igc: Add support for PF")
Signed-off-by: Lennert Buytenhek <buytenh@arista.com>
---
As initially reported on intel-wired-lan@ in February:
https://lists.osuosl.org/pipermail/intel-wired-lan/Week-of-Mon-20220214/027787.html
We're seeing these NULL pointer dereferences hit fairly reproducibly
when rebooting, presumably due to the particularities of reset
sequencing on the boards we see this hit on.
A link flap can be caused by toggling the Secondary Bus Reset bit
in the upstream PCIe bridge's Bridge Control register and can reliably
reproduce this problem.
drivers/net/ethernet/intel/igc/igc_main.c | 3 +++
drivers/net/ethernet/intel/igc/igc_regs.h | 5 ++++-
2 files changed, 7 insertions(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
index 74b2c590ed5d..38e46e9ba8bb 100644
--- a/drivers/net/ethernet/intel/igc/igc_main.c
+++ b/drivers/net/ethernet/intel/igc/igc_main.c
@@ -6171,6 +6171,9 @@ u32 igc_rd32(struct igc_hw *hw, u32 reg)
u8 __iomem *hw_addr = READ_ONCE(hw->hw_addr);
u32 value = 0;
+ if (IGC_REMOVED(hw_addr))
+ return ~value;
+
value = readl(&hw_addr[reg]);
/* reads should not return all F's */
diff --git a/drivers/net/ethernet/intel/igc/igc_regs.h b/drivers/net/ethernet/intel/igc/igc_regs.h
index e197a33d93a0..026c3b65fc37 100644
--- a/drivers/net/ethernet/intel/igc/igc_regs.h
+++ b/drivers/net/ethernet/intel/igc/igc_regs.h
@@ -306,7 +306,8 @@ u32 igc_rd32(struct igc_hw *hw, u32 reg);
#define wr32(reg, val) \
do { \
u8 __iomem *hw_addr = READ_ONCE((hw)->hw_addr); \
- writel((val), &hw_addr[(reg)]); \
+ if (!IGC_REMOVED(hw_addr)) \
+ writel((val), &hw_addr[(reg)]); \
} while (0)
#define rd32(reg) (igc_rd32(hw, reg))
@@ -318,4 +319,6 @@ do { \
#define array_rd32(reg, offset) (igc_rd32(hw, (reg) + ((offset) << 2)))
+#define IGC_REMOVED(h) unlikely(!(h))
+
#endif
--
2.36.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [Intel-wired-lan] [PATCH v1] igc: Reinstate IGC_REMOVED logic and implement it properly
@ 2022-06-02 15:58 ` Lennert Buytenhek
0 siblings, 0 replies; 6+ messages in thread
From: Lennert Buytenhek @ 2022-06-02 15:58 UTC (permalink / raw)
To: intel-wired-lan, Tony Nguyen, Jesse Brandeburg
Cc: netdev, Eric Dumazet, Jeff Kirsher, Jakub Kicinski, Paolo Abeni,
David S. Miller
The initially merged version of the igc driver code (via commit
146740f9abc4, "igc: Add support for PF") contained the following
IGC_REMOVED checks in the igc_rd32/wr32() MMIO accessors:
u32 igc_rd32(struct igc_hw *hw, u32 reg)
{
u8 __iomem *hw_addr = READ_ONCE(hw->hw_addr);
u32 value = 0;
if (IGC_REMOVED(hw_addr))
return ~value;
value = readl(&hw_addr[reg]);
/* reads should not return all F's */
if (!(~value) && (!reg || !(~readl(hw_addr))))
hw->hw_addr = NULL;
return value;
}
And:
#define wr32(reg, val) \
do { \
u8 __iomem *hw_addr = READ_ONCE((hw)->hw_addr); \
if (!IGC_REMOVED(hw_addr)) \
writel((val), &hw_addr[(reg)]); \
} while (0)
E.g. igb has similar checks in its MMIO accessors, and has a similar
macro E1000_REMOVED, which is implemented as follows:
#define E1000_REMOVED(h) unlikely(!(h))
These checks serve to detect and take note of an 0xffffffff MMIO read
return from the device, which can be caused by a PCIe link flap or some
other kind of PCI bus error, and to avoid performing MMIO reads and
writes from that point onwards.
However, the IGC_REMOVED macro was not originally implemented:
#ifndef IGC_REMOVED
#define IGC_REMOVED(a) (0)
#endif /* IGC_REMOVED */
This led to the IGC_REMOVED logic to be removed entirely in a
subsequent commit (commit 3c215fb18e70, "igc: remove IGC_REMOVED
function"), with the rationale that such checks matter only for
virtualization and that igc does not support virtualization -- but a
PCIe device can become detached even without virtualization being in
use, and without proper checks, a PCIe bus error affecting an igc
adapter will lead to various NULL pointer dereferences, as the first
access after the error will set hw->hw_addr to NULL, and subsequent
accesses will blindly dereference this now-NULL pointer.
This patch reinstates the IGC_REMOVED checks in igc_rd32/wr32(), and
implements IGC_REMOVED the way it is done for igb, by checking for the
unlikely() case of hw_addr being NULL. This change prevents the oopses
seen when a PCIe link flap occurs on an igc adapter.
Fixes: 146740f9abc4 ("igc: Add support for PF")
Signed-off-by: Lennert Buytenhek <buytenh@arista.com>
---
As initially reported on intel-wired-lan@ in February:
https://lists.osuosl.org/pipermail/intel-wired-lan/Week-of-Mon-20220214/027787.html
We're seeing these NULL pointer dereferences hit fairly reproducibly
when rebooting, presumably due to the particularities of reset
sequencing on the boards we see this hit on.
A link flap can be caused by toggling the Secondary Bus Reset bit
in the upstream PCIe bridge's Bridge Control register and can reliably
reproduce this problem.
drivers/net/ethernet/intel/igc/igc_main.c | 3 +++
drivers/net/ethernet/intel/igc/igc_regs.h | 5 ++++-
2 files changed, 7 insertions(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
index 74b2c590ed5d..38e46e9ba8bb 100644
--- a/drivers/net/ethernet/intel/igc/igc_main.c
+++ b/drivers/net/ethernet/intel/igc/igc_main.c
@@ -6171,6 +6171,9 @@ u32 igc_rd32(struct igc_hw *hw, u32 reg)
u8 __iomem *hw_addr = READ_ONCE(hw->hw_addr);
u32 value = 0;
+ if (IGC_REMOVED(hw_addr))
+ return ~value;
+
value = readl(&hw_addr[reg]);
/* reads should not return all F's */
diff --git a/drivers/net/ethernet/intel/igc/igc_regs.h b/drivers/net/ethernet/intel/igc/igc_regs.h
index e197a33d93a0..026c3b65fc37 100644
--- a/drivers/net/ethernet/intel/igc/igc_regs.h
+++ b/drivers/net/ethernet/intel/igc/igc_regs.h
@@ -306,7 +306,8 @@ u32 igc_rd32(struct igc_hw *hw, u32 reg);
#define wr32(reg, val) \
do { \
u8 __iomem *hw_addr = READ_ONCE((hw)->hw_addr); \
- writel((val), &hw_addr[(reg)]); \
+ if (!IGC_REMOVED(hw_addr)) \
+ writel((val), &hw_addr[(reg)]); \
} while (0)
#define rd32(reg) (igc_rd32(hw, reg))
@@ -318,4 +319,6 @@ do { \
#define array_rd32(reg, offset) (igc_rd32(hw, (reg) + ((offset) << 2)))
+#define IGC_REMOVED(h) unlikely(!(h))
+
#endif
--
2.36.1
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [Intel-wired-lan] [PATCH v1] igc: Reinstate IGC_REMOVED logic and implement it properly
2022-06-02 15:58 ` [Intel-wired-lan] " Lennert Buytenhek
@ 2022-06-30 7:23 ` naamax.meir
-1 siblings, 0 replies; 6+ messages in thread
From: naamax.meir @ 2022-06-30 7:23 UTC (permalink / raw)
To: Lennert Buytenhek, intel-wired-lan, Tony Nguyen, Jesse Brandeburg
Cc: netdev, Eric Dumazet, Jeff Kirsher, Jakub Kicinski, Paolo Abeni,
David S. Miller
On 6/2/2022 18:58, Lennert Buytenhek wrote:
> The initially merged version of the igc driver code (via commit
> 146740f9abc4, "igc: Add support for PF") contained the following
> IGC_REMOVED checks in the igc_rd32/wr32() MMIO accessors:
>
> u32 igc_rd32(struct igc_hw *hw, u32 reg)
> {
> u8 __iomem *hw_addr = READ_ONCE(hw->hw_addr);
> u32 value = 0;
>
> if (IGC_REMOVED(hw_addr))
> return ~value;
>
> value = readl(&hw_addr[reg]);
>
> /* reads should not return all F's */
> if (!(~value) && (!reg || !(~readl(hw_addr))))
> hw->hw_addr = NULL;
>
> return value;
> }
>
> And:
>
> #define wr32(reg, val) \
> do { \
> u8 __iomem *hw_addr = READ_ONCE((hw)->hw_addr); \
> if (!IGC_REMOVED(hw_addr)) \
> writel((val), &hw_addr[(reg)]); \
> } while (0)
>
> E.g. igb has similar checks in its MMIO accessors, and has a similar
> macro E1000_REMOVED, which is implemented as follows:
>
> #define E1000_REMOVED(h) unlikely(!(h))
>
> These checks serve to detect and take note of an 0xffffffff MMIO read
> return from the device, which can be caused by a PCIe link flap or some
> other kind of PCI bus error, and to avoid performing MMIO reads and
> writes from that point onwards.
>
> However, the IGC_REMOVED macro was not originally implemented:
>
> #ifndef IGC_REMOVED
> #define IGC_REMOVED(a) (0)
> #endif /* IGC_REMOVED */
>
> This led to the IGC_REMOVED logic to be removed entirely in a
> subsequent commit (commit 3c215fb18e70, "igc: remove IGC_REMOVED
> function"), with the rationale that such checks matter only for
> virtualization and that igc does not support virtualization -- but a
> PCIe device can become detached even without virtualization being in
> use, and without proper checks, a PCIe bus error affecting an igc
> adapter will lead to various NULL pointer dereferences, as the first
> access after the error will set hw->hw_addr to NULL, and subsequent
> accesses will blindly dereference this now-NULL pointer.
>
> This patch reinstates the IGC_REMOVED checks in igc_rd32/wr32(), and
> implements IGC_REMOVED the way it is done for igb, by checking for the
> unlikely() case of hw_addr being NULL. This change prevents the oopses
> seen when a PCIe link flap occurs on an igc adapter.
>
> Fixes: 146740f9abc4 ("igc: Add support for PF")
> Signed-off-by: Lennert Buytenhek <buytenh@arista.com>
> ---
> As initially reported on intel-wired-lan@ in February:
>
> https://lists.osuosl.org/pipermail/intel-wired-lan/Week-of-Mon-20220214/027787.html
>
> We're seeing these NULL pointer dereferences hit fairly reproducibly
> when rebooting, presumably due to the particularities of reset
> sequencing on the boards we see this hit on.
>
> A link flap can be caused by toggling the Secondary Bus Reset bit
> in the upstream PCIe bridge's Bridge Control register and can reliably
> reproduce this problem.
>
> drivers/net/ethernet/intel/igc/igc_main.c | 3 +++
> drivers/net/ethernet/intel/igc/igc_regs.h | 5 ++++-
> 2 files changed, 7 insertions(+), 1 deletion(-)
Tested-by: Naama Meir <naamax.meir@linux.intel.com>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Intel-wired-lan] [PATCH v1] igc: Reinstate IGC_REMOVED logic and implement it properly
@ 2022-06-30 7:23 ` naamax.meir
0 siblings, 0 replies; 6+ messages in thread
From: naamax.meir @ 2022-06-30 7:23 UTC (permalink / raw)
To: Lennert Buytenhek, intel-wired-lan, Tony Nguyen, Jesse Brandeburg
Cc: netdev, Eric Dumazet, Jeff Kirsher, Jakub Kicinski, Paolo Abeni,
David S. Miller
On 6/2/2022 18:58, Lennert Buytenhek wrote:
> The initially merged version of the igc driver code (via commit
> 146740f9abc4, "igc: Add support for PF") contained the following
> IGC_REMOVED checks in the igc_rd32/wr32() MMIO accessors:
>
> u32 igc_rd32(struct igc_hw *hw, u32 reg)
> {
> u8 __iomem *hw_addr = READ_ONCE(hw->hw_addr);
> u32 value = 0;
>
> if (IGC_REMOVED(hw_addr))
> return ~value;
>
> value = readl(&hw_addr[reg]);
>
> /* reads should not return all F's */
> if (!(~value) && (!reg || !(~readl(hw_addr))))
> hw->hw_addr = NULL;
>
> return value;
> }
>
> And:
>
> #define wr32(reg, val) \
> do { \
> u8 __iomem *hw_addr = READ_ONCE((hw)->hw_addr); \
> if (!IGC_REMOVED(hw_addr)) \
> writel((val), &hw_addr[(reg)]); \
> } while (0)
>
> E.g. igb has similar checks in its MMIO accessors, and has a similar
> macro E1000_REMOVED, which is implemented as follows:
>
> #define E1000_REMOVED(h) unlikely(!(h))
>
> These checks serve to detect and take note of an 0xffffffff MMIO read
> return from the device, which can be caused by a PCIe link flap or some
> other kind of PCI bus error, and to avoid performing MMIO reads and
> writes from that point onwards.
>
> However, the IGC_REMOVED macro was not originally implemented:
>
> #ifndef IGC_REMOVED
> #define IGC_REMOVED(a) (0)
> #endif /* IGC_REMOVED */
>
> This led to the IGC_REMOVED logic to be removed entirely in a
> subsequent commit (commit 3c215fb18e70, "igc: remove IGC_REMOVED
> function"), with the rationale that such checks matter only for
> virtualization and that igc does not support virtualization -- but a
> PCIe device can become detached even without virtualization being in
> use, and without proper checks, a PCIe bus error affecting an igc
> adapter will lead to various NULL pointer dereferences, as the first
> access after the error will set hw->hw_addr to NULL, and subsequent
> accesses will blindly dereference this now-NULL pointer.
>
> This patch reinstates the IGC_REMOVED checks in igc_rd32/wr32(), and
> implements IGC_REMOVED the way it is done for igb, by checking for the
> unlikely() case of hw_addr being NULL. This change prevents the oopses
> seen when a PCIe link flap occurs on an igc adapter.
>
> Fixes: 146740f9abc4 ("igc: Add support for PF")
> Signed-off-by: Lennert Buytenhek <buytenh@arista.com>
> ---
> As initially reported on intel-wired-lan@ in February:
>
> https://lists.osuosl.org/pipermail/intel-wired-lan/Week-of-Mon-20220214/027787.html
>
> We're seeing these NULL pointer dereferences hit fairly reproducibly
> when rebooting, presumably due to the particularities of reset
> sequencing on the boards we see this hit on.
>
> A link flap can be caused by toggling the Secondary Bus Reset bit
> in the upstream PCIe bridge's Bridge Control register and can reliably
> reproduce this problem.
>
> drivers/net/ethernet/intel/igc/igc_main.c | 3 +++
> drivers/net/ethernet/intel/igc/igc_regs.h | 5 ++++-
> 2 files changed, 7 insertions(+), 1 deletion(-)
Tested-by: Naama Meir <naamax.meir@linux.intel.com>
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v1] igc: Reinstate IGC_REMOVED logic and implement it properly
2022-06-02 15:58 ` [Intel-wired-lan] " Lennert Buytenhek
@ 2022-07-05 4:23 ` Neftin, Sasha
-1 siblings, 0 replies; 6+ messages in thread
From: Neftin, Sasha @ 2022-07-05 4:23 UTC (permalink / raw)
To: Lennert Buytenhek, intel-wired-lan, Tony Nguyen, Jesse Brandeburg
Cc: netdev, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Vitaly Lifshits, Aaron Brown, Jeff Kirsher
On 6/2/2022 18:58, Lennert Buytenhek wrote:
> The initially merged version of the igc driver code (via commit
> 146740f9abc4, "igc: Add support for PF") contained the following
> IGC_REMOVED checks in the igc_rd32/wr32() MMIO accessors:
>
> u32 igc_rd32(struct igc_hw *hw, u32 reg)
> {
> u8 __iomem *hw_addr = READ_ONCE(hw->hw_addr);
> u32 value = 0;
>
> if (IGC_REMOVED(hw_addr))
> return ~value;
>
> value = readl(&hw_addr[reg]);
>
> /* reads should not return all F's */
> if (!(~value) && (!reg || !(~readl(hw_addr))))
> hw->hw_addr = NULL;
>
> return value;
> }
>
> And:
>
> #define wr32(reg, val) \
> do { \
> u8 __iomem *hw_addr = READ_ONCE((hw)->hw_addr); \
> if (!IGC_REMOVED(hw_addr)) \
> writel((val), &hw_addr[(reg)]); \
> } while (0)
>
> E.g. igb has similar checks in its MMIO accessors, and has a similar
> macro E1000_REMOVED, which is implemented as follows:
>
> #define E1000_REMOVED(h) unlikely(!(h))
>
> These checks serve to detect and take note of an 0xffffffff MMIO read
> return from the device, which can be caused by a PCIe link flap or some
> other kind of PCI bus error, and to avoid performing MMIO reads and
> writes from that point onwards.
>
> However, the IGC_REMOVED macro was not originally implemented:
>
> #ifndef IGC_REMOVED
> #define IGC_REMOVED(a) (0)
> #endif /* IGC_REMOVED */
>
> This led to the IGC_REMOVED logic to be removed entirely in a
> subsequent commit (commit 3c215fb18e70, "igc: remove IGC_REMOVED
> function"), with the rationale that such checks matter only for
> virtualization and that igc does not support virtualization -- but a
> PCIe device can become detached even without virtualization being in
> use, and without proper checks, a PCIe bus error affecting an igc
> adapter will lead to various NULL pointer dereferences, as the first
> access after the error will set hw->hw_addr to NULL, and subsequent
> accesses will blindly dereference this now-NULL pointer.
>
> This patch reinstates the IGC_REMOVED checks in igc_rd32/wr32(), and
> implements IGC_REMOVED the way it is done for igb, by checking for the
> unlikely() case of hw_addr being NULL. This change prevents the oopses
> seen when a PCIe link flap occurs on an igc adapter.
>
> Fixes: 146740f9abc4 ("igc: Add support for PF")
> Signed-off-by: Lennert Buytenhek <buytenh@arista.com>
> ---
> As initially reported on intel-wired-lan@ in February:
>
> https://lists.osuosl.org/pipermail/intel-wired-lan/Week-of-Mon-20220214/027787.html
>
> We're seeing these NULL pointer dereferences hit fairly reproducibly
> when rebooting, presumably due to the particularities of reset
> sequencing on the boards we see this hit on.
>
> A link flap can be caused by toggling the Secondary Bus Reset bit
> in the upstream PCIe bridge's Bridge Control register and can reliably
> reproduce this problem.
>
> drivers/net/ethernet/intel/igc/igc_main.c | 3 +++
> drivers/net/ethernet/intel/igc/igc_regs.h | 5 ++++-
> 2 files changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
> index 74b2c590ed5d..38e46e9ba8bb 100644
> --- a/drivers/net/ethernet/intel/igc/igc_main.c
> +++ b/drivers/net/ethernet/intel/igc/igc_main.c
> @@ -6171,6 +6171,9 @@ u32 igc_rd32(struct igc_hw *hw, u32 reg)
> u8 __iomem *hw_addr = READ_ONCE(hw->hw_addr);
> u32 value = 0;
>
> + if (IGC_REMOVED(hw_addr))
> + return ~value;
> +
> value = readl(&hw_addr[reg]);
>
> /* reads should not return all F's */
> diff --git a/drivers/net/ethernet/intel/igc/igc_regs.h b/drivers/net/ethernet/intel/igc/igc_regs.h
> index e197a33d93a0..026c3b65fc37 100644
> --- a/drivers/net/ethernet/intel/igc/igc_regs.h
> +++ b/drivers/net/ethernet/intel/igc/igc_regs.h
> @@ -306,7 +306,8 @@ u32 igc_rd32(struct igc_hw *hw, u32 reg);
> #define wr32(reg, val) \
> do { \
> u8 __iomem *hw_addr = READ_ONCE((hw)->hw_addr); \
> - writel((val), &hw_addr[(reg)]); \
> + if (!IGC_REMOVED(hw_addr)) \
> + writel((val), &hw_addr[(reg)]); \
> } while (0)
>
> #define rd32(reg) (igc_rd32(hw, reg))
> @@ -318,4 +319,6 @@ do { \
>
> #define array_rd32(reg, offset) (igc_rd32(hw, (reg) + ((offset) << 2)))
>
> +#define IGC_REMOVED(h) unlikely(!(h))
> +
> #endif
Acked-by: Sasha Neftin <sasha.neftin@intel.com>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Intel-wired-lan] [PATCH v1] igc: Reinstate IGC_REMOVED logic and implement it properly
@ 2022-07-05 4:23 ` Neftin, Sasha
0 siblings, 0 replies; 6+ messages in thread
From: Neftin, Sasha @ 2022-07-05 4:23 UTC (permalink / raw)
To: Lennert Buytenhek, intel-wired-lan, Tony Nguyen, Jesse Brandeburg
Cc: netdev, Eric Dumazet, Jeff Kirsher, Jakub Kicinski, Paolo Abeni,
David S. Miller
On 6/2/2022 18:58, Lennert Buytenhek wrote:
> The initially merged version of the igc driver code (via commit
> 146740f9abc4, "igc: Add support for PF") contained the following
> IGC_REMOVED checks in the igc_rd32/wr32() MMIO accessors:
>
> u32 igc_rd32(struct igc_hw *hw, u32 reg)
> {
> u8 __iomem *hw_addr = READ_ONCE(hw->hw_addr);
> u32 value = 0;
>
> if (IGC_REMOVED(hw_addr))
> return ~value;
>
> value = readl(&hw_addr[reg]);
>
> /* reads should not return all F's */
> if (!(~value) && (!reg || !(~readl(hw_addr))))
> hw->hw_addr = NULL;
>
> return value;
> }
>
> And:
>
> #define wr32(reg, val) \
> do { \
> u8 __iomem *hw_addr = READ_ONCE((hw)->hw_addr); \
> if (!IGC_REMOVED(hw_addr)) \
> writel((val), &hw_addr[(reg)]); \
> } while (0)
>
> E.g. igb has similar checks in its MMIO accessors, and has a similar
> macro E1000_REMOVED, which is implemented as follows:
>
> #define E1000_REMOVED(h) unlikely(!(h))
>
> These checks serve to detect and take note of an 0xffffffff MMIO read
> return from the device, which can be caused by a PCIe link flap or some
> other kind of PCI bus error, and to avoid performing MMIO reads and
> writes from that point onwards.
>
> However, the IGC_REMOVED macro was not originally implemented:
>
> #ifndef IGC_REMOVED
> #define IGC_REMOVED(a) (0)
> #endif /* IGC_REMOVED */
>
> This led to the IGC_REMOVED logic to be removed entirely in a
> subsequent commit (commit 3c215fb18e70, "igc: remove IGC_REMOVED
> function"), with the rationale that such checks matter only for
> virtualization and that igc does not support virtualization -- but a
> PCIe device can become detached even without virtualization being in
> use, and without proper checks, a PCIe bus error affecting an igc
> adapter will lead to various NULL pointer dereferences, as the first
> access after the error will set hw->hw_addr to NULL, and subsequent
> accesses will blindly dereference this now-NULL pointer.
>
> This patch reinstates the IGC_REMOVED checks in igc_rd32/wr32(), and
> implements IGC_REMOVED the way it is done for igb, by checking for the
> unlikely() case of hw_addr being NULL. This change prevents the oopses
> seen when a PCIe link flap occurs on an igc adapter.
>
> Fixes: 146740f9abc4 ("igc: Add support for PF")
> Signed-off-by: Lennert Buytenhek <buytenh@arista.com>
> ---
> As initially reported on intel-wired-lan@ in February:
>
> https://lists.osuosl.org/pipermail/intel-wired-lan/Week-of-Mon-20220214/027787.html
>
> We're seeing these NULL pointer dereferences hit fairly reproducibly
> when rebooting, presumably due to the particularities of reset
> sequencing on the boards we see this hit on.
>
> A link flap can be caused by toggling the Secondary Bus Reset bit
> in the upstream PCIe bridge's Bridge Control register and can reliably
> reproduce this problem.
>
> drivers/net/ethernet/intel/igc/igc_main.c | 3 +++
> drivers/net/ethernet/intel/igc/igc_regs.h | 5 ++++-
> 2 files changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
> index 74b2c590ed5d..38e46e9ba8bb 100644
> --- a/drivers/net/ethernet/intel/igc/igc_main.c
> +++ b/drivers/net/ethernet/intel/igc/igc_main.c
> @@ -6171,6 +6171,9 @@ u32 igc_rd32(struct igc_hw *hw, u32 reg)
> u8 __iomem *hw_addr = READ_ONCE(hw->hw_addr);
> u32 value = 0;
>
> + if (IGC_REMOVED(hw_addr))
> + return ~value;
> +
> value = readl(&hw_addr[reg]);
>
> /* reads should not return all F's */
> diff --git a/drivers/net/ethernet/intel/igc/igc_regs.h b/drivers/net/ethernet/intel/igc/igc_regs.h
> index e197a33d93a0..026c3b65fc37 100644
> --- a/drivers/net/ethernet/intel/igc/igc_regs.h
> +++ b/drivers/net/ethernet/intel/igc/igc_regs.h
> @@ -306,7 +306,8 @@ u32 igc_rd32(struct igc_hw *hw, u32 reg);
> #define wr32(reg, val) \
> do { \
> u8 __iomem *hw_addr = READ_ONCE((hw)->hw_addr); \
> - writel((val), &hw_addr[(reg)]); \
> + if (!IGC_REMOVED(hw_addr)) \
> + writel((val), &hw_addr[(reg)]); \
> } while (0)
>
> #define rd32(reg) (igc_rd32(hw, reg))
> @@ -318,4 +319,6 @@ do { \
>
> #define array_rd32(reg, offset) (igc_rd32(hw, (reg) + ((offset) << 2)))
>
> +#define IGC_REMOVED(h) unlikely(!(h))
> +
> #endif
Acked-by: Sasha Neftin <sasha.neftin@intel.com>
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-07-05 4:23 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-06-02 15:58 [PATCH v1] igc: Reinstate IGC_REMOVED logic and implement it properly Lennert Buytenhek
2022-06-02 15:58 ` [Intel-wired-lan] " Lennert Buytenhek
2022-06-30 7:23 ` naamax.meir
2022-06-30 7:23 ` naamax.meir
2022-07-05 4:23 ` Neftin, Sasha
2022-07-05 4:23 ` [Intel-wired-lan] " Neftin, Sasha
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.