[PATCH 0/3] s390/dasd patches

[PATCH 0/3] s390/dasd patches

[Stefan Haberland]

Hi Jens,

please see the following three patches that

 - fix a dead code path for DASD
 - fix a memleak in DASD error case
 - fix typo in copyright statement

Regards,
Stefan

Jan Höppner (1):
  s390/dasd/cio: Interpret ccw_device_get_mdc return value correctly

Stefan Haberland (2):
  s390/dasd: fix memleak in path handling error case
  s390/dasd: fix typo in copyright statement

 drivers/s390/block/dasd_eckd.c | 28 +++++++---------------------
 drivers/s390/block/dasd_fba.h  |  2 +-
 drivers/s390/block/dasd_proc.c |  2 +-
 drivers/s390/cio/device_ops.c  |  2 +-
 4 files changed, 10 insertions(+), 24 deletions(-)

-- 
2.17.1

[PATCH 1/3] s390/dasd/cio: Interpret ccw_device_get_mdc return value correctly

[Stefan Haberland]

From: Jan Höppner <hoeppner@linux.ibm.com>

The max data count (mdc) is an unsigned 16-bit integer value as per AR
documentation and is received via ccw_device_get_mdc() for a specific
path mask from the CIO layer. The function itself also always returns a
positive mdc value or 0 in case mdc isn't supported or couldn't be
determined.

Though, the comment for this function describes a negative return value
to indicate failures.

As a result, the DASD device driver interprets the return value of
ccw_device_get_mdc() incorrectly. The error case is essentially a dead
code path.

To fix this behaviour, check explicitly for a return value of 0 and
change the comment for ccw_device_get_mdc() accordingly.

This fix merely enables the error code path in the DASD functions
get_fcx_max_data() and verify_fcx_max_data(). The actual functionality
stays the same and is still correct.

Signed-off-by: Jan Höppner <hoeppner@linux.ibm.com>
Acked-by: Peter Oberparleiter <oberpar@linux.ibm.com>
Reviewed-by: Stefan Haberland <sth@linux.ibm.com>
Signed-off-by: Stefan Haberland <sth@linux.ibm.com>
---
 drivers/s390/block/dasd_eckd.c | 9 +++++----
 drivers/s390/cio/device_ops.c  | 2 +-
 2 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/s390/block/dasd_eckd.c b/drivers/s390/block/dasd_eckd.c
index c94184d080f8..f5622f4a2ecf 100644
--- a/drivers/s390/block/dasd_eckd.c
+++ b/drivers/s390/block/dasd_eckd.c
@@ -1128,7 +1128,8 @@ static u32 get_fcx_max_data(struct dasd_device *device)
 {
 	struct dasd_eckd_private *private = device->private;
 	int fcx_in_css, fcx_in_gneq, fcx_in_features;
-	int tpm, mdc;
+	unsigned int mdc;
+	int tpm;
 
 	if (dasd_nofcx)
 		return 0;
@@ -1142,7 +1143,7 @@ static u32 get_fcx_max_data(struct dasd_device *device)
 		return 0;
 
 	mdc = ccw_device_get_mdc(device->cdev, 0);
-	if (mdc < 0) {
+	if (mdc == 0) {
 		dev_warn(&device->cdev->dev, "Detecting the maximum supported data size for zHPF requests failed\n");
 		return 0;
 	} else {
@@ -1153,12 +1154,12 @@ static u32 get_fcx_max_data(struct dasd_device *device)
 static int verify_fcx_max_data(struct dasd_device *device, __u8 lpm)
 {
 	struct dasd_eckd_private *private = device->private;
-	int mdc;
+	unsigned int mdc;
 	u32 fcx_max_data;
 
 	if (private->fcx_max_data) {
 		mdc = ccw_device_get_mdc(device->cdev, lpm);
-		if ((mdc < 0)) {
+		if (mdc == 0) {
 			dev_warn(&device->cdev->dev,
 				 "Detecting the maximum data size for zHPF "
 				 "requests failed (rc=%d) for a new path %x\n",
diff --git a/drivers/s390/cio/device_ops.c b/drivers/s390/cio/device_ops.c
index 65841af15748..ccecf6b9504e 100644
--- a/drivers/s390/cio/device_ops.c
+++ b/drivers/s390/cio/device_ops.c
@@ -635,7 +635,7 @@ EXPORT_SYMBOL(ccw_device_tm_start_timeout);
  * @mask: mask of paths to use
  *
  * Return the number of 64K-bytes blocks all paths at least support
- * for a transport command. Return values <= 0 indicate failures.
+ * for a transport command. Return value 0 indicates failure.
  */
 int ccw_device_get_mdc(struct ccw_device *cdev, u8 mask)
 {
-- 
2.17.1

[PATCH 2/3] s390/dasd: fix memleak in path handling error case

[Stefan Haberland]

If for whatever reason the dasd_eckd_check_characteristics() function
exits after at least some paths have their configuration data
allocated those data is never freed again. In the error case the
device->private pointer is set to NULL and dasd_eckd_uncheck_device()
will exit without freeing the path data because of this NULL pointer.

Fix by calling dasd_eckd_clear_conf_data() for error cases.

Also use dasd_eckd_clear_conf_data() in dasd_eckd_uncheck_device()
to avoid code duplication.

Reported-by: Qian Cai <cai@lca.pw>
Reviewed-by: Jan Hoeppner <hoeppner@linux.ibm.com>
Signed-off-by: Stefan Haberland <sth@linux.ibm.com>
---
 drivers/s390/block/dasd_eckd.c | 19 ++-----------------
 1 file changed, 2 insertions(+), 17 deletions(-)

diff --git a/drivers/s390/block/dasd_eckd.c b/drivers/s390/block/dasd_eckd.c
index f5622f4a2ecf..a28b9ff82378 100644
--- a/drivers/s390/block/dasd_eckd.c
+++ b/drivers/s390/block/dasd_eckd.c
@@ -2074,7 +2074,7 @@ dasd_eckd_check_characteristics(struct dasd_device *device)
 	dasd_free_block(device->block);
 	device->block = NULL;
 out_err1:
-	kfree(private->conf_data);
+	dasd_eckd_clear_conf_data(device);
 	kfree(device->private);
 	device->private = NULL;
 	return rc;
@@ -2083,7 +2083,6 @@ dasd_eckd_check_characteristics(struct dasd_device *device)
 static void dasd_eckd_uncheck_device(struct dasd_device *device)
 {
 	struct dasd_eckd_private *private = device->private;
-	int i;
 
 	if (!private)
 		return;
@@ -2093,21 +2092,7 @@ static void dasd_eckd_uncheck_device(struct dasd_device *device)
 	private->sneq = NULL;
 	private->vdsneq = NULL;
 	private->gneq = NULL;
-	private->conf_len = 0;
-	for (i = 0; i < 8; i++) {
-		kfree(device->path[i].conf_data);
-		if ((__u8 *)device->path[i].conf_data ==
-		    private->conf_data) {
-			private->conf_data = NULL;
-			private->conf_len = 0;
-		}
-		device->path[i].conf_data = NULL;
-		device->path[i].cssid = 0;
-		device->path[i].ssid = 0;
-		device->path[i].chpid = 0;
-	}
-	kfree(private->conf_data);
-	private->conf_data = NULL;
+	dasd_eckd_clear_conf_data(device);
 }
 
 static struct dasd_ccw_req *
-- 
2.17.1

[PATCH 3/3] s390/dasd: fix typo in copyright statement

[Stefan Haberland]

coypright -> copyright

Reported-by: Kate Stewart <kstewart@linuxfoundation.org>
Signed-off-by: Stefan Haberland <sth@linux.ibm.com>
---
 drivers/s390/block/dasd_fba.h  | 2 +-
 drivers/s390/block/dasd_proc.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/s390/block/dasd_fba.h b/drivers/s390/block/dasd_fba.h
index 8f75df06e893..45ddabec4017 100644
--- a/drivers/s390/block/dasd_fba.h
+++ b/drivers/s390/block/dasd_fba.h
@@ -2,7 +2,7 @@
 /*
  * Author(s)......: Holger Smolinski <Holger.Smolinski@de.ibm.com>
  * Bugreports.to..: <Linux390@de.ibm.com>
- * Coypright IBM Corp. 1999, 2000
+ * Copyright IBM Corp. 1999, 2000
  *
  */
 
diff --git a/drivers/s390/block/dasd_proc.c b/drivers/s390/block/dasd_proc.c
index 1770b99f607e..8d4d69ea5baf 100644
--- a/drivers/s390/block/dasd_proc.c
+++ b/drivers/s390/block/dasd_proc.c
@@ -5,7 +5,7 @@
  *		    Carsten Otte <Cotte@de.ibm.com>
  *		    Martin Schwidefsky <schwidefsky@de.ibm.com>
  * Bugreports.to..: <Linux390@de.ibm.com>
- * Coypright IBM Corp. 1999, 2002
+ * Copyright IBM Corp. 1999, 2002
  *
  * /proc interface for the dasd driver.
  *
-- 
2.17.1

Re: [PATCH 1/3] s390/dasd/cio: Interpret ccw_device_get_mdc return value correctly

[Cornelia Huck]

On Thu, 19 Dec 2019 09:43:50 +0100
Stefan Haberland <sth@linux.ibm.com> wrote:

> From: Jan Höppner <hoeppner@linux.ibm.com>
> 
> The max data count (mdc) is an unsigned 16-bit integer value as per AR
> documentation and is received via ccw_device_get_mdc() for a specific
> path mask from the CIO layer. The function itself also always returns a
> positive mdc value or 0 in case mdc isn't supported or couldn't be
> determined.
> 
> Though, the comment for this function describes a negative return value
> to indicate failures.

Note that this used to be true before 040495d110ba ("s390/cio: make use
of newly added format 1 channel-path data").

> 
> As a result, the DASD device driver interprets the return value of
> ccw_device_get_mdc() incorrectly. The error case is essentially a dead
> code path.

To be pedantic: It did not check for <= 0 (as documented) either :)

> 
> To fix this behaviour, check explicitly for a return value of 0 and
> change the comment for ccw_device_get_mdc() accordingly.
> 
> This fix merely enables the error code path in the DASD functions
> get_fcx_max_data() and verify_fcx_max_data(). The actual functionality
> stays the same and is still correct.
> 
> Signed-off-by: Jan Höppner <hoeppner@linux.ibm.com>
> Acked-by: Peter Oberparleiter <oberpar@linux.ibm.com>
> Reviewed-by: Stefan Haberland <sth@linux.ibm.com>
> Signed-off-by: Stefan Haberland <sth@linux.ibm.com>
> ---
>  drivers/s390/block/dasd_eckd.c | 9 +++++----
>  drivers/s390/cio/device_ops.c  | 2 +-
>  2 files changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/s390/block/dasd_eckd.c b/drivers/s390/block/dasd_eckd.c
> index c94184d080f8..f5622f4a2ecf 100644
> --- a/drivers/s390/block/dasd_eckd.c
> +++ b/drivers/s390/block/dasd_eckd.c
> @@ -1128,7 +1128,8 @@ static u32 get_fcx_max_data(struct dasd_device *device)
>  {
>  	struct dasd_eckd_private *private = device->private;
>  	int fcx_in_css, fcx_in_gneq, fcx_in_features;
> -	int tpm, mdc;
> +	unsigned int mdc;
> +	int tpm;
>  
>  	if (dasd_nofcx)
>  		return 0;
> @@ -1142,7 +1143,7 @@ static u32 get_fcx_max_data(struct dasd_device *device)
>  		return 0;
>  
>  	mdc = ccw_device_get_mdc(device->cdev, 0);
> -	if (mdc < 0) {
> +	if (mdc == 0) {
>  		dev_warn(&device->cdev->dev, "Detecting the maximum supported data size for zHPF requests failed\n");
>  		return 0;
>  	} else {
> @@ -1153,12 +1154,12 @@ static u32 get_fcx_max_data(struct dasd_device *device)
>  static int verify_fcx_max_data(struct dasd_device *device, __u8 lpm)
>  {
>  	struct dasd_eckd_private *private = device->private;
> -	int mdc;
> +	unsigned int mdc;
>  	u32 fcx_max_data;
>  
>  	if (private->fcx_max_data) {
>  		mdc = ccw_device_get_mdc(device->cdev, lpm);
> -		if ((mdc < 0)) {
> +		if (mdc == 0) {
>  			dev_warn(&device->cdev->dev,
>  				 "Detecting the maximum data size for zHPF "
>  				 "requests failed (rc=%d) for a new path %x\n",
> diff --git a/drivers/s390/cio/device_ops.c b/drivers/s390/cio/device_ops.c
> index 65841af15748..ccecf6b9504e 100644
> --- a/drivers/s390/cio/device_ops.c
> +++ b/drivers/s390/cio/device_ops.c
> @@ -635,7 +635,7 @@ EXPORT_SYMBOL(ccw_device_tm_start_timeout);
>   * @mask: mask of paths to use
>   *
>   * Return the number of 64K-bytes blocks all paths at least support
> - * for a transport command. Return values <= 0 indicate failures.
> + * for a transport command. Return value 0 indicates failure.

s/Return value/The return value/ ?

>   */
>  int ccw_device_get_mdc(struct ccw_device *cdev, u8 mask)
>  {

Reviewed-by: Cornelia Huck <cohuck@redhat.com>

Re: [PATCH 3/3] s390/dasd: fix typo in copyright statement

[Cornelia Huck]

On Thu, 19 Dec 2019 09:43:52 +0100
Stefan Haberland <sth@linux.ibm.com> wrote:

> coypright -> copyright
> 
> Reported-by: Kate Stewart <kstewart@linuxfoundation.org>
> Signed-off-by: Stefan Haberland <sth@linux.ibm.com>
> ---
>  drivers/s390/block/dasd_fba.h  | 2 +-
>  drivers/s390/block/dasd_proc.c | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/s390/block/dasd_fba.h b/drivers/s390/block/dasd_fba.h
> index 8f75df06e893..45ddabec4017 100644
> --- a/drivers/s390/block/dasd_fba.h
> +++ b/drivers/s390/block/dasd_fba.h
> @@ -2,7 +2,7 @@
>  /*
>   * Author(s)......: Holger Smolinski <Holger.Smolinski@de.ibm.com>
>   * Bugreports.to..: <Linux390@de.ibm.com>

No comment on the actual patch; but do you really still want people to
send bug reports to that address?

(A quick grep shows that it is mostly present in the dasd code, but also
in some other code parts.)

> - * Coypright IBM Corp. 1999, 2000
> + * Copyright IBM Corp. 1999, 2000
>   *
>   */
>  
> diff --git a/drivers/s390/block/dasd_proc.c b/drivers/s390/block/dasd_proc.c
> index 1770b99f607e..8d4d69ea5baf 100644
> --- a/drivers/s390/block/dasd_proc.c
> +++ b/drivers/s390/block/dasd_proc.c
> @@ -5,7 +5,7 @@
>   *		    Carsten Otte <Cotte@de.ibm.com>
>   *		    Martin Schwidefsky <schwidefsky@de.ibm.com>
>   * Bugreports.to..: <Linux390@de.ibm.com>
> - * Coypright IBM Corp. 1999, 2002
> + * Copyright IBM Corp. 1999, 2002
>   *
>   * /proc interface for the dasd driver.
>   *

Re: [PATCH 0/3] s390/dasd patches

[Jens Axboe]

On 12/19/19 1:43 AM, Stefan Haberland wrote:
> Hi Jens,
> 
> please see the following three patches that
> 
>  - fix a dead code path for DASD
>  - fix a memleak in DASD error case
>  - fix typo in copyright statement

Applied for 5.5, thanks

-- 
Jens Axboe