From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 078A3A0A0C; Fri, 2 Jul 2021 18:24:25 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 8782B41363; Fri, 2 Jul 2021 18:24:25 +0200 (CEST) Received: from shelob.oktetlabs.ru (shelob.oktetlabs.ru [91.220.146.113]) by mails.dpdk.org (Postfix) with ESMTP id BD2A341353 for ; Fri, 2 Jul 2021 18:24:23 +0200 (CEST) Received: from [192.168.38.17] (aros.oktetlabs.ru [192.168.38.17]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by shelob.oktetlabs.ru (Postfix) with ESMTPSA id 440DF7F504; Fri, 2 Jul 2021 19:24:23 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 shelob.oktetlabs.ru 440DF7F504 DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=oktetlabs.ru; s=default; t=1625243063; bh=98I3g3lBqZ9yYXxneLxmBbctjBixhzOiKRkYzrlL0Yc=; h=Subject:To:References:From:Date:In-Reply-To; b=VEPWS0vJaJuA+4HxwsnXq9pqw1DaoaWyWX46DA0l7myYzHdjemy3jWgFTl+46eEOA q22rt4bG+8p7bOJA7ZmC3tkplAo3xOA9EDZWxAVVYzDwUjDGT5AB+3SQ2kXryj5UQ0 vi42/jTPhadBdAHyL/uMA4AJqLYPIhBbeoUFq/o0= To: Jiawen Wu , dev@dpdk.org References: <20210617110005.4132926-1-jiawenwu@trustnetic.com> <20210617110005.4132926-11-jiawenwu@trustnetic.com> From: Andrew Rybchenko Organization: OKTET Labs Message-ID: <52460389-4422-31c8-76fa-0a8790aa033d@oktetlabs.ru> Date: Fri, 2 Jul 2021 19:24:23 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.11.0 MIME-Version: 1.0 In-Reply-To: <20210617110005.4132926-11-jiawenwu@trustnetic.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH v6 10/19] net/ngbe: support link update X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list 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 6/17/21 1:59 PM, Jiawen Wu wrote: > Register to handle device interrupt. > > Signed-off-by: Jiawen Wu [snip] > diff --git a/doc/guides/nics/ngbe.rst b/doc/guides/nics/ngbe.rst > index 54d0665db9..0918cc2918 100644 > --- a/doc/guides/nics/ngbe.rst > +++ b/doc/guides/nics/ngbe.rst > @@ -8,6 +8,11 @@ The NGBE PMD (librte_pmd_ngbe) provides poll mode driver support > for Wangxun 1 Gigabit Ethernet NICs. > > > +Features > +-------- > + > +- Link state information > + Two empty lines before the section. > Prerequisites > ------------- > [snip] > diff --git a/drivers/net/ngbe/ngbe_ethdev.c b/drivers/net/ngbe/ngbe_ethdev.c > index deca64137d..c952023e8b 100644 > --- a/drivers/net/ngbe/ngbe_ethdev.c > +++ b/drivers/net/ngbe/ngbe_ethdev.c > @@ -141,6 +175,23 @@ eth_ngbe_dev_init(struct rte_eth_dev *eth_dev, void *init_params __rte_unused) > return -ENOMEM; > } > > + ctrl_ext = rd32(hw, NGBE_PORTCTL); > + /* let hardware know driver is loaded */ > + ctrl_ext |= NGBE_PORTCTL_DRVLOAD; > + /* Set PF Reset Done bit so PF/VF Mail Ops can work */ > + ctrl_ext |= NGBE_PORTCTL_RSTDONE; > + wr32(hw, NGBE_PORTCTL, ctrl_ext); > + ngbe_flush(hw); > + > + rte_intr_callback_register(intr_handle, > + ngbe_dev_interrupt_handler, eth_dev); > + > + /* enable uio/vfio intr/eventfd mapping */ > + rte_intr_enable(intr_handle); > + > + /* enable support intr */ > + ngbe_enable_intr(eth_dev); > + I don't understand why it is done unconditionally regardless of the corresponding bit in dev_conf. > return 0; > } > > @@ -180,11 +231,25 @@ static int eth_ngbe_pci_remove(struct rte_pci_device *pci_dev) > > static struct rte_pci_driver rte_ngbe_pmd = { > .id_table = pci_id_ngbe_map, > - .drv_flags = RTE_PCI_DRV_NEED_MAPPING, > + .drv_flags = RTE_PCI_DRV_NEED_MAPPING | > + RTE_PCI_DRV_INTR_LSC, > .probe = eth_ngbe_pci_probe, > .remove = eth_ngbe_pci_remove, > }; > > +static int > +ngbe_dev_configure(struct rte_eth_dev *dev) > +{ > + struct ngbe_interrupt *intr = ngbe_dev_intr(dev); > + > + PMD_INIT_FUNC_TRACE(); > + > + /* set flag to update link status after init */ > + intr->flags |= NGBE_FLAG_NEED_LINK_UPDATE; > + > + return 0; > +} > + > /* > * Reset and stop device. > */ > @@ -198,6 +263,315 @@ ngbe_dev_close(struct rte_eth_dev *dev) > return -EINVAL; > } > > +static int > +ngbe_dev_info_get(struct rte_eth_dev *dev, struct rte_eth_dev_info *dev_info) > +{ > + RTE_SET_USED(dev); > + > + dev_info->speed_capa = ETH_LINK_SPEED_1G | ETH_LINK_SPEED_100M | > + ETH_LINK_SPEED_10M; > + > + return 0; > +} > + > +/* return 0 means link status changed, -1 means not changed */ > +int > +ngbe_dev_link_update_share(struct rte_eth_dev *dev, > + int wait_to_complete) > +{ > + struct ngbe_hw *hw = ngbe_dev_hw(dev); > + struct rte_eth_link link; > + u32 link_speed = NGBE_LINK_SPEED_UNKNOWN; > + u32 lan_speed = 0; > + struct ngbe_interrupt *intr = ngbe_dev_intr(dev); > + bool link_up; > + int err; > + int wait = 1; > + > + memset(&link, 0, sizeof(link)); > + link.link_status = ETH_LINK_DOWN; > + link.link_speed = ETH_SPEED_NUM_NONE; > + link.link_duplex = ETH_LINK_HALF_DUPLEX; > + link.link_autoneg = !(dev->data->dev_conf.link_speeds & > + ~ETH_LINK_SPEED_AUTONEG); > + > + hw->mac.get_link_status = true; > + > + if (intr->flags & NGBE_FLAG_NEED_LINK_CONFIG) > + return rte_eth_linkstatus_set(dev, &link); > + > + /* check if it needs to wait to complete, if lsc interrupt is enabled */ > + if (wait_to_complete == 0 || dev->data->dev_conf.intr_conf.lsc != 0) > + wait = 0; > + > + err = hw->mac.check_link(hw, &link_speed, &link_up, wait); > + Please, remove empty line which adds unnecessary logical separation of the operation and its result check. > + if (err != 0) { > + link.link_speed = ETH_SPEED_NUM_100M; > + link.link_duplex = ETH_LINK_FULL_DUPLEX; > + return rte_eth_linkstatus_set(dev, &link); > + } > + > + if (!link_up) > + return rte_eth_linkstatus_set(dev, &link); > + > + intr->flags &= ~NGBE_FLAG_NEED_LINK_CONFIG; > + link.link_status = ETH_LINK_UP; > + link.link_duplex = ETH_LINK_FULL_DUPLEX; > + > + switch (link_speed) { > + default: > + case NGBE_LINK_SPEED_UNKNOWN: > + link.link_duplex = ETH_LINK_FULL_DUPLEX; > + link.link_speed = ETH_SPEED_NUM_100M; May be ETH_SPEED_NUM_NONE? > + break; > + > + case NGBE_LINK_SPEED_10M_FULL: > + link.link_speed = ETH_SPEED_NUM_10M; > + lan_speed = 0; > + break; > + > + case NGBE_LINK_SPEED_100M_FULL: > + link.link_speed = ETH_SPEED_NUM_100M; > + lan_speed = 1; > + break; > + > + case NGBE_LINK_SPEED_1GB_FULL: > + link.link_speed = ETH_SPEED_NUM_1G; > + lan_speed = 2; > + break; > + } > + > + if (hw->is_pf) { > + wr32m(hw, NGBE_LAN_SPEED, NGBE_LAN_SPEED_MASK, lan_speed); > + if (link_speed & (NGBE_LINK_SPEED_1GB_FULL | > + NGBE_LINK_SPEED_100M_FULL | NGBE_LINK_SPEED_10M_FULL)) { Such indent is very confusing since it is the same as on the next line. Please, add extra TAB to indent above line further. > + wr32m(hw, NGBE_MACTXCFG, NGBE_MACTXCFG_SPEED_MASK, > + NGBE_MACTXCFG_SPEED_1G | NGBE_MACTXCFG_TE); > + } > + } > + > + return rte_eth_linkstatus_set(dev, &link); > +} > + > +static int > +ngbe_dev_link_update(struct rte_eth_dev *dev, int wait_to_complete) > +{ > + return ngbe_dev_link_update_share(dev, wait_to_complete); > +} > + > +/* > + * It reads ICR and sets flag for the link_update. > + * > + * @param dev > + * Pointer to struct rte_eth_dev. > + * > + * @return > + * - On success, zero. > + * - On failure, a negative value. > + */ > +static int > +ngbe_dev_interrupt_get_status(struct rte_eth_dev *dev) > +{ > + uint32_t eicr; > + struct ngbe_hw *hw = ngbe_dev_hw(dev); > + struct ngbe_interrupt *intr = ngbe_dev_intr(dev); > + > + /* clear all cause mask */ > + ngbe_disable_intr(hw); > + > + /* read-on-clear nic registers here */ > + eicr = ((u32 *)hw->isb_mem)[NGBE_ISB_MISC]; > + PMD_DRV_LOG(DEBUG, "eicr %x", eicr); > + > + intr->flags = 0; > + > + /* set flag for async link update */ > + if (eicr & NGBE_ICRMISC_PHY) > + intr->flags |= NGBE_FLAG_NEED_LINK_UPDATE; > + > + if (eicr & NGBE_ICRMISC_VFMBX) > + intr->flags |= NGBE_FLAG_MAILBOX; > + > + if (eicr & NGBE_ICRMISC_LNKSEC) > + intr->flags |= NGBE_FLAG_MACSEC; > + > + if (eicr & NGBE_ICRMISC_GPIO) > + intr->flags |= NGBE_FLAG_NEED_LINK_UPDATE; > + > + return 0; > +} > + > +/** > + * It gets and then prints the link status. > + * > + * @param dev > + * Pointer to struct rte_eth_dev. > + * > + * @return > + * - On success, zero. > + * - On failure, a negative value. > + */ > +static void > +ngbe_dev_link_status_print(struct rte_eth_dev *dev) > +{ > + struct rte_pci_device *pci_dev = RTE_ETH_DEV_TO_PCI(dev); > + struct rte_eth_link link; > + > + rte_eth_linkstatus_get(dev, &link); > + > + if (link.link_status == ETH_LINK_UP) { > + PMD_INIT_LOG(INFO, "Port %d: Link Up - speed %u Mbps - %s", > + (int)(dev->data->port_id), > + (unsigned int)link.link_speed, > + link.link_duplex == ETH_LINK_FULL_DUPLEX ? > + "full-duplex" : "half-duplex"); > + } else { > + PMD_INIT_LOG(INFO, " Port %d: Link Down", > + (int)(dev->data->port_id)); > + } > + PMD_INIT_LOG(DEBUG, "PCI Address: " PCI_PRI_FMT, > + pci_dev->addr.domain, > + pci_dev->addr.bus, > + pci_dev->addr.devid, > + pci_dev->addr.function); > +} > + > +/* > + * It executes link_update after knowing an interrupt occurred. > + * > + * @param dev > + * Pointer to struct rte_eth_dev. > + * > + * @return > + * - On success, zero. > + * - On failure, a negative value. > + */ > +static int > +ngbe_dev_interrupt_action(struct rte_eth_dev *dev) > +{ > + struct ngbe_interrupt *intr = ngbe_dev_intr(dev); > + int64_t timeout; > + > + PMD_DRV_LOG(DEBUG, "intr action type %d", intr->flags); > + > + if (intr->flags & NGBE_FLAG_NEED_LINK_UPDATE) { > + struct rte_eth_link link; > + > + /*get the link status before link update, for predicting later*/ > + rte_eth_linkstatus_get(dev, &link); > + > + ngbe_dev_link_update(dev, 0); > + > + /* likely to up */ > + if (link.link_status != ETH_LINK_UP) > + /* handle it 1 sec later, wait it being stable */ > + timeout = NGBE_LINK_UP_CHECK_TIMEOUT; > + /* likely to down */ > + else > + /* handle it 4 sec later, wait it being stable */ > + timeout = NGBE_LINK_DOWN_CHECK_TIMEOUT; > + > + ngbe_dev_link_status_print(dev); > + if (rte_eal_alarm_set(timeout * 1000, > + ngbe_dev_interrupt_delayed_handler, > + (void *)dev) < 0) { > + PMD_DRV_LOG(ERR, "Error setting alarm"); > + } else { > + /* remember original mask */ > + intr->mask_misc_orig = intr->mask_misc; > + /* only disable lsc interrupt */ > + intr->mask_misc &= ~NGBE_ICRMISC_PHY; > + > + intr->mask_orig = intr->mask; > + /* only disable all misc interrupts */ > + intr->mask &= ~(1ULL << NGBE_MISC_VEC_ID); > + } > + } > + > + PMD_DRV_LOG(DEBUG, "enable intr immediately"); > + ngbe_enable_intr(dev); > + > + return 0; > +} > + > +/** > + * Interrupt handler which shall be registered for alarm callback for delayed > + * handling specific interrupt to wait for the stable nic state. As the > + * NIC interrupt state is not stable for ngbe after link is just down, > + * it needs to wait 4 seconds to get the stable status. > + * > + * @param handle > + * Pointer to interrupt handle. > + * @param param > + * The address of parameter (struct rte_eth_dev *) registered before. > + * > + * @return > + * void Such documentation of the void functions is useless. It may be simply omited. > + */ > +static void > +ngbe_dev_interrupt_delayed_handler(void *param) > +{ > + struct rte_eth_dev *dev = (struct rte_eth_dev *)param; > + struct ngbe_interrupt *intr = ngbe_dev_intr(dev); > + struct ngbe_hw *hw = ngbe_dev_hw(dev); > + uint32_t eicr; > + > + ngbe_disable_intr(hw); > + > + eicr = ((u32 *)hw->isb_mem)[NGBE_ISB_MISC]; > + > + if (intr->flags & NGBE_FLAG_NEED_LINK_UPDATE) { > + ngbe_dev_link_update(dev, 0); > + intr->flags &= ~NGBE_FLAG_NEED_LINK_UPDATE; > + ngbe_dev_link_status_print(dev); > + rte_eth_dev_callback_process(dev, RTE_ETH_EVENT_INTR_LSC, > + NULL); > + } > + > + if (intr->flags & NGBE_FLAG_MACSEC) { > + rte_eth_dev_callback_process(dev, RTE_ETH_EVENT_MACSEC, > + NULL); > + intr->flags &= ~NGBE_FLAG_MACSEC; > + } > + > + /* restore original mask */ > + intr->mask_misc = intr->mask_misc_orig; > + intr->mask_misc_orig = 0; > + intr->mask = intr->mask_orig; > + intr->mask_orig = 0; > + > + PMD_DRV_LOG(DEBUG, "enable intr in delayed handler S[%08x]", eicr); > + ngbe_enable_intr(dev); > +} > + > +/** > + * Interrupt handler triggered by NIC for handling > + * specific interrupt. > + * > + * @param handle > + * Pointer to interrupt handle. > + * @param param > + * The address of parameter (struct rte_eth_dev *) registered before. > + * > + * @return > + * void Such documentation of the void functions is useless. It may be simply omited. > + */ > +static void > +ngbe_dev_interrupt_handler(void *param) > +{ > + struct rte_eth_dev *dev = (struct rte_eth_dev *)param; > + > + ngbe_dev_interrupt_get_status(dev); > + ngbe_dev_interrupt_action(dev); > +} > + > +static const struct eth_dev_ops ngbe_eth_dev_ops = { > + .dev_configure = ngbe_dev_configure, > + .dev_infos_get = ngbe_dev_info_get, > + .link_update = ngbe_dev_link_update, > +}; > + > RTE_PMD_REGISTER_PCI(net_ngbe, rte_ngbe_pmd); > RTE_PMD_REGISTER_PCI_TABLE(net_ngbe, pci_id_ngbe_map); > RTE_PMD_REGISTER_KMOD_DEP(net_ngbe, "* igb_uio | uio_pci_generic | vfio-pci"); > diff --git a/drivers/net/ngbe/ngbe_ethdev.h b/drivers/net/ngbe/ngbe_ethdev.h > index 87cc1cff6b..b67508a3de 100644 > --- a/drivers/net/ngbe/ngbe_ethdev.h > +++ b/drivers/net/ngbe/ngbe_ethdev.h > @@ -6,11 +6,30 @@ > #ifndef _NGBE_ETHDEV_H_ > #define _NGBE_ETHDEV_H_ > > +/* need update link, bit flag */ > +#define NGBE_FLAG_NEED_LINK_UPDATE (uint32_t)(1 << 0) > +#define NGBE_FLAG_MAILBOX (uint32_t)(1 << 1) > +#define NGBE_FLAG_PHY_INTERRUPT (uint32_t)(1 << 2) > +#define NGBE_FLAG_MACSEC (uint32_t)(1 << 3) > +#define NGBE_FLAG_NEED_LINK_CONFIG (uint32_t)(1 << 4) > + > +#define NGBE_MISC_VEC_ID RTE_INTR_VEC_ZERO_OFFSET > + > +/* structure for interrupt relative data */ > +struct ngbe_interrupt { > + uint32_t flags; > + uint32_t mask_misc; > + uint32_t mask_misc_orig; /* save mask during delayed handler */ > + uint64_t mask; > + uint64_t mask_orig; /* save mask during delayed handler */ > +}; > + > /* > * Structure to store private data for each driver instance (for each port). > */ > struct ngbe_adapter { > struct ngbe_hw hw; > + struct ngbe_interrupt intr; > }; > > static inline struct ngbe_adapter * > @@ -30,6 +49,21 @@ ngbe_dev_hw(struct rte_eth_dev *dev) > return hw; > } > > +static inline struct ngbe_interrupt * > +ngbe_dev_intr(struct rte_eth_dev *dev) > +{ > + struct ngbe_adapter *ad = ngbe_dev_adapter(dev); > + struct ngbe_interrupt *intr = &ad->intr; > + > + return intr; > +} > + > +int > +ngbe_dev_link_update_share(struct rte_eth_dev *dev, > + int wait_to_complete); > + > +#define NGBE_LINK_DOWN_CHECK_TIMEOUT 4000 /* ms */ > +#define NGBE_LINK_UP_CHECK_TIMEOUT 1000 /* ms */ > #define NGBE_VMDQ_NUM_UC_MAC 4096 /* Maximum nb. of UC MAC addr. */ > > #endif /* _NGBE_ETHDEV_H_ */ >