Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH 0/3] Remove in_interrupt() usage in sr.
To:     Sebastian Andrzej Siewior <bigeasy@linutronix.de>,
        linux-scsi@vger.kernel.org, linux-kernel@vger.kernel.org
Cc:     "James E.J. Bottomley" <jejb@linux.ibm.com>,
        "Martin K. Petersen" <martin.petersen@oracle.com>,
        "Ahmed S. Darwish" <a.darwish@linutronix.de>,
        Thomas Gleixner <tglx@linutronix.de>
References: <20201204164803.ovwurzs3257em2rp@linutronix.de>
From:   Jens Axboe <axboe@kernel.dk>
Message-ID: <c69b3d26-6fb7-674b-a3a6-bec80da2cff4@kernel.dk>
Date:   Sat, 12 Dec 2020 11:12:50 -0700
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.10.0
MIME-Version: 1.0
In-Reply-To: <20201204164803.ovwurzs3257em2rp@linutronix.de>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk

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