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

[Davidlohr Bueso <dave@stgolabs.net>]

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