Re: [PATCH v6 1/2] PCI/LMR: Add Lane Margining at the Receiver debugfs interface

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] pci/lmr: pci_lmr_no_cmd() fails due to U16_MAX strict match against Usage Model bit
- [Low] pci/lmr: Format string mismatch passing promoted int to %lu specifier
--

commit 0a41b591425afa70cf540096078e9a59a614786c
Author: Priyank Rathod <rathodpriyank@google.com>

PCI/LMR: Add Lane Margining at the Receiver debugfs interface

Lane Margining at the Receiver moves the sampling point of a receiver
away from its normal setting, in time or in voltage, while the link
stays up, and reports the errors the receiver sees.

> diff --git a/drivers/pci/pcie/margin.c b/drivers/pci/pcie/margin.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/pci/pcie/margin.c
[ ... ]
> +static int pci_lmr_no_cmd(struct pci_lmr_port *port, unsigned int lane)
> +{
> +	u16 cmd = pci_lmr_word(0, LMR_TYPE_NO_CMD, LMR_NO_CMD);
> +
> +	return pci_lmr_write_wait(port, lane, cmd, U16_MAX, NULL);

[Severity: Medium]
Will this wait timeout unconditionally?

Looking at pci_lmr_write_wait() in drivers/pci/pcie/margin.c, it uses the
U16_MAX mask to wait for the lane status to exactly match cmd. However, the
command encodes bit 6 (Usage Model) as 0, while hardware that supports
margining will always set this read-only bit to 1, as defined in
include/uapi/linux/pci_regs.h:

#define  PCI_LMR_LANE_USAGE        0x0040    /* Usage Model */

This seems like it would cause the exact match condition to permanently fail,
leading to a timeout for all margining operations.

> +}
[ ... ]
> +static int status_show(struct seq_file *s, void *unused)
> +{
[ ... ]
> +	seq_printf(s, "%s %s %lu\n",
> +		   type == LMR_TYPE_TIMING ? "timing" : "voltage",
> +		   pci_lmr_step_status[FIELD_GET(LMR_STEP_STATUS, payload)],
> +		   FIELD_GET(LMR_STEP_ERR_COUNT, payload));

[Severity: Low]
Does this seq_printf() call result in undefined behavior on 64-bit
architectures?

In status_show() within drivers/pci/pcie/margin.c, the payload variable
is a u8, and FIELD_GET() evaluates to a u8, which the varargs of seq_printf()
promote to a 32-bit int. However, the format specifier %lu demands an 8-byte
unsigned long. Reading 8 bytes for a 4-byte argument can produce garbage
output or undefined behavior.

> +
> +	return 0;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261006-pcie-link-endpoints-v6-0-154d71657d32@google.com?part=1