| Message ID | 1443523658-87622-34-git-send-email-hare@suse.de (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
Hi Hannes,
[auto build test results on v4.3-rc3 -- if it's inappropriate base, please ignore]
reproduce:
# apt-get install sparse
make ARCH=x86_64 allmodconfig
make C=1 CF=-D__CHECK_ENDIAN__
sparse warnings: (new ones prefixed by >>)
>> drivers/scsi/scsi_sysfs.c:98:12: sparse: symbol 'scsi_access_state_name' was not declared. Should it be static?
drivers/scsi/scsi_sysfs.c:230:25: sparse: symbol 'dev_attr_hstate' was not declared. Should it be static?
drivers/scsi/scsi_sysfs.c:405:24: sparse: symbol 'scsi_shost_attr_group' was not declared. Should it be static?
drivers/scsi/scsi_sysfs.c:806:1: sparse: incompatible types in comparison expression (different address spaces)
drivers/scsi/scsi_sysfs.c:807:1: sparse: incompatible types in comparison expression (different address spaces)
Please review and possibly fold the followup patch.
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
On 09/29/2015 03:47 AM, Hannes Reinecke wrote: > +static const struct { > + enum scsi_access_state value; > + char *name; Had you considered to use "const char *" here instead of "char *" ? > +const char *scsi_access_state_name(enum scsi_access_state state) > +{ > + int i; > + char *name = NULL; > + > + for (i = 0; i < ARRAY_SIZE(sdev_access_states); i++) { > + if (sdev_access_states[i].value == state) { > + name = sdev_access_states[i].name; > + break; > + } > + } > + return name; > +} The return value of this function is passed without further processing to the format specifier "%s". How about initializing "name" to "(?)" to avoid that "(null)" is printed if the access state is not recognized ? > +static ssize_t > +sdev_show_access_state(struct device *dev, > + struct device_attribute *attr, > + char *buf) > +{ > + struct scsi_device *sdev = to_scsi_device(dev); > + enum scsi_access_state access_state; > + bool pref = false; > + > + if (sdev->access_state & SCSI_ACCESS_STATE_PREFERRED) > + pref = true; > + > + access_state = (sdev->access_state & SCSI_ACCESS_STATE_MASK); > + > + return snprintf(buf, 32, "%s%s\n", > + scsi_access_state_name(access_state), > + pref ? " preferred" :""); > +} Shouldn't this function check whether a device handler has been associated with sdev and also whether the active device handler is the ALUA device handler ? Otherwise incorrect data will be reported before the ALUA device handler has been loaded, after it has been unloaded or if another device handler is active. Additionally, please insert a separator between the preferred state and the access state name. Had you considered to use PAGE_SIZE instead of "32" as output buffer length ? Magic constants in code always make the reader wonder where these come from ... How about reporting the target port group ID next to the ALUA state ? I think that data would be very useful because it allows users to verify which LUNs have been associated with which port group by this patch series. Thanks, Bart. -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c index 5d3e2ae..76e771e 100644 --- a/drivers/scsi/scsi_scan.c +++ b/drivers/scsi/scsi_scan.c @@ -230,6 +230,7 @@ static struct scsi_device *scsi_alloc_sdev(struct scsi_target *starget, sdev->lun = lun; sdev->channel = starget->channel; sdev->sdev_state = SDEV_CREATED; + sdev->access_state = SCSI_ACCESS_STATE_UNKNOWN; INIT_LIST_HEAD(&sdev->siblings); INIT_LIST_HEAD(&sdev->same_target_siblings); INIT_LIST_HEAD(&sdev->cmd_list); diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c index 37799dc..38e157a 100644 --- a/drivers/scsi/scsi_sysfs.c +++ b/drivers/scsi/scsi_sysfs.c @@ -81,6 +81,34 @@ const char *scsi_host_state_name(enum scsi_host_state state) return name; } +static const struct { + enum scsi_access_state value; + char *name; +} sdev_access_states[] = { + { SCSI_ACCESS_STATE_OPTIMAL, "active/optimized" }, + { SCSI_ACCESS_STATE_ACTIVE, "active/non-optimized" }, + { SCSI_ACCESS_STATE_STANDBY, "standby" }, + { SCSI_ACCESS_STATE_UNAVAILABLE, "unavailable" }, + { SCSI_ACCESS_STATE_LBA, "lba-dependent" }, + { SCSI_ACCESS_STATE_OFFLINE, "offline" }, + { SCSI_ACCESS_STATE_TRANSITIONING, "transitioning" }, + { SCSI_ACCESS_STATE_UNKNOWN, "unknown" }, +}; + +const char *scsi_access_state_name(enum scsi_access_state state) +{ + int i; + char *name = NULL; + + for (i = 0; i < ARRAY_SIZE(sdev_access_states); i++) { + if (sdev_access_states[i].value == state) { + name = sdev_access_states[i].name; + break; + } + } + return name; +} + static int check_set(unsigned long long *val, char *src) { char *last; @@ -932,6 +960,26 @@ sdev_store_dh_state(struct device *dev, struct device_attribute *attr, static DEVICE_ATTR(dh_state, S_IRUGO | S_IWUSR, sdev_show_dh_state, sdev_store_dh_state); + +static ssize_t +sdev_show_access_state(struct device *dev, + struct device_attribute *attr, + char *buf) +{ + struct scsi_device *sdev = to_scsi_device(dev); + enum scsi_access_state access_state; + bool pref = false; + + if (sdev->access_state & SCSI_ACCESS_STATE_PREFERRED) + pref = true; + + access_state = (sdev->access_state & SCSI_ACCESS_STATE_MASK); + + return snprintf(buf, 32, "%s%s\n", + scsi_access_state_name(access_state), + pref ? " preferred" :""); +} +static DEVICE_ATTR(access_state, S_IRUGO, sdev_show_access_state, NULL); #endif static ssize_t @@ -1005,6 +1053,7 @@ static struct attribute *scsi_sdev_attrs[] = { &dev_attr_queue_type.attr, #ifdef CONFIG_SCSI_DH &dev_attr_dh_state.attr, + &dev_attr_access_state.attr, #endif &dev_attr_queue_ramp_up_period.attr, REF_EVT(media_change), diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h index cfc23a4..666359f 100644 --- a/include/scsi/scsi_device.h +++ b/include/scsi/scsi_device.h @@ -200,6 +200,7 @@ struct scsi_device { struct scsi_device_handler *handler; void *handler_data; + enum scsi_access_state access_state; enum scsi_device_state sdev_state; unsigned long sdev_data[0]; } __attribute__((aligned(sizeof(unsigned long)))); diff --git a/include/scsi/scsi_proto.h b/include/scsi/scsi_proto.h index a9fbf1b..80e85e7 100644 --- a/include/scsi/scsi_proto.h +++ b/include/scsi/scsi_proto.h @@ -277,5 +277,18 @@ struct scsi_lun { __u8 scsi_lun[8]; }; +/* SPC asymmetric access states */ +enum scsi_access_state { + SCSI_ACCESS_STATE_OPTIMAL = 0, + SCSI_ACCESS_STATE_ACTIVE, + SCSI_ACCESS_STATE_STANDBY, + SCSI_ACCESS_STATE_UNAVAILABLE, + SCSI_ACCESS_STATE_LBA, + SCSI_ACCESS_STATE_OFFLINE = 0xe, + SCSI_ACCESS_STATE_TRANSITIONING = 0xf, + SCSI_ACCESS_STATE_UNKNOWN = 0x70, +}; +#define SCSI_ACCESS_STATE_MASK 0x0f +#define SCSI_ACCESS_STATE_PREFERRED 0x80 #endif /* _SCSI_PROTO_H_ */
Add an 'access_state' attribute to struct scsi_device to display the asymmetric LUN access state. Signed-off-by: Hannes Reinecke <hare@suse.de> --- drivers/scsi/scsi_scan.c | 1 + drivers/scsi/scsi_sysfs.c | 49 ++++++++++++++++++++++++++++++++++++++++++++++ include/scsi/scsi_device.h | 1 + include/scsi/scsi_proto.h | 13 ++++++++++++ 4 files changed, 64 insertions(+)