WiMAX patches for 2.6.33 (batch #4)

PATCH New Inaky Perez-Gonzalez 1 participant 14 messages Nov 4, 2009

Subsystems (2)

Networking Network drivers

lore

Share & tools

lore

Inaky Perez-Gonzalez

author

2009-11-04 21:42 UTC #0 lore raw

cover WiMAX patches for 2.6.33 (batch #4)

[this series of patches has been split in four batches as it was too long; the grouping is just sequential as they are not really related, except for dependencies].

Misc independent small fixes; most relevant are: don't overwrite MAC headers so tcpdump/wireshark can use it, fix of an OOPS during TX shutdown; fix OOPS when exiting USB kthreads.

Please pull from:

git://git.kernel.org/pub/scm/linux/kernel/git/inaky/wimax.git

Patches follow for ease of review.

Cindy H Kao (2):
wimax/i2400m: use JUMP cmd for last FW chunk indication wimax/i2400m: change the bcf_len to exclude the extended header size

Inaky Perez-Gonzalez (11):
wimax/i2400m: reduce verbosity of debug messages in boot mode wimax/i2400m: fix race condition with tcpdump et al
wimax: allow user space to send messages once the device is
registered
wimax: allow WIMAX_RF_QUERY calls when state is still uninitialized wimax/i2400m: when stopping the device, cancel any pending message wimax/i2400m: fix deadlock: don't do BUS reset under

    i2400m->init_mutex

wimax/i2400m: move i2400m_init() out of i2400m.h wimax/i2400m: queue device's report until the driver is ready for
them
wimax/i2400m: fix oops in TX when tearing down the device wimax/i2400m: Let device's status reports change the device state wimax/i2400m: fix oops caused by race condition when exiting USB
kthreads

 drivers/net/wimax/i2400m/driver.c  |  117 +++++++++++-------------------
 drivers/net/wimax/i2400m/fw.c      |   30 +++++---
 drivers/net/wimax/i2400m/i2400m.h  |   48 ++++--------
 drivers/net/wimax/i2400m/netdev.c  |   21 +++++-
 drivers/net/wimax/i2400m/rx.c      |  142 ++++++++++++++++++++++++++++-------
 drivers/net/wimax/i2400m/sdio-rx.c |    2 +-
 drivers/net/wimax/i2400m/sdio-tx.c |    5 +-
 drivers/net/wimax/i2400m/tx.c      |   18 ++++-
 drivers/net/wimax/i2400m/usb-rx.c  |   35 +++++++--
 drivers/net/wimax/i2400m/usb-tx.c  |   35 ++++++++-
 drivers/net/wimax/i2400m/usb.c     |   14 +++-
 net/wimax/op-msg.c                 |    2 +
 net/wimax/op-rfkill.c              |    9 ++-
 13 files changed, 310 insertions(+), 168 deletions(-)

Inaky Perez-Gonzalez

author

2009-11-04 21:42 UTC #1 lore raw

PATCH 1/13 wimax/i2400m: reduce verbosity of debug messages in boot mode

Missed a debug message that was being constantly printed as a dev_err(); became annoying. Demote it to a debug message.

Signed-off-by Inaky Perez-Gonzalez <[email protected]>

---
 drivers/net/wimax/i2400m/sdio-rx.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

drivers/net/wimax/i2400m/sdio-rx.c +1 -1

index 98ee7fd..8adf6c9 100644
@@ -165,7 +165,7 @@ void i2400ms_rx(struct i2400ms *i2400ms)
 		spin_unlock(&i2400m->rx_lock);
 		memcpy(i2400m->bm_ack_buf, skb->data, rx_size);
 		wake_up(&i2400ms->bm_wfa_wq);
-		dev_err(dev, "RX: SDIO boot mode message\n");
+		d_printf(5, dev, "RX: SDIO boot mode message\n");
 		kfree_skb(skb);
 		goto out;
 	}

Signature · 1 line

1.6.2.5

Inaky Perez-Gonzalez

author

2009-11-04 21:42 UTC #2 lore raw

PATCH 2/13 wimax/i2400m: fix race condition with tcpdump et al

In reply to Inaky Perez-Gonzalez

tcpdump and friends were not being able to decode packets sent via WiMAX; they had a zero ethernet type, even when the stack was properly sending them to the device with the right type.

It happens that the driver was overwriting the (fake) ethernet header for creating the hardware header and that was bitting the cloning used by tcpdump (et al) to look into the packets.

Use pkskb_expand_head() [method copied from the e1000 driver] to fix.

Thanks to Herbert Xu and Andi Kleen for helping to diagnose and pointing to the right fix.

Cc Herbert Xu <gondor.apana.org.au>

Cc Andi Kleen <[email protected]>

Signed-off-by Inaky Perez-Gonzalez <[email protected]>

---
 drivers/net/wimax/i2400m/netdev.c |   21 ++++++++++++++++++---
 1 files changed, 18 insertions(+), 3 deletions(-)

drivers/net/wimax/i2400m/netdev.c +18 -3

index fefd794..e7d1a51 100644
@@ -358,6 +358,20 @@ netdev_tx_t i2400m_hard_start_xmit(struct sk_buff *skb,
 	int result;
 
 	d_fnstart(3, dev, "(skb %p net_dev %p)\n", skb, net_dev);
+	if (skb_header_cloned(skb)) {
+		/*
+		 * Make tcpdump/wireshark happy -- if they are
+		 * running, the skb is cloned and we will overwrite
+		 * the mac fields in i2400m_tx_prep_header. Expand
+		 * seems to fix this...
+		 */
+		result = pskb_expand_head(skb, 0, 0, GFP_ATOMIC);
+		if (result) {
+			result = NETDEV_TX_BUSY;
+			goto error_expand;
+		}
+	}
+
 	if (i2400m->state == I2400M_SS_IDLE)
 		result = i2400m_net_wake_tx(i2400m, net_dev, skb);
 	else

@@ -368,10 +382,11 @@ netdev_tx_t i2400m_hard_start_xmit(struct sk_buff *skb,
 		net_dev->stats.tx_packets++;
 		net_dev->stats.tx_bytes += skb->len;
 	}
+	result = NETDEV_TX_OK;
+error_expand:
 	kfree_skb(skb);
-
-	d_fnend(3, dev, "(skb %p net_dev %p)\n", skb, net_dev);
-	return NETDEV_TX_OK;
+	d_fnend(3, dev, "(skb %p net_dev %p) = %d\n", skb, net_dev, result);
+	return result;
 }

Signature · 1 line

1.6.2.5

Inaky Perez-Gonzalez

author

2009-11-04 21:42 UTC #3 lore raw

PATCH 3/13 wimax: allow user space to send messages once the device is registered

In reply to Inaky Perez-Gonzalez

It makes sense that the messaging pipe to the device can be used before the device is fully ready, as long as it is registered with the stack. Some debugging tools need it.

Signed-off-by Inaky Perez-Gonzalez <[email protected]>

---
 net/wimax/op-msg.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

net/wimax/op-msg.c +2 -0

index d631a17..d3bfb6e 100644
@@ -388,6 +388,8 @@ int wimax_gnl_doit_msg_from_user(struct sk_buff *skb, struct genl_info *info)
 	}
 	mutex_lock(&wimax_dev->mutex);
 	result = wimax_dev_is_ready(wimax_dev);
+	if (result == -ENOMEDIUM)
+		result = 0;
 	if (result < 0)
 		goto error_not_ready;
 	result = -ENOSYS;

Signature · 1 line

1.6.2.5

Inaky Perez-Gonzalez

author

2009-11-04 21:42 UTC #4 lore raw

PATCH 4/13 wimax: allow WIMAX_RF_QUERY calls when state is still uninitialized

In reply to Inaky Perez-Gonzalez

Until now, calls to wimax_rfkill() will be blocked until the device is at least past the WIMAX_ST_UNINITIALIZED state, return -ENOMEDIUM when the device is in the WIMAX_ST_DOWN state.

In parallel, wimax-tools would issue a wimax_rfkill(WIMAX_RF_QUERY) call right after opening a handle with wimaxll_open() as means to verify if the interface is really a WiMAX interface [newer kernel version will have a call specifically for this].

The combination of these two facts is that in some cases, before the driver has finalized initializing its device's firmware, a wimaxll_open() call would fail, when it should not.

Thus, change the wimax_rfkill() code to allow queries when the device is in WIMAX_ST_UNINITIALIZED state.

Signed-off-by Inaky Perez-Gonzalez <[email protected]>

---
 net/wimax/op-rfkill.c |    9 ++++++++-
 1 files changed, 8 insertions(+), 1 deletions(-)

net/wimax/op-rfkill.c +8 -1

index 40e1210..94d339c 100644
@@ -305,8 +305,15 @@ int wimax_rfkill(struct wimax_dev *wimax_dev, enum wimax_rf_state state)
 	d_fnstart(3, dev, "(wimax_dev %p state %u)\n", wimax_dev, state);
 	mutex_lock(&wimax_dev->mutex);
 	result = wimax_dev_is_ready(wimax_dev);
-	if (result < 0)
+	if (result < 0) {
+		/* While initializing, < 1.4.3 wimax-tools versions use
+		 * this call to check if the device is a valid WiMAX
+		 * device; so we allow it to proceed always,
+		 * considering the radios are all off. */
+		if (result == -ENOMEDIUM && state == WIMAX_RF_QUERY)
+			result = WIMAX_RF_OFF << 1 | WIMAX_RF_OFF;
 		goto error_not_ready;
+	}
 	switch (state) {
 	case WIMAX_RF_ON:
 	case WIMAX_RF_OFF:

Signature · 1 line

1.6.2.5

Inaky Perez-Gonzalez

author

2009-11-04 21:42 UTC #5 lore raw

PATCH 5/13 wimax/i2400m: use JUMP cmd for last FW chunk indication

In reply to Inaky Perez-Gonzalez

From: Cindy H Kao <[email protected]>

Both secure and non-secure boot must set the JUMP command in the bootmode header as the last FW chunk, so we change to use the JUMP command to decide if the FW chunk download is completed.

Since we tend to use one single FW to support both secure and non-secure boot for most of the time, I2400M_BRH_SIGNED_JUMP is actually found even for non-secure boot. But in case the FW does come with I2400M_BRH_JUMP, we check for both of them in i2400m_dnload_bcf().

Signed-off-by Cindy H Kao <[email protected]>

Signed-off-by Inaky Perez-Gonzalez <[email protected]>

---
 drivers/net/wimax/i2400m/fw.c |   15 +++++++++------
 1 files changed, 9 insertions(+), 6 deletions(-)

drivers/net/wimax/i2400m/fw.c +9 -6

index 3d67bcf..97ea784 100644
@@ -720,14 +720,17 @@ ssize_t i2400m_dnload_bcf(struct i2400m *i2400m,
 			 "downloading section #%zu (@%zu %zu B) to 0x%08x\n",
 			 section, offset, sizeof(*bh) + data_size,
 			 le32_to_cpu(bh->target_addr));
-		if (i2400m_brh_get_opcode(bh) == I2400M_BRH_SIGNED_JUMP) {
-			/* Secure boot needs to stop here */
-			d_printf(5, dev,  "signed jump found @%zu\n", offset);
+		/*
+		 * We look for JUMP cmd from the bootmode header,
+		 * either I2400M_BRH_SIGNED_JUMP for secure boot
+		 * or I2400M_BRH_JUMP for unsecure boot, the last chunk
+		 * should be the bootmode header with JUMP cmd.
+		 */
+		if (i2400m_brh_get_opcode(bh) == I2400M_BRH_SIGNED_JUMP ||
+			i2400m_brh_get_opcode(bh) == I2400M_BRH_JUMP) {
+			d_printf(5, dev,  "jump found @%zu\n", offset);
 			break;
 		}
-		if (offset + section_size == bcf_len)
-			/* Non-secure boot stops here */
-			break;
 		if (offset + section_size > bcf_len) {
 			dev_err(dev, "fw %s: bad section #%zu, "
 				"end (@%zu) beyond EOF (@%zu)\n",

Signature · 1 line

1.6.2.5

Inaky Perez-Gonzalez

author

2009-11-04 21:42 UTC #6 lore raw

PATCH 6/13 wimax/i2400m: change the bcf_len to exclude the extended header size

In reply to Inaky Perez-Gonzalez

From: Cindy H Kao <[email protected]>

The actual fw->size may not equal to the bcf size indicated in the bcf header if the extended bcf debug header is added in the tail.

To reflect the actual fw size that will be downloaded to the device, it is now retrived from from the size field indicated in the bcf header.

All of the headers (if there are extended headers) should indicate same value for the size field since only one set of firmware chunks is downloaded

Signed-off-by Cindy H Kao <[email protected]>

---
 drivers/net/wimax/i2400m/fw.c |   15 +++++++++++----
 1 files changed, 11 insertions(+), 4 deletions(-)

drivers/net/wimax/i2400m/fw.c +11 -4

index 97ea784..fda54bf 100644
@@ -1388,15 +1388,16 @@ const struct i2400m_bcf_hdr *i2400m_bcf_hdr_find(struct i2400m *i2400m)
  */
 static
 int i2400m_fw_dnload(struct i2400m *i2400m, const struct i2400m_bcf_hdr *bcf,
-		     size_t bcf_size, enum i2400m_bri flags)
+		     size_t fw_size, enum i2400m_bri flags)
 {
 	int ret = 0;
 	struct device *dev = i2400m_dev(i2400m);
 	int count = i2400m->bus_bm_retries;
 	const struct i2400m_bcf_hdr *bcf_hdr;
+	size_t bcf_size;
 
-	d_fnstart(5, dev, "(i2400m %p bcf %p size %zu)\n",
-		  i2400m, bcf, bcf_size);
+	d_fnstart(5, dev, "(i2400m %p bcf %p fw size %zu)\n",
+		  i2400m, bcf, fw_size);
 	i2400m->boot_mode = 1;
 	wmb();		/* Make sure other readers see it */
 hw_reboot:

@@ -1434,6 +1435,12 @@ hw_reboot:
 	if (ret < 0)
 		goto error_dnload_init;
 
+	/*
+	 * bcf_size refers to one header size plus the fw sections size
+	 * indicated by the header,ie. if there are other extended headers
+	 * at the tail, they are not counted
+	 */
+	bcf_size = sizeof(u32) * le32_to_cpu(bcf_hdr->size);
 	ret = i2400m_dnload_bcf(i2400m, bcf, bcf_size);
 	if (ret == -ERESTARTSYS)
 		goto error_dev_rebooted;

@@ -1464,7 +1471,7 @@ error_bcf_hdr_find:
 error_bootrom_init:
 error_too_many_reboots:
 	d_fnend(5, dev, "(i2400m %p bcf %p size %zu) = %d\n",
-		i2400m, bcf, bcf_size, ret);
+		i2400m, bcf, fw_size, ret);
 	return ret;
 
 error_dev_rebooted:

Signature · 1 line

1.6.2.5

Inaky Perez-Gonzalez

author

2009-11-04 21:42 UTC #7 lore raw

PATCH 7/13 wimax/i2400m: when stopping the device, cancel any pending message

In reply to Inaky Perez-Gonzalez

The stop procedure for the device must make sure that any task that is waiting on a message is properly cancelled.

This was being taken care of only by the __i2400m_dev_reset_handle() path and the rest was working by chance because the waits have a timeout.

Fixed by adding a proper cancellation in __i2400m_dev_stop().

Signed-off-by Inaky Perez-Gonzalez <[email protected]>

---
 drivers/net/wimax/i2400m/driver.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

drivers/net/wimax/i2400m/driver.c +2 -0

index 1f6aa2a..810eda7 100644
@@ -537,6 +537,8 @@ void __i2400m_dev_stop(struct i2400m *i2400m)
 
 	d_fnstart(3, dev, "(i2400m %p)\n", i2400m);
 	wimax_state_change(wimax_dev, __WIMAX_ST_QUIESCING);
+	i2400m_msg_to_dev_cancel_wait(i2400m, -EL3RST);
+	complete(&i2400m->msg_completion);
 	i2400m_net_wake_stop(i2400m);
 	i2400m_dev_shutdown(i2400m);
 	/*

Signature · 1 line

1.6.2.5

Inaky Perez-Gonzalez

author

2009-11-04 21:42 UTC #8 lore raw

PATCH 8/13 wimax/i2400m: fix deadlock: don't do BUS reset under i2400m->init_mutex

In reply to Inaky Perez-Gonzalez

Since the addition of the pre/post reset handlers, it became clear that we cannot do a I2400M-RT-BUS type reset while holding the init_mutex, as in the case of USB, it will deadlock when trying to call i2400m_pre_reset().

Thus, the following changes:

- clarify the fact that calling bus_reset() w/ I2400M_RT_BUS while holding init_mutex is a no-no.

- i2400m_dev_reset_handle() will do a BUS reset to recover a gone device after unlocking init_mutex.

- in the USB reset implementation, when cold and warm reset fails, fallback to QUEUING a usb reset, not executing a USB reset, so it happens from another context and does not deadlock.

Signed-off-by Inaky Perez-Gonzalez <[email protected]>

---
 drivers/net/wimax/i2400m/driver.c |   10 +++++++---
 drivers/net/wimax/i2400m/i2400m.h |    3 +++
 drivers/net/wimax/i2400m/usb.c    |   14 ++++++++++----
 3 files changed, 20 insertions(+), 7 deletions(-)

drivers/net/wimax/i2400m/driver.c +7 -3

index 810eda7..10673af 100644
@@ -765,9 +765,7 @@ void __i2400m_dev_reset_handle(struct work_struct *ws)
 		wmb();		/* see i2400m->updown's documentation  */
 		dev_err(dev, "%s: cannot start the device: %d\n",
 			reason, result);
-		result = i2400m->bus_reset(i2400m, I2400M_RT_BUS);
-		if (result >= 0)
-			result = -ENODEV;
+		result = -EUCLEAN;
 	}
 out_unlock:
 	if (i2400m->reset_ctx) {

@@ -775,6 +773,12 @@ out_unlock:
 		complete(&ctx->completion);
 	}
 	mutex_unlock(&i2400m->init_mutex);
+	if (result == -EUCLEAN) {
+		/* ops, need to clean up [w/ init_mutex not held] */
+		result = i2400m->bus_reset(i2400m, I2400M_RT_BUS);
+		if (result >= 0)
+			result = -ENODEV;
+	}
 out:
 	i2400m_put(i2400m);
 	kfree(iw);

drivers/net/wimax/i2400m/i2400m.h +3 -0

index 8fc8a0c..f5ed7d5 100644
@@ -281,6 +281,9 @@ struct i2400m_barker_db;
  *     process, so it cannot rely on common infrastructure being laid
  *     out.
  *
+ *     IMPORTANT: don't call reset on RT_BUS with i2400m->init_mutex
+ *     held, as the .pre/.post reset handlers will deadlock.
+ *
  * @bus_bm_retries: [fill] How many times shall a firmware upload /
  *     device initialization be retried? Different models of the same
  *     device might need different values, hence it is set by the

drivers/net/wimax/i2400m/usb.c +10 -4

index 8b246cc..418db12 100644
@@ -254,7 +254,6 @@ int i2400mu_bus_reset(struct i2400m *i2400m, enum i2400m_reset_type rt)
 			sizeof(i2400m_COLD_BOOT_BARKER),
 			i2400mu->endpoint_cfg.reset_cold);
 	else if (rt == I2400M_RT_BUS) {
-do_bus_reset:
 		result = usb_reset_device(i2400mu->usb_dev);
 		switch (result) {
 		case 0:

@@ -262,7 +261,7 @@ do_bus_reset:
 		case -ENODEV:
 		case -ENOENT:
 		case -ESHUTDOWN:
-			result = rt == I2400M_RT_WARM ? -ENODEV : 0;
+			result = 0;
 			break;	/* We assume the device is disconnected */
 		default:
 			dev_err(dev, "USB reset failed (%d), giving up!\n",

@@ -275,10 +274,17 @@ do_bus_reset:
 	if (result < 0
 	    && result != -EINVAL	/* device is gone */
 	    && rt != I2400M_RT_BUS) {
+		/*
+		 * Things failed -- resort to lower level reset, that
+		 * we queue in another context; the reason for this is
+		 * that the pre and post reset functionality requires
+		 * the i2400m->init_mutex; RT_WARM and RT_COLD can
+		 * come from areas where i2400m->init_mutex is taken.
+		 */
 		dev_err(dev, "%s reset failed (%d); trying USB reset\n",
 			rt == I2400M_RT_WARM ? "warm" : "cold", result);
-		rt = I2400M_RT_BUS;
-		goto do_bus_reset;
+		usb_queue_reset_device(i2400mu->usb_iface);
+		result = -ENODEV;
 	}
 	d_fnend(3, dev, "(i2400m %p rt %u) = %d\n", i2400m, rt, result);
 	return result;

Signature · 1 line

1.6.2.5

Inaky Perez-Gonzalez

author

2009-11-04 21:42 UTC #9 lore raw

PATCH 9/13 wimax/i2400m: move i2400m_init() out of i2400m.h

In reply to Inaky Perez-Gonzalez

Upcoming changes will have to add things to this function that expose more internals, which would mean more forward declarators.

Frankly, it doesn't need to be an inline, so moved to driver.c, where the declarations will be taken from the header file.

Signed-off-by Inaky Perez-Gonzalez <[email protected]>

---
 drivers/net/wimax/i2400m/driver.c |   30 ++++++++++++++++++++++++++++++
 drivers/net/wimax/i2400m/i2400m.h |   31 +------------------------------
 2 files changed, 31 insertions(+), 30 deletions(-)

drivers/net/wimax/i2400m/driver.c +30 -0

index 10673af..9b78e05 100644
@@ -849,6 +849,36 @@ void i2400m_bm_buf_free(struct i2400m *i2400m)
 
 
 /**
+ * i2400m_init - Initialize a 'struct i2400m' from all zeroes
+ *
+ * This is a bus-generic API call.
+ */
+void i2400m_init(struct i2400m *i2400m)
+{
+	wimax_dev_init(&i2400m->wimax_dev);
+
+	i2400m->boot_mode = 1;
+	i2400m->rx_reorder = 1;
+	init_waitqueue_head(&i2400m->state_wq);
+
+	spin_lock_init(&i2400m->tx_lock);
+	i2400m->tx_pl_min = UINT_MAX;
+	i2400m->tx_size_min = UINT_MAX;
+
+	spin_lock_init(&i2400m->rx_lock);
+	i2400m->rx_pl_min = UINT_MAX;
+	i2400m->rx_size_min = UINT_MAX;
+
+	mutex_init(&i2400m->msg_mutex);
+	init_completion(&i2400m->msg_completion);
+
+	mutex_init(&i2400m->init_mutex);
+	/* wake_tx_ws is initialized in i2400m_tx_setup() */
+}
+EXPORT_SYMBOL_GPL(i2400m_init);
+
+
+/**
  * i2400m_setup - bus-generic setup function for the i2400m device
  *
  * @i2400m: device descriptor (bus-specific parts have been initialized)

drivers/net/wimax/i2400m/i2400m.h +1 ~~-30~~

index f5ed7d5..4f8815d 100644
@@ -576,36 +576,6 @@ struct i2400m {
 
 
 /*
- * Initialize a 'struct i2400m' from all zeroes
- *
- * This is a bus-generic API call.
- */
-static inline
-void i2400m_init(struct i2400m *i2400m)
-{
-	wimax_dev_init(&i2400m->wimax_dev);
-
-	i2400m->boot_mode = 1;
-	i2400m->rx_reorder = 1;
-	init_waitqueue_head(&i2400m->state_wq);
-
-	spin_lock_init(&i2400m->tx_lock);
-	i2400m->tx_pl_min = UINT_MAX;
-	i2400m->tx_size_min = UINT_MAX;
-
-	spin_lock_init(&i2400m->rx_lock);
-	i2400m->rx_pl_min = UINT_MAX;
-	i2400m->rx_size_min = UINT_MAX;
-
-	mutex_init(&i2400m->msg_mutex);
-	init_completion(&i2400m->msg_completion);
-
-	mutex_init(&i2400m->init_mutex);
-	/* wake_tx_ws is initialized in i2400m_tx_setup() */
-}
-
-
-/*
  * Bus-generic internal APIs
  * -------------------------
  */

@@ -737,6 +707,7 @@ unsigned i2400m_brh_get_signature(const struct i2400m_bootrom_header *hdr)
 /*
  * Driver / device setup and internal functions
  */
+extern void i2400m_init(struct i2400m *);
 extern void i2400m_netdev_setup(struct net_device *net_dev);
 extern int i2400m_sysfs_setup(struct device_driver *);
 extern void i2400m_sysfs_release(struct device_driver *);

Signature · 1 line

1.6.2.5

Inaky Perez-Gonzalez

author

2009-11-04 21:42 UTC #10 lore raw

PATCH 10/13 wimax/i2400m: queue device's report until the driver is ready for them

In reply to Inaky Perez-Gonzalez

The i2400m might start sending reports to the driver before it is done setting up all the infrastructure needed for handling them.

Currently we were just dropping them when the driver wasn't ready and that is bad in certain situations, as the sync between the driver's idea of the device's state and the device's state dissapears.

This changes that by implementing a queue for handling reports. Incoming reports are appended to it and a workstruct is woken to process the list of queued reports.

When the device is not yet ready to handle them, the workstruct is not woken, but at soon as the device becomes ready again, the queue is processed.

As a consequence of this, i2400m_queue_work() is no longer used, and thus removed.

Signed-off-by Inaky Perez-Gonzalez <[email protected]>

---
 drivers/net/wimax/i2400m/driver.c |   74 +------------------
 drivers/net/wimax/i2400m/i2400m.h |   14 +++-
 drivers/net/wimax/i2400m/rx.c     |  142 +++++++++++++++++++++++++++++--------
 3 files changed, 128 insertions(+), 102 deletions(-)

drivers/net/wimax/i2400m/driver.c +4 ~~-70~~

index 9b78e05..42102eb 100644
@@ -128,76 +128,6 @@ struct i2400m_work *__i2400m_work_setup(
 }
 
 
-/**
- * i2400m_queue_work - schedule work on a i2400m's queue
- *
- * @i2400m: device descriptor
- *
- * @fn: function to run to execute work. It gets passed a 'struct
- *     work_struct' that is wrapped in a 'struct i2400m_work'. Once
- *     done, you have to (1) i2400m_put(i2400m_work->i2400m) and then
- *     (2) kfree(i2400m_work).
- *
- * @gfp_flags: GFP flags for memory allocation.
- *
- * @pl: pointer to a payload buffer that you want to pass to the _work
- *     function. Use this to pack (for example) a struct with extra
- *     arguments.
- *
- * @pl_size: size of the payload buffer.
- *
- * We do this quite often, so this just saves typing; allocate a
- * wrapper for a i2400m, get a ref to it, pack arguments and launch
- * the work.
- *
- * A usual workflow is:
- *
- * struct my_work_args {
- *         void *something;
- *         int whatever;
- * };
- * ...
- *
- * struct my_work_args my_args = {
- *         .something = FOO,
- *         .whaetever = BLAH
- * };
- * i2400m_queue_work(i2400m, 1, my_work_function, GFP_KERNEL,
- *                   &args, sizeof(args))
- *
- * And now the work function can unpack the arguments and call the
- * real function (or do the job itself):
- *
- * static
- * void my_work_fn((struct work_struct *ws)
- * {
- *         struct i2400m_work *iw =
- *	           container_of(ws, struct i2400m_work, ws);
- *	   struct my_work_args *my_args = (void *) iw->pl;
- *
- *	   my_work(iw->i2400m, my_args->something, my_args->whatevert);
- * }
- */
-int i2400m_queue_work(struct i2400m *i2400m,
-		      void (*fn)(struct work_struct *), gfp_t gfp_flags,
-		      const void *pl, size_t pl_size)
-{
-	int result;
-	struct i2400m_work *iw;
-
-	BUG_ON(i2400m->work_queue == NULL);
-	result = -ENOMEM;
-	iw = __i2400m_work_setup(i2400m, fn, gfp_flags, pl, pl_size);
-	if (iw != NULL) {
-		result = queue_work(i2400m->work_queue, &iw->ws);
-		if (WARN_ON(result == 0))
-			result = -ENXIO;
-	}
-	return result;
-}
-EXPORT_SYMBOL_GPL(i2400m_queue_work);
-
-
 /*
  * Schedule i2400m's specific work on the system's queue.
  *

@@ -459,6 +389,8 @@ retry:
 		goto error_bus_dev_start;
 	i2400m->ready = 1;
 	wmb();		/* see i2400m->ready's documentation  */
+	/* process pending reports from the device */
+	queue_work(i2400m->work_queue, &i2400m->rx_report_ws);
 	result = i2400m_firmware_check(i2400m);	/* fw versions ok? */
 	if (result < 0)
 		goto error_fw_check;

@@ -868,6 +800,8 @@ void i2400m_init(struct i2400m *i2400m)
 	spin_lock_init(&i2400m->rx_lock);
 	i2400m->rx_pl_min = UINT_MAX;
 	i2400m->rx_size_min = UINT_MAX;
+	INIT_LIST_HEAD(&i2400m->rx_reports);
+	INIT_WORK(&i2400m->rx_report_ws, i2400m_report_hook_work);
 
 	mutex_init(&i2400m->msg_mutex);
 	init_completion(&i2400m->msg_completion);

drivers/net/wimax/i2400m/i2400m.h +11 -3

index 4f8815d..55bca43 100644
@@ -421,6 +421,13 @@ struct i2400m_barker_db;
  *     delivered. Then the driver can release them to the host. See
  *     drivers/net/i2400m/rx.c for details.
  *
+ * @rx_reports: reports received from the device that couldn't be
+ *     processed because the driver wasn't still ready; when ready,
+ *     they are pulled from here and chewed.
+ *
+ * @rx_reports_ws: Work struct used to kick a scan of the RX reports
+ *     list and to process each.
+ *
  * @src_mac_addr: MAC address used to make ethernet packets be coming
  *     from. This is generated at i2400m_setup() time and used during
  *     the life cycle of the instance. See i2400m_fake_eth_header().

@@ -548,6 +555,8 @@ struct i2400m {
 		rx_num, rx_size_acc, rx_size_min, rx_size_max;
 	struct i2400m_roq *rx_roq;	/* not under rx_lock! */
 	u8 src_mac_addr[ETH_HLEN];
+	struct list_head rx_reports;	/* under rx_lock! */
+	struct work_struct rx_report_ws;
 
 	struct mutex msg_mutex;		/* serialize command execution */
 	struct completion msg_completion;

@@ -830,9 +839,7 @@ struct i2400m_work {
 	size_t pl_size;
 	u8 pl[0];
 };
-extern int i2400m_queue_work(struct i2400m *,
-			     void (*)(struct work_struct *), gfp_t,
-			     const void *, size_t);
+
 extern int i2400m_schedule_work(struct i2400m *,
 				void (*)(struct work_struct *), gfp_t,
 				const void *, size_t);

@@ -847,6 +854,7 @@ extern void i2400m_msg_ack_hook(struct i2400m *,
 				const struct i2400m_l3l4_hdr *, size_t);
 extern void i2400m_report_hook(struct i2400m *,
 			       const struct i2400m_l3l4_hdr *, size_t);
+extern void i2400m_report_hook_work(struct work_struct *);
 extern int i2400m_cmd_enter_powersave(struct i2400m *);
 extern int i2400m_cmd_get_state(struct i2400m *);
 extern int i2400m_cmd_exit_idle(struct i2400m *);

drivers/net/wimax/i2400m/rx.c +113 ~~-29~~

168 lines not on this page · raw message

Signature · 1 line

1.6.2.5

Inaky Perez-Gonzalez

author

2009-11-04 21:42 UTC #11 lore raw

PATCH 11/13 wimax/i2400m: fix oops in TX when tearing down the device

In reply to Inaky Perez-Gonzalez

All the entry points into the TX module should check if the device has been torn down. Otherwise, when the device resets or shuts down, there are windows when a call to i2400m_tx*() will oops the system.

For that, make i2400m_tx_release() set i2400m->tx_buf to NULL under the tx_lock. Then, any entry point [i2400m_tx(), _tx_msg_sent(), _tx_msg_get()] will check for i2400m->tx_buf to be NULL and exit gracefully.

Signed-off-by Inaky Perez-Gonzalez <[email protected]>

---
 drivers/net/wimax/i2400m/sdio-tx.c |    5 ++++-
 drivers/net/wimax/i2400m/tx.c      |   18 +++++++++++++++++-
 2 files changed, 21 insertions(+), 2 deletions(-)

drivers/net/wimax/i2400m/sdio-tx.c +4 -1

index 5105a5e..de66d06 100644
@@ -149,5 +149,8 @@ int i2400ms_tx_setup(struct i2400ms *i2400ms)
 
 void i2400ms_tx_release(struct i2400ms *i2400ms)
 {
-	destroy_workqueue(i2400ms->tx_workqueue);
+	if (i2400ms->tx_workqueue) {
+		destroy_workqueue(i2400ms->tx_workqueue);
+		i2400ms->tx_workqueue = NULL;
+	}
 }

drivers/net/wimax/i2400m/tx.c +17 -1

index 8c20802..54480e8 100644
@@ -642,6 +642,9 @@ int i2400m_tx(struct i2400m *i2400m, const void *buf, size_t buf_len,
 	 * current one is out of payload slots or we have a singleton,
 	 * close it and start a new one */
 	spin_lock_irqsave(&i2400m->tx_lock, flags);
+	result = -ESHUTDOWN;
+	if (i2400m->tx_buf == NULL)
+		goto error_tx_new;
 try_new:
 	if (unlikely(i2400m->tx_msg == NULL))
 		i2400m_tx_new(i2400m);

@@ -697,7 +700,10 @@ try_new:
 	}
 error_tx_new:
 	spin_unlock_irqrestore(&i2400m->tx_lock, flags);
-	i2400m->bus_tx_kick(i2400m);	/* always kick, might free up space */
+	/* kick in most cases, except when the TX subsys is down, as
+	 * it might free space */
+	if (likely(result != -ESHUTDOWN))
+		i2400m->bus_tx_kick(i2400m);
 	d_fnend(3, dev, "(i2400m %p skb %p [%zu bytes] pt %u) = %d\n",
 		i2400m, buf, buf_len, pl_type, result);
 	return result;

@@ -740,6 +746,9 @@ struct i2400m_msg_hdr *i2400m_tx_msg_get(struct i2400m *i2400m,
 
 	d_fnstart(3, dev, "(i2400m %p bus_size %p)\n", i2400m, bus_size);
 	spin_lock_irqsave(&i2400m->tx_lock, flags);
+	tx_msg_moved = NULL;
+	if (i2400m->tx_buf == NULL)
+		goto out_unlock;
 skip:
 	tx_msg_moved = NULL;
 	if (i2400m->tx_in == i2400m->tx_out) {	/* Empty FIFO? */

@@ -829,6 +838,8 @@ void i2400m_tx_msg_sent(struct i2400m *i2400m)
 
 	d_fnstart(3, dev, "(i2400m %p)\n", i2400m);
 	spin_lock_irqsave(&i2400m->tx_lock, flags);
+	if (i2400m->tx_buf == NULL)
+		goto out_unlock;
 	i2400m->tx_out += i2400m->tx_msg_size;
 	d_printf(2, dev, "TX: sent %zu b\n", (size_t) i2400m->tx_msg_size);
 	i2400m->tx_msg_size = 0;

@@ -837,6 +848,7 @@ void i2400m_tx_msg_sent(struct i2400m *i2400m)
 	n = i2400m->tx_out / I2400M_TX_BUF_SIZE;
 	i2400m->tx_out %= I2400M_TX_BUF_SIZE;
 	i2400m->tx_in -= n * I2400M_TX_BUF_SIZE;
+out_unlock:
 	spin_unlock_irqrestore(&i2400m->tx_lock, flags);
 	d_fnend(3, dev, "(i2400m %p) = void\n", i2400m);
 }

@@ -876,5 +888,9 @@ int i2400m_tx_setup(struct i2400m *i2400m)
  */
 void i2400m_tx_release(struct i2400m *i2400m)
 {
+	unsigned long flags;
+	spin_lock_irqsave(&i2400m->tx_lock, flags);
 	kfree(i2400m->tx_buf);
+	i2400m->tx_buf = NULL;
+	spin_unlock_irqrestore(&i2400m->tx_lock, flags);
 }

Signature · 1 line

1.6.2.5

Inaky Perez-Gonzalez

author

2009-11-04 21:42 UTC #12 lore raw

PATCH 12/13 wimax/i2400m: Let device's status reports change the device state

In reply to Inaky Perez-Gonzalez

Currently __i2400m_dev_start was forcing, after uploading firmware and doing a few checks to WIMAX_ST_UNINITIALIZED.

This can be overriding state changes that the device might have caused by sending reports; thus it makes more sense to remove it and let the device update the status on its own by sending reports.

Signed-off-by Inaky Perez-Gonzalez <[email protected]>

---
 drivers/net/wimax/i2400m/driver.c |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

drivers/net/wimax/i2400m/driver.c +0 -1

index 42102eb..cc900b9 100644
@@ -398,7 +398,6 @@ retry:
 	result = i2400m_check_mac_addr(i2400m);
 	if (result < 0)
 		goto error_check_mac_addr;
-	wimax_state_change(wimax_dev, WIMAX_ST_UNINITIALIZED);
 	result = i2400m_dev_initialize(i2400m);
 	if (result < 0)
 		goto error_dev_initialize;

Signature · 1 line

1.6.2.5

Inaky Perez-Gonzalez

author

2009-11-04 21:42 UTC #13 lore raw

PATCH 13/13 wimax/i2400m: fix oops caused by race condition when exiting USB kthreads

In reply to Inaky Perez-Gonzalez

Current i2400m USB code had to threads (one for processing RX, one for TX). When calling i2400m_{tx,rx}_release(), it would crash if the thread had exited already due to an error.

So changed the code to have the thread fill in/out i2400mu->{tx,rx}_kthread under a spinlock; then the _release() function will call kthread_stop() only if {rx,tx}_kthread is still set.

Signed-off-by Inaky Perez-Gonzalez <[email protected]>

---
 drivers/net/wimax/i2400m/usb-rx.c |   35 +++++++++++++++++++++++++++++------
 drivers/net/wimax/i2400m/usb-tx.c |   35 ++++++++++++++++++++++++++++++-----
 2 files changed, 59 insertions(+), 11 deletions(-)

drivers/net/wimax/i2400m/usb-rx.c +29 -6

index e494e37..245587f 100644
@@ -316,10 +316,15 @@ int i2400mu_rxd(void *_i2400mu)
 	size_t pending;
 	int rx_size;
 	struct sk_buff *rx_skb;
+	unsigned long flags;
 
 	d_fnstart(4, dev, "(i2400mu %p)\n", i2400mu);
+	spin_lock_irqsave(&i2400m->rx_lock, flags);
+	BUG_ON(i2400mu->rx_kthread != NULL);
+	i2400mu->rx_kthread = current;
+	spin_unlock_irqrestore(&i2400m->rx_lock, flags);
 	while (1) {
-		d_printf(2, dev, "TX: waiting for messages\n");
+		d_printf(2, dev, "RX: waiting for messages\n");
 		pending = 0;
 		wait_event_interruptible(
 			i2400mu->rx_wq,

@@ -367,6 +372,9 @@ int i2400mu_rxd(void *_i2400mu)
 	}
 	result = 0;
 out:
+	spin_lock_irqsave(&i2400m->rx_lock, flags);
+	i2400mu->rx_kthread = NULL;
+	spin_unlock_irqrestore(&i2400m->rx_lock, flags);
 	d_fnend(4, dev, "(i2400mu %p) = %d\n", i2400mu, result);
 	return result;

@@ -403,18 +411,33 @@ int i2400mu_rx_setup(struct i2400mu *i2400mu)
 	struct i2400m *i2400m = &i2400mu->i2400m;
 	struct device *dev = &i2400mu->usb_iface->dev;
 	struct wimax_dev *wimax_dev = &i2400m->wimax_dev;
+	struct task_struct *kthread;
 
-	i2400mu->rx_kthread = kthread_run(i2400mu_rxd, i2400mu, "%s-rx",
-					  wimax_dev->name);
-	if (IS_ERR(i2400mu->rx_kthread)) {
-		result = PTR_ERR(i2400mu->rx_kthread);
+	kthread = kthread_run(i2400mu_rxd, i2400mu, "%s-rx",
+			      wimax_dev->name);
+	/* the kthread function sets i2400mu->rx_thread */
+	if (IS_ERR(kthread)) {
+		result = PTR_ERR(kthread);
 		dev_err(dev, "RX: cannot start thread: %d\n", result);
 	}
 	return result;
 }
 
+
 void i2400mu_rx_release(struct i2400mu *i2400mu)
 {
-	kthread_stop(i2400mu->rx_kthread);
+	unsigned long flags;
+	struct i2400m *i2400m = &i2400mu->i2400m;
+	struct device *dev = i2400m_dev(i2400m);
+	struct task_struct *kthread;
+
+	spin_lock_irqsave(&i2400m->rx_lock, flags);
+	kthread = i2400mu->rx_kthread;
+	i2400mu->rx_kthread = NULL;
+	spin_unlock_irqrestore(&i2400m->rx_lock, flags);
+	if (kthread)
+		kthread_stop(kthread);
+	else
+		d_printf(1, dev, "RX: kthread had already exited\n");
 }

drivers/net/wimax/i2400m/usb-tx.c +30 -5

index 90dfff1..a3c46e9 100644
@@ -161,9 +161,15 @@ int i2400mu_txd(void *_i2400mu)
 	struct device *dev = &i2400mu->usb_iface->dev;
 	struct i2400m_msg_hdr *tx_msg;
 	size_t tx_msg_size;
+	unsigned long flags;
 
 	d_fnstart(4, dev, "(i2400mu %p)\n", i2400mu);
 
+	spin_lock_irqsave(&i2400m->tx_lock, flags);
+	BUG_ON(i2400mu->tx_kthread != NULL);
+	i2400mu->tx_kthread = current;
+	spin_unlock_irqrestore(&i2400m->tx_lock, flags);
+
 	while (1) {
 		d_printf(2, dev, "TX: waiting for messages\n");
 		tx_msg = NULL;

@@ -183,6 +189,11 @@ int i2400mu_txd(void *_i2400mu)
 		if (result < 0)
 			break;
 	}
+
+	spin_lock_irqsave(&i2400m->tx_lock, flags);
+	i2400mu->tx_kthread = NULL;
+	spin_unlock_irqrestore(&i2400m->tx_lock, flags);
+
 	d_fnend(4, dev, "(i2400mu %p) = %d\n", i2400mu, result);
 	return result;
 }

@@ -213,11 +224,13 @@ int i2400mu_tx_setup(struct i2400mu *i2400mu)
 	struct i2400m *i2400m = &i2400mu->i2400m;
 	struct device *dev = &i2400mu->usb_iface->dev;
 	struct wimax_dev *wimax_dev = &i2400m->wimax_dev;
+	struct task_struct *kthread;
 
-	i2400mu->tx_kthread = kthread_run(i2400mu_txd, i2400mu, "%s-tx",
-					  wimax_dev->name);
-	if (IS_ERR(i2400mu->tx_kthread)) {
-		result = PTR_ERR(i2400mu->tx_kthread);
+	kthread = kthread_run(i2400mu_txd, i2400mu, "%s-tx",
+			      wimax_dev->name);
+	/* the kthread function sets i2400mu->tx_thread */
+	if (IS_ERR(kthread)) {
+		result = PTR_ERR(kthread);
 		dev_err(dev, "TX: cannot start thread: %d\n", result);
 	}
 	return result;

@@ -225,5 +238,17 @@ int i2400mu_tx_setup(struct i2400mu *i2400mu)
 
 void i2400mu_tx_release(struct i2400mu *i2400mu)
 {
-	kthread_stop(i2400mu->tx_kthread);
+	unsigned long flags;
+	struct i2400m *i2400m = &i2400mu->i2400m;
+	struct device *dev = i2400m_dev(i2400m);
+	struct task_struct *kthread;
+
+	spin_lock_irqsave(&i2400m->tx_lock, flags);
+	kthread = i2400mu->tx_kthread;
+	i2400mu->tx_kthread = NULL;
+	spin_unlock_irqrestore(&i2400m->tx_lock, flags);
+	if (kthread)
+		kthread_stop(kthread);
+	else
+		d_printf(1, dev, "TX: kthread had already exited\n");
 }

Signature · 1 line

1.6.2.5