From: Mike Christie <michaelc@cs.wisc.edu>
To: Zhao Hongjiang <zhaohongjiang@huawei.com>
Cc: JBottomley@parallels.com, ravi.anand@qlogic.com,
vikas.chaudhary@qlogic.com, linux-scsi@vger.kernel.org
Subject: Re: [PATCH] scsi: add put_device() after device_find_child()
Date: Mon, 06 May 2013 12:06:56 -0500 [thread overview]
Message-ID: <5187E330.1030806@cs.wisc.edu> (raw)
In-Reply-To: <51879A29.7070900@huawei.com>
[-- Attachment #1: Type: text/plain, Size: 1069 bytes --]
On 05/06/2013 06:55 AM, Zhao Hongjiang wrote:
> The qla4xxx_sysfs_ddb_add() function uses device_find_child() but it does not drop
> the reference of the retrieved child.
>
> Signed-off-by: Zhao Hongjiang <zhaohongjiang@huawei.com>
> ---
> drivers/scsi/qla4xxx/ql4_os.c | 1 +
> 1 files changed, 1 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/scsi/qla4xxx/ql4_os.c b/drivers/scsi/qla4xxx/ql4_os.c
> index a47f999..b378d9e 100644
> --- a/drivers/scsi/qla4xxx/ql4_os.c
> +++ b/drivers/scsi/qla4xxx/ql4_os.c
> @@ -5605,6 +5605,7 @@ static int qla4xxx_sysfs_ddb_add(struct Scsi_Host *shost, const char *buf,
> ql4_printk(KERN_ERR, ha,
> "%s: A non-persistent entry %s found\n",
> __func__, dev->kobj.name);
> + put_device(dev);
> goto exit_ddb_add;
> }
>
>
Thanks for catching this. I messed up when reviewing it. For some dumb
reason I thought it was not doing refcounting.
I think there are a couple bugs from other similar functions. What about
the attached patch? It is only compile tested. If it is ok, Vikas,
please test.
[-- Attachment #2: iscsi-fix-find-fn-use.patch --]
[-- Type: text/plain, Size: 9495 bytes --]
iscsi class, qla4xxx: fix sess/conn refcounting when find fns are used
This fixes a bug where the iscsi class/driver did not do a put_device
when a sess/conn device was found. This also simplifies the interface
by not having to pass in some arguments that were duplicated and did
not need to be exported.
Signed-off-by: Mike Christie <michaelc@cs.wisc.edu>
diff --git a/drivers/scsi/qla4xxx/ql4_os.c b/drivers/scsi/qla4xxx/ql4_os.c
index d777332..4d231c1 100644
--- a/drivers/scsi/qla4xxx/ql4_os.c
+++ b/drivers/scsi/qla4xxx/ql4_os.c
@@ -5605,6 +5605,7 @@ static int qla4xxx_sysfs_ddb_add(struct Scsi_Host *shost, const char *buf,
ql4_printk(KERN_ERR, ha,
"%s: A non-persistent entry %s found\n",
__func__, dev->kobj.name);
+ put_device(dev);
goto exit_ddb_add;
}
@@ -6112,8 +6113,7 @@ qla4xxx_sysfs_ddb_get_param(struct iscsi_bus_flash_session *fnode_sess,
int parent_type, parent_index = 0xffff;
int rc = 0;
- dev = iscsi_find_flashnode_conn(fnode_sess, NULL,
- iscsi_is_flashnode_conn_dev);
+ dev = iscsi_find_flashnode_conn(fnode_sess);
if (!dev)
return -EIO;
@@ -6347,6 +6347,8 @@ qla4xxx_sysfs_ddb_get_param(struct iscsi_bus_flash_session *fnode_sess,
rc = -ENOSYS;
break;
}
+
+ put_device(dev);
return rc;
}
diff --git a/drivers/scsi/scsi_transport_iscsi.c b/drivers/scsi/scsi_transport_iscsi.c
index 475265a..133926b 100644
--- a/drivers/scsi/scsi_transport_iscsi.c
+++ b/drivers/scsi/scsi_transport_iscsi.c
@@ -1019,8 +1019,7 @@ exit_match_index:
/**
* iscsi_get_flashnode_by_index -finds flashnode session entry by index
* @shost: pointer to host data
- * @data: pointer to data containing value to use for comparison
- * @fn: function pointer that does actual comparison
+ * @idx: index to match
*
* Finds the flashnode session object for the passed index
*
@@ -1029,13 +1028,13 @@ exit_match_index:
* %NULL on failure
*/
static struct iscsi_bus_flash_session *
-iscsi_get_flashnode_by_index(struct Scsi_Host *shost, void *data,
- int (*fn)(struct device *dev, void *data))
+iscsi_get_flashnode_by_index(struct Scsi_Host *shost, uint32_t idx)
{
struct iscsi_bus_flash_session *fnode_sess = NULL;
struct device *dev;
- dev = device_find_child(&shost->shost_gendev, data, fn);
+ dev = device_find_child(&shost->shost_gendev, &idx,
+ flashnode_match_index);
if (dev)
fnode_sess = iscsi_dev_to_flash_session(dev);
@@ -1059,18 +1058,13 @@ struct device *
iscsi_find_flashnode_sess(struct Scsi_Host *shost, void *data,
int (*fn)(struct device *dev, void *data))
{
- struct device *dev;
-
- dev = device_find_child(&shost->shost_gendev, data, fn);
- return dev;
+ return device_find_child(&shost->shost_gendev, data, fn);
}
EXPORT_SYMBOL_GPL(iscsi_find_flashnode_sess);
/**
* iscsi_find_flashnode_conn - finds flashnode connection entry
* @fnode_sess: pointer to parent flashnode session entry
- * @data: pointer to data containing value to use for comparison
- * @fn: function pointer that does actual comparison
*
* Finds the flashnode connection object comparing the data passed using logic
* defined in passed function pointer
@@ -1080,14 +1074,10 @@ EXPORT_SYMBOL_GPL(iscsi_find_flashnode_sess);
* %NULL on failure
*/
struct device *
-iscsi_find_flashnode_conn(struct iscsi_bus_flash_session *fnode_sess,
- void *data,
- int (*fn)(struct device *dev, void *data))
+iscsi_find_flashnode_conn(struct iscsi_bus_flash_session *fnode_sess)
{
- struct device *dev;
-
- dev = device_find_child(&fnode_sess->dev, data, fn);
- return dev;
+ return device_find_child(&fnode_sess->dev, NULL,
+ iscsi_is_flashnode_conn_dev);
}
EXPORT_SYMBOL_GPL(iscsi_find_flashnode_conn);
@@ -2808,7 +2798,7 @@ static int iscsi_set_flashnode_param(struct iscsi_transport *transport,
struct iscsi_bus_flash_session *fnode_sess;
struct iscsi_bus_flash_conn *fnode_conn;
struct device *dev;
- uint32_t *idx;
+ uint32_t idx;
int err = 0;
if (!transport->set_flashnode_param) {
@@ -2824,25 +2814,27 @@ static int iscsi_set_flashnode_param(struct iscsi_transport *transport,
goto put_host;
}
- idx = &ev->u.set_flashnode.flashnode_idx;
- fnode_sess = iscsi_get_flashnode_by_index(shost, idx,
- flashnode_match_index);
+ idx = ev->u.set_flashnode.flashnode_idx;
+ fnode_sess = iscsi_get_flashnode_by_index(shost, idx);
if (!fnode_sess) {
pr_err("%s could not find flashnode %u for host no %u\n",
- __func__, *idx, ev->u.set_flashnode.host_no);
+ __func__, idx, ev->u.set_flashnode.host_no);
err = -ENODEV;
goto put_host;
}
- dev = iscsi_find_flashnode_conn(fnode_sess, NULL,
- iscsi_is_flashnode_conn_dev);
+ dev = iscsi_find_flashnode_conn(fnode_sess);
if (!dev) {
err = -ENODEV;
- goto put_host;
+ goto put_sess;
}
fnode_conn = iscsi_dev_to_flash_conn(dev);
err = transport->set_flashnode_param(fnode_sess, fnode_conn, data, len);
+ put_device(dev);
+
+put_sess:
+ put_device(&fnode_sess->dev);
put_host:
scsi_host_put(shost);
@@ -2891,7 +2883,7 @@ static int iscsi_del_flashnode(struct iscsi_transport *transport,
{
struct Scsi_Host *shost;
struct iscsi_bus_flash_session *fnode_sess;
- uint32_t *idx;
+ uint32_t idx;
int err = 0;
if (!transport->del_flashnode) {
@@ -2907,17 +2899,17 @@ static int iscsi_del_flashnode(struct iscsi_transport *transport,
goto put_host;
}
- idx = &ev->u.del_flashnode.flashnode_idx;
- fnode_sess = iscsi_get_flashnode_by_index(shost, idx,
- flashnode_match_index);
+ idx = ev->u.del_flashnode.flashnode_idx;
+ fnode_sess = iscsi_get_flashnode_by_index(shost, idx);
if (!fnode_sess) {
pr_err("%s could not find flashnode %u for host no %u\n",
- __func__, *idx, ev->u.del_flashnode.host_no);
+ __func__, idx, ev->u.del_flashnode.host_no);
err = -ENODEV;
goto put_host;
}
err = transport->del_flashnode(fnode_sess);
+ put_device(&fnode_sess->dev);
put_host:
scsi_host_put(shost);
@@ -2933,7 +2925,7 @@ static int iscsi_login_flashnode(struct iscsi_transport *transport,
struct iscsi_bus_flash_session *fnode_sess;
struct iscsi_bus_flash_conn *fnode_conn;
struct device *dev;
- uint32_t *idx;
+ uint32_t idx;
int err = 0;
if (!transport->login_flashnode) {
@@ -2949,25 +2941,27 @@ static int iscsi_login_flashnode(struct iscsi_transport *transport,
goto put_host;
}
- idx = &ev->u.login_flashnode.flashnode_idx;
- fnode_sess = iscsi_get_flashnode_by_index(shost, idx,
- flashnode_match_index);
+ idx = ev->u.login_flashnode.flashnode_idx;
+ fnode_sess = iscsi_get_flashnode_by_index(shost, idx);
if (!fnode_sess) {
pr_err("%s could not find flashnode %u for host no %u\n",
- __func__, *idx, ev->u.login_flashnode.host_no);
+ __func__, idx, ev->u.login_flashnode.host_no);
err = -ENODEV;
goto put_host;
}
- dev = iscsi_find_flashnode_conn(fnode_sess, NULL,
- iscsi_is_flashnode_conn_dev);
+ dev = iscsi_find_flashnode_conn(fnode_sess);
if (!dev) {
err = -ENODEV;
- goto put_host;
+ goto put_sess;
}
fnode_conn = iscsi_dev_to_flash_conn(dev);
err = transport->login_flashnode(fnode_sess, fnode_conn);
+ put_device(dev);
+
+put_sess:
+ put_device(&fnode_sess->dev);
put_host:
scsi_host_put(shost);
@@ -2983,7 +2977,7 @@ static int iscsi_logout_flashnode(struct iscsi_transport *transport,
struct iscsi_bus_flash_session *fnode_sess;
struct iscsi_bus_flash_conn *fnode_conn;
struct device *dev;
- uint32_t *idx;
+ uint32_t idx;
int err = 0;
if (!transport->logout_flashnode) {
@@ -2999,26 +2993,28 @@ static int iscsi_logout_flashnode(struct iscsi_transport *transport,
goto put_host;
}
- idx = &ev->u.logout_flashnode.flashnode_idx;
- fnode_sess = iscsi_get_flashnode_by_index(shost, idx,
- flashnode_match_index);
+ idx = ev->u.logout_flashnode.flashnode_idx;
+ fnode_sess = iscsi_get_flashnode_by_index(shost, idx);
if (!fnode_sess) {
pr_err("%s could not find flashnode %u for host no %u\n",
- __func__, *idx, ev->u.logout_flashnode.host_no);
+ __func__, idx, ev->u.logout_flashnode.host_no);
err = -ENODEV;
goto put_host;
}
- dev = iscsi_find_flashnode_conn(fnode_sess, NULL,
- iscsi_is_flashnode_conn_dev);
+ dev = iscsi_find_flashnode_conn(fnode_sess);
if (!dev) {
err = -ENODEV;
- goto put_host;
+ goto put_sess;
}
fnode_conn = iscsi_dev_to_flash_conn(dev);
err = transport->logout_flashnode(fnode_sess, fnode_conn);
+ put_device(dev);
+
+put_sess:
+ put_device(&fnode_sess->dev);
put_host:
scsi_host_put(shost);
diff --git a/include/scsi/scsi_transport_iscsi.h b/include/scsi/scsi_transport_iscsi.h
index 4a58cca..d0f1602 100644
--- a/include/scsi/scsi_transport_iscsi.h
+++ b/include/scsi/scsi_transport_iscsi.h
@@ -471,14 +471,10 @@ iscsi_destroy_flashnode_sess(struct iscsi_bus_flash_session *fnode_sess);
extern void iscsi_destroy_all_flashnode(struct Scsi_Host *shost);
extern int iscsi_flashnode_bus_match(struct device *dev,
struct device_driver *drv);
-extern int iscsi_is_flashnode_conn_dev(struct device *dev, void *data);
-
extern struct device *
iscsi_find_flashnode_sess(struct Scsi_Host *shost, void *data,
int (*fn)(struct device *dev, void *data));
-
extern struct device *
-iscsi_find_flashnode_conn(struct iscsi_bus_flash_session *fnode_sess,
- void *data,
- int (*fn)(struct device *dev, void *data));
+iscsi_find_flashnode_conn(struct iscsi_bus_flash_session *fnode_sess);
+
#endif
next prev parent reply other threads:[~2013-05-06 17:07 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-05-06 11:55 [PATCH] scsi: add put_device() after device_find_child() Zhao Hongjiang
2013-05-06 17:06 ` Mike Christie [this message]
2013-05-08 12:46 ` Vikas Chaudhary
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=5187E330.1030806@cs.wisc.edu \
--to=michaelc@cs.wisc.edu \
--cc=JBottomley@parallels.com \
--cc=linux-scsi@vger.kernel.org \
--cc=ravi.anand@qlogic.com \
--cc=vikas.chaudhary@qlogic.com \
--cc=zhaohongjiang@huawei.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox