Re: [PATCH v2] ARM: dma-mapping: fix potential uninitialized return

[Russell King - ARM Linux <linux@armlinux.org.uk>]

On Thu, Nov 29, 2018 at 09:58:46AM -0500, Nathan Jones wrote:
> No, I had some bad code which was passing a wrong length and receiving the
> strange error code.

While adding some networking documentation, I tripped over the comments
at the bottom of Documentation/networking/netdev-FAQ.rst which seem
very good at guiding what should be in the commit message, specifically:

  If your change is a bug fix, make sure your commit log indicates the
  end-user visible symptom, the underlying reason as to why it happens,
  and then if necessary, explain why the fix proposed is the best way to
  get things done.

This is actually rather important, as we may need to look back at a
commit, and end up wondering what it was about.  A case in point is the
patch that added the /proc/<PID>/syscall interface:

    /proc/PID/syscall

    This adds /proc/PID/syscall and /proc/PID/task/TID/syscall magic files.
    These use task_current_syscall() to show the task's current system call
    number and argument registers, stack pointer and PC.  For a task blocked
    but not in a syscall, the file shows "-1" in place of the syscall number,
    followed by only the SP and PC.  For a task that's not blocked, it shows
    "running".

This doesn't say what the purpose of this new user interface is, why it
is needed, nor is there documentation describing its behaviour (such as
what happens if the thread is being traced.)  The covering message for
the series omitted to talk about this new interface.  So we're now at
the position where we have a bug reported against this interface, and
no one knows what the right behaviour is really supposed to be.

The commit message describes mostly what we can gather from reading the
patch, and some of the behaviour but is entirely insufficient - we're
left scratching our heads as to what the behaviour should be for
restarted syscalls.

So, augmenting your commit message with something like:

"While trying to use the dma_mmap_*() interface, it was noticed that
 this interface returns strange values when passed an incorrect
 length."

would be nice.

Thanks.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel