linux-kernel - Re: [PATCH 3/9] watchdog: s3c2410_wdt: update to use new exynos_pmu

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 for Android: free password hash cracker in your pocket

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

Date: Tue, 23 Jan 2024 19:37:34 -0800
From: Saravana Kannan <saravanak@...gle.com>
To: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
Cc: Peter Griffin <peter.griffin@...aro.org>, arnd@...db.de, robh+dt@...nel.org, 
	krzysztof.kozlowski+dt@...aro.org, linux@...ck-us.net, wim@...ux-watchdog.org, 
	conor+dt@...nel.org, alim.akhtar@...sung.com, jaewon02.kim@...sung.com, 
	chanho61.park@...sung.com, semen.protsenko@...aro.org, 
	kernel-team@...roid.com, tudor.ambarus@...aro.org, andre.draszik@...aro.org, 
	willmcvicker@...gle.com, linux-fsd@...la.com, linux-watchdog@...r.kernel.org, 
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org, 
	linux-arm-kernel@...ts.infradead.org, linux-samsung-soc@...r.kernel.org
Subject: Re: [PATCH 3/9] watchdog: s3c2410_wdt: update to use new
 exynos_pmu_*() apis

On Tue, Jan 23, 2024 at 10:12 AM Krzysztof Kozlowski
<krzysztof.kozlowski@...aro.org> wrote:
>
> On 23/01/2024 18:30, Peter Griffin wrote:
> >>>               dev_warn(wdt->dev, "Couldn't get RST_STAT register\n");
> >>>       else if (rst_stat & BIT(wdt->drv_data->rst_stat_bit))
> >>> @@ -698,14 +699,6 @@ static int s3c2410wdt_probe(struct platform_device *pdev)
> >>>       if (ret)
> >>>               return ret;
> >>>
> >>> -     if (wdt->drv_data->quirks & QUIRKS_HAVE_PMUREG) {
> >>> -             wdt->pmureg = syscon_regmap_lookup_by_phandle(dev->of_node,
> >>> -                                             "samsung,syscon-phandle");
> >>> -             if (IS_ERR(wdt->pmureg))
> >>> -                     return dev_err_probe(dev, PTR_ERR(wdt->pmureg),
> >>> -                                          "syscon regmap lookup failed.\n");
> >>
> >>
> >> Continuing topic from the binding: I don't see how you handle probe
> >> deferral, suspend ordering.
> >
> > The current implementation is simply relying on exynos-pmu being
> > postcore_initcall level.
> >
> > I was just looking around for any existing Linux APIs that could be a
> > more robust solution. It looks like
> >
> > of_parse_phandle()
> > and
> > of_find_device_by_node();
> >
> > Are often used to solve this type of probe deferral issue between
> > devices. Is that what you would recommend using? Or is there something
> > even better?
>
> I think you should keep the phandle and then set device link based on
> of_find_device_by_node(). This would actually improve the code, because
> syscon_regmap_lookup_by_phandle() does not create device links.

I kinda agree with this. Just because we no longer use a syscon API to
find the PMU register address doesn't mean the WDT doesn't depend on
the PMU.

However, I think we should move to a generic "syscon" property. Then I
can add support for "syscon" property to fw_devlink and then things
will just work in terms of probe ordering, suspend/resume and also
showing the dependency in DT even if you don't use the syscon APIs.

Side note 1:

I think we really should officially document a generic syscon DT
property similar to how we have a generic "clocks" or "dmas" property.
Then we can have a syscon_get_regmap() that's like so:

struct regmap *syscon_get_regmap(struct device *dev)
{
        return syscon_regmap_lookup_by_phandle(dev->of_node, "syscon");
}

Instead of every device defining its own bespoke DT property to do the
exact same thing. I did a quick "back of the envelope" grep on this
and I get about 143 unique properties just to get the syscon regmap.
$ git grep -A1 syscon_regmap_lookup_by_phandle | grep '"' | sed -e
's/^[^"]*//' -e 's/"[^"]*$/"/' | sort | uniq | wc -l
143

Side note 2:

How are we making sure that it's the exynos-pmu driver that ends up
probing the PMU and not the generic syscon driver? Both of these are
platform drivers. And the exynos PMU device lists both the exynos
compatible string and the syscon property. Is it purely a link order
coincidence?

-Saravana