Re: [PATCH v3 0/3] Display cxl1.1 device link status

[Dan Williams <dan.j.williams@intel.com>]

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.