linux-kernel - Re: [PATCH] Moorestown RAR Handler driver, MRST 2.6.31-rc3

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20090801073053.GA30389@merkur.ravnborg.org>
Date:	Sat, 1 Aug 2009 09:30:53 +0200
From:	Sam Ravnborg <sam@...nborg.org>
To:	Ossama Othman <ossama.othman@...el.com>
Cc:	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] Moorestown RAR Handler driver, MRST 2.6.31-rc3

> 
> diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
> index b9e5010..3e36269 100644
> --- a/drivers/misc/Kconfig
> +++ b/drivers/misc/Kconfig
> @@ -150,6 +150,18 @@ config ATMEL_SSC
>  
>  	  If unsure, say N.
>  
> +config MRST_RAR_HANDLER
> +	tristate "RAR handler driver for Intel Moorestown platform"
> +	depends on X86
> +	select RAR_REGISTER
> +	default n

n is default - so no need to spell it out.

> +	---help---
> +	  This driver provides a memory management interface to
> +	  restricted access regions available in the Intel Moorestown
> +	  platform.
> +
> +	  If unsure, say N.
> +
>  config MRST_VIB
>  	tristate "vibrator driver for Intel Moorestown platform"
>  	help
> diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
> index 0238835..a69bc26 100644
> --- a/drivers/misc/Makefile
> +++ b/drivers/misc/Makefile
> @@ -14,6 +14,8 @@ obj-$(CONFIG_TIFM_7XX1)       	+= tifm_7xx1.o
>  obj-$(CONFIG_PHANTOM)		+= phantom.o
>  obj-$(CONFIG_SGI_IOC4)		+= ioc4.o
>  obj-$(CONFIG_MSTWN_POWER_MGMT)	+= moorestown/
> +obj-$(CONFIG_MRST_RAR_HANDLER)	+= memrar.o
> +memrar-objs			:= memrar_allocator.o memrar_handler.o

Please use:
+ memrar-y := memrar_allocator.o memrar_handler.o

The foo-objs syntax is deprecated.


> +
> +
> +#include "memrar_allocator.h"
> +#include <linux/slab.h>
> +#include <linux/bug.h>

<linux/...> comes _before_ you own includes.
And an empty line in between.

> +
> +
> +struct memrar_allocator *memrar_create_allocator(unsigned long base,
> +						 size_t capacity,
> +						 size_t block_size)
> +{
> +	struct memrar_allocator *allocator = 0;

It is recommended to separate definition,
and assignment.
So this should look like this:

	struct memrar_allocator *allocator;

	allocator = 0;

> +
> +	/* Validate parameters. */
> +	if (/*
> +	     * Make sure we can allocate the entire memory allocator
> +	     * space.
> +	     */
> +		ULONG_MAX - capacity >= base
> +
> +		/* Zero capacity or block size are obviously invalid. */
> +		&& capacity != 0
> +		&& block_size != 0) {

Very ugly way to comment your if (..) IMHO
Put the comment abvoe the if.

> +		/*
> +		 * There isn't much point in creating a memory
> +		 * allocator that is only capable of holding one block
> +		 * but we'll allow, and issue a diagnostic.
> +		 */
> +		WARN(capacity < block_size * 2,
> +		     "Memory allocator is only large enough to "
> +		     "hold one block.\n");

Try to avoid breaking your printable line.
It is much easier to grep for the line when it is on a sinlge
line.
If you violate the 80 chars per line rule do to this then accept
that grepable lines is better than keeping them 80 or less wide.

This comment applies in several places.

> +			INIT_LIST_HEAD(&allocator->free_list.list);
> +
> +			first_node =
> +				kmalloc(sizeof(*first_node), GFP_KERNEL);

No need to break up this line.

> +			if (first_node != 0) {
> +				/* Full range of blocks is available. */
> +				first_node->begin = base;
> +				first_node->end =
> +					base + allocator->capacity;
> +				list_add(&first_node->list,
> +					 &allocator->free_list.list);
> +			} else {
> +				kfree(allocator);
> +				allocator = 0;
> +			}
> +		}
> +	}
> +
> +	return allocator;
> +}

I'm short on time - so this is the comments for today...

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