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:
pgpUufUZNF_A4.pgp
Description: PGP signature
