Initialisation bug in IDE patch

Initialisation bug in IDE patch

[Andrew Pam]

In the latest available kernel 2.2 IDE patch "ide-2.2.20.01102002.patch"
there is a bug that prevents ide_setup in drivers/block/ide.c
from accepting kernel parameters selecting special IDE hardware.

The ide_init_default_hwifs() function in include/asm-*/ide.h fails to
initialise the "hw_regs_t hw" variable, thus leaving uninitialised data
in some fields.  Specifically, the "chipset" field is uninitialised which
causes the "if (hwif->chipset != ide_unknown)" test in drivers/block/ide.c
to always fail with the error message " -- BAD OPTION".

Cheers,
	Andrew Pam

Patch follows:

diff -Nurd linux-2.2.20+ide-orig/include/asm-alpha/ide.h linux-2.2.20+ide/include/asm-alpha/ide.h
--- linux-2.2.20+ide-orig/include/asm-alpha/ide.h	Wed May 22 14:35:58 2002
+++ linux-2.2.20+ide/include/asm-alpha/ide.h	Wed May 22 14:47:15 2002
@@ -74,6 +74,11 @@
 	hw_regs_t hw;
 	int index;
 
+	/*
+	 * Initialise before use!
+	 * memset added 2002/05/22 by Andrew Pam <xanni@sericyb.com.au>
+	 */
+	memset(&hw, 0, sizeof(hw_regs_t));
 	for (index = 0; index < MAX_HWIFS; index++) {
 		ide_init_hwif_ports(&hw, ide_default_io_base(index), 0, NULL);
 		hw.irq = ide_default_irq(ide_default_io_base(index));
diff -Nurd linux-2.2.20+ide-orig/include/asm-i386/ide.h linux-2.2.20+ide/include/asm-i386/ide.h
--- linux-2.2.20+ide-orig/include/asm-i386/ide.h	Wed May 22 14:35:59 2002
+++ linux-2.2.20+ide/include/asm-i386/ide.h	Wed May 22 14:46:49 2002
@@ -77,6 +77,11 @@
 	hw_regs_t hw;
 	int index;
 
+	/*
+	 * Initialise before use!
+	 * memset added 2002/05/22 by Andrew Pam <xanni@sericyb.com.au>
+	 */
+	memset(&hw, 0, sizeof(hw_regs_t));
 	for(index = 0; index < MAX_HWIFS; index++) {
 		ide_init_hwif_ports(&hw, ide_default_io_base(index), 0, NULL);
 		hw.irq = ide_default_irq(ide_default_io_base(index));
diff -Nurd linux-2.2.20+ide-orig/include/asm-mips/ide.h linux-2.2.20+ide/include/asm-mips/ide.h
--- linux-2.2.20+ide-orig/include/asm-mips/ide.h	Wed May 22 14:36:00 2002
+++ linux-2.2.20+ide/include/asm-mips/ide.h	Wed May 22 14:48:54 2002
@@ -70,6 +70,11 @@
 	hw_regs_t hw;
 	int index;
 
+	/*
+	 * Initialise before use!
+	 * memset added 2002/05/22 by Andrew Pam <xanni@sericyb.com.au>
+	 */
+	memset(&hw, 0, sizeof(hw_regs_t));
 	for (index = 0; index < MAX_HWIFS; index++) {
 		ide_init_hwif_ports(&hw, ide_default_io_base(index), 0, 0);
 		hw.irq = ide_default_irq(ide_default_io_base(index));
diff -Nurd linux-2.2.20+ide-orig/include/asm-ppc/ide.h linux-2.2.20+ide/include/asm-ppc/ide.h
--- linux-2.2.20+ide-orig/include/asm-ppc/ide.h	Wed May 22 14:36:00 2002
+++ linux-2.2.20+ide/include/asm-ppc/ide.h	Wed May 22 14:49:10 2002
@@ -89,6 +89,11 @@
 	hw_regs_t hw;
 	int index;
 
+	/*
+	 * Initialise before use!
+	 * memset added 2002/05/22 by Andrew Pam <xanni@sericyb.com.au>
+	 */
+	memset(&hw, 0, sizeof(hw_regs_t));
 	for(index = 0; index < MAX_HWIFS; index++) {
 		base = ide_default_io_base(index);
 		if (base == 0)
diff -Nurd linux-2.2.20+ide-orig/include/asm-sparc64/ide.h linux-2.2.20+ide/include/asm-sparc64/ide.h
--- linux-2.2.20+ide-orig/include/asm-sparc64/ide.h	Wed May 22 14:36:00 2002
+++ linux-2.2.20+ide/include/asm-sparc64/ide.h	Wed May 22 14:49:14 2002
@@ -57,6 +57,11 @@
 	hw_regs_t hw;
 	int index;
 
+	/*
+	 * Initialise before use!
+	 * memset added 2002/05/22 by Andrew Pam <xanni@sericyb.com.au>
+	 */
+	memset(&hw, 0, sizeof(hw_regs_t));
 	for (index = 0; index < MAX_HWIFS; index++) {
 		ide_init_hwif_ports(&hw, ide_default_io_base(index), 0, 0);
 		hw.irq = ide_default_irq(ide_default_io_base(index));
-- 
mailto:xanni@xanadu.net                         Andrew Pam
http://www.xanadu.com.au/                       Chief Scientist, Xanadu
http://www.glasswings.com.au/                   Technology Manager, Glass Wings
http://www.sericyb.com.au/                      Manager, Serious Cybernetics
http://two-cents-worth.com/?105347&EG		Donate two cents to our work!
P.O. Box 477, Blackburn VIC 3130 Australia	Phone +61 401 258 915

Re: Initialisation bug in IDE patch

[Andrew Pam]

On Wed, May 22, 2002 at 03:15:10PM +1000, Andrew Pam wrote:
> In the latest available kernel 2.2 IDE patch "ide-2.2.20.01102002.patch"
> there is a bug that prevents ide_setup in drivers/block/ide.c
> from accepting kernel parameters selecting special IDE hardware.
> 
> The ide_init_default_hwifs() function in include/asm-*/ide.h fails to
> initialise the "hw_regs_t hw" variable, thus leaving uninitialised data
> in some fields.  Specifically, the "chipset" field is uninitialised which
> causes the "if (hwif->chipset != ide_unknown)" test in drivers/block/ide.c
> to always fail with the error message " -- BAD OPTION".

This bug also appears to be present in the mainline 2.4 kernel IDE code.
I haven't checked the 2.5 code yet.

Regards,
	Andrew Pam
-- 
mailto:xanni@xanadu.net                         Andrew Pam
http://www.xanadu.com.au/                       Chief Scientist, Xanadu
http://www.glasswings.com.au/                   Technology Manager, Glass Wings
http://www.sericyb.com.au/                      Manager, Serious Cybernetics
http://two-cents-worth.com/?105347&EG		Donate two cents to our work!
P.O. Box 477, Blackburn VIC 3130 Australia	Phone +61 401 258 915

Re: Initialisation bug in IDE patch

[Andre Hedrick]

Please check the function in the top of ide.c

linux-2.4.19-p7 ide.c

static void init_hwif_data (unsigned int index)
{
        unsigned int unit;
        hw_regs_t hw;
        ide_hwif_t *hwif = &ide_hwifs[index];

        /* bulk initialize hwif & drive info with zeros */
        memset(hwif, 0, sizeof(ide_hwif_t));
        memset(&hw, 0, sizeof(hw_regs_t));

        /* fill in any non-zero initial values */
        hwif->index     = index;
        ide_init_hwif_ports(&hw, ide_default_io_base(index), 0, &hwif->irq);
        memcpy(&hwif->hw, &hw, sizeof(hw));
        memcpy(hwif->io_ports, hw.io_ports, sizeof(hw.io_ports));
        hwif->noprobe   = !hwif->io_ports[IDE_DATA_OFFSET];

Obviously you are calling things outside of the normal usage of the
driver.  

2.2.19 ide.c

/*
 * Do not even *think* about calling this!
 */
static void init_hwif_data (unsigned int index)
{
        unsigned int unit;
        hw_regs_t hw;
        ide_hwif_t *hwif = &ide_hwifs[index];

        /* bulk initialize hwif & drive info with zeros */
        memset(hwif, 0, sizeof(ide_hwif_t));
        memset(&hw, 0, sizeof(hw_regs_t));

        /* fill in any non-zero initial values */
        hwif->index     = index;
        ide_init_hwif_ports(&hw, ide_default_io_base(index), 0, &hwif->irq);
        memcpy(&hwif->hw, &hw, sizeof(hw));
        memcpy(hwif->io_ports, hw.io_ports, sizeof(hw.io_ports));
        hwif->noprobe   = !hwif->io_ports[IDE_DATA_OFFSET];
#ifdef CONFIG_BLK_DEV_HD
        if (hwif->io_ports[IDE_DATA_OFFSET] == HD_DATA)

If you are attempting to introduce a hw.chipset which is unknown to the
driver please submit a patch to register it or the enum index will barf.

Cheers,

On Wed, 22 May 2002, Andrew Pam wrote:

> On Wed, May 22, 2002 at 03:15:10PM +1000, Andrew Pam wrote:
> > In the latest available kernel 2.2 IDE patch "ide-2.2.20.01102002.patch"
> > there is a bug that prevents ide_setup in drivers/block/ide.c
> > from accepting kernel parameters selecting special IDE hardware.
> > 
> > The ide_init_default_hwifs() function in include/asm-*/ide.h fails to
> > initialise the "hw_regs_t hw" variable, thus leaving uninitialised data
> > in some fields.  Specifically, the "chipset" field is uninitialised which
> > causes the "if (hwif->chipset != ide_unknown)" test in drivers/block/ide.c
> > to always fail with the error message " -- BAD OPTION".
> 
> This bug also appears to be present in the mainline 2.4 kernel IDE code.
> I haven't checked the 2.5 code yet.
> 
> Regards,
> 	Andrew Pam
> -- 
> mailto:xanni@xanadu.net                         Andrew Pam
> http://www.xanadu.com.au/                       Chief Scientist, Xanadu
> http://www.glasswings.com.au/                   Technology Manager, Glass Wings
> http://www.sericyb.com.au/                      Manager, Serious Cybernetics
> http://two-cents-worth.com/?105347&EG		Donate two cents to our work!
> P.O. Box 477, Blackburn VIC 3130 Australia	Phone +61 401 258 915
> 

Andre Hedrick
LAD Storage Consulting Group

Re: Initialisation bug in IDE patch

[Andrew Pam]

On Tue, May 21, 2002 at 11:00:45PM -0700, Andre Hedrick wrote:
> 
> Please check the function in the top of ide.c
> 
> linux-2.4.19-p7 ide.c
> 
> static void init_hwif_data (unsigned int index)
> {
>         unsigned int unit;
>         hw_regs_t hw;
>         ide_hwif_t *hwif = &ide_hwifs[index];
> 
>         /* bulk initialize hwif & drive info with zeros */
>         memset(hwif, 0, sizeof(ide_hwif_t));
>         memset(&hw, 0, sizeof(hw_regs_t));
> 
>         /* fill in any non-zero initial values */
>         hwif->index     = index;
>         ide_init_hwif_ports(&hw, ide_default_io_base(index), 0, &hwif->irq);
>         memcpy(&hwif->hw, &hw, sizeof(hw));
>         memcpy(hwif->io_ports, hw.io_ports, sizeof(hw.io_ports));
>         hwif->noprobe   = !hwif->io_ports[IDE_DATA_OFFSET];
> 
> Obviously you are calling things outside of the normal usage of the
> driver.  

No, this is the existing driver as shipped; I have not modified the code.
The init_hwif_data roiutine has not yet been called while the ide_setup()
routine is parsing the kernel parameters, hence the bug.

> 2.2.19 ide.c
> 
> /*
>  * Do not even *think* about calling this!
>  */
> static void init_hwif_data (unsigned int index)
> {
>         unsigned int unit;
>         hw_regs_t hw;
>         ide_hwif_t *hwif = &ide_hwifs[index];
> 
>         /* bulk initialize hwif & drive info with zeros */
>         memset(hwif, 0, sizeof(ide_hwif_t));
>         memset(&hw, 0, sizeof(hw_regs_t));
> 
>         /* fill in any non-zero initial values */
>         hwif->index     = index;
>         ide_init_hwif_ports(&hw, ide_default_io_base(index), 0, &hwif->irq);
>         memcpy(&hwif->hw, &hw, sizeof(hw));
>         memcpy(hwif->io_ports, hw.io_ports, sizeof(hw.io_ports));
>         hwif->noprobe   = !hwif->io_ports[IDE_DATA_OFFSET];
> #ifdef CONFIG_BLK_DEV_HD
>         if (hwif->io_ports[IDE_DATA_OFFSET] == HD_DATA)
> 
> If you are attempting to introduce a hw.chipset which is unknown to the
> driver please submit a patch to register it or the enum index will barf.

No, I have a qd6580 which is already supported.  This bug is in existing
kernel code; I am not adding any new code, but spent some hours debugging
the problem with printk statements.

Regards,
	Andrew Pam
-- 
mailto:xanni@xanadu.net                         Andrew Pam
http://www.xanadu.com.au/                       Chief Scientist, Xanadu
http://www.glasswings.com.au/                   Technology Manager, Glass Wings
http://www.sericyb.com.au/                      Manager, Serious Cybernetics
http://two-cents-worth.com/?105347&EG		Donate two cents to our work!
P.O. Box 477, Blackburn VIC 3130 Australia	Phone +61 401 258 915

Re: Initialisation bug in IDE patch

[Andre Hedrick]

from ide.c

 */
int __init ide_setup (char *s)
{
        int i, vals[3];
<snip>
        printk("ide_setup: %s", s);
        init_ide_data ();


#define MAGIC_COOKIE 0x12345678
static void __init init_ide_data (void)
{
        unsigned int index;
        static unsigned long magic_cookie = MAGIC_COOKIE;

        if (magic_cookie != MAGIC_COOKIE)
                return;         /* already initialized */
        magic_cookie = 0;

        /* Initialise all interface structures */
        for (index = 0; index < MAX_HWIFS; ++index)
                init_hwif_data(index);

        /* Add default hw interfaces */
        ide_init_default_hwifs();

        idebus_parameter = 0;
        system_bus_speed = 0;
}


Now you have me puzzled........
If "ide_setup" which parses the passed settings calls "init_ide_data"
which initalizes all "hwif" groups and sets a cookie to prevent "ide_init"
from re-initalizing thus purging the contents place in by "ide_setup", how
are you getting a "BAD -- OPTION"?

Would please post what you are attempting to pass ?

On Wed, 22 May 2002, Andrew Pam wrote:

> On Tue, May 21, 2002 at 11:00:45PM -0700, Andre Hedrick wrote:
> > 
> > Please check the function in the top of ide.c
> > 
> > linux-2.4.19-p7 ide.c
> > 
> > static void init_hwif_data (unsigned int index)
> > {
> >         unsigned int unit;
> >         hw_regs_t hw;
> >         ide_hwif_t *hwif = &ide_hwifs[index];
> > 
> >         /* bulk initialize hwif & drive info with zeros */
> >         memset(hwif, 0, sizeof(ide_hwif_t));
> >         memset(&hw, 0, sizeof(hw_regs_t));
> > 
> >         /* fill in any non-zero initial values */
> >         hwif->index     = index;
> >         ide_init_hwif_ports(&hw, ide_default_io_base(index), 0, &hwif->irq);
> >         memcpy(&hwif->hw, &hw, sizeof(hw));
> >         memcpy(hwif->io_ports, hw.io_ports, sizeof(hw.io_ports));
> >         hwif->noprobe   = !hwif->io_ports[IDE_DATA_OFFSET];
> > 
> > Obviously you are calling things outside of the normal usage of the
> > driver.  
> 
> No, this is the existing driver as shipped; I have not modified the code.
> The init_hwif_data roiutine has not yet been called while the ide_setup()
> routine is parsing the kernel parameters, hence the bug.
> 
> > 2.2.19 ide.c
> > 
> > /*
> >  * Do not even *think* about calling this!
> >  */
> > static void init_hwif_data (unsigned int index)
> > {
> >         unsigned int unit;
> >         hw_regs_t hw;
> >         ide_hwif_t *hwif = &ide_hwifs[index];
> > 
> >         /* bulk initialize hwif & drive info with zeros */
> >         memset(hwif, 0, sizeof(ide_hwif_t));
> >         memset(&hw, 0, sizeof(hw_regs_t));
> > 
> >         /* fill in any non-zero initial values */
> >         hwif->index     = index;
> >         ide_init_hwif_ports(&hw, ide_default_io_base(index), 0, &hwif->irq);
> >         memcpy(&hwif->hw, &hw, sizeof(hw));
> >         memcpy(hwif->io_ports, hw.io_ports, sizeof(hw.io_ports));
> >         hwif->noprobe   = !hwif->io_ports[IDE_DATA_OFFSET];
> > #ifdef CONFIG_BLK_DEV_HD
> >         if (hwif->io_ports[IDE_DATA_OFFSET] == HD_DATA)
> > 
> > If you are attempting to introduce a hw.chipset which is unknown to the
> > driver please submit a patch to register it or the enum index will barf.
> 
> No, I have a qd6580 which is already supported.  This bug is in existing
> kernel code; I am not adding any new code, but spent some hours debugging
> the problem with printk statements.
> 
> Regards,
> 	Andrew Pam
> -- 
> mailto:xanni@xanadu.net                         Andrew Pam
> http://www.xanadu.com.au/                       Chief Scientist, Xanadu
> http://www.glasswings.com.au/                   Technology Manager, Glass Wings
> http://www.sericyb.com.au/                      Manager, Serious Cybernetics
> http://two-cents-worth.com/?105347&EG		Donate two cents to our work!
> P.O. Box 477, Blackburn VIC 3130 Australia	Phone +61 401 258 915
> 

Andre Hedrick
LAD Storage Consulting Group

Re: Initialisation bug in IDE patch

[Andrew Pam]

On Tue, May 21, 2002 at 11:18:56PM -0700, Andre Hedrick wrote:
> Now you have me puzzled........
> If "ide_setup" which parses the passed settings calls "init_ide_data"
> which initalizes all "hwif" groups and sets a cookie to prevent "ide_init"
> from re-initalizing thus purging the contents place in by "ide_setup", how
> are you getting a "BAD -- OPTION"?
> 
> Would please post what you are attempting to pass ?

"ide0=qd6580"

Don't tell me, you're going to want me to put all my printk debugging
back in and show exactly where the data remains uninitialised...

Cheers,
	Andrew Pam
-- 
mailto:xanni@xanadu.net                         Andrew Pam
http://www.xanadu.com.au/                       Chief Scientist, Xanadu
http://www.glasswings.com.au/                   Technology Manager, Glass Wings
http://www.sericyb.com.au/                      Manager, Serious Cybernetics
http://two-cents-worth.com/?105347&EG		Donate two cents to our work!
P.O. Box 477, Blackburn VIC 3130 Australia	Phone +61 401 258 915

Re: Initialisation bug in IDE patch

[Andrew Pam]

On Tue, May 21, 2002 at 11:18:56PM -0700, Andre Hedrick wrote:
> Now you have me puzzled........
> If "ide_setup" which parses the passed settings calls "init_ide_data"
> which initalizes all "hwif" groups and sets a cookie to prevent "ide_init"
> from re-initalizing thus purging the contents place in by "ide_setup", how
> are you getting a "BAD -- OPTION"?

Off the top of my head, it looks like ide_init_default_hwifs creates
an uninitialised "hw_regs_t hw;" variable, fills in some fields, then
calls "ide_register_hw(&hw, NULL);" which does "memcpy(&hwif->hw, hw,
sizeof(*hw));" thus copying the unitialised fields (such as "chipset")
right over the area zeroed by init_ide_data().

Am I on the right track?

Cheers,
	Andrew Pam
-- 
mailto:xanni@xanadu.net                         Andrew Pam
http://www.xanadu.com.au/                       Chief Scientist, Xanadu
http://www.glasswings.com.au/                   Technology Manager, Glass Wings
http://www.sericyb.com.au/                      Manager, Serious Cybernetics
http://two-cents-worth.com/?105347&EG		Donate two cents to our work!
P.O. Box 477, Blackburn VIC 3130 Australia	Phone +61 401 258 915

Re: Initialisation bug in IDE patch

[Andre Hedrick]

First question for 2.2.X, in your .config is "CONFIG_BLK_DEV_QD6580" set ?
If not then ide0=qd6580 will yield "-- BAD OPTION"

Second question for 2.4.X in your .config is "CONFIG_BLK_DEV_QD65XX" set ?
If not then ide0=qd65xx will yield "-- BAD OPTION"

Please check those first.

Cheers,


On Wed, 22 May 2002, Andrew Pam wrote:

> On Tue, May 21, 2002 at 11:18:56PM -0700, Andre Hedrick wrote:
> > Now you have me puzzled........
> > If "ide_setup" which parses the passed settings calls "init_ide_data"
> > which initalizes all "hwif" groups and sets a cookie to prevent "ide_init"
> > from re-initalizing thus purging the contents place in by "ide_setup", how
> > are you getting a "BAD -- OPTION"?
> 
> Off the top of my head, it looks like ide_init_default_hwifs creates
> an uninitialised "hw_regs_t hw;" variable, fills in some fields, then
> calls "ide_register_hw(&hw, NULL);" which does "memcpy(&hwif->hw, hw,
> sizeof(*hw));" thus copying the unitialised fields (such as "chipset")
> right over the area zeroed by init_ide_data().
> 
> Am I on the right track?
> 
> Cheers,
> 	Andrew Pam
> -- 
> mailto:xanni@xanadu.net                         Andrew Pam
> http://www.xanadu.com.au/                       Chief Scientist, Xanadu
> http://www.glasswings.com.au/                   Technology Manager, Glass Wings
> http://www.sericyb.com.au/                      Manager, Serious Cybernetics
> http://two-cents-worth.com/?105347&EG		Donate two cents to our work!
> P.O. Box 477, Blackburn VIC 3130 Australia	Phone +61 401 258 915
> 

Andre Hedrick
LAD Storage Consulting Group

Re: Initialisation bug in IDE patch

[Andrew Pam]

On Tue, May 21, 2002 at 11:59:35PM -0700, Andre Hedrick wrote:
> 
> First question for 2.2.X, in your .config is "CONFIG_BLK_DEV_QD6580" set ?
> If not then ide0=qd6580 will yield "-- BAD OPTION"
> 
> Second question for 2.4.X in your .config is "CONFIG_BLK_DEV_QD65XX" set ?
> If not then ide0=qd65xx will yield "-- BAD OPTION"
> 
> Please check those first.

Yes, they are set.

Regards,
	Andrew Pam
-- 
mailto:xanni@xanadu.net                         Andrew Pam
http://www.xanadu.com.au/                       Chief Scientist, Xanadu
http://www.glasswings.com.au/                   Technology Manager, Glass Wings
http://www.sericyb.com.au/                      Manager, Serious Cybernetics
http://two-cents-worth.com/?105347&EG		Donate two cents to our work!
P.O. Box 477, Blackburn VIC 3130 Australia	Phone +61 401 258 915