Re: [RFC PATCH] nvme: retain split access workaround for capability reads

From: Ard Biesheuvel <ard.biesheuvel@linaro.org>
To: Keith Busch <kbusch@kernel.org>
Cc: axboe@fb.com, Ilias Apalodimas <ilias.apalodimas@linaro.org>,
	sagi@grimberg.me, linux-nvme@lists.infradead.org,
	Christoph Hellwig <hch@lst.de>
Subject: Re: [RFC PATCH] nvme: retain split access workaround for capability reads
Date: Thu, 3 Oct 2019 13:51:03 +0200	[thread overview]
Message-ID: <CAKv+Gu_YCRwvtc=QSu0V2i1GESxHv9ndkqe0cyJsDaL6LQATDw@mail.gmail.com> (raw)
In-Reply-To: <20191003114901.GA34459@C02WT3WMHTD6.fritz.box>

On Thu, 3 Oct 2019 at 13:49, Keith Busch <kbusch@kernel.org> wrote:
>
> On Wed, Oct 02, 2019 at 09:36:43AM +0200, Ard Biesheuvel wrote:
> > Recent changes to the NVMe core have re-introduced an issue that we
> > have attempted to work around in the past, in commit a310acd7a7ea
> > ("NVMe: use split lo_hi_{read,write}q").
> >
> > The problem is that some PCIe NVMe controllers do not implement 64-bit
> > outbound accesses correctly, which is why the commit above switched
> > to using lo_hi_[read|write]q for all 64-bit BAR accesses.
> >
> > In the mean time, the NVMe subsystem has been refactored, and now calls
> > into the PCIe support layer for NVMe via a .reg_read64() method, which
> > fails to use lo_hi_readq(), and thus reintroduces the problem that the
> > commit above aimed to address.
> >
> > Given that, at the moment, .reg_read64() is only used to read the
> > capability register [which is known to tolerate split reads, which is
> > not guaranteed in the general case, given that the NVMe BAR may be
> > non-prefetchable], let's switch .reg_read64() to lo_hi_readq() as
> > well.
> >
> > To ensure that we will spot any changes that will start using the
> > .reg_read64() method for other purposes, WARN() if the requested
> > offset != NVME_REG_CAP.
> >
> > This fixes a boot issue on some ARM boxes with NVMe behind a
> > Synopsys DesignWare PCIe host controller.
> >
> > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > ---
> > Broken since v5.3, so if this gets fixed one way or the other, please
> > add cc: stable.
>
> Since 5.3?! 'git blame' says the code has been this way since 7fd8930f26be4,
> which was from 2015 during the 4.4 development cycle.
>

That may be true, but the box in question happily boots a v5.2 kernel.

> Please add a:
>
>   Fixes: 7fd8930f26be4 ("nvme: add a common helper to read Identify Controller data")
>
> to your change log above your sign-off, and remove the WARN_ON().

Sure.

Thanks,
Ard.

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme