lpfc: lpfc no longer discovers lun 255

PATCH In review Michael Reed 5 participants 8 messages Dec 4, 2009 – Dec 10, 2009

Subsystems (2)

SCSI Emulex/Broadcom lpfc FC/FCoE SCSI driver

lore

Share & tools

lore

Michael Reed

author

2009-12-04 16:04 UTC #0 lore raw

PATCH 1/1 lpfc: lpfc no longer discovers lun 255

A change in lun scanning range in scsi_scan.c ( ">" becomes ">=") results in lpfc no longer discovering lun 255 of my test configuration. This patch adjusts lpfc_attr.c to discover lun 255 again.

scsi: host 3 channel 0 id 17 lun255 has a LUN larger than allowed by the host adapter

Signed-off-by Michael Reed <[email protected]>

drivers/scsi/lpfc/lpfc_attr.c +2 -2

@@ -3099,10 +3099,10 @@ LPFC_VPORT_ATTR(discovery_threads, 32, 1
 
 /*
 # lpfc_max_luns: maximum allowed LUN.
-# Value range is [0,65535]. Default value is 255.
+# Value range is [0,65535]. Default value is 256, 1 greater than max lun number.
 # NOTE: The SCSI layer might probe all allowed LUN on some old targets.
 */
-LPFC_VPORT_ATTR_R(max_luns, 255, 0, 65535, "Maximum allowed LUN");
+LPFC_VPORT_ATTR_R(max_luns, 256, 0, 65535, "Maximum allowed LUN");
 
 /*
 # lpfc_poll_tmo: .Milliseconds driver will wait between polling FCP ring.

James Smart

Acked-by on v1 1/1

2009-12-04 17:41 UTC #1 lore raw

Thanks Mike. Wonder if that change broke other drivers too...

Acked-by James Smart <[email protected]>

-- james s

Quoted 27 lines · Michael Reed

A change in lun scanning range in scsi_scan.c ( ">" becomes ">=") results in lpfc no longer discovering lun 255 of my test configuration. This patch adjusts lpfc_attr.c to discover lun 255 again.
scsi: host 3 channel 0 id 17 lun255 has a LUN larger than allowed by the host adapter
Signed-off-by Michael Reed <[email protected]>
drivers/scsi/lpfc/lpfc_attr.c +2 -2
@@ -3099,10 +3099,10 @@ LPFC_VPORT_ATTR(discovery_threads, 32, 1
 
 /*
 # lpfc_max_luns: maximum allowed LUN.
-# Value range is [0,65535]. Default value is 255.
+# Value range is [0,65535]. Default value is 256, 1 greater than max lun number.
 # NOTE: The SCSI layer might probe all allowed LUN on some old targets.
 */
-LPFC_VPORT_ATTR_R(max_luns, 255, 0, 65535, "Maximum allowed LUN");
+LPFC_VPORT_ATTR_R(max_luns, 256, 0, 65535, "Maximum allowed LUN");
 
 /*
 # lpfc_poll_tmo: .Milliseconds driver will wait between polling FCP ring.

Yeah. I'm working on a patch for qla1280.c also. Probably others have been hit. It would have been nice if the patch author had fixed all the drivers instead of introducing this change and waiting for people to discover their last lun is now missing.

Quoted 38 lines · James Smart

Thanks Mike. Wonder if that change broke other drivers too...
Acked-by James Smart <[email protected]>
-- james s
Quoted 17 lines · Michael Reed
A change in lun scanning range in scsi_scan.c ( ">" becomes ">=") results in lpfc no longer discovering lun 255 of my test configuration. This patch adjusts lpfc_attr.c to discover lun 255 again.
scsi: host 3 channel 0 id 17 lun255 has a LUN larger than allowed by the host adapter
Signed-off-by Michael Reed <[email protected]>
drivers/scsi/lpfc/lpfc_attr.c +0 -0
@@ -3099,10 +3099,10 @@ LPFC_VPORT_ATTR(discovery_threads, 32, 1
 
 /*
drivers/scsi/lpfc/lpfc_attr.c LPFC_VPORT_ATTR(discovery_threads, 32, 1 · line 3108
Quoted 10 lines
drivers/scsi/lpfc/lpfc_attr.c +2 -2
 # lpfc_max_luns: maximum allowed LUN.
-# Value range is [0,65535]. Default value is 255.
+# Value range is [0,65535]. Default value is 256, 1 greater than max lun number.
 # NOTE: The SCSI layer might probe all allowed LUN on some old targets.
 */
-LPFC_VPORT_ATTR_R(max_luns, 255, 0, 65535, "Maximum allowed LUN");
+LPFC_VPORT_ATTR_R(max_luns, 256, 0, 65535, "Maximum allowed LUN");
 
 /*
 # lpfc_poll_tmo: .Milliseconds driver will wait between polling FCP ring.
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

James Bottomley

2009-12-04 17:58 UTC #3 lore raw

Quoted 4 lines · Michael Reed

Yeah. I'm working on a patch for qla1280.c also. Probably others have been hit. It would have been nice if the patch author had fixed all the drivers instead of introducing this change and waiting for people to discover their last lun is now missing.

Actually, lets just revert this:

commit 71c309995bff5b5e84253931888b6e8163ee1df0

Author: Ed Lin <[email protected]>
Date:   Fri Oct 9 15:23:27 2009 -0700

[SCSI] fix inconsistent usage of max_lun

Which is causing all the problems. Perhaps we need to update the documentation instead?

James

Michael Reed

author

2009-12-04 18:43 UTC #4 lore raw

Quoted 12 lines · James Bottomley

Quoted 4 lines · Michael Reed
Yeah. I'm working on a patch for qla1280.c also. Probably others have been hit. It would have been nice if the patch author had fixed all the drivers instead of introducing this change and waiting for people to discover their last lun is now missing.
Actually, lets just revert this:
commit 71c309995bff5b5e84253931888b6e8163ee1df0
Author: Ed Lin <[email protected]>
Date:   Fri Oct 9 15:23:27 2009 -0700

Quoted 6 lines

[SCSI] fix inconsistent usage of max_lun
Which is causing all the problems. Perhaps we need to update the documentation instead?
James

As much as I would whine about it, I think moving to a consistent usage of max_lun is a good thing. There are only about 140 references to max_lun in the cscope output so it wouldn't be that difficult for people to go through and fix up their drivers. And, I suspect a good portion will not require any change as they already either work correctly with sequential lun scan by design or have misinterpreted the meaning of max_lun, and thus will continue to work correctly with report lun scan with Mr. Lin's patch applied.

I won't pretend to know the right answer.

Mike

Quoted 6 lines

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Brian King

2009-12-10 13:01 UTC #5 lore raw

Quoted 20 lines · Michael Reed

Quoted 11 lines · James Bottomley
Quoted 4 lines · Michael Reed
Yeah. I'm working on a patch for qla1280.c also. Probably others have been hit. It would have been nice if the patch author had fixed all the drivers instead of introducing this change and waiting for people to discover their last lun is now missing.
Actually, lets just revert this:
commit 71c309995bff5b5e84253931888b6e8163ee1df0
Author: Ed Lin <[email protected]>
Date:   Fri Oct 9 15:23:27 2009 -0700
Quoted 6 lines
[SCSI] fix inconsistent usage of max_lun
Which is causing all the problems. Perhaps we need to update the documentation instead?
James

Quoted 8 lines

As much as I would whine about it, I think moving to a consistent usage of max_lun is a good thing. There are only about 140 references to max_lun in the cscope output so it wouldn't be that difficult for people to go through and fix up their drivers. And, I suspect a good portion will not require any change as they already either work correctly with sequential lun scan by design or have misinterpreted the meaning of max_lun, and thus will continue to work correctly with report lun scan with Mr. Lin's patch applied.

The ibmvfc driver was using 0xffffffff to indicate it essentially had no limit on the number of LUNs. It appears this can no longer be done with the commit above since max_lun is an unsigned int.

-Brian

Signature · 3 lines

Brian King
Linux on Power Virtualization
IBM Linux Technology Center

James Bottomley

2009-12-10 15:43 UTC #6 lore raw

Quoted 30 lines · Brian King

Quoted 20 lines · Michael Reed
Quoted 11 lines · James Bottomley
Quoted 4 lines · Michael Reed
Yeah. I'm working on a patch for qla1280.c also. Probably others have been hit. It would have been nice if the patch author had fixed all the drivers instead of introducing this change and waiting for people to discover their last lun is now missing.
Actually, lets just revert this:
commit 71c309995bff5b5e84253931888b6e8163ee1df0
Author: Ed Lin <[email protected]>
Date:   Fri Oct 9 15:23:27 2009 -0700
Quoted 6 lines
[SCSI] fix inconsistent usage of max_lun
Which is causing all the problems. Perhaps we need to update the documentation instead?
James
Quoted 8 lines
As much as I would whine about it, I think moving to a consistent usage of max_lun is a good thing. There are only about 140 references to max_lun in the cscope output so it wouldn't be that difficult for people to go through and fix up their drivers. And, I suspect a good portion will not require any change as they already either work correctly with sequential lun scan by design or have misinterpreted the meaning of max_lun, and thus will continue to work correctly with report lun scan with Mr. Lin's patch applied.

Quoted 3 lines

The ibmvfc driver was using 0xffffffff to indicate it essentially had no limit on the number of LUNs. It appears this can no longer be done with the commit above since max_lun is an unsigned int.

Right, I've pulled the commit. It was largely cosmetic ... moving what we actually do to match the documentation. However, since it appears everyone reads the code not the docs, the better course might be moving the documentation to match what we actually do.

James

Ed Lin - PTU

2009-12-10 20:19 UTC #7 lore raw

Quoted 47 lines

-----Original Message-----
From: James Bottomley [mailto:[email protected]]
Sent: 2009年12月10日 7:43
To: Brian King
Cc: Michael Reed; James Smart; linux-scsi; Ed Lin - PTU
Subject: Re: [PATCH 1/1] lpfc: lpfc no longer discovers lun 255
Quoted 5 lines · Brian King
Quoted 4 lines · Michael Reed
Quoted 2 lines · James Bottomley
Quoted 1 line · Michael Reed
Yeah. I'm working on a patch for qla1280.c also.
Probably others
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
have been hit. It would have been nice if the patch author had fixed all the drivers instead of introducing this change
and waiting
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
Quoted 1 line
for people to discover their last lun is now missing.
Actually, lets just revert this:
commit 71c309995bff5b5e84253931888b6e8163ee1df0
Author: Ed Lin <[email protected]>
Date:   Fri Oct 9 15:23:27 2009 -0700
[SCSI] fix inconsistent usage of max_lun
Quoted 7 lines
Quoted 7 lines
Quoted 4 lines
Which is causing all the problems. Perhaps we need to update the documentation instead?
James
As much as I would whine about it, I think moving to a consistent usage of max_lun is a good thing. There are only about
140 references
Quoted 4 lines
Quoted 4 lines
to max_lun in the cscope output so it wouldn't be that difficult for people to go through and fix up their drivers. And, I suspect a good portion will not require any change as they already either work correctly with sequential lun scan by design or have
misinterpreted
Quoted 4 lines
Quoted 2 lines
the meaning of max_lun, and thus will continue to work correctly with report lun scan with Mr. Lin's patch applied.
The ibmvfc driver was using 0xffffffff to indicate it
essentially had no limit

Quoted 9 lines

Quoted 1 line
on the number of LUNs. It appears this can no longer be done
with the commit
Quoted 1 line
above since max_lun is an unsigned int.
Right, I've pulled the commit. It was largely cosmetic ... moving what we actually do to match the documentation. However, since it appears everyone reads the code not the docs, the better course might be moving the documentation to match what we actually do.

Well, for the stex driver it is not just cosmetic. Unfortunately the member "lun" in a request message of stex driver is only u8, which means, if user manually scans lun 256, it will become lun 0. The original patch was intended to address the issue.

But I didn't notice the repost lun case could have problem when preparing the original patch. It seems there is no easy fix for it. There is no indication which case is using sequential scan and which one is using report lun. The confusion in the manual scan case will continue (but it may do no harm if no one reported problem). So, I may need to prepare a patch to the stex driver, either changing max_lun to 255 (so that user can not manually scan lun 256), or comparing lun in the scsi command with max_lun to make sure it is in a valid range.

Thanks,

Ed Lin