[v2,2/4] scsi: scsi_error: Add counters for New Media and Power On/Reset UNIT ATTENTIONs

The purpose of the counters is to enable all ULDs attached to a
device to find out that a New Media or/and Power On/Reset Unit
Attentions has/have been set, even if another ULD catches the
Unit Attention as response to a SCSI command.

The ULDs can read the counters using the scsi_get_ua_new_media_ctr()
and scsi_get_ua_por_ctr() macros (argument pointer to the scsi_device
struct).

Signed-off-by: Kai Mäkisara <Kai.Makisara@kolumbus.fi>
---
 drivers/scsi/scsi_error.c  | 12 ++++++++++++
 include/scsi/scsi_device.h |  9 +++++++++
 2 files changed, 21 insertions(+)

Reviewed-by: John Meneghini <jmeneghi@redhat.com>
Tested-by: John Meneghini <jmeneghi@redhat.com>

On 11/25/24 09:02, Kai Mäkisara wrote:
> The purpose of the counters is to enable all ULDs attached to a
> device to find out that a New Media or/and Power On/Reset Unit
> Attentions has/have been set, even if another ULD catches the
> Unit Attention as response to a SCSI command.
> 
> The ULDs can read the counters using the scsi_get_ua_new_media_ctr()
> and scsi_get_ua_por_ctr() macros (argument pointer to the scsi_device
> struct).
> 
> Signed-off-by: Kai Mäkisara <Kai.Makisara@kolumbus.fi>
> ---
>   drivers/scsi/scsi_error.c  | 12 ++++++++++++
>   include/scsi/scsi_device.h |  9 +++++++++
>   2 files changed, 21 insertions(+)
> 
> diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
> index 10154d78e336..6ef0711c4ec3 100644
> --- a/drivers/scsi/scsi_error.c
> +++ b/drivers/scsi/scsi_error.c
> @@ -547,6 +547,18 @@ enum scsi_disposition scsi_check_sense(struct scsi_cmnd *scmd)
>   
>   	scsi_report_sense(sdev, &sshdr);
>   
> +	if (sshdr.sense_key == UNIT_ATTENTION) {
> +		/*
> +		 * increment the counters for Power on/Reset or New Media so
> +		 * that all ULDs interested in these can see that those have
> +		 * happened, even if someone else gets the sense data.
> +		 */
> +		if (sshdr.asc == 0x28)
> +			scmd->device->ua_new_media_ctr++;
> +		else if (sshdr.asc == 0x29)
> +			scmd->device->ua_por_ctr++;
> +	}
> +
>   	if (scsi_sense_is_deferred(&sshdr))
>   		return NEEDS_RETRY;
>   
> diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
> index 9c540f5468eb..b184a5efc27e 100644
> --- a/include/scsi/scsi_device.h
> +++ b/include/scsi/scsi_device.h
> @@ -247,6 +247,9 @@ struct scsi_device {
>   	unsigned int queue_stopped;	/* request queue is quiesced */
>   	bool offline_already;		/* Device offline message logged */
>   
> +	unsigned char ua_new_media_ctr;	/* Counter for New Media UNIT ATTENTIONs */
> +	unsigned char ua_por_ctr;	/* Counter for Power On / Reset UAs */
> +
>   	atomic_t disk_events_disable_depth; /* disable depth for disk events */
>   
>   	DECLARE_BITMAP(supported_events, SDEV_EVT_MAXBITS); /* supported events */
> @@ -684,6 +687,12 @@ static inline int scsi_device_busy(struct scsi_device *sdev)
>   	return sbitmap_weight(&sdev->budget_map);
>   }
>   
> +/* Macros to access the UNIT ATTENTION counters */
> +#define scsi_get_ua_new_media_ctr(sdev) \
> +	((const unsigned int)(sdev->ua_new_media_ctr))
> +#define scsi_get_ua_por_ctr(sdev) \
> +	((const unsigned int)(sdev->ua_por_ctr))
> +
>   #define MODULE_ALIAS_SCSI_DEVICE(type) \
>   	MODULE_ALIAS("scsi:t-" __stringify(type) "*")
>   #define SCSI_DEVICE_MODALIAS_FMT "scsi:t-0x%02x"

On 11/25/24 6:02 AM, Kai Mäkisara wrote:
> +	unsigned char ua_new_media_ctr;	/* Counter for New Media UNIT ATTENTIONs */
> +	unsigned char ua_por_ctr;	/* Counter for Power On / Reset UAs */

Why unsigned char instead of e.g. u16 or u32? With one of the latter two 
data types, no cast would be necessary in the macros below.

> +/* Macros to access the UNIT ATTENTION counters */
> +#define scsi_get_ua_new_media_ctr(sdev) \
> +	((const unsigned int)(sdev->ua_new_media_ctr))
> +#define scsi_get_ua_por_ctr(sdev) \
> +	((const unsigned int)(sdev->ua_por_ctr))

Please introduce macros in the patch that introduces the first user of 
these macros. I don't see any users of these macros in this patch?

Thanks,

Bart.

> On 12. Dec 2024, at 0.14, Bart Van Assche <bvanassche@acm.org> wrote:
> 
> On 11/25/24 6:02 AM, Kai Mäkisara wrote:
>> + unsigned char ua_new_media_ctr; /* Counter for New Media UNIT ATTENTIONs */
>> + unsigned char ua_por_ctr; /* Counter for Power On / Reset UAs */
> 
> Why unsigned char instead of e.g. u16 or u32? With one of the latter two data types, no cast would be necessary in the macros below.

The purpose was to save memory. (Because of the alignment rules, probably nothing is saved now compared to u16.)
I will change teh counters to u16.

The const casts are there to prevent the users to use the macros to change the counter values.

> 
>> +/* Macros to access the UNIT ATTENTION counters */
>> +#define scsi_get_ua_new_media_ctr(sdev) \
>> + ((const unsigned int)(sdev->ua_new_media_ctr))
>> +#define scsi_get_ua_por_ctr(sdev) \
>> + ((const unsigned int)(sdev->ua_por_ctr))
> 
> Please introduce macros in the patch that introduces the first user of these macros. I don't see any users of these macros in this patch?

The idea was to introduce the mechanism in one patch and the user in the next. But I will move the macros to the next patch.

Thanks,
Kai