Re: [PATCH] 1/2 Maple: Update bus driver to allow support of VMUdevice

[Andrew Morton]

On Thu, 20 Mar 2008 22:23:17 +0000
Adrian McMenamin <adrian@xxxxxxxxxxxxxxxxxxxxxxxx> wrote:

> 
> > urgh, down_trylock().  And a secret, undocumented one too.
> > 
> > A trylock is always an exceptional thing.  How is *any* reader of this code
> > supposed to work out what the heck it's doing there?  Convert it into
> > down(), run the code and decrypt the lockdep warnings, I suspect.
> > 
> > <looks>
> > 
> > Nope, I can't see any other lock being held when we call this function.
> > 
> > The trylocks are an utter mystery to me.  Please don't write mysterious
> > code.
> > 
> 
> OK, I am sure this is my problem but I have no idea why you are
> describing down_trylock as undocumented

I'm describing your use of it!  I'm sitting here trying to work out why on
earth this code is using the highly unusual (and highly suspicious) trylock
idiom and this is far from clear.

And I assume that if I can't work out why code is doing unusual-looking
things, then other readers will not be able to do so either.  Hence for
maintainability, that code of yours needs documenting.  I think.

Now it's true that I am unfamilar witht he arcanery of the maple driver and
the interfaces which it calls into.  But an experienced kernel developer
should be able to pick up a piece of code and have a decent shot at
understanding it.  And being able to review it...

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/