| Message ID | 62eaaace3713751cb1ecac3163e857737107ca0e.1700737841.git.quic_jsuraj@quicinc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Ethernet DWMAC5 fault IRQ support | expand |
On 11/23/23 12:38, Suraj Jaiswal wrote: > Add IRQ support to listen HW fault like ECC,DPP,FSM > fault and print the fault information in the kernel > log. > > Signed-off-by: Suraj Jaiswal <quic_jsuraj@quicinc.com> > --- > drivers/net/ethernet/stmicro/stmmac/common.h | 1 + > drivers/net/ethernet/stmicro/stmmac/stmmac.h | 2 ++ > .../net/ethernet/stmicro/stmmac/stmmac_main.c | 18 +++++++++++++++++ > .../ethernet/stmicro/stmmac/stmmac_platform.c | 20 +++++++++++++++++++ > 4 files changed, 41 insertions(+) > > diff --git a/drivers/net/ethernet/stmicro/stmmac/common.h b/drivers/net/ethernet/stmicro/stmmac/common.h > index 6b935922054d..c4821c7ab674 100644 > --- a/drivers/net/ethernet/stmicro/stmmac/common.h > +++ b/drivers/net/ethernet/stmicro/stmmac/common.h > @@ -347,6 +347,7 @@ enum request_irq_err { > REQ_IRQ_ERR_SFTY_UE, > REQ_IRQ_ERR_SFTY_CE, > REQ_IRQ_ERR_LPI, > + REQ_IRQ_ERR_SAFETY, > REQ_IRQ_ERR_WOL, > REQ_IRQ_ERR_MAC, > REQ_IRQ_ERR_NO, > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac.h b/drivers/net/ethernet/stmicro/stmmac/stmmac.h > index cd7a9768de5f..8893d4b7fa38 100644 > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac.h > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac.h > @@ -33,6 +33,7 @@ struct stmmac_resources { > int irq; > int sfty_ce_irq; > int sfty_ue_irq; > + int safety_common_intr; > int rx_irq[MTL_MAX_RX_QUEUES]; > int tx_irq[MTL_MAX_TX_QUEUES]; > }; > @@ -331,6 +332,7 @@ struct stmmac_priv { > /* XDP BPF Program */ > unsigned long *af_xdp_zc_qps; > struct bpf_prog *xdp_prog; > + int safety_common_intr; other interrupts use _irq yet you seem to use _intr, plus the aforementioned "safety" vs "fault" naming [...] > > +int stmmac_get_fault_intr_config(struct platform_device *pdev, struct stmmac_resources *res) > +{ > + int ret = 0; get rid of ret and return directly > + > + res->safety_common_intr = platform_get_irq_byname(pdev, "safety"); > + stray newline? > + if (res->safety_common_intr < 0) { > + if (res->safety_common_intr != -EPROBE_DEFER) > + dev_err(&pdev->dev, "safety IRQ configuration information not found\n"); > + ret = 1; > + } > + > + return ret; > +} > + > int stmmac_get_platform_resources(struct platform_device *pdev, > struct stmmac_resources *stmmac_res) > { > + int ret = 0; Missing newline between declarations and code Unnecessary initialization > memset(stmmac_res, 0, sizeof(*stmmac_res)); > > /* Get IRQ information early to have an ability to ask for deferred > @@ -702,6 +718,10 @@ int stmmac_get_platform_resources(struct platform_device *pdev, > if (stmmac_res->irq < 0) > return stmmac_res->irq; > > + ret = stmmac_get_fault_intr_config(pdev, stmmac_res); > + if (ret) > + dev_err(&pdev->dev, "Fault interrupt not present\n"); Since you're getting the return value, perhaps the errno should be propagated Konrad
On Thu, Nov 23, 2023 at 05:08:15PM +0530, Suraj Jaiswal wrote: > Add IRQ support to listen HW fault like ECC,DPP,FSM > fault and print the fault information in the kernel > log. > > Signed-off-by: Suraj Jaiswal <quic_jsuraj@quicinc.com> ... > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c > index 1ffde555da47..bae1704d5f4b 100644 > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c > @@ -690,9 +690,25 @@ devm_stmmac_probe_config_dt(struct platform_device *pdev, u8 *mac) > #endif /* CONFIG_OF */ > EXPORT_SYMBOL_GPL(devm_stmmac_probe_config_dt); > > +int stmmac_get_fault_intr_config(struct platform_device *pdev, struct stmmac_resources *res) Hi Suraj, stmmac_get_fault_intr_config() appears to only be used in this function. If so it should be static. Also, please consider limiting Networking code to 80 columns wide. static int stmmac_get_fault_intr_config(struct platform_device *pdev, struct stmmac_resources *res) ...
> @@ -702,6 +718,10 @@ int stmmac_get_platform_resources(struct platform_device *pdev, > if (stmmac_res->irq < 0) > return stmmac_res->irq; > > + ret = stmmac_get_fault_intr_config(pdev, stmmac_res); > + if (ret) > + dev_err(&pdev->dev, "Fault interrupt not present\n"); This fault/saftey/foobar interrupt is optional? So printing any error message it is missing does not seem like a good idea. Andrew
Hi Suraj, kernel test robot noticed the following build warnings: [auto build test WARNING on net-next/main] url: https://github.com/intel-lab-lkp/linux/commits/Suraj-Jaiswal/dt-bindings-net-qcom-ethqos-add-binding-doc-for-fault-IRQ-for-sa8775p/20231123-202252 base: net-next/main patch link: https://lore.kernel.org/r/62eaaace3713751cb1ecac3163e857737107ca0e.1700737841.git.quic_jsuraj%40quicinc.com patch subject: [PATCH net-next v3 3/3] net: stmmac: Add driver support for DWMAC5 fault IRQ Support config: csky-randconfig-r081-20231124 (https://download.01.org/0day-ci/archive/20231124/202311241444.wkNnpI5Q-lkp@intel.com/config) compiler: csky-linux-gcc (GCC) 13.2.0 reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231124/202311241444.wkNnpI5Q-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202311241444.wkNnpI5Q-lkp@intel.com/ All warnings (new ones prefixed by >>): >> drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c:693:5: warning: no previous prototype for 'stmmac_get_fault_intr_config' [-Wmissing-prototypes] 693 | int stmmac_get_fault_intr_config(struct platform_device *pdev, struct stmmac_resources *res) | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~ vim +/stmmac_get_fault_intr_config +693 drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c 692 > 693 int stmmac_get_fault_intr_config(struct platform_device *pdev, struct stmmac_resources *res) 694 { 695 int ret = 0; 696 697 res->safety_common_intr = platform_get_irq_byname(pdev, "safety"); 698 699 if (res->safety_common_intr < 0) { 700 if (res->safety_common_intr != -EPROBE_DEFER) 701 dev_err(&pdev->dev, "safety IRQ configuration information not found\n"); 702 ret = 1; 703 } 704 705 return ret; 706 } 707
Hi Suraj, kernel test robot noticed the following build warnings: [auto build test WARNING on net-next/main] url: https://github.com/intel-lab-lkp/linux/commits/Suraj-Jaiswal/dt-bindings-net-qcom-ethqos-add-binding-doc-for-fault-IRQ-for-sa8775p/20231123-202252 base: net-next/main patch link: https://lore.kernel.org/r/62eaaace3713751cb1ecac3163e857737107ca0e.1700737841.git.quic_jsuraj%40quicinc.com patch subject: [PATCH net-next v3 3/3] net: stmmac: Add driver support for DWMAC5 fault IRQ Support config: arm-defconfig (https://download.01.org/0day-ci/archive/20231124/202311241513.D6JpHWGg-lkp@intel.com/config) compiler: clang version 14.0.6 (https://github.com/llvm/llvm-project.git f28c006a5895fc0e329fe15fead81e37457cb1d1) reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231124/202311241513.D6JpHWGg-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202311241513.D6JpHWGg-lkp@intel.com/ All warnings (new ones prefixed by >>): >> drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c:693:5: warning: no previous prototype for function 'stmmac_get_fault_intr_config' [-Wmissing-prototypes] int stmmac_get_fault_intr_config(struct platform_device *pdev, struct stmmac_resources *res) ^ drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c:693:1: note: declare 'static' if the function is not intended to be used outside of this translation unit int stmmac_get_fault_intr_config(struct platform_device *pdev, struct stmmac_resources *res) ^ static 1 warning generated. vim +/stmmac_get_fault_intr_config +693 drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c 692 > 693 int stmmac_get_fault_intr_config(struct platform_device *pdev, struct stmmac_resources *res) 694 { 695 int ret = 0; 696 697 res->safety_common_intr = platform_get_irq_byname(pdev, "safety"); 698 699 if (res->safety_common_intr < 0) { 700 if (res->safety_common_intr != -EPROBE_DEFER) 701 dev_err(&pdev->dev, "safety IRQ configuration information not found\n"); 702 ret = 1; 703 } 704 705 return ret; 706 } 707
On Thu, Nov 23, 2023 at 05:08:15PM +0530, Suraj Jaiswal wrote: > Add IRQ support to listen HW fault like ECC,DPP,FSM > fault and print the fault information in the kernel > log. > > Signed-off-by: Suraj Jaiswal <quic_jsuraj@quicinc.com> > --- > drivers/net/ethernet/stmicro/stmmac/common.h | 1 + > drivers/net/ethernet/stmicro/stmmac/stmmac.h | 2 ++ > .../net/ethernet/stmicro/stmmac/stmmac_main.c | 18 +++++++++++++++++ > .../ethernet/stmicro/stmmac/stmmac_platform.c | 20 +++++++++++++++++++ > 4 files changed, 41 insertions(+) > > diff --git a/drivers/net/ethernet/stmicro/stmmac/common.h b/drivers/net/ethernet/stmicro/stmmac/common.h > index 6b935922054d..c4821c7ab674 100644 > --- a/drivers/net/ethernet/stmicro/stmmac/common.h > +++ b/drivers/net/ethernet/stmicro/stmmac/common.h > @@ -347,6 +347,7 @@ enum request_irq_err { > REQ_IRQ_ERR_SFTY_UE, > REQ_IRQ_ERR_SFTY_CE, > REQ_IRQ_ERR_LPI, > + REQ_IRQ_ERR_SAFETY, > REQ_IRQ_ERR_WOL, > REQ_IRQ_ERR_MAC, > REQ_IRQ_ERR_NO, > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac.h b/drivers/net/ethernet/stmicro/stmmac/stmmac.h > index cd7a9768de5f..8893d4b7fa38 100644 > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac.h > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac.h > @@ -33,6 +33,7 @@ struct stmmac_resources { > int irq; > int sfty_ce_irq; > int sfty_ue_irq; > + int safety_common_intr; > int rx_irq[MTL_MAX_RX_QUEUES]; > int tx_irq[MTL_MAX_TX_QUEUES]; > }; > @@ -331,6 +332,7 @@ struct stmmac_priv { > /* XDP BPF Program */ > unsigned long *af_xdp_zc_qps; > struct bpf_prog *xdp_prog; > + int safety_common_intr; > }; > > enum stmmac_state { > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > index 8964fc8a955f..2ae4f34444de 100644 > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > @@ -3530,6 +3530,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_SAFETY: > + if (priv->safety_common_intr > 0 && priv->safety_common_intr != dev->irq) > + free_irq(priv->safety_common_intr, dev); > + fallthrough; > case REQ_IRQ_ERR_WOL: > free_irq(dev->irq, dev); > fallthrough; > @@ -3736,6 +3740,18 @@ static int stmmac_request_irq_single(struct net_device *dev) > } > } > > + if (priv->safety_common_intr > 0 && priv->safety_common_intr != dev->irq) { > + ret = request_irq(priv->safety_common_intr, stmmac_safety_interrupt, > + 0, "safety", dev); > + if (unlikely(ret < 0)) { > + netdev_err(priv->dev, > + "%s: alloc safety failed %d (error: %d)\n", > + __func__, priv->safety_common_intr, ret); > + irq_err = REQ_IRQ_ERR_SAFETY; > + goto irq_error; > + } > + } > + > return 0; > > irq_error: > @@ -7398,6 +7414,8 @@ int stmmac_dvr_probe(struct device *device, > priv->lpi_irq = res->lpi_irq; > priv->sfty_ce_irq = res->sfty_ce_irq; > priv->sfty_ue_irq = res->sfty_ue_irq; > + priv->safety_common_intr = res->safety_common_intr; > + > 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..bae1704d5f4b 100644 > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c > @@ -690,9 +690,25 @@ devm_stmmac_probe_config_dt(struct platform_device *pdev, u8 *mac) > #endif /* CONFIG_OF */ > EXPORT_SYMBOL_GPL(devm_stmmac_probe_config_dt); > > +int stmmac_get_fault_intr_config(struct platform_device *pdev, struct stmmac_resources *res) > +{ > + int ret = 0; > + > + res->safety_common_intr = platform_get_irq_byname(pdev, "safety"); > + > + if (res->safety_common_intr < 0) { > + if (res->safety_common_intr != -EPROBE_DEFER) > + dev_err(&pdev->dev, "safety IRQ configuration information not found\n"); > + ret = 1; > + } > + > + return ret; > +} I think other reviewers have covered most of what I want to say, but I think this doesn't deserve its own function and should just be done directly, as is done for eth_lpi for example. I think it also should be considered an optional interrupt based on my understanding of its purpose (just like eth_lpi). > + > int stmmac_get_platform_resources(struct platform_device *pdev, > struct stmmac_resources *stmmac_res) > { > + int ret = 0; > memset(stmmac_res, 0, sizeof(*stmmac_res)); > > /* Get IRQ information early to have an ability to ask for deferred > @@ -702,6 +718,10 @@ int stmmac_get_platform_resources(struct platform_device *pdev, > if (stmmac_res->irq < 0) > return stmmac_res->irq; > > + ret = stmmac_get_fault_intr_config(pdev, stmmac_res); > + if (ret) > + dev_err(&pdev->dev, "Fault interrupt not present\n"); > + > /* On some platforms e.g. SPEAr the wake up irq differs from the mac irq > * The external wake up irq can be passed through the platform code > * named as "eth_wake_irq" > -- > 2.25.1 >
diff --git a/drivers/net/ethernet/stmicro/stmmac/common.h b/drivers/net/ethernet/stmicro/stmmac/common.h index 6b935922054d..c4821c7ab674 100644 --- a/drivers/net/ethernet/stmicro/stmmac/common.h +++ b/drivers/net/ethernet/stmicro/stmmac/common.h @@ -347,6 +347,7 @@ enum request_irq_err { REQ_IRQ_ERR_SFTY_UE, REQ_IRQ_ERR_SFTY_CE, REQ_IRQ_ERR_LPI, + REQ_IRQ_ERR_SAFETY, REQ_IRQ_ERR_WOL, REQ_IRQ_ERR_MAC, REQ_IRQ_ERR_NO, diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac.h b/drivers/net/ethernet/stmicro/stmmac/stmmac.h index cd7a9768de5f..8893d4b7fa38 100644 --- a/drivers/net/ethernet/stmicro/stmmac/stmmac.h +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac.h @@ -33,6 +33,7 @@ struct stmmac_resources { int irq; int sfty_ce_irq; int sfty_ue_irq; + int safety_common_intr; int rx_irq[MTL_MAX_RX_QUEUES]; int tx_irq[MTL_MAX_TX_QUEUES]; }; @@ -331,6 +332,7 @@ struct stmmac_priv { /* XDP BPF Program */ unsigned long *af_xdp_zc_qps; struct bpf_prog *xdp_prog; + int safety_common_intr; }; enum stmmac_state { diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c index 8964fc8a955f..2ae4f34444de 100644 --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c @@ -3530,6 +3530,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_SAFETY: + if (priv->safety_common_intr > 0 && priv->safety_common_intr != dev->irq) + free_irq(priv->safety_common_intr, dev); + fallthrough; case REQ_IRQ_ERR_WOL: free_irq(dev->irq, dev); fallthrough; @@ -3736,6 +3740,18 @@ static int stmmac_request_irq_single(struct net_device *dev) } } + if (priv->safety_common_intr > 0 && priv->safety_common_intr != dev->irq) { + ret = request_irq(priv->safety_common_intr, stmmac_safety_interrupt, + 0, "safety", dev); + if (unlikely(ret < 0)) { + netdev_err(priv->dev, + "%s: alloc safety failed %d (error: %d)\n", + __func__, priv->safety_common_intr, ret); + irq_err = REQ_IRQ_ERR_SAFETY; + goto irq_error; + } + } + return 0; irq_error: @@ -7398,6 +7414,8 @@ int stmmac_dvr_probe(struct device *device, priv->lpi_irq = res->lpi_irq; priv->sfty_ce_irq = res->sfty_ce_irq; priv->sfty_ue_irq = res->sfty_ue_irq; + priv->safety_common_intr = res->safety_common_intr; + 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..bae1704d5f4b 100644 --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c @@ -690,9 +690,25 @@ devm_stmmac_probe_config_dt(struct platform_device *pdev, u8 *mac) #endif /* CONFIG_OF */ EXPORT_SYMBOL_GPL(devm_stmmac_probe_config_dt); +int stmmac_get_fault_intr_config(struct platform_device *pdev, struct stmmac_resources *res) +{ + int ret = 0; + + res->safety_common_intr = platform_get_irq_byname(pdev, "safety"); + + if (res->safety_common_intr < 0) { + if (res->safety_common_intr != -EPROBE_DEFER) + dev_err(&pdev->dev, "safety IRQ configuration information not found\n"); + ret = 1; + } + + return ret; +} + int stmmac_get_platform_resources(struct platform_device *pdev, struct stmmac_resources *stmmac_res) { + int ret = 0; memset(stmmac_res, 0, sizeof(*stmmac_res)); /* Get IRQ information early to have an ability to ask for deferred @@ -702,6 +718,10 @@ int stmmac_get_platform_resources(struct platform_device *pdev, if (stmmac_res->irq < 0) return stmmac_res->irq; + ret = stmmac_get_fault_intr_config(pdev, stmmac_res); + if (ret) + dev_err(&pdev->dev, "Fault interrupt not present\n"); + /* On some platforms e.g. SPEAr the wake up irq differs from the mac irq * The external wake up irq can be passed through the platform code * named as "eth_wake_irq"
Add IRQ support to listen HW fault like ECC,DPP,FSM fault and print the fault information in the kernel log. Signed-off-by: Suraj Jaiswal <quic_jsuraj@quicinc.com> --- drivers/net/ethernet/stmicro/stmmac/common.h | 1 + drivers/net/ethernet/stmicro/stmmac/stmmac.h | 2 ++ .../net/ethernet/stmicro/stmmac/stmmac_main.c | 18 +++++++++++++++++ .../ethernet/stmicro/stmmac/stmmac_platform.c | 20 +++++++++++++++++++ 4 files changed, 41 insertions(+)