Received-SPF: pass (google.com: domain of linux-kernel+bounces-24839-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Message-ID: <f4cabd98-2759-4cd3-81ac-4afdf9aadf56@intel.com>
Date: Fri, 12 Jan 2024 09:05:42 -0700
User-Agent: Betterbird (Linux)
Subject: Re: [PATCH] cxl/pci: Skip irq features if MSI/MSI-X are not supported
To: Ira Weiny <ira.weiny@intel.com>, Davidlohr Bueso <dave@stgolabs.net>,
	Jonathan Cameron <jonathan.cameron@huawei.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>
References: <20240111-dont-fail-irq-v1-1-802c22a79ecb@intel.com>
Content-Language: en-US
From: Dave Jiang <dave.jiang@intel.com>
In-Reply-To: <20240111-dont-fail-irq-v1-1-802c22a79ecb@intel.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?Z1AxaklFS21YSk02d1k0WXdmMGl1NzBFTDlKdnhhdGxKOUhUVkgrRnhwSFcw?=
 =?utf-8?B?SVlmaTBYd2hReU1SL1lrSUlzQ1NJOUFnUFRlNG9WSEZRVTVCSDhrdHp6YW04?=
 =?utf-8?B?WC9zVkVxZitxQnorclVGcWM3eEdqNVJZN2lwc1VsdWlhT0Z0bFVWdTBKVVd3?=
 =?utf-8?B?SkVsUWJOMEJ0Vnp0SHVndEFNbVR4d1hqTmZ3S3RnUzZWdk9HS2p0c2VPeW1m?=
 =?utf-8?B?OTBXM0dXRTREei84QTFwTW40SXZ5a2E0d0RSZS9scnVxd0MzUVNKdjk0TzJv?=
 =?utf-8?B?bnExOWw5KzFzU2lja1hhd0ljMnBpTFZ0aXRRWnMvc0NxOEphb3BTVER5b3ht?=
 =?utf-8?B?di9FSTlqRzJrT2tTOTEwTmF3eVY4YUVOSzJlS1A0TSt1a3JBRmdBNGZjT3lB?=
 =?utf-8?B?UzFYK1FFSUpDQTIveFREV2xYTWdKc05YWFhSdm1nOXR6d0pPY3hVbGZJMXpV?=
 =?utf-8?B?T016U09RT3gyeXhBMlVod2E2a2RyQlZiNk9FcnNSV2JBMmd5NEg2c3ovWDFl?=
 =?utf-8?B?SDRBaS9tTHBxM25RUVdPL08zR1c4Nmh2dWFtNDdtTmhpcHYzVnhsTzBOV1dH?=
 =?utf-8?B?U0xMSEtvbUEySDZqalY2di9lK2cweklyYlNFaXlOaDlzcXNLVjJCaU1TK1Fo?=
 =?utf-8?B?Tmt3NnJ5c0VBOTJBUzBKZWJ2ZDlWTWc0S0tERkJ3d1lNVnRpazd5MmNTOUda?=
 =?utf-8?B?YW5scmluNDhTK29vVWVFM25YKzlhNmdOd1Z0aC9RSWlLa0Jkd2RuNWZyNDJw?=
 =?utf-8?B?TDZvclIyTk9JNmFBZWF4TXJzbEpTeTFWeFNQNHowWUJmd3ZoYnF3Rzd0eEFV?=
 =?utf-8?B?bU94Ylo1YTNWOE90S0NwN1NpOTdXUDRZNC9DZm1xam91RytlTGM5M3VMOGU1?=
 =?utf-8?B?UTlzdS9Qdk5DOVdrS1ZwNUNtYUdDbUhWYjdVOXZkcm9BZno5RkxhajlScDNG?=
 =?utf-8?B?WFQ3UFRYL1pMNzVwcUhoRitBUnJaMWVTVEV4dVNmelpRZkJUWXIrbUdJYWs3?=
 =?utf-8?B?Y29LVjFqeHcwVlBQVk5QSk5wWkhBaUlia051MGVUR0R1VXMrUjVTc0lCcTZ3?=
 =?utf-8?B?R09yVVJXbmhnUUN2RWFzSW9aTTc1YmhiTU9hR2ppaHZRWWtKNzhSSUNVZ0hQ?=
 =?utf-8?B?SnQ5cW9JTXBIUkJnUFptNEYvNUJ6THErT2FncE9UUFFRQVJsL0hCZ2k1UFNT?=
 =?utf-8?B?L0pnYS96UzdHc2xzV0d3S2wxK3dWb2FHQmlneE1vMnp0aWRLR0k0M21STi9y?=
 =?utf-8?B?cFFRU0VKL0FQSnBSSFduQTRPbHJZakRZSC9yR3F2YjdUb3N2clMvQlk2RUdD?=
 =?utf-8?B?WTNuOHIvZ0lWKzR5ejVheDU5dWZqanB6ejdIbm9JR05pd011MzBVZnpGWUdp?=
 =?utf-8?B?dUxlVkkzQmYyQUlYcmtLa3dKTkRoNkpacDd3ai8zcU53TGpxMGhVNGVtRkgr?=
 =?utf-8?B?aExOMlRtOXErcmRVMXp2dG93UWJ5Wk1zOVBROC9jNWlNb2ZHLy8vQUMwb3JK?=
 =?utf-8?B?MEdVQS9XTVY5ZFQxY3VWZnl0M3ltU2RmaCtHZDZUMUdMNnU5VmRYUEJxWGVL?=
 =?utf-8?B?SldWeWl3TVY4QWxRdXlKeWt5WDZsZVp6Njd4MVc2SlBJSGJlZEsrb2RRRk5L?=
 =?utf-8?B?NjNRSjVhT0tVNDVPenlYU2ZoVlh3YVd5M0tDZXUrcE1MNmlFWGpscjF5YWpt?=
 =?utf-8?B?T0lLWFhGTnh2bHRkcmRWUlB3SENveUY1bDBBN1JhYUpEMmg1Szk3d3B0bFdW?=
 =?utf-8?B?Y3JmeTB1aS9vMU9HZWVHenU2WHFrU0VyTTNmUVFyTTRKK0I0OFZ4aUFQR0lX?=
 =?utf-8?B?RlY3dXZUY0NNbmhwZVBlOVV4NXhlQmlFWDc0M2N1VWZmczZSWWtld3ZoYUZB?=
 =?utf-8?B?MjdZckhrOTBCNjlOK1ZGV0JHMkFBUDJwOENTV29QYk5DOE1HTlhsREVIUkcy?=
 =?utf-8?B?Uk9GUk9vMGNQMGZWWlBENjgwMUR0UjBHc2RqUTFQeEFqeVE2TEpiQjhCbmZs?=
 =?utf-8?B?VTBFaUJsOE5BM0pMVzNWMjljTkl6eEhVMjcyQWl0a3VYMWM5Zm9BV2Q5U094?=
 =?utf-8?B?WEkrME9EUW1OWVd1V3pFWDUzVmk5RjRyc1NyVUtKT0RPSDRMTmxkVnd6dmpY?=
 =?utf-8?Q?9lVNunKkfgOHp9jbyC8UGwnYc?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 0e31a4cc-984a-4c7c-78ea-08dc138858c1
X-MS-Exchange-CrossTenant-AuthSource: PH7PR11MB5984.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 12 Jan 2024 16:05:50.6739
 (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: XR2EXAN7vvBl4zQhwf94fjP3R1Q6WU93vcBm29BnlNjFYbjXylPkpNp8IJQ30R0blLNmagW11951k2inOxHtjg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM4PR11MB5295
X-OriginatorOrg: intel.com


On 1/11/24 18:54, 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>

v2?

Reviewed-by: Dave Jiang <dave.jiang@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;
> +	}
> +
>  	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");
> +		return 0;
> +	}
> +
>  	rc = cxl_mem_alloc_event_buf(mds);
>  	if (rc)
>  		return rc;
> @@ -788,6 +798,7 @@ static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>  	struct cxl_register_map map;
>  	struct cxl_memdev *cxlmd;
>  	int i, rc, pmu_count;
> +	bool irq_avail;
>  
>  	/*
>  	 * Double check the anonymous union trickery in struct cxl_regs
> @@ -845,11 +856,9 @@ static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>  	else
>  		dev_warn(&pdev->dev, "Media not active (%d)\n", rc);
>  
> -	rc = cxl_alloc_irq_vectors(pdev);
> -	if (rc)
> -		return rc;
> +	irq_avail = cxl_alloc_irq_vectors(pdev);
>  
> -	rc = cxl_pci_setup_mailbox(mds);
> +	rc = cxl_pci_setup_mailbox(mds, irq_avail);
>  	if (rc)
>  		return rc;
>  
> @@ -908,7 +917,7 @@ static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>  		}
>  	}
>  
> -	rc = cxl_event_config(host_bridge, mds);
> +	rc = cxl_event_config(host_bridge, mds, irq_avail);
>  	if (rc)
>  		return rc;
>  
> 
> ---
> base-commit: 0dd3ee31125508cd67f7e7172247f05b7fd1753a
> change-id: 20240108-dont-fail-irq-a96310368f0f
> 
> Best regards,