Re: [PATCH v10 crypto 09/11] chtls: Inline TLS request Tx/Rx

From: Stefano Brivio <sbrivio@redhat.com>
To: Atul Gupta <atul.gupta@chelsio.com>
Cc: davejwatson@fb.com, davem@davemloft.net,
	herbert@gondor.apana.org.au, sd@queasysnail.net,
	linux-crypto@vger.kernel.org, netdev@vger.kernel.org,
	ganeshgr@chelsio.com
Subject: Re: [PATCH v10 crypto 09/11] chtls: Inline TLS request Tx/Rx
Date: Mon, 12 Mar 2018 14:28:26 +0100	[thread overview]
Message-ID: <20180312142826.5536fad8@epycfail> (raw)
In-Reply-To: <1520622616-6557-8-git-send-email-atul.gupta@chelsio.com>

On Sat, 10 Mar 2018 00:40:14 +0530
Atul Gupta <atul.gupta@chelsio.com> wrote:

> TLS handler for record transmit and receive.
> Create Inline TLS work request and post to FW.
> Create Inline TLS record CPLs for hardware
> 
> Signed-off-by: Atul Gupta <atul.gupta@chelsio.com>
> ---
>  drivers/crypto/chelsio/chtls/chtls_io.c | 1863 +++++++++++++++++++++++++++++++
>  1 file changed, 1863 insertions(+)
>  create mode 100644 drivers/crypto/chelsio/chtls/chtls_io.c
> 
> diff --git a/drivers/crypto/chelsio/chtls/chtls_io.c b/drivers/crypto/chelsio/chtls/chtls_io.c
> new file mode 100644
> index 0000000..f7f5826
> --- /dev/null
> +++ b/drivers/crypto/chelsio/chtls/chtls_io.c
> @@ -0,0 +1,1863 @@
> +/*
> + * Copyright (c) 2017 Chelsio Communications, Inc.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * Written by: Atul Gupta (atul.gupta@chelsio.com)
> + */
> +
> +#include <linux/module.h>
> +#include <linux/list.h>
> +#include <linux/workqueue.h>
> +#include <linux/skbuff.h>
> +#include <linux/timer.h>
> +#include <linux/notifier.h>
> +#include <linux/inetdevice.h>
> +#include <linux/ip.h>
> +#include <linux/tcp.h>
> +#include <linux/sched/signal.h>
> +#include <net/tcp.h>
> +#include <net/busy_poll.h>
> +#include <crypto/aes.h>
> +
> +#include "chtls.h"
> +#include "chtls_cm.h"
> +
> +static bool is_tls_hw(struct chtls_sock *csk)
> +{
> +	return csk->tlshws.ofld;
> +}

Do you actually need this function?

> +static bool is_tls_rx(struct chtls_sock *csk)
> +{
> +	return (csk->tlshws.rxkey >= 0);
> +}

You can drop the ().

> +
> +static bool is_tls_tx(struct chtls_sock *csk)
> +{
> +	return (csk->tlshws.txkey >= 0);
> +}

You can drop the ().

> +static bool is_tls_skb(struct chtls_sock *csk, const struct sk_buff *skb)
> +{
> +	return (is_tls_hw(csk) && skb_ulp_tls_skb_flags(skb));
> +}

You can drop the ().

> +static int key_size(void *sk)
> +{
> +	return 16; /* Key on DDR */
> +}

Do you actually need this function? Can't you turn that into a #define?

> +
> +#define ceil(x, y) \
> +	({ unsigned long __x = (x), __y = (y); (__x + __y - 1) / __y; })

This doesn't look like a ceiling function, could you perhaps name it
more descriptively?

> +static int data_sgl_len(const struct sk_buff *skb)
> +{
> +	unsigned int cnt;
> +
> +	cnt = skb_shinfo(skb)->nr_frags;
> +	return (sgl_len(cnt) * 8);

You can drop the ().

> +}
> +
> +static int nos_ivs(struct sock *sk, unsigned int size)
> +{
> +	struct chtls_sock *csk = rcu_dereference_sk_user_data(sk);
> +
> +	return ceil(size, csk->tlshws.mfs);
> +}
> +
> +#define TLS_WR_CPL_LEN \
> +	(sizeof(struct fw_tlstx_data_wr) + \
> +	sizeof(struct cpl_tx_tls_sfo))
> +
> +static int is_ivs_imm(struct sock *sk, const struct sk_buff *skb)

Shouldn't this be a bool?

> +{
> +	int ivs_size = nos_ivs(sk, skb->len) * CIPHER_BLOCK_SIZE;
> +	int hlen = TLS_WR_CPL_LEN + data_sgl_len(skb);
> +
> +	if ((hlen + key_size(sk) + ivs_size) <
> +	    MAX_IMM_OFLD_TX_DATA_WR_LEN) {
> +		ULP_SKB_CB(skb)->ulp.tls.iv = 1;
> +		return 1;
> +	}
> +	ULP_SKB_CB(skb)->ulp.tls.iv = 0;

Are these assignments intended? They don't seem to fit with the name of
the function.

> +	return 0;
> +}
> +
> +static int max_ivs_size(struct sock *sk, int size)
> +{
> +	return (nos_ivs(sk, size) * CIPHER_BLOCK_SIZE);
> +}

You can drop the ().

> +
> +static int ivs_size(struct sock *sk, const struct sk_buff *skb)
> +{
> +	return (is_ivs_imm(sk, skb) ? (nos_ivs(sk, skb->len) *
> +		 CIPHER_BLOCK_SIZE) : 0);
> +}

You can drop the ().

> [...]
>
> +static u64 tls_sequence_number(struct chtls_hws *hws)
> +{
> +	return (hws->tx_seq_no++);
> +}

You can drop the (), and I find the name of this function a bit misleading
as you are actually increasing the sequence number, not just returning
it.

> +
> +static int is_sg_request(const struct sk_buff *skb)
> +{
> +	return (skb->peeked ||
> +		(skb->len > MAX_IMM_ULPTX_WR_LEN));
> +}

You can drop the (). This should be a bool.

> +
> +/*
> + * Returns true if an sk_buff carries urgent data.
> + */
> +static int skb_urgent(struct sk_buff *skb)
> +{
> +	return (ULP_SKB_CB(skb)->flags & ULPCB_FLAG_URG) != 0;
> +}

This should be a bool, you can drop the (), and avoid that != 0
comparison.

-- 
Stefano