[dpdk-dev] [PATCH v2 0/5] example/ethtool: add bus info and fw version get

[Zhang, Helin]

> -----Original Message-----
> From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Thomas Monjalon
> Sent: Thursday, December 22, 2016 11:31 PM
> To: Yigit, Ferruh
> Cc: Yang, Qiming; dev at dpdk.org; Horton, Remy
> Subject: Re: [dpdk-dev] [PATCH v2 0/5] example/ethtool: add bus info and fw
> version get
> 
> 2016-12-22 15:05, Ferruh Yigit:
> > On 12/22/2016 2:47 PM, Thomas Monjalon wrote:
> > > 2016-12-22 14:36, Ferruh Yigit:
> > >> On 12/22/2016 11:07 AM, Thomas Monjalon wrote:
> > >>> I think it is OK to add a new dev_ops and a new API function for
> > >>> firmware query. Generally speaking, it is a good thing to avoid
> > >>> putting all informations in the same structure (e.g. rte_eth_dev_info).
> > >>
> > >> OK.
> > >>
> > >>> However, there
> > >>> is a balance to find. Could we plan to add more info to this new query?
> > >>> Instead of
> > >>> 	rte_eth_dev_fwver_get(uint8_t port_id, char *fw_version, int
> > >>> fw_length)
> > > [...]
> > >>> could it fill a struct?
> > >>> 	rte_eth_dev_fw_info_get(uint8_t port_id, struct
> > >>> rte_eth_dev_fw_info *fw_info)
> > >>
> > >> I believe this is better. But the problem we are having with this
> > >> usage
> > >> is: ABI breakage.
> > >>
> > >> Since this struct will be a public structure, in the future if we
> > >> want to add a new field to the struct, it will break the ABI, and
> > >> just this change will cause a new version for whole ethdev library!
> > >>
> > >> When all required fields received via arguments, one by one,
> > >> instead of struct, at least ABI versioning can be done on the API
> > >> when new field added, and can be possible to escape from ABI
> > >> breakage. But this will be ugly when number of arguments increased.
> > >>
> > >> Or any other opinion on how to define API to reduce ABI breakage?
> > >
> > > You're right.
> > > But I don't think we should have a function per data. Just because
> > > it would be ugly :)
> >
> > I am no suggesting function per data, instead something like:
> >
> > rte_eth_dev_fw_info_get(uint8_t port_id, uint32_t maj, uint32_t min);
> >
> > And in the future if we need etrack_id too, we can have both in
> > versioned manner:
> >
> > rte_eth_dev_fw_info_get(uint8_t port_id, uint32_t maj, uint32_t min);
> >
> > rte_eth_dev_fw_info_get(uint8_t port_id, uint32_t maj, uint32_t min,
> > 	uint32_t etrack_id);
> 
> Oh I see. So it can be versioned with compat macros.
> 
> > So my concern was if the number of the arguments becomes too many by
> time.
> 
> It looks to be a good proposal. We should not have a dozen of arguments.

I'd suggest to do that the similar way of kernel driver/ethtool (Linux or FreeBSD) does,
which should be well discussed.
In addition, for future extention, and avoid breaking any ABI in a strcuture,
we can just pre-define a lot of bytes as reserved, e.g. 64 bytes. Inside DPDK,
there are several strucutres defined like this, e.g. mbuf.

Thanks,
Helin