linux-kernel - Re: [PATCH v4 2/2] watchdog: Add watchdog driver for Sunplus SP7021

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] [day] [month] [year] [list]

Message-ID: <8af9cf3d-c048-b9ed-0bac-832ee262917d@roeck-us.net>
Date:   Wed, 5 Jan 2022 07:53:02 -0800
From:   Guenter Roeck <linux@...ck-us.net>
To:     xt.hu[胡先韬] <xt.hu@...lus1.com>,
        Christophe JAILLET <christophe.jaillet@...adoo.fr>
Cc:     "wim@...ux-watchdog.org" <wim@...ux-watchdog.org>,
        "p.zabel@...gutronix.de" <p.zabel@...gutronix.de>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-watchdog@...r.kernel.org" <linux-watchdog@...r.kernel.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        Wells Lu 呂芳騰 <wells.lu@...plus.com>,
        qinjian[覃健] <qinjian@...lus1.com>,
        Rob Herring <robh@...nel.org>
Subject: Re: [PATCH v4 2/2] watchdog: Add watchdog driver for Sunplus SP7021

On 12/30/21 6:50 PM, xt.hu[胡先韬] wrote:
> Hi Chrustophe,
> 
> 	Thanks for your respond.
> 
> Best Regards,
> Xiantao
>> -----Original Message-----
>> From: Christophe JAILLET <christophe.jaillet@...adoo.fr>
>> To: Xiantao Hu <xt.hu@...lus1.com>,
>> 	wim@...ux-watchdog.org, p.zabel@...gutronix.de,
>> 	linux-kernel@...r.kernel.org, linux-watchdog@...r.kernel.org,
>> 	linux@...ck-us.net, robh+dt@...nel.org,
>> 	devicetree@...r.kernel.org
>> Cc: wells.lu@...plus.com, qinjian@...lus1.com
>> Subject: Re: [PATCH v4 2/2] watchdog: Add watchdog driver for Sunplus SP7021
>> Date: Wed, 29 Dec 2021 10:39:08 +0100	[thread overview]
>> Message-ID: <0b102fa0-cbfc-a97e-8e7f-cce8146450bc@...adoo.fr> (raw)
>> In-Reply-To: <20211229054308.63168-3-xt.hu@...lus1.com>
>>
>> ...
>>
>>> +static int sp_wdt_probe(struct platform_device *pdev)
>>> +{
>>> +	struct device *dev = &pdev->dev;
>>> +	struct sp_wdt_priv *priv;
>>> +	int err;
>>> +
>>> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
>>> +	if (!priv)
>>> +		return -ENOMEM;
>>> +
>>> +	priv->clk = devm_clk_get(dev, NULL);
>>> +	if (IS_ERR(priv->clk)) {
>>> +		dev_err(dev, "Can't find clock source\n");
>>> +		return PTR_ERR(priv->clk);
>>> +	}
>>> +
>>> +	err = clk_prepare_enable(priv->clk);
>>> +	if (err) {
>>> +		dev_err(dev, "Clock can't be enabled correctly\n");
>>> +		return err;
>>> +	}
>>> +
>>> +	/* The timer and watchdog shared the STC reset */
>>> +	priv->rstc = devm_reset_control_get_shared(dev, NULL);
>>> +	if (!IS_ERR(priv->rstc))
>>> +		reset_control_deassert(priv->rstc);
>>> +
>>> +	err = devm_add_action_or_reset(dev, sp_reset_control_assert,
>>> +				       priv->rstc);
>>> +	if (err)
>>> +		return err;
>> This looks odd.
>> We could undo something that was not done. (if IS_ERR(priv->rstc))
>> This is also not really consistent with what is done in suspedn/resume.
>> In these functions, we don't check for IS_ERR(priv->rstc).
>>
> 
> Here I refer to mt7621_wdt.c. I'm sure I need deassert reset to reset
> watchdog register value when driver probe. accordingly I assert reset
> in devm_add_action_or_reset() to ensure that the registers of watchdog
> can't be operated after module remove.
> 

You are missing the point here. This is called even after
devm_reset_control_get_shared() failed, and it is called even though
the reset was not yet deasserted. The above code should be called after
(and only after) deasserting the reset.

>>> +
>>> +	err = devm_add_action_or_reset(dev, sp_clk_disable_unprepare,
>>> +				       priv->clk);
>>> +	if (err)
>>> +		return err;
>> Shouldn't this be just after clk_prepare_enable()?
> 
> I tested the order of execution of the added functions which is similar to
> push and pop. First in, last out. I think I should disable clock last.
> 
Correct. That is why you have to call devm_add_action_or_reset on disable_unprepare
first, because on driver removal the devm_ cleanup will be called in reverse order.

Guenter