| 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
| ||
|
Date: Tue, 31 Oct 2017 18:53:15 -0700
From: Guenter Roeck <linux@...ck-us.net>
To: Joel Stanley <joel@....id.au>, Rob Herring <robh+dt@...nel.org>
Cc: Philipp Zabel <philipp.zabel@...il.com>,
Mykola Kostenok <c_mykolak@...lanox.com>,
Jaghathiswari Rankappagounder Natarajan <jaghu@...gle.com>,
devicetree@...r.kernel.org, linux-hwmon@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH] hwmon: (aspeed-pwm-tacho) Deassert reset in probe
On 10/31/2017 06:34 PM, Joel Stanley wrote:
> The ASPEED SoC must deassert a reset in order to use the PWM/tach
> peripheral.
>
> The device tree bindings are updated to document the resets phandle, and
> the example is updated to match what is expected for both the reset and
> clock phandle. Note that the bindings should have always had the reset
> controller, as the hardware is unusable without it.
>
> Signed-off-by: Joel Stanley <joel@....id.au>
Presumably the driver is being used. This change makes it incompatible with
existing users. This is unacceptable; after all, it is possible that the
device is taken out of reset by ROMMON or BIOS.
On top of that, the reset controller code is quite strict and issues a
backtrace if CONFIG_RESET_CONTROLLER is not enabled. Yet, there is no
dependency added on RESET_CONTROLLER. You might want to consider making
the new control optional and using devm_reset_control_get_optional_exclusive().
The DT change should be a separate patch.
More comments below.
> ---
> .../devicetree/bindings/hwmon/aspeed-pwm-tacho.txt | 14 ++++-------
> drivers/hwmon/aspeed-pwm-tacho.c | 27 +++++++++++++++++++---
> 2 files changed, 29 insertions(+), 12 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/hwmon/aspeed-pwm-tacho.txt b/Documentation/devicetree/bindings/hwmon/aspeed-pwm-tacho.txt
> index 367c8203213b..3ac02988a1a5 100644
> --- a/Documentation/devicetree/bindings/hwmon/aspeed-pwm-tacho.txt
> +++ b/Documentation/devicetree/bindings/hwmon/aspeed-pwm-tacho.txt
> @@ -22,8 +22,9 @@ Required properties for pwm-tacho node:
> - compatible : should be "aspeed,ast2400-pwm-tacho" for AST2400 and
> "aspeed,ast2500-pwm-tacho" for AST2500.
>
> -- clocks : a fixed clock providing input clock frequency(PWM
> - and Fan Tach clock)
> +- clocks : phandle to clock provider with the clock number in the second cell
> +
> +- resets : phandle to reset controller with the reset number in the second cell
>
> fan subnode format:
> ===================
> @@ -48,19 +49,14 @@ Required properties for each child node:
>
> Examples:
>
> -pwm_tacho_fixed_clk: fixedclk {
> - compatible = "fixed-clock";
> - #clock-cells = <0>;
> - clock-frequency = <24000000>;
> -};
> -
> pwm_tacho: pwmtachocontroller@...86000 {
> #address-cells = <1>;
> #size-cells = <1>;
> #cooling-cells = <2>;
> reg = <0x1E786000 0x1000>;
> compatible = "aspeed,ast2500-pwm-tacho";
> - clocks = <&pwm_tacho_fixed_clk>;
> + clocks = <&syscon ASPEED_CLK_APB>;
> + resets = <&syscon ASPEED_RESET_PWM>;
> pinctrl-names = "default";
> pinctrl-0 = <&pinctrl_pwm0_default &pinctrl_pwm1_default>;
>
> diff --git a/drivers/hwmon/aspeed-pwm-tacho.c b/drivers/hwmon/aspeed-pwm-tacho.c
> index f914e5f41048..346a4c5952a3 100644
> --- a/drivers/hwmon/aspeed-pwm-tacho.c
> +++ b/drivers/hwmon/aspeed-pwm-tacho.c
> @@ -7,19 +7,20 @@
> */
>
> #include <linux/clk.h>
> +#include <linux/delay.h>
> #include <linux/errno.h>
> #include <linux/gpio/consumer.h>
> -#include <linux/delay.h>
Unrelated change.
> #include <linux/hwmon.h>
> #include <linux/hwmon-sysfs.h>
> #include <linux/io.h>
> #include <linux/kernel.h>
> #include <linux/module.h>
> -#include <linux/of_platform.h>
> #include <linux/of_device.h>
> +#include <linux/of_platform.h>
Same here. I don't mind the include file reordering, but as a separate patch, please.
> #include <linux/platform_device.h>
> -#include <linux/sysfs.h>
> #include <linux/regmap.h>
> +#include <linux/reset.h>
> +#include <linux/sysfs.h>
> #include <linux/thermal.h>
>
> /* ASPEED PWM & FAN Tach Register Definition */
> @@ -181,6 +182,7 @@ struct aspeed_cooling_device {
>
> struct aspeed_pwm_tacho_data {
> struct regmap *regmap;
> + struct reset_control *rst;
> unsigned long clk_freq;
> bool pwm_present[8];
> bool fan_tach_present[16];
> @@ -931,6 +933,15 @@ static int aspeed_pwm_tacho_probe(struct platform_device *pdev)
> &aspeed_pwm_tacho_regmap_config);
> if (IS_ERR(priv->regmap))
> return PTR_ERR(priv->regmap);
> +
> + priv->rst = devm_reset_control_get_exclusive(dev, NULL);
> + if (IS_ERR(priv->rst)) {
> + dev_err(dev,
> + "missing or invalid reset controller device tree entry");
> + return PTR_ERR(priv->rst);
> + }
> + reset_control_deassert(priv->rst);
> +
> regmap_write(priv->regmap, ASPEED_PTCR_TACH_SOURCE, 0);
> regmap_write(priv->regmap, ASPEED_PTCR_TACH_SOURCE_EXT, 0);
>
> @@ -960,6 +971,15 @@ static int aspeed_pwm_tacho_probe(struct platform_device *pdev)
> return PTR_ERR_OR_ZERO(hwmon);
> }
>
> +static int aspeed_pwm_tacho_remove(struct platform_device *pdev)
> +{
> + struct aspeed_pwm_tacho_data *priv = platform_get_drvdata(pdev);
> +
> + reset_control_deassert(priv->rst);
This seems to be quite pointless. Also, did you test this code ?
> +
> + return 0;
> +}
> +
> static const struct of_device_id of_pwm_tacho_match_table[] = {
> { .compatible = "aspeed,ast2400-pwm-tacho", },
> { .compatible = "aspeed,ast2500-pwm-tacho", },
> @@ -969,6 +989,7 @@ MODULE_DEVICE_TABLE(of, of_pwm_tacho_match_table);
>
> static struct platform_driver aspeed_pwm_tacho_driver = {
> .probe = aspeed_pwm_tacho_probe,
> + .probe = aspeed_pwm_tacho_remove,
> .driver = {
> .name = "aspeed_pwm_tacho",
> .of_match_table = of_pwm_tacho_match_table,
>
Powered by blists - more mailing lists