Re: [PATCH 1/2] net/mlx5: support device removal event

["Nélio Laranjeiro" <nelio.laranjeiro@6wind.com>]

On Wed, Aug 23, 2017 at 07:44:45PM +0000, Matan Azrad wrote:
> Hi Nelio
> 
> > -----Original Message-----
> > From: Nélio Laranjeiro [mailto:nelio.laranjeiro@6wind.com]
> > Sent: Wednesday, August 23, 2017 12:41 PM
> > To: Matan Azrad <matan@mellanox.com>
> > Cc: Adrien Mazarguil <adrien.mazarguil@6wind.com>; dev@dpdk.org
> > Subject: Re: [PATCH 1/2] net/mlx5: support device removal event
> > 
> > Hi Matan,
> > 
> > On Sun, Aug 13, 2017 at 03:25:11PM +0300, Matan Azrad wrote:
> > > Extend the LSC event handling to support the device removal as well.
> > > The Verbs library may send several related events, which are different
> > > from LSC event.
> > >
> > > The mlx5 event handling has been made capable of receiving and
> > > signaling several event types at once.
> > >
> > > This support includes next:
> > > 1. Removal event detection according to the user configuration.
> > > 2. Calling to all registered mlx5 removal callbacks.
> > > 3. Capabilities extension to include removal interrupt handling.
> > >
> > > Signed-off-by: Matan Azrad <matan@mellanox.com>
> > > ---
> > >  drivers/net/mlx5/mlx5.c        |   2 +-
> > >  drivers/net/mlx5/mlx5_ethdev.c | 100
> > > +++++++++++++++++++++++++++--------------
> > >  2 files changed, 68 insertions(+), 34 deletions(-)
> > >
> > > Hi
> > > This patch based on top of last Nelio mlx5 cleanup patches.
> > >
> > > diff --git a/drivers/net/mlx5/mlx5.c b/drivers/net/mlx5/mlx5.c index
> > > bd66a7c..1a3d7f1 100644
> > > --- a/drivers/net/mlx5/mlx5.c
> > > +++ b/drivers/net/mlx5/mlx5.c
> > > @@ -865,7 +865,7 @@ static struct rte_pci_driver mlx5_driver = {
> > >  	},
> > >  	.id_table = mlx5_pci_id_map,
> > >  	.probe = mlx5_pci_probe,
> > > -	.drv_flags = RTE_PCI_DRV_INTR_LSC,
> > > +	.drv_flags = RTE_PCI_DRV_INTR_LSC | RTE_PCI_DRV_INTR_RMV,
> > >  };
> > >
> > >  /**
> > > diff --git a/drivers/net/mlx5/mlx5_ethdev.c
> > > b/drivers/net/mlx5/mlx5_ethdev.c index 57f6237..404d8f4 100644
> > > --- a/drivers/net/mlx5/mlx5_ethdev.c
> > > +++ b/drivers/net/mlx5/mlx5_ethdev.c
> > > @@ -1112,47 +1112,75 @@ mlx5_ibv_device_to_pci_addr(const struct
> > > ibv_device *device,  }
> > >
> > >  /**
> > > - * Link status handler.
> > > + * Update the link status.
> > > + * Set alarm if the device link status is inconsistent.
> > 
> > Adding such comment should also comment about the issue this alarm is
> > solving i.e. why the link is inconsistent and why the alarm help to fix the
> > issue.
> > 
> I didn't see any comments about that in the old code , Hence I didn't write it.

Normal as the alarm is a work around specifically necessary to Mellanox PMD.
Now you explicitly announce that this function program an alarm, the question
is why is it necessary?

> I think you right and this could be added.(even before this patch).

No, in the current code, it update the link, if it inconsistent it tries to
have a link correct ASAP.  There is no need to inform this function will
program an alarm, it is internal cooking.

> > >   *
> > >   * @param priv
> > >   *   Pointer to private structure.
> > > - * @param dev
> > > - *   Pointer to the rte_eth_dev structure.
> > >   *
> > >   * @return
> > > - *   Nonzero if the callback process can be called immediately.
> > > + *   Zero if alarm is not set and the link status is consistent.
> > >   */
> > >  static int
> > > -priv_dev_link_status_handler(struct priv *priv, struct rte_eth_dev
> > > *dev)
> > > +priv_link_status_alarm_update(struct priv *priv)
> > 	
> > The old name is more accurate, the fact we need to program an alarm is a
> > work around to get the correct status from ethtool.  If it was possible to avoid
> > it, this alarm would not exists.
> > 
> Probably because of the git +- format and this specific patch you got confuse here.

No I applied your patch and read your code.  You did not understand my
comment.

>[...]

When I read:

>  void
>  mlx5_dev_link_status_handler(void *arg)
>  {
>         struct rte_eth_dev *dev = arg;
>         struct priv *priv = dev->data->dev_private;
>         int ret;
> 
>         priv_lock(priv);
>         assert(priv->pending_alarm == 1);
>         priv->pending_alarm = 0;
> -       ret = priv_dev_link_status_handler(priv, dev);
> +       ret = priv_link_status_alarm_update(priv);
>         priv_unlock(priv);
> -       if (ret)
> +       if (!ret)
>                 _rte_eth_dev_callback_process(dev, RTE_ETH_EVENT_INTR_LSC, NULL,
> -                                             NULL);
> +                       NULL);
>  }

I am expecting to find something related to a link update, what I see is an alarm
update.  I don't expect to update an alarm but a link.  The names and action
are inconsistent i.e. mlx5_dev_link_status_handler() should handle a link not
an alarm.

I understand there is a need to add more function levels, but the
priv_link_status_alarm_update() should be renamed to something like
priv_link_status_update().

Regards,

-- 
Nélio Laranjeiro
6WIND