linux-kernel - Re: [PATCH 02/05] resource: add resource_type() and IORESOURCE_TYPE

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: <20080718075601.GL24620@fluff.org.uk>
Date:	Fri, 18 Jul 2008 08:56:01 +0100
From:	Ben Dooks <ben-linux@...ff.org>
To:	Magnus Damm <magnus.damm@...il.com>
Cc:	linux-kernel@...r.kernel.org, linux-sh@...r.kernel.org,
	gregkh@...e.de, lethal@...ux-sh.org, i2c@...sensors.org,
	akpm@...ux-foundation.org
Subject: Re: [PATCH 02/05] resource: add resource_type() and
	IORESOURCE_TYPE_BITS

On Fri, Jul 18, 2008 at 04:40:18PM +0900, Magnus Damm wrote:
> From: Magnus Damm <damm@...l.co.jp>
> 
> This patch adds resource_type() and IORESOURCE_TYPE_BITS. They make it
> easier to add more resource types without having to rewrite tons of code.

This looks ok to me, with one concern as shown below.

> Signed-off-by: Magnus Damm <damm@...l.co.jp>
> ---
> 
>  drivers/base/platform.c |   31 +++++++++++++++++--------------
>  include/linux/ioport.h  |    7 ++++++-
>  2 files changed, 23 insertions(+), 15 deletions(-)
> 
> --- 0001/drivers/base/platform.c
> +++ work/drivers/base/platform.c	2008-07-09 20:19:16.000000000 +0900
> @@ -42,10 +42,8 @@ struct resource *platform_get_resource(s
>  	for (i = 0; i < dev->num_resources; i++) {
>  		struct resource *r = &dev->resource[i];
>  
> -		if ((r->flags & (IORESOURCE_IO|IORESOURCE_MEM|
> -				 IORESOURCE_IRQ|IORESOURCE_DMA)) == type)
> -			if (num-- == 0)
> -				return r;
> +		if (type == resource_type(r) && num-- == 0)
> +			return r;
>  	}
>  	return NULL;
>  }
> @@ -78,10 +76,8 @@ struct resource *platform_get_resource_b
>  	for (i = 0; i < dev->num_resources; i++) {
>  		struct resource *r = &dev->resource[i];
>  
> -		if ((r->flags & (IORESOURCE_IO|IORESOURCE_MEM|
> -				 IORESOURCE_IRQ|IORESOURCE_DMA)) == type)
> -			if (!strcmp(r->name, name))
> -				return r;
> +		if (type == resource_type(r) && !strcmp(r->name, name))
> +			return r;
>  	}
>  	return NULL;
>  }
> @@ -259,9 +255,9 @@ int platform_device_add(struct platform_
>  
>  		p = r->parent;
>  		if (!p) {
> -			if (r->flags & IORESOURCE_MEM)
> +			if (resource_type(r) == IORESOURCE_MEM)
>  				p = &iomem_resource;
> -			else if (r->flags & IORESOURCE_IO)
> +			else if (resource_type(r) == IORESOURCE_IO)
>  				p = &ioport_resource;
>  		}

You are changing a simple test to a mask and compare, is anyone going
to produce resources with an IORESOURCE_MEM and an IORESOURCE_IO
together?
  
> @@ -282,9 +278,14 @@ int platform_device_add(struct platform_
>  		return ret;
>  
>   failed:
> -	while (--i >= 0)
> -		if (pdev->resource[i].flags & (IORESOURCE_MEM|IORESOURCE_IO))
> -			release_resource(&pdev->resource[i]);
> +	while (--i >= 0) {
> +		struct resource *r = &pdev->resource[i];
> +		unsigned long type = resource_type(r);
> +
> +		if (type == IORESOURCE_MEM || type == IORESOURCE_IO)
> +			release_resource(r);
> +	}
> +
>  	return ret;
>  }
>  EXPORT_SYMBOL_GPL(platform_device_add);
> @@ -306,7 +307,9 @@ void platform_device_del(struct platform
>  
>  		for (i = 0; i < pdev->num_resources; i++) {
>  			struct resource *r = &pdev->resource[i];
> -			if (r->flags & (IORESOURCE_MEM|IORESOURCE_IO))
> +			unsigned long type = resource_type(r);
> +
> +			if (type == IORESOURCE_MEM || type == IORESOURCE_IO)
>  				release_resource(r);
>  		}
>  	}
> --- 0002/include/linux/ioport.h
> +++ work/include/linux/ioport.h	2008-07-09 15:16:48.000000000 +0900
> @@ -34,7 +34,8 @@ struct resource_list {
>   */
>  #define IORESOURCE_BITS		0x000000ff	/* Bus-specific bits */
>  
> -#define IORESOURCE_IO		0x00000100	/* Resource type */
> +#define IORESOURCE_TYPE_BITS	0x00000f00	/* Resource type */
> +#define IORESOURCE_IO		0x00000100
>  #define IORESOURCE_MEM		0x00000200
>  #define IORESOURCE_IRQ		0x00000400
>  #define IORESOURCE_DMA		0x00000800
> @@ -117,6 +118,10 @@ static inline resource_size_t resource_s
>  {
>  	return res->end - res->start + 1;
>  }
> +static inline unsigned long resource_type(struct resource *res)
> +{
> +	return res->flags & IORESOURCE_TYPE_BITS;
> +}
>  
>  /* Convenience shorthand with allocation */
>  #define request_region(start,n,name)	__request_region(&ioport_resource, (start), (n), (name))
> --
> 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/

-- 
Ben (ben@...ff.org, http://www.fluff.org/)

  'a smiley only costs 4 bytes'
--
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/