Re: [PATCH 2/3] cxl/mbox: support aborting the current background operation

[Davidlohr Bueso]

On Tue, 14 Jan 2025, Dave Jiang wrote:

> On 10/21/24 8:18 PM, Davidlohr Bueso wrote:
> > CXL 3.1 introduced the ability to request that the current on-going
> > background command be aborted. Add support for this, where the current
> > policy is for the request to occur whenever a new incoming bg command
> > wants to run. As such everything is left to user discretion and it
> > becomes impossible to hog the device/mailbox.
>
> Are you trying to say that the patch is changing the current behavior to where every time a new bg command comes in, it will abort the previous one?

Yes.

> > The context of doing the cancellation request is the same as the new
> > incoming command, and will always hold the mbox_mutex, guaranteeing
> > that any successful cancel does not race with a third thread coming
> > in and stealing the effort.
> >
> > - For Sanitize, the thread doing the will cancel the work, and clean
>
> doing the? seems to be missing a word here.

'doing the request', will update.

...

> > +/*
> > + * Return true implies that the request was successful and the on-going
> > + * background operation was in fact aborted. This also guarantees that
> > + * the respective thread is done.
> > + */
> > +static bool cxl_try_to_cancel_background(struct cxl_mailbox *cxl_mbox)
> > +{
> > +	int rc;
> > +	struct cxl_dev_state *cxlds = mbox_to_cxlds(cxl_mbox);
> > +	struct cxl_memdev_state *mds = to_cxl_memdev_state(cxlds);
> > +	struct device *dev = cxlds->dev;
> > +	struct cxl_mbox_cmd cmd = {
> > +		.opcode = CXL_MBOX_OP_REQUEST_ABORT_BG_OP
> > +	};
> > +
> > +	lockdep_assert_held(&cxl_mbox->mbox_mutex);
> > +
> > +	rc = __cxl_pci_mbox_send_cmd(cxl_mbox, &cmd);
> > +	if (rc) {
> > +		dev_dbg(dev, "Failed to send abort request : %d\n", rc);
> > +		return false;
> > +	}
> > +
> > +	if (!cxl_mbox_background_complete(cxlds))
> > +		return false;
> > +
> > +	if (mds->security.sanitize_active) {
> > +		/*
> > +		 * Cancel the work and cleanup on its behalf - we hold
> > +		 * the mbox_mutex, cannot race with cxl_mbox_sanitize_work().
> > +		 */
> > +		cancel_delayed_work_sync(&mds->security.poll_dwork);
> > +		mds->security.poll_tmo_secs = 0;
> > +		if (mds->security.sanitize_node)
> > +			sysfs_notify_dirent(mds->security.sanitize_node);
> > +		mds->security.sanitize_active = false;
>
> Should this line happen before the sysfs notification?

It is benign as we hold the lock, but yes. I will also abstract this in a helper,
such that both cxl_mbox_sanitize_work() and cxl_try_to_cancel_background() can
use it.

Thanks,
Davidlohr