Re: [PATCH v2 1/1] PCI/bwctrl: Replace lbms_count with PCI_LINK_LBMS_SEEN flag

[Lukas Wunner <lukas@wunner.de>]

On Fri, Apr 25, 2025 at 03:24:45PM +0300, Ilpo Järvinen wrote:
> On Fri, 25 Apr 2025, Lukas Wunner wrote:
> > On Thu, Apr 24, 2025 at 03:37:38PM +0300, Ilpo Järvinen wrote:
> > > On Thu, 24 Apr 2025, Lukas Wunner wrote:
> > > >   The only concern here is whether the cached
> > > >   link speed is updated.  pcie_bwctrl_change_speed() does call
> > > >   pcie_update_link_speed() after calling pcie_retrain_link(), so that
> > > >   looks fine.  But there's a second caller of pcie_retrain_link():
> > > >   pcie_aspm_configure_common_clock().  It doesn't update the cached
> > > >   link speed after calling pcie_retrain_link().  Not sure if this can
> > > >   lead to a change in link speed and therefore the cached link speed
> > > >   should be updated?  The Target Link Speed isn't changed, but maybe
> > > >   the link fails to retrain to the same speed for electrical reasons?
> > > 
> > > I've never seen that to happen but it would seem odd if that is forbidden 
> > > (as the alternative is probably that the link remains down).
> > > 
> > > Perhaps pcie_reset_lbms() should just call pcie_update_link_speed() as the 
> > > last step, then the irq handler returning IRQ_NONE doesn't matter.
> > 
> > Why pcie_reset_lbms()?  I was rather thinking that pcie_update_link_speed()
> > should be called from pcie_retrain_link().  Maybe right after the final
> > pcie_wait_for_link_status().
> 
> My reasonale for having it in pcie_reset_lbms() is that LBMS is cleared
> there which races with the irq handler reading LBMS. If LBMS is cleared 
> before the irq handler reads linksta register, it returns IRQ_NONE and 
> will misses the LBMS event. So this race problem is directly associated 
> with the write-to-clear of LBMS.

pcie_reset_lbms() is only called from two places:

(1) pciehp's remove_board() -- no need to update the link speed of an empty
    slot and you've proven that the speed *is* updated by board_added()
    once there is a new card in the slot.

(2) pcie_retrain_link() -- retraining could always lead to a different
    speed, e.g. due to electrical issues, so unconditionally updating
    the link speed makes sense.

It feels awkward that a function named pcie_reset_lbms() would also
update the link speed as a side effect.

> While I don't disagree with that spec interpretation, in case of ASPM, the 
> question is more complex than that. The link was already trained to speed 
> x, can the new link training result in failing to train to x (in 
> practice).

It's probably rare but bad wiring or soldering issues can always cause
a lower or higher speed than before.

My recommendation would be to move the invocation of
pcie_update_link_speed() from pcie_bwctrl_change_speed()
to pcie_retrain_link().

Just to cover the case that the retraining initiated by
pcie_aspm_configure_common_clock() leads to a different speed
and pcie_reset_lbms() wins the race against the bwctrl irq handler.
It's a corner case, but if we've identified it now, might as well
fix it I guess?

Thanks,

Lukas