linux-kernel - Re: [EXT] Re: [PATCH 2/2] Watchdog: Add octeontx2 GTI watchdog driver

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]

Message-ID: <512ac643-c487-ed83-9bf5-242f4890c680@linaro.org>
Date:   Fri, 14 Apr 2023 14:28:13 +0200
From:   Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
To:     Bharat Bhushan <bbhushan2@...vell.com>,
        "wim@...ux-watchdog.org" <wim@...ux-watchdog.org>,
        "linux@...ck-us.net" <linux@...ck-us.net>,
        "robh+dt@...nel.org" <robh+dt@...nel.org>,
        "krzysztof.kozlowski+dt@...aro.org" 
        <krzysztof.kozlowski+dt@...aro.org>,
        "linux-watchdog@...r.kernel.org" <linux-watchdog@...r.kernel.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [EXT] Re: [PATCH 2/2] Watchdog: Add octeontx2 GTI watchdog driver

On 14/04/2023 13:44, Bharat Bhushan wrote:
> Please see inline 
> 
>> -----Original Message-----
>> From: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
>> Sent: Friday, April 14, 2023 4:58 PM
>> To: Bharat Bhushan <bbhushan2@...vell.com>; wim@...ux-watchdog.org;
>> linux@...ck-us.net; robh+dt@...nel.org; krzysztof.kozlowski+dt@...aro.org;
>> linux-watchdog@...r.kernel.org; devicetree@...r.kernel.org; linux-
>> kernel@...r.kernel.org
>> Subject: [EXT] Re: [PATCH 2/2] Watchdog: Add octeontx2 GTI watchdog driver
>>
>> External Email
>>
>> ----------------------------------------------------------------------
>> On 14/04/2023 12:23, Bharat Bhushan wrote:
>>> GTI watchdog timer are programmed in "interrupt + del3t + reset mode"
>>> and del3t traps are not enabled.
>>> GTI watchdog exception flow is:
>>>  - 1st timer expiration generates watchdog interrupt.
>>>  - 2nd timer expiration is ignored
>>>  - On 3rd timer expiration will trigger a system-wide core reset.
>>>
>>> Signed-off-by: Bharat Bhushan <bbhushan2@...vell.com>
>>> ---
>>>  drivers/watchdog/Kconfig         |   9 ++
>>>  drivers/watchdog/Makefile        |   1 +
>>>  drivers/watchdog/octeontx2_wdt.c | 238
>>> +++++++++++++++++++++++++++++++
>>>  3 files changed, 248 insertions(+)
>>>  create mode 100644 drivers/watchdog/octeontx2_wdt.c
>>>
>>> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig index
>>> f0872970daf9..31ff282c62ad 100644
>>> --- a/drivers/watchdog/Kconfig
>>> +++ b/drivers/watchdog/Kconfig
>>> @@ -2212,4 +2212,13 @@ config KEEMBAY_WATCHDOG
>>>  	  To compile this driver as a module, choose M here: the
>>>  	  module will be called keembay_wdt.
>>>
>>> +config OCTEONTX2_WATCHDOG
>>> +	tristate "OCTEONTX2 Watchdog driver"
>>> +	depends on ARCH_THUNDER || (COMPILE_TEST && 64BIT)
>>
>> Why it cannot be compile tested on 32-bit?
> 
> Used in 64 bit configuration but no harm getting compile tested for 32bit.
> Will change
> 
>>
>>> +	help
>>> +	 OCTEONTX2 GTI hardware supports watchdog timer. This watchdog
>> timer are
>>> +	 programmed in "interrupt + del3t + reset" mode. On first expiry it will
>>> +	 generate interrupt. Second expiry (del3t) is ignored and system will reset
>>> +	 on final timer expiry.
>>> +
>>>  endif # WATCHDOG
>>> diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
>>> index 9cbf6580f16c..aa1b813ad1f9 100644
>>> --- a/drivers/watchdog/Makefile
>>> +++ b/drivers/watchdog/Makefile
>>> @@ -230,3 +230,4 @@ obj-$(CONFIG_MENZ069_WATCHDOG) +=
>> menz69_wdt.o
>>>  obj-$(CONFIG_RAVE_SP_WATCHDOG) += rave-sp-wdt.o
>>>  obj-$(CONFIG_STPMIC1_WATCHDOG) += stpmic1_wdt.o
>>>  obj-$(CONFIG_SL28CPLD_WATCHDOG) += sl28cpld_wdt.o
>>> +obj-$(CONFIG_OCTEONTX2_WATCHDOG) += octeontx2_wdt.o
>>
>> Please tell me that you added it in some reasonable place, not just at the end...
>> The same in Kconfig.
> 
> Is it alphabetical order, any suggestion?

'O' is not after 'S', thus for sure you did not add it in alphabetical
order.

Or what is a question? If so, then yes, usually we try to keep
alphabetical order. Anyway adding entries to the end is conflictprone.


>>> +	dev_info(dev, "Watchdog enabled (timeout=%d sec)\n", wdog_dev-
>>> timeout);
>>> +	return 0;
>>> +}
>>> +
>>> +static int octeontx2_wdt_remove(struct platform_device *pdev) {
>>> +	struct octeontx2_wdt_priv *priv = platform_get_drvdata(pdev);
>>> +
>>> +	if (priv->irq)
>>
>> Is it possible?

You did not reply, so I assume it is not possible. Drop.

>>
>>> +		free_irq(priv->irq, priv);
>>
>> Anyway, your order of cleanup is a bit surprising. It is expected to be reversed
>> from probe. In probe() you requested IRQs before watchdog, but cleanup will be
>> done before watchdog release? This does not look right.
> 
> Watchdog release happen outside this driver as we used devm_*. 

I know, this is why I raised the question. Don't repeat the obvious but
rather address the problem mentioned here.


> Will convert request_irq to devm_request_irq(). 

If interrupt is not shared, then looks like correct approach.

Best regards,
Krzysztof