Re: [RFC PATCH 01/13] cxl: Move mailbox related bits to the same context

Jonathan Cameron
2024-07-26T17:28:39Z

On Thu, 18 Jul 2024 14:32:19 -0700
Dave Jiang <dave.jiang@intel.com> wrote:

> Create a new 'struct cxl_mailbox' and move all mailbox related bits to
> it. This allows isolation of all CXL mailbox data in order to export
> some of the calls to external caller (fwctl) and avoid exporting of
> CXL driver specific bits such has device states.
>
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
Some minor comments inline.

> diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
> index 2626f3fff201..783cb5ed823f 100644
> --- a/drivers/cxl/core/mbox.c
> +++ b/drivers/cxl/core/mbox.c

> @@ -1411,6 +1424,7 @@ EXPORT_SYMBOL_NS_GPL(cxl_poison_state_init, CXL);
> struct cxl_memdev_state *cxl_memdev_state_create(struct device *dev)
> {
> struct cxl_memdev_state *mds;
> + struct cxl_mailbox *cxl_mbox;
>
> mds = devm_kzalloc(dev, sizeof(*mds), GFP_KERNEL);
> if (!mds) {
> @@ -1418,7 +1432,9 @@ struct cxl_memdev_state *cxl_memdev_state_create(struct device *dev)
> return ERR_PTR(-ENOMEM);
> }
>
> - mutex_init(&mds->mbox_mutex);
> + cxl_mbox = &mds->cxlds.cxl_mbox;
> + mutex_init(&cxl_mbox->mbox_mutex);

Probably factor out a cxl_mbox_init()
Maybe you do that in later patches...

> /*
> * Here are the steps from 8.2.8.4 of the CXL 2.0 spec.
> @@ -315,10 +320,10 @@ static int __cxl_pci_mbox_send_cmd(struct cxl_memdev_state *mds,
>
> timeout = mbox_cmd->poll_interval_ms;
> for (i = 0; i < mbox_cmd->poll_count; i++) {
> - if (rcuwait_wait_event_timeout(&mds->mbox_wait,
> - cxl_mbox_background_complete(cxlds),
> - TASK_UNINTERRUPTIBLE,
> - msecs_to_jiffies(timeout)) > 0)
> + if (rcuwait_wait_event_timeout(&cxl_mbox->mbox_wait,
> + cxl_mbox_background_complete(cxlds),
> + TASK_UNINTERRUPTIBLE,
> + msecs_to_jiffies(timeout)) > 0)
Keep original indent - will reduce diff.

> diff --git a/include/linux/cxl/mailbox.h b/include/linux/cxl/mailbox.h
> new file mode 100644
> index 000000000000..654df6175828
> --- /dev/null
> +++ b/include/linux/cxl/mailbox.h
> @@ -0,0 +1,63 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/* Copyright(c) 2024 Intel Corporation. */
> +#ifndef __CXL_MBOX_H__
> +#define __CXL_MBOX_H__
> +
> +#include <linux/auxiliary_bus.h>

> +/**
> + * struct cxl_mailbox - context for CXL mailbox operations
> + * @host: device that hosts the mailbox
> + * @adev: auxiliary device for fw-ctl
> + * @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
> + * @mbox_wait: rcuwait for mailbox
> + * @mbox_send: @dev specific transport for transmitting mailbox commands
> + */
> +struct cxl_mailbox {
> + struct device *host;
> + struct auxiliary_device adev; /* For fw-ctl */
I'm not sure the embedding makes sense here.
Also not used yet, so don't do it in this patch.

> + size_t payload_size;
> + struct mutex mbox_mutex; /* lock to protect mailbox context */
Maybe drop the mbox_prefix on this.
> + struct rcuwait mbox_wait;
and this
> + int (*mbox_send)(struct cxl_mailbox *cxl_mbox, struct cxl_mbox_cmd *cmd);
and this
as now they are inside struct cxl_mailbox the mbox bit is pretty obvious
and we all love shorter line lengths.

> +};
> +
> +#endif