diff mbox series

[1/2] scsi: iscsi: Add helper functions to alloc and add iscsi_cls_conn

Message ID 20220308005654.2281343-2-haowenchao@huawei.com (mailing list archive)
State Superseded
Headers show
Series scsi:libiscsi: Add iscsi_cls_conn device to sysfs correctly | expand

Commit Message

Wenchao Hao March 8, 2022, 12:56 a.m. UTC 
iscsi_alloc_conn() would alloc and initialize iscsi_cls_conn but do
not expose it to userspace.
iscsi_add_conn() would expose it to userspace.

LLDs should split the alloc and register to 2 steps.

And simplify iscsi_create_conn() with these helper functions.

Signed-off-by: Wenchao Hao <haowenchao@huawei.com>
Signed-off-by: Wu Bo <wubo40@huawei.com>
---
 drivers/scsi/scsi_transport_iscsi.c | 85 +++++++++++++++++++++++------
 include/scsi/scsi_transport_iscsi.h |  3 +
 2 files changed, 72 insertions(+), 16 deletions(-)

Comments

Mike Christie March 7, 2022, 4:05 p.m. UTC | #1 
On 3/7/22 6:56 PM, Wenchao Hao wrote:
> iscsi_alloc_conn() would alloc and initialize iscsi_cls_conn but do
> not expose it to userspace.
> iscsi_add_conn() would expose it to userspace.
> 
> LLDs should split the alloc and register to 2 steps.
> 
> And simplify iscsi_create_conn() with these helper functions.
> 
> Signed-off-by: Wenchao Hao <haowenchao@huawei.com>
> Signed-off-by: Wu Bo <wubo40@huawei.com>
> ---
>  drivers/scsi/scsi_transport_iscsi.c | 85 +++++++++++++++++++++++------
>  include/scsi/scsi_transport_iscsi.h |  3 +
>  2 files changed, 72 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/scsi/scsi_transport_iscsi.c b/drivers/scsi/scsi_transport_iscsi.c
> index 554b6f784223..092d4429bb1d 100644
> --- a/drivers/scsi/scsi_transport_iscsi.c
> +++ b/drivers/scsi/scsi_transport_iscsi.c
> @@ -2341,7 +2341,7 @@ void iscsi_free_session(struct iscsi_cls_session *session)
>  EXPORT_SYMBOL_GPL(iscsi_free_session);
>  
>  /**
> - * iscsi_create_conn - create iscsi class connection
> + * iscsi_alloc_conn - alloc iscsi class connection
>   * @session: iscsi cls session
>   * @dd_size: private driver data size
>   * @cid: connection id
> @@ -2356,12 +2356,10 @@ EXPORT_SYMBOL_GPL(iscsi_free_session);
>   * non-zero.
>   */
>  struct iscsi_cls_conn *
> -iscsi_create_conn(struct iscsi_cls_session *session, int dd_size, uint32_t cid)
> +iscsi_alloc_conn(struct iscsi_cls_session *session, int dd_size, uint32_t cid)
>  {
>  	struct iscsi_transport *transport = session->transport;
>  	struct iscsi_cls_conn *conn;
> -	unsigned long flags;
> -	int err;
>  
>  	conn = kzalloc(sizeof(*conn) + dd_size, GFP_KERNEL);
>  	if (!conn)
> @@ -2383,35 +2381,90 @@ iscsi_create_conn(struct iscsi_cls_session *session, int dd_size, uint32_t cid)
>  	dev_set_name(&conn->dev, "connection%d:%u", session->sid, cid);
>  	conn->dev.parent = &session->dev;
>  	conn->dev.release = iscsi_conn_release;
> +
> +	return conn;
> +
> +free_conn:
> +	kfree(conn);
> +	return NULL;
> +}
> +EXPORT_SYMBOL_GPL(iscsi_alloc_conn);
> +
> +/**
> + * iscsi_add_conn - add iscsi class connection
> + * @conn: iscsi cls connection
> + *
> + * this would expose iscsi_cls_conn to sysfs, so make sure the related
> + * resources when access sysfs attributes are initialized before calling this.
> + */
> +int iscsi_add_conn(struct iscsi_cls_conn *conn)
> +{
> +	int err;
> +	unsigned long flags;
> +	struct iscsi_cls_session *session = iscsi_dev_to_session(conn->dev.parent);
> +
>  	err = device_register(&conn->dev);

You should use device_initialize in iscsi_alloc_conn. Here use device_add.

Then make a iscsi_remove_conn function which does device_del. The iscsi_free_conn
does a device_put.

Patch2 should not be doing kfree on the cls_conn because it has no idea what
the iscsi class needs to do to unravel things. For example, your patch leaks the
parent on failures because it doesn't do a put on the parent/session to handle
the get that's done in iscsi_alloc_conn.

In a patch3 you can then fix up iscsi_conn_teardown to remove the tmp_* kfree
code. So at the beginning of iscsi_conn_teardown do the iscsi_remove_conn.
Then after we have cleaned everything up in there do the iscsi_free_conn.

We then don't need the tmp_* code. We can just do:

       kfree(conn->persistent_address);
       kfree(conn->local_ipaddr);

anytime after the iscsi_remove_conn call.


>  	if (err) {
>  		iscsi_cls_session_printk(KERN_ERR, session, "could not "
>  					 "register connection's dev\n");
> -		goto release_parent_ref;
> +		put_device(&session->dev);
> +		return err;
>  	}
>  	err = transport_register_device(&conn->dev);
>  	if (err) {
>  		iscsi_cls_session_printk(KERN_ERR, session, "could not "
>  					 "register transport's dev\n");
> -		goto release_conn_ref;
> +		device_unregister(&conn->dev);
> +		put_device(&session->dev);
> +		return err;
>  	}
>  
>  	spin_lock_irqsave(&connlock, flags);
>  	list_add(&conn->conn_list, &connlist);
>  	spin_unlock_irqrestore(&connlock, flags);
>  
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(iscsi_add_conn);
> +
> +/**
> + * iscsi_create_conn - create iscsi class connection
> + * @session: iscsi cls session
> + * @dd_size: private driver data size
> + * @cid: connection id
> + *
> + * This can be called from a LLD or iscsi_transport. The connection
> + * is child of the session so cid must be unique for all connections
> + * on the session.
> + *
> + * Since we do not support MCS, cid will normally be zero. In some cases
> + * for software iscsi we could be trying to preallocate a connection struct
> + * in which case there could be two connection structs and cid would be
> + * non-zero.
> + *
> + * Note: iscsi_cls_conn would be exposed to sysfs after this function, it
> + * means attributes of iscsi_cls_conn are accessible to userspace. So the
> + * caller must make sure everything related these sysfs attributes are
> + * already initialized.
> + */
> +struct iscsi_cls_conn *
> +iscsi_create_conn(struct iscsi_cls_session *session, int dd_size, uint32_t cid)


You should kill this since it's not used anymore.

> +{
> +	struct iscsi_cls_conn *conn;
> +	int err;
> +
> +	conn = iscsi_alloc_conn(session, dd_size, cid);
> +	if (!conn)
> +		return NULL;
> +
> +	err = iscsi_add_conn(conn);
> +	if (err) {
> +		kfree(conn);
> +		return NULL;
> +	}
> +
>  	ISCSI_DBG_TRANS_CONN(conn, "Completed conn creation\n");
>  	return conn;
> -
> -release_conn_ref:
> -	device_unregister(&conn->dev);
> -	put_device(&session->dev);
> -	return NULL;
> -release_parent_ref:
> -	put_device(&session->dev);
> -free_conn:
> -	kfree(conn);
> -	return NULL;
>  }
>  
>  EXPORT_SYMBOL_GPL(iscsi_create_conn);
> diff --git a/include/scsi/scsi_transport_iscsi.h b/include/scsi/scsi_transport_iscsi.h
> index c5d7810fd792..fd9ce99c2186 100644
> --- a/include/scsi/scsi_transport_iscsi.h
> +++ b/include/scsi/scsi_transport_iscsi.h
> @@ -441,6 +441,9 @@ extern struct iscsi_cls_session *iscsi_create_session(struct Scsi_Host *shost,
>  						unsigned int target_id);
>  extern void iscsi_remove_session(struct iscsi_cls_session *session);
>  extern void iscsi_free_session(struct iscsi_cls_session *session);
> +extern int iscsi_add_conn(struct iscsi_cls_conn *conn);
> +extern struct iscsi_cls_conn *iscsi_alloc_conn(struct iscsi_cls_session *sess,
> +						int dd_size, uint32_t cid);
>  extern struct iscsi_cls_conn *iscsi_create_conn(struct iscsi_cls_session *sess,
>  						int dd_size, uint32_t cid);
>  extern void iscsi_put_conn(struct iscsi_cls_conn *conn);
diff mbox series

Patch

diff --git a/drivers/scsi/scsi_transport_iscsi.c b/drivers/scsi/scsi_transport_iscsi.c
index 554b6f784223..092d4429bb1d 100644
--- a/drivers/scsi/scsi_transport_iscsi.c
+++ b/drivers/scsi/scsi_transport_iscsi.c
@@ -2341,7 +2341,7 @@  void iscsi_free_session(struct iscsi_cls_session *session)
 EXPORT_SYMBOL_GPL(iscsi_free_session);
 
 /**
- * iscsi_create_conn - create iscsi class connection
+ * iscsi_alloc_conn - alloc iscsi class connection
  * @session: iscsi cls session
  * @dd_size: private driver data size
  * @cid: connection id
@@ -2356,12 +2356,10 @@  EXPORT_SYMBOL_GPL(iscsi_free_session);
  * non-zero.
  */
 struct iscsi_cls_conn *
-iscsi_create_conn(struct iscsi_cls_session *session, int dd_size, uint32_t cid)
+iscsi_alloc_conn(struct iscsi_cls_session *session, int dd_size, uint32_t cid)
 {
 	struct iscsi_transport *transport = session->transport;
 	struct iscsi_cls_conn *conn;
-	unsigned long flags;
-	int err;
 
 	conn = kzalloc(sizeof(*conn) + dd_size, GFP_KERNEL);
 	if (!conn)
@@ -2383,35 +2381,90 @@  iscsi_create_conn(struct iscsi_cls_session *session, int dd_size, uint32_t cid)
 	dev_set_name(&conn->dev, "connection%d:%u", session->sid, cid);
 	conn->dev.parent = &session->dev;
 	conn->dev.release = iscsi_conn_release;
+
+	return conn;
+
+free_conn:
+	kfree(conn);
+	return NULL;
+}
+EXPORT_SYMBOL_GPL(iscsi_alloc_conn);
+
+/**
+ * iscsi_add_conn - add iscsi class connection
+ * @conn: iscsi cls connection
+ *
+ * this would expose iscsi_cls_conn to sysfs, so make sure the related
+ * resources when access sysfs attributes are initialized before calling this.
+ */
+int iscsi_add_conn(struct iscsi_cls_conn *conn)
+{
+	int err;
+	unsigned long flags;
+	struct iscsi_cls_session *session = iscsi_dev_to_session(conn->dev.parent);
+
 	err = device_register(&conn->dev);
 	if (err) {
 		iscsi_cls_session_printk(KERN_ERR, session, "could not "
 					 "register connection's dev\n");
-		goto release_parent_ref;
+		put_device(&session->dev);
+		return err;
 	}
 	err = transport_register_device(&conn->dev);
 	if (err) {
 		iscsi_cls_session_printk(KERN_ERR, session, "could not "
 					 "register transport's dev\n");
-		goto release_conn_ref;
+		device_unregister(&conn->dev);
+		put_device(&session->dev);
+		return err;
 	}
 
 	spin_lock_irqsave(&connlock, flags);
 	list_add(&conn->conn_list, &connlist);
 	spin_unlock_irqrestore(&connlock, flags);
 
+	return 0;
+}
+EXPORT_SYMBOL_GPL(iscsi_add_conn);
+
+/**
+ * iscsi_create_conn - create iscsi class connection
+ * @session: iscsi cls session
+ * @dd_size: private driver data size
+ * @cid: connection id
+ *
+ * This can be called from a LLD or iscsi_transport. The connection
+ * is child of the session so cid must be unique for all connections
+ * on the session.
+ *
+ * Since we do not support MCS, cid will normally be zero. In some cases
+ * for software iscsi we could be trying to preallocate a connection struct
+ * in which case there could be two connection structs and cid would be
+ * non-zero.
+ *
+ * Note: iscsi_cls_conn would be exposed to sysfs after this function, it
+ * means attributes of iscsi_cls_conn are accessible to userspace. So the
+ * caller must make sure everything related these sysfs attributes are
+ * already initialized.
+ */
+struct iscsi_cls_conn *
+iscsi_create_conn(struct iscsi_cls_session *session, int dd_size, uint32_t cid)
+{
+	struct iscsi_cls_conn *conn;
+	int err;
+
+	conn = iscsi_alloc_conn(session, dd_size, cid);
+	if (!conn)
+		return NULL;
+
+	err = iscsi_add_conn(conn);
+	if (err) {
+		kfree(conn);
+		return NULL;
+	}
+
 	ISCSI_DBG_TRANS_CONN(conn, "Completed conn creation\n");
 	return conn;
-
-release_conn_ref:
-	device_unregister(&conn->dev);
-	put_device(&session->dev);
-	return NULL;
-release_parent_ref:
-	put_device(&session->dev);
-free_conn:
-	kfree(conn);
-	return NULL;
 }
 
 EXPORT_SYMBOL_GPL(iscsi_create_conn);
diff --git a/include/scsi/scsi_transport_iscsi.h b/include/scsi/scsi_transport_iscsi.h
index c5d7810fd792..fd9ce99c2186 100644
--- a/include/scsi/scsi_transport_iscsi.h
+++ b/include/scsi/scsi_transport_iscsi.h
@@ -441,6 +441,9 @@  extern struct iscsi_cls_session *iscsi_create_session(struct Scsi_Host *shost,
 						unsigned int target_id);
 extern void iscsi_remove_session(struct iscsi_cls_session *session);
 extern void iscsi_free_session(struct iscsi_cls_session *session);
+extern int iscsi_add_conn(struct iscsi_cls_conn *conn);
+extern struct iscsi_cls_conn *iscsi_alloc_conn(struct iscsi_cls_session *sess,
+						int dd_size, uint32_t cid);
 extern struct iscsi_cls_conn *iscsi_create_conn(struct iscsi_cls_session *sess,
 						int dd_size, uint32_t cid);
 extern void iscsi_put_conn(struct iscsi_cls_conn *conn);