Re: [PATCH v3] 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>,
	Christoph Hellwig <hch@lst.de>,
	linux-nvme@lists.infradead.org, sagi@grimberg.me
Subject: Re: [PATCH v3] nvme: retain split access workaround for capability reads
Date: Mon, 7 Oct 2019 15:20:11 +0200	[thread overview]
Message-ID: <CAKv+Gu-2yBVx2sh1wZ4d8+eJ8ffc44be86=FNt6K4euh+J1+OA@mail.gmail.com> (raw)
In-Reply-To: <20191007124843.GA53339@C02WT3WMHTD6>

On Mon, 7 Oct 2019 at 14:48, Keith Busch <kbusch@kernel.org> wrote:
>
> On Mon, Oct 07, 2019 at 02:32:43PM +0200, Ard Biesheuvel wrote:
> > On Mon, 7 Oct 2019 at 14:27, Christoph Hellwig <hch@lst.de> wrote:
> > >
> > > On Mon, Oct 07, 2019 at 02:24:58PM +0200, Ard Biesheuvel wrote:
> > > > > If you interconnect doesn't support 8-byte MMIO read/write TLPs you
> > > > > have a much deeper problem, as this will break all drivers using
> > > > > readq/writeq.  And we currently only have compile time detection for
> > > > > readq/writeq, not runtime so you'll have to invent a scheme if this
> > > > > works at all or not.
> > > >
> > > > Sure. But the practical reality is that the hardware in question
> > > > (including the Apple controller) worked perfectly fine until commit
> > > > 7fd8930f26be4 introduced a readq() call into a file that had
> > > > deliberately been switched to using lo_hi_readq() because readq()
> > > > doesn't work reliably for all hardware we would like it to support.
> > > > Theorizing about *why* readq() doesn't work reliably in which
> > > > particular case doesn't seem that useful to me, given how trivial the
> > > > fix is.
> > >
> > > My point here is that if it isn't the PCIe device that is broken like
> > > in the apple case, but your interconnect you have a problem that can't
> > > be fixed just in the nvme driver.  We have tons of other drivers relying
> > > in readq/writeq working if it is available.  You'll need to find a more
> > > general workaround, independent of the fact that we have a few NVMe
> > > controllers that always need this workaround.  And at least for NVMe
> > > the spec specically allows split 32-bit access at least.
> >
> > OK, that is good to know. Mind you, I used 'interconnect' in the
> > abstract sense, meaning whatever sits between the CPU doing the read
> > and the 64-bit register in the BAR space.
> >
> > But I fail to see your point. Why is it relevant for deciding whether
> > to apply a NVMe fix if the affected hardware can or cannot use other
> > types of PCIe devices? Note that I am not proposing some hacky
> > workaround to be applied, but just to stick with the workaround that
> > was already accepted (and I'm pretty sure that this Apple hardware got
> > broken too with commit 7fd8930f26be4)
>
> The point is the reasoning in the changelog does not justify *this* patch. If
> you change the wording to not mention your host controller, and instead just
> refer to the previous NVMe behavior (and modify your comment accordingly), then
> we should be fine.
>
> If you explain this patch by saying it's to fix a host controller, then the
> patch is no longer sufficient on it's own and should be fixed elsewhere,
> perhaps by providing a special pci_ops structure for your controller.

Fair enough. Any suggestions for the wording of the comment?

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