Received-SPF: softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
Date:   Sat, 21 May 2022 15:55:30 +0200
From:   Andrew Lunn <andrew@lunn.ch>
To:     Pavel Skripkin <paskripkin@gmail.com>
Cc:     vladimir.oltean@nxp.com, claudiu.manoil@nxp.com,
        alexandre.belloni@bootlin.com, UNGLinuxDriver@microchip.com,
        kuba@kernel.org, clement.leger@bootlin.com, netdev@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2] net: ocelot: fix wrong time_after usage
Message-ID: <YojvUsJ090H/wfEk@lunn.ch>
References: <YoeMW+/KGk8VpbED@lunn.ch>
 <20220520213115.7832-1-paskripkin@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220520213115.7832-1-paskripkin@gmail.com>
Precedence: bulk

On Sat, May 21, 2022 at 12:31:15AM +0300, Pavel Skripkin wrote:
> Accidentally noticed, that this driver is the only user of
> while (time_after(jiffies...)).
> 
> It looks like typo, because likely this while loop will finish after 1st
> iteration, because time_after() returns true when 1st argument _is after_
> 2nd one.
> 
> There is one possible problem with this poll loop: the scheduler could put
> the thread to sleep, and it does not get woken up for
> OCELOT_FDMA_CH_SAFE_TIMEOUT_US. During that time, the hardware has done
> its thing, but you exit the while loop and return -ETIMEDOUT.
> 
> Fix it by using sane poll API that avoids all problems described above
> 
> Fixes: 753a026cfec1 ("net: ocelot: add FDMA support")
> Suggested-by: Andrew Lunn <andrew@lunn.ch>
> Signed-off-by: Pavel Skripkin <paskripkin@gmail.com>
> ---
> 
> I can't say if 0 is a good choise for 5th readx_poll_timeout() argument,
> so this patch is build-tested only.

> Testing and suggestions are welcomed!

If you had the hardware, i would suggest you profile how often it does
complete on the first iteration. And when it does not complete on the
first iteration, how many more iterations it needs.

Tobias made an interesting observation with the mv88e6xxx switch. He
found that two tight polls was enough 99% of the time. Putting a sleep
in there doubles the time it took to setup the switch. So he ended up
with a hybrid of open coded polling twice, followed by iopoll with a
timer value set.

That was with a heavily used poll function. How often is this function
used? No point in overly optimising this if it is not used much.

      Andrew