Re: [PATCH v2 2/3] char drivers: ramoops record_size module parameter

[Sergiu Iordache]

On Sat, Jul 2, 2011 at 1:04 AM, Marco Stornelli
<marco.stornelli@xxxxxxxxx> wrote:
> Il 01/07/2011 20:41, Sergiu Iordache ha scritto:
>>
>> On Fri, Jul 1, 2011 at 10:57 AM, Marco Stornelli
>> <marco.stornelli@xxxxxxxxx>  wrote:
>>>
>>> Hi,
>>>
>>> Il 01/07/2011 03:28, Sergiu Iordache ha scritto:
>>>>
>>>> The size of the dump is currently set using the RECORD_SIZE macro which
>>>> is set to a page size. This patch makes the record size a module
>>>> parameter and allows it to be set through platform data as well to allow
>>>> larger dumps if needed.
>>>>
>>>> Signed-off-by: Sergiu Iordache<sergiu@xxxxxxxxxxxx>
>>>> Change-Id: Ie5a53acb50d5881d51354f5d9d13e3d6bedf6a2e
>>>> ---
>>>> The patch was built on the 2.6.38 kernel and is based on the following
>>>> patches which were applied from the mmotm tree:
>>>> ramoops-add-new-line-to-each-print
>>>> ramoops-use-module-parameters-instead-of-platform-data-if-not-available
>>>>
>>>>
>>>> ramoops-use-module-parameters-instead-of-platform-data-if-not-available-checkpatch-fixes
>>>>
>>>>  drivers/char/ramoops.c  |   33 ++++++++++++++++++++++++++-------
>>>>  include/linux/ramoops.h |    1 +
>>>>  2 files changed, 27 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/drivers/char/ramoops.c b/drivers/char/ramoops.c
>>>> index 5349d94..f34077e 100644
>>>> --- a/drivers/char/ramoops.c
>>>> +++ b/drivers/char/ramoops.c
>>>> @@ -32,8 +32,12 @@
>>>>  #include<linux/ramoops.h>
>>>>
>>>>  #define RAMOOPS_KERNMSG_HDR "===="
>>>> +#define MIN_MEM_SIZE 4096UL
>>>>
>>>> -#define RECORD_SIZE 4096UL
>>>> +static ulong record_size = 4096UL;
>>>> +module_param(record_size, ulong, 0400);
>>>> +MODULE_PARM_DESC(record_size,
>>>> +               "size of each dump done on oops/panic");
>>>>
>>>>  static ulong mem_address;
>>>>  module_param(mem_address, ulong, 0400);
>>>> @@ -55,6 +59,7 @@ static struct ramoops_context {
>>>>        void *virt_addr;
>>>>        phys_addr_t phys_addr;
>>>>        unsigned long size;
>>>> +       unsigned long record_size;
>>>>        int dump_oops;
>>>>        int count;
>>>>        int max_count;
>>>> @@ -84,10 +89,10 @@ static void ramoops_do_dump(struct kmsg_dumper
>>>> *dumper,
>>>>        if (reason == KMSG_DUMP_OOPS&&    !cxt->dump_oops)
>>>>                return;
>>>>
>>>> -       buf = cxt->virt_addr + (cxt->count * RECORD_SIZE);
>>>> +       buf = cxt->virt_addr + (cxt->count * cxt->record_size);
>>>>        buf_orig = buf;
>>>>
>>>> -       memset(buf, '\0', RECORD_SIZE);
>>>> +       memset(buf, '\0', cxt->record_size);
>>>>        res = sprintf(buf, "%s", RAMOOPS_KERNMSG_HDR);
>>>>        buf += res;
>>>>        do_gettimeofday(&timestamp);
>>>> @@ -95,8 +100,8 @@ static void ramoops_do_dump(struct kmsg_dumper
>>>> *dumper,
>>>>        buf += res;
>>>>
>>>>        hdr_size = buf - buf_orig;
>>>> -       l2_cpy = min(l2, RECORD_SIZE - hdr_size);
>>>> -       l1_cpy = min(l1, RECORD_SIZE - hdr_size - l2_cpy);
>>>> +       l2_cpy = min(l2, cxt->record_size - hdr_size);
>>>> +       l1_cpy = min(l1, cxt->record_size - hdr_size - l2_cpy);
>>>>
>>>>        s2_start = l2 - l2_cpy;
>>>>        s1_start = l1 - l1_cpy;
>>>> @@ -119,16 +124,29 @@ static int __init ramoops_probe(struct
>>>> platform_device *pdev)
>>>>        }
>>>>
>>>>        rounddown_pow_of_two(pdata->mem_size);
>>>> +       rounddown_pow_of_two(pdata->record_size);
>>>>
>>>> -       if (pdata->mem_size<    RECORD_SIZE) {
>>>> +       /* Check for the minimum memory size */
>>>> +       if (pdata->mem_size<    MIN_MEM_SIZE) {
>>>> +               pr_err("memory size too small, min %lu\n",
>>>> MIN_MEM_SIZE);
>>>> +               goto fail3;
>>>> +       }
>>>> +
>>>> +       if (pdata->mem_size<    pdata->record_size) {
>>>>                pr_err("size too small\n");
>>>>                goto fail3;
>>>>        }
>>>>
>>>> -       cxt->max_count = pdata->mem_size / RECORD_SIZE;
>>>> +       if (pdata->record_size<= 0) {
>>>> +               pr_err("record size too small\n");
>>>> +               goto fail3;
>>>> +       }
>>>
>>> There is something wrong here. First of all if record_size is unsigned it
>>> can't negative. In addition, if we are here, we know that:
>>>
>>> record_size>= mem_size>= MIN_MEM_SIZE
>>>
>>> So this check have no sense.
>>
>> The pdata->record size<= 0 check is indeed redundant and should be
>> removed.
>>
>> I didn't completely understand the second comment, the module errors
>> if mem_size<  MIN_MEM_SIZE or mem_size<  record_size, which means that
>> mem_size should be larger than MIN_MEM_SIZE and record_size (which
>> leads to record_size being between 0 and mem_size). Am I missing
>> something?
>>
>> (Resent after not reply-ing to all by mistake)
>>
>> Sergiu
>>
>
> Yes, my fault. I meant we should check that mem_size *and* record_size are
> bigger or equals than MIN_MEM_SIZE. After that, we can check that
> record_size is lesser than mem_size (I guess has no sense to use record_size
> lesser than MIN_MEM_SIZE).

That sounds fair. A check to see if record_size != 0 would probably be
needed as well so we don't divide by 0. I'll add those to the next
patch series and submit them for review.

Sergiu
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/