diff mbox series

scsi: scsi_transport_fc: use %u for dev_loss_tmo

Message ID 20220902131519.16513-1-mwilck@suse.com (mailing list archive)
State Accepted
Headers show
Series scsi: scsi_transport_fc: use %u for dev_loss_tmo | expand

Commit Message

Martin Wilck Sept. 2, 2022, 1:15 p.m. UTC
From: Martin Wilck <mwilck@suse.com>

dev_loss_tmo is an unsigned value. Using "%d" as output format
causes irritating negative values to be shown in sysfs.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 drivers/scsi/scsi_transport_fc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Steffen Maier Sept. 5, 2022, 5:22 p.m. UTC | #1
On 9/2/22 15:15, mwilck@suse.com wrote:
> From: Martin Wilck <mwilck@suse.com>
> 
> dev_loss_tmo is an unsigned value. Using "%d" as output format
> causes irritating negative values to be shown in sysfs.
> 

Yes, I had also noticed this a while ago and took the following notes along 
with the workaround clamping -1 to the old max value in my automated zfcp test 
script in user space:

read val < ${fcrportsys}/dev_loss_tmo
# 
https://github.com/opensvc/multipath-tools/commit/d01ccd33fca99884baeb7da037f729e6058c03ef
# ("libmultipath: dev_loss_tmo is unsigned") changing MAX_DEV_LOSS_TMO from 
0x7FFFFFFF==2147483647 to UINT_MAX
# on top of the older
# 
https://github.com/opensvc/multipath-tools/commit/15e5070fdc22cde2dd2a9f4a8022e124e9425bd9
# ("libmultipath: ensure dev_loss_tmo will be update to MAX_DEV_LOSS_TMO if 
no_path_retry set to queue")
# vvv
# The deamon seems to complain:
# multipathd[...]: rport-1:0-3: Cannot parse dev_loss_tmo attribute '-1'
# ^^^
[ "$val" = "-1" ] && val=2147483647


Even though I don't care too much about the default value source in fc_host, I 
wonder if we should also fix this for consistency:
> fc_private_host_show_function(dev_loss_tmo, "%d\n", 20, );
=> fc_private_host_show_function(dev_loss_tmo, "%u\n", 20, );
> static FC_DEVICE_ATTR(host, dev_loss_tmo, S_IRUGO | S_IWUSR,
> 		      show_fc_host_dev_loss_tmo,
> 		      store_fc_private_host_dev_loss_tmo);

After all, the other default value source is already a proper unsigned int:
> /*
>  * dev_loss_tmo: the default number of seconds that the FC transport
>  *   should insulate the loss of a remote port.
>  *   The maximum will be capped by the value of SCSI_DEVICE_BLOCK_MAX_TIMEOUT.
>  */
> static unsigned int fc_dev_loss_tmo = 60;		/* seconds */
> 
> module_param_named(dev_loss_tmo, fc_dev_loss_tmo, uint, S_IRUGO|S_IWUSR);
> MODULE_PARM_DESC(dev_loss_tmo,
> 		 "Maximum number of seconds that the FC transport should"
> 		 " insulate the loss of a remote port. Once this value is"
> 		 " exceeded, the scsi target is removed. Value should be"
> 		 " between 1 and SCSI_DEVICE_BLOCK_MAX_TIMEOUT if"
> 		 " fast_io_fail_tmo is not set.");



Reviewed-by: Steffen Maier <maier@linux.ibm.com>

> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>   drivers/scsi/scsi_transport_fc.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/scsi/scsi_transport_fc.c b/drivers/scsi/scsi_transport_fc.c
> index a2524106206db..df4aa4a5f83c4 100644
> --- a/drivers/scsi/scsi_transport_fc.c
> +++ b/drivers/scsi/scsi_transport_fc.c
> @@ -1170,7 +1170,7 @@ static int fc_rport_set_dev_loss_tmo(struct fc_rport *rport,
>   	return 0;
>   }
> 
> -fc_rport_show_function(dev_loss_tmo, "%d\n", 20, )
> +fc_rport_show_function(dev_loss_tmo, "%u\n", 20, )
>   static ssize_t
>   store_fc_rport_dev_loss_tmo(struct device *dev, struct device_attribute *attr,
>   			    const char *buf, size_t count)
Martin K. Petersen Sept. 16, 2022, 2:37 a.m. UTC | #2
Martin,

> dev_loss_tmo is an unsigned value. Using "%d" as output format causes
> irritating negative values to be shown in sysfs.

Applied to 6.1/scsi-staging, thanks!
diff mbox series

Patch

diff --git a/drivers/scsi/scsi_transport_fc.c b/drivers/scsi/scsi_transport_fc.c
index a2524106206db..df4aa4a5f83c4 100644
--- a/drivers/scsi/scsi_transport_fc.c
+++ b/drivers/scsi/scsi_transport_fc.c
@@ -1170,7 +1170,7 @@  static int fc_rport_set_dev_loss_tmo(struct fc_rport *rport,
 	return 0;
 }
 
-fc_rport_show_function(dev_loss_tmo, "%d\n", 20, )
+fc_rport_show_function(dev_loss_tmo, "%u\n", 20, )
 static ssize_t
 store_fc_rport_dev_loss_tmo(struct device *dev, struct device_attribute *attr,
 			    const char *buf, size_t count)