From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx3-rdu2.redhat.com ([66.187.233.73]:44440 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1751379AbeCLN2d (ORCPT ); Mon, 12 Mar 2018 09:28:33 -0400 Date: Mon, 12 Mar 2018 14:28:26 +0100 From: Stefano Brivio To: Atul Gupta 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 Message-ID: <20180312142826.5536fad8@epycfail> In-Reply-To: <1520622616-6557-8-git-send-email-atul.gupta@chelsio.com> References: <1520622616-6557-1-git-send-email-atul.gupta@chelsio.com> <1520622616-6557-8-git-send-email-atul.gupta@chelsio.com> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Sender: netdev-owner@vger.kernel.org List-ID: On Sat, 10 Mar 2018 00:40:14 +0530 Atul Gupta 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 > --- > 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 > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#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