On 5/8/2019 7:16 PM, Benjamin Herrenschmidt wrote:
On Wed, 2019-05-08 at 18:19 -0700, Jae Hyun Yoo wrote:
I changed that from a bool because the maintainer of this code, Eddie
doesn't like adding of an additional flag. I'll change it back with
codes in the first submit:
https://www.spinics.net/lists/linux-media/msg148955.html
Eddie,
Please let me know if you have any objection on that.
Ok, so random flags ... ugh.
Well, you can approach it either way. Have them all be bitops or all be
bool.
The tricky thing however is that if they are bitops you need to ensure
that they are *all* manipulated under the same lock. If not you have to
use the atomic bitops variants.
The reason I don't like that is that experience shows that most uses of
such atomic variants in drivers usually are failed attempts at papering
over broken locking.
If everything is covered by a lock, then using the non-atomic versions
is more efficient, but so is using bool (optionally with :1 bitfield
qualifiers to avoid wasting memory), which from a pure C language
perspective I think is more expressive of what you are doing and more
readable.
Okay. Probably I need to add one another patch in this series to address
what you pointed out.
I have one question. I reviewed again all bitops in this driver and
checked that some bitops are protected by a spinlock and some others
are not. In this case, can I mix use atomic and non-atomic bitops
depend on each bitop's protection state by the spinlock? Or, would it be
better to change all of them to bool in this case?
Thanks,
Jae
Cheers,
Ben.
