Re: [PATCH V8 06/10] cxl/pci: Find the DOE mailbox which supports CDAT

[Jonathan Cameron]

On Thu, 14 Apr 2022 13:32:33 -0700
ira.weiny@xxxxxxxxx wrote:

> From: Ira Weiny <ira.weiny@xxxxxxxxx>
> 
> Each CXL device may have multiple DOE mailbox capabilities and each
> mailbox may support multiple protocols.
> 
> Search the DOE auxiliary devices for one which supports the CDAT
> protocol.  Cache that device to be used for future queries.
> 
> Signed-off-by: Ira Weiny <ira.weiny@xxxxxxxxx>

One question inline.  With that addressed (or convincing argument why not)
the rest looks good.

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx>

> 
> ---
> Changes from V7
> 	Minor code clean ups
> 
> Changes from V6
> 	Adjust for aux devices being a CXL only concept
> 	Update commit msg.
> 	Ensure devices iterated by auxiliary_find_device() are checked
> 		to be DOE devices prior to checking for the CDAT
> 		protocol
> 	From Ben
> 		Ensure reference from auxiliary_find_device() is dropped
> ---
>  drivers/cxl/cxl.h    |  2 ++
>  drivers/cxl/cxlmem.h |  2 ++
>  drivers/cxl/pci.c    | 76 +++++++++++++++++++++++++++++++++++++++++++-
>  3 files changed, 79 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> index 990b6670222e..50817fd2c912 100644
> --- a/drivers/cxl/cxl.h
> +++ b/drivers/cxl/cxl.h
> @@ -90,6 +90,8 @@ static inline int cxl_hdm_decoder_count(u32 cap_hdr)
>  #define CXLDEV_MBOX_BG_CMD_STATUS_OFFSET 0x18
>  #define CXLDEV_MBOX_PAYLOAD_OFFSET 0x20
>  
> +#define CXL_DOE_PROTOCOL_TABLE_ACCESS 2
> +
>  /*
>   * Using struct_group() allows for per register-block-type helper routines,
>   * without requiring block-type agnostic code to include the prefix.
> diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
> index 7235d2f976e5..0dc6988afb30 100644
> --- a/drivers/cxl/cxlmem.h
> +++ b/drivers/cxl/cxlmem.h
> @@ -168,6 +168,7 @@ struct cxl_endpoint_dvsec_info {
>   * Currently only memory devices are represented.
>   *
>   * @dev: The device associated with this CXL state
> + * @cdat_doe: Auxiliary DOE device capabile of reading CDAT
>   * @regs: Parsed register blocks
>   * @cxl_dvsec: Offset to the PCIe device DVSEC
>   * @payload_size: Size of space for payload
> @@ -200,6 +201,7 @@ struct cxl_endpoint_dvsec_info {
>  struct cxl_dev_state {
>  	struct device *dev;
>  
> +	struct cxl_doe_dev *cdat_doe;
>  	struct cxl_regs regs;
>  	int cxl_dvsec;
>  
> diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> index 0dec1f1a3f38..622cac925262 100644
> --- a/drivers/cxl/pci.c
> +++ b/drivers/cxl/pci.c
> @@ -706,6 +706,80 @@ static int cxl_pci_create_doe_devices(struct pci_dev *pdev)
>  	return 0;
>  }
>  
> +static bool cxl_doe_dev_supports_prot(struct cxl_doe_dev *doe_dev, u16 vid, u16 pid)
> +{
> +	struct cxl_doe_drv_state *doe_ds;
> +	bool ret;
> +
> +	doe_ds = cxl_pci_doe_get_drv(doe_dev);
> +	if (!doe_ds) {
> +		cxl_pci_doe_put_drv(doe_dev);

That means the get_drv has a rather unexpected side effect.
Could you move the NULL check into the function so that we don't retain
a reference if it fails?

> +		return false;
> +	}
> +	ret = pci_doe_supports_prot(doe_ds->doe_mb, vid, pid);
> +	cxl_pci_doe_put_drv(doe_dev);
> +	return ret;
> +}
> +
> +static int cxl_match_cdat_doe_device(struct device *dev, const void *data)
> +{
> +	const struct cxl_dev_state *cxlds = data;
> +	struct auxiliary_device *adev = to_auxiliary_dev(dev);
> +	struct cxl_doe_dev *doe_dev;
> +
> +	/* Ensure this is a DOE device */
> +	if (strcmp(DOE_DEV_NAME, adev->name))
> +		return 0;
> +
> +	/* Determine if this auxiliary device belongs to the cxlds */
> +	if (cxlds->dev != dev->parent)
> +		return 0;
> +
> +	doe_dev = container_of(adev, struct cxl_doe_dev, adev);
> +
> +	/* If it is one of ours check for the CDAT protocol */
> +	if (!cxl_doe_dev_supports_prot(doe_dev, PCI_DVSEC_VENDOR_ID_CXL,
> +				   CXL_DOE_PROTOCOL_TABLE_ACCESS))
> +		return 0;
> +
> +	return 1;
> +}
> +
> +static void drop_cdat_doe_ref(void *data)
> +{
> +	struct cxl_doe_dev *cdat_doe = data;
> +
> +	put_device(&cdat_doe->adev.dev);
> +}
> +
> +static int cxl_setup_doe_devices(struct cxl_dev_state *cxlds)
> +{
> +	struct device *dev = cxlds->dev;
> +	struct pci_dev *pdev = to_pci_dev(dev);
> +	struct auxiliary_device *adev;
> +	int rc;
> +
> +	rc = cxl_pci_create_doe_devices(pdev);
> +	if (rc)
> +		return rc;
> +
> +	adev = auxiliary_find_device(NULL, cxlds, &cxl_match_cdat_doe_device);
> +
> +	if (adev) {
> +		struct cxl_doe_dev *doe_dev = container_of(adev,
> +							   struct cxl_doe_dev,
> +							   adev);
> +
> +		/* auxiliary_find_device() takes the reference */
> +		rc = devm_add_action_or_reset(dev, drop_cdat_doe_ref, doe_dev);
> +		if (rc)
> +			return rc;
> +		cxlds->cdat_doe = doe_dev;
> +	}
> +
> +	return 0;
> +}
> +
>  static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>  {
>  	struct cxl_register_map map;
> @@ -772,7 +846,7 @@ static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>  	if (rc)
>  		return rc;
>  
> -	rc = cxl_pci_create_doe_devices(pdev);
> +	rc = cxl_setup_doe_devices(cxlds);
>  	if (rc)
>  		return rc;
>