Patches for SCSI scanning

Patches for SCSI scanning

[Kurt Garloff]

[-- Attachment #1: Type: text/plain, Size: 1131 bytes --]

Hi,

at SUSE, the way that Linux scan SCSI devices has constantly generated
a certain support load. Not extremely but still astonishing.
As a consequence, we have created a number of patches to our 2.4 kernels
that did improve some things, such as the backport of the REPORT_LUNS
scanning from 2.6 and a number of boot/module parameters that allowed
users to influence how the scanning is done.
http://www.suse.de/~garloff/Linux/scsi-scan/scsi-scanning.html

With kernel 2.6, the REPORT_LUNS is standard for SCSI-3 devices,
but still many arrays exist that report as SCSI-2 and will require
BLIST_SPARSELUN|BLIST_LARGELUN.

An alternative to the blacklist is to offer boot/module parameters
again. This is what the following series of 6 patches do.
Most don't really depend on each other, but they'll only apply
cleanly in the order sent.

I did not yet compare our 2.4 blacklist with the 2.6 one; I'll
port our additional 2.4 entries to 2.6 soon.

Regards,
-- 
Kurt Garloff  <garloff@suse.de>                            Cologne, DE 
SUSE LINUX AG, Nuernberg, DE                          SUSE Labs (Head)

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: Patches for SCSI scanning

[James Bottomley]

On Sun, 2004-04-18 at 13:57, Kurt Garloff wrote:
> An alternative to the blacklist is to offer boot/module parameters
> again. This is what the following series of 6 patches do.
> Most don't really depend on each other, but they'll only apply
> cleanly in the order sent.
> 
> I did not yet compare our 2.4 blacklist with the 2.6 one; I'll
> port our additional 2.4 entries to 2.6 soon.

Erm, Kurt, we already have a module load time parameter for altering
both blacklist entries and scanning defaults.

It's dev_flags and default_dev_flags in scsi_devinfo.c

Could you first explain why you can't use this existing infrastructure?

Thanks,

James

Re: Patches for SCSI scanning

[Fabien Salvi]

Kurt Garloff a écrit :
> Hi,
> 
> at SUSE, the way that Linux scan SCSI devices has constantly generated
> a certain support load. Not extremely but still astonishing.
> As a consequence, we have created a number of patches to our 2.4 kernels
> that did improve some things, such as the backport of the REPORT_LUNS
> scanning from 2.6 and a number of boot/module parameters that allowed
> users to influence how the scanning is done.
> http://www.suse.de/~garloff/Linux/scsi-scan/scsi-scanning.html

The correct URL is :
http://www.suse.de/~garloff/linux/scsi-scan/scsi-scanning.html

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

Re: Patches for SCSI scanning

[Kurt Garloff]

[-- Attachment #1: Type: text/plain, Size: 1686 bytes --]

Hi James,

On Sun, Apr 18, 2004 at 06:16:31PM -0500, James Bottomley wrote:
> Erm, Kurt, we already have a module load time parameter for altering
> both blacklist entries and scanning defaults.
> 
> It's dev_flags and default_dev_flags in scsi_devinfo.c
> 
> Could you first explain why you can't use this existing infrastructure?

I've overlooked that possibility.
default_dev_flags obsoletes patch 1 (sparselun and largelun paramters -- 
almost, but max_sparseluns is probably only useful in corner cases), 
and dev_flags obsoletes the patch 6 (llun_blklst).

I'll drop them.

What about the others:
* Patch 2: Drop REPORT_LUN config option and introduce 
  noreportlun and reportlun2 parameters (the latter allowing SCSI-2
  devs to be scanned with REPORT_LUNS).
* Patch 3: Allow host adapters to avoid REPORT_LUNS.
* Patch 4: allow_ghost_devices parameter
* Patch 5: inq_timeout parameter 

If we introduce two more BLIST flags (BLIST_NOREPORTLUN and
BLIST_REPORTLUN2), we can get rid of patch 2 and 3, if we add another one
(BLIST_LUN0ONLINE), we can get rid off 4.

We have 16 bits left, so it should be possible.

If there's consensus to go that way, I can code it up, no problem.
Only patch 5 will remain then ...

Sidenote: We should drop SCSI_FORCELUN and CONFIG_SCSI_MULTI_LUN, otherwise 
people will misconfigure their kernel (such as the HP guy) and start to want 
every multi-lun device listed.

Regards,
-- 
Kurt Garloff                   <kurt@garloff.de>             [Koeln, DE]
Physics:Plasma modeling <garloff@plasimo.phys.tue.nl> [TU Eindhoven, NL]
Linux: SUSE Labs (Head)        <garloff@suse.de>    [SUSE Nuernberg, DE]

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: Patches for SCSI scanning

[Christoph Hellwig]

> * Patch 2: Drop REPORT_LUN config option and introduce 
>   noreportlun and reportlun2 parameters (the latter allowing SCSI-2
>   devs to be scanned with REPORT_LUNS).

I'm all for this one.

> * Patch 3: Allow host adapters to avoid REPORT_LUNS.

Alson sounds okay, do you already have drivers that would set it?

> * Patch 4: allow_ghost_devices parameter

I still think this should be a blacklist flag as it's really a spec
violation from EMC if they really don't get it right.  And I still
haven't an answer why they can't work around it by setting those
magic devices online from userland.

> * Patch 5: inq_timeout parameter

Maybe this one should actually be a per-device tunable in sysfs?  Sure,
to actually make use of it for the root device you'd need an initrd but
the distros do that anyway.

> Sidenote: We should drop SCSI_FORCELUN and CONFIG_SCSI_MULTI_LUN, otherwise 
> people will misconfigure their kernel (such as the HP guy) and start to want 
> every multi-lun device listed.

Agreed.

Re: Patches for SCSI scanning

[Kurt Garloff]

[-- Attachment #1: Type: text/plain, Size: 1970 bytes --]

Hi Christoph,

On Tue, Apr 20, 2004 at 01:04:14PM +0100, Christoph Hellwig wrote:
> > * Patch 2: Drop REPORT_LUN config option and introduce 
> >   noreportlun and reportlun2 parameters (the latter allowing SCSI-2
> >   devs to be scanned with REPORT_LUNS).
> 
> I'm all for this one.

Alternatively, two BLIST flags would do as well.
The more I think about it, the more I like the BLIST approach and
default_dev_flags=0x10000 (NOREPORTLUN) / 0x20000 (REPORTLUN2).

Just wondering whether 0x20000 should be default anyway.
(No, it won't break USB crap, as shost->max_lun will not be > 8
 for usb-storage.)

> > * Patch 3: Allow host adapters to avoid REPORT_LUNS.
> 
> Alson sounds okay, do you already have drivers that would set it?

zfcp on S/390 (as mentioned in the patch).

(With BLIST flags, one would just pass dev_flags=IBM:ESS:0x10000)

> > * Patch 4: allow_ghost_devices parameter
> 
> I still think this should be a blacklist flag as it's really a spec
> violation from EMC if they really don't get it right.  And I still
> haven't an answer why they can't work around it by setting those
> magic devices online from userland.

Me neither.

> > * Patch 5: inq_timeout parameter
> 
> Maybe this one should actually be a per-device tunable in sysfs?  Sure,
> to actually make use of it for the root device you'd need an initrd but
> the distros do that anyway.

Where would you assign it? Per hostadapter?
Per SCSI device is useless, of course, as the device is not yet known
when it gets scanned ...

> > Sidenote: We should drop SCSI_FORCELUN and CONFIG_SCSI_MULTI_LUN, otherwise 
                             BLIST
> > people will misconfigure their kernel (such as the HP guy) and start to want 
> > every multi-lun device listed.
> 
> Agreed.

Regards,
-- 
Kurt Garloff  <garloff@suse.de>                            Cologne, DE 
SUSE LINUX AG, Nuernberg, DE                          SUSE Labs (Head)

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: Patches for SCSI scanning

[James Bottomley]

On Tue, 2004-04-20 at 06:54, Kurt Garloff wrote:
> What about the others:
> * Patch 2: Drop REPORT_LUN config option and introduce 
>   noreportlun and reportlun2 parameters (the latter allowing SCSI-2
>   devs to be scanned with REPORT_LUNS).

Agree, but should be a blacklist flag to force the use of report luns in
the scan.

> * Patch 3: Allow host adapters to avoid REPORT_LUNS.

This should be a black list flag too, but first explain why we need it. 
The only devices we do REPORT_LUNS for are SCSI 3 ones.  Which SCSI 3
device is so badly implemented as to respond incorrectly?  I mean these
are recent devices, so the implementors should have learned from past
mistakes, right ?

> * Patch 4: allow_ghost_devices parameter

Blacklist flag again (oh and "Yuk!!!" by the way).

> * Patch 5: inq_timeout parameter

OK, just remove the arch gating of the default (ppc64 will have to have
a permanent boot/module flag).

> If we introduce two more BLIST flags (BLIST_NOREPORTLUN and
> BLIST_REPORTLUN2), we can get rid of patch 2 and 3, if we add another one
> (BLIST_LUN0ONLINE), we can get rid off 4.
> 
> We have 16 bits left, so it should be possible.
> 
> If there's consensus to go that way, I can code it up, no problem.
> Only patch 5 will remain then ...

Yes, I'm fine with that (as long as the arch specific code from patch 5
goes).

> Sidenote: We should drop SCSI_FORCELUN and CONFIG_SCSI_MULTI_LUN, otherwise 
> people will misconfigure their kernel (such as the HP guy) and start to want 
> every multi-lun device listed.

Yes, sounds reasonable.  We can do it all with the boot flags now.

James

Re: Patches for SCSI scanning

[Kurt Garloff]

[-- Attachment #1: Type: text/plain, Size: 2070 bytes --]

On Tue, Apr 20, 2004 at 09:38:00AM -0500, James Bottomley wrote:
> Agree, but should be a blacklist flag to force the use of report luns in
> the scan.

Sure, I favour that solution as well.

> > * Patch 3: Allow host adapters to avoid REPORT_LUNS.
> 
> This should be a black list flag too, but first explain why we need it. 
> The only devices we do REPORT_LUNS for are SCSI 3 ones.  Which SCSI 3
> device is so badly implemented as to respond incorrectly?  I mean these
> are recent devices, so the implementors should have learned from past
> mistakes, right ?

Linux maps 8 byte LUNs to a 32bit number in scsilun_to_int().
There are several ways to use the 8 bytes, but the mapping assumes
a particular model which is often used and the mapping is nice in
that it results in LUNs being 0, 1, 2, ...
However, there are other possibilities. And as soon as the last
four bytes are non-zero, the mapping will break down :-/
The S/390 folks seem to have such devices.

Anyway, we get the BLIST flag for free when we replace patch 2
with BLIST_NOREPORTLUN and BLIST_REPORTLUN2.

> > * Patch 4: allow_ghost_devices parameter
> 
> Blacklist flag again (oh and "Yuk!!!" by the way).

OK.

> > * Patch 5: inq_timeout parameter
> 
> OK, just remove the arch gating of the default (ppc64 will have to have
> a permanent boot/module flag).

Sure, I already agreed to remove that ugly part.

> > If we introduce two more BLIST flags (BLIST_NOREPORTLUN and
> > BLIST_REPORTLUN2), we can get rid of patch 2 and 3, if we add another one
> > (BLIST_LUN0ONLINE), we can get rid off 4.
> > 
> > We have 16 bits left, so it should be possible.
> > 
> > If there's consensus to go that way, I can code it up, no problem.
> > Only patch 5 will remain then ...
> 
> Yes, I'm fine with that (as long as the arch specific code from patch 5
> goes).

Expect patches later today.

Regards,
-- 
Kurt Garloff  <garloff@suse.de>                            Cologne, DE 
SUSE LINUX AG, Nuernberg, DE                          SUSE Labs (Head)

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: Patches for SCSI scanning

[James Bottomley]

On Tue, 2004-04-20 at 11:03, Kurt Garloff wrote:
> Linux maps 8 byte LUNs to a 32bit number in scsilun_to_int().
> There are several ways to use the 8 bytes, but the mapping assumes
> a particular model which is often used and the mapping is nice in
> that it results in LUNs being 0, 1, 2, ...
> However, there are other possibilities. And as soon as the last
> four bytes are non-zero, the mapping will break down :-/
> The S/390 folks seem to have such devices.

This looks like it needs fixing in our lun mapping support, doesn't it,
rather than trying to work around it with a flag?

I'd rather only have an actual blacklist flag if there's genuinely a
stupid device with a real problem, rather than just a "we don't do this
entirely correctly in the kernel" problem.

James

Re: Patches for SCSI scanning

[Patrick Mansfield]

On Tue, Apr 20, 2004 at 09:38:00AM -0500, James Bottomley wrote:
> On Tue, 2004-04-20 at 06:54, Kurt Garloff wrote:

> > Sidenote: We should drop SCSI_FORCELUN and CONFIG_SCSI_MULTI_LUN, otherwise 
> > people will misconfigure their kernel (such as the HP guy) and start to want 
> > every multi-lun device listed.
> 
> Yes, sounds reasonable.  We can do it all with the boot flags now.

max_scsi_luns should default to zero to avoid devices that should be black
listed as single lun.

So how can you remove those and maintain compatibilty? The removal should
wait for 2.7.

-- Patrick Mansfield

Re: Patches for SCSI scanning

[Kurt Garloff]

[-- Attachment #1: Type: text/plain, Size: 1160 bytes --]

Hi Patrick,

On Tue, Apr 20, 2004 at 09:26:51AM -0700, Patrick Mansfield wrote:
> On Tue, Apr 20, 2004 at 09:38:00AM -0500, James Bottomley wrote:
> > Yes, sounds reasonable.  We can do it all with the boot flags now.
> 
> max_scsi_luns should default to zero to avoid devices that should be black
> listed as single lun.

I strongly disagree.

The target should be to blacklist broken devices not working ones. 

Devices that have multiple LUNs are not, devices that have only one but
report severals are.

With max_scsi_luns=1, we would need to "black"list every single multi lun
device. I very much dislike that idea.

Fortunately, distributors don't do this.

> So how can you remove those and maintain compatibilty? The removal should
> wait for 2.7.

Expecting your multi lun devices to work with max_scsi_luns=1 is just
wrong. max_scsi_luns should not default to 1.
Anyways, BLIST_SPARSELUN will have the side-effect of also working
around that misconfiguration.

Regards,
-- 
Kurt Garloff  <garloff@suse.de>                            Cologne, DE 
SUSE LINUX AG, Nuernberg, DE                          SUSE Labs (Head)

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: Patches for SCSI scanning

[Patrick Mansfield]

On Tue, Apr 20, 2004 at 06:42:38PM +0200, Kurt Garloff wrote:
> Hi Patrick,
> 
> On Tue, Apr 20, 2004 at 09:26:51AM -0700, Patrick Mansfield wrote:
> > On Tue, Apr 20, 2004 at 09:38:00AM -0500, James Bottomley wrote:
> > > Yes, sounds reasonable.  We can do it all with the boot flags now.
> > 
> > max_scsi_luns should default to zero to avoid devices that should be black
> > listed as single lun.
> 
> I strongly disagree.

There were some posts in the past by either Alan or Doug L on this, I
can't find them. Redhat ships with CONFIG_SCSI_MULTI_LUN not set.

> The target should be to blacklist broken devices not working ones. 

I agree that is where we would like to be, but that can break current usage.

> Devices that have multiple LUNs are not, devices that have only one but
> report severals are.
> 
> With max_scsi_luns=1, we would need to "black"list every single multi lun
> device. I very much dislike that idea.

Not individually, just boot with max_scsi_luns=128 or whatever as is done
today for redhat, or boot with default_dev_flags=0x040 (BLIST_SPARSELUN).

> Fortunately, distributors don't do this.

Suse doesn't, Redhat does, I don't know about others.

> > So how can you remove those and maintain compatibilty? The removal should
> > wait for 2.7.
> 
> Expecting your multi lun devices to work with max_scsi_luns=1 is just
> wrong. max_scsi_luns should not default to 1.
> Anyways, BLIST_SPARSELUN will have the side-effect of also working
> around that misconfiguration.

Again that is not compatible with the current 2.6.x code, users with
non-black listed single lun devices will get failures if we default to
max_scsi_luns > 1.

-- Patrick Mansfield

Re: Patches for SCSI scanning

[Kurt Garloff]

[-- Attachment #1: Type: text/plain, Size: 1325 bytes --]

On Tue, Apr 20, 2004 at 06:03:34PM +0200, Kurt Garloff wrote:
> Expect patches later today.

Here we go.

Patch 1: scsi-scan-depr-forcelun
	Document that we don;'t want every single multi-lun device 
	be listed with FORCELUN. That way lays madness.
Patch 2: scsi-scan-blist-replun
	Drop CONFIG_SCSI_REPORT_LUN config param. Instead provide
	BLIST_NOREPORTLUN, which can be used globally (or per device).
	Also add BLIST_REPORTLUN2, which allows REPORT_LUNS even for 
	SCSI-2 if the host adapter support more than 8 LUNs.
	Bump default no of supported LUNs to 511 (so the kmalloc fits
	in one 4k page). 
Patch 3: scsi-scan-no-offl-pq-notcon
	Don't mark devices with PQ 1 or 3 offline. Leave them, so they
	can be accessed via sg. Prevent the sd driver from attachind
	to such devices.
	This is the port of Mike Anderson's patch as suggested by Pat.
Patch 4: scsi-scan-dont-att-pq-notcon
	The other high level drivers (except sg, which is not really 
	high) should not attach to PQ = 1 or 3 devices either.
Patch 5: scsi-scan-inq-timeout
	Make the INQUIRY timeout during SCSI scanning adjustable, so
	users can work around broken devices.

Regards,
-- 
Kurt Garloff  <garloff@suse.de>                            Cologne, DE 
SUSE LINUX AG, Nuernberg, DE                          SUSE Labs (Head)

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: Patches for SCSI scanning

[Kurt Garloff]

[-- Attachment #1: Type: text/plain, Size: 760 bytes --]

On Tue, Apr 20, 2004 at 11:08:49AM -0500, James Bottomley wrote:
> This looks like it needs fixing in our lun mapping support, doesn't it,
> rather than trying to work around it with a flag?

Any ideas?
We need to use full 8byte LUNs then. 
* We can use them as opaque handles. Clean, but resulting in crazy numbers
  which sysadmins will hate us for.
* Reorder the bytes as currently done, but for all 8 of them. Less clean
  conceptually, but resulting in low LUNs and not changing the behaviour
  for already supported devices.

I'm in favour of the second solution.
Want a patch?

Regards,
-- 
Kurt Garloff  <garloff@suse.de>                            Cologne, DE 
SUSE LINUX AG, Nuernberg, DE                          SUSE Labs (Head)

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: Patches for SCSI scanning

[Kurt Garloff]

[-- Attachment #1: Type: text/plain, Size: 848 bytes --]

On Tue, Apr 20, 2004 at 10:44:08AM -0700, Patrick Mansfield wrote:
> Again that is not compatible with the current 2.6.x code, users with
> non-black listed single lun devices will get failures if we default to
> max_scsi_luns > 1.

Failure is good sometimes. 
They can work around with default_dev_flags=1 or better with 
dev_flags=Vendor:Model:1 or in the worst case with max_luns=1.

I suggest to not accept any more FORCELUN entries for now and
remove them all in 2.7.

I still think expecting multi lun devices to work with max_luns=1
(aka CONFIG_SCSI_MUiLTI_LUN=n) is just a configuration error.
I don't like fixing sysadmins' mistakes by adding blacklist entries.

Regards,
-- 
Kurt Garloff  <garloff@suse.de>                            Cologne, DE 
SUSE LINUX AG, Nuernberg, DE                          SUSE Labs (Head)

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

PATCH 1/5: scsi-scan-deprecate-forcelun

[Kurt Garloff]

[-- Attachment #1: Type: text/plain, Size: 1975 bytes --]


Cleanup

Mark BLIST_FORCELUN as deprecated, as we don't want to collect a list
of perfectly working multi-LUN devices with BLIST_FORCELUN. Instead
document max_luns boot/module parameter.

diff -uNrp linux-2.6.5.scsi-orig/drivers/scsi/Kconfig linux-2.6.5.depr-forcelun/drivers/scsi/Kconfig
--- linux-2.6.5.scsi-orig/drivers/scsi/Kconfig	2004-04-21 14:17:50.000000000 +0200
+++ linux-2.6.5.depr-forcelun/drivers/scsi/Kconfig	2004-04-21 14:23:23.000000000 +0200
@@ -167,8 +167,8 @@ config SCSI_MULTI_LUN
 	  can say Y here to force the SCSI driver to probe for multiple LUNs.
 	  A SCSI device with multiple LUNs acts logically like multiple SCSI
 	  devices. The vast majority of SCSI devices have only one LUN, and
-	  so most people can say N here and should in fact do so, because it
-	  is safer.
+	  so most people can say N here. The max_luns boot/module parameter 
+	  allows to override this setting.
 
 config SCSI_REPORT_LUNS
 	bool "Build with SCSI REPORT LUNS support"
diff -uNrp linux-2.6.5.scsi-orig/include/scsi/scsi_devinfo.h linux-2.6.5.depr-forcelun/include/scsi/scsi_devinfo.h
--- linux-2.6.5.scsi-orig/include/scsi/scsi_devinfo.h	2004-04-04 05:36:14.000000000 +0200
+++ linux-2.6.5.depr-forcelun/include/scsi/scsi_devinfo.h	2004-04-21 14:24:40.000000000 +0200
@@ -4,7 +4,8 @@
  * Flags for SCSI devices that need special treatment
  */
 #define BLIST_NOLUN     	0x001	/* Only scan LUN 0 */
-#define BLIST_FORCELUN  	0x002	/* Known to have LUNs, force scanning */
+#define BLIST_FORCELUN  	0x002	/* Known to have LUNs, force scanning,
+					   deprecated: Use max_luns=N */
 #define BLIST_BORKEN    	0x004	/* Flag for broken handshaking */
 #define BLIST_KEY       	0x008	/* unlock by special command */
 #define BLIST_SINGLELUN 	0x010	/* Do not use LUNs in parallel */

-- 
Kurt Garloff  <garloff@suse.de>                            Cologne, DE 
SUSE LINUX AG, Nuernberg, DE                          SUSE Labs (Head)

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

PATCH 2/5: scsi-scan-blist_replun

[Kurt Garloff]

[-- Attachment #1: Type: text/plain, Size: 4604 bytes --]


Cleanup/Feature

Remove CONFIG_SCSI_REPORT_LUNS config option.
Instead provide BLIST_NOREPORTLUN that can be passed as default_dev_flags
(but also per device if needed).
Provide BLIST_REPORTLUN2 that allows trying to use REPORT_LUNS for SCSI-2
devices, if they are connected to a host adapter supporting more than 8 LUNs
(and thus avoiding the usual USB crap to render this feature useless when
used with default_dev_flags).

diff -uNrp linux-2.6.5.depr-forcelun/drivers/scsi/Kconfig linux-2.6.5.replun/drivers/scsi/Kconfig
--- linux-2.6.5.depr-forcelun/drivers/scsi/Kconfig	2004-04-21 14:23:23.000000000 +0200
+++ linux-2.6.5.replun/drivers/scsi/Kconfig	2004-04-21 14:16:15.000000000 +0200
@@ -170,17 +170,6 @@ config SCSI_MULTI_LUN
 	  so most people can say N here. The max_luns boot/module parameter 
 	  allows to override this setting.
 
-config SCSI_REPORT_LUNS
-	bool "Build with SCSI REPORT LUNS support"
-	depends on SCSI
-	default y
-	help
-	  If you want support for SCSI REPORT LUNS, say Y here.
-	  The REPORT LUNS command is useful for devices (such as disk arrays)
-	  with large numbers of LUNs where the LUN values are not contiguous
-	  (sparse LUN).  REPORT LUNS scanning is done only for SCSI-3 devices.
-	  Most users can safely answer N here.
-
 config SCSI_CONSTANTS
 	bool "Verbose SCSI error reporting (kernel size +=12K)"
 	depends on SCSI
diff -uNrp linux-2.6.5.depr-forcelun/drivers/scsi/scsi_scan.c linux-2.6.5.replun/drivers/scsi/scsi_scan.c
--- linux-2.6.5.depr-forcelun/drivers/scsi/scsi_scan.c	2004-04-20 21:54:37.000000000 +0200
+++ linux-2.6.5.replun/drivers/scsi/scsi_scan.c	2004-04-21 14:26:31.000000000 +0200
@@ -80,7 +80,6 @@ module_param_named(max_luns, max_scsi_lu
 MODULE_PARM_DESC(max_luns,
 		 "last scsi LUN (should be between 1 and 2^32-1)");
 
-#ifdef CONFIG_SCSI_REPORT_LUNS
 /*
  * max_scsi_report_luns: the maximum number of LUNS that will be
  * returned from the REPORT LUNS command. 8 times this value must
@@ -88,13 +87,12 @@ MODULE_PARM_DESC(max_luns,
  * in practice, the maximum number of LUNs suppored by any device
  * is about 16k.
  */
-static unsigned int max_scsi_report_luns = 128;
+static unsigned int max_scsi_report_luns = 511;
 
 module_param_named(max_report_luns, max_scsi_report_luns, int, S_IRUGO|S_IWUSR);
 MODULE_PARM_DESC(max_report_luns,
 		 "REPORT LUNS maximum number of LUNS received (should be"
 		 " between 1 and 16384)");
-#endif
 
 /**
  * scsi_unlock_floptical - unlock device via a special MODE SENSE command
@@ -860,7 +858,6 @@ static void scsi_sequential_lun_scan(str
 			return;
 }
 
-#ifdef CONFIG_SCSI_REPORT_LUNS
 /**
  * scsilun_to_int: convert a scsi_lun to an int
  * @scsilun:	struct scsi_lun to be converted.
@@ -921,9 +918,14 @@ static int scsi_report_lun_scan(struct s
 	u8 *data;
 
 	/*
-	 * Only support SCSI-3 and up devices.
-	 */
-	if (sdev->scsi_level < SCSI_3)
+	 * Only support SCSI-3 and up devices if BLIST_NOREPORTLUN is not set.
+	 * Also allow SCSI-2 if BLIST_REPORTLUN2 is set and host adapter does
+	 * support more than 8 LUNs.
+	 */
+	if ((bflags & BLIST_NOREPORTLUN) || 
+	     sdev->scsi_level < SCSI_2 ||
+	    (sdev->scsi_level < SCSI_3 && 
+	     (!(bflags & BLIST_REPORTLUN2) || sdev->host->max_lun <= 8)) )
 		return 1;
 	if (bflags & BLIST_NOLUN)
 		return 0;
@@ -1087,9 +1089,6 @@ static int scsi_report_lun_scan(struct s
 	printk(ALLOC_FAILURE_MSG, __FUNCTION__);
 	return 0;
 }
-#else
-# define scsi_report_lun_scan(sdev, blags, rescan)	(1)
-#endif	/* CONFIG_SCSI_REPORT_LUNS */
 
 struct scsi_device *scsi_add_device(struct Scsi_Host *shost,
 				    uint channel, uint id, uint lun)
diff -uNrp linux-2.6.5.depr-forcelun/include/scsi/scsi_devinfo.h linux-2.6.5.replun/include/scsi/scsi_devinfo.h
--- linux-2.6.5.depr-forcelun/include/scsi/scsi_devinfo.h	2004-04-21 14:24:40.000000000 +0200
+++ linux-2.6.5.replun/include/scsi/scsi_devinfo.h	2004-04-21 14:25:37.000000000 +0200
@@ -20,4 +20,7 @@
 #define BLIST_MS_SKIP_PAGE_08	0x2000	/* do not send ms page 0x08 */
 #define BLIST_MS_SKIP_PAGE_3F	0x4000	/* do not send ms page 0x3f */
 #define BLIST_USE_10_BYTE_MS	0x8000	/* use 10 byte ms before 6 byte ms */
+#define BLIST_NOREPORTLUN	0x10000	/* don't try REPORT_LUNS scan (SCSI-3 devs) */
+#define BLIST_REPORTLUN2	0x20000	/* try REPORT_LUNS even for SCSI-2 devs
+					   (if HBA supports more than 8 LUNs) */
 #endif
-- 
Kurt Garloff  <garloff@suse.de>                            Cologne, DE 
SUSE LINUX AG, Nuernberg, DE                          SUSE Labs (Head)

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

PATCH 3/5: scsi-scan-no-offl-pq-notcon

[Kurt Garloff]

[-- Attachment #1: Type: text/plain, Size: 2874 bytes --]


Bugfix

Don't mark devices with PQ = 1 (or 3) offline, as they can't be accessed
at all then any more, not even by sg.
Instead check in sd HL driver not to connect to NOT_CON or NOT_CAP LUNs.
SPC3, 7.4.2.

diff -uNrp linux-2.6.5.replun/drivers/scsi/scsi_scan.c linux-2.6.5.nooffline/drivers/scsi/scsi_scan.c
--- linux-2.6.5.replun/drivers/scsi/scsi_scan.c	2004-04-21 14:26:31.000000000 +0200
+++ linux-2.6.5.nooffline/drivers/scsi/scsi_scan.c	2004-04-21 14:28:13.000000000 +0200
@@ -542,16 +542,10 @@ static int scsi_add_lun(struct scsi_devi
 	 * 011 the same. Stay compatible with previous code, and create a
 	 * Scsi_Device for a PQ of 1
 	 *
-	 * XXX Save the PQ field let the upper layers figure out if they
-	 * want to attach or not to this device, do not set online FALSE;
-	 * otherwise, offline devices still get an sd allocated, and they
-	 * use up an sd slot.
-	 */
-	if (((inq_result[0] >> 5) & 7) == 1) {
-		SCSI_LOG_SCAN_BUS(3, printk(KERN_INFO "scsi scan: peripheral"
-				" qualifier of 1, device offlined\n"));
-		sdev->online = FALSE;
-	}
+	 * 2004-04-20: Don't set the device offline here; rather let the 
+	 * upper level drivers eval the PQ to decide whether they should
+	 * attach. So remove ((inq_result[0] >> 5) & 7) == 1 check.
+	 */ 
 
 	sdev->removable = (0x80 & inq_result[1]) >> 7;
 	sdev->lockable = sdev->removable;
diff -uNrp linux-2.6.5.replun/drivers/scsi/sd.c linux-2.6.5.nooffline/drivers/scsi/sd.c
--- linux-2.6.5.replun/drivers/scsi/sd.c	2004-04-20 21:51:50.000000000 +0200
+++ linux-2.6.5.nooffline/drivers/scsi/sd.c	2004-04-21 14:28:13.000000000 +0200
@@ -1383,10 +1383,13 @@ static int sd_probe(struct device *dev)
 	struct gendisk *gd;
 	u32 index;
 	int error, devno;
+	int pq = (sdp->inquiry[0] >> 5) & 7;
 
 	error = -ENODEV;
-	if ((sdp->type != TYPE_DISK) && (sdp->type != TYPE_MOD))
+	if (pq != SCSI_INQ_PQ_CON ||
+	    (sdp->type != TYPE_DISK && sdp->type != TYPE_MOD))
 		goto out;
+	
 
 	SCSI_LOG_HLQUEUE(3, printk("sd_attach: scsi device: <%d,%d,%d,%d>\n", 
 			 sdp->host->host_no, sdp->channel, sdp->id, sdp->lun));
diff -uNrp linux-2.6.5.replun/include/scsi/scsi.h linux-2.6.5.nooffline/include/scsi/scsi.h
--- linux-2.6.5.replun/include/scsi/scsi.h	2004-04-20 21:51:40.000000000 +0200
+++ linux-2.6.5.nooffline/include/scsi/scsi.h	2004-04-21 14:28:13.000000000 +0200
@@ -365,6 +365,13 @@ struct scsi_lun {
 #define SCSI_2          3
 #define SCSI_3          4
 
+/*
+ * INQ PERIPHERAL QUALIFIERS
+ */
+#define SCSI_INQ_PQ_CON         0x00
+#define SCSI_INQ_PQ_NOT_CON     0x01
+#define SCSI_INQ_PQ_NOT_CAP     0x03
+
 
 /*
  * Here are some scsi specific ioctl commands which are sometimes useful.
-- 
Kurt Garloff  <garloff@suse.de>                            Cologne, DE 
SUSE LINUX AG, Nuernberg, DE                          SUSE Labs (Head)

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

PATCH 4/5: scsi-scan-dont-att-pq-notcon

[Kurt Garloff]

[-- Attachment #1: Type: text/plain, Size: 2314 bytes --]


Bugfix

Don't attach high level drivers (except sg) to devices with peripheral
qualifier indicating not connected (1) or not capable (3).
SPC3, 7.4.2.

diff -uNrp linux-2.6.5.nooffline/drivers/scsi/osst.c linux-2.6.5.dontattachoffl/drivers/scsi/osst.c
--- linux-2.6.5.nooffline/drivers/scsi/osst.c	2004-04-04 05:37:06.000000000 +0200
+++ linux-2.6.5.dontattachoffl/drivers/scsi/osst.c	2004-04-21 14:29:38.000000000 +0200
@@ -5433,8 +5433,11 @@ static int osst_probe(struct device *dev
 	OSST_buffer    * buffer;
 	struct gendisk * drive;
 	int              i, mode, dev_num;
+	int 		 pq = (SDp->inquiry[0] >> 5) & 7;
 
-	if (SDp->type != TYPE_TAPE || !osst_supports(SDp))
+	if (SDp->type != TYPE_TAPE ||
+	    !osst_supports(SDp) ||
+	    pq != SCSI_INQ_PQ_CON)
 		return -ENODEV;
 
 	drive = alloc_disk(1);
diff -uNrp linux-2.6.5.nooffline/drivers/scsi/sr.c linux-2.6.5.dontattachoffl/drivers/scsi/sr.c
--- linux-2.6.5.nooffline/drivers/scsi/sr.c	2004-04-20 21:51:41.000000000 +0200
+++ linux-2.6.5.dontattachoffl/drivers/scsi/sr.c	2004-04-21 14:29:38.000000000 +0200
@@ -553,9 +553,11 @@ static int sr_probe(struct device *dev)
 	struct gendisk *disk;
 	struct scsi_cd *cd;
 	int minor, error;
+	int pq = (sdev->inquiry[0] >> 5) & 7;
 
 	error = -ENODEV;
-	if (sdev->type != TYPE_ROM && sdev->type != TYPE_WORM)
+	if ((sdev->type != TYPE_ROM && sdev->type != TYPE_WORM) ||
+	    pq != SCSI_INQ_PQ_CON)
 		goto fail;
 
 	if ((error = scsi_device_get(sdev)) != 0)
diff -uNrp linux-2.6.5.nooffline/drivers/scsi/st.c linux-2.6.5.dontattachoffl/drivers/scsi/st.c
--- linux-2.6.5.nooffline/drivers/scsi/st.c	2004-04-04 05:37:36.000000000 +0200
+++ linux-2.6.5.dontattachoffl/drivers/scsi/st.c	2004-04-21 14:29:38.000000000 +0200
@@ -3736,10 +3736,12 @@ static int st_probe(struct device *dev)
 	ST_partstat *STps;
 	ST_buffer *buffer;
 	int i, j, mode, dev_num, error;
+	int pq = (SDp->inquiry[0] >> 5) & 7;
 	char *stp;
 	u64 bounce_limit;
 
-	if (SDp->type != TYPE_TAPE)
+	if (SDp->type != TYPE_TAPE ||
+	    pq != SCSI_INQ_PQ_CON)
 		return -ENODEV;
 	if ((stp = st_incompatible(SDp))) {
 		printk(KERN_INFO
-- 
Kurt Garloff  <garloff@suse.de>                            Cologne, DE 
SUSE LINUX AG, Nuernberg, DE                          SUSE Labs (Head)

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

PATCH 5/5: scsi-scan-inq-timeout

[Kurt Garloff]

[-- Attachment #1: Type: text/plain, Size: 2233 bytes --]


Feature

Make the timeout for INQUIRY during SCSI scan adjustable via boot parameter.
Note that the second INQUIRY does use a shorter timeout, as the long timeout
is for recovery from the initial reset, not because existing devices would
take so long to answer INQUIRY. SPC3 says that INQUIRY should be available
right away, but real life is different unfortunately.

diff -uNrp linux-2.6.5.dontattachoffl/drivers/scsi/scsi_scan.c linux-2.6.5.inq_timeout/drivers/scsi/scsi_scan.c
--- linux-2.6.5.dontattachoffl/drivers/scsi/scsi_scan.c	2004-04-21 14:28:13.000000000 +0200
+++ linux-2.6.5.inq_timeout/drivers/scsi/scsi_scan.c	2004-04-21 14:32:04.000000000 +0200
@@ -94,6 +94,13 @@ MODULE_PARM_DESC(max_report_luns,
 		 "REPORT LUNS maximum number of LUNS received (should be"
 		 " between 1 and 16384)");
 
+static unsigned int scsi_inq_timeout = SCSI_TIMEOUT/HZ+3;
+
+module_param_named(inq_timeout, scsi_inq_timeout, int, S_IRUGO|S_IWUSR);
+MODULE_PARM_DESC(inq_timeout, 
+		 "Timeout (in seconds) waiting for devices to answer INQUIRY."
+		 " Default is 5. Some non-compliant devices need more.");
+
 /**
  * scsi_unlock_floptical - unlock device via a special MODE SENSE command
  * @sreq:	used to send the command
@@ -344,7 +351,7 @@ static void scsi_probe_lun(struct scsi_r
 
 	memset(inq_result, 0, 36);
 	scsi_wait_req(sreq, (void *) scsi_cmd, (void *) inq_result, 36,
-		      SCSI_TIMEOUT + 4 * HZ, 3);
+		      HZ/2 + HZ*scsi_inq_timeout, 3);
 
 	SCSI_LOG_SCAN_BUS(3, printk(KERN_INFO "scsi scan: 1st INQUIRY %s with"
 			" code 0x%x\n", sreq->sr_result ?
@@ -393,7 +400,7 @@ static void scsi_probe_lun(struct scsi_r
 		memset(inq_result, 0, possible_inq_resp_len);
 		scsi_wait_req(sreq, (void *) scsi_cmd,
 			      (void *) inq_result,
-			      possible_inq_resp_len, SCSI_TIMEOUT + 4 * HZ, 3);
+			      possible_inq_resp_len, (1+scsi_inq_timeout)*(HZ/2), 3);
 		SCSI_LOG_SCAN_BUS(3, printk(KERN_INFO "scsi scan: 2nd INQUIRY"
 				" %s with code 0x%x\n", sreq->sr_result ?
 				"failed" : "successful", sreq->sr_result));
-- 
Kurt Garloff  <garloff@suse.de>                            Cologne, DE 
SUSE LINUX AG, Nuernberg, DE                          SUSE Labs (Head)

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: PATCH 4/5: scsi-scan-dont-att-pq-notcon

[Christoph Hellwig]

On Wed, Apr 21, 2004 at 04:14:17PM +0200, Kurt Garloff wrote:
> 
> Bugfix
> 
> Don't attach high level drivers (except sg) to devices with peripheral
> qualifier indicating not connected (1) or not capable (3).
> SPC3, 7.4.2.

Please don't duplicate it in every driver but in scsi_bus_match.  sg uses
a completely different attachment path so it shouldn't be affected by that.

Re: PATCH 2/5: scsi-scan-blist_replun

[Christoph Hellwig]

On Wed, Apr 21, 2004 at 04:12:34PM +0200, Kurt Garloff wrote:
> 
> Cleanup/Feature
> 
> Remove CONFIG_SCSI_REPORT_LUNS config option.
> Instead provide BLIST_NOREPORTLUN that can be passed as default_dev_flags
> (but also per device if needed).
> Provide BLIST_REPORTLUN2

Can we make this simply BLIST_REPORTLUN?  I always have to think why the
2 is there until I guess this could mean only for scsi <= 2..

> -#ifdef CONFIG_SCSI_REPORT_LUNS
>  /*
>   * max_scsi_report_luns: the maximum number of LUNS that will be
>   * returned from the REPORT LUNS command. 8 times this value must
> @@ -88,13 +87,12 @@ MODULE_PARM_DESC(max_luns,
>   * in practice, the maximum number of LUNs suppored by any device
>   * is about 16k.
>   */
> -static unsigned int max_scsi_report_luns = 128;
> +static unsigned int max_scsi_report_luns = 511;

Hmm, where does this number come from?

>  	/*
> -	 * Only support SCSI-3 and up devices.
> -	 */
> -	if (sdev->scsi_level < SCSI_3)
> +	 * Only support SCSI-3 and up devices if BLIST_NOREPORTLUN is not set.
> +	 * Also allow SCSI-2 if BLIST_REPORTLUN2 is set and host adapter does
> +	 * support more than 8 LUNs.

This comment and the nested if is a little confusing.  Also why shouldn't
the arbitrary SCSI2 limit for BLIST_REPORTLUN2?  I'm sure we won't find
SCSI1 devices supporting it, but there should not be any harm leaving out
that check - and doing so makes the logic much cleaner.

	/*
	 * REPORT_LUN is used by default only for SCSI 3 devices (unless
	 * BLIST_NOREPORTLUN is set).  For other devices BLIST_REPORTLUN
	 * enables it if the HBA does support more than 8 LUNs.

	 if (bflags & BLIST_NOREPORTLUN)
	 	return 1;
	 if (sdev->scsi_level < 3 && sdev->host->max_lun <= 8 &&
	     !(bflags & BLIST_REPORTLUN2))
	     	return 1;

Note that I'd prefer not to add BLIST_NOREPORTLUN if the discussing
about the LUN mapping comes to a useable consensus.

Re: PATCH 4/5: scsi-scan-dont-att-pq-notcon

[Kurt Garloff]

[-- Attachment #1.1: Type: text/plain, Size: 667 bytes --]

On Wed, Apr 21, 2004 at 04:02:58PM +0100, Christoph Hellwig wrote:
> On Wed, Apr 21, 2004 at 04:14:17PM +0200, Kurt Garloff wrote:
> > 
> > Don't attach high level drivers (except sg) to devices with peripheral
> > qualifier indicating not connected (1) or not capable (3).
> > SPC3, 7.4.2.
> 
> Please don't duplicate it in every driver but in scsi_bus_match.  sg uses
> a completely different attachment path so it shouldn't be affected by that.

Is it safe to assume it will stay like this?

Regards,
-- 
Kurt Garloff  <garloff@suse.de>                            Cologne, DE 
SUSE LINUX AG, Nuernberg, DE                          SUSE Labs (Head)

[-- Attachment #1.2: scsi-scan-nomatch-pq-notcon --]
[-- Type: text/plain, Size: 659 bytes --]

diff -uNrp linux-2.6.5.attach/drivers/scsi/scsi_sysfs.c linux-2.6.5.noattach/drivers/scsi/scsi_sysfs.c
--- linux-2.6.5.attach/drivers/scsi/scsi_sysfs.c	2004-04-04 05:36:26.000000000 +0200
+++ linux-2.6.5.noattach/drivers/scsi/scsi_sysfs.c	2004-04-21 17:22:22.000000000 +0200
@@ -155,7 +155,9 @@ struct class sdev_class = {
 /* all probing is done in the individual ->probe routines */
 static int scsi_bus_match(struct device *dev, struct device_driver *gendrv)
 {
-	return 1;
+	struct scsi_device *sdp = to_scsi_device(dev);
+	int pq = (sdp->inquiry[0] >> 5) & 7;
+	return (pq == SCSI_INQ_PQ_CON)? 1: 0;
 }
 
 struct bus_type scsi_bus_type = {

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: PATCH 2/5: scsi-scan-blist_replun

[Kurt Garloff]

[-- Attachment #1: Type: text/plain, Size: 2043 bytes --]

Hi Christoph,

On Wed, Apr 21, 2004 at 04:14:12PM +0100, Christoph Hellwig wrote:
> Hmm, where does this number come from?

1 page. I've mentioned it in the email.

> >  	/*
> > -	 * Only support SCSI-3 and up devices.
> > -	 */
> > -	if (sdev->scsi_level < SCSI_3)
> > +	 * Only support SCSI-3 and up devices if BLIST_NOREPORTLUN is not set.
> > +	 * Also allow SCSI-2 if BLIST_REPORTLUN2 is set and host adapter does
> > +	 * support more than 8 LUNs.
> 
> This comment and the nested if is a little confusing.  Also why shouldn't
> the arbitrary SCSI2 limit for BLIST_REPORTLUN2?  I'm sure we won't find
> SCSI1 devices supporting it, but there should not be any harm leaving out
> that check - and doing so makes the logic much cleaner.

It's just straightforward plain simple boolean logic ;-)

I think default_dev_flags=0x20000 would be a good default for many
machines out there, so don't take a risk of breaking SCSI-1 devices.
That's why the check for a HBA which support more than 8 LUNs is in
there as well.

> 	/*
> 	 * REPORT_LUN is used by default only for SCSI 3 devices (unless
> 	 * BLIST_NOREPORTLUN is set).  For other devices BLIST_REPORTLUN
> 	 * enables it if the HBA does support more than 8 LUNs.
> 
> 	 if (bflags & BLIST_NOREPORTLUN)
> 	 	return 1;
> 	 if (sdev->scsi_level < 3 && sdev->host->max_lun <= 8 &&
> 	     !(bflags & BLIST_REPORTLUN2))
> 	     	return 1;
> 
> Note that I'd prefer not to add BLIST_NOREPORTLUN if the discussing
> about the LUN mapping comes to a useable consensus.

I'm sure there will be devices that are SCSI-3 and break on REPORT_LUNS.
It's not just for helping those devs that don't fit into our 32bit LUN
scheme. Also we need to have a replacement for the CONFIG option that
used to be there before.

Regards,
-- 
Kurt Garloff                   <kurt@garloff.de>             [Koeln, DE]
Physics:Plasma modeling <garloff@plasimo.phys.tue.nl> [TU Eindhoven, NL]
Linux: SUSE Labs (Head)        <garloff@suse.de>    [SUSE Nuernberg, DE]

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: PATCH 4/5: scsi-scan-dont-att-pq-notcon

[Mike Anderson]

Kurt Garloff [garloff@suse.de] wrote:
> On Wed, Apr 21, 2004 at 04:02:58PM +0100, Christoph Hellwig wrote:
> > On Wed, Apr 21, 2004 at 04:14:17PM +0200, Kurt Garloff wrote:
> > > 
> > > Don't attach high level drivers (except sg) to devices with peripheral
> > > qualifier indicating not connected (1) or not capable (3).
> > > SPC3, 7.4.2.
> > 
> > Please don't duplicate it in every driver but in scsi_bus_match.  sg uses
> > a completely different attachment path so it shouldn't be affected by that.
> 
> Is it safe to assume it will stay like this?
> 

It should stay like this through 2.6. The driver model does not allow
multi-binding. Also all ULD are using the scsi_register_driver interface
which means prior to there probe routine scsi_bus_match is called.


-andmike
--
Michael Anderson
andmike@us.ibm.com

Re: PATCH 4/5: scsi-scan-dont-att-pq-notcon

[Christoph Hellwig]

On Wed, Apr 21, 2004 at 05:24:36PM +0200, Kurt Garloff wrote:
> > Please don't duplicate it in every driver but in scsi_bus_match.  sg uses
> > a completely different attachment path so it shouldn't be affected by that.
> 
> Is it safe to assume it will stay like this?

Yes.  The driver model assumes to zero or one drivers per device, and to
support sg in that world it needs to be a magic special case.

(also needs a respin of the sd.c part of the previous patch, btw - and while
you're at it can you kill the date in the scsi_scan.c comment?  Everyone can
see when it was applied in bk annotate or the web interface for it)

Re: Patches for SCSI scanning

[James Bottomley]

On Wed, 2004-04-21 at 08:48, Kurt Garloff wrote:
> Any ideas?
> We need to use full 8byte LUNs then. 
> * We can use them as opaque handles. Clean, but resulting in crazy numbers

Actually, I favour this.  Internally we can a structure for the lun. 
All we need to modify are the output routines (setting struct device
bus_id) so that we print it correctly according to LUN type.

>   which sysadmins will hate us for.

Only if the device requests a flat space and then has huge numbers ...
in which case, we're only reporting what the device actually said...

James

Re: PATCH 4/5: scsi-scan-dont-att-pq-notcon

[James Bottomley]

On Wed, 2004-04-21 at 09:14, Kurt Garloff wrote:
> Don't attach high level drivers (except sg) to devices with peripheral
> qualifier indicating not connected (1) or not capable (3).
> SPC3, 7.4.2.

I'd really rather the pq field became part of struct scsi_device (just
like type is) so that it's parsed out in one place (scsi_scan.c) on the
initial inquiry.  Then each ULD can simply look in that field.

This will future proof us against any expansion of the field.

James

Re: PATCH 2/5: scsi-scan-blist_replun

[Kurt Garloff]

[-- Attachment #1: Type: text/plain, Size: 682 bytes --]

On Wed, Apr 21, 2004 at 04:14:12PM +0100, Christoph Hellwig wrote:
> > Remove CONFIG_SCSI_REPORT_LUNS config option.
> > Instead provide BLIST_NOREPORTLUN that can be passed as default_dev_flags
> > (but also per device if needed).
> > Provide BLIST_REPORTLUN2
> 
> Can we make this simply BLIST_REPORTLUN?  I always have to think why the
> 2 is there until I guess this could mean only for scsi <= 2..

The 2 is indicating for SCSI-2, indeed, as normally REPORT_LUNS is only 
for SCSI-3. I like it this way.

Regards,
-- 
Kurt Garloff  <garloff@suse.de>                            Cologne, DE 
SUSE LINUX AG, Nuernberg, DE                          SUSE Labs (Head)

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: PATCH 4/5: scsi-scan-dont-att-pq-notcon

[Kurt Garloff]

[-- Attachment #1: Type: text/plain, Size: 1411 bytes --]

Hi,

On Wed, Apr 21, 2004 at 04:33:36PM +0100, Christoph Hellwig wrote:
> On Wed, Apr 21, 2004 at 05:24:36PM +0200, Kurt Garloff wrote:
> > > Please don't duplicate it in every driver but in scsi_bus_match.  sg uses
> > > a completely different attachment path so it shouldn't be affected by that.
> > 
> > Is it safe to assume it will stay like this?
> 
> Yes.  The driver model assumes to zero or one drivers per device, and to
> support sg in that world it needs to be a magic special case.

OK, that seems safe enough.

> (also needs a respin of the sd.c part of the previous patch, btw - and while

Sure.

> you're at it can you kill the date in the scsi_scan.c comment?  Everyone can
> see when it was applied in bk annotate or the web interface for it)

Except that I'm not allowed to use BK. But sure, the date can go ...

But unfortunately James prefers yet another approach, where we add
a inq_pq field to struct scsi_device ...
Patrick did not like it, if I parsed his mail correctly. 
So we've conflicting requirements.

I'll wait for this to be sorted. If it's only used at one place
(bus_match), I believe parsing it directly from the inquiry data
is perfectly fine. The code dupl in UL drivers was not.

Regards,
-- 
Kurt Garloff  <garloff@suse.de>                            Cologne, DE 
SUSE LINUX AG, Nuernberg, DE                          SUSE Labs (Head)

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: PATCH 4/5: scsi-scan-dont-att-pq-notcon

[Kurt Garloff]

[-- Attachment #1: Type: text/plain, Size: 3159 bytes --]

On Wed, Apr 21, 2004 at 04:33:36PM +0100, Christoph Hellwig wrote:
> (also needs a respin of the sd.c part of the previous patch, btw - and while
> you're at it can you kill the date in the scsi_scan.c comment?  Everyone can
> see when it was applied in bk annotate or the web interface for it)

OTOH, there's so few to be changed that I can resend right away ;-)

garloff@suse.de

Bugfix

Don't mark devices with PQ = 1 (or 3) offline, as they can't be accessed
at all then any more, not even by sg.
Don't attach other upper level drivers if PQ != 0.
SPC3, 7.4.2.

diff -uNrp linux-2.6.5.replun/drivers/scsi/scsi_scan.c linux-2.6.5.nooffline/drivers/scsi/scsi_scan.c
--- linux-2.6.5.replun/drivers/scsi/scsi_scan.c	2004-04-21 14:26:31.000000000 +0200
+++ linux-2.6.5.nooffline/drivers/scsi/scsi_scan.c	2004-04-21 14:28:13.000000000 +0200
@@ -542,16 +542,10 @@ static int scsi_add_lun(struct scsi_devi
 	 * 011 the same. Stay compatible with previous code, and create a
 	 * Scsi_Device for a PQ of 1
 	 *
-	 * XXX Save the PQ field let the upper layers figure out if they
-	 * want to attach or not to this device, do not set online FALSE;
-	 * otherwise, offline devices still get an sd allocated, and they
-	 * use up an sd slot.
-	 */
-	if (((inq_result[0] >> 5) & 7) == 1) {
-		SCSI_LOG_SCAN_BUS(3, printk(KERN_INFO "scsi scan: peripheral"
-				" qualifier of 1, device offlined\n"));
-		sdev->online = FALSE;
-	}
+	 * Don't set the device offline here; rather let the upper
+	 * level drivers eval the PQ to decide whether they should
+	 * attach. So remove ((inq_result[0] >> 5) & 7) == 1 check.
+	 */ 
 
 	sdev->removable = (0x80 & inq_result[1]) >> 7;
 	sdev->lockable = sdev->removable;
diff -uNrp linux-2.6.5.replun/include/scsi/scsi.h linux-2.6.5.nooffline/include/scsi/scsi.h
--- linux-2.6.5.replun/include/scsi/scsi.h	2004-04-20 21:51:40.000000000 +0200
+++ linux-2.6.5.nooffline/include/scsi/scsi.h	2004-04-21 14:28:13.000000000 +0200
@@ -365,6 +365,13 @@ struct scsi_lun {
 #define SCSI_2          3
 #define SCSI_3          4
 
+/*
+ * INQ PERIPHERAL QUALIFIERS
+ */
+#define SCSI_INQ_PQ_CON         0x00
+#define SCSI_INQ_PQ_NOT_CON     0x01
+#define SCSI_INQ_PQ_NOT_CAP     0x03
+
 
 /*
  * Here are some scsi specific ioctl commands which are sometimes useful.
diff -uNrp linux-2.6.5.attach/drivers/scsi/scsi_sysfs.c linux-2.6.5.noattach/drivers/scsi/scsi_sysfs.c
--- linux-2.6.5.attach/drivers/scsi/scsi_sysfs.c	2004-04-04 05:36:26.000000000 +0200
+++ linux-2.6.5.noattach/drivers/scsi/scsi_sysfs.c	2004-04-21 17:22:22.000000000 +0200
@@ -155,7 +155,9 @@ struct class sdev_class = {
 /* all probing is done in the individual ->probe routines */
 static int scsi_bus_match(struct device *dev, struct device_driver *gendrv)
 {
-	return 1;
+	struct scsi_device *sdp = to_scsi_device(dev);
+	int pq = (sdp->inquiry[0] >> 5) & 7;
+	return (pq == SCSI_INQ_PQ_CON)? 1: 0;
 }
 
 struct bus_type scsi_bus_type = {

Regards,
-- 
Kurt Garloff  <garloff@suse.de>                            Cologne, DE 
SUSE LINUX AG, Nuernberg, DE                          SUSE Labs (Head)

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: PATCH 4/5: scsi-scan-dont-att-pq-notcon

[James Bottomley]

On Wed, 2004-04-21 at 11:08, Kurt Garloff wrote:
> But unfortunately James prefers yet another approach, where we add
> a inq_pq field to struct scsi_device ...
> Patrick did not like it, if I parsed his mail correctly. 
> So we've conflicting requirements.
> 
> I'll wait for this to be sorted. If it's only used at one place
> (bus_match), I believe parsing it directly from the inquiry data
> is perfectly fine. The code dupl in UL drivers was not.

Well, I'm happy to have the debate.

My principle is that inquiry data should be abstracted as much as
possible on the grounds that it's the hottest piece of the standard in
terms of everyone grabbing fields to indicate extra features.  I'd like
problems caused by inquiry field changes to be confined to scsi_scan.c
(so we have a single parsing routine for common inquiry fields.  The
thing I can definitely see someone wanting to do is to overflow either
the type or pq field).

There's logic to parsing data where it's needed, but it makes it
difficult to locate all the places when it changes...

James

Re: PATCH 4/5: scsi-scan-dont-att-pq-notcon

[Patrick Mansfield]

On Wed, Apr 21, 2004 at 11:18:30AM -0500, James Bottomley wrote:
> On Wed, 2004-04-21 at 11:08, Kurt Garloff wrote:
> > But unfortunately James prefers yet another approach, where we add
> > a inq_pq field to struct scsi_device ...
> > Patrick did not like it, if I parsed his mail correctly. 
> > So we've conflicting requirements.

Yes.

> > I'll wait for this to be sorted. If it's only used at one place
> > (bus_match), I believe parsing it directly from the inquiry data
> > is perfectly fine. The code dupl in UL drivers was not.

And yeh I agree.

> 
> Well, I'm happy to have the debate.
> 
> My principle is that inquiry data should be abstracted as much as
> possible on the grounds that it's the hottest piece of the standard in
> terms of everyone grabbing fields to indicate extra features.  I'd like
> problems caused by inquiry field changes to be confined to scsi_scan.c
> (so we have a single parsing routine for common inquiry fields.  The
> thing I can definitely see someone wanting to do is to overflow either
> the type or pq field).
> 
> There's logic to parsing data where it's needed, but it makes it
> difficult to locate all the places when it changes...

So add a macro or inline function to parse the data, like a sdev_pq(sdev),
then we have one location for the parsing, and don't have to duplicate the
data. And if sdev->inquiry changes or goes away we need only change
sdev_pq().

-- Patrick Mansfield

Re: PATCH 4/5: scsi-scan-dont-att-pq-notcon

[Kurt Garloff]

[-- Attachment #1: Type: text/plain, Size: 3455 bytes --]

On Wed, Apr 21, 2004 at 11:18:30AM -0500, James Bottomley wrote:
> There's logic to parsing data where it's needed, but it makes it
> difficult to locate all the places when it changes...

OK, then consider this patch.


diff -uNrp linux-2.6.5/drivers/scsi/scsi_scan.c linux-2.6.5.nooffl/drivers/scsi/scsi_scan.c
--- linux-2.6.5/drivers/scsi/scsi_scan.c	2004-04-21 18:53:52.000000000 +0200
+++ linux-2.6.5.nooffl/drivers/scsi/scsi_scan.c	2004-04-21 18:56:02.000000000 +0200
@@ -542,17 +542,12 @@ static int scsi_add_lun(struct scsi_devi
 	 * 011 the same. Stay compatible with previous code, and create a
 	 * Scsi_Device for a PQ of 1
 	 *
-	 * XXX Save the PQ field let the upper layers figure out if they
-	 * want to attach or not to this device, do not set online FALSE;
-	 * otherwise, offline devices still get an sd allocated, and they
-	 * use up an sd slot.
-	 */
-	if (((inq_result[0] >> 5) & 7) == 1) {
-		SCSI_LOG_SCAN_BUS(3, printk(KERN_INFO "scsi scan: peripheral"
-				" qualifier of 1, device offlined\n"));
-		sdev->online = FALSE;
-	}
+	 * Don't set the device offline here; rather let the upper
+	 * level drivers eval the PQ to decide whether they should
+	 * attach. So remove ((inq_result[0] >> 5) & 7) == 1 check.
+	 */ 
 
+	sdev->inq_periph_qual = (inq_result[0] >> 5) & 7;
 	sdev->removable = (0x80 & inq_result[1]) >> 7;
 	sdev->lockable = sdev->removable;
 	sdev->soft_reset = (inq_result[7] & 1) && ((inq_result[3] & 7) == 2);
diff -uNrp linux-2.6.5/drivers/scsi/scsi_sysfs.c linux-2.6.5.nooffl/drivers/scsi/scsi_sysfs.c
--- linux-2.6.5/drivers/scsi/scsi_sysfs.c	2004-04-04 05:36:26.000000000 +0200
+++ linux-2.6.5.nooffl/drivers/scsi/scsi_sysfs.c	2004-04-21 18:56:33.000000000 +0200
@@ -155,7 +155,8 @@ struct class sdev_class = {
 /* all probing is done in the individual ->probe routines */
 static int scsi_bus_match(struct device *dev, struct device_driver *gendrv)
 {
-	return 1;
+	struct scsi_device *sdp = to_scsi_device(dev);
+	return (sdp->inq_periph_qual == SCSI_INQ_PQ_CON)? 1: 0;
 }
 
 struct bus_type scsi_bus_type = {
diff -uNrp linux-2.6.5/include/scsi/scsi.h linux-2.6.5.nooffl/include/scsi/scsi.h
--- linux-2.6.5/include/scsi/scsi.h	2004-04-21 18:53:51.000000000 +0200
+++ linux-2.6.5.nooffl/include/scsi/scsi.h	2004-04-21 18:54:43.000000000 +0200
@@ -365,6 +365,13 @@ struct scsi_lun {
 #define SCSI_2          3
 #define SCSI_3          4
 
+/*
+ * INQ PERIPHERAL QUALIFIERS
+ */
+#define SCSI_INQ_PQ_CON         0x00
+#define SCSI_INQ_PQ_NOT_CON     0x01
+#define SCSI_INQ_PQ_NOT_CAP     0x03
+
 
 /*
  * Here are some scsi specific ioctl commands which are sometimes useful.
diff -uNrp linux-2.6.5/include/scsi/scsi_device.h linux-2.6.5.nooffl/include/scsi/scsi_device.h
--- linux-2.6.5/include/scsi/scsi_device.h	2004-04-21 18:53:45.000000000 +0200
+++ linux-2.6.5.nooffl/include/scsi/scsi_device.h	2004-04-21 18:55:21.000000000 +0200
@@ -59,6 +59,7 @@ struct scsi_device {
 	char devfs_name[256];	/* devfs junk */
 	char type;
 	char scsi_level;
+	char inq_periph_qual;	/* PQ from INQUIRY data */	
 	unsigned char inquiry_len;	/* valid bytes in 'inquiry' */
 	unsigned char * inquiry;	/* INQUIRY response data */
 	char * vendor;		/* [back_compat] point into 'inquiry' ... */

-- 
Kurt Garloff  <garloff@suse.de>                            Cologne, DE 
SUSE LINUX AG, Nuernberg, DE                          SUSE Labs (Head)

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: PATCH 5/5: scsi-scan-inq-timeout

[Patrick Mansfield]

On Wed, Apr 21, 2004 at 04:14:56PM +0200, Kurt Garloff wrote:
> 
> Feature
> 
> Make the timeout for INQUIRY during SCSI scan adjustable via boot parameter.
> Note that the second INQUIRY does use a shorter timeout, as the long timeout
> is for recovery from the initial reset, not because existing devices would
> take so long to answer INQUIRY. SPC3 says that INQUIRY should be available
> right away, but real life is different unfortunately.
> 
> diff -uNrp linux-2.6.5.dontattachoffl/drivers/scsi/scsi_scan.c linux-2.6.5.inq_timeout/drivers/scsi/scsi_scan.c
> --- linux-2.6.5.dontattachoffl/drivers/scsi/scsi_scan.c	2004-04-21 14:28:13.000000000 +0200
> +++ linux-2.6.5.inq_timeout/drivers/scsi/scsi_scan.c	2004-04-21 14:32:04.000000000 +0200
> @@ -94,6 +94,13 @@ MODULE_PARM_DESC(max_report_luns,
>  		 "REPORT LUNS maximum number of LUNS received (should be"
>  		 " between 1 and 16384)");
>  
> +static unsigned int scsi_inq_timeout = SCSI_TIMEOUT/HZ+3;
> +
> +module_param_named(inq_timeout, scsi_inq_timeout, int, S_IRUGO|S_IWUSR);
> +MODULE_PARM_DESC(inq_timeout, 
> +		 "Timeout (in seconds) waiting for devices to answer INQUIRY."
> +		 " Default is 5. Some non-compliant devices need more.");
> +
>  /**
>   * scsi_unlock_floptical - unlock device via a special MODE SENSE command
>   * @sreq:	used to send the command
> @@ -344,7 +351,7 @@ static void scsi_probe_lun(struct scsi_r
>  
>  	memset(inq_result, 0, 36);
>  	scsi_wait_req(sreq, (void *) scsi_cmd, (void *) inq_result, 36,
> -		      SCSI_TIMEOUT + 4 * HZ, 3);
> +		      HZ/2 + HZ*scsi_inq_timeout, 3);

SCSI_TIMEOUT is 2 * HZ.

So we defaulted to 6 seconds, and now we default to 5.5 seconds. Why?

Why not get rid of the HZ/2, and set scsi_inq_timeout = SCSI_TIMEOUT/HZ +
4 so we default to 6 again?

So if a device needs at least N + .5 seconds then just set the
scsi_inq_timeout to N + 1.

>  
>  	SCSI_LOG_SCAN_BUS(3, printk(KERN_INFO "scsi scan: 1st INQUIRY %s with"
>  			" code 0x%x\n", sreq->sr_result ?
> @@ -393,7 +400,7 @@ static void scsi_probe_lun(struct scsi_r
>  		memset(inq_result, 0, possible_inq_resp_len);
>  		scsi_wait_req(sreq, (void *) scsi_cmd,
>  			      (void *) inq_result,
> -			      possible_inq_resp_len, SCSI_TIMEOUT + 4 * HZ, 3);
> +			      possible_inq_resp_len, (1+scsi_inq_timeout)*(HZ/2), 3);

IMO just make the two timeouts the same and avoid any potential problems
(like some funky device always takes 4 seconds to reponsd to an INQUIRY).

-- Patrick Mansfield

Re: PATCH 5/5: scsi-scan-inq-timeout

[Kurt Garloff]

[-- Attachment #1: Type: text/plain, Size: 2111 bytes --]

Hi Patrick,

On Wed, Apr 21, 2004 at 01:24:54PM -0700, Patrick Mansfield wrote:
> SCSI_TIMEOUT is 2 * HZ.
> 
> So we defaulted to 6 seconds, and now we default to 5.5 seconds. Why?

True.

I have added half a second to be safe against 0.
Thus, I had the choice of rounding 6 to 6.5 or 5.5.
I chose 5.5, because the old value was chosen very conservative, as it
had to be enough for even sick devices. Now we have a boot parameter
to accomodate these, thus we can be less conservative. Thus 5.5.

> Why not get rid of the HZ/2, and set scsi_inq_timeout = SCSI_TIMEOUT/HZ +
> 4 so we default to 6 again?
> 
> So if a device needs at least N + .5 seconds then just set the
> scsi_inq_timeout to N + 1.

Why do you dislike HZ/2. The number is computed by the compiler at
compile time ...

> >  	SCSI_LOG_SCAN_BUS(3, printk(KERN_INFO "scsi scan: 1st INQUIRY %s with"
> >  			" code 0x%x\n", sreq->sr_result ?
> > @@ -393,7 +400,7 @@ static void scsi_probe_lun(struct scsi_r
> >  		memset(inq_result, 0, possible_inq_resp_len);
> >  		scsi_wait_req(sreq, (void *) scsi_cmd,
> >  			      (void *) inq_result,
> > -			      possible_inq_resp_len, SCSI_TIMEOUT + 4 * HZ, 3);
> > +			      possible_inq_resp_len, (1+scsi_inq_timeout)*(HZ/2), 3);
> 
> IMO just make the two timeouts the same and avoid any potential problems
> (like some funky device always takes 4 seconds to reponsd to an INQUIRY).

It's really only the reset recivery time which makes us choose such long
times. Otherwise SCSI_TIMEOUT would be _plenty_ of time to answer an 
INQUIRY. An INQUIRY is easier: No medium access is needed, see SPC3.
When this second INQUIRY is done, a first one has succeeded already, so 
the recovery from the reset has happened already.

Anyway, if you dislike it, I don't care much. If the first INQUIRY
succeeded, it's very unlikely that the second one fails, so the timeout
is even less important in real life.

Regards,
-- 
Kurt Garloff  <garloff@suse.de>                            Cologne, DE 
SUSE LINUX AG, Nuernberg, DE                          SUSE Labs (Head)

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: PATCH 4/5: scsi-scan-dont-att-pq-notcon

[Kurt Garloff]

[-- Attachment #1: Type: text/plain, Size: 956 bytes --]

On Wed, Apr 21, 2004 at 09:55:01AM -0700, Patrick Mansfield wrote:
> On Wed, Apr 21, 2004 at 11:18:30AM -0500, James Bottomley wrote:
> > There's logic to parsing data where it's needed, but it makes it
> > difficult to locate all the places when it changes...
> 
> So add a macro or inline function to parse the data, like a sdev_pq(sdev),
> then we have one location for the parsing, and don't have to duplicate the
> data. And if sdev->inquiry changes or goes away we need only change
> sdev_pq().

#define sdev_pg(sdev) ((sdev->inquiry[0] >> 5) & 7)

Just there is no scsi_scan.h where we could stick this.
So either we have it at some non-obvious place or just once in
scsi_sysfs.c. Or in scsi_scan.c and add one char to struct scsi_dev.
I believe it's a very minor issue.

Regards,
-- 
Kurt Garloff  <garloff@suse.de>                            Cologne, DE 
SUSE LINUX AG, Nuernberg, DE                          SUSE Labs (Head)

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: PATCH 5/5: scsi-scan-inq-timeout

[Patrick Mansfield]

On Thu, Apr 22, 2004 at 12:48:43AM +0200, Kurt Garloff wrote:
> Hi Patrick,
> 
> On Wed, Apr 21, 2004 at 01:24:54PM -0700, Patrick Mansfield wrote:
> > SCSI_TIMEOUT is 2 * HZ.
> > 
> > So we defaulted to 6 seconds, and now we default to 5.5 seconds. Why?
> 
> True.
> 
> I have added half a second to be safe against 0.
> Thus, I had the choice of rounding 6 to 6.5 or 5.5.
> I chose 5.5, because the old value was chosen very conservative, as it
> had to be enough for even sick devices. Now we have a boot parameter
> to accomodate these, thus we can be less conservative. Thus 5.5.

I have no idea why 6 seconds was picked, but it has been there for so
long, it should remain as the default. If a user picks 0, then too bad.
(It would be nice if there was some module_param with a range, so it could
never be set to 0, it looks like there is stuff in place for adding your
own types and your own "check" macro.)

> > Why not get rid of the HZ/2, and set scsi_inq_timeout = SCSI_TIMEOUT/HZ +
> > 4 so we default to 6 again?
> > 
> > So if a device needs at least N + .5 seconds then just set the
> > scsi_inq_timeout to N + 1.
> 
> Why do you dislike HZ/2. The number is computed by the compiler at
> compile time ...

It's not the HZ/2 I dislike, just the adjustment of the timeout value.

> > >  	SCSI_LOG_SCAN_BUS(3, printk(KERN_INFO "scsi scan: 1st INQUIRY %s with"
> > >  			" code 0x%x\n", sreq->sr_result ?
> > > @@ -393,7 +400,7 @@ static void scsi_probe_lun(struct scsi_r
> > >  		memset(inq_result, 0, possible_inq_resp_len);
> > >  		scsi_wait_req(sreq, (void *) scsi_cmd,
> > >  			      (void *) inq_result,
> > > -			      possible_inq_resp_len, SCSI_TIMEOUT + 4 * HZ, 3);
> > > +			      possible_inq_resp_len, (1+scsi_inq_timeout)*(HZ/2), 3);
> > 
> > IMO just make the two timeouts the same and avoid any potential problems
> > (like some funky device always takes 4 seconds to reponsd to an INQUIRY).
> 
> It's really only the reset recivery time which makes us choose such long
> times. Otherwise SCSI_TIMEOUT would be _plenty_ of time to answer an 
> INQUIRY. An INQUIRY is easier: No medium access is needed, see SPC3.
> When this second INQUIRY is done, a first one has succeeded already, so 
> the recovery from the reset has happened already.

> Anyway, if you dislike it, I don't care much. If the first INQUIRY
> succeeded, it's very unlikely that the second one fails, so the timeout
> is even less important in real life.

It should be plenty of time, but you can't guarantee it, so it's better to
be cautious and just use the same timeout.

-- Patrick Mansfield

Re: PATCH 4/5: scsi-scan-dont-att-pq-notcon

[Patrick Mansfield]

On Thu, Apr 22, 2004 at 12:51:37AM +0200, Kurt Garloff wrote:
> On Wed, Apr 21, 2004 at 09:55:01AM -0700, Patrick Mansfield wrote:
> > On Wed, Apr 21, 2004 at 11:18:30AM -0500, James Bottomley wrote:
> > > There's logic to parsing data where it's needed, but it makes it
> > > difficult to locate all the places when it changes...
> > 
> > So add a macro or inline function to parse the data, like a sdev_pq(sdev),
> > then we have one location for the parsing, and don't have to duplicate the
> > data. And if sdev->inquiry changes or goes away we need only change
> > sdev_pq().
> 
> #define sdev_pg(sdev) ((sdev->inquiry[0] >> 5) & 7)
> 
> Just there is no scsi_scan.h where we could stick this.
> So either we have it at some non-obvious place or just once in
> scsi_sysfs.c. Or in scsi_scan.c and add one char to struct scsi_dev.
> I believe it's a very minor issue.

Yes, it's minor; James hasn't replied, and there aren't any other macros
like I'm suggesting, so go with the extra char.

-- Patrick Mansfield

Re: PATCH 4/5: scsi-scan-dont-att-pq-notcon

[James Bottomley]

On Thu, 2004-04-22 at 16:39, Patrick Mansfield wrote:
> Yes, it's minor; James hasn't replied, and there aren't any other macros
> like I'm suggesting, so go with the extra char.

Well ... I'm agnostic.  I care about the abstraction rather than the
implementation of the abstraction.  I suppose I have a slight preference
for an extra field, because that can be fixed up if there's an
identified problem with the inquiry data but I can't think of a single
case where that would be necessary.

James