linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Jens Axboe <axboe@kernel.dk>
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>
Subject: Re: [PATCH 0/3] Remove in_interrupt() usage in sr.
Date: Sat, 12 Dec 2020 11:12:50 -0700	[thread overview]
Message-ID: <c69b3d26-6fb7-674b-a3a6-bec80da2cff4@kernel.dk> (raw)
In-Reply-To: <20201204164803.ovwurzs3257em2rp@linutronix.de>

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


      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 \
    --in-reply-to=c69b3d26-6fb7-674b-a3a6-bec80da2cff4@kernel.dk \
    --to=axboe@kernel.dk \
    --cc=a.darwish@linutronix.de \
    --cc=bigeasy@linutronix.de \
    --cc=jejb@linux.ibm.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-scsi@vger.kernel.org \
    --cc=martin.petersen@oracle.com \
    --cc=tglx@linutronix.de \
    --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).