On Tue, Aug 31, 2021 at 01:50:05PM +0200, Fabio M. De Francesco wrote: > On Tuesday, August 31, 2021 10:07:38 AM CEST Johan Hovold wrote: > > Since most people can't really test their changes to Greybus perhaps it > > isn't the best example of code that can be safely modified. But yeah, > > parts of it are still in staging and, yes, staging has been promoted as > > place were some churn is accepted. > I cannot test my changes to Greybus, but I think that trivial changes are > just required to build. To stay on the safe side I had left those > mutex_lock() around xa_load(). I wasn't sure about it, but since the original > code had the Mutexes I thought it wouldn't hurt to leave them there. But if you weren't sure that your patch was correct, you can't also claim that it was trivial. Patches dealing with locking and concurrency usually are not. I too had go look up the XArray interface and review the Greybus uart code (which is broken and that doesn't make it easier for any of us). So no, this wasn't trivial, it carries a cost and should therefore in the end also have some gain. And what that was wasn't clear from the commit message (since any small efficiency gain is irrelevant in this case). Sorry you got stuck in the middle. Perhaps you can see it as a first-hand experience of some of the trade offs involved when doing kernel development. And remember that a good commit message describing the motivation for the change (avoiding boiler-plate marketing terms) is a good start if you want to get your patches accepted. Johan
