[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
RE: [PATCH v3 01/15] hw/block/nvme: Define 64 bit cqe.result
From: |
Dmitry Fomichev |
Subject: |
RE: [PATCH v3 01/15] hw/block/nvme: Define 64 bit cqe.result |
Date: |
Mon, 21 Sep 2020 21:39:09 +0000 |
> -----Original Message-----
> From: Keith Busch <kbusch@kernel.org>
> Sent: Friday, September 18, 2020 5:10 PM
> To: Klaus Jensen <its@irrelevant.dk>
> Cc: Dmitry Fomichev <Dmitry.Fomichev@wdc.com>; Fam Zheng
> <fam@euphon.net>; Kevin Wolf <kwolf@redhat.com>; Damien Le Moal
> <Damien.LeMoal@wdc.com>; qemu-block@nongnu.org; Niklas Cassel
> <Niklas.Cassel@wdc.com>; Klaus Jensen <k.jensen@samsung.com>; qemu-
> devel@nongnu.org; Alistair Francis <Alistair.Francis@wdc.com>; Philippe
> Mathieu-Daudé <philmd@redhat.com>; Matias Bjorling
> <Matias.Bjorling@wdc.com>
> Subject: Re: [PATCH v3 01/15] hw/block/nvme: Define 64 bit cqe.result
>
> On Tue, Sep 15, 2020 at 10:48:49PM +0200, Klaus Jensen wrote:
> > On Sep 15 20:44, Dmitry Fomichev wrote:
> > >
> > > It is not necessary to change it in NST patch since result64 field is not
> > > used
> > > in that patch. But if you insist, I can move it to NST patch :)
> >
> > You are right of course, but since it is a change to the "spec" related
> > data structures that go into include/block/nvme.h, I think it belongs in
> > "hw/block/nvme: Introduce the Namespace Types definitions".
>
> Just my $.02, unless you're making exernal APIs, I really find it easier
> to review internal changes inline with the patches that use it.
>
> Another example, there are patches in this series that introduce trace
> points, but the patch that use them come later. I find that harder to
> review since I need to look at two different patches to understand its
> value.
>
I decided to split the trace events to be separate because I felt that it
could make the reviewing process simpler. Since the majority of the patches
in this series are on the large side, I thought it would be easier to open
them in separate sessions rather than to review a large single diff.
Let me know if you'd like me to fold the tracing stuff together with
the code...
DF
> I realize this particular patch is implementing a spec feature, but I'd
> prefer to see how it's used over of making a round trip to the spec.
- [PATCH v3 00/15] hw/block/nvme: Support Namespace Types and Zoned Namespace Command Set, Dmitry Fomichev, 2020/09/13
- [PATCH v3 01/15] hw/block/nvme: Define 64 bit cqe.result, Dmitry Fomichev, 2020/09/13
- Re: [PATCH v3 01/15] hw/block/nvme: Define 64 bit cqe.result, Klaus Jensen, 2020/09/15
- RE: [PATCH v3 01/15] hw/block/nvme: Define 64 bit cqe.result, Dmitry Fomichev, 2020/09/15
- Re: [PATCH v3 01/15] hw/block/nvme: Define 64 bit cqe.result, Klaus Jensen, 2020/09/15
- RE: [PATCH v3 01/15] hw/block/nvme: Define 64 bit cqe.result, Dmitry Fomichev, 2020/09/15
- Re: [PATCH v3 01/15] hw/block/nvme: Define 64 bit cqe.result, Klaus Jensen, 2020/09/15
- Re: [PATCH v3 01/15] hw/block/nvme: Define 64 bit cqe.result, Keith Busch, 2020/09/18
- RE: [PATCH v3 01/15] hw/block/nvme: Define 64 bit cqe.result,
Dmitry Fomichev <=
[PATCH v3 02/15] hw/block/nvme: Report actual LBA data shift in LBAF, Dmitry Fomichev, 2020/09/13
[PATCH v3 03/15] hw/block/nvme: Add Commands Supported and Effects log, Dmitry Fomichev, 2020/09/13
[PATCH v3 04/15] hw/block/nvme: Introduce the Namespace Types definitions, Dmitry Fomichev, 2020/09/13
[PATCH v3 05/15] hw/block/nvme: Define trace events related to NS Types, Dmitry Fomichev, 2020/09/13
[PATCH v3 06/15] hw/block/nvme: Add support for Namespace Types, Dmitry Fomichev, 2020/09/13