From: Jens Axboe <email@example.com> To: Sebastian Andrzej Siewior <firstname.lastname@example.org>, email@example.com, firstname.lastname@example.org Cc: "James E.J. Bottomley" <email@example.com>, "Martin K. Petersen" <firstname.lastname@example.org>, "Ahmed S. Darwish" <email@example.com>, Thomas Gleixner <firstname.lastname@example.org> Subject: Re: [PATCH 0/3] Remove in_interrupt() usage in sr. Date: Sat, 12 Dec 2020 11:12:50 -0700 [thread overview] Message-ID: <email@example.com> (raw) In-Reply-To: <firstname.lastname@example.org> 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
prev parent reply other threads:[~2020-12-12 18:13 UTC|newest] Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top 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-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 ` Jens Axboe [this message]
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --subject='Re: [PATCH 0/3] Remove in_interrupt() usage in sr.' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
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).