| Message ID | 20201204164803.ovwurzs3257em2rp@linutronix.de |
|---|---|
| Headers | show |
| Series | Remove in_interrupt() usage in sr. | expand |
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