On 1/8/20 2:06 PM, Thomas Monjalon wrote:
08/01/2020 13:59, Ferruh Yigit:
On 1/8/2020 10:31 AM, Laurent Hardy wrote:
Hi all,
On 1/8/20 10:55 AM, David Marchand wrote:
On Wed, Jan 8, 2020 at 10:09 AM Ferruh Yigit <ferruh.yi...@intel.com> wrote:
On 1/8/2020 8:56 AM, David Marchand wrote:
Hello Laurent,
Bonne année.
Cc: maintainers.
On Tue, Jan 7, 2020 at 3:57 PM Laurent Hardy <laurent.ha...@6wind.com> wrote:
In current led control API we have no way to know if a device is able
to handle on/off requests coming from the application.
Knowing if the device is led control capable could be useful to avoid
exchanges between application and kernel.
Using the on/off requests to flag if the device is led control capable
(based on the ENOSUP returned error) is not convenient as such request
can change the led state on device.
This patch adds a new function rte_eth_led_ctrl_capable() that will look
for led_off/on dev ops availability on the related pmd, to know if the
device is able to handle such led control requests (on/off).
This patch breaks the ABI, which is BAD :-).
Why it is an ABI break, dev_ops should be between library and drivers, so it
should be out of the ABI concern, isn't it.
You are right.
So in our context, this is not an ABI breakage.
But abidiff still reports it, so maybe some filtering is required to
avoid this false positive.
Note that if we insert an ops before rx_queue_count, we would have a
real ABI breakage, as this ops is accessed via an inline wrapper by
applications.
This new api only needs to look at the existing ops, so you can remove
the (unused in your patch) dev_led_ctrl_capable ops.
OTOH, would it make sense to expose this capability in dev_flags?
'rte_eth_led_on()' & 'rte_eth_led_off()' APIs returns '-ENOTSUP' when the not
supported, can that help application to understand?
You might want to know it is supported without changing the state.
Laurent?
First, happy new year :)
Yes exactly, the purpose of this patch is to query if the device is led
control capable or not without changing the led state.
About exposing the capability through a dev_flags, means to make some
modification in each pmds. It looks more easy in term of pmds
maintenance to relying on the rte_eth_led_off()/on() dev ops
availability at rte_ethdev level, right ?
'dev_flag' definition is not clear, right now it holds the combination of status
and capability. And we have 'rte_eth_dev_info' struct, which is again
combination of device capability and status.
I agree capabilities in ethdev are a bit of a mess.
I would appreciate someone makes a complete audit of it
so we can discuss how to improve the situation.
Perhaps we should have explicit capabilities and status fields, even in the
rte_device level which inherited by net/crypto devices etc..
No, ethdev capabilities should stay in ethdev.
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.
Only we should have it without side effect,
1- adding an additional 'dry-run' parameter can work, but this means breaking
ABI and updating majority of the ethdev APIs :)
2- Adding 'is_supported' versions of the APIs as we need can be an option, like
'rte_eth_led_on_is_supported()'
3- Olivier's suggestion to add a new API to get the led status, so that this
information can be used select led API which won't cause side affect and let us
learn if it is supported.
Any other alternatives?
I would prefer the 2) in above ones, which is very similar to the original
patch.
I can provide a V2 which will remove the useless dev_led_ctrl_capable ops.
About the 'is_supported()' versions of APIs, in the current patch I
factorize
the check on dev ops on and off availability in a same function named
"led_ctrl_capable" but I can rename it if required.
Just in this specific case I don't dissociate on and off capability, as
being
able to set the led off without a way to set it on again sounds a bit
unusual :)
The other alternatives are in rte_eth_dev_info and dev_flags.
