| Message ID | 1705604904-471889-2-git-send-email-radhey.shyam.pandey@amd.com |
|---|---|
| State | New |
| Headers | show |
| Series | ata: ahci_ceva: fix xilinx GT PHY support | expand |
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 }
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>
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 >
> -----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
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 --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; }