Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1221487 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH] ahci: qoriq: fixed using uninitialized variable warnings Tejun Heo <tj@kernel.org> - 2015-09-09 16:10 +0200
    RE: [PATCH] ahci: qoriq: fixed using uninitialized variable warnings Yuantian Tang <Yuantian.Tang@freescale.com> - 2015-09-10 08:20 +0200
    RE: [PATCH] ahci: qoriq: fixed using uninitialized variable warnings Yuantian Tang <Yuantian.Tang@freescale.com> - 2015-09-11 07:30 +0200
      Re: [PATCH] ahci: qoriq: fixed using uninitialized variable warnings Tejun Heo <tj@kernel.org> - 2015-09-11 16:00 +0200
        RE: [PATCH] ahci: qoriq: fixed using uninitialized variable warnings Yuantian Tang <Yuantian.Tang@freescale.com> - 2015-09-14 05:20 +0200
          Re: [PATCH] ahci: qoriq: fixed using uninitialized variable warnings Fengguang Wu <fengguang.wu@intel.com> - 2015-09-14 08:10 +0200
            RE: [PATCH] ahci: qoriq: fixed using uninitialized variable warnings Yuantian Tang <Yuantian.Tang@freescale.com> - 2015-09-14 09:00 +0200
              Re: [PATCH] ahci: qoriq: fixed using uninitialized variable warnings Fengguang Wu <fengguang.wu@intel.com> - 2015-09-14 09:00 +0200
                Re: [PATCH] ahci: qoriq: fixed using uninitialized variable warnings Arnd Bergmann <arnd@arndb.de> - 2015-09-14 09:50 +0200
                  Re: [PATCH] ahci: qoriq: fixed using uninitialized variable warnings Tejun Heo <tj@kernel.org> - 2015-09-14 18:10 +0200

#1221487 — 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1221942

> -----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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1222537

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1222860

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1223820

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1223840

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1223858

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1223864

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1223886

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);

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1224288

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web