Re: [PATCH] aic94xx: Fixup compilation warning

[Tomas Henzl <thenzl@redhat.com>]

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