Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <690b12b7-5586-6ade-de83-99f463bc8397@vivo.com>
Date:   Mon, 3 Jul 2023 17:13:29 +0800
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.10.1
Subject: Re: [PATCH v2 01/15] genirq/devres: Add error information printing
 for devm_request_threaded_irq()
To:     Thomas Gleixner <tglx@linutronix.de>,
        =?UTF-8?Q?Uwe_Kleine-K=c3=b6nig?= <u.kleine-koenig@pengutronix.de>
Cc:     miquel.raynal@bootlin.com, rafael@kernel.org,
        daniel.lezcano@linaro.org, amitk@kernel.org, rui.zhang@intel.com,
        mmayer@broadcom.com, bcm-kernel-feedback-list@broadcom.com,
        florian.fainelli@broadcom.com, shawnguo@kernel.org,
        s.hauer@pengutronix.de, kernel@pengutronix.de, festevam@gmail.com,
        linux-imx@nxp.com, agross@kernel.org, andersson@kernel.org,
        konrad.dybcio@linaro.org, thara.gopinath@gmail.com,
        heiko@sntech.de, mcoquelin.stm32@gmail.com,
        alexandre.torgue@foss.st.com, thierry.reding@gmail.com,
        jonathanh@nvidia.com, matthias.bgg@gmail.com,
        angelogioacchino.delregno@collabora.com,
        srinivas.pandruvada@linux.intel.com,
        DLG-Adam.Ward.opensource@dm.renesas.com, shangxiaojing@huawei.com,
        bchihi@baylibre.com, wenst@chromium.org,
        hayashi.kunihiko@socionext.com,
        niklas.soderlund+renesas@ragnatech.se, chi.minghao@zte.com.cn,
        johan+linaro@kernel.org, jernej.skrabec@gmail.com,
        linux-pm@vger.kernel.org, linux-arm-msm@vger.kernel.org,
        linux-kernel@vger.kernel.org, linux-rockchip@lists.infradead.org,
        linux-mediatek@lists.infradead.org, linux-tegra@vger.kernel.org,
        linux-stm32@st-md-mailman.stormreply.com,
        linux-arm-kernel@lists.infradead.org
References: <20230627101215.58798-1-frank.li@vivo.com>
 <20230627110025.vgtplc6nluiiuvoh@pengutronix.de> <87h6qpyzkd.ffs@tglx>
From:   Yangtao Li <frank.li@vivo.com>
In-Reply-To: <87h6qpyzkd.ffs@tglx>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?dHJlZ3VDTGJWMlozNkFoRlgxTSt6S2pTS0J6d2wyQU9DVEJVVEJhUStDSU9m?=
 =?utf-8?B?K3hGdXRYeFRocWFwTzdNeUx4V1Z4emZKdGdnYS9iWmpRQlFPeGNVYmlGWHRw?=
 =?utf-8?B?ZUptbU9lVXhha0lDZ1k4WVdJUW95MEV1MTA5SzFYVVhWMERMbXhRM0dWdmVR?=
 =?utf-8?B?Tnl4OWNzMm1qS2JISVc4WGtkWWFiQ2krNUlTNjd4Rkl5MW9mL1VDNXZhckY5?=
 =?utf-8?B?M2htazFsY0pDVjlYei9kVjlxTDRJcllhS3VWbGQxWnUvWTB4VFZXNk9oVFNQ?=
 =?utf-8?B?a21vMU5yL1U1ZDFzRTNMdVgyOUphNXhrbjBoMEVnR1Z2d2R0clh3R0FtUHVp?=
 =?utf-8?B?SUk4WXFrTzhFVTI3WkNNSXE5ekgzL09nK3EwZEpOTWpKc1BaWmxHSXNqY044?=
 =?utf-8?B?Zm9ISHR4UWo1SHBlRjk3by9jUTc3TG9VK3B3RmtpQ1ZVU2JubTFvN1ZWRzhj?=
 =?utf-8?B?WXlILzV2ODEwcFB6NHRVUGEyQWM1MzRXbUREM2hEK25XdjZtU1Fmb2E1T1FY?=
 =?utf-8?B?cm1rMlhPbVF3UWR3UG5qajQrMVFKSzkyVXlNcVBiMjFNL3YxeUxidCtVbzhZ?=
 =?utf-8?B?UmlWK21EUE4ySkM0US9sOGc1SGl5ZnpVVCs3bjJneGhvL1NTWVJPczFsdmhw?=
 =?utf-8?B?bjNzamF3dUdVWVloQm4zMi9adWhaSC9iOWp0RmFhbEYrYitxNDRWRnE5U092?=
 =?utf-8?B?em0vemd1R3RWOEFSSUREcDZ0RmoxRlVNeDVXRi8vazBPZ29nbnI4bFg5a0ta?=
 =?utf-8?B?MW1XN0RTT1RnditDb3V3YjRsQnJlUlR6VUU1N3ZjWTBqN0RFeE5nNWU4eHg4?=
 =?utf-8?B?L09lU3Rlclp4RmE4djljNkRDZGdrekFUdXZvS1FHWDMwZm1FMlB1bVhMWW1I?=
 =?utf-8?B?Zmw5NlNDcE52T2piaUhDcmN1WE9CZS9oRUdHTitLNEpKSVN6cVMzUHdjUnl4?=
 =?utf-8?B?SldzaklaUFFyUXVzSE1Bb0VCbGdxdnFSN3VlY3NIQ0FNdGx3ZjkybW9sdytX?=
 =?utf-8?B?M293VEFhTURpWmhuUTl0QzJTYlp6aytqaVZBekcwc0lSZ0UzbWhkU1ZzQkJT?=
 =?utf-8?B?UkJSRmVSa0dVUDBEd1ZRVzFCc3JYbzRjWDk0akZmZlVqOUN0a09TeTc4bzhI?=
 =?utf-8?B?U0dtMXR3ZzVnaWsyY1I5dXRnUTU2YjRQSnJkMFNJWnFvNnRscE1kSXhDdlhy?=
 =?utf-8?B?bXVEWDZrM3BLajlpTU85dUtZVy9EMkpsSjM3VjU4NmZObGF6N2Zrdm5HMHRQ?=
 =?utf-8?B?aGNHY1pXR2xuM1Nsalh0b3NXNmYwRmttRXNsRnN4empMV0dWNnhhR0c3YWJO?=
 =?utf-8?B?VUl3Z0RhZVNtVmJZU2tIalRlTnU0ZXliQTVqK0huc3ZzY21RV2loNjVQS1FR?=
 =?utf-8?B?WE9GOG9EZFJ2bEJGYzYyakN6S01NcU5uWDJLSVQ0UHFVUWdsbEhyM2EvV2Zk?=
 =?utf-8?B?UjBFWXRralJxbFJwM29KL2hLUlpxWkdzbnB5N1E4SlBua2I1TnlMMFRsRHNs?=
 =?utf-8?B?S0d3WlphKzY0WDdZNWxDM1Y1NnUvc2VQU2tySHQvSFRUZ1lxWW9MSzMxWHNO?=
 =?utf-8?B?TVdEbzhvSG0yRlp3TU1Ia2hNNnZZUUxFUGNrT3BZWWFhVVNWbG9CaVJCSzlD?=
 =?utf-8?B?eGRhZGZiNXdaRUN2d3IwRlF5ZkZBWjZqcEJOdDVSbWpIbmp6SGxZZFRvMFZw?=
 =?utf-8?B?ZHc1dDBYZWc5MEpLNlU3a01SbnQ4d212Q0FSL0w1cjMyNUtOcGdCRGcyVXdX?=
 =?utf-8?B?T21CcDlxaWtKR1NkN2s0Y0FjcFFmUzR4WHVFYU9iNHNpenJ1R2ZDczUwc1p2?=
 =?utf-8?B?aVlhcGdkUkpScHlQd0drY2FSNWZyelVsYk52aVZ2OUVPWk1kcjBvekFHcnZs?=
 =?utf-8?B?ZmZ5TXlnNHJXR004WTVoU3lodjN5UFcwQkNLWW5INEFTT01DTDhZRE9QK1Bi?=
 =?utf-8?B?UTUrdzBiTXZ4SnJNdlg4cUNUOCtFQm15d0lDVDNrWDZ6amdWejFmWUZ3RTRW?=
 =?utf-8?B?a24vaGpraFovK3ZYOUNsZjc1aE8wZExJcmpMaTRRREJzZzZ0OHhoMXRBVWhL?=
 =?utf-8?B?SXlBdFdlVzdlTUZ4bGR6aldhY0liQTRmWWd4MDJ2TTFMTU9VeWM3SkJrU1V2?=
 =?utf-8?Q?tcer0ixeYUQPoh739LvZi58+g?=
X-OriginatorOrg: vivo.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 12bc9ed4-346a-4e04-1fe5-08db7ba5c992
X-MS-Exchange-CrossTenant-AuthSource: SEZPR06MB5269.apcprd06.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 03 Jul 2023 09:13:38.6811
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 923e42dc-48d5-4cbe-b582-1a797a6412ed
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: 45HHPW41jyFfU6ygCABPNLCVpuXp5YnPFEFqyreTnASH2xNOJ9mJy5fIzpwYHJuIiVU7Rtje5G9JegX6uRNFyA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SEYPR06MB6507
X-Spam-Status: No, score=-2.2 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,RCVD_IN_DNSWL_NONE,
        RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,SPF_PASS,T_SCC_BODY_TEXT_LINE
        autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Hi,

On 2023/6/30 19:11, Thomas Gleixner wrote:
> On Tue, Jun 27 2023 at 13:00, Uwe Kleine-König wrote:
>> On Tue, Jun 27, 2023 at 06:12:01PM +0800, Yangtao Li wrote:
>>
>> While I assume changing to dev_err_probe is a result of my concern that
>> no error should be printed when rc=-EPROBEDEFER, my other concern that
>> adding an error message to a generic allocation function is a bad idea
>> still stands.
> I agree in general, but if you 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.
>
> Yangtao: The way how this is attempted is not useful at all.
>
>      1) The changelog is word salad and provides 0 rationale
>
>         Also such series require a cover letter...
>
>      2) The dev_err() which is added is not informative at all and cannot
>         replace actually useful error messages. It's not that hard to
>         make it useful.
>
>      2) Adding the printks unconditionally first will emit two messages
>         with different content.
>
>         This is not how such changes are done.
>
>         The proper approach is to create a wrapper function which emits
>         the error message:
>
>         wrapper(....., const char *info)
>         {
>              ret = devm_request_threaded_irq(....);
>              if (ret < 0) {
>                 dev_err(dev, "Failed to request %sinterrupt %u %s %s: %d\n,
>                         thread_fn ? "threaded " : "",
>                         irq, devname, info ? : "", ret);
>              }
>              return ret;
>         }
>
>         Then convert the callsites over one by one with proper
>         changelogs and justification.
>
>         See?


Yes, thanks a lot for the suggestion, and v3 has been sent.


MBR,

Yangtao


>
> Thanks,
>
>          tglx