Re: [PATCH 2/7] cxl/mbox: Add background cmd handling machinery

[Jonathan Cameron <Jonathan.Cameron@Huawei.com>]

> >  
> >>   * @return_code: (output) Error code returned from hardware.
> >>   *
> >>   * This is the primary mechanism used to send commands to the hardware.
> >> @@ -123,6 +126,8 @@ struct cxl_mbox_cmd {
> >>	size_t size_in;
> >>	size_t size_out;
> >>	size_t min_out;
> >> +	int poll_count;
> >> +	int poll_interval;
> >>	u16 return_code;
> >>  };
> >>
> >> diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> >> index 39b829a29f6c..aa1bb74a52a1 100644
> >> --- a/drivers/cxl/pci.c
> >> +++ b/drivers/cxl/pci.c
> >> @@ -51,6 +51,7 @@
> >>  static unsigned short mbox_ready_timeout = 60;
> >>  module_param(mbox_ready_timeout, ushort, 0644);
> >>  MODULE_PARM_DESC(mbox_ready_timeout, "seconds to wait for mailbox ready");
> >> +static DECLARE_WAIT_QUEUE_HEAD(mbox_wait);  
> >
> >I see in discussion you are moving to a per device approach so I won't review
> >that bit on this version.  
> 
> Right, fyi the latest vesion is here:
> 

> https://lore.kernel.org/linux-cxl/gtvozgdx2ak7tekc3heczk5g7gj3cwuoptez6tjmkecader4lo@7t2em7rclcxn/
I'll pretend I'll look at that :)
(81 more emails to read on CXL alone.. sigh


> >  
> >>		dev_dbg(dev, "Mailbox operation had an error: %s\n",
> >>			cxl_mbox_cmd_rc2str(mbox_cmd));
> >>		return 0; /* completed but caller must check return_code */
> >> @@ -224,6 +304,7 @@ static int cxl_pci_setup_mailbox(struct cxl_dev_state *cxlds)
> >>	const int cap = readl(cxlds->regs.mbox + CXLDEV_MBOX_CAPS_OFFSET);
> >>	unsigned long timeout;
> >>	u64 md_status;
> >> +	int rc, irq;
> >>
> >>	timeout = jiffies + mbox_ready_timeout * HZ;
> >>	do {
> >> @@ -272,6 +353,27 @@ static int cxl_pci_setup_mailbox(struct cxl_dev_state *cxlds)
> >>	dev_dbg(cxlds->dev, "Mailbox payload sized %zu",
> >>		cxlds->payload_size);
> >>
> >> +	if (cap & CXLDEV_MBOX_CAP_BG_CMD_IRQ) {
> >> +		struct pci_dev *pdev = to_pci_dev(cxlds->dev);
> >> +
> >> +		irq = pci_irq_vector(pdev,
> >> +			     FIELD_GET(CXLDEV_MBOX_CAP_IRQ_MSGNUM_MASK, cap));
> >> +		if (irq < 0)
> >> +			goto mbox_poll;
> >> +
> >> +		rc = devm_request_irq(cxlds->dev, irq, cxl_pci_mbox_irq,
> >> +				      IRQF_SHARED, "mailbox", cxlds);
> >> +		if (rc)
> >> +			goto mbox_poll;  
> >
> >Hmm. The old argument of whether to carry on when something unexpected happens.  
> 
> Well yes and no. The reason I am very tolerant upon errors here is that the
> background cmd polling will be done regardless of the device's interrupt
> capability. So I find it way too harsh to just fail the probe altogether
> when effectively no harm is done.

We'll never teach people to do things right if their broken config works anyway! :)