Re: [RFC][AGPGART] Allow multiple backends to be initialized

[Christoph Hellwig <hch@lst.de>]

Last time I checked Dave Jones (davej@codemonkey.org.uk) was agpgart
maintainer, not Rusty or me.  So please resend to Dave and the
linux-kernel list so other people (like me can comment).

Comments on the patch format:  Please don't send output of bk export
over multiple changesets - while you can use that to generate the
patch please strip the bk checkin comments and write a single patch
description that explains your overall changes, aka what you did
and if it's not obvious why.

If you patch does multiple things at once split it into one patch per
problem or feature.


> -struct agp_bridge_data agp_bridge_dummy = { .type = NOT_SUPPORTED };
> -struct agp_bridge_data *agp_bridge = &agp_bridge_dummy;
> +struct agp_bridge_data *agp_bridge = NULL;

there's no need to initialize global variables to NULL - the C standard
guarantees that gets done implicitly - and letting the compile do that
decreases the size of the kernel image.

> -/* XXX Kludge alert: agpgart isn't ready for multiple bridges yet */
>  struct agp_bridge_data *agp_alloc_bridge(void)
>  {
> -	return agp_bridge;
> +	struct  agp_bridge_data *bridge = kmalloc(sizeof(struct agp_bridge_data), 
> GFP_KERNEL);
> +	if(!agp_count) agp_bridge = bridge;
> +	return bridge;
>  }

Some style problems here - never create lines longer than 80
charactersm, and follow Documentation/CodingStyle indentation (or just
look at the other agpgart code).

Also you should check the kmalloc return value;

It should look like:

struct agp_bridge_data *agp_alloc_bridge(void)
{
	struct agp_bridge_data *bridge;

	bridge = kmalloc(sizeof(*bridge), GFP_KERNEL);
	if (!bridge)
		return NULL;

	/* cludge for the transition to passing the agp_bridge around */
	if (!agp_count)
		agp_bridge = bridge;

	return bridge;
}

Also where do you free the kmalloced bridges again?

> +	if(!agp_count) {

	if (!agp_count) {

> +	if(!agp_count) {

Dito.

>  	memset(info, 0, sizeof(struct agp_kern_info));
> -	if (!agp_bridge || agp_bridge->type == NOT_SUPPORTED ||
> -	    !agp_bridge->version) {
> +	if ((agp_bridge == NULL) || (agp_bridge->type == NOT_SUPPORTED)) {

Why do you drop the version check here?