linux-kernel - Re: [PATCH v1 1/4] bus: hisi_lpc: Don't dereference fwnode handle

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: <08a2a92d-caac-2472-05e0-e6749bcdd15b@huawei.com>
Date:   Tue, 5 Jul 2022 16:38:17 +0100
From:   John Garry <john.garry@...wei.com>
To:     Andy Shevchenko <andy.shevchenko@...il.com>
CC:     Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        "Rafael J. Wysocki" <rafael.j.wysocki@...el.com>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v1 1/4] bus: hisi_lpc: Don't dereference fwnode handle

On 05/07/2022 16:23, Andy Shevchenko wrote:
> On Tue, Jul 5, 2022 at 5:11 PM John Garry<john.garry@...wei.com>  wrote:
>> On 05/07/2022 12:43, Andy Shevchenko wrote:
>>> Use dev_fwnode() and acpi_fwnode_handle() instead of dereferencing
>>> an fwnode handle directly.
>> ...which is a better coding practice, right? If so, it would be nice to
>> mention it - well at least I think so.
> Not only. In the case of fwnode it's a long story behind its corner
> case(s) where in the future we might switch from embedded structure to
> linked list, for example, in order to address those corner cases.
> Should I write a paragraph for that as well?
> 

If you just say that it's a better coding practice to use available APIs 
to access structure members rather than access them directly, then that 
is good enough. Or maybe you can think of something better along the 
lines of what you wrote above. My issue is that there was no reason. So 
I'll leave it to you.

> ...
> 
>> Apart from above and nit, below:
> See below my answer.
> 
>> Acked-by: John Garry<john.garry@...wei.com>
> Thanks.
> 
> ...
> 
>>> -     sys_port = logic_pio_trans_hwaddr(&host->fwnode, res->start, len);
>>> +     sys_port = logic_pio_trans_hwaddr(acpi_fwnode_handle(host), res->start, len);
>> nit: currently the driver keeps to the old 80 character line limit.
>> While the rules may have been relaxed, I'd rather we still maintain it.
> First of all, even before the 100 characters era the rule had two exceptions:
> 1) the string literals;

Sure

> 2) the readability over strictness of the 80 characters rule.
> 
> While I agree in general with you, in this case I think keeping
> strictness makes readability worse.

ok, fine. I was going to suggest introduce a varible to hold 
acpi_fwnode_handle(host) but then we may not want a variable of 
fwnode_handle* type hanging around. Anyway I don't feel too strongly 
about it.

Thanks,
John