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

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:

quoted

tls_device structure to register Inline TLS drivers
with net/tls

Signed-off-by: Atul Gupta <redacted>
---
 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?

quoted

+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

quoted

+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

quoted

+	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]

quoted

+	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

quoted

+};
 
 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