linux-kernel - Re: [PATCH 1/1] lib: devres: Add error information printing for __devm_ioremap

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: <d8d98365-98f6-d0a4-233e-279172004f51@huawei.com>
Date:   Mon, 7 Jun 2021 15:10:58 +0800
From:   "Leizhen (ThunderTown)" <thunder.leizhen@...wei.com>
To:     Geert Uytterhoeven <geert@...ux-m68k.org>
CC:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        Vladimir Oltean <vladimir.oltean@....com>
Subject: Re: [PATCH 1/1] lib: devres: Add error information printing for
 __devm_ioremap_resource()



On 2021/5/17 15:04, Geert Uytterhoeven wrote:
> Hi Zhen,
> 
> On Wed, Apr 28, 2021 at 8:33 AM Zhen Lei <thunder.leizhen@...wei.com> wrote:
>> 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.
>>
>> Fixes: 35bd8c07db2c ("devres: keep both device name and resource name in pretty name")
>> Signed-off-by: Zhen Lei <thunder.leizhen@...wei.com>
> 
> Thanks for your patch, which is now commit 5c3e241f5246445d ("lib:
> devres: Add error information printing for __devm_ioremap_resource()")
> in driver-core-next.
> 
>> --- a/lib/devres.c
>> +++ b/lib/devres.c
>> @@ -157,8 +157,10 @@ __devm_ioremap_resource(struct device *dev, const struct resource *res,
>>                                              dev_name(dev), res->name);
>>         else
>>                 pretty_name = devm_kstrdup(dev, dev_name(dev), GFP_KERNEL);
>> -       if (!pretty_name)
>> +       if (!pretty_name) {
>> +               dev_err(dev, "can't generate pretty name for resource %pR\n", res);
> 
> If the above fails, it is due to a memory allocation failure, and the
> memory allocation core will have already printed an error message.
> So there is no need to print a second message.
> Hence IMHO this commit should be reverted.

If the memory allocation fails, only the 'size' is printed, but the start address of
the 'res' is not printed. So the printing here is not repeated. The start address is
the more critical help information.

> 
>>                 return IOMEM_ERR_PTR(-ENOMEM);
>> +       }
>>
>>         if (!devm_request_mem_region(dev, res->start, size, pretty_name)) {
>>                 dev_err(dev, "can't request region for resource %pR\n", res);
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
>