Re: [PATCH 13/26] cxl/mem: Configure dynamic capacity interrupts

Alison Schofield
2024-04-10T23:23:53Z

On Sun, Mar 24, 2024 at 04:18:16PM -0700, Ira Weiny wrote:
> From: Navneet Singh <navneet.singh@intel.com>
>
> Dynamic Capacity Devices (DCD) support extent change notifications
> through the event log mechanism. The interrupt mailbox commands were
> extended in CXL 3.1 to support these notifications.
>
> Firmware can't configure DCD events to be FW controlled but can retain
> control of memory events. Split irq configuration of memory events and
> DCD events to allow for FW control of memory events while DCD is host
> controlled.
>
> Configure DCD event log interrupts on devices supporting dynamic
> capacity. Disable DCD if interrupts are not supported.
>
> Signed-off-by: Navneet Singh <navneet.singh@intel.com>
> Co-developed-by: Ira Weiny <ira.weiny@intel.com>
> Signed-off-by: Ira Weiny <ira.weiny@intel.com>
>
> ---

snip

> diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> index 12cd5d399230..ef482eae09e9 100644
> --- a/drivers/cxl/pci.c
> +++ b/drivers/cxl/pci.c

snip

>
> +static int cxl_irqsetup(struct cxl_memdev_state *mds,
> + struct cxl_event_interrupt_policy *policy,
> + bool native_cxl)
> +{
> + struct cxl_dev_state *cxlds = &mds->cxlds;
> + int rc;
> +
> + if (native_cxl) {
> + rc = cxl_event_irqsetup(mds, policy);
> + if (rc)
> + return rc;
> + }
> +
> + if (cxl_dcd_supported(mds)) {
> + rc = cxl_event_req_irq(cxlds, policy->dcd_settings);
> + if (rc) {
> + dev_err(cxlds->dev, "Failed to get interrupt for DCD event log\n");
move this..

> + cxl_disable_dcd(mds);

to after you've done the disabling...

dev_err(cxlds->dev, "DCD disabled: failed to get interrupt for event log\n");
> + return rc;

not sure I got the words right.

> + }
> + }
> +
> + return 0;
> +}
> +
> static bool cxl_event_int_is_fw(u8 setting)
> {
> u8 mode = FIELD_GET(CXLDEV_EVENT_INT_MODE_MASK, setting);
> @@ -757,17 +793,25 @@ static int cxl_event_config(struct pci_host_bridge *host_bridge,
> struct cxl_memdev_state *mds, bool irq_avail)
> {
> struct cxl_event_interrupt_policy policy = { 0 };
> + bool native_cxl = host_bridge->native_cxl_error;
> int rc;
>
> /*
> * When BIOS maintains CXL error reporting control, it will process
> * event records. Only one agent can do so.
> + *
> + * If BIOS has control of events and DCD is not supported skip event
> + * configuration.
> */
> - if (!host_bridge->native_cxl_error)
> + if (!native_cxl && !cxl_dcd_supported(mds))
> return 0;
>
> if (!irq_avail) {
> dev_info(mds->cxlds.dev, "No interrupt support, disable event processing.\n");
> + if (cxl_dcd_supported(mds)) {
> + dev_info(mds->cxlds.dev, "DCD requires interrupts, disable DCD\n");

Similar here -
Maybe better to disable, and just say it's done because this sounds a bit like a request to the user.

> + cxl_disable_dcd(mds);

dev_info(mds->cxlds.dev, "DCD disabled: no interrupt support\n");

How come this one is dev_info() and prior case of disabling was a
dev_err()/

snip to end

-- Alison