linux-kernel - Re: [PATCH v4 05/21] thermal/drivers/tegra: convert to use devm_request*_irq

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

Message-ID: <6db7b1ba-6b5b-3aef-fff7-e36a8fe5739d@kernel.org>
Date:   Mon, 10 Jul 2023 15:55:00 +0200
From:   Krzysztof Kozlowski <krzk@...nel.org>
To:     Thierry Reding <thierry.reding@...il.com>,
        Yangtao Li <frank.li@...o.com>
Cc:     "Rafael J. Wysocki" <rafael@...nel.org>,
        Daniel Lezcano <daniel.lezcano@...aro.org>,
        Amit Kucheria <amitk@...nel.org>,
        Zhang Rui <rui.zhang@...el.com>,
        Jonathan Hunter <jonathanh@...dia.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Uwe Kleine-König <u.kleine-koenig@...gutronix.de>,
        Jonathan Cameron <Jonathan.Cameron@...wei.com>,
        AngeloGioacchino Del Regno 
        <angelogioacchino.delregno@...labora.com>,
        linux-pm@...r.kernel.org, linux-tegra@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4 05/21] thermal/drivers/tegra: convert to use
 devm_request*_irq_probe()

On 10/07/2023 15:16, Thierry Reding wrote:
> On Mon, Jul 10, 2023 at 05:59:09PM +0800, Yangtao Li wrote:
>> There are more than 700 calls to devm_request_threaded_irq method and
>> more than 1000 calls to devm_request_irq method. Most drivers only
>> request one interrupt resource, and these error messages are basically
>> the same. If error messages are printed everywhere, more than 2000 lines
>> of code can be saved by removing the msg in the driver.
>>
>> And tglx point out that:
>>
>>   If we actually look at the call sites of
>>   devm_request_threaded_irq() then the vast majority of them print more or
>>   less lousy error messages. A quick grep/sed/awk/sort/uniq revealed
>>
>>      519 messages total (there are probably more)
>>
>>      352 unique messages
>>
>>      323 unique messages after lower casing
>>
>>          Those 323 are mostly just variants of the same patterns with
>>          slight modifications in formatting and information provided.
>>
>>      186 of these messages do not deliver any useful information,
>>          e.g. "no irq", "
>>
>>      The most useful one of all is: "could request wakeup irq: %d"
>>
>>   So there is certainly an argument to be made that this particular
>>   function should print a well formatted and informative error message.
>>
>>   It's not a general allocator like kmalloc(). It's specialized and in the
>>   vast majority of cases failing to request the interrupt causes the
>>   device probe to fail. So having proper and consistent information why
>>   the device cannot be used _is_ useful.
>>
>> So convert to use devm_request*_irq_probe() API, which ensure that all
>> error handling branches print error information.
>>
>> In this way, when this function fails, the upper-layer functions can
>> directly return an error code without missing debugging information.
>> Otherwise, the error message will be printed redundantly or missing.
> 
> Do we really need to keep repeating this same commit message for each
> and everyone of these commits? It's already in the cover letter and
> presumably on the patch that introduces the new helper, so surely we can
> come up with a denser version for individual subsystem patches.

Yeah, this is way too long to put in every commit doing the same but for
different drivers.

Best regards,
Krzysztof