From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ferruh Yigit Subject: Re: [PATCH v5 00/15] common ethdev linkstatus functions Date: Wed, 17 Jan 2018 16:18:07 +0000 Message-ID: <24375588-ec05-9e54-264d-69dcb48f7f16@intel.com> References: <20180116183755.24542-1-stephen@networkplumber.org> <42f4f5a2-008d-c3e8-4c00-ed9ef59065c8@solarflare.com> <27f327e5-5632-69cc-feaa-10cf9384a701@intel.com> <20180117080555.5afd8c78@xeon-e3> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Cc: Andrew Rybchenko , dev@dpdk.org, Yuanhan Liu To: Stephen Hemminger Return-path: Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by dpdk.org (Postfix) with ESMTP id EF70E199B0 for ; Wed, 17 Jan 2018 17:18:10 +0100 (CET) In-Reply-To: <20180117080555.5afd8c78@xeon-e3> Content-Language: en-US List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On 1/17/2018 4:05 PM, Stephen Hemminger wrote: > On Wed, 17 Jan 2018 14:32:17 +0000 > Ferruh Yigit 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? > 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.