On 21 October 2014 17:10, Li, Yanbo <yanbol@xxxxxxxxxxxxxxxx> wrote: >> -----Original Message----- >> From: Michal Kazior [mailto:michal.kazior@xxxxxxxxx] >> On 20 October 2014 18:38, Yanbo Li <yanbol@xxxxxxxxxxxxxxxx> wrote: [...] >> > 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. There's plenty of tools that can do that for you, e.g. xxd. If mem_value would expect binary input you could do: echo 0x01 0x02 0x03 0x04 | xxd -r > mem_value This would effectively put 4 bytes into the mem_value. Or you can use perl and pack() to create endianess aware byte streams. [...] >> > + 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? This is not a problem with locking. The problem is you assume ath10k_mem_value_write() is going to be called once. I'm pretty sure that the above example (dd bs=1) will yield many ath10k_mem_value_write() each with a single-byte buffer. This means two things: a) kstrotu32() parsing will break b) you always call ath10k_hif_diag_write() with mem_addr without any offset so even if (a) is ignored you'll just keep overwriting first word in target memory over and over again Having binary input (instead of hex string words) is going to help with (a). To fix (b) you'll need to add *ppos to mem_addr when you call ath10k_hif_diag_write(). Michał -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
