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

[Vasanthakumar Thiagarajan <vthiagar@qca.qualcomm.com>]

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