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@xxxxxxxxxx>
>> ---
>> 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
--
nvpublic
