[PATCH wireless-drivers-next 0/4] TI wlcore firmware log fixes

[PATCH wireless-drivers-next 0/4] TI wlcore firmware log fixes

[Russell King]

Hi,

(I'm assuming this is the correct way to submit for
wireless-drivers-next.)

The following series fixes a number of issues with the firmware logging
on TI wireless devices, noticed while looking at AP mode issues on the
WL18xx family of devices.

Patch 1 tidies up the use of "fw_log.actual_buff_size" removing
multiple unnecessary endian conversions by caching the CPU endian value
in a local variable "actual_size".

Patch 2 makes the buffer calculations more obvious and adds comments
to describe what is going on. In particular, the addition of "addr_ptr"
eliminates several "addr + internal_fw_addrbase" calculations in the
code.

Patch 3 fixes an error in the calculation of "clean_addr" when it hits
the end of the buffer, which was causing the kernel console to spew
"errors" when firmware tracing is in effect.

Patch 4 removes the error message fixed in patch 3, which can still
occur when we race with the firmware reading the log structure. The
write pointer is where the firmware is writing its next message to,
whereas "clean_ptr" is the point that we've read the firmware log
to. It is fine that these may not match.

As this is a debug facility, I don't see any urgent need to push these
patches into -rc nor stable kernels.

 drivers/net/wireless/ti/wlcore/event.c | 67  +++++++++++++++++++---------------
 1 file changed, 38 insertions(+), 29 deletions(-)

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

[PATCH wireless-drivers-next 1/4] net: wlcore: tidy up use of fw_log.actual_buff_size

[Russell King]

Tidy up the use of fw_log.actual_buff_size - rather than reading it
multiple times and applying the endian conversion, read it once into
actual_len and use that instead.

Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
 drivers/net/wireless/ti/wlcore/event.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/net/wireless/ti/wlcore/event.c b/drivers/net/wireless/ti/wlcore/event.c
index a68bbadae043..a5c261affdc7 100644
--- a/drivers/net/wireless/ti/wlcore/event.c
+++ b/drivers/net/wireless/ti/wlcore/event.c
@@ -40,7 +40,7 @@ int wlcore_event_fw_logger(struct wl1271 *wl)
 	buffer = kzalloc(WL18XX_LOGGER_SDIO_BUFF_MAX, GFP_KERNEL);
 	if (!buffer) {
 		wl1271_error("Fail to allocate fw logger memory");
-		fw_log.actual_buff_size = cpu_to_le32(0);
+		actual_len = 0;
 		goto out;
 	}
 
@@ -49,30 +49,30 @@ int wlcore_event_fw_logger(struct wl1271 *wl)
 	if (ret < 0) {
 		wl1271_error("Fail to read logger buffer, error_id = %d",
 			     ret);
-		fw_log.actual_buff_size = cpu_to_le32(0);
+		actual_len = 0;
 		goto free_out;
 	}
 
 	memcpy(&fw_log, buffer, sizeof(fw_log));
 
-	if (le32_to_cpu(fw_log.actual_buff_size) == 0)
+	actual_len = le32_to_cpu(fw_log.actual_buff_size);
+	if (actual_len == 0)
 		goto free_out;
 
-	actual_len = le32_to_cpu(fw_log.actual_buff_size);
 	start_loc = (le32_to_cpu(fw_log.buff_read_ptr) -
 			internal_fw_addrbase) - addr;
 	end_buff_addr += le32_to_cpu(fw_log.max_buff_size);
 	available_len = end_buff_addr -
 			(le32_to_cpu(fw_log.buff_read_ptr) -
 				 internal_fw_addrbase);
-	actual_len = min(actual_len, available_len);
-	len = actual_len;
 
+	/* Copy initial part from end of ring buffer */
+	len = min(actual_len, available_len);
 	wl12xx_copy_fwlog(wl, &buffer[start_loc], len);
-	clear_addr = addr + start_loc + le32_to_cpu(fw_log.actual_buff_size) +
-			internal_fw_addrbase;
+	clear_addr = addr + start_loc + actual_len + internal_fw_addrbase;
 
-	len = le32_to_cpu(fw_log.actual_buff_size) - len;
+	/* Copy any remaining part from beginning of ring buffer */
+	len = actual_len - len;
 	if (len) {
 		wl12xx_copy_fwlog(wl,
 				  &buffer[WL18XX_LOGGER_BUFF_OFFSET],
@@ -93,7 +93,7 @@ int wlcore_event_fw_logger(struct wl1271 *wl)
 free_out:
 	kfree(buffer);
 out:
-	return le32_to_cpu(fw_log.actual_buff_size);
+	return actual_len;
 }
 EXPORT_SYMBOL_GPL(wlcore_event_fw_logger);
 
-- 
2.20.1

Re: [wireless-drivers-next,1/4] wlcore: tidy up use of fw_log.actual_buff_size

[Kalle Valo]

Russell King <rmk+kernel@armlinux.org.uk> wrote:

> Tidy up the use of fw_log.actual_buff_size - rather than reading it
> multiple times and applying the endian conversion, read it once into
> actual_len and use that instead.
> 
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>

4 patches applied to wireless-drivers-next.git, thanks.

913112398d5e wlcore: tidy up use of fw_log.actual_buff_size
98e94771cadc wlcore: make some of the fwlog calculations more obvious
87ab9cbaee7c wlcore: fix bug reading fwlog
01de6fe49ca4 wlcore: fix read pointer update

-- 
https://patchwork.kernel.org/project/linux-wireless/patch/E1lolvS-0003Ql-NJ@rmk-PC.armlinux.org.uk/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

[PATCH wireless-drivers-next 2/4] net: wlcore: make some of the fwlog calculations more obvious

[Russell King]

Make some of the fwlog calculations more obvious by calculating bits
that get used and documenting what they are. Validate the read pointer
while we're at it to ensure we do not overflow the data block we have
allocated and read.

Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
 drivers/net/wireless/ti/wlcore/event.c | 43 +++++++++++++++++---------
 1 file changed, 28 insertions(+), 15 deletions(-)

diff --git a/drivers/net/wireless/ti/wlcore/event.c b/drivers/net/wireless/ti/wlcore/event.c
index a5c261affdc7..875198fb1480 100644
--- a/drivers/net/wireless/ti/wlcore/event.c
+++ b/drivers/net/wireless/ti/wlcore/event.c
@@ -29,11 +29,13 @@ int wlcore_event_fw_logger(struct wl1271 *wl)
 	u8  *buffer;
 	u32 internal_fw_addrbase = WL18XX_DATA_RAM_BASE_ADDRESS;
 	u32 addr = WL18XX_LOGGER_SDIO_BUFF_ADDR;
-	u32 end_buff_addr = WL18XX_LOGGER_SDIO_BUFF_ADDR +
-				WL18XX_LOGGER_BUFF_OFFSET;
+	u32 addr_ptr;
+	u32 buff_start_ptr;
+	u32 buff_read_ptr;
+	u32 buff_end_ptr;
 	u32 available_len;
 	u32 actual_len;
-	u32 clear_addr;
+	u32 clear_ptr;
 	size_t len;
 	u32 start_loc;
 
@@ -59,17 +61,29 @@ int wlcore_event_fw_logger(struct wl1271 *wl)
 	if (actual_len == 0)
 		goto free_out;
 
-	start_loc = (le32_to_cpu(fw_log.buff_read_ptr) -
-			internal_fw_addrbase) - addr;
-	end_buff_addr += le32_to_cpu(fw_log.max_buff_size);
-	available_len = end_buff_addr -
-			(le32_to_cpu(fw_log.buff_read_ptr) -
-				 internal_fw_addrbase);
+	/* Calculate the internal pointer to the fwlog structure */
+	addr_ptr = internal_fw_addrbase + addr;
 
-	/* Copy initial part from end of ring buffer */
+	/* Calculate the internal pointers to the start and end of log buffer */
+	buff_start_ptr = addr_ptr + WL18XX_LOGGER_BUFF_OFFSET;
+	buff_end_ptr = buff_start_ptr + le32_to_cpu(fw_log.max_buff_size);
+
+	/* Read the read pointer and validate it */
+	buff_read_ptr = le32_to_cpu(fw_log.buff_read_ptr);
+	if (buff_read_ptr < buff_start_ptr ||
+	    buff_read_ptr >= buff_end_ptr) {
+		wl1271_error("buffer read pointer out of bounds: %x not in (%x-%x)\n",
+			     buff_read_ptr, buff_start_ptr, buff_end_ptr);
+		goto free_out;
+	}
+
+	start_loc = buff_read_ptr - addr_ptr;
+	available_len = buff_end_ptr - buff_read_ptr;
+
+	/* Copy initial part up to the end of ring buffer */
 	len = min(actual_len, available_len);
 	wl12xx_copy_fwlog(wl, &buffer[start_loc], len);
-	clear_addr = addr + start_loc + actual_len + internal_fw_addrbase;
+	clear_ptr = addr_ptr + start_loc + actual_len;
 
 	/* Copy any remaining part from beginning of ring buffer */
 	len = actual_len - len;
@@ -77,14 +91,13 @@ int wlcore_event_fw_logger(struct wl1271 *wl)
 		wl12xx_copy_fwlog(wl,
 				  &buffer[WL18XX_LOGGER_BUFF_OFFSET],
 				  len);
-		clear_addr = addr + WL18XX_LOGGER_BUFF_OFFSET + len +
-				internal_fw_addrbase;
+		clear_ptr = addr_ptr + WL18XX_LOGGER_BUFF_OFFSET + len;
 	}
 
 	/* double check that clear address and write pointer are the same */
-	if (clear_addr != le32_to_cpu(fw_log.buff_write_ptr)) {
+	if (clear_ptr != le32_to_cpu(fw_log.buff_write_ptr)) {
 		wl1271_error("Calculate of clear addr Clear = %x, write = %x",
-			     clear_addr, le32_to_cpu(fw_log.buff_write_ptr));
+			     clear_ptr, le32_to_cpu(fw_log.buff_write_ptr));
 	}
 
 	/* indicate FW about Clear buffer */
-- 
2.20.1

[PATCH wireless-drivers-next 3/4] net: wlcore: fix bug reading fwlog

[Russell King]

With logging enabled, it has been observed that the driver spews
messages such as:

wlcore: ERROR Calculate of clear addr Clear = 204025b0, write = 204015b0

The problem occurs because 204025b0 is the end of the buffer, and
204015b0 is the beginning, and the calculation for "clear"ing the
buffer does not take into account that if we read to the very end
of the ring buffer, we are actually at the beginning of the buffer.

Fix this.

Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
 drivers/net/wireless/ti/wlcore/event.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/wireless/ti/wlcore/event.c b/drivers/net/wireless/ti/wlcore/event.c
index 875198fb1480..8a67a708c96e 100644
--- a/drivers/net/wireless/ti/wlcore/event.c
+++ b/drivers/net/wireless/ti/wlcore/event.c
@@ -84,6 +84,8 @@ int wlcore_event_fw_logger(struct wl1271 *wl)
 	len = min(actual_len, available_len);
 	wl12xx_copy_fwlog(wl, &buffer[start_loc], len);
 	clear_ptr = addr_ptr + start_loc + actual_len;
+	if (clear_ptr == buff_end_ptr)
+		clear_ptr = buff_start_ptr;
 
 	/* Copy any remaining part from beginning of ring buffer */
 	len = actual_len - len;
-- 
2.20.1

[PATCH wireless-drivers-next 4/4] net: wlcore: fix read pointer update

[Russell King]

When reading the fw_log structure from the device's memory, we could
race with the firmware updating the actual_buff_size and buff_write_ptr
members of this structure. This would lead to bytes being dropped from
the log.

Fix this by writing back the actual - now fixed - clear_ptr which
reflects where we read up to in the buffer.

This also means that we must not check that the clear_ptr matches the
current write pointer, so remove that check.

Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
 drivers/net/wireless/ti/wlcore/event.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/drivers/net/wireless/ti/wlcore/event.c b/drivers/net/wireless/ti/wlcore/event.c
index 8a67a708c96e..46ab69eab26a 100644
--- a/drivers/net/wireless/ti/wlcore/event.c
+++ b/drivers/net/wireless/ti/wlcore/event.c
@@ -96,15 +96,9 @@ int wlcore_event_fw_logger(struct wl1271 *wl)
 		clear_ptr = addr_ptr + WL18XX_LOGGER_BUFF_OFFSET + len;
 	}
 
-	/* double check that clear address and write pointer are the same */
-	if (clear_ptr != le32_to_cpu(fw_log.buff_write_ptr)) {
-		wl1271_error("Calculate of clear addr Clear = %x, write = %x",
-			     clear_ptr, le32_to_cpu(fw_log.buff_write_ptr));
-	}
-
-	/* indicate FW about Clear buffer */
+	/* Update the read pointer */
 	ret = wlcore_write32(wl, addr + WL18XX_LOGGER_READ_POINT_OFFSET,
-			     fw_log.buff_write_ptr);
+			     clear_ptr);
 free_out:
 	kfree(buffer);
 out:
-- 
2.20.1