| Message ID | a77739e256b044c2e125e106165efc751ce4a0f6.1520422535.git.arvind.yadav.cs@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
> Never directly free @dev after calling device_register(), even > if it returned an error! Always use put_device() to give up the > reference initialized. Lee, Chris: Please review!
On Wed, Mar 07, 2018 at 05:07:33PM +0530, Arvind Yadav wrote: > Never directly free @dev after calling device_register(), even > if it returned an error! Always use put_device() to give up the > reference initialized. > > Signed-off-by: Arvind Yadav <arvind.yadav.cs@gmail.com> > --- > drivers/scsi/scsi_transport_iscsi.c | 27 +++++++++++++-------------- > 1 file changed, 13 insertions(+), 14 deletions(-) > > @@ -783,7 +781,7 @@ struct iscsi_iface * > > free_iface: > put_device(iface->dev.parent); > - kfree(iface); > + put_device(&iface->dev); > return NULL; > } > EXPORT_SYMBOL_GPL(iscsi_create_iface); Am I reading the device code correctly that the parent reference is only released in the unregister path (device_unregister calls device_del) and so the manual put_device on the parent here is still correct? Just want to make sure that's still needed with the call to put_device. Other than that question, I this all looks good. Thanks. Signed-off-by: Chris Leech <cleech@redhat.com>
On Thursday 15 March 2018 11:14 PM, Chris Leech wrote: > On Wed, Mar 07, 2018 at 05:07:33PM +0530, Arvind Yadav wrote: >> Never directly free @dev after calling device_register(), even >> if it returned an error! Always use put_device() to give up the >> reference initialized. >> >> Signed-off-by: Arvind Yadav <arvind.yadav.cs@gmail.com> >> --- >> drivers/scsi/scsi_transport_iscsi.c | 27 +++++++++++++-------------- >> 1 file changed, 13 insertions(+), 14 deletions(-) >> >> @@ -783,7 +781,7 @@ struct iscsi_iface * >> >> free_iface: >> put_device(iface->dev.parent); >> - kfree(iface); >> + put_device(&iface->dev); >> return NULL; >> } >> EXPORT_SYMBOL_GPL(iscsi_create_iface); > Am I reading the device code correctly that the parent reference is only > released in the unregister path (device_unregister calls device_del) and > so the manual put_device on the parent here is still correct? > > Just want to make sure that's still needed with the call to put_device. The manual put_device on the parent is not correct. This should be removed for here. we should call put_device(&iface->dev) only and It'll released parent reference. > Other than that question, I this all looks good. > Thanks. > > Signed-off-by: Chris Leech <cleech@redhat.com> > ~arvind
diff --git a/drivers/scsi/scsi_transport_iscsi.c b/drivers/scsi/scsi_transport_iscsi.c index f4b52b4..aacb7ab 100644 --- a/drivers/scsi/scsi_transport_iscsi.c +++ b/drivers/scsi/scsi_transport_iscsi.c @@ -221,8 +221,10 @@ struct iscsi_endpoint * ep->dev.class = &iscsi_endpoint_class; dev_set_name(&ep->dev, "ep-%llu", (unsigned long long) id); err = device_register(&ep->dev); - if (err) - goto free_ep; + if (err) { + put_device(&ep->dev); + return NULL; + } err = sysfs_create_group(&ep->dev.kobj, &iscsi_endpoint_group); if (err) @@ -235,10 +237,6 @@ struct iscsi_endpoint * unregister_dev: device_unregister(&ep->dev); return NULL; - -free_ep: - kfree(ep); - return NULL; } EXPORT_SYMBOL_GPL(iscsi_create_endpoint); @@ -783,7 +781,7 @@ struct iscsi_iface * free_iface: put_device(iface->dev.parent); - kfree(iface); + put_device(&iface->dev); return NULL; } EXPORT_SYMBOL_GPL(iscsi_create_iface); @@ -1260,7 +1258,7 @@ struct iscsi_bus_flash_session * return fnode_sess; free_fnode_sess: - kfree(fnode_sess); + put_device(&fnode_sess->dev); return NULL; } EXPORT_SYMBOL_GPL(iscsi_create_flashnode_sess); @@ -1308,7 +1306,7 @@ struct iscsi_bus_flash_conn * return fnode_conn; free_fnode_conn: - kfree(fnode_conn); + put_device(&fnode_conn->dev); return NULL; } EXPORT_SYMBOL_GPL(iscsi_create_flashnode_conn); @@ -2268,6 +2266,8 @@ struct iscsi_cls_conn * release_parent_ref: put_device(&session->dev); + put_device(&conn->dev); + conn = NULL; free_conn: kfree(conn); return NULL; @@ -4420,8 +4420,10 @@ struct scsi_transport_template * priv->dev.class = &iscsi_transport_class; dev_set_name(&priv->dev, "%s", tt->name); err = device_register(&priv->dev); - if (err) - goto free_priv; + if (err) { + put_device(&priv->dev); + return NULL; + } err = sysfs_create_group(&priv->dev.kobj, &iscsi_transport_group); if (err) @@ -4456,9 +4458,6 @@ struct scsi_transport_template * unregister_dev: device_unregister(&priv->dev); return NULL; -free_priv: - kfree(priv); - return NULL; } EXPORT_SYMBOL_GPL(iscsi_register_transport);
Never directly free @dev after calling device_register(), even if it returned an error! Always use put_device() to give up the reference initialized. Signed-off-by: Arvind Yadav <arvind.yadav.cs@gmail.com> --- drivers/scsi/scsi_transport_iscsi.c | 27 +++++++++++++-------------- 1 file changed, 13 insertions(+), 14 deletions(-)