Re: [PATCH v7] Add udmabuf misc device

quoted

quoted

quoted

+	if (WARN_ON(vmf->pgoff >= ubuf->pagecount))
+		return VM_FAULT_SIGBUS;

Just curious, when do you expect this to happen ?

It should not.  If it actually happens it would be a bug somewhere,
thats why the WARN_ON.

But you seem to consider that this condition that should never happen still 
has a high enough chance of happening that it's worth a WARN_ON(). I was 
wondering why this one in particular, and not other conditions that also can't 
happen and are not checked through the code. 

Added it while writing the code, to get any coding mistake I make
flagged right away instead of things exploding later on.

I can drop it.

quoted

quoted

quoted

+	ubuf = kzalloc(sizeof(struct udmabuf), GFP_KERNEL);

sizeof(*ubuf)

Why?  Should not make a difference ...

Because the day we replace

	struct udmabuf *ubuf;

with

	struct udmabuf_ext *ubuf;

and forget to change the next line, we'll introduce a bug. That's why 
sizeof(variable) is preferred over sizeof(type). Another reason is that I can 
easily see that

	ubuf = kzalloc(sizeof(*ubuf), GFP_KERNEL);

is correct, while using sizeof(type) requires me to go and look up the 
declaration of the variable.

So it simplifies review, ok, will change it.

BTW: Maybe the kernel should pick up a neat trick from glib:

g_new0() is a macro which takes the type instead of the size as first
argument, and it casts the return value to that type.  So the compiler
will throw warnings in case of a mismatch.  That'll work better than
depending purely on the coder being careful and review catching the
remaining issues.

cheers,
  Gerd