From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx3-rdu2.redhat.com ([66.187.233.73]:48806 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1751087AbeCGKQa (ORCPT ); Wed, 7 Mar 2018 05:16:30 -0500 Date: Wed, 7 Mar 2018 11:16:25 +0100 From: Sabrina Dubroca To: Atul Gupta Cc: davejwatson@fb.com, davem@davemloft.net, herbert@gondor.apana.org.au, linux-crypto@vger.kernel.org, netdev@vger.kernel.org, ganeshgr@chelsio.com Subject: Re: [PATCH v9 crypto 01/12] tls: tls_device struct to register TLS drivers Message-ID: <20180307101625.GA20949@bistromath.localdomain> References: <1520350523-11743-1-git-send-email-atul.gupta@chelsio.com> <1520350580-11796-1-git-send-email-atul.gupta@chelsio.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <1520350580-11796-1-git-send-email-atul.gupta@chelsio.com> Sender: netdev-owner@vger.kernel.org List-ID: Hello Atul, One quick note before you start replying: please fix your email client, as you've been told before. Quoting has to add a quoting marker (the '>' character) at the beginning of the line, otherwise it's impossible to separate your reply from the email you're quoting. 2018-03-06, 21:06:20 +0530, Atul Gupta wrote: > tls_device structure to register Inline TLS drivers > with net/tls > > Signed-off-by: Atul Gupta > --- > include/net/tls.h | 26 ++++++++++++++++++++++++++ > 1 file changed, 26 insertions(+) > > diff --git a/include/net/tls.h b/include/net/tls.h > index 4913430..9bfb91f 100644 > --- a/include/net/tls.h > +++ b/include/net/tls.h > @@ -55,6 +55,28 @@ > #define TLS_RECORD_TYPE_DATA 0x17 > > #define TLS_AAD_SPACE_SIZE 13 > +#define TLS_DEVICE_NAME_MAX 32 Why 32 characters? > +enum { > + TLS_BASE_TX, > + TLS_SW_TX, > + TLS_FULL_HW, /* TLS record processed Inline */ > + TLS_NUM_CONFIG, > +}; > + > +extern struct proto tls_prots[TLS_NUM_CONFIG]; Don't break bisection. The code has to compile after every patch. These are already defined in net/tls/tls_main.c. > +struct tls_device { > + char name[TLS_DEVICE_NAME_MAX]; I could find only one use of it, in chtls_register_dev. Is this actually needed? > + struct list_head dev_list; > + > + /* netdev present in registered inline tls driver */ > + int (*netdev)(struct tls_device *device, > + struct net_device *netdev); I was trying to figure out what this did, because the name is really not explicit, and the comment doesn't make sense, but noticed it's never actually called. > + int (*feature)(struct tls_device *device); > + int (*hash)(struct tls_device *device, struct sock *sk); > + void (*unhash)(struct tls_device *device, struct sock *sk); I think you should add a kerneldoc comment, like all the ndo_* methods have. > +}; > > struct tls_sw_context { > struct crypto_aead *aead_send; > @@ -115,6 +137,8 @@ struct tls_context { > int (*getsockopt)(struct sock *sk, int level, > int optname, char __user *optval, > int __user *optlen); > + int (*hash)(struct sock *sk); > + void (*unhash)(struct sock *sk); > }; > > int wait_on_pending_writer(struct sock *sk, long *timeo); > @@ -256,5 +280,7 @@ static inline struct tls_offload_context *tls_offload_ctx( > > int tls_proccess_cmsg(struct sock *sk, struct msghdr *msg, > unsigned char *record_type); > +void tls_register_device(struct tls_device *device); > +void tls_unregister_device(struct tls_device *device); Prototype without implementation, please don't do that. This happens because you split your patchset so that each patch has all the changes for exactly one file. -- Sabrina