scsi_dh: Make scsi device handler modules automatically inserted

Hello,

Currently, SCSI targets doesn't have modalias support. It wasn't an issue until SCSI device handler came along.

We want the SCSI device handler modules to be insmodded automatically when the specific SCSI targets are probed and found.

This set of patches adds the modalias support for SCSI targets and also makes the relevant changes to SCSI device handler modules to make use of it.

Applies cleanly on 2.6.29-rc8 and is tested on the same.

Please review and consider this for 2.6.30.

Thanks & Regards,

chandra

drivers/scsi/scsi_sysfs.c +5 -0

+	rc = format_scsi_modalias(sdev, buffer, 500);
+	if (rc < 0)
+		return rc;
+
+	add_uevent_var(env, "MODALIAS=%s", buffer);
 	return 0;
 }

(quoted diff) +1 -0

+	return rc;
 }

.. snip..

drivers/scsi/scsi_sysfs.c static int scsi_bus_match(struct device · line 409

Quoted 4 lines

static int scsi_bus_uevent(struct device *dev, struct kobj_uevent_env *env) {

drivers/scsi/scsi_sysfs.c +1 -0

 	struct scsi_device *sdev;
+	char buffer[501];

Why '501' ? How did you come up with that number?

drivers/scsi/scsi_sysfs.c static int scsi_bus_match(struct device · line 417

Quoted 9 lines

drivers/scsi/scsi_sysfs.c +2 -1

+	int rc;
 
 	if (dev->type != &scsi_dev_type)
 		return 0;
 
 	sdev = to_scsi_device(dev);
 
-	add_uevent_var(env, "MODALIAS=" SCSI_DEVICE_MODALIAS_FMT, sdev->type);
+	buffer[500] = '\0';

Shouldn't that be buffer[501] ?

Quoted 7 lines · Konrad Rzeszutek

.. snip..

drivers/scsi/scsi_sysfs.c static int scsi_bus_match(struct device · line 409

Quoted 4 lines

static int scsi_bus_uevent(struct device *dev, struct kobj_uevent_env *env) {

drivers/scsi/scsi_sysfs.c +1 -0

 	struct scsi_device *sdev;
+	char buffer[501];

Why '501' ? How did you come up with that number?

A random 500 bytes of data plus room for a string terminator?

Quoted 8 lines

Quoted 8 lines

drivers/scsi/scsi_sysfs.c +1 -0

+	int rc;
 
 	if (dev->type != &scsi_dev_type)
 		return 0;
 
 	sdev = to_scsi_device(dev);

Quoted 4 lines

drivers/scsi/scsi_sysfs.c static int scsi_bus_match(struct device · line 417

Quoted 2 lines

drivers/scsi/scsi_sysfs.c +1 -1

-	add_uevent_var(env, "MODALIAS=" SCSI_DEVICE_MODALIAS_FMT, sdev->type);
+	buffer[500] = '\0';

Shouldn't that be buffer[501] ?

No, buffer[501] would be off the end of the reserved space. The definition char buf[501] allows you to access from buf[0] to buf[500].

James

.. snip..

drivers/scsi/device_handler/scsi_dh_alua.c static int alua_prep_fn(struct scsi_devi · line 694

Quoted 5 lines

drivers/scsi/device_handler/scsi_dh_alua.c +5 -0

+	{TYPE_ANY, "HP", "HSV300" },
+	{TYPE_ANY, "IBM", "2107900" },
+	{TYPE_ANY, "IBM", "2145" },
+	{TYPE_ANY, "Pillar", "Axiom" },
+	{0, "", ""},

Is there a define for '0' value?

Quoted 6 lines

drivers/scsi/device_handler/scsi_dh.c +0 -0

@@ -75,7 +75,10 @@ static int scsi_dh_handler_lookup(struct
 {
 	int i, found = 0;
 

drivers/scsi/device_handler/scsi_dh.c static int scsi_dh_handler_lookup(struct · line 84

Quoted 8 lines

drivers/scsi/device_handler/scsi_dh.c +4 -1

-	for(i = 0; scsi_dh->devlist[i].vendor; i++) {
+	for(i = 0; scsi_dh->devlist[i].vendor[0]; i++) {
+		if ((scsi_dh->devlist[i].type != TYPE_ANY) &&
+				(scsi_dh->devlist[i].type != sdev->type))
+			continue;
 		if (!strncmp(sdev->vendor, scsi_dh->devlist[i].vendor,
 			     strlen(scsi_dh->devlist[i].vendor)) &&
 		    !strncmp(sdev->model, scsi_dh->devlist[i].model,

Quoted 8 lines · Konrad Rzeszutek

.. snip..

Review

Quoted 5 lines

(quoted diff) +5 -0

+	{TYPE_ANY, "HP", "HSV300" },
+	{TYPE_ANY, "IBM", "2107900" },
+	{TYPE_ANY, "IBM", "2145" },
+	{TYPE_ANY, "Pillar", "Axiom" },
+	{0, "", ""},

Is there a define for '0' value?

This is an array terminator. Almost nobody uses cpp defines for terminating values. (In this case, the actual terminator is "" in the 2nd struct member.)

BTW, Peter,

Quoted 6 lines

Quoted 6 lines

drivers/scsi/device_handler/scsi_dh.c +0 -0

@@ -75,7 +75,10 @@ static int scsi_dh_handler_lookup(struct
 {
 	int i, found = 0;
 

Quoted 8 lines

drivers/scsi/device_handler/scsi_dh.c static int scsi_dh_handler_lookup(struct · line 84

Quoted 8 lines

drivers/scsi/device_handler/scsi_dh.c +4 -1

-	for(i = 0; scsi_dh->devlist[i].vendor; i++) {
+	for(i = 0; scsi_dh->devlist[i].vendor[0]; i++) {
+		if ((scsi_dh->devlist[i].type != TYPE_ANY) &&
+				(scsi_dh->devlist[i].type != sdev->type))
+			continue;
 		if (!strncmp(sdev->vendor, scsi_dh->devlist[i].vendor,
 			     strlen(scsi_dh->devlist[i].vendor)) &&
 		    !strncmp(sdev->model, scsi_dh->devlist[i].model,

AFAICS you could have kept the check for .vendor != NULL instead of .vendor[0] != '\0' and write the terminator thusly:

	{},

Saves the space for a one byte long string in the object files. :-)

Quoted 5 lines · Stefan Richter

AFAICS you could have kept the check for .vendor != NULL instead of .vendor[0] != '\0' and write the terminator thusly:

	{},

Quoted 1 line

Saves the space for a one byte long string in the object files. :-)

Tried it after your response. Got a panic :(.

Did I code your suggestion correctly ?

Here is the patch

Quoted 9 lines · Chandra Seetharaman

Quoted 6 lines · Stefan Richter

AFAICS you could have kept the check for .vendor != NULL instead of .vendor[0] != '\0' and write the terminator thusly:

     {},

Saves the space for a one byte long string in the object files.  :-)

Tried it after your response. Got a panic :(.

From: Peter Jones <[email protected]>

This patch allows the use of modaliases on scsi targets to correctly load scsi device handler modules when the devices are found. +++ linux-2.6.28/include/linux/mod_devicetable.h

From: Peter Jones <[email protected]>

This patch allows the use of modaliases on scsi targets to correctly load scsi device handler modules when the devices are found.

Signed-off-by Peter Jones <[email protected]>

Signed-off-by Chandra Seetharaman <[email protected]>

Quoted 6 lines · Chandra Seetharaman

Quoted 5 lines · Stefan Richter

AFAICS you could have kept the check for .vendor != NULL instead of .vendor[0] != '\0' and write the terminator thusly:

	{},

Quoted 7 lines

Quoted 1 line

Saves the space for a one byte long string in the object files. :-)

Tried it after your response. Got a panic :(.

Did I code your suggestion correctly ?

Here is the patch

...

Was scsi_dh_rdac the only device handler which you built? (All others configured off?)

Quoted 7 lines · Chandra Seetharaman

From: Peter Jones <[email protected]>

This patch allows the use of modaliases on scsi targets to correctly load scsi device handler modules when the devices are found.

Signed-off-by Peter Jones <[email protected]>

Signed-off-by Chandra Seetharaman <[email protected]>

I have to say this is a bit icky.

overloading the modalias type like this produces several nasty effects:

1. You don't actually care about type for any of the scsi_dh handlers, so they all have it as a useless extra field
2. TYPE_ANY is a bogus (non SAM) definition ... I suppose it's unlikely ever to clash, but you never know

From (1) and (2) are you suggesting _not_ to use the TYPE field for

3. scsi_dh handlers would now get loaded on *any* system ... regardless of whether it's using multipathing or not ... that's going to cause problems with other multi path solutions, I bet.

Why not use a separately defined module alias for this ... and then program udev to understand it and do the loading only if multipath is actually present? I think we might have to add the INQUIRY strings to the uenv for this, but it would be a much more elegant solution.

James

Quoted 9 lines · Chandra Seetharaman

From: Peter Jones <[email protected]>

This patch allows the use of modaliases on scsi targets to correctly load scsi device handler modules when the devices are found. +++ linux-2.6.28/include/linux/mod_devicetable.h

include/linux/mod_devicetable.h +0 -0

@@ -454,4 +454,10 @@ struct dmi_system_id {
 
 #define DMI_MATCH(a, b)        { a, b }
 

include/linux/mod_devicetable.h struct dmi_system_id { · line 461

Quoted 5 lines

include/linux/mod_devicetable.h +5 -0

+struct scsi_dh_device_id {
+       unsigned char type;
+       char vendor[9];
+       char model[17];
+};

Doesn't the static array waste space, when used for the long lists of entries stuffed in arrays of this structure? It will carry a lot of \0 chars, and identical strings can not be de-duplicated by the compiler, unlike when pointers are used?

Thanks,
Kay
--
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

Quoted 5 lines · Stefan Richter

Quoted 4 lines · Chandra Seetharaman

Quoted 3 lines · Stefan Richter

AFAICS you could have kept the check for .vendor != NULL instead of .vendor[0] != '\0' and write the terminator thusly:

Quoted 4 lines

Quoted 4 lines

Quoted 3 lines

     {},

Saves the space for a one byte long string in the object files. :-)

Tried it after your response. Got a panic :(.

It's a static char array not a pointer to a string.

yes (all others are off).

that is the only storage I have in my system

Quoted 3 lines

Quoted 3 lines · Stefan Richter

Quoted 2 lines

AFAICS you could have kept the check for .vendor != NULL instead of .vendor[0] != '\0'

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

and write the terminator thusly:

	{},

Quoted 1 line

Quoted 1 line

Quoted 1 line

Saves the space for a one byte long string in the object files. :-)

Quoted 13 lines · James Bottomley

Quoted 7 lines · Chandra Seetharaman

From: Peter Jones <[email protected]>

This patch allows the use of modaliases on scsi targets to correctly load scsi device handler modules when the devices are found.

Signed-off-by Peter Jones <[email protected]>

Signed-off-by Chandra Seetharaman <[email protected]>

I have to say this is a bit icky.

overloading the modalias type like this produces several nasty effects:

Quoted 6 lines

1. You don't actually care about type for any of the scsi_dh handlers, so they all have it as a useless extra field
2. TYPE_ANY is a bogus (non SAM) definition ... I suppose it's unlikely ever to clash, but you never know

From (1) and (2) are you suggesting _not_ to use the TYPE field for

scsi_dh handlers ?

Quoted 3 lines

3. scsi_dh handlers would now get loaded on *any* system ... regardless of whether it's using multipathing or not ... that's going to cause problems with other multi path solutions, I bet.

Actually that is the intent. We _do_ want scsi_dh handlers to be loaded before multipath comes into picture.

Basically we want the handlers to be available ASAP after the device is configured. The reason is:
- These devices are active/passive, SCSI doesn't know about it and any I/O sent to SCSI will be sent down to the device, irrespective of the active/passive nature of the device in that path. When the device (path) is passive, I/O leads to time delay and extraneous error message (these are the two main reasons we moved the device handler code from dm-multipath layer to SCSI(scsi_dh)).

With scsi_dh, I/O to the passive path will be short circuited in prep_fn() and errors are REQ_QUIET'd.

Currently I suggest users to add these modules to their initrd to make them available ASAP (as described at
http://sources.redhat.com/lvm2/wiki/MultipathUsageGuide#head-fb3efbb82fa69ca86b7db26423c235ae6c280caa)

If we have the support thru modalias, then appropriate modules will be included in initrd by the installer, thereby making it easier for the user.

BTW, dm-multipath currently have code to insert appropriate modules if needed (if they are not already made available).

Quoted 14 lines · Chandra Seetharaman

Quoted 13 lines · James Bottomley

Quoted 7 lines · Chandra Seetharaman

From: Peter Jones <[email protected]>

This patch allows the use of modaliases on scsi targets to correctly load scsi device handler modules when the devices are found.

Signed-off-by Peter Jones <[email protected]>

Signed-off-by Chandra Seetharaman <[email protected]>

I have to say this is a bit icky.

overloading the modalias type like this produces several nasty effects:

Quoted 7 lines

Quoted 6 lines

     1. You don't actually care about type for any of the scsi_dh handlers, so they all have it as a useless extra field
     2. TYPE_ANY is a bogus (non SAM) definition ... I suppose it's unlikely ever to clash, but you never know

From (1) and (2) are you suggesting _not_ to use the TYPE field for

scsi_dh handlers ?

Well, you don't ever set it to anything other than TYPE_ANY, do you? so it's completely superfluous as far as you're concerned.  That's why overloading the SCSI ULD modalias looks rather contrived.

Quoted 23 lines

Quoted 3 lines

     3. scsi_dh handlers would now get loaded on *any* system ... regardless of whether it's using multipathing or not ... that's going to cause problems with other multi path solutions, I bet.

Actually that is the intent. We _do_ want scsi_dh handlers to be loaded before multipath comes into picture.

Basically we want the handlers to be available ASAP after the device is configured. The reason is:
 - These devices are active/passive, SCSI doesn't know about it and any I/O sent to SCSI will be sent down to the device, irrespective of the active/passive nature of the device in that path. When the device (path) is passive, I/O leads to time delay and extraneous error message (these are the two main reasons we moved the device handler code from dm-multipath layer to SCSI(scsi_dh)).

With scsi_dh, I/O to the passive path will be short circuited in prep_fn() and errors are REQ_QUIET'd.

Currently I suggest users to add these modules to their initrd to make them available ASAP (as described at
http://sources.redhat.com/lvm2/wiki/MultipathUsageGuide#head-fb3efbb82fa69ca86b7db26423c235ae6c280caa)

Quoted 3 lines

If we have the support thru modalias, then appropriate modules will be included in initrd by the installer, thereby making it easier for the user.

Um, is that correct?  the MODALIAS env is lost as soon as it goes through udev.  For initrd to base module selection on the MODALIAS, it will have to reconstruct it after the fact (and so need modifying to use the new modalias).

Quoted 50 lines · James Bottomley

Quoted 14 lines · Chandra Seetharaman

Quoted 13 lines · James Bottomley

Quoted 7 lines · Chandra Seetharaman

From: Peter Jones <[email protected]>

This patch allows the use of modaliases on scsi targets to correctly load scsi device handler modules when the devices are found.

Signed-off-by Peter Jones <[email protected]>

Signed-off-by Chandra Seetharaman <[email protected]>

I have to say this is a bit icky.

overloading the modalias type like this produces several nasty effects:

Quoted 7 lines

Quoted 6 lines

1. You don't actually care about type for any of the scsi_dh handlers, so they all have it as a useless extra field
2. TYPE_ANY is a bogus (non SAM) definition ... I suppose it's unlikely ever to clash, but you never know

From (1) and (2) are you suggesting _not_ to use the TYPE field for

scsi_dh handlers ?

Well, you don't ever set it to anything other than TYPE_ANY, do you? so it's completely superfluous as far as you're concerned. That's why overloading the SCSI ULD modalias looks rather contrived.

Quoted 23 lines

Quoted 3 lines

3. scsi_dh handlers would now get loaded on *any* system ... regardless of whether it's using multipathing or not ... that's going to cause problems with other multi path solutions, I bet.

Actually that is the intent. We _do_ want scsi_dh handlers to be loaded before multipath comes into picture.

Basically we want the handlers to be available ASAP after the device is configured. The reason is:
- These devices are active/passive, SCSI doesn't know about it and any I/O sent to SCSI will be sent down to the device, irrespective of the active/passive nature of the device in that path. When the device (path) is passive, I/O leads to time delay and extraneous error message (these are the two main reasons we moved the device handler code from dm-multipath layer to SCSI(scsi_dh)).

With scsi_dh, I/O to the passive path will be short circuited in prep_fn() and errors are REQ_QUIET'd.

Currently I suggest users to add these modules to their initrd to make them available ASAP (as described at
http://sources.redhat.com/lvm2/wiki/MultipathUsageGuide#head-fb3efbb82fa69ca86b7db26423c235ae6c280caa)

Quoted 8 lines

Quoted 3 lines

If we have the support thru modalias, then appropriate modules will be included in initrd by the installer, thereby making it easier for the user.

Um, is that correct? the MODALIAS env is lost as soon as it goes through udev. For initrd to base module selection on the MODALIAS, it will have to reconstruct it after the fact (and so need modifying to use the new modalias).

A device's modalias is usually also available as an attribute called "modalias". The entire device uevent environment is in almost all cases also available by reading the "uevent" file of the device at any time later.

Does that help? I might misread your "MODALIAS env is lost" sentence.

Quoted 14 lines · Chandra Seetharaman

Quoted 13 lines · James Bottomley

Quoted 7 lines · Chandra Seetharaman

From: Peter Jones <[email protected]>

This patch allows the use of modaliases on scsi targets to correctly load scsi device handler modules when the devices are found.

Signed-off-by Peter Jones <[email protected]>

Signed-off-by Chandra Seetharaman <[email protected]>

I have to say this is a bit icky.

overloading the modalias type like this produces several nasty effects:

Quoted 7 lines

Quoted 6 lines

1. You don't actually care about type for any of the scsi_dh handlers, so they all have it as a useless extra field
2. TYPE_ANY is a bogus (non SAM) definition ... I suppose it's unlikely ever to clash, but you never know

From (1) and (2) are you suggesting _not_ to use the TYPE field for

scsi_dh handlers ?

Well, you don't ever set it to anything other than TYPE_ANY, do you? so it's completely superfluous as far as you're concerned. That's why overloading the SCSI ULD modalias looks rather contrived.

Quoted 23 lines

Quoted 3 lines

3. scsi_dh handlers would now get loaded on *any* system ... regardless of whether it's using multipathing or not ... that's going to cause problems with other multi path solutions, I bet.

Actually that is the intent. We _do_ want scsi_dh handlers to be loaded before multipath comes into picture.

Basically we want the handlers to be available ASAP after the device is configured. The reason is:
- These devices are active/passive, SCSI doesn't know about it and any I/O sent to SCSI will be sent down to the device, irrespective of the active/passive nature of the device in that path. When the device (path) is passive, I/O leads to time delay and extraneous error message (these are the two main reasons we moved the device handler code from dm-multipath layer to SCSI(scsi_dh)).

With scsi_dh, I/O to the passive path will be short circuited in prep_fn() and errors are REQ_QUIET'd.

Currently I suggest users to add these modules to their initrd to make them available ASAP (as described at
http://sources.redhat.com/lvm2/wiki/MultipathUsageGuide#head-fb3efbb82fa69ca86b7db26423c235ae6c280caa)

Quoted 3 lines

If we have the support thru modalias, then appropriate modules will be included in initrd by the installer, thereby making it easier for the user.

Um, is that correct? the MODALIAS env is lost as soon as it goes through udev. For initrd to base module selection on the MODALIAS, it will have to reconstruct it after the fact (and so need modifying to use the new modalias).

Quoted 2 lines

BTW, dm-multipath currently have code to insert appropriate modules if needed (if they are not already made available).

So all of this is just to keep the initrd boot quiet?

James

Quoted 10 lines · Kay Sievers

Quoted 9 lines · Chandra Seetharaman

From: Peter Jones <[email protected]>

This patch allows the use of modaliases on scsi targets to correctly load scsi device handler modules when the devices are found. +++ linux-2.6.28/include/linux/mod_devicetable.h

include/linux/mod_devicetable.h +0 -0

@@ -454,4 +454,10 @@ struct dmi_system_id {
 
 #define DMI_MATCH(a, b)        { a, b }
 

Quoted 11 lines

include/linux/mod_devicetable.h struct dmi_system_id { · line 461

Quoted 5 lines

include/linux/mod_devicetable.h +5 -0

+struct scsi_dh_device_id {
+       unsigned char type;
+       char vendor[9];
+       char model[17];
+};

Doesn't the static array waste space, when used for the long lists of entries stuffed in arrays of this structure? It will carry a lot of \0 chars, and identical strings can not be de-duplicated by the compiler, unlike when pointers are used?

I had some problems when we used the pointers instead of arrays. Don't recall it now.

Will retry and report.

Quoted 6 lines

Thanks,
Kay
--
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

Quoted 9 lines · Chandra Seetharaman

Quoted 8 lines · James Bottomley

Quoted 7 lines · Chandra Seetharaman

From: Peter Jones <[email protected]>

This patch allows the use of modaliases on scsi targets to correctly load scsi device handler modules when the devices are found.

Signed-off-by Peter Jones <[email protected]>

Signed-off-by Chandra Seetharaman <[email protected]>

Quoted 8 lines

Quoted 8 lines

I have to say this is a bit icky.

overloading the modalias type like this produces several nasty effects:

1. You don't actually care about type for any of the scsi_dh handlers, so they all have it as a useless extra field
2. TYPE_ANY is a bogus (non SAM) definition ... I suppose it's unlikely ever to clash, but you never know

Quoted 2 lines

Quoted 1 line

From (1) and (2) are you suggesting _not_ to use the TYPE field for

scsi_dh handlers ?

Well, you don't ever set it to anything other than TYPE_ANY, do you? so it's completely superfluous as far as you're concerned. That's why overloading the SCSI ULD modalias looks rather contrived.

Quoted 2 lines

BTW, dm-multipath currently have code to insert appropriate modules if needed (if they are not already made available).

So all of this is just to keep the initrd boot quiet?

Quoted 21 lines · Chandra Seetharaman

Quoted 10 lines · Kay Sievers

Quoted 9 lines · Chandra Seetharaman

From: Peter Jones <[email protected]>

This patch allows the use of modaliases on scsi targets to correctly load scsi device handler modules when the devices are found. +++ linux-2.6.28/include/linux/mod_devicetable.h

include/linux/mod_devicetable.h +0 -0

@@ -454,4 +454,10 @@ struct dmi_system_id {
 
 #define DMI_MATCH(a, b)        { a, b }
 

Quoted 10 lines

include/linux/mod_devicetable.h struct dmi_system_id { · line 461

Quoted 5 lines

include/linux/mod_devicetable.h +5 -0

+struct scsi_dh_device_id {
+       unsigned char type;
+       char vendor[9];
+       char model[17];
+};

Doesn't the static array waste space, when used for the long lists of entries stuffed in arrays of this structure? It will carry a lot of \0 chars, and identical strings can not be de-duplicated by the compiler, unlike when pointers are used?

Quoted 4 lines

I had some problems when we used the pointers instead of arrays. Don't recall it now.

Will retry and report.

Hi Kay,

I tried it and realized modpost is what is giving the problem.

When the array for vendor and model is changed to pointers, modpost (at the end of make modules) dies with a segmentation fault.

Peter, Any details that you can provide ?

Quoted 27 lines · Chandra Seetharaman

Quoted 21 lines · Chandra Seetharaman

Quoted 10 lines · Kay Sievers

Quoted 9 lines · Chandra Seetharaman

From: Peter Jones <[email protected]>

This patch allows the use of modaliases on scsi targets to correctly load scsi device handler modules when the devices are found. +++ linux-2.6.28/include/linux/mod_devicetable.h

include/linux/mod_devicetable.h +0 -0

@@ -454,4 +454,10 @@ struct dmi_system_id {
 
 #define DMI_MATCH(a, b)        { a, b }
 

Quoted 10 lines

include/linux/mod_devicetable.h struct dmi_system_id { · line 461

Quoted 5 lines

include/linux/mod_devicetable.h +5 -0

+struct scsi_dh_device_id {
+       unsigned char type;
+       char vendor[9];
+       char model[17];
+};

Doesn't the static array waste space, when used for the long lists of entries stuffed in arrays of this structure? It will carry a lot of \0 chars, and identical strings can not be de-duplicated by the compiler, unlike when pointers are used?

Quoted 4 lines

I had some problems when we used the pointers instead of arrays. Don't recall it now.

Will retry and report.

Quoted 8 lines

Hi Kay,

I tried it and realized modpost is what is giving the problem.

When the array for vendor and model is changed to pointers, modpost (at the end of make modules) dies with a segmentation fault.

Peter, Any details that you can provide ?

In this case, the tail wags the dog -- it's a string instead of a pointer basicallly so that the new bits in file2alias.c can look like the other parts of that same file. It's sortof inconvenient for them to be pointers in there...

Hi James,

Are your concerns answered with Peter's response ?

Or, changes are required for the patches I posted ?

Thanks and regards,

chandra

Quoted 39 lines · Peter Jones

Quoted 27 lines · Chandra Seetharaman

Quoted 21 lines · Chandra Seetharaman

Quoted 10 lines · Kay Sievers

Quoted 9 lines · Chandra Seetharaman

From: Peter Jones <[email protected]>

This patch allows the use of modaliases on scsi targets to correctly load scsi device handler modules when the devices are found. +++ linux-2.6.28/include/linux/mod_devicetable.h

include/linux/mod_devicetable.h +0 -0

@@ -454,4 +454,10 @@ struct dmi_system_id {
 
 #define DMI_MATCH(a, b)        { a, b }
 

Quoted 10 lines

include/linux/mod_devicetable.h struct dmi_system_id { · line 461

Quoted 5 lines

include/linux/mod_devicetable.h +5 -0

+struct scsi_dh_device_id {
+       unsigned char type;
+       char vendor[9];
+       char model[17];
+};

Doesn't the static array waste space, when used for the long lists of entries stuffed in arrays of this structure? It will carry a lot of \0 chars, and identical strings can not be de-duplicated by the compiler, unlike when pointers are used?

Quoted 4 lines

I had some problems when we used the pointers instead of arrays. Don't recall it now.

Will retry and report.

Quoted 8 lines

Hi Kay,

I tried it and realized modpost is what is giving the problem.

When the array for vendor and model is changed to pointers, modpost (at the end of make modules) dies with a segmentation fault.

Peter, Any details that you can provide ?

In this case, the tail wags the dog -- it's a string instead of a pointer basicallly so that the new bits in file2alias.c can look like the other parts of that same file. It's sortof inconvenient for them to be pointers in there...

Hi James,

Do you still have any concerns (after Peter's response) ?

Hi James,

Do you still have any concerns (after Peter's response) ?

If not, Can you accept the patches, please.

Quoted 3 lines · Chandra Seetharaman

Hi James,

Do you still have any concerns (after Peter's response) ?

Yes, the basic concerns still remain:

1. You're forcing autoload now even if the user isn't running dm ... this is going to cause problems with non-dm based path handlers

2. autoloading in this fashion is essentially trying to work around a problem in the initrd tools. The kernel isn't the right place to implement the fix.

The risks of this approach seem very high, and the rewards pretty small.

James

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

Hi Chandra,

Quoted 6 lines · Chandra Seetharaman

Hi James,

Do you still have any concerns (after Peter's response) ?

If not, Can you accept the patches, please.

The problem here is that module autoloading doesn't work for device_handler. We have to have the callbacks in place _before_ driver probing starts as we might need to intercept the I/O requests and/or errors. When using modalias the module loading will be too late and the hooks will only be established after the probing has already happened.
(The module autoloading is an asynchronous call in device_add(), where we don't write for it to succeed. So probing will continue and we woulnd't have the callbacks in place when probing starts).

So adding the modalias will be of a purely informational value which I doubt is worth it.

Cheers,

Hannes

Quoted 4 lines · Hannes Reinecke

Quoted 2 lines · Chandra Seetharaman

If not, Can you accept the patches, please.

Quoted 10 lines

The problem here is that module autoloading doesn't work for device_handler. We have to have the callbacks in place _before_ driver probing starts as we might need to intercept the I/O requests and/or errors. When using modalias the module loading will be too late and the hooks will only be established after the probing has already happened.
(The module autoloading is an asynchronous call in device_add(), where we don't write for it to succeed. So probing will continue and we woulnd't have the callbacks in place when probing starts).

Your analysis is correct.

But, the chicken-n-egg problem you described exists only for the first device that is probed, module will be in place for the other devices.

The first device race is handled by acting on the bus notification for the BUS_NOTIFY_BOUND_DRIVER event (patch 3/3 in my patch list).

Quoted 3 lines · Chandra Seetharaman

Hi James,

Do you still have any concerns (after Peter's response) ?

Yes, the basic concerns still remain:

1. You're forcing autoload now even if the user isn't running dm ... this is going to cause problems with non-dm based path handlers

2. autoloading in this fashion is essentially trying to work around a problem in the initrd tools. The kernel isn't the right place to implement the fix.

The risks of this approach seem very high, and the rewards pretty small.

Quoted 21 lines · Chandra Seetharaman

Quoted 4 lines · Hannes Reinecke

Quoted 2 lines · Chandra Seetharaman

If not, Can you accept the patches, please.

Quoted 10 lines

The problem here is that module autoloading doesn't work for device_handler. We have to have the callbacks in place _before_ driver probing starts as we might need to intercept the I/O requests and/or errors. When using modalias the module loading will be too late and the hooks will only be established after the probing has already happened.
(The module autoloading is an asynchronous call in device_add(), where we don't write for it to succeed. So probing will continue and we woulnd't have the callbacks in place when probing starts).

Your analysis is correct.

But, the chicken-n-egg problem you described exists only for the first device that is probed, module will be in place for the other devices.

Quoted 2 lines

The first device race is handled by acting on the bus notification for the BUS_NOTIFY_BOUND_DRIVER event (patch 3/3 in my patch list).

How do you make sure, that the module is initialized when the BOUND_DRIVER event is happening? The userspace process loading the module can take any time to load, and I don't see how that is handled. Care to explain that in detail?

I guess, you either need to implement a "device scan" from the module loading routine to find all currently unhandled devices, or you need

to issue a blocking request_module() from inside the kernel and try to find a matching module, and then run the find again. The modalias

seems not like the right solution here.

Thanks,
Kay

Quoted 10 lines · James Bottomley

Quoted 3 lines · Chandra Seetharaman

Hi James,

Do you still have any concerns (after Peter's response) ?

Yes, the basic concerns still remain:

1. You're forcing autoload now even if the user isn't running dm ... this is going to cause problems with non-dm based path handlers

(Chandra covered this pretty well, so I'll leave it be.)

Quoted 3 lines

2. autoloading in this fashion is essentially trying to work around a problem in the initrd tools. The kernel isn't the right place to implement the fix.

This seems backwards to me. It's not trying to work around a problem in the initrd tools; it's trying to avoid creating one by making this subsystem unlike others.

The point of having modaliases is to allow the kernel to announce that it's got a hardware device and notify the userland that appropriate modules should be loaded. That's exactly what we've done here. What we're trying to avoid in the initrd tools is having to have a special handler for this subsystem; instead, we'd much rather use the generic mechanism that already exists for this purpose.

Quoted 1 line

The risks of this approach seem very high, and the rewards pretty small.

Can you please explain what the high risks you're thinking of are? I'm not clear on what undesirable behavior you expect to occur.

The reward is that scsi targets behave exactly like most other types of hardware and kernel modules in this regard, without having to write special probing for this subsystem in the userland and special handling for loading these modules. That's a pretty big win for maintainability.

Hi James,

Are your concerns answered ?

Hannes, Kay,

Is the description related to bus notify BIND DEVICE issue clear now ?

Please respond :)