[PATCH] ahci: qoriq: fixed using uninitialized variable warnings

[PATCH] ahci: qoriq: fixed using uninitialized variable warnings

[Yuantian.Tang]

From: Tang Yuantian <Yuantian.Tang@freescale.com>

kbuild test robot reports the warnings:
drivers/ata/ahci_qoriq.c: In function 'ahci_qoriq_hardreset':
>> include/asm-generic/io.h:163:2: warning: 'px_is' may be used
>> uninitialized in this function [-Wuninitialized]
drivers/ata/ahci_qoriq.c:70:14: note: 'px_is' was declared here
>> include/asm-generic/io.h:163:2: warning: 'px_cmd' may be used
>> uninitialized in this function [-Wuninitialized]
drivers/ata/ahci_qoriq.c:70:6: note: 'px_cmd' was declared here

This patch fixed it by assigning 0 to px_is and px_cmd variables.
This patch also remove line 'struct ccsr_ahci *reg_base' which is
not referred by any other codes and thus a dead one.

Signed-off-by: Tang Yuantian <Yuantian.Tang@freescale.com>
---
 drivers/ata/ahci_qoriq.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/ata/ahci_qoriq.c b/drivers/ata/ahci_qoriq.c
index e5e4988..f790852 100644
--- a/drivers/ata/ahci_qoriq.c
+++ b/drivers/ata/ahci_qoriq.c
@@ -49,7 +49,6 @@ enum ahci_qoriq_type {
 };
 
 struct ahci_qoriq_priv {
-	struct ccsr_ahci *reg_base;
 	enum ahci_qoriq_type type;
 	void __iomem *ecc_addr;
 };
@@ -67,7 +66,7 @@ static int ahci_qoriq_hardreset(struct ata_link *link, unsigned int *class,
 {
 	const unsigned long *timing = sata_ehc_deb_timing(&link->eh_context);
 	void __iomem *port_mmio = ahci_port_base(link->ap);
-	u32 px_cmd, px_is, px_val;
+	u32 px_cmd = 0, px_is = 0, px_val;
 	struct ata_port *ap = link->ap;
 	struct ahci_port_priv *pp = ap->private_data;
 	struct ahci_host_priv *hpriv = ap->host->private_data;
-- 
2.1.0.27.g96db324

Re: [PATCH] ahci: qoriq: fixed using uninitialized variable warnings

[Hans de Goede]

Hi,

On 09-09-15 11:16, Yuantian.Tang@freescale.com wrote:
> From: Tang Yuantian <Yuantian.Tang@freescale.com>
>
> kbuild test robot reports the warnings:
> drivers/ata/ahci_qoriq.c: In function 'ahci_qoriq_hardreset':
>>> include/asm-generic/io.h:163:2: warning: 'px_is' may be used
>>> uninitialized in this function [-Wuninitialized]
> drivers/ata/ahci_qoriq.c:70:14: note: 'px_is' was declared here
>>> include/asm-generic/io.h:163:2: warning: 'px_cmd' may be used
>>> uninitialized in this function [-Wuninitialized]
> drivers/ata/ahci_qoriq.c:70:6: note: 'px_cmd' was declared here
>
> This patch fixed it by assigning 0 to px_is and px_cmd variables.
> This patch also remove line 'struct ccsr_ahci *reg_base' which is
> not referred by any other codes and thus a dead one.
>
> Signed-off-by: Tang Yuantian <Yuantian.Tang@freescale.com>

LGTM: Acked-by: Hans de Goede <hdegoede@redhat.com>

Regards,

Hans


> ---
>   drivers/ata/ahci_qoriq.c | 3 +--
>   1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/ata/ahci_qoriq.c b/drivers/ata/ahci_qoriq.c
> index e5e4988..f790852 100644
> --- a/drivers/ata/ahci_qoriq.c
> +++ b/drivers/ata/ahci_qoriq.c
> @@ -49,7 +49,6 @@ enum ahci_qoriq_type {
>   };
>
>   struct ahci_qoriq_priv {
> -	struct ccsr_ahci *reg_base;
>   	enum ahci_qoriq_type type;
>   	void __iomem *ecc_addr;
>   };
> @@ -67,7 +66,7 @@ static int ahci_qoriq_hardreset(struct ata_link *link, unsigned int *class,
>   {
>   	const unsigned long *timing = sata_ehc_deb_timing(&link->eh_context);
>   	void __iomem *port_mmio = ahci_port_base(link->ap);
> -	u32 px_cmd, px_is, px_val;
> +	u32 px_cmd = 0, px_is = 0, px_val;
>   	struct ata_port *ap = link->ap;
>   	struct ahci_port_priv *pp = ap->private_data;
>   	struct ahci_host_priv *hpriv = ap->host->private_data;
>

Re: [PATCH] ahci: qoriq: fixed using uninitialized variable warnings

[Tejun Heo]

On Wed, Sep 09, 2015 at 05:16:22PM +0800, Yuantian.Tang@freescale.com wrote:
> From: Tang Yuantian <Yuantian.Tang@freescale.com>
> 
> kbuild test robot reports the warnings:
> drivers/ata/ahci_qoriq.c: In function 'ahci_qoriq_hardreset':
> >> include/asm-generic/io.h:163:2: warning: 'px_is' may be used
> >> uninitialized in this function [-Wuninitialized]
> drivers/ata/ahci_qoriq.c:70:14: note: 'px_is' was declared here
> >> include/asm-generic/io.h:163:2: warning: 'px_cmd' may be used
> >> uninitialized in this function [-Wuninitialized]
> drivers/ata/ahci_qoriq.c:70:6: note: 'px_cmd' was declared here
> 
> This patch fixed it by assigning 0 to px_is and px_cmd variables.
> This patch also remove line 'struct ccsr_ahci *reg_base' which is
> not referred by any other codes and thus a dead one.

Hmm... I think the problem here is that the complier can't know
whether qoriq_priv->type would change across intervening function
calls.  Maybe a better solution is caching the type in a local
variable so that the compiler can tell that those two tests will
always move together?  It generally isn't a good idea to clear
variables unnecessarily as that can hide actual bugs that compiler can
detect.

Thanks.

-- 
tejun

RE: [PATCH] ahci: qoriq: fixed using uninitialized variable warnings

[Yuantian Tang]

> -----Original Message-----
> From: Tejun Heo [mailto:htejun@gmail.com] On Behalf Of Tejun Heo
> Sent: Wednesday, September 09, 2015 10:02 PM
> To: Tang Yuantian-B29983 <Yuantian.Tang@freescale.com>
> Cc: hdegoede@redhat.com; linux-ide@vger.kernel.org; linux-
> kernel@vger.kernel.org; devicetree@vger.kernel.org
> Subject: Re: [PATCH] ahci: qoriq: fixed using uninitialized variable warnings
> 
> On Wed, Sep 09, 2015 at 05:16:22PM +0800, Yuantian.Tang@freescale.com
> wrote:
> > From: Tang Yuantian <Yuantian.Tang@freescale.com>
> >
> > kbuild test robot reports the warnings:
> > drivers/ata/ahci_qoriq.c: In function 'ahci_qoriq_hardreset':
> > >> include/asm-generic/io.h:163:2: warning: 'px_is' may be used
> > >> uninitialized in this function [-Wuninitialized]
> > drivers/ata/ahci_qoriq.c:70:14: note: 'px_is' was declared here
> > >> include/asm-generic/io.h:163:2: warning: 'px_cmd' may be used
> > >> uninitialized in this function [-Wuninitialized]
> > drivers/ata/ahci_qoriq.c:70:6: note: 'px_cmd' was declared here
> >
> > This patch fixed it by assigning 0 to px_is and px_cmd variables.
> > This patch also remove line 'struct ccsr_ahci *reg_base' which is not
> > referred by any other codes and thus a dead one.
> 
> Hmm... I think the problem here is that the complier can't know whether
> qoriq_priv->type would change across intervening function calls.  Maybe a
> better solution is caching the type in a local variable so that the compiler can
> tell that those two tests will always move together?  It generally isn't a good
> idea to clear variables unnecessarily as that can hide actual bugs that compiler
> can detect.
> 
I am not sure if the warning can be removed this way.
But I will send the patch as your suggestion. 
Unfortunately, I can't produce this warning no matter if I add -Wuninitialized.
So please let me know if the new patch is working.

Regards,
Yuantian

> Thanks.
> 
> --
> tejun

RE: [PATCH] ahci: qoriq: fixed using uninitialized variable warnings

[Yuantian Tang]

Hi Tejun,

Could you please take the version 1 patch?
The version 2 patch can't address these warnings, and the version 1 can definitely remove them.
In this case, that would cause any hidden bugs, so no worries.

Regards,
Yuantian

> -----Original Message-----
> From: Tejun Heo [mailto:htejun@gmail.com] On Behalf Of Tejun Heo
> Sent: Wednesday, September 09, 2015 10:02 PM
> To: Tang Yuantian-B29983 <Yuantian.Tang@freescale.com>
> Cc: hdegoede@redhat.com; linux-ide@vger.kernel.org; linux-
> kernel@vger.kernel.org; devicetree@vger.kernel.org
> Subject: Re: [PATCH] ahci: qoriq: fixed using uninitialized variable warnings
> 
> On Wed, Sep 09, 2015 at 05:16:22PM +0800, Yuantian.Tang@freescale.com
> wrote:
> > From: Tang Yuantian <Yuantian.Tang@freescale.com>
> >
> > kbuild test robot reports the warnings:
> > drivers/ata/ahci_qoriq.c: In function 'ahci_qoriq_hardreset':
> > >> include/asm-generic/io.h:163:2: warning: 'px_is' may be used
> > >> uninitialized in this function [-Wuninitialized]
> > drivers/ata/ahci_qoriq.c:70:14: note: 'px_is' was declared here
> > >> include/asm-generic/io.h:163:2: warning: 'px_cmd' may be used
> > >> uninitialized in this function [-Wuninitialized]
> > drivers/ata/ahci_qoriq.c:70:6: note: 'px_cmd' was declared here
> >
> > This patch fixed it by assigning 0 to px_is and px_cmd variables.
> > This patch also remove line 'struct ccsr_ahci *reg_base' which is not
> > referred by any other codes and thus a dead one.
> 
> Hmm... I think the problem here is that the complier can't know whether
> qoriq_priv->type would change across intervening function calls.  Maybe a
> better solution is caching the type in a local variable so that the compiler can
> tell that those two tests will always move together?  It generally isn't a good
> idea to clear variables unnecessarily as that can hide actual bugs that compiler
> can detect.
> 
> Thanks.
> 
> --
> tejun

Re: [PATCH] ahci: qoriq: fixed using uninitialized variable warnings

[Tejun Heo]

Hello, Yuantian.

On Fri, Sep 11, 2015 at 05:27:25AM +0000, Yuantian Tang wrote:
> Hi Tejun,
> 
> Could you please take the version 1 patch?
> The version 2 patch can't address these warnings, and the version 1 can definitely remove them.
> In this case, that would cause any hidden bugs, so no worries.

Ugh... I really hate changes which are made to just work around
compiler silliness.  If this is something which goes away with newer
gcc, Fengguang can just make it as a known false positive.  Yuantian,
which compiler are you using?

Thanks.

-- 
tejun

RE: [PATCH] ahci: qoriq: fixed using uninitialized variable warnings

[Yuantian Tang]

Hello Tejun,

The toolchain I used is:
gcc version 4.8.3 20140401 (prerelease) (Linaro GCC 4.8-2014.04)

I have not found this warning using this tool chain with -Wuninitialized flag.

Regards,
Yuantian 

> -----Original Message-----
> From: Tejun Heo [mailto:htejun@gmail.com] On Behalf Of Tejun Heo
> Sent: Friday, September 11, 2015 9:55 PM
> To: Tang Yuantian-B29983 <Yuantian.Tang@freescale.com>
> Cc: hdegoede@redhat.com; linux-ide@vger.kernel.org; linux-
> kernel@vger.kernel.org; devicetree@vger.kernel.org; Fengguang Wu
> <fengguang.wu@intel.com>
> Subject: Re: [PATCH] ahci: qoriq: fixed using uninitialized variable warnings
> 
> Hello, Yuantian.
> 
> On Fri, Sep 11, 2015 at 05:27:25AM +0000, Yuantian Tang wrote:
> > Hi Tejun,
> >
> > Could you please take the version 1 patch?
> > The version 2 patch can't address these warnings, and the version 1 can
> definitely remove them.
> > In this case, that would cause any hidden bugs, so no worries.
> 
> Ugh... I really hate changes which are made to just work around compiler
> silliness.  If this is something which goes away with newer gcc, Fengguang can
> just make it as a known false positive.  Yuantian, which compiler are you
> using?
> 
> Thanks.
> 
> --
> tejun

Re: [PATCH] ahci: qoriq: fixed using uninitialized variable warnings

[Fengguang Wu]

Yuantian,

It's cross compiling on ARCH=openrisc.

Thanks,
Fengguang

On Mon, Sep 14, 2015 at 03:02:27AM +0000, Yuantian Tang wrote:
> Hello Tejun,
> 
> The toolchain I used is:
> gcc version 4.8.3 20140401 (prerelease) (Linaro GCC 4.8-2014.04)
> 
> I have not found this warning using this tool chain with -Wuninitialized flag.
> 
> Regards,
> Yuantian 
> 
> > -----Original Message-----
> > From: Tejun Heo [mailto:htejun@gmail.com] On Behalf Of Tejun Heo
> > Sent: Friday, September 11, 2015 9:55 PM
> > To: Tang Yuantian-B29983 <Yuantian.Tang@freescale.com>
> > Cc: hdegoede@redhat.com; linux-ide@vger.kernel.org; linux-
> > kernel@vger.kernel.org; devicetree@vger.kernel.org; Fengguang Wu
> > <fengguang.wu@intel.com>
> > Subject: Re: [PATCH] ahci: qoriq: fixed using uninitialized variable warnings
> > 
> > Hello, Yuantian.
> > 
> > On Fri, Sep 11, 2015 at 05:27:25AM +0000, Yuantian Tang wrote:
> > > Hi Tejun,
> > >
> > > Could you please take the version 1 patch?
> > > The version 2 patch can't address these warnings, and the version 1 can
> > definitely remove them.
> > > In this case, that would cause any hidden bugs, so no worries.
> > 
> > Ugh... I really hate changes which are made to just work around compiler
> > silliness.  If this is something which goes away with newer gcc, Fengguang can
> > just make it as a known false positive.  Yuantian, which compiler are you
> > using?
> > 
> > Thanks.
> > 
> > --
> > tejun

RE: [PATCH] ahci: qoriq: fixed using uninitialized variable warnings

[Yuantian Tang]

The ARCH should have been ARM for this driver.
Do you think this warning would go away if adding a dependency on ARM?

Regards,
Yuantian

> -----Original Message-----
> From: Fengguang Wu [mailto:fengguang.wu@intel.com]
> Sent: Monday, September 14, 2015 12:05 PM
> To: Tang Yuantian-B29983 <Yuantian.Tang@freescale.com>
> Cc: Tejun Heo <tj@kernel.org>; hdegoede@redhat.com; linux-
> ide@vger.kernel.org; linux-kernel@vger.kernel.org;
> devicetree@vger.kernel.org
> Subject: Re: [PATCH] ahci: qoriq: fixed using uninitialized variable warnings
> 
> Yuantian,
> 
> It's cross compiling on ARCH=openrisc.
> 
> Thanks,
> Fengguang
> 
> On Mon, Sep 14, 2015 at 03:02:27AM +0000, Yuantian Tang wrote:
> > Hello Tejun,
> >
> > The toolchain I used is:
> > gcc version 4.8.3 20140401 (prerelease) (Linaro GCC 4.8-2014.04)
> >
> > I have not found this warning using this tool chain with -Wuninitialized flag.
> >
> > Regards,
> > Yuantian
> >
> > > -----Original Message-----
> > > From: Tejun Heo [mailto:htejun@gmail.com] On Behalf Of Tejun Heo
> > > Sent: Friday, September 11, 2015 9:55 PM
> > > To: Tang Yuantian-B29983 <Yuantian.Tang@freescale.com>
> > > Cc: hdegoede@redhat.com; linux-ide@vger.kernel.org; linux-
> > > kernel@vger.kernel.org; devicetree@vger.kernel.org; Fengguang Wu
> > > <fengguang.wu@intel.com>
> > > Subject: Re: [PATCH] ahci: qoriq: fixed using uninitialized variable
> > > warnings
> > >
> > > Hello, Yuantian.
> > >
> > > On Fri, Sep 11, 2015 at 05:27:25AM +0000, Yuantian Tang wrote:
> > > > Hi Tejun,
> > > >
> > > > Could you please take the version 1 patch?
> > > > The version 2 patch can't address these warnings, and the version
> > > > 1 can
> > > definitely remove them.
> > > > In this case, that would cause any hidden bugs, so no worries.
> > >
> > > Ugh... I really hate changes which are made to just work around
> > > compiler silliness.  If this is something which goes away with newer
> > > gcc, Fengguang can just make it as a known false positive.
> > > Yuantian, which compiler are you using?
> > >
> > > Thanks.
> > >
> > > --
> > > tejun

Re: [PATCH] ahci: qoriq: fixed using uninitialized variable warnings

[Fengguang Wu]

On Mon, Sep 14, 2015 at 06:51:36AM +0000, Yuantian Tang wrote:
> The ARCH should have been ARM for this driver.
> Do you think this warning would go away if adding a dependency on ARM?

Yes, that may work.

Thanks,
Fengguang

> > -----Original Message-----
> > From: Fengguang Wu [mailto:fengguang.wu@intel.com]
> > Sent: Monday, September 14, 2015 12:05 PM
> > To: Tang Yuantian-B29983 <Yuantian.Tang@freescale.com>
> > Cc: Tejun Heo <tj@kernel.org>; hdegoede@redhat.com; linux-
> > ide@vger.kernel.org; linux-kernel@vger.kernel.org;
> > devicetree@vger.kernel.org
> > Subject: Re: [PATCH] ahci: qoriq: fixed using uninitialized variable warnings
> > 
> > Yuantian,
> > 
> > It's cross compiling on ARCH=openrisc.
> > 
> > Thanks,
> > Fengguang
> > 
> > On Mon, Sep 14, 2015 at 03:02:27AM +0000, Yuantian Tang wrote:
> > > Hello Tejun,
> > >
> > > The toolchain I used is:
> > > gcc version 4.8.3 20140401 (prerelease) (Linaro GCC 4.8-2014.04)
> > >
> > > I have not found this warning using this tool chain with -Wuninitialized flag.
> > >
> > > Regards,
> > > Yuantian
> > >
> > > > -----Original Message-----
> > > > From: Tejun Heo [mailto:htejun@gmail.com] On Behalf Of Tejun Heo
> > > > Sent: Friday, September 11, 2015 9:55 PM
> > > > To: Tang Yuantian-B29983 <Yuantian.Tang@freescale.com>
> > > > Cc: hdegoede@redhat.com; linux-ide@vger.kernel.org; linux-
> > > > kernel@vger.kernel.org; devicetree@vger.kernel.org; Fengguang Wu
> > > > <fengguang.wu@intel.com>
> > > > Subject: Re: [PATCH] ahci: qoriq: fixed using uninitialized variable
> > > > warnings
> > > >
> > > > Hello, Yuantian.
> > > >
> > > > On Fri, Sep 11, 2015 at 05:27:25AM +0000, Yuantian Tang wrote:
> > > > > Hi Tejun,
> > > > >
> > > > > Could you please take the version 1 patch?
> > > > > The version 2 patch can't address these warnings, and the version
> > > > > 1 can
> > > > definitely remove them.
> > > > > In this case, that would cause any hidden bugs, so no worries.
> > > >
> > > > Ugh... I really hate changes which are made to just work around
> > > > compiler silliness.  If this is something which goes away with newer
> > > > gcc, Fengguang can just make it as a known false positive.
> > > > Yuantian, which compiler are you using?
> > > >
> > > > Thanks.
> > > >
> > > > --
> > > > tejun

Re: [PATCH] ahci: qoriq: fixed using uninitialized variable warnings

[Arnd Bergmann]

On Monday 14 September 2015 14:54:32 Fengguang Wu wrote:
> On Mon, Sep 14, 2015 at 06:51:36AM +0000, Yuantian Tang wrote:
> > The ARCH should have been ARM for this driver.
> > Do you think this warning would go away if adding a dependency on ARM?
> 
> Yes, that may work.

In general, we really want to leave drivers with a COMPILE_TEST dependency
so they at least get cross-built on x86, ideally on all architectures.

Does something like the below help? I think we really just need to help
gcc a little to see the obvious.

	Arnd

Signed-off-by: Arnd Bergmann <arnd@arndb.de>

diff --git a/drivers/ata/ahci_qoriq.c b/drivers/ata/ahci_qoriq.c
index e5e498812554..fc57208d8dcd 100644
--- a/drivers/ata/ahci_qoriq.c
+++ b/drivers/ata/ahci_qoriq.c
@@ -75,6 +75,7 @@ static int ahci_qoriq_hardreset(struct ata_link *link, unsigned int *class,
 	u8 *d2h_fis = pp->rx_fis + RX_FIS_D2H_REG;
 	struct ata_taskfile tf;
 	bool online;
+	bool ls101021a_workaround = (qoriq_priv->type == AHCI_LS1021A);
 	int rc;
 
 	DPRINTK("ENTER\n");
@@ -92,7 +93,7 @@ static int ahci_qoriq_hardreset(struct ata_link *link, unsigned int *class,
 	 * After the sequence is complete, software should restore the
 	 * PxCMD and PxIS with the stored values.
 	 */
-	if (qoriq_priv->type == AHCI_LS1021A) {
+	if (ls101021a_workaround) {
 		px_cmd = readl(port_mmio + PORT_CMD);
 		px_is = readl(port_mmio + PORT_IRQ_STAT);
 	}
@@ -106,7 +107,7 @@ static int ahci_qoriq_hardreset(struct ata_link *link, unsigned int *class,
 				 ahci_check_ready);
 
 	/* restore the PxCMD and PxIS on ls1021 */
-	if (qoriq_priv->type == AHCI_LS1021A) {
+	if (ls101021a_workaround) {
 		px_val = readl(port_mmio + PORT_CMD);
 		if (px_val != px_cmd)
 			writel(px_cmd, port_mmio + PORT_CMD);

Re: [PATCH] ahci: qoriq: fixed using uninitialized variable warnings

[Tejun Heo]

Hello, Arnd.

On Mon, Sep 14, 2015 at 09:45:33AM +0200, Arnd Bergmann wrote:
> In general, we really want to leave drivers with a COMPILE_TEST dependency
> so they at least get cross-built on x86, ideally on all architectures.

Yeah, as long as it doesn't trigger silly warnings or errors on x86,
COMPILE_TEST is usually a good idea.

> Does something like the below help? I think we really just need to help
> gcc a little to see the obvious.

We already tried cachine qoriq_priv->type in a local.  Not sure this
would make much difference.  It looks like the warning goes away with
newer gcc.  I'm inclined to just leave it as-is.

Thanks.

-- 
tejun