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

[Dave Jiang <dave.jiang@intel.com>]

On 9/9/24 4:18 PM, Ira Weiny wrote:
> 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()?

Yes. Thanks. Leftover from previously.


> 
>> 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?

I split out the initialization of data structures and the setup of the hardware. We can further deal with that as things become more clearer what a type2 device that wants a mailbox would look like.

> 
>> +
>> +	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?

Yes this is a new check. Previous code assumed that we never fail when it comes to mailboxes. I'm adding this to fail if mailbox isn't discovered as this is not in compliance with the spec.

> 
>> +
>> +	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.

I think I had it running earlier to hit that check in case a mailbox did not exist so we can just bail without doing all the additional work. At least that was my thought process.

> 
> Ira
> 
> [snip]