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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

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.