| Message ID | 20220201071952.900068-7-ira.weiny@intel.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | CXL: Read CDAT and DSMAS data from the device | expand |
On 22-01-31 23:19:48, ira.weiny@intel.com wrote: > From: Ira Weiny <ira.weiny@intel.com> > > Memory devices need the CDAT data from the device. This data is read > from a DOE mailbox which supports the CDAT protocol. > > Search the DOE auxiliary devices for the one which supports the CDAT > protocol. Cache that device to be used for future queries. > > Signed-off-by: Ira Weiny <ira.weiny@intel.com> > --- > drivers/cxl/cxl.h | 3 +++ > drivers/cxl/cxlmem.h | 2 ++ > drivers/cxl/pci.c | 43 ++++++++++++++++++++++++++++++++++++++++++- > 3 files changed, 47 insertions(+), 1 deletion(-) > > diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h > index 962629c5775f..7169101db553 100644 > --- a/drivers/cxl/cxl.h > +++ b/drivers/cxl/cxl.h > @@ -90,6 +90,9 @@ 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_COMPLIANCE 0 > +#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 5d33ce24fe09..0fefe43951e3 100644 > --- a/drivers/cxl/cxlmem.h > +++ b/drivers/cxl/cxlmem.h > @@ -117,6 +117,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 > @@ -149,6 +150,7 @@ struct cxl_endpoint_dvsec_info { > struct cxl_dev_state { > struct device *dev; > > + struct pci_doe_dev *cdat_doe; > struct cxl_regs regs; > int cxl_dvsec; > > diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c > index d4ae79b62a14..dcc55c4efd85 100644 > --- a/drivers/cxl/pci.c > +++ b/drivers/cxl/pci.c > @@ -536,12 +536,53 @@ static int cxl_dvsec_ranges(struct cxl_dev_state *cxlds) > return rc; > } > > +static int cxl_match_cdat_doe_device(struct device *dev, const void *data) > +{ > + const struct cxl_dev_state *cxlds = data; > + struct auxiliary_device *adev; > + struct pci_doe_dev *doe_dev; > + > + /* First determine if this auxiliary device belongs to the cxlds */ > + if (cxlds->dev != dev->parent) > + return 0; I don't understand auxiliary bus but I'm wondering why it's checking the parent of the device? > + > + adev = to_auxiliary_dev(dev); > + doe_dev = container_of(adev, struct pci_doe_dev, adev); > + > + /* If it is one of ours check for the CDAT protocol */ > + if (pci_doe_supports_prot(doe_dev, PCI_DVSEC_VENDOR_ID_CXL, > + CXL_DOE_PROTOCOL_TABLE_ACCESS)) > + return 1; > + > + return 0; > +} > + > 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; > > - return pci_doe_create_doe_devices(pdev); > + rc = pci_doe_create_doe_devices(pdev); > + if (rc) > + return rc; > + > + adev = auxiliary_find_device(NULL, cxlds, &cxl_match_cdat_doe_device); > + > + if (adev) { > + struct pci_doe_dev *doe_dev = container_of(adev, > + struct pci_doe_dev, > + adev); > + > + /* > + * No reference need be taken. The DOE device lifetime is > + * longer that the CXL device state lifetime > + */ You're holding a reference to the adev here. Did you mean to drop it? > + cxlds->cdat_doe = doe_dev; > + } > + > + return 0; > } > > static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id) > -- > 2.31.1 >
On Tue, Feb 01, 2022 at 10:49:47AM -0800, Widawsky, Ben wrote: > On 22-01-31 23:19:48, ira.weiny@intel.com wrote: > > From: Ira Weiny <ira.weiny@intel.com> > > > > Memory devices need the CDAT data from the device. This data is read > > from a DOE mailbox which supports the CDAT protocol. > > > > Search the DOE auxiliary devices for the one which supports the CDAT > > protocol. Cache that device to be used for future queries. > > > > Signed-off-by: Ira Weiny <ira.weiny@intel.com> [snip] > > > > diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c > > index d4ae79b62a14..dcc55c4efd85 100644 > > --- a/drivers/cxl/pci.c > > +++ b/drivers/cxl/pci.c > > @@ -536,12 +536,53 @@ static int cxl_dvsec_ranges(struct cxl_dev_state *cxlds) > > return rc; > > } > > > > +static int cxl_match_cdat_doe_device(struct device *dev, const void *data) > > +{ > > + const struct cxl_dev_state *cxlds = data; > > + struct auxiliary_device *adev; > > + struct pci_doe_dev *doe_dev; > > + > > + /* First determine if this auxiliary device belongs to the cxlds */ > > + if (cxlds->dev != dev->parent) > > + return 0; > > I don't understand auxiliary bus but I'm wondering why it's checking the parent > of the device? auxiliary_find_device() iterates all the auxiliary devices in the system. This check was a way for the match function to know if the auxiliary device belongs to the cxlds we are interested in... But now that I think about it we could have other auxiliary devices attached which are not DOE... :-/ So this check is not complete. FWIW I'm not thrilled with the way auxiliary_find_device() is defined. And now that I look at it I think the only user of it currently is wrong. They too have a check like this but it is after another check... :-/ I was hoping to avoid having a list of DOE devices in the cxlds and simply let the auxiliary bus infrastructure do that somehow. IIRC Jonathan was thinking along the same lines. I think he actually suggested auxiliary_find_device()... It would be nice if I could have an aux_find_child() or something which iterated the auxiliary devices attached to a particular parent device. I've just not figured out exactly how to implement that better than what I did here. > > > + > > + adev = to_auxiliary_dev(dev); > > + doe_dev = container_of(adev, struct pci_doe_dev, adev); > > + > > + /* If it is one of ours check for the CDAT protocol */ > > + if (pci_doe_supports_prot(doe_dev, PCI_DVSEC_VENDOR_ID_CXL, > > + CXL_DOE_PROTOCOL_TABLE_ACCESS)) > > + return 1; > > + > > + return 0; > > +} > > + > > 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; > > > > - return pci_doe_create_doe_devices(pdev); > > + rc = pci_doe_create_doe_devices(pdev); > > + if (rc) > > + return rc; > > + > > + adev = auxiliary_find_device(NULL, cxlds, &cxl_match_cdat_doe_device); > > + > > + if (adev) { > > + struct pci_doe_dev *doe_dev = container_of(adev, > > + struct pci_doe_dev, > > + adev); > > + > > + /* > > + * No reference need be taken. The DOE device lifetime is > > + * longer that the CXL device state lifetime > > + */ > > You're holding a reference to the adev here. Did you mean to drop it? Does find device get a reference? ... Ah shoot I did not see that. Yea the reference should be dropped somewhere. Thanks, Ira > > > + cxlds->cdat_doe = doe_dev; > > + } > > + > > + return 0; > > } > > > > static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id) > > -- > > 2.31.1 > >
On Tue, 1 Feb 2022 14:18:41 -0800 Ira Weiny <ira.weiny@intel.com> wrote: > On Tue, Feb 01, 2022 at 10:49:47AM -0800, Widawsky, Ben wrote: > > On 22-01-31 23:19:48, ira.weiny@intel.com wrote: > > > From: Ira Weiny <ira.weiny@intel.com> > > > > > > Memory devices need the CDAT data from the device. This data is read > > > from a DOE mailbox which supports the CDAT protocol. > > > > > > Search the DOE auxiliary devices for the one which supports the CDAT > > > protocol. Cache that device to be used for future queries. > > > > > > Signed-off-by: Ira Weiny <ira.weiny@intel.com> > > [snip] > > > > > > > diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c > > > index d4ae79b62a14..dcc55c4efd85 100644 > > > --- a/drivers/cxl/pci.c > > > +++ b/drivers/cxl/pci.c > > > @@ -536,12 +536,53 @@ static int cxl_dvsec_ranges(struct cxl_dev_state *cxlds) > > > return rc; > > > } > > > > > > +static int cxl_match_cdat_doe_device(struct device *dev, const void *data) > > > +{ > > > + const struct cxl_dev_state *cxlds = data; > > > + struct auxiliary_device *adev; > > > + struct pci_doe_dev *doe_dev; > > > + > > > + /* First determine if this auxiliary device belongs to the cxlds */ > > > + if (cxlds->dev != dev->parent) > > > + return 0; > > > > I don't understand auxiliary bus but I'm wondering why it's checking the parent > > of the device? > > auxiliary_find_device() iterates all the auxiliary devices in the system. This > check was a way for the match function to know if the auxiliary device belongs > to the cxlds we are interested in... > > But now that I think about it we could have other auxiliary devices attached > which are not DOE... :-/ So this check is not complete. > > FWIW I'm not thrilled with the way auxiliary_find_device() is defined. And now > that I look at it I think the only user of it currently is wrong. They too > have a check like this but it is after another check... :-/ > > I was hoping to avoid having a list of DOE devices in the cxlds and simply let > the auxiliary bus infrastructure do that somehow. IIRC Jonathan was thinking > along the same lines. I think he actually suggested auxiliary_find_device()... Ah.. I think I'd been thinking it was scoped to a single parent rather than all devices in the system. Definitely rather horrible. Can we do something with device_for_each_child() instead with a match on bus type to check its an auxilliary bus device then I guess a name based check on whether that is a doe. etc. > > It would be nice if I could have an aux_find_child() or something which > iterated the auxiliary devices attached to a particular parent device. I've > just not figured out exactly how to implement that better than what I did here. > > > > > > + > > > + adev = to_auxiliary_dev(dev); > > > + doe_dev = container_of(adev, struct pci_doe_dev, adev); > > > + > > > + /* If it is one of ours check for the CDAT protocol */ > > > + if (pci_doe_supports_prot(doe_dev, PCI_DVSEC_VENDOR_ID_CXL, > > > + CXL_DOE_PROTOCOL_TABLE_ACCESS)) > > > + return 1; > > > + > > > + return 0; > > > +} > > > + > > > 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; > > > > > > - return pci_doe_create_doe_devices(pdev); > > > + rc = pci_doe_create_doe_devices(pdev); > > > + if (rc) > > > + return rc; > > > + > > > + adev = auxiliary_find_device(NULL, cxlds, &cxl_match_cdat_doe_device); > > > + > > > + if (adev) { > > > + struct pci_doe_dev *doe_dev = container_of(adev, > > > + struct pci_doe_dev, > > > + adev); > > > + > > > + /* > > > + * No reference need be taken. The DOE device lifetime is > > > + * longer that the CXL device state lifetime > > > + */ > > > > You're holding a reference to the adev here. Did you mean to drop it? > > Does find device get a reference? ... Ah shoot I did not see that. > > Yea the reference should be dropped somewhere. > > Thanks, > Ira > > > > > > + cxlds->cdat_doe = doe_dev; > > > + } > > > + > > > + return 0; > > > } > > > > > > static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id) > > > -- > > > 2.31.1 > > >
diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h index 962629c5775f..7169101db553 100644 --- a/drivers/cxl/cxl.h +++ b/drivers/cxl/cxl.h @@ -90,6 +90,9 @@ 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_COMPLIANCE 0 +#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 5d33ce24fe09..0fefe43951e3 100644 --- a/drivers/cxl/cxlmem.h +++ b/drivers/cxl/cxlmem.h @@ -117,6 +117,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 @@ -149,6 +150,7 @@ struct cxl_endpoint_dvsec_info { struct cxl_dev_state { struct device *dev; + struct pci_doe_dev *cdat_doe; struct cxl_regs regs; int cxl_dvsec; diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c index d4ae79b62a14..dcc55c4efd85 100644 --- a/drivers/cxl/pci.c +++ b/drivers/cxl/pci.c @@ -536,12 +536,53 @@ static int cxl_dvsec_ranges(struct cxl_dev_state *cxlds) return rc; } +static int cxl_match_cdat_doe_device(struct device *dev, const void *data) +{ + const struct cxl_dev_state *cxlds = data; + struct auxiliary_device *adev; + struct pci_doe_dev *doe_dev; + + /* First determine if this auxiliary device belongs to the cxlds */ + if (cxlds->dev != dev->parent) + return 0; + + adev = to_auxiliary_dev(dev); + doe_dev = container_of(adev, struct pci_doe_dev, adev); + + /* If it is one of ours check for the CDAT protocol */ + if (pci_doe_supports_prot(doe_dev, PCI_DVSEC_VENDOR_ID_CXL, + CXL_DOE_PROTOCOL_TABLE_ACCESS)) + return 1; + + return 0; +} + 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; - return pci_doe_create_doe_devices(pdev); + rc = pci_doe_create_doe_devices(pdev); + if (rc) + return rc; + + adev = auxiliary_find_device(NULL, cxlds, &cxl_match_cdat_doe_device); + + if (adev) { + struct pci_doe_dev *doe_dev = container_of(adev, + struct pci_doe_dev, + adev); + + /* + * No reference need be taken. The DOE device lifetime is + * longer that the CXL device state lifetime + */ + cxlds->cdat_doe = doe_dev; + } + + return 0; } static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)