aic94xx: Fixup compilation warning

gcc complained about an uninitialized warning.

Can someone review this trivial patch for me? Thanks!

Quoted 1 line · Christoph Hellwig

Can someone review this trivial patch for me? Thanks!

I don't really think we need reviewers for trivial patches, do we?

However, the patch is clearly bogus. What's supposed to happen is that the unset variables (size and offs) get initialised to default values on error return, which is what's missing for offset, so this is the correct patch, isn't it?

James

---

drivers/scsi/aic94xx/aic94xx_sds.c +0 -0

index edb43fd..d11b4d7 100644

Can someone review this trivial patch for me? Thanks!

Quoted 2 lines · Christoph Hellwig

Can someone review this trivial patch for me? Thanks!

The compiler complains because when asd_find_flash_de fails, the offs is not initialized. When that happens this code is invoked :

        if (err) {
                ASD_DPRINTK("couldn't find CTRL-A user settings section\n");
                ASD_DPRINTK("Creating default CTRL-A user settings section\n");

                dflt_ps.id0 = 'h';
                dflt_ps.num_phys = 8;
                for (i =0; i < ASD_MAX_PHYS; i++) {
                        memcpy(dflt_ps.phy_ent[i].sas_addr,
                               asd_ha->hw_prof.sas_addr, SAS_ADDR_SIZE);
                        dflt_ps.phy_ent[i].sas_link_rates = 0x98;
                        dflt_ps.phy_ent[i].flags = 0x0;
                        dflt_ps.phy_ent[i].sata_link_rates = 0x0;
                }

                size = sizeof(struct asd_ctrla_phy_settings);
                ps = &dflt_ps;

the dflt_ps is initialized and the address assigned to 'ps', but none of them is used later
}

        if (size == 0)
                goto out;

        err = -ENOMEM;
        el = kmalloc(size, GFP_KERNEL);
        if (!el) {
                ASD_DPRINTK("no mem for ctrla user settings section\n");
                goto out;
        }

        err = asd_read_flash_seg(asd_ha, (void *)el, offs, size);
        if (err) {
                ASD_DPRINTK("couldn't read ctrla phy settings section\n");
                goto out2;
        }

        err = -ENOENT;
        ps = asd_find_ll_by_id(el, 'h', 0xFF);

here^ a new value is assigned to 'ps'

I have no idea what was intended in the originally so - it looks likely, but how do we know that the '0' is a correct value for offs?

Probably the error path is never used, this patch should fix the gcc warning too -

(quoted diff) +0 -5

@@ -990,20 +990,7 @@ static int asd_process_ctrl_a_user(struct asd_ha_struct *asd_ha,
 	err = asd_find_flash_de(flash_dir, FLASH_DE_CTRL_A_USER, &offs, &size);
 	if (err) {
 		ASD_DPRINTK("couldn't find CTRL-A user settings section\n");
-		ASD_DPRINTK("Creating default CTRL-A user settings section\n");
-
-		dflt_ps.id0 = 'h';
-		dflt_ps.num_phys = 8;
-		for (i =0; i < ASD_MAX_PHYS; i++) {

Quoted 42 lines · Tomas Henzl

Quoted 2 lines · Christoph Hellwig

Can someone review this trivial patch for me? Thanks!

The compiler complains because when asd_find_flash_de fails, the offs is not initialized. When that happens this code is invoked :

        if (err) {
                ASD_DPRINTK("couldn't find CTRL-A user settings section\n");
                ASD_DPRINTK("Creating default CTRL-A user settings section\n");

                dflt_ps.id0 = 'h';
                dflt_ps.num_phys = 8;
                for (i =0; i < ASD_MAX_PHYS; i++) {
                        memcpy(dflt_ps.phy_ent[i].sas_addr,
                               asd_ha->hw_prof.sas_addr, SAS_ADDR_SIZE);
                        dflt_ps.phy_ent[i].sas_link_rates = 0x98;
                        dflt_ps.phy_ent[i].flags = 0x0;
                        dflt_ps.phy_ent[i].sata_link_rates = 0x0;
                }

                size = sizeof(struct asd_ctrla_phy_settings);
                ps = &dflt_ps;

the dflt_ps is initialized and the address assigned to 'ps', but none of them is used later
}

        if (size == 0)
                goto out;

        err = -ENOMEM;
        el = kmalloc(size, GFP_KERNEL);
        if (!el) {
                ASD_DPRINTK("no mem for ctrla user settings section\n");
                goto out;
        }

        err = asd_read_flash_seg(asd_ha, (void *)el, offs, size);
        if (err) {
                ASD_DPRINTK("couldn't read ctrla phy settings section\n");
                goto out2;
        }

        err = -ENOENT;
        ps = asd_find_ll_by_id(el, 'h', 0xFF);

Quoted 5 lines

here^ a new value is assigned to 'ps'

I have no idea what was intended in the originally so - it looks likely, but how do we know that the '0' is a correct value for offs?

Yes, it seems to be the default from the hw ... if you don't have any nvram then the phy setting are up first.

Quoted 17 lines

Probably the error path is never used, this patch should fix the gcc warning too -

(quoted diff) +0 -12

@@ -990,20 +990,7 @@ static int asd_process_ctrl_a_user(struct asd_ha_struct *asd_ha,
 	err = asd_find_flash_de(flash_dir, FLASH_DE_CTRL_A_USER, &offs, &size);
 	if (err) {
 		ASD_DPRINTK("couldn't find CTRL-A user settings section\n");
-		ASD_DPRINTK("Creating default CTRL-A user settings section\n");
-
-		dflt_ps.id0 = 'h';
-		dflt_ps.num_phys = 8;
-		for (i =0; i < ASD_MAX_PHYS; i++) {
-			memcpy(dflt_ps.phy_ent[i].sas_addr,
-			       asd_ha->hw_prof.sas_addr, SAS_ADDR_SIZE);
-			dflt_ps.phy_ent[i].sas_link_rates = 0x98;
-			dflt_ps.phy_ent[i].flags = 0x0;
-			dflt_ps.phy_ent[i].sata_link_rates = 0x0;
-		}
-

Review static int asd_process_ctrl_a_user(struct asd_ha_struct *asd_ha, · line 993

Quoted 3 lines

(quoted diff) +1 -2

-		size = sizeof(struct asd_ctrla_phy_settings);
-		ps = &dflt_ps;
+		goto out;

I did think about that. There does seem to be a definite reason to expect the phy setting read to succeed even though the nvram read failed. I think the reason was that there were some aic94xx cards that had no nvram and therefore always failed the read, but I can't find any evidence of that any more.

James

N�����r��y���b�X��ǧv�^�)޺{.n�+����{���"�{ay�ʇڙ�,j��f���h���z��w������j:+v���w�j�m��������zZ+�����ݢj"��!tml=