| Message ID | 20240312080559.14904-4-kobayashi.da-06@fujitsu.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | Display cxl1.1 device link status | expand |
Kobayashi,Daisuke wrote: > From: KobayashiDaisuke <kobayashi.da-06@fujitsu.com> > > This patch adds a function to output the link status of the CXL1.1 device > when it is connected. > > In CXL1.1, the link status of the device is included in the RCRB mapped to > the memory mapped register area. The value of that register is outputted > to sysfs, and based on that, displays the link status information. > > Signed-off-by: "KobayashiDaisuke" <kobayashi.da-06@fujitsu.com> > --- > lib/access.c | 29 +++++++++++++++++++++ > lib/pci.h | 2 ++ > ls-caps.c | 73 ++++++++++++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 104 insertions(+) Including a userspace patch in a kernel patch submission breaks kernel patch management tools like b4 shazam: --- $ b4 shazam 20240312080559.14904-1-kobayashi.da-06@fujitsu.com Grabbing thread from lore.kernel.org/all/20240312080559.14904-1-kobayashi.da-06@fujitsu.com/t.mbox.gz Checking for newer revisions Grabbing search results from lore.kernel.org Analyzing 8 messages in the thread Checking attestation on all messages, may take a moment... --- ✓ [PATCH v3 1/3] Add sysfs attribute for CXL 1.1 device link status ✓ [PATCH v3 2/3] Remove conditional branch that is not suitable for cxl1.1 devices ✓ [PATCH v3 3/3] Add function to display cxl1.1 device link status --- ✓ Signed: DKIM/fujitsu.com --- Total patches: 3 --- Applying: Add sysfs attribute for CXL 1.1 device link status Applying: Remove conditional branch that is not suitable for cxl1.1 devices Applying: Add function to display cxl1.1 device link status Patch failed at 0003 Add function to display cxl1.1 device link status When you have resolved this problem, run "git am --continue". If you prefer to skip this patch, run "git am --skip" instead. To restore the original branch and stop patching, run "git am --abort". /home/dwillia2/git/linux/.git/rebase-apply/patch:117: trailing whitespace. w = (u16)strtoul(buf, NULL, 16); /home/dwillia2/git/linux/.git/rebase-apply/patch:130: trailing whitespace. w = (u16)strtoul(buf, NULL, 16); /home/dwillia2/git/linux/.git/rebase-apply/patch:158: trailing whitespace. error: lib/access.c: does not exist in index error: lib/pci.h: does not exist in index error: ls-caps.c: does not exist in index hint: Use 'git am --show-current-patch=diff' to see the failed patch --- ...and this patch should wait until the kernel change is accepted before being submitted directly to the PCI utils project which does not watch this linux-cxl mailing list.
Dan Williams wrote: > Kobayashi,Daisuke wrote: > > From: KobayashiDaisuke <kobayashi.da-06@fujitsu.com> > > > > This patch adds a function to output the link status of the CXL1.1 > > device when it is connected. > > > > In CXL1.1, the link status of the device is included in the RCRB > > mapped to the memory mapped register area. The value of that register > > is outputted to sysfs, and based on that, displays the link status information. > > > > Signed-off-by: "KobayashiDaisuke" <kobayashi.da-06@fujitsu.com> > > --- > > lib/access.c | 29 +++++++++++++++++++++ > > lib/pci.h | 2 ++ > > ls-caps.c | 73 > ++++++++++++++++++++++++++++++++++++++++++++++++ > ++++ > > 3 files changed, 104 insertions(+) > > Including a userspace patch in a kernel patch submission breaks kernel patch > management tools like b4 shazam: > > --- > $ b4 shazam 20240312080559.14904-1-kobayashi.da-06@fujitsu.com > Grabbing thread from > lore.kernel.org/all/20240312080559.14904-1-kobayashi.da-06@fujitsu.com/t. > mbox.gz > Checking for newer revisions > Grabbing search results from lore.kernel.org Analyzing 8 messages in the > thread Checking attestation on all messages, may take a moment... > --- > ✓ [PATCH v3 1/3] Add sysfs attribute for CXL 1.1 device link status > ✓ [PATCH v3 2/3] Remove conditional branch that is not suitable for cxl1.1 > devices > ✓ [PATCH v3 3/3] Add function to display cxl1.1 device link status > --- > ✓ Signed: DKIM/fujitsu.com > --- > Total patches: 3 > --- > Applying: Add sysfs attribute for CXL 1.1 device link status > Applying: Remove conditional branch that is not suitable for cxl1.1 devices > Applying: Add function to display cxl1.1 device link status Patch failed at 0003 > Add function to display cxl1.1 device link status When you have resolved this > problem, run "git am --continue". > If you prefer to skip this patch, run "git am --skip" instead. > To restore the original branch and stop patching, run "git am --abort". > /home/dwillia2/git/linux/.git/rebase-apply/patch:117: trailing whitespace. > w = (u16)strtoul(buf, NULL, 16); > /home/dwillia2/git/linux/.git/rebase-apply/patch:130: trailing whitespace. > w = (u16)strtoul(buf, NULL, 16); > /home/dwillia2/git/linux/.git/rebase-apply/patch:158: trailing whitespace. > > error: lib/access.c: does not exist in index > error: lib/pci.h: does not exist in index > error: ls-caps.c: does not exist in index > hint: Use 'git am --show-current-patch=diff' to see the failed patch > --- > > ...and this patch should wait until the kernel change is accepted before being > submitted directly to the PCI utils project which does not watch this linux-cxl > mailing list. Thank you! I will submit the patch for pciutils separately.
Hello! > This patch adds a function to output the link status of the CXL1.1 device > when it is connected. > > In CXL1.1, the link status of the device is included in the RCRB mapped to > the memory mapped register area. The value of that register is outputted > to sysfs, and based on that, displays the link status information. > diff --git a/lib/access.c b/lib/access.c > index 7d66123..bc75a84 100644 > --- a/lib/access.c > +++ b/lib/access.c [...] > @@ -268,3 +269,31 @@ pci_get_string_property(struct pci_dev *d, u32 prop) > > return NULL; > } > + > +#define OBJNAMELEN 1024 > +#define OBJBUFSIZE 64 > +int > +get_rcd_sysfs_obj_file(struct pci_dev *d, char *object, char *result) > +{ > +#ifdef PCI_HAVE_PM_LINUX_SYSFS > + char namebuf[OBJNAMELEN]; > + int n = snprintf(namebuf, OBJNAMELEN, "%s/devices/%04x:%02x:%02x.%d/%s", > + pci_get_param(d->access, "sysfs.path"), > + d->domain, d->bus, d->dev, d->func, object); > + if (n < 0 || n >= OBJNAMELEN){ > + d->access->error("Failed to get filename"); > + return -1; > + } > + int fd = open(namebuf, O_RDONLY); > + if(fd < 0) > + return -1; > + n = read(fd, result, OBJBUFSIZE); > + if (n < 0 || n >= OBJBUFSIZE){ > + d->access->error("Failed to read the file"); > + return -1; > + } > + return 0; > +#else > + return -1; > +#endif > +} This really is not the right place to read from sysfs. The libpci should provide a backend-indepenent interface for reading this information and the sysfs back-end (lib/sysfs.c) should provide one implementation of this interface. I think that we can easily extend pci_fill_info() and add another PCI_FILL_xxx flag for CXL RCD properties, which will be available in struct pci_dev (beware that new fields have to be added _after_ all public fields to keep ABI compatibility). > @@ -1445,6 +1515,9 @@ cap_express(struct device *d, int where, int cap) > cap_express_dev(d, where, type); > if (link) > cap_express_link(d, where, type); > + else if (type == PCI_EXP_TYPE_ROOT_INT_EP) > + cap_express_link_rcd(d); > + > if (slot) > cap_express_slot(d, where); > if (type == PCI_EXP_TYPE_ROOT_PORT || type == PCI_EXP_TYPE_ROOT_EC) Does it make sense to look up CXL RCD information for all PCIe devices of type PCI_EXP_TYPE_ROOT_INT_EP? Shouldn't it be done only for devices with the CXL capability? Have a nice fortnight
Martin Mareš wrote: [..] > > This really is not the right place to read from sysfs. The libpci should provide > a backend-indepenent interface for reading this information and the sysfs > back-end (lib/sysfs.c) should provide one implementation of this interface. > > I think that we can easily extend pci_fill_info() and add another PCI_FILL_xxx > flag for CXL RCD properties, which will be available in struct pci_dev (beware > that new fields have to be added _after_ all public fields to keep ABI compatibility). > > > @@ -1445,6 +1515,9 @@ cap_express(struct device *d, int where, int cap) > > cap_express_dev(d, where, type); > > if (link) > > cap_express_link(d, where, type); > > + else if (type == PCI_EXP_TYPE_ROOT_INT_EP) > > + cap_express_link_rcd(d); > > + > > if (slot) > > cap_express_slot(d, where); > > if (type == PCI_EXP_TYPE_ROOT_PORT || type == PCI_EXP_TYPE_ROOT_EC) > > Does it make sense to look up CXL RCD information for all PCIe devices of type > PCI_EXP_TYPE_ROOT_INT_EP? Shouldn't it be done only for devices with the CXL > capability? I think so, would this fit more naturally in pci_scan_caps() with a new scan for DVSEC caps ("PCI_EXT_CAP_ID_DVSEC" in Linux). However, isn't the trouble that this needs a DVSEC scan for CXL to know it needs to go back and fill in details that normally in appear in the base PCIe capability scan?
Hello! > > Does it make sense to look up CXL RCD information for all PCIe devices of type > > PCI_EXP_TYPE_ROOT_INT_EP? Shouldn't it be done only for devices with the CXL > > capability? > > I think so, would this fit more naturally in pci_scan_caps() with a new > scan for DVSEC caps ("PCI_EXT_CAP_ID_DVSEC" in Linux). However, isn't > the trouble that this needs a DVSEC scan for CXL to know it needs to go > back and fill in details that normally in appear in the base PCIe > capability scan? I would prefer to display all CXL stuff together (i.e., when showing the DVSEC caps). Is there any reason not to? Martin
Martin Mareš wrote: > Hello! > > > > Does it make sense to look up CXL RCD information for all PCIe devices of type > > > PCI_EXP_TYPE_ROOT_INT_EP? Shouldn't it be done only for devices with the CXL > > > capability? > > > > I think so, would this fit more naturally in pci_scan_caps() with a new > > scan for DVSEC caps ("PCI_EXT_CAP_ID_DVSEC" in Linux). However, isn't > > the trouble that this needs a DVSEC scan for CXL to know it needs to go > > back and fill in details that normally in appear in the base PCIe > > capability scan? > > I would prefer to display all CXL stuff together (i.e., when showing the > DVSEC caps). Is there any reason not to? Together with the DVSEC caps display makes sense to me as well.
Martin Mareš wrote: [..] > > This really is not the right place to read from sysfs. The libpci > should provide a backend-indepenent interface for reading this > information and the sysfs back-end (lib/sysfs.c) should provide one implementation of this interface. > > I think that we can easily extend pci_fill_info() and add another > PCI_FILL_xxx flag for CXL RCD properties, which will be available in > struct pci_dev (beware that new fields have to be added _after_ all public fields to keep ABI compatibility). > Thank you for your feedback. I understand that lib/sysfs.c is the appropriate location. I also understand that to extend it, I need to add code in pci_fill_info() to read the sysfs file and add a new flag at the bottom of PCI_FILL_xx. Thank you for the clarification. I will update this in the next patch and submit it separately from the driver implementation. Dan Williams wrote: > Martin Mareš wrote: > > Hello! > > > > > > Does it make sense to look up CXL RCD information for all PCIe devices > of type > > > > PCI_EXP_TYPE_ROOT_INT_EP? Shouldn't it be done only for devices > with the CXL > > > > capability? > > > > > > I think so, would this fit more naturally in pci_scan_caps() with a new > > > scan for DVSEC caps ("PCI_EXT_CAP_ID_DVSEC" in Linux). However, > isn't > > > the trouble that this needs a DVSEC scan for CXL to know it needs to go > > > back and fill in details that normally in appear in the base PCIe > > > capability scan? > > > > I would prefer to display all CXL stuff together (i.e., when showing the > > DVSEC caps). Is there any reason not to? > > Together with the DVSEC caps display makes sense to me as well. Here is my understanding of this point. Please let me know if there is a more appropriate approach. Based on my understanding, the information we are trying to display with this feature is included in the PCIe Capability. Therefore, I believe it is appropriate to display it within the cap_express() function. By checking the DVSEC, we can determine whether this device is a CXL device or not. However, I couldn't find a proper way to check the DVSEC within the cap_express() function. As a result, I explored other options and set the PCI_EXP_TYPE_ROOT_INT_EP as a branching condition.(cxl3.0 specification 7.3.1.2, 9.10) Furthermore, since rcd is identified as PCI_EXP_TYPE_ROOT_INT_EP by PCI_EXP_FLAGS_TYPE, cap_express_link() is not called. Alternatively, I might be able to create a new rcd variable for pci_fill_info() in struct pci_dev and make it a branch condition.
diff --git a/lib/access.c b/lib/access.c index 7d66123..bc75a84 100644 --- a/lib/access.c +++ b/lib/access.c @@ -12,6 +12,7 @@ #include <stdlib.h> #include <stdarg.h> #include <string.h> +#include <fcntl.h> #include "internal.h" @@ -268,3 +269,31 @@ pci_get_string_property(struct pci_dev *d, u32 prop) return NULL; } + +#define OBJNAMELEN 1024 +#define OBJBUFSIZE 64 +int +get_rcd_sysfs_obj_file(struct pci_dev *d, char *object, char *result) +{ +#ifdef PCI_HAVE_PM_LINUX_SYSFS + char namebuf[OBJNAMELEN]; + int n = snprintf(namebuf, OBJNAMELEN, "%s/devices/%04x:%02x:%02x.%d/%s", + pci_get_param(d->access, "sysfs.path"), + d->domain, d->bus, d->dev, d->func, object); + if (n < 0 || n >= OBJNAMELEN){ + d->access->error("Failed to get filename"); + return -1; + } + int fd = open(namebuf, O_RDONLY); + if(fd < 0) + return -1; + n = read(fd, result, OBJBUFSIZE); + if (n < 0 || n >= OBJBUFSIZE){ + d->access->error("Failed to read the file"); + return -1; + } + return 0; +#else + return -1; +#endif +} diff --git a/lib/pci.h b/lib/pci.h index 2322bf7..fb25069 100644 --- a/lib/pci.h +++ b/lib/pci.h @@ -187,6 +187,8 @@ int pci_write_long(struct pci_dev *, int pos, u32 data) PCI_ABI; int pci_read_block(struct pci_dev *, int pos, u8 *buf, int len) PCI_ABI; int pci_write_block(struct pci_dev *, int pos, u8 *buf, int len) PCI_ABI; +int get_rcd_sysfs_obj_file(struct pci_dev *d, char *object, char *result) PCI_ABI; + /* * Most device properties take some effort to obtain, so libpci does not * initialize them during default bus scan. Instead, you have to call diff --git a/ls-caps.c b/ls-caps.c index 1b63262..12c9f34 100644 --- a/ls-caps.c +++ b/ls-caps.c @@ -10,6 +10,8 @@ #include <stdio.h> #include <string.h> +#include <fcntl.h> +#include <stdlib.h> #include "lspci.h" @@ -1381,6 +1383,74 @@ static void cap_express_slot2(struct device *d UNUSED, int where UNUSED) /* No capabilities that require this field in PCIe rev2.0 spec. */ } +#define OBJBUFSIZE 64 +static void cap_express_link_rcd(struct device *d){ + u32 t, aspm, cap_speed, cap_width, sta_speed, sta_width; + u16 w; + struct pci_dev *pdev = d->dev; + char buf[OBJBUFSIZE]; + + if(get_rcd_sysfs_obj_file(pdev, "rcd_link_cap", buf)) + return; + t = (u32)strtoul(buf, NULL, 16); + + aspm = (t & PCI_EXP_LNKCAP_ASPM) >> 10; + cap_speed = t & PCI_EXP_LNKCAP_SPEED; + cap_width = (t & PCI_EXP_LNKCAP_WIDTH) >> 4; + printf("\t\tLnkCap:\tPort #%d, Speed %s, Width x%d, ASPM %s", + t >> 24, + link_speed(cap_speed), cap_width, + aspm_support(aspm)); + if (aspm) + { + printf(", Exit Latency "); + if (aspm & 1) + printf("L0s %s", latency_l0s((t & PCI_EXP_LNKCAP_L0S) >> 12)); + if (aspm & 2) + printf("%sL1 %s", (aspm & 1) ? ", " : "", + latency_l1((t & PCI_EXP_LNKCAP_L1) >> 15)); + } + printf("\n"); + printf("\t\t\tClockPM%c Surprise%c LLActRep%c BwNot%c ASPMOptComp%c\n", + FLAG(t, PCI_EXP_LNKCAP_CLOCKPM), + FLAG(t, PCI_EXP_LNKCAP_SURPRISE), + FLAG(t, PCI_EXP_LNKCAP_DLLA), + FLAG(t, PCI_EXP_LNKCAP_LBNC), + FLAG(t, PCI_EXP_LNKCAP_AOC)); + + if(!get_rcd_sysfs_obj_file(pdev, "rcd_link_ctrl", buf)){ + w = (u16)strtoul(buf, NULL, 16); + printf("\t\tLnkCtl:\tASPM %s;", aspm_enabled(w & PCI_EXP_LNKCTL_ASPM)); + printf(" Disabled%c CommClk%c\n\t\t\tExtSynch%c ClockPM%c AutWidDis%c BWInt%c AutBWInt%c\n", + FLAG(w, PCI_EXP_LNKCTL_DISABLE), + FLAG(w, PCI_EXP_LNKCTL_CLOCK), + FLAG(w, PCI_EXP_LNKCTL_XSYNCH), + FLAG(w, PCI_EXP_LNKCTL_CLOCKPM), + FLAG(w, PCI_EXP_LNKCTL_HWAUTWD), + FLAG(w, PCI_EXP_LNKCTL_BWMIE), + FLAG(w, PCI_EXP_LNKCTL_AUTBWIE)); + } + + if(!get_rcd_sysfs_obj_file(pdev, "rcd_link_status", buf)){ + w = (u16)strtoul(buf, NULL, 16); + sta_speed = w & PCI_EXP_LNKSTA_SPEED; + sta_width = (w & PCI_EXP_LNKSTA_WIDTH) >> 4; + printf("\t\tLnkSta:\tSpeed %s%s, Width x%d%s\n", + link_speed(sta_speed), + link_compare(PCI_EXP_TYPE_ROOT_INT_EP, sta_speed, cap_speed), + sta_width, + link_compare(PCI_EXP_TYPE_ROOT_INT_EP, sta_width, cap_width)); + printf("\t\t\tTrErr%c Train%c SlotClk%c DLActive%c BWMgmt%c ABWMgmt%c\n", + FLAG(w, PCI_EXP_LNKSTA_TR_ERR), + FLAG(w, PCI_EXP_LNKSTA_TRAIN), + FLAG(w, PCI_EXP_LNKSTA_SL_CLK), + FLAG(w, PCI_EXP_LNKSTA_DL_ACT), + FLAG(w, PCI_EXP_LNKSTA_BWMGMT), + FLAG(w, PCI_EXP_LNKSTA_AUTBW)); + } + return; +} + static int cap_express(struct device *d, int where, int cap) { @@ -1445,6 +1515,9 @@ cap_express(struct device *d, int where, int cap) cap_express_dev(d, where, type); if (link) cap_express_link(d, where, type); + else if (type == PCI_EXP_TYPE_ROOT_INT_EP) + cap_express_link_rcd(d); + if (slot) cap_express_slot(d, where); if (type == PCI_EXP_TYPE_ROOT_PORT || type == PCI_EXP_TYPE_ROOT_EC)