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
