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