| Message ID | 1708107297-1798828-1-git-send-email-radhey.shyam.pandey@amd.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v3] ata: ahci_ceva: fix error handling for Xilinx GT PHY support | expand |
On Fri, Feb 16, 2024 at 11:44:57PM +0530, Radhey Shyam Pandey wrote: > Platform clock and phy error resources are not cleaned up in Xilinx GT PHY > error path. > > To fix introduce the function ceva_ahci_platform_enable_resources() which > is a customized version of ahci_platform_enable_resources() and inline with > SATA IP programming sequence it does: > > - Assert SATA reset > - Program PS GTR phy > - Bring SATA by de-asserting the reset > - Wait for GT lane PLL to be locked > > ceva_ahci_platform_enable_resources() is also used in the resume path > as the same SATA programming sequence (as in probe) should be followed. > Also cleanup the mixed usage of ahci_platform_enable_resources() and custom > implementation in the probe function as both are not required. > > Fixes: 9a9d3abe24bb ("ata: ahci: ceva: Update the driver to support xilinx GT phy") > Signed-off-by: Radhey Shyam Pandey <radhey.shyam.pandey@amd.com> > Reviewed-by: Damien Le Moal <dlemoal@kernel.org> > --- > Changes for v3: > - Modified commit description as suggested by Damien Le Moal. > - Add missing return in dev_err_probe("failed to get reset") > pointed by Niklas. > > Changes for v2: > > - Create wrapper ceva_ahci_platform_enable_resources() > - Remove legacy ahci_platform_enable_resources() and its related code. > - Modified commit description and merge 1/2 and 2/2 fix as it is > automatically done when reusing ahci_platform_enable_resources() > logic. > - Drop Reviewed-by: Damien Le Moal <dlemoal@kernel.org> tag. > --- Applied: https://git.kernel.org/pub/scm/linux/kernel/git/libata/linux.git/log/?h=for-6.8-fixes
> > Platform clock and phy error resources are not cleaned up in Xilinx GT PHY > > error path. > > > > To fix introduce the function ceva_ahci_platform_enable_resources() … > Applied: > https://git.kernel.org/pub/scm/linux/kernel/git/libata/linux.git/log/?h=for-6.8-fixes The error code “-EINVAL” was set before the statement “goto disable_resources” multiple times in the adjusted implementation of the function “ceva_ahci_probe”. I suggest to add a jump target so that a bit of exception handling can be better reused at the end of this function. How do you think about to apply the following script for the semantic patch language (Coccinelle software) accordingly? @replacement1@ identifier rc; @@ <+... if (...) { ... when != rc - rc = -EINVAL; goto - disable_resources + e_inval ; } ...+> return 0; + +e_inval: +rc = -EINVAL; disable_resources: ahci_platform_disable_resources(hpriv); @replacement2 disable neg_if, drop_else@ identifier replacement1.rc; statement is; @@ if (...) is else { ... when != rc - rc = -EINVAL; goto - disable_resources + e_inval ; } Regards, Markus
> -----Original Message----- > From: Markus Elfring <Markus.Elfring@web.de> > Sent: Monday, February 19, 2024 9:27 PM > To: Niklas Cassel <cassel@kernel.org>; Pandey, Radhey Shyam > <radhey.shyam.pandey@amd.com>; Damien Le Moal > <dlemoal@kernel.org>; Jens Axboe <axboe@kernel.dk>; Simek, Michal > <michal.simek@amd.com>; Philipp Zabel <p.zabel@pengutronix.de>; linux- > ide@vger.kernel.org; kernel-janitors@vger.kernel.org > Cc: LKML <linux-kernel@vger.kernel.org>; git (AMD-Xilinx) <git@amd.com> > Subject: Re: [PATCH v3] ata: ahci_ceva: fix error handling for Xilinx GT PHY > support > > > > Platform clock and phy error resources are not cleaned up in Xilinx GT > PHY > > > error path. > > > > > > To fix introduce the function ceva_ahci_platform_enable_resources() > … > > Applied: > > > https://git.kernel.org/pub/scm/linux/kernel/git/libata/linux.git/log/?h=for- > 6.8-fixes > > The error code “-EINVAL” was set before the statement “goto > disable_resources” > multiple times in the adjusted implementation of the function > “ceva_ahci_probe”. > I suggest to add a jump target so that a bit of exception handling > can be better reused at the end of this function. > > > How do you think about to apply the following script for the semantic > patch language (Coccinelle software) accordingly? > > > @replacement1@ > identifier rc; > @@ > <+... > if (...) > { > ... when != rc > - rc = -EINVAL; > goto > - disable_resources > + e_inval > ; > } > ...+> > return 0; > + > +e_inval: > +rc = -EINVAL; > disable_resources: > ahci_platform_disable_resources(hpriv); > > @replacement2 disable neg_if, drop_else@ > identifier replacement1.rc; > statement is; > @@ > if (...) > is > else > { > ... when != rc > - rc = -EINVAL; > goto > - disable_resources > + e_inval > ; > } > > Thanks for the suggestion. However, taking a look at the existing implementation i think we should return error code *as is * from of_property_read() APIs. and get rid of rc=-EINVAL reassignment itself. If it sounds ok, I can add it to my to-do list and send out a patch. Thanks, Radhey
Hello Radhey, Markus, On Mon, Feb 19, 2024 at 06:42:49PM +0000, Pandey, Radhey Shyam wrote: > > -----Original Message----- > > From: Markus Elfring <Markus.Elfring@web.de> > > Sent: Monday, February 19, 2024 9:27 PM > > To: Niklas Cassel <cassel@kernel.org>; Pandey, Radhey Shyam > > <radhey.shyam.pandey@amd.com>; Damien Le Moal > > <dlemoal@kernel.org>; Jens Axboe <axboe@kernel.dk>; Simek, Michal > > <michal.simek@amd.com>; Philipp Zabel <p.zabel@pengutronix.de>; linux- > > ide@vger.kernel.org; kernel-janitors@vger.kernel.org > > Cc: LKML <linux-kernel@vger.kernel.org>; git (AMD-Xilinx) <git@amd.com> > > Subject: Re: [PATCH v3] ata: ahci_ceva: fix error handling for Xilinx GT PHY > > support > > > > > > Platform clock and phy error resources are not cleaned up in Xilinx GT > > PHY > > > > error path. > > > > > > > > To fix introduce the function ceva_ahci_platform_enable_resources() > > … > > > Applied: > > > > > https://git.kernel.org/pub/scm/linux/kernel/git/libata/linux.git/log/?h=for- > > 6.8-fixes > > > > The error code “-EINVAL” was set before the statement “goto > > disable_resources” > > multiple times in the adjusted implementation of the function > > “ceva_ahci_probe”. > > I suggest to add a jump target so that a bit of exception handling > > can be better reused at the end of this function. > > > > > > How do you think about to apply the following script for the semantic > > patch language (Coccinelle software) accordingly? > > > > > > @replacement1@ > > identifier rc; > > @@ > > <+... > > if (...) > > { > > ... when != rc > > - rc = -EINVAL; > > goto > > - disable_resources > > + e_inval > > ; > > } > > ...+> > > return 0; > > + > > +e_inval: > > +rc = -EINVAL; > > disable_resources: > > ahci_platform_disable_resources(hpriv); > > > > @replacement2 disable neg_if, drop_else@ > > identifier replacement1.rc; > > statement is; > > @@ > > if (...) > > is > > else > > { > > ... when != rc > > - rc = -EINVAL; > > goto > > - disable_resources > > + e_inval > > ; > > } > > > > > Thanks for the suggestion. However, taking a look at the existing implementation > i think we should return error code *as is * from of_property_read() APIs. > and get rid of rc=-EINVAL reassignment itself. > > If it sounds ok, I can add it to my to-do list and send out a patch. Sounds good to me. Kind regards, Niklas
diff --git a/drivers/ata/ahci_ceva.c b/drivers/ata/ahci_ceva.c index 64f7f7d6ba84..11a2c199a7c2 100644 --- a/drivers/ata/ahci_ceva.c +++ b/drivers/ata/ahci_ceva.c @@ -88,7 +88,6 @@ struct ceva_ahci_priv { u32 axicc; bool is_cci_enabled; int flags; - struct reset_control *rst; }; static unsigned int ceva_ahci_read_id(struct ata_device *dev, @@ -189,6 +188,60 @@ static const struct scsi_host_template ahci_platform_sht = { AHCI_SHT(DRV_NAME), }; +static int ceva_ahci_platform_enable_resources(struct ahci_host_priv *hpriv) +{ + int rc, i; + + rc = ahci_platform_enable_regulators(hpriv); + if (rc) + return rc; + + rc = ahci_platform_enable_clks(hpriv); + if (rc) + goto disable_regulator; + + /* Assert the controller reset */ + rc = ahci_platform_assert_rsts(hpriv); + if (rc) + goto disable_clks; + + for (i = 0; i < hpriv->nports; i++) { + rc = phy_init(hpriv->phys[i]); + if (rc) + goto disable_rsts; + } + + /* De-assert the controller reset */ + ahci_platform_deassert_rsts(hpriv); + + for (i = 0; i < hpriv->nports; i++) { + rc = phy_power_on(hpriv->phys[i]); + if (rc) { + phy_exit(hpriv->phys[i]); + goto disable_phys; + } + } + + return 0; + +disable_rsts: + ahci_platform_deassert_rsts(hpriv); + +disable_phys: + while (--i >= 0) { + phy_power_off(hpriv->phys[i]); + phy_exit(hpriv->phys[i]); + } + +disable_clks: + ahci_platform_disable_clks(hpriv); + +disable_regulator: + ahci_platform_disable_regulators(hpriv); + + return rc; +} + static int ceva_ahci_probe(struct platform_device *pdev) { struct device_node *np = pdev->dev.of_node; @@ -203,47 +256,19 @@ static int ceva_ahci_probe(struct platform_device *pdev) return -ENOMEM; cevapriv->ahci_pdev = pdev; - - cevapriv->rst = devm_reset_control_get_optional_exclusive(&pdev->dev, - NULL); - if (IS_ERR(cevapriv->rst)) - dev_err_probe(&pdev->dev, PTR_ERR(cevapriv->rst), - "failed to get reset\n"); - hpriv = ahci_platform_get_resources(pdev, 0); if (IS_ERR(hpriv)) return PTR_ERR(hpriv); - if (!cevapriv->rst) { - rc = ahci_platform_enable_resources(hpriv); - if (rc) - return rc; - } else { - int i; + hpriv->rsts = devm_reset_control_get_optional_exclusive(&pdev->dev, + NULL); + if (IS_ERR(hpriv->rsts)) + return dev_err_probe(&pdev->dev, PTR_ERR(hpriv->rsts), + "failed to get reset\n"); - rc = ahci_platform_enable_clks(hpriv); - if (rc) - return rc; - /* Assert the controller reset */ - reset_control_assert(cevapriv->rst); - - for (i = 0; i < hpriv->nports; i++) { - rc = phy_init(hpriv->phys[i]); - if (rc) - return rc; - } - - /* De-assert the controller reset */ - reset_control_deassert(cevapriv->rst); - - for (i = 0; i < hpriv->nports; i++) { - rc = phy_power_on(hpriv->phys[i]); - if (rc) { - phy_exit(hpriv->phys[i]); - return rc; - } - } - } + rc = ceva_ahci_platform_enable_resources(hpriv); + if (rc) + return rc; if (of_property_read_bool(np, "ceva,broken-gen2")) cevapriv->flags = CEVA_FLAG_BROKEN_GEN2; @@ -252,52 +277,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_resources; } 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_resources; } /* 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_resources; } 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_resources; } /* 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_resources; } 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_resources; } /* 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_resources; } 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_resources; } /* @@ -335,7 +368,7 @@ static int __maybe_unused ceva_ahci_resume(struct device *dev) struct ahci_host_priv *hpriv = host->private_data; int rc; - rc = ahci_platform_enable_resources(hpriv); + rc = ceva_ahci_platform_enable_resources(hpriv); if (rc) return rc;
Platform clock and phy error resources are not cleaned up in Xilinx GT PHY error path. To fix introduce the function ceva_ahci_platform_enable_resources() which is a customized version of ahci_platform_enable_resources() and inline with SATA IP programming sequence it does: - Assert SATA reset - Program PS GTR phy - Bring SATA by de-asserting the reset - Wait for GT lane PLL to be locked ceva_ahci_platform_enable_resources() is also used in the resume path as the same SATA programming sequence (as in probe) should be followed. Also cleanup the mixed usage of ahci_platform_enable_resources() and custom implementation in the probe function as both are not required. Fixes: 9a9d3abe24bb ("ata: ahci: ceva: Update the driver to support xilinx GT phy") Signed-off-by: Radhey Shyam Pandey <radhey.shyam.pandey@amd.com> Reviewed-by: Damien Le Moal <dlemoal@kernel.org> --- Changes for v3: - Modified commit description as suggested by Damien Le Moal. - Add missing return in dev_err_probe("failed to get reset") pointed by Niklas. Changes for v2: - Create wrapper ceva_ahci_platform_enable_resources() - Remove legacy ahci_platform_enable_resources() and its related code. - Modified commit description and merge 1/2 and 2/2 fix as it is automatically done when reusing ahci_platform_enable_resources() logic. - Drop Reviewed-by: Damien Le Moal <dlemoal@kernel.org> tag. --- drivers/ata/ahci_ceva.c | 125 +++++++++++++++++++++++++--------------- 1 file changed, 79 insertions(+), 46 deletions(-)