> From: Andrew Lunn <andrew@xxxxxxx>
> Sent: Wednesday, 26 June 2024 20:43
> To: Danielle Ratson <danieller@xxxxxxxxxx>
> Cc: netdev@xxxxxxxxxxxxxxx; davem@xxxxxxxxxxxxx; edumazet@xxxxxxxxxx;
> kuba@xxxxxxxxxx; pabeni@xxxxxxxxxx; corbet@xxxxxxx;
> linux@xxxxxxxxxxxxxxx; sdf@xxxxxxxxxx; kory.maincent@xxxxxxxxxxx;
> maxime.chevallier@xxxxxxxxxxx; vladimir.oltean@xxxxxxx;
> przemyslaw.kitszel@xxxxxxxxx; ahmed.zaki@xxxxxxxxx;
> richardcochran@xxxxxxxxx; shayagr@xxxxxxxxxx;
> paul.greenwalt@xxxxxxxxx; jiri@xxxxxxxxxxx; linux-doc@xxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx; mlxsw <mlxsw@xxxxxxxxxx>; Ido Schimmel
> <idosch@xxxxxxxxxx>; Petr Machata <petrm@xxxxxxxxxx>
> Subject: Re: [PATCH net-next v7 7/9] ethtool: cmis_cdb: Add a layer for
> supporting CDB commands
>
> > > Please could you test it.
> > >
> > > 65535 jiffies is i think 655 seconds? That is probably too long to
> > > loop when the module has been ejected. Maybe replace it with HZ?
> > >
> >
> > Well actually it is 65535 msec which is ~65 sec and a bit over 1 minute.
>
> I _think_ it depends on CONFIG_HZ, which can be 100, 250, 300 and 1000.
>
> > The test you are asking for is a bit complicated since I don’t have a
> > machine physically nearby, do you find it very much important?
>
> > I mean, it is not very reasonable thing to do, burning fw on a module
> > and in the exact same time eject it.
>
> Shooting yourself in the foot is not a very reasonable thing to do, but the Unix
> philosophy is to all root to do it. Do we really want 60 to 600 seconds of the
> kernel spamming the log when somebody does do this?
Ok i checked it and using netdev_err_once() fulfill that issue. Thanks!
>
> > > Maybe netdev_err() should become netdev_dbg()? And please add a 20ms
> > > delay before the continue.
> > >
> > > > > > > + }
> > > > > > > +
> > > > > > > + if ((*cond_success)(rpl.state))
> > > > > > > + return 0;
> > > > > > > +
> > > > > > > + if (*cond_fail && (*cond_fail)(rpl.state))
> > > > > > > + break;
> > > > > > > +
> > > > > > > + msleep(20);
> > > > > > > + } while (time_before(jiffies, end));
> > > > > >
>
> > > O.K. Please evaluate the condition again after the while() just so
> > > ETIMEDOUT is not returned in error.
> >
> > Not sure I understood.
> > Do you want to have one more polling in the end of the loop? What could
> return ETIMEDOUT?
>
> Consider what happens when msleep(20) actually sleeps a lot longer.
>
> Look at the core code which gets this correct:
>
> #define read_poll_timeout(op, val, cond, sleep_us, timeout_us, \
> sleep_before_read, args...) \ ({ \
> u64 __timeout_us = (timeout_us); \
> unsigned long __sleep_us = (sleep_us); \
> ktime_t __timeout = ktime_add_us(ktime_get(), __timeout_us); \
> might_sleep_if((__sleep_us) != 0); \
> if (sleep_before_read && __sleep_us) \
> usleep_range((__sleep_us >> 2) + 1, __sleep_us); \
> for (;;) { \
> (val) = op(args); \
> if (cond) \
> break; \
> if (__timeout_us && \
> ktime_compare(ktime_get(), __timeout) > 0) { \
> (val) = op(args); \
> break; \
> } \
> if (__sleep_us) \
> usleep_range((__sleep_us >> 2) + 1, __sleep_us); \
> cpu_relax(); \
> } \
> (cond) ? 0 : -ETIMEDOUT; \
> })
>
> So after breaking out of the for loop with a timeout, it evaluates the condition
> one more time, and uses that to decide on 0 or ETIMEDOUT. So it does not
> matter if usleep_range() range slept for 60 seconds, not 60ms, the exit code
> will be correct.
>
> Andrew
Ok ill fix it, thanks.
