On Tue, Sep 09, 2025 at 01:58:22PM -0700, Jakub Kicinski wrote:
> On Tue, 9 Sep 2025 19:59:11 +0530 Anwar, Md Danish wrote:
> > > +int mucse_mbx_sync_fw(struct mucse_hw *hw)
> > > +{
> > > + int try_cnt = 3;
> > > + int err;
> > > +
> > > + do {
> > > + err = mucse_mbx_get_info(hw);
> > > + if (err == -ETIMEDOUT)
> > > + continue;
> > > + break;
> > > + } while (try_cnt--);
> > > +
> > > + return err;
> > > +}
> >
> > There's a logical issue in the code. The loop structure attempts to
> > retry on ETIMEDOUT errors, but the unconditional break statement after
> > the if-check will always exit the loop after the first attempt,
> > regardless of the error. The do-while loop will never actually retry
> > because the break statement is placed outside of the if condition that
> > checks for timeout errors.
>
What is expected is 'retry on ETIMEDOUT' and 'no retry others'.
https://lore.kernel.org/netdev/a066746c-2f12-4e70-b63a-7996392a9...@lunn.ch/
> The other way around. continue; in a do {} while () look does *not*
> evaluate the condition. So this can loop forever.
>
Maybe I can update like this ?
int mucse_mbx_sync_fw(struct mucse_hw *hw)
{
int try_cnt = 3;
int err;
do {
err = mucse_mbx_get_info(hw);
if (err != -ETIMEDOUT)
break;
/* only retry with ETIMEDOUT, others just return */
} while (try_cnt--);
return err;
}
Thanks for your feedback.
