Hi Andrew,
Thanks for reviewing the patches.
> From: Andrew Lunn <andrew@xxxxxxx>
> Sent: Monday, 24 June 2024 22:51
> 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
>
> > +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.
But it is less likely than the scenario I wanted to cover.
According to SPEC 5.2:
"
7.2.5.1 Foreground Mode CDB Messaging
[...]
In foreground mode the module rejects any register ACCESS until a currently executing CDB command execution has completed.
Note: READs of the CdbStatus registers 00h:37 or 00h:38 (see Table 8-13) will also be rejected by the module.
"
So in that case the module won't be able to respond and we need to wait for it to be responsive and the status to be valid.
>
> > + }
> > +
> > + 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.
>
> > +static u8 cmis_cdb_calc_checksum(const void *data, size_t size) {
> > + const u8 *bytes = (const u8 *)data;
> > + u8 checksum = 0;
> > +
> > + for (size_t i = 0; i < size; i++)
> > + checksum += bytes[i];
> > +
> > + return ~checksum;
> > +}
>
> I expect there is already a helper do that somewhere.
>
> Andrew
Yes it does, but actually it is an helper that occurs in specific places (for example pci_vpd_check_csum()), that i can use from here.
>
> ---
> pw-bot: cr
