| Message ID | 20240312080559.14904-1-kobayashi.da-06@fujitsu.com |
|---|---|
| Headers | show |
| Series | Display cxl1.1 device link status | expand |
Kindly ping Any comment is welcome. On Tue, March 12, 2024, Kobayashi,Daisuke wrote: > -----Original Message----- > From: Kobayashi,Daisuke <kobayashi.da-06@fujitsu.com> > Sent: Tuesday, March 12, 2024 5:06 PM > To: Kobayashi, Daisuke/小林 大介 <kobayashi.da-06@fujitsu.com>; > linux-cxl@vger.kernel.org > Cc: Gotou, Yasunori/五島 康文 <y-goto@fujitsu.com>; > linux-pci@vger.kernel.org; mj@ucw.cz; dan.j.williams@intel.com; Kobayashi, > Daisuke/小林 大介 <kobayashi.da-06@fujitsu.com> > Subject: [PATCH v3 0/3] Display cxl1.1 device link status > > Hello. > > This patch series adds a feature that displays the link status > of the CXL1.1 device. > > CXL devices are extensions of PCIe. Therefore, from CXL2.0 onwards, > the link status can be output in the same way as traditional PCIe. > However, unlike devices from CXL2.0 onwards, CXL1.1 requires a > different method to obtain the link status from traditional PCIe. > This is because the link status of the CXL1.1 device is not mapped > in the configuration space (as per cxl3.0 specification 8.1). > Instead, the configuration space containing the link status is mapped > to the memory mapped register region (as per cxl3.0 specification 8.2, > Table 8-18). Therefore, the current lspci has a problem where it does > not display the link status of the CXL1.1 device. > This patch solves these issues. > > The procedure is as follows: > First, obtain the RCRB address within the cxl driver, then access > the configuration space. Next, output the link status information from > the configuration space to sysfs. Finally, read sysfs within lspci to > display the link status information. > > Changes > v1[1] -> v2: > The following are the main changes made based on the feedback from Dan > Williams. > - Modified to perform rcrb access within the CXL driver. > - Added new attributes to the sysfs of the PCI device. > - Output the link status information to the sysfs of the PCI device. > - Retrieve information from sysfs as the source when displaying information in > lspci. > > v2[2] -> v3: > - Fix unnecessary initialization and wrong types (Bjohn). > - Create a helper function for getting a PCIe capability offset (Bjohn). > - Move platform-specific implementation to the lib directory in pciutils > (Martin). > > [1] > https://lore.kernel.org/linux-cxl/20231220050738.178481-1-kobayashi.da-06 > @fujitsu.com/ > [2] > https://lore.kernel.org/linux-cxl/20240227083313.87699-1-kobayashi.da-06@ > fujitsu.com/ > > Kobayashi,Daisuke (3): > Add sysfs attribute for CXL 1.1 device link status > Remove conditional branch that is not suitable for cxl1.1 devices > > drivers/cxl/acpi.c | 4 - > drivers/cxl/pci.c | 193 > +++++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 193 insertions(+), 4 deletions(-) > > Add function to display cxl1.1 device link status > > lib/access.c | 29 +++++++++++++++++++++ > lib/pci.h | 2 ++ > ls-caps.c | 73 > ++++++++++++++++++++++++++++++++++++++++++++++++ > ++++ > 3 files changed, 104 insertions(+) > -- > 2.43.0
Kobayashi,Daisuke wrote: > Hello. > > This patch series adds a feature that displays the link status > of the CXL1.1 device. Good write up! One minor feedback going forward is to drop usage of "This patch" in a description as that is self evident. > CXL devices are extensions of PCIe. Therefore, from CXL2.0 onwards, > the link status can be output in the same way as traditional PCIe. > However, unlike devices from CXL2.0 onwards, CXL1.1 requires a > different method to obtain the link status from traditional PCIe. > This is because the link status of the CXL1.1 device is not mapped > in the configuration space (as per cxl3.0 specification 8.1). > Instead, the configuration space containing the link status is mapped > to the memory mapped register region (as per cxl3.0 specification 8.2, > Table 8-18). Therefore, the current lspci has a problem where it does > not display the link status of the CXL1.1 device. > This patch solves these issues. One common way to rewrite a "This patch..." sentence is in "imperative" tense, so for example: "Solve these issues with sysfs attributes to expose the status registers hidden in the RCRB." ...i.e. write the commit log as if the patch is commanding the code to change. You can find some more notes about this here: https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#patch-submission-notes ...but really, this cover letter is higher quality than most, so thanks for that! > > The procedure is as follows: > First, obtain the RCRB address within the cxl driver, then access > the configuration space. Next, output the link status information from > the configuration space to sysfs. Finally, read sysfs within lspci to > display the link status information. > > Changes > v1[1] -> v2: > The following are the main changes made based on the feedback from Dan Williams. > - Modified to perform rcrb access within the CXL driver. > - Added new attributes to the sysfs of the PCI device. > - Output the link status information to the sysfs of the PCI device. > - Retrieve information from sysfs as the source when displaying information in lspci. > > v2[2] -> v3: > - Fix unnecessary initialization and wrong types (Bjohn). > - Create a helper function for getting a PCIe capability offset (Bjohn). > - Move platform-specific implementation to the lib directory in pciutils (Martin). > > [1] > https://lore.kernel.org/linux-cxl/20231220050738.178481-1-kobayashi.da-06@fujitsu.com/ > [2] > https://lore.kernel.org/linux-cxl/20240227083313.87699-1-kobayashi.da-06@fujitsu.com/ > > Kobayashi,Daisuke (3): > Add sysfs attribute for CXL 1.1 device link status > Remove conditional branch that is not suitable for cxl1.1 devices > > drivers/cxl/acpi.c | 4 - > drivers/cxl/pci.c | 193 +++++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 193 insertions(+), 4 deletions(-) > > Add function to display cxl1.1 device link status > > lib/access.c | 29 +++++++++++++++++++++ > lib/pci.h | 2 ++ > ls-caps.c | 73 ++++++++++++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 104 insertions(+) The typical way I handle cases where I am updating the kernel side and the user tooling side of a problem is to post the kernel changes and then separately post the user changes with a lore link to the kernel submission. For PCI utils I believe you will need to send a separate pull request via github once the kernel changes are accepted.
Dan Williams wrote: > Kobayashi,Daisuke wrote: > > Hello. > > > > This patch series adds a feature that displays the link status > > of the CXL1.1 device. > > Good write up! One minor feedback going forward is to drop usage of > "This patch" in a description as that is self evident. > > > CXL devices are extensions of PCIe. Therefore, from CXL2.0 onwards, > > the link status can be output in the same way as traditional PCIe. > > However, unlike devices from CXL2.0 onwards, CXL1.1 requires a > > different method to obtain the link status from traditional PCIe. > > This is because the link status of the CXL1.1 device is not mapped > > in the configuration space (as per cxl3.0 specification 8.1). > > Instead, the configuration space containing the link status is mapped > > to the memory mapped register region (as per cxl3.0 specification 8.2, > > Table 8-18). Therefore, the current lspci has a problem where it does > > not display the link status of the CXL1.1 device. > > This patch solves these issues. > > One common way to rewrite a "This patch..." sentence is in "imperative" > tense, so for example: > > "Solve these issues with sysfs attributes to expose the status > registers hidden in the RCRB." > > ...i.e. write the commit log as if the patch is commanding the code to > change. You can find some more notes about this here: > > https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#patch- > submission-notes > > ...but really, this cover letter is higher quality than most, so thanks > for that! > Thank you for your kind feedback. In the updated patch, I will revise the text and submit it. > > Kobayashi,Daisuke (3): > > Add sysfs attribute for CXL 1.1 device link status > > Remove conditional branch that is not suitable for cxl1.1 devices > > > > drivers/cxl/acpi.c | 4 - > > drivers/cxl/pci.c | 193 > +++++++++++++++++++++++++++++++++++++++++++++ > > 2 files changed, 193 insertions(+), 4 deletions(-) > > > > Add function to display cxl1.1 device link status > > > > lib/access.c | 29 +++++++++++++++++++++ > > lib/pci.h | 2 ++ > > ls-caps.c | 73 > ++++++++++++++++++++++++++++++++++++++++++++++++ > ++++ > > 3 files changed, 104 insertions(+) > > The typical way I handle cases where I am updating the kernel side and > the user tooling side of a problem is to post the kernel changes and > then separately post the user changes with a lore link to the kernel > submission. > > For PCI utils I believe you will need to send a separate pull request > via github once the kernel changes are accepted. I understand that it’s better to post separately. After the changes to the kernel are accepted, I will submit a patch to pciutils.