[dpdk-dev] [PATCH v5 00/15] common ethdev linkstatus functions

[Ferruh Yigit]

On 1/17/2018 4:18 PM, Ferruh Yigit wrote:
> On 1/17/2018 4:05 PM, Stephen Hemminger wrote:
>> On Wed, 17 Jan 2018 14:32:17 +0000
>> Ferruh Yigit <ferruh.yigit at intel.com> wrote:
>>
>>> On 1/17/2018 7:56 AM, Andrew Rybchenko wrote:
>>>> On 01/16/2018 09:37 PM, Stephen Hemminger wrote:  
>>>>> While reviewing drivers, noticed a lot of unnecessary
>>>>> duplication of code in drivers for handling the eth_dev link status
>>>>> information. While consolidating this, it also became obvious that
>>>>> some drivers behave differently for no good reason.
>>>>>
>>>>> It also was a good chance to introduce atomic exchange primitives
>>>>> in EAL because there are other places using cmpset where not
>>>>> necessary (such as bonding).
>>>>>
>>>>> Mostly only compile tested only, don't have all of the hardware
>>>>> available (except ixgbe and virtio) to test.
>>>>>
>>>>> Note: the eth_dev_link_update function return value is inconsistent
>>>>> across drivers. Should be changed to be void.  
>>>>
>>>> I would say "link_update" callback return value is inconsistent across
>>>> drivers. I'm not sure which direction is right here: make it consistent
>>>> or make it void. Also any changes in link information could be
>>>> important. As I understand it should not happen without up/down,
>>>> but bugs with loss of intermediate transitions are definitely possible.
>>>> So, notifying about any changes in link information is definitely safer.
>>>> May be not now.  
>>>
>>> Again, why not return previous link status, it is simple enough to prevent
>>> inconsistent usage.
>>>
>>> rte_eth_link_get() already discards the return value, so won't be a problem there.
>>>
>>> For the cases PMD would like know about link changes, they will need to
>>> implement almost same link_update function with a return value, so why not use
>>> existing link_update function?
>>>
>>> Like been in virtio, link_update() used in interrupt handler, and calls a
>>> callback process if status changes. When link_update() return status changed to
>>> void, I guess they will need to implement another version of the link_update
>>> with return and use it.
>>
>> The interrupt and non-interrupt model are different.
> 
> Yes. But for virtio specific usage:
> 
> virtio_interrupt_handler()
>   virtio_dev_link_update() == 0
>     _rte_eth_dev_callback_process()
> 
> meantime same exact virtio_dev_link_update() used as:
>   .link_update             = virtio_dev_link_update,
> 
> so updating virtio_dev_link_update() to not return status change, will update
> logic in virtio_interrupt_handler(), no?

I would like to see this patch in, because it is useful and almost done. The
concern I mentioned above effects virtio.

Can virtio maintainers check if it is OK to get this as it is please?

> 
>> Also the driver internally may want to do something different, this is about
>> the return value for dev_ops->link_update.  
> 
> Agreed, driver may do something different. And the function needs to be
> implemented will be very close to dev_ops->link_update. I thought making
> dev_ops->link_update more generic can prevent duplication there. And aligns with
> virtio usage..
> 
>> The code in rte_eth_dev never
>> used the return value. The bonding driver was expecting it to work but it
>> doesn't.
> 
> Agreed.
> 
>> Anyway drivers shouldn't in general be directly calling other
>> devices eth_dev_ops
> 
> I guess now there are a few overlay PMDs does this.
>