Re: Commit a692b0e broke my mvsas card

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]


On Tue, Apr 17, 2012 at 10:17 AM, Dan Williams <dan.j.williams@xxxxxxxxx> wrote:
> On Tue, Apr 17, 2012 at 8:47 AM, James Bottomley
> <James.Bottomley@xxxxxxxxxxxxxxxxxxxxx> wrote:
>> On Mon, 2012-04-16 at 20:25 -0700, Dan Williams wrote:
>>> Agh, sorry, I rushed that one.  The phy array is initialized later, here
>>> is another run at it:
>>
>> Are we sure it's initialised correctly in all the other SAS drivers that
>> use (well, one other: aic94xx)?
>
> Looks like we need:
>
> diff --git a/drivers/scsi/aic94xx/aic94xx_init.c
> b/drivers/scsi/aic94xx/aic94xx_init.c
> index ff80552..830f438 100644
> --- a/drivers/scsi/aic94xx/aic94xx_init.c
> +++ b/drivers/scsi/aic94xx/aic94xx_init.c
> @@ -250,6 +250,7 @@ static int __devinit asd_common_setup(struct
> asd_ha_struct *asd_ha)
>                        SAS_LINK_RATE_1_5_GBPS;
>                asd_ha->hw_prof.phy_desc[i].min_sata_lrate =
>                        SAS_LINK_RATE_1_5_GBPS;
> +               asd_ha->phys[i].sas_phy.id = i;
>        }
>
>        return 0;
>
>> Given the oops issue, perhaps revert this for now and get a working
>> patch in for the next merge window?
>
> I have no strong feelings either way, but aic94xx and mvsas
> maintainers have been hard to reach and I'm not encouraged more time
> will yield a different result versus just moving ahead with these
> fixes.
>
> That said we still have Tom's discovery regression which is a separate issue.

I take it back.

I overlooked what Tom said at the very beginning.  Everything is fixed
by the revert, and I now see why my later "fix" made it worse.  The
fix overlooked that mvsas is indeed initializing the phy ids, but in
the "multi-chip" case it does a rather annoying duplication of phy ids
in the array passed to libsas.  So, for example, chip0 has phy0-3 at
ha phy index 0-3 and chip1 has its phy0-3 at ha phy index 4-7.  So the
"fix" was breaking mvsas's ability to lookup phys by id and the
original commit is tripped up by mvsas's scheme of putting non-unique
ids in the sas_ha_struct.

Question for a later day, but why isn't mvsas creating a scsi_host per
chip??  That can only help performance and is more in line with
reality.

To properly support commit a692b0e mvsas would either need to convert
to a shost-per-chip or use a helper like:

static inline struct mvs_phy *to_mvs_phy(struct mvs_info *mvi, int phy_id)
{
        return mvi->phy[phy_id - mvi->chip->n_phy * mvi->id];
}

...everywhere it converts from a libsas phy id back to a local phy structure.

Both of these options are way too big for 3.4-rc4, so I'll queue up
the revert with a Reported-by and Tested-by from Tom.

Thanks again for the report and help Tom.

--
Dan
--
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


[SCSI Target Devel]     [Linux SCSI Target Infrastructure]     [Kernel Newbies]     [Share Photos]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Photos]     [Yosemite]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Linux IIO]     [Samba]     [Video 4 Linux]     [Device Mapper]     [Linux Resources]

Add to Google Powered by Linux