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 09:07:47 -0700
From:   Ira Weiny <ira.weiny@intel.com>
To:     Jonathan Cameron <Jonathan.Cameron@huawei.com>
CC:     Davidlohr Bueso <dave@stgolabs.net>, <dan.j.williams@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: <Y0mJU6IV+Xm8Pu/m@iweiny-desk3>
References: <20221012180432.473373-1-dave@stgolabs.net>
 <20221013131913.0000038b@huawei.com>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <20221013131913.0000038b@huawei.com>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?Kbe3KEICOSmuNFZ8CvOlWHGgZn3K87ECKIg9wh1Ax2BNTzwYkEvmcEC7RdHh?=
 =?us-ascii?Q?bYtxzM9CxjSBiLySihZZ26w56SgneiAeQfs0Q+nFXktGJxsR3hxLjDsivv4G?=
 =?us-ascii?Q?D8V3oMlNaKmrwFIJR8p/GsPBdZyCpj4sl96I+Mm7/NJ70DcaJfr/ty+UPirm?=
 =?us-ascii?Q?aLdgsjtm0h4x50nCRjYs7A1Tu+QU7A6RG15nnOYRy8iRwt7jHKUMK1c0nSyW?=
 =?us-ascii?Q?79Z0zpmkdbMpTHBj+bOPAyS49bYwsuhPZAs3omwcj8Clr3Bk/YpfOP6UdYoN?=
 =?us-ascii?Q?heuzlWns2d3gucOqiPxfLeFr1ZRkLLU/6CN5WfoltiRNrvfOjNfDMk7bDys3?=
 =?us-ascii?Q?yIQRBYNGQPDHCsLvkiPOTwxPJoHI7YhxZQeDUzal3oIF/H2d4WVK0fwrITYr?=
 =?us-ascii?Q?YJjPVMFu+wsQKKtI4S2/5Qpd/00l8mB22eZwFCvDKnM7AqGVgRh09H2ANj+p?=
 =?us-ascii?Q?7UlmhDPdCjDhoOFEBkChmt+fKPrmTMG4CZg6xOZ2n8tEWZjudoPhIK6aa51F?=
 =?us-ascii?Q?u7UHIWFDbxrcNxH3I1xmanGqg9x5I5Qma9Q/6Yfjqaxyh+wImoi6SfDMxl60?=
 =?us-ascii?Q?nUy0SfUr2Uo/AWLyOEsqDkfKujvC0oP/PQ0+bmAEZtEQT262hQbLdXiPxid1?=
 =?us-ascii?Q?YIk/GmORTtr/uwpS7oIm6+BNNr7lRMxuKK2q7+akHz7Y0deJc9UsnwhiQr7w?=
 =?us-ascii?Q?gYGOBkuDVJMlNpopj90EeJoR/O3c2J8pdNNad6IyUI9du1CMsOH6ZhOHPmFY?=
 =?us-ascii?Q?GHF0Y0LqXJIs71fiy6UifQZA+mhqrqfmJKlc6+Ar5AGAk3mgB2gKlwAT+LeO?=
 =?us-ascii?Q?ruVA6ujpT9wk7qXJ0YBHqz+HV08oHlrKKwh9axKnsKo6fY93lhSkYy9CDTQA?=
 =?us-ascii?Q?qKnamYZuRfTtLk+ZT+thJvRP3iHQRxaID/7IiodgIw3SXOAcf5lM8z+U06SV?=
 =?us-ascii?Q?FqevI/ZSNd7UkPJvRQs3CpMJGsTJT0RQOpUFvIkdjdD4aZe4pZkcZ9LJEfWe?=
 =?us-ascii?Q?yU0EoWkjTEWsrSlCmngtS6eHP9UMmCTyXKEAmzIzfBAPUl7eM7F25GzVuT9J?=
 =?us-ascii?Q?GjWG7kKWF7CWj++CFhBqDGPn0dfgb1GflWseD7A1z27CZ/pR83982W/w548f?=
 =?us-ascii?Q?IBlfocNqWdHJwk7A+Nkg7HGTGOZbX2R67LCAMUwG6JMutmtQ0kTxL/mvWM/G?=
 =?us-ascii?Q?xCs7xHE2VZAhwX51c9Ndjm11BkMrHYqp7mT2BDxc+IvSIQ26qthcRi15uCCE?=
 =?us-ascii?Q?ix4X+p0Sjt15s0N0PWeTCLgxOhkGP7/Or6P5kfw9WtV0Bi3BYq+mfDc+DT3R?=
 =?us-ascii?Q?doHaP2ZQdpESOn6YIktM72Y64dYPvVCZ7c0Sah75xOBOwG5Aqz2j7brpTstx?=
 =?us-ascii?Q?ie7rE9wkEX962f+mn3OMQftHhf5Z5izB7BRKIEucrS9F1B+cV9K+C5upDrX3?=
 =?us-ascii?Q?YGqwU2B1Z5kUMX4eoDIGofdRycMcor5eqFFONJY8F6aOhc544dt3dqE+ccHC?=
 =?us-ascii?Q?QPWhXJhraLYbZvBtB11wIORQIxyfOkgQmb4eGV7IzM4cbVARKtrPf8NiA452?=
 =?us-ascii?Q?NGAV+gMvw1fYqaKGLdT/c8XUNne90z6YIqtVdNV6?=
X-MS-Exchange-CrossTenant-Network-Message-Id: dbecadd2-9b02-4c34-053b-08daadfe3efa
X-MS-Exchange-CrossTenant-AuthSource: SA1PR11MB6733.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 14 Oct 2022 16:07:51.8092
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: mUJGF7uHs/oE1Lb/x3LWSRbGt2ojDp+j/l2W2zdUjo8AocP467Wp09Qyc5gDyAA5cQCFdkUdagt9F1ckzTnkFw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BN9PR11MB5545
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-4.4 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_MED,
        RCVD_IN_MSPIKE_H3,RCVD_IN_MSPIKE_WL,SPF_HELO_NONE,SPF_NONE
        autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Thu, Oct 13, 2022 at 01:19:13PM +0100, Jonathan Cameron wrote:
> On Wed, 12 Oct 2022 11:04:32 -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.
> > 
> > Create an infrastructure to query the max vectors required for the CXL
> > device.
> > 
> > Signed-off-by: Davidlohr Bueso <dave@stgolabs.net>
> 
> Hi Davidlohr,
> 
> Basically good, but a few comments inline.
> 
> I'll role this onto front of the v2 of the CPMU set as well.

And I don't mind this landing ahead of the event stuff.  I'll take this in my
series too but expect it to drop out when applied.

Ira

> 
> > ---
> >  drivers/cxl/pci.c | 63 +++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 63 insertions(+)
> > 
> > diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> > index faeb5d9d7a7a..467f2d568e3e 100644
> > --- a/drivers/cxl/pci.c
> > +++ b/drivers/cxl/pci.c
> > @@ -428,6 +428,66 @@ static void devm_cxl_pci_create_doe(struct cxl_dev_state *cxlds)
> >  	}
> >  }
> >  
> > +/**
> > + * struct cxl_irq_cap - CXL feature that is capable of receiving MSI/MSI-X irqs.
> > + *
> > + * @name: Name of the device generating this interrupt.
> > + * @get_max_msgnum: Get the feature's largest interrupt message number.  If the
> > + *		    feature does not have the Interrupt Supported bit set, then
> > + *		    return -1.
> > + */
> > +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.
> 
> Also need it later to map the resulting register blocks to go find the irq before
> then dropping them mappings so that the resulting CPMU device can grab them
> later.
> 
> > +};
> > +
> > +static const struct cxl_irq_cap cxl_irq_cap_table[] = {
> > +	{ "isolation", NULL },
> > +	{ "pmu_overflow", NULL },
> > +	{ "mailbox", NULL },
> > +	{ "event", NULL },
> 
> Fill these in as we provide them, not upfront. I'd rather see this
> attached to one (or possibly several) of the series that are coming along
> than stand alone.  so start off with an empty table.
> 
> 
> 
> > +};
> > +
> > +static void cxl_pci_free_irq_vectors(void *data)
> > +{
> > +	pci_free_irq_vectors(data);
> > +}
> > +
> > +static int cxl_pci_alloc_irq_vectors(struct cxl_dev_state *cxlds)
> > +{
> > +	struct device *dev = cxlds->dev;
> > +	struct pci_dev *pdev = to_pci_dev(dev);
> > +	int rc, i, vectors = -1;
> > +
> > +	for (i = 0; i < ARRAY_SIZE(cxl_irq_cap_table); i++) {
> > +		int irq;
> > +
> > +		if (!cxl_irq_cap_table[i].get_max_msgnum)
> > +			continue;
> > +
> > +		irq = cxl_irq_cap_table[i].get_max_msgnum(cxlds);
> > +		vectors = max_t(int, irq, vectors);
> > +	}
> > +
> > +	if (vectors == -1)
> > +		return -EINVAL; /* no irq support whatsoever */
> 
> return 0 in this case.  No irqs present is a 'good' result if there
> aren't any.  Will be up to the consumers of the interrupts to get
> their own interrupt vector numbers and they should get the same
> answers!
> 
> > +
> > +	vectors++;
> > +	rc = pci_alloc_irq_vectors(pdev, vectors, vectors,
> > +				   PCI_IRQ_MSIX | PCI_IRQ_MSI);
> > +	if (rc < 0)
> > +		return rc;
> > +
> > +	if (rc != vectors) {
> > +		dev_err(dev, "Not enough interrupts; use polling where supported\n");
> > +		/* Some got allocated; clean them up */
> > +		cxl_pci_free_irq_vectors(pdev);
> > +		return -ENOSPC;
> > +	}
> > +
> > +	return devm_add_action_or_reset(dev, cxl_pci_free_irq_vectors, pdev);
> > +}
> > +
> >  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.
> 
> 
> 
> >  	if (resource_size(&cxlds->pmem_res) && IS_ENABLED(CONFIG_CXL_PMEM))
> >  		rc = devm_cxl_add_nvdimm(&pdev->dev, cxlmd);
> >  
>