Re: [PATCH] SAS: use sas_rphy instead of sas_end_device to obtain address.

[James Bottomley]

On Fri, 2016-08-12 at 16:39 +0200, Hannes Reinecke wrote:
> On 08/12/2016 04:34 PM, James Bottomley wrote:
> > On Fri, 2016-08-12 at 15:11 +0200, Johannes Thumshirn wrote:
> > > On Fri, Aug 12, 2016 at 12:08:54PM +0200, Johannes Thumshirn
> > > wrote:
> > > > Ok, we can't use the rphy because of wide-ports. We can't fix 
> > > > it to an end device either, as this makes some peoples systems
> > > > unbootable. Now let's find a third option satisfying the needs 
> > > > of SAS wide-ports and my customers (and others running 4.5+ 
> > > > with a SAS enclosure).
> > > > 
> > > > I'm digging...
> > > 
> > > 
> > > To answer myself, Hannes suggested doing it like this:
> > > 
> > > diff --git a/drivers/scsi/ses.c b/drivers/scsi/ses.c
> > > index 53ef1cb6..1d82053 100644
> > > --- a/drivers/scsi/ses.c
> > > +++ b/drivers/scsi/ses.c
> > > @@ -587,7 +587,7 @@ static void ses_match_to_enclosure(struct
> > > enclosure_device *edev,
> > > 
> > >         ses_enclosure_data_process(edev, to_scsi_device(edev
> > > ->edev.parent), 0);
> > > 
> > > -       if (is_sas_attached(sdev))
> > > +       if (scsi_is_sas_rphy(&sdev->sdev_gendev))
> > >                 efd.addr = sas_get_address(sdev);
> > > 
> > > 		if (efd.addr) {
> > > 
> > > 
> > > The reasoning behind this is, we only read the address if we have 
> > > an actual sas_rphy.
> > > 
> > > Would this be OK for you?
> > 
> > Could you please debug the why? first before we start throwing 
> > patches around.  is_sas_attached(sdev) returns true if the sdev is 
> > the child of a SAS controller.  What is this thing you've found 
> > that has a sdev attached to a SAS controller but isn't and end
> > device?
> > 
> > if is_sas_attached() passes but scsi_is_sas_rphy() doesn't you've 
> > got a device that is the child of a SAS host which has an rphy but 
> > which isn't an expander or end device.  That's pretty much the end 
> > of the list of things which can lie at the end of rphys since we 
> > lump the SATA possibilities in with end devices.
> > 
> hpsa magic.
> 
> The hpsa driver has some sdevs handled by the SAS transport class 
> (for the pass-through devices) and some sdevs (eg logical volumes) 
> which are not. As 'is_sas_attached' only checks if the _host_ has the 
> SAS transport class attached (which it will have), it will not work 
> as expected for devices which are not handled by the SAS transport 
> class (like the 'normal' hpsa logical volumes). And the logical 
> volumes don't even has a SAS address assigned to them, so in either
> case the original check will draw a blank here.

Thanks!  I've found it in hpsa_add_device() for logical vs physical
setups and, indeed, the way they call scsi_add_device will ensure that
we don't attach the SAS transport class because the rphy isn't properly
initialised so the last check (rphy->identify.device_type ==
SAS_END_DEVICE) won't pass .... in fact it's a bit of a mess.

The change looks fine, since it's tighter than the original and, since
this will be the last consumer of is_sas_attached(), you can remove
that as well.

James