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
