[PATCH] regulator: core: Avoid debugfs: Directory ... already present! error

[PATCH] regulator: core: Avoid debugfs: Directory ... already present! error

[Hans de Goede]

Sometimes regulator_get() gets called twice for the same supply on the
same device. This may happen e.g. when a framework / library is used
which uses the regulator; and the driver itself also needs to enable
the regulator in some cases where the framework will not enable it.

Commit ff268b56ce8c ("regulator: core: Don't spew backtraces on
duplicate sysfs") already takes care of the backtrace which would
trigger when creating a duplicate consumer symlink under
/sys/class/regulator/regulator.%d in this scenario.

Commit c33d442328f5 ("debugfs: make error message a bit more verbose")
causes a new error to get logged in this scenario:

[   26.938425] debugfs: Directory 'wm5102-codec-MICVDD' with parent 'spi-WM510204:00-MICVDD' already present!

There is no _nowarn variant of debugfs_create_dir(), but we can detect
and avoid this problem by checking the return value of the earlier
sysfs_create_link_nowarn() call.

Add a check for the earlier sysfs_create_link_nowarn() failing with
-EEXIST and skip the debugfs_create_dir() call in that case, avoiding
this error getting logged.

Fixes: c33d442328f5 ("debugfs: make error message a bit more verbose")
Cc: Charles Keepax <ckeepax@opensource.cirrus.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/regulator/core.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index ca03d8e70bd1..75ec6f334506 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -1617,7 +1617,7 @@ static struct regulator *create_regulator(struct regulator_dev *rdev,
 					  const char *supply_name)
 {
 	struct regulator *regulator;
-	int err;
+	int err = 0;
 
 	if (dev) {
 		char buf[REG_STR_SIZE];
@@ -1663,8 +1663,8 @@ static struct regulator *create_regulator(struct regulator_dev *rdev,
 		}
 	}
 
-	regulator->debugfs = debugfs_create_dir(supply_name,
-						rdev->debugfs);
+	if (err != -EEXIST)
+		regulator->debugfs = debugfs_create_dir(supply_name, rdev->debugfs);
 	if (!regulator->debugfs) {
 		rdev_dbg(rdev, "Failed to create debugfs directory\n");
 	} else {
-- 
2.28.0

Re: [PATCH] regulator: core: Avoid debugfs: Directory ... already present! error

[Charles Keepax]

On Fri, Jan 22, 2021 at 07:32:50PM +0100, Hans de Goede wrote:
> Sometimes regulator_get() gets called twice for the same supply on the
> same device. This may happen e.g. when a framework / library is used
> which uses the regulator; and the driver itself also needs to enable
> the regulator in some cases where the framework will not enable it.
> 
> Commit ff268b56ce8c ("regulator: core: Don't spew backtraces on
> duplicate sysfs") already takes care of the backtrace which would
> trigger when creating a duplicate consumer symlink under
> /sys/class/regulator/regulator.%d in this scenario.
> 
> Commit c33d442328f5 ("debugfs: make error message a bit more verbose")
> causes a new error to get logged in this scenario:
> 
> [   26.938425] debugfs: Directory 'wm5102-codec-MICVDD' with parent 'spi-WM510204:00-MICVDD' already present!
> 
> There is no _nowarn variant of debugfs_create_dir(), but we can detect
> and avoid this problem by checking the return value of the earlier
> sysfs_create_link_nowarn() call.
> 
> Add a check for the earlier sysfs_create_link_nowarn() failing with
> -EEXIST and skip the debugfs_create_dir() call in that case, avoiding
> this error getting logged.
> 
> Fixes: c33d442328f5 ("debugfs: make error message a bit more verbose")
> Cc: Charles Keepax <ckeepax@opensource.cirrus.com>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---

Reviewed-by: Charles Keepax <ckeepax@opensource.cirrus.com>

Thanks,
Charles

> -	int err;
> +	int err = 0;
>  
> @@ -1663,8 +1663,8 @@ static struct regulator *create_regulator(struct regulator_dev *rdev,
>  
> -	regulator->debugfs = debugfs_create_dir(supply_name,
> -						rdev->debugfs);
> +	if (err != -EEXIST)
> +		regulator->debugfs = debugfs_create_dir(supply_name, rdev->debugfs);

There is a slight oddity here in that if this regulator has
no struct device we will still get the warning. However, I
am totally not clear on when/why a regulator might not have a
dev, and am fairly sure it isn't common. So my vote would be
to cross that bridge if we ever come to it.

Thanks,
Charles

Re: [PATCH] regulator: core: Avoid debugfs: Directory ... already present! error

[Hans de Goede]

Hi,

On 1/23/21 11:29 AM, Charles Keepax wrote:
> On Fri, Jan 22, 2021 at 07:32:50PM +0100, Hans de Goede wrote:
>> Sometimes regulator_get() gets called twice for the same supply on the
>> same device. This may happen e.g. when a framework / library is used
>> which uses the regulator; and the driver itself also needs to enable
>> the regulator in some cases where the framework will not enable it.
>>
>> Commit ff268b56ce8c ("regulator: core: Don't spew backtraces on
>> duplicate sysfs") already takes care of the backtrace which would
>> trigger when creating a duplicate consumer symlink under
>> /sys/class/regulator/regulator.%d in this scenario.
>>
>> Commit c33d442328f5 ("debugfs: make error message a bit more verbose")
>> causes a new error to get logged in this scenario:
>>
>> [   26.938425] debugfs: Directory 'wm5102-codec-MICVDD' with parent 'spi-WM510204:00-MICVDD' already present!
>>
>> There is no _nowarn variant of debugfs_create_dir(), but we can detect
>> and avoid this problem by checking the return value of the earlier
>> sysfs_create_link_nowarn() call.
>>
>> Add a check for the earlier sysfs_create_link_nowarn() failing with
>> -EEXIST and skip the debugfs_create_dir() call in that case, avoiding
>> this error getting logged.
>>
>> Fixes: c33d442328f5 ("debugfs: make error message a bit more verbose")
>> Cc: Charles Keepax <ckeepax@opensource.cirrus.com>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
> 
> Reviewed-by: Charles Keepax <ckeepax@opensource.cirrus.com>
> 
> Thanks,
> Charles
> 
>> -	int err;
>> +	int err = 0;
>>  
>> @@ -1663,8 +1663,8 @@ static struct regulator *create_regulator(struct regulator_dev *rdev,
>>  
>> -	regulator->debugfs = debugfs_create_dir(supply_name,
>> -						rdev->debugfs);
>> +	if (err != -EEXIST)
>> +		regulator->debugfs = debugfs_create_dir(supply_name, rdev->debugfs);
> 
> There is a slight oddity here in that if this regulator has
> no struct device we will still get the warning. However, I
> am totally not clear on when/why a regulator might not have a
> dev, and am fairly sure it isn't common. So my vote would be
> to cross that bridge if we ever come to it.

Yes, I expect the combination of having 2 consumers which both get the
regulator with a NULL device pointer to be very rare and hopefully
it does not happen at all.

Regards,

Hans

Re: [PATCH] regulator: core: Avoid debugfs: Directory ... already present! error

[Mark Brown]

On Fri, 22 Jan 2021 19:32:50 +0100, Hans de Goede wrote:
> Sometimes regulator_get() gets called twice for the same supply on the
> same device. This may happen e.g. when a framework / library is used
> which uses the regulator; and the driver itself also needs to enable
> the regulator in some cases where the framework will not enable it.
> 
> Commit ff268b56ce8c ("regulator: core: Don't spew backtraces on
> duplicate sysfs") already takes care of the backtrace which would
> trigger when creating a duplicate consumer symlink under
> /sys/class/regulator/regulator.%d in this scenario.
> 
> [...]

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git for-next

Thanks!

[1/1] regulator: core: Avoid debugfs: Directory ... already present! error
      commit: dbe954d8f1635f949a1d9a5d6e6fb749ae022b47

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark