[3/3] scsi: st: clear pos_unknown when the por ua is expected

Commit Message

John Meneghini Oct. 31, 2024, 1 a.m. UTC

From: Kai Makisara <[e-mail]>

The change for commit 9604eea5bd3a ("scsi: st: Add third party poweron
reset handling") was introduced to handle the problem where an
unexpected power/on reset resulted in an undetected position lost
condition which caused lost data as the driver flushed data and wrote a
file mark at the beginning of the tape.

To solve this problem code was added which detected the unexpected a
power on/reset Unit Attention in st_chk_results(). This correctly set
pos_unknown in the driver and prevented further access to the device
until the tape is re-positioned or rewound.

The problem with this solution is that it is catching POR Unit Attentions
that are expected as well as those that are unexpected. This results in
an unwanted EIO response to MTIOCGET.

The change for commit 3d882cca73be ("scsi: st: Fix input/output error on
empty drive reset") attempts to fix this problem by returning success in
flush_buffer() when ready != to ST_READY. However, once ST_READY is set,
if pos_unknown remains uncleared, MTIOGET can still return EIO. This is
confusing as some tape drives will set a POR UA when the system
reboots or the driver is reloaded.

Fixes: 3d882cca73be ("scsi: st: Fix input/output error on empty drive reset")
Closes: https://bugzilla.kernel.org/show_bug.cgi?id=219419
Signed-off-by: Kai Makisara <[e-mail]>
Tested-by: John Meneghini <[e-mail]>
---
 drivers/scsi/st.c | 31 ++++++++++++++++++++++---------
 1 file changed, 22 insertions(+), 9 deletions(-)

Comments

Kai Mäkisara (Kolumbus) Oct. 31, 2024, 2:17 p.m. UTC | #1

On 31. Oct 2024, at 3.00, John Meneghini <[e-mail]> wrote:
> 
> From: Kai Makisara <[e-mail]>
> 

The patch in this message is a WIP patch discussed in Bugzilla. It is not
meant to be submitted as such. (In addition to changes, it contains a bug
fix not related to the changes.) Those interested in the progress can look
at the Bugzilla entry shown below.

> Fixes: 3d882cca73be ("scsi: st: Fix input/output error on empty drive reset")
> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=219419
> Signed-off-by: Kai Makisara <[e-mail]>
   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
I have NOT signed of this patch.

> Tested-by: John Meneghini <[e-mail]>

John has done extensive tests to see if the patch works.

> ---
> drivers/scsi/st.c | 31 ++++++++++++++++++++++---------
> 1 file changed, 22 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/scsi/st.c b/drivers/scsi/st.c
> index e9d1cb6c8a86..0260361d19fa 100644
> --- a/drivers/scsi/st.c
> +++ b/drivers/scsi/st.c
...

John Meneghini Oct. 31, 2024, 4:43 p.m. UTC | #2

On 10/31/24 10:17, "Kai Mäkisara (Kolumbus)" wrote:
> On 31. Oct 2024, at 3.00, John Meneghini <[e-mail]> wrote:
>>
>> From: Kai Makisara <[e-mail]>
>>
> 
> The patch in this message is a WIP patch discussed in Bugzilla. It is not
> meant to be submitted as such. (In addition to changes, it contains a bug
> fix not related to the changes.) Those interested in the progress can look
> at the Bugzilla entry shown below.

Agreed.

>> Fixes: 3d882cca73be ("scsi: st: Fix input/output error on empty drive reset")
>> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=219419
>> Signed-off-by: Kai Makisara <[e-mail]>
>     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> I have NOT signed of this patch.

Sorry, I added this because I wanted to publish the changes and checkpatch.pl was complaining that your name wasn't contained in 
the signed-off.

>> Tested-by: John Meneghini <[e-mail]>
> 
> John has done extensive tests to see if the patch works.

Yes, please see the bugzilla for more information.

/John

Patch

diff --git a/drivers/scsi/st.c b/drivers/scsi/st.c
index e9d1cb6c8a86..0260361d19fa 100644
--- a/drivers/scsi/st.c
+++ b/drivers/scsi/st.c
@@ -3538,6 +3538,7 @@  static long st_ioctl(struct file *file, unsigned int cmd_in, unsigned long arg)
 	int i, cmd_nr, cmd_type, bt;
 	int retval = 0;
 	unsigned int blk;
+	bool cmd_mtiocget;
 	struct scsi_tape *STp = file->private_data;
 	struct st_modedef *STm;
 	struct st_partstat *STps;
@@ -3651,6 +3652,7 @@  static long st_ioctl(struct file *file, unsigned int cmd_in, unsigned long arg)
 			 */
 			if (mtc.mt_op != MTREW &&
 			    mtc.mt_op != MTOFFL &&
+			    mtc.mt_op != MTLOAD &&
 			    mtc.mt_op != MTRETEN &&
 			    mtc.mt_op != MTERASE &&
 			    mtc.mt_op != MTSEEK &&
@@ -3764,17 +3766,28 @@  static long st_ioctl(struct file *file, unsigned int cmd_in, unsigned long arg)
 		goto out;
 	}
 
+	cmd_mtiocget = cmd_type == _IOC_TYPE(MTIOCGET) && cmd_nr == _IOC_NR(MTIOCGET);
+
 	if ((i = flush_buffer(STp, 0)) < 0) {
-		retval = i;
-		goto out;
-	}
-	if (STp->can_partitions &&
-	    (i = switch_partition(STp)) < 0) {
-		retval = i;
-		goto out;
+		/* flush fails -> modify status accordingly */
+		if (cmd_mtiocget && STp->pos_unknown) {
+			reset_state(STp);
+			STp->pos_unknown = 1;
+		} else { /* return error */
+			retval = i;
+			goto out;
+		}
+	} else { /* flush_buffer succeeds */
+		if (STp->can_partitions) {
+			i = switch_partition(STp);
+			if (i < 0) {
+				retval = i;
+				goto out;
+			}
+		}
 	}
 
-	if (cmd_type == _IOC_TYPE(MTIOCGET) && cmd_nr == _IOC_NR(MTIOCGET)) {
+	if (cmd_mtiocget) {
 		struct mtget mt_status;
 
 		if (_IOC_SIZE(cmd_in) != sizeof(struct mtget)) {
@@ -3788,7 +3801,7 @@  static long st_ioctl(struct file *file, unsigned int cmd_in, unsigned long arg)
 		    ((STp->density << MT_ST_DENSITY_SHIFT) & MT_ST_DENSITY_MASK);
 		mt_status.mt_blkno = STps->drv_block;
 		mt_status.mt_fileno = STps->drv_file;
-		if (STp->block_size != 0) {
+		if (STp->block_size != 0 && mt_status.mt_blkno >= 0) {
 			if (STps->rw == ST_WRITING)
 				mt_status.mt_blkno +=
 				    (STp->buffer)->buffer_bytes / STp->block_size;