Re: [PATCH 08/11] cxl/mem: Wire up event interrupts

[Ira Weiny <ira.weiny@intel.com>]

On Tue, Nov 15, 2022 at 04:13:24PM -0700, Jiang, Dave wrote:
> 
> 
> On 11/10/2022 10:57 AM, ira.weiny@intel.com wrote:

[snip]

> > +int cxl_event_config_msgnums(struct cxl_dev_state *cxlds)
> > +{
> > +	struct cxl_event_interrupt_policy *policy = &cxlds->evt_int_policy;
> > +	size_t policy_size = sizeof(*policy);
> > +	bool retry = true;
> > +	int rc;
> > +
> > +	policy->info_settings = CXL_INT_MSI_MSIX;
> > +	policy->warn_settings = CXL_INT_MSI_MSIX;
> > +	policy->failure_settings = CXL_INT_MSI_MSIX;
> > +	policy->fatal_settings = CXL_INT_MSI_MSIX;
> > +	policy->dyn_cap_settings = CXL_INT_MSI_MSIX;
> > +
> > +again:
> > +	rc = cxl_mbox_send_cmd(cxlds, CXL_MBOX_OP_SET_EVT_INT_POLICY,
> > +			       policy, policy_size, NULL, 0);
> > +	if (rc < 0) {
> > +		/*
> > +		 * If the device does not support dynamic capacity it may fail
> > +		 * the command due to an invalid payload.  Retry without
> > +		 * dynamic capacity.
> > +		 */
> > +		if (retry) {
> > +			retry = false;
> > +			policy->dyn_cap_settings = 0;
> > +			policy_size = sizeof(*policy) - sizeof(policy->dyn_cap_settings);
> > +			goto again;
> > +		}
> > +		dev_err(cxlds->dev, "Failed to set event interrupt policy : %d",
> > +			rc);
> > +		memset(policy, CXL_INT_NONE, sizeof(*policy));
> > +		return rc;
> > +	}
> 
> Up to you, but I think you can avoid the goto:

I think this is a bit more confusing because we are not really retrying 2
times.

> 
> 	int retry = 2;
> 	do {
> 		rc = cxl_mbox_send_cmd(...);
> 		if (rc == 0 || retry == 1)

Specifically this looks confusing to me.  Why break on retry == 1?

> 			break;
> 		policy->dyn_cap_settings = 0;
> 		policy_size = sizeof(*policy) - sizeof(policy->dyn_cap_settings);
> 		retry--;
> 	} while (retry);
> 
> 	if (rc < 0) {
> 		dev_err(...);
> 		memset(policy, ...);
> 		return rc;
> 	}

That said perhaps the retry should be based on policy_size...  :-/  I'm not
sure that adds much.  I'm going to leave it as is.

[snip]

> > +
> > +static irqreturn_t cxl_event_int_handler(int irq, void *id)
> > +{
> > +	struct cxl_event_irq_id *cxlid = id;
> > +	struct cxl_dev_state *cxlds = cxlid->cxlds;
> > +	u32 status = readl(cxlds->regs.status + CXLDEV_DEV_EVENT_STATUS_OFFSET);
> > +
> > +	if (cxlid->status & status)
> > +		return IRQ_WAKE_THREAD;
> > +	return IRQ_HANDLED;
> 
> IRQ_NONE since your handler did not handle anything and this is a shared
> interrupt?

Yes.  Good catch thanks!

> 
> > +}
> > +
> > +static void cxl_free_event_irq(void *id)
> > +{
> > +	struct cxl_event_irq_id *cxlid = id;
> > +	struct pci_dev *pdev = to_pci_dev(cxlid->cxlds->dev);
> > +
> > +	pci_free_irq(pdev, cxlid->msgnum, id);
> > +}
> > +
> > +static u32 log_type_to_status(enum cxl_event_log_type log_type)
> > +{
> > +	switch (log_type) {
> > +	case CXL_EVENT_TYPE_INFO:
> > +		return CXLDEV_EVENT_STATUS_INFO | CXLDEV_EVENT_STATUS_DYNAMIC_CAP;
> > +	case CXL_EVENT_TYPE_WARN:
> > +		return CXLDEV_EVENT_STATUS_WARN;
> > +	case CXL_EVENT_TYPE_FAIL:
> > +		return CXLDEV_EVENT_STATUS_FAIL;
> > +	case CXL_EVENT_TYPE_FATAL:
> > +		return CXLDEV_EVENT_STATUS_FATAL;
> > +	default:
> > +		break;
> > +	}
> > +	return 0;
> > +}
> > +
> > +static int cxl_request_event_irq(struct cxl_dev_state *cxlds,
> > +				 enum cxl_event_log_type log_type,
> > +				 u8 setting)
> > +{
> > +	struct device *dev = cxlds->dev;
> > +	struct pci_dev *pdev = to_pci_dev(dev);
> > +	struct cxl_event_irq_id *id;
> > +	unsigned int msgnum = CXL_EVENT_INT_MSGNUM(setting);
> > +	int irq;
> 
> int rc? pci_request_irq() returns an errno or 0, not the number of irq. The
> variable naming is a bit confusing.

Indeed.  Changed, and thanks,
Ira

> 
> DJ
>