Re: [PATCH] aic94xx: Fixup compilation warning

[James Bottomley <jbottomley@parallels.com>]

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