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

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

[Li, Yanbo]

SGkgTWljaGFsLA0KDQpUaGFua3MgZm9yIHlvdXIgY29tbWVudHMuIGFuc3dlcmVkIGFzIGJlbG93
LA0KDQo+IC0tLS0tT3JpZ2luYWwgTWVzc2FnZS0tLS0tDQo+IEZyb206IE1pY2hhbCBLYXppb3Ig
W21haWx0bzptaWNoYWwua2F6aW9yQHRpZXRvLmNvbV0NCj4gU2VudDogVHVlc2RheSwgT2N0b2Jl
ciAyMSwgMjAxNCAyOjU2IFBNDQo+IFRvOiBMaSwgWWFuYm8NCj4gQ2M6IFZhbG8sIEthbGxlOyBh
dGgxMGstZGV2ZWw7IEJhbGFzdWJyYW1hbmlhbiwgU2VudGhpbCBLdW1hcjsNCj4gZHJlYW1mbHky
ODFAZ21haWwuY29tOyBsaW51eC13aXJlbGVzczsgYXRoMTBrQGxpc3RzLmluZnJhZGVhZC5vcmcN
Cj4gU3ViamVjdDogUmU6IFtQQVRDSCB2M10gQWRkIHRoZSB0YXJnZXQgcmVnaXN0ZXIgcmVhZC93
cml0ZSBhbmQgbWVtb3J5IGR1bXANCj4gZGVidWdmcyBpbnRlcmZhY2UNCj4gDQo+IFlvdSdyZSBt
aXNzaW5nICJhdGgxMGs6ICIgcHJlZml4IGluIHRoZSBwYXRjaCBzdWJqZWN0Lg0KPiANCkNoYW5n
ZSB0aGUgc3ViamVjdCwgOikNCj4gDQo+IE9uIDIwIE9jdG9iZXIgMjAxNCAxODozOCwgWWFuYm8g
TGkgPHlhbmJvbEBxdGkucXVhbGNvbW0uY29tPiB3cm90ZToNCj4gPiBUaGUgZGVidWdmcyBpbnRl
cmZhY2UgcmVnX2FkZHImcmVnX3ZhbCB1c2VkIHRvIHJlYWQgYW5kIHdyaXRlIHRoZQ0KPiA+IHRh
cmdldCByZWdpc3Rlci4NCj4gPiBUaGUgaW50ZXJmYWNlIG1lbV9hZGRyJm1lbV92YWwgdXNlZCB0
byBkdW1wIHRoZSB0YXJnZXIgbWVtb3J5IGFuZCBhbHNvDQo+ID4gY2FuIGJlIHVzZWQgdG8gYXNz
aWduIHZhbHVlIHRvIHRhcmdldCBtZW1vcnkNCj4gPg0KPiA+IFRoZSBiYXNpYyB1c2FnZSBleHBs
YWluIGFzIGJlbG93Og0KPiA+DQo+ID4gUmVnaXN0ZXIgcmVhZC93cml0ZToNCj4gPiByZWdfYWRk
ciAgICAgICAodGhlIHJlZ2lzdGVyIGFkZHJlc3MpICAgICAgICAgICAgICAgICAgICAocmVhZCZ3
cml0ZSkNCj4gPiByZWdfdmFsdWUgICAgICAodGhlIHJlZ2lzdGVyIHZhbHVlLCAgb3V0cHV0IGlz
IEFTQ0lJKSAgICAocmVhZCZ3cml0ZSkNCj4gPg0KPiA+IFJlYWQgb3BlcmF0aW9uOg0KPiA+IDEu
IFdyaXRlIHRoZSByZWcgYWRkcmVzcyB0byByZWdfYWRkcg0KPiA+ICAgIElFOiAgZWNobyAweDEw
MDAwMCA+IHJlZ19hZGRyDQo+ID4gMi4gUmVhZCB0aGUgdmFsdWUgZnJvbSByZWdfdmFsdWUNCj4g
PiAgICBJRTogIGNhdCByZWdfdmFsdWUNCj4gPiBXcml0ZSBvcGVyYXRpb246DQo+ID4gMS4gV3Jp
dGUgdGhlIHJlZyBhZGRyZXNzIHRvIHJlZ19hZGRyIElFOiAgZWNobyAweDEwMDAwMCA+IHJlZ19h
ZGRyIDIuDQo+ID4gV3JpdGUgdGhlIHZhbHVlIHRvIHRoZSByZWdfdmFsdWUgIElFOiAgZWNobyAg
MHgyNDAwID4gcmVnX3ZhbHVlDQo+ID4NCj4gPiBUYXJnZXQgbWVtb3J5IGR1bXA6DQo+ID4gbWVt
X2FkZHIgKHRoZSBtZW1vcnkgYWRkcmVzcyB0aGUgbGVuZ3RoIGFsc28gY2FuIHVzZWQgYXMgdGhl
IHNlY29uZA0KPiA+IHBhcmFtZW50ZXIgYWRkcmVzcyBsZW5ndGgpICAgICAgICAgICAgICAgICAg
ICAgICAgICAgICAgKHJlYWQmd3JpdGUpDQo+ID4gbWVtX3ZhbHVlICh0aGUgY29udGVudCBvZiB0
aGUgbWVtb3J5IHdpdGggdGhlIGxlbmd0aCwgb3V0cHV0IGlzIGJpbmFyeSkNCj4gPiAgICAgICAg
ICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgIChyZWFkJndy
aXRlKQ0KPiA+IFJlYWQgb3BlcmF0aW9uOg0KPiA+IDEuIFdyaXRlIHRoZSBhZGRyZXNzIGFuZCBs
ZW5ndGggdG8gbWVtX2FkZHINCj4gPiAgICBJRTogIGVjaG8gMHg0MDAwMDAgMTAyNCA+IG1lbV9h
ZGRyDQo+IA0KPiBTbyBtZW1fYWRkciBpcyBhY3R1YWxseSB1c2VkIHRvIHN0b3JlIGJvdGggX2Fk
ZHJfIGFuZCBfbGVuZ3RoXy4gSSB0aGluayBsZW5ndGgNCj4gY291bGQgYmUgc2tpcHBlZCBhbmQg
YmUgcmVhZCBpbXBsaWNpdGx5IGZyb20gZWFjaCByZWFkKCkgcmVxdWVzdCBhbG9uZyB3aXRoIHRo
ZQ0KPiBvZmZzZXQuIFRoaXMgd2F5IHVzZXJzcGFjZSB3b3VsZCBiZSBhYmxlIHRvIGRldGVybWlu
ZSByZWFkIGxlbmd0aCBvbiBkZW1hbmQsIGUuZy4NCj4gdmlhICJkZCBpZj1tZW1fdmFsdWUgYnM9
MTAyNCBjb3VudD00IiAocmVhZCA0IEtpQiBvZiBkYXRhKS4gSWYgeW91IHdhbnQgdG8gc2VlDQo+
IGhleCwgeW91IHdvdWxkIHBpcGUgaXQgdGhyb3VnaCAiaGV4ZHVtcCIgb3IgInh4ZCIgb3IgeW91
ciBvdGhlciBmYXZvdXJpdGUNCj4gaGV4ZHVtcCBwcm9ncmFtLg0KPiBPciBldmVuIGJldHRlciAt
IHlvdSBjb3VsZCBkcm9wIHRoZSBtZW1fYWRkciBhbHRvZ2V0aGVyIGFuZCBleHBlY3QgdXNlcnNw
YWNlIHRvDQo+IHNlZWsgJiByZWFkL3dyaXRlIGRhdGEgaXQgd2FudHMgdmlhLCBlLmcuIGBkZGAg
cHJvZ3JhbToNCj4gDQo+ICAgZGQgaWY9bWVtX3ZhbHVlIGJzPTEgY291bnQ9MTAyNCBza2lwPSQo
cHJpbnRmICIlZCIgMHg0MDAwMDApIHwgeHhkIC1nMQ0KPiAgIGVjaG8gMHgwMTAyMDMwNCB8IHh4
ZCAtciB8IGRkIG9mPW1lbV92YWx1ZSBicz0xIHNlZWs9JChwcmludGYgIiVkIiAweDQwMDQwMCkN
Cj4gDQpBZ3JlZSB3aXRoIHlvdSwgdGhhdCB3aWxsIGJlIGVhc3kgdG8gYWNjZXNzIGFuZCBhdm9p
ZCB0aGUgb25lIHZhcmlhYmxlIChtZW1fYWRkcilzdG9yZSB0d28gZGlmZmVyZW50IHZhbHVlcy4g
DQo+IA0KPiA+IDIuIFJlYWQgdGhlIHZhbHVlIGZyb20gbWVtIHZhbHVlLCB0aGUgb3V0cHV0IGlz
IGJpbmFyeSBmb3JtYXQNCj4gPiAgICBJRTogIGhleGR1bXAgbWVtX3ZhbHVlDQo+ID4gV3JpdGUg
b3BlcmF0aW9uOg0KPiA+IDEuIFdyaXRlIHRoZSBzdGFydCBtZW0gYWRkcmVzcyB0byBtZW1fYWRk
cg0KPiA+ICAgIElFOiAgZWNobyAweDQwMDAwMCA+IG1lbV9hZGRyDQo+ID4gMi4gRHVtcCB0aGUg
YmluYXJ5IHZhbHVlIHRvIHRoZSBtZW1fdmFsdWUNCj4gPiAgICBJRTogIGVjaG8gIDB4MjQwMCA+
IG1lbV92YWx1ZSBvciBlY2hvIDB4MjQwMCAweDM0MDAgLi4uID4gbWVtX2FkZHINCj4gPiAgICAo
dGhlIHZhbHVlIG51bWJlciBzaG91bGQgYmUgc2VwYXJhdGVkIHdpdGggc3BhY2ViYXIpDQo+IA0K
PiBJIGRvbid0IHJlYWxseSBsaWtlIHRoZSBpZGVhIG9mIGlucHV0IGJlaW5nIGRpZmZlcmVudCB0
aGFuIHRoZSBvdXRwdXQsIGJ1dCBtYXliZSB0aGF0J3MNCj4ganVzdCBtZS4NCj4gDQoNCkhtbSwg
SSB3aWxsIGltcGxlbWVudCAgYW5vdGhlciB2ZXJzaW9uIHRvIGtlZXAgdGhlIHJlYWQgYW5kIHdy
aXRlIHVzZSB0aGUgc2FtZSBpbnRlcmZhY2UsDQp0aGUgZXh0cmEgZWZmb3J0IGlzIHRoZSB1c2Vy
IG5lZWQgZWRpdCB0aGUgYmluYXJ5IGZpbGUgdG8gY29uc3RydWN0IHRoZSBpbnB1dCBudW1iZXIu
DQo+IA0KPiBbLi4uXQ0KPiA+ICtzdGF0aWMgc3NpemVfdCBhdGgxMGtfbWVtX3ZhbHVlX3dyaXRl
KHN0cnVjdCBmaWxlICpmaWxlLA0KPiA+ICsgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAg
ICAgICAgY29uc3QgY2hhciBfX3VzZXIgKnVzZXJfYnVmLA0KPiA+ICsgICAgICAgICAgICAgICAg
ICAgICAgICAgICAgICAgICAgICAgc2l6ZV90IGNvdW50LCBsb2ZmX3QgKnBwb3MpIHsNCj4gPiAr
ICAgICAgIHN0cnVjdCBhdGgxMGsgKmFyID0gZmlsZS0+cHJpdmF0ZV9kYXRhOw0KPiA+ICsgICAg
ICAgdTggKmJ1ZjsNCj4gPiArICAgICAgIF9fbGUzMiAqdmFsdWU7DQo+ID4gKyAgICAgICBjaGFy
ICpzcHRyLCAqdG9rZW47DQo+ID4gKyAgICAgICBpbnQgaSwgcmV0Ow0KPiA+ICsgICAgICAgdTMy
IG1lbV9hZGRyOw0KPiA+ICsgICAgICAgdTMyIG1lbV92YWw7DQo+ID4gKw0KPiA+ICsgICAgICAg
bWVtX2FkZHIgPSAgYXItPmRlYnVnLm1lbV9hZGRyOw0KPiANCj4gVGhlcmUncyBkb3VibGUgc3Bh
Y2UgZm9sbG93aW5nICI9Ii4NCj4gDQpHb29kIGNhdGNoLg0KPiANCj4gWy4uLl0NCj4gPiArICAg
ICAgIGkgPSAwOw0KPiA+ICsgICAgICAgd2hpbGUgKHNwdHIpIHsNCj4gPiArICAgICAgICAgICAg
ICAgaWYgKCFzcHRyKQ0KPiA+ICsgICAgICAgICAgICAgICAgICAgICAgIGJyZWFrOw0KPiA+ICsN
Cj4gPiArICAgICAgICAgICAgICAgdG9rZW4gPSBzdHJzZXAoJnNwdHIsICIgIik7DQo+ID4gKw0K
PiA+ICsgICAgICAgICAgICAgICByZXQgPSBrc3RydG91MzIodG9rZW4sIDAsICZtZW1fdmFsKTsN
Cj4gPiArICAgICAgICAgICAgICAgaWYgKHJldCkNCj4gPiArICAgICAgICAgICAgICAgICAgICAg
ICBnb3RvIGVycl9mcmVlX3ZhbHVlOw0KPiA+ICsNCj4gPiArICAgICAgICAgICAgICAgdmFsdWVb
aV0gPSBtZW1fdmFsOw0KPiA+ICsgICAgICAgICAgICAgICBpKys7DQo+ID4gKyAgICAgICB9DQo+
ID4gKw0KPiA+ICsgICAgICAgcmV0ID0gYXRoMTBrX2hpZl9kaWFnX3dyaXRlKGFyLCBtZW1fYWRk
ciwgKGNvbnN0IHZvaWQgKil2YWx1ZSwNCj4gPiArICAgICAgICAgICAgICAgICAgICAgICAgICAg
ICAgICAgICBpICogc2l6ZW9mKG1lbV92YWwpKTsNCj4gPiArICAgICAgIGlmIChyZXQpIHsNCj4g
PiArICAgICAgICAgICAgICAgYXRoMTBrX3dhcm4oYXIsICJmYWlsZWQgdG8gd3JpdGUgYWRkcmVz
cyAweCUwOHggdmlhIGRpYWdub3NlIHdpbmRvdw0KPiBmcm9tIGRlYnVnZnM6ICVkXG4iLA0KPiA+
ICsgICAgICAgICAgICAgICAgICAgICAgICAgICBtZW1fYWRkciwgcmV0KTsNCj4gPiArICAgICAg
ICAgICAgICAgZ290byBlcnJfZnJlZV92YWx1ZTsNCj4gPiArICAgICAgIH0NCj4gDQo+IEkgYmVs
aWV2ZSB1c2Vyc3BhY2UgbWF5IGNhbGwgd3JpdGUoKSBtdWx0aXBsZSBudW1iZXIgb2YgdGltZXMg
d2l0aCBkaWZmZXJlbnQgb2Zmc2V0cw0KPiBhbmQgZnJhZ21lbnQgdGhlIGRhdGEgYnJlYWtpbmcg
aXQgaW4gdGhlIG1pZGRsZSBvZiB5b3VyIHN0cmluZyBoZXggd29yZHMuIFRoaXMNCj4gY29kZSB3
aWxsIG1vc3QgbGlrZWx5IGZhaWwgd2l0aCBsYXJnZXIgY2h1bmtzIG9mIGRhdGEgb3IgeW91IGNh
biB0cnkgdG8gc2ltdWxhdGUgdGhlDQo+IGZyYWdtZW50YXRpb24gd2l0aDoNCj4gDQo+ICAgZWNo
byAweDAwMDEgMHgwMDAyIHwgZGQgYnM9MSBvZj1tZW1fdmFsdWUNCj4gDQo+IChQbGVhc2UgY29y
cmVjdCBtZSBpZiBJJ20gd3JvbmcgOi0pDQo+IA0KTm90IGZ1bGwgdW5kZXJzdGFuZCB3aGF04oCZ
cyB5b3UgbWVhbiwgZG8geW91IG1lYW4gdGhpcyBzZWdtZW50IG5lZWQgYmUgcHJvdGVjdCB3aXRo
IGxvY2sgDQp0byBhdm9pZCBtdWx0aSB3cml0ZSBvcGVyYXRpb24gYWNjZXNzIHRoZSBtZW1fdmFs
dWUgYXQgdGhlIHNhbWUgdGltZT8NCg0KPiANCj4gTWljaGHFgg0K

RE: [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@tieto.com]
> Sent: Tuesday, October 21, 2014 2:56 PM
> To: Li, Yanbo
> Cc: Valo, Kalle; ath10k-devel; Balasubramanian, Senthil Kumar;
> dreamfly281@gmail.com; linux-wireless; ath10k@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@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ł
_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

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

[Michal Kazior]

On 21 October 2014 17:10, Li, Yanbo <yanbol@qti.qualcomm.com> wrote:
>> -----Original Message-----
>> From: Michal Kazior [mailto:michal.kazior@tieto.com]
>> On 20 October 2014 18:38, Yanbo Li <yanbol@qti.qualcomm.com> 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ł

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

[Michal Kazior]

On 21 October 2014 17:10, Li, Yanbo <yanbol@qti.qualcomm.com> wrote:
>> -----Original Message-----
>> From: Michal Kazior [mailto:michal.kazior@tieto.com]
>> On 20 October 2014 18:38, Yanbo Li <yanbol@qti.qualcomm.com> 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ł

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k