All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] libata: ata_host_suspend() always returns 0
@ 2022-01-31 19:57 ` Sergey Shtylyov
  0 siblings, 0 replies; 6+ messages in thread
From: Sergey Shtylyov @ 2022-01-31 19:57 UTC (permalink / raw)
  To: Damien Le Moal, linux-ide
  Cc: Shawn Guo, Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam,
	NXP Linux Team, linux-arm-kernel

ata_host_suspend() always returns 0, so the result checks in the drivers
are pointless.  However I chose not to change ata_host_suspend() to *void*
not to have to change

	return ata_host_suspend(...);
to
	ata_host_suspend(...);
	return 0;

Found by Linux Verification Center (linuxtesting.org) with the SVACE static
analysis tool.

Signed-off-by: Sergey Shtylyov <s.shtylyov@omp.ru>

---
This patch is against the 'for-next' branch of Damien Le Moal's 'libata.git'
repo.

 drivers/ata/ata_piix.c     |    5 +----
 drivers/ata/libata-core.c  |    7 +++----
 drivers/ata/pata_cs5520.c  |    5 +----
 drivers/ata/pata_imx.c     |   15 ++++++---------
 drivers/ata/pata_macio.c   |    6 +-----
 drivers/ata/pata_triflex.c |    5 +----
 drivers/ata/sata_mv.c      |    6 +++---
 drivers/ata/sata_rcar.c    |   18 ++++++++----------
 8 files changed, 24 insertions(+), 43 deletions(-)

Index: libata/drivers/ata/ata_piix.c
===================================================================
--- libata.orig/drivers/ata/ata_piix.c
+++ libata/drivers/ata/ata_piix.c
@@ -993,11 +993,8 @@ static int piix_pci_device_suspend(struc
 {
 	struct ata_host *host = pci_get_drvdata(pdev);
 	unsigned long flags;
-	int rc = 0;
 
-	rc = ata_host_suspend(host, mesg);
-	if (rc)
-		return rc;
+	ata_host_suspend(host, mesg);
 
 	/* Some braindamaged ACPI suspend implementations expect the
 	 * controller to be awake on entry; otherwise, it burns cpu
Index: libata/drivers/ata/libata-core.c
===================================================================
--- libata.orig/drivers/ata/libata-core.c
+++ libata/drivers/ata/libata-core.c
@@ -5168,6 +5168,8 @@ EXPORT_SYMBOL_GPL(ata_sas_port_resume);
  *	@host: host to suspend
  *	@mesg: PM message
  *
+ *	Return: always 0
+ *
  *	Suspend @host.  Actual operation is performed by port suspend.
  */
 int ata_host_suspend(struct ata_host *host, pm_message_t mesg)
@@ -6090,11 +6092,8 @@ EXPORT_SYMBOL_GPL(ata_pci_device_do_resu
 int ata_pci_device_suspend(struct pci_dev *pdev, pm_message_t mesg)
 {
 	struct ata_host *host = pci_get_drvdata(pdev);
-	int rc = 0;
 
-	rc = ata_host_suspend(host, mesg);
-	if (rc)
-		return rc;
+	ata_host_suspend(host, mesg);
 
 	ata_pci_device_do_suspend(pdev, mesg);
 
Index: libata/drivers/ata/pata_cs5520.c
===================================================================
--- libata.orig/drivers/ata/pata_cs5520.c
+++ libata/drivers/ata/pata_cs5520.c
@@ -259,11 +259,8 @@ static int cs5520_reinit_one(struct pci_
 static int cs5520_pci_device_suspend(struct pci_dev *pdev, pm_message_t mesg)
 {
 	struct ata_host *host = pci_get_drvdata(pdev);
-	int rc = 0;
 
-	rc = ata_host_suspend(host, mesg);
-	if (rc)
-		return rc;
+	ata_host_suspend(host, mesg);
 
 	pci_save_state(pdev);
 	return 0;
Index: libata/drivers/ata/pata_imx.c
===================================================================
--- libata.orig/drivers/ata/pata_imx.c
+++ libata/drivers/ata/pata_imx.c
@@ -223,17 +223,14 @@ static int pata_imx_suspend(struct devic
 {
 	struct ata_host *host = dev_get_drvdata(dev);
 	struct pata_imx_priv *priv = host->private_data;
-	int ret;
 
-	ret = ata_host_suspend(host, PMSG_SUSPEND);
-	if (!ret) {
-		__raw_writel(0, priv->host_regs + PATA_IMX_ATA_INT_EN);
-		priv->ata_ctl =
-			__raw_readl(priv->host_regs + PATA_IMX_ATA_CONTROL);
-		clk_disable_unprepare(priv->clk);
-	}
+	ata_host_suspend(host, PMSG_SUSPEND);
 
-	return ret;
+	__raw_writel(0, priv->host_regs + PATA_IMX_ATA_INT_EN);
+	priv->ata_ctl = __raw_readl(priv->host_regs + PATA_IMX_ATA_CONTROL);
+	clk_disable_unprepare(priv->clk);
+
+	return 0;
 }
 
 static int pata_imx_resume(struct device *dev)
Index: libata/drivers/ata/pata_macio.c
===================================================================
--- libata.orig/drivers/ata/pata_macio.c
+++ libata/drivers/ata/pata_macio.c
@@ -853,12 +853,8 @@ static int pata_macio_slave_config(struc
 #ifdef CONFIG_PM_SLEEP
 static int pata_macio_do_suspend(struct pata_macio_priv *priv, pm_message_t mesg)
 {
-	int rc;
-
 	/* First, core libata suspend to do most of the work */
-	rc = ata_host_suspend(priv->host, mesg);
-	if (rc)
-		return rc;
+	ata_host_suspend(priv->host, mesg);
 
 	/* Restore to default timings */
 	pata_macio_default_timings(priv);
Index: libata/drivers/ata/pata_triflex.c
===================================================================
--- libata.orig/drivers/ata/pata_triflex.c
+++ libata/drivers/ata/pata_triflex.c
@@ -198,11 +198,8 @@ static const struct pci_device_id trifle
 static int triflex_ata_pci_device_suspend(struct pci_dev *pdev, pm_message_t mesg)
 {
 	struct ata_host *host = pci_get_drvdata(pdev);
-	int rc = 0;
 
-	rc = ata_host_suspend(host, mesg);
-	if (rc)
-		return rc;
+	ata_host_suspend(host, mesg);
 
 	/*
 	 * We must not disable or powerdown the device.
Index: libata/drivers/ata/sata_mv.c
===================================================================
--- libata.orig/drivers/ata/sata_mv.c
+++ libata/drivers/ata/sata_mv.c
@@ -4235,10 +4235,10 @@ static int mv_platform_remove(struct pla
 static int mv_platform_suspend(struct platform_device *pdev, pm_message_t state)
 {
 	struct ata_host *host = platform_get_drvdata(pdev);
+
 	if (host)
-		return ata_host_suspend(host, state);
-	else
-		return 0;
+		ata_host_suspend(host, state);
+	return 0;
 }
 
 static int mv_platform_resume(struct platform_device *pdev)
Index: libata/drivers/ata/sata_rcar.c
===================================================================
--- libata.orig/drivers/ata/sata_rcar.c
+++ libata/drivers/ata/sata_rcar.c
@@ -945,19 +945,17 @@ static int sata_rcar_suspend(struct devi
 	struct ata_host *host = dev_get_drvdata(dev);
 	struct sata_rcar_priv *priv = host->private_data;
 	void __iomem *base = priv->base;
-	int ret;
 
-	ret = ata_host_suspend(host, PMSG_SUSPEND);
-	if (!ret) {
-		/* disable interrupts */
-		iowrite32(0, base + ATAPI_INT_ENABLE_REG);
-		/* mask */
-		iowrite32(priv->sataint_mask, base + SATAINTMASK_REG);
+	ata_host_suspend(host, PMSG_SUSPEND);
 
-		pm_runtime_put(dev);
-	}
+	/* disable interrupts */
+	iowrite32(0, base + ATAPI_INT_ENABLE_REG);
+	/* mask */
+	iowrite32(priv->sataint_mask, base + SATAINTMASK_REG);
 
-	return ret;
+	pm_runtime_put(dev);
+
+	return 0;
 }
 
 static int sata_rcar_resume(struct device *dev)

^ permalink raw reply	[flat|nested] 6+ messages in thread
* [PATCH] libata: ata_host_suspend() always returns 0
@ 2022-01-31 19:57 ` Sergey Shtylyov
  0 siblings, 0 replies; 6+ messages in thread
From: Sergey Shtylyov @ 2022-01-31 19:57 UTC (permalink / raw)
  To: Damien Le Moal, linux-ide
  Cc: Shawn Guo, Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam,
	NXP Linux Team, linux-arm-kernel

ata_host_suspend() always returns 0, so the result checks in the drivers
are pointless.  However I chose not to change ata_host_suspend() to *void*
not to have to change

	return ata_host_suspend(...);
to
	ata_host_suspend(...);
	return 0;

Found by Linux Verification Center (linuxtesting.org) with the SVACE static
analysis tool.

Signed-off-by: Sergey Shtylyov <s.shtylyov@omp.ru>

---
This patch is against the 'for-next' branch of Damien Le Moal's 'libata.git'
repo.

 drivers/ata/ata_piix.c     |    5 +----
 drivers/ata/libata-core.c  |    7 +++----
 drivers/ata/pata_cs5520.c  |    5 +----
 drivers/ata/pata_imx.c     |   15 ++++++---------
 drivers/ata/pata_macio.c   |    6 +-----
 drivers/ata/pata_triflex.c |    5 +----
 drivers/ata/sata_mv.c      |    6 +++---
 drivers/ata/sata_rcar.c    |   18 ++++++++----------
 8 files changed, 24 insertions(+), 43 deletions(-)

Index: libata/drivers/ata/ata_piix.c
===================================================================
--- libata.orig/drivers/ata/ata_piix.c
+++ libata/drivers/ata/ata_piix.c
@@ -993,11 +993,8 @@ static int piix_pci_device_suspend(struc
 {
 	struct ata_host *host = pci_get_drvdata(pdev);
 	unsigned long flags;
-	int rc = 0;
 
-	rc = ata_host_suspend(host, mesg);
-	if (rc)
-		return rc;
+	ata_host_suspend(host, mesg);
 
 	/* Some braindamaged ACPI suspend implementations expect the
 	 * controller to be awake on entry; otherwise, it burns cpu
Index: libata/drivers/ata/libata-core.c
===================================================================
--- libata.orig/drivers/ata/libata-core.c
+++ libata/drivers/ata/libata-core.c
@@ -5168,6 +5168,8 @@ EXPORT_SYMBOL_GPL(ata_sas_port_resume);
  *	@host: host to suspend
  *	@mesg: PM message
  *
+ *	Return: always 0
+ *
  *	Suspend @host.  Actual operation is performed by port suspend.
  */
 int ata_host_suspend(struct ata_host *host, pm_message_t mesg)
@@ -6090,11 +6092,8 @@ EXPORT_SYMBOL_GPL(ata_pci_device_do_resu
 int ata_pci_device_suspend(struct pci_dev *pdev, pm_message_t mesg)
 {
 	struct ata_host *host = pci_get_drvdata(pdev);
-	int rc = 0;
 
-	rc = ata_host_suspend(host, mesg);
-	if (rc)
-		return rc;
+	ata_host_suspend(host, mesg);
 
 	ata_pci_device_do_suspend(pdev, mesg);
 
Index: libata/drivers/ata/pata_cs5520.c
===================================================================
--- libata.orig/drivers/ata/pata_cs5520.c
+++ libata/drivers/ata/pata_cs5520.c
@@ -259,11 +259,8 @@ static int cs5520_reinit_one(struct pci_
 static int cs5520_pci_device_suspend(struct pci_dev *pdev, pm_message_t mesg)
 {
 	struct ata_host *host = pci_get_drvdata(pdev);
-	int rc = 0;
 
-	rc = ata_host_suspend(host, mesg);
-	if (rc)
-		return rc;
+	ata_host_suspend(host, mesg);
 
 	pci_save_state(pdev);
 	return 0;
Index: libata/drivers/ata/pata_imx.c
===================================================================
--- libata.orig/drivers/ata/pata_imx.c
+++ libata/drivers/ata/pata_imx.c
@@ -223,17 +223,14 @@ static int pata_imx_suspend(struct devic
 {
 	struct ata_host *host = dev_get_drvdata(dev);
 	struct pata_imx_priv *priv = host->private_data;
-	int ret;
 
-	ret = ata_host_suspend(host, PMSG_SUSPEND);
-	if (!ret) {
-		__raw_writel(0, priv->host_regs + PATA_IMX_ATA_INT_EN);
-		priv->ata_ctl =
-			__raw_readl(priv->host_regs + PATA_IMX_ATA_CONTROL);
-		clk_disable_unprepare(priv->clk);
-	}
+	ata_host_suspend(host, PMSG_SUSPEND);
 
-	return ret;
+	__raw_writel(0, priv->host_regs + PATA_IMX_ATA_INT_EN);
+	priv->ata_ctl = __raw_readl(priv->host_regs + PATA_IMX_ATA_CONTROL);
+	clk_disable_unprepare(priv->clk);
+
+	return 0;
 }
 
 static int pata_imx_resume(struct device *dev)
Index: libata/drivers/ata/pata_macio.c
===================================================================
--- libata.orig/drivers/ata/pata_macio.c
+++ libata/drivers/ata/pata_macio.c
@@ -853,12 +853,8 @@ static int pata_macio_slave_config(struc
 #ifdef CONFIG_PM_SLEEP
 static int pata_macio_do_suspend(struct pata_macio_priv *priv, pm_message_t mesg)
 {
-	int rc;
-
 	/* First, core libata suspend to do most of the work */
-	rc = ata_host_suspend(priv->host, mesg);
-	if (rc)
-		return rc;
+	ata_host_suspend(priv->host, mesg);
 
 	/* Restore to default timings */
 	pata_macio_default_timings(priv);
Index: libata/drivers/ata/pata_triflex.c
===================================================================
--- libata.orig/drivers/ata/pata_triflex.c
+++ libata/drivers/ata/pata_triflex.c
@@ -198,11 +198,8 @@ static const struct pci_device_id trifle
 static int triflex_ata_pci_device_suspend(struct pci_dev *pdev, pm_message_t mesg)
 {
 	struct ata_host *host = pci_get_drvdata(pdev);
-	int rc = 0;
 
-	rc = ata_host_suspend(host, mesg);
-	if (rc)
-		return rc;
+	ata_host_suspend(host, mesg);
 
 	/*
 	 * We must not disable or powerdown the device.
Index: libata/drivers/ata/sata_mv.c
===================================================================
--- libata.orig/drivers/ata/sata_mv.c
+++ libata/drivers/ata/sata_mv.c
@@ -4235,10 +4235,10 @@ static int mv_platform_remove(struct pla
 static int mv_platform_suspend(struct platform_device *pdev, pm_message_t state)
 {
 	struct ata_host *host = platform_get_drvdata(pdev);
+
 	if (host)
-		return ata_host_suspend(host, state);
-	else
-		return 0;
+		ata_host_suspend(host, state);
+	return 0;
 }
 
 static int mv_platform_resume(struct platform_device *pdev)
Index: libata/drivers/ata/sata_rcar.c
===================================================================
--- libata.orig/drivers/ata/sata_rcar.c
+++ libata/drivers/ata/sata_rcar.c
@@ -945,19 +945,17 @@ static int sata_rcar_suspend(struct devi
 	struct ata_host *host = dev_get_drvdata(dev);
 	struct sata_rcar_priv *priv = host->private_data;
 	void __iomem *base = priv->base;
-	int ret;
 
-	ret = ata_host_suspend(host, PMSG_SUSPEND);
-	if (!ret) {
-		/* disable interrupts */
-		iowrite32(0, base + ATAPI_INT_ENABLE_REG);
-		/* mask */
-		iowrite32(priv->sataint_mask, base + SATAINTMASK_REG);
+	ata_host_suspend(host, PMSG_SUSPEND);
 
-		pm_runtime_put(dev);
-	}
+	/* disable interrupts */
+	iowrite32(0, base + ATAPI_INT_ENABLE_REG);
+	/* mask */
+	iowrite32(priv->sataint_mask, base + SATAINTMASK_REG);
 
-	return ret;
+	pm_runtime_put(dev);
+
+	return 0;
 }
 
 static int sata_rcar_resume(struct device *dev)

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] libata: ata_host_suspend() always returns 0
  2022-01-31 19:57 ` Sergey Shtylyov
@ 2022-02-02  7:47   ` Damien Le Moal
  -1 siblings, 0 replies; 6+ messages in thread
From: Damien Le Moal @ 2022-02-02  7:47 UTC (permalink / raw)
  To: Sergey Shtylyov, linux-ide
  Cc: Shawn Guo, Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam,
	NXP Linux Team, linux-arm-kernel

On 2/1/22 04:57, Sergey Shtylyov wrote:
> ata_host_suspend() always returns 0, so the result checks in the drivers
> are pointless.  However I chose not to change ata_host_suspend() to *void*
> not to have to change
> 
> 	return ata_host_suspend(...);
> to
> 	ata_host_suspend(...);
> 	return 0;

Nice cleanup, I like it. But given how simple ata_host_suspend() is, I
would prefer that it is turned into a void function...

My view is: if the function returns an int error code, it should be
checked, always. If said function always return "success", then it
should be void. This makes it clear for the users (the different
drivers) how the function should be used.

With your change, anybody looking at the driver code and at
ata_host_suspend() interface in the header file may think "the error
check is missing", unless the person also look at the function code...

So let's simplify and make ata_host_suspend() a void function. There are
not that many call sites to change. Not all users do "return
ata_host_suspend()".

> 
> Found by Linux Verification Center (linuxtesting.org) with the SVACE static
> analysis tool.
> 
> Signed-off-by: Sergey Shtylyov <s.shtylyov@omp.ru>
> 
> ---
> This patch is against the 'for-next' branch of Damien Le Moal's 'libata.git'
> repo.
> 
>  drivers/ata/ata_piix.c     |    5 +----
>  drivers/ata/libata-core.c  |    7 +++----
>  drivers/ata/pata_cs5520.c  |    5 +----
>  drivers/ata/pata_imx.c     |   15 ++++++---------
>  drivers/ata/pata_macio.c   |    6 +-----
>  drivers/ata/pata_triflex.c |    5 +----
>  drivers/ata/sata_mv.c      |    6 +++---
>  drivers/ata/sata_rcar.c    |   18 ++++++++----------
>  8 files changed, 24 insertions(+), 43 deletions(-)
> 
> Index: libata/drivers/ata/ata_piix.c
> ===================================================================
> --- libata.orig/drivers/ata/ata_piix.c
> +++ libata/drivers/ata/ata_piix.c
> @@ -993,11 +993,8 @@ static int piix_pci_device_suspend(struc
>  {
>  	struct ata_host *host = pci_get_drvdata(pdev);
>  	unsigned long flags;
> -	int rc = 0;
>  
> -	rc = ata_host_suspend(host, mesg);
> -	if (rc)
> -		return rc;
> +	ata_host_suspend(host, mesg);
>  
>  	/* Some braindamaged ACPI suspend implementations expect the
>  	 * controller to be awake on entry; otherwise, it burns cpu
> Index: libata/drivers/ata/libata-core.c
> ===================================================================
> --- libata.orig/drivers/ata/libata-core.c
> +++ libata/drivers/ata/libata-core.c
> @@ -5168,6 +5168,8 @@ EXPORT_SYMBOL_GPL(ata_sas_port_resume);
>   *	@host: host to suspend
>   *	@mesg: PM message
>   *
> + *	Return: always 0
> + *
>   *	Suspend @host.  Actual operation is performed by port suspend.
>   */
>  int ata_host_suspend(struct ata_host *host, pm_message_t mesg)
> @@ -6090,11 +6092,8 @@ EXPORT_SYMBOL_GPL(ata_pci_device_do_resu
>  int ata_pci_device_suspend(struct pci_dev *pdev, pm_message_t mesg)
>  {
>  	struct ata_host *host = pci_get_drvdata(pdev);
> -	int rc = 0;
>  
> -	rc = ata_host_suspend(host, mesg);
> -	if (rc)
> -		return rc;
> +	ata_host_suspend(host, mesg);
>  
>  	ata_pci_device_do_suspend(pdev, mesg);
>  
> Index: libata/drivers/ata/pata_cs5520.c
> ===================================================================
> --- libata.orig/drivers/ata/pata_cs5520.c
> +++ libata/drivers/ata/pata_cs5520.c
> @@ -259,11 +259,8 @@ static int cs5520_reinit_one(struct pci_
>  static int cs5520_pci_device_suspend(struct pci_dev *pdev, pm_message_t mesg)
>  {
>  	struct ata_host *host = pci_get_drvdata(pdev);
> -	int rc = 0;
>  
> -	rc = ata_host_suspend(host, mesg);
> -	if (rc)
> -		return rc;
> +	ata_host_suspend(host, mesg);
>  
>  	pci_save_state(pdev);
>  	return 0;
> Index: libata/drivers/ata/pata_imx.c
> ===================================================================
> --- libata.orig/drivers/ata/pata_imx.c
> +++ libata/drivers/ata/pata_imx.c
> @@ -223,17 +223,14 @@ static int pata_imx_suspend(struct devic
>  {
>  	struct ata_host *host = dev_get_drvdata(dev);
>  	struct pata_imx_priv *priv = host->private_data;
> -	int ret;
>  
> -	ret = ata_host_suspend(host, PMSG_SUSPEND);
> -	if (!ret) {
> -		__raw_writel(0, priv->host_regs + PATA_IMX_ATA_INT_EN);
> -		priv->ata_ctl =
> -			__raw_readl(priv->host_regs + PATA_IMX_ATA_CONTROL);
> -		clk_disable_unprepare(priv->clk);
> -	}
> +	ata_host_suspend(host, PMSG_SUSPEND);
>  
> -	return ret;
> +	__raw_writel(0, priv->host_regs + PATA_IMX_ATA_INT_EN);
> +	priv->ata_ctl = __raw_readl(priv->host_regs + PATA_IMX_ATA_CONTROL);
> +	clk_disable_unprepare(priv->clk);
> +
> +	return 0;
>  }
>  
>  static int pata_imx_resume(struct device *dev)
> Index: libata/drivers/ata/pata_macio.c
> ===================================================================
> --- libata.orig/drivers/ata/pata_macio.c
> +++ libata/drivers/ata/pata_macio.c
> @@ -853,12 +853,8 @@ static int pata_macio_slave_config(struc
>  #ifdef CONFIG_PM_SLEEP
>  static int pata_macio_do_suspend(struct pata_macio_priv *priv, pm_message_t mesg)
>  {
> -	int rc;
> -
>  	/* First, core libata suspend to do most of the work */
> -	rc = ata_host_suspend(priv->host, mesg);
> -	if (rc)
> -		return rc;
> +	ata_host_suspend(priv->host, mesg);
>  
>  	/* Restore to default timings */
>  	pata_macio_default_timings(priv);
> Index: libata/drivers/ata/pata_triflex.c
> ===================================================================
> --- libata.orig/drivers/ata/pata_triflex.c
> +++ libata/drivers/ata/pata_triflex.c
> @@ -198,11 +198,8 @@ static const struct pci_device_id trifle
>  static int triflex_ata_pci_device_suspend(struct pci_dev *pdev, pm_message_t mesg)
>  {
>  	struct ata_host *host = pci_get_drvdata(pdev);
> -	int rc = 0;
>  
> -	rc = ata_host_suspend(host, mesg);
> -	if (rc)
> -		return rc;
> +	ata_host_suspend(host, mesg);
>  
>  	/*
>  	 * We must not disable or powerdown the device.
> Index: libata/drivers/ata/sata_mv.c
> ===================================================================
> --- libata.orig/drivers/ata/sata_mv.c
> +++ libata/drivers/ata/sata_mv.c
> @@ -4235,10 +4235,10 @@ static int mv_platform_remove(struct pla
>  static int mv_platform_suspend(struct platform_device *pdev, pm_message_t state)
>  {
>  	struct ata_host *host = platform_get_drvdata(pdev);
> +
>  	if (host)
> -		return ata_host_suspend(host, state);
> -	else
> -		return 0;
> +		ata_host_suspend(host, state);
> +	return 0;
>  }
>  
>  static int mv_platform_resume(struct platform_device *pdev)
> Index: libata/drivers/ata/sata_rcar.c
> ===================================================================
> --- libata.orig/drivers/ata/sata_rcar.c
> +++ libata/drivers/ata/sata_rcar.c
> @@ -945,19 +945,17 @@ static int sata_rcar_suspend(struct devi
>  	struct ata_host *host = dev_get_drvdata(dev);
>  	struct sata_rcar_priv *priv = host->private_data;
>  	void __iomem *base = priv->base;
> -	int ret;
>  
> -	ret = ata_host_suspend(host, PMSG_SUSPEND);
> -	if (!ret) {
> -		/* disable interrupts */
> -		iowrite32(0, base + ATAPI_INT_ENABLE_REG);
> -		/* mask */
> -		iowrite32(priv->sataint_mask, base + SATAINTMASK_REG);
> +	ata_host_suspend(host, PMSG_SUSPEND);
>  
> -		pm_runtime_put(dev);
> -	}
> +	/* disable interrupts */
> +	iowrite32(0, base + ATAPI_INT_ENABLE_REG);
> +	/* mask */
> +	iowrite32(priv->sataint_mask, base + SATAINTMASK_REG);
>  
> -	return ret;
> +	pm_runtime_put(dev);
> +
> +	return 0;
>  }
>  
>  static int sata_rcar_resume(struct device *dev)


-- 
Damien Le Moal
Western Digital Research

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] libata: ata_host_suspend() always returns 0
@ 2022-02-02  7:47   ` Damien Le Moal
  0 siblings, 0 replies; 6+ messages in thread
From: Damien Le Moal @ 2022-02-02  7:47 UTC (permalink / raw)
  To: Sergey Shtylyov, linux-ide
  Cc: Shawn Guo, Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam,
	NXP Linux Team, linux-arm-kernel

On 2/1/22 04:57, Sergey Shtylyov wrote:
> ata_host_suspend() always returns 0, so the result checks in the drivers
> are pointless.  However I chose not to change ata_host_suspend() to *void*
> not to have to change
> 
> 	return ata_host_suspend(...);
> to
> 	ata_host_suspend(...);
> 	return 0;

Nice cleanup, I like it. But given how simple ata_host_suspend() is, I
would prefer that it is turned into a void function...

My view is: if the function returns an int error code, it should be
checked, always. If said function always return "success", then it
should be void. This makes it clear for the users (the different
drivers) how the function should be used.

With your change, anybody looking at the driver code and at
ata_host_suspend() interface in the header file may think "the error
check is missing", unless the person also look at the function code...

So let's simplify and make ata_host_suspend() a void function. There are
not that many call sites to change. Not all users do "return
ata_host_suspend()".

> 
> Found by Linux Verification Center (linuxtesting.org) with the SVACE static
> analysis tool.
> 
> Signed-off-by: Sergey Shtylyov <s.shtylyov@omp.ru>
> 
> ---
> This patch is against the 'for-next' branch of Damien Le Moal's 'libata.git'
> repo.
> 
>  drivers/ata/ata_piix.c     |    5 +----
>  drivers/ata/libata-core.c  |    7 +++----
>  drivers/ata/pata_cs5520.c  |    5 +----
>  drivers/ata/pata_imx.c     |   15 ++++++---------
>  drivers/ata/pata_macio.c   |    6 +-----
>  drivers/ata/pata_triflex.c |    5 +----
>  drivers/ata/sata_mv.c      |    6 +++---
>  drivers/ata/sata_rcar.c    |   18 ++++++++----------
>  8 files changed, 24 insertions(+), 43 deletions(-)
> 
> Index: libata/drivers/ata/ata_piix.c
> ===================================================================
> --- libata.orig/drivers/ata/ata_piix.c
> +++ libata/drivers/ata/ata_piix.c
> @@ -993,11 +993,8 @@ static int piix_pci_device_suspend(struc
>  {
>  	struct ata_host *host = pci_get_drvdata(pdev);
>  	unsigned long flags;
> -	int rc = 0;
>  
> -	rc = ata_host_suspend(host, mesg);
> -	if (rc)
> -		return rc;
> +	ata_host_suspend(host, mesg);
>  
>  	/* Some braindamaged ACPI suspend implementations expect the
>  	 * controller to be awake on entry; otherwise, it burns cpu
> Index: libata/drivers/ata/libata-core.c
> ===================================================================
> --- libata.orig/drivers/ata/libata-core.c
> +++ libata/drivers/ata/libata-core.c
> @@ -5168,6 +5168,8 @@ EXPORT_SYMBOL_GPL(ata_sas_port_resume);
>   *	@host: host to suspend
>   *	@mesg: PM message
>   *
> + *	Return: always 0
> + *
>   *	Suspend @host.  Actual operation is performed by port suspend.
>   */
>  int ata_host_suspend(struct ata_host *host, pm_message_t mesg)
> @@ -6090,11 +6092,8 @@ EXPORT_SYMBOL_GPL(ata_pci_device_do_resu
>  int ata_pci_device_suspend(struct pci_dev *pdev, pm_message_t mesg)
>  {
>  	struct ata_host *host = pci_get_drvdata(pdev);
> -	int rc = 0;
>  
> -	rc = ata_host_suspend(host, mesg);
> -	if (rc)
> -		return rc;
> +	ata_host_suspend(host, mesg);
>  
>  	ata_pci_device_do_suspend(pdev, mesg);
>  
> Index: libata/drivers/ata/pata_cs5520.c
> ===================================================================
> --- libata.orig/drivers/ata/pata_cs5520.c
> +++ libata/drivers/ata/pata_cs5520.c
> @@ -259,11 +259,8 @@ static int cs5520_reinit_one(struct pci_
>  static int cs5520_pci_device_suspend(struct pci_dev *pdev, pm_message_t mesg)
>  {
>  	struct ata_host *host = pci_get_drvdata(pdev);
> -	int rc = 0;
>  
> -	rc = ata_host_suspend(host, mesg);
> -	if (rc)
> -		return rc;
> +	ata_host_suspend(host, mesg);
>  
>  	pci_save_state(pdev);
>  	return 0;
> Index: libata/drivers/ata/pata_imx.c
> ===================================================================
> --- libata.orig/drivers/ata/pata_imx.c
> +++ libata/drivers/ata/pata_imx.c
> @@ -223,17 +223,14 @@ static int pata_imx_suspend(struct devic
>  {
>  	struct ata_host *host = dev_get_drvdata(dev);
>  	struct pata_imx_priv *priv = host->private_data;
> -	int ret;
>  
> -	ret = ata_host_suspend(host, PMSG_SUSPEND);
> -	if (!ret) {
> -		__raw_writel(0, priv->host_regs + PATA_IMX_ATA_INT_EN);
> -		priv->ata_ctl =
> -			__raw_readl(priv->host_regs + PATA_IMX_ATA_CONTROL);
> -		clk_disable_unprepare(priv->clk);
> -	}
> +	ata_host_suspend(host, PMSG_SUSPEND);
>  
> -	return ret;
> +	__raw_writel(0, priv->host_regs + PATA_IMX_ATA_INT_EN);
> +	priv->ata_ctl = __raw_readl(priv->host_regs + PATA_IMX_ATA_CONTROL);
> +	clk_disable_unprepare(priv->clk);
> +
> +	return 0;
>  }
>  
>  static int pata_imx_resume(struct device *dev)
> Index: libata/drivers/ata/pata_macio.c
> ===================================================================
> --- libata.orig/drivers/ata/pata_macio.c
> +++ libata/drivers/ata/pata_macio.c
> @@ -853,12 +853,8 @@ static int pata_macio_slave_config(struc
>  #ifdef CONFIG_PM_SLEEP
>  static int pata_macio_do_suspend(struct pata_macio_priv *priv, pm_message_t mesg)
>  {
> -	int rc;
> -
>  	/* First, core libata suspend to do most of the work */
> -	rc = ata_host_suspend(priv->host, mesg);
> -	if (rc)
> -		return rc;
> +	ata_host_suspend(priv->host, mesg);
>  
>  	/* Restore to default timings */
>  	pata_macio_default_timings(priv);
> Index: libata/drivers/ata/pata_triflex.c
> ===================================================================
> --- libata.orig/drivers/ata/pata_triflex.c
> +++ libata/drivers/ata/pata_triflex.c
> @@ -198,11 +198,8 @@ static const struct pci_device_id trifle
>  static int triflex_ata_pci_device_suspend(struct pci_dev *pdev, pm_message_t mesg)
>  {
>  	struct ata_host *host = pci_get_drvdata(pdev);
> -	int rc = 0;
>  
> -	rc = ata_host_suspend(host, mesg);
> -	if (rc)
> -		return rc;
> +	ata_host_suspend(host, mesg);
>  
>  	/*
>  	 * We must not disable or powerdown the device.
> Index: libata/drivers/ata/sata_mv.c
> ===================================================================
> --- libata.orig/drivers/ata/sata_mv.c
> +++ libata/drivers/ata/sata_mv.c
> @@ -4235,10 +4235,10 @@ static int mv_platform_remove(struct pla
>  static int mv_platform_suspend(struct platform_device *pdev, pm_message_t state)
>  {
>  	struct ata_host *host = platform_get_drvdata(pdev);
> +
>  	if (host)
> -		return ata_host_suspend(host, state);
> -	else
> -		return 0;
> +		ata_host_suspend(host, state);
> +	return 0;
>  }
>  
>  static int mv_platform_resume(struct platform_device *pdev)
> Index: libata/drivers/ata/sata_rcar.c
> ===================================================================
> --- libata.orig/drivers/ata/sata_rcar.c
> +++ libata/drivers/ata/sata_rcar.c
> @@ -945,19 +945,17 @@ static int sata_rcar_suspend(struct devi
>  	struct ata_host *host = dev_get_drvdata(dev);
>  	struct sata_rcar_priv *priv = host->private_data;
>  	void __iomem *base = priv->base;
> -	int ret;
>  
> -	ret = ata_host_suspend(host, PMSG_SUSPEND);
> -	if (!ret) {
> -		/* disable interrupts */
> -		iowrite32(0, base + ATAPI_INT_ENABLE_REG);
> -		/* mask */
> -		iowrite32(priv->sataint_mask, base + SATAINTMASK_REG);
> +	ata_host_suspend(host, PMSG_SUSPEND);
>  
> -		pm_runtime_put(dev);
> -	}
> +	/* disable interrupts */
> +	iowrite32(0, base + ATAPI_INT_ENABLE_REG);
> +	/* mask */
> +	iowrite32(priv->sataint_mask, base + SATAINTMASK_REG);
>  
> -	return ret;
> +	pm_runtime_put(dev);
> +
> +	return 0;
>  }
>  
>  static int sata_rcar_resume(struct device *dev)


-- 
Damien Le Moal
Western Digital Research

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] libata: ata_host_suspend() always returns 0
  2022-02-02  7:47   ` Damien Le Moal
@ 2022-02-02 19:10     ` Sergey Shtylyov
  -1 siblings, 0 replies; 6+ messages in thread
From: Sergey Shtylyov @ 2022-02-02 19:10 UTC (permalink / raw)
  To: Damien Le Moal, linux-ide
  Cc: Shawn Guo, Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam,
	NXP Linux Team, linux-arm-kernel

On 2/2/22 10:47 AM, Damien Le Moal wrote:

>> ata_host_suspend() always returns 0, so the result checks in the drivers
>> are pointless.  However I chose not to change ata_host_suspend() to *void*
>> not to have to change
>>
>> 	return ata_host_suspend(...);
>> to
>> 	ata_host_suspend(...);
>> 	return 0;
> 
> Nice cleanup, I like it. But given how simple ata_host_suspend() is, I
> would prefer that it is turned into a void function...
> 
> My view is: if the function returns an int error code, it should be
> checked, always. If said function always return "success", then it
> should be void. This makes it clear for the users (the different
> drivers) how the function should be used.
> 
> With your change, anybody looking at the driver code and at
> ata_host_suspend() interface in the header file may think "the error
> check is missing", unless the person also look at the function code...

  This isseu largely eavdes me as I use Emacs' support of the TAGS file. :-)

> So let's simplify and make ata_host_suspend() a void function. There are
> not that many call sites to change. Not all users do "return
> ata_host_suspend()".

   OK, I've prepared 2 variants of this patch in anticipation of your reaction --
will just post the 2nd one. :-)

>> Found by Linux Verification Center (linuxtesting.org) with the SVACE static
>> analysis tool.
>>
>> Signed-off-by: Sergey Shtylyov <s.shtylyov@omp.ru>
[...]

MBR, Sergey

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] libata: ata_host_suspend() always returns 0
@ 2022-02-02 19:10     ` Sergey Shtylyov
  0 siblings, 0 replies; 6+ messages in thread
From: Sergey Shtylyov @ 2022-02-02 19:10 UTC (permalink / raw)
  To: Damien Le Moal, linux-ide
  Cc: Shawn Guo, Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam,
	NXP Linux Team, linux-arm-kernel

On 2/2/22 10:47 AM, Damien Le Moal wrote:

>> ata_host_suspend() always returns 0, so the result checks in the drivers
>> are pointless.  However I chose not to change ata_host_suspend() to *void*
>> not to have to change
>>
>> 	return ata_host_suspend(...);
>> to
>> 	ata_host_suspend(...);
>> 	return 0;
> 
> Nice cleanup, I like it. But given how simple ata_host_suspend() is, I
> would prefer that it is turned into a void function...
> 
> My view is: if the function returns an int error code, it should be
> checked, always. If said function always return "success", then it
> should be void. This makes it clear for the users (the different
> drivers) how the function should be used.
> 
> With your change, anybody looking at the driver code and at
> ata_host_suspend() interface in the header file may think "the error
> check is missing", unless the person also look at the function code...

  This isseu largely eavdes me as I use Emacs' support of the TAGS file. :-)

> So let's simplify and make ata_host_suspend() a void function. There are
> not that many call sites to change. Not all users do "return
> ata_host_suspend()".

   OK, I've prepared 2 variants of this patch in anticipation of your reaction --
will just post the 2nd one. :-)

>> Found by Linux Verification Center (linuxtesting.org) with the SVACE static
>> analysis tool.
>>
>> Signed-off-by: Sergey Shtylyov <s.shtylyov@omp.ru>
[...]

MBR, Sergey

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-02-02 19:12 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-01-31 19:57 [PATCH] libata: ata_host_suspend() always returns 0 Sergey Shtylyov
2022-01-31 19:57 ` Sergey Shtylyov
2022-02-02  7:47 ` Damien Le Moal
2022-02-02  7:47   ` Damien Le Moal
2022-02-02 19:10   ` Sergey Shtylyov
2022-02-02 19:10     ` Sergey Shtylyov

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.