On 1/8/20 5:27 PM, Thomas Monjalon wrote: > 08/01/2020 15:15, Andrew Rybchenko: >> On 1/8/20 4:52 PM, Ferruh Yigit wrote: >>> On 1/8/2020 1:25 PM, Thomas Monjalon wrote: >>>> 08/01/2020 14:20, Ferruh Yigit: >>>>> On 1/8/2020 1:06 PM, Thomas Monjalon wrote: >>>>>> 08/01/2020 13:59, Ferruh Yigit: >>>>>>> But for dev_ops, instead of having another capabilities indicator, which >>>>>>> requires PMDs to keep this synchronized, I think it is better if we can >>>>>>> self >>>>>>> contain this information within dev_ops, like not implementing dev_ops >>>>>>> would >>>>>>> mean it is not supported, this way it is easier to maintain and less >>>>>>> error prone. >>>>>> It means the dev_ops is resetted at init if a device does not support >>>>>> the feature. >>>>>> It is against having const dev_ops. >>>>> I didn't get your comment. >>>>> For example getting FW version, I am saying instead of keeping another >>>>> piece of >>>>> information to say if it is supported by device/driver, better to grasp >>>>> this >>>>> from if the driver implemented 'fw_version_get' dev_ops or not. >>>> I like this approach. >>>> Capabilities should be expressed by setting the function pointer or not >>>> (NULL). >>>> But a driver may support a feature for a subset of devices. >>> In that case dev_ops itself can return the '-ENOTSUP', since application >>> interaction will be through the ethdev API, either API send '-ENOTSUP' >>> because >>> the dev_ops is NULL or dev_ops itself send the '-ENOTSUP' because of the >>> underlying version of the device, for application it will be clear that that >>> feature is not supported. >> I think it is a good illustration why deriving the capability >> from dev_ops pointer is not that good idea. >> >>>> If a device does not support a feature, the function pointer must be set >>>> to NULL. >>>> The only issue is having dev_ops as a const struct. >>> Not sure about changing the dev_ops on runtime, it can be very hard to >>> debug. >> I hope it was just an idea to copy dev_ops and adjust in >> accordance with the device capabilities on register. >> I.e. not fully dynamic changes in runtime. > Changing a function pointer in runtime is tough :) > I was thinking about changing it during init but I don't really see a great > value.
Yes exactly, copying just solve the 'const' problem. > Probably better to return ENOTSUP. > > Anyway it does not address the capability info need. Yes, I agree. Back to other branch of the thread: dev_info flag vs dedicated dev_ops function.
