[PATCH 0/3] scsi_dh: Make scsi device handler modules automatically inserted

[PATCH 0/3] scsi_dh: Make scsi device handler modules automatically inserted

[Chandra Seetharaman]

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

[PATCH 1/3] scsi_dh: Add modalias support for SCSI targets

[Chandra Seetharaman]

From: Peter Jones <pjones@redhat.com>

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 <pjones@redhat.com>
Signed-off-by: Chandra Seetharaman <sekharan@us.ibm.com>

---
 drivers/scsi/scsi_sysfs.c       |   55 +++++++++++++++++++++++++++++++++++++++-
 include/linux/mod_devicetable.h |    6 ++++
 include/linux/string_helpers.h  |    2 +
 include/scsi/scsi.h             |    1 +
 include/scsi/scsi_device.h      |    9 +-----
 lib/string_helpers.c            |   32 +++++++++++++++++++++++
 scripts/mod/file2alias.c        |   38 ++++++++++++++++++++++++++++
 7 files changed, 134 insertions(+), 9 deletions(-)

Index: linux-2.6.28/drivers/scsi/scsi_sysfs.c
===================================================================
--- linux-2.6.28.orig/drivers/scsi/scsi_sysfs.c
+++ linux-2.6.28/drivers/scsi/scsi_sysfs.c
@@ -10,6 +10,7 @@
 #include <linux/init.h>
 #include <linux/blkdev.h>
 #include <linux/device.h>
+#include <linux/string_helpers.h>
 
 #include <scsi/scsi.h>
 #include <scsi/scsi_device.h>
@@ -362,16 +363,63 @@ static int scsi_bus_match(struct device 
 	return (sdp->inq_periph_qual == SCSI_INQ_PQ_CON)? 1: 0;
 }
 
+static ssize_t format_scsi_modalias(struct scsi_device *sdev, char *buffer,
+				    ssize_t len)
+{
+	char vendor[9];
+	char *hex_vendor;
+	char model[17];
+	char *hex_model;
+	int i;
+
+	strncpy(vendor, sdev->vendor, 8);
+	vendor[8] = '\0';
+	for (i = strlen(vendor) - 1; i >= 0; i--) {
+		if (vendor[i] != ' ')
+			break;
+		vendor[i] = '\0';
+	}
+	hex_vendor = string_to_hex(vendor);
+	if (!hex_vendor)
+		return -ENOMEM;
+
+	strncpy(model, sdev->model, 16);
+	model[8] = '\0';
+	for (i = strlen(model) - 1; i >= 0; i--) {
+		if (model[i] != ' ')
+			break;
+		model[i] = '\0';
+	}
+	hex_model = string_to_hex(model);
+	if (!hex_model) {
+		kfree(hex_vendor);
+		return -ENOMEM;
+	}
+
+	i = snprintf(buffer, len, "scsi:t-0x%02xv%.16sm%.32s", sdev->type,
+		 hex_vendor, hex_model);
+	kfree(hex_vendor);
+	kfree(hex_model);
+	return i;
+}
+
 static int scsi_bus_uevent(struct device *dev, struct kobj_uevent_env *env)
 {
 	struct scsi_device *sdev;
+	char buffer[501];
+	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';
+	rc = format_scsi_modalias(sdev, buffer, 500);
+	if (rc < 0)
+		return rc;
+
+	add_uevent_var(env, "MODALIAS=%s", buffer);
 	return 0;
 }
 
@@ -697,8 +745,11 @@ static ssize_t
 sdev_show_modalias(struct device *dev, struct device_attribute *attr, char *buf)
 {
 	struct scsi_device *sdev;
+	ssize_t rc;
+
 	sdev = to_scsi_device(dev);
-	return snprintf (buf, 20, SCSI_DEVICE_MODALIAS_FMT "\n", sdev->type);
+	rc = format_scsi_modalias(sdev, buf, 500);
+	return rc;
 }
 static DEVICE_ATTR(modalias, S_IRUGO, sdev_show_modalias, NULL);
 
Index: linux-2.6.28/include/linux/mod_devicetable.h
===================================================================
--- linux-2.6.28.orig/include/linux/mod_devicetable.h
+++ linux-2.6.28/include/linux/mod_devicetable.h
@@ -454,4 +454,10 @@ struct dmi_system_id {
 
 #define DMI_MATCH(a, b)	{ a, b }
 
+struct scsi_dh_device_id {
+	unsigned char type;
+	char vendor[9];
+	char model[17];
+};
+
 #endif /* LINUX_MOD_DEVICETABLE_H */
Index: linux-2.6.28/include/linux/string_helpers.h
===================================================================
--- linux-2.6.28.orig/include/linux/string_helpers.h
+++ linux-2.6.28/include/linux/string_helpers.h
@@ -13,4 +13,6 @@ enum string_size_units {
 int string_get_size(u64 size, enum string_size_units units,
 		    char *buf, int len);
 
+unsigned char *string_to_hex(const unsigned char *s);
+
 #endif
Index: linux-2.6.28/include/scsi/scsi.h
===================================================================
--- linux-2.6.28.orig/include/scsi/scsi.h
+++ linux-2.6.28/include/scsi/scsi.h
@@ -264,6 +264,7 @@ static inline int scsi_status_is_good(in
 #define TYPE_ENCLOSURE      0x0d    /* Enclosure Services Device */
 #define TYPE_RBC	    0x0e
 #define TYPE_NO_LUN         0x7f
+#define TYPE_ANY	    0xff
 
 /* SCSI protocols; these are taken from SPC-3 section 7.5 */
 enum scsi_protocol {
Index: linux-2.6.28/include/scsi/scsi_device.h
===================================================================
--- linux-2.6.28.orig/include/scsi/scsi_device.h
+++ linux-2.6.28/include/scsi/scsi_device.h
@@ -1,6 +1,7 @@
 #ifndef _SCSI_SCSI_DEVICE_H
 #define _SCSI_SCSI_DEVICE_H
 
+#include <linux/mod_devicetable.h>
 #include <linux/device.h>
 #include <linux/list.h>
 #include <linux/spinlock.h>
@@ -169,11 +170,6 @@ struct scsi_device {
 	unsigned long		sdev_data[0];
 } __attribute__((aligned(sizeof(unsigned long))));
 
-struct scsi_dh_devlist {
-	char *vendor;
-	char *model;
-};
-
 struct scsi_device_handler {
 	/* Used by the infrastructure */
 	struct list_head list; /* list of scsi_device_handlers */
@@ -181,7 +177,7 @@ struct scsi_device_handler {
 	/* Filled by the hardware handler */
 	struct module *module;
 	const char *name;
-	const struct scsi_dh_devlist *devlist;
+	const struct scsi_dh_device_id *devlist;
 	int (*check_sense)(struct scsi_device *, struct scsi_sense_hdr *);
 	int (*attach)(struct scsi_device *);
 	void (*detach)(struct scsi_device *);
@@ -456,6 +452,5 @@ static inline int scsi_device_protection
 
 #define MODULE_ALIAS_SCSI_DEVICE(type) \
 	MODULE_ALIAS("scsi:t-" __stringify(type) "*")
-#define SCSI_DEVICE_MODALIAS_FMT "scsi:t-0x%02x"
 
 #endif /* _SCSI_SCSI_DEVICE_H */
Index: linux-2.6.28/lib/string_helpers.c
===================================================================
--- linux-2.6.28.orig/lib/string_helpers.c
+++ linux-2.6.28/lib/string_helpers.c
@@ -66,3 +66,35 @@ int string_get_size(u64 size, const enum
 	return 0;
 }
 EXPORT_SYMBOL(string_get_size);
+
+/**
+ * string_to_hex - convert a string to a series of hexidecimal values
+ * @s:	The string to operate on
+ *
+ * This function returns a GFP_KERNEL allocated buffer filled with
+ * the hexidecimal representation of the value of each character in @s .
+ * Returns a pointer to the allocated string on success and NULL on error,
+ * and the returned string is zero terminated.
+ *
+ */
+unsigned char *string_to_hex(const unsigned char *s)
+{
+	unsigned char *ret, *ptr;
+	static const unsigned char *hex = "0123456789ABCDEF";
+	int len;
+
+	len = strlen(s);
+
+	ret = ptr = kmalloc(len * 2 + 1, GFP_KERNEL);
+	if (!ret)
+		return NULL;
+
+	for (; *s; s++) {
+		*ptr++ = hex[(*s & 0xf0)>>4];
+		*ptr++ = hex[*s & 0x0f];
+	}
+	*ptr = '\0';
+
+	return ret;
+}
+EXPORT_SYMBOL(string_to_hex);
Index: linux-2.6.28/scripts/mod/file2alias.c
===================================================================
--- linux-2.6.28.orig/scripts/mod/file2alias.c
+++ linux-2.6.28/scripts/mod/file2alias.c
@@ -51,6 +51,22 @@ do {                                    
                 sprintf(str + strlen(str), "*");                \
 } while(0)
 
+#define ADD_HEX_STR(str, sep, cond, field)				\
+do {									\
+	strcat(str, sep);						\
+	if (cond) {							\
+		char * _s = str + strlen(str);				\
+		char * _f = field;					\
+		static const char *_hex = "0123456789ABCDEF";		\
+									\
+		for (; *_f; _f++) {					\
+			*_s++ = _hex[(*_f & 0xf0)>>4];			\
+			*_s++ = _hex[*_f & 0xf];			\
+		}							\
+	} else								\
+		strcat(str, "*");					\
+} while(0)
+
 /* Always end in a wildcard, for future extension */
 static inline void add_wildcard(char *str)
 {
@@ -710,6 +726,23 @@ static int do_dmi_entry(const char *file
 	strcat(alias, ":");
 	return 1;
 }
+
+/* Looks like: scsi:t-NvSmS */
+/* defining TYPE_ANY here is a gross hack to avoid moving all the scsi.h
+ * TYPE_ definitions into mod_devicetable.h */
+#define TYPE_ANY 0xff
+static int do_scsi_entry(const char *filename,
+			struct scsi_dh_device_id *id, char *alias)
+{
+	strcpy(alias, "scsi:");
+	ADD(alias, "t-", id->type != TYPE_ANY, id->type);
+	ADD_HEX_STR(alias, "v", id->vendor[0] != '\0', id->vendor);
+	ADD_HEX_STR(alias, "m", id->model[0] != '\0', id->model);
+
+	add_wildcard(alias);
+	return 1;
+}
+
 /* Ignore any prefix, eg. some architectures prepend _ */
 static inline int sym_is(const char *symbol, const char *name)
 {
@@ -736,6 +769,7 @@ static void do_table(void *symval, unsig
 	size -= id_size;
 
 	for (i = 0; i < size; i += id_size) {
+		memset(alias, '\0', 500);
 		if (do_entry(mod->name, symval+i, alias)) {
 			buf_printf(&mod->dev_table_buf,
 				   "MODULE_ALIAS(\"%s\");\n", alias);
@@ -849,6 +883,10 @@ void handle_moddevtable(struct module *m
 		do_table(symval, sym->st_size,
 			 sizeof(struct dmi_system_id), "dmi",
 			 do_dmi_entry, mod);
+	else if (sym_is(symname, "__mod_scsi_dh_device_table"))
+		do_table(symval, sym->st_size,
+			 sizeof(struct scsi_dh_device_id), "scsi",
+			 do_scsi_entry, mod);
 	free(zeros);
 }
 

[PATCH 2/3] scsi_dh: Change scsi device handler modules to utilize modalias

[Chandra Seetharaman]

From: Peter Jones <pjones@redhat.com>

This patch changes the scsi_dh files to make use of the modalias feature.

Signed-off-by: Peter Jones <pjones@redhat.com>
Signed-off-by: Chandra Seetharaman <sekharan@us.ibm.com>
---

 drivers/scsi/device_handler/scsi_dh.c       |    5 +++-
 drivers/scsi/device_handler/scsi_dh_alua.c  |   23 ++++++++-------
 drivers/scsi/device_handler/scsi_dh_emc.c   |   11 ++++---
 drivers/scsi/device_handler/scsi_dh_hp_sw.c |   13 +++++----
 drivers/scsi/device_handler/scsi_dh_rdac.c  |   42 +++++++++++++++-------------
 5 files changed, 51 insertions(+), 43 deletions(-)

Index: linux-2.6.28/drivers/scsi/device_handler/scsi_dh.c
===================================================================
--- linux-2.6.28.orig/drivers/scsi/device_handler/scsi_dh.c
+++ linux-2.6.28/drivers/scsi/device_handler/scsi_dh.c
@@ -75,7 +75,10 @@ static int scsi_dh_handler_lookup(struct
 {
 	int i, found = 0;
 
-	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,
Index: linux-2.6.28/drivers/scsi/device_handler/scsi_dh_alua.c
===================================================================
--- linux-2.6.28.orig/drivers/scsi/device_handler/scsi_dh_alua.c
+++ linux-2.6.28/drivers/scsi/device_handler/scsi_dh_alua.c
@@ -681,18 +681,19 @@ static int alua_prep_fn(struct scsi_devi
 
 }
 
-static const struct scsi_dh_devlist alua_dev_list[] = {
-	{"HP", "MSA VOLUME" },
-	{"HP", "HSV101" },
-	{"HP", "HSV111" },
-	{"HP", "HSV200" },
-	{"HP", "HSV210" },
-	{"HP", "HSV300" },
-	{"IBM", "2107900" },
-	{"IBM", "2145" },
-	{"Pillar", "Axiom" },
-	{NULL, NULL}
+static const struct scsi_dh_device_id alua_dev_list[] = {
+	{TYPE_ANY, "HP", "MSA VOLUME" },
+	{TYPE_ANY, "HP", "HSV101" },
+	{TYPE_ANY, "HP", "HSV111" },
+	{TYPE_ANY, "HP", "HSV200" },
+	{TYPE_ANY, "HP", "HSV210" },
+	{TYPE_ANY, "HP", "HSV300" },
+	{TYPE_ANY, "IBM", "2107900" },
+	{TYPE_ANY, "IBM", "2145" },
+	{TYPE_ANY, "Pillar", "Axiom" },
+	{0, "", ""},
 };
+MODULE_DEVICE_TABLE(scsi_dh, alua_dev_list);
 
 static int alua_bus_attach(struct scsi_device *sdev);
 static void alua_bus_detach(struct scsi_device *sdev);
Index: linux-2.6.28/drivers/scsi/device_handler/scsi_dh_emc.c
===================================================================
--- linux-2.6.28.orig/drivers/scsi/device_handler/scsi_dh_emc.c
+++ linux-2.6.28/drivers/scsi/device_handler/scsi_dh_emc.c
@@ -562,12 +562,13 @@ done:
 	return result;
 }
 
-static const struct scsi_dh_devlist clariion_dev_list[] = {
-	{"DGC", "RAID"},
-	{"DGC", "DISK"},
-	{"DGC", "VRAID"},
-	{NULL, NULL},
+static const struct scsi_dh_device_id clariion_dev_list[] = {
+	{TYPE_ANY, "DGC", "RAID"},
+	{TYPE_ANY, "DGC", "DISK"},
+	{TYPE_ANY, "DGC", "VRAID"},
+	{0, "", ""},
 };
+MODULE_DEVICE_TABLE(scsi_dh, clariion_dev_list);
 
 static int clariion_bus_attach(struct scsi_device *sdev);
 static void clariion_bus_detach(struct scsi_device *sdev);
Index: linux-2.6.28/drivers/scsi/device_handler/scsi_dh_hp_sw.c
===================================================================
--- linux-2.6.28.orig/drivers/scsi/device_handler/scsi_dh_hp_sw.c
+++ linux-2.6.28/drivers/scsi/device_handler/scsi_dh_hp_sw.c
@@ -286,13 +286,14 @@ static int hp_sw_activate(struct scsi_de
 	return ret;
 }
 
-static const struct scsi_dh_devlist hp_sw_dh_data_list[] = {
-	{"COMPAQ", "MSA1000 VOLUME"},
-	{"COMPAQ", "HSV110"},
-	{"HP", "HSV100"},
-	{"DEC", "HSG80"},
-	{NULL, NULL},
+static const struct scsi_dh_device_id hp_sw_dh_data_list[] = {
+	{TYPE_ANY, "COMPAQ", "MSA1000 VOLUME"},
+	{TYPE_ANY, "COMPAQ", "HSV110"},
+	{TYPE_ANY, "HP", "HSV100"},
+	{TYPE_ANY, "DEC", "HSG80"},
+	{0, "", ""},
 };
+MODULE_DEVICE_TABLE(scsi_dh, hp_sw_dh_data_list);
 
 static int hp_sw_bus_attach(struct scsi_device *sdev);
 static void hp_sw_bus_detach(struct scsi_device *sdev);
Index: linux-2.6.28/drivers/scsi/device_handler/scsi_dh_rdac.c
===================================================================
--- linux-2.6.28.orig/drivers/scsi/device_handler/scsi_dh_rdac.c
+++ linux-2.6.28/drivers/scsi/device_handler/scsi_dh_rdac.c
@@ -585,28 +585,30 @@ static int rdac_check_sense(struct scsi_
 	return SCSI_RETURN_NOT_HANDLED;
 }
 
-static const struct scsi_dh_devlist rdac_dev_list[] = {
-	{"IBM", "1722"},
-	{"IBM", "1724"},
-	{"IBM", "1726"},
-	{"IBM", "1742"},
-	{"IBM", "1814"},
-	{"IBM", "1815"},
-	{"IBM", "1818"},
-	{"IBM", "3526"},
-	{"SGI", "TP9400"},
-	{"SGI", "TP9500"},
-	{"SGI", "IS"},
-	{"STK", "OPENstorage D280"},
-	{"SUN", "CSM200_R"},
-	{"SUN", "LCSM100_F"},
-	{"DELL", "MD3000"},
-	{"DELL", "MD3000i"},
-	{"LSI", "INF-01-00"},
-	{"ENGENIO", "INF-01-00"},
-	{NULL, NULL},
+static const struct scsi_dh_device_id rdac_dev_list[] = {
+	{TYPE_ANY, "IBM", "1722"},
+	{TYPE_ANY, "IBM", "1724"},
+	{TYPE_ANY, "IBM", "1726"},
+	{TYPE_ANY, "IBM", "1742"},
+	{TYPE_ANY, "IBM", "1814"},
+	{TYPE_ANY, "IBM", "1815"},
+	{TYPE_ANY, "IBM", "1818"},
+	{TYPE_ANY, "IBM", "3526"},
+	{TYPE_ANY, "SGI", "TP9400"},
+	{TYPE_ANY, "SGI", "TP9500"},
+	{TYPE_ANY, "SGI", "IS"},
+	{TYPE_ANY, "STK", "OPENstorage D280"},
+	{TYPE_ANY, "SUN", "CSM200_R"},
+	{TYPE_ANY, "SUN", "LCSM100_F"},
+	{TYPE_ANY, "DELL", "MD3000"},
+	{TYPE_ANY, "DELL", "MD3000i"},
+	{TYPE_ANY, "LSI", "INF-01-00"},
+	{TYPE_ANY, "ENGENIO", "INF-01-00"},
+	{0, "", ""},
 };
 
+MODULE_DEVICE_TABLE(scsi_dh, rdac_dev_list);
+
 static int rdac_bus_attach(struct scsi_device *sdev);
 static void rdac_bus_detach(struct scsi_device *sdev);
 

[PATCH 3/3] scsi_dh: Workaround a race condition in module insertion

[Chandra Seetharaman]

From: Chandra Seetharaman <sekharan@us.ibm.com>

scsi_dh odule is getting inserted as soon as the first device is seen.
But, the first device is not seen by the module as we were past the 
ADD_DEVICE handling in the module.

Catch the first device by handling BUS_NOTIFY_BOUND_DRIVER event,
and not handle that event for any of the future devices (as they would
have been handled by ADD_DEVICE event).

Signed-off-by: Chandra Seetharaman <sekharan@us.ibm.com>
Signed-off-by: Peter Jones <pjones@redhat.com>

---
 drivers/scsi/device_handler/scsi_dh.c |   11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

Index: linux-2.6.28/drivers/scsi/device_handler/scsi_dh.c
===================================================================
--- linux-2.6.28.orig/drivers/scsi/device_handler/scsi_dh.c
+++ linux-2.6.28/drivers/scsi/device_handler/scsi_dh.c
@@ -294,7 +294,16 @@ static int scsi_dh_notifier(struct notif
 
 	sdev = to_scsi_device(dev);
 
-	if (action == BUS_NOTIFY_ADD_DEVICE) {
+	if ((action == BUS_NOTIFY_ADD_DEVICE) ||
+			(action == BUS_NOTIFY_BOUND_DRIVER)) {
+		/*
+		 * This can happen if device was configured already
+		 * with BUS_NOTIFY_ADD_DEVICE and we are called
+		 * now with BUS_NOTIFY_BOUND_DRIVER
+		 */
+		if (sdev->scsi_dh_data)
+			goto out;
+
 		devinfo = device_handler_match(NULL, sdev);
 		if (!devinfo)
 			goto out;

Re: [PATCH 0/3] scsi_dh: Make scsi device handler modules automatically inserted

[Hannes Reinecke]

Hi Chandra,

Chandra Seetharaman wrote:
> 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
> 
Well, agreed in principle. But do note that ALUA handling really
isn't supported well here.

Problem is that ALUA hooks off a different setting (namely the
'TGPS' bit in the inquiry data) and _not_ on the device name.
In fact, it's well possible to support _both_, old-style
proprietary failover and ALUA.

So to handle this properly we should be adding a 'tgps'
setting to the sysfs data and use this for ALUA keying.
With that we basically can get rid of the device table
in ALUA and live happily ever after.

I'll be posting a patch for this shortly.
We really should be integrating both of them, as
the proposed modalias support is a Good Thing (tm).

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Markus Rex, HRB 16746 (AG Nürnberg)
--
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: [PATCH 1/3] scsi_dh: Add modalias support for SCSI targets

[Konrad Rzeszutek]

.. snip.. 
>  static int scsi_bus_uevent(struct device *dev, struct kobj_uevent_env *env)
>  {
>  	struct scsi_device *sdev;
> +	char buffer[501];

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

> +	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] ?

> +	rc = format_scsi_modalias(sdev, buffer, 500);
> +	if (rc < 0)
> +		return rc;
> +
> +	add_uevent_var(env, "MODALIAS=%s", buffer);
>  	return 0;
>  }
>  
> @@ -697,8 +745,11 @@ static ssize_t
>  sdev_show_modalias(struct device *dev, struct device_attribute *attr, char *buf)
>  {
>  	struct scsi_device *sdev;
> +	ssize_t rc;
> +
>  	sdev = to_scsi_device(dev);
> -	return snprintf (buf, 20, SCSI_DEVICE_MODALIAS_FMT "\n", sdev->type);
> +	rc = format_scsi_modalias(sdev, buf, 500);

Maybe a #define for that magic number?

> +	return rc;
>  }
.. snip ..
>  /* Ignore any prefix, eg. some architectures prepend _ */
>  static inline int sym_is(const char *symbol, const char *name)
>  {
> @@ -736,6 +769,7 @@ static void do_table(void *symval, unsig
>  	size -= id_size;
>  
>  	for (i = 0; i < size; i += id_size) {
> +		memset(alias, '\0', 500);

How about:
memset (alias, 0x0, A_BIG_NUMBER);

Re: [PATCH 2/3] scsi_dh: Change scsi device handler modules to utilize modalias

[Konrad Rzeszutek]

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

Is there a define for '0' value?

Re: [PATCH 1/3] scsi_dh: Add modalias support for SCSI targets

[James Bottomley]

On Wed, 2009-03-18 at 09:44 -0400, Konrad Rzeszutek wrote:
> .. snip.. 
> >  static int scsi_bus_uevent(struct device *dev, struct kobj_uevent_env *env)
> >  {
> >  	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?

> > +	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] ?

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

Re: [PATCH 1/3] scsi_dh: Add modalias support for SCSI targets

[Konrad Rzeszutek]

On Wed, Mar 18, 2009 at 02:02:43PM +0000, James Bottomley wrote:
> On Wed, 2009-03-18 at 09:44 -0400, Konrad Rzeszutek wrote:
> > .. snip.. 
> > >  static int scsi_bus_uevent(struct device *dev, struct kobj_uevent_env *env)
> > >  {
> > >  	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?

No no. I was thinking why the length of 500 bytes. why not 256 for example.
It doesn't matter that much really, but I am curious to why.

> 
> > > +	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] ?
> 
> 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].

Ugh. You are right. I need some more coffee.
> 
> James
> 

Re: [PATCH 2/3] scsi_dh: Change scsi device handler modules to utilize modalias

[Stefan Richter]

Konrad Rzeszutek wrote:
> .. snip..
>> +	{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,

>> --- linux-2.6.28.orig/drivers/scsi/device_handler/scsi_dh.c
>> +++ linux-2.6.28/drivers/scsi/device_handler/scsi_dh.c
>> @@ -75,7 +75,10 @@ static int scsi_dh_handler_lookup(struct
>>  {
>>  	int i, found = 0;
>>  
>> -	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.  :-)
-- 
Stefan Richter
-=====-==--= --== =--=-
http://arcgraph.de/sr/

Re: [PATCH 2/3] scsi_dh: Change scsi device handler modules to utilize modalias

[Chandra Seetharaman]

On Wed, 2009-03-18 at 16:43 +0100, Stefan Richter wrote:
> Konrad Rzeszutek wrote:
> > .. snip..
> >> +	{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,
> 
> >> --- linux-2.6.28.orig/drivers/scsi/device_handler/scsi_dh.c
> >> +++ linux-2.6.28/drivers/scsi/device_handler/scsi_dh.c
> >> @@ -75,7 +75,10 @@ static int scsi_dh_handler_lookup(struct
> >>  {
> >>  	int i, found = 0;
> >>  
> >> -	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.  :-)

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

Did I code your suggestion correctly ?

Here is the patch
--------------
Index: linux-2.6.29-rc8/drivers/scsi/device_handler/scsi_dh.c
===================================================================
--- linux-2.6.29-rc8.orig/drivers/scsi/device_handler/scsi_dh.c
+++ linux-2.6.29-rc8/drivers/scsi/device_handler/scsi_dh.c
@@ -75,7 +75,7 @@ static int scsi_dh_handler_lookup(struct
 {
        int i, found = 0;
 
-       for(i = 0; scsi_dh->devlist[i].vendor[0]; i++) {
+       for(i = 0; scsi_dh->devlist[i].vendor; i++) {
                if ((scsi_dh->devlist[i].type != TYPE_ANY) &&
                                (scsi_dh->devlist[i].type !=
sdev->type))
                        continue;
Index: linux-2.6.29-rc8/drivers/scsi/device_handler/scsi_dh_rdac.c
===================================================================
--- linux-2.6.29-rc8.orig/drivers/scsi/device_handler/scsi_dh_rdac.c
+++ linux-2.6.29-rc8/drivers/scsi/device_handler/scsi_dh_rdac.c
@@ -604,7 +604,7 @@ static const struct scsi_dh_device_id rd
        {TYPE_ANY, "DELL", "MD3000i"},
        {TYPE_ANY, "LSI", "INF-01-00"},
        {TYPE_ANY, "ENGENIO", "INF-01-00"},
-       {0, "", ""},
+       {},
 };
 
 MODULE_DEVICE_TABLE(scsi_dh, rdac_dev_list);
--------------
Here is the panic
--------------

Faulting instruction address: 0xd0000000017e01c4
cpu 0x0: Vector: 300 (Data Access) at [c0000000e2fbb680]
    pc: d0000000017e01c4: .scsi_dh_handler_lookup+0x24/0xc0 [scsi_dh]
    lr: d0000000017e0200: .scsi_dh_handler_lookup+0x60/0xc0 [scsi_dh]
    sp: c0000000e2fbb900
   msr: 8000000000009032
   dar: d000000002f80017
 dsisr: 40000000
  current = 0xc0000000e7a84200
  paca    = 0xc000000000723400
    pid   = 3516, comm = modprobe
enter ? for help
[c0000000e2fbb900] c0000000e2fbb9e0 (unreliable)
[c0000000e2fbb9a0] d0000000017e0604 .device_handler_match+0x110/0x278
[scsi_dh]
[c0000000e2fbba50] d0000000017e0bd0 .scsi_dh_notifier_add+0x54/0x94
[scsi_dh]
[c0000000e2fbbae0] c0000000002d9d40 .bus_for_each_dev+0x80/0xd8
[c0000000e2fbbb90] d0000000017e0dc0 .scsi_register_device_handler
+0x90/0xcc [scsi_dh]
[c0000000e2fbbc20] d000000002f70ce8 .rdac_init+0x20/0x4d8 [scsi_dh_rdac]
[c0000000e2fbbca0] c0000000000090b0 .do_one_initcall+0x90/0x1a8
[c0000000e2fbbd90] c0000000000a0b28 .SyS_init_module+0xc4/0x214
[c0000000e2fbbe30] c000000000008534 syscall_exit+0x0/0x40
--- Exception: c00 (System Call) at 000000000ff11a6c
SP (ffb6f7e0) is in userspace
0:mon> 

Re: [PATCH 2/3] scsi_dh: Change scsi device handler modules to utilize modalias

[Stefan Richter]

Chandra Seetharaman wrote:
> On Wed, 2009-03-18 at 16:43 +0100, Stefan Richter wrote:
>> 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 :(.
> 
> 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?)
-- 
Stefan Richter
-=====-=-=== -=-= -==-=
http://arcgraph.de/sr/

Re: [PATCH 2/3] scsi_dh: Change scsi device handler modules to utilize modalias

[Kay Sievers]

On Wed, Mar 18, 2009 at 18:50, Stefan Richter <stefanr@s5r6.in-berlin.de> wrote:
> Chandra Seetharaman wrote:
>> On Wed, 2009-03-18 at 16:43 +0100, Stefan Richter wrote:
>>> 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 :(.

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

Kay
--
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: [PATCH 1/3] scsi_dh: Add modalias support for SCSI targets

[Kay Sievers]

On Wed, Mar 18, 2009 at 02:36, Chandra Seetharaman <sekharan@us.ibm.com> wrote:
> From: Peter Jones <pjones@redhat.com>
>
> 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
> @@ -454,4 +454,10 @@ struct dmi_system_id {
>
>  #define DMI_MATCH(a, b)        { a, b }
>
> +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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/3] scsi_dh: Add modalias support for SCSI targets

[James Bottomley]

On Tue, 2009-03-17 at 18:36 -0700, Chandra Seetharaman wrote:
> From: Peter Jones <pjones@redhat.com>
> 
> 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 <pjones@redhat.com>
> Signed-off-by: Chandra Seetharaman <sekharan@us.ibm.com>

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

Re: [PATCH 2/3] scsi_dh: Change scsi device handler modules to utilize modalias

[Chandra Seetharaman]

yes (all others are off).

 that is the only storage I have in my system 
On Wed, 2009-03-18 at 18:50 +0100, Stefan Richter wrote:
> Chandra Seetharaman wrote:
> > On Wed, 2009-03-18 at 16:43 +0100, Stefan Richter wrote:
> >> 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 :(.
> > 
> > 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?)

Re: [PATCH 1/3] scsi_dh: Add modalias support for SCSI targets

[Chandra Seetharaman]

On Wed, 2009-03-18 at 18:47 +0000, James Bottomley wrote:
> On Tue, 2009-03-17 at 18:36 -0700, Chandra Seetharaman wrote:
> > From: Peter Jones <pjones@redhat.com>
> > 
> > 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 <pjones@redhat.com>
> > Signed-off-by: Chandra Seetharaman <sekharan@us.ibm.com>
> 
> 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
scsi_dh handlers ?

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

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

Re: [PATCH 1/3] scsi_dh: Add modalias support for SCSI targets

[Chandra Seetharaman]

On Wed, 2009-03-18 at 19:30 +0100, Kay Sievers wrote:
> On Wed, Mar 18, 2009 at 02:36, Chandra Seetharaman <sekharan@us.ibm.com> wrote:
> > From: Peter Jones <pjones@redhat.com>
> >
> > 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
> > @@ -454,4 +454,10 @@ struct dmi_system_id {
> >
> >  #define DMI_MATCH(a, b)        { a, b }
> >
> > +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.
> Thanks,
> Kay
> --
> 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: [PATCH 2/3] scsi_dh: Change scsi device handler modules to utilize modalias

[Stefan Richter]

Kay Sievers wrote:
> On Wed, Mar 18, 2009 at 18:50, Stefan Richter <stefanr@s5r6.in-berlin.de> wrote:
>> Chandra Seetharaman wrote:
>>> On Wed, 2009-03-18 at 16:43 +0100, Stefan Richter wrote:
>>>> 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 :(.
> 
> It's a static char array not a pointer to a string.

Right, I missed that part of the switch from struct scsi_dh_devlist to
struct scsi_dh_device_id.  #-|
-- 
Stefan Richter
-=====-=-=== -=-= -==-=
http://arcgraph.de/sr/

Re: [PATCH 2/3] scsi_dh: Change scsi device handler modules to utilize modalias

[Stefan Richter]

Chandra Seetharaman wrote:
> yes (all others are off).
> 
>  that is the only storage I have in my system

My mistake.

>>> On Wed, 2009-03-18 at 16:43 +0100, Stefan Richter wrote:
>>>> AFAICS you could have kept the check for .vendor != NULL instead of
>>>> .vendor[0] != '\0'

Wrong.

>>>> and write the terminator thusly:
>>>>
>>>> 	{},

Right.

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

Wrong.
-- 
Stefan Richter
-=====-=-=== -=-= -==-=
http://arcgraph.de/sr/

Re: [PATCH 1/3] scsi_dh: Add modalias support for SCSI targets

[James Bottomley]

On Wed, 2009-03-18 at 12:12 -0700, Chandra Seetharaman wrote:
> On Wed, 2009-03-18 at 18:47 +0000, James Bottomley wrote:
> > On Tue, 2009-03-17 at 18:36 -0700, Chandra Seetharaman wrote:
> > > From: Peter Jones <pjones@redhat.com>
> > > 
> > > 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 <pjones@redhat.com>
> > > Signed-off-by: Chandra Seetharaman <sekharan@us.ibm.com>
> > 
> > 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
> 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.

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

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

> 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

Re: [PATCH 1/3] scsi_dh: Add modalias support for SCSI targets

[Kay Sievers]

On Wed, Mar 18, 2009 at 21:09, James Bottomley
<James.Bottomley@hansenpartnership.com> wrote:
> On Wed, 2009-03-18 at 12:12 -0700, Chandra Seetharaman wrote:
>> On Wed, 2009-03-18 at 18:47 +0000, James Bottomley wrote:
>> > On Tue, 2009-03-17 at 18:36 -0700, Chandra Seetharaman wrote:
>> > > From: Peter Jones <pjones@redhat.com>
>> > >
>> > > 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 <pjones@redhat.com>
>> > > Signed-off-by: Chandra Seetharaman <sekharan@us.ibm.com>
>> >
>> > 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
>> 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.
>
>> >      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.
>
> 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.

Thanks,
Kay
--
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: [PATCH 1/3] scsi_dh: Add modalias support for SCSI targets

[James Bottomley]

On Wed, 2009-03-18 at 21:24 +0100, Kay Sievers wrote:
> On Wed, Mar 18, 2009 at 21:09, James Bottomley
> <James.Bottomley@hansenpartnership.com> wrote:
> > On Wed, 2009-03-18 at 12:12 -0700, Chandra Seetharaman wrote:
> >> On Wed, 2009-03-18 at 18:47 +0000, James Bottomley wrote:
> >> > On Tue, 2009-03-17 at 18:36 -0700, Chandra Seetharaman wrote:
> >> > > From: Peter Jones <pjones@redhat.com>
> >> > >
> >> > > 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 <pjones@redhat.com>
> >> > > Signed-off-by: Chandra Seetharaman <sekharan@us.ibm.com>
> >> >
> >> > 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
> >> 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.
> >
> >> >      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.
> >
> > 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.

Yes .. I'd forgotten that; I was thinking of the environment as it
passes through udev.

James

Re: [PATCH 1/3] scsi_dh: Add modalias support for SCSI targets

[Chandra Seetharaman]

On Wed, 2009-03-18 at 20:09 +0000, James Bottomley wrote:
> On Wed, 2009-03-18 at 12:12 -0700, Chandra Seetharaman wrote:
> > On Wed, 2009-03-18 at 18:47 +0000, James Bottomley wrote:
> > > On Tue, 2009-03-17 at 18:36 -0700, Chandra Seetharaman wrote:
> > > > From: Peter Jones <pjones@redhat.com>
> > > > 
> > > > 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 <pjones@redhat.com>
> > > > Signed-off-by: Chandra Seetharaman <sekharan@us.ibm.com>
> > > 
> > > 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
> > 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.
> 
> > >      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.
> 
> 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).
> 

The idea is that installer would see the module thru modalias and would
add it to initrd. So, initrd would have the module only in systems where
the specific storage is found (and is done automatically).

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

to keep boot quiet and fast (with about 100 luns, boot time delay is
about 50 mins, and it increases proportionately)

> 
> James
> 
> 

Re: [PATCH 1/3] scsi_dh: Add modalias support for SCSI targets

[Chandra Seetharaman]

On Wed, 2009-03-18 at 12:18 -0700, Chandra Seetharaman wrote:
> On Wed, 2009-03-18 at 19:30 +0100, Kay Sievers wrote:
> > On Wed, Mar 18, 2009 at 02:36, Chandra Seetharaman <sekharan@us.ibm.com> wrote:
> > > From: Peter Jones <pjones@redhat.com>
> > >
> > > 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
> > > @@ -454,4 +454,10 @@ struct dmi_system_id {
> > >
> > >  #define DMI_MATCH(a, b)        { a, b }
> > >
> > > +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.

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 ?

> > Thanks,
> > Kay
> > --
> > 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: [PATCH 1/3] scsi_dh: Add modalias support for SCSI targets

[Peter Jones]

On 03/18/2009 04:09 PM, James Bottomley wrote:
> On Wed, 2009-03-18 at 12:12 -0700, Chandra Seetharaman wrote:
>> On Wed, 2009-03-18 at 18:47 +0000, James Bottomley wrote:
>>> On Tue, 2009-03-17 at 18:36 -0700, Chandra Seetharaman wrote:
>>>> From: Peter Jones <pjones@redhat.com>
>>>>
>>>> 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 <pjones@redhat.com>
>>>> Signed-off-by: Chandra Seetharaman <sekharan@us.ibm.com>
>>> 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

Yeah, I wasn't really enthusiastic about this hack when I wrote it,
either.  More on that below.

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

To be honest, I only used that particular modalias because it seemed
like the most natural place for it; so TYPE_ANY is really only there
because it already was.  Though I could imagine somebody writing a device
handler for e.g. a tape robot.  Or we could make the structure have
another field that says how to treat the type field, but that seems
sloppy.

If there's a /different/ object on which you think the modalias for
the scsi target itself should go, that'd be fine by me, I just didn't
see anywhere that looked better.

[...]
>> 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?

Well, really it's so that we don't have to add /extra/ code in userland
to decide which device handlers to load and to load them.  Especially
since that's what modaliases are *for*.

--
  Peter

Re: [PATCH 1/3] scsi_dh: Add modalias support for SCSI targets

[Peter Jones]

On 03/19/2009 02:54 PM, Chandra Seetharaman wrote:
> On Wed, 2009-03-18 at 12:18 -0700, Chandra Seetharaman wrote:
>> On Wed, 2009-03-18 at 19:30 +0100, Kay Sievers wrote:
>>> On Wed, Mar 18, 2009 at 02:36, Chandra Seetharaman <sekharan@us.ibm.com> wrote:
>>>> From: Peter Jones <pjones@redhat.com>
>>>>
>>>> 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
>>>> @@ -454,4 +454,10 @@ struct dmi_system_id {
>>>>
>>>>  #define DMI_MATCH(a, b)        { a, b }
>>>>
>>>> +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.
> 
> 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...

-- 
        Peter

If the facts don't fit the theory, change the facts.
		-- Einstein

Re: [PATCH 1/3] scsi_dh: Add modalias support for SCSI targets

[Chandra Seetharaman]

Hi James,

Are your concerns answered with Peter's response ?

Or, changes are required for the patches I posted ?

Thanks and regards,

chandra
On Fri, 2009-03-20 at 14:24 -0400, Peter Jones wrote:
> On 03/19/2009 02:54 PM, Chandra Seetharaman wrote:
> > On Wed, 2009-03-18 at 12:18 -0700, Chandra Seetharaman wrote:
> >> On Wed, 2009-03-18 at 19:30 +0100, Kay Sievers wrote:
> >>> On Wed, Mar 18, 2009 at 02:36, Chandra Seetharaman <sekharan@us.ibm.com> wrote:
> >>>> From: Peter Jones <pjones@redhat.com>
> >>>>
> >>>> 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
> >>>> @@ -454,4 +454,10 @@ struct dmi_system_id {
> >>>>
> >>>>  #define DMI_MATCH(a, b)        { a, b }
> >>>>
> >>>> +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.
> > 
> > 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...
> 

Re: [PATCH 1/3] scsi_dh: Add modalias support for SCSI targets

[Chandra Seetharaman]

Hi James,

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

If not, Can you accept the patches, please.

Thanks & Regards,

chandra
On Mon, 2009-03-23 at 15:13 -0700, Chandra Seetharaman wrote:
> Hi James,
> 
> Are your concerns answered with Peter's response ?
> 
> Or, changes are required for the patches I posted ?
> 
> Thanks and regards,
> 
> chandra
> On Fri, 2009-03-20 at 14:24 -0400, Peter Jones wrote:
> > On 03/19/2009 02:54 PM, Chandra Seetharaman wrote:
> > > On Wed, 2009-03-18 at 12:18 -0700, Chandra Seetharaman wrote:
> > >> On Wed, 2009-03-18 at 19:30 +0100, Kay Sievers wrote:
> > >>> On Wed, Mar 18, 2009 at 02:36, Chandra Seetharaman <sekharan@us.ibm.com> wrote:
> > >>>> From: Peter Jones <pjones@redhat.com>
> > >>>>
> > >>>> 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
> > >>>> @@ -454,4 +454,10 @@ struct dmi_system_id {
> > >>>>
> > >>>>  #define DMI_MATCH(a, b)        { a, b }
> > >>>>
> > >>>> +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.
> > > 
> > > 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...
> > 

Re: [PATCH 1/3] scsi_dh: Add modalias support for SCSI targets

[James Bottomley]

On Fri, 2009-04-03 at 15:43 -0700, Chandra Seetharaman wrote:
> 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

Re: [PATCH 1/3] scsi_dh: Add modalias support for SCSI targets

[Hannes Reinecke]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=utf-8, Size: 1271 bytes --]

Hi Chandra,

On Fri, Apr 03, 2009 at 03:43:10PM -0700, Chandra Seetharaman wrote:
> 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
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Markus Rex, HRB 16746 (AG Nürnberg)
--
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: [PATCH 1/3] scsi_dh: Add modalias support for SCSI targets

[Chandra Seetharaman]

Thanks for the response James.
On Tue, 2009-04-07 at 20:59 +0000, James Bottomley wrote:
> On Fri, 2009-04-03 at 15:43 -0700, Chandra Seetharaman wrote:
> > 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

As I mentioned in an earlier email, the purpose of moving the device
handler to SCSI layer from dm layer was to make sure that the device
in-differences are taken care of.

For example, the lsi rdac storage is active/passive and we do want the
SCSI layer to handle appropriately so that the I/O is not tried
repeatedly in the passive path. With the device handler in SCSI layer we
were able to handle it properly, thereby reducing the boot time nicely.

So, for all practical purposes these device handlers serve a purpose by
themselves, even without dm-multipath ever needing to use it.

What dm-multipath does by way of calling scsi_dh_activate can also be
done by the interface Hannes added to the sysfs.

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

I defer this question to Peter :)

> 
> 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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/3] scsi_dh: Add modalias support for SCSI targets

[Chandra Seetharaman]

Thanks for your comment, Hannes.

On Wed, 2009-04-08 at 01:22 +0200, Hannes Reinecke wrote:
> Hi Chandra,
> 
> On Fri, Apr 03, 2009 at 03:43:10PM -0700, Chandra Seetharaman wrote:
> > 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).

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

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

Not true. I have tested this code, module gets inserted and the devices
come under the module as expected during device probing.

Please test it and let me know if you find it otherwise.
> 
> Cheers,
> 
> Hannes

Re: [PATCH 1/3] scsi_dh: Add modalias support for SCSI targets

[Kay Sievers]

On Tue, Apr 7, 2009 at 16:50, Chandra Seetharaman <sekharan@us.ibm.com> wrote:
> On Wed, 2009-04-08 at 01:22 +0200, Hannes Reinecke wrote:
>> On Fri, Apr 03, 2009 at 03:43:10PM -0700, Chandra Seetharaman wrote:

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

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

Re: [PATCH 1/3] scsi_dh: Add modalias support for SCSI targets

[Peter Jones]

On 04/07/2009 04:59 PM, James Bottomley wrote:
> On Fri, 2009-04-03 at 15:43 -0700, Chandra Seetharaman wrote:
>> 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.)

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

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

-- 
        Peter

Re: [PATCH 1/3] scsi_dh: Add modalias support for SCSI targets

[Chandra Seetharaman]

Hi Kay,

Thanks for looking into this.

On Tue, 2009-04-07 at 22:15 -0700, Kay Sievers wrote:
> On Tue, Apr 7, 2009 at 16:50, Chandra Seetharaman <sekharan@us.ibm.com> wrote:
> > On Wed, 2009-04-08 at 01:22 +0200, Hannes Reinecke wrote:
> >> On Fri, Apr 03, 2009 at 03:43:10PM -0700, Chandra Seetharaman wrote:
> 
> >> > 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).
> >
> > 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).
> 
> 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?

Sure.

Currently, when a scsi_dh (like scsi_dh_rdac) module is insmodded, it
sets up a notification for BUS_NOTIFY_ADD_DEVICE, and will walk thru the
bus list and attach all the devices that _are_ already configured.

With the attached patches, there are three ways that a device
can get attached to the module.
 1 scanning of the bus list by the module, if the module is inserted
   after the device is configured.
 2 by handling NOTIFY_ADD_DEVICE by the module, if the device comes
   after the module is inserted.
 3 the third case, which is the race Hannes mentioned, which is 
   mostly w.r.t the device that initiates this modalias chain of
   events, is that the device is past the NOTIFY_ADD_DEVICE point
   in the device configuration path and is still not completely 
   configured hence it is not in the bus_list (so 1 and 2 will 
   miss this). This device will be caught the BOUND_DEVICE
   notification which is just before adding the device to the 
   bus list in the device configuration path.

Hope it is clear now.
> 
> I guess, you either need to implement a "device scan" from the module
> loading routine to find all currently unhandled devices, or you need

We do that currently (without these patches) in scsi_dh. But the problem
is how do we get the module in place.

> 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

You mean call request_module() during device configuration ? Even if we
do that, we will not find the same devices again if we probe, so we will
have the same issue as we have in hand as of now. So not much of an
improvement.

> seems not like the right solution here.
> 
> Thanks,
> Kay

Re: [PATCH 1/3] scsi_dh: Add modalias support for SCSI targets

[Chandra Seetharaman]

Hi James,

Are your concerns answered ?

Hannes, Kay,

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

Please respond :)

chandra
On Wed, 2009-04-08 at 11:08 -0400, Peter Jones wrote:
> On 04/07/2009 04:59 PM, James Bottomley wrote:
> > On Fri, 2009-04-03 at 15:43 -0700, Chandra Seetharaman wrote:
> >> 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.)
> 
> >      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.
> 
> > 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.
> 

Re: [PATCH 1/3] scsi_dh: Add modalias support for SCSI targets

[Hannes Reinecke]

Chandra Seetharaman wrote:
> Hi James,
> 
> Are your concerns answered ?
> 
> Hannes, Kay,
> 
> Is the description related to bus notify BIND DEVICE issue clear now ?
> 
> Please respond :)
> 
Yes, issue is clearer now.

Basically this is another instance where multi-driver binding would make
a difference. Using the 'notifier_chain' approach is basically doing
the very same thing whilst _not_ using driver core binding.

So from that point of view:

Acked-by: Hannes Reinecke <hare@suse.de>

However, I would really like to see it merged with my ALUA update
for scsi_dh; for ALUA matching on the vendor/product tuple is really
quite pointless and we should be using the 'TPGS' setting in
the inquiry data.

Trading your ack for mine :-)

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Markus Rex, HRB 16746 (AG Nürnberg)
--
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

[PATCH 1/3] scsi_dh: Add modalias support for SCSI targets

[Chandra Seetharaman]

From: Peter Jones <pjones@redhat.com>

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 <pjones@redhat.com>
Signed-off-by: Chandra Seetharaman <sekharan@us.ibm.com>
Acked-by: Hannes Reinecke <hare@suse.de>

---
 drivers/scsi/scsi_sysfs.c       |   55 +++++++++++++++++++++++++++++++++++++++-
 include/linux/mod_devicetable.h |    6 ++++
 include/linux/string_helpers.h  |    2 +
 include/scsi/scsi.h             |    1 +
 include/scsi/scsi_device.h      |    9 +-----
 lib/string_helpers.c            |   32 +++++++++++++++++++++++
 scripts/mod/file2alias.c        |   38 ++++++++++++++++++++++++++++
 7 files changed, 134 insertions(+), 9 deletions(-)

Index: linux-2.6.29/drivers/scsi/scsi_sysfs.c
===================================================================
--- linux-2.6.29.orig/drivers/scsi/scsi_sysfs.c
+++ linux-2.6.29/drivers/scsi/scsi_sysfs.c
@@ -10,6 +10,7 @@
 #include <linux/init.h>
 #include <linux/blkdev.h>
 #include <linux/device.h>
+#include <linux/string_helpers.h>
 
 #include <scsi/scsi.h>
 #include <scsi/scsi_device.h>
@@ -362,16 +363,63 @@ static int scsi_bus_match(struct device
 	return (sdp->inq_periph_qual == SCSI_INQ_PQ_CON)? 1: 0;
 }
 
+static ssize_t format_scsi_modalias(struct scsi_device *sdev, char *buffer,
+				    ssize_t len)
+{
+	char vendor[9];
+	char *hex_vendor;
+	char model[17];
+	char *hex_model;
+	int i;
+
+	strncpy(vendor, sdev->vendor, 8);
+	vendor[8] = '\0';
+	for (i = strlen(vendor) - 1; i >= 0; i--) {
+		if (vendor[i] != ' ')
+			break;
+		vendor[i] = '\0';
+	}
+	hex_vendor = string_to_hex(vendor);
+	if (!hex_vendor)
+		return -ENOMEM;
+
+	strncpy(model, sdev->model, 16);
+	model[8] = '\0';
+	for (i = strlen(model) - 1; i >= 0; i--) {
+		if (model[i] != ' ')
+			break;
+		model[i] = '\0';
+	}
+	hex_model = string_to_hex(model);
+	if (!hex_model) {
+		kfree(hex_vendor);
+		return -ENOMEM;
+	}
+
+	i = snprintf(buffer, len, "scsi:t-0x%02xv%.16sm%.32s", sdev->type,
+		 hex_vendor, hex_model);
+	kfree(hex_vendor);
+	kfree(hex_model);
+	return i;
+}
+
 static int scsi_bus_uevent(struct device *dev, struct kobj_uevent_env *env)
 {
 	struct scsi_device *sdev;
+	char buffer[501];
+	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';
+	rc = format_scsi_modalias(sdev, buffer, 500);
+	if (rc < 0)
+		return rc;
+
+	add_uevent_var(env, "MODALIAS=%s", buffer);
 	return 0;
 }
 
@@ -697,8 +745,11 @@ static ssize_t
 sdev_show_modalias(struct device *dev, struct device_attribute *attr, char *buf)
 {
 	struct scsi_device *sdev;
+	ssize_t rc;
+
 	sdev = to_scsi_device(dev);
-	return snprintf (buf, 20, SCSI_DEVICE_MODALIAS_FMT "\n", sdev->type);
+	rc = format_scsi_modalias(sdev, buf, 500);
+	return rc;
 }
 static DEVICE_ATTR(modalias, S_IRUGO, sdev_show_modalias, NULL);
 
Index: linux-2.6.29/include/linux/mod_devicetable.h
===================================================================
--- linux-2.6.29.orig/include/linux/mod_devicetable.h
+++ linux-2.6.29/include/linux/mod_devicetable.h
@@ -454,6 +454,12 @@ struct dmi_system_id {
 
 #define DMI_MATCH(a, b)	{ a, b }
 
+struct scsi_dh_device_id {
+	unsigned char type;
+	char vendor[9];
+	char model[17];
+};
+
 #define PLATFORM_NAME_SIZE	20
 #define PLATFORM_MODULE_PREFIX	"platform:"
 
Index: linux-2.6.29/include/linux/string_helpers.h
===================================================================
--- linux-2.6.29.orig/include/linux/string_helpers.h
+++ linux-2.6.29/include/linux/string_helpers.h
@@ -13,4 +13,6 @@ enum string_size_units {
 int string_get_size(u64 size, enum string_size_units units,
 		    char *buf, int len);
 
+unsigned char *string_to_hex(const unsigned char *s);
+
 #endif
Index: linux-2.6.29/include/scsi/scsi.h
===================================================================
--- linux-2.6.29.orig/include/scsi/scsi.h
+++ linux-2.6.29/include/scsi/scsi.h
@@ -266,6 +266,7 @@ static inline int scsi_status_is_good(in
 #define TYPE_RBC	    0x0e
 #define TYPE_OSD            0x11
 #define TYPE_NO_LUN         0x7f
+#define TYPE_ANY	    0xff
 
 /* SCSI protocols; these are taken from SPC-3 section 7.5 */
 enum scsi_protocol {
Index: linux-2.6.29/include/scsi/scsi_device.h
===================================================================
--- linux-2.6.29.orig/include/scsi/scsi_device.h
+++ linux-2.6.29/include/scsi/scsi_device.h
@@ -1,6 +1,7 @@
 #ifndef _SCSI_SCSI_DEVICE_H
 #define _SCSI_SCSI_DEVICE_H
 
+#include <linux/mod_devicetable.h>
 #include <linux/device.h>
 #include <linux/list.h>
 #include <linux/spinlock.h>
@@ -169,11 +170,6 @@ struct scsi_device {
 	unsigned long		sdev_data[0];
 } __attribute__((aligned(sizeof(unsigned long))));
 
-struct scsi_dh_devlist {
-	char *vendor;
-	char *model;
-};
-
 struct scsi_device_handler {
 	/* Used by the infrastructure */
 	struct list_head list; /* list of scsi_device_handlers */
@@ -181,7 +177,7 @@ struct scsi_device_handler {
 	/* Filled by the hardware handler */
 	struct module *module;
 	const char *name;
-	const struct scsi_dh_devlist *devlist;
+	const struct scsi_dh_device_id *devlist;
 	int (*check_sense)(struct scsi_device *, struct scsi_sense_hdr *);
 	int (*attach)(struct scsi_device *);
 	void (*detach)(struct scsi_device *);
@@ -452,6 +448,5 @@ static inline int scsi_device_protection
 
 #define MODULE_ALIAS_SCSI_DEVICE(type) \
 	MODULE_ALIAS("scsi:t-" __stringify(type) "*")
-#define SCSI_DEVICE_MODALIAS_FMT "scsi:t-0x%02x"
 
 #endif /* _SCSI_SCSI_DEVICE_H */
Index: linux-2.6.29/lib/string_helpers.c
===================================================================
--- linux-2.6.29.orig/lib/string_helpers.c
+++ linux-2.6.29/lib/string_helpers.c
@@ -66,3 +66,35 @@ int string_get_size(u64 size, const enum
 	return 0;
 }
 EXPORT_SYMBOL(string_get_size);
+
+/**
+ * string_to_hex - convert a string to a series of hexidecimal values
+ * @s:	The string to operate on
+ *
+ * This function returns a GFP_KERNEL allocated buffer filled with
+ * the hexidecimal representation of the value of each character in @s .
+ * Returns a pointer to the allocated string on success and NULL on error,
+ * and the returned string is zero terminated.
+ *
+ */
+unsigned char *string_to_hex(const unsigned char *s)
+{
+	unsigned char *ret, *ptr;
+	static const unsigned char *hex = "0123456789ABCDEF";
+	int len;
+
+	len = strlen(s);
+
+	ret = ptr = kmalloc(len * 2 + 1, GFP_KERNEL);
+	if (!ret)
+		return NULL;
+
+	for (; *s; s++) {
+		*ptr++ = hex[(*s & 0xf0)>>4];
+		*ptr++ = hex[*s & 0x0f];
+	}
+	*ptr = '\0';
+
+	return ret;
+}
+EXPORT_SYMBOL(string_to_hex);
Index: linux-2.6.29/scripts/mod/file2alias.c
===================================================================
--- linux-2.6.29.orig/scripts/mod/file2alias.c
+++ linux-2.6.29/scripts/mod/file2alias.c
@@ -51,6 +51,22 @@ do {
                 sprintf(str + strlen(str), "*");                \
 } while(0)
 
+#define ADD_HEX_STR(str, sep, cond, field)				\
+do {									\
+	strcat(str, sep);						\
+	if (cond) {							\
+		char * _s = str + strlen(str);				\
+		char * _f = field;					\
+		static const char *_hex = "0123456789ABCDEF";		\
+									\
+		for (; *_f; _f++) {					\
+			*_s++ = _hex[(*_f & 0xf0)>>4];			\
+			*_s++ = _hex[*_f & 0xf];			\
+		}							\
+	} else								\
+		strcat(str, "*");					\
+} while(0)
+
 /* Always end in a wildcard, for future extension */
 static inline void add_wildcard(char *str)
 {
@@ -718,6 +734,23 @@ static int do_platform_entry(const char
 	return 1;
 }
 
+
+/* Looks like: scsi:t-NvSmS */
+/* defining TYPE_ANY here is a gross hack to avoid moving all the scsi.h
+ * TYPE_ definitions into mod_devicetable.h */
+#define TYPE_ANY 0xff
+static int do_scsi_entry(const char *filename,
+			struct scsi_dh_device_id *id, char *alias)
+{
+	strcpy(alias, "scsi:");
+	ADD(alias, "t-", id->type != TYPE_ANY, id->type);
+	ADD_HEX_STR(alias, "v", id->vendor[0] != '\0', id->vendor);
+	ADD_HEX_STR(alias, "m", id->model[0] != '\0', id->model);
+
+	add_wildcard(alias);
+	return 1;
+}
+
 /* Ignore any prefix, eg. some architectures prepend _ */
 static inline int sym_is(const char *symbol, const char *name)
 {
@@ -744,6 +777,7 @@ static void do_table(void *symval, unsig
 	size -= id_size;
 
 	for (i = 0; i < size; i += id_size) {
+		memset(alias, '\0', 500);
 		if (do_entry(mod->name, symval+i, alias)) {
 			buf_printf(&mod->dev_table_buf,
 				   "MODULE_ALIAS(\"%s\");\n", alias);
@@ -861,6 +895,10 @@ void handle_moddevtable(struct module *m
 		do_table(symval, sym->st_size,
 			 sizeof(struct platform_device_id), "platform",
 			 do_platform_entry, mod);
+	else if (sym_is(symname, "__mod_scsi_dh_device_table"))
+		do_table(symval, sym->st_size,
+			 sizeof(struct scsi_dh_device_id), "scsi",
+			 do_scsi_entry, mod);
 	free(zeros);
 }