Message ID | 20181030093139.10226-1-kurt@linutronix.de (mailing list archive) |
---|---|
Headers | show |
Series | net: xlinx: mdio: recheck condition after timeout | expand |
On Tue, Oct 30, 2018 at 10:31:37AM +0100, Kurt Kanzenbach wrote: > Hi, > > the Xilinx mdio wait functions may return false positives under certain > circumstances: If the functions get preempted between reading the corresponding > mdio register and checking for the timeout, they could falsely indicate a > timeout. Hi Kurt I wonder if it would be possible to add a readx_poll_timeout() which passes two parameters to op()? I keep seeing this basic problem in various drivers, and try to point people towards readx_poll_timeout(), but it is not the best of fit. Otherwise, could you add a axienet_ior_read_mcr(lp), and use readx_poll_timeout() as is? Andrew
On Tue, Oct 30, 2018 at 10:31:39AM +0100, Kurt Kanzenbach wrote: > The function could report a false positive if it gets preempted between reading > the XEL_MDIOCTRL_OFFSET register and checking for the timeout. In such a case, > the condition has to be rechecked to avoid false positives. > > Therefore, check for expected condition even after the timeout occurred. > > Signed-off-by: Kurt Kanzenbach <kurt@linutronix.de> > --- > drivers/net/ethernet/xilinx/xilinx_emaclite.c | 20 +++++++++++++++----- > 1 file changed, 15 insertions(+), 5 deletions(-) > > diff --git a/drivers/net/ethernet/xilinx/xilinx_emaclite.c b/drivers/net/ethernet/xilinx/xilinx_emaclite.c > index 639e3e99af46..957d03085bd0 100644 > --- a/drivers/net/ethernet/xilinx/xilinx_emaclite.c > +++ b/drivers/net/ethernet/xilinx/xilinx_emaclite.c > @@ -714,19 +714,29 @@ static irqreturn_t xemaclite_interrupt(int irq, void *dev_id) > static int xemaclite_mdio_wait(struct net_local *lp) > { > unsigned long end = jiffies + 2; > + u32 val; > > /* wait for the MDIO interface to not be busy or timeout > * after some time. > */ > - while (xemaclite_readl(lp->base_addr + XEL_MDIOCTRL_OFFSET) & > - XEL_MDIOCTRL_MDIOSTS_MASK) { > + while (1) { > + val = xemaclite_readl(lp->base_addr + XEL_MDIOCTRL_OFFSET); Hi Kurt It looks like readx_poll_timeout() should work here. Andrew
<snip> > > On Tue, Oct 30, 2018 at 10:31:39AM +0100, Kurt Kanzenbach wrote: > > The function could report a false positive if it gets preempted between > reading > > the XEL_MDIOCTRL_OFFSET register and checking for the timeout. In such a > case, > > the condition has to be rechecked to avoid false positives. > > > > Therefore, check for expected condition even after the timeout occurred. > > > > Signed-off-by: Kurt Kanzenbach <kurt@linutronix.de> > > --- > > drivers/net/ethernet/xilinx/xilinx_emaclite.c | 20 +++++++++++++++----- > > 1 file changed, 15 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/net/ethernet/xilinx/xilinx_emaclite.c > b/drivers/net/ethernet/xilinx/xilinx_emaclite.c > > index 639e3e99af46..957d03085bd0 100644 > > --- a/drivers/net/ethernet/xilinx/xilinx_emaclite.c > > +++ b/drivers/net/ethernet/xilinx/xilinx_emaclite.c > > @@ -714,19 +714,29 @@ static irqreturn_t xemaclite_interrupt(int irq, void > *dev_id) > > static int xemaclite_mdio_wait(struct net_local *lp) > > { > > unsigned long end = jiffies + 2; > > + u32 val; > > > > /* wait for the MDIO interface to not be busy or timeout > > * after some time. > > */ > > - while (xemaclite_readl(lp->base_addr + XEL_MDIOCTRL_OFFSET) & > > - XEL_MDIOCTRL_MDIOSTS_MASK) { > > + while (1) { > > + val = xemaclite_readl(lp->base_addr + > XEL_MDIOCTRL_OFFSET); > > Hi Kurt > > It looks like readx_poll_timeout() should work here. Yes, valid point. readx_poll_timeout API repoll addr after timeout. Reusing it would simplify the flow. > > Andrew
Hi Andrew, On Tue, Oct 30, 2018 at 01:08:31PM +0100, Andrew Lunn wrote: > On Tue, Oct 30, 2018 at 10:31:37AM +0100, Kurt Kanzenbach wrote: > > Hi, > > > > the Xilinx mdio wait functions may return false positives under certain > > circumstances: If the functions get preempted between reading the corresponding > > mdio register and checking for the timeout, they could falsely indicate a > > timeout. > > Hi Kurt > > I wonder if it would be possible to add a readx_poll_timeout() which > passes two parameters to op()? actually I was thinking about using readx_poll_timeout(). But as you already pointed out, it expects only one parameter for op(). I'm not sure about adding a new readx_poll_timeout() macro. > > I keep seeing this basic problem in various drivers, and try to point > people towards readx_poll_timeout(), but it is not the best of fit. > > Otherwise, could you add a axienet_ior_read_mcr(lp), and use > readx_poll_timeout() as is? I guess that would work. I'll use readx_poll_timeout() for both wait functions and send a v2. Thanks, Kurt