All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/6] zfcp: cleanups and qdio code refactor for 5.13/5.14
@ 2021-04-14 17:07 Benjamin Block
  2021-04-14 17:07 ` [PATCH 1/6] zfcp: remove unneeded INIT_LIST_HEAD() for FSF requests Benjamin Block
                   ` (7 more replies)
  0 siblings, 8 replies; 10+ messages in thread
From: Benjamin Block @ 2021-04-14 17:07 UTC (permalink / raw)
  To: James E.J. Bottomley, Martin K. Petersen
  Cc: Benjamin Block, Steffen Maier, Heiko Carstens, Vasily Gorbik,
	Christian Borntraeger, Fedor Loshakov, Sumangala Bannur Subraya,
	Julian Wiedmann, Yevhen Viktorov, Qinglang Miao, linux-scsi,
	linux-s390

Hello James, Martin,

I know I am pretty late; we have some patches queued that I didn't come
around posting yet; its nothing world-changing, so if you don't want to
pull them for 5.13 anymore, no worries.

Most of these are cleanups, apart from the last patch from Julian. This
lifts the handling of our outbound queue from the common QDIO layer
(common for s390x, that is) to our driver. There is more details in the
patch itself. We already did the same for the inbound queue handling
some time ago.

All of these have run successfully in our internal CI for a bit already
(most for quite some time actually), and so far nothing came up. I also
retested them today with our regression suite on top of 5.12.0-rc7. So
I'm pretty confident this should work without problems.


best regards,
 - Benjamin


Julian Wiedmann (4):
  zfcp: remove unneeded INIT_LIST_HEAD() for FSF requests
  zfcp: fix sysfs roll-back on error in zfcp_adapter_enqueue()
  zfcp: clean up sysfs code for SFP diagnostics
  scsi: zfcp: lift Request Queue tasklet & timer from qdio

Qinglang Miao (1):
  scsi: zfcp: move the position of put_device

Yevhen Viktorov (1):
  zfcp: fix indentation coding style issue

 drivers/s390/scsi/zfcp_aux.c   | 28 +++++++++-----
 drivers/s390/scsi/zfcp_def.h   |  6 +--
 drivers/s390/scsi/zfcp_diag.c  | 42 ---------------------
 drivers/s390/scsi/zfcp_diag.h  |  7 ----
 drivers/s390/scsi/zfcp_ext.h   |  4 +-
 drivers/s390/scsi/zfcp_fsf.c   |  1 -
 drivers/s390/scsi/zfcp_qdio.c  | 68 ++++++++++++++++++++++++++++------
 drivers/s390/scsi/zfcp_qdio.h  |  5 +++
 drivers/s390/scsi/zfcp_sysfs.c | 14 +++++--
 drivers/s390/scsi/zfcp_unit.c  |  4 +-
 10 files changed, 97 insertions(+), 82 deletions(-)

-- 
2.30.2


^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH 1/6] zfcp: remove unneeded INIT_LIST_HEAD() for FSF requests
  2021-04-14 17:07 [PATCH 0/6] zfcp: cleanups and qdio code refactor for 5.13/5.14 Benjamin Block
@ 2021-04-14 17:07 ` Benjamin Block
  2021-04-14 17:08 ` [PATCH 2/6] zfcp: fix indentation coding style issue Benjamin Block
                   ` (6 subsequent siblings)
  7 siblings, 0 replies; 10+ messages in thread
From: Benjamin Block @ 2021-04-14 17:07 UTC (permalink / raw)
  To: James E.J. Bottomley, Martin K. Petersen
  Cc: Julian Wiedmann, Benjamin Block, Steffen Maier, Heiko Carstens,
	Vasily Gorbik, Christian Borntraeger, Fedor Loshakov,
	Sumangala Bannur Subraya, linux-scsi, linux-s390

From: Julian Wiedmann <jwi@linux.ibm.com>

INIT_LIST_HEAD() is only needed for actual list heads, while req->list
is used as a list entry.

Note that when the error path in zfcp_fsf_req_send() removes the request
from the adapter's list of pending requests, it actually looks up the
request from the zfcp_reqlist - rather than just calling list_del().
So there's no risk of us calling list_del() on a request that hasn't
been added to any list yet.

Signed-off-by: Julian Wiedmann <jwi@linux.ibm.com>
Reviewed-by: Benjamin Block <bblock@linux.ibm.com>
Reviewed-by: Steffen Maier <maier@linux.ibm.com>
Signed-off-by: Benjamin Block <bblock@linux.ibm.com>
---
 drivers/s390/scsi/zfcp_fsf.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/s390/scsi/zfcp_fsf.c b/drivers/s390/scsi/zfcp_fsf.c
index 485028324eae..2e4804ef2fb9 100644
--- a/drivers/s390/scsi/zfcp_fsf.c
+++ b/drivers/s390/scsi/zfcp_fsf.c
@@ -846,7 +846,6 @@ static struct zfcp_fsf_req *zfcp_fsf_req_create(struct zfcp_qdio *qdio,
 	if (adapter->req_no == 0)
 		adapter->req_no++;
 
-	INIT_LIST_HEAD(&req->list);
 	timer_setup(&req->timer, NULL, 0);
 	init_completion(&req->completion);
 
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH 2/6] zfcp: fix indentation coding style issue
  2021-04-14 17:07 [PATCH 0/6] zfcp: cleanups and qdio code refactor for 5.13/5.14 Benjamin Block
  2021-04-14 17:07 ` [PATCH 1/6] zfcp: remove unneeded INIT_LIST_HEAD() for FSF requests Benjamin Block
@ 2021-04-14 17:08 ` Benjamin Block
  2021-04-14 17:08 ` [PATCH 3/6] zfcp: fix sysfs roll-back on error in zfcp_adapter_enqueue() Benjamin Block
                   ` (5 subsequent siblings)
  7 siblings, 0 replies; 10+ messages in thread
From: Benjamin Block @ 2021-04-14 17:08 UTC (permalink / raw)
  To: James E.J. Bottomley, Martin K. Petersen
  Cc: Yevhen Viktorov, Benjamin Block, Steffen Maier, Heiko Carstens,
	Vasily Gorbik, Christian Borntraeger, Fedor Loshakov,
	Sumangala Bannur Subraya, linux-scsi, linux-s390

From: Yevhen Viktorov <yevhen.viktorov@virginmedia.com>

code indent should use tabs where possible

Signed-off-by: Yevhen Viktorov <yevhen.viktorov@virginmedia.com>
Signed-off-by: Steffen Maier <maier@linux.ibm.com>
Signed-off-by: Benjamin Block <bblock@linux.ibm.com>
---
 drivers/s390/scsi/zfcp_def.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/s390/scsi/zfcp_def.h b/drivers/s390/scsi/zfcp_def.h
index 26c89c232ef2..94de55304a02 100644
--- a/drivers/s390/scsi/zfcp_def.h
+++ b/drivers/s390/scsi/zfcp_def.h
@@ -156,7 +156,7 @@ struct zfcp_adapter {
 	u32			fsf_lic_version;
 	u32			adapter_features;  /* FCP channel features */
 	u32			connection_features; /* host connection features */
-        u32			hardware_version;  /* of FCP channel */
+	u32			hardware_version;  /* of FCP channel */
 	u32			fc_security_algorithms; /* of FCP channel */
 	u32			fc_security_algorithms_old; /* of FCP channel */
 	u16			timer_ticks;       /* time int for a tick */
@@ -180,7 +180,7 @@ struct zfcp_adapter {
 	rwlock_t		erp_lock;
 	wait_queue_head_t	erp_done_wqh;
 	struct zfcp_erp_action	erp_action;	   /* pending error recovery */
-        atomic_t                erp_counter;
+	atomic_t		erp_counter;
 	u32			erp_total_count;   /* total nr of enqueued erp
 						      actions */
 	u32			erp_low_mem_count; /* nr of erp actions waiting
@@ -217,7 +217,7 @@ struct zfcp_port {
 	u32		       d_id;	       /* D_ID */
 	u32		       handle;	       /* handle assigned by FSF */
 	struct zfcp_erp_action erp_action;     /* pending error recovery */
-        atomic_t               erp_counter;
+	atomic_t	       erp_counter;
 	u32                    maxframe_size;
 	u32                    supported_classes;
 	u32                    connection_info;
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH 3/6] zfcp: fix sysfs roll-back on error in zfcp_adapter_enqueue()
  2021-04-14 17:07 [PATCH 0/6] zfcp: cleanups and qdio code refactor for 5.13/5.14 Benjamin Block
  2021-04-14 17:07 ` [PATCH 1/6] zfcp: remove unneeded INIT_LIST_HEAD() for FSF requests Benjamin Block
  2021-04-14 17:08 ` [PATCH 2/6] zfcp: fix indentation coding style issue Benjamin Block
@ 2021-04-14 17:08 ` Benjamin Block
  2021-04-14 17:08 ` [PATCH 4/6] zfcp: clean up sysfs code for SFP diagnostics Benjamin Block
                   ` (4 subsequent siblings)
  7 siblings, 0 replies; 10+ messages in thread
From: Benjamin Block @ 2021-04-14 17:08 UTC (permalink / raw)
  To: James E.J. Bottomley, Martin K. Petersen
  Cc: Julian Wiedmann, Benjamin Block, Steffen Maier, Heiko Carstens,
	Vasily Gorbik, Christian Borntraeger, Fedor Loshakov,
	Sumangala Bannur Subraya, linux-scsi, linux-s390

From: Julian Wiedmann <jwi@linux.ibm.com>

When zfcp_adapter_enqueue() fails to create the zfcp_sysfs_adapter_attrs
group, it calls zfcp_adapter_unregister() to tear down the adapter state
again. This then unconditionally attempts to remove the
zfcp_sysfs_adapter_attrs group, resulting in a "group not found" WARN
from sysfs code.

Avoid this by copying most of zfcp_adapter_unregister() into the error
path, allowing for more fine-granular roll-back. Then skip the sysfs
tear-down steps if we haven't progressed this far in the initialization.

Signed-off-by: Julian Wiedmann <jwi@linux.ibm.com>
Reviewed-by: Benjamin Block <bblock@linux.ibm.com>
Reviewed-by: Steffen Maier <maier@linux.ibm.com>
Signed-off-by: Benjamin Block <bblock@linux.ibm.com>
---
 drivers/s390/scsi/zfcp_aux.c | 20 ++++++++++++++++++--
 1 file changed, 18 insertions(+), 2 deletions(-)

diff --git a/drivers/s390/scsi/zfcp_aux.c b/drivers/s390/scsi/zfcp_aux.c
index 768873dd55b8..abad77694e72 100644
--- a/drivers/s390/scsi/zfcp_aux.c
+++ b/drivers/s390/scsi/zfcp_aux.c
@@ -418,7 +418,7 @@ struct zfcp_adapter *zfcp_adapter_enqueue(struct ccw_device *ccw_device)
 		goto failed;
 
 	if (zfcp_diag_sysfs_setup(adapter))
-		goto failed;
+		goto err_diag_sysfs;
 
 	/* report size limit per scatter-gather segment */
 	adapter->ccw_device->dev.dma_parms = &adapter->dma_parms;
@@ -427,8 +427,24 @@ struct zfcp_adapter *zfcp_adapter_enqueue(struct ccw_device *ccw_device)
 
 	return adapter;
 
+err_diag_sysfs:
+	sysfs_remove_group(&ccw_device->dev.kobj, &zfcp_sysfs_adapter_attrs);
 failed:
-	zfcp_adapter_unregister(adapter);
+	/* TODO: make this more fine-granular */
+	cancel_delayed_work_sync(&adapter->scan_work);
+	cancel_work_sync(&adapter->stat_work);
+	cancel_work_sync(&adapter->ns_up_work);
+	cancel_work_sync(&adapter->version_change_lost_work);
+	zfcp_destroy_adapter_work_queue(adapter);
+
+	zfcp_fc_wka_ports_force_offline(adapter->gs);
+	zfcp_scsi_adapter_unregister(adapter);
+
+	zfcp_erp_thread_kill(adapter);
+	zfcp_dbf_adapter_unregister(adapter);
+	zfcp_qdio_destroy(adapter->qdio);
+
+	zfcp_ccw_adapter_put(adapter); /* final put to release */
 	return ERR_PTR(-ENOMEM);
 }
 
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH 4/6] zfcp: clean up sysfs code for SFP diagnostics
  2021-04-14 17:07 [PATCH 0/6] zfcp: cleanups and qdio code refactor for 5.13/5.14 Benjamin Block
                   ` (2 preceding siblings ...)
  2021-04-14 17:08 ` [PATCH 3/6] zfcp: fix sysfs roll-back on error in zfcp_adapter_enqueue() Benjamin Block
@ 2021-04-14 17:08 ` Benjamin Block
  2021-04-14 17:08 ` [PATCH 5/6] scsi: zfcp: move the position of put_device Benjamin Block
                   ` (3 subsequent siblings)
  7 siblings, 0 replies; 10+ messages in thread
From: Benjamin Block @ 2021-04-14 17:08 UTC (permalink / raw)
  To: James E.J. Bottomley, Martin K. Petersen
  Cc: Julian Wiedmann, Benjamin Block, Steffen Maier, Heiko Carstens,
	Vasily Gorbik, Christian Borntraeger, Fedor Loshakov,
	Sumangala Bannur Subraya, linux-scsi, linux-s390

From: Julian Wiedmann <jwi@linux.ibm.com>

The error path from zfcp_adapter_enqueue() no longer attempts to remove
the diagnostics attributes if they haven't been created yet.

So remove the manual 'sysfs_established' guard for this case, and use
device_add_groups() to add all adapter-related sysfs attributes
in one go.

Signed-off-by: Julian Wiedmann <jwi@linux.ibm.com>
Reviewed-by: Benjamin Block <bblock@linux.ibm.com>
Reviewed-by: Steffen Maier <maier@linux.ibm.com>
Signed-off-by: Benjamin Block <bblock@linux.ibm.com>
---
 drivers/s390/scsi/zfcp_aux.c   | 14 ++++--------
 drivers/s390/scsi/zfcp_diag.c  | 42 ----------------------------------
 drivers/s390/scsi/zfcp_diag.h  |  7 ------
 drivers/s390/scsi/zfcp_ext.h   |  4 ++--
 drivers/s390/scsi/zfcp_sysfs.c | 10 ++++++--
 5 files changed, 14 insertions(+), 63 deletions(-)

diff --git a/drivers/s390/scsi/zfcp_aux.c b/drivers/s390/scsi/zfcp_aux.c
index abad77694e72..fd2f1c31bd21 100644
--- a/drivers/s390/scsi/zfcp_aux.c
+++ b/drivers/s390/scsi/zfcp_aux.c
@@ -413,12 +413,8 @@ struct zfcp_adapter *zfcp_adapter_enqueue(struct ccw_device *ccw_device)
 
 	dev_set_drvdata(&ccw_device->dev, adapter);
 
-	if (sysfs_create_group(&ccw_device->dev.kobj,
-			       &zfcp_sysfs_adapter_attrs))
-		goto failed;
-
-	if (zfcp_diag_sysfs_setup(adapter))
-		goto err_diag_sysfs;
+	if (device_add_groups(&ccw_device->dev, zfcp_sysfs_adapter_attr_groups))
+		goto err_sysfs;
 
 	/* report size limit per scatter-gather segment */
 	adapter->ccw_device->dev.dma_parms = &adapter->dma_parms;
@@ -427,8 +423,7 @@ struct zfcp_adapter *zfcp_adapter_enqueue(struct ccw_device *ccw_device)
 
 	return adapter;
 
-err_diag_sysfs:
-	sysfs_remove_group(&ccw_device->dev.kobj, &zfcp_sysfs_adapter_attrs);
+err_sysfs:
 failed:
 	/* TODO: make this more fine-granular */
 	cancel_delayed_work_sync(&adapter->scan_work);
@@ -460,8 +455,7 @@ void zfcp_adapter_unregister(struct zfcp_adapter *adapter)
 
 	zfcp_fc_wka_ports_force_offline(adapter->gs);
 	zfcp_scsi_adapter_unregister(adapter);
-	zfcp_diag_sysfs_destroy(adapter);
-	sysfs_remove_group(&cdev->dev.kobj, &zfcp_sysfs_adapter_attrs);
+	device_remove_groups(&cdev->dev, zfcp_sysfs_adapter_attr_groups);
 
 	zfcp_erp_thread_kill(adapter);
 	zfcp_dbf_adapter_unregister(adapter);
diff --git a/drivers/s390/scsi/zfcp_diag.c b/drivers/s390/scsi/zfcp_diag.c
index 67a8f4e57db1..4d2d89d9c15a 100644
--- a/drivers/s390/scsi/zfcp_diag.c
+++ b/drivers/s390/scsi/zfcp_diag.c
@@ -10,8 +10,6 @@
 #include <linux/spinlock.h>
 #include <linux/jiffies.h>
 #include <linux/string.h>
-#include <linux/kernfs.h>
-#include <linux/sysfs.h>
 #include <linux/errno.h>
 #include <linux/slab.h>
 
@@ -79,46 +77,6 @@ void zfcp_diag_adapter_free(struct zfcp_adapter *const adapter)
 	adapter->diagnostics = NULL;
 }
 
-/**
- * zfcp_diag_sysfs_setup() - Setup the sysfs-group for adapter-diagnostics.
- * @adapter: target adapter to which the group should be added.
- *
- * Return: 0 on success; Something else otherwise (see sysfs_create_group()).
- */
-int zfcp_diag_sysfs_setup(struct zfcp_adapter *const adapter)
-{
-	int rc = sysfs_create_group(&adapter->ccw_device->dev.kobj,
-				    &zfcp_sysfs_diag_attr_group);
-	if (rc == 0)
-		adapter->diagnostics->sysfs_established = 1;
-
-	return rc;
-}
-
-/**
- * zfcp_diag_sysfs_destroy() - Remove the sysfs-group for adapter-diagnostics.
- * @adapter: target adapter from which the group should be removed.
- */
-void zfcp_diag_sysfs_destroy(struct zfcp_adapter *const adapter)
-{
-	if (adapter->diagnostics == NULL ||
-	    !adapter->diagnostics->sysfs_established)
-		return;
-
-	/*
-	 * We need this state-handling so we can prevent warnings being printed
-	 * on the kernel-console in case we have to abort a halfway done
-	 * zfcp_adapter_enqueue(), in which the sysfs-group was not yet
-	 * established. sysfs_remove_group() does this checking as well, but
-	 * still prints a warning in case we try to remove a group that has not
-	 * been established before
-	 */
-	adapter->diagnostics->sysfs_established = 0;
-	sysfs_remove_group(&adapter->ccw_device->dev.kobj,
-			   &zfcp_sysfs_diag_attr_group);
-}
-
-
 /**
  * zfcp_diag_update_xdata() - Update a diagnostics buffer.
  * @hdr: the meta data to update.
diff --git a/drivers/s390/scsi/zfcp_diag.h b/drivers/s390/scsi/zfcp_diag.h
index 3852367f15f6..da55133da8fe 100644
--- a/drivers/s390/scsi/zfcp_diag.h
+++ b/drivers/s390/scsi/zfcp_diag.h
@@ -40,8 +40,6 @@ struct zfcp_diag_header {
 /**
  * struct zfcp_diag_adapter - central storage for all diagnostics concerning an
  *			      adapter.
- * @sysfs_established: flag showing that the associated sysfs-group was created
- *		       during run of zfcp_adapter_enqueue().
  * @max_age: maximum age of data in diagnostic buffers before they need to be
  *	     refreshed (in ms).
  * @port_data: data retrieved using exchange port data.
@@ -52,8 +50,6 @@ struct zfcp_diag_header {
  * @config_data.data: cached QTCB Bottom of command exchange config data.
  */
 struct zfcp_diag_adapter {
-	u64	sysfs_established	:1;
-
 	unsigned long	max_age;
 
 	struct zfcp_diag_adapter_port_data {
@@ -69,9 +65,6 @@ struct zfcp_diag_adapter {
 int zfcp_diag_adapter_setup(struct zfcp_adapter *const adapter);
 void zfcp_diag_adapter_free(struct zfcp_adapter *const adapter);
 
-int zfcp_diag_sysfs_setup(struct zfcp_adapter *const adapter);
-void zfcp_diag_sysfs_destroy(struct zfcp_adapter *const adapter);
-
 void zfcp_diag_update_xdata(struct zfcp_diag_header *const hdr,
 			    const void *const data, const bool incomplete);
 
diff --git a/drivers/s390/scsi/zfcp_ext.h b/drivers/s390/scsi/zfcp_ext.h
index 58879213f225..6bc96d70254d 100644
--- a/drivers/s390/scsi/zfcp_ext.h
+++ b/drivers/s390/scsi/zfcp_ext.h
@@ -11,6 +11,7 @@
 #define ZFCP_EXT_H
 
 #include <linux/types.h>
+#include <linux/sysfs.h>
 #include <scsi/fc/fc_els.h>
 #include "zfcp_def.h"
 #include "zfcp_fc.h"
@@ -179,13 +180,12 @@ extern void zfcp_scsi_shost_update_port_data(
 	const struct fsf_qtcb_bottom_port *const bottom);
 
 /* zfcp_sysfs.c */
+extern const struct attribute_group *zfcp_sysfs_adapter_attr_groups[];
 extern const struct attribute_group *zfcp_unit_attr_groups[];
-extern struct attribute_group zfcp_sysfs_adapter_attrs;
 extern const struct attribute_group *zfcp_port_attr_groups[];
 extern struct mutex zfcp_sysfs_port_units_mutex;
 extern struct device_attribute *zfcp_sysfs_sdev_attrs[];
 extern struct device_attribute *zfcp_sysfs_shost_attrs[];
-extern const struct attribute_group zfcp_sysfs_diag_attr_group;
 bool zfcp_sysfs_port_is_removing(const struct zfcp_port *const port);
 
 /* zfcp_unit.c */
diff --git a/drivers/s390/scsi/zfcp_sysfs.c b/drivers/s390/scsi/zfcp_sysfs.c
index 8d9662e8b717..f76946dcbd59 100644
--- a/drivers/s390/scsi/zfcp_sysfs.c
+++ b/drivers/s390/scsi/zfcp_sysfs.c
@@ -435,7 +435,7 @@ static struct attribute *zfcp_adapter_attrs[] = {
 	NULL
 };
 
-struct attribute_group zfcp_sysfs_adapter_attrs = {
+static const struct attribute_group zfcp_sysfs_adapter_attr_group = {
 	.attrs = zfcp_adapter_attrs,
 };
 
@@ -906,7 +906,13 @@ static struct attribute *zfcp_sysfs_diag_attrs[] = {
 	NULL,
 };
 
-const struct attribute_group zfcp_sysfs_diag_attr_group = {
+static const struct attribute_group zfcp_sysfs_diag_attr_group = {
 	.name = "diagnostics",
 	.attrs = zfcp_sysfs_diag_attrs,
 };
+
+const struct attribute_group *zfcp_sysfs_adapter_attr_groups[] = {
+	&zfcp_sysfs_adapter_attr_group,
+	&zfcp_sysfs_diag_attr_group,
+	NULL,
+};
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH 5/6] scsi: zfcp: move the position of put_device
  2021-04-14 17:07 [PATCH 0/6] zfcp: cleanups and qdio code refactor for 5.13/5.14 Benjamin Block
                   ` (3 preceding siblings ...)
  2021-04-14 17:08 ` [PATCH 4/6] zfcp: clean up sysfs code for SFP diagnostics Benjamin Block
@ 2021-04-14 17:08 ` Benjamin Block
  2021-04-14 17:08 ` [PATCH 6/6] scsi: zfcp: lift Request Queue tasklet & timer from qdio Benjamin Block
                   ` (2 subsequent siblings)
  7 siblings, 0 replies; 10+ messages in thread
From: Benjamin Block @ 2021-04-14 17:08 UTC (permalink / raw)
  To: James E.J. Bottomley, Martin K. Petersen
  Cc: Qinglang Miao, Benjamin Block, Steffen Maier, Heiko Carstens,
	Vasily Gorbik, Christian Borntraeger, Fedor Loshakov,
	Sumangala Bannur Subraya, linux-scsi, linux-s390

From: Qinglang Miao <miaoqinglang@huawei.com>

Have the `put_device()` call after `device_unregister()` in both
`zfcp_unit_remove()` and `zfcp_sysfs_port_remove_store()` to make
it more natural, for put_device() ought to be the last time we
touch the object in both functions.

And add comments after put_device to make codes clearer.

Suggested-by: Steffen Maier <maier@linux.ibm.com>
Suggested-by: Benjamin Block <bblock@linux.ibm.com>
Signed-off-by: Qinglang Miao <miaoqinglang@huawei.com>
Signed-off-by: Benjamin Block <bblock@linux.ibm.com>
---
 drivers/s390/scsi/zfcp_sysfs.c | 4 ++--
 drivers/s390/scsi/zfcp_unit.c  | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/s390/scsi/zfcp_sysfs.c b/drivers/s390/scsi/zfcp_sysfs.c
index f76946dcbd59..544efd4c42f0 100644
--- a/drivers/s390/scsi/zfcp_sysfs.c
+++ b/drivers/s390/scsi/zfcp_sysfs.c
@@ -327,10 +327,10 @@ static ssize_t zfcp_sysfs_port_remove_store(struct device *dev,
 	list_del(&port->list);
 	write_unlock_irq(&adapter->port_list_lock);
 
-	put_device(&port->dev);
-
 	zfcp_erp_port_shutdown(port, 0, "syprs_1");
 	device_unregister(&port->dev);
+
+	put_device(&port->dev); /* undo zfcp_get_port_by_wwpn() */
  out:
 	zfcp_ccw_adapter_put(adapter);
 	return retval ? retval : (ssize_t) count;
diff --git a/drivers/s390/scsi/zfcp_unit.c b/drivers/s390/scsi/zfcp_unit.c
index e67bf7388cae..59333f0257a8 100644
--- a/drivers/s390/scsi/zfcp_unit.c
+++ b/drivers/s390/scsi/zfcp_unit.c
@@ -255,9 +255,9 @@ int zfcp_unit_remove(struct zfcp_port *port, u64 fcp_lun)
 		scsi_device_put(sdev);
 	}
 
-	put_device(&unit->dev);
-
 	device_unregister(&unit->dev);
 
+	put_device(&unit->dev); /* undo _zfcp_unit_find() */
+
 	return 0;
 }
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH 6/6] scsi: zfcp: lift Request Queue tasklet & timer from qdio
  2021-04-14 17:07 [PATCH 0/6] zfcp: cleanups and qdio code refactor for 5.13/5.14 Benjamin Block
                   ` (4 preceding siblings ...)
  2021-04-14 17:08 ` [PATCH 5/6] scsi: zfcp: move the position of put_device Benjamin Block
@ 2021-04-14 17:08 ` Benjamin Block
  2021-04-16  2:20 ` [PATCH 0/6] zfcp: cleanups and qdio code refactor for 5.13/5.14 Martin K. Petersen
  2021-04-20  2:29 ` Martin K. Petersen
  7 siblings, 0 replies; 10+ messages in thread
From: Benjamin Block @ 2021-04-14 17:08 UTC (permalink / raw)
  To: James E.J. Bottomley, Martin K. Petersen
  Cc: Julian Wiedmann, Benjamin Block, Steffen Maier, Heiko Carstens,
	Vasily Gorbik, Christian Borntraeger, Fedor Loshakov,
	Sumangala Bannur Subraya, linux-scsi, linux-s390

From: Julian Wiedmann <jwi@linux.ibm.com>

The qdio layer currently provides its own infrastructure to scan for
Request Queue completions & to report them to the device driver.
This comes with several drawbacks - having an async tasklet & timer
construct in qdio introduces additional lifetime complexity, and makes
it harder to integrate them with the rest of the device driver. The
timeouts are also currently hard-coded, and can't be tweaked without
affecting other qdio drivers (ie. qeth).

But due to recent enhancements to the qdio layer, zfcp can actually take
full control of the Request Queue completion processing. It merely needs
to opt-out from the qdio layer mechanisms by setting the scan_threshold
to 0, and then use qdio_inspect_queue() to scan for completions.

So re-implement the tasklet & timer mechanism in zfcp, while initially
copying the scan conditions from qdio's handle_outbound() and
qdio_outbound_tasklet(). One minor behavioural change is that
zfcp_qdio_send() will unconditionally reduce the timeout to 1 HZ, rather
than leaving it at 10 Hz if it was last armed by the tasklet. This just
makes things more consistent. Also note that we can drop a lot of the
accumulated cruft in qdio_outbound_tasklet(), as zfcp doesn't even use
PCI interrupt requests any longer.

This also slightly touches the Response Queue processing, as
qdio_get_next_buffers() will no longer implicitly scan for Request Queue
completions. So complete the migration to qdio_inspect_queue() here as
well and make the tasklet_schedule() visible.

Signed-off-by: Julian Wiedmann <jwi@linux.ibm.com>
Reviewed-by: Benjamin Block <bblock@linux.ibm.com>
Signed-off-by: Benjamin Block <bblock@linux.ibm.com>
---
 drivers/s390/scsi/zfcp_qdio.c | 68 ++++++++++++++++++++++++++++-------
 drivers/s390/scsi/zfcp_qdio.h |  5 +++
 2 files changed, 61 insertions(+), 12 deletions(-)

diff --git a/drivers/s390/scsi/zfcp_qdio.c b/drivers/s390/scsi/zfcp_qdio.c
index 23ab16d65f2a..16a332d501e7 100644
--- a/drivers/s390/scsi/zfcp_qdio.c
+++ b/drivers/s390/scsi/zfcp_qdio.c
@@ -20,6 +20,9 @@ static bool enable_multibuffer = true;
 module_param_named(datarouter, enable_multibuffer, bool, 0400);
 MODULE_PARM_DESC(datarouter, "Enable hardware data router support (default on)");
 
+#define ZFCP_QDIO_REQUEST_RESCAN_MSECS	(MSEC_PER_SEC * 10)
+#define ZFCP_QDIO_REQUEST_SCAN_MSECS	MSEC_PER_SEC
+
 static void zfcp_qdio_handler_error(struct zfcp_qdio *qdio, char *dbftag,
 				    unsigned int qdio_err)
 {
@@ -70,15 +73,41 @@ static void zfcp_qdio_int_req(struct ccw_device *cdev, unsigned int qdio_err,
 		zfcp_qdio_handler_error(qdio, "qdireq1", qdio_err);
 		return;
 	}
+}
 
-	/* cleanup all SBALs being program-owned now */
-	zfcp_qdio_zero_sbals(qdio->req_q, idx, count);
+static void zfcp_qdio_request_tasklet(struct tasklet_struct *tasklet)
+{
+	struct zfcp_qdio *qdio = from_tasklet(qdio, tasklet, request_tasklet);
+	struct ccw_device *cdev = qdio->adapter->ccw_device;
+	unsigned int start, error;
+	int completed;
 
-	spin_lock_irq(&qdio->stat_lock);
-	zfcp_qdio_account(qdio);
-	spin_unlock_irq(&qdio->stat_lock);
-	atomic_add(count, &qdio->req_q_free);
-	wake_up(&qdio->req_q_wq);
+	completed = qdio_inspect_queue(cdev, 0, false, &start, &error);
+	if (completed > 0) {
+		if (error) {
+			zfcp_qdio_handler_error(qdio, "qdreqt1", error);
+		} else {
+			/* cleanup all SBALs being program-owned now */
+			zfcp_qdio_zero_sbals(qdio->req_q, start, completed);
+
+			spin_lock_irq(&qdio->stat_lock);
+			zfcp_qdio_account(qdio);
+			spin_unlock_irq(&qdio->stat_lock);
+			atomic_add(completed, &qdio->req_q_free);
+			wake_up(&qdio->req_q_wq);
+		}
+	}
+
+	if (atomic_read(&qdio->req_q_free) < QDIO_MAX_BUFFERS_PER_Q)
+		timer_reduce(&qdio->request_timer,
+			     jiffies + msecs_to_jiffies(ZFCP_QDIO_REQUEST_RESCAN_MSECS));
+}
+
+static void zfcp_qdio_request_timer(struct timer_list *timer)
+{
+	struct zfcp_qdio *qdio = from_timer(qdio, timer, request_timer);
+
+	tasklet_schedule(&qdio->request_tasklet);
 }
 
 static void zfcp_qdio_int_resp(struct ccw_device *cdev, unsigned int qdio_err,
@@ -139,8 +168,11 @@ static void zfcp_qdio_irq_tasklet(struct tasklet_struct *tasklet)
 	unsigned int start, error;
 	int completed;
 
-	/* Check the Response Queue, and kick off the Request Queue tasklet: */
-	completed = qdio_get_next_buffers(cdev, 0, &start, &error);
+	if (atomic_read(&qdio->req_q_free) < QDIO_MAX_BUFFERS_PER_Q)
+		tasklet_schedule(&qdio->request_tasklet);
+
+	/* Check the Response Queue: */
+	completed = qdio_inspect_queue(cdev, 0, true, &start, &error);
 	if (completed < 0)
 		return;
 	if (completed > 0)
@@ -286,7 +318,7 @@ int zfcp_qdio_send(struct zfcp_qdio *qdio, struct zfcp_qdio_req *q_req)
 
 	/*
 	 * This should actually be a spin_lock_bh(stat_lock), to protect against
-	 * zfcp_qdio_int_req() in tasklet context.
+	 * Request Queue completion processing in tasklet context.
 	 * But we can't do so (and are safe), as we always get called with IRQs
 	 * disabled by spin_lock_irq[save](req_q_lock).
 	 */
@@ -308,6 +340,12 @@ int zfcp_qdio_send(struct zfcp_qdio *qdio, struct zfcp_qdio_req *q_req)
 		return retval;
 	}
 
+	if (atomic_read(&qdio->req_q_free) <= 2 * ZFCP_QDIO_MAX_SBALS_PER_REQ)
+		tasklet_schedule(&qdio->request_tasklet);
+	else
+		timer_reduce(&qdio->request_timer,
+			     jiffies + msecs_to_jiffies(ZFCP_QDIO_REQUEST_SCAN_MSECS));
+
 	/* account for transferred buffers */
 	qdio->req_q_idx += sbal_number;
 	qdio->req_q_idx %= QDIO_MAX_BUFFERS_PER_Q;
@@ -368,6 +406,8 @@ void zfcp_qdio_close(struct zfcp_qdio *qdio)
 	wake_up(&qdio->req_q_wq);
 
 	tasklet_disable(&qdio->irq_tasklet);
+	tasklet_disable(&qdio->request_tasklet);
+	del_timer_sync(&qdio->request_timer);
 	qdio_stop_irq(adapter->ccw_device);
 	qdio_shutdown(adapter->ccw_device, QDIO_FLAG_CLEANUP_USING_CLEAR);
 
@@ -428,8 +468,6 @@ int zfcp_qdio_open(struct zfcp_qdio *qdio)
 	init_data.int_parm = (unsigned long) qdio;
 	init_data.input_sbal_addr_array = input_sbals;
 	init_data.output_sbal_addr_array = output_sbals;
-	init_data.scan_threshold =
-		QDIO_MAX_BUFFERS_PER_Q - ZFCP_QDIO_MAX_SBALS_PER_REQ * 2;
 
 	if (qdio_establish(cdev, &init_data))
 		goto failed_establish;
@@ -471,6 +509,8 @@ int zfcp_qdio_open(struct zfcp_qdio *qdio)
 	atomic_set(&qdio->req_q_free, QDIO_MAX_BUFFERS_PER_Q);
 	atomic_or(ZFCP_STATUS_ADAPTER_QDIOUP, &qdio->adapter->status);
 
+	/* Enable processing for Request Queue completions: */
+	tasklet_enable(&qdio->request_tasklet);
 	/* Enable processing for QDIO interrupts: */
 	tasklet_enable(&qdio->irq_tasklet);
 	/* This results in a qdio_start_irq(): */
@@ -494,6 +534,7 @@ void zfcp_qdio_destroy(struct zfcp_qdio *qdio)
 		return;
 
 	tasklet_kill(&qdio->irq_tasklet);
+	tasklet_kill(&qdio->request_tasklet);
 
 	if (qdio->adapter->ccw_device)
 		qdio_free(qdio->adapter->ccw_device);
@@ -520,8 +561,11 @@ int zfcp_qdio_setup(struct zfcp_adapter *adapter)
 
 	spin_lock_init(&qdio->req_q_lock);
 	spin_lock_init(&qdio->stat_lock);
+	timer_setup(&qdio->request_timer, zfcp_qdio_request_timer, 0);
 	tasklet_setup(&qdio->irq_tasklet, zfcp_qdio_irq_tasklet);
+	tasklet_setup(&qdio->request_tasklet, zfcp_qdio_request_tasklet);
 	tasklet_disable(&qdio->irq_tasklet);
+	tasklet_disable(&qdio->request_tasklet);
 
 	adapter->qdio = qdio;
 	return 0;
diff --git a/drivers/s390/scsi/zfcp_qdio.h b/drivers/s390/scsi/zfcp_qdio.h
index 9c1f310db155..390706867df3 100644
--- a/drivers/s390/scsi/zfcp_qdio.h
+++ b/drivers/s390/scsi/zfcp_qdio.h
@@ -30,6 +30,9 @@
  * @req_q_util: used for accounting
  * @req_q_full: queue full incidents
  * @req_q_wq: used to wait for SBAL availability
+ * @irq_tasklet: used for QDIO interrupt processing
+ * @request_tasklet: used for Request Queue completion processing
+ * @request_timer: used to trigger the Request Queue completion processing
  * @adapter: adapter used in conjunction with this qdio structure
  * @max_sbale_per_sbal: qdio limit per sbal
  * @max_sbale_per_req: qdio limit per request
@@ -46,6 +49,8 @@ struct zfcp_qdio {
 	atomic_t		req_q_full;
 	wait_queue_head_t	req_q_wq;
 	struct tasklet_struct	irq_tasklet;
+	struct tasklet_struct	request_tasklet;
+	struct timer_list	request_timer;
 	struct zfcp_adapter	*adapter;
 	u16			max_sbale_per_sbal;
 	u16			max_sbale_per_req;
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH 0/6] zfcp: cleanups and qdio code refactor for 5.13/5.14
  2021-04-14 17:07 [PATCH 0/6] zfcp: cleanups and qdio code refactor for 5.13/5.14 Benjamin Block
                   ` (5 preceding siblings ...)
  2021-04-14 17:08 ` [PATCH 6/6] scsi: zfcp: lift Request Queue tasklet & timer from qdio Benjamin Block
@ 2021-04-16  2:20 ` Martin K. Petersen
  2021-04-18 16:04   ` Benjamin Block
  2021-04-20  2:29 ` Martin K. Petersen
  7 siblings, 1 reply; 10+ messages in thread
From: Martin K. Petersen @ 2021-04-16  2:20 UTC (permalink / raw)
  To: Benjamin Block
  Cc: James E.J. Bottomley, Martin K. Petersen, Steffen Maier,
	Heiko Carstens, Vasily Gorbik, Christian Borntraeger,
	Fedor Loshakov, Sumangala Bannur Subraya, Julian Wiedmann,
	Yevhen Viktorov, Qinglang Miao, linux-scsi, linux-s390


Benjamin,

> I know I am pretty late; we have some patches queued that I didn't
> come around posting yet; its nothing world-changing, so if you don't
> want to pull them for 5.13 anymore, no worries.

Applied to 5.13/scsi-staging, thanks! We'll see whether we get an -rc8
or not.

-- 
Martin K. Petersen	Oracle Linux Engineering

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 0/6] zfcp: cleanups and qdio code refactor for 5.13/5.14
  2021-04-16  2:20 ` [PATCH 0/6] zfcp: cleanups and qdio code refactor for 5.13/5.14 Martin K. Petersen
@ 2021-04-18 16:04   ` Benjamin Block
  0 siblings, 0 replies; 10+ messages in thread
From: Benjamin Block @ 2021-04-18 16:04 UTC (permalink / raw)
  To: Martin K. Petersen
  Cc: James E.J. Bottomley, Steffen Maier, Heiko Carstens,
	Vasily Gorbik, Christian Borntraeger, Fedor Loshakov,
	Sumangala Bannur Subraya, Julian Wiedmann, Yevhen Viktorov,
	Qinglang Miao, linux-scsi, linux-s390

On Thu, Apr 15, 2021 at 10:20:55PM -0400, Martin K. Petersen wrote:
> 
> Benjamin,
> 
> > I know I am pretty late; we have some patches queued that I didn't
> > come around posting yet; its nothing world-changing, so if you don't
> > want to pull them for 5.13 anymore, no worries.
> 
> Applied to 5.13/scsi-staging, thanks! We'll see whether we get an -rc8
> or not.
> 

Alright, thanks Martin.

-- 
Best Regards, Benjamin Block  / Linux on IBM Z Kernel Development / IBM Systems
IBM Deutschland Research & Development GmbH    /    https://www.ibm.com/privacy
Vorsitz. AufsR.: Gregor Pillen         /        Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen / Registergericht: AmtsG Stuttgart, HRB 243294

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 0/6] zfcp: cleanups and qdio code refactor for 5.13/5.14
  2021-04-14 17:07 [PATCH 0/6] zfcp: cleanups and qdio code refactor for 5.13/5.14 Benjamin Block
                   ` (6 preceding siblings ...)
  2021-04-16  2:20 ` [PATCH 0/6] zfcp: cleanups and qdio code refactor for 5.13/5.14 Martin K. Petersen
@ 2021-04-20  2:29 ` Martin K. Petersen
  7 siblings, 0 replies; 10+ messages in thread
From: Martin K. Petersen @ 2021-04-20  2:29 UTC (permalink / raw)
  To: Benjamin Block, James E.J. Bottomley
  Cc: Martin K . Petersen, Yevhen Viktorov, Steffen Maier,
	Vasily Gorbik, Sumangala Bannur Subraya, Christian Borntraeger,
	Qinglang Miao, Julian Wiedmann, Fedor Loshakov, linux-s390,
	Heiko Carstens, linux-scsi

On Wed, 14 Apr 2021 19:07:58 +0200, Benjamin Block wrote:

> I know I am pretty late; we have some patches queued that I didn't come
> around posting yet; its nothing world-changing, so if you don't want to
> pull them for 5.13 anymore, no worries.
> 
> Most of these are cleanups, apart from the last patch from Julian. This
> lifts the handling of our outbound queue from the common QDIO layer
> (common for s390x, that is) to our driver. There is more details in the
> patch itself. We already did the same for the inbound queue handling
> some time ago.
> 
> [...]

Applied to 5.13/scsi-queue, thanks!

[1/6] zfcp: remove unneeded INIT_LIST_HEAD() for FSF requests
      https://git.kernel.org/mkp/scsi/c/91cf21ec6d04
[2/6] zfcp: fix indentation coding style issue
      https://git.kernel.org/mkp/scsi/c/8824db894dd1
[3/6] zfcp: fix sysfs roll-back on error in zfcp_adapter_enqueue()
      https://git.kernel.org/mkp/scsi/c/ab1fa88062f8
[4/6] zfcp: clean up sysfs code for SFP diagnostics
      https://git.kernel.org/mkp/scsi/c/20540a5645f0
[5/6] scsi: zfcp: move the position of put_device
      https://git.kernel.org/mkp/scsi/c/be46e39ae3be
[6/6] scsi: zfcp: lift Request Queue tasklet & timer from qdio
      https://git.kernel.org/mkp/scsi/c/b3f0a1ee9e39

-- 
Martin K. Petersen	Oracle Linux Engineering

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2021-04-20  2:29 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-14 17:07 [PATCH 0/6] zfcp: cleanups and qdio code refactor for 5.13/5.14 Benjamin Block
2021-04-14 17:07 ` [PATCH 1/6] zfcp: remove unneeded INIT_LIST_HEAD() for FSF requests Benjamin Block
2021-04-14 17:08 ` [PATCH 2/6] zfcp: fix indentation coding style issue Benjamin Block
2021-04-14 17:08 ` [PATCH 3/6] zfcp: fix sysfs roll-back on error in zfcp_adapter_enqueue() Benjamin Block
2021-04-14 17:08 ` [PATCH 4/6] zfcp: clean up sysfs code for SFP diagnostics Benjamin Block
2021-04-14 17:08 ` [PATCH 5/6] scsi: zfcp: move the position of put_device Benjamin Block
2021-04-14 17:08 ` [PATCH 6/6] scsi: zfcp: lift Request Queue tasklet & timer from qdio Benjamin Block
2021-04-16  2:20 ` [PATCH 0/6] zfcp: cleanups and qdio code refactor for 5.13/5.14 Martin K. Petersen
2021-04-18 16:04   ` Benjamin Block
2021-04-20  2:29 ` Martin K. Petersen

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.