linux-kernel - Re: [PATCH] reset: generate reset_control

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: <20160721120600.GC14925@dell>
Date:	Thu, 21 Jul 2016 13:06:00 +0100
From:	Lee Jones <lee.jones@...aro.org>
To:	Masahiro Yamada <yamada.masahiro@...ionext.com>
Cc:	Philipp Zabel <p.zabel@...gutronix.de>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	Hans de Goede <hdegoede@...hat.com>
Subject: Re: [PATCH] reset: generate reset_control_get variants with macro
 expansion

On Thu, 21 Jul 2016, Masahiro Yamada wrote:
> 2016-07-21 17:41 GMT+09:00 Philipp Zabel <p.zabel@...gutronix.de>:
> > Am Donnerstag, den 21.07.2016, 14:05 +0900 schrieb Masahiro Yamada:
> >> The recent update in the reset subsystem requires all reset consumers
> >> to be explicit about the requested reset lines; _explicit or _shared.
> >> This effectively doubled the number of reset_control_get variants.
> >> Also, we already had _optional variants.
> >>
> >> We see some pattern in the reset_control_get APIs.
> >>
> >> There are 6 base functions in terms of function prototype:
> >>   reset_control_get
> >>   reset_control_get_by_index
> >>   of_reset_control_get
> >>   of_reset_control_get_by_index
> >>   devm_reset_control_get
> >>   devm_reset_control_get_by_index
> >>
> >> Each of them has 4 variants with the following suffixes:
> >>   _exclusive
> >>   _shared
> >>   _optional_exclusive
> >>   _optional_shared
> >>
> >> The exhaustive set of functions (6 * 4) can be generated with macro
> >> expansion.  It will mitigate the pain of maintaining proliferating
> >> APIs.
> >>
> >> I merged similar comment blocks into two, for functions in core.c
> >> because copy-paste work for similar comment blocks is error-prone.
> >>
> >> Signed-off-by: Masahiro Yamada <yamada.masahiro@...ionext.com>

[...]

I did entertain several ways of reducing this API myself, so I know
where you are coming from but ...

> > I think when Lee suggested the API change, I initially leaned towards a
> > single entry point with flags, similar to the irq request API:
> >   reset_control_get(..., RSTF_EXCLUSIVE)
> >   reset_control_get(..., RSTF_SHARED)
> >   reset_control_get(..., RSTF_OPTIONAL | RSTF_EXCLUSIVE)
> >   reset_control_get(..., RSTF_OPTIONAL | RSTF_SHARED)
> > On the other hand, with that API users could forget the flags or use
> > incompatible combinations, which is impossible with the current API.

This is the age-old 'lines-of-code over {grep,read,maintain}ability'
argument, and I'm afraid to say that this patch, although saves some
lines, the {grep,read,maintain}ability takes too much of a big hit
IMHO.

As a developer, I find it very annoying when I can't directly grep for
functions that have been written in a 'clever' (read 'obfuscated')
way.  And since this is a) a header file and b) all of the verboseness
is in a single/central place, it's better left as it is.

I'd entertain the FLAGS idea, since this is very common in the kernel
and users/developers know their way around those already, but MACRO
driven function names, no way!

> If you are unhappy with this patch,
> I will send an alternative one,
> which adds 4 more functions in a stupid and straightforward way.

Yes, 'simple' and straightforward is what we're looking for and is the
way forward.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog