Re: [PATCH v4 2/3] cxl: Move mailbox related bits to the same context

[Ira Weiny <ira.weiny@intel.com>]

Dave Jiang 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 kernel callers and avoid exporting of CXL
> driver specific bits such has device states. The allocation of
> 'struct cxl_mailbox' is also split out with cxl_mailbox_create() so the

						cxl_mailbox_init()?

> mailbox can be created independently.
> 
> Reviewed-by: Fan Ni <fan.ni@samsung.com>
> Reviewed-by: Alejandro Lucero <alucerop@amd.com>
> Reviewed-by: Alison Schofield <alison.schofield@intel.com>
> Link: https://patch.msgid.link/20240724185649.2574627-2-dave.jiang@intel.com
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> 
> ---
> v4:
> - cxl_mbox could be NULL in cxl_mailbox_init. (Alison)
> - rework after headers moves
> ---
>  drivers/cxl/core/mbox.c      | 57 +++++++++++++++++++--------
>  drivers/cxl/core/memdev.c    | 18 +++++----
>  drivers/cxl/cxlmem.h         | 21 +++++-----
>  drivers/cxl/pci.c            | 76 ++++++++++++++++++++++++------------
>  drivers/cxl/pmem.c           |  4 +-
>  include/cxl/mailbox.h        | 28 +++++++++++++
>  tools/testing/cxl/test/mem.c | 44 +++++++++++++++------
>  7 files changed, 176 insertions(+), 72 deletions(-)
>  create mode 100644 include/cxl/mailbox.h
> 

[snip]

>  
> +int cxl_mailbox_init(struct cxl_mailbox *cxl_mbox, struct device *host)
> +{
> +	if (!cxl_mbox || !host)
> +		return -EINVAL;
> +
> +	cxl_mbox->host = host;
> +	mutex_init(&cxl_mbox->mbox_mutex);
> +	rcuwait_init(&cxl_mbox->mbox_wait);

I would have expected a few more things in here from
cxl_pci_setup_mailbox().  For example is irq setup needed?  Perhaps
deferred for now?

> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_NS_GPL(cxl_mailbox_init, CXL);
> +
>  struct cxl_memdev_state *cxl_memdev_state_create(struct device *dev)
>  {
>  	struct cxl_memdev_state *mds;
> @@ -1418,7 +1444,6 @@ struct cxl_memdev_state *cxl_memdev_state_create(struct device *dev)
>  		return ERR_PTR(-ENOMEM);
>  	}
>  
> -	mutex_init(&mds->mbox_mutex);
>  	mutex_init(&mds->event.log_lock);
>  	mds->cxlds.dev = dev;
>  	mds->cxlds.reg_map.host = dev;

[snip]

>  
> +static int cxl_pci_type3_init_mailbox(struct cxl_dev_state *cxlds)
> +{
> +	int rc;
> +
> +	/*
> +	 * Fail the init if there's no mailbox. For a type3 this is out of spec.
> +	 */
> +	if (!cxlds->reg_map.device_map.mbox.valid)
> +		return -ENODEV;

This seems like new functionality.  I don't think it is wrong but it was
not checked before right?  If I missed the check are we not checking 2x?

> +
> +	rc = cxl_mailbox_init(&cxlds->cxl_mbox, cxlds->dev);
> +	if (rc)
> +		return rc;
> +
> +	return 0;
> +}
> +
>  static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>  {
>  	struct pci_host_bridge *host_bridge = pci_find_host_bridge(pdev->bus);
> @@ -846,6 +868,10 @@ static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>  	if (rc)
>  		dev_dbg(&pdev->dev, "Failed to map RAS capability.\n");
>  
> +	rc = cxl_pci_type3_init_mailbox(cxlds);
> +	if (rc)
> +		return rc;
> +

I feel like this should be just before cxl_pci_setup_mailbox() after alloc
irq.  Because we are removing some of the setup from
cxl_pci_setup_mailbox() and putting it into cxl_pci_type3_init_mailbox()
it just flows better in my mind.  I don't think there is a requirement to
put it there though.

Ira

[snip]