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

[Sergiu Iordache <sergiu@google.com>]

On Mon, Jul 11, 2011 at 11:41 PM, Marco Stornelli
<marco.stornelli@gmail.com> wrote:
> 2011/7/11 Sergiu Iordache <sergiu@chromium.org>:
>> On Thu, Jul 7, 2011 at 11:43 PM, Marco Stornelli
>> <marco.stornelli@gmail.com> wrote:
>>> 2011/7/8 Greg KH <greg@kroah.com>:
>>>> On Thu, Jul 07, 2011 at 04:27:27PM -0700, Andrew Morton wrote:
>>>>> On Thu, 7 Jul 2011 16:16:43 -0700
>>>>> Sergiu Iordache <sergiu@google.com> wrote:
>>>>>
>>>>> > Ramoops currently dumps the log of a panic/oops in a memory area which
>>>>> > is known not to be overwritten on restart (for example 1MB starting at
>>>>> > 15MB). The way it works is by dividing the memory area in records of a
>>>>> > set size (fixed at 4K before my patches, configurable after) and by
>>>>> > dumping a record there for each oops/panic. The problem is that right
>>>>> > now you have to access that memory area through other means, such as
>>>>> > /dev/mem, which is not always possible.
>>>>> >
>>>>> > What my patch did was to add a debugfs entry which returns a valid
>>>>> > record each time (a single dump done by ramoops). The first call
>>>>> > returns the first dump. The first call after the last valid dump
>>>>> > returns an empty buffer. .
>>>>>
>>>>> Please fully describe this "record" in the v2 patch changelog.  We'll
>>>>> want to review it for endianness, 32/64-bit compat issues,
>>>>> maintainability, extensibility, etc.
>>>>>
>>>>> > After it has returned nothing, the next
>>>>> > calls return records from the start again.
>>>>>
>>>>> That sounds a bit weird.  One would expect it to keep returning zero,
>>>>> requiring userspace to lseek or close/open.
>>>>>
>>>>> > The validity of a dump is
>>>>> > checked by looking after the header. Any comments on this approach are
>>>>> > welcome.
>>>>> >
>>>>> > Changing the entry from debugfs to sysfs wouldn't be a problem. If
>>>>> > sysfs is a valid solution I'll come with a patch that updates the
>>>>> > documentation as well along with the sysfs entry.
>>>>>
>>>>> sysfs sounds OK to me.  Then again, sysfs is supposed to be
>>>>> one-value-per-file, so using it would be naughty.
>>>>>
>>>>> I dunno, I'd be inclined to abuse the sysfs rule and hope that nobody
>>>>> notices rather than create a fake char device.  But there's certainly
>>>>> plenty of precedent for the fake char driver.
>>>>
>>>> No, please don't abuse sysfs that way.
>>>>
>>>> Use debugfs or a char device node.
>>>>
>>>> thanks,
>>>>
>>>> greg k-h
>>>>
>>>
>>> I agree with Greg. I asked to not break the existent way to read data
>>> via /dev/mem because for me it's the right way to do this thing.
>>> However to do an easy *debug* a debugfs entry can be useful. IMHO, a
>>> "production" script/application that use debugfs instead of /dev/mem
>>> in this case is simply broken because the debugfs can't be like a
>>> system call or other kernel interaction mechanism. Debugfs should be
>>> used only for debug.
>>>
>>> Marco
>>
>> Any consensus/decision on how to go on with this patch idea?
>>
>> The options that I see right now are:
>> - keep access through /dev/mem only (but access to /dev/mem is
>> sometimes restricted);
>> - keep the debugfs entry as well(as in the patch);
>> - remove the debugfs entry and add a char driver to access the memory
>> using read and seek operations.
>>
>> + the rejected(?) options from before
>>
>> Sergiu
>>
>
> For me the best option it's to use a sysfs/proc entry to export
> (read-only) the memory address, record size etc. At that point we can
> use a generic script/program to access via /dev/mem. However I let
> Andrew/Greg say the last word.

Well, since the only method to read the dump data is /dev/mem,
exporting the record size/address/etc is needed in order to parse it
properly. But as far as I can see the data is already exported through
sysfs in /sys/module/ramoops/parameters/.
The current module still needs a patch to write the variables of the
module parameters from the platform data in case that is used, but is
there any reason why we would need other sysfs entries except these?

Sergiu