[PATCH v3] ath10k: Add the target register read/write and memory dump debugfs interface

[Li, Yanbo]

Hi Michal,

Thanks for your comments. answered as below,

> -----Original Message-----
> From: Michal Kazior [mailto:michal.kazior at tieto.com]
> Sent: Tuesday, October 21, 2014 2:56 PM
> To: Li, Yanbo
> Cc: Valo, Kalle; ath10k-devel; Balasubramanian, Senthil Kumar;
> dreamfly281 at gmail.com; linux-wireless; ath10k at lists.infradead.org
> Subject: Re: [PATCH v3] Add the target register read/write and memory dump
> debugfs interface
> 
> You're missing "ath10k: " prefix in the patch subject.
> 
Change the subject, :)
> 
> On 20 October 2014 18:38, Yanbo Li <yanbol at qti.qualcomm.com> wrote:
> > The debugfs interface reg_addr&reg_val used to read and write the
> > target register.
> > The interface mem_addr&mem_val used to dump the targer memory and also
> > can be used to assign value to target memory
> >
> > The basic usage explain as below:
> >
> > Register read/write:
> > reg_addr       (the register address)                    (read&write)
> > reg_value      (the register value,  output is ASCII)    (read&write)
> >
> > Read operation:
> > 1. Write the reg address to reg_addr
> >    IE:  echo 0x100000 > reg_addr
> > 2. Read the value from reg_value
> >    IE:  cat reg_value
> > Write operation:
> > 1. Write the reg address to reg_addr IE:  echo 0x100000 > reg_addr 2.
> > Write the value to the reg_value  IE:  echo  0x2400 > reg_value
> >
> > Target memory dump:
> > mem_addr (the memory address the length also can used as the second
> > paramenter address length)                              (read&write)
> > mem_value (the content of the memory with the length, output is binary)
> >                                                         (read&write)
> > Read operation:
> > 1. Write the address and length to mem_addr
> >    IE:  echo 0x400000 1024 > mem_addr
> 
> So mem_addr is actually used to store both _addr_ and _length_. I think length
> could be skipped and be read implicitly from each read() request along with the
> offset. This way userspace would be able to determine read length on demand, e.g.
> via "dd if=mem_value bs=1024 count=4" (read 4 KiB of data). If you want to see
> hex, you would pipe it through "hexdump" or "xxd" or your other favourite
> hexdump program.
> Or even better - you could drop the mem_addr altogether and expect userspace to
> seek & read/write data it wants via, e.g. `dd` program:
> 
>   dd if=mem_value bs=1 count=1024 skip=$(printf "%d" 0x400000) | xxd -g1
>   echo 0x01020304 | xxd -r | dd of=mem_value bs=1 seek=$(printf "%d" 0x400400)
> 
Agree with you, that will be easy to access and avoid the one variable (mem_addr)store two different values. 
> 
> > 2. Read the value from mem value, the output is binary format
> >    IE:  hexdump mem_value
> > Write operation:
> > 1. Write the start mem address to mem_addr
> >    IE:  echo 0x400000 > mem_addr
> > 2. Dump the binary value to the mem_value
> >    IE:  echo  0x2400 > mem_value or echo 0x2400 0x3400 ... > mem_addr
> >    (the value number should be separated with spacebar)
> 
> I don't really like the idea of input being different than the output, but maybe that's
> just me.
> 

Hmm, I will implement  another version to keep the read and write use the same interface,
the extra effort is the user need edit the binary file to construct the input number.
> 
> [...]
> > +static ssize_t ath10k_mem_value_write(struct file *file,
> > +                                     const char __user *user_buf,
> > +                                     size_t count, loff_t *ppos) {
> > +       struct ath10k *ar = file->private_data;
> > +       u8 *buf;
> > +       __le32 *value;
> > +       char *sptr, *token;
> > +       int i, ret;
> > +       u32 mem_addr;
> > +       u32 mem_val;
> > +
> > +       mem_addr =  ar->debug.mem_addr;
> 
> There's double space following "=".
> 
Good catch.
> 
> [...]
> > +       i = 0;
> > +       while (sptr) {
> > +               if (!sptr)
> > +                       break;
> > +
> > +               token = strsep(&sptr, " ");
> > +
> > +               ret = kstrtou32(token, 0, &mem_val);
> > +               if (ret)
> > +                       goto err_free_value;
> > +
> > +               value[i] = mem_val;
> > +               i++;
> > +       }
> > +
> > +       ret = ath10k_hif_diag_write(ar, mem_addr, (const void *)value,
> > +                                   i * sizeof(mem_val));
> > +       if (ret) {
> > +               ath10k_warn(ar, "failed to write address 0x%08x via diagnose window
> from debugfs: %d\n",
> > +                           mem_addr, ret);
> > +               goto err_free_value;
> > +       }
> 
> I believe userspace may call write() multiple number of times with different offsets
> and fragment the data breaking it in the middle of your string hex words. This
> code will most likely fail with larger chunks of data or you can try to simulate the
> fragmentation with:
> 
>   echo 0x0001 0x0002 | dd bs=1 of=mem_value
> 
> (Please correct me if I'm wrong :-)
> 
Not full understand what’s you mean, do you mean this segment need be protect with lock 
to avoid multi write operation access the mem_value at the same time?

> 
> Michał