On Wednesday, September 1, 2021 2:09:16 PM CEST Alex Elder wrote:
On 8/31/21 6:50 AM, Fabio M. De Francesco wrote:
I was wrong in assuming that trivial patches to Greybus are welcome as
they
are for other drivers.
This is not a correct statement.
Yes, I agree: it's not a correct statement. Please let me explain what I was trying to convey with that consideration...
The Mutexes were there around idr_find() and I decided to leave the code as it was. Who am I to say that they are not necessary? I must stay on the safe side. First because I don't know how the drivers work (can that critical section really be entered by different threads that could possibly share the gb_tty that is retrieved by xa_load()? Even if xa_load() always give you back the right gb_tty, how do I know if in the while other threads change its fields or destroy the object? I guess I should stay on the safe side and leave the Mutexes there, exactly were they were.
These are the reason why v1 was indeed a trivial patch. But v2 *was not* because you wrote that you were pretty sure they were unneeded and you asked me to leave them or remove them and in either case I had to provide a reason why.
I guess that in v1 I should not provide a reason why they are still there, as well as I don't have to provide any reason on why the greybus code (line by line) is as it is: it is out of the scope of my patch. Am I wrong?
Your note about the possibility that the mutexes could be removed pushed me beyond what I need to know to accomplish the intended task.
Anyway I tried to reason about it. I perfectly know what is required to protect critical sections of code, but I don't know how drivers work; I mean I don't know whether or not different threads that run concurrently could really interfere in that specific section. This is because I simply reason in terms of general rules of protection of critical section but I really don't know how Greybus works or (more in general) how drivers work.
I still think that if I stayed within the bounds of my original purpose I didn't have to reason about this topic and that the v1 patch was trivial. v2 was not!
I'm sorry because I'm still not sure if I was able to conveyed what I thought and still think.
But as Johan pointed out, even for a trivial patch if you must understand the consequences of what the change does. If testing is not possible, you must work extra hard to ensure your patch is correct.
Again, I don't see any possible harm with the mutexes in place :)
In the first (or an early) version of your patch I pointed out a bug. Later, I suggested the lock might not be necessary and asked you to either confirm it was or explain why it was not, but you didn't do that.
This was beyond my knowledge and perhaps unnecessary (sorry if I insist on that :)).
I agree that the change appeared trivial, and even sensible, but even trivial patches must result in correct code. And all patches should have good and complete explanations.
- Alex
Is v2 correct with the mutexes restored where they were? I guess it is.
Thanks for you kind review and the time you spent for me. I appreciated it, seriously.
Fabio