Re: [PATCH] regmap: fix of_regmap_get_endian()

[Thierry Reding]

On Mon, Aug 18, 2014 at 04:14:04PM -0600, Stephen Warren wrote:
> From: Stephen Warren <swarren@xxxxxxxxxx>
> 
> Commit d647c199510c ("regmap: add DT endianness binding support") has
> some issues. Specifically, if config->reg_format_endian is not explicitly
> set, it will be zero, i.e. REGMAP_ENDIAN_DEFAULT. The switch statement
> that looks up the *endian from DT for the type==REGMAP_ENDIAN_VAL case
> doesn't change *endian in the type==REGMAP_ENDIAN_REG case. However, the
> test immediately following, compares *endian against REGMAP_ENDIAN_NATIVE,
> and if not equal, returns *endian as is. This ends up returning
> REGMAP_ENDIAN_DEFAULT, which the calling code does not expect, eventually
> leading to e.g.:
> 
> Unable to handle kernel NULL pointer dereference at virtual address 00000024
> ...
> [<c02efd64>] (regcache_cache_only) from [<c0465fa8>] (tegra30_ahub_probe+0x1b8/0x430)
> [<c0465fa8>] (tegra30_ahub_probe) from [<c02e196c>] (platform_drv_probe+0x2c/0x5c)
> [<c02e196c>] (platform_drv_probe) from [<c02e0580>] (driver_probe_device+0x10c/0x22c)
> [<c02e0580>] (driver_probe_device) from [<c02e072c>] (__driver_attach+0x8c/0x90)
> 
> This patch solves this by:
> * When looking up the endianness from DT, don't change *endian at all if
>   there is no DT property; leave it set to REGMAP_ENDIAN_DEFAULT so the
>   code falls through to other data sources in the same way as before.
>   Now, the "unspecified" case acts the same for both REGMAP_ENDIAN_REG and
>   REGMAP_ENDIAN_VAL.
> * After potentially looking up the endianness from DT, check *endian
>   against REGMAP_ENDIAN_DEFAULT instead of REGMAP_ENDIAN_NATIVE to avoid
>   returning unexpected values.
> 
> Also, clean up the code a bit:
> 
> * Make the comments briefer, and only refer to the specific action taken
>   at their location. This makes most of the comments independent of DT,
>   and easier to follow.
> * Restore the overall default of REGMAP_ENDIAN_BIG if none of the config,
>   DT, or the bus specify any endianness. Since all busses specify an
>   endianness now, this makes no difference, but I saw no justification in
>   the patch description for changing the default default.
> * s/of_regmap_get_endian/regmap_get_endian/ since the function isn't DT-
>   specific, even if the reason it was originally added was to add some
>   DT-specific features.
> 
> Reported-by: Thierry Reding <treding@xxxxxxxxxx>
> Fixes: d647c199510c ("regmap: add DT endianness binding support")
> Cc: Xiubo Li <Li.Xiubo@xxxxxxxxxxxxx>
> Signed-off-by: Stephen Warren <swarren@xxxxxxxxxx>
> ---
>  drivers/base/regmap/regmap.c | 58 ++++++++++++++------------------------------
>  1 file changed, 18 insertions(+), 40 deletions(-)

Thanks for fixing this, Stephen.

Tested-by: Thierry Reding <treding@xxxxxxxxxx>

Attachment: pgpr5t2vLkleO.pgp
Description: PGP signature