diff mbox series

[1/2] ata: ahci_ceva: fix error handling for Xilinx GT PHY support

Message ID 1705604904-471889-2-git-send-email-radhey.shyam.pandey@amd.com
State New
Headers
Series ata: ahci_ceva: fix xilinx GT PHY support |

Commit Message

Pandey, Radhey Shyam Jan. 18, 2024, 7:08 p.m. UTC 
  From: Piyush Mehta <piyush.mehta@amd.com>

Platform clock and phy error resources are not cleaned up in Xilinx GT PHY
error path. To fix introduce error label for ahci_platform_disable_clks and
phy_power_off/exit and call them in error path. No functional change.

Fixes: 9a9d3abe24bb ("ata: ahci: ceva: Update the driver to support xilinx GT phy")
Signed-off-by: Piyush Mehta <piyush.mehta@amd.com>
Signed-off-by: Radhey Shyam Pandey <radhey.shyam.pandey@amd.com>
---
---
 drivers/ata/ahci_ceva.c | 47 +++++++++++++++++++++++++++++------------
 1 file changed, 33 insertions(+), 14 deletions(-)

Comments

Dan Carpenter Jan. 22, 2024, 7:39 a.m. UTC | #1 
Hi Radhey,

kernel test robot noticed the following build warnings:

https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Radhey-Shyam-Pandey/ata-ahci_ceva-fix-error-handling-for-Xilinx-GT-PHY-support/20240119-031129
base:   linus/master
patch link:    https://lore.kernel.org/r/1705604904-471889-2-git-send-email-radhey.shyam.pandey%40amd.com
patch subject: [PATCH 1/2] ata: ahci_ceva: fix error handling for Xilinx GT PHY support
config: i386-randconfig-141-20240120 (https://download.01.org/0day-ci/archive/20240122/202401220603.dgjTZ08O-lkp@intel.com/config)
compiler: ClangBuiltLinux clang version 17.0.6 (https://github.com/llvm/llvm-project 6009708b4367171ccdbf4b5905cb6a803753fe18)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
| Closes: https://lore.kernel.org/r/202401220603.dgjTZ08O-lkp@intel.com/

smatch warnings:
drivers/ata/ahci_ceva.c:335 ceva_ahci_probe() error: uninitialized symbol 'i'.

vim +/i +335 drivers/ata/ahci_ceva.c

a73ed35052ca85 Suneel Garapati      2015-06-09  192  static int ceva_ahci_probe(struct platform_device *pdev)
a73ed35052ca85 Suneel Garapati      2015-06-09  193  {
a73ed35052ca85 Suneel Garapati      2015-06-09  194  	struct device_node *np = pdev->dev.of_node;
a73ed35052ca85 Suneel Garapati      2015-06-09  195  	struct device *dev = &pdev->dev;
a73ed35052ca85 Suneel Garapati      2015-06-09  196  	struct ahci_host_priv *hpriv;
a73ed35052ca85 Suneel Garapati      2015-06-09  197  	struct ceva_ahci_priv *cevapriv;
3bc867de85b5bf Anurag Kumar Vulisha 2017-08-21  198  	enum dev_dma_attr attr;
b1600f5880a13f Piyush Mehta         2024-01-19  199  	int rc, i;

i needs to be initialized to zero here.

a73ed35052ca85 Suneel Garapati      2015-06-09  200  
a73ed35052ca85 Suneel Garapati      2015-06-09  201  	cevapriv = devm_kzalloc(dev, sizeof(*cevapriv), GFP_KERNEL);
a73ed35052ca85 Suneel Garapati      2015-06-09  202  	if (!cevapriv)
a73ed35052ca85 Suneel Garapati      2015-06-09  203  		return -ENOMEM;
a73ed35052ca85 Suneel Garapati      2015-06-09  204  
a73ed35052ca85 Suneel Garapati      2015-06-09  205  	cevapriv->ahci_pdev = pdev;
a73ed35052ca85 Suneel Garapati      2015-06-09  206  
9a9d3abe24bb6b Piyush Mehta         2021-02-08  207  	cevapriv->rst = devm_reset_control_get_optional_exclusive(&pdev->dev,
9a9d3abe24bb6b Piyush Mehta         2021-02-08  208  								  NULL);
fa4b42b2a968dc Piyush Mehta         2021-03-05  209  	if (IS_ERR(cevapriv->rst))
fa4b42b2a968dc Piyush Mehta         2021-03-05  210  		dev_err_probe(&pdev->dev, PTR_ERR(cevapriv->rst),
fa4b42b2a968dc Piyush Mehta         2021-03-05  211  			      "failed to get reset\n");
9a9d3abe24bb6b Piyush Mehta         2021-02-08  212  
16af2d65842d34 Kunihiko Hayashi     2018-08-22  213  	hpriv = ahci_platform_get_resources(pdev, 0);
a73ed35052ca85 Suneel Garapati      2015-06-09  214  	if (IS_ERR(hpriv))
a73ed35052ca85 Suneel Garapati      2015-06-09  215  		return PTR_ERR(hpriv);
a73ed35052ca85 Suneel Garapati      2015-06-09  216  
9a9d3abe24bb6b Piyush Mehta         2021-02-08  217  	if (!cevapriv->rst) {
a73ed35052ca85 Suneel Garapati      2015-06-09  218  		rc = ahci_platform_enable_resources(hpriv);
a73ed35052ca85 Suneel Garapati      2015-06-09  219  		if (rc)
a73ed35052ca85 Suneel Garapati      2015-06-09  220  			return rc;

i is uninitialized on this path.

9a9d3abe24bb6b Piyush Mehta         2021-02-08  221  	} else {
9a9d3abe24bb6b Piyush Mehta         2021-02-08  222  		rc = ahci_platform_enable_clks(hpriv);
9a9d3abe24bb6b Piyush Mehta         2021-02-08  223  		if (rc)
9a9d3abe24bb6b Piyush Mehta         2021-02-08  224  			return rc;
9a9d3abe24bb6b Piyush Mehta         2021-02-08  225  		/* Assert the controller reset */
9a9d3abe24bb6b Piyush Mehta         2021-02-08  226  		reset_control_assert(cevapriv->rst);
9a9d3abe24bb6b Piyush Mehta         2021-02-08  227  
9a9d3abe24bb6b Piyush Mehta         2021-02-08  228  		for (i = 0; i < hpriv->nports; i++) {
9a9d3abe24bb6b Piyush Mehta         2021-02-08  229  			rc = phy_init(hpriv->phys[i]);
b1600f5880a13f Piyush Mehta         2024-01-19  230  			if (rc) {
b1600f5880a13f Piyush Mehta         2024-01-19  231  				while (--i >= 0)
b1600f5880a13f Piyush Mehta         2024-01-19  232  					phy_exit(hpriv->phys[i]);
b1600f5880a13f Piyush Mehta         2024-01-19  233  				goto disable_clks;
b1600f5880a13f Piyush Mehta         2024-01-19  234  			}
9a9d3abe24bb6b Piyush Mehta         2021-02-08  235  		}
9a9d3abe24bb6b Piyush Mehta         2021-02-08  236  
9a9d3abe24bb6b Piyush Mehta         2021-02-08  237  		/* De-assert the controller reset */
9a9d3abe24bb6b Piyush Mehta         2021-02-08  238  		reset_control_deassert(cevapriv->rst);
9a9d3abe24bb6b Piyush Mehta         2021-02-08  239  
9a9d3abe24bb6b Piyush Mehta         2021-02-08  240  		for (i = 0; i < hpriv->nports; i++) {
9a9d3abe24bb6b Piyush Mehta         2021-02-08  241  			rc = phy_power_on(hpriv->phys[i]);
9a9d3abe24bb6b Piyush Mehta         2021-02-08  242  			if (rc) {
9a9d3abe24bb6b Piyush Mehta         2021-02-08  243  				phy_exit(hpriv->phys[i]);
b1600f5880a13f Piyush Mehta         2024-01-19  244  				goto disable_phys;
9a9d3abe24bb6b Piyush Mehta         2021-02-08  245  			}
9a9d3abe24bb6b Piyush Mehta         2021-02-08  246  		}
9a9d3abe24bb6b Piyush Mehta         2021-02-08  247  	}
a73ed35052ca85 Suneel Garapati      2015-06-09  248  
a73ed35052ca85 Suneel Garapati      2015-06-09  249  	if (of_property_read_bool(np, "ceva,broken-gen2"))
a73ed35052ca85 Suneel Garapati      2015-06-09  250  		cevapriv->flags = CEVA_FLAG_BROKEN_GEN2;
a73ed35052ca85 Suneel Garapati      2015-06-09  251  
fe8365bbf8ac58 Anurag Kumar Vulisha 2017-08-21  252  	/* Read OOB timing value for COMINIT from device-tree */
fe8365bbf8ac58 Anurag Kumar Vulisha 2017-08-21  253  	if (of_property_read_u8_array(np, "ceva,p0-cominit-params",
fe8365bbf8ac58 Anurag Kumar Vulisha 2017-08-21  254  					(u8 *)&cevapriv->pp2c[0], 4) < 0) {
fe8365bbf8ac58 Anurag Kumar Vulisha 2017-08-21  255  		dev_warn(dev, "ceva,p0-cominit-params property not defined\n");
b1600f5880a13f Piyush Mehta         2024-01-19  256  		rc = -EINVAL;
b1600f5880a13f Piyush Mehta         2024-01-19  257  		goto disable_phys;
fe8365bbf8ac58 Anurag Kumar Vulisha 2017-08-21  258  	}
fe8365bbf8ac58 Anurag Kumar Vulisha 2017-08-21  259  
fe8365bbf8ac58 Anurag Kumar Vulisha 2017-08-21  260  	if (of_property_read_u8_array(np, "ceva,p1-cominit-params",
fe8365bbf8ac58 Anurag Kumar Vulisha 2017-08-21  261  					(u8 *)&cevapriv->pp2c[1], 4) < 0) {
fe8365bbf8ac58 Anurag Kumar Vulisha 2017-08-21  262  		dev_warn(dev, "ceva,p1-cominit-params property not defined\n");
b1600f5880a13f Piyush Mehta         2024-01-19  263  		rc = -EINVAL;
b1600f5880a13f Piyush Mehta         2024-01-19  264  		goto disable_phys;
fe8365bbf8ac58 Anurag Kumar Vulisha 2017-08-21  265  	}
fe8365bbf8ac58 Anurag Kumar Vulisha 2017-08-21  266  
fe8365bbf8ac58 Anurag Kumar Vulisha 2017-08-21  267  	/* Read OOB timing value for COMWAKE from device-tree*/
fe8365bbf8ac58 Anurag Kumar Vulisha 2017-08-21  268  	if (of_property_read_u8_array(np, "ceva,p0-comwake-params",
fe8365bbf8ac58 Anurag Kumar Vulisha 2017-08-21  269  					(u8 *)&cevapriv->pp3c[0], 4) < 0) {
fe8365bbf8ac58 Anurag Kumar Vulisha 2017-08-21  270  		dev_warn(dev, "ceva,p0-comwake-params property not defined\n");
b1600f5880a13f Piyush Mehta         2024-01-19  271  		rc = -EINVAL;
b1600f5880a13f Piyush Mehta         2024-01-19  272  		goto disable_phys;
fe8365bbf8ac58 Anurag Kumar Vulisha 2017-08-21  273  	}
fe8365bbf8ac58 Anurag Kumar Vulisha 2017-08-21  274  
fe8365bbf8ac58 Anurag Kumar Vulisha 2017-08-21  275  	if (of_property_read_u8_array(np, "ceva,p1-comwake-params",
fe8365bbf8ac58 Anurag Kumar Vulisha 2017-08-21  276  					(u8 *)&cevapriv->pp3c[1], 4) < 0) {
fe8365bbf8ac58 Anurag Kumar Vulisha 2017-08-21  277  		dev_warn(dev, "ceva,p1-comwake-params property not defined\n");
b1600f5880a13f Piyush Mehta         2024-01-19  278  		rc = -EINVAL;
b1600f5880a13f Piyush Mehta         2024-01-19  279  		goto disable_phys;
fe8365bbf8ac58 Anurag Kumar Vulisha 2017-08-21  280  	}
fe8365bbf8ac58 Anurag Kumar Vulisha 2017-08-21  281  
fe8365bbf8ac58 Anurag Kumar Vulisha 2017-08-21  282  	/* Read phy BURST timing value from device-tree */
fe8365bbf8ac58 Anurag Kumar Vulisha 2017-08-21  283  	if (of_property_read_u8_array(np, "ceva,p0-burst-params",
fe8365bbf8ac58 Anurag Kumar Vulisha 2017-08-21  284  					(u8 *)&cevapriv->pp4c[0], 4) < 0) {
fe8365bbf8ac58 Anurag Kumar Vulisha 2017-08-21  285  		dev_warn(dev, "ceva,p0-burst-params property not defined\n");
b1600f5880a13f Piyush Mehta         2024-01-19  286  		rc = -EINVAL;
b1600f5880a13f Piyush Mehta         2024-01-19  287  		goto disable_phys;
fe8365bbf8ac58 Anurag Kumar Vulisha 2017-08-21  288  	}
fe8365bbf8ac58 Anurag Kumar Vulisha 2017-08-21  289  
fe8365bbf8ac58 Anurag Kumar Vulisha 2017-08-21  290  	if (of_property_read_u8_array(np, "ceva,p1-burst-params",
fe8365bbf8ac58 Anurag Kumar Vulisha 2017-08-21  291  					(u8 *)&cevapriv->pp4c[1], 4) < 0) {
fe8365bbf8ac58 Anurag Kumar Vulisha 2017-08-21  292  		dev_warn(dev, "ceva,p1-burst-params property not defined\n");
b1600f5880a13f Piyush Mehta         2024-01-19  293  		rc = -EINVAL;
b1600f5880a13f Piyush Mehta         2024-01-19  294  		goto disable_phys;
fe8365bbf8ac58 Anurag Kumar Vulisha 2017-08-21  295  	}
fe8365bbf8ac58 Anurag Kumar Vulisha 2017-08-21  296  
fe8365bbf8ac58 Anurag Kumar Vulisha 2017-08-21  297  	/* Read phy RETRY interval timing value from device-tree */
fe8365bbf8ac58 Anurag Kumar Vulisha 2017-08-21  298  	if (of_property_read_u16_array(np, "ceva,p0-retry-params",
fe8365bbf8ac58 Anurag Kumar Vulisha 2017-08-21  299  					(u16 *)&cevapriv->pp5c[0], 2) < 0) {
fe8365bbf8ac58 Anurag Kumar Vulisha 2017-08-21  300  		dev_warn(dev, "ceva,p0-retry-params property not defined\n");
b1600f5880a13f Piyush Mehta         2024-01-19  301  		rc = -EINVAL;
b1600f5880a13f Piyush Mehta         2024-01-19  302  		goto disable_phys;
fe8365bbf8ac58 Anurag Kumar Vulisha 2017-08-21  303  	}
fe8365bbf8ac58 Anurag Kumar Vulisha 2017-08-21  304  
fe8365bbf8ac58 Anurag Kumar Vulisha 2017-08-21  305  	if (of_property_read_u16_array(np, "ceva,p1-retry-params",
fe8365bbf8ac58 Anurag Kumar Vulisha 2017-08-21  306  					(u16 *)&cevapriv->pp5c[1], 2) < 0) {
fe8365bbf8ac58 Anurag Kumar Vulisha 2017-08-21  307  		dev_warn(dev, "ceva,p1-retry-params property not defined\n");
b1600f5880a13f Piyush Mehta         2024-01-19  308  		rc = -EINVAL;
b1600f5880a13f Piyush Mehta         2024-01-19  309  		goto disable_phys;
fe8365bbf8ac58 Anurag Kumar Vulisha 2017-08-21  310  	}
fe8365bbf8ac58 Anurag Kumar Vulisha 2017-08-21  311  
3bc867de85b5bf Anurag Kumar Vulisha 2017-08-21  312  	/*
3bc867de85b5bf Anurag Kumar Vulisha 2017-08-21  313  	 * Check if CCI is enabled for SATA. The DEV_DMA_COHERENT is returned
3bc867de85b5bf Anurag Kumar Vulisha 2017-08-21  314  	 * if CCI is enabled, so check for DEV_DMA_COHERENT.
3bc867de85b5bf Anurag Kumar Vulisha 2017-08-21  315  	 */
3bc867de85b5bf Anurag Kumar Vulisha 2017-08-21  316  	attr = device_get_dma_attr(dev);
3bc867de85b5bf Anurag Kumar Vulisha 2017-08-21  317  	cevapriv->is_cci_enabled = (attr == DEV_DMA_COHERENT);
3bc867de85b5bf Anurag Kumar Vulisha 2017-08-21  318  
a73ed35052ca85 Suneel Garapati      2015-06-09  319  	hpriv->plat_data = cevapriv;
a73ed35052ca85 Suneel Garapati      2015-06-09  320  
a73ed35052ca85 Suneel Garapati      2015-06-09  321  	/* CEVA specific initialization */
a73ed35052ca85 Suneel Garapati      2015-06-09  322  	ahci_ceva_setup(hpriv);
a73ed35052ca85 Suneel Garapati      2015-06-09  323  
a73ed35052ca85 Suneel Garapati      2015-06-09  324  	rc = ahci_platform_init_host(pdev, hpriv, &ahci_ceva_port_info,
a73ed35052ca85 Suneel Garapati      2015-06-09  325  					&ahci_platform_sht);
a73ed35052ca85 Suneel Garapati      2015-06-09  326  	if (rc)
a73ed35052ca85 Suneel Garapati      2015-06-09  327  		goto disable_resources;
a73ed35052ca85 Suneel Garapati      2015-06-09  328  
a73ed35052ca85 Suneel Garapati      2015-06-09  329  	return 0;
a73ed35052ca85 Suneel Garapati      2015-06-09  330  
a73ed35052ca85 Suneel Garapati      2015-06-09  331  disable_resources:
a73ed35052ca85 Suneel Garapati      2015-06-09  332  	ahci_platform_disable_resources(hpriv);
b1600f5880a13f Piyush Mehta         2024-01-19  333  
b1600f5880a13f Piyush Mehta         2024-01-19  334  disable_phys:
b1600f5880a13f Piyush Mehta         2024-01-19 @335  	while (--i >= 0) {
                                                               ^^^

b1600f5880a13f Piyush Mehta         2024-01-19  336  		phy_power_off(hpriv->phys[i]);
b1600f5880a13f Piyush Mehta         2024-01-19  337  		phy_exit(hpriv->phys[i]);
b1600f5880a13f Piyush Mehta         2024-01-19  338  	}
b1600f5880a13f Piyush Mehta         2024-01-19  339  
b1600f5880a13f Piyush Mehta         2024-01-19  340  disable_clks:
b1600f5880a13f Piyush Mehta         2024-01-19  341  	ahci_platform_disable_clks(hpriv);
b1600f5880a13f Piyush Mehta         2024-01-19  342  
a73ed35052ca85 Suneel Garapati      2015-06-09  343  	return rc;
a73ed35052ca85 Suneel Garapati      2015-06-09  344  }
Damien Le Moal Jan. 22, 2024, 10:28 a.m. UTC | #2 
On 1/19/24 04:08, Radhey Shyam Pandey wrote:
> From: Piyush Mehta <piyush.mehta@amd.com>
> 
> Platform clock and phy error resources are not cleaned up in Xilinx GT PHY
> error path. To fix introduce error label for ahci_platform_disable_clks and
> phy_power_off/exit and call them in error path. No functional change.
> 
> Fixes: 9a9d3abe24bb ("ata: ahci: ceva: Update the driver to support xilinx GT phy")
> Signed-off-by: Piyush Mehta <piyush.mehta@amd.com>
> Signed-off-by: Radhey Shyam Pandey <radhey.shyam.pandey@amd.com>
> ---
> ---

Your patch format is strange... There is one too many "---" line here.

Other than that, looks OK to me.

Reviewed-by: Damien Le Moal <dlemoal@kernel.org>
Niklas Cassel Jan. 22, 2024, 1:02 p.m. UTC | #3 
Hello Radhey,

On Fri, Jan 19, 2024 at 12:38:23AM +0530, Radhey Shyam Pandey wrote:
> From: Piyush Mehta <piyush.mehta@amd.com>
> 
> Platform clock and phy error resources are not cleaned up in Xilinx GT PHY
> error path. To fix introduce error label for ahci_platform_disable_clks and
> phy_power_off/exit and call them in error path. No functional change.
> 
> Fixes: 9a9d3abe24bb ("ata: ahci: ceva: Update the driver to support xilinx GT phy")
> Signed-off-by: Piyush Mehta <piyush.mehta@amd.com>
> Signed-off-by: Radhey Shyam Pandey <radhey.shyam.pandey@amd.com>
> ---
> ---
>  drivers/ata/ahci_ceva.c | 47 +++++++++++++++++++++++++++++------------
>  1 file changed, 33 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/ata/ahci_ceva.c b/drivers/ata/ahci_ceva.c
> index 64f7f7d6ba84..bfc513f1d0b3 100644
> --- a/drivers/ata/ahci_ceva.c
> +++ b/drivers/ata/ahci_ceva.c
> @@ -196,7 +196,7 @@ static int ceva_ahci_probe(struct platform_device *pdev)
>  	struct ahci_host_priv *hpriv;
>  	struct ceva_ahci_priv *cevapriv;
>  	enum dev_dma_attr attr;
> -	int rc;
> +	int rc, i;
>  
>  	cevapriv = devm_kzalloc(dev, sizeof(*cevapriv), GFP_KERNEL);
>  	if (!cevapriv)
> @@ -219,8 +219,6 @@ static int ceva_ahci_probe(struct platform_device *pdev)
>  		if (rc)
>  			return rc;
>  	} else {
> -		int i;
> -
>  		rc = ahci_platform_enable_clks(hpriv);
>  		if (rc)
>  			return rc;
> @@ -229,8 +227,11 @@ static int ceva_ahci_probe(struct platform_device *pdev)
>  
>  		for (i = 0; i < hpriv->nports; i++) {
>  			rc = phy_init(hpriv->phys[i]);
> -			if (rc)
> -				return rc;
> +			if (rc) {
> +				while (--i >= 0)
> +					phy_exit(hpriv->phys[i]);

It is ugly to have a loop both here and at the end of the function.
Why don't you just goto disable_phys here?

Just like how it is done in ahci_platform_enable_phys():
https://github.com/torvalds/linux/blob/v6.7/drivers/ata/libahci_platform.c#L52-L54


> +				goto disable_clks;
> +			}
>  		}
>  
>  		/* De-assert the controller reset */
> @@ -240,7 +241,7 @@ static int ceva_ahci_probe(struct platform_device *pdev)
>  			rc = phy_power_on(hpriv->phys[i]);
>  			if (rc) {
>  				phy_exit(hpriv->phys[i]);
> -				return rc;
> +				goto disable_phys;
>  			}
>  		}
>  	}
> @@ -252,52 +253,60 @@ static int ceva_ahci_probe(struct platform_device *pdev)
>  	if (of_property_read_u8_array(np, "ceva,p0-cominit-params",
>  					(u8 *)&cevapriv->pp2c[0], 4) < 0) {
>  		dev_warn(dev, "ceva,p0-cominit-params property not defined\n");
> -		return -EINVAL;
> +		rc = -EINVAL;
> +		goto disable_phys;
>  	}
>  
>  	if (of_property_read_u8_array(np, "ceva,p1-cominit-params",
>  					(u8 *)&cevapriv->pp2c[1], 4) < 0) {
>  		dev_warn(dev, "ceva,p1-cominit-params property not defined\n");
> -		return -EINVAL;
> +		rc = -EINVAL;
> +		goto disable_phys;
>  	}
>  
>  	/* Read OOB timing value for COMWAKE from device-tree*/
>  	if (of_property_read_u8_array(np, "ceva,p0-comwake-params",
>  					(u8 *)&cevapriv->pp3c[0], 4) < 0) {
>  		dev_warn(dev, "ceva,p0-comwake-params property not defined\n");
> -		return -EINVAL;
> +		rc = -EINVAL;
> +		goto disable_phys;
>  	}
>  
>  	if (of_property_read_u8_array(np, "ceva,p1-comwake-params",
>  					(u8 *)&cevapriv->pp3c[1], 4) < 0) {
>  		dev_warn(dev, "ceva,p1-comwake-params property not defined\n");
> -		return -EINVAL;
> +		rc = -EINVAL;
> +		goto disable_phys;
>  	}
>  
>  	/* Read phy BURST timing value from device-tree */
>  	if (of_property_read_u8_array(np, "ceva,p0-burst-params",
>  					(u8 *)&cevapriv->pp4c[0], 4) < 0) {
>  		dev_warn(dev, "ceva,p0-burst-params property not defined\n");
> -		return -EINVAL;
> +		rc = -EINVAL;
> +		goto disable_phys;
>  	}
>  
>  	if (of_property_read_u8_array(np, "ceva,p1-burst-params",
>  					(u8 *)&cevapriv->pp4c[1], 4) < 0) {
>  		dev_warn(dev, "ceva,p1-burst-params property not defined\n");
> -		return -EINVAL;
> +		rc = -EINVAL;
> +		goto disable_phys;
>  	}
>  
>  	/* Read phy RETRY interval timing value from device-tree */
>  	if (of_property_read_u16_array(np, "ceva,p0-retry-params",
>  					(u16 *)&cevapriv->pp5c[0], 2) < 0) {
>  		dev_warn(dev, "ceva,p0-retry-params property not defined\n");
> -		return -EINVAL;
> +		rc = -EINVAL;
> +		goto disable_phys;
>  	}
>  
>  	if (of_property_read_u16_array(np, "ceva,p1-retry-params",
>  					(u16 *)&cevapriv->pp5c[1], 2) < 0) {
>  		dev_warn(dev, "ceva,p1-retry-params property not defined\n");
> -		return -EINVAL;
> +		rc = -EINVAL;
> +		goto disable_phys;
>  	}
>  
>  	/*
> @@ -321,6 +330,16 @@ static int ceva_ahci_probe(struct platform_device *pdev)
>  
>  disable_resources:
>  	ahci_platform_disable_resources(hpriv);

Looking at ahci_platform_disable_resources(),
it calls ahci_platform_disable_phys(), which calls
phy_power_off() and phy_exit().

This means that if you jump to disable_resources,
you will call phy_power_off() and phy_exit() twice.
Looking at the phy code, it does not handle these functions being called
twice.


You already call ahci_platform_get_resources(), so why don't you just set
AHCI_PLATFORM_GET_RESETS, that way you should be able to remove a bunch of
duplicated code from this driver.

One major difference seems to be that ahci_platform_enable_resources() does
not assert reset before deasserting it.
Can't we just add a reset_control_assert() + some small usleep
(e.g. usleep_range(1000, 1500)) before the reset_control_deassert()?
Have you tried doing that?


Kind regards,
Niklas

> +
> +disable_phys:
> +	while (--i >= 0) {
> +		phy_power_off(hpriv->phys[i]);
> +		phy_exit(hpriv->phys[i]);
> +	}
> +
> +disable_clks:
> +	ahci_platform_disable_clks(hpriv);
> +
>  	return rc;
>  }
>  
> -- 
> 2.34.1
>
Pandey, Radhey Shyam Feb. 7, 2024, 6:28 p.m. UTC | #4 
> -----Original Message-----
> From: Niklas Cassel <cassel@kernel.org>
> Sent: Monday, January 22, 2024 6:33 PM
> To: Pandey, Radhey Shyam <radhey.shyam.pandey@amd.com>
> Cc: dlemoal@kernel.org; richardcochran@gmail.com;
> piyush.mehta@xilinx.com; axboe@kernel.dk; Simek, Michal
> <michal.simek@amd.com>; linux-ide@vger.kernel.org; linux-
> kernel@vger.kernel.org; git (AMD-Xilinx) <git@amd.com>; Mehta, Piyush
> <piyush.mehta@amd.com>
> Subject: Re: [PATCH 1/2] ata: ahci_ceva: fix error handling for Xilinx GT PHY
> support
> 
> Hello Radhey,
> 
> On Fri, Jan 19, 2024 at 12:38:23AM +0530, Radhey Shyam Pandey wrote:
> > From: Piyush Mehta <piyush.mehta@amd.com>
> >
> > Platform clock and phy error resources are not cleaned up in Xilinx GT
> > PHY error path. To fix introduce error label for
> > ahci_platform_disable_clks and phy_power_off/exit and call them in error
> path. No functional change.
> >
> > Fixes: 9a9d3abe24bb ("ata: ahci: ceva: Update the driver to support
> > xilinx GT phy")
> > Signed-off-by: Piyush Mehta <piyush.mehta@amd.com>
> > Signed-off-by: Radhey Shyam Pandey <radhey.shyam.pandey@amd.com>
> > ---
> > ---
> >  drivers/ata/ahci_ceva.c | 47
> > +++++++++++++++++++++++++++++------------
> >  1 file changed, 33 insertions(+), 14 deletions(-)
> >
> > diff --git a/drivers/ata/ahci_ceva.c b/drivers/ata/ahci_ceva.c index
> > 64f7f7d6ba84..bfc513f1d0b3 100644
> > --- a/drivers/ata/ahci_ceva.c
> > +++ b/drivers/ata/ahci_ceva.c
> > @@ -196,7 +196,7 @@ static int ceva_ahci_probe(struct platform_device
> *pdev)
> >  	struct ahci_host_priv *hpriv;
> >  	struct ceva_ahci_priv *cevapriv;
> >  	enum dev_dma_attr attr;
> > -	int rc;
> > +	int rc, i;
> >
> >  	cevapriv = devm_kzalloc(dev, sizeof(*cevapriv), GFP_KERNEL);
> >  	if (!cevapriv)
> > @@ -219,8 +219,6 @@ static int ceva_ahci_probe(struct platform_device
> *pdev)
> >  		if (rc)
> >  			return rc;
> >  	} else {
> > -		int i;
> > -
> >  		rc = ahci_platform_enable_clks(hpriv);
> >  		if (rc)
> >  			return rc;
> > @@ -229,8 +227,11 @@ static int ceva_ahci_probe(struct platform_device
> > *pdev)
> >
> >  		for (i = 0; i < hpriv->nports; i++) {
> >  			rc = phy_init(hpriv->phys[i]);
> > -			if (rc)
> > -				return rc;
> > +			if (rc) {
> > +				while (--i >= 0)
> > +					phy_exit(hpriv->phys[i]);
> 
> It is ugly to have a loop both here and at the end of the function.
> Why don't you just goto disable_phys here?
> 
> Just like how it is done in ahci_platform_enable_phys():
> https://github.com/torvalds/linux/blob/v6.7/drivers/ata/libahci_platform.c#
> L52-L54
> 
> 
> > +				goto disable_clks;
> > +			}
> >  		}
> >
> >  		/* De-assert the controller reset */ @@ -240,7 +241,7 @@
> static int
> > ceva_ahci_probe(struct platform_device *pdev)
> >  			rc = phy_power_on(hpriv->phys[i]);
> >  			if (rc) {
> >  				phy_exit(hpriv->phys[i]);
> > -				return rc;
> > +				goto disable_phys;
> >  			}
> >  		}
> >  	}
> > @@ -252,52 +253,60 @@ static int ceva_ahci_probe(struct
> platform_device *pdev)
> >  	if (of_property_read_u8_array(np, "ceva,p0-cominit-params",
> >  					(u8 *)&cevapriv->pp2c[0], 4) < 0) {
> >  		dev_warn(dev, "ceva,p0-cominit-params property not
> defined\n");
> > -		return -EINVAL;
> > +		rc = -EINVAL;
> > +		goto disable_phys;
> >  	}
> >
> >  	if (of_property_read_u8_array(np, "ceva,p1-cominit-params",
> >  					(u8 *)&cevapriv->pp2c[1], 4) < 0) {
> >  		dev_warn(dev, "ceva,p1-cominit-params property not
> defined\n");
> > -		return -EINVAL;
> > +		rc = -EINVAL;
> > +		goto disable_phys;
> >  	}
> >
> >  	/* Read OOB timing value for COMWAKE from device-tree*/
> >  	if (of_property_read_u8_array(np, "ceva,p0-comwake-params",
> >  					(u8 *)&cevapriv->pp3c[0], 4) < 0) {
> >  		dev_warn(dev, "ceva,p0-comwake-params property not
> defined\n");
> > -		return -EINVAL;
> > +		rc = -EINVAL;
> > +		goto disable_phys;
> >  	}
> >
> >  	if (of_property_read_u8_array(np, "ceva,p1-comwake-params",
> >  					(u8 *)&cevapriv->pp3c[1], 4) < 0) {
> >  		dev_warn(dev, "ceva,p1-comwake-params property not
> defined\n");
> > -		return -EINVAL;
> > +		rc = -EINVAL;
> > +		goto disable_phys;
> >  	}
> >
> >  	/* Read phy BURST timing value from device-tree */
> >  	if (of_property_read_u8_array(np, "ceva,p0-burst-params",
> >  					(u8 *)&cevapriv->pp4c[0], 4) < 0) {
> >  		dev_warn(dev, "ceva,p0-burst-params property not
> defined\n");
> > -		return -EINVAL;
> > +		rc = -EINVAL;
> > +		goto disable_phys;
> >  	}
> >
> >  	if (of_property_read_u8_array(np, "ceva,p1-burst-params",
> >  					(u8 *)&cevapriv->pp4c[1], 4) < 0) {
> >  		dev_warn(dev, "ceva,p1-burst-params property not
> defined\n");
> > -		return -EINVAL;
> > +		rc = -EINVAL;
> > +		goto disable_phys;
> >  	}
> >
> >  	/* Read phy RETRY interval timing value from device-tree */
> >  	if (of_property_read_u16_array(np, "ceva,p0-retry-params",
> >  					(u16 *)&cevapriv->pp5c[0], 2) < 0) {
> >  		dev_warn(dev, "ceva,p0-retry-params property not
> defined\n");
> > -		return -EINVAL;
> > +		rc = -EINVAL;
> > +		goto disable_phys;
> >  	}
> >
> >  	if (of_property_read_u16_array(np, "ceva,p1-retry-params",
> >  					(u16 *)&cevapriv->pp5c[1], 2) < 0) {
> >  		dev_warn(dev, "ceva,p1-retry-params property not
> defined\n");
> > -		return -EINVAL;
> > +		rc = -EINVAL;
> > +		goto disable_phys;
> >  	}
> >
> >  	/*
> > @@ -321,6 +330,16 @@ static int ceva_ahci_probe(struct platform_device
> > *pdev)
> >
> >  disable_resources:
> >  	ahci_platform_disable_resources(hpriv);
> 
> Looking at ahci_platform_disable_resources(),
> it calls ahci_platform_disable_phys(), which calls
> phy_power_off() and phy_exit().
> 
> This means that if you jump to disable_resources, you will call
> phy_power_off() and phy_exit() twice.
> Looking at the phy code, it does not handle these functions being called
> twice.
> 
> 
> You already call ahci_platform_get_resources(), so why don't you just set
> AHCI_PLATFORM_GET_RESETS, that way you should be able to remove a
> bunch of duplicated code from this driver.
> 
> One major difference seems to be that ahci_platform_enable_resources()
> does not assert reset before deasserting it.
> Can't we just add a reset_control_assert() + some small usleep (e.g.
> usleep_range(1000, 1500)) before the reset_control_deassert()?
> Have you tried doing that?

As I understand AHCI-platform library supports shared reset control 
lines and AHCI SATA platform driver request exclusive reset lines.

So we need to move reset request out of ahci platform library and
make it driver specific as done in commit "9a9d3abe24bb 
("ata: ahci: ceva: Update the driver to support xilinx GT phy")

Furthermore SATA IP programming mentioned in 
Zynq UltraScale+ Device TRM mentions -

Assert SATA reset.
Set PS-GTR configuration (Part of phy_init)
Bring SATA by de-asserting the reset.

Now looking at ahci_platform_enable_resources(), it 
enables all ahci_platform managed resources in the
following order
1) Regulator
2) Clocks (through ahci_platform_enable_clks)
3) Resets
4) Phys

Here reset is de-asserted before phy_init() which
is different from SATA Controller Programming Flow.

I assume because of above programming sequence
differences we earlier didn't use generic 
ahci_platform_enable_resources() and instead used 
a custom implementation derived from it in ceva
AHCI SATA platform driver.

Coming to reusing ahci_platform_enable_resources(),
if we have to do we have skip step-3. and let ceva 
ahci driver request reset and then call
reset_control_assert()/deassert. 

However for single controller IP programming sequence
deviation I am less inclined to make modification in 
generic enable_resources() API . But if you strongly 
feel we should go ahead and make changes to the generic
enable_resources API, please suggest your thoughts
and then we can get started.

Thanks,
Radhey
Niklas Cassel Feb. 7, 2024, 8:42 p.m. UTC | #5 
Hello Radhey,

On Wed, Feb 07, 2024 at 06:28:25PM +0000, Pandey, Radhey Shyam wrote:

(snip)

> However for single controller IP programming sequence
> deviation I am less inclined to make modification in
> generic enable_resources() API . But if you strongly
> feel we should go ahead and make changes to the generic
> enable_resources API, please suggest your thoughts
> and then we can get started.

My first suggestion was to try to add a reset_control_assert() + msleep(1)
before the reset_control_deassert() in ahci_platform_enable_resources().
This should be safe for all platforms as in order to trigger a reset,
you actually need to pulse it. It is possible that your platform has
reset deasserted from boot, and in that case, since the generic version
currently only does a deassert, it is possible that no reset was done
at all on your platform.

If my first suggestion did not work, then my second suggestion was to
add a new flag which your platform driver sets
(e.g. HOLD_RESET_DURING_PHY_INIT), and implement that in a new function
in libahci_platform.c. The generic version could check for this flag at
the start of the function, and return the result from your new function
in libahci_platform.c instead.


However, since you have a TRM, let's ignore these two proposals and instead
re-spin your existing series, addressing the other outstanding issues:
1) The new kernel test robot build warnings introduced by your patch.

- The other review comments that are still applicable:
2) Your patch format is strange... There is one too many "---" line here.
3) Use goto disable_phys so you don't need to loop twice.
4) If you jump to disable_resources, you will call phy_power_off()
  and phy_exit() twice, which the PHY lib does not handle.
5) Please be a bit more specific in your commit message.
6) Describe your changes in imperative mood.
7) A new comment that I see when looking at your driver just now,
you should simply remove:

"""
	if (!cevapriv->rst) {
		rc = ahci_platform_enable_resources(hpriv);
		if (rc)
			return rc;
	} else {
"""

Either you want to use ahci_platform_enable_resources(),
or your own way, but you don't get to do both.

You call devm_reset_control_get_optional_exclusive(), which will
return NULL for platforms that do not supply a reset.

Both reset_control_assert() and reset_control_deassert() are no-ops
if you send in a NULL pointer. The are designed this way to work
with devm_reset_control_get_optional_exclusive() which will return
NULL for platforms that do not provide a reset, such that drivers,
such as yours, should not need
if () {
} else {
depending on if get_reset() returned non-NULL or not.

Just always call reset_control_assert()/reset_control_deassert(),
even for the !cevapriv->rst case, as they will be no-ops anyway.


Kind regards,
Niklas
diff mbox series

Patch

diff --git a/drivers/ata/ahci_ceva.c b/drivers/ata/ahci_ceva.c
index 64f7f7d6ba84..bfc513f1d0b3 100644
--- a/drivers/ata/ahci_ceva.c
+++ b/drivers/ata/ahci_ceva.c
@@ -196,7 +196,7 @@  static int ceva_ahci_probe(struct platform_device *pdev)
 	struct ahci_host_priv *hpriv;
 	struct ceva_ahci_priv *cevapriv;
 	enum dev_dma_attr attr;
-	int rc;
+	int rc, i;
 
 	cevapriv = devm_kzalloc(dev, sizeof(*cevapriv), GFP_KERNEL);
 	if (!cevapriv)
@@ -219,8 +219,6 @@  static int ceva_ahci_probe(struct platform_device *pdev)
 		if (rc)
 			return rc;
 	} else {
-		int i;
-
 		rc = ahci_platform_enable_clks(hpriv);
 		if (rc)
 			return rc;
@@ -229,8 +227,11 @@  static int ceva_ahci_probe(struct platform_device *pdev)
 
 		for (i = 0; i < hpriv->nports; i++) {
 			rc = phy_init(hpriv->phys[i]);
-			if (rc)
-				return rc;
+			if (rc) {
+				while (--i >= 0)
+					phy_exit(hpriv->phys[i]);
+				goto disable_clks;
+			}
 		}
 
 		/* De-assert the controller reset */
@@ -240,7 +241,7 @@  static int ceva_ahci_probe(struct platform_device *pdev)
 			rc = phy_power_on(hpriv->phys[i]);
 			if (rc) {
 				phy_exit(hpriv->phys[i]);
-				return rc;
+				goto disable_phys;
 			}
 		}
 	}
@@ -252,52 +253,60 @@  static int ceva_ahci_probe(struct platform_device *pdev)
 	if (of_property_read_u8_array(np, "ceva,p0-cominit-params",
 					(u8 *)&cevapriv->pp2c[0], 4) < 0) {
 		dev_warn(dev, "ceva,p0-cominit-params property not defined\n");
-		return -EINVAL;
+		rc = -EINVAL;
+		goto disable_phys;
 	}
 
 	if (of_property_read_u8_array(np, "ceva,p1-cominit-params",
 					(u8 *)&cevapriv->pp2c[1], 4) < 0) {
 		dev_warn(dev, "ceva,p1-cominit-params property not defined\n");
-		return -EINVAL;
+		rc = -EINVAL;
+		goto disable_phys;
 	}
 
 	/* Read OOB timing value for COMWAKE from device-tree*/
 	if (of_property_read_u8_array(np, "ceva,p0-comwake-params",
 					(u8 *)&cevapriv->pp3c[0], 4) < 0) {
 		dev_warn(dev, "ceva,p0-comwake-params property not defined\n");
-		return -EINVAL;
+		rc = -EINVAL;
+		goto disable_phys;
 	}
 
 	if (of_property_read_u8_array(np, "ceva,p1-comwake-params",
 					(u8 *)&cevapriv->pp3c[1], 4) < 0) {
 		dev_warn(dev, "ceva,p1-comwake-params property not defined\n");
-		return -EINVAL;
+		rc = -EINVAL;
+		goto disable_phys;
 	}
 
 	/* Read phy BURST timing value from device-tree */
 	if (of_property_read_u8_array(np, "ceva,p0-burst-params",
 					(u8 *)&cevapriv->pp4c[0], 4) < 0) {
 		dev_warn(dev, "ceva,p0-burst-params property not defined\n");
-		return -EINVAL;
+		rc = -EINVAL;
+		goto disable_phys;
 	}
 
 	if (of_property_read_u8_array(np, "ceva,p1-burst-params",
 					(u8 *)&cevapriv->pp4c[1], 4) < 0) {
 		dev_warn(dev, "ceva,p1-burst-params property not defined\n");
-		return -EINVAL;
+		rc = -EINVAL;
+		goto disable_phys;
 	}
 
 	/* Read phy RETRY interval timing value from device-tree */
 	if (of_property_read_u16_array(np, "ceva,p0-retry-params",
 					(u16 *)&cevapriv->pp5c[0], 2) < 0) {
 		dev_warn(dev, "ceva,p0-retry-params property not defined\n");
-		return -EINVAL;
+		rc = -EINVAL;
+		goto disable_phys;
 	}
 
 	if (of_property_read_u16_array(np, "ceva,p1-retry-params",
 					(u16 *)&cevapriv->pp5c[1], 2) < 0) {
 		dev_warn(dev, "ceva,p1-retry-params property not defined\n");
-		return -EINVAL;
+		rc = -EINVAL;
+		goto disable_phys;
 	}
 
 	/*
@@ -321,6 +330,16 @@  static int ceva_ahci_probe(struct platform_device *pdev)
 
 disable_resources:
 	ahci_platform_disable_resources(hpriv);
+
+disable_phys:
+	while (--i >= 0) {
+		phy_power_off(hpriv->phys[i]);
+		phy_exit(hpriv->phys[i]);
+	}
+
+disable_clks:
+	ahci_platform_disable_clks(hpriv);
+
 	return rc;
 }