RE: [PATCH net-next v7 7/9] ethtool: cmis_cdb: Add a layer for supporting CDB commands

[Danielle Ratson]

> 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.