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

[Marco Stornelli <marco.stornelli@gmail.com>]

Il 01/07/2011 20:41, Sergiu Iordache ha scritto:
> On Fri, Jul 1, 2011 at 10:57 AM, Marco Stornelli
> <marco.stornelli@gmail.com>  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@chromium.org>
>>> 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).

Marco