linux-kernel - Re: [PATCH] regulator: core: Don't link consumers on the same device

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 9 Jul 2018 15:32:14 +0200
From:   Marek Szyprowski <m.szyprowski@...sung.com>
To:     Mark Brown <broonie@...nel.org>, pascal paillet <p.paillet@...com>,
        benjamin.gaignard@...aro.org
Cc:     Krzysztof Kozlowski <krzk@...nel.org>,
        Bartlomiej Zolnierkiewicz <b.zolnierkie@...sung.com>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] regulator: core: Don't link consumers on the same
 device

Hi Mark,

On 2018-07-09 14:54, Mark Brown wrote:
> In some cases a device may end up supplying itself, for example when a
> DCDC is used to improve the efficiency of LDOs or when a LDO is provided
> to clean up sensitive supplies.  In these cases the driver core will
> warn loudly about attempts to add links so suppress them here.
>
> Reported-by: Marek Szyprowski <m.szyprowski@...sung.com>
> Signed-off-by: Mark Brown <broonie@...nel.org>
> ---
>
> Compile tested only.

This patch doesn't help, because wm8994 creates separate struct device for
each registered regulator, so you need to check the regulator parents too,
see my fixes below in the patch.

I don't like such fixes for this specific case. It's not that uncommon
that a complex device provides various functionalities used by its
components, so it is not a bad design to use regulator provided by
its parent/grandparent/other ancestor device.

Frankly, either the device_is_dependent() function should be exported and
used in regulator core or device_link_add() should have another flag:
DL_FLAG_SKIP_DEPENDANT.

>   drivers/regulator/core.c | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
> index b5db0257bc40..1302e66e80ac 100644
> --- a/drivers/regulator/core.c
> +++ b/drivers/regulator/core.c
> @@ -1741,7 +1741,8 @@ struct regulator *_regulator_get(struct device *dev, const char *id,
>   			rdev->use_count = 0;
>   	}
>   
> -	device_link_add(dev, &rdev->dev, DL_FLAG_STATELESS);
> +	if (dev != &rdev->dev)

    if (dev != &rdev->dev && dev != rdev->dev.parent)

> +		device_link_add(dev, &rdev->dev, DL_FLAG_STATELESS);
>   
>   	return regulator;
>   }
> @@ -1840,7 +1841,7 @@ static void _regulator_put(struct regulator *regulator)
>   			if (r->dev == regulator->dev)
>   				count++;
>   
> -		if (count == 1)
> +		if (count == 1 && regulator->dev != &rdev->dev)

    if (count == 1 && regulator->dev != &rdev->dev &&
                     regulator->dev != rdev->dev.parent)

>   			device_link_remove(regulator->dev, &rdev->dev);
>   
>   		/* remove any sysfs entries */

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland