| Message ID | 1555410544-14889-1-git-send-email-jonathanh@nvidia.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [1/3] soc/tegra: pmc: Fix definition of reset sources and levels | expand |
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@nvidia.com> > --- > 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
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@nvidia.com> >> --- >> 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
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; 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" }; /** @@ -2165,9 +2175,7 @@ static const struct tegra_pmc_soc tegra20_pmc_soc = { .init = tegra20_pmc_init, .setup_irq_polarity = tegra20_pmc_setup_irq_polarity, .reset_sources = NULL, - .num_reset_sources = 0, .reset_levels = NULL, - .num_reset_levels = 0, }; static const char * const tegra30_powergates[] = { @@ -2212,9 +2220,7 @@ static const struct tegra_pmc_soc tegra30_pmc_soc = { .init = tegra20_pmc_init, .setup_irq_polarity = tegra20_pmc_setup_irq_polarity, .reset_sources = tegra30_reset_sources, - .num_reset_sources = 5, .reset_levels = NULL, - .num_reset_levels = 0, }; static const char * const tegra114_powergates[] = { @@ -2263,9 +2269,7 @@ static const struct tegra_pmc_soc tegra114_pmc_soc = { .init = tegra20_pmc_init, .setup_irq_polarity = tegra20_pmc_setup_irq_polarity, .reset_sources = tegra30_reset_sources, - .num_reset_sources = 5, .reset_levels = NULL, - .num_reset_levels = 0, }; static const char * const tegra124_powergates[] = { @@ -2374,9 +2378,7 @@ static const struct tegra_pmc_soc tegra124_pmc_soc = { .init = tegra20_pmc_init, .setup_irq_polarity = tegra20_pmc_setup_irq_polarity, .reset_sources = tegra30_reset_sources, - .num_reset_sources = 5, .reset_levels = NULL, - .num_reset_levels = 0, }; static const char * const tegra210_powergates[] = { @@ -2479,10 +2481,8 @@ static const struct tegra_pmc_soc tegra210_pmc_soc = { .regs = &tegra20_pmc_regs, .init = tegra20_pmc_init, .setup_irq_polarity = tegra20_pmc_setup_irq_polarity, - .reset_sources = tegra30_reset_sources, - .num_reset_sources = 5, + .reset_sources = tegra210_reset_sources, .reset_levels = NULL, - .num_reset_levels = 0, }; #define TEGRA186_IO_PAD_TABLE(_pad) \ @@ -2605,9 +2605,7 @@ static const struct tegra_pmc_soc tegra186_pmc_soc = { .init = NULL, .setup_irq_polarity = tegra186_pmc_setup_irq_polarity, .reset_sources = tegra186_reset_sources, - .num_reset_sources = 14, .reset_levels = tegra186_reset_levels, - .num_reset_levels = 3, .num_wake_events = ARRAY_SIZE(tegra186_wake_events), .wake_events = tegra186_wake_events, };
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@nvidia.com> --- drivers/soc/tegra/pmc.c | 32 +++++++++++++++----------------- 1 file changed, 15 insertions(+), 17 deletions(-)