linux-kernel - Re: [PATCH v4 2/4] can: m_can: Add hrtimer to generate software interrupt

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: <f8077b36-ed89-e957-1aba-f9cda7acd566@ti.com>
Date:   Wed, 10 May 2023 09:23:46 -0500
From:   Judith Mendez <jm@...com>
To:     Marc Kleine-Budde <mkl@...gutronix.de>
CC:     Chandrasekar Ramakrishnan <rcsekar@...sung.com>,
        Wolfgang Grandegger <wg@...ndegger.com>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        "David S . Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>, <linux-can@...r.kernel.org>,
        <netdev@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        Schuyler Patton <spatton@...com>, Nishanth Menon <nm@...com>,
        Vignesh Raghavendra <vigneshr@...com>,
        Tero Kristo <kristo@...nel.org>,
        Rob Herring <robh+dt@...nel.org>,
        <linux-arm-kernel@...ts.infradead.org>,
        <devicetree@...r.kernel.org>,
        Oliver Hartkopp <socketcan@...tkopp.net>,
        Simon Horman <simon.horman@...igine.com>
Subject: Re: [PATCH v4 2/4] can: m_can: Add hrtimer to generate software
 interrupt

Hello Marc,

On 5/10/23 02:21, Marc Kleine-Budde wrote:
> On 09.05.2023 17:18:06, Judith Mendez wrote:
> [...]
>>>> +	if (!mcan_class->polling && irq < 0) {
>>>> +		ret = -ENXIO;
>>>> +		dev_err_probe(mcan_class->dev, ret, "IRQ int0 not found and polling not activated\n");
>>>> +		goto probe_fail;
>>>> +	}
>>>> +
>>>> +	if (mcan_class->polling) {
>>>> +		if (irq > 0) {
>>>> +			mcan_class->polling = 0;
>>>
>>> false
>>>
>>>> +			dev_dbg(mcan_class->dev, "Polling enabled and hardware IRQ found, use hardware IRQ\n");
>>>
>>> "...using hardware IRQ"
>>>
>>> Use dev_info(), as there is something not 100% correct with the DT.
>>
>> Is it dev_info or dev_dbg?
> 
> dev_info() - But without an explicit "poll-interval' in the DT, this
> code path doesn't exist anymore.
> 
>> I used to have dev_info since it was nice to see when polling was
>> enabled.
> 
> Re-read your code, this is not about enabling polling. This message
> handles the case where an IRQ was given _and_ "poll-interval" was
> specified. So there is something not 100% correct with the DT (IRQ _and_
> polling), but this is obsolete now.

Sorry, I meant it was nice to see when polling was enabled or not. In

this case, if polling was enabled but hardware IRQ exists, it is good

information to see this print. But I understand now how this is a case

where 'something is strange'.

>> Also, I had seen this print and the next as informative prints, hence the dev_info().
> 
> We don't print messages when IRQs are enabled, so enabling polling
> should be a dev_dbg(), too.
> 
>> However, I was told in this review process to change to dev_dbg. Which is correct?
> 
> Driver works correct -> dev_dbg()
> Something is strange -> dev_info()

Got it, this is very helpful. Thanks.



regards,

Judith