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

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

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.

Marco