[PATCH 0/3] ehea fixes for 3.4

[PATCH 0/3] ehea fixes for 3.4

[Thadeu Lima de Souza Cascardo]

Hi, Dave.

These are bug fixes for 3.4, including a crash and bad behaviour with
promiscuous and multicast.

Thanks.
Cascardo.

Thadeu Lima de Souza Cascardo (3):
  ehea: fix allmulticast support
  ehea: fix promiscuous mode
  ehea: do not dereference possible NULL port

 drivers/net/ethernet/ibm/ehea/ehea_main.c |   43 +++++++++++++++-------------
 drivers/net/ethernet/ibm/ehea/ehea_phyp.h |    2 +-
 2 files changed, 24 insertions(+), 21 deletions(-)

-- 
1.7.4.4

[PATCH 1/3] ehea: fix allmulticast support

[Thadeu Lima de Souza Cascardo]

There was a bug in the mask of regtype parameter for registering a
multicast filter. It was ignoring the scope bit, which was wrongly being
used for all filters. The SCOPE_ALL value adds a filter that allows all
multicast packets and ignores the MAC parameter, just what allmulticast
needs. The normals filters, however, should not use SCOPE_ALL.

Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@linux.vnet.ibm.com>
---
 drivers/net/ethernet/ibm/ehea/ehea_main.c |   24 +++++++++++++++---------
 drivers/net/ethernet/ibm/ehea/ehea_phyp.h |    2 +-
 2 files changed, 16 insertions(+), 10 deletions(-)

diff --git a/drivers/net/ethernet/ibm/ehea/ehea_main.c b/drivers/net/ethernet/ibm/ehea/ehea_main.c
index 3516e17..25d1258 100644
--- a/drivers/net/ethernet/ibm/ehea/ehea_main.c
+++ b/drivers/net/ethernet/ibm/ehea/ehea_main.c
@@ -290,16 +290,18 @@ static void ehea_update_bcmc_registrations(void)
 
 				arr[i].adh = adapter->handle;
 				arr[i].port_id = port->logical_port_id;
-				arr[i].reg_type = EHEA_BCMC_SCOPE_ALL |
-						  EHEA_BCMC_MULTICAST |
+				arr[i].reg_type = EHEA_BCMC_MULTICAST |
 						  EHEA_BCMC_UNTAGGED;
+				if (mc_entry->macaddr == 0)
+					arr[i].reg_type |= EHEA_BCMC_SCOPE_ALL;
 				arr[i++].macaddr = mc_entry->macaddr;
 
 				arr[i].adh = adapter->handle;
 				arr[i].port_id = port->logical_port_id;
-				arr[i].reg_type = EHEA_BCMC_SCOPE_ALL |
-						  EHEA_BCMC_MULTICAST |
+				arr[i].reg_type = EHEA_BCMC_MULTICAST |
 						  EHEA_BCMC_VLANID_ALL;
+				if (mc_entry->macaddr == 0)
+					arr[i].reg_type |= EHEA_BCMC_SCOPE_ALL;
 				arr[i++].macaddr = mc_entry->macaddr;
 				num_registrations -= 2;
 			}
@@ -1838,8 +1840,9 @@ static u64 ehea_multicast_reg_helper(struct ehea_port *port, u64 mc_mac_addr,
 	u64 hret;
 	u8 reg_type;
 
-	reg_type = EHEA_BCMC_SCOPE_ALL | EHEA_BCMC_MULTICAST
-		 | EHEA_BCMC_UNTAGGED;
+	reg_type = EHEA_BCMC_MULTICAST | EHEA_BCMC_UNTAGGED;
+	if (mc_mac_addr == 0)
+		reg_type |= EHEA_BCMC_SCOPE_ALL;
 
 	hret = ehea_h_reg_dereg_bcmc(port->adapter->handle,
 				     port->logical_port_id,
@@ -1847,8 +1850,9 @@ static u64 ehea_multicast_reg_helper(struct ehea_port *port, u64 mc_mac_addr,
 	if (hret)
 		goto out;
 
-	reg_type = EHEA_BCMC_SCOPE_ALL | EHEA_BCMC_MULTICAST
-		 | EHEA_BCMC_VLANID_ALL;
+	reg_type = EHEA_BCMC_MULTICAST | EHEA_BCMC_VLANID_ALL;
+	if (mc_mac_addr == 0)
+		reg_type |= EHEA_BCMC_SCOPE_ALL;
 
 	hret = ehea_h_reg_dereg_bcmc(port->adapter->handle,
 				     port->logical_port_id,
@@ -1898,7 +1902,7 @@ static void ehea_allmulti(struct net_device *dev, int enable)
 				netdev_err(dev,
 					   "failed enabling IFF_ALLMULTI\n");
 		}
-	} else
+	} else {
 		if (!enable) {
 			/* Disable ALLMULTI */
 			hret = ehea_multicast_reg_helper(port, 0, H_DEREG_BCMC);
@@ -1908,6 +1912,7 @@ static void ehea_allmulti(struct net_device *dev, int enable)
 				netdev_err(dev,
 					   "failed disabling IFF_ALLMULTI\n");
 		}
+	}
 }
 
 static void ehea_add_multicast_entry(struct ehea_port *port, u8 *mc_mac_addr)
@@ -2463,6 +2468,7 @@ static int ehea_down(struct net_device *dev)
 		return 0;
 
 	ehea_drop_multicast_list(dev);
+	ehea_allmulti(dev, 0);
 	ehea_broadcast_reg_helper(port, H_DEREG_BCMC);
 
 	ehea_free_interrupts(dev);
diff --git a/drivers/net/ethernet/ibm/ehea/ehea_phyp.h b/drivers/net/ethernet/ibm/ehea/ehea_phyp.h
index 52c456e..8364815 100644
--- a/drivers/net/ethernet/ibm/ehea/ehea_phyp.h
+++ b/drivers/net/ethernet/ibm/ehea/ehea_phyp.h
@@ -450,7 +450,7 @@ u64 ehea_h_modify_ehea_port(const u64 adapter_handle, const u16 port_num,
 			    void *cb_addr);
 
 #define H_REGBCMC_PN            EHEA_BMASK_IBM(48, 63)
-#define H_REGBCMC_REGTYPE       EHEA_BMASK_IBM(61, 63)
+#define H_REGBCMC_REGTYPE       EHEA_BMASK_IBM(60, 63)
 #define H_REGBCMC_MACADDR       EHEA_BMASK_IBM(16, 63)
 #define H_REGBCMC_VLANID        EHEA_BMASK_IBM(52, 63)
 
-- 
1.7.4.4

[PATCH 2/3] ehea: fix promiscuous mode

[Thadeu Lima de Souza Cascardo]

commit a4910b744486254cfa61995954c118fb2283c4fd has broken promiscuous
mode, which is never set. port->promisc just reflects the last setting
of PROMISCUOUS mode to avoid doing an extra hypercall when it's already
set.

However, since it may fail because of hypervisor permissions, we should
still respect the multicast settings and not simply exit after setting
promiscuous mode.

Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@linux.vnet.ibm.com>
Cc: Breno Leitao <leitao@linux.vnet.ibm.com>
---
 drivers/net/ethernet/ibm/ehea/ehea_main.c |    6 +-----
 1 files changed, 1 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/ibm/ehea/ehea_main.c b/drivers/net/ethernet/ibm/ehea/ehea_main.c
index 25d1258..35caeb5 100644
--- a/drivers/net/ethernet/ibm/ehea/ehea_main.c
+++ b/drivers/net/ethernet/ibm/ehea/ehea_main.c
@@ -1946,11 +1946,7 @@ static void ehea_set_multicast_list(struct net_device *dev)
 	struct netdev_hw_addr *ha;
 	int ret;
 
-	if (port->promisc) {
-		ehea_promiscuous(dev, 1);
-		return;
-	}
-	ehea_promiscuous(dev, 0);
+	ehea_promiscuous(dev, !!(dev->flags & IFF_PROMISC));
 
 	if (dev->flags & IFF_ALLMULTI) {
 		ehea_allmulti(dev, 1);
-- 
1.7.4.4

[PATCH 3/3] ehea: do not dereference possible NULL port

[Thadeu Lima de Souza Cascardo]

port may be NULL when we receive an interrupt too early in the probe.

commit 8c4877a4128e7931077b024a891a4b284d8756a3, in particular, has
introduced this problem in the case of port state change interrupt.

This causes crashes in some situations:

[c000000f7ff7fd60] d000000008e223f0 .ehea_neq_tasklet+0x78/0x148 [ehea]
[c000000f7ff7fe00] c0000000000b6cac .tasklet_hi_action+0xdc/0x210
[c000000f7ff7fea0] c0000000000b7cc8 .__do_softirq+0x178/0x300
[c000000f7ff7ff90] c000000000022694 .call_do_softirq+0x14/0x24
[c000000f68ee7900] c000000000010e04 .do_softirq+0xec/0x110
[c000000f68ee79a0] c0000000000b789c .irq_exit+0xac/0xe0
[c000000f68ee7a20] c0000000000110bc .do_IRQ+0x114/0x2a8
[c000000f68ee7ae0] c00000000000553c hardware_interrupt_entry+0x18/0x1c
--- Exception: 501 (Hardware Interrupt) at c00000000000e6bc
.arch_local_irq_rest
ore+0x34/0x48
[link register   ] c000000000017a7c .cpu_idle+0x194/0x2f8
[c000000f68ee7dd0] c000000000017a70 .cpu_idle+0x188/0x2f8 (unreliable)
[c000000f68ee7e90] c00000000066b264 .start_secondary+0x3e4/0x524
[c000000f68ee7f90] c0000000000092e8 .start_secondary_prolog+0x10/0x14

cpu 0x8: Vector: 300 (Data Access) at [c000000f7ff7fa40]
    pc: d000000008e21fac: .ehea_parse_eqe+0x6c/0x438 [ehea]
    lr: d000000008e223f0: .ehea_neq_tasklet+0x78/0x148 [ehea]
    sp: c000000f7ff7fcc0
   msr: 8000000000009032
   dar: 8
 dsisr: 40000000
  current = 0xc000000f68efc0c0
  paca    = 0xc00000000ff41800
    pid   = 0, comm = kworker/0:1

Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@linux.vnet.ibm.com>
Cc: Joe Perches <joe@perches.com>
---
 drivers/net/ethernet/ibm/ehea/ehea_main.c |   13 +++++++------
 1 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/ibm/ehea/ehea_main.c b/drivers/net/ethernet/ibm/ehea/ehea_main.c
index 35caeb5..5e64e66 100644
--- a/drivers/net/ethernet/ibm/ehea/ehea_main.c
+++ b/drivers/net/ethernet/ibm/ehea/ehea_main.c
@@ -1156,21 +1156,22 @@ static void ehea_parse_eqe(struct ehea_adapter *adapter, u64 eqe)
 	u8 ec;
 	u8 portnum;
 	struct ehea_port *port;
-	struct net_device *dev;
+	struct net_device *dev = NULL;
 
 	ec = EHEA_BMASK_GET(NEQE_EVENT_CODE, eqe);
 	portnum = EHEA_BMASK_GET(NEQE_PORTNUM, eqe);
 	port = ehea_get_port(adapter, portnum);
+	if (!port) {
+		pr_err("%s: unknown portnum %d, event %x\n",
+					adapter->ofdev->name, portnum, ec);
+		return;
+	}
+
 	dev = port->netdev;
 
 	switch (ec) {
 	case EHEA_EC_PORTSTATE_CHG:	/* port state change */
 
-		if (!port) {
-			netdev_err(dev, "unknown portnum %x\n", portnum);
-			break;
-		}
-
 		if (EHEA_BMASK_GET(NEQE_PORT_UP, eqe)) {
 			if (!netif_carrier_ok(dev)) {
 				ret = ehea_sense_port_attr(port);
-- 
1.7.4.4

Re: [PATCH 3/3] ehea: do not dereference possible NULL port

[David Miller]

From: Thadeu Lima de Souza Cascardo <cascardo@linux.vnet.ibm.com>
Date: Tue, 17 Apr 2012 11:41:55 -0300

> port may be NULL when we receive an interrupt too early in the probe.
> 
> commit 8c4877a4128e7931077b024a891a4b284d8756a3, in particular, has
> introduced this problem in the case of port state change interrupt.
> 
> This causes crashes in some situations:

Never fix this kind of bug like this.  Do not make a tasklet or
an irq handler pay the price of the probe not doing things in the
right order.

Do not register asynchronous threads of control until all the
state is setup properly.

That's the right way to fix this problem.

Re: [PATCH 3/3] ehea: do not dereference possible NULL port

[Thadeu Lima de Souza Cascardo]

On Tue, Apr 17, 2012 at 10:29:12PM -0400, David Miller wrote:
> From: Thadeu Lima de Souza Cascardo <cascardo@linux.vnet.ibm.com>
> Date: Tue, 17 Apr 2012 11:41:55 -0300
> 
> > port may be NULL when we receive an interrupt too early in the probe.
> > 
> > commit 8c4877a4128e7931077b024a891a4b284d8756a3, in particular, has
> > introduced this problem in the case of port state change interrupt.
> > 
> > This causes crashes in some situations:
> 
> Never fix this kind of bug like this.  Do not make a tasklet or
> an irq handler pay the price of the probe not doing things in the
> right order.
> 
> Do not register asynchronous threads of control until all the
> state is setup properly.
> 
> That's the right way to fix this problem.
> 

Sure. That was in my plans too.

However, at first, I suspected this interrupt may have come from some
other port from ehca, for example, considering a possible firmware bug.
After more investigation, I found out that the real problem was some
physical ports in some odd states, that caused the IRQs before the ports
were enabled. Note that this is not really supposed to happen.

So, consider the possibility that we receive, for a reason like a
firmware bug, an interrupt is received and the EQE indicates a port
number we don't know about. We oops even if we have registered the IRQ
after enabling the ports.

So, I think it's better to be safe to just check for a NULL pointer in
the interrupt (which was being checked either way, just not in the right
place).

So, in fact, I just moved a pointer check because the pointer is now
being used when it wasn't before, due to the commit I referred to.

Would you apply this patch if I include the other one, registering the
IRQ later in the probe process?

Thanks for the comment.
Cascardo.

Re: [PATCH 3/3] ehea: do not dereference possible NULL port

[David Miller]

From: Thadeu Lima de Souza Cascardo <cascardo@linux.vnet.ibm.com>
Date: Wed, 18 Apr 2012 09:42:52 -0300

> However, at first, I suspected this interrupt may have come from some
> other port from ehca, for example, considering a possible firmware bug.
> After more investigation, I found out that the real problem was some
> physical ports in some odd states, that caused the IRQs before the ports
> were enabled. Note that this is not really supposed to happen.

It doesn't matter how it happens from the hardware side.

What matters is that you should not register the IRQ handler
at all until all the datastructures are properly setup.