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: <1469090461.3435.15.camel@pengutronix.de>
Date:	Thu, 21 Jul 2016 10:41:01 +0200
From:	Philipp Zabel <p.zabel@...gutronix.de>
To:	Masahiro Yamada <yamada.masahiro@...ionext.com>
Cc:	linux-kernel@...r.kernel.org, Hans de Goede <hdegoede@...hat.com>,
	Lee Jones <lee.jones@...aro.org>
Subject: Re: [PATCH] reset: generate reset_control_get variants with macro
 expansion

Hi Masahiro,

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>

Thank you for the patch, but while I'm all for reducing unnecessary
duplication, I do not like this change for two reasons:
First, the macro generated API functions can not be found by name using
tools like ctags or grep anymore.
And secondly, we would now have detailed kerneldoc comments for two
functions that are never called directly, but the public facing API
itself is completely without documentation. I wouldn't mind removing
duplicated documentation paragraphs though, for example by referencing
reset_control_get_* from the of_reset_control_get_* kerneldoc comments.

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.

regards
Philipp