linux-kernel - Re: [PATCH v3 2/2] drivers: mux: Add Generic regmap bitfield-based multiplexer in mmio-mux

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]

Date:   Tue, 26 Feb 2019 08:20:11 +0000
From:   Peter Rosin <peda@...ntia.se>
To:     Pankaj Bansal <pankaj.bansal@....com>, Leo Li <leoyang.li@....com>
CC:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v3 2/2] drivers: mux: Add Generic regmap bitfield-based
 multiplexer in mmio-mux

On 2019-02-26 07:08, Pankaj Bansal wrote:
>>> +	if (IS_ERR_OR_NULL(regmap)) {
>>> +		ret = PTR_ERR_OR_ZERO(regmap) ? PTR_ERR(regmap) : -
>> ENODEV;
>>
>> The above is not correct, this should be better (untested):
>>
>> 		ret = PTR_ERR(regmap) ?: -ENODEV;
> 
> Omitting the second operand in ternary operator is not standard. https://stackoverflow.com/questions/34559705/ternary-operator-without-the-middle-expression

Irrelevant, a great many non-standard features are used in the kernel.

> Although, it *has been* used in kernel in many places
> https://livegrep.com/search/linux?q=file%3A%5C.c%24%20%5C%3F%5C%3A&fold_case=auto&regex=true&context=true

Yes, a local grep suggests that there are plenty of instances in the
kernel. In fact, there are multiple instances even in the kernel/
directory so it's not some fringe thing. My conclusion is that it
makes for readable code and is perfectly safe to use in the kernel,
which apparently does not care about compilers that do not understand
the construct.

$ git grep '?:' *.[ch] | wc
    773    4830   54975

I you still don't buy that and don't want to sign-off on it, then
write

		ret = IS_ERR(regmap) ? PTR_ERR(regmap) : -ENODEV;

But that's just wordy and less readable, methinks.

However, the best thing to do, according to your maintainer, is still
the original suggestion to fixup the NULL as early as possible with

		regmap = dev_get_regmap(dev->parent, NULL) ?: ERR_PTR(-ENODEV);

And leave the error handling block alone. Can you please just do that
and be done with it?

Cheers,
Peter