Received-SPF: pass (google.com: domain of linux-kernel+bounces-25093-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Fri, 12 Jan 2024 15:16:45 -0800
From: Dan Williams <dan.j.williams@intel.com>
To: Ira Weiny <ira.weiny@intel.com>, Davidlohr Bueso <dave@stgolabs.net>,
	Jonathan Cameron <jonathan.cameron@huawei.com>, Dave Jiang
	<dave.jiang@intel.com>, Alison Schofield <alison.schofield@intel.com>,
	"Vishal Verma" <vishal.l.verma@intel.com>, Dan Williams
	<dan.j.williams@intel.com>
CC: <linux-cxl@vger.kernel.org>, <linux-kernel@vger.kernel.org>, Ira Weiny
	<ira.weiny@intel.com>
Subject: RE: [PATCH] cxl/pci: Skip irq features if MSI/MSI-X are not supported
Message-ID: <65a1c85d95649_37ad2945c@dwillia2-xfh.jf.intel.com.notmuch>
References: <20240111-dont-fail-irq-v1-1-802c22a79ecb@intel.com>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <20240111-dont-fail-irq-v1-1-802c22a79ecb@intel.com>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?V0fENChNb9wVaKxz5ezYKIyur8J2k1avj/1gVE/YhgP1i4eJ6/IQ6f1Pj6KX?=
 =?us-ascii?Q?1YGjawDOcBOUKPWlXpmQZtpmT8K+EwfbBRwGDPsq8H/7b1EXaTbTRHP1qNKI?=
 =?us-ascii?Q?6Rn4ZeFYVHPaCNlkTVVb91ClTcRaDEE1f2YNMKERmQ9ZzePUAzaHIO59BxUB?=
 =?us-ascii?Q?EKlFGOYv+DP6e6H7ajA81YYHqVJz6GVil8hA+TpkmnbQ6pyPgUyolRU+2Gwa?=
 =?us-ascii?Q?BvZOFurmq6NETvKldV7I5JHsFHt9PX8Y1yopACAo6Kcpl/NGXy+CF3ItjJuM?=
 =?us-ascii?Q?RQQuFyPncecjD4Zs7uTYjOhIWQOLBFRb+IfJkvASrYita18x5yUURVz6QYvc?=
 =?us-ascii?Q?LCB0yBqR6lR4OByAyDS2sOUyYvVvkmvIRim5xxjk+ViosyR2IZurCnpYUQWp?=
 =?us-ascii?Q?2omz3/skXouqW6b4m39ax5SqfOqOhS3TQ75IfyuzzqTpd6jcSwTUSm58onAG?=
 =?us-ascii?Q?MDecBRKrA+DnBAhNuQ6R/7ePnXkzxGNL4wmLwiWo8qVZu46iCrolPVotui80?=
 =?us-ascii?Q?QrOptbZNB9o44khRCwiVJWa0lU1jphbj0cQ22O1a4zJYptEm4OY891z4ErTD?=
 =?us-ascii?Q?QYmTIUJFgLnk/1lxr7Sjp2I7k04jEn4KSy88FipiJHl4LHxDnu4m42Y8dPFw?=
 =?us-ascii?Q?Hq9KFGWG+OYat74XT2YygFvuyuaf415CyDijiPycs/2TSDFilmE27U//oJqY?=
 =?us-ascii?Q?ykNJmYkwxyBNhLkdNHWJYMBMPKtSKQihECTStVaBmngtZruzoRCLtc5MiFPK?=
 =?us-ascii?Q?K72WOyAbRy+rhw8r6Eb2fV4vn7XPkFlAA9Tw+lTRtV6vovoN/YaoUP/jCt1e?=
 =?us-ascii?Q?T5L00FbmDmDrlT84jABQXEtR1PLoFe+e47vv4BLA9sGxgGivHb7X3qdVJxUN?=
 =?us-ascii?Q?DpC14o7eG+hkZ2Rg792gTSxQjpKYCb2g3/hvlJ+EFfkDsNFc1/QTu1ZDnLp1?=
 =?us-ascii?Q?/Xu5Z0EpzjVjgEdFbfloreIFVkOiaN+o2anUOGuxp85vTXY4ddxurMI4gATB?=
 =?us-ascii?Q?jcu/RvSFTX9CHcU7e5kAppbxLRzaSWSQGlGYBrE1pD0/KRTGasDjP0wCmsiT?=
 =?us-ascii?Q?2+jUhOlSRtLiPZBdfIL6ZAyImu909wMP8xdhW60KHAaESyOdGMRlGh/rauWT?=
 =?us-ascii?Q?rndWsXbHLwPUeh1jwhei22MkwykWWTTkQuv/YH5u6d1cgPEFDBPSt4YfPrqq?=
 =?us-ascii?Q?CyUjZreUWACRdDE4m47ZLZPrsd4obLaf5b9HAL4Re+pV4NUpxg01BUS+mo7f?=
 =?us-ascii?Q?SzncJuEhi1HOQbtygNADBSbTum9rBVoDccy2mDSH5JtAyFXO3Jzbr+6IHEF0?=
 =?us-ascii?Q?C4sOjUNXM4cgfhaKB7+V8tHNiQGSL/4hqgrptQSBdnH1L83xiD+4cCR0JBCL?=
 =?us-ascii?Q?hIYFjfNVz0IgGQxavLMRPEdQgv9t96LVJW0NjFYefxg7MfZJ7uc/qK3Cx1NX?=
 =?us-ascii?Q?ie8RP/nbAxdmzyL/1mfywfzk0W6zOQmXRUkLWlEXDems1/kI6pkm5/LxnqEc?=
 =?us-ascii?Q?5d2ZK5jj6vdLPwjmsSPNwy+cIQZNpkzsw6D45QRHH+XCpsfWF0rD6UG0piFT?=
 =?us-ascii?Q?1vlukfu5CfmVUifKTJUoKC78J3RDQ1nFJrYQkGc8R560o5631irqk2WjLIn8?=
 =?us-ascii?Q?hw=3D=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 1c26f7b3-e2e2-46d6-e68b-08dc13c48ce7
X-MS-Exchange-CrossTenant-AuthSource: PH8PR11MB8107.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 12 Jan 2024 23:16:47.9838
 (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: xDuD79DiP1vWLfPAd0f4/1C62VOTau23O4jC3BQVwzgr49jIqrqEZB/sb9KBZQDkofmmoyQw8/BR/85eqgx5H+RosSNKp8uNe/gi2P3FigI=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: IA0PR11MB7837
X-OriginatorOrg: intel.com

Ira Weiny wrote:
> CXL 3.1 Section 3.1.1 states:
> 
> 	"A Function on a CXL device must not generate INTx messages if
> 	that Function participates in CXL.cache protocol or CXL.mem
> 	protocols."
> 
> The generic CXL memory driver only supports devices which use the
> CXL.mem protocol.  The current driver attempts to allocate MSI/MSI-X
> vectors in anticipation of their need for mailbox interrupts or event
> processing.  However, the above requirement does not require a device to
> support interrupts, only that they use MSI/MSI-X.  For example, a device
> may disable mailbox interrupts and be configured for firmware first
> event processing and function well with the driver.
> 
> Rather than fail device probe if interrupts are not supported; flag that
> irqs are not enabled and avoid features which require interrupts.
> Emit messages appropriate for the situation to aid in debugging should
> device behavior be unexpected due to a failure to allocate vectors.
> 
> Note that it is possible for a device to have host based event
> processing through polling.  However, the driver does not support
> polling and it is not anticipated to be required.  Leave that case to
> the future if such a device comes along.
> 
> Signed-off-by: Ira Weiny <ira.weiny@intel.com>
> ---
> Changes in v1:
> - [djbw: remove persistent irq boolean]
> - [djbw: Simplify error messages]
> - [Alison: spell check]
> - [iweiny: test]
> - Link to RFC: https://lore.kernel.org/r/20240108-dont-fail-irq-v1-1-4407228debd2@intel.com
> ---
>  drivers/cxl/pci.c | 29 +++++++++++++++++++----------
>  1 file changed, 19 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> index 0155fb66b580..bd12b97bb38e 100644
> --- a/drivers/cxl/pci.c
> +++ b/drivers/cxl/pci.c
> @@ -381,7 +381,7 @@ static int cxl_pci_mbox_send(struct cxl_memdev_state *mds,
>  	return rc;
>  }
>  
> -static int cxl_pci_setup_mailbox(struct cxl_memdev_state *mds)
> +static int cxl_pci_setup_mailbox(struct cxl_memdev_state *mds, bool irq_avail)
>  {
>  	struct cxl_dev_state *cxlds = &mds->cxlds;
>  	const int cap = readl(cxlds->regs.mbox + CXLDEV_MBOX_CAPS_OFFSET);
> @@ -443,6 +443,11 @@ static int cxl_pci_setup_mailbox(struct cxl_memdev_state *mds)
>  	if (!(cap & CXLDEV_MBOX_CAP_BG_CMD_IRQ))
>  		return 0;
>  
> +	if (!irq_avail) {
> +		dev_err(cxlds->dev, "Mailbox irq enabled but no interrupt vectors.\n");
> +		return 0;
> +	}

It is just odd for this to be dev_err() when the very next thing this
function does is:

        irq = pci_irq_vector(to_pci_dev(cxlds->dev), msgnum); 
        if (irq < 0) 
                return 0; 

..which is silently return if using the interrupt does not work, or
there is a problem with the device where it returns an invalid @msgnum.
Why does one device "bug" deserve a dev_err() callout and the other
doesn't?

So at a minimum the dev_err() should be a dev_dbg(), but I would just
fold the @irq_avail check with the @cap check and be silent, because
nobody will care that this device is not sending mailbox interrupts
polling will just work.

> +
>  	msgnum = FIELD_GET(CXLDEV_MBOX_CAP_IRQ_MSGNUM_MASK, cap);
>  	irq = pci_irq_vector(to_pci_dev(cxlds->dev), msgnum);
>  	if (irq < 0)
> @@ -587,7 +592,7 @@ static int cxl_mem_alloc_event_buf(struct cxl_memdev_state *mds)
>  	return devm_add_action_or_reset(mds->cxlds.dev, free_event_buf, buf);
>  }
>  
> -static int cxl_alloc_irq_vectors(struct pci_dev *pdev)
> +static bool cxl_alloc_irq_vectors(struct pci_dev *pdev)
>  {
>  	int nvecs;
>  
> @@ -604,9 +609,9 @@ static int cxl_alloc_irq_vectors(struct pci_dev *pdev)
>  				      PCI_IRQ_MSIX | PCI_IRQ_MSI);
>  	if (nvecs < 1) {
>  		dev_dbg(&pdev->dev, "Failed to alloc irq vectors: %d\n", nvecs);
> -		return -ENXIO;
> +		return false;
>  	}
> -	return 0;
> +	return true;
>  }
>  
>  static irqreturn_t cxl_event_thread(int irq, void *id)
> @@ -742,7 +747,7 @@ static bool cxl_event_int_is_fw(u8 setting)
>  }
>  
>  static int cxl_event_config(struct pci_host_bridge *host_bridge,
> -			    struct cxl_memdev_state *mds)
> +			    struct cxl_memdev_state *mds, bool irq_avail)
>  {
>  	struct cxl_event_interrupt_policy policy;
>  	int rc;
> @@ -754,6 +759,11 @@ static int cxl_event_config(struct pci_host_bridge *host_bridge,
>  	if (!host_bridge->native_cxl_error)
>  		return 0;
>  
> +	if (!irq_avail) {
> +		dev_info(mds->cxlds.dev, "No interrupt vectors, no polling, skip event processing.\n");

Not going to demand dev_dbg() here just because a device is mandated to
have event command support and it is driver policy to skip enabling it.
However, do drop the "no polling" that's implied. So I think it is:

"No interrupt support, disable event processing."