| 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
| ||
|
Message-ID: <AM6PR0402MB3911BA1CDA911F94071BE320F54A0@AM6PR0402MB3911.eurprd04.prod.outlook.com>
Date: Wed, 13 Mar 2019 08:11:22 +0000
From: Anson Huang <anson.huang@....com>
To: Rob Herring <robh@...nel.org>
CC: Guenter Roeck <linux@...ck-us.net>,
"mark.rutland@....com" <mark.rutland@....com>,
"ulf.hansson@...aro.org" <ulf.hansson@...aro.org>,
"heiko@...ech.de" <heiko@...ech.de>,
"catalin.marinas@....com" <catalin.marinas@....com>,
"will.deacon@....com" <will.deacon@....com>,
"bjorn.andersson@...aro.org" <bjorn.andersson@...aro.org>,
"festevam@...il.com" <festevam@...il.com>,
"jagan@...rulasolutions.com" <jagan@...rulasolutions.com>,
Andy Gross <andy.gross@...aro.org>,
dl-linux-imx <linux-imx@....com>,
"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
"linux-watchdog@...r.kernel.org" <linux-watchdog@...r.kernel.org>,
"arnd@...db.de" <arnd@...db.de>,
"marc.w.gonzalez@...e.fr" <marc.w.gonzalez@...e.fr>,
"s.hauer@...gutronix.de" <s.hauer@...gutronix.de>,
"enric.balletbo@...labora.com" <enric.balletbo@...labora.com>,
"horms+renesas@...ge.net.au" <horms+renesas@...ge.net.au>,
"wim@...ux-watchdog.org" <wim@...ux-watchdog.org>,
Daniel Baluta <daniel.baluta@....com>,
"linux-arm-kernel@...ts.infradead.org"
<linux-arm-kernel@...ts.infradead.org>,
Aisheng Dong <aisheng.dong@....com>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"kernel@...gutronix.de" <kernel@...gutronix.de>,
"olof@...om.net" <olof@...om.net>,
"shawnguo@...nel.org" <shawnguo@...nel.org>
Subject: RE: [PATCH RESEND V2 1/4] dt-bindings: fsl: scu: add watchdog binding
Hi, Rob
Do you have any feedback about adding imx scu watchdog node in dts? Thanks.
Best Regards!
Anson Huang
> -----Original Message-----
> From: Anson Huang
> Sent: 2019年3月6日 22:45
> To: 'Rob Herring' <robh@...nel.org>
> Cc: Guenter Roeck <linux@...ck-us.net>; mark.rutland@....com;
> ulf.hansson@...aro.org; heiko@...ech.de; catalin.marinas@....com;
> will.deacon@....com; bjorn.andersson@...aro.org; festevam@...il.com;
> jagan@...rulasolutions.com; Andy Gross <andy.gross@...aro.org>; dl-
> linux-imx <linux-imx@....com>; devicetree@...r.kernel.org; linux-
> watchdog@...r.kernel.org; arnd@...db.de; marc.w.gonzalez@...e.fr;
> s.hauer@...gutronix.de; enric.balletbo@...labora.com;
> horms+renesas@...ge.net.au; wim@...ux-watchdog.org; Daniel Baluta
> <daniel.baluta@....com>; linux-arm-kernel@...ts.infradead.org; Aisheng
> Dong <aisheng.dong@....com>; linux-kernel@...r.kernel.org;
> kernel@...gutronix.de; olof@...om.net; shawnguo@...nel.org
> Subject: RE: [PATCH RESEND V2 1/4] dt-bindings: fsl: scu: add watchdog
> binding
>
> Hi, Rob
> Sorry to bring back this topic again about whether to put "imx-sc-
> wdt" node inside DT's SCU node, after some discussion with my team, there
> is case of virtualization, disabling "imx-sc-wdt" for Linux OS, while enable it
> for Android OS running together on same SoC, then Linux needs to disable
> watchdog from its DTB while Android can enable it. For such kind of scenario,
> do you think it is reasonable to have "imx-sc-wdt" node inside SCU node in
> DT? We do NOT have good idea for this scenario if imx-sc-wdt is added as
> built-in platform device inside SCU driver.
>
> Best Regards!
> Anson Huang
>
> > -----Original Message-----
> > From: Rob Herring [mailto:robh@...nel.org]
> > Sent: 2019年2月27日 5:34
> > To: Anson Huang <anson.huang@....com>
> > Cc: Guenter Roeck <linux@...ck-us.net>; mark.rutland@....com;
> > ulf.hansson@...aro.org; heiko@...ech.de; catalin.marinas@....com;
> > will.deacon@....com; bjorn.andersson@...aro.org; festevam@...il.com;
> > jagan@...rulasolutions.com; Andy Gross <andy.gross@...aro.org>; dl-
> > linux-imx <linux-imx@....com>; devicetree@...r.kernel.org; linux-
> > watchdog@...r.kernel.org; arnd@...db.de; marc.w.gonzalez@...e.fr;
> > s.hauer@...gutronix.de; enric.balletbo@...labora.com;
> > horms+renesas@...ge.net.au; wim@...ux-watchdog.org; Daniel Baluta
> > <daniel.baluta@....com>; linux-arm-kernel@...ts.infradead.org; Aisheng
> > Dong <aisheng.dong@....com>; linux-kernel@...r.kernel.org;
> > kernel@...gutronix.de; olof@...om.net; shawnguo@...nel.org
> > Subject: Re: [PATCH RESEND V2 1/4] dt-bindings: fsl: scu: add watchdog
> > binding
> >
> > On Sun, Feb 24, 2019 at 8:26 PM Anson Huang <anson.huang@....com>
> > wrote:
> > >
> > > Hi, Guenter
> > >
> > > Best Regards!
> > > Anson Huang
> > >
> > > > -----Original Message-----
> > > > From: Guenter Roeck [mailto:groeck7@...il.com] On Behalf Of
> > > > Guenter Roeck
> > > > Sent: 2019年2月24日 11:20
> > > > To: Anson Huang <anson.huang@....com>; Rob Herring
> > <robh@...nel.org>
> > > > Cc: mark.rutland@....com; shawnguo@...nel.org;
> > > > s.hauer@...gutronix.de; kernel@...gutronix.de;
> festevam@...il.com;
> > > > catalin.marinas@....com; will.deacon@....com;
> > > > wim@...ux-watchdog.org; Aisheng Dong <aisheng.dong@....com>;
> > > > ulf.hansson@...aro.org; Daniel Baluta <daniel.baluta@....com>;
> > > > Andy Gross <andy.gross@...aro.org>;
> > > > horms+renesas@...ge.net.au; heiko@...ech.de; arnd@...db.de;
> > > > bjorn.andersson@...aro.org; jagan@...rulasolutions.com;
> > > > enric.balletbo@...labora.com; marc.w.gonzalez@...e.fr;
> > > > olof@...om.net; devicetree@...r.kernel.org;
> > > > linux-kernel@...r.kernel.org; linux-arm-
> > > > kernel@...ts.infradead.org; linux-watchdog@...r.kernel.org;
> > > > dl-linux-imx <linux-imx@....com>
> > > > Subject: Re: [PATCH RESEND V2 1/4] dt-bindings: fsl: scu: add
> > > > watchdog binding
> > > >
> > > > On 2/23/19 7:04 PM, Anson Huang wrote:
> > > > > Hi, Guenter/Rob
> > > > >
> > > > > Best Regards!
> > > > > Anson Huang
> > > > >
> > > > >> -----Original Message-----
> > > > >> From: Guenter Roeck [mailto:groeck7@...il.com] On Behalf Of
> > > > >> Guenter Roeck
> > > > >> Sent: 2019年2月24日 1:08
> > > > >> To: Rob Herring <robh@...nel.org>; Anson Huang
> > > > <anson.huang@....com>
> > > > >> Cc: mark.rutland@....com; shawnguo@...nel.org;
> > > > >> s.hauer@...gutronix.de; kernel@...gutronix.de;
> > > > >> festevam@...il.com; catalin.marinas@....com;
> > will.deacon@....com;
> > > > >> wim@...ux-
> > > > watchdog.org;
> > > > >> Aisheng Dong <aisheng.dong@....com>; ulf.hansson@...aro.org;
> > > > >> Daniel Baluta <daniel.baluta@....com>; Andy Gross
> > > > >> <andy.gross@...aro.org>;
> > > > >> horms+renesas@...ge.net.au; heiko@...ech.de; arnd@...db.de;
> > > > >> bjorn.andersson@...aro.org; jagan@...rulasolutions.com;
> > > > >> enric.balletbo@...labora.com; marc.w.gonzalez@...e.fr;
> > > > >> olof@...om.net; devicetree@...r.kernel.org;
> > > > >> linux-kernel@...r.kernel.org; linux-arm-
> > > > >> kernel@...ts.infradead.org; linux-watchdog@...r.kernel.org;
> > > > >> dl-linux-imx <linux-imx@....com>
> > > > >> Subject: Re: [PATCH RESEND V2 1/4] dt-bindings: fsl: scu: add
> > > > >> watchdog binding
> > > > >>
> > > > >> On 2/22/19 11:52 AM, Rob Herring wrote:
> > > > >>> On Mon, Feb 18, 2019 at 06:53:48AM +0000, Anson Huang wrote:
> > > > >>>> Add i.MX8QXP system controller watchdog binding.
> > > > >>>>
> > > > >>>> Signed-off-by: Anson Huang <Anson.Huang@....com>
> > > > >>>> ---
> > > > >>>> Changes since V1:
> > > > >>>> - update dts node name to "watchdog";
> > > > >>>> ---
> > > > >>>>
> > > > >>>> Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> > > > >>>> | 10
> > > > >> ++++++++++
> > > > >>>> 1 file changed, 10 insertions(+)
> > > > >>>>
> > > > >>>> diff --git
> > > > >>>> a/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> > > > >>>> b/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> > > > >>>> index 4b79751..f388ec6 100644
> > > > >>>> ---
> > > > >>>> a/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> > > > >>>> +++ b/Documentation/devicetree/bindings/arm/freescale/fsl,scu
> > > > >>>> +++ .t
> > > > >>>> +++ xt
> > > > >>>> @@ -136,6 +136,12 @@ Required properties:
> > > > >>>> resource id for thermal driver to
> > > > >>>> get temperature
> > > > >> via
> > > > >>>> SCU IPC.
> > > > >>>>
> > > > >>>> +Watchdog bindings based on SCU Message Protocol
> > > > >>>> +------------------------------------------------------------
> > > > >>>> +
> > > > >>>> +Required properties:
> > > > >>>> +- compatible: should be "fsl,imx8qxp-sc-wdt";
> > > > >>>> +
> > > > >>>> Example (imx8qxp):
> > > > >>>> -------------
> > > > >>>> lsio_mu1: mailbox@...c0000 { @@ -188,6 +194,10 @@
> firmware
> > {
> > > > >>>> tsens-num = <1>;
> > > > >>>> #thermal-sensor-cells = <1>;
> > > > >>>> };
> > > > >>>> +
> > > > >>>> + watchdog: watchdog {
> > > > >>>> + compatible = "fsl,imx8qxp-sc-wdt";
> > > > >>>
> > > > >>> As-is, there's no reason for this to be in DT. The parent
> > > > >>> node's driver can instantiate the wdog.
> > > > >>>
> > > > >>
> > > > >> As the driver is currently written, you are correct, since it
> > > > >> doesn't accept watchdog timeout configuration through devicetree.
> > > > >>
> > > > >> Question is if that is intended. Is it ?
> > > > >
> > > > > I am a little confused, do you mean we no need to have "watchdog"
> > > > > node
> > > > in side "scu"
> > > > > node? Or we need to modify the watchdog node's compatible string
> to "
> > > > > fsl,imx-sc-wdt" to make it more generic for other platforms? If
> > > > > yes, I can
> > > > resend the patch series to modify it.
> > > > >
> > > >
> > > > I think Rob suggested that the SCU parent driver should
> > > > instantiate the watchdog without explicit watchdog node. That
> > > > would be possible, but it currently uses
> > > > devm_of_platform_populate() to do the instantiation, and changing
> > > > that would be a mess. Besides, it does sem to me that your
> > > > suggested node would describe the hardware, so I am not sure I
> > > > understand the
> > reasoning.
> >
> > It would just be a call to create a platform device instead. How is that a
> mess?
> >
> > It's describing firmware. We have DT for describing h/w we've failed
> > to make discoverable. We should not repeat that and just describe
> firmware in DT.
> > Make the firmware discoverable! Though there are cases like firmware
> > provided clocks where we still need something in DT, but this is not
> > one of them.
> >
> > > >
> > > > For my part I referred to
> > > > watchdog_init_timeout(imx_sc_wdd, DEFAULT_TIMEOUT, &pdev-
> > > > >dev); in the driver, which guarantees that the timeout property
> > > > >will not be
> > > > used to set the timeout. A more common implementation would have
> > > > been
> > > >
> > > > imx_sc_wdd->timeout = DEFAULT_TIMEOUT;
> > > > ret = watchdog_init_timeout(imx_sc_wdd, timeout,
> > > > &pdev->dev);
> > > >
> > > > where 'timeout' is the module parameter. Which is actually not
> > > > used in your driver.
> > > > Hmm ... I wasn't careful enough with my review. The timeout
> > > > initialization as- is doesn't make sense. I'll comment on that in the patch.
> > >
> > > I understand now, in our cases, I would still prefer to have
> > > watchdog node under the SCU parent node, since there could be other
> > > property setting difference between different i.MX platforms with
> > > system controller watchdog inside, using the SCU node to instantiate
> > > makes us a little confused about the watchdog, so if it is NOT that
> > > critical, I think we should keep watchdog node. But to make the
> > > watchdog driver more generic for other i.MX platforms, I changed the
> > > compatible string to
> > "fsl,imx-sc-wdt" in driver, and each SoC should has it as fallback if
> > it can reuse this watchdog driver.
> >
> > You handle differences between SoCs by having specific compatibles. So
> > "fsl,imx-sc-wdt" moves in the wrong direction assuming we have a node
> > in the first place.
> >
> > Rob
Powered by blists - more mailing lists