All of lore.kernel.org
 help / color / mirror / Atom feed
* [net-next-2.6 PATCH] infiniband: convert to use netdev_for_each_mc_addr
@ 2010-02-24 15:11 Jiri Pirko
       [not found] ` <20100224151108.GD2663-YzwxZg+R7evMbnheQZGK0N5OCZ2W11yPFxja6HXR22MAvxtiuMwx3w@public.gmane.org>
  0 siblings, 1 reply; 15+ messages in thread
From: Jiri Pirko @ 2010-02-24 15:11 UTC (permalink / raw)
  To: netdev-u79uwXL29TY76Z2rM5mHXA
  Cc: davem-fT/PcQaiUtIeIZ0/mPfg9Q,
	faisal.latif-ral2JQCrhuEAvxtiuMwx3w,
	chien.tin.tung-ral2JQCrhuEAvxtiuMwx3w,
	linux-rdma-u79uwXL29TY76Z2rM5mHXA

Due to the loop complexicity in nes_nic.c, I'm using char* to copy mc addresses
to it.

Signed-off-by: Jiri Pirko <jpirko-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 drivers/infiniband/hw/nes/nes_nic.c            |   85 ++++++++++++++----------
 drivers/infiniband/ulp/ipoib/ipoib_multicast.c |    8 +--
 2 files changed, 51 insertions(+), 42 deletions(-)

diff --git a/drivers/infiniband/hw/nes/nes_nic.c b/drivers/infiniband/hw/nes/nes_nic.c
index c04f8fc..9384f5d 100644
--- a/drivers/infiniband/hw/nes/nes_nic.c
+++ b/drivers/infiniband/hw/nes/nes_nic.c
@@ -810,6 +810,20 @@ static int nes_netdev_set_mac_address(struct net_device *netdev, void *p)
 }
 
 
+static void set_allmulti(struct nes_device *nesdev, u32 nic_active_bit)
+{
+	u32 nic_active;
+
+	nic_active = nes_read_indexed(nesdev, NES_IDX_NIC_MULTICAST_ALL);
+	nic_active |= nic_active_bit;
+	nes_write_indexed(nesdev, NES_IDX_NIC_MULTICAST_ALL, nic_active);
+	nic_active = nes_read_indexed(nesdev, NES_IDX_NIC_UNICAST_ALL);
+	nic_active &= ~nic_active_bit;
+	nes_write_indexed(nesdev, NES_IDX_NIC_UNICAST_ALL, nic_active);
+}
+
+#define get_addr(addrs, index) ((addrs) + (index) * ETH_ALEN)
+
 /**
  * nes_netdev_set_multicast_list
  */
@@ -818,7 +832,6 @@ static void nes_netdev_set_multicast_list(struct net_device *netdev)
 	struct nes_vnic *nesvnic = netdev_priv(netdev);
 	struct nes_device *nesdev = nesvnic->nesdev;
 	struct nes_adapter *nesadapter = nesvnic->nesdev->nesadapter;
-	struct dev_mc_list *multicast_addr;
 	u32 nic_active_bit;
 	u32 nic_active;
 	u32 perfect_filter_register_address;
@@ -831,6 +844,7 @@ static void nes_netdev_set_multicast_list(struct net_device *netdev)
 					nics_per_function, 4);
 	u8 max_pft_entries_avaiable = NES_PFT_SIZE - pft_entries_preallocated;
 	unsigned long flags;
+	int mc_count = netdev_mc_count(netdev);
 
 	spin_lock_irqsave(&nesadapter->resource_lock, flags);
 	nic_active_bit = 1 << nesvnic->nic_index;
@@ -845,12 +859,7 @@ static void nes_netdev_set_multicast_list(struct net_device *netdev)
 		mc_all_on = 1;
 	} else if ((netdev->flags & IFF_ALLMULTI) ||
 			   (nesvnic->nic_index > 3)) {
-		nic_active = nes_read_indexed(nesdev, NES_IDX_NIC_MULTICAST_ALL);
-		nic_active |= nic_active_bit;
-		nes_write_indexed(nesdev, NES_IDX_NIC_MULTICAST_ALL, nic_active);
-		nic_active = nes_read_indexed(nesdev, NES_IDX_NIC_UNICAST_ALL);
-		nic_active &= ~nic_active_bit;
-		nes_write_indexed(nesdev, NES_IDX_NIC_UNICAST_ALL, nic_active);
+		set_allmulti(nesdev, nic_active_bit);
 		mc_all_on = 1;
 	} else {
 		nic_active = nes_read_indexed(nesdev, NES_IDX_NIC_MULTICAST_ALL);
@@ -862,19 +871,30 @@ static void nes_netdev_set_multicast_list(struct net_device *netdev)
 	}
 
 	nes_debug(NES_DBG_NIC_RX, "Number of MC entries = %d, Promiscous = %d, All Multicast = %d.\n",
-		  netdev_mc_count(netdev), !!(netdev->flags & IFF_PROMISC),
+		  mc_count, !!(netdev->flags & IFF_PROMISC),
 		  !!(netdev->flags & IFF_ALLMULTI));
 	if (!mc_all_on) {
-		multicast_addr = netdev->mc_list;
+		char *addrs;
+		int i;
+		struct dev_mc_list *mcaddr;
+
+		addrs = kmalloc(ETH_ALEN * mc_count, GFP_ATOMIC);
+		if (!addrs) {
+			set_allmulti(nesdev, nic_active_bit);
+			goto unlock;
+		}
+		i = 0;
+		netdev_for_each_mc_addr(mcaddr, netdev)
+			memcpy(get_addr(addrs, i++),
+			       mcaddr->dmi_addr, ETH_ALEN);
+
 		perfect_filter_register_address = NES_IDX_PERFECT_FILTER_LOW +
 						pft_entries_preallocated * 0x8;
-		for (mc_index = 0; mc_index < max_pft_entries_avaiable;
-		mc_index++) {
-			while (multicast_addr && nesvnic->mcrq_mcast_filter &&
+		for (i = 0, mc_index = 0; mc_index < max_pft_entries_avaiable;
+		     mc_index++) {
+			while (i < mc_count && nesvnic->mcrq_mcast_filter &&
 			((mc_nic_index = nesvnic->mcrq_mcast_filter(nesvnic,
-					multicast_addr->dmi_addr)) == 0)) {
-				multicast_addr = multicast_addr->next;
-			}
+					get_addr(addrs, i++))) == 0));
 			if (mc_nic_index < 0)
 				mc_nic_index = nesvnic->nic_index;
 			while (nesadapter->pft_mcast_map[mc_index] < 16 &&
@@ -890,17 +910,19 @@ static void nes_netdev_set_multicast_list(struct net_device *netdev)
 			}
 			if (mc_index >= max_pft_entries_avaiable)
 				break;
-			if (multicast_addr) {
+			if (i < mc_count) {
+				char *addr = get_addr(addrs, i++);
+
 				nes_debug(NES_DBG_NIC_RX, "Assigning MC Address %pM to register 0x%04X nic_idx=%d\n",
-					  multicast_addr->dmi_addr,
+					  addr,
 					  perfect_filter_register_address+(mc_index * 8),
 					  mc_nic_index);
-				macaddr_high  = ((u16)multicast_addr->dmi_addr[0]) << 8;
-				macaddr_high += (u16)multicast_addr->dmi_addr[1];
-				macaddr_low   = ((u32)multicast_addr->dmi_addr[2]) << 24;
-				macaddr_low  += ((u32)multicast_addr->dmi_addr[3]) << 16;
-				macaddr_low  += ((u32)multicast_addr->dmi_addr[4]) << 8;
-				macaddr_low  += (u32)multicast_addr->dmi_addr[5];
+				macaddr_high  = ((u16) addr[0]) << 8;
+				macaddr_high += (u16) addr[1];
+				macaddr_low   = ((u32) addr[2]) << 24;
+				macaddr_low  += ((u32) addr[3]) << 16;
+				macaddr_low  += ((u32) addr[4]) << 8;
+				macaddr_low  += (u32) addr[5];
 				nes_write_indexed(nesdev,
 						perfect_filter_register_address+(mc_index * 8),
 						macaddr_low);
@@ -908,7 +930,6 @@ static void nes_netdev_set_multicast_list(struct net_device *netdev)
 						perfect_filter_register_address+4+(mc_index * 8),
 						(u32)macaddr_high | NES_MAC_ADDR_VALID |
 						((((u32)(1<<mc_nic_index)) << 16)));
-				multicast_addr = multicast_addr->next;
 				nesadapter->pft_mcast_map[mc_index] =
 							nesvnic->nic_index;
 			} else {
@@ -920,21 +941,13 @@ static void nes_netdev_set_multicast_list(struct net_device *netdev)
 				nesadapter->pft_mcast_map[mc_index] = 255;
 			}
 		}
+		kfree(addrs);
 		/* PFT is not large enough */
-		if (multicast_addr && multicast_addr->next) {
-			nic_active = nes_read_indexed(nesdev,
-						NES_IDX_NIC_MULTICAST_ALL);
-			nic_active |= nic_active_bit;
-			nes_write_indexed(nesdev, NES_IDX_NIC_MULTICAST_ALL,
-								nic_active);
-			nic_active = nes_read_indexed(nesdev,
-						NES_IDX_NIC_UNICAST_ALL);
-			nic_active &= ~nic_active_bit;
-			nes_write_indexed(nesdev, NES_IDX_NIC_UNICAST_ALL,
-								nic_active);
-		}
+		if (i < mc_count)
+			set_allmulti(nesdev, nic_active_bit);
 	}
 
+unlock:
 	spin_unlock_irqrestore(&nesadapter->resource_lock, flags);
 }
 
diff --git a/drivers/infiniband/ulp/ipoib/ipoib_multicast.c b/drivers/infiniband/ulp/ipoib/ipoib_multicast.c
index 8763c1e..19eba3c 100644
--- a/drivers/infiniband/ulp/ipoib/ipoib_multicast.c
+++ b/drivers/infiniband/ulp/ipoib/ipoib_multicast.c
@@ -767,11 +767,8 @@ void ipoib_mcast_dev_flush(struct net_device *dev)
 	}
 }
 
-static int ipoib_mcast_addr_is_valid(const u8 *addr, unsigned int addrlen,
-				     const u8 *broadcast)
+static int ipoib_mcast_addr_is_valid(const u8 *addr, const u8 *broadcast)
 {
-	if (addrlen != INFINIBAND_ALEN)
-		return 0;
 	/* reserved QPN, prefix, scope */
 	if (memcmp(addr, broadcast, 6))
 		return 0;
@@ -811,11 +808,10 @@ void ipoib_mcast_restart_task(struct work_struct *work)
 		clear_bit(IPOIB_MCAST_FLAG_FOUND, &mcast->flags);
 
 	/* Mark all of the entries that are found or don't exist */
-	for (mclist = dev->mc_list; mclist; mclist = mclist->next) {
+	netdev_for_each_mc_addr(mclist, dev) {
 		union ib_gid mgid;
 
 		if (!ipoib_mcast_addr_is_valid(mclist->dmi_addr,
-					       mclist->dmi_addrlen,
 					       dev->broadcast))
 			continue;
 
-- 
1.6.6

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

* RE: [net-next-2.6 PATCH] infiniband: convert to use netdev_for_each_mc_addr
       [not found] ` <20100224151108.GD2663-YzwxZg+R7evMbnheQZGK0N5OCZ2W11yPFxja6HXR22MAvxtiuMwx3w@public.gmane.org>
@ 2010-02-24 19:46   ` Tung, Chien Tin
       [not found]     ` <603F8A3875DCE940BA37B49D0A6EA0AE842606EE-uLM7Qlg6Mbekrb+BlOpmy7fspsVTdybXVpNB7YpNyf8@public.gmane.org>
  2010-02-25  8:00   ` Or Gerlitz
  2010-02-26 12:22   ` David Miller
  2 siblings, 1 reply; 15+ messages in thread
From: Tung, Chien Tin @ 2010-02-24 19:46 UTC (permalink / raw)
  To: Jiri Pirko, netdev-u79uwXL29TY76Z2rM5mHXA
  Cc: davem-fT/PcQaiUtIeIZ0/mPfg9Q, Latif, Faisal,
	linux-rdma-u79uwXL29TY76Z2rM5mHXA

Hi Jiri,

I am having trouble applying your patch.  It is erroring out on hunk #5.
Current nes_nic.c does not have this line:

>-		  netdev_mc_count(netdev), !!(netdev->flags & IFF_PROMISC),

Did I miss an old patch?

Chien

For reference:

>diff --git a/drivers/infiniband/hw/nest/nes_nic.c b/drivers/infiniband/hw/nest/nes_nic.c
>index c04f8fc..9384f5d 100644
>--- a/drivers/infiniband/hw/nes/nes_nic.c
>+++ b/drivers/infiniband/hw/nest/nes_nic.c
[...]
>@@ -862,19 +871,30 @@ static void nes_netdev_set_multicast_list(struct net_device *netdev)
> 	}
>
> 	nes_debug(NES_DBG_NIC_RX, "Number of MC entries = %d, Promiscous = %d, All Multicast = %d.\n",
>-		  netdev_mc_count(netdev), !!(netdev->flags & IFF_PROMISC),
>+		  mc_count, !!(netdev->flags & IFF_PROMISC),
> 		  !!(netdev->flags & IFF_ALLMULTI));
> 	if (!mc_all_on) {
>-		multicast_addr = netdev->mc_list;
>+		char *addrs;
>+		int i;
>+		struct dev_mc_list *mcaddr;
>+
>+		addrs = kmalloc(ETH_ALEN * mc_count, GFP_ATOMIC);
>+		if (!addrs) {
>+			set_allmulti(nesdev, nic_active_bit);
>+			goto unlock;
>+		}
>+		i = 0;
>+		netdev_for_each_mc_addr(mcaddr, netdev)
>+			memcpy(get_addr(addrs, i++),
>+			       mcaddr->dmi_addr, ETH_ALEN);
>+
> 		perfect_filter_register_address = NES_IDX_PERFECT_FILTER_LOW +
> 						pft_entries_preallocated * 0x8;
>-		for (mc_index = 0; mc_index < max_pft_entries_avaiable;
>-		mc_index++) {
>-			while (multicast_addr && nesvnic->mcrq_mcast_filter &&
>+		for (i = 0, mc_index = 0; mc_index < max_pft_entries_avaiable;
>+		     mc_index++) {
>+			while (i < mc_count && nesvnic->mcrq_mcast_filter &&
> 			((mc_nic_index = nesvnic->mcrq_mcast_filter(nesvnic,
>-					multicast_addr->dmi_addr)) == 0)) {
>-				multicast_addr = multicast_addr->next;
>-			}
>+					get_addr(addrs, i++))) == 0));
> 			if (mc_nic_index < 0)
> 				mc_nic_index = nesvnic->nic_index;
> 			while (nesadapter->pft_mcast_map[mc_index] < 16 &&
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

* Re: [net-next-2.6 PATCH] infiniband: convert to use netdev_for_each_mc_addr
       [not found]     ` <603F8A3875DCE940BA37B49D0A6EA0AE842606EE-uLM7Qlg6Mbekrb+BlOpmy7fspsVTdybXVpNB7YpNyf8@public.gmane.org>
@ 2010-02-24 20:18       ` Jiri Pirko
  0 siblings, 0 replies; 15+ messages in thread
From: Jiri Pirko @ 2010-02-24 20:18 UTC (permalink / raw)
  To: Tung, Chien Tin
  Cc: netdev-u79uwXL29TY76Z2rM5mHXA, davem-fT/PcQaiUtIeIZ0/mPfg9Q,
	Latif, Faisal, linux-rdma-u79uwXL29TY76Z2rM5mHXA

Wed, Feb 24, 2010 at 08:46:24PM CET, chien.tin.tung-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org wrote:
>Hi Jiri,
>
>I am having trouble applying your patch.  It is erroring out on hunk #5.
>Current nes_nic.c does not have this line:
>
>>-		  netdev_mc_count(netdev), !!(netdev->flags & IFF_PROMISC),

This is present in current net-next tree.

Jirka

>
>Did I miss an old patch?
>
>Chien
>
>For reference:
>
>>diff --git a/drivers/infiniband/hw/nest/nes_nic.c b/drivers/infiniband/hw/nest/nes_nic.c
>>index c04f8fc..9384f5d 100644
>>--- a/drivers/infiniband/hw/nes/nes_nic.c
>>+++ b/drivers/infiniband/hw/nest/nes_nic.c
>[...]
>>@@ -862,19 +871,30 @@ static void nes_netdev_set_multicast_list(struct net_device *netdev)
>> 	}
>>
>> 	nes_debug(NES_DBG_NIC_RX, "Number of MC entries = %d, Promiscous = %d, All Multicast = %d.\n",
>>-		  netdev_mc_count(netdev), !!(netdev->flags & IFF_PROMISC),
>>+		  mc_count, !!(netdev->flags & IFF_PROMISC),
>> 		  !!(netdev->flags & IFF_ALLMULTI));
>> 	if (!mc_all_on) {
>>-		multicast_addr = netdev->mc_list;
>>+		char *addrs;
>>+		int i;
>>+		struct dev_mc_list *mcaddr;
>>+
>>+		addrs = kmalloc(ETH_ALEN * mc_count, GFP_ATOMIC);
>>+		if (!addrs) {
>>+			set_allmulti(nesdev, nic_active_bit);
>>+			goto unlock;
>>+		}
>>+		i = 0;
>>+		netdev_for_each_mc_addr(mcaddr, netdev)
>>+			memcpy(get_addr(addrs, i++),
>>+			       mcaddr->dmi_addr, ETH_ALEN);
>>+
>> 		perfect_filter_register_address = NES_IDX_PERFECT_FILTER_LOW +
>> 						pft_entries_preallocated * 0x8;
>>-		for (mc_index = 0; mc_index < max_pft_entries_avaiable;
>>-		mc_index++) {
>>-			while (multicast_addr && nesvnic->mcrq_mcast_filter &&
>>+		for (i = 0, mc_index = 0; mc_index < max_pft_entries_avaiable;
>>+		     mc_index++) {
>>+			while (i < mc_count && nesvnic->mcrq_mcast_filter &&
>> 			((mc_nic_index = nesvnic->mcrq_mcast_filter(nesvnic,
>>-					multicast_addr->dmi_addr)) == 0)) {
>>-				multicast_addr = multicast_addr->next;
>>-			}
>>+					get_addr(addrs, i++))) == 0));
>> 			if (mc_nic_index < 0)
>> 				mc_nic_index = nesvnic->nic_index;
>> 			while (nesadapter->pft_mcast_map[mc_index] < 16 &&
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

* Re: [net-next-2.6 PATCH] infiniband: convert to use netdev_for_each_mc_addr
       [not found] ` <20100224151108.GD2663-YzwxZg+R7evMbnheQZGK0N5OCZ2W11yPFxja6HXR22MAvxtiuMwx3w@public.gmane.org>
  2010-02-24 19:46   ` Tung, Chien Tin
@ 2010-02-25  8:00   ` Or Gerlitz
       [not found]     ` <4B862E07.7020002-hKgKHo2Ms0FWk0Htik3J/w@public.gmane.org>
  2010-02-26 12:22   ` David Miller
  2 siblings, 1 reply; 15+ messages in thread
From: Or Gerlitz @ 2010-02-25  8:00 UTC (permalink / raw)
  To: Jiri Pirko
  Cc: netdev-u79uwXL29TY76Z2rM5mHXA, Jason Gunthorpe,
	linux-rdma-u79uwXL29TY76Z2rM5mHXA

Jiri Pirko wrote:
> --- a/drivers/infiniband/ulp/ipoib/ipoib_multicast.c
> +++ b/drivers/infiniband/ulp/ipoib/ipoib_multicast.c
> @@ -767,11 +767,8 @@ void ipoib_mcast_dev_flush(struct net_device *dev)
> -static int ipoib_mcast_addr_is_valid(const u8 *addr, unsigned int addrlen,
> -				     const u8 *broadcast)
> +static int ipoib_mcast_addr_is_valid(const u8 *addr, const u8 *broadcast)
>  {
> -	if (addrlen != INFINIBAND_ALEN)
> -		return 0;

This check was added by commit 5e47596b "IPoIB: Check multicast address format", may I ask what is the reason for removing it now?

Or.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

* Re: [net-next-2.6 PATCH] infiniband: convert to use netdev_for_each_mc_addr
       [not found]     ` <4B862E07.7020002-hKgKHo2Ms0FWk0Htik3J/w@public.gmane.org>
@ 2010-02-25  8:49       ` Jiri Pirko
  2010-02-26 12:22         ` David Miller
       [not found]         ` <20100225084915.GA3171-YzwxZg+R7et1/kRsl7OVgNvLeJWuRmrY@public.gmane.org>
  0 siblings, 2 replies; 15+ messages in thread
From: Jiri Pirko @ 2010-02-25  8:49 UTC (permalink / raw)
  To: Or Gerlitz
  Cc: netdev-u79uwXL29TY76Z2rM5mHXA, Jason Gunthorpe,
	linux-rdma-u79uwXL29TY76Z2rM5mHXA, davem-fT/PcQaiUtIeIZ0/mPfg9Q

Thu, Feb 25, 2010 at 09:00:07AM CET, ogerlitz-smomgflXvOZWk0Htik3J/w@public.gmane.org wrote:
>Jiri Pirko wrote:
>> --- a/drivers/infiniband/ulp/ipoib/ipoib_multicast.c
>> +++ b/drivers/infiniband/ulp/ipoib/ipoib_multicast.c
>> @@ -767,11 +767,8 @@ void ipoib_mcast_dev_flush(struct net_device *dev)
>> -static int ipoib_mcast_addr_is_valid(const u8 *addr, unsigned int addrlen,
>> -				     const u8 *broadcast)
>> +static int ipoib_mcast_addr_is_valid(const u8 *addr, const u8 *broadcast)
>>  {
>> -	if (addrlen != INFINIBAND_ALEN)
>> -		return 0;
>
>This check was added by commit 5e47596b "IPoIB: Check multicast address format", may I ask what is the reason for removing it now?

Yes, at this very moment the check is not needless but it will be in a brief
future. dev_mc_add will look very similar like dev_unicast_add. But ok. Here's
patch adding the check in dev_mc_add right now to correct this state. Thanks Or.

Subject: [net-next-2.6 PATCH] net: add addr len check to dev_mc_add

Signed-off-by: Jiri Pirko <jpirko-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

diff --git a/net/core/dev_mcast.c b/net/core/dev_mcast.c
index 9e2fa39..fd91569 100644
--- a/net/core/dev_mcast.c
+++ b/net/core/dev_mcast.c
@@ -96,6 +96,8 @@ int dev_mc_add(struct net_device *dev, void *addr, int alen, int glbl)
 	int err;
 
 	netif_addr_lock_bh(dev);
+	if (alen != dev->addr_len)
+		return -EINVAL;
 	err = __dev_addr_add(&dev->mc_list, &dev->mc_count, addr, alen, glbl);
 	if (!err)
 		__dev_set_rx_mode(dev);
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

* Re: [net-next-2.6 PATCH] infiniband: convert to use netdev_for_each_mc_addr
  2010-02-25  8:49       ` Jiri Pirko
@ 2010-02-26 12:22         ` David Miller
       [not found]         ` <20100225084915.GA3171-YzwxZg+R7et1/kRsl7OVgNvLeJWuRmrY@public.gmane.org>
  1 sibling, 0 replies; 15+ messages in thread
From: David Miller @ 2010-02-26 12:22 UTC (permalink / raw)
  To: jpirko; +Cc: ogerlitz, netdev, jgunthorpe, linux-rdma

From: Jiri Pirko <jpirko@redhat.com>
Date: Thu, 25 Feb 2010 09:49:15 +0100

> Subject: [net-next-2.6 PATCH] net: add addr len check to dev_mc_add
> 
> Signed-off-by: Jiri Pirko <jpirko@redhat.com>

Applied.

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

* Re: [net-next-2.6 PATCH] infiniband: convert to use netdev_for_each_mc_addr
       [not found] ` <20100224151108.GD2663-YzwxZg+R7evMbnheQZGK0N5OCZ2W11yPFxja6HXR22MAvxtiuMwx3w@public.gmane.org>
  2010-02-24 19:46   ` Tung, Chien Tin
  2010-02-25  8:00   ` Or Gerlitz
@ 2010-02-26 12:22   ` David Miller
  2 siblings, 0 replies; 15+ messages in thread
From: David Miller @ 2010-02-26 12:22 UTC (permalink / raw)
  To: jpirko-H+wXaHxf7aLQT0dZR+AlfA
  Cc: netdev-u79uwXL29TY76Z2rM5mHXA,
	faisal.latif-ral2JQCrhuEAvxtiuMwx3w,
	chien.tin.tung-ral2JQCrhuEAvxtiuMwx3w,
	linux-rdma-u79uwXL29TY76Z2rM5mHXA

From: Jiri Pirko <jpirko-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Date: Wed, 24 Feb 2010 16:11:08 +0100

> Due to the loop complexicity in nes_nic.c, I'm using char* to copy mc addresses
> to it.
> 
> Signed-off-by: Jiri Pirko <jpirko-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

Applied.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

* Re: [net-next-2.6 PATCH] infiniband: convert to use netdev_for_each_mc_addr
       [not found]         ` <20100225084915.GA3171-YzwxZg+R7et1/kRsl7OVgNvLeJWuRmrY@public.gmane.org>
@ 2010-02-27  6:01           ` Jason Gunthorpe
       [not found]             ` <20100227060146.GA13231-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
  0 siblings, 1 reply; 15+ messages in thread
From: Jason Gunthorpe @ 2010-02-27  6:01 UTC (permalink / raw)
  To: Jiri Pirko
  Cc: Or Gerlitz, netdev-u79uwXL29TY76Z2rM5mHXA,
	linux-rdma-u79uwXL29TY76Z2rM5mHXA, davem-fT/PcQaiUtIeIZ0/mPfg9Q

On Thu, Feb 25, 2010 at 09:49:15AM +0100, Jiri Pirko wrote:
> Thu, Feb 25, 2010 at 09:00:07AM CET, ogerlitz-smomgflXvOZWk0Htik3J/w@public.gmane.org wrote:
> >Jiri Pirko wrote:
> >> +++ b/drivers/infiniband/ulp/ipoib/ipoib_multicast.c
> >> @@ -767,11 +767,8 @@ void ipoib_mcast_dev_flush(struct net_device *dev)
> >> -static int ipoib_mcast_addr_is_valid(const u8 *addr, unsigned int addrlen,
> >> -				     const u8 *broadcast)
> >> +static int ipoib_mcast_addr_is_valid(const u8 *addr, const u8 *broadcast)
> >>  {
> >> -	if (addrlen != INFINIBAND_ALEN)
> >> -		return 0;
> >
> >This check was added by commit 5e47596b "IPoIB: Check multicast address format", may I ask what is the reason for removing it now?
> 
> Yes, at this very moment the check is not needless but it will be in a brief
> future. dev_mc_add will look very similar like dev_unicast_add. But ok. Here's
> patch adding the check in dev_mc_add right now to correct this state. Thanks Or.

Is this enough?

The problem this statement is trying to solve had to do with bonding
creating multicast addresess for ethernet rather than infiniband in
some cases. This happens because bonding makes a device that switches
from ethernet to infiniband during its lifetime. I'm not quite
sure what kind of life cycle those addresses go through, but if they
somehow stay on the mc_list then ipoib_mcast_addr_is_valid will still
need to have the check.

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

* Re: [net-next-2.6 PATCH] infiniband: convert to use netdev_for_each_mc_addr
       [not found]             ` <20100227060146.GA13231-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
@ 2010-02-27 10:38               ` Jiri Pirko
  2010-02-27 10:44                 ` David Miller
       [not found]                 ` <20100227103835.GA2862-YzwxZg+R7et1/kRsl7OVgNvLeJWuRmrY@public.gmane.org>
  0 siblings, 2 replies; 15+ messages in thread
From: Jiri Pirko @ 2010-02-27 10:38 UTC (permalink / raw)
  To: Jason Gunthorpe
  Cc: Or Gerlitz, netdev-u79uwXL29TY76Z2rM5mHXA,
	linux-rdma-u79uwXL29TY76Z2rM5mHXA, davem-fT/PcQaiUtIeIZ0/mPfg9Q

Sat, Feb 27, 2010 at 07:01:46AM CET, jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org wrote:
>On Thu, Feb 25, 2010 at 09:49:15AM +0100, Jiri Pirko wrote:
>> Thu, Feb 25, 2010 at 09:00:07AM CET, ogerlitz-smomgflXvOZWk0Htik3J/w@public.gmane.org wrote:
>> >Jiri Pirko wrote:
>> >> +++ b/drivers/infiniband/ulp/ipoib/ipoib_multicast.c
>> >> @@ -767,11 +767,8 @@ void ipoib_mcast_dev_flush(struct net_device *dev)
>> >> -static int ipoib_mcast_addr_is_valid(const u8 *addr, unsigned int addrlen,
>> >> -				     const u8 *broadcast)
>> >> +static int ipoib_mcast_addr_is_valid(const u8 *addr, const u8 *broadcast)
>> >>  {
>> >> -	if (addrlen != INFINIBAND_ALEN)
>> >> -		return 0;
>> >
>> >This check was added by commit 5e47596b "IPoIB: Check multicast address format", may I ask what is the reason for removing it now?
>> 
>> Yes, at this very moment the check is not needless but it will be in a brief
>> future. dev_mc_add will look very similar like dev_unicast_add. But ok. Here's
>> patch adding the check in dev_mc_add right now to correct this state. Thanks Or.
>
>Is this enough?
>
>The problem this statement is trying to solve had to do with bonding
>creating multicast addresess for ethernet rather than infiniband in
>some cases. This happens because bonding makes a device that switches
>from ethernet to infiniband during its lifetime. I'm not quite
>sure what kind of life cycle those addresses go through, but if they
>somehow stay on the mc_list then ipoib_mcast_addr_is_valid will still
>need to have the check.
>

Ok I see your point now. But in this case, the length check should be in all
drivers because if for example bonding device changes from infiniband back to
eth, the addresses stored would be "cut" to 6 bytes and would make no sense.

I see two solutions:
1) check the length in all drivers to ensure correctness.
2) when bonding changes it's type, flush mc addresses and start over.

Second option looks nicer to me, but I might be missing something, thouths?

Thanks

Jirka

>Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

* Re: [net-next-2.6 PATCH] infiniband: convert to use netdev_for_each_mc_addr
  2010-02-27 10:38               ` Jiri Pirko
@ 2010-02-27 10:44                 ` David Miller
       [not found]                 ` <20100227103835.GA2862-YzwxZg+R7et1/kRsl7OVgNvLeJWuRmrY@public.gmane.org>
  1 sibling, 0 replies; 15+ messages in thread
From: David Miller @ 2010-02-27 10:44 UTC (permalink / raw)
  To: jpirko; +Cc: jgunthorpe, ogerlitz, netdev, linux-rdma

From: Jiri Pirko <jpirko@redhat.com>
Date: Sat, 27 Feb 2010 11:38:37 +0100

> I see two solutions:
> 1) check the length in all drivers to ensure correctness.
> 2) when bonding changes it's type, flush mc addresses and start over.
> 
> Second option looks nicer to me, but I might be missing something, thouths?

If bonding is causing this problem, perhaps we should make bonding
go through a programmatic interface to do it's work so that the
check can be done generically and in a place that will get
caught during code changes and audits.

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

* Re: [net-next-2.6 PATCH] infiniband: convert to use netdev_for_each_mc_addr
       [not found]                 ` <20100227103835.GA2862-YzwxZg+R7et1/kRsl7OVgNvLeJWuRmrY@public.gmane.org>
@ 2010-02-27 18:44                   ` Jason Gunthorpe
       [not found]                     ` <20100227184410.GB13231-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
  0 siblings, 1 reply; 15+ messages in thread
From: Jason Gunthorpe @ 2010-02-27 18:44 UTC (permalink / raw)
  To: Jiri Pirko
  Cc: Or Gerlitz, netdev-u79uwXL29TY76Z2rM5mHXA,
	linux-rdma-u79uwXL29TY76Z2rM5mHXA, davem-fT/PcQaiUtIeIZ0/mPfg9Q,
	Moni Shoua

On Sat, Feb 27, 2010 at 11:38:37AM +0100, Jiri Pirko wrote:

> >The problem this statement is trying to solve had to do with bonding
> >creating multicast addresess for ethernet rather than infiniband in
> >some cases. This happens because bonding makes a device that switches
> >from ethernet to infiniband during its lifetime. I'm not quite
> >sure what kind of life cycle those addresses go through, but if they
> >somehow stay on the mc_list then ipoib_mcast_addr_is_valid will still
> >need to have the check.

> Ok I see your point now. But in this case, the length check should be in all
> drivers because if for example bonding device changes from infiniband back to
> eth, the addresses stored would be "cut" to 6 bytes and would make no sense.

Ideally, but the other wrinkle is due to other problems in the ipoib
driver one bad multicast address blocks further addresses from being
processed - effectively rendering the interface useless for
multicast. Ethernet drivers will just subscribe to a bogus address, no
real harm done.

I would prefer to see the check remain in ipoib until we can see
through testing that it is no longer triggering.

> 2) when bonding changes it's type, flush mc addresses and start over.
> 
> Second option looks nicer to me, but I might be missing something, thouths?

There was a patch posted that tried to do something like what you are
describing:
From: Moni Shoua <monis-hKgKHo2Ms0F+cjeuK/JdrQ@public.gmane.org>
Subject: Re: [PATCH RESEND] bonding: remap muticast addresses without using dev_close() and dev_open()

Thanks,
Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

* Re: [net-next-2.6 PATCH] infiniband: convert to use netdev_for_each_mc_addr
       [not found]                     ` <20100227184410.GB13231-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
@ 2010-02-27 22:07                       ` Jiri Pirko
  2010-02-28  9:33                         ` David Miller
  2010-03-01  7:48                       ` Or Gerlitz
  1 sibling, 1 reply; 15+ messages in thread
From: Jiri Pirko @ 2010-02-27 22:07 UTC (permalink / raw)
  To: Jason Gunthorpe
  Cc: Or Gerlitz, netdev-u79uwXL29TY76Z2rM5mHXA,
	linux-rdma-u79uwXL29TY76Z2rM5mHXA, davem-fT/PcQaiUtIeIZ0/mPfg9Q,
	Moni Shoua

Sat, Feb 27, 2010 at 07:44:10PM CET, jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org wrote:
>On Sat, Feb 27, 2010 at 11:38:37AM +0100, Jiri Pirko wrote:
>
>> >The problem this statement is trying to solve had to do with bonding
>> >creating multicast addresess for ethernet rather than infiniband in
>> >some cases. This happens because bonding makes a device that switches
>> >from ethernet to infiniband during its lifetime. I'm not quite
>> >sure what kind of life cycle those addresses go through, but if they
>> >somehow stay on the mc_list then ipoib_mcast_addr_is_valid will still
>> >need to have the check.
>
>> Ok I see your point now. But in this case, the length check should be in all
>> drivers because if for example bonding device changes from infiniband back to
>> eth, the addresses stored would be "cut" to 6 bytes and would make no sense.
>
>Ideally, but the other wrinkle is due to other problems in the ipoib
>driver one bad multicast address blocks further addresses from being
>processed - effectively rendering the interface useless for
>multicast. Ethernet drivers will just subscribe to a bogus address, no
>real harm done.
>
>I would prefer to see the check remain in ipoib until we can see
>through testing that it is no longer triggering.

Fair enough.

Subject: [net-next-2.6 PATCH] ipoib: returned back addrle check for mc addresses

Apparently bogus mc address can brake IPOIB multicast processing. Therefore
returning the check for addrlen back until this is resolved in bonding (I don't
see any other point from where mc address with non-dev->addr_len length can came
from).

Signed-off-by: Jiri Pirko <jpirko-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

diff --git a/drivers/infiniband/ulp/ipoib/ipoib_multicast.c b/drivers/infiniband/ulp/ipoib/ipoib_multicast.c
index 19eba3c..d41ea27 100644
--- a/drivers/infiniband/ulp/ipoib/ipoib_multicast.c
+++ b/drivers/infiniband/ulp/ipoib/ipoib_multicast.c
@@ -767,8 +767,11 @@ void ipoib_mcast_dev_flush(struct net_device *dev)
 	}
 }
 
-static int ipoib_mcast_addr_is_valid(const u8 *addr, const u8 *broadcast)
+static int ipoib_mcast_addr_is_valid(const u8 *addr, unsigned int addrlen,
+				     const u8 *broadcast)
 {
+	if (addrlen != INFINIBAND_ALEN)
+		return 0;
 	/* reserved QPN, prefix, scope */
 	if (memcmp(addr, broadcast, 6))
 		return 0;
@@ -812,6 +815,7 @@ void ipoib_mcast_restart_task(struct work_struct *work)
 		union ib_gid mgid;
 
 		if (!ipoib_mcast_addr_is_valid(mclist->dmi_addr,
+					       mclist->dmi_addrlen,
 					       dev->broadcast))
 			continue;
 
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

* Re: [net-next-2.6 PATCH] infiniband: convert to use netdev_for_each_mc_addr
  2010-02-27 22:07                       ` Jiri Pirko
@ 2010-02-28  9:33                         ` David Miller
  0 siblings, 0 replies; 15+ messages in thread
From: David Miller @ 2010-02-28  9:33 UTC (permalink / raw)
  To: jpirko; +Cc: jgunthorpe, ogerlitz, netdev, linux-rdma, monis

From: Jiri Pirko <jpirko@redhat.com>
Date: Sat, 27 Feb 2010 23:07:33 +0100

> Subject: [net-next-2.6 PATCH] ipoib: returned back addrle check for mc addresses
> 
> Apparently bogus mc address can brake IPOIB multicast processing. Therefore
> returning the check for addrlen back until this is resolved in bonding (I don't
> see any other point from where mc address with non-dev->addr_len length can came
> from).
> 
> Signed-off-by: Jiri Pirko <jpirko@redhat.com>

Applied.

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

* Re: [net-next-2.6 PATCH] infiniband: convert to use netdev_for_each_mc_addr
       [not found]                     ` <20100227184410.GB13231-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
  2010-02-27 22:07                       ` Jiri Pirko
@ 2010-03-01  7:48                       ` Or Gerlitz
       [not found]                         ` <4B8B7139.9050707-hKgKHo2Ms0FWk0Htik3J/w@public.gmane.org>
  1 sibling, 1 reply; 15+ messages in thread
From: Or Gerlitz @ 2010-03-01  7:48 UTC (permalink / raw)
  To: Jason Gunthorpe
  Cc: Jiri Pirko, netdev-u79uwXL29TY76Z2rM5mHXA,
	linux-rdma-u79uwXL29TY76Z2rM5mHXA, Moni Shoua

Jason Gunthorpe wrote:
> Jiri Pirko wrote:
>> when bonding changes it's type, flush mc addresses and start over.

> There was a patch posted that tried to do something like what you are describing

Indeed, Jason, commit 75c785 "bonding: remap muticast addresses without using dev_close() and dev_open()" from Moni Shoua <monis-smomgflXvOZWk0Htik3J/w@public.gmane.org> does exactly that, isn't it?

Or.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

* Re: [net-next-2.6 PATCH] infiniband: convert to use netdev_for_each_mc_addr
       [not found]                         ` <4B8B7139.9050707-hKgKHo2Ms0FWk0Htik3J/w@public.gmane.org>
@ 2010-03-02 12:01                           ` Jiri Pirko
  0 siblings, 0 replies; 15+ messages in thread
From: Jiri Pirko @ 2010-03-02 12:01 UTC (permalink / raw)
  To: Or Gerlitz
  Cc: Jason Gunthorpe, netdev-u79uwXL29TY76Z2rM5mHXA,
	linux-rdma-u79uwXL29TY76Z2rM5mHXA, Moni Shoua

Mon, Mar 01, 2010 at 08:48:09AM CET, ogerlitz-smomgflXvOZWk0Htik3J/w@public.gmane.org wrote:
>Jason Gunthorpe wrote:
>> Jiri Pirko wrote:
>>> when bonding changes it's type, flush mc addresses and start over.
>
>> There was a patch posted that tried to do something like what you are describing
>
>Indeed, Jason, commit 75c785 "bonding: remap muticast addresses without using dev_close() and dev_open()" from Moni Shoua <monis-smomgflXvOZWk0Htik3J/w@public.gmane.org> does exactly that, isn't it?

This affects different mc_list.

>
>Or.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

end of thread, other threads:[~2010-03-02 12:01 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2010-02-24 15:11 [net-next-2.6 PATCH] infiniband: convert to use netdev_for_each_mc_addr Jiri Pirko
     [not found] ` <20100224151108.GD2663-YzwxZg+R7evMbnheQZGK0N5OCZ2W11yPFxja6HXR22MAvxtiuMwx3w@public.gmane.org>
2010-02-24 19:46   ` Tung, Chien Tin
     [not found]     ` <603F8A3875DCE940BA37B49D0A6EA0AE842606EE-uLM7Qlg6Mbekrb+BlOpmy7fspsVTdybXVpNB7YpNyf8@public.gmane.org>
2010-02-24 20:18       ` Jiri Pirko
2010-02-25  8:00   ` Or Gerlitz
     [not found]     ` <4B862E07.7020002-hKgKHo2Ms0FWk0Htik3J/w@public.gmane.org>
2010-02-25  8:49       ` Jiri Pirko
2010-02-26 12:22         ` David Miller
     [not found]         ` <20100225084915.GA3171-YzwxZg+R7et1/kRsl7OVgNvLeJWuRmrY@public.gmane.org>
2010-02-27  6:01           ` Jason Gunthorpe
     [not found]             ` <20100227060146.GA13231-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
2010-02-27 10:38               ` Jiri Pirko
2010-02-27 10:44                 ` David Miller
     [not found]                 ` <20100227103835.GA2862-YzwxZg+R7et1/kRsl7OVgNvLeJWuRmrY@public.gmane.org>
2010-02-27 18:44                   ` Jason Gunthorpe
     [not found]                     ` <20100227184410.GB13231-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
2010-02-27 22:07                       ` Jiri Pirko
2010-02-28  9:33                         ` David Miller
2010-03-01  7:48                       ` Or Gerlitz
     [not found]                         ` <4B8B7139.9050707-hKgKHo2Ms0FWk0Htik3J/w@public.gmane.org>
2010-03-02 12:01                           ` Jiri Pirko
2010-02-26 12:22   ` David Miller

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.