On Wed, Aug 23, 2017 at 07:44:45PM +0000, Matan Azrad wrote:
> Hi Nelio
>
> > -----Original Message-----
> > From: Nélio Laranjeiro [mailto:nelio.laranje...@6wind.com]
> > Sent: Wednesday, August 23, 2017 12:41 PM
> > To: Matan Azrad <ma...@mellanox.com>
> > Cc: Adrien Mazarguil <adrien.mazarg...@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 <ma...@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
