Re: [PATCH v3 3/3] char drivers: ramoops debugfs entry

[Andrew Morton <akpm@linux-foundation.org>]

On Wed,  6 Jul 2011 16:29:50 -0700
Sergiu Iordache <sergiu@chromium.org> wrote:

> While ramoops writes to ram, accessing the dump requires using /dev/mem
> and knowing the memory location (or a similar solution). This patch
> provides a debugfs interface through which the respective memory
> area can be easily accessed.
> 
> The entry added is /sys/kernel/debug/ramoops/next
> 
> The entry returns a dump of size record_size each time, skipping invalid
> dumps. When it reaches the end of the memory area reserved for dumps it
> returns an empty record and resets the current record count.

The patch increases the size of ramoops.o text&data from 1725 bytes to
2282 even when CONFIG_DEBUG_FS=n.  That's quite a lot of bloat!

Furthermore, I think debugfs is the wrong place to put this.  debugfs
is, umm, for debugging.  It's a place in which to put transitory junk
which may or may not be there and where interfaces may change without
notice and whose presence userspace should not assume.  But in this
patch you're proposing a permanent and formal new interface into the
ramoops driver.  It should go into a permanent well-known place where
it will be maintained with the formality and care of all the other
parts of the kernel API.

Also, you're proposing a new kernel/userspace API but it wasn't
documented anywhere.  We don't want to have to reverse-engineer your
proposed API from the implementation for review purposes.  That API
should have been exhaustively documented in the changelog so others can
decide whether they like it.

Finally, we should provide user documentation for the new interface so
others can find out that it exists and can use it correctly.  We seem
not to have bothered documenting ramoops so we don't currently have a
context in which to do this.