Message-ID: <20201204164803.ovwurzs3257em2rp@linutronix.de>
Date:   Fri, 4 Dec 2020 17:48:03 +0100
From:   Sebastian Andrzej Siewior <bigeasy@...utronix.de>
To:     linux-scsi@...r.kernel.org, linux-kernel@...r.kernel.org
Cc:     Jens Axboe <axboe@...nel.dk>,
        "James E.J. Bottomley" <jejb@...ux.ibm.com>,
        "Martin K. Petersen" <martin.petersen@...cle.com>,
        "Ahmed S. Darwish" <a.darwish@...utronix.de>,
        Thomas Gleixner <tglx@...utronix.de>
Subject: [PATCH 0/3] Remove in_interrupt() usage in sr.

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

Powered by blists - more mailing lists