| 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 |
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, > 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 --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)