Hi Michal, Thanks for your comments. answered as below, > -----Original Message----- > From: Michal Kazior [mailto:michal.kazior@xxxxxxxxx] > Sent: Tuesday, October 21, 2014 2:56 PM > To: Li, Yanbo > Cc: Valo, Kalle; ath10k-devel; Balasubramanian, Senthil Kumar; > dreamfly281@xxxxxxxxx; linux-wireless; ath10k@xxxxxxxxxxxxxxxxxxx > 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@xxxxxxxxxxxxxxxx> wrote: > > The debugfs interface reg_addr®_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ł ��.n��������+%������w��{.n�����{���zW����ܨ}���Ơz�j:+v�����w����ޙ��&�)ߡ�a����z�ޗ���ݢj��w�f
