On Mon, Oct 18, 2010 at 1:44 AM, Ohad Ben-Cohen <firstname.lastname@example.org> wrote:
Hi Ohad, A couple of comments below.
I strongly recommend reconsidering the ERR_PTR() pattern in new driver
code. It is impossible to tell from looking at the prototype of a
function if it returns an ERR_PTR() value, or a NULL on failure. I
pretty much guarantee that new users of this code will miss the
subtlety and introduce new bugs by assuming that the return value can
be tested with "if (!hwlock)".
ERR_PTR() is only appropriate when the caller actually cares about the
failure code and has different behaviour depending on the result. For
example, filesystem code that needs to return to userspace a specific
error code. Very seldom does driver code like users of this API
actually care. Using it is just asking for bugs with no benefit.
Disabling irqs *might* be a concern as a source of RT latency. It
might be better to make the caller responsible for managing local spin
locks and irq disable/enable.
OTOH, I also see that a spin lock is still needed internally to
protect the hwspinlock data structure.