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� ʇڙ�,j��f���h���z� �w��� ���j:+v���w�j�m��������zZ+�����ݢj"��!tml=
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
