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

[Cornelia Huck <cohuck@redhat.com>]

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>