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

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

[Johannes Thumshirn]

Since commit 3f8d6f2a0 ('ses: fix discovery of SATA devices in SAS
enclosures') ses_match_to_enclosure() is calling sas_get_address(),
which is coming from commit bcf508c13385 ('scsi_transport_sas: add
function to get SAS endpoint address'). sas_get_address() itself calls
sas_sdev_to_rdev() which BUG_ON()s if a given scsi_device's rphy is
not a SAS_END_DEVICE.

As SAS Enclosure is a SAS expander device, we really shouldn't tie the
lookup of a SAS address to the SAS End Device but the sas_rphy, which
holds the address information.

Fixes: 3f8d6f2a0 ('ses: fix discovery of SATA devices in SAS enclosures')
Cc: stable@vger.kernel.org # v4.5+
Signed-off-by: Johannes Thumshirn <jthumshirn@suse.de>
---
 drivers/scsi/scsi_transport_sas.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/scsi_transport_sas.c b/drivers/scsi/scsi_transport_sas.c
index 3f0ff07..6b6d7b0 100644
--- a/drivers/scsi/scsi_transport_sas.c
+++ b/drivers/scsi/scsi_transport_sas.c
@@ -390,9 +390,9 @@ EXPORT_SYMBOL(sas_remove_host);
  */
 u64 sas_get_address(struct scsi_device *sdev)
 {
-	struct sas_end_device *rdev = sas_sdev_to_rdev(sdev);
+	struct sas_rphy *rphy = target_to_rphy(sdev->sdev_target);
 
-	return rdev->rphy.identify.sas_address;
+	return rphy->identify.sas_address;
 }
 EXPORT_SYMBOL(sas_get_address);
 
-- 
1.8.5.6

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

[James Bottomley]

On Thu, 2016-08-11 at 09:59 +0200, Johannes Thumshirn wrote:
> Since commit 3f8d6f2a0 ('ses: fix discovery of SATA devices in SAS
> enclosures') ses_match_to_enclosure() is calling sas_get_address(),
> which is coming from commit bcf508c13385 ('scsi_transport_sas: add
> function to get SAS endpoint address'). sas_get_address() itself 
> calls sas_sdev_to_rdev() which BUG_ON()s if a given scsi_device's 
> rphy is not a SAS_END_DEVICE.

Is the BUG_ON the problem?  you're supposed to gate this call with
is_sas_attached().

> As SAS Enclosure is a SAS expander device,

This isn't necessarily true.  There are several separated enclosure
chips even in the SAS world (although most of the new ones are
integrated).

>  we really shouldn't tie the lookup of a SAS address to the SAS End
> Device but the sas_rphy, which holds the address information.

This is conceptually wrong.  A wide end device may have many rphys
forming a port.  In that case the end device address is likely to be
only one of the rphy addresses ... how do you know this code picks the
right one?

> Fixes: 3f8d6f2a0 ('ses: fix discovery of SATA devices in SAS
> enclosures')
> Cc: stable@vger.kernel.org # v4.5+

What's the actual bug being fixed here?

James

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

[Johannes Thumshirn]

On Thu, Aug 11, 2016 at 08:09:35AM -0700, James Bottomley wrote:
> On Thu, 2016-08-11 at 09:59 +0200, Johannes Thumshirn wrote:
> > Since commit 3f8d6f2a0 ('ses: fix discovery of SATA devices in SAS
> > enclosures') ses_match_to_enclosure() is calling sas_get_address(),
> > which is coming from commit bcf508c13385 ('scsi_transport_sas: add
> > function to get SAS endpoint address'). sas_get_address() itself 
> > calls sas_sdev_to_rdev() which BUG_ON()s if a given scsi_device's 
> > rphy is not a SAS_END_DEVICE.
> 
> Is the BUG_ON the problem?  you're supposed to gate this call with
> is_sas_attached().
> 
> > As SAS Enclosure is a SAS expander device,
> 
> This isn't necessarily true.  There are several separated enclosure
> chips even in the SAS world (although most of the new ones are
> integrated).

Maybe change it to "As a SAS enclosure can be a SAS expander device, [..]"

> 
> >  we really shouldn't tie the lookup of a SAS address to the SAS End
> > Device but the sas_rphy, which holds the address information.
> 
> This is conceptually wrong.  A wide end device may have many rphys
> forming a port.  In that case the end device address is likely to be
> only one of the rphy addresses ... how do you know this code picks the
> right one?
> 
> > Fixes: 3f8d6f2a0 ('ses: fix discovery of SATA devices in SAS
> > enclosures')
> > Cc: stable@vger.kernel.org # v4.5+
> 
> What's the actual bug being fixed here?

This is the callchain I have:

ses_init()
`-> scsi_register_interface()
    `-> class_interface_register()
        `-> ses_intf_add()
	    `-> ses_match_to_enclosure()
	        `-> sas_get_address()
		    `-> sas_sdev_to_rdev()
                        `-> BUG_ON(rphy->identify.device_type != SAS_END_DEVICE);
		  	    `-> KABOOM, Bug report, yelling release manager, etc..

Unfortunately I do not have a SAS enclosure here so all I could do was
read the spec and code and have the customer test the patch.

But from my git archeology:
Since 3f8d6f2a0 sas_match_to_enclosure() is calling
sas_get_address(). Which in turn is calling sas_sdev_to_rdev() and so
on...
	
The thing is, in sas_get_address() we want to get the address of a sas
device, don't we?
And looking at the UML diagram in the SAS spec, we see an enclosure as
well as an end device do have a rphy attached to it, which in turn has
a SAS address.

So where is the point in fixing the SAS address retreival to end
devices and not the rphy?

Johannes
-- 
Johannes Thumshirn                                          Storage
jthumshirn@suse.de                                +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

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

[James Bottomley]

On Thu, 2016-08-11 at 18:43 +0200, Johannes Thumshirn wrote:
> On Thu, Aug 11, 2016 at 08:09:35AM -0700, James Bottomley wrote:
> > On Thu, 2016-08-11 at 09:59 +0200, Johannes Thumshirn wrote:
> > > Since commit 3f8d6f2a0 ('ses: fix discovery of SATA devices in
> > > SAS
> > > enclosures') ses_match_to_enclosure() is calling
> > > sas_get_address(),
> > > which is coming from commit bcf508c13385 ('scsi_transport_sas:
> > > add
> > > function to get SAS endpoint address'). sas_get_address() itself 
> > > calls sas_sdev_to_rdev() which BUG_ON()s if a given scsi_device's
> > > rphy is not a SAS_END_DEVICE.
> > 
> > Is the BUG_ON the problem?  you're supposed to gate this call with
> > is_sas_attached().
> > 
> > > As SAS Enclosure is a SAS expander device,
> > 
> > This isn't necessarily true.  There are several separated enclosure
> > chips even in the SAS world (although most of the new ones are
> > integrated).
> 
> Maybe change it to "As a SAS enclosure can be a SAS expander device,
> [..]"
> 
> > 
> > >  we really shouldn't tie the lookup of a SAS address to the SAS
> > > End
> > > Device but the sas_rphy, which holds the address information.
> > 
> > This is conceptually wrong.  A wide end device may have many rphys
> > forming a port.  In that case the end device address is likely to
> > be
> > only one of the rphy addresses ... how do you know this code picks
> > the
> > right one?
> > 
> > > Fixes: 3f8d6f2a0 ('ses: fix discovery of SATA devices in SAS
> > > enclosures')
> > > Cc: stable@vger.kernel.org # v4.5+
> > 
> > What's the actual bug being fixed here?
> 
> This is the callchain I have:
> 
> ses_init()
> `-> scsi_register_interface()
>     `-> class_interface_register()
>         `-> ses_intf_add()
> 	    `-> ses_match_to_enclosure()
> 	        `-> sas_get_address()
> 		    `-> sas_sdev_to_rdev()
>                         `-> BUG_ON(rphy->identify.device_type !=
> SAS_END_DEVICE);
> 		  	    `-> KABOOM, Bug report, yelling release
> manager, etc..

So what is at the end?  is_sas_attached() indicates the transport class
attached to something and that something generated an sdev ... that can
only happen I think if that something is an end device.

> Unfortunately I do not have a SAS enclosure here so all I could do 
> was read the spec and code and have the customer test the patch.
> 
> But from my git archeology:
> Since 3f8d6f2a0 sas_match_to_enclosure() is calling
> sas_get_address(). Which in turn is calling sas_sdev_to_rdev() and so
> on...
> 	
> The thing is, in sas_get_address() we want to get the address of a 
> sas device, don't we? And looking at the UML diagram in the SAS spec, 
> we see an enclosure as well as an end device do have a rphy attached 
> to it, which in turn has a SAS address.

Because, as I said in the previous reply, the rphy is only the sas
address for non-wide ports.  They phy layer is just above the physical
layer.  The end device sits at the application layer, which is four
layers above that. Whatever diagram you're looking at is probably
showing port layer connections.  The way the transport class works is
that each rphy, even when part of a formed wide port, is individually
addressable, so we can send SMP phy controls to it, but the device is
separately addressable by its own SAS address.

James


> So where is the point in fixing the SAS address retreival to end
> devices and not the rphy?
> 
> Johannes

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

[Johannes Thumshirn]

On Thu, Aug 11, 2016 at 11:00:07AM -0700, James Bottomley wrote:
> On Thu, 2016-08-11 at 18:43 +0200, Johannes Thumshirn wrote:
> > On Thu, Aug 11, 2016 at 08:09:35AM -0700, James Bottomley wrote:
> > > On Thu, 2016-08-11 at 09:59 +0200, Johannes Thumshirn wrote:
> > > > Since commit 3f8d6f2a0 ('ses: fix discovery of SATA devices in
> > > > SAS
> > > > enclosures') ses_match_to_enclosure() is calling
> > > > sas_get_address(),
> > > > which is coming from commit bcf508c13385 ('scsi_transport_sas:
> > > > add
> > > > function to get SAS endpoint address'). sas_get_address() itself 
> > > > calls sas_sdev_to_rdev() which BUG_ON()s if a given scsi_device's
> > > > rphy is not a SAS_END_DEVICE.
> > > 
> > > Is the BUG_ON the problem?  you're supposed to gate this call with
> > > is_sas_attached().
> > > 
> > > > As SAS Enclosure is a SAS expander device,
> > > 
> > > This isn't necessarily true.  There are several separated enclosure
> > > chips even in the SAS world (although most of the new ones are
> > > integrated).
> > 
> > Maybe change it to "As a SAS enclosure can be a SAS expander device,
> > [..]"
> > 
> > > 
> > > >  we really shouldn't tie the lookup of a SAS address to the SAS
> > > > End
> > > > Device but the sas_rphy, which holds the address information.
> > > 
> > > This is conceptually wrong.  A wide end device may have many rphys
> > > forming a port.  In that case the end device address is likely to
> > > be
> > > only one of the rphy addresses ... how do you know this code picks
> > > the
> > > right one?
> > > 
> > > > Fixes: 3f8d6f2a0 ('ses: fix discovery of SATA devices in SAS
> > > > enclosures')
> > > > Cc: stable@vger.kernel.org # v4.5+
> > > 
> > > What's the actual bug being fixed here?
> > 
> > This is the callchain I have:
> > 
> > ses_init()
> > `-> scsi_register_interface()
> >     `-> class_interface_register()
> >         `-> ses_intf_add()
> > 	    `-> ses_match_to_enclosure()
> > 	        `-> sas_get_address()
> > 		    `-> sas_sdev_to_rdev()
> >                         `-> BUG_ON(rphy->identify.device_type !=
> > SAS_END_DEVICE);
> > 		  	    `-> KABOOM, Bug report, yelling release
> > manager, etc..
> 
> So what is at the end?  is_sas_attached() indicates the transport class
> attached to something and that something generated an sdev ... that can
> only happen I think if that something is an end device.

sas_get_address() in ses_match_to_enclosure() _is_ guarded by
is_sas_attached(), so it probably can not only happen if that
something is an end device or the BUG_ON() wouldn't fire.

> 
> > Unfortunately I do not have a SAS enclosure here so all I could do 
> > was read the spec and code and have the customer test the patch.
> > 
> > But from my git archeology:
> > Since 3f8d6f2a0 sas_match_to_enclosure() is calling
> > sas_get_address(). Which in turn is calling sas_sdev_to_rdev() and so
> > on...
> > 	
> > The thing is, in sas_get_address() we want to get the address of a 
> > sas device, don't we? And looking at the UML diagram in the SAS spec, 
> > we see an enclosure as well as an end device do have a rphy attached 
> > to it, which in turn has a SAS address.
> 
> Because, as I said in the previous reply, the rphy is only the sas
> address for non-wide ports.  They phy layer is just above the physical
> layer.  The end device sits at the application layer, which is four
> layers above that. Whatever diagram you're looking at is probably
> showing port layer connections.  The way the transport class works is
> that each rphy, even when part of a formed wide port, is individually
> addressable, so we can send SMP phy controls to it, but the device is
> separately addressable by its own SAS address.

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

-- 
Johannes Thumshirn                                          Storage
jthumshirn@suse.de                                +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

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

[Johannes Thumshirn]

On Fri, Aug 12, 2016 at 12:08:54PM +0200, Johannes Thumshirn wrote:
> On Thu, Aug 11, 2016 at 11:00:07AM -0700, James Bottomley wrote:
> > On Thu, 2016-08-11 at 18:43 +0200, Johannes Thumshirn wrote:
> > > On Thu, Aug 11, 2016 at 08:09:35AM -0700, James Bottomley wrote:
> > > > On Thu, 2016-08-11 at 09:59 +0200, Johannes Thumshirn wrote:
> > > > > Since commit 3f8d6f2a0 ('ses: fix discovery of SATA devices in
> > > > > SAS
> > > > > enclosures') ses_match_to_enclosure() is calling
> > > > > sas_get_address(),
> > > > > which is coming from commit bcf508c13385 ('scsi_transport_sas:
> > > > > add
> > > > > function to get SAS endpoint address'). sas_get_address() itself 
> > > > > calls sas_sdev_to_rdev() which BUG_ON()s if a given scsi_device's
> > > > > rphy is not a SAS_END_DEVICE.
> > > > 
> > > > Is the BUG_ON the problem?  you're supposed to gate this call with
> > > > is_sas_attached().
> > > > 
> > > > > As SAS Enclosure is a SAS expander device,
> > > > 
> > > > This isn't necessarily true.  There are several separated enclosure
> > > > chips even in the SAS world (although most of the new ones are
> > > > integrated).
> > > 
> > > Maybe change it to "As a SAS enclosure can be a SAS expander device,
> > > [..]"
> > > 
> > > > 
> > > > >  we really shouldn't tie the lookup of a SAS address to the SAS
> > > > > End
> > > > > Device but the sas_rphy, which holds the address information.
> > > > 
> > > > This is conceptually wrong.  A wide end device may have many rphys
> > > > forming a port.  In that case the end device address is likely to
> > > > be
> > > > only one of the rphy addresses ... how do you know this code picks
> > > > the
> > > > right one?
> > > > 
> > > > > Fixes: 3f8d6f2a0 ('ses: fix discovery of SATA devices in SAS
> > > > > enclosures')
> > > > > Cc: stable@vger.kernel.org # v4.5+
> > > > 
> > > > What's the actual bug being fixed here?
> > > 
> > > This is the callchain I have:
> > > 
> > > ses_init()
> > > `-> scsi_register_interface()
> > >     `-> class_interface_register()
> > >         `-> ses_intf_add()
> > > 	    `-> ses_match_to_enclosure()
> > > 	        `-> sas_get_address()
> > > 		    `-> sas_sdev_to_rdev()
> > >                         `-> BUG_ON(rphy->identify.device_type !=
> > > SAS_END_DEVICE);
> > > 		  	    `-> KABOOM, Bug report, yelling release
> > > manager, etc..
> > 
> > So what is at the end?  is_sas_attached() indicates the transport class
> > attached to something and that something generated an sdev ... that can
> > only happen I think if that something is an end device.
> 
> sas_get_address() in ses_match_to_enclosure() _is_ guarded by
> is_sas_attached(), so it probably can not only happen if that
> something is an end device or the BUG_ON() wouldn't fire.
> 
> > 
> > > Unfortunately I do not have a SAS enclosure here so all I could do 
> > > was read the spec and code and have the customer test the patch.
> > > 
> > > But from my git archeology:
> > > Since 3f8d6f2a0 sas_match_to_enclosure() is calling
> > > sas_get_address(). Which in turn is calling sas_sdev_to_rdev() and so
> > > on...
> > > 	
> > > The thing is, in sas_get_address() we want to get the address of a 
> > > sas device, don't we? And looking at the UML diagram in the SAS spec, 
> > > we see an enclosure as well as an end device do have a rphy attached 
> > > to it, which in turn has a SAS address.
> > 
> > Because, as I said in the previous reply, the rphy is only the sas
> > address for non-wide ports.  They phy layer is just above the physical
> > layer.  The end device sits at the application layer, which is four
> > layers above that. Whatever diagram you're looking at is probably
> > showing port layer connections.  The way the transport class works is
> > that each rphy, even when part of a formed wide port, is individually
> > addressable, so we can send SMP phy controls to it, but the device is
> > separately addressable by its own SAS address.
> 
> 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?

      Johannes

-- 
Johannes Thumshirn                                          Storage
jthumshirn@suse.de                                +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

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

[James Bottomley]

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.

James

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

[Hannes Reinecke]

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.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)

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

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@suse.de                                +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