All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH net v2] nfp: clean mc addresses in application firmware when closing port
@ 2023-07-03 12:01 Louis Peens
  2023-07-03 16:10 ` Alexander Lobakin
  0 siblings, 1 reply; 5+ messages in thread
From: Louis Peens @ 2023-07-03 12:01 UTC (permalink / raw)
  To: David Miller, Jakub Kicinski, Paolo Abeni
  Cc: Jacob Keller, Simon Horman, Yinjun Zhang, netdev, stable, oss-drivers

From: Yinjun Zhang <yinjun.zhang@corigine.com>

When moving devices from one namespace to another, mc addresses are
cleaned in software while not removed from application firmware. Thus
the mc addresses are remained and will cause resource leak.

Now use `__dev_mc_unsync` to clean mc addresses when closing port.

Fixes: e20aa071cd95 ("nfp: fix schedule in atomic context when sync mc address")
Cc: stable@vger.kernel.org
Signed-off-by: Yinjun Zhang <yinjun.zhang@corigine.com>
Acked-by: Simon Horman <simon.horman@corigine.com>
Signed-off-by: Louis Peens <louis.peens@corigine.com>
---
Changes since v1:

* Use __dev_mc_unsyc to clean mc addresses instead of tracking mc addresses by
  driver itself.
* Clean mc addresses when closing port instead of driver exits,
  so that the issue of moving devices between namespaces can be fixed.
* Modify commit message accordingly.

 .../ethernet/netronome/nfp/nfp_net_common.c   | 171 +++++++++---------
 1 file changed, 87 insertions(+), 84 deletions(-)

diff --git a/drivers/net/ethernet/netronome/nfp/nfp_net_common.c b/drivers/net/ethernet/netronome/nfp/nfp_net_common.c
index 49f2f081ebb5..37b6a034c5d2 100644
--- a/drivers/net/ethernet/netronome/nfp/nfp_net_common.c
+++ b/drivers/net/ethernet/netronome/nfp/nfp_net_common.c
@@ -914,6 +914,90 @@ static void nfp_net_write_mac_addr(struct nfp_net *nn, const u8 *addr)
 	nn_writew(nn, NFP_NET_CFG_MACADDR + 6, get_unaligned_be16(addr + 4));
 }
 
+int nfp_net_sched_mbox_amsg_work(struct nfp_net *nn, u32 cmd, const void *data, size_t len,
+				 int (*cb)(struct nfp_net *, struct nfp_mbox_amsg_entry *))
+{
+	struct nfp_mbox_amsg_entry *entry;
+
+	entry = kmalloc(sizeof(*entry) + len, GFP_ATOMIC);
+	if (!entry)
+		return -ENOMEM;
+
+	memcpy(entry->msg, data, len);
+	entry->cmd = cmd;
+	entry->cfg = cb;
+
+	spin_lock_bh(&nn->mbox_amsg.lock);
+	list_add_tail(&entry->list, &nn->mbox_amsg.list);
+	spin_unlock_bh(&nn->mbox_amsg.lock);
+
+	schedule_work(&nn->mbox_amsg.work);
+
+	return 0;
+}
+
+static void nfp_net_mbox_amsg_work(struct work_struct *work)
+{
+	struct nfp_net *nn = container_of(work, struct nfp_net, mbox_amsg.work);
+	struct nfp_mbox_amsg_entry *entry, *tmp;
+	struct list_head tmp_list;
+
+	INIT_LIST_HEAD(&tmp_list);
+
+	spin_lock_bh(&nn->mbox_amsg.lock);
+	list_splice_init(&nn->mbox_amsg.list, &tmp_list);
+	spin_unlock_bh(&nn->mbox_amsg.lock);
+
+	list_for_each_entry_safe(entry, tmp, &tmp_list, list) {
+		int err = entry->cfg(nn, entry);
+
+		if (err)
+			nn_err(nn, "Config cmd %d to HW failed %d.\n", entry->cmd, err);
+
+		list_del(&entry->list);
+		kfree(entry);
+	}
+}
+
+static int nfp_net_mc_cfg(struct nfp_net *nn, struct nfp_mbox_amsg_entry *entry)
+{
+	unsigned char *addr = entry->msg;
+	int ret;
+
+	ret = nfp_net_mbox_lock(nn, NFP_NET_CFG_MULTICAST_SZ);
+	if (ret)
+		return ret;
+
+	nn_writel(nn, nn->tlv_caps.mbox_off + NFP_NET_CFG_MULTICAST_MAC_HI,
+		  get_unaligned_be32(addr));
+	nn_writew(nn, nn->tlv_caps.mbox_off + NFP_NET_CFG_MULTICAST_MAC_LO,
+		  get_unaligned_be16(addr + 4));
+
+	return nfp_net_mbox_reconfig_and_unlock(nn, entry->cmd);
+}
+
+static int nfp_net_mc_sync(struct net_device *netdev, const unsigned char *addr)
+{
+	struct nfp_net *nn = netdev_priv(netdev);
+
+	if (netdev_mc_count(netdev) > NFP_NET_CFG_MAC_MC_MAX) {
+		nn_err(nn, "Requested number of MC addresses (%d) exceeds maximum (%d).\n",
+		       netdev_mc_count(netdev), NFP_NET_CFG_MAC_MC_MAX);
+		return -EINVAL;
+	}
+
+	return nfp_net_sched_mbox_amsg_work(nn, NFP_NET_CFG_MBOX_CMD_MULTICAST_ADD, addr,
+					    NFP_NET_CFG_MULTICAST_SZ, nfp_net_mc_cfg);
+}
+
+static int nfp_net_mc_unsync(struct net_device *netdev, const unsigned char *addr)
+{
+	struct nfp_net *nn = netdev_priv(netdev);
+
+	return nfp_net_sched_mbox_amsg_work(nn, NFP_NET_CFG_MBOX_CMD_MULTICAST_DEL, addr,
+					    NFP_NET_CFG_MULTICAST_SZ, nfp_net_mc_cfg);
+}
+
 /**
  * nfp_net_clear_config_and_disable() - Clear control BAR and disable NFP
  * @nn:      NFP Net device to reconfigure
@@ -1084,6 +1168,9 @@ static int nfp_net_netdev_close(struct net_device *netdev)
 
 	/* Step 2: Tell NFP
 	 */
+	if (nn->cap_w1 & NFP_NET_CFG_CTRL_MCAST_FILTER)
+		__dev_mc_unsync(netdev, nfp_net_mc_unsync);
+
 	nfp_net_clear_config_and_disable(nn);
 	nfp_port_configure(netdev, false);
 
@@ -1335,90 +1422,6 @@ int nfp_ctrl_open(struct nfp_net *nn)
 	return err;
 }
 
-int nfp_net_sched_mbox_amsg_work(struct nfp_net *nn, u32 cmd, const void *data, size_t len,
-				 int (*cb)(struct nfp_net *, struct nfp_mbox_amsg_entry *))
-{
-	struct nfp_mbox_amsg_entry *entry;
-
-	entry = kmalloc(sizeof(*entry) + len, GFP_ATOMIC);
-	if (!entry)
-		return -ENOMEM;
-
-	memcpy(entry->msg, data, len);
-	entry->cmd = cmd;
-	entry->cfg = cb;
-
-	spin_lock_bh(&nn->mbox_amsg.lock);
-	list_add_tail(&entry->list, &nn->mbox_amsg.list);
-	spin_unlock_bh(&nn->mbox_amsg.lock);
-
-	schedule_work(&nn->mbox_amsg.work);
-
-	return 0;
-}
-
-static void nfp_net_mbox_amsg_work(struct work_struct *work)
-{
-	struct nfp_net *nn = container_of(work, struct nfp_net, mbox_amsg.work);
-	struct nfp_mbox_amsg_entry *entry, *tmp;
-	struct list_head tmp_list;
-
-	INIT_LIST_HEAD(&tmp_list);
-
-	spin_lock_bh(&nn->mbox_amsg.lock);
-	list_splice_init(&nn->mbox_amsg.list, &tmp_list);
-	spin_unlock_bh(&nn->mbox_amsg.lock);
-
-	list_for_each_entry_safe(entry, tmp, &tmp_list, list) {
-		int err = entry->cfg(nn, entry);
-
-		if (err)
-			nn_err(nn, "Config cmd %d to HW failed %d.\n", entry->cmd, err);
-
-		list_del(&entry->list);
-		kfree(entry);
-	}
-}
-
-static int nfp_net_mc_cfg(struct nfp_net *nn, struct nfp_mbox_amsg_entry *entry)
-{
-	unsigned char *addr = entry->msg;
-	int ret;
-
-	ret = nfp_net_mbox_lock(nn, NFP_NET_CFG_MULTICAST_SZ);
-	if (ret)
-		return ret;
-
-	nn_writel(nn, nn->tlv_caps.mbox_off + NFP_NET_CFG_MULTICAST_MAC_HI,
-		  get_unaligned_be32(addr));
-	nn_writew(nn, nn->tlv_caps.mbox_off + NFP_NET_CFG_MULTICAST_MAC_LO,
-		  get_unaligned_be16(addr + 4));
-
-	return nfp_net_mbox_reconfig_and_unlock(nn, entry->cmd);
-}
-
-static int nfp_net_mc_sync(struct net_device *netdev, const unsigned char *addr)
-{
-	struct nfp_net *nn = netdev_priv(netdev);
-
-	if (netdev_mc_count(netdev) > NFP_NET_CFG_MAC_MC_MAX) {
-		nn_err(nn, "Requested number of MC addresses (%d) exceeds maximum (%d).\n",
-		       netdev_mc_count(netdev), NFP_NET_CFG_MAC_MC_MAX);
-		return -EINVAL;
-	}
-
-	return nfp_net_sched_mbox_amsg_work(nn, NFP_NET_CFG_MBOX_CMD_MULTICAST_ADD, addr,
-					    NFP_NET_CFG_MULTICAST_SZ, nfp_net_mc_cfg);
-}
-
-static int nfp_net_mc_unsync(struct net_device *netdev, const unsigned char *addr)
-{
-	struct nfp_net *nn = netdev_priv(netdev);
-
-	return nfp_net_sched_mbox_amsg_work(nn, NFP_NET_CFG_MBOX_CMD_MULTICAST_DEL, addr,
-					    NFP_NET_CFG_MULTICAST_SZ, nfp_net_mc_cfg);
-}
-
 static void nfp_net_set_rx_mode(struct net_device *netdev)
 {
 	struct nfp_net *nn = netdev_priv(netdev);
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH net v2] nfp: clean mc addresses in application firmware when closing port
  2023-07-03 12:01 [PATCH net v2] nfp: clean mc addresses in application firmware when closing port Louis Peens
@ 2023-07-03 16:10 ` Alexander Lobakin
  2023-07-04  1:50   ` Yinjun Zhang
  0 siblings, 1 reply; 5+ messages in thread
From: Alexander Lobakin @ 2023-07-03 16:10 UTC (permalink / raw)
  To: Louis Peens
  Cc: David Miller, Jakub Kicinski, Paolo Abeni, Jacob Keller,
	Simon Horman, Yinjun Zhang, netdev, stable, oss-drivers

From: Louis Peens <louis.peens@corigine.com>
Date: Mon,  3 Jul 2023 14:01:16 +0200

> From: Yinjun Zhang <yinjun.zhang@corigine.com>
> 
> When moving devices from one namespace to another, mc addresses are
> cleaned in software while not removed from application firmware. Thus
> the mc addresses are remained and will cause resource leak.
> 
> Now use `__dev_mc_unsync` to clean mc addresses when closing port.
> 
> Fixes: e20aa071cd95 ("nfp: fix schedule in atomic context when sync mc address")
> Cc: stable@vger.kernel.org
> Signed-off-by: Yinjun Zhang <yinjun.zhang@corigine.com>
> Acked-by: Simon Horman <simon.horman@corigine.com>
> Signed-off-by: Louis Peens <louis.peens@corigine.com>
> ---
> Changes since v1:
> 
> * Use __dev_mc_unsyc to clean mc addresses instead of tracking mc addresses by
>   driver itself.
> * Clean mc addresses when closing port instead of driver exits,
>   so that the issue of moving devices between namespaces can be fixed.
> * Modify commit message accordingly.
> 
>  .../ethernet/netronome/nfp/nfp_net_common.c   | 171 +++++++++---------
>  1 file changed, 87 insertions(+), 84 deletions(-)

[...]

> +static int nfp_net_mc_sync(struct net_device *netdev, const unsigned char *addr)
> +{
> +	struct nfp_net *nn = netdev_priv(netdev);
> +
> +	if (netdev_mc_count(netdev) > NFP_NET_CFG_MAC_MC_MAX) {
> +		nn_err(nn, "Requested number of MC addresses (%d) exceeds maximum (%d).\n",
> +		       netdev_mc_count(netdev), NFP_NET_CFG_MAC_MC_MAX);
> +		return -EINVAL;
> +	}
> +
> +	return nfp_net_sched_mbox_amsg_work(nn, NFP_NET_CFG_MBOX_CMD_MULTICAST_ADD, addr,
> +					    NFP_NET_CFG_MULTICAST_SZ, nfp_net_mc_cfg);
> +}
> +
> +static int nfp_net_mc_unsync(struct net_device *netdev, const unsigned char *addr)
> +{
> +	struct nfp_net *nn = netdev_priv(netdev);
> +
> +	return nfp_net_sched_mbox_amsg_work(nn, NFP_NET_CFG_MBOX_CMD_MULTICAST_DEL, addr,
> +					    NFP_NET_CFG_MULTICAST_SZ, nfp_net_mc_cfg);
> +}

You can just declare nfp_net_mc_unsync()'s prototype here, so that it
will be visible to nfp_net_netdev_close(), without moving the whole set
of functions. Either way works, but that one would allow avoiding big
diffs not really related to fixing things going through the net-fixes tree.

> +
>  /**
>   * nfp_net_clear_config_and_disable() - Clear control BAR and disable NFP
>   * @nn:      NFP Net device to reconfigure
> @@ -1084,6 +1168,9 @@ static int nfp_net_netdev_close(struct net_device *netdev)
>  
>  	/* Step 2: Tell NFP
>  	 */
> +	if (nn->cap_w1 & NFP_NET_CFG_CTRL_MCAST_FILTER)
> +		__dev_mc_unsync(netdev, nfp_net_mc_unsync);
> +
>  	nfp_net_clear_config_and_disable(nn);
>  	nfp_port_configure(netdev, false);
[...]

Thanks,
Olek

^ permalink raw reply	[flat|nested] 5+ messages in thread

* RE: [PATCH net v2] nfp: clean mc addresses in application firmware when closing port
  2023-07-03 16:10 ` Alexander Lobakin
@ 2023-07-04  1:50   ` Yinjun Zhang
  2023-07-04  8:39     ` Paolo Abeni
  2023-07-05 17:29     ` Jacob Keller
  0 siblings, 2 replies; 5+ messages in thread
From: Yinjun Zhang @ 2023-07-04  1:50 UTC (permalink / raw)
  To: Alexander Lobakin, Louis Peens
  Cc: David Miller, Jakub Kicinski, Paolo Abeni, Jacob Keller,
	Simon Horman, netdev, stable, oss-drivers

On Tuesday, July 4, 2023 12:11 AM, Alexander Lobakin wrote:
> From: Louis Peens <louis.peens@corigine.com>
> Date: Mon,  3 Jul 2023 14:01:16 +0200
> 
> > From: Yinjun Zhang <yinjun.zhang@corigine.com>
> >
> > When moving devices from one namespace to another, mc addresses are
> > cleaned in software while not removed from application firmware. Thus
> > the mc addresses are remained and will cause resource leak.
> >
> > Now use `__dev_mc_unsync` to clean mc addresses when closing port.
> >
> > Fixes: e20aa071cd95 ("nfp: fix schedule in atomic context when sync mc
> address")
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Yinjun Zhang <yinjun.zhang@corigine.com>
> > Acked-by: Simon Horman <simon.horman@corigine.com>
> > Signed-off-by: Louis Peens <louis.peens@corigine.com>
> > ---
> > Changes since v1:
> >
> > * Use __dev_mc_unsyc to clean mc addresses instead of tracking mc
> addresses by
> >   driver itself.
> > * Clean mc addresses when closing port instead of driver exits,
> >   so that the issue of moving devices between namespaces can be fixed.
> > * Modify commit message accordingly.
> >
> >  .../ethernet/netronome/nfp/nfp_net_common.c   | 171 +++++++++---------
> >  1 file changed, 87 insertions(+), 84 deletions(-)
> 
> [...]
> 
> > +static int nfp_net_mc_sync(struct net_device *netdev, const unsigned char
> *addr)
> > +{
> > +	struct nfp_net *nn = netdev_priv(netdev);
> > +
> > +	if (netdev_mc_count(netdev) > NFP_NET_CFG_MAC_MC_MAX) {
> > +		nn_err(nn, "Requested number of MC addresses (%d)
> exceeds maximum (%d).\n",
> > +		       netdev_mc_count(netdev),
> NFP_NET_CFG_MAC_MC_MAX);
> > +		return -EINVAL;
> > +	}
> > +
> > +	return nfp_net_sched_mbox_amsg_work(nn,
> NFP_NET_CFG_MBOX_CMD_MULTICAST_ADD, addr,
> > +					    NFP_NET_CFG_MULTICAST_SZ,
> nfp_net_mc_cfg);
> > +}
> > +
> > +static int nfp_net_mc_unsync(struct net_device *netdev, const unsigned
> char *addr)
> > +{
> > +	struct nfp_net *nn = netdev_priv(netdev);
> > +
> > +	return nfp_net_sched_mbox_amsg_work(nn,
> NFP_NET_CFG_MBOX_CMD_MULTICAST_DEL, addr,
> > +					    NFP_NET_CFG_MULTICAST_SZ,
> nfp_net_mc_cfg);
> > +}
> 
> You can just declare nfp_net_mc_unsync()'s prototype here, so that it
> will be visible to nfp_net_netdev_close(), without moving the whole set
> of functions. Either way works, but that one would allow avoiding big
> diffs not really related to fixing things going through the net-fixes tree.

I didn't know which was preferred. Looks like minimum change is concerned
more. I'll change it.

> 
> > +
> >  /**
> >   * nfp_net_clear_config_and_disable() - Clear control BAR and disable NFP
> >   * @nn:      NFP Net device to reconfigure
> > @@ -1084,6 +1168,9 @@ static int nfp_net_netdev_close(struct net_device
> *netdev)
> >
> >  	/* Step 2: Tell NFP
> >  	 */
> > +	if (nn->cap_w1 & NFP_NET_CFG_CTRL_MCAST_FILTER)
> > +		__dev_mc_unsync(netdev, nfp_net_mc_unsync);
> > +
> >  	nfp_net_clear_config_and_disable(nn);
> >  	nfp_port_configure(netdev, false);
> [...]
> 
> Thanks,
> Olek

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH net v2] nfp: clean mc addresses in application firmware when closing port
  2023-07-04  1:50   ` Yinjun Zhang
@ 2023-07-04  8:39     ` Paolo Abeni
  2023-07-05 17:29     ` Jacob Keller
  1 sibling, 0 replies; 5+ messages in thread
From: Paolo Abeni @ 2023-07-04  8:39 UTC (permalink / raw)
  To: Yinjun Zhang, Alexander Lobakin, Louis Peens
  Cc: David Miller, Jakub Kicinski, Jacob Keller, Simon Horman, netdev,
	stable, oss-drivers

On Tue, 2023-07-04 at 01:50 +0000, Yinjun Zhang wrote:
> On Tuesday, July 4, 2023 12:11 AM, Alexander Lobakin wrote:
> > From: Louis Peens <louis.peens@corigine.com>
> > Date: Mon,  3 Jul 2023 14:01:16 +0200
> > 
> > > From: Yinjun Zhang <yinjun.zhang@corigine.com>
> > > 
> > > When moving devices from one namespace to another, mc addresses are
> > > cleaned in software while not removed from application firmware. Thus
> > > the mc addresses are remained and will cause resource leak.
> > > 
> > > Now use `__dev_mc_unsync` to clean mc addresses when closing port.
> > > 
> > > Fixes: e20aa071cd95 ("nfp: fix schedule in atomic context when sync mc
> > address")
> > > Cc: stable@vger.kernel.org
> > > Signed-off-by: Yinjun Zhang <yinjun.zhang@corigine.com>
> > > Acked-by: Simon Horman <simon.horman@corigine.com>
> > > Signed-off-by: Louis Peens <louis.peens@corigine.com>
> > > ---
> > > Changes since v1:
> > > 
> > > * Use __dev_mc_unsyc to clean mc addresses instead of tracking mc
> > addresses by
> > >   driver itself.
> > > * Clean mc addresses when closing port instead of driver exits,
> > >   so that the issue of moving devices between namespaces can be fixed.
> > > * Modify commit message accordingly.
> > > 
> > >  .../ethernet/netronome/nfp/nfp_net_common.c   | 171 +++++++++---------
> > >  1 file changed, 87 insertions(+), 84 deletions(-)
> > 
> > [...]
> > 
> > > +static int nfp_net_mc_sync(struct net_device *netdev, const unsigned char
> > *addr)
> > > +{
> > > +	struct nfp_net *nn = netdev_priv(netdev);
> > > +
> > > +	if (netdev_mc_count(netdev) > NFP_NET_CFG_MAC_MC_MAX) {
> > > +		nn_err(nn, "Requested number of MC addresses (%d)
> > exceeds maximum (%d).\n",
> > > +		       netdev_mc_count(netdev),
> > NFP_NET_CFG_MAC_MC_MAX);
> > > +		return -EINVAL;
> > > +	}
> > > +
> > > +	return nfp_net_sched_mbox_amsg_work(nn,
> > NFP_NET_CFG_MBOX_CMD_MULTICAST_ADD, addr,
> > > +					    NFP_NET_CFG_MULTICAST_SZ,
> > nfp_net_mc_cfg);
> > > +}
> > > +
> > > +static int nfp_net_mc_unsync(struct net_device *netdev, const unsigned
> > char *addr)
> > > +{
> > > +	struct nfp_net *nn = netdev_priv(netdev);
> > > +
> > > +	return nfp_net_sched_mbox_amsg_work(nn,
> > NFP_NET_CFG_MBOX_CMD_MULTICAST_DEL, addr,
> > > +					    NFP_NET_CFG_MULTICAST_SZ,
> > nfp_net_mc_cfg);
> > > +}
> > 
> > You can just declare nfp_net_mc_unsync()'s prototype here, so that it
> > will be visible to nfp_net_netdev_close(), without moving the whole set
> > of functions. Either way works, but that one would allow avoiding big
> > diffs not really related to fixing things going through the net-fixes tree.
> 
> I didn't know which was preferred. Looks like minimum change is concerned
> more. I'll change it.

That is de-facto mandatory for changes targeting stable:

https://elixir.bootlin.com/linux/latest/source/Documentation/process/stable-kernel-rules.rst#L10

Cheers,

Paolo


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH net v2] nfp: clean mc addresses in application firmware when closing port
  2023-07-04  1:50   ` Yinjun Zhang
  2023-07-04  8:39     ` Paolo Abeni
@ 2023-07-05 17:29     ` Jacob Keller
  1 sibling, 0 replies; 5+ messages in thread
From: Jacob Keller @ 2023-07-05 17:29 UTC (permalink / raw)
  To: Yinjun Zhang, Alexander Lobakin, Louis Peens
  Cc: David Miller, Jakub Kicinski, Paolo Abeni, Simon Horman, netdev,
	stable, oss-drivers



On 7/3/2023 6:50 PM, Yinjun Zhang wrote:
> On Tuesday, July 4, 2023 12:11 AM, Alexander Lobakin wrote:
>> From: Louis Peens <louis.peens@corigine.com>
>> Date: Mon,  3 Jul 2023 14:01:16 +0200
>>
>>> From: Yinjun Zhang <yinjun.zhang@corigine.com>
>>>
>>> When moving devices from one namespace to another, mc addresses are
>>> cleaned in software while not removed from application firmware. Thus
>>> the mc addresses are remained and will cause resource leak.
>>>
>>> Now use `__dev_mc_unsync` to clean mc addresses when closing port.
>>>
>>> Fixes: e20aa071cd95 ("nfp: fix schedule in atomic context when sync mc
>> address")
>>> Cc: stable@vger.kernel.org
>>> Signed-off-by: Yinjun Zhang <yinjun.zhang@corigine.com>
>>> Acked-by: Simon Horman <simon.horman@corigine.com>
>>> Signed-off-by: Louis Peens <louis.peens@corigine.com>
>>> ---
>>> Changes since v1:
>>>
>>> * Use __dev_mc_unsyc to clean mc addresses instead of tracking mc
>> addresses by
>>>   driver itself.
>>> * Clean mc addresses when closing port instead of driver exits,
>>>   so that the issue of moving devices between namespaces can be fixed.
>>> * Modify commit message accordingly.
>>>
>>>  .../ethernet/netronome/nfp/nfp_net_common.c   | 171 +++++++++---------
>>>  1 file changed, 87 insertions(+), 84 deletions(-)
>>
>> [...]
>>
>>> +static int nfp_net_mc_sync(struct net_device *netdev, const unsigned char
>> *addr)
>>> +{
>>> +	struct nfp_net *nn = netdev_priv(netdev);
>>> +
>>> +	if (netdev_mc_count(netdev) > NFP_NET_CFG_MAC_MC_MAX) {
>>> +		nn_err(nn, "Requested number of MC addresses (%d)
>> exceeds maximum (%d).\n",
>>> +		       netdev_mc_count(netdev),
>> NFP_NET_CFG_MAC_MC_MAX);
>>> +		return -EINVAL;
>>> +	}
>>> +
>>> +	return nfp_net_sched_mbox_amsg_work(nn,
>> NFP_NET_CFG_MBOX_CMD_MULTICAST_ADD, addr,
>>> +					    NFP_NET_CFG_MULTICAST_SZ,
>> nfp_net_mc_cfg);
>>> +}
>>> +
>>> +static int nfp_net_mc_unsync(struct net_device *netdev, const unsigned
>> char *addr)
>>> +{
>>> +	struct nfp_net *nn = netdev_priv(netdev);
>>> +
>>> +	return nfp_net_sched_mbox_amsg_work(nn,
>> NFP_NET_CFG_MBOX_CMD_MULTICAST_DEL, addr,
>>> +					    NFP_NET_CFG_MULTICAST_SZ,
>> nfp_net_mc_cfg);
>>> +}
>>
>> You can just declare nfp_net_mc_unsync()'s prototype here, so that it
>> will be visible to nfp_net_netdev_close(), without moving the whole set
>> of functions. Either way works, but that one would allow avoiding big
>> diffs not really related to fixing things going through the net-fixes tree.
> 
> I didn't know which was preferred. Looks like minimum change is concerned
> more. I'll change it.
> 

net-next might prefer code re-ordering and avoiding the extra
declaration, but net would definitely want the smaller fix.

For what its worth, I double check this kind of thing by applying the
patch to my git tree and using git's "color moved lines" options to diff.

Doing so for this patch shows that the change really is a straight
forward re-ordering without any additional changes accidentally included.

Thus, I have no objection to this version as-is, but a smaller v3 with
the prototype is also fine with me.

Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>

Thanks,
Jake

>>
>>> +
>>>  /**
>>>   * nfp_net_clear_config_and_disable() - Clear control BAR and disable NFP
>>>   * @nn:      NFP Net device to reconfigure
>>> @@ -1084,6 +1168,9 @@ static int nfp_net_netdev_close(struct net_device
>> *netdev)
>>>
>>>  	/* Step 2: Tell NFP
>>>  	 */
>>> +	if (nn->cap_w1 & NFP_NET_CFG_CTRL_MCAST_FILTER)
>>> +		__dev_mc_unsync(netdev, nfp_net_mc_unsync);
>>> +
>>>  	nfp_net_clear_config_and_disable(nn);
>>>  	nfp_port_configure(netdev, false);
>> [...]
>>
>> Thanks,
>> Olek

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2023-07-05 17:30 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-07-03 12:01 [PATCH net v2] nfp: clean mc addresses in application firmware when closing port Louis Peens
2023-07-03 16:10 ` Alexander Lobakin
2023-07-04  1:50   ` Yinjun Zhang
2023-07-04  8:39     ` Paolo Abeni
2023-07-05 17:29     ` Jacob Keller

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.