mirror of https://lore.kernel.org/lkml/
 help / color / mirror / Atom feed
From: Andreas Hindborg <a.hindborg@kernel.org>
To: James Bottomley <James.Bottomley@HansenPartnership.com>,
	Greg KH <gregkh@linuxfoundation.org>,
	jaemyung.lee@samsung.com
Cc: "Miguel Ojeda" <ojeda@kernel.org>,
	"Boqun Feng" <boqun@kernel.org>, "Gary Guo" <gary@garyguo.net>,
	"Björn Roy Baron" <bjorn3_gh@protonmail.com>,
	"Benno Lossin" <lossin@kernel.org>,
	"Alice Ryhl" <aliceryhl@google.com>,
	"Trevor Gross" <tmgross@umich.edu>,
	"Danilo Krummrich" <dakr@kernel.org>,
	"Daniel Almeida" <daniel.almeida@collabora.com>,
	"Tamir Duberstein" <tamird@kernel.org>,
	"Alexandre Courbot" <acourbot@nvidia.com>,
	"Onur Özkan" <work@onurozkan.dev>,
	linux-kernel@vger.kernel.org, rust-for-linux@vger.kernel.org,
	linux-block@vger.kernel.org, linux-scsi@vger.kernel.org,
	linux-arm-msm@vger.kernel.org, "Jens Axboe" <axboe@kernel.dk>,
	"Martin K. Petersen" <martin.petersen@oracle.com>,
	"Bart Van Assche" <bvanassche@acm.org>,
	"Bean Huo" <beanhuo@iokpp.de>
Subject: Re: [PATCH RFC] drivers/rufs: add Rust UFS host controller driver
Date: Sat, 12 Sep 2026 17:45:45 +0200	[thread overview]
Message-ID: <87bja2v5g6.fsf@kernel.org> (raw)
In-Reply-To: <77e00073a9254d5ea1ade771590da81a859b75a6.camel@HansenPartnership.com>

"James Bottomley" <James.Bottomley@HansenPartnership.com> writes:

> On Sat, 2026-09-12 at 14:48 +0200, Andreas Hindborg wrote:
>> "James Bottomley" <James.Bottomley@HansenPartnership.com> writes:
>>
>> > On Sat, 2026-09-12 at 13:00 +0200, Andreas Hindborg wrote:
>> > > "Greg KH" <gregkh@linuxfoundation.org> writes:
>> > >
>> > > > On Sat, Sep 12, 2026 at 12:52:55AM +0900, Jaemyung Lee via B4
>> > > > Relay
>> > > > wrote:
>> > > > > Comments on the decision to bypass the SCSI midlayer, the
>> > > > > blk-mq model used in its place, and the proposed prerequisite
>> > > > > API boundaries would be especially welcome.
>> > > >
>> > > > Many many years ago, the USB subsystem tried to bypass the SCSI
>> > > > midlayer for its storage driver, and while it was a "quick
>> > > > solution" at the time, in the end, it didn't work out and we
>> > > > dropped the driver as it just made no sense to keep duplicating
>> > > > all of the logic all the time.
>> > > >
>> > > > So I wouldn't recommend it, as long as UFS builds on top of the
>> > > > SCSI commands and the like, you should not attempt to duplicate
>> > > > it in a separate driver, no matter how much "simpler" it
>> > > > initially seems to be.
>> > >
>> > > The scsi related code in this driver is less than 300 lines and
>> > > is mostly struct packing/unpacking. I guess we could lift the
>> > > struct definitions from the scsi layer via bindgen. But at 280
>> > > lines I am not sure it is worth it, and it hardly counts as
>> > > duplication.
>> >
>> > You didn't actually read the above did you?
>>
>> Yes.
>>
>> > The problem isn't necessarily the duplication of code per-se it's
>> > the duplication of function without understanding the subtlety.  
>> > Every fool thinks they can duplicate the core features of SCSI in a
>> > few hundred lines (andthey can: the core SCSI model looks
>> > beguilingly simple).
>>
>> Yes, it's really just encoding/decoding. Not much semantics or logic.
>>
>> > They then usually spend years adding back the necessary corner
>> > cases and quirks before finally concluding that actually they
>> > shouldn't have tried in the first place.
>>
>> Please educate this fool then. I am curious what SCSI logic we would
>> spend years re-implementing in a detached ufshci driver.
>
> The "duplication" is incomplete (it's actually only sense code handling
> in scsi.rs, but the rest of the duplication is sprayed all over the
> driver).

The SCSI logic is the CDB encoding in scsi.rs and about 50 lines of
completion disposition in queue.rs (status to block status, unit
attention requeue). I agree that the latter is the start of what
scsi_decide_disposition and sd_done do, and that it is incomplete. What
I am not yet convinced of is that the complete version for UFS is large
or subtle compared to scsi midlayer combined with current ufshcd.

> The incompleteness is fine if you never test for it, but your
> users will run into it in the field (almost certainly in error
> handling), which is when you begin the reinvention.

Correct, the RFC deliberately leaves out recovery and task management.
We wanted to get this discussion going sooner rather than later.

Of course we would validate the driver with proper testing before
declaring it stable.

And yes, the encoding/decoding surface will expand a bit once we
implement TMF and recovery. And there would have to be a bit of logic to
handle that as well. And that logic would be similar in semantics to
code in the scsi midlayer. I doubt it will be many lines, but I feel
like I have to build it first to convince you of that.

> As Greg said, this
> is the exact path USB storage tried ... and they did it because their
> devices barely behave like SCSI ones so they seemed different enough
> that there was little overlap with core SCSI (i.e. exactly like the
> JEDEC comment in the cover letter).
>
> What I don't quite get is this isn't a criticism of the language or the
> rust project, it's a criticism of the implementation methodology of
> this particular driver; and it isn't a theoretical criticism: it's
> practical; this path has been tried before and failed.  The definition
> of insanity is doing the same thing over and expecting different
> results.

Agreed, and I did not read it as a criticism of the language.

For ub, the deprecation commit from 2012 says it was removed because
every device it handled was also handled by usb-storage. Not because the
SCSI midlayer turned out to be deep. The UFS situation is different. The
device population is a handful of vendors behind one host controller
spec.

If we expected this new driver to be a strict subset of ufshcd, we would
not build it. The reason to do it is the features that do not fit the
scsi midlayer.

Here is what I count the midlayer doing for UFS today: sense to
retry/errno disposition (scsi_decide_disposition, scsi_check_sense,
sd_done), the abort to LU reset to host reset escalation in scsi_eh with
ufshcd supplying the handlers, the sd device model (capacity, write
cache, UNMAP, runtime PM), and the SG_IO ioctl plus the scsi_device
sysfs tree. Which of these is the part you expect to take years?

Best regards,
Andreas Hindborg



  reply	other threads:[~2026-09-12 15:45 UTC|newest]

Thread overview: 20+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-09-11 15:52 Jaemyung Lee via B4 Relay
2026-09-11 16:18 ` Andreas Hindborg
2026-09-11 19:03 ` Bart Van Assche
2026-09-12 11:33   ` Andreas Hindborg
2026-09-11 20:27 ` Greg KH
2026-09-12 11:00   ` Andreas Hindborg
2026-09-12 12:10     ` James Bottomley
2026-09-12 12:48       ` Andreas Hindborg
2026-09-12 13:37         ` Bart Van Assche
2026-09-12 13:55         ` James Bottomley
2026-09-12 15:45           ` Andreas Hindborg [this message]
2026-09-13  0:30             ` Bart Van Assche
2026-09-14 20:27               ` Bart Van Assche
2026-09-14 10:02             ` Johannes Thumshirn
2026-09-13 12:23     ` Bean Huo
2026-09-13 13:44       ` Andreas Hindborg
2026-09-11 21:44 ` Bean Huo
2026-09-12 11:20   ` Andreas Hindborg
2026-09-13 12:09     ` Bean Huo
2026-09-13 13:10       ` Andreas Hindborg

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=87bja2v5g6.fsf@kernel.org \
    --to=a.hindborg@kernel.org \
    --cc=James.Bottomley@HansenPartnership.com \
    --cc=acourbot@nvidia.com \
    --cc=aliceryhl@google.com \
    --cc=axboe@kernel.dk \
    --cc=beanhuo@iokpp.de \
    --cc=bjorn3_gh@protonmail.com \
    --cc=boqun@kernel.org \
    --cc=bvanassche@acm.org \
    --cc=dakr@kernel.org \
    --cc=daniel.almeida@collabora.com \
    --cc=gary@garyguo.net \
    --cc=gregkh@linuxfoundation.org \
    --cc=jaemyung.lee@samsung.com \
    --cc=linux-arm-msm@vger.kernel.org \
    --cc=linux-block@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-scsi@vger.kernel.org \
    --cc=lossin@kernel.org \
    --cc=martin.petersen@oracle.com \
    --cc=ojeda@kernel.org \
    --cc=rust-for-linux@vger.kernel.org \
    --cc=tamird@kernel.org \
    --cc=tmgross@umich.edu \
    --cc=work@onurozkan.dev \
    /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
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox

all inboxes | Powered by JetHome®