From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from stargate.chelsio.com ([12.32.117.8]:44854 "EHLO stargate.chelsio.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751050AbeCGLWH (ORCPT ); Wed, 7 Mar 2018 06:22:07 -0500 Subject: Re: [PATCH v9 crypto 01/12] tls: tls_device struct to register TLS drivers To: Sabrina Dubroca Cc: davejwatson@fb.com, davem@davemloft.net, herbert@gondor.apana.org.au, linux-crypto@vger.kernel.org, netdev@vger.kernel.org, ganeshgr@chelsio.com References: <1520350523-11743-1-git-send-email-atul.gupta@chelsio.com> <1520350580-11796-1-git-send-email-atul.gupta@chelsio.com> <20180307101625.GA20949@bistromath.localdomain> From: Atul Gupta Message-ID: Date: Wed, 7 Mar 2018 16:51:51 +0530 MIME-Version: 1.0 In-Reply-To: <20180307101625.GA20949@bistromath.localdomain> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Content-Language: en-US Sender: netdev-owner@vger.kernel.org List-ID: On 3/7/2018 3:46 PM, Sabrina Dubroca wrote: > 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? This is considered to accommodate registering device name, should it be bigger? > > >> +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. Will take care > >> +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? help to identify Inline TLS drivers registered with net/tls > >> + 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. Was used Initially, removed in last few cleanup [thanks for pointing] > >> + 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. Will take care > >> +}; >> >> 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. will have declaration and definition together >