Re: [PATCH v4 1/5] ethdev: add access to eeprom

[Thomas Monjalon <thomas@monjalon.net>]

25/04/2018 10:21, Zijie Pan:
> Hi Thomas,
> 
> > > +/**
> > > + * Placeholder for accessing plugin module eeprom
> > > + */
> > > +struct rte_dev_module_info {
> > > + uint32_t type; /**< Type of plugin module eeprom */
> > > + uint32_t eeprom_len; /**< Length of plugin module eeprom */
> > > +};
> >
> > I am not sure "plugin module" is descriptive enough.
> 
> Here is the description when "man ethtool":
> -m --dump-module-eeprom --module-info
>     Retrieves  and if possible decodes the EEPROM from plugin modules, e.g SFP+, QSFP.
> Is there any suggestion how to describe it?

No better idea.

> > And I think the structure name should be rte_eth_dev_module_info
> > to make clear that we are talking about NIC modules.
> > Any better idea?
> 
> Yes, rte_eth_dev_module_info is better than rte_dev_module_info.
> But the name of other structures should be also changed. And the file name
> should be changed to rte_eth_dev_info.h.
> Do we need to make a patch first to change the name of the file and structures?

No you can just focus on your change.
If more clean-up is required in your opinion, you can do it later.


> > > +int __rte_experimental
> > > +rte_eth_dev_get_module_eeprom(uint16_t port_id,
> > > +       struct rte_dev_eeprom_info *info)
> > > +{
> > > + struct rte_eth_dev *dev;
> > > +
> > > + RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
> > > +
> > > + dev = &rte_eth_devices[port_id];
> > > + RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->get_module_eeprom, -ENOTSUP);
> > > + return (*dev->dev_ops->get_module_eeprom)(dev, info);
> > > +}
> > > +
> > >  int
> > >  rte_eth_dev_get_dcb_info(uint16_t port_id,
> >
> > Please move this code after other EEPROM related functions.
> I put the functions after rte_eth_dev_set_eeprom(). Please apply the patch and check it.

OK

> > > --- a/lib/librte_ether/rte_ethdev_version.map
> > > +++ b/lib/librte_ether/rte_ethdev_version.map
> > > @@ -229,5 +229,7 @@ EXPERIMENTAL {
> > >  rte_mtr_policer_actions_update;
> > >  rte_mtr_stats_read;
> > >  rte_mtr_stats_update;
> > > + rte_eth_dev_get_module_info;
> > > + rte_eth_dev_get_module_eeprom;
> >
> > This must be inserted in alphabetical order.
> I will update it in v5 patchset.

Please send v5 ASAP, thanks.