[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <baf84bde-79d3-4570-a1df-e6adbe14c823@lunn.ch>
Date: Wed, 26 Jun 2024 15:40:08 +0200
From: Andrew Lunn <andrew@...n.ch>
To: Danielle Ratson <danieller@...dia.com>
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
> > > > +int ethtool_cmis_wait_for_cond(struct net_device *dev, u8 flags, u8 flag,
> > > > + u16 max_duration, u32 offset,
> > > > + bool (*cond_success)(u8), bool (*cond_fail)(u8),
> > > > + u8 *state)
> > > > +{
> > > > + const struct ethtool_ops *ops = dev->ethtool_ops;
> > > > + struct ethtool_module_eeprom page_data = {0};
> > > > + struct cmis_wait_for_cond_rpl rpl = {};
> > > > + struct netlink_ext_ack extack = {};
> > > > + unsigned long end;
> > > > + int err;
> > > > +
> > > > + if (!(flags & flag))
> > > > + return 0;
> > > > +
> > > > + if (max_duration == 0)
> > > > + max_duration = U16_MAX;
> > > > +
> > > > + end = jiffies + msecs_to_jiffies(max_duration);
> > > > + do {
> > > > + ethtool_cmis_page_init(&page_data, 0, offset, sizeof(rpl));
> > > > + page_data.data = (u8 *)&rpl;
> > > > +
> > > > + err = ops->get_module_eeprom_by_page(dev, &page_data,
> > > &extack);
> > > > + if (err < 0) {
> > > > + if (extack._msg)
> > > > + netdev_err(dev, "%s\n", extack._msg);
> > > > + continue;
> > >
> > > continue here is interested. Say you get -EIO because the module has
> > > been ejected. I would say that is fatal. Won't this spam the logs, as
> > > fast as the I2C bus can fail, without the 20ms sleep, for 65535 jiffies?
> >
> > If the module is ejected from some reason, it might span the logs I guess.
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?
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));
> > >
> > > Please could you implement this using iopoll.h. This appears to have
> > > the usual problem. Say msleep(20) actually sleeps a lot longer,
> > > because the system is busy doing other things. time_before(jiffies,
> > > end)) is false, because of the long delay, but in fact the operation
> > > has completed without error. Yet you return EBUSY. iopoll.h gets this
> > > correct, it does one more evaluation of the condition after exiting
> > > the loop to handle this issue.
> >
> > OK.
>
> Hi Andrew,
>
> Therefore, unfortunately in this case I'd rather stay with the origin code.
O.K. Please evaluate the condition again after the while() just so
ETIMEDOUT is not returned in error.
Andrew
Powered by blists - more mailing lists