On Wed, Apr 27, 2022 at 06:49:56PM +0100, Jonathan Cameron wrote:
> On Thu, 14 Apr 2022 13:32:33 -0700
> [email protected] wrote:
>
> > From: Ira Weiny <[email protected]>
> >
> > 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 <[email protected]>
>
> One question inline. With that addressed (or convincing argument why not)
> the rest looks good.
>
> Reviewed-by: Jonathan Cameron <[email protected]>

Thanks!

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

Indeed. I struggled a bit with this because get_* is usually taking a
reference on the subject of the get but in this case we are really getting a
reference on the device not the driver...

I originally open coded this and the functions were 'get_device'...

:-/

I think this also may have been why the device was originally part of the
driver state.

I think this is the better way to code these access functions:

static struct cxl_doe_drv_state *cxl_pci_doe_get_drv(struct cxl_doe_dev *doe_dev)
{
struct cxl_doe_drv_state *cxl_ds;

down_read(&doe_dev->driver_access);
cxl_ds = auxiliary_get_drvdata(&doe_dev->adev);
if (!cxl_ds)
up_read(&doe_dev->driver_access);
return cxl_ds;
}

static void cxl_pci_doe_put_drv(struct cxl_doe_drv_state *cxl_ds)
{
if (!cxl_ds)
return;
up_read(&cxl_ds->doe_dev->driver_access);
}

Ira

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