| Message ID | 164298427373.3018233.9309741847039301834.stgit@dwillia2-desk3.amr.corp.intel.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 523e594d9cc03db962c741ce02c8a58aab58a123 |
| Headers | show |
| Series | CXL.mem Topology Discovery and Hotplug Support | expand |
On Sun, 23 Jan 2022 16:31:13 -0800 Dan Williams <dan.j.williams@intel.com> wrote: > From: Ben Widawsky <ben.widawsky@intel.com> > > CXL 2.0 8.1.3.8.2 states: > > Memory_Active: When set, indicates that the CXL Range 1 memory is > fully initialized and available for software use. Must be set within > Range 1. Memory_Active_Timeout of deassertion of reset to CXL device > if CXL.mem HwInit Mode=1 > > Unfortunately, Memory_Active can take quite a long time depending on > media size (up to 256s per 2.0 spec). Provide a callback for the > eventual establishment of CXL.mem operations via the 'cxl_mem' driver > the 'struct cxl_memdev'. The implementation waits for 60s by default for > now and can be overridden by the mbox_ready_time module parameter. > > Signed-off-by: Ben Widawsky <ben.widawsky@intel.com> > [djbw: switch to sleeping wait] > Signed-off-by: Dan Williams <dan.j.williams@intel.com> Not being a memory device person, I'm not sure whether my query below is realistic but I worry a little that minimum sleep if not immediately ready of 1 second is a bit long. Perhaps that's something to optimize once there are a large number of implementations to assess if it is worth bothering or not. Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > --- > diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c > index 5c43886dc2af..513cb0e2a70a 100644 > --- a/drivers/cxl/pci.c > +++ b/drivers/cxl/pci.c > @@ -49,7 +49,7 @@ > static unsigned short mbox_ready_timeout = 60; > module_param(mbox_ready_timeout, ushort, 0600); > MODULE_PARM_DESC(mbox_ready_timeout, > - "seconds to wait for mailbox ready status"); > + "seconds to wait for mailbox ready / memory active status"); > > static int cxl_pci_mbox_wait_for_doorbell(struct cxl_dev_state *cxlds) > { > @@ -417,6 +417,51 @@ static int wait_for_valid(struct cxl_dev_state *cxlds) > return -ETIMEDOUT; > } > > +/* > + * Wait up to @mbox_ready_timeout for the device to report memory > + * active. > + */ > +static int wait_for_media_ready(struct cxl_dev_state *cxlds) > +{ > + struct pci_dev *pdev = to_pci_dev(cxlds->dev); > + int d = cxlds->cxl_dvsec; > + bool active = false; > + u64 md_status; > + int rc, i; > + > + rc = wait_for_valid(cxlds); > + if (rc) > + return rc; > + > + for (i = mbox_ready_timeout; i; i--) { > + u32 temp; > + int rc; > + > + rc = pci_read_config_dword( > + pdev, d + CXL_DVSEC_RANGE_SIZE_LOW(0), &temp); > + if (rc) > + return rc; > + > + active = FIELD_GET(CXL_DVSEC_MEM_ACTIVE, temp); > + if (active) > + break; > + msleep(1000); Whilst it can be a while, this seems a bit of an excessive step to me. If the thing is ready in 10msecs we stil end up waiting a second. Might be worth checking more often, or doing some sort of fall off in frequency of checking. > + } > + > + if (!active) { > + dev_err(&pdev->dev, > + "timeout awaiting memory active after %d seconds\n", > + mbox_ready_timeout); > + return -ETIMEDOUT; > + } > + > + md_status = readq(cxlds->regs.memdev + CXLMDEV_STATUS_OFFSET); > + if (!CXLMDEV_READY(md_status)) > + return -EIO; > + > + return 0; > +} > +
On Mon, Jan 31, 2022 at 10:30 AM Jonathan Cameron <Jonathan.Cameron@huawei.com> wrote: > > On Sun, 23 Jan 2022 16:31:13 -0800 > Dan Williams <dan.j.williams@intel.com> wrote: > > > From: Ben Widawsky <ben.widawsky@intel.com> > > > > CXL 2.0 8.1.3.8.2 states: > > > > Memory_Active: When set, indicates that the CXL Range 1 memory is > > fully initialized and available for software use. Must be set within > > Range 1. Memory_Active_Timeout of deassertion of reset to CXL device > > if CXL.mem HwInit Mode=1 > > > > Unfortunately, Memory_Active can take quite a long time depending on > > media size (up to 256s per 2.0 spec). Provide a callback for the > > eventual establishment of CXL.mem operations via the 'cxl_mem' driver > > the 'struct cxl_memdev'. The implementation waits for 60s by default for > > now and can be overridden by the mbox_ready_time module parameter. > > > > Signed-off-by: Ben Widawsky <ben.widawsky@intel.com> > > [djbw: switch to sleeping wait] > > Signed-off-by: Dan Williams <dan.j.williams@intel.com> > > Not being a memory device person, I'm not sure whether my query below > is realistic but I worry a little that minimum sleep if not immediately > ready of 1 second is a bit long. Perhaps, but I think the chance of getting to this point is slim in the common case where platform firmware has already done CXL memory init. > Perhaps that's something to optimize once there are a large number > of implementations to assess if it is worth bothering or not. Sounds good. > > > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > --- > > > diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c > > index 5c43886dc2af..513cb0e2a70a 100644 > > --- a/drivers/cxl/pci.c > > +++ b/drivers/cxl/pci.c > > @@ -49,7 +49,7 @@ > > static unsigned short mbox_ready_timeout = 60; > > module_param(mbox_ready_timeout, ushort, 0600); > > MODULE_PARM_DESC(mbox_ready_timeout, > > - "seconds to wait for mailbox ready status"); > > + "seconds to wait for mailbox ready / memory active status"); > > > > static int cxl_pci_mbox_wait_for_doorbell(struct cxl_dev_state *cxlds) > > { > > @@ -417,6 +417,51 @@ static int wait_for_valid(struct cxl_dev_state *cxlds) > > return -ETIMEDOUT; > > } > > > > +/* > > + * Wait up to @mbox_ready_timeout for the device to report memory > > + * active. > > + */ > > +static int wait_for_media_ready(struct cxl_dev_state *cxlds) > > +{ > > + struct pci_dev *pdev = to_pci_dev(cxlds->dev); > > + int d = cxlds->cxl_dvsec; > > + bool active = false; > > + u64 md_status; > > + int rc, i; > > + > > + rc = wait_for_valid(cxlds); > > + if (rc) > > + return rc; > > + > > + for (i = mbox_ready_timeout; i; i--) { > > + u32 temp; > > + int rc; > > + > > + rc = pci_read_config_dword( > > + pdev, d + CXL_DVSEC_RANGE_SIZE_LOW(0), &temp); > > + if (rc) > > + return rc; > > + > > + active = FIELD_GET(CXL_DVSEC_MEM_ACTIVE, temp); > > + if (active) > > + break; > > + msleep(1000); > Whilst it can be a while, this seems a bit of an excessive step to me. > If the thing is ready in 10msecs we stil end up waiting a second. > Might be worth checking more often, or doing some sort of fall off > in frequency of checking. I dunno, when the minimum hardware precision in the spec is 1 second it's not clear that the driver can do better than this in practice. Let's see what real platforms do. Part of me also thinks that this is an incentive for devices to get ready before the OS might penalize them with a coarse wait.
diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h index 00f55f4066b9..e70838e5dc17 100644 --- a/drivers/cxl/cxlmem.h +++ b/drivers/cxl/cxlmem.h @@ -132,6 +132,7 @@ struct cxl_endpoint_dvsec_info { * @component_reg_phys: register base of component registers * @info: Cached DVSEC information about the device. * @mbox_send: @dev specific transport for transmitting mailbox commands + * @wait_media_ready: @dev specific method to await media ready * * See section 8.2.9.5.2 Capacity Configuration and Label Storage for * details on capacity parameters. @@ -165,6 +166,7 @@ struct cxl_dev_state { struct cxl_endpoint_dvsec_info info; int (*mbox_send)(struct cxl_dev_state *cxlds, struct cxl_mbox_cmd *cmd); + int (*wait_media_ready)(struct cxl_dev_state *cxlds); }; enum cxl_opcode { diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c index 5c43886dc2af..513cb0e2a70a 100644 --- a/drivers/cxl/pci.c +++ b/drivers/cxl/pci.c @@ -49,7 +49,7 @@ static unsigned short mbox_ready_timeout = 60; module_param(mbox_ready_timeout, ushort, 0600); MODULE_PARM_DESC(mbox_ready_timeout, - "seconds to wait for mailbox ready status"); + "seconds to wait for mailbox ready / memory active status"); static int cxl_pci_mbox_wait_for_doorbell(struct cxl_dev_state *cxlds) { @@ -417,6 +417,51 @@ static int wait_for_valid(struct cxl_dev_state *cxlds) return -ETIMEDOUT; } +/* + * Wait up to @mbox_ready_timeout for the device to report memory + * active. + */ +static int wait_for_media_ready(struct cxl_dev_state *cxlds) +{ + struct pci_dev *pdev = to_pci_dev(cxlds->dev); + int d = cxlds->cxl_dvsec; + bool active = false; + u64 md_status; + int rc, i; + + rc = wait_for_valid(cxlds); + if (rc) + return rc; + + for (i = mbox_ready_timeout; i; i--) { + u32 temp; + int rc; + + rc = pci_read_config_dword( + pdev, d + CXL_DVSEC_RANGE_SIZE_LOW(0), &temp); + if (rc) + return rc; + + active = FIELD_GET(CXL_DVSEC_MEM_ACTIVE, temp); + if (active) + break; + msleep(1000); + } + + if (!active) { + dev_err(&pdev->dev, + "timeout awaiting memory active after %d seconds\n", + mbox_ready_timeout); + return -ETIMEDOUT; + } + + md_status = readq(cxlds->regs.memdev + CXLMDEV_STATUS_OFFSET); + if (!CXLMDEV_READY(md_status)) + return -EIO; + + return 0; +} + static int cxl_dvsec_ranges(struct cxl_dev_state *cxlds) { struct cxl_endpoint_dvsec_info *info = &cxlds->info; @@ -520,6 +565,8 @@ static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id) return -ENXIO; } + cxlds->wait_media_ready = wait_for_media_ready; + rc = cxl_setup_regs(pdev, CXL_REGLOC_RBI_MEMDEV, &map); if (rc) return rc; diff --git a/tools/testing/cxl/test/mem.c b/tools/testing/cxl/test/mem.c index 8c2086c4caef..3af3f94de0c3 100644 --- a/tools/testing/cxl/test/mem.c +++ b/tools/testing/cxl/test/mem.c @@ -4,6 +4,7 @@ #include <linux/platform_device.h> #include <linux/mod_devicetable.h> #include <linux/module.h> +#include <linux/delay.h> #include <linux/sizes.h> #include <linux/bits.h> #include <cxlmem.h> @@ -236,6 +237,12 @@ static int cxl_mock_mbox_send(struct cxl_dev_state *cxlds, struct cxl_mbox_cmd * return rc; } +static int cxl_mock_wait_media_ready(struct cxl_dev_state *cxlds) +{ + msleep(100); + return 0; +} + static void label_area_release(void *lsa) { vfree(lsa); @@ -262,6 +269,7 @@ static int cxl_mock_mem_probe(struct platform_device *pdev) return PTR_ERR(cxlds); cxlds->mbox_send = cxl_mock_mbox_send; + cxlds->wait_media_ready = cxl_mock_wait_media_ready; cxlds->payload_size = SZ_4K; rc = cxl_enumerate_cmds(cxlds);