linux-kernel - Re: [PATCH v9 2/2] PCI: trace: Add a RAS tracepoint to monitor link speed changes

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <aISJHjFrHkxVNFzJ@wunner.de>
Date: Sat, 26 Jul 2025 09:51:58 +0200
From: Lukas Wunner <lukas@...ner.de>
To: Bjorn Helgaas <helgaas@...nel.org>
Cc: Shuai Xue <xueshuai@...ux.alibaba.com>, rostedt@...dmis.org,
	linux-pci@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-edac@...r.kernel.org, linux-trace-kernel@...r.kernel.org,
	ilpo.jarvinen@...ux.intel.com, mattc@...estorage.com,
	Jonathan.Cameron@...wei.com, bhelgaas@...gle.com,
	tony.luck@...el.com, bp@...en8.de, mhiramat@...nel.org,
	mathieu.desnoyers@...icios.com, oleg@...hat.com, naveen@...nel.org,
	davem@...emloft.net, anil.s.keshavamurthy@...el.com,
	mark.rutland@....com, peterz@...radead.org,
	tianruidong@...ux.alibaba.com
Subject: Re: [PATCH v9 2/2] PCI: trace: Add a RAS tracepoint to monitor link
 speed changes

On Fri, Jul 25, 2025 at 04:09:21PM -0500, Bjorn Helgaas wrote:
> > @@ -319,8 +319,7 @@ int pciehp_check_link_status(struct controller *ctrl)
> >  		return -1;
> >  	}
> >  
> > -	pcie_capability_read_word(pdev, PCI_EXP_LNKSTA2, &linksta2);
> > -	__pcie_update_link_speed(ctrl->pcie->port->subordinate, lnk_status, linksta2);
> > +	pcie_update_link_speed(ctrl->pcie->port->subordinate, PCIE_HOTPLUG);
> 
> It kind of bugs me that the hot-add flow reads LNKSTA three times and
> generates both pci_hp_event LINK_UP and link_event tracepoints:
> 
>   pciehp_handle_presence_or_link_change
>     link_active = pciehp_check_link_active()
>       pcie_capability_read_word(PCI_EXP_LNKSTA)
>     if (link_active)
>       ctrl_info(ctrl, "Slot(%s): Link Up\n")

This LNKSTA read decides whether to bring up the slot.
It can't be eliminated.

>       trace_pci_hp_event(PCI_HOTPLUG_LINK_UP)
>       pciehp_enable_slot
>         __pciehp_enable_slot
>           board_added
>             pciehp_check_link_status
>               pcie_capability_read_word(PCI_EXP_LNKSTA)

This is sort of a final check whether the link is (still) active
before commencing device enumeration.  Doesn't look like it can
safely be eliminated either.

>               pcie_update_link_speed
>                 pcie_capability_read_word(PCI_EXP_LNKSTA)
>                 pcie_capability_read_word(PCI_EXP_LNKSTA2)
>                 trace_pcie_link_event(<REASON>)

This third register read is introduced by the present patch and is
indeed somewhat a step back, given that pciehp_check_link_status()
currently deliberately calls __pcie_update_link_speed() to pass
the already read LNKSTA value.

I'm wondering if the tracepoint can be moved down to
__pcie_update_link_speed()?

> And maybe we need both a bare LINK_UP event and a link_event with all
> the details, but again it seems a little weird to me that there are
> two tracepoints when there's really only one event and we know all the
> link_event information from the very first LNKSTA read.

One of the reasons is that a "Link Down" event would have to
contain dummy values for link speed etc, so it seemed cleaner
to separate the hotplug event from the link speed event.

Thanks,

Lukas