lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Sat, 22 Oct 2022 15:17:26 -0700 From: Dan Williams <dan.j.williams@...el.com> To: Jonathan Cameron <Jonathan.Cameron@...wei.com>, Ira Weiny <ira.weiny@...el.com> CC: Davidlohr Bueso <dave@...olabs.net>, <dan.j.williams@...el.com>, <dave.jiang@...el.com>, <alison.schofield@...el.com>, <bwidawsk@...nel.org>, <vishal.l.verma@...el.com>, <a.manzanares@...sung.com>, <linux-cxl@...r.kernel.org>, <linux-kernel@...r.kernel.org> Subject: Re: [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@...el.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@...wei.com> wrote: > > > > > > > On Mon, 17 Oct 2022 20:00:09 -0700 > > > > Davidlohr Bueso <dave@...olabs.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@...el.com> > > > > > Signed-off-by: Davidlohr Bueso <dave@...olabs.net> > > > > > > > > A few nitpicks inline. > > > > > > > > With the comment one tidied up (other one optional) > > > > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@...wei.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.
Powered by blists - more mailing lists