mbox series

[net-next,0/3] cxgb4/ch_ktls: updates in net-next

Message ID 20200922174501.14943-1-rohitm@chelsio.com
Headers show
Series cxgb4/ch_ktls: updates in net-next | expand

Message

Rohit Maheshwari Sept. 22, 2020, 5:44 p.m. UTC 
This series of patches improves connections setup and statistics.

This series is broken down as follows:

Patch 1 fixes the handling of connection setup failure in HW. Driver
shouldn't return success to tls_dev_add, until HW returns success.

Patch 2 avoids the log flood.

Patch 3 adds ktls statistics at port level.

Rohit Maheshwari (3):
  ch_ktls: Issue if connection offload fails
  cxgb4: Avoid log flood
  cxgb4/ch_ktls: ktls stats are added at port level

 .../ethernet/chelsio/cxgb4/cxgb4_debugfs.c    |  35 +--
 .../ethernet/chelsio/cxgb4/cxgb4_ethtool.c    |  50 +++-
 .../net/ethernet/chelsio/cxgb4/cxgb4_uld.c    |   8 +-
 .../net/ethernet/chelsio/cxgb4/cxgb4_uld.h    |  21 +-
 .../chelsio/inline_crypto/ch_ktls/chcr_ktls.c | 278 +++++++++---------
 .../chelsio/inline_crypto/ch_ktls/chcr_ktls.h |  15 +-
 6 files changed, 206 insertions(+), 201 deletions(-)

Comments

Jakub Kicinski Sept. 22, 2020, 10:44 p.m. UTC | #1 
On Tue, 22 Sep 2020 23:14:59 +0530 Rohit Maheshwari wrote:
> Since driver first return success to tls_dev_add, if req to HW is
> successful, but later if HW returns failure, that connection traffic
> fails permanently and connection status remains unknown to stack.
> 
> Fixes: 34aba2c45024 ("cxgb4/chcr : Register to tls add and del callback")
> Signed-off-by: Rohit Maheshwari <rohitm@chelsio.com>

>  #if IS_ENABLED(CONFIG_IPV6)
>  	} else {
> -		if (!sk->sk_ipv6only &&
> -		    ipv6_addr_type(&sk->sk_v6_daddr) == IPV6_ADDR_MAPPED) {
> -			tx_info->ip_family = AF_INET;
> -			ret = chcr_ktls_act_open_req(sk, tx_info, atid);
> -		} else {
> -			tx_info->ip_family = AF_INET6;
> -			ret = cxgb4_clip_get(tx_info->netdev,
> -					     (const u32 *)
> -					     &sk->sk_v6_rcv_saddr.s6_addr,
> -					     1);
> -			if (ret)
> -				goto out;
> -			ret = chcr_ktls_act_open_req6(sk, tx_info, atid);
> -		}
> +		ret = cxgb4_clip_get(tx_info->netdev, (const u32 *)
> +				     &sk->sk_v6_rcv_saddr,
> +				     1);
> +		if (ret)
> +			return ret;
> +		ret = chcr_ktls_act_open_req6(sk, tx_info, atid);

You removed the mapped socket handling which seems unrelated to the
rest of the patch.

> +	spin_lock(&tx_info->lock);
> +	tx_info->conn_up = true;
> +	spin_unlock(&tx_info->lock);

What's the context this lock is taken in? You seem to always do only
spin_lock(), does the control path not need to be _bh() or _irq()?
Rohit Maheshwari Sept. 24, 2020, 4:45 a.m. UTC | #2 
On 23/09/20 4:14 AM, Jakub Kicinski wrote:
> On Tue, 22 Sep 2020 23:14:59 +0530 Rohit Maheshwari wrote:

>> Since driver first return success to tls_dev_add, if req to HW is

>> successful, but later if HW returns failure, that connection traffic

>> fails permanently and connection status remains unknown to stack.

>>

>> Fixes: 34aba2c45024 ("cxgb4/chcr : Register to tls add and del callback")

>> Signed-off-by: Rohit Maheshwari <rohitm@chelsio.com>

>>   #if IS_ENABLED(CONFIG_IPV6)

>>   	} else {

>> -		if (!sk->sk_ipv6only &&

>> -		    ipv6_addr_type(&sk->sk_v6_daddr) == IPV6_ADDR_MAPPED) {

>> -			tx_info->ip_family = AF_INET;

>> -			ret = chcr_ktls_act_open_req(sk, tx_info, atid);

>> -		} else {

>> -			tx_info->ip_family = AF_INET6;

>> -			ret = cxgb4_clip_get(tx_info->netdev,

>> -					     (const u32 *)

>> -					     &sk->sk_v6_rcv_saddr.s6_addr,

>> -					     1);

>> -			if (ret)

>> -				goto out;

>> -			ret = chcr_ktls_act_open_req6(sk, tx_info, atid);

>> -		}

>> +		ret = cxgb4_clip_get(tx_info->netdev, (const u32 *)

>> +				     &sk->sk_v6_rcv_saddr,

>> +				     1);

>> +		if (ret)

>> +			return ret;

>> +		ret = chcr_ktls_act_open_req6(sk, tx_info, atid);

> You removed the mapped socket handling which seems unrelated to the

> rest of the patch.


This mapped check is taken care in tls_dev_add, and this extra if

isn't needed anymore.

>> +	spin_lock(&tx_info->lock);

>> +	tx_info->conn_up = true;

>> +	spin_unlock(&tx_info->lock);

> What's the context this lock is taken in? You seem to always do only

> spin_lock(), does the control path not need to be _bh() or _irq()?

This conn_up isn't required anymore. I'll remove this.