netdev - Re: [PATCH v2 01/16] reset: add non CONFIG_RESET

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]

Date:	Fri, 10 Jan 2014 14:30:37 +0100
From:	Philipp Zabel <p.zabel@...gutronix.de>
To:	Chen-Yu Tsai <wens@...e.org>
Cc:	Srinivas Kandagatla <srinivas.kandagatla@...com>,
	Giuseppe Cavallaro <peppe.cavallaro@...com>,
	Maxime Ripard <maxime.ripard@...e-electrons.com>,
	netdev@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
	linux-sunxi@...glegroups.com, linux-kernel@...r.kernel.org,
	Rob Herring <rob.herring@...xeda.com>,
	Emilio Lopez <emilio@...pez.com.ar>,
	Mike Turquette <mturquette@...aro.org>,
	"Ivan T. Ivanov" <iivanov@...sol.com>,
	Barry Song <Baohua.Song@....com>,
	Stephen Warren <swarren@...dotorg.org>
Subject: Re: [PATCH v2 01/16] reset: add non CONFIG_RESET_CONTROLLER routines

Hi,

[Added Ivan, Stephen and Barry to Cc:]

Am Freitag, den 10.01.2014, 15:00 +0800 schrieb Chen-Yu Tsai:
> Some drivers are shared between platforms that may or may not
> have RESET_CONTROLLER selected for them.

I expected that drivers compiled for platforms without reset controllers
but use the reset API should select or depend on RESET_CONTROLLER.
Stubbing out device_reset and reset_control_get will turn a compile time
error into a runtime error for everyone forgetting to do this when
writing a new driver that needs the reset.

>  Add dummy functions
> when RESET_CONTROLLER is not selected, thereby eliminating the
> need for drivers to enclose reset_control_*() calls within
> #ifdef CONFIG_RESET_CONTROLLER, #endif
> 
> Signed-off-by: Chen-Yu Tsai <wens@...e.org>

This was already proposed by Ivan and Barry earlier, and so far we
didn't get to a proper conclusion:

https://lkml.org/lkml/2013/10/10/179
http://lists.infradead.org/pipermail/linux-arm-kernel/2013-June/174758.html

If included, the stubs should at least return an error to indicate a
reset couldn't be performed, but then I lose the compile time error for
drivers which should select RESET_CONTROLLER but don't.

Also this alone won't help you if you build multi-arch kernels where one
platform enables RESET_CONTROLLER and the other expects it to be
disabled.

regards
Philipp

> ---
>  include/linux/reset.h | 39 +++++++++++++++++++++++++++++++++++++++
>  1 file changed, 39 insertions(+)
> 
> diff --git a/include/linux/reset.h b/include/linux/reset.h
> index 6082247..38aa616 100644
> --- a/include/linux/reset.h
> +++ b/include/linux/reset.h
> @@ -4,6 +4,8 @@
>  struct device;
>  struct reset_control;
>  
> +#ifdef CONFIG_RESET_CONTROLLER
> +
>  int reset_control_reset(struct reset_control *rstc);
>  int reset_control_assert(struct reset_control *rstc);
>  int reset_control_deassert(struct reset_control *rstc);
> @@ -14,4 +16,41 @@ struct reset_control *devm_reset_control_get(struct device *dev, const char *id)
>  
>  int device_reset(struct device *dev);
>  
> +#else /* !CONFIG_RESET_CONTROLLER */
> +
> +static inline int reset_control_reset(struct reset_control *rstc)
> +{
> +	return 0;
> +}
> +
> +static inline int reset_control_assert(struct reset_control *rstc)
> +{
> +	return 0;
> +}
> +
> +static inline int reset_control_deassert(struct reset_control *rstc)
> +{
> +	return 0;
> +}

Those should probably have a WARN_ON(1) like the GPIO API stubs.

> +
> +static inline struct reset_control *reset_control_get(struct device *dev,
> +		const char *id)
> +{
> +	return NULL;
> +}
[...]
> +static inline struct reset_control *devm_reset_control_get(struct device *dev,
> +		const char *id)
> +{
> +	return NULL;
> +}

These should return ERR_PTR(-ENOSYS).

> +
> +static inline int device_reset(struct device *dev)
> +{
> +	return 0;
> +}

And this should return -ENOSYS.

Drivers that also need to run on platforms with CONFIG_RESET_CONTROLLER
disabled (and that don't need the reset) should ignore -ENOSYS and
-ENOENT return values from device_reset/(devm_)reset_control_get.

I wonder if it might be a good idea to add a RESET_CONTROLLER_OPTIONAL
that drivers need to select to enable the API stubs? That way we could
keep the compile time error for new drivers that need resets but forget
to select RESET_CONTROLLER.
Or add a
#warning If this driver can work without reset, please select CONFIG_RESET_CONTROLLER_OPTIONAL

Another possibility would be to add device_reset_optional and
(devm_)reset_control_get_optional variants and only provide stubs for
those, but not for device_reset/(devm_)reset_control_get. Then drivers
that need to work on platforms without the reset controller API enabled
could explicitly use the _optional variants, and all other drivers would
still be checked at compile time.

regards
Philipp

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html