Re: [dpdk-dev] [PATCH] librte_ethdev: extend dpdk api led control to query capability

[Ferruh Yigit <ferruh.yigit@intel.com>]

On 1/8/2020 12:27 PM, Olivier Matz wrote:
> On Wed, Jan 08, 2020 at 12:12:11PM +0000, Ferruh Yigit wrote:
>> On 1/8/2020 9:42 AM, Olivier Matz wrote:
>>> On Wed, Jan 08, 2020 at 09:09:29AM +0000, Ferruh Yigit 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.hardy@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.
>>>>
>>>>> 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?
>>>
>>> In our case, it is not possible to use rte_eth_led_on/off() to check if
>>> the feature is supported: on success, it would change the value of the
>>> led, and it seems it is not recoverable on some drivers.
>>
>> What does it mean it is not recoverable, like can you turn on the led but can't
>> turn off it back? Can't this be fixed in the PMD?
> 
> In the case there is only one LED, which is by default used to display
> the link status, it can never display the status again without resetting
> the device.

Is there a specific PMD are we talking about?

> 
> Maybe an alternative solution would be to add a function, in addition to
> on() and off():
> 
>   led_ctrl_status_link() to display the status link on the led.
> 
> 
>>
>>>
>>> Today it is not possible to know if the feature is available without
>>> side effect.
>>>
>>