[PATCH 0/3] firewire-sbp2: Various fixes

[PATCH 0/3] firewire-sbp2: Various fixes

[Chris Boot]

Hi,

In writing my SBP-2 target code I found a few areas in the firewire-sbp2
initiator code that needed some attention:

1. The Unit_Unique_ID property in SBP-2 unit directories is ignored, but this
   should be used when working out the target port name instead of the node's
   GUID if it is present. (Suggestion of Stefan Richter)

2. The initiator tries to login to any and all SBP-2 targets, including ones on
   the local node. This fails due to how the firewire stack / initiator code
   works but is most likely undesirable behaviour, so this patch makes
   firewire-sbp2 ignore SBP-2 targets on the local node.

3. SBP-2 sends SCSI sense data over the wire in an unusual format which
   requires un-mangling before we can pass it to the SCSI subsystem. Some of
   this mangling is currently being done incorrectly, so this patch fixes it.

Cheers,
Chris

-- 
Chris Boot
bootc@bootc.net

[PATCH 1/3] firewire-sbp2: Take into account Unit_Unique_ID

[Chris Boot]

If the target's unit directory contains a Unit_Unique_ID entry, we
should use that as the target's GUID for identification purposes. The
SBP-2 standards document says:

"Although the node unique ID (EUI-64) present in the bus information
block is sufficient to uniquely identify nodes attached to Serial Bus,
it is insufficient to identify a target when a vendor implements a
device with multiple Serial Bus node connections. In this case initiator
software requires information by which a particular target may be
uniquely identified, regardless of the Serial Bus access path used."

[ IEEE T10 P1155D Revision 4, Section 7.6 (page 51) ] and
[ IEEE T10 P1467D Revision 5, Section 7.9 (page 74) ]

Signed-off-by: Chris Boot <bootc@bootc.net>
Cc: Stefan Richter <stefanr@s5r6.in-berlin.de>
---
 drivers/firewire/sbp2.c |   17 +++++++++++++++++
 1 files changed, 17 insertions(+), 0 deletions(-)

diff --git a/drivers/firewire/sbp2.c b/drivers/firewire/sbp2.c
index 80e95aa..ed5bbbf 100644
--- a/drivers/firewire/sbp2.c
+++ b/drivers/firewire/sbp2.c
@@ -211,6 +211,7 @@ static struct fw_device *target_device(struct sbp2_target *tgt)
 #define SBP2_CSR_UNIT_CHARACTERISTICS	0x3a
 #define SBP2_CSR_FIRMWARE_REVISION	0x3c
 #define SBP2_CSR_LOGICAL_UNIT_NUMBER	0x14
+#define SBP2_CSR_UNIT_UNIQUE_ID		0x8d
 #define SBP2_CSR_LOGICAL_UNIT_DIRECTORY	0xd4
 
 /* Management orb opcodes */
@@ -997,6 +998,17 @@ static int sbp2_add_logical_unit(struct sbp2_target *tgt, int lun_entry)
 	return 0;
 }
 
+static int sbp2_get_unit_unique_id(struct sbp2_target *tgt,
+				    const u32 *leaf)
+{
+	if ((leaf[0] & 0xffff0000) != 0x00020000)
+		return -EINVAL;
+
+	tgt->guid = (u64)leaf[1] << 32 | leaf[2];
+
+	return 0;
+}
+
 static int sbp2_scan_logical_unit_dir(struct sbp2_target *tgt,
 				      const u32 *directory)
 {
@@ -1048,6 +1060,11 @@ static int sbp2_scan_unit_dir(struct sbp2_target *tgt, const u32 *directory,
 				return -ENOMEM;
 			break;
 
+		case SBP2_CSR_UNIT_UNIQUE_ID:
+			if (sbp2_get_unit_unique_id(tgt, ci.p - 1 + value) < 0)
+				return -EINVAL;
+			break;
+
 		case SBP2_CSR_LOGICAL_UNIT_DIRECTORY:
 			/* Adjust for the increment in the iterator */
 			if (sbp2_scan_logical_unit_dir(tgt, ci.p - 1 + value) < 0)
-- 
1.7.8.3

Re: [PATCH 1/3] firewire-sbp2: Take into account Unit_Unique_ID

[Stefan Richter]

On Feb 10 Chris Boot wrote:
> If the target's unit directory contains a Unit_Unique_ID entry, we
> should use that as the target's GUID for identification purposes. The
> SBP-2 standards document says:
> 
> "Although the node unique ID (EUI-64) present in the bus information
> block is sufficient to uniquely identify nodes attached to Serial Bus,
> it is insufficient to identify a target when a vendor implements a
> device with multiple Serial Bus node connections. In this case initiator
> software requires information by which a particular target may be
> uniquely identified, regardless of the Serial Bus access path used."
> 
> [ IEEE T10 P1155D Revision 4, Section 7.6 (page 51) ] and
> [ IEEE T10 P1467D Revision 5, Section 7.9 (page 74) ]
> 
> Signed-off-by: Chris Boot <bootc@bootc.net>
> Cc: Stefan Richter <stefanr@s5r6.in-berlin.de>
> ---
>  drivers/firewire/sbp2.c |   17 +++++++++++++++++
>  1 files changed, 17 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/firewire/sbp2.c b/drivers/firewire/sbp2.c
> index 80e95aa..ed5bbbf 100644
> --- a/drivers/firewire/sbp2.c
> +++ b/drivers/firewire/sbp2.c
> @@ -211,6 +211,7 @@ static struct fw_device *target_device(struct sbp2_target *tgt)
>  #define SBP2_CSR_UNIT_CHARACTERISTICS	0x3a
>  #define SBP2_CSR_FIRMWARE_REVISION	0x3c
>  #define SBP2_CSR_LOGICAL_UNIT_NUMBER	0x14
> +#define SBP2_CSR_UNIT_UNIQUE_ID		0x8d
>  #define SBP2_CSR_LOGICAL_UNIT_DIRECTORY	0xd4
>  
>  /* Management orb opcodes */
> @@ -997,6 +998,17 @@ static int sbp2_add_logical_unit(struct sbp2_target *tgt, int lun_entry)
>  	return 0;
>  }
>  
> +static int sbp2_get_unit_unique_id(struct sbp2_target *tgt,
> +				    const u32 *leaf)
> +{
> +	if ((leaf[0] & 0xffff0000) != 0x00020000)
> +		return -EINVAL;

This could be relaxed to "if (leaf[0] < 0x00020000)", but the stricter
check is fine too.

> +
> +	tgt->guid = (u64)leaf[1] << 32 | leaf[2];
> +
> +	return 0;
> +}
> +
>  static int sbp2_scan_logical_unit_dir(struct sbp2_target *tgt,
>  				      const u32 *directory)
>  {
> @@ -1048,6 +1060,11 @@ static int sbp2_scan_unit_dir(struct sbp2_target *tgt, const u32 *directory,
>  				return -ENOMEM;
>  			break;
>  
> +		case SBP2_CSR_UNIT_UNIQUE_ID:
> +			if (sbp2_get_unit_unique_id(tgt, ci.p - 1 + value) < 0)
> +				return -EINVAL;
> +			break;
> +
>  		case SBP2_CSR_LOGICAL_UNIT_DIRECTORY:
>  			/* Adjust for the increment in the iterator */
>  			if (sbp2_scan_logical_unit_dir(tgt, ci.p - 1 + value) < 0)

The error return here is wrong.  Garbage in a non-essential part of the
Config ROM is no reason to refuse to work with a device.  It is too common
for firmware to have various bogus values in there.  For instance, we
never check the CRC of a Config ROM block because wrongly calculated CRCs
or even zero CRC is quite commonly seen with otherwise correct Config ROMs.

And there is another problem with the patch:  In fringe cases, we might
now create more than one scsi_device instances with the same ieee1394_id
sysfs attribute value.  Those cases are:
 1. There are two targets present which expose the same, hence non-unique
    and thus standards-violating Unit_Unique_ID. Or
 2. There is a single target connected through more than one link, it has
    got a Unit_Unique_ID, and either
     2.a  it accepts concurrent login despite firewire-sbp2 demanding an
          exclusive login (which is its default mode), or
     2.b  firewire-sbp2 is configured to work in concurrent login mode and
          the target grants concurrent logins.

We do not need to care for case 1.  It cannot be distinguished from case
2, and we already do not care for the case that there are two or more
nodes with a non-unique Node_Unique_ID.  Devices with the latter bug exist
but are rare, judging from historical discussion on linux1394-devel.

Case 2.a is highly unlikely, and I think we should not worry about that
either.

Should we do something about case 2.b?  Where in the Linux SCSI
initiator stack is multipathing handled --- in transport layer drivers or
higher up?  (Cc'ing LSML for this question.)
-- 
Stefan Richter
-=====-===-- --=- -=-==
http://arcgraph.de/sr/

Re: [PATCH 1/3] firewire-sbp2: Take into account Unit_Unique_ID

[Chris Boot]

On 11/02/2012 11:12, Stefan Richter wrote:
> On Feb 10 Chris Boot wrote:
>> If the target's unit directory contains a Unit_Unique_ID entry, we
>> should use that as the target's GUID for identification purposes. The
>> SBP-2 standards document says:
>>
>> "Although the node unique ID (EUI-64) present in the bus information
>> block is sufficient to uniquely identify nodes attached to Serial Bus,
>> it is insufficient to identify a target when a vendor implements a
>> device with multiple Serial Bus node connections. In this case initiator
>> software requires information by which a particular target may be
>> uniquely identified, regardless of the Serial Bus access path used."
>>
>> [ IEEE T10 P1155D Revision 4, Section 7.6 (page 51) ] and
>> [ IEEE T10 P1467D Revision 5, Section 7.9 (page 74) ]
>>
>> Signed-off-by: Chris Boot<bootc@bootc.net>
>> Cc: Stefan Richter<stefanr@s5r6.in-berlin.de>
>> ---
>>   drivers/firewire/sbp2.c |   17 +++++++++++++++++
>>   1 files changed, 17 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/firewire/sbp2.c b/drivers/firewire/sbp2.c
>> index 80e95aa..ed5bbbf 100644
>> --- a/drivers/firewire/sbp2.c
>> +++ b/drivers/firewire/sbp2.c
>> @@ -211,6 +211,7 @@ static struct fw_device *target_device(struct sbp2_target *tgt)
>>   #define SBP2_CSR_UNIT_CHARACTERISTICS	0x3a
>>   #define SBP2_CSR_FIRMWARE_REVISION	0x3c
>>   #define SBP2_CSR_LOGICAL_UNIT_NUMBER	0x14
>> +#define SBP2_CSR_UNIT_UNIQUE_ID		0x8d
>>   #define SBP2_CSR_LOGICAL_UNIT_DIRECTORY	0xd4
>>
>>   /* Management orb opcodes */
>> @@ -997,6 +998,17 @@ static int sbp2_add_logical_unit(struct sbp2_target *tgt, int lun_entry)
>>   	return 0;
>>   }
>>
>> +static int sbp2_get_unit_unique_id(struct sbp2_target *tgt,
>> +				    const u32 *leaf)
>> +{
>> +	if ((leaf[0]&  0xffff0000) != 0x00020000)
>> +		return -EINVAL;
>
> This could be relaxed to "if (leaf[0]<  0x00020000)", but the stricter
> check is fine too.

Well the standard does say the length must be exactly 2 rather than just 
defining it a leaf node that contains an EUI-64. But I did not realise 
various firmware gets things quite so wrong sometimes...

>> +
>> +	tgt->guid = (u64)leaf[1]<<  32 | leaf[2];
>> +
>> +	return 0;
>> +}
>> +
>>   static int sbp2_scan_logical_unit_dir(struct sbp2_target *tgt,
>>   				      const u32 *directory)
>>   {
>> @@ -1048,6 +1060,11 @@ static int sbp2_scan_unit_dir(struct sbp2_target *tgt, const u32 *directory,
>>   				return -ENOMEM;
>>   			break;
>>
>> +		case SBP2_CSR_UNIT_UNIQUE_ID:
>> +			if (sbp2_get_unit_unique_id(tgt, ci.p - 1 + value)<  0)
>> +				return -EINVAL;
>> +			break;
>> +
>>   		case SBP2_CSR_LOGICAL_UNIT_DIRECTORY:
>>   			/* Adjust for the increment in the iterator */
>>   			if (sbp2_scan_logical_unit_dir(tgt, ci.p - 1 + value)<  0)
>
> The error return here is wrong.  Garbage in a non-essential part of the
> Config ROM is no reason to refuse to work with a device.  It is too common
> for firmware to have various bogus values in there.  For instance, we
> never check the CRC of a Config ROM block because wrongly calculated CRCs
> or even zero CRC is quite commonly seen with otherwise correct Config ROMs.

Wow. I didn't expect things to get so bad. :-( I guess in this case we 
simply ignore the return value, which would have the effect of not 
setting the GUID.

> And there is another problem with the patch:  In fringe cases, we might
> now create more than one scsi_device instances with the same ieee1394_id
> sysfs attribute value.   Those cases are:
>   1. There are two targets present which expose the same, hence non-unique
>      and thus standards-violating Unit_Unique_ID. Or
>   2. There is a single target connected through more than one link, it has
>      got a Unit_Unique_ID, and either
>       2.a  it accepts concurrent login despite firewire-sbp2 demanding an
>            exclusive login (which is its default mode), or
>       2.b  firewire-sbp2 is configured to work in concurrent login mode and
>            the target grants concurrent logins.
>
> We do not need to care for case 1.  It cannot be distinguished from case
> 2, and we already do not care for the case that there are two or more
> nodes with a non-unique Node_Unique_ID.  Devices with the latter bug exist
> but are rare, judging from historical discussion on linux1394-devel.
>
> Case 2.a is highly unlikely, and I think we should not worry about that
> either.
>
> Should we do something about case 2.b?  Where in the Linux SCSI
> initiator stack is multipathing handled --- in transport layer drivers or
> higher up?  (Cc'ing LSML for this question.)

I believe multipathing is handled by multipathd, which uses devmapper to 
handle the actual data flow. Multipathd itself works out which LUNs it 
can see over multiple paths (using multiple /dev/sdX devices) and just 
creates the devmapper mappings as necessary. I'm not even convinced 
multipathd would care about the SBP-2 target port identifier, preferring 
instead to use the WWN on the LUN.

HTH,
Chris

-- 
Chris Boot
bootc@bootc.net

Re: [PATCH 1/3] firewire-sbp2: Take into account Unit_Unique_ID

[Stefan Richter]

On Feb 11 Chris Boot wrote:
> On 11/02/2012 11:12, Stefan Richter wrote:
> > On Feb 10 Chris Boot wrote:
> >> +	if ((leaf[0]&  0xffff0000) != 0x00020000)
> >> +		return -EINVAL;
> >
> > This could be relaxed to "if (leaf[0]<  0x00020000)", but the stricter
> > check is fine too.
> 
> Well the standard does say the length must be exactly 2 rather than just 
> defining it a leaf node that contains an EUI-64. But I did not realise 
> various firmware gets things quite so wrong sometimes...

I expect firmware writers to get length == 2 right, but we need to
protect ourselves against very dumb mistakes or malicious input of course.

Whether the length field is 2 or not could be totally ignored by
firewire-sbp2 just like we ignore the block CRC, except that we need to
catch the corner case of a bogus Config ROM where the descriptor leaf
overlaps the 1 kB ROM size limit, or that it is placed right at the end of
the ROM but is shorter than 2 quadlets.

core-device.c::read_config_rom() already catches the cases of blocks
overlapping the end of the ROM but merely handles them by overwriting the
block length by 0.  Higher-level code which reads a descriptor block or
directory block in the Config ROM cache is required to respect the length
field of the block.  We could simplify this for upper layer code by
overwriting the pointer to the block instead of the header of the block,
like it is done already with pointers to blocks outside of the ROM.

Anyway; if the length of an alleged EUI-64 descriptor leaf is greater than
2, I agree that there is little reason to look at the rest of the
descriptor; it would likely contain garbage too.
-- 
Stefan Richter
-=====-===-- --=- -=-==
http://arcgraph.de/sr/

[PATCH 2/3] firewire-sbp2: Ignore SBP-2 targets on the local node

[Chris Boot]

The firewire-sbp2 module tries to login to an SBP-2/3 target even when
it is running on the local node, which fails because of the inability to
fetch data from DMA mapped regions using firewire transactions on the
local node. It also doesn't make much sense to have the initiator and
target on the same node, so this patch prevents this behaviour.

Signed-off-by: Chris Boot <bootc@bootc.net>
Cc: Stefan Richter <stefanr@s5r6.in-berlin.de>
---
 drivers/firewire/sbp2.c |   19 +++++++++++++++++--
 1 files changed, 17 insertions(+), 2 deletions(-)

diff --git a/drivers/firewire/sbp2.c b/drivers/firewire/sbp2.c
index ed5bbbf..1572a79 100644
--- a/drivers/firewire/sbp2.c
+++ b/drivers/firewire/sbp2.c
@@ -1138,6 +1138,12 @@ static int sbp2_probe(struct device *dev)
 	struct Scsi_Host *shost;
 	u32 model, firmware_revision;
 
+	/* ignore targets on the local node */
+	if (device->node == device->card->local_node) {
+		dev_set_drvdata(&unit->device, NULL);
+		return 0;
+	}
+
 	if (dma_get_max_seg_size(device->card->device) > SBP2_MAX_SEG_SIZE)
 		BUG_ON(dma_set_max_seg_size(device->card->device,
 					    SBP2_MAX_SEG_SIZE));
@@ -1204,6 +1210,10 @@ static void sbp2_update(struct fw_unit *unit)
 	struct sbp2_target *tgt = dev_get_drvdata(&unit->device);
 	struct sbp2_logical_unit *lu;
 
+	/* no drvdata means a target on a local node; ignoring */
+	if (!tgt)
+		return;
+
 	fw_device_enable_phys_dma(fw_parent_device(unit));
 
 	/*
@@ -1223,10 +1233,15 @@ static int sbp2_remove(struct device *dev)
 	struct fw_device *device = fw_parent_device(unit);
 	struct sbp2_target *tgt = dev_get_drvdata(&unit->device);
 	struct sbp2_logical_unit *lu, *next;
-	struct Scsi_Host *shost =
-		container_of((void *)tgt, struct Scsi_Host, hostdata[0]);
+	struct Scsi_Host *shost;
 	struct scsi_device *sdev;
 
+	/* no drvdata means a target on a local node; ignoring */
+	if (!tgt)
+		return 0;
+
+	shost = container_of((void *)tgt, struct Scsi_Host, hostdata[0]);
+
 	/* prevent deadlocks */
 	sbp2_unblock(tgt);
 
-- 
1.7.8.3

Re: [PATCH 2/3] firewire-sbp2: Ignore SBP-2 targets on the local node

[Stefan Richter]

On Feb 10 Chris Boot wrote:
> The firewire-sbp2 module tries to login to an SBP-2/3 target even when
> it is running on the local node, which fails because of the inability to
> fetch data from DMA mapped regions using firewire transactions on the
> local node. It also doesn't make much sense to have the initiator and
> target on the same node, so this patch prevents this behaviour.
> 
> Signed-off-by: Chris Boot <bootc@bootc.net>
> Cc: Stefan Richter <stefanr@s5r6.in-berlin.de>
> ---
>  drivers/firewire/sbp2.c |   19 +++++++++++++++++--
>  1 files changed, 17 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/firewire/sbp2.c b/drivers/firewire/sbp2.c
> index ed5bbbf..1572a79 100644
> --- a/drivers/firewire/sbp2.c
> +++ b/drivers/firewire/sbp2.c
> @@ -1138,6 +1138,12 @@ static int sbp2_probe(struct device *dev)
>  	struct Scsi_Host *shost;
>  	u32 model, firmware_revision;
>  
> +	/* ignore targets on the local node */
> +	if (device->node == device->card->local_node) {
> +		dev_set_drvdata(&unit->device, NULL);
> +		return 0;
> +	}
> +
>  	if (dma_get_max_seg_size(device->card->device) > SBP2_MAX_SEG_SIZE)
>  		BUG_ON(dma_set_max_seg_size(device->card->device,
>  					    SBP2_MAX_SEG_SIZE));
> @@ -1204,6 +1210,10 @@ static void sbp2_update(struct fw_unit *unit)
>  	struct sbp2_target *tgt = dev_get_drvdata(&unit->device);
>  	struct sbp2_logical_unit *lu;
>  
> +	/* no drvdata means a target on a local node; ignoring */
> +	if (!tgt)
> +		return;
> +
>  	fw_device_enable_phys_dma(fw_parent_device(unit));
>  
>  	/*
> @@ -1223,10 +1233,15 @@ static int sbp2_remove(struct device *dev)
>  	struct fw_device *device = fw_parent_device(unit);
>  	struct sbp2_target *tgt = dev_get_drvdata(&unit->device);
>  	struct sbp2_logical_unit *lu, *next;
> -	struct Scsi_Host *shost =
> -		container_of((void *)tgt, struct Scsi_Host, hostdata[0]);
> +	struct Scsi_Host *shost;
>  	struct scsi_device *sdev;
>  
> +	/* no drvdata means a target on a local node; ignoring */
> +	if (!tgt)
> +		return 0;
> +
> +	shost = container_of((void *)tgt, struct Scsi_Host, hostdata[0]);
> +
>  	/* prevent deadlocks */
>  	sbp2_unblock(tgt);
>  

In the long run, we might want to support target and initiator set up to
reside on the same node and talking to each other via loopback, if
somebody really needs it and if it can be done with reasonably little
effort.  Until then, the patch is fine with me WRT what it accomplishes.

But I do wonder:  Shouldn't this be implemented by returning from the
driver probe method with an error?  If so, which errno should be
returned?  (Cc'ing driver core maintainer for this question.)
-- 
Stefan Richter
-=====-===-- --=- -=-==
http://arcgraph.de/sr/

Re: [PATCH 2/3] firewire-sbp2: Ignore SBP-2 targets on the local node

[Clemens Ladisch]

Stefan Richter wrote:
> On Feb 10 Chris Boot wrote:
>> The firewire-sbp2 module tries to login to an SBP-2/3 target even when
>> it is running on the local node, which fails because of the inability to
>> fetch data from DMA mapped regions using firewire transactions on the
>> local node.
>
> In the long run, we might want to support target and initiator set up to
> reside on the same node and talking to each other via loopback, if
> somebody really needs it and if it can be done with reasonably little
> effort.

Handling SBP data packets in the driver is required if we do not want to
allow remote DMA from any device that claims to be a target.  This is
somewhere on my todo list.

>> +	/* ignore targets on the local node */
>> +	if (device->node == device->card->local_node) {
>> +		dev_set_drvdata(&unit->device, NULL);
>> +		return 0;
>> +	}
>
> But I do wonder:  Shouldn't this be implemented by returning from the
> driver probe method with an error?

AFAIK zero means "attach", and the drvdata pointer has no meaning to the
core.

> If so, which errno should be returned?

-ENODEV or -ENXIO.


Regards,
Clemens

Re: [PATCH 2/3] firewire-sbp2: Ignore SBP-2 targets on the local node

[Chris Boot]

On 11/02/2012 12:16, Clemens Ladisch wrote:
> Stefan Richter wrote:
>> On Feb 10 Chris Boot wrote:
>>> The firewire-sbp2 module tries to login to an SBP-2/3 target even when
>>> it is running on the local node, which fails because of the inability to
>>> fetch data from DMA mapped regions using firewire transactions on the
>>> local node.
>>
>> In the long run, we might want to support target and initiator set up to
>> reside on the same node and talking to each other via loopback, if
>> somebody really needs it and if it can be done with reasonably little
>> effort.
>
> Handling SBP data packets in the driver is required if we do not want to
> allow remote DMA from any device that claims to be a target.  This is
> somewhere on my todo list.

I just made it ignore it completely as it just doesn't work at all at 
the moment. If the firewire-sbp2 driver is changed so it could work in 
future, then a module option or similar sounds like a good idea.

>>> +	/* ignore targets on the local node */
>>> +	if (device->node == device->card->local_node) {
>>> +		dev_set_drvdata(&unit->device, NULL);
>>> +		return 0;
>>> +	}
>>
>> But I do wonder:  Shouldn't this be implemented by returning from the
>> driver probe method with an error?
>
> AFAIK zero means "attach", and the drvdata pointer has no meaning to the
> core.
>
>> If so, which errno should be returned?
>
> -ENODEV or -ENXIO.

Perhaps, but the meaning of those isn't quite what is happening here. We 
aren't saying the device doesn't exist or is inaccessible, just that we 
don't want to talk to it...

Chris

-- 
Chris Boot
bootc@bootc.net

Re: [PATCH 2/3] firewire-sbp2: Ignore SBP-2 targets on the local node

[Clemens Ladisch]

Chris Boot wrote:
> On 11/02/2012 12:16, Clemens Ladisch wrote:
>> Stefan Richter wrote:
>>> On Feb 10 Chris Boot wrote:
>>>> +    /* ignore targets on the local node */
>>>> +    if (device->node == device->card->local_node) {
>>>> +        dev_set_drvdata(&unit->device, NULL);
>>>> +        return 0;
>>>> +    }
>>>
>>> But I do wonder:  Shouldn't this be implemented by returning from the
>>> driver probe method with an error? If so, which errno should be returned?
>>
>> -ENODEV or -ENXIO.
>
> Perhaps,

It's what really_probe() in drivers/base/dd.c requires:

	if (ret != -ENODEV && ret != -ENXIO) {
		/* driver matched but the probe failed */
		printk(KERN_WARNING
		       "%s: probe of %s failed with error %d\n",
		       drv->name, dev_name(dev), ret);
	} else {
		pr_debug("%s: probe of %s rejects match %d\n",
		       drv->name, dev_name(dev), ret);
	}

> but the meaning of those isn't quite what is happening here. We aren't
> saying the device doesn't exist or is inaccessible, just that we don't
> want to talk to it...

ENODEV does not mean "no device" but "no _such_ device".


Regards,
Clemens

Re: [PATCH 2/3] firewire-sbp2: Ignore SBP-2 targets on the local node

[Chris Boot]

On 11/02/2012 15:46, Clemens Ladisch wrote:
> Chris Boot wrote:
>> On 11/02/2012 12:16, Clemens Ladisch wrote:
>>> Stefan Richter wrote:
>>>> On Feb 10 Chris Boot wrote:
>>>>> +    /* ignore targets on the local node */
>>>>> +    if (device->node == device->card->local_node) {
>>>>> +        dev_set_drvdata(&unit->device, NULL);
>>>>> +        return 0;
>>>>> +    }
>>>>
>>>> But I do wonder:  Shouldn't this be implemented by returning from the
>>>> driver probe method with an error? If so, which errno should be returned?
>>>
>>> -ENODEV or -ENXIO.
>>
>> Perhaps,
>
> It's what really_probe() in drivers/base/dd.c requires:
>
> 	if (ret != -ENODEV&&  ret != -ENXIO) {
> 		/* driver matched but the probe failed */
> 		printk(KERN_WARNING
> 		       "%s: probe of %s failed with error %d\n",
> 		       drv->name, dev_name(dev), ret);
> 	} else {
> 		pr_debug("%s: probe of %s rejects match %d\n",
> 		       drv->name, dev_name(dev), ret);
> 	}
>
>> but the meaning of those isn't quite what is happening here. We aren't
>> saying the device doesn't exist or is inaccessible, just that we don't
>> want to talk to it...
>
> ENODEV does not mean "no device" but "no _such_ device".

Sounds fair. I'll update my patch.

Cheers,
Chris

-- 
Chris Boot
bootc@bootc.net

Re: [PATCH 2/3] firewire-sbp2: Ignore SBP-2 targets on the local node

[Stefan Richter]

On Feb 10 Chris Boot wrote:
> --- a/drivers/firewire/sbp2.c
> +++ b/drivers/firewire/sbp2.c
> @@ -1138,6 +1138,12 @@ static int sbp2_probe(struct device *dev)
>  	struct Scsi_Host *shost;
>  	u32 model, firmware_revision;
>  
> +	/* ignore targets on the local node */
> +	if (device->node == device->card->local_node) {
> +		dev_set_drvdata(&unit->device, NULL);
> +		return 0;
> +	}

I forgot:  This test can yield false negatives because card->local_node
may change anytime.  Check device->is_local instead.
-- 
Stefan Richter
-=====-===-- --=- -=-==
http://arcgraph.de/sr/

Re: [PATCH 2/3] firewire-sbp2: Ignore SBP-2 targets on the local node

[Chris Boot]

On 11/02/2012 11:56, Stefan Richter wrote:
> On Feb 10 Chris Boot wrote:
>> --- a/drivers/firewire/sbp2.c
>> +++ b/drivers/firewire/sbp2.c
>> @@ -1138,6 +1138,12 @@ static int sbp2_probe(struct device *dev)
>>   	struct Scsi_Host *shost;
>>   	u32 model, firmware_revision;
>>
>> +	/* ignore targets on the local node */
>> +	if (device->node == device->card->local_node) {
>> +		dev_set_drvdata(&unit->device, NULL);
>> +		return 0;
>> +	}
>
> I forgot:  This test can yield false negatives because card->local_node
> may change anytime.  Check device->is_local instead.

Oh. How did I miss that?! I'll get this changed...

Chris

-- 
Chris Boot
bootc@bootc.net

[PATCH 3/3] firewire-sbp2: Fix SCSI sense data mangling

[Chris Boot]

SCSI sense data in SBP-2/3 is carried in an unusual format that means we
have to un-mangle it on our end before we pass it to the SCSI subsystem.
Currently our un-mangling code doesn't quite follow the SBP-2 standard
in that we always assume Current and never Deferred error types, we
never set the VALID bit, and we mishandle the FILEMARK, EOM and ILI
bits.

This patch fixes the sense un-mangling to correctly handle those and
follow the spec.

Signed-off-by: Chris Boot <bootc@bootc.net>
Cc: Stefan Richter <stefanr@s5r6.in-berlin.de>
---
 drivers/firewire/sbp2.c |   13 +++++++++++--
 1 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/firewire/sbp2.c b/drivers/firewire/sbp2.c
index 1572a79..ff632c2 100644
--- a/drivers/firewire/sbp2.c
+++ b/drivers/firewire/sbp2.c
@@ -1318,10 +1318,19 @@ static void sbp2_unmap_scatterlist(struct device *card_device,
 static unsigned int sbp2_status_to_sense_data(u8 *sbp2_status, u8 *sense_data)
 {
 	int sam_status;
+	int sfmt = (sbp2_status[0] >> 6) & 0x03;
 
-	sense_data[0] = 0x70;
+	if (sfmt == 2 || sfmt == 3) {
+		/*
+		 * Reserved for future standardization (2) or
+		 * Status block format vendor-dependent (3)
+		 */
+		return DID_ERROR << 16;
+	}
+
+	sense_data[0] = 0x70 | sfmt | (sbp2_status[1] & 0x80);
 	sense_data[1] = 0x0;
-	sense_data[2] = sbp2_status[1];
+	sense_data[2] = ((sbp2_status[1] << 1) & 0xe0) | (sbp2_status[1] & 0x0f);
 	sense_data[3] = sbp2_status[4];
 	sense_data[4] = sbp2_status[5];
 	sense_data[5] = sbp2_status[6];
-- 
1.7.8.3

[PATCH v2 0/3] firewire-sbp2: Various fixes

[Chris Boot]

In writing my SBP-2 target code I found a few areas in the firewire-sbp2
initiator code that needed some attention:

1. The Unit_Unique_ID property in SBP-2 unit directories is ignored, but this
   should be used when working out the target port name instead of the node's
   GUID if it is present. (Suggestion of Stefan Richter)

2. The initiator tries to login to any and all SBP-2 targets, including ones on
   the local node. This fails due to how the firewire stack / initiator code
   works but is most likely undesirable behaviour, so this patch makes
   firewire-sbp2 ignore SBP-2 targets on the local node.

3. SBP-2 sends SCSI sense data over the wire in an unusual format which
   requires un-mangling before we can pass it to the SCSI subsystem. Some of
   this mangling is currently being done incorrectly, so this patch fixes it.

Changes since v1:
* Don't fail on malformed Unit_Unique_ID properties, just ignore them
* Just return -ENODEV when we encounter the local node

[PATCH v2 1/3] firewire-sbp2: Take into account Unit_Unique_ID

[Chris Boot]

If the target's unit directory contains a Unit_Unique_ID entry, we
should use that as the target's GUID for identification purposes. The
SBP-2 standards document says:

"Although the node unique ID (EUI-64) present in the bus information
block is sufficient to uniquely identify nodes attached to Serial Bus,
it is insufficient to identify a target when a vendor implements a
device with multiple Serial Bus node connections. In this case initiator
software requires information by which a particular target may be
uniquely identified, regardless of the Serial Bus access path used."

[ IEEE T10 P1155D Revision 4, Section 7.6 (page 51) ] and
[ IEEE T10 P1467D Revision 5, Section 7.9 (page 74) ]

Signed-off-by: Chris Boot <bootc@bootc.net>
Cc: Stefan Richter <stefanr@s5r6.in-berlin.de>
Cc: Clemens Ladisch <clemens@ladisch.de>
---
 drivers/firewire/sbp2.c |   12 ++++++++++++
 1 files changed, 12 insertions(+), 0 deletions(-)

diff --git a/drivers/firewire/sbp2.c b/drivers/firewire/sbp2.c
index 80e95aa..a8f43d2 100644
--- a/drivers/firewire/sbp2.c
+++ b/drivers/firewire/sbp2.c
@@ -211,6 +211,7 @@ static struct fw_device *target_device(struct sbp2_target *tgt)
 #define SBP2_CSR_UNIT_CHARACTERISTICS	0x3a
 #define SBP2_CSR_FIRMWARE_REVISION	0x3c
 #define SBP2_CSR_LOGICAL_UNIT_NUMBER	0x14
+#define SBP2_CSR_UNIT_UNIQUE_ID		0x8d
 #define SBP2_CSR_LOGICAL_UNIT_DIRECTORY	0xd4
 
 /* Management orb opcodes */
@@ -997,6 +998,13 @@ static int sbp2_add_logical_unit(struct sbp2_target *tgt, int lun_entry)
 	return 0;
 }
 
+static void sbp2_get_unit_unique_id(struct sbp2_target *tgt,
+				    const u32 *leaf)
+{
+	if ((leaf[0] & 0xffff0000) == 0x00020000)
+		tgt->guid = (u64)leaf[1] << 32 | leaf[2];
+}
+
 static int sbp2_scan_logical_unit_dir(struct sbp2_target *tgt,
 				      const u32 *directory)
 {
@@ -1048,6 +1056,10 @@ static int sbp2_scan_unit_dir(struct sbp2_target *tgt, const u32 *directory,
 				return -ENOMEM;
 			break;
 
+		case SBP2_CSR_UNIT_UNIQUE_ID:
+			sbp2_get_unit_unique_id(tgt, ci.p - 1 + value);
+			break;
+
 		case SBP2_CSR_LOGICAL_UNIT_DIRECTORY:
 			/* Adjust for the increment in the iterator */
 			if (sbp2_scan_logical_unit_dir(tgt, ci.p - 1 + value) < 0)
-- 
1.7.9

[PATCH v2 2/3] firewire-sbp2: Ignore SBP-2 targets on the local node

[Chris Boot]

The firewire-sbp2 module tries to login to an SBP-2/3 target even when
it is running on the local node, which fails because of the inability to
fetch data from DMA mapped regions using firewire transactions on the
local node. It also doesn't make much sense to have the initiator and
target on the same node, so this patch prevents this behaviour.

Signed-off-by: Chris Boot <bootc@bootc.net>
Cc: Stefan Richter <stefanr@s5r6.in-berlin.de>
Cc: Clemens Ladisch <clemens@ladisch.de>
---
 drivers/firewire/sbp2.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/drivers/firewire/sbp2.c b/drivers/firewire/sbp2.c
index a8f43d2..9e9631f 100644
--- a/drivers/firewire/sbp2.c
+++ b/drivers/firewire/sbp2.c
@@ -1133,6 +1133,10 @@ static int sbp2_probe(struct device *dev)
 	struct Scsi_Host *shost;
 	u32 model, firmware_revision;
 
+	/* ignore targets on the local node */
+	if (device->is_local)
+		return -ENODEV;
+
 	if (dma_get_max_seg_size(device->card->device) > SBP2_MAX_SEG_SIZE)
 		BUG_ON(dma_set_max_seg_size(device->card->device,
 					    SBP2_MAX_SEG_SIZE));
-- 
1.7.9

[PATCH v2 3/3] firewire-sbp2: Fix SCSI sense data mangling

[Chris Boot]

SCSI sense data in SBP-2/3 is carried in an unusual format that means we
have to un-mangle it on our end before we pass it to the SCSI subsystem.
Currently our un-mangling code doesn't quite follow the SBP-2 standard
in that we always assume Current and never Deferred error types, we
never set the VALID bit, and we mishandle the FILEMARK, EOM and ILI
bits.

This patch fixes the sense un-mangling to correctly handle those and
follow the spec.

Signed-off-by: Chris Boot <bootc@bootc.net>
Cc: Stefan Richter <stefanr@s5r6.in-berlin.de>
Cc: Clemens Ladisch <clemens@ladisch.de>
---
(This patch is unchanged from v1)

 drivers/firewire/sbp2.c |   13 +++++++++++--
 1 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/firewire/sbp2.c b/drivers/firewire/sbp2.c
index 9e9631f..b12c6ba 100644
--- a/drivers/firewire/sbp2.c
+++ b/drivers/firewire/sbp2.c
@@ -1302,10 +1302,19 @@ static void sbp2_unmap_scatterlist(struct device *card_device,
 static unsigned int sbp2_status_to_sense_data(u8 *sbp2_status, u8 *sense_data)
 {
 	int sam_status;
+	int sfmt = (sbp2_status[0] >> 6) & 0x03;
 
-	sense_data[0] = 0x70;
+	if (sfmt == 2 || sfmt == 3) {
+		/*
+		 * Reserved for future standardization (2) or
+		 * Status block format vendor-dependent (3)
+		 */
+		return DID_ERROR << 16;
+	}
+
+	sense_data[0] = 0x70 | sfmt | (sbp2_status[1] & 0x80);
 	sense_data[1] = 0x0;
-	sense_data[2] = sbp2_status[1];
+	sense_data[2] = ((sbp2_status[1] << 1) & 0xe0) | (sbp2_status[1] & 0x0f);
 	sense_data[3] = sbp2_status[4];
 	sense_data[4] = sbp2_status[5];
 	sense_data[5] = sbp2_status[6];
-- 
1.7.9

Re: [PATCH v2 0/3] firewire-sbp2: Various fixes

[Stefan Richter]

On Feb 15 Chris Boot wrote:
> In writing my SBP-2 target code I found a few areas in the firewire-sbp2
> initiator code that needed some attention:
> 
> 1. The Unit_Unique_ID property in SBP-2 unit directories is ignored, but this
>    should be used when working out the target port name instead of the node's
>    GUID if it is present. (Suggestion of Stefan Richter)
> 
> 2. The initiator tries to login to any and all SBP-2 targets, including ones on
>    the local node. This fails due to how the firewire stack / initiator code
>    works but is most likely undesirable behaviour, so this patch makes
>    firewire-sbp2 ignore SBP-2 targets on the local node.
> 
> 3. SBP-2 sends SCSI sense data over the wire in an unusual format which
>    requires un-mangling before we can pass it to the SCSI subsystem. Some of
>    this mangling is currently being done incorrectly, so this patch fixes it.

Thank you.  Pushed out to linux1394.git
-- 
Stefan Richter
-=====-===-- --=- =-==-
http://arcgraph.de/sr/