Re: [PATCH] cxl/features: Serialize multi-part Get/Set Feature transfers

On Mon, Jul 13, 2026 at 10:15:07AM +0800, Dave Jiang wrote:
> 
> 
> On 7/13/26 2:52 AM, Richard Cheng wrote:
> > On Thu, Jul 09, 2026 at 08:58:41AM +0800, Dave Jiang wrote:
> >> A Get or Set Feature payload larger than the mailbox payload size is
> >> split into several mailbox commands. mbox_mutex only serializes
> >> individual mailbox commands and is dropped between iterations of these
> >> loops. Nothing serializes the multi-part transfer as a whole.
> >> cxl_get_feature() and cxl_set_feature() are reachable concurrently
> >> from fwctl (per-fd RPCs run under a read-held registration lock) and
> >> from the EDAC scrub/ECS/repair paths, so two transfers to the same
> >> mailbox can interleave their parts and corrupt the device's transfer
> >> context.
> >>
> > 
> > Hi Dave,
> > 
> > I have some opinion maybe worth discussing.
> > 
> > Get Feature does not have transfer state, it uses an offset and count.
> > For Set Feature, the device must reject a second transfer with Feature
> > Transfer in progress.
> > 
> > Maybe we should reword this as preventing concurrent Set failures and
> > preventing a multi-part Get from spanning a Set update? I don't think
> > "currupt the device's transfer context" is accutate.
> 
> I'll update that.
> 
> > 
> >> Add a per-mailbox feat_mutex and hold it across the whole transfer in
> >> both functions. It nests outside mbox_mutex (which is taken inside
> >> cxl_internal_send_cmd()), and is taken nowhere else, so no lock-ordering
> >> inversion is introduced.
> >>
> >> Link: https://sashiko.dev/#/patchset/[email protected]?part=1
> >> Fixes: 5e5ac21f629d ("cxl/mbox: Add GET_FEATURE mailbox command")
> >> Fixes: 14d502cc2718 ("cxl/mbox: Add SET_FEATURE mailbox command")
> >> Assisted-by: Claude:claude-opus-4-8
> >> Signed-off-by: Dave Jiang <[email protected]>
> >> ---
> >>  drivers/cxl/core/features.c | 3 +++
> >>  drivers/cxl/core/mbox.c     | 1 +
> >>  include/cxl/mailbox.h       | 2 ++
> >>  3 files changed, 6 insertions(+)
> >>
> >> diff --git a/drivers/cxl/core/features.c b/drivers/cxl/core/features.c
> >> index 85185af46b72..85d6bcdc360e 100644
> >> --- a/drivers/cxl/core/features.c
> >> +++ b/drivers/cxl/core/features.c
> >> @@ -240,6 +240,8 @@ size_t cxl_get_feature(struct cxl_mailbox *cxl_mbox, const uuid_t *feat_uuid,
> >>  	size_out = min(feat_out_size, cxl_mbox->payload_size);
> >>  	uuid_copy(&pi.uuid, feat_uuid);
> >>  	pi.selection = selection;
> >> +
> >> +	guard(mutex)(&cxl_mbox->feat_mutex);
> >>  	do {
> >>  		data_to_rd_size = min(feat_out_size - data_rcvd_size,
> >>  				      cxl_mbox->payload_size);
> >> @@ -314,6 +316,7 @@ int cxl_set_feature(struct cxl_mailbox *cxl_mbox,
> >>  		data_in_size = cxl_mbox->payload_size - hdr_size;
> >>  	}
> >>  
> >> +	guard(mutex)(&cxl_mbox->feat_mutex);
> > 
> > What about RAW command path ?
> > With CONFIG_CXL_MEM_RAW_COMMANDS=y, GET_FEATURE and SET_FEATURE can be
> > sent directly through mbox_send() without taking feat_mutex.
> > 
> > Or you want to keep the scope minimum so RAW path is out of scope ?
> 
> Yeah I was only trying to fix fwctl side with this patch.
> 
> DJ

Hi Dave,

No problem, then I think I can handle the RAW path part, I'll send v1
later on top of this.

Reviewed-by: Richard Cheng <[email protected]>

--Richard
> > 
> > Otherwise the locking and ordering looks correct to me.
> > 
> > Best regards,
> > Richard Cheng.
> > 
> >>  	do {
> >>  		int rc;
> >>  
> >> diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
> >> index 7c6c5b7450a5..0370ac39ec4a 100644
> >> --- a/drivers/cxl/core/mbox.c
> >> +++ b/drivers/cxl/core/mbox.c
> >> @@ -1516,6 +1516,7 @@ int cxl_mailbox_init(struct cxl_mailbox *cxl_mbox, struct device *host)
> >>  
> >>  	cxl_mbox->host = host;
> >>  	mutex_init(&cxl_mbox->mbox_mutex);
> >> +	mutex_init(&cxl_mbox->feat_mutex);
> >>  	rcuwait_init(&cxl_mbox->mbox_wait);
> >>  
> >>  	return 0;
> >> diff --git a/include/cxl/mailbox.h b/include/cxl/mailbox.h
> >> index c4e99e2e3a9d..d008b9db07aa 100644
> >> --- a/include/cxl/mailbox.h
> >> +++ b/include/cxl/mailbox.h
> >> @@ -50,6 +50,7 @@ struct cxl_mbox_cmd {
> >>   * @payload_size: Size of space for payload
> >>   *                (CXL 3.1 8.2.8.4.3 Mailbox Capabilities Register)
> >>   * @mbox_mutex: mutex protects device mailbox and firmware
> >> + * @feat_mutex: serializes multi-part Get/Set Feature transfers
> >>   * @mbox_wait: rcuwait for mailbox
> >>   * @mbox_send: @dev specific transport for transmitting mailbox commands
> >>   * @feat_cap: Features capability
> >> @@ -60,6 +61,7 @@ struct cxl_mailbox {
> >>  	DECLARE_BITMAP(exclusive_cmds, CXL_MEM_COMMAND_ID_MAX);
> >>  	size_t payload_size;
> >>  	struct mutex mbox_mutex; /* lock to protect mailbox context */
> >> +	struct mutex feat_mutex;
> >>  	struct rcuwait mbox_wait;
> >>  	int (*mbox_send)(struct cxl_mailbox *cxl_mbox, struct cxl_mbox_cmd *cmd);
> >>  	enum cxl_features_capability feat_cap;
> >>
> >> base-commit: 8cdeaa50eae8dad34885515f62559ee83e7e8dda
> >> -- 
> >> 2.54.0
> >>
> >>
>