Re: [PATCH 2/2] can: xilinx_can: Add support for controller reset

[Marc Kleine-Budde]

On 11.07.2023 16:03:55, Michal Simek wrote:
> From: Srinivas Neeli <srinivas.neeli@xxxxxxx>
> 
> 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@xxxxxxx>
> Signed-off-by: Michal Simek <michal.simek@xxxxxxx>
> ---
> 
>  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   |

Attachment: signature.asc
Description: PGP signature