Re: [PATCH v2 2/4] cxl/pci: Cleanup 'sanitize' to always poll

[Dan Williams <dan.j.williams@intel.com>]

Jonathan Cameron wrote:
> On Fri, 29 Sep 2023 16:09:39 -0700
> Dan Williams <dan.j.williams@intel.com> wrote:
> 
> > In preparation for fixing the init/teardown of the 'sanitize' workqueue
> > and sysfs notification mechanism, arrange for cxl_mbox_sanitize_work()
> > to be the single location where the sysfs attribute is notified. With
> > that change there is no distinction between polled mode and interrupt
> > mode. All the interrupt does is accelerate the polling interval.
> > 
> > The change to check for "mds->security.sanitize_node" under the lock is
> > there to ensure that the interrupt, the work routine and the
> > setup/teardown code can all have a consistent view of the registered
> > notifier and the workqueue state. I.e. the expectation is that the
> > interrupt is live past the point that the sanitize sysfs attribute is
> > published, and it may race teardown, so it must be consulted under a
> > lock.
> > 
> > Lastly, some opportunistic replacements of
> > "queue_delayed_work(system_wq, ...)", which is just open coded
> > schedule_delayed_work(), are included.
> > 
> > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> One trivial comment inline, otherwise looks fine to me
> 
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 
> > ---
> >  drivers/cxl/core/memdev.c |    3 +-
> >  drivers/cxl/cxlmem.h      |    2 --
> >  drivers/cxl/pci.c         |   60 +++++++++++++++++++--------------------------
> >  3 files changed, 26 insertions(+), 39 deletions(-)
> > 
> > diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c
> > index 14b547c07f54..2a7a07f6d165 100644
> > --- a/drivers/cxl/core/memdev.c
> > +++ b/drivers/cxl/core/memdev.c
> > @@ -561,8 +561,7 @@ static void cxl_memdev_security_shutdown(struct device *dev)
> >  	struct cxl_memdev *cxlmd = to_cxl_memdev(dev);
> >  	struct cxl_memdev_state *mds = to_cxl_memdev_state(cxlmd->cxlds);
> >  
> > -	if (mds->security.poll)
> > -		cancel_delayed_work_sync(&mds->security.poll_dwork);
> > +	cancel_delayed_work_sync(&mds->security.poll_dwork);
> >  }
> >  
> >  static void cxl_memdev_shutdown(struct device *dev)
> > diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
> > index 706f8a6d1ef4..55f00ad17a77 100644
> > --- a/drivers/cxl/cxlmem.h
> > +++ b/drivers/cxl/cxlmem.h
> > @@ -360,7 +360,6 @@ struct cxl_fw_state {
> >   *
> >   * @state: state of last security operation
> >   * @enabled_cmds: All security commands enabled in the CEL
> > - * @poll: polling for sanitization is enabled, device has no mbox irq support
> >   * @poll_tmo_secs: polling timeout
> >   * @poll_dwork: polling work item
> >   * @sanitize_node: sanitation sysfs file to notify
> > @@ -368,7 +367,6 @@ struct cxl_fw_state {
> >  struct cxl_security_state {
> >  	unsigned long state;
> >  	DECLARE_BITMAP(enabled_cmds, CXL_SEC_ENABLED_MAX);
> > -	bool poll;
> >  	int poll_tmo_secs;
> >  	struct delayed_work poll_dwork;
> >  	struct kernfs_node *sanitize_node;
> > diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> > index aa1b3dd9e64c..ac4e434b0806 100644
> > --- a/drivers/cxl/pci.c
> > +++ b/drivers/cxl/pci.c
> > @@ -128,10 +128,10 @@ static irqreturn_t cxl_pci_mbox_irq(int irq, void *id)
> >  	reg = readq(cxlds->regs.mbox + CXLDEV_MBOX_BG_CMD_STATUS_OFFSET);
> >  	opcode = FIELD_GET(CXLDEV_MBOX_BG_CMD_COMMAND_OPCODE_MASK, reg);
> >  	if (opcode == CXL_MBOX_OP_SANITIZE) {
> > +		mutex_lock(&mds->mbox_mutex);
> >  		if (mds->security.sanitize_node)
> > -			sysfs_notify_dirent(mds->security.sanitize_node);
> > -
> > -		dev_dbg(cxlds->dev, "Sanitization operation ended\n");
> > +			mod_delayed_work(system_wq, &mds->security.poll_dwork, 0);
> > +		mutex_unlock(&mds->mbox_mutex);
> >  	} else {
> >  		/* short-circuit the wait in __cxl_pci_mbox_send_cmd() */
> >  		rcuwait_wake_up(&mds->mbox_wait);
> > @@ -160,8 +160,7 @@ static void cxl_mbox_sanitize_work(struct work_struct *work)
> >  		int timeout = mds->security.poll_tmo_secs + 10;
> >  
> >  		mds->security.poll_tmo_secs = min(15 * 60, timeout);
> > -		queue_delayed_work(system_wq, &mds->security.poll_dwork,
> > -				   timeout * HZ);
> > +		schedule_delayed_work(&mds->security.poll_dwork, timeout * HZ);
> >  	}
> >  	mutex_unlock(&mds->mbox_mutex);
> >  }
> > @@ -293,15 +292,11 @@ static int __cxl_pci_mbox_send_cmd(struct cxl_memdev_state *mds,
> >  		 * and allow userspace to poll(2) for completion.
> >  		 */
> >  		if (mbox_cmd->opcode == CXL_MBOX_OP_SANITIZE) {
> > -			if (mds->security.poll) {
> > -				/* give first timeout a second */
> > -				timeout = 1;
> > -				mds->security.poll_tmo_secs = timeout;
> > -				queue_delayed_work(system_wq,
> > -						   &mds->security.poll_dwork,
> > -						   timeout * HZ);
> > -			}
> > -
> > +			/* give first timeout a second */
> > +			timeout = 1;
> > +			mds->security.poll_tmo_secs = timeout;
> > +			schedule_delayed_work(&mds->security.poll_dwork,
> > +					      timeout * HZ);
> >  			dev_dbg(dev, "Sanitization operation started\n");
> >  			goto success;
> >  		}
> > @@ -384,7 +379,9 @@ static int cxl_pci_setup_mailbox(struct cxl_memdev_state *mds)
> >  	const int cap = readl(cxlds->regs.mbox + CXLDEV_MBOX_CAPS_OFFSET);
> >  	struct device *dev = cxlds->dev;
> >  	unsigned long timeout;
> > +	int irq, msgnum;
> >  	u64 md_status;
> > +	u32 ctrl;
> >  
> >  	timeout = jiffies + mbox_ready_timeout * HZ;
> >  	do {
> > @@ -432,33 +429,26 @@ static int cxl_pci_setup_mailbox(struct cxl_memdev_state *mds)
> >  	dev_dbg(dev, "Mailbox payload sized %zu", mds->payload_size);
> >  
> >  	rcuwait_init(&mds->mbox_wait);
> > +	INIT_DELAYED_WORK(&mds->security.poll_dwork, cxl_mbox_sanitize_work);
> >  
> > -	if (cap & CXLDEV_MBOX_CAP_BG_CMD_IRQ) {
> > -		u32 ctrl;
> > -		int irq, msgnum;
> > -		struct pci_dev *pdev = to_pci_dev(cxlds->dev);
> > -
> > -		msgnum = FIELD_GET(CXLDEV_MBOX_CAP_IRQ_MSGNUM_MASK, cap);
> > -		irq = pci_irq_vector(pdev, msgnum);
> > -		if (irq < 0)
> > -			goto mbox_poll;
> > -
> > -		if (cxl_request_irq(cxlds, irq, cxl_pci_mbox_irq, NULL))
> > -			goto mbox_poll;
> > +	/* background command interrupts are optional */
> > +	if ((cap & CXLDEV_MBOX_CAP_BG_CMD_IRQ) == 0)
> 
> Nit, but it's a boolean flag, so to me
> 	if (!(cap & CXLDEV_MBOX_CAP_BG_CMD_IRQ))
> 
> feels slightly better.
> 
> There are instances of this done with !() elsewhere in the file,
> so not obviously a case of wanting to match local style.

Ok.