[PATCH v3 0/4] scsi: st: scsi_error: More reset patches

[PATCH v3 0/4] scsi: st: scsi_error: More reset patches

[Kai Mäkisara]

The first patch re-applies after device reset some settings changed
by the user (partition, density, block size).

The second and third patch address the case where more than one ULD
access the same device. The Unit Attention (UA) sense data is sent only
to one ULD and the others miss it. The st driver needs to find out
if device reset or media change has happened.

The second patch adds counters for New Media and Power On/Reset (POR)
Unit Attentions to the scsi_device struct. The third one changes st
so that these are used: if the value in the scsi_device struct does
not match the one stored locally, the corresponding UA has happened.
Use of the was_reset flag has been removed.

The fourth patch adds a file to sysfs to tell the user if reads/writes
to a tape are blocked following a device reset.
---
Changes since V2:
- applies to 6.13 + patch "scsi: st: Regression fix: Don't set
  pos_unknown just after device recognition"
- the UA counters changed to u16 (suggested by Bart van Assche)
- the macros to use the counters moved from patch 2 to patch 3
  (suggested by Bart Van Assche)
- remove clearing was_reset in patch 3 and not in patch 1

Changes since V1:
- replace the patch removing was_reset handling with patches two and three
- add sysfs file reset_blocked

Kai Mäkisara (4):
  scsi: st: Restore some drive settings after reset
  scsi: scsi_error: Add counters for New Media and Power On/Reset UNIT
    ATTENTIONs
  scsi: st: scsi_device: Modify st.c to use the new scsi_error counters
  scsi: st: Add sysfs file reset_blocked

 Documentation/scsi/st.rst  |  5 +++
 drivers/scsi/scsi_error.c  | 12 +++++++
 drivers/scsi/st.c          | 73 +++++++++++++++++++++++++++++++++-----
 drivers/scsi/st.h          |  6 ++++
 include/scsi/scsi_device.h |  9 +++++
 5 files changed, 97 insertions(+), 8 deletions(-)

-- 
2.43.0

[PATCH v3 1/4] scsi: st: Restore some drive settings after reset

[Kai Mäkisara]

Some of the allowed operations put the tape into a known position
to continue operation assuming only the tape position has changed.
But reset sets partition, density and block size to drive default
values. These should be restored to the values before reset.

Normally the current block size and density are stored by the drive.
If the settings have been changed, the changed values have to be
saved by the driver across reset.

Signed-off-by: Kai Mäkisara <Kai.Makisara@kolumbus.fi>
---
 drivers/scsi/st.c | 24 +++++++++++++++++++++---
 drivers/scsi/st.h |  2 ++
 2 files changed, 23 insertions(+), 3 deletions(-)

diff --git a/drivers/scsi/st.c b/drivers/scsi/st.c
index ebbd50ec0cda..0fc9afe60862 100644
--- a/drivers/scsi/st.c
+++ b/drivers/scsi/st.c
@@ -952,7 +952,6 @@ static void reset_state(struct scsi_tape *STp)
 		STp->partition = find_partition(STp);
 		if (STp->partition < 0)
 			STp->partition = 0;
-		STp->new_partition = STp->partition;
 	}
 }
 \f
@@ -2930,14 +2929,17 @@ static int st_int_ioctl(struct scsi_tape *STp, unsigned int cmd_in, unsigned lon
 		if (cmd_in == MTSETDENSITY) {
 			(STp->buffer)->b_data[4] = arg;
 			STp->density_changed = 1;	/* At least we tried ;-) */
+			STp->changed_density = arg;
 		} else if (cmd_in == SET_DENS_AND_BLK)
 			(STp->buffer)->b_data[4] = arg >> 24;
 		else
 			(STp->buffer)->b_data[4] = STp->density;
 		if (cmd_in == MTSETBLK || cmd_in == SET_DENS_AND_BLK) {
 			ltmp = arg & MT_ST_BLKSIZE_MASK;
-			if (cmd_in == MTSETBLK)
+			if (cmd_in == MTSETBLK) {
 				STp->blksize_changed = 1; /* At least we tried ;-) */
+				STp->changed_blksize = arg;
+			}
 		} else
 			ltmp = STp->block_size;
 		(STp->buffer)->b_data[9] = (ltmp >> 16);
@@ -3636,9 +3638,25 @@ static long st_ioctl(struct file *file, unsigned int cmd_in, unsigned long arg)
 				retval = (-EIO);
 				goto out;
 			}
-			reset_state(STp);
+			reset_state(STp); /* Clears pos_unknown */
 			/* remove this when the midlevel properly clears was_reset */
 			STp->device->was_reset = 0;
+
+			/* Fix the device settings after reset, ignore errors */
+			if (mtc.mt_op == MTREW || mtc.mt_op == MTSEEK ||
+				mtc.mt_op == MTEOM) {
+				if (STp->can_partitions) {
+					/* STp->new_partition contains the
+					 *  latest partition set
+					 */
+					STp->partition = 0;
+					switch_partition(STp);
+				}
+				if (STp->density_changed)
+					st_int_ioctl(STp, MTSETDENSITY, STp->changed_density);
+				if (STp->blksize_changed)
+					st_int_ioctl(STp, MTSETBLK, STp->changed_blksize);
+			}
 		}
 
 		if (mtc.mt_op != MTNOP && mtc.mt_op != MTSETBLK &&
diff --git a/drivers/scsi/st.h b/drivers/scsi/st.h
index 1aaaf5369a40..6d31b894ee84 100644
--- a/drivers/scsi/st.h
+++ b/drivers/scsi/st.h
@@ -165,6 +165,7 @@ struct scsi_tape {
 	unsigned char compression_changed;
 	unsigned char drv_buffer;
 	unsigned char density;
+	unsigned char changed_density;
 	unsigned char door_locked;
 	unsigned char autorew_dev;   /* auto-rewind device */
 	unsigned char rew_at_close;  /* rewind necessary at close */
@@ -172,6 +173,7 @@ struct scsi_tape {
 	unsigned char cleaning_req;  /* cleaning requested? */
 	unsigned char first_tur;     /* first TEST UNIT READY */
 	int block_size;
+	int changed_blksize;
 	int min_block;
 	int max_block;
 	int recover_count;     /* From tape opening */
-- 
2.43.0

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

[Kai Mäkisara]

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 and see if the values have changed from
the previous check.

Signed-off-by: Kai Mäkisara <Kai.Makisara@kolumbus.fi>
---
 drivers/scsi/scsi_error.c  | 12 ++++++++++++
 include/scsi/scsi_device.h |  3 +++
 2 files changed, 15 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..f5c0f07a053a 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 */
 
+	u16 ua_new_media_ctr;		/* Counter for New Media UNIT ATTENTIONs */
+	u16 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 */
-- 
2.43.0

[PATCH v3 3/4] scsi: st: scsi_device: Modify st.c to use the new scsi_error counters

[Kai Mäkisara]

Compare the stored values of por_ctr and new_media_ctr against
the values in the device struct. In case of mismatch, the
Unit Attention corresponding to the counter has happened.
This is a safeguard against another ULD catching the
Unit Attention sense data.

Macros scsi_get_ua_new_media_ctr and scsi_get_ua_por_ctr are added to
read the current values of the counters.

Signed-off-by: Kai Mäkisara <Kai.Makisara@kolumbus.fi>
---
 drivers/scsi/st.c          | 30 +++++++++++++++++++++++++-----
 drivers/scsi/st.h          |  4 ++++
 include/scsi/scsi_device.h |  6 ++++++
 3 files changed, 35 insertions(+), 5 deletions(-)

diff --git a/drivers/scsi/st.c b/drivers/scsi/st.c
index 0fc9afe60862..6ec1cfeb92ff 100644
--- a/drivers/scsi/st.c
+++ b/drivers/scsi/st.c
@@ -163,9 +163,11 @@ static const char *st_formats[] = {
 
 static int debugging = DEBUG;
 
+/* Setting these non-zero may risk recognizing resets */
 #define MAX_RETRIES 0
 #define MAX_WRITE_RETRIES 0
 #define MAX_READY_RETRIES 0
+
 #define NO_TAPE  NOT_READY
 
 #define ST_TIMEOUT (900 * HZ)
@@ -357,10 +359,18 @@ static int st_chk_result(struct scsi_tape *STp, struct st_request * SRpnt)
 {
 	int result = SRpnt->result;
 	u8 scode;
+	unsigned int ctr;
 	DEB(const char *stp;)
 	char *name = STp->name;
 	struct st_cmdstatus *cmdstatp;
 
+	ctr = scsi_get_ua_por_ctr(STp->device);
+	if (ctr != STp->por_ctr) {
+		STp->por_ctr = ctr;
+		STp->pos_unknown = 1; /* ASC => power on / reset */
+		st_printk(KERN_WARNING, STp, "Power on/reset recognized.");
+	}
+
 	if (!result)
 		return 0;
 
@@ -413,10 +423,11 @@ static int st_chk_result(struct scsi_tape *STp, struct st_request * SRpnt)
 	if (cmdstatp->have_sense &&
 	    cmdstatp->sense_hdr.asc == 0 && cmdstatp->sense_hdr.ascq == 0x17)
 		STp->cleaning_req = 1; /* ASC and ASCQ => cleaning requested */
-	if (cmdstatp->have_sense && scode == UNIT_ATTENTION && cmdstatp->sense_hdr.asc == 0x29)
+	if (cmdstatp->have_sense && scode == UNIT_ATTENTION &&
+		cmdstatp->sense_hdr.asc == 0x29 && !STp->pos_unknown) {
 		STp->pos_unknown = 1; /* ASC => power on / reset */
-
-	STp->pos_unknown |= STp->device->was_reset;
+		st_printk(KERN_WARNING, STp, "Power on/reset recognized.");
+	}
 
 	if (cmdstatp->have_sense &&
 	    scode == RECOVERED_ERROR
@@ -968,6 +979,7 @@ static int test_ready(struct scsi_tape *STp, int do_wait)
 {
 	int attentions, waits, max_wait, scode;
 	int retval = CHKRES_READY, new_session = 0;
+	unsigned int ctr;
 	unsigned char cmd[MAX_COMMAND_SIZE];
 	struct st_request *SRpnt = NULL;
 	struct st_cmdstatus *cmdstatp = &STp->buffer->cmdstat;
@@ -1024,6 +1036,13 @@ static int test_ready(struct scsi_tape *STp, int do_wait)
 			}
 		}
 
+		ctr = scsi_get_ua_new_media_ctr(STp->device);
+		if (ctr != STp->new_media_ctr) {
+			STp->new_media_ctr = ctr;
+			new_session = 1;
+			DEBC_printk(STp, "New tape session.");
+		}
+
 		retval = (STp->buffer)->syscall_result;
 		if (!retval)
 			retval = new_session ? CHKRES_NEW_SESSION : CHKRES_READY;
@@ -3639,8 +3658,6 @@ static long st_ioctl(struct file *file, unsigned int cmd_in, unsigned long arg)
 				goto out;
 			}
 			reset_state(STp); /* Clears pos_unknown */
-			/* remove this when the midlevel properly clears was_reset */
-			STp->device->was_reset = 0;
 
 			/* Fix the device settings after reset, ignore errors */
 			if (mtc.mt_op == MTREW || mtc.mt_op == MTSEEK ||
@@ -4402,6 +4419,9 @@ static int st_probe(struct device *dev)
 		goto out_idr_remove;
 	}
 
+	tpnt->new_media_ctr = scsi_get_ua_new_media_ctr(SDp);
+	tpnt->por_ctr = scsi_get_ua_por_ctr(SDp);
+
 	dev_set_drvdata(dev, tpnt);
 
 
diff --git a/drivers/scsi/st.h b/drivers/scsi/st.h
index 6d31b894ee84..0d7c4b8c2c8a 100644
--- a/drivers/scsi/st.h
+++ b/drivers/scsi/st.h
@@ -179,6 +179,10 @@ struct scsi_tape {
 	int recover_count;     /* From tape opening */
 	int recover_reg;       /* From last status call */
 
+	/* The saved values of midlevel counters */
+	unsigned int new_media_ctr;
+	unsigned int por_ctr;
+
 #if DEBUG
 	unsigned char write_pending;
 	int nbr_finished;
diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index f5c0f07a053a..013018608370 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -687,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"
-- 
2.43.0

[PATCH v3 4/4] scsi: st: Add sysfs file reset_blocked

[Kai Mäkisara]

If the value read from the file is 1, reads and writes from/to the
device are blocked because the tape position may not match user's
expectation (tape rewound after device reset).

Signed-off-by: Kai Mäkisara <Kai.Makisara@kolumbus.fi>
---
 Documentation/scsi/st.rst |  5 +++++
 drivers/scsi/st.c         | 19 +++++++++++++++++++
 2 files changed, 24 insertions(+)

diff --git a/Documentation/scsi/st.rst b/Documentation/scsi/st.rst
index d3b28c28d74c..2209f03faad3 100644
--- a/Documentation/scsi/st.rst
+++ b/Documentation/scsi/st.rst
@@ -157,6 +157,11 @@ enabled driver and mode options. The value in the file is a bit mask where the
 bit definitions are the same as those used with MTSETDRVBUFFER in setting the
 options.
 
+Each directory contains the entry 'reset_blocked'. If this value is one,
+reading and writing to the device is blocked after device reset. Most
+devices rewind the tape after reset and the writes/read don't access the
+tape position the user expects.
+
 A link named 'tape' is made from the SCSI device directory to the class
 directory corresponding to the mode 0 auto-rewind device (e.g., st0).
 
diff --git a/drivers/scsi/st.c b/drivers/scsi/st.c
index 6ec1cfeb92ff..e4fda0954004 100644
--- a/drivers/scsi/st.c
+++ b/drivers/scsi/st.c
@@ -4703,6 +4703,24 @@ options_show(struct device *dev, struct device_attribute *attr, char *buf)
 }
 static DEVICE_ATTR_RO(options);
 
+/**
+ * reset_blocked_show - Value 1 indicates that reads, writes, etc. are blocked
+ * because a device reset has occurred and no operation positioning the tape
+ * has been issued.
+ * @dev: struct device
+ * @attr: attribute structure
+ * @buf: buffer to return formatted data in
+ */
+static ssize_t reset_blocked_show(struct device *dev,
+	struct device_attribute *attr, char *buf)
+{
+	struct st_modedef *STm = dev_get_drvdata(dev);
+	struct scsi_tape *STp = STm->tape;
+
+	return sprintf(buf, "%d", STp->pos_unknown);
+}
+static DEVICE_ATTR_RO(reset_blocked);
+
 /* Support for tape stats */
 
 /**
@@ -4887,6 +4905,7 @@ static struct attribute *st_dev_attrs[] = {
 	&dev_attr_default_density.attr,
 	&dev_attr_default_compression.attr,
 	&dev_attr_options.attr,
+	&dev_attr_reset_blocked.attr,
 	NULL,
 };
 
-- 
2.43.0

Re: [PATCH v3 2/4] scsi: scsi_error: Add counters for New Media and Power On/Reset UNIT ATTENTIONs

[John Meneghini]

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

On 1/20/25 2:49 PM, 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 and see if the values have changed from
> the previous check.
> 
> Signed-off-by: Kai Mäkisara <Kai.Makisara@kolumbus.fi>
> ---
>   drivers/scsi/scsi_error.c  | 12 ++++++++++++
>   include/scsi/scsi_device.h |  3 +++
>   2 files changed, 15 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..f5c0f07a053a 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 */
>   
> +	u16 ua_new_media_ctr;		/* Counter for New Media UNIT ATTENTIONs */
> +	u16 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 */

Re: [PATCH v3 1/4] scsi: st: Restore some drive settings after reset

[John Meneghini]

I've tested this patch out and it works as expected.

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

On 1/20/25 2:49 PM, Kai Mäkisara wrote:
> Some of the allowed operations put the tape into a known position
> to continue operation assuming only the tape position has changed.
> But reset sets partition, density and block size to drive default
> values. These should be restored to the values before reset.
> 
> Normally the current block size and density are stored by the drive.
> If the settings have been changed, the changed values have to be
> saved by the driver across reset.
> 
> Signed-off-by: Kai Mäkisara <Kai.Makisara@kolumbus.fi>

> diff --git a/drivers/scsi/st.c b/drivers/scsi/st.c
> index ebbd50ec0cda..0fc9afe60862 100644
> --- a/drivers/scsi/st.c
> +++ b/drivers/scsi/st.c
>   \f
> @@ -2930,14 +2929,17 @@ static int st_int_ioctl(struct scsi_tape *STp, unsigned int cmd_in, unsigned lon
>   		if (cmd_in == MTSETDENSITY) {
>   			(STp->buffer)->b_data[4] = arg;
>   			STp->density_changed = 1;	/* At least we tried ;-) */
> +			STp->changed_density = arg;
>   		} else if (cmd_in == SET_DENS_AND_BLK)
>   			(STp->buffer)->b_data[4] = arg >> 24;
>   		else
>   			(STp->buffer)->b_data[4] = STp->density;
>   		if (cmd_in == MTSETBLK || cmd_in == SET_DENS_AND_BLK) {
>   			ltmp = arg & MT_ST_BLKSIZE_MASK;
> -			if (cmd_in == MTSETBLK)
> +			if (cmd_in == MTSETBLK) {
>   				STp->blksize_changed = 1; /* At least we tried ;-) */
> +				STp->changed_blksize = arg;
> +			}
>   		} else
>   			ltmp = STp->block_size;
>   		(STp->buffer)->b_data[9] = (ltmp >> 16);
> @@ -3636,9 +3638,25 @@ static long st_ioctl(struct file *file, unsigned int cmd_in, unsigned long arg)
>   				retval = (-EIO);
>   				goto out;
>   			}
> -			reset_state(STp);
> +			reset_state(STp); /* Clears pos_unknown */
>   			/* remove this when the midlevel properly clears was_reset */
>   			STp->device->was_reset = 0;

I see we are still clearing the mid layer was_reset flag here.  Is there any way we can remove this in a future patch?

/John

Re: [PATCH v3 3/4] scsi: st: scsi_device: Modify st.c to use the new scsi_error counters

[John Meneghini]

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

Testing of this patch with my current tests show no regressions.

I'll work on a multi-ULD test to improve those tests.

/John

On 1/20/25 2:49 PM, Kai Mäkisara wrote:
> Compare the stored values of por_ctr and new_media_ctr against
> the values in the device struct. In case of mismatch, the
> Unit Attention corresponding to the counter has happened.
> This is a safeguard against another ULD catching the
> Unit Attention sense data.
> 
> Macros scsi_get_ua_new_media_ctr and scsi_get_ua_por_ctr are added to
> read the current values of the counters.
> 
> Signed-off-by: Kai Mäkisara <Kai.Makisara@kolumbus.fi>
> ---
>   drivers/scsi/st.c          | 30 +++++++++++++++++++++++++-----
>   drivers/scsi/st.h          |  4 ++++
>   include/scsi/scsi_device.h |  6 ++++++
>   3 files changed, 35 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/scsi/st.c b/drivers/scsi/st.c
> index 0fc9afe60862..6ec1cfeb92ff 100644
> --- a/drivers/scsi/st.c
> +++ b/drivers/scsi/st.c
> @@ -163,9 +163,11 @@ static const char *st_formats[] = {
>   
>   static int debugging = DEBUG;
>   
> +/* Setting these non-zero may risk recognizing resets */
>   #define MAX_RETRIES 0
>   #define MAX_WRITE_RETRIES 0
>   #define MAX_READY_RETRIES 0
> +
>   #define NO_TAPE  NOT_READY
>   
>   #define ST_TIMEOUT (900 * HZ)
> @@ -357,10 +359,18 @@ static int st_chk_result(struct scsi_tape *STp, struct st_request * SRpnt)
>   {
>   	int result = SRpnt->result;
>   	u8 scode;
> +	unsigned int ctr;
>   	DEB(const char *stp;)
>   	char *name = STp->name;
>   	struct st_cmdstatus *cmdstatp;
>   
> +	ctr = scsi_get_ua_por_ctr(STp->device);
> +	if (ctr != STp->por_ctr) {
> +		STp->por_ctr = ctr;
> +		STp->pos_unknown = 1; /* ASC => power on / reset */
> +		st_printk(KERN_WARNING, STp, "Power on/reset recognized.");
> +	}
> +
>   	if (!result)
>   		return 0;
>   
> @@ -413,10 +423,11 @@ static int st_chk_result(struct scsi_tape *STp, struct st_request * SRpnt)
>   	if (cmdstatp->have_sense &&
>   	    cmdstatp->sense_hdr.asc == 0 && cmdstatp->sense_hdr.ascq == 0x17)
>   		STp->cleaning_req = 1; /* ASC and ASCQ => cleaning requested */
> -	if (cmdstatp->have_sense && scode == UNIT_ATTENTION && cmdstatp->sense_hdr.asc == 0x29)
> +	if (cmdstatp->have_sense && scode == UNIT_ATTENTION &&
> +		cmdstatp->sense_hdr.asc == 0x29 && !STp->pos_unknown) {
>   		STp->pos_unknown = 1; /* ASC => power on / reset */
> -
> -	STp->pos_unknown |= STp->device->was_reset;
> +		st_printk(KERN_WARNING, STp, "Power on/reset recognized.");
> +	}
>   
>   	if (cmdstatp->have_sense &&
>   	    scode == RECOVERED_ERROR
> @@ -968,6 +979,7 @@ static int test_ready(struct scsi_tape *STp, int do_wait)
>   {
>   	int attentions, waits, max_wait, scode;
>   	int retval = CHKRES_READY, new_session = 0;
> +	unsigned int ctr;
>   	unsigned char cmd[MAX_COMMAND_SIZE];
>   	struct st_request *SRpnt = NULL;
>   	struct st_cmdstatus *cmdstatp = &STp->buffer->cmdstat;
> @@ -1024,6 +1036,13 @@ static int test_ready(struct scsi_tape *STp, int do_wait)
>   			}
>   		}
>   
> +		ctr = scsi_get_ua_new_media_ctr(STp->device);
> +		if (ctr != STp->new_media_ctr) {
> +			STp->new_media_ctr = ctr;
> +			new_session = 1;
> +			DEBC_printk(STp, "New tape session.");
> +		}
> +
>   		retval = (STp->buffer)->syscall_result;
>   		if (!retval)
>   			retval = new_session ? CHKRES_NEW_SESSION : CHKRES_READY;
> @@ -3639,8 +3658,6 @@ static long st_ioctl(struct file *file, unsigned int cmd_in, unsigned long arg)
>   				goto out;
>   			}
>   			reset_state(STp); /* Clears pos_unknown */
> -			/* remove this when the midlevel properly clears was_reset */
> -			STp->device->was_reset = 0;
>   
>   			/* Fix the device settings after reset, ignore errors */
>   			if (mtc.mt_op == MTREW || mtc.mt_op == MTSEEK ||
> @@ -4402,6 +4419,9 @@ static int st_probe(struct device *dev)
>   		goto out_idr_remove;
>   	}
>   
> +	tpnt->new_media_ctr = scsi_get_ua_new_media_ctr(SDp);
> +	tpnt->por_ctr = scsi_get_ua_por_ctr(SDp);
> +
>   	dev_set_drvdata(dev, tpnt);
>   
>   
> diff --git a/drivers/scsi/st.h b/drivers/scsi/st.h
> index 6d31b894ee84..0d7c4b8c2c8a 100644
> --- a/drivers/scsi/st.h
> +++ b/drivers/scsi/st.h
> @@ -179,6 +179,10 @@ struct scsi_tape {
>   	int recover_count;     /* From tape opening */
>   	int recover_reg;       /* From last status call */
>   
> +	/* The saved values of midlevel counters */
> +	unsigned int new_media_ctr;
> +	unsigned int por_ctr;
> +
>   #if DEBUG
>   	unsigned char write_pending;
>   	int nbr_finished;
> diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
> index f5c0f07a053a..013018608370 100644
> --- a/include/scsi/scsi_device.h
> +++ b/include/scsi/scsi_device.h
> @@ -687,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"

Re: [PATCH v3 4/4] scsi: st: Add sysfs file reset_blocked

[John Meneghini]

One suggestion here.

On 1/20/25 2:49 PM, Kai Mäkisara wrote:
> If the value read from the file is 1, reads and writes from/to the
> device are blocked because the tape position may not match user's
> expectation (tape rewound after device reset).
> 
> Signed-off-by: Kai Mäkisara <Kai.Makisara@kolumbus.fi>
> ---
>   Documentation/scsi/st.rst |  5 +++++
>   drivers/scsi/st.c         | 19 +++++++++++++++++++
>   2 files changed, 24 insertions(+)
> 
> diff --git a/Documentation/scsi/st.rst b/Documentation/scsi/st.rst
> index d3b28c28d74c..2209f03faad3 100644
> --- a/Documentation/scsi/st.rst
> +++ b/Documentation/scsi/st.rst
> @@ -157,6 +157,11 @@ enabled driver and mode options. The value in the file is a bit mask where the
>   bit definitions are the same as those used with MTSETDRVBUFFER in setting the
>   options.
>   
> +Each directory contains the entry 'reset_blocked'. If this value is one,

I suggest calling this 'position_lost' or 'position_unknown' rather than 'reset_blocked'.

> +reading and writing to the device is blocked after device reset. Most
> +devices rewind the tape after reset and the writes/read don't access the
> +tape position the user expects.
> +
>   A link named 'tape' is made from the SCSI device directory to the class
>   directory corresponding to the mode 0 auto-rewind device (e.g., st0).
>   
> diff --git a/drivers/scsi/st.c b/drivers/scsi/st.c
> index 6ec1cfeb92ff..e4fda0954004 100644
> --- a/drivers/scsi/st.c
> +++ b/drivers/scsi/st.c
> @@ -4703,6 +4703,24 @@ options_show(struct device *dev, struct device_attribute *attr, char *buf)
>   }
>   static DEVICE_ATTR_RO(options);
>   
> +/**
> + * reset_blocked_show - Value 1 indicates that reads, writes, etc. are blocked

       position_lost_show

> + * because a device reset has occurred and no operation positioning the tape
> + * has been issued.
> + * @dev: struct device
> + * @attr: attribute structure
> + * @buf: buffer to return formatted data in
> + */
> +static ssize_t reset_blocked_show(struct device *dev,
> +	struct device_attribute *attr, char *buf)
> +{
> +	struct st_modedef *STm = dev_get_drvdata(dev);
> +	struct scsi_tape *STp = STm->tape;
> +
> +	return sprintf(buf, "%d", STp->pos_unknown);
                                    ^^^^^^^^^^^^^^

I'd like the name of this function/sysfs variable to more closely match the code
that drives this state.

> +}
> +static DEVICE_ATTR_RO(reset_blocked);
> +
>   /* Support for tape stats */
>   
>   /**
> @@ -4887,6 +4905,7 @@ static struct attribute *st_dev_attrs[] = {
>   	&dev_attr_default_density.attr,
>   	&dev_attr_default_compression.attr,
>   	&dev_attr_options.attr,
> +	&dev_attr_reset_blocked.attr,
>   	NULL,
>   };
>   

Otherwise, I've tested this out and everything works as expected!

https://github.com/johnmeneghini/tape_tests/commit/9e04599605f4726050f9f2032b84df2daa6cd6ea

Thanks!

Re: [PATCH v3 4/4] scsi: st: Add sysfs file reset_blocked

["Kai Mäkisara (Kolumbus)"]

> On 30. Jan 2025, at 20.27, John Meneghini <jmeneghi@redhat.com> wrote:
> 
> One suggestion here.
> 
> On 1/20/25 2:49 PM, Kai Mäkisara wrote:
>> If the value read from the file is 1, reads and writes from/to the

...
>> --- a/Documentation/scsi/st.rst
>> +++ b/Documentation/scsi/st.rst
>> @@ -157,6 +157,11 @@ enabled driver and mode options. The value in the file is a bit mask where the
>>  bit definitions are the same as those used with MTSETDRVBUFFER in setting the
>>  options.
>>  +Each directory contains the entry 'reset_blocked'. If this value is one,
> 
> I suggest calling this 'position_lost' or 'position_unknown' rather than 'reset_blocked'.
> 
>> +reading and writing to the device is blocked after device reset. Most
>>  +/**
...
>> + * reset_blocked_show - Value 1 indicates that reads, writes, etc. are blocked
> 
>      position_lost_show

Yes. The function name should be changed when the file name is changed.

> 
>> + * because a device reset has occurred and no operation positioning the tape
>> + * has been issued.
>> + * @dev: struct device
>> + * @attr: attribute structure
>> + * @buf: buffer to return formatted data in
>> + */
>> +static ssize_t reset_blocked_show(struct device *dev,
>> + struct device_attribute *attr, char *buf)
>> +{
>> + struct st_modedef *STm = dev_get_drvdata(dev);
>> + struct scsi_tape *STp = STm->tape;
>> +
>> + return sprintf(buf, "%d", STp->pos_unknown);
>                                   ^^^^^^^^^^^^^^
> 
> I'd like the name of this function/sysfs variable to more closely match the code
> that drives this state.

I am not too fond of the name "reset_blocked", but I had to choose something :-)
This name is based on the phenomenon causing the situation (device reset) and
what it causes (blocks many st operations).

I think "reset" should be part of the name. Device reset may mean that the tape
position is changed (rewind), but it can also mean more: the device settings are
reset to the saved values. Patch 1/4 restores the values set using st, but it does
not restore the values set by other means (e.g., encryption). The user should
remember to re-set also these before continuing.

Combining the above and your suggestion, what about "position_lost_in_reset"
or "pos_lost_in_reset"? (Whatever the name is, the user should check what
has really happened. The name should point to the correct direction,
but it should be short enough.)

Thanks!

Re: [PATCH v3 4/4] scsi: st: Add sysfs file reset_blocked

[John Meneghini]

On 1/31/25 5:49 AM, "Kai Mäkisara (Kolumbus)" wrote:
> Combining the above and your suggestion, what about "position_lost_in_reset"
> or "pos_lost_in_reset"? (Whatever the name is, the user should check what
> has really happened. The name should point to the correct direction,
> but it should be short enough.)

I like it!

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

[PATCH v3b 4/4] scsi: st: Add sysfs file position_lost_in_reset

[Kai Mäkisara]

If the value read from the file is 1, reads and writes from/to the
device are blocked because the tape position may not match user's
expectation (tape rewound after device reset).

Signed-off-by: Kai Mäkisara <Kai.Makisara@kolumbus.fi>
Reviewed-by: John Meneghini <jmeneghi@redhat.com>
Tested-by: John Meneghini <jmeneghi@redhat.com>
---

Changed in v3b:
- sysfs file name has been changed to position_lost_in_reset

---
 Documentation/scsi/st.rst |  5 +++++
 drivers/scsi/st.c         | 19 +++++++++++++++++++
 2 files changed, 24 insertions(+)

diff --git a/Documentation/scsi/st.rst b/Documentation/scsi/st.rst
index d3b28c28d74c..b4a092faa9c8 100644
--- a/Documentation/scsi/st.rst
+++ b/Documentation/scsi/st.rst
@@ -157,6 +157,11 @@ enabled driver and mode options. The value in the file is a bit mask where the
 bit definitions are the same as those used with MTSETDRVBUFFER in setting the
 options.
 
+Each directory contains the entry 'position_lost_in_reset'. If this value is
+one, reading and writing to the device is blocked after device reset. Most
+devices rewind the tape after reset and the writes/read don't access the
+tape position the user expects.
+
 A link named 'tape' is made from the SCSI device directory to the class
 directory corresponding to the mode 0 auto-rewind device (e.g., st0).
 
diff --git a/drivers/scsi/st.c b/drivers/scsi/st.c
index 6ec1cfeb92ff..85867120c8a9 100644
--- a/drivers/scsi/st.c
+++ b/drivers/scsi/st.c
@@ -4703,6 +4703,24 @@ options_show(struct device *dev, struct device_attribute *attr, char *buf)
 }
 static DEVICE_ATTR_RO(options);
 
+/**
+ * position_lost_in_reset_show - Value 1 indicates that reads, writes, etc.
+ * are blocked because a device reset has occurred and no operation positioning
+ * the tape has been issued.
+ * @dev: struct device
+ * @attr: attribute structure
+ * @buf: buffer to return formatted data in
+ */
+static ssize_t position_lost_in_reset_show(struct device *dev,
+	struct device_attribute *attr, char *buf)
+{
+	struct st_modedef *STm = dev_get_drvdata(dev);
+	struct scsi_tape *STp = STm->tape;
+
+	return sprintf(buf, "%d", STp->pos_unknown);
+}
+static DEVICE_ATTR_RO(position_lost_in_reset);
+
 /* Support for tape stats */
 
 /**
@@ -4887,6 +4905,7 @@ static struct attribute *st_dev_attrs[] = {
 	&dev_attr_default_density.attr,
 	&dev_attr_default_compression.attr,
 	&dev_attr_options.attr,
+	&dev_attr_position_lost_in_reset.attr,
 	NULL,
 };
 
-- 
2.43.0

Re: [PATCH v3 0/4] scsi: st: scsi_error: More reset patches

[Martin K. Petersen]

Kai,

> The first patch re-applies after device reset some settings changed
> by the user (partition, density, block size).

Applied to 6.15/scsi-staging, thanks!

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH v3 0/4] scsi: st: scsi_error: More reset patches

[Martin K. Petersen]

On Mon, 20 Jan 2025 21:49:21 +0200, Kai Mäkisara wrote:

> The first patch re-applies after device reset some settings changed
> by the user (partition, density, block size).
> 
> The second and third patch address the case where more than one ULD
> access the same device. The Unit Attention (UA) sense data is sent only
> to one ULD and the others miss it. The st driver needs to find out
> if device reset or media change has happened.
> 
> [...]

Applied to 6.15/scsi-queue, thanks!

[1/4] scsi: st: Restore some drive settings after reset
      https://git.kernel.org/mkp/scsi/c/7081dc75df79
[2/4] scsi: scsi_error: Add counters for New Media and Power On/Reset UNIT ATTENTIONs
      https://git.kernel.org/mkp/scsi/c/a5d518cd4e3e
[3/4] scsi: st: scsi_device: Modify st.c to use the new scsi_error counters
      https://git.kernel.org/mkp/scsi/c/341128dfe10a
[4/4] scsi: st: Add sysfs file position_lost_in_reset
      https://git.kernel.org/mkp/scsi/c/2c445d5f832a

-- 
Martin K. Petersen	Oracle Linux Engineering