Re: [PATCH 1/2] cxl/pci: Add generic MSI-X/MSI irq support

From: Dan Williams <dan.j.williams@intel.com>
To: Jonathan Cameron <Jonathan.Cameron@huawei.com>,
	Ira Weiny <ira.weiny@intel.com>
Cc: Davidlohr Bueso <dave@stgolabs.net>, <dan.j.williams@intel.com>,
	<dave.jiang@intel.com>, <alison.schofield@intel.com>,
	<bwidawsk@kernel.org>, <vishal.l.verma@intel.com>,
	<a.manzanares@samsung.com>, <linux-cxl@vger.kernel.org>,
	<linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 1/2] cxl/pci: Add generic MSI-X/MSI irq support
Date: Sat, 22 Oct 2022 15:17:26 -0700	[thread overview]
Message-ID: <63546bf682ccf_14192944f@dwillia2-mobl3.amr.corp.intel.com.notmuch> (raw)
In-Reply-To: <20221021095818.00006ed1@huawei.com>

Jonathan Cameron wrote:
> On Thu, 20 Oct 2022 21:14:29 -0700
> Ira Weiny <ira.weiny@intel.com> wrote:
> 
> > On Tue, Oct 18, 2022 at 11:52:27AM +0100, Jonathan Cameron wrote:
> > > On Tue, 18 Oct 2022 10:36:19 +0100
> > > Jonathan Cameron <Jonathan.Cameron@huawei.com> wrote:
> > >   
> > > > On Mon, 17 Oct 2022 20:00:09 -0700
> > > > Davidlohr Bueso <dave@stgolabs.net> wrote:
> > > >   
> > > > > Introduce a generic irq table for CXL components/features that can have
> > > > > standard irq support - DOE requires dynamic vector sizing and is not
> > > > > considered here. For now the table is empty.
> > > > > 
> > > > > Create an infrastructure to query the max vectors required for the CXL
> > > > > device. Upon successful allocation, users can plug in their respective isr
> > > > > at any point thereafter, which is supported by a new cxlds->has_irq flag,
> > > > > for example, if the irq setup is not done in the PCI driver, such as
> > > > > the case of the CXL-PMU.
> > > > > 
> > > > > Reviewed-by: Dave Jiang <dave.jiang@intel.com>
> > > > > Signed-off-by: Davidlohr Bueso <dave@stgolabs.net>    
> > > > 
> > > > A few nitpicks inline.
> > > > 
> > > > With the comment one tidied up (other one optional)
> > > > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > > > 
> > > > I'll rebase my cpmu code on top of this shortly.  
> > > Hi Davidlohr,
> > > 
> > > Doing the CPMU rebase has shown up that using this generic infrastructure
> > > ends up rather ugly.
> > > 
> > > Previously I had a local array to manage the required register maps
> > > that was then freed.  Now I have to move that into the cxl device state
> > > just so I can get at it from the irq finding callback.
> > > 
> > > So I have an extra step to be able to use this generic framework.
> > > 
> > > 1. Query how many CPMU devices there are.  Stash that and register map
> > >    info in cxlds.  I could do this in the callback but that's really really
> > >    horrible layering issue as most of what is done has nothing to do
> > >    with finding the vector numbers.  
> > 
> > FWIW I did this for the event stuff and did not find it so distasteful...  :-/
> > 
> > However the information I am stashing in the cxlds is all interrupt
> > information.  So I think it is different from what I see in the CPMU stuff.
> 
> Right now I'm just stashing the max interrupt number to squirt into a callback
> a few lines later. That feels like a hack to get around parsing the structures
> 4 times.  If it's an acceptable hack then fair enough.
> 
> > 
> > > 2. The callback below to find those numbers 
> > > 3. Registration of the cpmu devices.
> > > 
> > > Reality is that it is cleaner to more or less ignore the infrastructure
> > > proposed in this patch.
> > > 
> > > 1. Query how many CPMU devices there are. Whilst there stash the maximim
> > >    cpmu vector number in the cxlds.
> > > 2. Run a stub in this infrastructure that does max(irq, cxlds->irq_num);
> > > 3. Carry on as before.
> > > 
> > > Thus destroying the point of this infrastructure for that usecase at least
> > > and leaving an extra bit of state in the cxl_dev_state that is just
> > > to squirt a value into the callback...  
> > 
> > I'm not sure I follow?  Do you mean this?
> > 
> > static int cxl_cpmu_get_max_msgnum(struct cxl_dev_state *cxlds)
> > {
> > 	return cxlds->cpmu_max_vector;
> > }
> 
> Yup. That state is no relevance to the cxl_dev_state outside of this tiny
> block of code.  Hence I really don't like putting it in there.

Yeah, I tend to agree. cxl_dev_state is the catch-all of last resort,
but if there is a more appropriate / less-abstract object to carry a
given property it should.