linux-kernel - Re: [PATCH] serial: 8250

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: <1495613916.3840.16.camel@pengutronix.de>
Date:   Wed, 24 May 2017 10:18:36 +0200
From:   Philipp Zabel <p.zabel@...gutronix.de>
To:     Joel Stanley <joel@....id.au>
Cc:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Rob Herring <robh+dt@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        linux-serial@...r.kernel.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] serial: 8250_of: Add reset support

Hi Joel,

On Wed, 2017-05-24 at 14:53 +1000, Joel Stanley wrote:
> This adds the hooks for an optional reset controller in the 8250 device
> tree node.
> 
> Signed-off-by: Joel Stanley <joel@....id.au>
> ---
>  Documentation/devicetree/bindings/serial/8250.txt |  1 +
>  drivers/tty/serial/8250/8250_of.c                 | 16 ++++++++++++++++
>  2 files changed, 17 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/serial/8250.txt b/Documentation/devicetree/bindings/serial/8250.txt
> index 10276a46ecef..63e32393f82b 100644
> --- a/Documentation/devicetree/bindings/serial/8250.txt
> +++ b/Documentation/devicetree/bindings/serial/8250.txt
> @@ -45,6 +45,7 @@ Optional properties:
>    property.
>  - tx-threshold: Specify the TX FIFO low water indication for parts with
>    programmable TX FIFO thresholds.
> +- resets : phandle + reset specifier pairs
>  
>  Note:
>  * fsl,ns16550:
> diff --git a/drivers/tty/serial/8250/8250_of.c b/drivers/tty/serial/8250/8250_of.c
> index 1cbadafc6889..f34dd23376f4 100644
> --- a/drivers/tty/serial/8250/8250_of.c
> +++ b/drivers/tty/serial/8250/8250_of.c
> @@ -19,11 +19,13 @@
>  #include <linux/of_irq.h>
>  #include <linux/of_platform.h>
>  #include <linux/clk.h>
> +#include <linux/reset.h>
>  
>  #include "8250.h"
>  
>  struct of_serial_info {
>  	struct clk *clk;
> +	struct reset_control *rst;
>  	int type;
>  	int line;
>  };
> @@ -132,6 +134,18 @@ static int of_platform_serial_setup(struct platform_device *ofdev,
>  		}
>  	}
>  
> +	info->rst = devm_reset_control_get_optional(&ofdev->dev, NULL);

Please use devm_reset_control_get_optional_shared instead.

This looks like shared (clock-like) reset use, where you just have to
make sure that the reset is deasserted while the module is in use, but
you don't care whether it is actually asserted all the time otherwise.

> +	if (IS_ERR(info->rst)) {
> +		ret = PTR_ERR(info->rst);
> +		if (ret == -EPROBE_DEFER)
> +			goto out;
> +		info->rst = NULL;

devm_reset_control_get_optional returns NULL if no reset is specified in
the device tree specifically so we don't have to do this in all the
drivers. If an error is returned, just goto out unconditionally, ...

> +	} else {
> +		ret = reset_control_deassert(info->rst);
> +		if (ret)
> +			goto out;
> +	}

... then reset_control_deassert can be called unconditionally, too.

> +
>  	port->type = type;
>  	port->uartclk = clk;
>  	port->flags = UPF_SHARE_IRQ | UPF_BOOT_AUTOCONF | UPF_IOREMAP
> @@ -231,6 +245,8 @@ static int of_platform_serial_remove(struct platform_device *ofdev)
>  
>  	if (info->clk)
>  		clk_disable_unprepare(info->clk);
> +	if (info->rst)

Not necessary, reset_control_assert checks this, too.

> +		reset_control_assert(info->rst);
>  	kfree(info);
>  	return 0;
>  }

regards
Philipp