search

RevolutionPi / piControl

Kernel module for data exchange with RevPi I/O-Modules and Gateways
81 stars 24 forks source link

piControl: add missing lock #10

Closed linosanfilippo-kunbus closed 4 years ago

linosanfilippo-kunbus commented 4 years ago

As a comment implies the author of the code did not see the requirement of locking the copy of a byte value in case of the KB_GET_VALUE ioctl. Unfortunately this assumption is wrong: In case of two threads A (writer) and B (reader) that access the same data we have to ensure memory coherency. Thus for thread A we must ensure that the written value actually is written to its RAM destination and not held in cache. For thread B we must ensure that the value is actually read from RAM and not from cache. Since the spinlock lock()/unlock() operations imply the required memory barriers both can be achieved by using spinlocks.

The writer already uses a spinlock to access the data. Use this for the reader, too.

Signed-off-by: Lino Sanfilippo l.sanfilippo@kunbus.com

l1k commented 4 years ago

Hm, wouldn't an smp_rmb() before accessing the process image be cheaper? (Plus a comment explaining its necessity.) Particularly on the single-core CM1 which doesn't need a memory barrier.

BTW the merge conflict is caused by your locking-fix branch missing the commits from your previous pulls.

linosanfilippo-kunbus commented 4 years ago

I dont think that this kind of optimization is needed in this case, so I would prefer the current solution. However the commit message needs a s/spinlock/mutex so I will rephrase it and create a new pull request...

l1k commented 4 years ago
I dont think that this kind of optimization is needed in this case

Why do you think so?

I will rephrase it and create a new pull request...

You can just force-push to your branch and the existing pull request will be updated automatically.

linosanfilippo-kunbus commented 4 years ago

You can just force-push to your branch and the existing pull request will be updated automatically.

Thanks for the hint, that worked like a charm :)