Re: Commit a692b0e broke my mvsas card

[James Bottomley <James.Bottomley@HansenPartnership.com>]

On Tue, 2012-04-17 at 22:11 -0700, Dan Williams wrote:
> On Tue, Apr 17, 2012 at 10:17 AM, Dan Williams <dan.j.williams@intel.com> wrote:
> > On Tue, Apr 17, 2012 at 8:47 AM, James Bottomley
> > <James.Bottomley@hansenpartnership.com> 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.

Actually, no need ... I can add a revert to the fixes branch (when I
manage to get it built).

James