[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <Y4u5BoSm9D90jbp/@iweiny-mobl>
Date: Sat, 3 Dec 2022 13:00:54 -0800
From: Ira Weiny <ira.weiny@...el.com>
To: Dan Williams <dan.j.williams@...el.com>
CC: Jonathan Cameron <Jonathan.Cameron@...wei.com>,
Dave Jiang <dave.jiang@...el.com>,
Alison Schofield <alison.schofield@...el.com>,
"Vishal Verma" <vishal.l.verma@...el.com>,
Ben Widawsky <bwidawsk@...nel.org>,
"Steven Rostedt" <rostedt@...dmis.org>,
Davidlohr Bueso <dave@...olabs.net>,
<linux-kernel@...r.kernel.org>, <linux-cxl@...r.kernel.org>
Subject: Re: [PATCH V2 04/11] cxl/mem: Clear events on driver load
On Fri, Dec 02, 2022 at 03:34:20PM -0800, Dan Williams wrote:
> Ira Weiny wrote:
> > On Thu, Dec 01, 2022 at 06:48:12PM -0800, Dan Williams wrote:
> > > cxl/mem is cxl_mem.ko, This is cxl/pci.
[snip]
> > > > + cxl_mem_get_event_records(cxlds);
> > > > +
> > >
> > > This hunk likely goes with the first patch that actually implements some
> > > mocked events.
> >
> > If this patch was squashed into the other patches yes. But as a patch which
> > does exactly 1 thing "Clear events on driver load" it works IMO. I could just
> > have well put this patch at the very end.
> >
> > Now that the Get/Clear operations are more settled I'll split this out and
> > squash it as you suggest. Jonathan suggested squashing Get/Clear too but again
> > I really prefer the 1 thing/patch and each of those operations seemed like a
> > good breakdown.
> >
>
> I'll preface this by saying if you ask 3 kernel developers how to split
> a patch series you'll get 5 answers.
Indeed.
> For me though, a patch should be a
> bisectable full-thought. That at each step of a series the kernel is
> incrementally better in a way that makes sense. The kernel that gets Get
> Events likely needs to clear them too to complete 1 full thought about
> enbling Event handling. Otherwise a kernel that just retrieves some
> events until they overflow feels like a POC.
I've squashed it.
Ira
Powered by blists - more mailing lists