linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/3] Remove in_interrupt() usage in sr.
@ 2020-12-04 16:48 Sebastian Andrzej Siewior
  2020-12-04 16:48 ` [PATCH 1/3] cdrom: Reset sector_size back it is not 2048 Sebastian Andrzej Siewior
  2020-12-12 18:12 ` [PATCH 0/3] Remove in_interrupt() usage in sr Jens Axboe
  0 siblings, 2 replies; 6+ messages in thread
From: Sebastian Andrzej Siewior @ 2020-12-04 16:48 UTC (permalink / raw)
  To: linux-scsi, linux-kernel
  Cc: Jens Axboe, James E.J. Bottomley, Martin K. Petersen,
	Ahmed S. Darwish, Thomas Gleixner

Before v2.1.62 sr_read_sector() did MODE_SELECT to set the requested sector size,
issued READ_10 and then used MODE_SELECT to select a sector size of 2048 bytes.
This function was used to serve ioctl()'s command CDROMREADMODE2 and CDROMREADRAW
which do not use 2048 bytes as sector size.

In v2.1.62 sr_read_sector() changed to use READ_CD first and fallback to
MODE_SELECT and READ_10 if READ_CD was not supported. Since this version it did
not reset the sector size back to 2048 after the READ_10 opcode and
instead gained a lazy reset in do_sr_request() and sr_release().
It kept the new sector size and only changed if needed. On closing the
device node sr_release() reset the sector size back to its default
value.

In v2.3.16 the ioctl() (CDROMREADMODE2, CDROMREADRAW) were consolidated since
both stacks (SCSI and IDE) did mostly the same thing. For the ioctl handling
the SCSI implementation (doing sr_read_sector()) was removed and the ioctl was
now served based on what the IDE implementation had to offer which was
using cdrom_read_block(). cdrom_read_block() was also updated to use
READ_CD and invoke the ->generic_packeto() callback.
It is worth noting that READ_CD is now mandatory by the software stack.
The old function with the fallback (sr_read_sector()) is only used
sr_is_xa().

In v2.4.0-test2pre2 it is no longer mandatory to support the READ_CD
opcode. A fallback mechanism was added in case the device did not
supported the opcode. The mechanism had a small variance compared to the
one from v2.1.62 and did: MODE_SELECT of the requested sector size,
READ_10 and MODE_SELECT of the _requested_ sector size instead the
previous sector size. To quote a comment from the changelog
area of the file from when the change was introduced:
| -- Fix Video-CD on SCSI drives that don't support READ_CD command. In
| that case switch block size and issue plain READ_10 again, then switch
| back.

but the code did not switch back, the changed sector size remained. The comment
around the code says:
|/* FIXME: switch back again... */

which leaves me puzzled. My interpretation of my archaeological research
is that MODE_SELECT + READ_10 + FIXME was added first as the needed
workaround. Later within the same release the FIXME was addressed by
unfortunately using the wrong sector size, the FIXME comment remained
and the changelog comment was added.

This is what we have today. Lets move on with this background in mind.

The in_interrupt() check in sr_init_command() is a leftover from v2.1.62
change when the delayed sector size reset was used. It remained even
after it was no longer used after v2.3.16. 

The sector size change was introduced back in v2.4.0-test2pre2 for SCSI
devices that lack the READ_CD command but it was implemented
differently. It sends directly a CDB which is not inspected by
sr_packet() so the ->sector_size variable is never updated as it used
to be back at the time when ioctl() was served by `sr'. As a consequence
sr_release() is not resetting the sector size nor does
sr_init_command(). I did not find anything that would allow to update
the sector size at run time (other than a media change).

Side note: sr_init_command() is often invoked indirectly by
__blk_mq_run_hw_queue() which has a WARN_ON_ONCE(in_interrupt()) check
and acquires a rcu_readlock() so sleeping is not allowed and not
detected by in_interrupt().

Sebastian

^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH 1/3] cdrom: Reset sector_size back it is not 2048.
  2020-12-04 16:48 [PATCH 0/3] Remove in_interrupt() usage in sr Sebastian Andrzej Siewior
@ 2020-12-04 16:48 ` Sebastian Andrzej Siewior
  2020-12-04 16:48   ` [PATCH 2/3] sr: Switch the sector size back to 2048 if sr_read_sector() changed it Sebastian Andrzej Siewior
  2020-12-04 16:48   ` [PATCH 3/3] sr: Remove in_interrupt() usage in sr_init_command() Sebastian Andrzej Siewior
  2020-12-12 18:12 ` [PATCH 0/3] Remove in_interrupt() usage in sr Jens Axboe
  1 sibling, 2 replies; 6+ messages in thread
From: Sebastian Andrzej Siewior @ 2020-12-04 16:48 UTC (permalink / raw)
  To: linux-scsi, linux-kernel
  Cc: Jens Axboe, James E.J. Bottomley, Martin K. Petersen,
	Ahmed S . Darwish, Thomas Gleixner, Sebastian Andrzej Siewior

In v2.4.0-test2pre2 mmc_ioctl_cdrom_read_data() was extended by issuing
a MODE_SELECT opcode to change the sector size and READ_10 to perform
the actual read if the READ_CD opcode is not support.
The sector size is never changed back to the previous value of 2048
bytes which is however denoted by the comment for version 3.09 of the
cdrom.c file.

Use cdrom_switch_blocksize() to change the sector size only if the
requested size deviates from 2048. Change it back to 2048 after the read
operation if a change was mode.

Link: https://lkml.kernel.org/r/20201204164803.ovwurzs3257em2rp@linutronix.de
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/cdrom/cdrom.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/cdrom/cdrom.c b/drivers/cdrom/cdrom.c
index 0c271b9e3c5b7..8f0e52a714938 100644
--- a/drivers/cdrom/cdrom.c
+++ b/drivers/cdrom/cdrom.c
@@ -2996,13 +2996,15 @@ static noinline int mmc_ioctl_cdrom_read_data(struct cdrom_device_info *cdi,
 		 * SCSI-II devices are not required to support
 		 * READ_CD, so let's try switching block size
 		 */
-		/* FIXME: switch back again... */
-		ret = cdrom_switch_blocksize(cdi, blocksize);
-		if (ret)
-			goto out;
+		if (blocksize != CD_FRAMESIZE) {
+			ret = cdrom_switch_blocksize(cdi, blocksize);
+			if (ret)
+				goto out;
+		}
 		cgc->sshdr = NULL;
 		ret = cdrom_read_cd(cdi, cgc, lba, blocksize, 1);
-		ret |= cdrom_switch_blocksize(cdi, blocksize);
+		if (blocksize != CD_FRAMESIZE)
+			ret |= cdrom_switch_blocksize(cdi, CD_FRAMESIZE);
 	}
 	if (!ret && copy_to_user(arg, cgc->buffer, blocksize))
 		ret = -EFAULT;
-- 
2.29.2


^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH 2/3] sr: Switch the sector size back to 2048 if sr_read_sector() changed it.
  2020-12-04 16:48 ` [PATCH 1/3] cdrom: Reset sector_size back it is not 2048 Sebastian Andrzej Siewior
@ 2020-12-04 16:48   ` Sebastian Andrzej Siewior
  2020-12-04 16:48   ` [PATCH 3/3] sr: Remove in_interrupt() usage in sr_init_command() Sebastian Andrzej Siewior
  1 sibling, 0 replies; 6+ messages in thread
From: Sebastian Andrzej Siewior @ 2020-12-04 16:48 UTC (permalink / raw)
  To: linux-scsi, linux-kernel
  Cc: Jens Axboe, James E.J. Bottomley, Martin K. Petersen,
	Ahmed S . Darwish, Thomas Gleixner, Sebastian Andrzej Siewior

sr_read_sector() is hardly used since v2.3.16. Its only purpose is to
check if it is a XA medium via sr_is_xa(). This check is only enabled if
the module parameter `xa_test' is enabled.

Change the sector size back to 2048 if it was changed. With this change,
there is no lazy sector size changing left.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/scsi/sr_ioctl.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/scsi/sr_ioctl.c b/drivers/scsi/sr_ioctl.c
index ffcf902da3901..5703f8400b73c 100644
--- a/drivers/scsi/sr_ioctl.c
+++ b/drivers/scsi/sr_ioctl.c
@@ -549,6 +549,8 @@ static int sr_read_sector(Scsi_CD *cd, int lba, int blksize, unsigned char *dest
 	cgc.timeout = IOCTL_TIMEOUT;
 	rc = sr_do_ioctl(cd, &cgc);
 
+	if (blksize != CD_FRAMESIZE)
+		rc |= sr_set_blocklength(cd, CD_FRAMESIZE);
 	return rc;
 }
 
-- 
2.29.2


^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH 3/3] sr: Remove in_interrupt() usage in sr_init_command().
  2020-12-04 16:48 ` [PATCH 1/3] cdrom: Reset sector_size back it is not 2048 Sebastian Andrzej Siewior
  2020-12-04 16:48   ` [PATCH 2/3] sr: Switch the sector size back to 2048 if sr_read_sector() changed it Sebastian Andrzej Siewior
@ 2020-12-04 16:48   ` Sebastian Andrzej Siewior
  2020-12-11 17:09     ` Sebastian Andrzej Siewior
  1 sibling, 1 reply; 6+ messages in thread
From: Sebastian Andrzej Siewior @ 2020-12-04 16:48 UTC (permalink / raw)
  To: linux-scsi, linux-kernel
  Cc: Jens Axboe, James E.J. Bottomley, Martin K. Petersen,
	Ahmed S . Darwish, Thomas Gleixner, Sebastian Andrzej Siewior

The in_interrupt() check in sr_init_command() is a leftover from the
past, pre v2.3.16 era to be exact. Back then the ioctl() was served by
`sr' itself and sector size changes by CDROMREADMODE2 (as noted in the
comment) were accounted within sr's data structures which allowed a
"lazy" reset so it could be skipped on the next request and reset back
to the default value once the device node was closed or before a command
from the blockqueue was issued.

This does not work like that anymore. The CDROMREADMODE2 is served by
cdrom's mmc_ioctl() function which may change the sector size but the
`sr' driver does not learn about it and so its ->sector_size is not
updated.
The ioctl() resets the changed sector size back to 2048.
sr_read_sector() also resets the sector size back to the default once it
is done.

Remove the conditional sector size update from sr_init_command() and
sr_release() because it is not needed.

Link: https://lkml.kernel.org/r/20201204164803.ovwurzs3257em2rp@linutronix.de
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
This change makes also ide-cd providing the last non-empty
cdrom_device_info::release callback.

 drivers/scsi/sr.c | 17 -----------------
 1 file changed, 17 deletions(-)

diff --git a/drivers/scsi/sr.c b/drivers/scsi/sr.c
index fd4b582110b29..e4633b84c556a 100644
--- a/drivers/scsi/sr.c
+++ b/drivers/scsi/sr.c
@@ -416,19 +416,7 @@ static blk_status_t sr_init_command(struct scsi_cmnd *SCpnt)
 		goto out;
 	}
 
-	/*
-	 * we do lazy blocksize switching (when reading XA sectors,
-	 * see CDROMREADMODE2 ioctl) 
-	 */
 	s_size = cd->device->sector_size;
-	if (s_size > 2048) {
-		if (!in_interrupt())
-			sr_set_blocklength(cd, 2048);
-		else
-			scmd_printk(KERN_INFO, SCpnt,
-				    "can't switch blocksize: in interrupt\n");
-	}
-
 	if (s_size != 512 && s_size != 1024 && s_size != 2048) {
 		scmd_printk(KERN_ERR, SCpnt, "bad sector size %d\n", s_size);
 		goto out;
@@ -701,11 +689,6 @@ static int sr_open(struct cdrom_device_info *cdi, int purpose)
 
 static void sr_release(struct cdrom_device_info *cdi)
 {
-	struct scsi_cd *cd = cdi->handle;
-
-	if (cd->device->sector_size > 2048)
-		sr_set_blocklength(cd, 2048);
-
 }
 
 static int sr_probe(struct device *dev)
-- 
2.29.2


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 3/3] sr: Remove in_interrupt() usage in sr_init_command().
  2020-12-04 16:48   ` [PATCH 3/3] sr: Remove in_interrupt() usage in sr_init_command() Sebastian Andrzej Siewior
@ 2020-12-11 17:09     ` Sebastian Andrzej Siewior
  0 siblings, 0 replies; 6+ messages in thread
From: Sebastian Andrzej Siewior @ 2020-12-11 17:09 UTC (permalink / raw)
  To: linux-scsi, linux-kernel
  Cc: Jens Axboe, James E.J. Bottomley, Martin K. Petersen,
	Ahmed S . Darwish, Thomas Gleixner

On 2020-12-04 17:48:50 [+0100], To linux-scsi@vger.kernel.org wrote:
> The in_interrupt() check in sr_init_command() is a leftover from the
> past, pre v2.3.16 era to be exact. Back then the ioctl() was served by
> `sr' itself and sector size changes by CDROMREADMODE2 (as noted in the
> comment) were accounted within sr's data structures which allowed a
> "lazy" reset so it could be skipped on the next request and reset back
> to the default value once the device node was closed or before a command
> from the blockqueue was issued.
> 
> This does not work like that anymore. The CDROMREADMODE2 is served by
> cdrom's mmc_ioctl() function which may change the sector size but the
> `sr' driver does not learn about it and so its ->sector_size is not
> updated.
> The ioctl() resets the changed sector size back to 2048.
> sr_read_sector() also resets the sector size back to the default once it
> is done.
> 
> Remove the conditional sector size update from sr_init_command() and
> sr_release() because it is not needed.
> 
> Link: https://lkml.kernel.org/r/20201204164803.ovwurzs3257em2rp@linutronix.de
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>

Any chance to get this reviewed/merged?

Sebastian

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 0/3] Remove in_interrupt() usage in sr.
  2020-12-04 16:48 [PATCH 0/3] Remove in_interrupt() usage in sr Sebastian Andrzej Siewior
  2020-12-04 16:48 ` [PATCH 1/3] cdrom: Reset sector_size back it is not 2048 Sebastian Andrzej Siewior
@ 2020-12-12 18:12 ` Jens Axboe
  1 sibling, 0 replies; 6+ messages in thread
From: Jens Axboe @ 2020-12-12 18:12 UTC (permalink / raw)
  To: Sebastian Andrzej Siewior, linux-scsi, linux-kernel
  Cc: James E.J. Bottomley, Martin K. Petersen, Ahmed S. Darwish,
	Thomas Gleixner

On 12/4/20 9:48 AM, Sebastian Andrzej Siewior wrote:
> Before v2.1.62 sr_read_sector() did MODE_SELECT to set the requested sector size,
> issued READ_10 and then used MODE_SELECT to select a sector size of 2048 bytes.
> This function was used to serve ioctl()'s command CDROMREADMODE2 and CDROMREADRAW
> which do not use 2048 bytes as sector size.
> 
> In v2.1.62 sr_read_sector() changed to use READ_CD first and fallback to
> MODE_SELECT and READ_10 if READ_CD was not supported. Since this version it did
> not reset the sector size back to 2048 after the READ_10 opcode and
> instead gained a lazy reset in do_sr_request() and sr_release().
> It kept the new sector size and only changed if needed. On closing the
> device node sr_release() reset the sector size back to its default
> value.
> 
> In v2.3.16 the ioctl() (CDROMREADMODE2, CDROMREADRAW) were consolidated since
> both stacks (SCSI and IDE) did mostly the same thing. For the ioctl handling
> the SCSI implementation (doing sr_read_sector()) was removed and the ioctl was
> now served based on what the IDE implementation had to offer which was
> using cdrom_read_block(). cdrom_read_block() was also updated to use
> READ_CD and invoke the ->generic_packeto() callback.
> It is worth noting that READ_CD is now mandatory by the software stack.
> The old function with the fallback (sr_read_sector()) is only used
> sr_is_xa().
> 
> In v2.4.0-test2pre2 it is no longer mandatory to support the READ_CD
> opcode. A fallback mechanism was added in case the device did not
> supported the opcode. The mechanism had a small variance compared to the
> one from v2.1.62 and did: MODE_SELECT of the requested sector size,
> READ_10 and MODE_SELECT of the _requested_ sector size instead the
> previous sector size. To quote a comment from the changelog
> area of the file from when the change was introduced:
> | -- Fix Video-CD on SCSI drives that don't support READ_CD command. In
> | that case switch block size and issue plain READ_10 again, then switch
> | back.
> 
> but the code did not switch back, the changed sector size remained. The comment
> around the code says:
> |/* FIXME: switch back again... */
> 
> which leaves me puzzled. My interpretation of my archaeological research
> is that MODE_SELECT + READ_10 + FIXME was added first as the needed
> workaround. Later within the same release the FIXME was addressed by
> unfortunately using the wrong sector size, the FIXME comment remained
> and the changelog comment was added.
> 
> This is what we have today. Lets move on with this background in mind.
> 
> The in_interrupt() check in sr_init_command() is a leftover from v2.1.62
> change when the delayed sector size reset was used. It remained even
> after it was no longer used after v2.3.16. 
> 
> The sector size change was introduced back in v2.4.0-test2pre2 for SCSI
> devices that lack the READ_CD command but it was implemented
> differently. It sends directly a CDB which is not inspected by
> sr_packet() so the ->sector_size variable is never updated as it used
> to be back at the time when ioctl() was served by `sr'. As a consequence
> sr_release() is not resetting the sector size nor does
> sr_init_command(). I did not find anything that would allow to update
> the sector size at run time (other than a media change).
> 
> Side note: sr_init_command() is often invoked indirectly by
> __blk_mq_run_hw_queue() which has a WARN_ON_ONCE(in_interrupt()) check
> and acquires a rcu_readlock() so sleeping is not allowed and not
> detected by in_interrupt().

Applied, thanks.

-- 
Jens Axboe


^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2020-12-12 18:13 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-12-04 16:48 [PATCH 0/3] Remove in_interrupt() usage in sr Sebastian Andrzej Siewior
2020-12-04 16:48 ` [PATCH 1/3] cdrom: Reset sector_size back it is not 2048 Sebastian Andrzej Siewior
2020-12-04 16:48   ` [PATCH 2/3] sr: Switch the sector size back to 2048 if sr_read_sector() changed it Sebastian Andrzej Siewior
2020-12-04 16:48   ` [PATCH 3/3] sr: Remove in_interrupt() usage in sr_init_command() Sebastian Andrzej Siewior
2020-12-11 17:09     ` Sebastian Andrzej Siewior
2020-12-12 18:12 ` [PATCH 0/3] Remove in_interrupt() usage in sr Jens Axboe

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).