Re: [PATCH v9 crypto 01/12] tls: tls_device struct to register TLS drivers

[Sabrina Dubroca <sd@queasysnail.net>]

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 <atul.gupta@chelsio.com>
> ---
>  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