> -----Original Message----- > From: Keith Busch <kbu...@kernel.org> > Sent: Friday, September 18, 2020 5:10 PM > To: Klaus Jensen <i...@irrelevant.dk> > Cc: Dmitry Fomichev <dmitry.fomic...@wdc.com>; Fam Zheng > <f...@euphon.net>; Kevin Wolf <kw...@redhat.com>; Damien Le Moal > <damien.lem...@wdc.com>; qemu-bl...@nongnu.org; Niklas Cassel > <niklas.cas...@wdc.com>; Klaus Jensen <k.jen...@samsung.com>; qemu- > de...@nongnu.org; Alistair Francis <alistair.fran...@wdc.com>; Philippe > Mathieu-Daudé <phi...@redhat.com>; Matias Bjorling > <matias.bjorl...@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.
