2012-02-06 06:23:37

by Kalle Valo

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

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 <[email protected]>
---
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);
}




2012-02-08 09:30:40

by Kalle Valo

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

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 <[email protected]>

Both patches applied.

Kalle

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

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 <[email protected]>
> ---
> +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

2012-02-06 17:48:07

by Kalle Valo

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

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

2012-02-06 12:37:36

by Kalle Valo

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

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 <[email protected]>

[...]

>> + 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

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

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

2012-02-06 06:23:46

by Kalle Valo

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

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 <[email protected]>
---
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);



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

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 <[email protected]>
>
> [...]
>
> >> + 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