[PATCH] aic94xx: Fixup compilation warning

[PATCH] aic94xx: Fixup compilation warning

[Hannes Reinecke]

gcc complained about an uninitialized warning.

Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 drivers/scsi/aic94xx/aic94xx_sds.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/scsi/aic94xx/aic94xx_sds.c b/drivers/scsi/aic94xx/aic94xx_sds.c
index edb43fd..6f6a5b8 100644
--- a/drivers/scsi/aic94xx/aic94xx_sds.c
+++ b/drivers/scsi/aic94xx/aic94xx_sds.c
@@ -982,7 +982,7 @@ static int asd_process_ctrl_a_user(struct asd_ha_struct *asd_ha,
 				   struct asd_flash_dir *flash_dir)
 {
 	int err, i;
-	u32 offs, size;
+	u32 offs = 0, size;
 	struct asd_ll_el *el;
 	struct asd_ctrla_phy_settings *ps;
 	struct asd_ctrla_phy_settings dflt_ps;
-- 
1.8.5.2

Re: [PATCH] aic94xx: Fixup compilation warning

[Christoph Hellwig]

On Tue, Nov 04, 2014 at 08:10:59AM +0100, Hannes Reinecke wrote:
> gcc complained about an uninitialized warning.

Looks fine to me, although it very much is gcc overreacting.

Re: [PATCH] aic94xx: Fixup compilation warning

[Christoph Hellwig]

Can someone review this trivial patch for me?  Thanks!

Re: [PATCH] aic94xx: Fixup compilation warning

[James Bottomley]

On Mon, 2014-11-24 at 05:40 -0800, Christoph Hellwig wrote:
> 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

---
diff --git a/drivers/scsi/aic94xx/aic94xx_sds.c b/drivers/scsi/aic94xx/aic94xx_sds.c
index edb43fd..d11b4d7 100644
--- a/drivers/scsi/aic94xx/aic94xx_sds.c
+++ b/drivers/scsi/aic94xx/aic94xx_sds.c
@@ -1003,6 +1003,7 @@ static int asd_process_ctrl_a_user(struct asd_ha_struct *asd_ha,
 		}
 
 		size = sizeof(struct asd_ctrla_phy_settings);
+		offs = 0;
 		ps = &dflt_ps;
 	}
 

Re: [PATCH] aic94xx: Fixup compilation warning

[Hannes Reinecke]

On 11/24/2014 04:39 PM, James Bottomley wrote:
> On Mon, 2014-11-24 at 05:40 -0800, Christoph Hellwig wrote:
>> 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
> 
> ---
> diff --git a/drivers/scsi/aic94xx/aic94xx_sds.c b/drivers/scsi/aic94xx/aic94xx_sds.c
> index edb43fd..d11b4d7 100644
> --- a/drivers/scsi/aic94xx/aic94xx_sds.c
> +++ b/drivers/scsi/aic94xx/aic94xx_sds.c
> @@ -1003,6 +1003,7 @@ static int asd_process_ctrl_a_user(struct asd_ha_struct *asd_ha,
>  		}
>  
>  		size = sizeof(struct asd_ctrla_phy_settings);
> +		offs = 0;
>  		ps = &dflt_ps;
>  	}
>  
> 
Yep, that would work, as well.

Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 21284 (AG Nürnberg)
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] aic94xx: Fixup compilation warning

[Tomas Henzl]

On 11/24/2014 02:40 PM, Christoph Hellwig wrote:
> 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 -
@@ -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;
-		}
-
-		size = sizeof(struct asd_ctrla_phy_settings);
-		ps = &dflt_ps;
+		goto out;
 	}
 
 	if (size == 0)
................

Somebody with access to this hw might help and test "offs = 0;".
It's a question whether we should touch old drivers at all.

Tomas

Re: [PATCH] aic94xx: Fixup compilation warning

[James Bottomley]

On Mon, 2014-11-24 at 17:04 +0100, Tomas Henzl wrote:
> On 11/24/2014 02:40 PM, Christoph Hellwig wrote:
> > 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?

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

> Probably the error path is never used, this patch should fix the gcc warning too -
> @@ -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;
> -		}
> -
> -		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

Re: [PATCH] aic94xx: Fixup compilation warning

[Tomas Henzl]

On 11/24/2014 05:10 PM, James Bottomley wrote:
> On Mon, 2014-11-24 at 17:04 +0100, Tomas Henzl wrote:
>> On 11/24/2014 02:40 PM, Christoph Hellwig wrote:
>>> 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?
> Yes, it seems to be the default from the hw ... if you don't have any
> nvram then the phy setting are up first.
>
>> Probably the error path is never used, this patch should fix the gcc warning too -
>> @@ -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;
>> -		}
>> -
>> -		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.

The dead code there brought me to the idea that the error path was not much used...
Using a '0' is at least so good as some random value, so I'm fine
with your or Hannes's patch.

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

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