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

[Laurent Hardy <laurent.hardy@6wind.com>]

On 1/8/20 3:08 PM, Ferruh Yigit wrote:
> 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?

e1000 pmd which seems to have different behavior according to the

nics used (at least it has been reported using i210 and i350 copper).

>
>> 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.
>>>>