> On May 14, 2017, at 11:04 PM, Hannes Reinecke <hare@xxxxxxx> wrote:
>
> On 05/12/2017 09:02 PM, Song Liu wrote:
>>
>>> On May 3, 2017, at 5:50 PM, Song Liu <songliubraving@xxxxxx> wrote:
>>>
>>> This patch adds capability for SCSI layer to generate uevent for SCSI
>>> sense code. The feature is gated by CONFIG_SCSI_SENSE_UEVENT.
>>>
>>> We can configure which sense keys generate uevent for each device
>>> through sysfs entry sense_event_filter. For example, the following
>>> enables uevent for MEDIUM_ERROR (0x03) and HARDWARE_ERROR (0x04)
>>> on scsi drive sdc:
>>>
>>> echo 0x000c > /sys/block/sdc/device/sense_event_filter
>>>
>>> Here is an example output captured by udevadm:
>>>
>>> KERNEL[1214.945358] change /devices/pci0000:00/XXXXXXX
>>> ACTION=change
>>> DEVPATH=/devices/pci0000:00/0000:00:01.0/0000:01:00.0/host6/XXXXXXX
>>> DEVTYPE=scsi_device
>>> DRIVER=sd
>>> LBA=0
>>> MODALIAS=scsi:t-0x00
>>> SDEV_UA=SCSI_SENSE
>>> SENSE_CODE=3/11/14
>>> SEQNUM=4536
>>> SIZE=4096
>>> SUBSYSTEM=scsi
>>>
>>> Signed-off-by: Song Liu <songliubraving@xxxxxx>
>>> ---
>>> drivers/scsi/Kconfig | 14 +++++++++++
>>> drivers/scsi/scsi_error.c | 26 ++++++++++++++++++++
>>> drivers/scsi/scsi_lib.c | 27 +++++++++++++++++++--
>>> drivers/scsi/scsi_sysfs.c | 60 ++++++++++++++++++++++++++++++++++++++++++++++
>>> include/scsi/scsi_device.h | 26 +++++++++++++++++++-
>>> 5 files changed, 150 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/scsi/Kconfig b/drivers/scsi/Kconfig
>>> index 3c52867..4f7f211 100644
>>> --- a/drivers/scsi/Kconfig
>>> +++ b/drivers/scsi/Kconfig
>>> @@ -237,6 +237,20 @@ config SCSI_LOGGING
>>> there should be no noticeable performance impact as long as you have
>>> logging turned off.
>>>
>>> +config SCSI_SENSE_UEVENT
>>> + bool "SCSI sense code logging"
>>> + depends on SCSI
>>> + default n
>>> + ---help---
>>> + This turns on uevent for SCSI sense code.
>>> +
>>> + You can configure which sense keys generate uevent for each device
>>> + through sysfs entry sense_event_filter. For example, the following
>>> + enables uevent for MEDIUM_ERROR (0x03) and HARDWARE_ERROR (0x04)
>>> + on scsi drive sdc:
>>> +
>>> + echo 0x000c > /sys/block/sdc/device/sense_event_filter
>>> +
>>> config SCSI_SCAN_ASYNC
>>> bool "Asynchronous SCSI scanning"
>>> depends on SCSI
>>> diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
>>> index d70c67c..eda150e 100644
>>> --- a/drivers/scsi/scsi_error.c
>>> +++ b/drivers/scsi/scsi_error.c
>>> @@ -426,6 +426,31 @@ static void scsi_report_sense(struct scsi_device *sdev,
>>> }
>>> }
>>>
>>> +/*
>>> + * generate uevent when receiving sense code from device
>>> + */
>>> +static void scsi_send_sense_uevent(struct scsi_device *sdev,
>>> + struct scsi_cmnd *scmd,
>>> + struct scsi_sense_hdr *sshdr)
>>> +{
>>> +#ifdef CONFIG_SCSI_SENSE_UEVENT
>>> + struct scsi_event *evt;
>>> +
>>> + if (!test_bit(sshdr->sense_key & 0xf,
>>> + &sdev->sense_event_filter))
>>> + return;
>>> + evt = sdev_evt_alloc(SDEV_EVT_SCSI_SENSE, GFP_ATOMIC);
>>> + if (!evt)
>>> + return;
>>> +
>>> + evt->sense_evt_data.lba = scsi_get_lba(scmd);
>>> + evt->sense_evt_data.size = blk_rq_bytes(scmd->request);
>>> + memcpy(&evt->sense_evt_data.sshdr, sshdr,
>>> + sizeof(struct scsi_sense_hdr));
>>> + sdev_evt_send(sdev, evt);
>>> +#endif
>>> +}
>>> +
>>> /**
>>> * scsi_check_sense - Examine scsi cmd sense
>>> * @scmd: Cmd to have sense checked.
>>> @@ -446,6 +471,7 @@ int scsi_check_sense(struct scsi_cmnd *scmd)
>>> return FAILED; /* no valid sense data */
>>>
>>> scsi_report_sense(sdev, &sshdr);
>>> + scsi_send_sense_uevent(sdev, scmd, &sshdr);
>>>
>>> if (scsi_sense_is_deferred(&sshdr))
>>> return NEEDS_RETRY;
>>> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
>>> index 95f963b..1095f27 100644
>>> --- a/drivers/scsi/scsi_lib.c
>>> +++ b/drivers/scsi/scsi_lib.c
>>> @@ -2656,8 +2656,9 @@ EXPORT_SYMBOL(scsi_device_set_state);
>>> */
>>> static void scsi_evt_emit(struct scsi_device *sdev, struct scsi_event *evt)
>>> {
>>> - int idx = 0;
>>> - char *envp[3];
>>> + int idx = 0, i;
>>> + char *envp[5]; /* SDEV_EVT_SCSI_SENSE needs most entries (4) */
>>> + int free_envp = -1;
>>>
>>> switch (evt->evt_type) {
>>> case SDEV_EVT_MEDIA_CHANGE:
>>> @@ -2682,6 +2683,23 @@ static void scsi_evt_emit(struct scsi_device *sdev, struct scsi_event *evt)
>>> case SDEV_EVT_ALUA_STATE_CHANGE_REPORTED:
>>> envp[idx++] = "SDEV_UA=ASYMMETRIC_ACCESS_STATE_CHANGED";
>>> break;
>>> +#ifdef CONFIG_SCSI_SENSE_UEVENT
>>> + case SDEV_EVT_SCSI_SENSE:
>>> + envp[idx++] = "SDEV_UA=SCSI_SENSE";
>>> + for (i = idx; i < idx + 3; ++i) {
>>> + envp[i] = kzalloc(32, GFP_ATOMIC);
>>> + if (!envp[i])
>>> + break;
>>> + free_envp = i;
>>> + }
>>> + snprintf(envp[idx++], 32, "LBA=%lu", evt->sense_evt_data.lba);
>>> + snprintf(envp[idx++], 32, "SIZE=%d", evt->sense_evt_data.size);
>>> + snprintf(envp[idx++], 32, "SENSE_CODE=%1x/%02x/%02x",
>>> + evt->sense_evt_data.sshdr.sense_key,
>>> + evt->sense_evt_data.sshdr.asc,
>>> + evt->sense_evt_data.sshdr.ascq);
>>> + break;
>>> +#endif
>>> default:
>>> /* do nothing */
>>> break;
>>> @@ -2690,6 +2708,10 @@ static void scsi_evt_emit(struct scsi_device *sdev, struct scsi_event *evt)
>>> envp[idx++] = NULL;
>>>
>>> kobject_uevent_env(&sdev->sdev_gendev.kobj, KOBJ_CHANGE, envp);
>>> +
>>> + /* no need to free envp[0], so start with i = 1 */
>>> + for (i = 1 ; i < free_envp; ++i)
>>> + kfree(envp[i]);
>>> }
>>>
> This can't be right; you're just allocating envp starting from 'idx',
> but free up all of them.
Yeah, I did some math manually, which is not good. I will fix it with one more
variables (free_start_envp).
>
>>> /**
>>> @@ -2786,6 +2808,7 @@ struct scsi_event *sdev_evt_alloc(enum scsi_device_event evt_type,
>>> case SDEV_EVT_MODE_PARAMETER_CHANGE_REPORTED:
>>> case SDEV_EVT_LUN_CHANGE_REPORTED:
>>> case SDEV_EVT_ALUA_STATE_CHANGE_REPORTED:
>>> + case SDEV_EVT_SCSI_SENSE:
>>> default:
>>> /* do nothing */
>>> break;
>>> diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
>>> index 82dfe07..cfc7380 100644
>>> --- a/drivers/scsi/scsi_sysfs.c
>>> +++ b/drivers/scsi/scsi_sysfs.c
>>> @@ -1072,6 +1072,63 @@ static DEVICE_ATTR(queue_ramp_up_period, S_IRUGO | S_IWUSR,
>>> sdev_show_queue_ramp_up_period,
>>> sdev_store_queue_ramp_up_period);
>>>
>>> +#ifdef CONFIG_SCSI_SENSE_UEVENT
>>> +
>>> +/*
>>> + * SCSI sense key could be 0x00 - 0x08, 0x0a, 0x0b, 0x0d, 0x0e, so the
>>> + * mask is 0x6dff.
>>> + */
>>> +#define SCSI_SENSE_EVENT_FILTER_MASK 0x6dff
>>> +
>>> +static ssize_t
>>> +sdev_show_sense_event_filter(struct device *dev,
>>> + struct device_attribute *attr,
>>> + char *buf)
>>> +{
>>> + struct scsi_device *sdev;
>>> +
>>> + sdev = to_scsi_device(dev);
>>> + return snprintf(buf, 20, "0x%04lx\n",
>>> + (sdev->sense_event_filter &
>>> + SCSI_SENSE_EVENT_FILTER_MASK));
>>> +}
>>> +
>>> +static ssize_t
>>> +sdev_store_sense_event_filter(struct device *dev,
>>> + struct device_attribute *attr,
>>> + const char *buf, size_t count)
>>> +{
>>> + struct scsi_device *sdev = to_scsi_device(dev);
>>> + unsigned long filter;
>>> + int i;
>>> +
>>> + if (buf[0] == '0' && buf[1] == 'x') {
>>> + if (kstrtoul(buf + 2, 16, &filter))
>>> + return -EINVAL;
>>> + } else
>>> + if (kstrtoul(buf, 10, &filter))
>>> + return -EINVAL;
>>> +
>>> + /*
>>> + * Accurate mask for all sense keys is 0x6dff. However, we allow
>>> + * user to enable event for all sense keys by echoing 0xffff
>>> + */
>>> + if ((filter & 0xffff) != filter)
>>> + return -EINVAL;
>>> +
>>> + for (i = 0; i < 15; i++)
>>> + if (filter & SCSI_SENSE_EVENT_FILTER_MASK & (1 << i))
>>> + set_bit(i, &sdev->sense_event_filter);
>>> + else
>>> + clear_bit(i, &sdev->sense_event_filter);
>>> + return count;
>>> +}
>>> +
>>> +static DEVICE_ATTR(sense_event_filter, 0644,
>>> + sdev_show_sense_event_filter,
>>> + sdev_store_sense_event_filter);
>>> +#endif
>>> +
>>> static umode_t scsi_sdev_attr_is_visible(struct kobject *kobj,
>>> struct attribute *attr, int i)
>>> {
>>> @@ -1142,6 +1199,9 @@ static struct attribute *scsi_sdev_attrs[] = {
>>> &dev_attr_preferred_path.attr,
>>> #endif
>>> &dev_attr_queue_ramp_up_period.attr,
>>> +#ifdef CONFIG_SCSI_SENSE_UEVENT
>>> + &dev_attr_sense_event_filter.attr,
>>> +#endif
>>> REF_EVT(media_change),
>>> REF_EVT(inquiry_change_reported),
>>> REF_EVT(capacity_change_reported),
>>> diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
>>> index 05641ae..d160bd7 100644
>>> --- a/include/scsi/scsi_device.h
>>> +++ b/include/scsi/scsi_device.h
>>> @@ -64,13 +64,23 @@ enum scsi_device_event {
>>> SDEV_EVT_MODE_PARAMETER_CHANGE_REPORTED, /* 2A 01 UA reported */
>>> SDEV_EVT_LUN_CHANGE_REPORTED, /* 3F 0E UA reported */
>>> SDEV_EVT_ALUA_STATE_CHANGE_REPORTED, /* 2A 06 UA reported */
>>> + SDEV_EVT_SCSI_SENSE,
>>>
>>> SDEV_EVT_FIRST = SDEV_EVT_MEDIA_CHANGE,
>>> - SDEV_EVT_LAST = SDEV_EVT_ALUA_STATE_CHANGE_REPORTED,
>>> + SDEV_EVT_LAST = SDEV_EVT_SCSI_SENSE,
>>>
>>> SDEV_EVT_MAXBITS = SDEV_EVT_LAST + 1
>>> };
>>>
>>> +#ifdef CONFIG_SCSI_SENSE_UEVENT
>>> +/* data for for SDEV_EVT_SCSI_SENSE */
>>> +struct scsi_sense_uevent_data {
>>> + sector_t lba; /* LBA from the scsi command */
>>> + int size; /* size of the request */
>>> + struct scsi_sense_hdr sshdr;
>>> +};
>>> +#endif
>>> +
>>> struct scsi_event {
>>> enum scsi_device_event evt_type;
>>> struct list_head node;
>>> @@ -78,6 +88,11 @@ struct scsi_event {
>>> /* put union of data structures, for non-simple event types,
>>> * here
>>> */
>>> + union {
>>> +#ifdef CONFIG_SCSI_SENSE_UEVENT
>>> + struct scsi_sense_uevent_data sense_evt_data;
>>> +#endif
>>> + };
>>> };
>>>
>>> struct scsi_device {
>>> @@ -197,6 +212,15 @@ struct scsi_device {
>>> atomic_t iodone_cnt;
>>> atomic_t ioerr_cnt;
>>>
>>> +#ifdef CONFIG_SCSI_SENSE_UEVENT
>>> + /*
>>> + * filter of sense code uevent
>>> + * setting bit X (0x00 - 0x0e) of sense_event_filter enables
>>> + * uevent for sense key X
>>> + */
>>> + unsigned long sense_event_filter;
>>> +#endif
>>> +
>>> struct device sdev_gendev,
>>> sdev_dev;
>>>
>>> --
>>> 2.9.3
>>
> In general I like the idea; however, the 'filter' thingie is somewhat
> odd. I could somewhat buy into the idea of filtering for sense keys, but
> then I would have expected to use the 'sense_event_filter' as a bitmap
> of allowed sense keys.
> With your current design you can only filter for one specific sense key,
> _and_ you leave the remaining bits of the sense_event_filter variable
> unused.
> So either turn it into a bitmap and allow for several sense keys to be
> filtered, or treat it as mask for the _entire_ sense, but then you'd
> need for ASC and ASCQ, too.
Maybe I didn't explain/implement this cleanly, but my original design is
to use the filter as a bitmap:
>>> + if (!test_bit(sshdr->sense_key & 0xf,
>>> + &sdev->sense_event_filter))
>>> + return;
If the filter is set to 0x000f, the code will generate event for sense
keys 0, 1, 2, and 3, but not other sense keys.
Thanks,
Song
