Add support to listen Ethernet HW common safery IRQ for correctable and
uncorrectable fault. The safety IRQ will be triggered for ECC(error
correction code), DPP(data path parity, FSM(finite state machine) error.
Changes since v6:
- use name sfty_irq instead of safety_common_irq.
Changes since v5:
- Add description of ECC, DPP, FSM
Changes since v4:
- Fix DT_CHECKER warning
- use name safety for the IRQ.
Suraj Jaiswal (3):
dt-bindings: net: qcom,ethqos: add binding doc for safety IRQ for
sa8775p
arm64: dts: qcom: sa8775p: enable safety IRQ
net: stmmac: Add driver support for DWMAC5 common safety IRQ
.../devicetree/bindings/net/qcom,ethqos.yaml | 9 +++++---
.../devicetree/bindings/net/snps,dwmac.yaml | 6 ++++--
arch/arm64/boot/dts/qcom/sa8775p.dtsi | 10 +++++----
drivers/net/ethernet/stmicro/stmmac/common.h | 1 +
drivers/net/ethernet/stmicro/stmmac/stmmac.h | 3 +++
.../net/ethernet/stmicro/stmmac/stmmac_main.c | 21 +++++++++++++++++++
.../ethernet/stmicro/stmmac/stmmac_platform.c | 9 ++++++++
7 files changed, 50 insertions(+), 9 deletions(-)
--
2.25.1
Add binding doc for safety IRQ. The safety IRQ will be
triggered for ECC(error correction code), DPP(data path
parity), FSM(finite state machine) error.
Signed-off-by: Suraj Jaiswal <[email protected]>
Reviewed-by: Rob Herring <[email protected]>
---
Documentation/devicetree/bindings/net/qcom,ethqos.yaml | 9 ++++++---
Documentation/devicetree/bindings/net/snps,dwmac.yaml | 6 ++++--
2 files changed, 10 insertions(+), 5 deletions(-)
diff --git a/Documentation/devicetree/bindings/net/qcom,ethqos.yaml b/Documentation/devicetree/bindings/net/qcom,ethqos.yaml
index 7bdb412a0185..69a337c7e345 100644
--- a/Documentation/devicetree/bindings/net/qcom,ethqos.yaml
+++ b/Documentation/devicetree/bindings/net/qcom,ethqos.yaml
@@ -37,12 +37,14 @@ properties:
items:
- description: Combined signal for various interrupt events
- description: The interrupt that occurs when Rx exits the LPI state
+ - description: The interrupt that occurs when HW safety error triggered
interrupt-names:
minItems: 1
items:
- const: macirq
- - const: eth_lpi
+ - enum: [eth_lpi, sfty]
+ - const: sfty
clocks:
maxItems: 4
@@ -89,8 +91,9 @@ examples:
<&gcc GCC_ETH_PTP_CLK>,
<&gcc GCC_ETH_RGMII_CLK>;
interrupts = <GIC_SPI 56 IRQ_TYPE_LEVEL_HIGH>,
- <GIC_SPI 55 IRQ_TYPE_LEVEL_HIGH>;
- interrupt-names = "macirq", "eth_lpi";
+ <GIC_SPI 55 IRQ_TYPE_LEVEL_HIGH>,
+ <GIC_SPI 782 IRQ_TYPE_LEVEL_HIGH>;
+ interrupt-names = "macirq", "eth_lpi", "sfty";
rx-fifo-depth = <4096>;
tx-fifo-depth = <4096>;
diff --git a/Documentation/devicetree/bindings/net/snps,dwmac.yaml b/Documentation/devicetree/bindings/net/snps,dwmac.yaml
index 5c2769dc689a..9b04e2ed7c18 100644
--- a/Documentation/devicetree/bindings/net/snps,dwmac.yaml
+++ b/Documentation/devicetree/bindings/net/snps,dwmac.yaml
@@ -107,13 +107,15 @@ properties:
- description: Combined signal for various interrupt events
- description: The interrupt to manage the remote wake-up packet detection
- description: The interrupt that occurs when Rx exits the LPI state
+ - description: The interrupt that occurs when HW safety error triggered
interrupt-names:
minItems: 1
items:
- const: macirq
- - enum: [eth_wake_irq, eth_lpi]
- - const: eth_lpi
+ - enum: [eth_wake_irq, eth_lpi, sfty]
+ - enum: [eth_wake_irq, eth_lpi, sfty]
+ - enum: [eth_wake_irq, eth_lpi, sfty]
clocks:
minItems: 1
--
2.25.1
Add changes to support safety IRQ handling
support for ethernet.
Signed-off-by: Suraj Jaiswal <[email protected]>
Reviewed-by: Konrad Dybcio <[email protected]>
---
arch/arm64/boot/dts/qcom/sa8775p.dtsi | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/arch/arm64/boot/dts/qcom/sa8775p.dtsi b/arch/arm64/boot/dts/qcom/sa8775p.dtsi
index 2da567a6f64e..36380a4d2b0e 100644
--- a/arch/arm64/boot/dts/qcom/sa8775p.dtsi
+++ b/arch/arm64/boot/dts/qcom/sa8775p.dtsi
@@ -2394,8 +2394,9 @@ ethernet1: ethernet@23000000 {
<0x0 0x23016000 0x0 0x100>;
reg-names = "stmmaceth", "rgmii";
- interrupts = <GIC_SPI 929 IRQ_TYPE_LEVEL_HIGH>;
- interrupt-names = "macirq";
+ interrupts = <GIC_SPI 929 IRQ_TYPE_LEVEL_HIGH>,
+ <GIC_SPI 781 IRQ_TYPE_LEVEL_HIGH>;
+ interrupt-names = "macirq", "sfty";
clocks = <&gcc GCC_EMAC1_AXI_CLK>,
<&gcc GCC_EMAC1_SLV_AHB_CLK>,
@@ -2427,8 +2428,9 @@ ethernet0: ethernet@23040000 {
<0x0 0x23056000 0x0 0x100>;
reg-names = "stmmaceth", "rgmii";
- interrupts = <GIC_SPI 946 IRQ_TYPE_LEVEL_HIGH>;
- interrupt-names = "macirq";
+ interrupts = <GIC_SPI 946 IRQ_TYPE_LEVEL_HIGH>,
+ <GIC_SPI 782 IRQ_TYPE_LEVEL_HIGH>;
+ interrupt-names = "macirq", "sfty";
clocks = <&gcc GCC_EMAC0_AXI_CLK>,
<&gcc GCC_EMAC0_SLV_AHB_CLK>,
--
2.25.1
Add support to listen HW safety IRQ like ECC(error
correction code), DPP(data path parity), FSM(finite state
machine) fault in common IRQ line.
Signed-off-by: Suraj Jaiswal <[email protected]>
---
drivers/net/ethernet/stmicro/stmmac/common.h | 1 +
drivers/net/ethernet/stmicro/stmmac/stmmac.h | 3 +++
.../net/ethernet/stmicro/stmmac/stmmac_main.c | 21 +++++++++++++++++++
.../ethernet/stmicro/stmmac/stmmac_platform.c | 9 ++++++++
4 files changed, 34 insertions(+)
diff --git a/drivers/net/ethernet/stmicro/stmmac/common.h b/drivers/net/ethernet/stmicro/stmmac/common.h
index 721c1f8e892f..b9233b09b80f 100644
--- a/drivers/net/ethernet/stmicro/stmmac/common.h
+++ b/drivers/net/ethernet/stmicro/stmmac/common.h
@@ -344,6 +344,7 @@ enum request_irq_err {
REQ_IRQ_ERR_ALL,
REQ_IRQ_ERR_TX,
REQ_IRQ_ERR_RX,
+ REQ_IRQ_ERR_SFTY,
REQ_IRQ_ERR_SFTY_UE,
REQ_IRQ_ERR_SFTY_CE,
REQ_IRQ_ERR_LPI,
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac.h b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
index 9f89acf31050..ca3d93851bed 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac.h
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
@@ -31,6 +31,7 @@ struct stmmac_resources {
int wol_irq;
int lpi_irq;
int irq;
+ int sfty_irq;
int sfty_ce_irq;
int sfty_ue_irq;
int rx_irq[MTL_MAX_RX_QUEUES];
@@ -297,6 +298,7 @@ struct stmmac_priv {
void __iomem *ptpaddr;
void __iomem *estaddr;
unsigned long active_vlans[BITS_TO_LONGS(VLAN_N_VID)];
+ int sfty_irq;
int sfty_ce_irq;
int sfty_ue_irq;
int rx_irq[MTL_MAX_RX_QUEUES];
@@ -305,6 +307,7 @@ struct stmmac_priv {
char int_name_mac[IFNAMSIZ + 9];
char int_name_wol[IFNAMSIZ + 9];
char int_name_lpi[IFNAMSIZ + 9];
+ char int_name_sfty[IFNAMSIZ + 10];
char int_name_sfty_ce[IFNAMSIZ + 10];
char int_name_sfty_ue[IFNAMSIZ + 10];
char int_name_rx_irq[MTL_MAX_TX_QUEUES][IFNAMSIZ + 14];
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 47de466e432c..6cf289f192a7 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -3592,6 +3592,10 @@ static void stmmac_free_irq(struct net_device *dev,
if (priv->wol_irq > 0 && priv->wol_irq != dev->irq)
free_irq(priv->wol_irq, dev);
fallthrough;
+ case REQ_IRQ_ERR_SFTY:
+ if (priv->sfty_irq > 0 && priv->sfty_irq != dev->irq)
+ free_irq(priv->sfty_irq, dev);
+ fallthrough;
case REQ_IRQ_ERR_WOL:
free_irq(dev->irq, dev);
fallthrough;
@@ -3759,6 +3763,7 @@ static int stmmac_request_irq_single(struct net_device *dev)
struct stmmac_priv *priv = netdev_priv(dev);
enum request_irq_err irq_err;
int ret;
+ char *int_name;
ret = request_irq(dev->irq, stmmac_interrupt,
IRQF_SHARED, dev->name, dev);
@@ -3798,6 +3803,20 @@ static int stmmac_request_irq_single(struct net_device *dev)
}
}
+ if (priv->sfty_irq > 0 && priv->sfty_irq != dev->irq) {
+ int_name = priv->int_name_sfty;
+ sprintf(int_name, "%s:%s", dev->name, "safety");
+ ret = request_irq(priv->sfty_irq, stmmac_safety_interrupt,
+ 0, int_name, dev);
+ if (unlikely(ret < 0)) {
+ netdev_err(priv->dev,
+ "%s: alloc safety failed %d (error: %d)\n",
+ __func__, priv->sfty_irq, ret);
+ irq_err = REQ_IRQ_ERR_SFTY;
+ goto irq_error;
+ }
+ }
+
return 0;
irq_error:
@@ -7462,8 +7481,10 @@ int stmmac_dvr_probe(struct device *device,
priv->dev->irq = res->irq;
priv->wol_irq = res->wol_irq;
priv->lpi_irq = res->lpi_irq;
+ priv->sfty_irq = res->sfty_irq;
priv->sfty_ce_irq = res->sfty_ce_irq;
priv->sfty_ue_irq = res->sfty_ue_irq;
+
for (i = 0; i < MTL_MAX_RX_QUEUES; i++)
priv->rx_irq[i] = res->rx_irq[i];
for (i = 0; i < MTL_MAX_TX_QUEUES; i++)
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
index 1ffde555da47..3808a3225a7d 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
@@ -726,6 +726,15 @@ int stmmac_get_platform_resources(struct platform_device *pdev,
dev_info(&pdev->dev, "IRQ eth_lpi not found\n");
}
+ stmmac_res->sfty_irq =
+ platform_get_irq_byname_optional(pdev, "sfty");
+
+ if (stmmac_res->sfty_irq < 0) {
+ if (stmmac_res->sfty_irq == -EPROBE_DEFER)
+ return -EPROBE_DEFER;
+ dev_info(&pdev->dev, "IRQ safety IRQ not found\n");
+ }
+
stmmac_res->addr = devm_platform_ioremap_resource(pdev, 0);
return PTR_ERR_OR_ZERO(stmmac_res->addr);
--
2.25.1
On Tue, 2023-12-12 at 17:28 +0530, Suraj Jaiswal wrote:
> @@ -3759,6 +3763,7 @@ static int stmmac_request_irq_single(struct net_device *dev)
> struct stmmac_priv *priv = netdev_priv(dev);
> enum request_irq_err irq_err;
> int ret;
> + char *int_name;
I'm sorry to nit-pick, but please respect the reverse x-mas tree above,
Otherwise LGTM!
Cheers,
Paolo
Hi Suraj
On Tue, Dec 12, 2023 at 05:28:41PM +0530, Suraj Jaiswal wrote:
> Add support to listen HW safety IRQ like ECC(error
> correction code), DPP(data path parity), FSM(finite state
> machine) fault in common IRQ line.
>
> Signed-off-by: Suraj Jaiswal <[email protected]>
> ---
> drivers/net/ethernet/stmicro/stmmac/common.h | 1 +
> drivers/net/ethernet/stmicro/stmmac/stmmac.h | 3 +++
> .../net/ethernet/stmicro/stmmac/stmmac_main.c | 21 +++++++++++++++++++
> .../ethernet/stmicro/stmmac/stmmac_platform.c | 9 ++++++++
> 4 files changed, 34 insertions(+)
>
> diff --git a/drivers/net/ethernet/stmicro/stmmac/common.h b/drivers/net/ethernet/stmicro/stmmac/common.h
> index 721c1f8e892f..b9233b09b80f 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/common.h
> +++ b/drivers/net/ethernet/stmicro/stmmac/common.h
> @@ -344,6 +344,7 @@ enum request_irq_err {
> REQ_IRQ_ERR_ALL,
> REQ_IRQ_ERR_TX,
> REQ_IRQ_ERR_RX,
> + REQ_IRQ_ERR_SFTY,
> REQ_IRQ_ERR_SFTY_UE,
> REQ_IRQ_ERR_SFTY_CE,
> REQ_IRQ_ERR_LPI,
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac.h b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
> index 9f89acf31050..ca3d93851bed 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac.h
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
> @@ -31,6 +31,7 @@ struct stmmac_resources {
> int wol_irq;
> int lpi_irq;
> int irq;
> + int sfty_irq;
> int sfty_ce_irq;
> int sfty_ue_irq;
> int rx_irq[MTL_MAX_RX_QUEUES];
> @@ -297,6 +298,7 @@ struct stmmac_priv {
> void __iomem *ptpaddr;
> void __iomem *estaddr;
> unsigned long active_vlans[BITS_TO_LONGS(VLAN_N_VID)];
> + int sfty_irq;
> int sfty_ce_irq;
> int sfty_ue_irq;
> int rx_irq[MTL_MAX_RX_QUEUES];
> @@ -305,6 +307,7 @@ struct stmmac_priv {
> char int_name_mac[IFNAMSIZ + 9];
> char int_name_wol[IFNAMSIZ + 9];
> char int_name_lpi[IFNAMSIZ + 9];
> + char int_name_sfty[IFNAMSIZ + 10];
> char int_name_sfty_ce[IFNAMSIZ + 10];
> char int_name_sfty_ue[IFNAMSIZ + 10];
> char int_name_rx_irq[MTL_MAX_TX_QUEUES][IFNAMSIZ + 14];
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index 47de466e432c..6cf289f192a7 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -3592,6 +3592,10 @@ static void stmmac_free_irq(struct net_device *dev,
> if (priv->wol_irq > 0 && priv->wol_irq != dev->irq)
> free_irq(priv->wol_irq, dev);
> fallthrough;
> + case REQ_IRQ_ERR_SFTY:
> + if (priv->sfty_irq > 0 && priv->sfty_irq != dev->irq)
> + free_irq(priv->sfty_irq, dev);
> + fallthrough;
> case REQ_IRQ_ERR_WOL:
> free_irq(dev->irq, dev);
> fallthrough;
> @@ -3759,6 +3763,7 @@ static int stmmac_request_irq_single(struct net_device *dev)
> struct stmmac_priv *priv = netdev_priv(dev);
> enum request_irq_err irq_err;
> int ret;
> + char *int_name;
See my comment below.
>
> ret = request_irq(dev->irq, stmmac_interrupt,
> IRQF_SHARED, dev->name, dev);
> @@ -3798,6 +3803,20 @@ static int stmmac_request_irq_single(struct net_device *dev)
> }
> }
>
> + if (priv->sfty_irq > 0 && priv->sfty_irq != dev->irq) {
> + int_name = priv->int_name_sfty;
> + sprintf(int_name, "%s:%s", dev->name, "safety");
> + ret = request_irq(priv->sfty_irq, stmmac_safety_interrupt,
> + 0, int_name, dev);
> + if (unlikely(ret < 0)) {
> + netdev_err(priv->dev,
> + "%s: alloc safety failed %d (error: %d)\n",
> + __func__, priv->sfty_irq, ret);
> + irq_err = REQ_IRQ_ERR_SFTY;
> + goto irq_error;
> + }
> + }
> +
Omg, I thought this change belonged to stmmac_request_irq_multi_msi().
My bad, sorry. Please move the code above to
stmmac_request_irq_multi_msi() and get back the part in
stmmac_request_irq_single() as it was in v5, but instead of specifying
"safety" IRQ name use "dev->name" as the rest of similar code snippets
in here have:
+ if (priv->sfty_irq > 0 && priv->sfty_irq != dev->irq) {
+ ret = request_irq(priv->sfty_irq, stmmac_safety_interrupt,
+ 0, dev->name, dev);
+ if (unlikely(ret < 0)) {
+ netdev_err(priv->dev,
+ "%s: alloc safety failed %d (error: %d)\n",
+ __func__, priv->sfty_irq, ret);
+ irq_err = REQ_IRQ_ERR_SFTY;
+ goto irq_error;
+ }
+ }
I guess at some point afterwards we'll need to refactor the IRQs
request part of this driver: replace stmmac_request_irq_single() body
with the upper part of the stmmac_request_irq_multi_msi() method and
then just make the former method being called from the later one...
> return 0;
>
> irq_error:
> @@ -7462,8 +7481,10 @@ int stmmac_dvr_probe(struct device *device,
> priv->dev->irq = res->irq;
> priv->wol_irq = res->wol_irq;
> priv->lpi_irq = res->lpi_irq;
> + priv->sfty_irq = res->sfty_irq;
> priv->sfty_ce_irq = res->sfty_ce_irq;
> priv->sfty_ue_irq = res->sfty_ue_irq;
> +
Please drop this change. The code below is attached to the code above
because it basically does the same but in the loop.
> for (i = 0; i < MTL_MAX_RX_QUEUES; i++)
> priv->rx_irq[i] = res->rx_irq[i];
> for (i = 0; i < MTL_MAX_TX_QUEUES; i++)
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
> index 1ffde555da47..3808a3225a7d 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
> @@ -726,6 +726,15 @@ int stmmac_get_platform_resources(struct platform_device *pdev,
> dev_info(&pdev->dev, "IRQ eth_lpi not found\n");
> }
>
> + stmmac_res->sfty_irq =
> + platform_get_irq_byname_optional(pdev, "sfty");
> +
Please drop this change too. It's normal to have a method call
attached to the error check statement especially seeing the rest of
the similar code snippets are designed that way in this function.
-Serge(y)
> + if (stmmac_res->sfty_irq < 0) {
> + if (stmmac_res->sfty_irq == -EPROBE_DEFER)
> + return -EPROBE_DEFER;
> + dev_info(&pdev->dev, "IRQ safety IRQ not found\n");
> + }
> +
> stmmac_res->addr = devm_platform_ioremap_resource(pdev, 0);
>
> return PTR_ERR_OR_ZERO(stmmac_res->addr);
> --
> 2.25.1
>
>
Hi Serge,
Please find commnet inline & let me know if any further action needed
Thanks
Suraj
On 12/14/2023 8:42 PM, Serge Semin wrote:
> Hi Suraj
>
> On Tue, Dec 12, 2023 at 05:28:41PM +0530, Suraj Jaiswal wrote:
>> Add support to listen HW safety IRQ like ECC(error
>> correction code), DPP(data path parity), FSM(finite state
>> machine) fault in common IRQ line.
>>
>> Signed-off-by: Suraj Jaiswal <[email protected]>
>> ---
>> drivers/net/ethernet/stmicro/stmmac/common.h | 1 +
>> drivers/net/ethernet/stmicro/stmmac/stmmac.h | 3 +++
>> .../net/ethernet/stmicro/stmmac/stmmac_main.c | 21 +++++++++++++++++++
>> .../ethernet/stmicro/stmmac/stmmac_platform.c | 9 ++++++++
>> 4 files changed, 34 insertions(+)
>>
>> diff --git a/drivers/net/ethernet/stmicro/stmmac/common.h b/drivers/net/ethernet/stmicro/stmmac/common.h
>> index 721c1f8e892f..b9233b09b80f 100644
>> --- a/drivers/net/ethernet/stmicro/stmmac/common.h
>> +++ b/drivers/net/ethernet/stmicro/stmmac/common.h
>> @@ -344,6 +344,7 @@ enum request_irq_err {
>> REQ_IRQ_ERR_ALL,
>> REQ_IRQ_ERR_TX,
>> REQ_IRQ_ERR_RX,
>> + REQ_IRQ_ERR_SFTY,
>> REQ_IRQ_ERR_SFTY_UE,
>> REQ_IRQ_ERR_SFTY_CE,
>> REQ_IRQ_ERR_LPI,
>> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac.h b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
>> index 9f89acf31050..ca3d93851bed 100644
>> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac.h
>> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
>> @@ -31,6 +31,7 @@ struct stmmac_resources {
>> int wol_irq;
>> int lpi_irq;
>> int irq;
>> + int sfty_irq;
>> int sfty_ce_irq;
>> int sfty_ue_irq;
>> int rx_irq[MTL_MAX_RX_QUEUES];
>> @@ -297,6 +298,7 @@ struct stmmac_priv {
>> void __iomem *ptpaddr;
>> void __iomem *estaddr;
>> unsigned long active_vlans[BITS_TO_LONGS(VLAN_N_VID)];
>> + int sfty_irq;
>> int sfty_ce_irq;
>> int sfty_ue_irq;
>> int rx_irq[MTL_MAX_RX_QUEUES];
>> @@ -305,6 +307,7 @@ struct stmmac_priv {
>> char int_name_mac[IFNAMSIZ + 9];
>> char int_name_wol[IFNAMSIZ + 9];
>> char int_name_lpi[IFNAMSIZ + 9];
>> + char int_name_sfty[IFNAMSIZ + 10];
>> char int_name_sfty_ce[IFNAMSIZ + 10];
>> char int_name_sfty_ue[IFNAMSIZ + 10];
>> char int_name_rx_irq[MTL_MAX_TX_QUEUES][IFNAMSIZ + 14];
>> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
>> index 47de466e432c..6cf289f192a7 100644
>> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
>> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
>> @@ -3592,6 +3592,10 @@ static void stmmac_free_irq(struct net_device *dev,
>> if (priv->wol_irq > 0 && priv->wol_irq != dev->irq)
>> free_irq(priv->wol_irq, dev);
>> fallthrough;
>> + case REQ_IRQ_ERR_SFTY:
>> + if (priv->sfty_irq > 0 && priv->sfty_irq != dev->irq)
>> + free_irq(priv->sfty_irq, dev);
>> + fallthrough;
>> case REQ_IRQ_ERR_WOL:
>> free_irq(dev->irq, dev);
>> fallthrough;
>> @@ -3759,6 +3763,7 @@ static int stmmac_request_irq_single(struct net_device *dev)
>> struct stmmac_priv *priv = netdev_priv(dev);
>> enum request_irq_err irq_err;
>> int ret;
>
>> + char *int_name;
>
> See my comment below.
>
>>
>> ret = request_irq(dev->irq, stmmac_interrupt,
>> IRQF_SHARED, dev->name, dev);
>> @@ -3798,6 +3803,20 @@ static int stmmac_request_irq_single(struct net_device *dev)
>> }
>> }
>>
>
>> + if (priv->sfty_irq > 0 && priv->sfty_irq != dev->irq) {
>> + int_name = priv->int_name_sfty;
>> + sprintf(int_name, "%s:%s", dev->name, "safety");
>> + ret = request_irq(priv->sfty_irq, stmmac_safety_interrupt,
>> + 0, int_name, dev);
>> + if (unlikely(ret < 0)) {
>> + netdev_err(priv->dev,
>> + "%s: alloc safety failed %d (error: %d)\n",
>> + __func__, priv->sfty_irq, ret);
>> + irq_err = REQ_IRQ_ERR_SFTY;
>> + goto irq_error;
>> + }
>> + }
>> +
>
> Omg, I thought this change belonged to stmmac_request_irq_multi_msi().
> My bad, sorry. Please move the code above to
> stmmac_request_irq_multi_msi() and get back the part in
> stmmac_request_irq_single() as it was in v5, but instead of specifying
> "safety" IRQ name use "dev->name" as the rest of similar code snippets
> in here have:
>
> + if (priv->sfty_irq > 0 && priv->sfty_irq != dev->irq) {
> + ret = request_irq(priv->sfty_irq, stmmac_safety_interrupt,
> + 0, dev->name, dev);
> + if (unlikely(ret < 0)) {
> + netdev_err(priv->dev,
> + "%s: alloc safety failed %d (error: %d)\n",
> + __func__, priv->sfty_irq, ret);
> + irq_err = REQ_IRQ_ERR_SFTY;
> + goto irq_error;
> + }
> + }
<Suraj> We can not use "dev->name" as this is name already used by "stmmac_interrupt" @ https://elixir.bootlin.com/linux/v6.1.68/source/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c#L3655.
<
ret = request_irq(dev->irq, stmmac_interrupt,
IRQF_SHARED, dev->name, dev);
>
if we are using same "dev->name" while requesting safety IRQ as well then "/proc/interrupt" will show same name eth0/eth1 for both "stmmac_interrupt" & "safety interrupt" and by looking at "/proc/interrupt" output we can not say which IRQ is for safety and which is for stmmac_interrupt.
>
> I guess at some point afterwards we'll need to refactor the IRQs
> request part of this driver: replace stmmac_request_irq_single() body
> with the upper part of the stmmac_request_irq_multi_msi() method and
> then just make the former method being called from the later one...
>
>> return 0;
>>
>> irq_error:
>> @@ -7462,8 +7481,10 @@ int stmmac_dvr_probe(struct device *device,
>> priv->dev->irq = res->irq;
>> priv->wol_irq = res->wol_irq;
>> priv->lpi_irq = res->lpi_irq;
>> + priv->sfty_irq = res->sfty_irq;
>> priv->sfty_ce_irq = res->sfty_ce_irq;
>> priv->sfty_ue_irq = res->sfty_ue_irq;
>
>> +
>
> Please drop this change. The code below is attached to the code above
> because it basically does the same but in the loop.
<Suraj> below loop code "for (i = 0; i < MTL_MAX_RX_QUEUES; i++) priv->rx_irq[i] = res->rx_irq[i];" is not for rx_irq array and will not help for safety irq.
Let me know if I got your commnet properly .
>
>> for (i = 0; i < MTL_MAX_RX_QUEUES; i++)
>> priv->rx_irq[i] = res->rx_irq[i];
>> for (i = 0; i < MTL_MAX_TX_QUEUES; i++)
>> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
>> index 1ffde555da47..3808a3225a7d 100644
>> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
>> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
>> @@ -726,6 +726,15 @@ int stmmac_get_platform_resources(struct platform_device *pdev,
>> dev_info(&pdev->dev, "IRQ eth_lpi not found\n");
>> }
>>
>> + stmmac_res->sfty_irq =
>> + platform_get_irq_byname_optional(pdev, "sfty");
>
>> +
>
> Please drop this change too. It's normal to have a method call
> attached to the error check statement especially seeing the rest of
> the similar code snippets are designed that way in this function.
<Suraj> Do you means to remove all below code where we are printing the dev_info() message ?
We added this code similar to LPM code.
>
> -Serge(y)
>
>> + if (stmmac_res->sfty_irq < 0) {
>> + if (stmmac_res->sfty_irq == -EPROBE_DEFER)
>> + return -EPROBE_DEFER;
>> + dev_info(&pdev->dev, "IRQ safety IRQ not found\n");
>> + }
>> +
>> stmmac_res->addr = devm_platform_ioremap_resource(pdev, 0);
>>
>> return PTR_ERR_OR_ZERO(stmmac_res->addr);
>> --
>> 2.25.1
>>
>>
On Mon, Dec 18, 2023 at 03:27:54PM +0530, Suraj Jaiswal wrote:
>
> Hi Serge,
> Please find commnet inline & let me know if any further action needed
>
> Thanks
> Suraj
>
> On 12/14/2023 8:42 PM, Serge Semin wrote:
> > Hi Suraj
> >
> > On Tue, Dec 12, 2023 at 05:28:41PM +0530, Suraj Jaiswal wrote:
> >> Add support to listen HW safety IRQ like ECC(error
> >> correction code), DPP(data path parity), FSM(finite state
> >> machine) fault in common IRQ line.
> >>
> >> Signed-off-by: Suraj Jaiswal <[email protected]>
> >> ---
> >> drivers/net/ethernet/stmicro/stmmac/common.h | 1 +
> >> drivers/net/ethernet/stmicro/stmmac/stmmac.h | 3 +++
> >> .../net/ethernet/stmicro/stmmac/stmmac_main.c | 21 +++++++++++++++++++
> >> .../ethernet/stmicro/stmmac/stmmac_platform.c | 9 ++++++++
> >> 4 files changed, 34 insertions(+)
> >>
> >> diff --git a/drivers/net/ethernet/stmicro/stmmac/common.h b/drivers/net/ethernet/stmicro/stmmac/common.h
> >> index 721c1f8e892f..b9233b09b80f 100644
> >> --- a/drivers/net/ethernet/stmicro/stmmac/common.h
> >> +++ b/drivers/net/ethernet/stmicro/stmmac/common.h
> >> @@ -344,6 +344,7 @@ enum request_irq_err {
> >> REQ_IRQ_ERR_ALL,
> >> REQ_IRQ_ERR_TX,
> >> REQ_IRQ_ERR_RX,
> >> + REQ_IRQ_ERR_SFTY,
> >> REQ_IRQ_ERR_SFTY_UE,
> >> REQ_IRQ_ERR_SFTY_CE,
> >> REQ_IRQ_ERR_LPI,
> >> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac.h b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
> >> index 9f89acf31050..ca3d93851bed 100644
> >> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac.h
> >> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
> >> @@ -31,6 +31,7 @@ struct stmmac_resources {
> >> int wol_irq;
> >> int lpi_irq;
> >> int irq;
> >> + int sfty_irq;
> >> int sfty_ce_irq;
> >> int sfty_ue_irq;
> >> int rx_irq[MTL_MAX_RX_QUEUES];
> >> @@ -297,6 +298,7 @@ struct stmmac_priv {
> >> void __iomem *ptpaddr;
> >> void __iomem *estaddr;
> >> unsigned long active_vlans[BITS_TO_LONGS(VLAN_N_VID)];
> >> + int sfty_irq;
> >> int sfty_ce_irq;
> >> int sfty_ue_irq;
> >> int rx_irq[MTL_MAX_RX_QUEUES];
> >> @@ -305,6 +307,7 @@ struct stmmac_priv {
> >> char int_name_mac[IFNAMSIZ + 9];
> >> char int_name_wol[IFNAMSIZ + 9];
> >> char int_name_lpi[IFNAMSIZ + 9];
> >> + char int_name_sfty[IFNAMSIZ + 10];
> >> char int_name_sfty_ce[IFNAMSIZ + 10];
> >> char int_name_sfty_ue[IFNAMSIZ + 10];
> >> char int_name_rx_irq[MTL_MAX_TX_QUEUES][IFNAMSIZ + 14];
> >> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> >> index 47de466e432c..6cf289f192a7 100644
> >> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> >> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> >> @@ -3592,6 +3592,10 @@ static void stmmac_free_irq(struct net_device *dev,
> >> if (priv->wol_irq > 0 && priv->wol_irq != dev->irq)
> >> free_irq(priv->wol_irq, dev);
> >> fallthrough;
> >> + case REQ_IRQ_ERR_SFTY:
> >> + if (priv->sfty_irq > 0 && priv->sfty_irq != dev->irq)
> >> + free_irq(priv->sfty_irq, dev);
> >> + fallthrough;
> >> case REQ_IRQ_ERR_WOL:
> >> free_irq(dev->irq, dev);
> >> fallthrough;
> >> @@ -3759,6 +3763,7 @@ static int stmmac_request_irq_single(struct net_device *dev)
> >> struct stmmac_priv *priv = netdev_priv(dev);
> >> enum request_irq_err irq_err;
> >> int ret;
> >
> >> + char *int_name;
> >
> > See my comment below.
> >
> >>
> >> ret = request_irq(dev->irq, stmmac_interrupt,
> >> IRQF_SHARED, dev->name, dev);
> >> @@ -3798,6 +3803,20 @@ static int stmmac_request_irq_single(struct net_device *dev)
> >> }
> >> }
> >>
> >
> >> + if (priv->sfty_irq > 0 && priv->sfty_irq != dev->irq) {
> >> + int_name = priv->int_name_sfty;
> >> + sprintf(int_name, "%s:%s", dev->name, "safety");
> >> + ret = request_irq(priv->sfty_irq, stmmac_safety_interrupt,
> >> + 0, int_name, dev);
> >> + if (unlikely(ret < 0)) {
> >> + netdev_err(priv->dev,
> >> + "%s: alloc safety failed %d (error: %d)\n",
> >> + __func__, priv->sfty_irq, ret);
> >> + irq_err = REQ_IRQ_ERR_SFTY;
> >> + goto irq_error;
> >> + }
> >> + }
> >> +
> >
> > Omg, I thought this change belonged to stmmac_request_irq_multi_msi().
> > My bad, sorry. Please move the code above to
> > stmmac_request_irq_multi_msi() and get back the part in
> > stmmac_request_irq_single() as it was in v5,
Please note my comment regarding the common safety IRQ being supported
in both stmmac_request_irq_single() and stmmac_request_irq_multi_msi()
methods.
> but instead of specifying
> > "safety" IRQ name use "dev->name" as the rest of similar code snippets
> > in here have:
> >
> > + if (priv->sfty_irq > 0 && priv->sfty_irq != dev->irq) {
> > + ret = request_irq(priv->sfty_irq, stmmac_safety_interrupt,
> > + 0, dev->name, dev);
> > + if (unlikely(ret < 0)) {
> > + netdev_err(priv->dev,
> > + "%s: alloc safety failed %d (error: %d)\n",
> > + __func__, priv->sfty_irq, ret);
> > + irq_err = REQ_IRQ_ERR_SFTY;
> > + goto irq_error;
> > + }
> > + }
>
> <Suraj> We can not use "dev->name" as this is name already used by "stmmac_interrupt" @ https://elixir.bootlin.com/linux/v6.1.68/source/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c#L3655.
It's not that much of the problem. The main idea is to convert your
solution to following the _local_ coding convention. See, the rest of
the IRQs in stmmac_request_irq_single() are requested with "dev->name"
being specified as the IRQ name (irrespective to having such solution
being not that correct). That's what I was talking about. If you want
the safety IRQ to have an unique name, then please submit this patch
as I suggested above and _then_, on top of it, add a new patch which
would convert the entire stmmac_request_irq_single() method to
creating all IRQ names as it's, for instance, done in
stmmac_request_irq_multi_msi().
> <
> ret = request_irq(dev->irq, stmmac_interrupt,
> IRQF_SHARED, dev->name, dev);
> >
> if we are using same "dev->name" while requesting safety IRQ as well then "/proc/interrupt" will show same name eth0/eth1 for both "stmmac_interrupt" & "safety interrupt" and by looking at "/proc/interrupt" output we can not say which IRQ is for safety and which is for stmmac_interrupt.
Thanks. I am perfectly aware of that. Please see my comment above.
> >
> > I guess at some point afterwards we'll need to refactor the IRQs
> > request part of this driver: replace stmmac_request_irq_single() body
> > with the upper part of the stmmac_request_irq_multi_msi() method and
> > then just make the former method being called from the later one...
> >
> >> return 0;
> >>
> >> irq_error:
> >> @@ -7462,8 +7481,10 @@ int stmmac_dvr_probe(struct device *device,
> >> priv->dev->irq = res->irq;
> >> priv->wol_irq = res->wol_irq;
> >> priv->lpi_irq = res->lpi_irq;
> >> + priv->sfty_irq = res->sfty_irq;
> >> priv->sfty_ce_irq = res->sfty_ce_irq;
> >> priv->sfty_ue_irq = res->sfty_ue_irq;
> >
> >> +
> >
> > Please drop this change. The code below is attached to the code above
> > because it basically does the same but in the loop.
> <Suraj> below loop code "for (i = 0; i < MTL_MAX_RX_QUEUES; i++) priv->rx_irq[i] = res->rx_irq[i];" is not for rx_irq array and will not help for safety irq.
> Let me know if I got your commnet properly .
Sorry, you didn't. My comment concerned the _empty_ line you placed
between the code above and below. You shouldn't have done that.
> >
> >> for (i = 0; i < MTL_MAX_RX_QUEUES; i++)
> >> priv->rx_irq[i] = res->rx_irq[i];
> >> for (i = 0; i < MTL_MAX_TX_QUEUES; i++)
> >> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
> >> index 1ffde555da47..3808a3225a7d 100644
> >> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
> >> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
> >> @@ -726,6 +726,15 @@ int stmmac_get_platform_resources(struct platform_device *pdev,
> >> dev_info(&pdev->dev, "IRQ eth_lpi not found\n");
> >> }
> >>
> >> + stmmac_res->sfty_irq =
> >> + platform_get_irq_byname_optional(pdev, "sfty");
> >
> >> +
> >
> > Please drop this change too. It's normal to have a method call
> > attached to the error check statement especially seeing the rest of
> > the similar code snippets are designed that way in this function.
> <Suraj> Do you means to remove all below code where we are printing the dev_info() message ?
No. I was referring to the _empty_ line between the method above and
the error check code below. It's pointless and at the very least
breaks the local coding convention.
-Serge(y)
> We added this code similar to LPM code.
> >
> > -Serge(y)
> >
> >> + if (stmmac_res->sfty_irq < 0) {
> >> + if (stmmac_res->sfty_irq == -EPROBE_DEFER)
> >> + return -EPROBE_DEFER;
> >> + dev_info(&pdev->dev, "IRQ safety IRQ not found\n");
> >> + }
> >> +
> >> stmmac_res->addr = devm_platform_ioremap_resource(pdev, 0);
> >>
> >> return PTR_ERR_OR_ZERO(stmmac_res->addr);
> >> --
> >> 2.25.1
> >>
> >>
Hi Serge,
takan care of all commnets in V7 . Please review
Thanks
Suraj
On 12/18/2023 4:18 PM, Serge Semin wrote:
> On Mon, Dec 18, 2023 at 03:27:54PM +0530, Suraj Jaiswal wrote:
>>
>> Hi Serge,
>> Please find commnet inline & let me know if any further action needed
>>
>> Thanks
>> Suraj
>>
>> On 12/14/2023 8:42 PM, Serge Semin wrote:
>>> Hi Suraj
>>>
>>> On Tue, Dec 12, 2023 at 05:28:41PM +0530, Suraj Jaiswal wrote:
>>>> Add support to listen HW safety IRQ like ECC(error
>>>> correction code), DPP(data path parity), FSM(finite state
>>>> machine) fault in common IRQ line.
>>>>
>>>> Signed-off-by: Suraj Jaiswal <[email protected]>
>>>> ---
>>>> drivers/net/ethernet/stmicro/stmmac/common.h | 1 +
>>>> drivers/net/ethernet/stmicro/stmmac/stmmac.h | 3 +++
>>>> .../net/ethernet/stmicro/stmmac/stmmac_main.c | 21 +++++++++++++++++++
>>>> .../ethernet/stmicro/stmmac/stmmac_platform.c | 9 ++++++++
>>>> 4 files changed, 34 insertions(+)
>>>>
>>>> diff --git a/drivers/net/ethernet/stmicro/stmmac/common.h b/drivers/net/ethernet/stmicro/stmmac/common.h
>>>> index 721c1f8e892f..b9233b09b80f 100644
>>>> --- a/drivers/net/ethernet/stmicro/stmmac/common.h
>>>> +++ b/drivers/net/ethernet/stmicro/stmmac/common.h
>>>> @@ -344,6 +344,7 @@ enum request_irq_err {
>>>> REQ_IRQ_ERR_ALL,
>>>> REQ_IRQ_ERR_TX,
>>>> REQ_IRQ_ERR_RX,
>>>> + REQ_IRQ_ERR_SFTY,
>>>> REQ_IRQ_ERR_SFTY_UE,
>>>> REQ_IRQ_ERR_SFTY_CE,
>>>> REQ_IRQ_ERR_LPI,
>>>> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac.h b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
>>>> index 9f89acf31050..ca3d93851bed 100644
>>>> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac.h
>>>> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
>>>> @@ -31,6 +31,7 @@ struct stmmac_resources {
>>>> int wol_irq;
>>>> int lpi_irq;
>>>> int irq;
>>>> + int sfty_irq;
>>>> int sfty_ce_irq;
>>>> int sfty_ue_irq;
>>>> int rx_irq[MTL_MAX_RX_QUEUES];
>>>> @@ -297,6 +298,7 @@ struct stmmac_priv {
>>>> void __iomem *ptpaddr;
>>>> void __iomem *estaddr;
>>>> unsigned long active_vlans[BITS_TO_LONGS(VLAN_N_VID)];
>>>> + int sfty_irq;
>>>> int sfty_ce_irq;
>>>> int sfty_ue_irq;
>>>> int rx_irq[MTL_MAX_RX_QUEUES];
>>>> @@ -305,6 +307,7 @@ struct stmmac_priv {
>>>> char int_name_mac[IFNAMSIZ + 9];
>>>> char int_name_wol[IFNAMSIZ + 9];
>>>> char int_name_lpi[IFNAMSIZ + 9];
>>>> + char int_name_sfty[IFNAMSIZ + 10];
>>>> char int_name_sfty_ce[IFNAMSIZ + 10];
>>>> char int_name_sfty_ue[IFNAMSIZ + 10];
>>>> char int_name_rx_irq[MTL_MAX_TX_QUEUES][IFNAMSIZ + 14];
>>>> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
>>>> index 47de466e432c..6cf289f192a7 100644
>>>> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
>>>> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
>>>> @@ -3592,6 +3592,10 @@ static void stmmac_free_irq(struct net_device *dev,
>>>> if (priv->wol_irq > 0 && priv->wol_irq != dev->irq)
>>>> free_irq(priv->wol_irq, dev);
>>>> fallthrough;
>>>> + case REQ_IRQ_ERR_SFTY:
>>>> + if (priv->sfty_irq > 0 && priv->sfty_irq != dev->irq)
>>>> + free_irq(priv->sfty_irq, dev);
>>>> + fallthrough;
>>>> case REQ_IRQ_ERR_WOL:
>>>> free_irq(dev->irq, dev);
>>>> fallthrough;
>>>> @@ -3759,6 +3763,7 @@ static int stmmac_request_irq_single(struct net_device *dev)
>>>> struct stmmac_priv *priv = netdev_priv(dev);
>>>> enum request_irq_err irq_err;
>>>> int ret;
>>>
>>>> + char *int_name;
>>>
>>> See my comment below.
>>>
>>>>
>>>> ret = request_irq(dev->irq, stmmac_interrupt,
>>>> IRQF_SHARED, dev->name, dev);
>>>> @@ -3798,6 +3803,20 @@ static int stmmac_request_irq_single(struct net_device *dev)
>>>> }
>>>> }
>>>>
>>>
>>>> + if (priv->sfty_irq > 0 && priv->sfty_irq != dev->irq) {
>>>> + int_name = priv->int_name_sfty;
>>>> + sprintf(int_name, "%s:%s", dev->name, "safety");
>>>> + ret = request_irq(priv->sfty_irq, stmmac_safety_interrupt,
>>>> + 0, int_name, dev);
>>>> + if (unlikely(ret < 0)) {
>>>> + netdev_err(priv->dev,
>>>> + "%s: alloc safety failed %d (error: %d)\n",
>>>> + __func__, priv->sfty_irq, ret);
>>>> + irq_err = REQ_IRQ_ERR_SFTY;
>>>> + goto irq_error;
>>>> + }
>>>> + }
>>>> +
>>>
>
>>> Omg, I thought this change belonged to stmmac_request_irq_multi_msi().
>>> My bad, sorry. Please move the code above to
>>> stmmac_request_irq_multi_msi() and get back the part in
>>> stmmac_request_irq_single() as it was in v5,
>
> Please note my comment regarding the common safety IRQ being supported
> in both stmmac_request_irq_single() and stmmac_request_irq_multi_msi()
> methods.
>
>> but instead of specifying
>>> "safety" IRQ name use "dev->name" as the rest of similar code snippets
>>> in here have:
>>>
>>> + if (priv->sfty_irq > 0 && priv->sfty_irq != dev->irq) {
>>> + ret = request_irq(priv->sfty_irq, stmmac_safety_interrupt,
>>> + 0, dev->name, dev);
>>> + if (unlikely(ret < 0)) {
>>> + netdev_err(priv->dev,
>>> + "%s: alloc safety failed %d (error: %d)\n",
>>> + __func__, priv->sfty_irq, ret);
>>> + irq_err = REQ_IRQ_ERR_SFTY;
>>> + goto irq_error;
>>> + }
>>> + }
>>
>
>> <Suraj> We can not use "dev->name" as this is name already used by "stmmac_interrupt" @ https://elixir.bootlin.com/linux/v6.1.68/source/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c#L3655.
>
> It's not that much of the problem. The main idea is to convert your
> solution to following the _local_ coding convention. See, the rest of
> the IRQs in stmmac_request_irq_single() are requested with "dev->name"
> being specified as the IRQ name (irrespective to having such solution
> being not that correct). That's what I was talking about. If you want
> the safety IRQ to have an unique name, then please submit this patch
> as I suggested above and _then_, on top of it, add a new patch which
> would convert the entire stmmac_request_irq_single() method to
> creating all IRQ names as it's, for instance, done in
> stmmac_request_irq_multi_msi().
>
>> <
>> ret = request_irq(dev->irq, stmmac_interrupt,
>> IRQF_SHARED, dev->name, dev);
>>>
>
>> if we are using same "dev->name" while requesting safety IRQ as well then "/proc/interrupt" will show same name eth0/eth1 for both "stmmac_interrupt" & "safety interrupt" and by looking at "/proc/interrupt" output we can not say which IRQ is for safety and which is for stmmac_interrupt.
>
> Thanks. I am perfectly aware of that. Please see my comment above.
>
>>>
>>> I guess at some point afterwards we'll need to refactor the IRQs
>>> request part of this driver: replace stmmac_request_irq_single() body
>>> with the upper part of the stmmac_request_irq_multi_msi() method and
>>> then just make the former method being called from the later one...
>>>
>>>> return 0;
>>>>
>>>> irq_error:
>>>> @@ -7462,8 +7481,10 @@ int stmmac_dvr_probe(struct device *device,
>>>> priv->dev->irq = res->irq;
>>>> priv->wol_irq = res->wol_irq;
>>>> priv->lpi_irq = res->lpi_irq;
>>>> + priv->sfty_irq = res->sfty_irq;
>>>> priv->sfty_ce_irq = res->sfty_ce_irq;
>>>> priv->sfty_ue_irq = res->sfty_ue_irq;
>>>
>>>> +
>>>
>>> Please drop this change. The code below is attached to the code above
>>> because it basically does the same but in the loop.
>
>> <Suraj> below loop code "for (i = 0; i < MTL_MAX_RX_QUEUES; i++) priv->rx_irq[i] = res->rx_irq[i];" is not for rx_irq array and will not help for safety irq.
>> Let me know if I got your commnet properly .
>
> Sorry, you didn't. My comment concerned the _empty_ line you placed
> between the code above and below. You shouldn't have done that.
>
>>>
>>>> for (i = 0; i < MTL_MAX_RX_QUEUES; i++)
>>>> priv->rx_irq[i] = res->rx_irq[i];
>>>> for (i = 0; i < MTL_MAX_TX_QUEUES; i++)
>>>> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
>>>> index 1ffde555da47..3808a3225a7d 100644
>>>> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
>>>> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
>>>> @@ -726,6 +726,15 @@ int stmmac_get_platform_resources(struct platform_device *pdev,
>>>> dev_info(&pdev->dev, "IRQ eth_lpi not found\n");
>>>> }
>>>>
>>>> + stmmac_res->sfty_irq =
>>>> + platform_get_irq_byname_optional(pdev, "sfty");
>>>
>>>> +
>>>
>>> Please drop this change too. It's normal to have a method call
>>> attached to the error check statement especially seeing the rest of
>>> the similar code snippets are designed that way in this function.
>
>> <Suraj> Do you means to remove all below code where we are printing the dev_info() message ?
>
> No. I was referring to the _empty_ line between the method above and
> the error check code below. It's pointless and at the very least
> breaks the local coding convention.
>
> -Serge(y)
>
>> We added this code similar to LPM code.
>>>
>>> -Serge(y)
>>>
>>>> + if (stmmac_res->sfty_irq < 0) {
>>>> + if (stmmac_res->sfty_irq == -EPROBE_DEFER)
>>>> + return -EPROBE_DEFER;
>>>> + dev_info(&pdev->dev, "IRQ safety IRQ not found\n");
>>>> + }
>>>> +
>>>> stmmac_res->addr = devm_platform_ioremap_resource(pdev, 0);
>>>>
>>>> return PTR_ERR_OR_ZERO(stmmac_res->addr);
>>>> --
>>>> 2.25.1
>>>>
>>>>