[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID:
<DM6PR12MB45160145A33D8B802F5AE961D8D72@DM6PR12MB4516.namprd12.prod.outlook.com>
Date: Thu, 27 Jun 2024 13:12:48 +0000
From: Danielle Ratson <danieller@...dia.com>
To: Andrew Lunn <andrew@...n.ch>
CC: "netdev@...r.kernel.org" <netdev@...r.kernel.org>, "davem@...emloft.net"
<davem@...emloft.net>, "edumazet@...gle.com" <edumazet@...gle.com>,
"kuba@...nel.org" <kuba@...nel.org>, "pabeni@...hat.com" <pabeni@...hat.com>,
"corbet@....net" <corbet@....net>, "linux@...linux.org.uk"
<linux@...linux.org.uk>, "sdf@...gle.com" <sdf@...gle.com>,
"kory.maincent@...tlin.com" <kory.maincent@...tlin.com>,
"maxime.chevallier@...tlin.com" <maxime.chevallier@...tlin.com>,
"vladimir.oltean@....com" <vladimir.oltean@....com>,
"przemyslaw.kitszel@...el.com" <przemyslaw.kitszel@...el.com>,
"ahmed.zaki@...el.com" <ahmed.zaki@...el.com>, "richardcochran@...il.com"
<richardcochran@...il.com>, "shayagr@...zon.com" <shayagr@...zon.com>,
"paul.greenwalt@...el.com" <paul.greenwalt@...el.com>, "jiri@...nulli.us"
<jiri@...nulli.us>, "linux-doc@...r.kernel.org" <linux-doc@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>, mlxsw
<mlxsw@...dia.com>, Ido Schimmel <idosch@...dia.com>, Petr Machata
<petrm@...dia.com>
Subject: RE: [PATCH net-next v7 7/9] ethtool: cmis_cdb: Add a layer for
supporting CDB commands
> From: Andrew Lunn <andrew@...n.ch>
> Sent: Wednesday, 26 June 2024 20:43
> To: Danielle Ratson <danieller@...dia.com>
> Cc: netdev@...r.kernel.org; davem@...emloft.net; edumazet@...gle.com;
> kuba@...nel.org; pabeni@...hat.com; corbet@....net;
> linux@...linux.org.uk; sdf@...gle.com; kory.maincent@...tlin.com;
> maxime.chevallier@...tlin.com; vladimir.oltean@....com;
> przemyslaw.kitszel@...el.com; ahmed.zaki@...el.com;
> richardcochran@...il.com; shayagr@...zon.com;
> paul.greenwalt@...el.com; jiri@...nulli.us; linux-doc@...r.kernel.org; linux-
> kernel@...r.kernel.org; mlxsw <mlxsw@...dia.com>; Ido Schimmel
> <idosch@...dia.com>; Petr Machata <petrm@...dia.com>
> 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.
Powered by blists - more mailing lists