[U-Boot] [PATCH v2] cfi_flash: Fix unaligned accesses to cfi_qry structure

[Albert ARIBAUD <albert.u.boot@aribaud.net>]

Hi Andrew,

On Tue, 14 May 2013 12:27:52 -0500, Andrew Gabbasov
<andrew_gabbasov@mentor.com> wrote:

> Packed structure cfi_qry contains unaligned 16- and 32-bits members,
> accessing which causes problems when cfi_flash driver is compiled with
> -munaligned-access option: flash initialization hangs, probably
> due to data error.
> 
> Since the structure is supposed to replicate the actual data layout
> in CFI Flash chips, the alignment issue can't be fixed in the structure.
> So, unaligned fields need using of explicit unaligned access macros.
> 
> Signed-off-by: Andrew Gabbasov <andrew_gabbasov@mentor.com>
> ---
>  drivers/mtd/cfi_flash.c |   15 +++++++++------
>  include/mtd/cfi_flash.h |   18 +++++++++++-------
>  2 files changed, 20 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/mtd/cfi_flash.c b/drivers/mtd/cfi_flash.c
> index 22d8440..f6759a8 100644
> --- a/drivers/mtd/cfi_flash.c
> +++ b/drivers/mtd/cfi_flash.c
> @@ -38,6 +38,7 @@
>  #include <asm/processor.h>
>  #include <asm/io.h>
>  #include <asm/byteorder.h>
> +#include <asm/unaligned.h>
>  #include <environment.h>
>  #include <mtd/cfi_flash.h>
>  #include <watchdog.h>
> @@ -1640,9 +1641,10 @@ static void cfi_reverse_geometry(struct cfi_qry *qry)
>  	u32 tmp;
>  
>  	for (i = 0, j = qry->num_erase_regions - 1; i < j; i++, j--) {
> -		tmp = qry->erase_region_info[i];
> -		qry->erase_region_info[i] = qry->erase_region_info[j];
> -		qry->erase_region_info[j] = tmp;
> +		tmp = get_unaligned(&(qry->erase_region_info[i]));
> +		put_unaligned(get_unaligned(&(qry->erase_region_info[j])),
> +			      &(qry->erase_region_info[i]));
> +		put_unaligned(tmp, &(qry->erase_region_info[j]));
>  	}
>  }
>  
> @@ -2073,8 +2075,8 @@ ulong flash_get_size (phys_addr_t base, int banknum)
>  	info->start[0] = (ulong)map_physmem(base, info->portwidth, MAP_NOCACHE);
>  
>  	if (flash_detect_cfi (info, &qry)) {
> -		info->vendor = le16_to_cpu(qry.p_id);
> -		info->ext_addr = le16_to_cpu(qry.p_adr);
> +		info->vendor = le16_to_cpu(get_unaligned(&(qry.p_id)));
> +		info->ext_addr = le16_to_cpu(get_unaligned(&(qry.p_adr)));
>  		num_erase_regions = qry.num_erase_regions;
>  
>  		if (info->ext_addr) {
> @@ -2163,7 +2165,8 @@ ulong flash_get_size (phys_addr_t base, int banknum)
>  				break;
>  			}
>  
> -			tmp = le32_to_cpu(qry.erase_region_info[i]);
> +			tmp = le32_to_cpu(get_unaligned(
> +						&(qry.erase_region_info[i])));
>  			debug("erase region %u: 0x%08lx\n", i, tmp);
>  
>  			erase_region_count = (tmp & 0xffff) + 1;
> diff --git a/include/mtd/cfi_flash.h b/include/mtd/cfi_flash.h
> index 966b5e0..b644b91 100644
> --- a/include/mtd/cfi_flash.h
> +++ b/include/mtd/cfi_flash.h
> @@ -129,12 +129,16 @@ typedef union {
>  } cfiword_t;
>  
>  /* CFI standard query structure */
> +/* The offsets and sizes of this packed structure members correspond
> + * to the actual layout in CFI Flash chips. Some 16- and 32-bit members
> + * are unaligned and must be accessed with explicit unaligned access macros.
> + */
>  struct cfi_qry {
>  	u8	qry[3];
> -	u16	p_id;
> -	u16	p_adr;
> -	u16	a_id;
> -	u16	a_adr;
> +	u16	p_id;			/* unaligned */
> +	u16	p_adr;			/* unaligned */
> +	u16	a_id;			/* unaligned */
> +	u16	a_adr;			/* unaligned */
>  	u8	vcc_min;
>  	u8	vcc_max;
>  	u8	vpp_min;
> @@ -148,10 +152,10 @@ struct cfi_qry {
>  	u8	block_erase_timeout_max;
>  	u8	chip_erase_timeout_max;
>  	u8	dev_size;
> -	u16	interface_desc;
> -	u16	max_buf_write_size;
> +	u16	interface_desc;		/* aligned */
> +	u16	max_buf_write_size;	/* aligned */
>  	u8	num_erase_regions;
> -	u32	erase_region_info[NUM_ERASE_REGIONS];
> +	u32	erase_region_info[NUM_ERASE_REGIONS];	/* unaligned */
>  } __attribute__((packed));
>  
>  struct cfi_pri_hdr {

Reviewed-By: Albert ARIBAUD <albert.u.boot@aribaud.net>

Seems ok to me.

Now, seeing as this is global to all architectures, yet motivated by
ARM alignment considerations, which repo should this go to?

Amicalement,
-- 
Albert.