| Message ID | 1438627595-17126-1-git-send-email-barletz@gmail.com |
|---|---|
| State | Not Applicable |
| Delegated to: | David Miller |
| Headers | show |
On 08/03/2015 09:46 PM, Tomer Barletz wrote: > The variable spd0 might be used uninitialized when pdc20621_i2c_read() > fails. > This also generates a compilation warning with gcc 5.1. > Signed-off-by: Tomer Barletz <barletz@gmail.com> > --- > drivers/ata/sata_sx4.c | 16 ++++++++++++---- > 1 file changed, 12 insertions(+), 4 deletions(-) > > diff --git a/drivers/ata/sata_sx4.c b/drivers/ata/sata_sx4.c > index 3a18a8a..e1c1423 100644 > --- a/drivers/ata/sata_sx4.c > +++ b/drivers/ata/sata_sx4.c > @@ -1238,8 +1238,12 @@ static unsigned int pdc20621_prog_dimm_global(struct ata_host *host) > readl(mmio + PDC_SDRAM_CONTROL); > > /* Turn on for ECC */ > - pdc20621_i2c_read(host, PDC_DIMM0_SPD_DEV_ADDRESS, > - PDC_DIMM_SPD_TYPE, &spd0); > + if (!pdc20621_i2c_read(host, PDC_DIMM0_SPD_DEV_ADDRESS, > + PDC_DIMM_SPD_TYPE, &spd0)) { That won't do, you didn't fix the indentation here. > + printk(KERN_ERR "Failed in i2c read: device=%#x, subaddr=%#x\n", > + PDC_DIMM0_SPD_DEV_ADDRESS, PDC_DIMM_SPD_TYPE); > + return 1; > + } > if (spd0 == 0x02) { > data |= (0x01 << 16); > writel(data, mmio + PDC_SDRAM_CONTROL); > @@ -1380,8 +1384,12 @@ static unsigned int pdc20621_dimm_init(struct ata_host *host) > > /* ECC initiliazation. */ > > - pdc20621_i2c_read(host, PDC_DIMM0_SPD_DEV_ADDRESS, > - PDC_DIMM_SPD_TYPE, &spd0); > + if (!pdc20621_i2c_read(host, PDC_DIMM0_SPD_DEV_ADDRESS, > + PDC_DIMM_SPD_TYPE, &spd0)) { And here. > + printk(KERN_ERR "Failed in i2c read: device=%#x, subaddr=%#x\n", > + PDC_DIMM0_SPD_DEV_ADDRESS, PDC_DIMM_SPD_TYPE); > + return 1; > + } > if (spd0 == 0x02) { > void *buf; > VPRINTK("Start ECC initialization\n"); MBR, Sergei -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
I see how it makes sense to add a tab to align with the previous line of code, as it will always look similar in all editors, no matter how their tab character is set up to be. However, adding more tabs will just mess up editors that are not set up with 8-space width tabs. Is this a bug in checkpatch.pl, or are we saying everyone should have their editor set to 8-spaces width tabs? --Tomer On Mon, Aug 3, 2015 at 11:52 AM, Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> wrote: > On 08/03/2015 09:46 PM, Tomer Barletz wrote: > >> The variable spd0 might be used uninitialized when pdc20621_i2c_read() >> fails. >> This also generates a compilation warning with gcc 5.1. > > >> Signed-off-by: Tomer Barletz <barletz@gmail.com> >> --- >> drivers/ata/sata_sx4.c | 16 ++++++++++++---- >> 1 file changed, 12 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/ata/sata_sx4.c b/drivers/ata/sata_sx4.c >> index 3a18a8a..e1c1423 100644 >> --- a/drivers/ata/sata_sx4.c >> +++ b/drivers/ata/sata_sx4.c >> @@ -1238,8 +1238,12 @@ static unsigned int >> pdc20621_prog_dimm_global(struct ata_host *host) >> readl(mmio + PDC_SDRAM_CONTROL); >> >> /* Turn on for ECC */ >> - pdc20621_i2c_read(host, PDC_DIMM0_SPD_DEV_ADDRESS, >> - PDC_DIMM_SPD_TYPE, &spd0); >> + if (!pdc20621_i2c_read(host, PDC_DIMM0_SPD_DEV_ADDRESS, >> + PDC_DIMM_SPD_TYPE, &spd0)) { > > > That won't do, you didn't fix the indentation here. > >> + printk(KERN_ERR "Failed in i2c read: device=%#x, >> subaddr=%#x\n", >> + PDC_DIMM0_SPD_DEV_ADDRESS, PDC_DIMM_SPD_TYPE); >> + return 1; >> + } >> if (spd0 == 0x02) { >> data |= (0x01 << 16); >> writel(data, mmio + PDC_SDRAM_CONTROL); >> @@ -1380,8 +1384,12 @@ static unsigned int pdc20621_dimm_init(struct >> ata_host *host) >> >> /* ECC initiliazation. */ >> >> - pdc20621_i2c_read(host, PDC_DIMM0_SPD_DEV_ADDRESS, >> - PDC_DIMM_SPD_TYPE, &spd0); >> + if (!pdc20621_i2c_read(host, PDC_DIMM0_SPD_DEV_ADDRESS, >> + PDC_DIMM_SPD_TYPE, &spd0)) { > > > And here. > >> + printk(KERN_ERR "Failed in i2c read: device=%#x, >> subaddr=%#x\n", >> + PDC_DIMM0_SPD_DEV_ADDRESS, PDC_DIMM_SPD_TYPE); >> + return 1; >> + } >> if (spd0 == 0x02) { >> void *buf; >> VPRINTK("Start ECC initialization\n"); > > > MBR, Sergei > -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 2015-08-03 at 12:04 -0700, Tomer Barletz wrote: > I see how it makes sense to add a tab to align with the previous line > of code, as it will always look similar in all editors, no matter how > their tab character is set up to be. > However, adding more tabs will just mess up editors that are not set > up with 8-space width tabs. > > Is this a bug in checkpatch.pl, or are we saying everyone should have > their editor set to 8-spaces width tabs? from Documentation/CodingStyle: Chapter 1: Indentation Tabs are 8 characters, and thus indentations are also 8 characters. There are heretic movements that try to make indentations 4 (or even 2!) characters deep, and that is akin to trying to define the value of PI to be 3. Rationale: The whole idea behind indentation is to clearly define where a block of control starts and ends. Especially when you've been looking at your screen for 20 straight hours, you'll find it a lot easier to see how the indentation works if you have large indentations. Now, some people will claim that having 8-character indentations makes the code move too far to the right, and makes it hard to read on a 80-character terminal screen. The answer to that is that if you need more than 3 levels of indentation, you're screwed anyway, and should fix your program. In short, 8-char indents make things easier to read, and have the added benefit of warning you when you're nesting your functions too deep. Heed that warning. -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 08/03/2015 10:04 PM, Tomer Barletz wrote:
Please don't top-post.
> I see how it makes sense to add a tab to align with the previous line
> of code, as it will always look similar in all editors, no matter how
> their tab character is set up to be.
> However, adding more tabs will just mess up editors that are not set
> up with 8-space width tabs.
> Is this a bug in checkpatch.pl, or are we saying everyone should have
> their editor set to 8-spaces width tabs?
The latter. :-)
MBR, Sergei
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/ata/sata_sx4.c b/drivers/ata/sata_sx4.c index 3a18a8a..e1c1423 100644 --- a/drivers/ata/sata_sx4.c +++ b/drivers/ata/sata_sx4.c @@ -1238,8 +1238,12 @@ static unsigned int pdc20621_prog_dimm_global(struct ata_host *host) readl(mmio + PDC_SDRAM_CONTROL); /* Turn on for ECC */ - pdc20621_i2c_read(host, PDC_DIMM0_SPD_DEV_ADDRESS, - PDC_DIMM_SPD_TYPE, &spd0); + if (!pdc20621_i2c_read(host, PDC_DIMM0_SPD_DEV_ADDRESS, + PDC_DIMM_SPD_TYPE, &spd0)) { + printk(KERN_ERR "Failed in i2c read: device=%#x, subaddr=%#x\n", + PDC_DIMM0_SPD_DEV_ADDRESS, PDC_DIMM_SPD_TYPE); + return 1; + } if (spd0 == 0x02) { data |= (0x01 << 16); writel(data, mmio + PDC_SDRAM_CONTROL); @@ -1380,8 +1384,12 @@ static unsigned int pdc20621_dimm_init(struct ata_host *host) /* ECC initiliazation. */ - pdc20621_i2c_read(host, PDC_DIMM0_SPD_DEV_ADDRESS, - PDC_DIMM_SPD_TYPE, &spd0); + if (!pdc20621_i2c_read(host, PDC_DIMM0_SPD_DEV_ADDRESS, + PDC_DIMM_SPD_TYPE, &spd0)) { + printk(KERN_ERR "Failed in i2c read: device=%#x, subaddr=%#x\n", + PDC_DIMM0_SPD_DEV_ADDRESS, PDC_DIMM_SPD_TYPE); + return 1; + } if (spd0 == 0x02) { void *buf; VPRINTK("Start ECC initialization\n");
The variable spd0 might be used uninitialized when pdc20621_i2c_read() fails. This also generates a compilation warning with gcc 5.1. Signed-off-by: Tomer Barletz <barletz@gmail.com> --- drivers/ata/sata_sx4.c | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-)