lkml.org 
[lkml]   [2022]   [Oct]   [22]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 1/2] cxl/pci: Add generic MSI-X/MSI irq support
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.

\
 
 \ /
  Last update: 2022-10-23 00:18    [W:0.078 / U:0.496 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site