diff mbox series

[net-next,3/3] net: ethernet: am65-cpsw: Error out if Enable TX/RX channel fails

Message ID 20231113110708.137379-4-rogerq@kernel.org
State New
Headers
Series net: ethernet: am65-cpsw: Add ethtool standard MAC stats |

Commit Message

Roger Quadros Nov. 13, 2023, 11:07 a.m. UTC 
  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(-)

Comments

Andrew Lunn Nov. 13, 2023, 1:27 p.m. UTC | #1 
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
Vladimir Oltean Nov. 14, 2023, 12:07 p.m. UTC | #2 
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?
Roger Quadros Nov. 14, 2023, 7:53 p.m. UTC | #3 
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 mbox series

Patch

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);