| Message ID | 1602732462-10443-7-git-send-email-muneendra.kumar@broadcom.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | scsi: Support to handle Intermittent errors | expand |
Hi Muneendra,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on scsi/for-next]
[also build test WARNING on mkp-scsi/for-next next-20201015]
[cannot apply to v5.9]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Muneendra/scsi-Support-to-handle-Intermittent-errors/20201015-182315
base: https://git.kernel.org/pub/scm/linux/kernel/git/jejb/scsi.git for-next
config: m68k-allmodconfig (attached as .config)
compiler: m68k-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/6bb032ada0c283207bbbf0d17a5d8091f49ebb03
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Muneendra/scsi-Support-to-handle-Intermittent-errors/20201015-182315
git checkout 6bb032ada0c283207bbbf0d17a5d8091f49ebb03
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=m68k
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
In file included from include/linux/kernel.h:11,
from include/linux/list.h:9,
from include/linux/module.h:12,
from drivers/scsi/scsi_transport_fc.c:11:
include/linux/scatterlist.h: In function 'sg_set_buf':
arch/m68k/include/asm/page_mm.h:169:49: warning: ordered comparison of pointer with null pointer [-Wextra]
169 | #define virt_addr_valid(kaddr) ((void *)(kaddr) >= (void *)PAGE_OFFSET && (void *)(kaddr) < high_memory)
| ^~
include/linux/compiler.h:78:42: note: in definition of macro 'unlikely'
78 | # define unlikely(x) __builtin_expect(!!(x), 0)
| ^
include/linux/scatterlist.h:143:2: note: in expansion of macro 'BUG_ON'
143 | BUG_ON(!virt_addr_valid(buf));
| ^~~~~~
include/linux/scatterlist.h:143:10: note: in expansion of macro 'virt_addr_valid'
143 | BUG_ON(!virt_addr_valid(buf));
| ^~~~~~~~~~~~~~~
drivers/scsi/scsi_transport_fc.c: In function 'fc_rport_set_marginal_state':
>> drivers/scsi/scsi_transport_fc.c:982:6: warning: variable 'ret' set but not used [-Wunused-but-set-variable]
982 | int ret = 0;
| ^~~
vim +/ret +982 drivers/scsi/scsi_transport_fc.c
969
970 /*
971 * Sets port_state to Marginal/Online.
972 * On Marginal it Sets no retries on abort in scmd->state for all
973 * outstanding io of all the scsi_devs
974 * This only allows ONLINE->MARGINAL and MARGINAL->ONLINE
975 */
976 static ssize_t fc_rport_set_marginal_state(struct device *dev,
977 struct device_attribute *attr,
978 const char *buf, size_t count)
979 {
980 struct fc_rport *rport = transport_class_to_rport(dev);
981 enum fc_port_state port_state;
> 982 int ret = 0;
983
984 ret = get_fc_port_state_match(buf, &port_state);
985
986 if (port_state == FC_PORTSTATE_MARGINAL) {
987 /*
988 * Change the state to marginal only if the
989 * current rport state is Online
990 * Allow only Online->marginal
991 */
992 if (rport->port_state == FC_PORTSTATE_ONLINE) {
993 rport->port_state = port_state;
994 scsi_target_chg_noretries_abort(&rport->dev, 1);
995 }
996 } else if (port_state == FC_PORTSTATE_ONLINE) {
997 /*
998 * Change the state to Online only if the
999 * current rport state is Marginal
1000 * Allow only MArginal->Online
1001 */
1002 if (rport->port_state == FC_PORTSTATE_MARGINAL) {
1003 rport->port_state = port_state;
1004 scsi_target_chg_noretries_abort(&rport->dev, 0);
1005 }
1006 } else
1007 return -EINVAL;
1008 return count;
1009 }
1010
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
On 10/14/20 10:27 PM, Muneendra wrote: > Added a store functionality to set rport port_state using sysfs > under fc_remote_ports/rport-*/port_state > > With this functionality the user can move the port_state from > Marginal->Online and Online->Marginal. > > On Marginal :This interface will set SCMD_NORETRIES_ABORT bit in > scmd->state for all the io's on the scsi device associated > with remote port. > > On Online :This interface will clear SCMD_NORETRIES_ABORT bit in > scmd->state for all the pending io's on the scsi device associated > with remote port. > > Below is the interface provided to set the port state to Marginal > and Online. > > echo "Marginal" >> /sys/class/fc_remote_ports/rport-X\:Y-Z/port_state > echo "Online" >> /sys/class/fc_remote_ports/rport-X\:Y-Z/port_state > > Signed-off-by: Muneendra <muneendra.kumar@broadcom.com> > > --- > v3: > Removed the port_state from starget attributes. > Enabled the store functionality for port_state under remote port. > used the starget_for_each_device to traverse around all the devices > under rport > > v2: > Changed from a noretries_abort attribute under fc_transport/target*/ to > port_state for changing the port_state to a marginal state > --- > drivers/scsi/scsi_transport_fc.c | 85 +++++++++++++++++++++++++++++++- > 1 file changed, 83 insertions(+), 2 deletions(-) > > diff --git a/drivers/scsi/scsi_transport_fc.c b/drivers/scsi/scsi_transport_fc.c > index df66a51d62e6..ac5283b645a6 100644 > --- a/drivers/scsi/scsi_transport_fc.c > +++ b/drivers/scsi/scsi_transport_fc.c > @@ -943,7 +943,88 @@ show_fc_rport_roles (struct device *dev, struct device_attribute *attr, > static FC_DEVICE_ATTR(rport, roles, S_IRUGO, > show_fc_rport_roles, NULL); > > -fc_private_rport_rd_enum_attr(port_state, FC_PORTSTATE_MAX_NAMELEN); > +static void > +device_chg_noretries_abort(struct scsi_device *sdev, void *data) > +{ > + scsi_chg_noretries_abort_io_device(sdev, *(bool *)data); > +} > + > +static int > +target_chg_noretries_abort(struct device *dev, void *data) > +{ > + if (scsi_is_target_device(dev)) > + starget_for_each_device(to_scsi_target(dev), data, > + device_chg_noretries_abort); > + return 0; > +} > + > +static void scsi_target_chg_noretries_abort(struct device *dev, bool set) > +{ > + if (scsi_is_target_device(dev)) > + starget_for_each_device(to_scsi_target(dev), &set, > + device_chg_noretries_abort); > + else > + device_for_each_child(dev, &set, target_chg_noretries_abort); > +} > + > +/* > + * Sets port_state to Marginal/Online. > + * On Marginal it Sets no retries on abort in scmd->state for all > + * outstanding io of all the scsi_devs > + * This only allows ONLINE->MARGINAL and MARGINAL->ONLINE The above comments are not needed since not counting comments the code is almost the same number of lines as the comments. Plus the comments below say the same thing. And the functions/fields/variables you are using below are pretty clear in what they are doing. > + */ > +static ssize_t fc_rport_set_marginal_state(struct device *dev, > + struct device_attribute *attr, > + const char *buf, size_t count) > +{ > + struct fc_rport *rport = transport_class_to_rport(dev); > + enum fc_port_state port_state; > + int ret = 0; > + > + ret = get_fc_port_state_match(buf, &port_state); The kernel test robot mentioned this. > + > + if (port_state == FC_PORTSTATE_MARGINAL) { > + /* > + * Change the state to marginal only if the > + * current rport state is Online > + * Allow only Online->marginal > + */ > + if (rport->port_state == FC_PORTSTATE_ONLINE) { > + rport->port_state = port_state; > + scsi_target_chg_noretries_abort(&rport->dev, 1); > + } Should this return a failure if port_state is not online? > + } else if (port_state == FC_PORTSTATE_ONLINE) { > + /* > + * Change the state to Online only if the > + * current rport state is Marginal > + * Allow only MArginal->Online > + */ > + if (rport->port_state == FC_PORTSTATE_MARGINAL) { > + rport->port_state = port_state; > + scsi_target_chg_noretries_abort(&rport->dev, 0); > + } Same here. > + } else > + return -EINVAL; > + return count; > +} > + > +static ssize_t > +show_fc_rport_port_state(struct device *dev, > + struct device_attribute *attr, char *buf) > +{ > + const char *name; > + struct fc_rport *rport = transport_class_to_rport(dev); > + > + name = get_fc_port_state_name(rport->port_state); > + if (!name) > + return -EINVAL; > + > + return snprintf(buf, 20, "%s\n", name); > +} > + > +static FC_DEVICE_ATTR(rport, port_state, 0444 | 0200, > + show_fc_rport_port_state, fc_rport_set_marginal_state); > + > fc_private_rport_rd_attr(scsi_target_id, "%d\n", 20); > > /* > @@ -2266,7 +2347,7 @@ fc_attach_transport(struct fc_function_template *ft) > SETUP_PRIVATE_RPORT_ATTRIBUTE_RD(port_name); > SETUP_PRIVATE_RPORT_ATTRIBUTE_RD(port_id); > SETUP_PRIVATE_RPORT_ATTRIBUTE_RD(roles); > - SETUP_PRIVATE_RPORT_ATTRIBUTE_RD(port_state); > + SETUP_PRIVATE_RPORT_ATTRIBUTE_RW(port_state); > SETUP_PRIVATE_RPORT_ATTRIBUTE_RD(scsi_target_id); > SETUP_PRIVATE_RPORT_ATTRIBUTE_RW(fast_io_fail_tmo); > >
HI Mike, Thanks for the review. > +/* > + * Sets port_state to Marginal/Online. > + * On Marginal it Sets no retries on abort in scmd->state for all > + * outstanding io of all the scsi_devs > + * This only allows ONLINE->MARGINAL and MARGINAL->ONLINE The above comments are not needed since not counting comments the code is almost the same number of lines as the comments. Plus the comments below say the same thing. And the functions/fields/variables you are using below are pretty clear in what they are doing. [Munendra] Agreed. > + if (rport->port_state == FC_PORTSTATE_ONLINE) { > + rport->port_state = port_state; > + scsi_target_chg_noretries_abort(&rport->dev, 1); > + } Should this return a failure if port_state is not online? [Muneendra]Agreed. Thanks for pointing it .I will add these changes in my next version. Regards, Muneendra.
diff --git a/drivers/scsi/scsi_transport_fc.c b/drivers/scsi/scsi_transport_fc.c index df66a51d62e6..ac5283b645a6 100644 --- a/drivers/scsi/scsi_transport_fc.c +++ b/drivers/scsi/scsi_transport_fc.c @@ -943,7 +943,88 @@ show_fc_rport_roles (struct device *dev, struct device_attribute *attr, static FC_DEVICE_ATTR(rport, roles, S_IRUGO, show_fc_rport_roles, NULL); -fc_private_rport_rd_enum_attr(port_state, FC_PORTSTATE_MAX_NAMELEN); +static void +device_chg_noretries_abort(struct scsi_device *sdev, void *data) +{ + scsi_chg_noretries_abort_io_device(sdev, *(bool *)data); +} + +static int +target_chg_noretries_abort(struct device *dev, void *data) +{ + if (scsi_is_target_device(dev)) + starget_for_each_device(to_scsi_target(dev), data, + device_chg_noretries_abort); + return 0; +} + +static void scsi_target_chg_noretries_abort(struct device *dev, bool set) +{ + if (scsi_is_target_device(dev)) + starget_for_each_device(to_scsi_target(dev), &set, + device_chg_noretries_abort); + else + device_for_each_child(dev, &set, target_chg_noretries_abort); +} + +/* + * Sets port_state to Marginal/Online. + * On Marginal it Sets no retries on abort in scmd->state for all + * outstanding io of all the scsi_devs + * This only allows ONLINE->MARGINAL and MARGINAL->ONLINE + */ +static ssize_t fc_rport_set_marginal_state(struct device *dev, + struct device_attribute *attr, + const char *buf, size_t count) +{ + struct fc_rport *rport = transport_class_to_rport(dev); + enum fc_port_state port_state; + int ret = 0; + + ret = get_fc_port_state_match(buf, &port_state); + + if (port_state == FC_PORTSTATE_MARGINAL) { + /* + * Change the state to marginal only if the + * current rport state is Online + * Allow only Online->marginal + */ + if (rport->port_state == FC_PORTSTATE_ONLINE) { + rport->port_state = port_state; + scsi_target_chg_noretries_abort(&rport->dev, 1); + } + } else if (port_state == FC_PORTSTATE_ONLINE) { + /* + * Change the state to Online only if the + * current rport state is Marginal + * Allow only MArginal->Online + */ + if (rport->port_state == FC_PORTSTATE_MARGINAL) { + rport->port_state = port_state; + scsi_target_chg_noretries_abort(&rport->dev, 0); + } + } else + return -EINVAL; + return count; +} + +static ssize_t +show_fc_rport_port_state(struct device *dev, + struct device_attribute *attr, char *buf) +{ + const char *name; + struct fc_rport *rport = transport_class_to_rport(dev); + + name = get_fc_port_state_name(rport->port_state); + if (!name) + return -EINVAL; + + return snprintf(buf, 20, "%s\n", name); +} + +static FC_DEVICE_ATTR(rport, port_state, 0444 | 0200, + show_fc_rport_port_state, fc_rport_set_marginal_state); + fc_private_rport_rd_attr(scsi_target_id, "%d\n", 20); /* @@ -2266,7 +2347,7 @@ fc_attach_transport(struct fc_function_template *ft) SETUP_PRIVATE_RPORT_ATTRIBUTE_RD(port_name); SETUP_PRIVATE_RPORT_ATTRIBUTE_RD(port_id); SETUP_PRIVATE_RPORT_ATTRIBUTE_RD(roles); - SETUP_PRIVATE_RPORT_ATTRIBUTE_RD(port_state); + SETUP_PRIVATE_RPORT_ATTRIBUTE_RW(port_state); SETUP_PRIVATE_RPORT_ATTRIBUTE_RD(scsi_target_id); SETUP_PRIVATE_RPORT_ATTRIBUTE_RW(fast_io_fail_tmo);
Added a store functionality to set rport port_state using sysfs under fc_remote_ports/rport-*/port_state With this functionality the user can move the port_state from Marginal->Online and Online->Marginal. On Marginal :This interface will set SCMD_NORETRIES_ABORT bit in scmd->state for all the io's on the scsi device associated with remote port. On Online :This interface will clear SCMD_NORETRIES_ABORT bit in scmd->state for all the pending io's on the scsi device associated with remote port. Below is the interface provided to set the port state to Marginal and Online. echo "Marginal" >> /sys/class/fc_remote_ports/rport-X\:Y-Z/port_state echo "Online" >> /sys/class/fc_remote_ports/rport-X\:Y-Z/port_state Signed-off-by: Muneendra <muneendra.kumar@broadcom.com> --- v3: Removed the port_state from starget attributes. Enabled the store functionality for port_state under remote port. used the starget_for_each_device to traverse around all the devices under rport v2: Changed from a noretries_abort attribute under fc_transport/target*/ to port_state for changing the port_state to a marginal state --- drivers/scsi/scsi_transport_fc.c | 85 +++++++++++++++++++++++++++++++- 1 file changed, 83 insertions(+), 2 deletions(-)