Re: [PATCH v10 crypto 07/11] chtls: Program the TLS Key

[Stefano Brivio <sbrivio@redhat.com>]

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

> Initialize the space reserved for storing the TLS keys
> get and free the location where key is stored for the TLS
> connection
> Program the tx and rx key as received from user in
> struct tls12_crypto_info_aes_gcm_128 and understood by hardware.
> 
> Signed-off-by: Atul Gupta <atul.gupta@chelsio.com>
> ---
>  drivers/crypto/chelsio/chtls/chtls_hw.c | 371 ++++++++++++++++++++++++++++++++
>  1 file changed, 371 insertions(+)
>  create mode 100644 drivers/crypto/chelsio/chtls/chtls_hw.c
> 
> diff --git a/drivers/crypto/chelsio/chtls/chtls_hw.c b/drivers/crypto/chelsio/chtls/chtls_hw.c
> new file mode 100644
> index 0000000..40cf84f
> --- /dev/null
> +++ b/drivers/crypto/chelsio/chtls/chtls_hw.c
> @@ -0,0 +1,371 @@
> +/*
> + * 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/tls.h>
> +#include <net/tls.h>
> +
> +#include "chtls.h"
> +#include "chtls_cm.h"
> +
> +static void __set_tcb_field_direct(struct chtls_sock *csk,
> +				   struct cpl_set_tcb_field *req, u16 word,
> +				   u64 mask, u64 val, u8 cookie, int no_reply)
> +{
> +	struct ulptx_idata *sc;
> +
> +	INIT_TP_WR_CPL(req, CPL_SET_TCB_FIELD, csk->tid);
> +	req->wr.wr_mid |= htonl(FW_WR_FLOWID_V(csk->tid));
> +	req->reply_ctrl = htons(NO_REPLY_V(no_reply) |
> +				QUEUENO_V(csk->rss_qid));
> +	req->word_cookie = htons(TCB_WORD_V(word) | TCB_COOKIE_V(cookie));
> +	req->mask = cpu_to_be64(mask);
> +	req->val = cpu_to_be64(val);
> +	sc = (struct ulptx_idata *)(req + 1);
> +	sc->cmd_more = htonl(ULPTX_CMD_V(ULP_TX_SC_NOOP));
> +	sc->len = htonl(0);
> +}
> +
> +static void __set_tcb_field(struct sock *sk, struct sk_buff *skb, u16 word,
> +			    u64 mask, u64 val, u8 cookie, int no_reply)
> +{
> +	struct chtls_sock *csk = rcu_dereference_sk_user_data(sk);
> +	struct cpl_set_tcb_field *req;
> +	struct ulptx_idata *sc;
> +	unsigned int wrlen = roundup(sizeof(*req) + sizeof(*sc), 16);
> +
> +	req = (struct cpl_set_tcb_field *)__skb_put(skb, wrlen);
> +	__set_tcb_field_direct(csk, req, word, mask, val, cookie, no_reply);
> +	set_wr_txq(skb, CPL_PRIORITY_CONTROL, csk->port_id);
> +}
> +
> +/*
> + * Send control message to HW, message go as immediate data and packet
> + * is freed immediately.
> + */
> +static void chtls_set_tcb_field(struct sock *sk, u16 word, u64 mask, u64 val)
> +{
> +	struct chtls_sock *csk = rcu_dereference_sk_user_data(sk);
> +	struct cpl_set_tcb_field *req;
> +	struct ulptx_idata *sc;
> +	struct sk_buff *skb;
> +	unsigned int wrlen = roundup(sizeof(*req) + sizeof(*sc), 16);
> +	unsigned int credits_needed = DIV_ROUND_UP(wrlen, 16);
> +
> +	skb = alloc_skb(wrlen, GFP_ATOMIC);
> +	if (!skb)
> +		return;
> +
> +	__set_tcb_field(sk, skb, word, mask, val, 0, 1);
> +	set_queue(skb, (csk->txq_idx << 1) | CPL_PRIORITY_DATA, sk);
> +	csk->wr_credits -= credits_needed;
> +	csk->wr_unacked += credits_needed;
> +	enqueue_wr(csk, skb);
> +	cxgb4_ofld_send(csk->egress_dev, skb);
> +}

What happens if e.g. alloc_skb() fails here? Is it fine to ignore the
error altogether?

> +
> +/*
> + * Set one of the t_flags bits in the TCB.
> + */
> +void chtls_set_tcb_tflag(struct sock *sk, unsigned int bit_pos, int val)
> +{
> +	return chtls_set_tcb_field(sk, 1, 1ULL << bit_pos,
> +			    val << bit_pos);
> +}
> +
> +static void chtls_set_tcb_keyid(struct sock *sk, int keyid)
> +{
> +	return chtls_set_tcb_field(sk, 31, 0xFFFFFFFFULL, keyid);
> +}
> +
> +static void chtls_set_tcb_seqno(struct sock *sk)
> +{
> +	return chtls_set_tcb_field(sk, 28, ~0ULL, 0);
> +}
> +
> +static void chtls_set_tcb_quiesce(struct sock *sk, int val)
> +{
> +	return chtls_set_tcb_field(sk, 1, (1ULL << TF_RX_QUIESCE_S),
> +				   TF_RX_QUIESCE_V(val));
> +}

Why would you return from a void function?

IMHO, it would be more productive if you could address the comments you
are receiving a bit more carefully instead of patching them up quickly
and race to re-post, because this doesn't seem to gain any time.

-- 
Stefano