Re: [PATCH v2 1/2] PCI: Track Flit Mode Status & print it with link status

[Bjorn Helgaas <helgaas@kernel.org>]

On Fri, Feb 21, 2025 at 10:29:48AM -0500, Yazen Ghannam wrote:
> On Fri, Feb 07, 2025 at 06:18:35PM +0200, Ilpo Järvinen wrote:
> > PCIe r6.0 added Flit mode that mainly alters HW behavior but some OS
> > visible changes are also because of it. The OS visible changes include
> 
> The first sentence reads oddly. Maybe a slight change?
> 
> "...but there are some OS visible changes because of it."

Updated locally.

> > +	if (dev->bus && dev->bus->flit_mode)
> > +		flit_mode = ", in Flit mode";
> > +
> >  	if (bw_avail >= bw_cap && verbose)
> > -		pci_info(dev, "%u.%03u Gb/s available PCIe bandwidth (%s x%d link)\n",
> > +		pci_info(dev, "%u.%03u Gb/s available PCIe bandwidth (%s x%d link)%s\n",
> >  			 bw_cap / 1000, bw_cap % 1000,
> > -			 pci_speed_string(speed_cap), width_cap);
> > +			 pci_speed_string(speed_cap), width_cap, flit_mode);
> >  	else if (bw_avail < bw_cap)
> > -		pci_info(dev, "%u.%03u Gb/s available PCIe bandwidth, limited by %s x%d link at %s (capable of %u.%03u Gb/s with %s x%d link)\n",
> > +		pci_info(dev, "%u.%03u Gb/s available PCIe bandwidth, limited by %s x%d link at %s (capable of %u.%03u Gb/s with %s x%d link)%s\n",
> >  			 bw_avail / 1000, bw_avail % 1000,
> >  			 pci_speed_string(speed), width,
> >  			 limiting_dev ? pci_name(limiting_dev) : "<unknown>",
> >  			 bw_cap / 1000, bw_cap % 1000,
> > -			 pci_speed_string(speed_cap), width_cap);
> > +			 pci_speed_string(speed_cap), width_cap, flit_mode);
> 
> Does the "Flit mode" message *need* to go into these lines? Could it be
> its own message?

I suppose it doesn't need to be there, and these bandwidth lines are
already pretty long (my fault, open to suggestions to shorten them),
but I do think it's useful to have related info all on the same line.

>  +#include <linux/string_choices.h>
> 
>  @@ -6190,21 +6190,25 @@ void __pcie_print_link_status(struct pci_dev *dev, bool verbose)
>   	enum pci_bus_speed speed, speed_cap;
>   	struct pci_dev *limiting_dev = NULL;
>   	u32 bw_avail, bw_cap;
>   
>   	bw_cap = pcie_bandwidth_capable(dev, &speed_cap, &width_cap);
>   	bw_avail = pcie_bandwidth_available(dev, &limiting_dev, &speed, &width);
>   
>  +	if (dev->bus)
>  +		pci_info(dev, "Flit mode: %s\n", str_enabled_disabled(dev->bus->flit_mode);
>  +
>   	if (bw_avail >= bw_cap && verbose)
>  		pci_info(dev, "%u.%03u Gb/s available PCIe bandwidth (%s x%d link)\n",
>   			 bw_cap / 1000, bw_cap % 1000,
>  			 pci_speed_string(speed_cap), width_cap);
>   	else if (bw_avail < bw_cap)
>  		pci_info(dev, "%u.%03u Gb/s available PCIe bandwidth, limited by %s x%d link at %s (capable of %u.%03u Gb/s with %s x%d link)\n",
>   			 bw_avail / 1000, bw_avail % 1000,
>   			 pci_speed_string(speed), width,
>   			 limiting_dev ? pci_name(limiting_dev) : "<unknown>",
>   			 bw_cap / 1000, bw_cap % 1000,
>  			 pci_speed_string(speed_cap), width_cap);
> 
> >  }