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

[Johannes Thumshirn]

On Fri, Aug 12, 2016 at 07:54:03AM -0700, James Bottomley wrote:
> 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.

Sounds good, but for monday.

 Nice weekend all.

-- 
Johannes Thumshirn                                          Storage
jthumshirn@xxxxxxx                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850