From 37e6ba00720c2786330dec2a9a5081e9e049422f Mon Sep 17 00:00:00 2001 From: James Bottomley Date: Fri, 2 Oct 2009 13:30:08 -0500 Subject: [PATCH 1/6] [SCSI] fix memory leak in initialization The root cause of the problem is the fact that dev_set_name() now allocates storage instead of using the original array within the kobj. That means that the SCSI assumption that if you haven't made the containing object or any sub objects visible, you can just destroy it (and its component devices) lock stock and barrel becomes false. Fix this by doing the get of sdev_dev at parent time and thus do an extra put of it in scsi_destroy_sdev() (and all other destruction without add paths). Reported-by: Tetsuo Handa Signed-off-by: James Bottomley --- drivers/scsi/scsi_scan.c | 2 ++ drivers/scsi/scsi_sysfs.c | 7 ++----- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c index c44783801402..0547a7f44d42 100644 --- a/drivers/scsi/scsi_scan.c +++ b/drivers/scsi/scsi_scan.c @@ -317,6 +317,7 @@ static struct scsi_device *scsi_alloc_sdev(struct scsi_target *starget, out_device_destroy: scsi_device_set_state(sdev, SDEV_DEL); transport_destroy_device(&sdev->sdev_gendev); + put_device(&sdev->sdev_dev); put_device(&sdev->sdev_gendev); out: if (display_failure_msg) @@ -957,6 +958,7 @@ static inline void scsi_destroy_sdev(struct scsi_device *sdev) if (sdev->host->hostt->slave_destroy) sdev->host->hostt->slave_destroy(sdev); transport_destroy_device(&sdev->sdev_gendev); + put_device(&sdev->sdev_dev); put_device(&sdev->sdev_gendev); } diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c index fde54537d715..5c7eb63a19d1 100644 --- a/drivers/scsi/scsi_sysfs.c +++ b/drivers/scsi/scsi_sysfs.c @@ -864,10 +864,6 @@ int scsi_sysfs_add_sdev(struct scsi_device *sdev) goto clean_device; } - /* take a reference for the sdev_dev; this is - * released by the sdev_class .release */ - get_device(&sdev->sdev_gendev); - /* create queue files, which may be writable, depending on the host */ if (sdev->host->hostt->change_queue_depth) error = device_create_file(&sdev->sdev_gendev, &sdev_attr_queue_depth_rw); @@ -917,6 +913,7 @@ int scsi_sysfs_add_sdev(struct scsi_device *sdev) device_del(&sdev->sdev_gendev); transport_destroy_device(&sdev->sdev_gendev); + put_device(&sdev->sdev_dev); put_device(&sdev->sdev_gendev); return error; @@ -1065,7 +1062,7 @@ void scsi_sysfs_device_initialize(struct scsi_device *sdev) sdev->host->host_no, sdev->channel, sdev->id, sdev->lun); device_initialize(&sdev->sdev_dev); - sdev->sdev_dev.parent = &sdev->sdev_gendev; + sdev->sdev_dev.parent = get_device(&sdev->sdev_gendev); sdev->sdev_dev.class = &sdev_class; dev_set_name(&sdev->sdev_dev, "%d:%d:%d:%d", sdev->host->host_no, sdev->channel, sdev->id, sdev->lun); From d10c0858f618c20547d4eda8aee9c3afd91599cf Mon Sep 17 00:00:00 2001 From: Heiko Carstens Date: Tue, 13 Oct 2009 10:44:07 +0200 Subject: [PATCH 2/6] [SCSI] zfcp: fix kfree handling in zfcp_init_device_setup The pointer that is allocated with kmalloc() is passed to strsep() which modifies it. Later on the modified pointer value will be passed to kfree. Save the original pointer and pass that one to kfree instead. Signed-off-by: Heiko Carstens Signed-off-by: Christof Schmitt Signed-off-by: James Bottomley --- drivers/s390/scsi/zfcp_aux.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/drivers/s390/scsi/zfcp_aux.c b/drivers/s390/scsi/zfcp_aux.c index 0f79f3af4f54..2889e5f2dfd3 100644 --- a/drivers/s390/scsi/zfcp_aux.c +++ b/drivers/s390/scsi/zfcp_aux.c @@ -128,12 +128,13 @@ out_ccwdev: static void __init zfcp_init_device_setup(char *devstr) { char *token; - char *str; + char *str, *str_saved; char busid[ZFCP_BUS_ID_SIZE]; u64 wwpn, lun; /* duplicate devstr and keep the original for sysfs presentation*/ - str = kmalloc(strlen(devstr) + 1, GFP_KERNEL); + str_saved = kmalloc(strlen(devstr) + 1, GFP_KERNEL); + str = str_saved; if (!str) return; @@ -152,12 +153,12 @@ static void __init zfcp_init_device_setup(char *devstr) if (!token || strict_strtoull(token, 0, (unsigned long long *) &lun)) goto err_out; - kfree(str); + kfree(str_saved); zfcp_init_device_configure(busid, wwpn, lun); return; - err_out: - kfree(str); +err_out: + kfree(str_saved); pr_err("%s is not a valid SCSI device\n", devstr); } From 934aeb587bab3173b6dec8e7717b909d8efc77b0 Mon Sep 17 00:00:00 2001 From: Christof Schmitt Date: Wed, 14 Oct 2009 11:00:43 +0200 Subject: [PATCH 3/6] [SCSI] zfcp: Handle WWPN mismatch in PLOGI payload For ports, zfcp gets the DID from the FC nameserver and tries to open the port. If the open succeeds, zfcp compares the WWPN from the nameserver with the WWPN in the PLOGI payload. In case of a mismatch, zfcp assumes that the DID of the port just changed and we opened the wrong port. This means that zfcp has to forget the DID, lookup the DID again and retry. This error case had a problem that zfcp forgets the DID, but never looks up a new one, stalling the ERP in this case. Fix this by triggering the DID lookup and properly exit from the ERP. The DID lookup will trigger a new ERP action. Also ensure when trying to open the port again with the new DID, first close the open port, even in the NOESC case. Signed-off-by: Christof Schmitt Signed-off-by: James Bottomley --- drivers/s390/scsi/zfcp_erp.c | 22 ++++++++++------------ drivers/s390/scsi/zfcp_ext.h | 1 + drivers/s390/scsi/zfcp_fc.c | 11 +++++++++++ 3 files changed, 22 insertions(+), 12 deletions(-) diff --git a/drivers/s390/scsi/zfcp_erp.c b/drivers/s390/scsi/zfcp_erp.c index 73d366ba31e5..f73e2180f333 100644 --- a/drivers/s390/scsi/zfcp_erp.c +++ b/drivers/s390/scsi/zfcp_erp.c @@ -858,10 +858,7 @@ static int zfcp_erp_port_strategy_open_common(struct zfcp_erp_action *act) if (fc_host_port_type(adapter->scsi_host) == FC_PORTTYPE_PTP) return zfcp_erp_open_ptp_port(act); if (!port->d_id) { - zfcp_port_get(port); - if (!queue_work(adapter->work_queue, - &port->gid_pn_work)) - zfcp_port_put(port); + zfcp_fc_trigger_did_lookup(port); return ZFCP_ERP_EXIT; } return zfcp_erp_port_strategy_open_port(act); @@ -869,12 +866,11 @@ static int zfcp_erp_port_strategy_open_common(struct zfcp_erp_action *act) case ZFCP_ERP_STEP_PORT_OPENING: /* D_ID might have changed during open */ if (p_status & ZFCP_STATUS_COMMON_OPEN) { - if (port->d_id) - return ZFCP_ERP_SUCCEEDED; - else { - act->step = ZFCP_ERP_STEP_PORT_CLOSING; - return ZFCP_ERP_CONTINUES; + if (!port->d_id) { + zfcp_fc_trigger_did_lookup(port); + return ZFCP_ERP_EXIT; } + return ZFCP_ERP_SUCCEEDED; } if (port->d_id && !(p_status & ZFCP_STATUS_COMMON_NOESC)) { port->d_id = 0; @@ -889,19 +885,21 @@ static int zfcp_erp_port_strategy_open_common(struct zfcp_erp_action *act) static int zfcp_erp_port_strategy(struct zfcp_erp_action *erp_action) { struct zfcp_port *port = erp_action->port; + int p_status = atomic_read(&port->status); - if (atomic_read(&port->status) & ZFCP_STATUS_COMMON_NOESC) + if ((p_status & ZFCP_STATUS_COMMON_NOESC) && + !(p_status & ZFCP_STATUS_COMMON_OPEN)) goto close_init_done; switch (erp_action->step) { case ZFCP_ERP_STEP_UNINITIALIZED: zfcp_erp_port_strategy_clearstati(port); - if (atomic_read(&port->status) & ZFCP_STATUS_COMMON_OPEN) + if (p_status & ZFCP_STATUS_COMMON_OPEN) return zfcp_erp_port_strategy_close(erp_action); break; case ZFCP_ERP_STEP_PORT_CLOSING: - if (atomic_read(&port->status) & ZFCP_STATUS_COMMON_OPEN) + if (p_status & ZFCP_STATUS_COMMON_OPEN) return ZFCP_ERP_FAILED; break; } diff --git a/drivers/s390/scsi/zfcp_ext.h b/drivers/s390/scsi/zfcp_ext.h index 629edec70405..b3f28deb4505 100644 --- a/drivers/s390/scsi/zfcp_ext.h +++ b/drivers/s390/scsi/zfcp_ext.h @@ -96,6 +96,7 @@ extern int zfcp_fc_scan_ports(struct zfcp_adapter *); extern void _zfcp_fc_scan_ports_later(struct work_struct *); extern void zfcp_fc_incoming_els(struct zfcp_fsf_req *); extern void zfcp_fc_port_did_lookup(struct work_struct *); +extern void zfcp_fc_trigger_did_lookup(struct zfcp_port *); extern void zfcp_fc_plogi_evaluate(struct zfcp_port *, struct fsf_plogi *); extern void zfcp_fc_test_link(struct zfcp_port *); extern void zfcp_fc_link_test_work(struct work_struct *); diff --git a/drivers/s390/scsi/zfcp_fc.c b/drivers/s390/scsi/zfcp_fc.c index 722f22de8753..df23bcead23d 100644 --- a/drivers/s390/scsi/zfcp_fc.c +++ b/drivers/s390/scsi/zfcp_fc.c @@ -360,6 +360,17 @@ out: zfcp_port_put(port); } +/** + * zfcp_fc_trigger_did_lookup - trigger the d_id lookup using a GID_PN request + * @port: The zfcp_port to lookup the d_id for. + */ +void zfcp_fc_trigger_did_lookup(struct zfcp_port *port) +{ + zfcp_port_get(port); + if (!queue_work(port->adapter->work_queue, &port->gid_pn_work)) + zfcp_port_put(port); +} + /** * zfcp_fc_plogi_evaluate - evaluate PLOGI playload * @port: zfcp_port structure From 10d00f78e67223ef429fa5f4abfc9ea4ad740490 Mon Sep 17 00:00:00 2001 From: Christof Schmitt Date: Tue, 13 Oct 2009 10:44:09 +0200 Subject: [PATCH 4/6] [SCSI] zfcp: Warn about storage devices with broken PLOGI data After opening a remote port zfcp checks if the WWPN returned in the PLOGI maches the WWPN of the port that should have been opened. On a mismatch zfcp assumes that the DID just changed, queries the FC nameserver and tries again. If the situation persists the erp will give up. With this strategy, if the remote port always returns the wrong PLOGI data, the remote port will not be opened. Introduce a warning, so that the system administrator knows why the remote port is not being opened and to have a pointer to investigate the problem on the storage system. Reviewed-by: Swen Schillig Signed-off-by: Christof Schmitt Signed-off-by: James Bottomley --- drivers/s390/scsi/zfcp_fsf.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/drivers/s390/scsi/zfcp_fsf.c b/drivers/s390/scsi/zfcp_fsf.c index 38a7e4a6b639..5126461d846e 100644 --- a/drivers/s390/scsi/zfcp_fsf.c +++ b/drivers/s390/scsi/zfcp_fsf.c @@ -1475,9 +1475,16 @@ static void zfcp_fsf_open_port_handler(struct zfcp_fsf_req *req) plogi = (struct fsf_plogi *) req->qtcb->bottom.support.els; if (req->qtcb->bottom.support.els1_length >= FSF_PLOGI_MIN_LEN) { - if (plogi->serv_param.wwpn != port->wwpn) + if (plogi->serv_param.wwpn != port->wwpn) { port->d_id = 0; - else { + dev_warn(&port->adapter->ccw_device->dev, + "A port opened with WWPN 0x%016Lx " + "returned data that identifies it as " + "WWPN 0x%016Lx\n", + (unsigned long long) port->wwpn, + (unsigned long long) + plogi->serv_param.wwpn); + } else { port->wwnn = plogi->serv_param.wwnn; zfcp_fc_plogi_evaluate(port, plogi); } From 9d38500de156fb28ffa8741acb90c4dc90a9fb4b Mon Sep 17 00:00:00 2001 From: Christof Schmitt Date: Tue, 13 Oct 2009 10:44:10 +0200 Subject: [PATCH 5/6] [SCSI] zfcp: Fix timer initialization for ct and els requests Add HZ since the start_timer function expects jiffies, not seconds. Reviewed-by: Swen Schillig Signed-off-by: Christof Schmitt Signed-off-by: James Bottomley --- drivers/s390/scsi/zfcp_fsf.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/s390/scsi/zfcp_fsf.c b/drivers/s390/scsi/zfcp_fsf.c index 5126461d846e..4e41baa0c141 100644 --- a/drivers/s390/scsi/zfcp_fsf.c +++ b/drivers/s390/scsi/zfcp_fsf.c @@ -1079,7 +1079,7 @@ static int zfcp_fsf_setup_ct_els(struct zfcp_fsf_req *req, /* common settings for ct/gs and els requests */ req->qtcb->bottom.support.service_class = FSF_CLASS_3; req->qtcb->bottom.support.timeout = 2 * R_A_TOV; - zfcp_fsf_start_timer(req, 2 * R_A_TOV + 10); + zfcp_fsf_start_timer(req, (2 * R_A_TOV + 10) * HZ); return 0; } From 9e820afd0c4f3c8e8894aa91f5671fd7d11a787b Mon Sep 17 00:00:00 2001 From: Christof Schmitt Date: Tue, 13 Oct 2009 10:44:11 +0200 Subject: [PATCH 6/6] [SCSI] zfcp: Flush SCSI registration work when adding unit When configuring a LUN for use in zfcp, flush the SCSI work to ensure the SCSI device has been created before returning. This means that a configuration procedure can run these commands in a script and the SCSI device is available immediately after the unit_add: echo 1 > /sys/bus/ccw/drivers/zfcp/0.0.181d/online echo 0x401040C300000000 > \ /sys/bus/ccw/drivers/zfcp/0.0.181d/0x500507630313c562/unit_add lsscsi Reviewed-by: Swen Schillig Signed-off-by: Christof Schmitt Signed-off-by: James Bottomley --- drivers/s390/scsi/zfcp_sysfs.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/s390/scsi/zfcp_sysfs.c b/drivers/s390/scsi/zfcp_sysfs.c index 079a8cf518a3..d31000886ca8 100644 --- a/drivers/s390/scsi/zfcp_sysfs.c +++ b/drivers/s390/scsi/zfcp_sysfs.c @@ -224,6 +224,7 @@ static ssize_t zfcp_sysfs_unit_add_store(struct device *dev, zfcp_erp_unit_reopen(unit, 0, "syuas_1", NULL); zfcp_erp_wait(unit->port->adapter); + flush_work(&unit->scsi_work); zfcp_unit_put(unit); out: mutex_unlock(&zfcp_data.config_mutex);