linux-kernel - Re: [PATCH 2/2] can: xilinx_can: Add support for controller 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] [day] [month] [year] [list]
Message-ID: <20230712-recliner-subtly-196dece73001-mkl@pengutronix.de>
Date:   Wed, 12 Jul 2023 11:03:32 +0200
From:   Marc Kleine-Budde <mkl@...gutronix.de>
To:     Michal Simek <michal.simek@....com>
Cc:     linux-kernel@...r.kernel.org, monstr@...str.eu,
        michal.simek@...inx.com, git@...inx.com,
        Srinivas Neeli <srinivas.neeli@....com>,
        Appana Durga Kedareswara rao <appana.durga.rao@...inx.com>,
        "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Naga Sureshkumar Relli <naga.sureshkumar.relli@...inx.com>,
        Paolo Abeni <pabeni@...hat.com>,
        Philipp Zabel <p.zabel@...gutronix.de>,
        Wolfgang Grandegger <wg@...ndegger.com>,
        linux-arm-kernel@...ts.infradead.org, linux-can@...r.kernel.org,
        netdev@...r.kernel.org
Subject: Re: [PATCH 2/2] can: xilinx_can: Add support for controller reset

On 11.07.2023 16:03:55, Michal Simek wrote:
> From: Srinivas Neeli <srinivas.neeli@....com>
> 
> Add support for an optional reset for the CAN controller using the reset
> driver. If the CAN node contains the "resets" property, then this logic
> will perform CAN controller reset.
> 
> Signed-off-by: Srinivas Neeli <srinivas.neeli@....com>
> Signed-off-by: Michal Simek <michal.simek@....com>
> ---
> 
>  drivers/net/can/xilinx_can.c | 25 +++++++++++++++++++++----
>  1 file changed, 21 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/can/xilinx_can.c b/drivers/net/can/xilinx_can.c
> index 4d3283db3a13..929e00061898 100644
> --- a/drivers/net/can/xilinx_can.c
> +++ b/drivers/net/can/xilinx_can.c
> @@ -30,6 +30,7 @@
>  #include <linux/can/error.h>
>  #include <linux/phy/phy.h>
>  #include <linux/pm_runtime.h>
> +#include <linux/reset.h>
>  
>  #define DRIVER_NAME	"xilinx_can"
>  
> @@ -200,6 +201,7 @@ struct xcan_devtype_data {
>   * @can_clk:			Pointer to struct clk
>   * @devtype:			Device type specific constants
>   * @transceiver:		Optional pointer to associated CAN transceiver
> + * @rstc:			Pointer to reset control
>   */
>  struct xcan_priv {
>  	struct can_priv can;
> @@ -218,6 +220,7 @@ struct xcan_priv {
>  	struct clk *can_clk;
>  	struct xcan_devtype_data devtype;
>  	struct phy *transceiver;
> +	struct reset_control *rstc;
>  };
>  
>  /* CAN Bittiming constants as per Xilinx CAN specs */
> @@ -1799,6 +1802,16 @@ static int xcan_probe(struct platform_device *pdev)
>  	priv->can.do_get_berr_counter = xcan_get_berr_counter;
>  	priv->can.ctrlmode_supported = CAN_CTRLMODE_LOOPBACK |
>  					CAN_CTRLMODE_BERR_REPORTING;
> +	priv->rstc = devm_reset_control_get_optional_exclusive(&pdev->dev, NULL);
> +	if (IS_ERR(priv->rstc)) {
> +		dev_err(&pdev->dev, "Cannot get CAN reset.\n");
> +		ret = PTR_ERR(priv->rstc);
> +		goto err_free;
> +	}
> +
> +	ret = reset_control_reset(priv->rstc);
> +	if (ret)
> +		goto err_free;
>  
>  	if (devtype->cantype == XAXI_CANFD) {
>  		priv->can.data_bittiming_const =
> @@ -1827,7 +1840,7 @@ static int xcan_probe(struct platform_device *pdev)
>  	/* Get IRQ for the device */
>  	ret = platform_get_irq(pdev, 0);
>  	if (ret < 0)
> -		goto err_free;
> +		goto err_reset;
>  
>  	ndev->irq = ret;
>  
> @@ -1843,21 +1856,21 @@ static int xcan_probe(struct platform_device *pdev)
>  	if (IS_ERR(priv->can_clk)) {
>  		ret = dev_err_probe(&pdev->dev, PTR_ERR(priv->can_clk),
>  				    "device clock not found\n");
> -		goto err_free;
> +		goto err_reset;
>  	}
>  
>  	priv->bus_clk = devm_clk_get(&pdev->dev, devtype->bus_clk_name);
>  	if (IS_ERR(priv->bus_clk)) {
>  		ret = dev_err_probe(&pdev->dev, PTR_ERR(priv->bus_clk),
>  				    "bus clock not found\n");
> -		goto err_free;
> +		goto err_reset;
>  	}
>  
>  	transceiver = devm_phy_optional_get(&pdev->dev, NULL);
>  	if (IS_ERR(transceiver)) {
>  		ret = PTR_ERR(transceiver);
>  		dev_err_probe(&pdev->dev, ret, "failed to get phy\n");
> -		goto err_free;
> +		goto err_reset;
>  	}
>  	priv->transceiver = transceiver;
>  
> @@ -1904,6 +1917,8 @@ static int xcan_probe(struct platform_device *pdev)
>  err_disableclks:
>  	pm_runtime_put(priv->dev);
>  	pm_runtime_disable(&pdev->dev);
> +err_reset:
> +	reset_control_assert(priv->rstc);
>  err_free:
>  	free_candev(ndev);
>  err:
> @@ -1920,10 +1935,12 @@ static int xcan_probe(struct platform_device *pdev)
>  static void xcan_remove(struct platform_device *pdev)
>  {
>  	struct net_device *ndev = platform_get_drvdata(pdev);
> +	struct xcan_priv *priv = netdev_priv(ndev);
>  
>  	unregister_candev(ndev);
>  	pm_runtime_disable(&pdev->dev);
>  	free_candev(ndev);
> +	reset_control_assert(priv->rstc);

Nitpick: Can you make this symmetric with respect to the error handling
in xcan_probe()?

Oh - that's not a cosmetic issue, it's a use-after-free. free_candev()
frees your priv.

>  }
>  
>  static struct platform_driver xcan_driver = {
> -- 
> 2.36.1
> 
>

Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)