Messages in this thread | | | From | Sergiu Iordache <> | Date | Wed, 6 Jul 2011 10:08:43 -0700 | Subject | Re: [PATCH v2 2/3] char drivers: ramoops record_size module parameter |
| |
On Sat, Jul 2, 2011 at 1:04 AM, Marco Stornelli <marco.stornelli@gmail.com> wrote: > 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(×tamp); >>>> @@ -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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |