| Message ID | 20231113110708.137379-4-rogerq@kernel.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: ethernet: am65-cpsw: Add ethtool standard MAC stats | expand |
On Mon, Nov 13, 2023 at 01:07:08PM +0200, Roger Quadros wrote: > k3_udma_glue_enable_rx/tx_chn returns error code on failure. > Bail out on error while enabling TX/RX channel. > > Signed-off-by: Roger Quadros <rogerq@kernel.org> Reviewed-by: Andrew Lunn <andrew@lunn.ch> Andrew
On Mon, Nov 13, 2023 at 01:07:08PM +0200, Roger Quadros wrote: > k3_udma_glue_enable_rx/tx_chn returns error code on failure. > Bail out on error while enabling TX/RX channel. > > Signed-off-by: Roger Quadros <rogerq@kernel.org> > --- > drivers/net/ethernet/ti/am65-cpsw-nuss.c | 33 +++++++++++++++++++----- > 1 file changed, 26 insertions(+), 7 deletions(-) > > diff --git a/drivers/net/ethernet/ti/am65-cpsw-nuss.c b/drivers/net/ethernet/ti/am65-cpsw-nuss.c > index 7c440899c93c..340f25bf33b1 100644 > --- a/drivers/net/ethernet/ti/am65-cpsw-nuss.c > +++ b/drivers/net/ethernet/ti/am65-cpsw-nuss.c > @@ -372,7 +372,7 @@ static void am65_cpsw_init_port_emac_ale(struct am65_cpsw_port *port); > static int am65_cpsw_nuss_common_open(struct am65_cpsw_common *common) > { > struct am65_cpsw_host *host_p = am65_common_get_host(common); > - int port_idx, i, ret; > + int port_idx, i, ret, tx; > struct sk_buff *skb; > u32 val, port_mask; > > @@ -453,13 +453,22 @@ static int am65_cpsw_nuss_common_open(struct am65_cpsw_common *common) > } > kmemleak_not_leak(skb); > } > - k3_udma_glue_enable_rx_chn(common->rx_chns.rx_chn); > > - for (i = 0; i < common->tx_ch_num; i++) { > - ret = k3_udma_glue_enable_tx_chn(common->tx_chns[i].tx_chn); > - if (ret) > - return ret; Can you comment on the kmemleak_not_leak(skb) call above, and its relationship to the pre-existing error handling path in am65_cpsw_nuss_common_open()? I see that the dev_kfree_skb_any() call is being made from am65_cpsw_nuss_rx_cleanup(), which is only called from am65_cpsw_nuss_common_stop(). So if there are errors during am65_cpsw_nuss_common_open() and descriptors have already been added to the RX DMA channel, they will not be removed either from hardware or from software. How does that work?
On 14/11/2023 14:07, Vladimir Oltean wrote: > On Mon, Nov 13, 2023 at 01:07:08PM +0200, Roger Quadros wrote: >> k3_udma_glue_enable_rx/tx_chn returns error code on failure. >> Bail out on error while enabling TX/RX channel. >> >> Signed-off-by: Roger Quadros <rogerq@kernel.org> >> --- >> drivers/net/ethernet/ti/am65-cpsw-nuss.c | 33 +++++++++++++++++++----- >> 1 file changed, 26 insertions(+), 7 deletions(-) >> >> diff --git a/drivers/net/ethernet/ti/am65-cpsw-nuss.c b/drivers/net/ethernet/ti/am65-cpsw-nuss.c >> index 7c440899c93c..340f25bf33b1 100644 >> --- a/drivers/net/ethernet/ti/am65-cpsw-nuss.c >> +++ b/drivers/net/ethernet/ti/am65-cpsw-nuss.c >> @@ -372,7 +372,7 @@ static void am65_cpsw_init_port_emac_ale(struct am65_cpsw_port *port); >> static int am65_cpsw_nuss_common_open(struct am65_cpsw_common *common) >> { >> struct am65_cpsw_host *host_p = am65_common_get_host(common); >> - int port_idx, i, ret; >> + int port_idx, i, ret, tx; >> struct sk_buff *skb; >> u32 val, port_mask; >> >> @@ -453,13 +453,22 @@ static int am65_cpsw_nuss_common_open(struct am65_cpsw_common *common) >> } >> kmemleak_not_leak(skb); >> } >> - k3_udma_glue_enable_rx_chn(common->rx_chns.rx_chn); >> >> - for (i = 0; i < common->tx_ch_num; i++) { >> - ret = k3_udma_glue_enable_tx_chn(common->tx_chns[i].tx_chn); >> - if (ret) >> - return ret; > > Can you comment on the kmemleak_not_leak(skb) call above, and its > relationship to the pre-existing error handling path in am65_cpsw_nuss_common_open()? I am not aware why it was added. It sure looks odd and I'll get rid of it and add the necessary error handling. > I see that the dev_kfree_skb_any() call is being made from am65_cpsw_nuss_rx_cleanup(), > which is only called from am65_cpsw_nuss_common_stop(). > > So if there are errors during am65_cpsw_nuss_common_open() and > descriptors have already been added to the RX DMA channel, they will not > be removed either from hardware or from software. How does that work? I believe this is a gap and I will address it in the next revision. Thanks!
diff --git a/drivers/net/ethernet/ti/am65-cpsw-nuss.c b/drivers/net/ethernet/ti/am65-cpsw-nuss.c index 7c440899c93c..340f25bf33b1 100644 --- a/drivers/net/ethernet/ti/am65-cpsw-nuss.c +++ b/drivers/net/ethernet/ti/am65-cpsw-nuss.c @@ -372,7 +372,7 @@ static void am65_cpsw_init_port_emac_ale(struct am65_cpsw_port *port); static int am65_cpsw_nuss_common_open(struct am65_cpsw_common *common) { struct am65_cpsw_host *host_p = am65_common_get_host(common); - int port_idx, i, ret; + int port_idx, i, ret, tx; struct sk_buff *skb; u32 val, port_mask; @@ -453,13 +453,22 @@ static int am65_cpsw_nuss_common_open(struct am65_cpsw_common *common) } kmemleak_not_leak(skb); } - k3_udma_glue_enable_rx_chn(common->rx_chns.rx_chn); - for (i = 0; i < common->tx_ch_num; i++) { - ret = k3_udma_glue_enable_tx_chn(common->tx_chns[i].tx_chn); - if (ret) - return ret; - napi_enable(&common->tx_chns[i].napi_tx); + ret = k3_udma_glue_enable_rx_chn(common->rx_chns.rx_chn); + if (ret) { + dev_err(common->dev, "couldn't enable rx chn: %d\n", ret); + return ret; + } + + for (tx = 0; tx < common->tx_ch_num; tx++) { + ret = k3_udma_glue_enable_tx_chn(common->tx_chns[tx].tx_chn); + if (ret) { + dev_err(common->dev, "couldn't enable tx chn %d: %d\n", + tx, ret); + tx--; + goto fail_tx; + } + napi_enable(&common->tx_chns[tx].napi_tx); } napi_enable(&common->napi_rx); @@ -470,6 +479,16 @@ static int am65_cpsw_nuss_common_open(struct am65_cpsw_common *common) dev_dbg(common->dev, "cpsw_nuss started\n"); return 0; + +fail_tx: + while (tx >= 0) { + napi_disable(&common->tx_chns[tx].napi_tx); + k3_udma_glue_disable_tx_chn(common->tx_chns[tx].tx_chn); + tx--; + } + + k3_udma_glue_disable_rx_chn(common->rx_chns.rx_chn); + return ret; } static void am65_cpsw_nuss_tx_cleanup(void *data, dma_addr_t desc_dma);
k3_udma_glue_enable_rx/tx_chn returns error code on failure. Bail out on error while enabling TX/RX channel. Signed-off-by: Roger Quadros <rogerq@kernel.org> --- drivers/net/ethernet/ti/am65-cpsw-nuss.c | 33 +++++++++++++++++++----- 1 file changed, 26 insertions(+), 7 deletions(-)