On 6/17/21 1:59 PM, Jiawen Wu wrote:
> Register to handle device interrupt.
>
> Signed-off-by: Jiawen Wu <jiawe...@trustnetic.com>
[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_ */
>
