[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID:
<DM6PR12MB4516062B5684DA1F4C5F49FED8D62@DM6PR12MB4516.namprd12.prod.outlook.com>
Date: Wed, 26 Jun 2024 17:26:06 +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 16:40
> 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
>
> > > > > +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?
>
Well actually it is 65535 msec which is ~65 sec and a bit over 1 minute.
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.
> 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.
Not sure I understood.
Do you want to have one more polling in the end of the loop? What could return ETIMEDOUT?
Thanks,
Danielle
>
> Andrew
Powered by blists - more mailing lists