[PATCH 1/2] ath6kl: store firmware logs in skbuffs

[PATCH 1/2] ath6kl: store firmware logs in skbuffs

[Kalle Valo]

Currently firmware logs are stored in a circular buffer, but this was
not very flexible and fragile. It's a lot easier to store logs to struct
skbuffs and store them in a skb queue. Also this makes it possible
to easily increase the buffer size, even dynamically if we so want (but
that's not yet supported).

>From user space point of view nothing should change.

Signed-off-by: Kalle Valo <kvalo@qca.qualcomm.com>
---
 drivers/net/wireless/ath/ath6kl/core.h  |    5 +
 drivers/net/wireless/ath/ath6kl/debug.c |  121 ++++++++++---------------------
 2 files changed, 41 insertions(+), 85 deletions(-)

diff --git a/drivers/net/wireless/ath/ath6kl/core.h b/drivers/net/wireless/ath/ath6kl/core.h
index c4d66e0..9a58214 100644
--- a/drivers/net/wireless/ath/ath6kl/core.h
+++ b/drivers/net/wireless/ath/ath6kl/core.h
@@ -652,10 +652,9 @@ struct ath6kl {
 
 #ifdef CONFIG_ATH6KL_DEBUG
 	struct {
-		struct circ_buf fwlog_buf;
-		spinlock_t fwlog_lock;
-		void *fwlog_tmp;
+		struct sk_buff_head fwlog_queue;
 		u32 fwlog_mask;
+
 		unsigned int dbgfs_diag_reg;
 		u32 diag_reg_addr_wr;
 		u32 diag_reg_val_wr;
diff --git a/drivers/net/wireless/ath/ath6kl/debug.c b/drivers/net/wireless/ath/ath6kl/debug.c
index 6b546dc..9b25cbd 100644
--- a/drivers/net/wireless/ath/ath6kl/debug.c
+++ b/drivers/net/wireless/ath/ath6kl/debug.c
@@ -16,7 +16,7 @@
 
 #include "core.h"
 
-#include <linux/circ_buf.h>
+#include <linux/skbuff.h>
 #include <linux/fs.h>
 #include <linux/vmalloc.h>
 #include <linux/export.h>
@@ -32,9 +32,8 @@ struct ath6kl_fwlog_slot {
 	u8 payload[0];
 };
 
-#define ATH6KL_FWLOG_SIZE 32768
-#define ATH6KL_FWLOG_SLOT_SIZE (sizeof(struct ath6kl_fwlog_slot) + \
-				ATH6KL_FWLOG_PAYLOAD_SIZE)
+#define ATH6KL_FWLOG_MAX_ENTRIES 20
+
 #define ATH6KL_FWLOG_VALID_MASK 0x1ffff
 
 int ath6kl_printk(const char *level, const char *fmt, ...)
@@ -268,105 +267,77 @@ static const struct file_operations fops_war_stats = {
 	.llseek = default_llseek,
 };
 
-static void ath6kl_debug_fwlog_add(struct ath6kl *ar, const void *buf,
-				   size_t buf_len)
-{
-	struct circ_buf *fwlog = &ar->debug.fwlog_buf;
-	size_t space;
-	int i;
-
-	/* entries must all be equal size */
-	if (WARN_ON(buf_len != ATH6KL_FWLOG_SLOT_SIZE))
-		return;
-
-	space = CIRC_SPACE(fwlog->head, fwlog->tail, ATH6KL_FWLOG_SIZE);
-	if (space < buf_len)
-		/* discard oldest slot */
-		fwlog->tail = (fwlog->tail + ATH6KL_FWLOG_SLOT_SIZE) &
-			(ATH6KL_FWLOG_SIZE - 1);
-
-	for (i = 0; i < buf_len; i += space) {
-		space = CIRC_SPACE_TO_END(fwlog->head, fwlog->tail,
-					  ATH6KL_FWLOG_SIZE);
-
-		if ((size_t) space > buf_len - i)
-			space = buf_len - i;
-
-		memcpy(&fwlog->buf[fwlog->head], buf, space);
-		fwlog->head = (fwlog->head + space) & (ATH6KL_FWLOG_SIZE - 1);
-	}
-
-}
-
 void ath6kl_debug_fwlog_event(struct ath6kl *ar, const void *buf, size_t len)
 {
-	struct ath6kl_fwlog_slot *slot = ar->debug.fwlog_tmp;
+	struct ath6kl_fwlog_slot *slot;
+	struct sk_buff *skb;
 	size_t slot_len;
 
 	if (WARN_ON(len > ATH6KL_FWLOG_PAYLOAD_SIZE))
 		return;
 
-	spin_lock_bh(&ar->debug.fwlog_lock);
+	slot_len = sizeof(*slot) + len;
 
+	skb = alloc_skb(slot_len, GFP_KERNEL);
+	if (!skb)
+		return;
+
+	slot = (struct ath6kl_fwlog_slot *) skb_put(skb, slot_len);
 	slot->timestamp = cpu_to_le32(jiffies);
 	slot->length = cpu_to_le32(len);
 	memcpy(slot->payload, buf, len);
 
-	slot_len = sizeof(*slot) + len;
+	spin_lock(&ar->debug.fwlog_queue.lock);
 
-	if (slot_len < ATH6KL_FWLOG_SLOT_SIZE)
-		memset(slot->payload + len, 0,
-		       ATH6KL_FWLOG_SLOT_SIZE - slot_len);
+	__skb_queue_tail(&ar->debug.fwlog_queue, skb);
 
-	ath6kl_debug_fwlog_add(ar, slot, ATH6KL_FWLOG_SLOT_SIZE);
+	/* drop oldest entries */
+	while (skb_queue_len(&ar->debug.fwlog_queue) >
+	       ATH6KL_FWLOG_MAX_ENTRIES) {
+		skb = __skb_dequeue(&ar->debug.fwlog_queue);
+		kfree_skb(skb);
+	}
 
-	spin_unlock_bh(&ar->debug.fwlog_lock);
-}
+	spin_unlock(&ar->debug.fwlog_queue.lock);
 
-static bool ath6kl_debug_fwlog_empty(struct ath6kl *ar)
-{
-	return CIRC_CNT(ar->debug.fwlog_buf.head,
-			ar->debug.fwlog_buf.tail,
-			ATH6KL_FWLOG_SLOT_SIZE) == 0;
+	return;
 }
 
 static ssize_t ath6kl_fwlog_read(struct file *file, char __user *user_buf,
 				 size_t count, loff_t *ppos)
 {
 	struct ath6kl *ar = file->private_data;
-	struct circ_buf *fwlog = &ar->debug.fwlog_buf;
-	size_t len = 0, buf_len = count;
+	struct sk_buff *skb;
 	ssize_t ret_cnt;
+	size_t len = 0;
 	char *buf;
-	int ccnt;
 
-	buf = vmalloc(buf_len);
+	buf = vmalloc(count);
 	if (!buf)
 		return -ENOMEM;
 
 	/* read undelivered logs from firmware */
 	ath6kl_read_fwlogs(ar);
 
-	spin_lock_bh(&ar->debug.fwlog_lock);
+	spin_lock(&ar->debug.fwlog_queue.lock);
 
-	while (len < buf_len && !ath6kl_debug_fwlog_empty(ar)) {
-		ccnt = CIRC_CNT_TO_END(fwlog->head, fwlog->tail,
-				       ATH6KL_FWLOG_SIZE);
+	while ((skb = __skb_dequeue(&ar->debug.fwlog_queue))) {
+		if (skb->len > count - len) {
+			/* not enough space, put skb back and leave */
+			__skb_queue_head(&ar->debug.fwlog_queue, skb);
+			break;
+		}
 
-		if ((size_t) ccnt > buf_len - len)
-			ccnt = buf_len - len;
 
-		memcpy(buf + len, &fwlog->buf[fwlog->tail], ccnt);
-		len += ccnt;
+		memcpy(buf + len, skb->data, skb->len);
+		len += skb->len;
 
-		fwlog->tail = (fwlog->tail + ccnt) &
-			(ATH6KL_FWLOG_SIZE - 1);
+		kfree_skb(skb);
 	}
 
-	spin_unlock_bh(&ar->debug.fwlog_lock);
+	spin_unlock(&ar->debug.fwlog_queue.lock);
 
-	if (WARN_ON(len > buf_len))
-		len = buf_len;
+	/* FIXME: what to do if len == 0? */
 
 	ret_cnt = simple_read_from_buffer(user_buf, count, ppos, buf, len);
 
@@ -1649,17 +1620,7 @@ static const struct file_operations fops_power_params = {
 
 int ath6kl_debug_init(struct ath6kl *ar)
 {
-	ar->debug.fwlog_buf.buf = vmalloc(ATH6KL_FWLOG_SIZE);
-	if (ar->debug.fwlog_buf.buf == NULL)
-		return -ENOMEM;
-
-	ar->debug.fwlog_tmp = kmalloc(ATH6KL_FWLOG_SLOT_SIZE, GFP_KERNEL);
-	if (ar->debug.fwlog_tmp == NULL) {
-		vfree(ar->debug.fwlog_buf.buf);
-		return -ENOMEM;
-	}
-
-	spin_lock_init(&ar->debug.fwlog_lock);
+	skb_queue_head_init(&ar->debug.fwlog_queue);
 
 	/*
 	 * Actually we are lying here but don't know how to read the mask
@@ -1669,11 +1630,8 @@ int ath6kl_debug_init(struct ath6kl *ar)
 
 	ar->debugfs_phy = debugfs_create_dir("ath6kl",
 					     ar->wiphy->debugfsdir);
-	if (!ar->debugfs_phy) {
-		vfree(ar->debug.fwlog_buf.buf);
-		kfree(ar->debug.fwlog_tmp);
+	if (!ar->debugfs_phy)
 		return -ENOMEM;
-	}
 
 	debugfs_create_file("tgt_stats", S_IRUSR, ar->debugfs_phy, ar,
 			    &fops_tgt_stats);
@@ -1740,8 +1698,7 @@ int ath6kl_debug_init(struct ath6kl *ar)
 
 void ath6kl_debug_cleanup(struct ath6kl *ar)
 {
-	vfree(ar->debug.fwlog_buf.buf);
-	kfree(ar->debug.fwlog_tmp);
+	skb_queue_purge(&ar->debug.fwlog_queue);
 	kfree(ar->debug.roam_tbl);
 }
 

[PATCH 2/2] ath6kl: add blocking debugfs file for retrieving firmware logs

[Kalle Valo]

When debugging firmware issues it's not always enough to get
the latest firmware logs, sometimes we need to get logs from a longer
period. To make this possible, add a debugfs file named fwlog_block. When
reading from this file ath6kl will send firmware logs whenever available
and otherwise it will block and wait for new logs.

Signed-off-by: Kalle Valo <kvalo@qca.qualcomm.com>
---
 drivers/net/wireless/ath/ath6kl/core.h  |    3 +
 drivers/net/wireless/ath/ath6kl/debug.c |  104 +++++++++++++++++++++++++++++++
 2 files changed, 106 insertions(+), 1 deletions(-)

diff --git a/drivers/net/wireless/ath/ath6kl/core.h b/drivers/net/wireless/ath/ath6kl/core.h
index 9a58214..4ff06a3 100644
--- a/drivers/net/wireless/ath/ath6kl/core.h
+++ b/drivers/net/wireless/ath/ath6kl/core.h
@@ -653,6 +653,9 @@ struct ath6kl {
 #ifdef CONFIG_ATH6KL_DEBUG
 	struct {
 		struct sk_buff_head fwlog_queue;
+		struct completion fwlog_completion;
+		bool fwlog_open;
+
 		u32 fwlog_mask;
 
 		unsigned int dbgfs_diag_reg;
diff --git a/drivers/net/wireless/ath/ath6kl/debug.c b/drivers/net/wireless/ath/ath6kl/debug.c
index 9b25cbd..7f94eda 100644
--- a/drivers/net/wireless/ath/ath6kl/debug.c
+++ b/drivers/net/wireless/ath/ath6kl/debug.c
@@ -290,6 +290,7 @@ void ath6kl_debug_fwlog_event(struct ath6kl *ar, const void *buf, size_t len)
 	spin_lock(&ar->debug.fwlog_queue.lock);
 
 	__skb_queue_tail(&ar->debug.fwlog_queue, skb);
+	complete(&ar->debug.fwlog_completion);
 
 	/* drop oldest entries */
 	while (skb_queue_len(&ar->debug.fwlog_queue) >
@@ -303,6 +304,28 @@ void ath6kl_debug_fwlog_event(struct ath6kl *ar, const void *buf, size_t len)
 	return;
 }
 
+static int ath6kl_fwlog_open(struct inode *inode, struct file *file)
+{
+	struct ath6kl *ar = inode->i_private;
+
+	if (ar->debug.fwlog_open)
+		return -EBUSY;
+
+	ar->debug.fwlog_open = true;
+
+	file->private_data = inode->i_private;
+	return 0;
+}
+
+static int ath6kl_fwlog_release(struct inode *inode, struct file *file)
+{
+	struct ath6kl *ar = inode->i_private;
+
+	ar->debug.fwlog_open = false;
+
+	return 0;
+}
+
 static ssize_t ath6kl_fwlog_read(struct file *file, char __user *user_buf,
 				 size_t count, loff_t *ppos)
 {
@@ -347,12 +370,87 @@ static ssize_t ath6kl_fwlog_read(struct file *file, char __user *user_buf,
 }
 
 static const struct file_operations fops_fwlog = {
-	.open = ath6kl_debugfs_open,
+	.open = ath6kl_fwlog_open,
+	.release = ath6kl_fwlog_release,
 	.read = ath6kl_fwlog_read,
 	.owner = THIS_MODULE,
 	.llseek = default_llseek,
 };
 
+static ssize_t ath6kl_fwlog_block_read(struct file *file,
+				       char __user *user_buf,
+				       size_t count,
+				       loff_t *ppos)
+{
+	struct ath6kl *ar = file->private_data;
+	struct sk_buff *skb;
+	ssize_t ret_cnt;
+	size_t len = 0, not_copied;
+	char *buf;
+	int ret;
+
+	buf = vmalloc(count);
+	if (!buf)
+		return -ENOMEM;
+
+	spin_lock(&ar->debug.fwlog_queue.lock);
+
+	if (skb_queue_len(&ar->debug.fwlog_queue) == 0) {
+		/* we must init under queue lock */
+		init_completion(&ar->debug.fwlog_completion);
+
+		spin_unlock(&ar->debug.fwlog_queue.lock);
+
+		ret = wait_for_completion_interruptible(
+			&ar->debug.fwlog_completion);
+		if (ret == -ERESTARTSYS)
+			return ret;
+
+		spin_lock(&ar->debug.fwlog_queue.lock);
+	}
+
+	while ((skb = __skb_dequeue(&ar->debug.fwlog_queue))) {
+		if (skb->len > count - len) {
+			/* not enough space, put skb back and leave */
+			__skb_queue_head(&ar->debug.fwlog_queue, skb);
+			break;
+		}
+
+
+		memcpy(buf + len, skb->data, skb->len);
+		len += skb->len;
+
+		kfree_skb(skb);
+	}
+
+	spin_unlock(&ar->debug.fwlog_queue.lock);
+
+	/* FIXME: what to do if len == 0? */
+
+	not_copied = copy_to_user(user_buf, buf, len);
+	if (not_copied != 0) {
+		ret_cnt = -EFAULT;
+		goto out;
+	}
+
+	*ppos = *ppos + len;
+
+	ret_cnt = len;
+
+out:
+	vfree(buf);
+
+	return ret_cnt;
+}
+
+static const struct file_operations fops_fwlog_block = {
+	.open = ath6kl_fwlog_open,
+	.release = ath6kl_fwlog_release,
+	.read = ath6kl_fwlog_block_read,
+	.owner = THIS_MODULE,
+	.llseek = default_llseek,
+};
+
 static ssize_t ath6kl_fwlog_mask_read(struct file *file, char __user *user_buf,
 				      size_t count, loff_t *ppos)
 {
@@ -1621,6 +1719,7 @@ static const struct file_operations fops_power_params = {
 int ath6kl_debug_init(struct ath6kl *ar)
 {
 	skb_queue_head_init(&ar->debug.fwlog_queue);
+	init_completion(&ar->debug.fwlog_completion);
 
 	/*
 	 * Actually we are lying here but don't know how to read the mask
@@ -1645,6 +1744,9 @@ int ath6kl_debug_init(struct ath6kl *ar)
 	debugfs_create_file("fwlog", S_IRUSR, ar->debugfs_phy, ar,
 			    &fops_fwlog);
 
+	debugfs_create_file("fwlog_block", S_IRUSR, ar->debugfs_phy, ar,
+			    &fops_fwlog_block);
+
 	debugfs_create_file("fwlog_mask", S_IRUSR | S_IWUSR, ar->debugfs_phy,
 			    ar, &fops_fwlog_mask);
 

Re: [PATCH 2/2] ath6kl: add blocking debugfs file for retrieving firmware logs

[Vasanthakumar Thiagarajan]

On Mon, Feb 06, 2012 at 08:23:40AM +0200, Kalle Valo wrote:
> When debugging firmware issues it's not always enough to get
> the latest firmware logs, sometimes we need to get logs from a longer
> period. To make this possible, add a debugfs file named fwlog_block. When
> reading from this file ath6kl will send firmware logs whenever available
> and otherwise it will block and wait for new logs.
> 
> Signed-off-by: Kalle Valo <kvalo@qca.qualcomm.com>
> ---
> +static ssize_t ath6kl_fwlog_block_read(struct file *file,
> +				       char __user *user_buf,
> +				       size_t count,
> +				       loff_t *ppos)
> +{
> +	struct ath6kl *ar = file->private_data;
> +	struct sk_buff *skb;
> +	ssize_t ret_cnt;
> +	size_t len = 0, not_copied;
> +	char *buf;
> +	int ret;
> +
> +	buf = vmalloc(count);
> +	if (!buf)
> +		return -ENOMEM;
> +
> +	spin_lock(&ar->debug.fwlog_queue.lock);
> +
> +	if (skb_queue_len(&ar->debug.fwlog_queue) == 0) {
> +		/* we must init under queue lock */
> +		init_completion(&ar->debug.fwlog_completion);
> +
> +		spin_unlock(&ar->debug.fwlog_queue.lock);
> +
> +		ret = wait_for_completion_interruptible(
> +			&ar->debug.fwlog_completion);
> +		if (ret == -ERESTARTSYS)
> +			return ret;
> +
> +		spin_lock(&ar->debug.fwlog_queue.lock);
> +	}
> +
> +	while ((skb = __skb_dequeue(&ar->debug.fwlog_queue))) {
> +		if (skb->len > count - len) {
> +			/* not enough space, put skb back and leave */
> +			__skb_queue_head(&ar->debug.fwlog_queue, skb);
> +			break;
> +		}
> +
> +
> +		memcpy(buf + len, skb->data, skb->len);
> +		len += skb->len;
> +
> +		kfree_skb(skb);
> +	}
> +
> +	spin_unlock(&ar->debug.fwlog_queue.lock);
> +
> +	/* FIXME: what to do if len == 0? */
> +
> +	not_copied = copy_to_user(user_buf, buf, len);
> +	if (not_copied != 0) {
> +		ret_cnt = -EFAULT;
> +		goto out;
> +	}
> +
> +	*ppos = *ppos + len;

Why not to use simple_read_from_buffer()?, looks like it can also
takes care of len == 0 case in the following check.

if (pos >= available || !count)
        return 0;

when available (len) is 0, pos = available with
ath6kl_fwlog_block_read().

Vasanth

Re: [PATCH 2/2] ath6kl: add blocking debugfs file for retrieving firmware logs

[Kalle Valo]

On 02/06/2012 11:06 AM, Vasanthakumar Thiagarajan wrote:
> On Mon, Feb 06, 2012 at 08:23:40AM +0200, Kalle Valo wrote:
>> When debugging firmware issues it's not always enough to get
>> the latest firmware logs, sometimes we need to get logs from a longer
>> period. To make this possible, add a debugfs file named fwlog_block. When
>> reading from this file ath6kl will send firmware logs whenever available
>> and otherwise it will block and wait for new logs.
>>
>> Signed-off-by: Kalle Valo <kvalo@qca.qualcomm.com>

[...]

>> +	not_copied = copy_to_user(user_buf, buf, len);
>> +	if (not_copied != 0) {
>> +		ret_cnt = -EFAULT;
>> +		goto out;
>> +	}
>> +
>> +	*ppos = *ppos + len;
> 
> Why not to use simple_read_from_buffer()?, looks like it can also
> takes care of len == 0 case in the following check.
> 
> if (pos >= available || !count)
>         return 0;
> 
> when available (len) is 0, pos = available with
> ath6kl_fwlog_block_read().

I actually used simple_read_from_buffer() first, but the problem is that
it assumes that there's just one buffer from which the data is copied.
But in this case there can be multiple buffers from which I copy data.

Ok, that was a bit confusing, let's try to explain a bit differently :)

If 'ppos > 0' (for example during the second function call)
simple_read_from_buffer() will try to copy from 'user_buf + ppos' but I
would want to copy from 'user_buf'.

Kalle

Re: [PATCH 2/2] ath6kl: add blocking debugfs file for retrieving firmware logs

[Vasanthakumar Thiagarajan]

On Mon, Feb 06, 2012 at 02:37:29PM +0200, Kalle Valo wrote:
> On 02/06/2012 11:06 AM, Vasanthakumar Thiagarajan wrote:
> > On Mon, Feb 06, 2012 at 08:23:40AM +0200, Kalle Valo wrote:
> >> When debugging firmware issues it's not always enough to get
> >> the latest firmware logs, sometimes we need to get logs from a longer
> >> period. To make this possible, add a debugfs file named fwlog_block. When
> >> reading from this file ath6kl will send firmware logs whenever available
> >> and otherwise it will block and wait for new logs.
> >>
> >> Signed-off-by: Kalle Valo <kvalo@qca.qualcomm.com>
> 
> [...]
> 
> >> +	not_copied = copy_to_user(user_buf, buf, len);
> >> +	if (not_copied != 0) {
> >> +		ret_cnt = -EFAULT;
> >> +		goto out;
> >> +	}
> >> +
> >> +	*ppos = *ppos + len;
> > 
> > Why not to use simple_read_from_buffer()?, looks like it can also
> > takes care of len == 0 case in the following check.
> > 
> > if (pos >= available || !count)
> >         return 0;
> > 
> > when available (len) is 0, pos = available with
> > ath6kl_fwlog_block_read().
> 
> I actually used simple_read_from_buffer() first, but the problem is that
> it assumes that there's just one buffer from which the data is copied.
> But in this case there can be multiple buffers from which I copy data.
> 
> Ok, that was a bit confusing, let's try to explain a bit differently :)
> 
> If 'ppos > 0' (for example during the second function call)
> simple_read_from_buffer() will try to copy from 'user_buf + ppos' but I
> would want to copy from 'user_buf'.

I think you mean s/user_buf/buf. Are you not making sure that the
length of the data is not more than the requested one which is
passed to copy_to_user() so that read() is always called with
*ppos=0?. The following code seems to do that

 while ((skb = __skb_dequeue(&ar->debug.fwlog_queue))) {
+               if (skb->len > count - len) {
+                       /* not enough space, put skb back and leave
*/
+                       __skb_queue_head(&ar->debug.fwlog_queue,
skb);
+                       break;
+               }

May be my understanding is wrong. 

Vasanth

Re: [PATCH 2/2] ath6kl: add blocking debugfs file for retrieving firmware logs

[Kalle Valo]

On 02/06/2012 05:05 PM, Vasanthakumar Thiagarajan wrote:
> On Mon, Feb 06, 2012 at 02:37:29PM +0200, Kalle Valo wrote:
>> On 02/06/2012 11:06 AM, Vasanthakumar Thiagarajan wrote:
>>
>>> Why not to use simple_read_from_buffer()?, looks like it can also
>>> takes care of len == 0 case in the following check.
>>>
>>> if (pos >= available || !count)
>>>         return 0;
>>>
>>> when available (len) is 0, pos = available with
>>> ath6kl_fwlog_block_read().
>>
>> I actually used simple_read_from_buffer() first, but the problem is that
>> it assumes that there's just one buffer from which the data is copied.
>> But in this case there can be multiple buffers from which I copy data.
>>
>> Ok, that was a bit confusing, let's try to explain a bit differently :)
>>
>> If 'ppos > 0' (for example during the second function call)
>> simple_read_from_buffer() will try to copy from 'user_buf + ppos' but I
>> would want to copy from 'user_buf'.
> 
> I think you mean s/user_buf/buf.

Correct.

> Are you not making sure that the
> length of the data is not more than the requested one which is
> passed to copy_to_user() so that read() is always called with
> *ppos=0?. The following code seems to do that

But the function is called multiple times with increasing values of
*ppos as more data is returned to user space:

[  100.303747] ath6kl_fwlog_block_read(): *ppos 0
[  100.305252] ath6kl_fwlog_block_read(): *ppos 30116
[  101.768947] ath6kl_fwlog_block_read(): *ppos 31624
[  117.027469] ath6kl_fwlog_block_read(): *ppos 33124
[  117.090146] ath6kl_fwlog_block_read(): *ppos 34628
[  117.172338] ath6kl_fwlog_block_read(): *ppos 36128

So I can't assume that *ppos = 0.

Kalle

Re: [PATCH 2/2] ath6kl: add blocking debugfs file for retrieving firmware logs

[Vasanthakumar Thiagarajan]

On Mon, Feb 06, 2012 at 07:47:26PM +0200, Kalle Valo wrote:
> > Are you not making sure that the
> > length of the data is not more than the requested one which is
> > passed to copy_to_user() so that read() is always called with
> > *ppos=0?. The following code seems to do that
> 
> But the function is called multiple times with increasing values of
> *ppos as more data is returned to user space:
> 
> [  100.303747] ath6kl_fwlog_block_read(): *ppos 0
> [  100.305252] ath6kl_fwlog_block_read(): *ppos 30116
> [  101.768947] ath6kl_fwlog_block_read(): *ppos 31624
> [  117.027469] ath6kl_fwlog_block_read(): *ppos 33124
> [  117.090146] ath6kl_fwlog_block_read(): *ppos 34628
> [  117.172338] ath6kl_fwlog_block_read(): *ppos 36128
> 
> So I can't assume that *ppos = 0.

Right, unless we say we have no more data to pass
(when read() returns 0), *ppos would be keep on increasing
by the number of bytes copied to user space. Updating *ppos
just looks like a formality here though. But I'm also not
sure about doing it cleanly.

Vasanth

Re: [PATCH 1/2] ath6kl: store firmware logs in skbuffs

[Kalle Valo]

On 02/06/2012 08:23 AM, Kalle Valo wrote:
> Currently firmware logs are stored in a circular buffer, but this was
> not very flexible and fragile. It's a lot easier to store logs to struct
> skbuffs and store them in a skb queue. Also this makes it possible
> to easily increase the buffer size, even dynamically if we so want (but
> that's not yet supported).
> 
>>From user space point of view nothing should change.
> 
> Signed-off-by: Kalle Valo <kvalo@qca.qualcomm.com>

Both patches applied.

Kalle