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 <[email protected]>
Reviewed-by: Damien Le Moal <[email protected]>
---
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 <[email protected]> tag.
---
drivers/ata/ahci_ceva.c | 125 +++++++++++++++++++++++++---------------
1 file changed, 79 insertions(+), 46 deletions(-)
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;
--
2.34.1
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 <[email protected]>
> Reviewed-by: Damien Le Moal <[email protected]>
> ---
> 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 <[email protected]> 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 <[email protected]>
> Sent: Monday, February 19, 2024 9:27 PM
> To: Niklas Cassel <[email protected]>; Pandey, Radhey Shyam
> <[email protected]>; Damien Le Moal
> <[email protected]>; Jens Axboe <[email protected]>; Simek, Michal
> <[email protected]>; Philipp Zabel <[email protected]>; linux-
> [email protected]; [email protected]
> Cc: LKML <[email protected]>; git (AMD-Xilinx) <[email protected]>
> 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 <[email protected]>
> > Sent: Monday, February 19, 2024 9:27 PM
> > To: Niklas Cassel <[email protected]>; Pandey, Radhey Shyam
> > <[email protected]>; Damien Le Moal
> > <[email protected]>; Jens Axboe <[email protected]>; Simek, Michal
> > <[email protected]>; Philipp Zabel <[email protected]>; linux-
> > [email protected]; [email protected]
> > Cc: LKML <[email protected]>; git (AMD-Xilinx) <[email protected]>
> > 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