| Message ID | 20220309030916.2932316-1-haowenchao@huawei.com |
|---|---|
| Headers | show |
| Series | scsi:iscsi: handle iscsi_cls_conn device with sysfs | expand |
On 3/8/22 9:09 PM, Wenchao Hao wrote: > iscsi_alloc_conn(): alloc and initialize iscsi_cls_conn > iscsi_add_conn(): expose iscsi_cls_conn to userspace's via sysfs. > iscsi_remove_conn(): remove iscsi_cls_conn from sysfs > iscsi_free_conn(): free iscsi_cls_conn > > Signed-off-by: Wenchao Hao <haowenchao@huawei.com> > Signed-off-by: Wu Bo <wubo40@huawei.com> > --- > drivers/scsi/scsi_transport_iscsi.c | 107 ++++++++++++++++++++++++++++ > include/scsi/scsi_transport_iscsi.h | 5 ++ > 2 files changed, 112 insertions(+) > > diff --git a/drivers/scsi/scsi_transport_iscsi.c b/drivers/scsi/scsi_transport_iscsi.c > index 554b6f784223..8e97c6f88359 100644 > --- a/drivers/scsi/scsi_transport_iscsi.c > +++ b/drivers/scsi/scsi_transport_iscsi.c > @@ -2340,6 +2340,113 @@ void iscsi_free_session(struct iscsi_cls_session *session) > } > EXPORT_SYMBOL_GPL(iscsi_free_session); > > +/** > + * iscsi_alloc_conn - alloc 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. Is that with the upstream iscsi tools or your version? I don't think the comment is needed or is needed somewhere else. If this happens then they will have the same sysfs/device name so when we do the device_add it will spit an error about duplicate names. > + */ > +struct iscsi_cls_conn * > +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; > + > + conn = kzalloc(sizeof(*conn) + dd_size, GFP_KERNEL); > + if (!conn) > + return NULL; > + if (dd_size) > + conn->dd_data = &conn[1]; > + > + mutex_init(&conn->ep_mutex); > + INIT_LIST_HEAD(&conn->conn_list); > + INIT_WORK(&conn->cleanup_work, iscsi_cleanup_conn_work_fn); > + conn->transport = transport; > + conn->cid = cid; > + conn->state = ISCSI_CONN_DOWN; > + > + /* this is released in the dev's release function */ > + if (!get_device(&session->dev)) > + goto free_conn; > + > + dev_set_name(&conn->dev, "connection%d:%u", session->sid, cid); > + device_initialize(&conn->dev); > + 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_add(&conn->dev); > + if (err) { > + iscsi_cls_session_printk(KERN_ERR, session, > + "could not register connection's dev\n"); > + put_device(&session->dev); I would call iscsi_free_conn. instead of put_device. > + return err; > + } > + err = transport_register_device(&conn->dev); > + if (err) { > + iscsi_cls_session_printk(KERN_ERR, session, > + "could not register transport's dev\n"); > + device_del(&conn->dev); > + put_device(&session->dev); Is for the get_device(&session->dev) in iscsi_alloc_conn? If so you don't need to do it because when the last put is done on the conn->dev, it will call iscsi_conn_release which does the put on the session when it does "put_device(parent). Or did you mean to call put_device on the conn->dev? I would do device_el(&conn->dev) then do a goto free_conn at the bottom which does iscsi_free_conn. The place above should do the goto as well. > + 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_remove_conn - remove iscsi class connection from sysfs > + * @conn: iscsi cls connection > + * > + * this would remove iscsi_cls_conn from sysfs, and wait for previous > + * read/write of iscsi_cls_conn's attributes in sysfs finishing > + */ > +void iscsi_remove_conn(struct iscsi_cls_conn *conn) > +{ > + device_del(&conn->dev); This should have the guts of iscsi_destroy_conn which reverses what the iscsi_add_conn did: spin_lock_irqsave(&connlock, flags); list_del(&conn->conn_list); spin_unlock_irqrestore(&connlock, flags); transport_unregister_device(&conn->dev); device_del(&conn->dev). You can then kill iscsi_destroy_conn. > +} > +EXPORT_SYMBOL_GPL(iscsi_remove_conn); > + > +void iscsi_free_conn(struct iscsi_cls_conn *conn) > +{ > + put_device(&conn->dev); > +} > +EXPORT_SYMBOL_GPL(iscsi_free_conn); Sorry I made a mistake with this suggestion. Kill this and just have the user do iscsi_put_conn. > + > /** > * iscsi_create_conn - create iscsi class connection > * @session: iscsi cls session > diff --git a/include/scsi/scsi_transport_iscsi.h b/include/scsi/scsi_transport_iscsi.h > index c5d7810fd792..346f65bc3861 100644 > --- a/include/scsi/scsi_transport_iscsi.h > +++ b/include/scsi/scsi_transport_iscsi.h > @@ -441,6 +441,11 @@ 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 struct iscsi_cls_conn *iscsi_alloc_conn(struct iscsi_cls_session *sess, > + int dd_size, uint32_t cid); > +extern int iscsi_add_conn(struct iscsi_cls_conn *conn); > +extern void iscsi_remove_conn(struct iscsi_cls_conn *conn); > +extern void iscsi_free_conn(struct iscsi_cls_conn *conn); > 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);
On 3/8/22 9:09 PM, Wenchao Hao wrote: > @@ -3143,8 +3145,6 @@ void iscsi_conn_teardown(struct iscsi_cls_conn *cls_conn) > mutex_unlock(&session->eh_mutex); > > iscsi_destroy_conn(cls_conn); This should then be a iscsi_put_conn. So basically it balances out. iscsi_alloc_conn requires iscsi_put_conn. The put releases what is done in the alloc. The put on the parent is done in the release though. iscsi_add_conn requires iscsi_remove_conn. The remove undoes what was done in the add We don't want a iscsi_destroy_conn which does a iscsi_free_conn because it makes it confusing.
On 2022/3/9 1:19, Mike Christie wrote: > On 3/8/22 9:09 PM, Wenchao Hao wrote: >> iscsi_alloc_conn(): alloc and initialize iscsi_cls_conn >> iscsi_add_conn(): expose iscsi_cls_conn to userspace's via sysfs. >> iscsi_remove_conn(): remove iscsi_cls_conn from sysfs >> iscsi_free_conn(): free iscsi_cls_conn >> >> Signed-off-by: Wenchao Hao <haowenchao@huawei.com> >> Signed-off-by: Wu Bo <wubo40@huawei.com> >> --- >> drivers/scsi/scsi_transport_iscsi.c | 107 ++++++++++++++++++++++++++++ >> include/scsi/scsi_transport_iscsi.h | 5 ++ >> 2 files changed, 112 insertions(+) >> >> diff --git a/drivers/scsi/scsi_transport_iscsi.c b/drivers/scsi/scsi_transport_iscsi.c >> index 554b6f784223..8e97c6f88359 100644 >> --- a/drivers/scsi/scsi_transport_iscsi.c >> +++ b/drivers/scsi/scsi_transport_iscsi.c >> @@ -2340,6 +2340,113 @@ void iscsi_free_session(struct iscsi_cls_session *session) >> } >> EXPORT_SYMBOL_GPL(iscsi_free_session); >> >> +/** >> + * iscsi_alloc_conn - alloc 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. > > Is that with the upstream iscsi tools or your version? I don't think the comment > is needed or is needed somewhere else. > > If this happens then they will have the same sysfs/device name so when we do the > device_add it will spit an error about duplicate names. > > >> + */ >> +struct iscsi_cls_conn * >> +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; >> + >> + conn = kzalloc(sizeof(*conn) + dd_size, GFP_KERNEL); >> + if (!conn) >> + return NULL; >> + if (dd_size) >> + conn->dd_data = &conn[1]; >> + >> + mutex_init(&conn->ep_mutex); >> + INIT_LIST_HEAD(&conn->conn_list); >> + INIT_WORK(&conn->cleanup_work, iscsi_cleanup_conn_work_fn); >> + conn->transport = transport; >> + conn->cid = cid; >> + conn->state = ISCSI_CONN_DOWN; >> + >> + /* this is released in the dev's release function */ >> + if (!get_device(&session->dev)) >> + goto free_conn; >> + >> + dev_set_name(&conn->dev, "connection%d:%u", session->sid, cid); >> + device_initialize(&conn->dev); >> + 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_add(&conn->dev); >> + if (err) { >> + iscsi_cls_session_printk(KERN_ERR, session, >> + "could not register connection's dev\n"); >> + put_device(&session->dev); > > I would call iscsi_free_conn. instead of put_device. > Sorry I noticed it but forget to remove it. Here should not call put_device() or iscsi_free_conn(). If iscsi_add_conn() failed, we shoule not call any put operation which might cause resource free. >> + return err; >> + } >> + err = transport_register_device(&conn->dev); >> + if (err) { >> + iscsi_cls_session_printk(KERN_ERR, session, >> + "could not register transport's dev\n"); >> + device_del(&conn->dev); >> + put_device(&session->dev); > > > Is for the get_device(&session->dev) in iscsi_alloc_conn? If so you don't need to > do it because when the last put is done on the conn->dev, it will call > iscsi_conn_release which does the put on the session when it does "put_device(parent). > > Or did you mean to call put_device on the conn->dev? > As above, we shouldn't call put_device() here. > I would do device_el(&conn->dev) then do a goto free_conn at the bottom which > does iscsi_free_conn. The place above should do the goto as well. > >