| Message ID | 1555410544-14889-2-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:03AM +0100, Jon Hunter wrote: > Commit 5f84bb1a4099 ("soc/tegra: pmc: Add sysfs entries for reset info") > added sysfs entries for Tegra reset source and level. However, these > sysfs are not removed on error and so if the registering of PMC device > is probe deferred, then the next time we attempt to probe the PMC device > warnings such as the following will be displayed on boot ... > > sysfs: cannot create duplicate filename '/devices/platform/7000e400.pmc/reset_reason' > > Fix this by ... > > 1. Splitting the function tegra_pmc_reset_sysfs_init() into two separate > functions for initialising the reset reason sysfs entry and the reset > level sysfs entry. Given that not all devices support both this > simplifies the cleanup in the error path. > 2. If either of the sysfs entries fail to be created, then instead of > just printing a warning message, print an error message and return > the error to prevent the probe from succeeding and cleanup as > necessary. Previously the intention was to avoid the failure of > creating the sysfs entries to causing the PMC probe to fail, this > should never happen and so it is better to just return the error. I'd prefer to not make sysfs registration failure a fatal error. Granted, it's unlikely to ever happen, but PMC is kind of crucial to the operation of the system, so things like PCI (and therefore networking if the NIC is on the PCI bus) or display may not work if sysfs registration fails, and that in turn makes it really difficult to diagnose the problem. > > Fixes: 5f84bb1a4099 ("soc/tegra: pmc: Add sysfs entries for reset info") > Signed-off-by: Jon Hunter <jonathanh@nvidia.com> > --- > drivers/soc/tegra/pmc.c | 68 ++++++++++++++++++++++++++++++++++++------------- > 1 file changed, 50 insertions(+), 18 deletions(-) > > diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c > index 61c61994f17d..8d769e3ba668 100644 > --- a/drivers/soc/tegra/pmc.c > +++ b/drivers/soc/tegra/pmc.c > @@ -1771,26 +1771,48 @@ static ssize_t reset_level_show(struct device *dev, > > static DEVICE_ATTR_RO(reset_level); > > -static void tegra_pmc_reset_sysfs_init(struct tegra_pmc *pmc) > +static int tegra_pmc_reset_reason_init(struct tegra_pmc *pmc) > { > struct device *dev = pmc->dev; > - int err = 0; > + int err; > > - if (pmc->soc->reset_sources) { > - err = device_create_file(dev, &dev_attr_reset_reason); > - if (err < 0) > - dev_warn(dev, > - "failed to create attr \"reset_reason\": %d\n", > - err); > - } > + if (!pmc->soc->reset_sources) > + return 0; > > - if (pmc->soc->reset_levels) { > - err = device_create_file(dev, &dev_attr_reset_level); > - if (err < 0) > - dev_warn(dev, > - "failed to create attr \"reset_level\": %d\n", > - err); > - } > + err = device_create_file(dev, &dev_attr_reset_reason); > + if (err < 0) > + dev_err(dev, "failed to create attr \"reset_reason\": %d\n", > + err); > + > + return err; > +} > + > +static void tegra_pmc_reset_reason_remove(struct tegra_pmc *pmc) > +{ > + if (pmc->soc->reset_sources) > + device_remove_file(pmc->dev, &dev_attr_reset_reason); I don't think we necessarily need this check. kernfs_remove_by_name_ns() silently fails with -ENOENT if the attribute was never registered. Since we don't check the error code here, we could call it unconditonally, and condense the error handling a bit perhaps. Thierry > +} > + > +static int tegra_pmc_reset_level_init(struct tegra_pmc *pmc) > +{ > + struct device *dev = pmc->dev; > + int err; > + > + if (!pmc->soc->reset_levels) > + return 0; > + > + err = device_create_file(dev, &dev_attr_reset_level); > + if (err < 0) > + dev_err(dev, "failed to create attr \"reset_level\": %d\n", > + err); > + > + return err; > +} > + > +static void tegra_pmc_reset_level_remove(struct tegra_pmc *pmc) > +{ > + if (pmc->soc->reset_levels) > + device_remove_file(pmc->dev, &dev_attr_reset_level); > } > > static int tegra_pmc_irq_translate(struct irq_domain *domain, > @@ -2031,12 +2053,18 @@ static int tegra_pmc_probe(struct platform_device *pdev) > > tegra_pmc_init_tsense_reset(pmc); > > - tegra_pmc_reset_sysfs_init(pmc); > + err = tegra_pmc_reset_reason_init(pmc); > + if (err < 0) > + return err; > + > + err = tegra_pmc_reset_level_init(pmc); > + if (err < 0) > + goto cleanup_reset_reason; > > if (IS_ENABLED(CONFIG_DEBUG_FS)) { > err = tegra_powergate_debugfs_init(); > if (err < 0) > - return err; > + goto cleanup_reset_level; > } > > err = register_restart_handler(&tegra_pmc_restart_handler); > @@ -2067,6 +2095,10 @@ static int tegra_pmc_probe(struct platform_device *pdev) > unregister_restart_handler(&tegra_pmc_restart_handler); > cleanup_debugfs: > debugfs_remove(pmc->debugfs); > +cleanup_reset_level: > + tegra_pmc_reset_level_remove(pmc); > +cleanup_reset_reason: > + tegra_pmc_reset_reason_remove(pmc); > return err; > } > > -- > 2.7.4 >
On 16/04/2019 12:23, Thierry Reding wrote: > On Tue, Apr 16, 2019 at 11:29:03AM +0100, Jon Hunter wrote: >> Commit 5f84bb1a4099 ("soc/tegra: pmc: Add sysfs entries for reset info") >> added sysfs entries for Tegra reset source and level. However, these >> sysfs are not removed on error and so if the registering of PMC device >> is probe deferred, then the next time we attempt to probe the PMC device >> warnings such as the following will be displayed on boot ... >> >> sysfs: cannot create duplicate filename '/devices/platform/7000e400.pmc/reset_reason' >> >> Fix this by ... >> >> 1. Splitting the function tegra_pmc_reset_sysfs_init() into two separate >> functions for initialising the reset reason sysfs entry and the reset >> level sysfs entry. Given that not all devices support both this >> simplifies the cleanup in the error path. >> 2. If either of the sysfs entries fail to be created, then instead of >> just printing a warning message, print an error message and return >> the error to prevent the probe from succeeding and cleanup as >> necessary. Previously the intention was to avoid the failure of >> creating the sysfs entries to causing the PMC probe to fail, this >> should never happen and so it is better to just return the error. > > I'd prefer to not make sysfs registration failure a fatal error. > Granted, it's unlikely to ever happen, but PMC is kind of crucial to the > operation of the system, so things like PCI (and therefore networking if > the NIC is on the PCI bus) or display may not work if sysfs registration > fails, and that in turn makes it really difficult to diagnose the > problem. > >> >> Fixes: 5f84bb1a4099 ("soc/tegra: pmc: Add sysfs entries for reset info") >> Signed-off-by: Jon Hunter <jonathanh@nvidia.com> >> --- >> drivers/soc/tegra/pmc.c | 68 ++++++++++++++++++++++++++++++++++++------------- >> 1 file changed, 50 insertions(+), 18 deletions(-) >> >> diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c >> index 61c61994f17d..8d769e3ba668 100644 >> --- a/drivers/soc/tegra/pmc.c >> +++ b/drivers/soc/tegra/pmc.c >> @@ -1771,26 +1771,48 @@ static ssize_t reset_level_show(struct device *dev, >> >> static DEVICE_ATTR_RO(reset_level); >> >> -static void tegra_pmc_reset_sysfs_init(struct tegra_pmc *pmc) >> +static int tegra_pmc_reset_reason_init(struct tegra_pmc *pmc) >> { >> struct device *dev = pmc->dev; >> - int err = 0; >> + int err; >> >> - if (pmc->soc->reset_sources) { >> - err = device_create_file(dev, &dev_attr_reset_reason); >> - if (err < 0) >> - dev_warn(dev, >> - "failed to create attr \"reset_reason\": %d\n", >> - err); >> - } >> + if (!pmc->soc->reset_sources) >> + return 0; >> >> - if (pmc->soc->reset_levels) { >> - err = device_create_file(dev, &dev_attr_reset_level); >> - if (err < 0) >> - dev_warn(dev, >> - "failed to create attr \"reset_level\": %d\n", >> - err); >> - } >> + err = device_create_file(dev, &dev_attr_reset_reason); >> + if (err < 0) >> + dev_err(dev, "failed to create attr \"reset_reason\": %d\n", >> + err); >> + >> + return err; >> +} >> + >> +static void tegra_pmc_reset_reason_remove(struct tegra_pmc *pmc) >> +{ >> + if (pmc->soc->reset_sources) >> + device_remove_file(pmc->dev, &dev_attr_reset_reason); > > I don't think we necessarily need this check. kernfs_remove_by_name_ns() > silently fails with -ENOENT if the attribute was never registered. Since > we don't check the error code here, we could call it unconditonally, and > condense the error handling a bit perhaps. OK, actually this would simplify this change a lot if we can unconditionally call device_remove_file(). Jon
diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c index 61c61994f17d..8d769e3ba668 100644 --- a/drivers/soc/tegra/pmc.c +++ b/drivers/soc/tegra/pmc.c @@ -1771,26 +1771,48 @@ static ssize_t reset_level_show(struct device *dev, static DEVICE_ATTR_RO(reset_level); -static void tegra_pmc_reset_sysfs_init(struct tegra_pmc *pmc) +static int tegra_pmc_reset_reason_init(struct tegra_pmc *pmc) { struct device *dev = pmc->dev; - int err = 0; + int err; - if (pmc->soc->reset_sources) { - err = device_create_file(dev, &dev_attr_reset_reason); - if (err < 0) - dev_warn(dev, - "failed to create attr \"reset_reason\": %d\n", - err); - } + if (!pmc->soc->reset_sources) + return 0; - if (pmc->soc->reset_levels) { - err = device_create_file(dev, &dev_attr_reset_level); - if (err < 0) - dev_warn(dev, - "failed to create attr \"reset_level\": %d\n", - err); - } + err = device_create_file(dev, &dev_attr_reset_reason); + if (err < 0) + dev_err(dev, "failed to create attr \"reset_reason\": %d\n", + err); + + return err; +} + +static void tegra_pmc_reset_reason_remove(struct tegra_pmc *pmc) +{ + if (pmc->soc->reset_sources) + device_remove_file(pmc->dev, &dev_attr_reset_reason); +} + +static int tegra_pmc_reset_level_init(struct tegra_pmc *pmc) +{ + struct device *dev = pmc->dev; + int err; + + if (!pmc->soc->reset_levels) + return 0; + + err = device_create_file(dev, &dev_attr_reset_level); + if (err < 0) + dev_err(dev, "failed to create attr \"reset_level\": %d\n", + err); + + return err; +} + +static void tegra_pmc_reset_level_remove(struct tegra_pmc *pmc) +{ + if (pmc->soc->reset_levels) + device_remove_file(pmc->dev, &dev_attr_reset_level); } static int tegra_pmc_irq_translate(struct irq_domain *domain, @@ -2031,12 +2053,18 @@ static int tegra_pmc_probe(struct platform_device *pdev) tegra_pmc_init_tsense_reset(pmc); - tegra_pmc_reset_sysfs_init(pmc); + err = tegra_pmc_reset_reason_init(pmc); + if (err < 0) + return err; + + err = tegra_pmc_reset_level_init(pmc); + if (err < 0) + goto cleanup_reset_reason; if (IS_ENABLED(CONFIG_DEBUG_FS)) { err = tegra_powergate_debugfs_init(); if (err < 0) - return err; + goto cleanup_reset_level; } err = register_restart_handler(&tegra_pmc_restart_handler); @@ -2067,6 +2095,10 @@ static int tegra_pmc_probe(struct platform_device *pdev) unregister_restart_handler(&tegra_pmc_restart_handler); cleanup_debugfs: debugfs_remove(pmc->debugfs); +cleanup_reset_level: + tegra_pmc_reset_level_remove(pmc); +cleanup_reset_reason: + tegra_pmc_reset_reason_remove(pmc); return err; }
Commit 5f84bb1a4099 ("soc/tegra: pmc: Add sysfs entries for reset info") added sysfs entries for Tegra reset source and level. However, these sysfs are not removed on error and so if the registering of PMC device is probe deferred, then the next time we attempt to probe the PMC device warnings such as the following will be displayed on boot ... sysfs: cannot create duplicate filename '/devices/platform/7000e400.pmc/reset_reason' Fix this by ... 1. Splitting the function tegra_pmc_reset_sysfs_init() into two separate functions for initialising the reset reason sysfs entry and the reset level sysfs entry. Given that not all devices support both this simplifies the cleanup in the error path. 2. If either of the sysfs entries fail to be created, then instead of just printing a warning message, print an error message and return the error to prevent the probe from succeeding and cleanup as necessary. Previously the intention was to avoid the failure of creating the sysfs entries to causing the PMC probe to fail, this should never happen and so it is better to just return the error. Fixes: 5f84bb1a4099 ("soc/tegra: pmc: Add sysfs entries for reset info") Signed-off-by: Jon Hunter <jonathanh@nvidia.com> --- drivers/soc/tegra/pmc.c | 68 ++++++++++++++++++++++++++++++++++++------------- 1 file changed, 50 insertions(+), 18 deletions(-)