On 16/04/2019 12:13, Thierry Reding wrote: > 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. OK will do. Jon -- nvpublic