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
