Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 14 Oct 2022 16:56:53 +0100
From:   Jonathan Cameron <Jonathan.Cameron@huawei.com>
To:     Davidlohr Bueso <dave@stgolabs.net>
CC:     <dan.j.williams@intel.com>, <ira.weiny@intel.com>,
        <alison.schofield@intel.com>, <vishal.l.verma@intel.com>,
        <bwidawsk@kernel.org>, <a.manzanares@samsung.com>,
        <linux-kernel@vger.kernel.org>, <linux-cxl@vger.kernel.org>
Subject: Re: [PATCH] cxl: Add generic MSI/MSI-X interrupt support
Message-ID: <20221014165653.0000140e@huawei.com>
In-Reply-To: <20221013173703.th54drzlafvj74oo@offworld>
References: <20221012180432.473373-1-dave@stgolabs.net>
        <20221013131913.0000038b@huawei.com>
        <20221013173703.th54drzlafvj74oo@offworld>
MIME-Version: 1.0
Content-Type: text/plain; charset="US-ASCII"
Content-Transfer-Encoding: 7bit
Precedence: bulk

On Thu, 13 Oct 2022 10:37:03 -0700
Davidlohr Bueso <dave@stgolabs.net> wrote:

> Thanks for having a look.
> 
> On Thu, 13 Oct 2022, Jonathan Cameron wrote:
> 
> >> +struct cxl_irq_cap {
> >> +	const char *name;
> >> +	int (*get_max_msgnum)(struct cxl_dev_state *cxlds);  
> >
> >For the CPMU case I need to walk the register locator dvsec block so need
> >the callback to take the pci_dev not the cxl_dev_state.  
> 
> Hmm ok, however maybe I'm missing something, but given a pdev, do we have a
> way to get back to the cxlds?

I'd failed to register we can easily get from cxlds to pci dev.
Had wrong mental model of what embedded what.

> 
> ...
> 
> >>  static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> >>  {
> >>  	struct cxl_register_map map;
> >> @@ -498,6 +558,9 @@ static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> >>  	if (IS_ERR(cxlmd))
> >>  		return PTR_ERR(cxlmd);
> >>
> >> +	/* TODO: When there are users, this return value must be checked */
> >> +	cxl_pci_alloc_irq_vectors(cxlds);
> >> +  
> >
> >Gut feeling is this will end up moving ahead of any of the sub device creation
> >because many of them end up needing interrupts.
> >
> >Also check response from the start - can't see a reason to not do so as we
> >won't be registering any at all if no callbacks provided.
> >
> >So I'd move it above the devm_cxl_add_memdev() call.  
> 
> Will do. In addition, are you ok with grouping the irq setup for each cxl
> feature/component, ie:
> 
> if (cxl_pci_alloc_irq_vectors(cxlds) > 0) {
>     cxl_setup_mbox_irq();
>     cxl_setup_events_irq();
>     cxl_setup_pmu_irq();
> }
> 
> I ask mostly from the mailbox perspective, in that we already have
> a mbox setup call and can certainly understand if people would prefer
> it there, but I tend to prefer the above (logically wrt irqs).

I'd rather see it in each of the setup calls.  Out in pci.c we
should just be doing minimum to find that max irq number.
e.g. the CPMU driver will rewalk and find it's vector number directly
from it's own register space.

Jonathan
> 
> Thanks,
> Davidlohr