lkml.org 
[lkml]   [2022]   [Apr]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH V8 06/10] cxl/pci: Find the DOE mailbox which supports CDAT
    On Thu, 14 Apr 2022 13:32:33 -0700
    ira.weiny@intel.com wrote:

    > From: Ira Weiny <ira.weiny@intel.com>
    >
    > 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@intel.com>

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

    Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

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

    \
     
     \ /
      Last update: 2022-04-27 19:52    [W:7.646 / U:0.016 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site