| Message ID | 20241118164434.7551-5-alejandro.lucero-palau@amd.com |
|---|---|
| State | New |
| Headers | show |
| Series | cxl: add type2 device basic support | expand |
On 11/18/24 10:44 AM, alejandro.lucero-palau@amd.com wrote: > From: Alejandro Lucero <alucerop@amd.com> > > During CXL device initialization supported capabilities by the device > are discovered. Type3 and Type2 devices have different mandatory > capabilities and a Type2 expects a specific set including optional > capabilities. > > Add a function for checking expected capabilities against those found > during initialization. Allow those mandatory/expected capabilities to > be a subset of the capabilities found. > > Rely on this function for validating capabilities instead of when CXL > regs are probed. > > Signed-off-by: Alejandro Lucero <alucerop@amd.com> > --- > drivers/cxl/core/pci.c | 22 ++++++++++++++++++++++ > drivers/cxl/core/regs.c | 9 --------- > drivers/cxl/pci.c | 24 ++++++++++++++++++++++++ > include/cxl/cxl.h | 6 +++++- > 4 files changed, 51 insertions(+), 10 deletions(-) > > diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c > index ff266e91ea71..a1942b7be0bc 100644 > --- a/drivers/cxl/core/pci.c > +++ b/drivers/cxl/core/pci.c > @@ -8,6 +8,7 @@ > #include <linux/pci.h> > #include <linux/pci-doe.h> > #include <linux/aer.h> > +#include <cxl/cxl.h> > #include <cxlpci.h> > #include <cxlmem.h> > #include <cxl.h> > @@ -1055,3 +1056,24 @@ int cxl_pci_get_bandwidth(struct pci_dev *pdev, struct access_coordinate *c) > > return 0; > } > + > +bool cxl_pci_check_caps(struct cxl_dev_state *cxlds, unsigned long *expected_caps, > + unsigned long *current_caps, bool is_subset) > +{ > + DECLARE_BITMAP(subset, CXL_MAX_CAPS); > + > + if (current_caps) > + bitmap_copy(current_caps, cxlds->capabilities, CXL_MAX_CAPS); > + > + dev_dbg(cxlds->dev, "Checking cxlds caps 0x%08lx vs expected caps 0x%08lx\n", > + *cxlds->capabilities, *expected_caps); > + > + /* Checking a minimum of mandatory capabilities? */ > + if (is_subset) { > + bitmap_and(subset, cxlds->capabilities, expected_caps, CXL_MAX_CAPS); > + return bitmap_equal(subset, expected_caps, CXL_MAX_CAPS); It looks like there's a function called bitmap_subset(), does that not do the above? I didn't look at the function since it's the end of the day when I'm writing this and my brain is tired, but I'd rather that be used if possible. I also don't think you need this is_subset parameter and else branch. I don't see anyone using this function where some expected capabilities are optional and others mandatory. If that's the case then they'd probably split the calls instead. > + } else { > + return bitmap_equal(cxlds->capabilities, expected_caps, CXL_MAX_CAPS); > + } > +} > +EXPORT_SYMBOL_NS_GPL(cxl_pci_check_caps, CXL); > diff --git a/drivers/cxl/core/regs.c b/drivers/cxl/core/regs.c > index 8287ec45b018..3b3965706414 100644 > --- a/drivers/cxl/core/regs.c > +++ b/drivers/cxl/core/regs.c > @@ -444,15 +444,6 @@ static int cxl_probe_regs(struct cxl_register_map *map, unsigned long *caps) > case CXL_REGLOC_RBI_MEMDEV: > dev_map = &map->device_map; > cxl_probe_device_regs(host, base, dev_map, caps); > - if (!dev_map->status.valid || !dev_map->mbox.valid || > - !dev_map->memdev.valid) { > - dev_err(host, "registers not found: %s%s%s\n", > - !dev_map->status.valid ? "status " : "", > - !dev_map->mbox.valid ? "mbox " : "", > - !dev_map->memdev.valid ? "memdev " : ""); > - return -ENXIO; > - } > - > dev_dbg(host, "Probing device registers...\n"); > break; > default: > diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c > index 528d4ca79fd1..5de1473a79da 100644 > --- a/drivers/cxl/pci.c > +++ b/drivers/cxl/pci.c > @@ -813,6 +813,8 @@ static int cxl_pci_type3_init_mailbox(struct cxl_dev_state *cxlds) > static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id) > { > struct pci_host_bridge *host_bridge = pci_find_host_bridge(pdev->bus); > + DECLARE_BITMAP(expected, CXL_MAX_CAPS); > + DECLARE_BITMAP(found, CXL_MAX_CAPS); > struct cxl_memdev_state *mds; > struct cxl_dev_state *cxlds; > struct cxl_register_map map; > @@ -874,6 +876,28 @@ static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id) > if (rc) > dev_dbg(&pdev->dev, "Failed to map RAS capability.\n"); > > + bitmap_clear(expected, 0, CXL_MAX_CAPS); > + > + /* > + * These are the mandatory capabilities for a Type3 device. > + * Only checking capabilities used by current Linux drivers. > + */ > + bitmap_set(expected, CXL_DEV_CAP_HDM, 1); > + bitmap_set(expected, CXL_DEV_CAP_DEV_STATUS, 1); > + bitmap_set(expected, CXL_DEV_CAP_MAILBOX_PRIMARY, 1); > + bitmap_set(expected, CXL_DEV_CAP_DEV_STATUS, 1); > + > + /* > + * Checking mandatory caps are there as, at least, a subset of those > + * found. > + */ > + if (!cxl_pci_check_caps(cxlds, expected, found, true)) { > + dev_err(&pdev->dev, > + "Expected mandatory capabilities not found: (%08lx - %08lx)\n", > + *expected, *found); > + return -ENXIO; > + } > + > rc = cxl_pci_type3_init_mailbox(cxlds); > if (rc) > return rc; > diff --git a/include/cxl/cxl.h b/include/cxl/cxl.h > index dcc9ec8a0aec..ab243ab8024f 100644 > --- a/include/cxl/cxl.h > +++ b/include/cxl/cxl.h > @@ -39,7 +39,7 @@ enum cxl_dev_cap { > CXL_DEV_CAP_DEV_STATUS, > CXL_DEV_CAP_MAILBOX_PRIMARY, > CXL_DEV_CAP_MEMDEV, > - CXL_MAX_CAPS = 32 > + CXL_MAX_CAPS = 64 > }; > > struct cxl_dev_state *cxl_accel_state_create(struct device *dev); > @@ -48,4 +48,8 @@ void cxl_set_dvsec(struct cxl_dev_state *cxlds, u16 dvsec); > void cxl_set_serial(struct cxl_dev_state *cxlds, u64 serial); > int cxl_set_resource(struct cxl_dev_state *cxlds, struct resource res, > enum cxl_resource); > +bool cxl_pci_check_caps(struct cxl_dev_state *cxlds, > + unsigned long *expected_caps, > + unsigned long *current_caps, > + bool is_subset); > #endif
On 11/22/24 20:44, Ben Cheatham wrote: > On 11/18/24 10:44 AM, alejandro.lucero-palau@amd.com wrote: >> From: Alejandro Lucero <alucerop@amd.com> >> >> During CXL device initialization supported capabilities by the device >> are discovered. Type3 and Type2 devices have different mandatory >> capabilities and a Type2 expects a specific set including optional >> capabilities. >> >> Add a function for checking expected capabilities against those found >> during initialization. Allow those mandatory/expected capabilities to >> be a subset of the capabilities found. >> >> Rely on this function for validating capabilities instead of when CXL >> regs are probed. >> >> Signed-off-by: Alejandro Lucero <alucerop@amd.com> >> --- >> drivers/cxl/core/pci.c | 22 ++++++++++++++++++++++ >> drivers/cxl/core/regs.c | 9 --------- >> drivers/cxl/pci.c | 24 ++++++++++++++++++++++++ >> include/cxl/cxl.h | 6 +++++- >> 4 files changed, 51 insertions(+), 10 deletions(-) >> >> diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c >> index ff266e91ea71..a1942b7be0bc 100644 >> --- a/drivers/cxl/core/pci.c >> +++ b/drivers/cxl/core/pci.c >> @@ -8,6 +8,7 @@ >> #include <linux/pci.h> >> #include <linux/pci-doe.h> >> #include <linux/aer.h> >> +#include <cxl/cxl.h> >> #include <cxlpci.h> >> #include <cxlmem.h> >> #include <cxl.h> >> @@ -1055,3 +1056,24 @@ int cxl_pci_get_bandwidth(struct pci_dev *pdev, struct access_coordinate *c) >> >> return 0; >> } >> + >> +bool cxl_pci_check_caps(struct cxl_dev_state *cxlds, unsigned long *expected_caps, >> + unsigned long *current_caps, bool is_subset) >> +{ >> + DECLARE_BITMAP(subset, CXL_MAX_CAPS); >> + >> + if (current_caps) >> + bitmap_copy(current_caps, cxlds->capabilities, CXL_MAX_CAPS); >> + >> + dev_dbg(cxlds->dev, "Checking cxlds caps 0x%08lx vs expected caps 0x%08lx\n", >> + *cxlds->capabilities, *expected_caps); >> + >> + /* Checking a minimum of mandatory capabilities? */ >> + if (is_subset) { >> + bitmap_and(subset, cxlds->capabilities, expected_caps, CXL_MAX_CAPS); >> + return bitmap_equal(subset, expected_caps, CXL_MAX_CAPS); > > It looks like there's a function called bitmap_subset(), does that not do the above? I didn't > look at the function since it's the end of the day when I'm writing this and my brain is tired, > but I'd rather that be used if possible. I also don't think you need this is_subset parameter and > else branch. I don't see anyone using this function where some expected capabilities are optional > and others mandatory. If that's the case then they'd probably split the calls instead. That is a funny one. I did not realize such bitmap_subset did exist! I've just tried it and it works as expected. It is going to make the code simpler! Thanks! >> + } else { >> + return bitmap_equal(cxlds->capabilities, expected_caps, CXL_MAX_CAPS); >> + } >> +} >> +EXPORT_SYMBOL_NS_GPL(cxl_pci_check_caps, CXL); >> diff --git a/drivers/cxl/core/regs.c b/drivers/cxl/core/regs.c >> index 8287ec45b018..3b3965706414 100644 >> --- a/drivers/cxl/core/regs.c >> +++ b/drivers/cxl/core/regs.c >> @@ -444,15 +444,6 @@ static int cxl_probe_regs(struct cxl_register_map *map, unsigned long *caps) >> case CXL_REGLOC_RBI_MEMDEV: >> dev_map = &map->device_map; >> cxl_probe_device_regs(host, base, dev_map, caps); >> - if (!dev_map->status.valid || !dev_map->mbox.valid || >> - !dev_map->memdev.valid) { >> - dev_err(host, "registers not found: %s%s%s\n", >> - !dev_map->status.valid ? "status " : "", >> - !dev_map->mbox.valid ? "mbox " : "", >> - !dev_map->memdev.valid ? "memdev " : ""); >> - return -ENXIO; >> - } >> - >> dev_dbg(host, "Probing device registers...\n"); >> break; >> default: >> diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c >> index 528d4ca79fd1..5de1473a79da 100644 >> --- a/drivers/cxl/pci.c >> +++ b/drivers/cxl/pci.c >> @@ -813,6 +813,8 @@ static int cxl_pci_type3_init_mailbox(struct cxl_dev_state *cxlds) >> static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id) >> { >> struct pci_host_bridge *host_bridge = pci_find_host_bridge(pdev->bus); >> + DECLARE_BITMAP(expected, CXL_MAX_CAPS); >> + DECLARE_BITMAP(found, CXL_MAX_CAPS); >> struct cxl_memdev_state *mds; >> struct cxl_dev_state *cxlds; >> struct cxl_register_map map; >> @@ -874,6 +876,28 @@ static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id) >> if (rc) >> dev_dbg(&pdev->dev, "Failed to map RAS capability.\n"); >> >> + bitmap_clear(expected, 0, CXL_MAX_CAPS); >> + >> + /* >> + * These are the mandatory capabilities for a Type3 device. >> + * Only checking capabilities used by current Linux drivers. >> + */ >> + bitmap_set(expected, CXL_DEV_CAP_HDM, 1); >> + bitmap_set(expected, CXL_DEV_CAP_DEV_STATUS, 1); >> + bitmap_set(expected, CXL_DEV_CAP_MAILBOX_PRIMARY, 1); >> + bitmap_set(expected, CXL_DEV_CAP_DEV_STATUS, 1); >> + >> + /* >> + * Checking mandatory caps are there as, at least, a subset of those >> + * found. >> + */ >> + if (!cxl_pci_check_caps(cxlds, expected, found, true)) { >> + dev_err(&pdev->dev, >> + "Expected mandatory capabilities not found: (%08lx - %08lx)\n", >> + *expected, *found); >> + return -ENXIO; >> + } >> + >> rc = cxl_pci_type3_init_mailbox(cxlds); >> if (rc) >> return rc; >> diff --git a/include/cxl/cxl.h b/include/cxl/cxl.h >> index dcc9ec8a0aec..ab243ab8024f 100644 >> --- a/include/cxl/cxl.h >> +++ b/include/cxl/cxl.h >> @@ -39,7 +39,7 @@ enum cxl_dev_cap { >> CXL_DEV_CAP_DEV_STATUS, >> CXL_DEV_CAP_MAILBOX_PRIMARY, >> CXL_DEV_CAP_MEMDEV, >> - CXL_MAX_CAPS = 32 >> + CXL_MAX_CAPS = 64 >> }; >> >> struct cxl_dev_state *cxl_accel_state_create(struct device *dev); >> @@ -48,4 +48,8 @@ void cxl_set_dvsec(struct cxl_dev_state *cxlds, u16 dvsec); >> void cxl_set_serial(struct cxl_dev_state *cxlds, u64 serial); >> int cxl_set_resource(struct cxl_dev_state *cxlds, struct resource res, >> enum cxl_resource); >> +bool cxl_pci_check_caps(struct cxl_dev_state *cxlds, >> + unsigned long *expected_caps, >> + unsigned long *current_caps, >> + bool is_subset); >> #endif
diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c index ff266e91ea71..a1942b7be0bc 100644 --- a/drivers/cxl/core/pci.c +++ b/drivers/cxl/core/pci.c @@ -8,6 +8,7 @@ #include <linux/pci.h> #include <linux/pci-doe.h> #include <linux/aer.h> +#include <cxl/cxl.h> #include <cxlpci.h> #include <cxlmem.h> #include <cxl.h> @@ -1055,3 +1056,24 @@ int cxl_pci_get_bandwidth(struct pci_dev *pdev, struct access_coordinate *c) return 0; } + +bool cxl_pci_check_caps(struct cxl_dev_state *cxlds, unsigned long *expected_caps, + unsigned long *current_caps, bool is_subset) +{ + DECLARE_BITMAP(subset, CXL_MAX_CAPS); + + if (current_caps) + bitmap_copy(current_caps, cxlds->capabilities, CXL_MAX_CAPS); + + dev_dbg(cxlds->dev, "Checking cxlds caps 0x%08lx vs expected caps 0x%08lx\n", + *cxlds->capabilities, *expected_caps); + + /* Checking a minimum of mandatory capabilities? */ + if (is_subset) { + bitmap_and(subset, cxlds->capabilities, expected_caps, CXL_MAX_CAPS); + return bitmap_equal(subset, expected_caps, CXL_MAX_CAPS); + } else { + return bitmap_equal(cxlds->capabilities, expected_caps, CXL_MAX_CAPS); + } +} +EXPORT_SYMBOL_NS_GPL(cxl_pci_check_caps, CXL); diff --git a/drivers/cxl/core/regs.c b/drivers/cxl/core/regs.c index 8287ec45b018..3b3965706414 100644 --- a/drivers/cxl/core/regs.c +++ b/drivers/cxl/core/regs.c @@ -444,15 +444,6 @@ static int cxl_probe_regs(struct cxl_register_map *map, unsigned long *caps) case CXL_REGLOC_RBI_MEMDEV: dev_map = &map->device_map; cxl_probe_device_regs(host, base, dev_map, caps); - if (!dev_map->status.valid || !dev_map->mbox.valid || - !dev_map->memdev.valid) { - dev_err(host, "registers not found: %s%s%s\n", - !dev_map->status.valid ? "status " : "", - !dev_map->mbox.valid ? "mbox " : "", - !dev_map->memdev.valid ? "memdev " : ""); - return -ENXIO; - } - dev_dbg(host, "Probing device registers...\n"); break; default: diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c index 528d4ca79fd1..5de1473a79da 100644 --- a/drivers/cxl/pci.c +++ b/drivers/cxl/pci.c @@ -813,6 +813,8 @@ static int cxl_pci_type3_init_mailbox(struct cxl_dev_state *cxlds) static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id) { struct pci_host_bridge *host_bridge = pci_find_host_bridge(pdev->bus); + DECLARE_BITMAP(expected, CXL_MAX_CAPS); + DECLARE_BITMAP(found, CXL_MAX_CAPS); struct cxl_memdev_state *mds; struct cxl_dev_state *cxlds; struct cxl_register_map map; @@ -874,6 +876,28 @@ static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id) if (rc) dev_dbg(&pdev->dev, "Failed to map RAS capability.\n"); + bitmap_clear(expected, 0, CXL_MAX_CAPS); + + /* + * These are the mandatory capabilities for a Type3 device. + * Only checking capabilities used by current Linux drivers. + */ + bitmap_set(expected, CXL_DEV_CAP_HDM, 1); + bitmap_set(expected, CXL_DEV_CAP_DEV_STATUS, 1); + bitmap_set(expected, CXL_DEV_CAP_MAILBOX_PRIMARY, 1); + bitmap_set(expected, CXL_DEV_CAP_DEV_STATUS, 1); + + /* + * Checking mandatory caps are there as, at least, a subset of those + * found. + */ + if (!cxl_pci_check_caps(cxlds, expected, found, true)) { + dev_err(&pdev->dev, + "Expected mandatory capabilities not found: (%08lx - %08lx)\n", + *expected, *found); + return -ENXIO; + } + rc = cxl_pci_type3_init_mailbox(cxlds); if (rc) return rc; diff --git a/include/cxl/cxl.h b/include/cxl/cxl.h index dcc9ec8a0aec..ab243ab8024f 100644 --- a/include/cxl/cxl.h +++ b/include/cxl/cxl.h @@ -39,7 +39,7 @@ enum cxl_dev_cap { CXL_DEV_CAP_DEV_STATUS, CXL_DEV_CAP_MAILBOX_PRIMARY, CXL_DEV_CAP_MEMDEV, - CXL_MAX_CAPS = 32 + CXL_MAX_CAPS = 64 }; struct cxl_dev_state *cxl_accel_state_create(struct device *dev); @@ -48,4 +48,8 @@ void cxl_set_dvsec(struct cxl_dev_state *cxlds, u16 dvsec); void cxl_set_serial(struct cxl_dev_state *cxlds, u64 serial); int cxl_set_resource(struct cxl_dev_state *cxlds, struct resource res, enum cxl_resource); +bool cxl_pci_check_caps(struct cxl_dev_state *cxlds, + unsigned long *expected_caps, + unsigned long *current_caps, + bool is_subset); #endif