On Tue, Apr 16, 2019 at 11:29:02AM +0100, Jon Hunter wrote:
> Commit 5f84bb1a4099 ("soc/tegra: pmc: Add sysfs entries for reset info")
> added support for reading the Tegra reset source and level from sysfs.
> However, there are a few issues with this commit which are ...
> 1. The number of reset sources for Tegra210 is defined as 5 but it
> should be 6.
> 2. The number of reset sources for Tegra186 is defined as 13 but it
> should be 15.
> 3. The SoC data variables num_reset_sources and num_reset_levels are
> defined but never used.
>
> Fix the above by ...
> 1. Removing the reset source 'AOTAG' from the tegra30_reset_sources
> because this is only applicable for Tegra210.
> 2. Adding a new tegra210_reset_sources structure for Tegra210 reset
> sources.
> 3. Padding the reset sources structures with 'UNKNOWN' for any bit
> field values that are not valid. Although it should not really be
> necessary as these values should never be returned, it avoids
> having to check the value read from the register.
> 4. Removing the unused SoC data variables num_reset_sources and
> num_reset_levels.
>
> Fixes: 5f84bb1a4099 ("soc/tegra: pmc: Add sysfs entries for reset info")
> Signed-off-by: Jon Hunter <jonathanh@xxxxxxxxxx>
> ---
> drivers/soc/tegra/pmc.c | 32 +++++++++++++++-----------------
> 1 file changed, 15 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c
> index 0c5f79528e5f..61c61994f17d 100644
> --- a/drivers/soc/tegra/pmc.c
> +++ b/drivers/soc/tegra/pmc.c
> @@ -237,9 +237,7 @@ struct tegra_pmc_soc {
> bool invert);
>
> const char * const *reset_sources;
> - unsigned int num_reset_sources;
> const char * const *reset_levels;
> - unsigned int num_reset_levels;
Can we keep these and make sure that we properly initialize them? That
way we can check the bounds in the sysfs implementation to make sure we
don't accidentally return kernel memory beyond the array if someone
managed to inject an invalid value somehow. Or if we read a wrong value
for whatever reason. Like you said, it should never happen, but better
safe than sorry.
>
> const struct tegra_wake_event *wake_events;
> unsigned int num_wake_events;
> @@ -260,7 +258,8 @@ static const char * const tegra186_reset_sources[] = {
> "SWREST",
> "SC7",
> "HSM",
> - "CORESIGHT"
> + "CORESIGHT",
> + "UNKNOWN"
> };
>
> static const char * const tegra186_reset_levels[] = {
> @@ -273,7 +272,18 @@ static const char * const tegra30_reset_sources[] = {
> "SENSOR",
> "SW_MAIN",
> "LP0",
> - "AOTAG"
> + "UNKNOWN",
> + "UNKNOWN"
> +};
> +
> +static const char * const tegra210_reset_sources[] = {
> + "POWER_ON_RESET",
> + "WATCHDOG",
> + "SENSOR",
> + "SW_MAIN",
> + "LP0",
> + "AOTAG",
> + "UNKNOWN"
> };
Could we leave away the UNKNOWN strings and just return an error or
something from the sysfs implementation when we run into a situation
where the reset source or level is unknown? Might be worth even throwing
in a WARN in those cases to make sure we catch these things. If they
ever happen.
Thierry
Attachment:
signature.asc
Description: PGP signature
