[PATCH 0/7] zfcp: cleanups and small changes for 5.9

[PATCH 0/7] zfcp: cleanups and small changes for 5.9

[Benjamin Block]

Hello Martin, James, Jonathan,

here are some cleanups and small changes for zfcp I'd like to include in
5.9 if possible.

One of the changes touches documentation in `Documentation/scsi/`, so I
put Jonathan on To, hope that was correct. I hope you can still pull
this all in one go to minimize work. IBM did retire some old URLs and
content from our public website, so we have to clean that up in the
documentation so there are not dead links. I changed these in the hopes
we can minimize documentation churn going forward, just to replace URLs.

The other changes are mostly small cleanups for our driver that make at
least parts more sane than they've been.

Most of these have been in our CI for quite a while now - shame on me
for delaying sending them upstream so long -, so I am rather confident
that they don't break anything for us.

As always, feedback and reviews are appreciated :-)

Benjamin Block (2):
  scsi: docs: update outdated link to IBM developerworks
  scsi: docs: remove invalid link and update text for zfcp kernel config

George Spelvin (1):
  zfcp: use prandom_u32_max() for backoff

Julian Wiedmann (4):
  zfcp: fix an outdated comment for zfcp_qdio_send()
  zfcp: clean up zfcp_erp_action_ready()
  zfcp: replace open-coded list move
  zfcp: avoid benign overflow of the Request Queue's free-level

 Documentation/scsi/scsi-parameters.rst |  2 +-
 drivers/s390/scsi/zfcp_ccw.c           |  7 +++----
 drivers/s390/scsi/zfcp_erp.c           |  2 +-
 drivers/s390/scsi/zfcp_fc.c            |  2 +-
 drivers/s390/scsi/zfcp_qdio.c          |  7 +++++--
 drivers/scsi/Kconfig                   | 15 ++++++++++-----
 6 files changed, 21 insertions(+), 14 deletions(-)

-- 
2.26.2

[PATCH 1/7] zfcp: use prandom_u32_max() for backoff

[Benjamin Block]

From: George Spelvin <lkml@sdf.org>

We don't need crypto-grade random numbers for randomized backoffs.
Instead use prandom_u32_max(ep_ro) which generates a pseudo-random
number uniformly distributed in the interval [0, ep_ro).

Signed-off-by: George Spelvin <lkml@sdf.org>
Reviewed-by: Steffen Maier <maier@linux.ibm.com>
Signed-off-by: Benjamin Block <bblock@linux.ibm.com>
---
 drivers/s390/scsi/zfcp_fc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/s390/scsi/zfcp_fc.c b/drivers/s390/scsi/zfcp_fc.c
index b018b61bd168..d24cafe02708 100644
--- a/drivers/s390/scsi/zfcp_fc.c
+++ b/drivers/s390/scsi/zfcp_fc.c
@@ -48,7 +48,7 @@ unsigned int zfcp_fc_port_scan_backoff(void)
 {
 	if (!port_scan_backoff)
 		return 0;
-	return get_random_int() % port_scan_backoff;
+	return prandom_u32_max(port_scan_backoff);
 }
 
 static void zfcp_fc_port_scan_time(struct zfcp_adapter *adapter)
-- 
2.26.2

[PATCH 2/7] zfcp: fix an outdated comment for zfcp_qdio_send()

[Benjamin Block]

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

zfcp no longer uses the qdio PCI flag, update the comment.

Fixes: 21ddaa53f92d ("[SCSI] zfcp: Remove PCI flag")
Signed-off-by: Julian Wiedmann <jwi@linux.ibm.com>
Reviewed-by: Steffen Maier <maier@linux.ibm.com>
Reviewed-by: Fedor Loshakov <loshakov@linux.ibm.com>
Signed-off-by: Benjamin Block <bblock@linux.ibm.com>
---
 drivers/s390/scsi/zfcp_qdio.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/s390/scsi/zfcp_qdio.c b/drivers/s390/scsi/zfcp_qdio.c
index 3a7f3374d10a..d3d110a04884 100644
--- a/drivers/s390/scsi/zfcp_qdio.c
+++ b/drivers/s390/scsi/zfcp_qdio.c
@@ -246,7 +246,7 @@ int zfcp_qdio_sbal_get(struct zfcp_qdio *qdio)
 }
 
 /**
- * zfcp_qdio_send - set PCI flag in first SBALE and send req to QDIO
+ * zfcp_qdio_send - send req to QDIO
  * @qdio: pointer to struct zfcp_qdio
  * @q_req: pointer to struct zfcp_qdio_req
  * Returns: 0 on success, error otherwise
-- 
2.26.2

[PATCH 3/7] scsi: docs: update outdated link to IBM developerworks

[Benjamin Block]

IBM decided to retire a lot of the content that was previously hosted on
"developerworks", and so some of the links we've used for documentation
are now dead or redirect to some general landing page with no
correlation to what the links were meant to provide.

The s390-tools package is meanwhile also hosted on github, so we can
link to the script directly instead of to the archive.

Reviewed-by: Julian Wiedmann <jwi@linux.ibm.com>
Reviewed-by: Steffen Maier <maier@linux.ibm.com>
Signed-off-by: Benjamin Block <bblock@linux.ibm.com>
---
 Documentation/scsi/scsi-parameters.rst | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/scsi/scsi-parameters.rst b/Documentation/scsi/scsi-parameters.rst
index 9aba897c97ac..e5f68b431f5c 100644
--- a/Documentation/scsi/scsi-parameters.rst
+++ b/Documentation/scsi/scsi-parameters.rst
@@ -94,7 +94,7 @@ parameters may be changed at runtime by the command
 			(/proc/sys/dev/scsi/logging_level).
 			There is also a nice 'scsi_logging_level' script in the
 			S390-tools package, available for download at
-			http://www-128.ibm.com/developerworks/linux/linux390/s390-tools-1.5.4.html
+			https://github.com/ibm-s390-tools/s390-tools/blob/master/scripts/scsi_logging_level
 
 	scsi_mod.scan=	[SCSI] sync (default) scans SCSI busses as they are
 			discovered.  async scans them in kernel threads,
-- 
2.26.2

[PATCH 4/7] scsi: docs: remove invalid link and update text for zfcp kernel config

[Benjamin Block]

IBM decided to retire a lot of the content that was previously hosted on
"developerworks", and so some of the links we've used for documentation
are now dead or redirect to some general landing page with no correlation
to what the links were meant to provide.

Change the provided link in the Kconfig file for zfcp to rather refer to
our device drivers book that we regularly update and publish for free,
and whose name hasn't been changed since it was first published.

Our hardware is also not called "IBM eServer zSeries" anymore - in fact,
it hasn't been called like that since 2006. Use a broader term that
covers different server names over time.

Lastly, add a short paragraph about how our HBAs are typically named, to
have some more tangible references.

Reviewed-by: Julian Wiedmann <jwi@linux.ibm.com>
Reviewed-by: Steffen Maier <maier@linux.ibm.com>
Signed-off-by: Benjamin Block <bblock@linux.ibm.com>
---
 drivers/scsi/Kconfig | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/drivers/scsi/Kconfig b/drivers/scsi/Kconfig
index e9ff4cd5fbe9..571473a58f98 100644
--- a/drivers/scsi/Kconfig
+++ b/drivers/scsi/Kconfig
@@ -1469,14 +1469,19 @@ config SCSI_SUNESP
 	  module will be called sun_esp.
 
 config ZFCP
-	tristate "FCP host bus adapter driver for IBM eServer zSeries"
+	tristate "FCP host bus adapter driver for IBM mainframes"
 	depends on S390 && QDIO && SCSI
 	depends on SCSI_FC_ATTRS
 	help
-          If you want to access SCSI devices attached to your IBM eServer
-          zSeries by means of Fibre Channel interfaces say Y.
-          For details please refer to the documentation provided by IBM at
-          <http://oss.software.ibm.com/developerworks/opensource/linux390>
+	  If you want to access SCSI devices attached to your IBM mainframe by
+	  means of Fibre Channel Protocol host bus adapters say Y.
+
+	  Supported HBAs include different models of the FICON Express and FCP
+	  Express I/O cards.
+
+	  For a more complete list, and for more details about setup and
+	  operation refer to the IBM publication "Device Drivers, Features, and
+	  Commands", SC33-8411.
 
           This driver is also available as a module. This module will be
           called zfcp. If you want to compile it as a module, say M here
-- 
2.26.2

[PATCH 5/7] zfcp: clean up zfcp_erp_action_ready()

[Benjamin Block]

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

We already maintain a pointer to act->adapter. Use it consistently to
avoid any confusion about whose ->erp_ready_head and ->erp_ready_wq
we are accessing.

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

diff --git a/drivers/s390/scsi/zfcp_erp.c b/drivers/s390/scsi/zfcp_erp.c
index 79f6e8fb03ca..59e662df5774 100644
--- a/drivers/s390/scsi/zfcp_erp.c
+++ b/drivers/s390/scsi/zfcp_erp.c
@@ -68,7 +68,7 @@ static void zfcp_erp_action_ready(struct zfcp_erp_action *act)
 {
 	struct zfcp_adapter *adapter = act->adapter;
 
-	list_move(&act->list, &act->adapter->erp_ready_head);
+	list_move(&act->list, &adapter->erp_ready_head);
 	zfcp_dbf_rec_run("erardy1", act);
 	wake_up(&adapter->erp_ready_wq);
 	zfcp_dbf_rec_run("erardy2", act);
-- 
2.26.2

[PATCH 6/7] zfcp: replace open-coded list move

[Benjamin Block]

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

Instead of manually moving each element of the unit and port lists into
our temporary on-stack lists, splice them over in one go.

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

diff --git a/drivers/s390/scsi/zfcp_ccw.c b/drivers/s390/scsi/zfcp_ccw.c
index 49eda141ea43..d9fd0a41da64 100644
--- a/drivers/s390/scsi/zfcp_ccw.c
+++ b/drivers/s390/scsi/zfcp_ccw.c
@@ -124,13 +124,12 @@ static void zfcp_ccw_remove(struct ccw_device *cdev)
 		return;
 
 	write_lock_irq(&adapter->port_list_lock);
-	list_for_each_entry_safe(port, p, &adapter->port_list, list) {
+	list_for_each_entry(port, &adapter->port_list, list) {
 		write_lock(&port->unit_list_lock);
-		list_for_each_entry_safe(unit, u, &port->unit_list, list)
-			list_move(&unit->list, &unit_remove_lh);
+		list_splice_init(&port->unit_list, &unit_remove_lh);
 		write_unlock(&port->unit_list_lock);
-		list_move(&port->list, &port_remove_lh);
 	}
+	list_splice_init(&adapter->port_list, &port_remove_lh);
 	write_unlock_irq(&adapter->port_list_lock);
 	zfcp_ccw_adapter_put(adapter); /* put from zfcp_ccw_adapter_by_cdev */
 
-- 
2.26.2

[PATCH 7/7] zfcp: avoid benign overflow of the Request Queue's free-level

[Benjamin Block]

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

zfcp_qdio_send() and zfcp_qdio_int_req() run concurrently, adding and
completing SBALs on the Request Queue. There's a theoretical race where
zfcp_qdio_int_req() completes a number of SBALs & increments the queue's
free-level _before_ zfcp_qdio_send() was able to decrement it.

This can cause ->req_q_free to momentarily hold a value larger than
QDIO_MAX_BUFFERS_PER_Q. Luckily zfcp_qdio_send() is always called under
->req_q_lock, and all readers of the free-level also take this lock. So
we can trust that zfcp_qdio_send() will clean up such a temporary
overflow before anyone can actually observe it.

But it's still confusing and annoying to worry about. So adjust the code
to avoid this race.

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

diff --git a/drivers/s390/scsi/zfcp_qdio.c b/drivers/s390/scsi/zfcp_qdio.c
index d3d110a04884..e78d65bd46b1 100644
--- a/drivers/s390/scsi/zfcp_qdio.c
+++ b/drivers/s390/scsi/zfcp_qdio.c
@@ -260,17 +260,20 @@ int zfcp_qdio_send(struct zfcp_qdio *qdio, struct zfcp_qdio_req *q_req)
 	zfcp_qdio_account(qdio);
 	spin_unlock(&qdio->stat_lock);
 
+	atomic_sub(sbal_number, &qdio->req_q_free);
+
 	retval = do_QDIO(qdio->adapter->ccw_device, QDIO_FLAG_SYNC_OUTPUT, 0,
 			 q_req->sbal_first, sbal_number);
 
 	if (unlikely(retval)) {
+		/* Failed to submit the IO, roll back our modifications. */
+		atomic_add(sbal_number, &qdio->req_q_free);
 		zfcp_qdio_zero_sbals(qdio->req_q, q_req->sbal_first,
 				     sbal_number);
 		return retval;
 	}
 
 	/* account for transferred buffers */
-	atomic_sub(sbal_number, &qdio->req_q_free);
 	qdio->req_q_idx += sbal_number;
 	qdio->req_q_idx %= QDIO_MAX_BUFFERS_PER_Q;
 
-- 
2.26.2

Re: [PATCH 0/7] zfcp: cleanups and small changes for 5.9

[Martin K. Petersen]

On Fri, 3 Jul 2020 15:19:56 +0200, Benjamin Block wrote:

> here are some cleanups and small changes for zfcp I'd like to include in
> 5.9 if possible.
> 
> One of the changes touches documentation in `Documentation/scsi/`, so I
> put Jonathan on To, hope that was correct. I hope you can still pull
> this all in one go to minimize work. IBM did retire some old URLs and
> content from our public website, so we have to clean that up in the
> documentation so there are not dead links. I changed these in the hopes
> we can minimize documentation churn going forward, just to replace URLs.
> 
> [...]

Applied to 5.9/scsi-queue, thanks!

[1/7] scsi: zfcp: Use prandom_u32_max() for backoff
      https://git.kernel.org/mkp/scsi/c/0cd0e57ec858
[2/7] scsi: zfcp: Fix an outdated comment for zfcp_qdio_send()
      https://git.kernel.org/mkp/scsi/c/459ad085d87b
[3/7] scsi: docs: Update outdated link to IBM developerworks
      https://git.kernel.org/mkp/scsi/c/b9789bfbfe9d
[4/7] scsi: docs: Remove invalid link and update text for zfcp kernel config
      https://git.kernel.org/mkp/scsi/c/c06de6e28c9e
[5/7] scsi: zfcp: Clean up zfcp_erp_action_ready()
      https://git.kernel.org/mkp/scsi/c/b43cdb5ac856
[6/7] scsi: zfcp: Replace open-coded list move
      https://git.kernel.org/mkp/scsi/c/6bcb7c171a0c
[7/7] scsi: zfcp: Avoid benign overflow of the Request Queue's free-level
      https://git.kernel.org/mkp/scsi/c/c3bfffa5ec69

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH 0/7] zfcp: cleanups and small changes for 5.9

[Benjamin Block]

On Wed, Jul 08, 2020 at 02:06:45AM -0400, Martin K. Petersen wrote:
> On Fri, 3 Jul 2020 15:19:56 +0200, Benjamin Block wrote:
> 
> > here are some cleanups and small changes for zfcp I'd like to include in
> > 5.9 if possible.
> > 
> > One of the changes touches documentation in `Documentation/scsi/`, so I
> > put Jonathan on To, hope that was correct. I hope you can still pull
> > this all in one go to minimize work. IBM did retire some old URLs and
> > content from our public website, so we have to clean that up in the
> > documentation so there are not dead links. I changed these in the hopes
> > we can minimize documentation churn going forward, just to replace URLs.
> > 
> > [...]
> 
> Applied to 5.9/scsi-queue, thanks!
> 
> [1/7] scsi: zfcp: Use prandom_u32_max() for backoff
>       https://git.kernel.org/mkp/scsi/c/0cd0e57ec858
> [2/7] scsi: zfcp: Fix an outdated comment for zfcp_qdio_send()
>       https://git.kernel.org/mkp/scsi/c/459ad085d87b
> [3/7] scsi: docs: Update outdated link to IBM developerworks
>       https://git.kernel.org/mkp/scsi/c/b9789bfbfe9d
> [4/7] scsi: docs: Remove invalid link and update text for zfcp kernel config
>       https://git.kernel.org/mkp/scsi/c/c06de6e28c9e
> [5/7] scsi: zfcp: Clean up zfcp_erp_action_ready()
>       https://git.kernel.org/mkp/scsi/c/b43cdb5ac856
> [6/7] scsi: zfcp: Replace open-coded list move
>       https://git.kernel.org/mkp/scsi/c/6bcb7c171a0c
> [7/7] scsi: zfcp: Avoid benign overflow of the Request Queue's free-level
>       https://git.kernel.org/mkp/scsi/c/c3bfffa5ec69
> 

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