[PATCH 7/7] scsi_dh: attach to hardware handler from dm-mpath

[PATCH 7/7] scsi_dh: attach to hardware handler from dm-mpath

[Hannes Reinecke]

multipath keeps a separate device table which may be
more current than the built-in one.
So we should allow to override the multipath settings
by calling ->attach for each device.

Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 drivers/md/dm-mpath.c                 |   10 ++++++
 drivers/scsi/device_handler/scsi_dh.c |   52 ++++++++++++++++++++++++++++++--
 include/scsi/scsi_dh.h                |    1 +
 3 files changed, 59 insertions(+), 4 deletions(-)

diff --git a/drivers/md/dm-mpath.c b/drivers/md/dm-mpath.c
index e8f704a..b69cd87 100644
--- a/drivers/md/dm-mpath.c
+++ b/drivers/md/dm-mpath.c
@@ -546,6 +546,7 @@ static struct pgpath *parse_path(struct arg_set *as, struct path_selector *ps,
 {
 	int r;
 	struct pgpath *p;
+	struct multipath *m = (struct multipath *) ti->private;
 
 	/* we need at least a path arg */
 	if (as->argc < 1) {
@@ -564,6 +565,15 @@ static struct pgpath *parse_path(struct arg_set *as, struct path_selector *ps,
 		goto bad;
 	}
 
+	if (m->hw_handler_name) {
+		r = scsi_dh_attach(bdev_get_queue(p->path.dev->bdev),
+				   m->hw_handler_name);
+		if (r < 0) {
+			dm_put_device(ti, p->path.dev);
+			goto bad;
+		}
+	}
+
 	r = ps->type->add_path(ps, &p->path, as->argc, as->argv, &ti->error);
 	if (r) {
 		dm_put_device(ti, p->path.dev);
diff --git a/drivers/scsi/device_handler/scsi_dh.c b/drivers/scsi/device_handler/scsi_dh.c
index 2dbf84b..dfca3db 100644
--- a/drivers/scsi/device_handler/scsi_dh.c
+++ b/drivers/scsi/device_handler/scsi_dh.c
@@ -113,8 +113,12 @@ int scsi_dh_handler_attach(struct scsi_device *sdev,
 {
 	int err = -EBUSY;
 
-	if (sdev->scsi_dh_data)
-		return err;
+	if (sdev->scsi_dh_data) {
+		if (sdev->scsi_dh_data->scsi_dh != scsi_dh)
+			return err;
+		else
+			return 0;
+	}
 
 	err = scsi_dh->attach(sdev);
 
@@ -163,8 +167,11 @@ static int scsi_dh_notifier(struct notifier_block *nb,
 		goto out;
 
 	if (action == BUS_NOTIFY_ADD_DEVICE) {
-		scsi_dh_handler_attach(sdev, devinfo->handler);
-		device_create_file(dev, &scsi_dh_state_attr);
+		int err;
+
+		err = scsi_dh_handler_attach(sdev, devinfo->handler);
+		if (!err)
+			err = device_create_file(dev, &scsi_dh_state_attr);
 	} else if (action == BUS_NOTIFY_DEL_DEVICE) {
 		if (sdev->scsi_dh_data == NULL)
 			goto out;
@@ -345,6 +352,43 @@ int scsi_dh_handler_exist(const char *name)
 }
 EXPORT_SYMBOL_GPL(scsi_dh_handler_exist);
 
+/*
+ * scsi_dh_handler_attach - Attach device handler
+ * @sdev - sdev the handler should be attached to
+ * @name - name of the handler to attach
+ */
+int scsi_dh_attach(struct request_queue *q, const char *name)
+{
+	unsigned long flags;
+	struct scsi_device *sdev;
+	struct scsi_device_handler *scsi_dh;
+	int err = 0;
+
+	scsi_dh = get_device_handler(name);
+	if (!scsi_dh)
+		return -EINVAL;
+
+	spin_lock_irqsave(q->queue_lock, flags);
+	sdev = q->queuedata;
+	if (sdev && sdev->scsi_dh_data) {
+		if (sdev->scsi_dh_data->scsi_dh != scsi_dh) {
+			err = -EBUSY;
+		} else {
+			spin_unlock_irqrestore(q->queue_lock, flags);
+			return 0;
+		}
+	}
+	if (!err || !get_device(&sdev->sdev_gendev))
+		err = -ENODEV;
+	spin_unlock_irqrestore(q->queue_lock, flags);
+
+	if (scsi_dh->attach)
+		err = scsi_dh->attach(sdev);
+	put_device(&sdev->sdev_gendev);
+	return err;
+}
+EXPORT_SYMBOL_GPL(scsi_dh_attach);
+
 static struct notifier_block scsi_dh_nb = {
 	.notifier_call = scsi_dh_notifier
 };
diff --git a/include/scsi/scsi_dh.h b/include/scsi/scsi_dh.h
index 5d05ac0..567bd38 100644
--- a/include/scsi/scsi_dh.h
+++ b/include/scsi/scsi_dh.h
@@ -59,3 +59,4 @@ enum {
 
 extern int scsi_dh_activate(struct request_queue *);
 extern int scsi_dh_handler_exist(const char *);
+extern int scsi_dh_attach(struct request_queue *, const char *);
-- 
1.5.2.4

Re: [PATCH 7/7] scsi_dh: attach to hardware handler from dm-mpath

[Chandra Seetharaman]

What is the purpose of this feature ?

With dh_state functionality, one could associate a handler to a new
device and then if one does "multipath", the hardware handler would be
associated with the multipath device. What are we achieving by this ?

The review comments below are on the assumption that this is a needed
feature.
---

Shouldn't we provide a detach also ?

On Wed, 2008-05-14 at 16:43 +0200, Hannes Reinecke wrote:
> multipath keeps a separate device table which may be
> more current than the built-in one.
> So we should allow to override the multipath settings
> by calling ->attach for each device.
> 
> Signed-off-by: Hannes Reinecke <hare@suse.de>
> ---
>  drivers/md/dm-mpath.c                 |   10 ++++++
>  drivers/scsi/device_handler/scsi_dh.c |   52 ++++++++++++++++++++++++++++++--
>  include/scsi/scsi_dh.h                |    1 +
>  3 files changed, 59 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/md/dm-mpath.c b/drivers/md/dm-mpath.c
> index e8f704a..b69cd87 100644
> --- a/drivers/md/dm-mpath.c
> +++ b/drivers/md/dm-mpath.c
> @@ -546,6 +546,7 @@ static struct pgpath *parse_path(struct arg_set *as, struct path_selector *ps,
>  {
>  	int r;
>  	struct pgpath *p;
> +	struct multipath *m = (struct multipath *) ti->private;
> 
>  	/* we need at least a path arg */
>  	if (as->argc < 1) {
> @@ -564,6 +565,15 @@ static struct pgpath *parse_path(struct arg_set *as, struct path_selector *ps,
>  		goto bad;
>  	}
> 
> +	if (m->hw_handler_name) {
> +		r = scsi_dh_attach(bdev_get_queue(p->path.dev->bdev),
> +				   m->hw_handler_name);
> +		if (r < 0) {
> +			dm_put_device(ti, p->path.dev);
> +			goto bad;
> +		}
> +	}
> +
>  	r = ps->type->add_path(ps, &p->path, as->argc, as->argv, &ti->error);
>  	if (r) {
>  		dm_put_device(ti, p->path.dev);
> diff --git a/drivers/scsi/device_handler/scsi_dh.c b/drivers/scsi/device_handler/scsi_dh.c
> index 2dbf84b..dfca3db 100644
> --- a/drivers/scsi/device_handler/scsi_dh.c
> +++ b/drivers/scsi/device_handler/scsi_dh.c
> @@ -113,8 +113,12 @@ int scsi_dh_handler_attach(struct scsi_device *sdev,
>  {
>  	int err = -EBUSY;
> 
> -	if (sdev->scsi_dh_data)
> -		return err;
> +	if (sdev->scsi_dh_data) {
> +		if (sdev->scsi_dh_data->scsi_dh != scsi_dh)
> +			return err;
> +		else
> +			return 0;
> +	}

This need to be moved to an earlier patch.
> 
>  	err = scsi_dh->attach(sdev);
> 
> @@ -163,8 +167,11 @@ static int scsi_dh_notifier(struct notifier_block *nb,
>  		goto out;
> 
>  	if (action == BUS_NOTIFY_ADD_DEVICE) {
> -		scsi_dh_handler_attach(sdev, devinfo->handler);
> -		device_create_file(dev, &scsi_dh_state_attr);
> +		int err;
> +
> +		err = scsi_dh_handler_attach(sdev, devinfo->handler);
> +		if (!err)
> +			err = device_create_file(dev, &scsi_dh_state_attr);

This too.

>  	} else if (action == BUS_NOTIFY_DEL_DEVICE) {
>  		if (sdev->scsi_dh_data == NULL)
>  			goto out;
> @@ -345,6 +352,43 @@ int scsi_dh_handler_exist(const char *name)
>  }
>  EXPORT_SYMBOL_GPL(scsi_dh_handler_exist);
> 
> +/*
> + * scsi_dh_handler_attach - Attach device handler
> + * @sdev - sdev the handler should be attached to
> + * @name - name of the handler to attach
> + */
> +int scsi_dh_attach(struct request_queue *q, const char *name)
> +{

This code seems to do incorrect things (run the case with sdev == NULL
or sdev->scsi_dh_data->scsi_dh != scsi_dh).

This function can be simplified to just call scsi_dh_handler_attach()
after getting scsi_dh and a get_device.

We can avoid calling scsi_dh->attach directly. instead call
scsi_dh_device_handler(), which would do all the necessary checking.

> +	unsigned long flags;
> +	struct scsi_device *sdev;
> +	struct scsi_device_handler *scsi_dh;
> +	int err = 0;
> +
> +	scsi_dh = get_device_handler(name);
> +	if (!scsi_dh)
> +		return -EINVAL;
> +
> +	spin_lock_irqsave(q->queue_lock, flags);
> +	sdev = q->queuedata;
> +	if (sdev && sdev->scsi_dh_data) {
> +		if (sdev->scsi_dh_data->scsi_dh != scsi_dh) {
> +			err = -EBUSY;
> +		} else {
> +			spin_unlock_irqrestore(q->queue_lock, flags);
> +			return 0;
> +		}
> +	}
> +	if (!err || !get_device(&sdev->sdev_gendev))
> +		err = -ENODEV;
> +	spin_unlock_irqrestore(q->queue_lock, flags);
> +
> +	if (scsi_dh->attach)
> +		err = scsi_dh->attach(sdev);
> +	put_device(&sdev->sdev_gendev);
> +	return err;
> +}
> +EXPORT_SYMBOL_GPL(scsi_dh_attach);
> +
>  static struct notifier_block scsi_dh_nb = {
>  	.notifier_call = scsi_dh_notifier
>  };
> diff --git a/include/scsi/scsi_dh.h b/include/scsi/scsi_dh.h
> index 5d05ac0..567bd38 100644
> --- a/include/scsi/scsi_dh.h
> +++ b/include/scsi/scsi_dh.h
> @@ -59,3 +59,4 @@ enum {
> 
>  extern int scsi_dh_activate(struct request_queue *);
>  extern int scsi_dh_handler_exist(const char *);
> +extern int scsi_dh_attach(struct request_queue *, const char *);

Re: [PATCH 7/7] scsi_dh: attach to hardware handler from dm-mpath

[Hannes Reinecke]

Hi Chandra,

Chandra Seetharaman wrote:
> What is the purpose of this feature ?
> 
> With dh_state functionality, one could associate a handler to a new
> device and then if one does "multipath", the hardware handler would be
> associated with the multipath device. What are we achieving by this ?
> 
This allows multipath to override the device tables build into
the device handler. Reason here is that multipath has a configuration
file which allows the user to override the build-in defaults.
And this includes the hardware handler, so we need to make sure that
the hardware handler is indeed attached.

I'd rather have it here instead of doing it from userland, as I try
to avoid searching through sysfs wherever possible.

> The review comments below are on the assumption that this is a needed
> feature.
> ---
> 
> Shouldn't we provide a detach also ?
> 
Hmm. Maybe. I'll check if it's possible to distinguish between
built-in devices (which shouldn't be detached) and overridden
ones (which should be).

> On Wed, 2008-05-14 at 16:43 +0200, Hannes Reinecke wrote:
>> multipath keeps a separate device table which may be
>> more current than the built-in one.
>> So we should allow to override the multipath settings
>> by calling ->attach for each device.
>>
>> Signed-off-by: Hannes Reinecke <hare@suse.de>
>> ---
>>  drivers/md/dm-mpath.c                 |   10 ++++++
>>  drivers/scsi/device_handler/scsi_dh.c |   52 ++++++++++++++++++++++++++++++--
>>  include/scsi/scsi_dh.h                |    1 +
>>  3 files changed, 59 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/md/dm-mpath.c b/drivers/md/dm-mpath.c
>> index e8f704a..b69cd87 100644
>> --- a/drivers/md/dm-mpath.c
>> +++ b/drivers/md/dm-mpath.c
>> @@ -546,6 +546,7 @@ static struct pgpath *parse_path(struct arg_set *as, struct path_selector *ps,
>>  {
>>  	int r;
>>  	struct pgpath *p;
>> +	struct multipath *m = (struct multipath *) ti->private;
>>
>>  	/* we need at least a path arg */
>>  	if (as->argc < 1) {
>> @@ -564,6 +565,15 @@ static struct pgpath *parse_path(struct arg_set *as, struct path_selector *ps,
>>  		goto bad;
>>  	}
>>
>> +	if (m->hw_handler_name) {
>> +		r = scsi_dh_attach(bdev_get_queue(p->path.dev->bdev),
>> +				   m->hw_handler_name);
>> +		if (r < 0) {
>> +			dm_put_device(ti, p->path.dev);
>> +			goto bad;
>> +		}
>> +	}
>> +
>>  	r = ps->type->add_path(ps, &p->path, as->argc, as->argv, &ti->error);
>>  	if (r) {
>>  		dm_put_device(ti, p->path.dev);
>> diff --git a/drivers/scsi/device_handler/scsi_dh.c b/drivers/scsi/device_handler/scsi_dh.c
>> index 2dbf84b..dfca3db 100644
>> --- a/drivers/scsi/device_handler/scsi_dh.c
>> +++ b/drivers/scsi/device_handler/scsi_dh.c
>> @@ -113,8 +113,12 @@ int scsi_dh_handler_attach(struct scsi_device *sdev,
>>  {
>>  	int err = -EBUSY;
>>
>> -	if (sdev->scsi_dh_data)
>> -		return err;
>> +	if (sdev->scsi_dh_data) {
>> +		if (sdev->scsi_dh_data->scsi_dh != scsi_dh)
>> +			return err;
>> +		else
>> +			return 0;
>> +	}
> 
> This need to be moved to an earlier patch.
Yes.

>>  	err = scsi_dh->attach(sdev);
>>
>> @@ -163,8 +167,11 @@ static int scsi_dh_notifier(struct notifier_block *nb,
>>  		goto out;
>>
>>  	if (action == BUS_NOTIFY_ADD_DEVICE) {
>> -		scsi_dh_handler_attach(sdev, devinfo->handler);
>> -		device_create_file(dev, &scsi_dh_state_attr);
>> +		int err;
>> +
>> +		err = scsi_dh_handler_attach(sdev, devinfo->handler);
>> +		if (!err)
>> +			err = device_create_file(dev, &scsi_dh_state_attr);
> 
> This too.
> 
Ok.

>>  	} else if (action == BUS_NOTIFY_DEL_DEVICE) {
>>  		if (sdev->scsi_dh_data == NULL)
>>  			goto out;
>> @@ -345,6 +352,43 @@ int scsi_dh_handler_exist(const char *name)
>>  }
>>  EXPORT_SYMBOL_GPL(scsi_dh_handler_exist);
>>
>> +/*
>> + * scsi_dh_handler_attach - Attach device handler
>> + * @sdev - sdev the handler should be attached to
>> + * @name - name of the handler to attach
>> + */
>> +int scsi_dh_attach(struct request_queue *q, const char *name)
>> +{
> 
> This code seems to do incorrect things (run the case with sdev == NULL
> or sdev->scsi_dh_data->scsi_dh != scsi_dh).
> 
> This function can be simplified to just call scsi_dh_handler_attach()
> after getting scsi_dh and a get_device.
> 
> We can avoid calling scsi_dh->attach directly. instead call
> scsi_dh_device_handler(), which would do all the necessary checking.
> 
Correct. Fixed.

Chandra, thank you very much for your review.
That helped a lot (and fixed some bugs, too :-).

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 7/7] scsi_dh: attach to hardware handler from dm-mpath

[Chandra Seetharaman]

On Thu, 2008-05-15 at 11:46 +0200, Hannes Reinecke wrote:
> Hi Chandra,
> 
> Chandra Seetharaman wrote:
> > What is the purpose of this feature ?
> > 
> > With dh_state functionality, one could associate a handler to a new
> > device and then if one does "multipath", the hardware handler would be
> > associated with the multipath device. What are we achieving by this ?
> > 
> This allows multipath to override the device tables build into
> the device handler. Reason here is that multipath has a configuration
> file which allows the user to override the build-in defaults.
> And this includes the hardware handler, so we need to make sure that
> the hardware handler is indeed attached.

Hardware handler's existence is currently been verified during parsing
(by doing a request_module() followed by scsi_dh_handler_exist() in
parse_hw_handler()).

Hardware handler module is attached to the device itself (thru
try_module_get() in attach), so, the module will exist as long as the
device exists.

Hence, there is no need to attach it again from the multipath layer.

Or, I am missing something totally :)

<snip>

Re: [PATCH 7/7] scsi_dh: attach to hardware handler from dm-mpath

[Hannes Reinecke]

Hi Chandra,

Chandra Seetharaman wrote:
> On Thu, 2008-05-15 at 11:46 +0200, Hannes Reinecke wrote:
>> Hi Chandra,
>>
>> Chandra Seetharaman wrote:
>>> What is the purpose of this feature ?
>>>
>>> With dh_state functionality, one could associate a handler to a new
>>> device and then if one does "multipath", the hardware handler would be
>>> associated with the multipath device. What are we achieving by this ?
>>>
>> This allows multipath to override the device tables build into
>> the device handler. Reason here is that multipath has a configuration
>> file which allows the user to override the build-in defaults.
>> And this includes the hardware handler, so we need to make sure that
>> the hardware handler is indeed attached.
> 
> Hardware handler's existence is currently been verified during parsing
> (by doing a request_module() followed by scsi_dh_handler_exist() in
> parse_hw_handler()).
> 
Yes, but this only verifies that the _handler_ exist, not that it is
attached to this sdev

> Hardware handler module is attached to the device itself (thru
> try_module_get() in attach), so, the module will exist as long as the
> device exists.
> 
Not quite. It's only attached automatically if the device map has
an entry for this module.
However, given this patchset we can now easily manually attach any
device to the device handler. Or at least try to do so.
It's then up to the device handler to refuse binding if none of
the necessary pages/information etc. was found.

> Hence, there is no need to attach it again from the multipath layer.
> 
Yes, you do. The user might have it's own custom configuration file,
covering new/unknown/unhandled/testing devices, which out of necessity
are _not_ hardcoded in the device table of the device handler.
So multipath has to have a way of attaching device handler to those
devices, too.

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 7/7] scsi_dh: attach to hardware handler from dm-mpath

[Chandra Seetharaman]

On Mon, 2008-05-19 at 12:21 +0200, Hannes Reinecke wrote:
> Hi Chandra,
> 
> Chandra Seetharaman wrote:
> > On Thu, 2008-05-15 at 11:46 +0200, Hannes Reinecke wrote:
> >> Hi Chandra,
> >>
> >> Chandra Seetharaman wrote:
> >>> What is the purpose of this feature ?
> >>>
> >>> With dh_state functionality, one could associate a handler to a new
> >>> device and then if one does "multipath", the hardware handler would be
> >>> associated with the multipath device. What are we achieving by this ?
> >>>
> >> This allows multipath to override the device tables build into
> >> the device handler. Reason here is that multipath has a configuration
> >> file which allows the user to override the build-in defaults.
> >> And this includes the hardware handler, so we need to make sure that
> >> the hardware handler is indeed attached.
> > 
> > Hardware handler's existence is currently been verified during parsing
> > (by doing a request_module() followed by scsi_dh_handler_exist() in
> > parse_hw_handler()).
> > 
> Yes, but this only verifies that the _handler_ exist, not that it is
> attached to this sdev

Agreed. And it works properly with the original design.

Here is my thinking on how it would work with the new feature (ability
to attach any device using dh_state) you added:
 - User adds a new device
 - User knows which scsi hardware handler their device can attach to,
   lets say, "mydev".
 - User attaches the device with scsi hardware handler (writing "mydev"
   to "dh_state").
   Device is now attached to "mydev"
 - User updates his/her multipath.conf file with "mydev" for the
   specific device.
 - User invokes multipath, which checks for the existence of "mydev"
   and allows the table insertion.
 - When pg_init was required in the future, multipath calls "activate"
   for "mydev" correctly.

Wouldn't this work ?

Note: In the absence of this patch (7/7) only.

> 
> > Hardware handler module is attached to the device itself (thru
> > try_module_get() in attach), so, the module will exist as long as the
> > device exists.
> > 
> Not quite. It's only attached automatically if the device map has
> an entry for this module.
> However, given this patchset we can now easily manually attach any
> device to the device handler. Or at least try to do so.
> It's then up to the device handler to refuse binding if none of
> the necessary pages/information etc. was found.

Doesn't dh_state solve this issue ?

> 
> > Hence, there is no need to attach it again from the multipath layer.
> > 
> Yes, you do. The user might have it's own custom configuration file,
> covering new/unknown/unhandled/testing devices, which out of necessity
> are _not_ hardcoded in the device table of the device handler.
> So multipath has to have a way of attaching device handler to those
> devices, too.

dh_state allows the user to do this, in which case why do we need to do
this in multipath layer ?

> 
> Cheers,
> 
> Hannes

Re: [PATCH 7/7] scsi_dh: attach to hardware handler from dm-mpath

[Hannes Reinecke]

Hi Chandra,

Chandra Seetharaman wrote:
> On Mon, 2008-05-19 at 12:21 +0200, Hannes Reinecke wrote:
>> Hi Chandra,
>>
[ .. ]
>> Yes, but this only verifies that the _handler_ exist, not that it is
>> attached to this sdev
> 
> Agreed. And it works properly with the original design.
> 
> Here is my thinking on how it would work with the new feature (ability
> to attach any device using dh_state) you added:
>  - User adds a new device
>  - User knows which scsi hardware handler their device can attach to,
>    lets say, "mydev".
>  - User attaches the device with scsi hardware handler (writing "mydev"
>    to "dh_state").
>    Device is now attached to "mydev"
>  - User updates his/her multipath.conf file with "mydev" for the
>    specific device.
>  - User invokes multipath, which checks for the existence of "mydev"
>    and allows the table insertion.
>  - When pg_init was required in the future, multipath calls "activate"
>    for "mydev" correctly.
> 
> Wouldn't this work ?
> 
> Note: In the absence of this patch (7/7) only.
> 
Yes, but only for the first time. After reboot the user would
have to do the reconfiguration again.

>>> Hardware handler module is attached to the device itself (thru
>>> try_module_get() in attach), so, the module will exist as long as the
>>> device exists.
>>>
>> Not quite. It's only attached automatically if the device map has
>> an entry for this module.
>> However, given this patchset we can now easily manually attach any
>> device to the device handler. Or at least try to do so.
>> It's then up to the device handler to refuse binding if none of
>> the necessary pages/information etc. was found.
> 
> Doesn't dh_state solve this issue ?
> 
In theory, yes.

>>> Hence, there is no need to attach it again from the multipath layer.
>>>
>> Yes, you do. The user might have it's own custom configuration file,
>> covering new/unknown/unhandled/testing devices, which out of necessity
>> are _not_ hardcoded in the device table of the device handler.
>> So multipath has to have a way of attaching device handler to those
>> devices, too.
> 
> dh_state allows the user to do this, in which case why do we need to do
> this in multipath layer ?
> 
Because then we (read: I) don't have to modify multipath-tools.
If we don't take this patch we'll have to modify multipath-tools
to check for the dh_state attribute explicitly and activate
the hardware handler directly via sysfs.

But this also means that existing multipath-tools programs won't
be able to correctly activate the hardware handler in
_some_ cases (ie those cases, where the hardware table for the
device_handler doesn't contain the device in question).
And I can just imagine the bugs I'll be getting ...
So if we decide to take that road we should kill the hardware
handler feature from the device-mapper multipath interface
altogether to notify the user that the programs won't work
anymore. Otherwise it's just heading for trouble.

So I'd rather keep the userland interface stable here and add
some code to the kernel.

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 7/7] scsi_dh: attach to hardware handler from dm-mpath

[Hannes Reinecke]

multipath keeps a separate device table which may be
more current than the built-in one.
So we should make sure to always call ->attach whenever
a multipath map with hardware handler is instantiated.
And we should call ->detach on removal, too.

Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 drivers/md/dm-mpath.c                 |   13 ++++++
 drivers/scsi/device_handler/scsi_dh.c |   74 +++++++++++++++++++++++++++++++++
 include/scsi/scsi_dh.h                |    2 +
 3 files changed, 89 insertions(+), 0 deletions(-)

diff --git a/drivers/md/dm-mpath.c b/drivers/md/dm-mpath.c
index e8f704a..bd2bcf4 100644
--- a/drivers/md/dm-mpath.c
+++ b/drivers/md/dm-mpath.c
@@ -147,9 +147,12 @@ static struct priority_group *alloc_priority_group(void)
 static void free_pgpaths(struct list_head *pgpaths, struct dm_target *ti)
 {
 	struct pgpath *pgpath, *tmp;
+	struct multipath *m = (struct multipath *) ti->private;
 
 	list_for_each_entry_safe(pgpath, tmp, pgpaths, list) {
 		list_del(&pgpath->list);
+		if (m->hw_handler_name)
+			scsi_dh_detach(bdev_get_queue(pgpath->path.dev->bdev));
 		dm_put_device(ti, pgpath->path.dev);
 		free_pgpath(pgpath);
 	}
@@ -546,6 +549,7 @@ static struct pgpath *parse_path(struct arg_set *as, struct path_selector *ps,
 {
 	int r;
 	struct pgpath *p;
+	struct multipath *m = (struct multipath *) ti->private;
 
 	/* we need at least a path arg */
 	if (as->argc < 1) {
@@ -564,6 +568,15 @@ static struct pgpath *parse_path(struct arg_set *as, struct path_selector *ps,
 		goto bad;
 	}
 
+	if (m->hw_handler_name) {
+		r = scsi_dh_attach(bdev_get_queue(p->path.dev->bdev),
+				   m->hw_handler_name);
+		if (r < 0) {
+			dm_put_device(ti, p->path.dev);
+			goto bad;
+		}
+	}
+
 	r = ps->type->add_path(ps, &p->path, as->argc, as->argv, &ti->error);
 	if (r) {
 		dm_put_device(ti, p->path.dev);
diff --git a/drivers/scsi/device_handler/scsi_dh.c b/drivers/scsi/device_handler/scsi_dh.c
index b80fae7..a9404b5 100644
--- a/drivers/scsi/device_handler/scsi_dh.c
+++ b/drivers/scsi/device_handler/scsi_dh.c
@@ -362,6 +362,80 @@ int scsi_dh_handler_exist(const char *name)
 }
 EXPORT_SYMBOL_GPL(scsi_dh_handler_exist);
 
+/*
+ * scsi_dh_handler_attach - Attach device handler
+ * @sdev - sdev the handler should be attached to
+ * @name - name of the handler to attach
+ */
+int scsi_dh_attach(struct request_queue *q, const char *name)
+{
+	unsigned long flags;
+	struct scsi_device *sdev;
+	struct scsi_device_handler *scsi_dh;
+	int err = 0;
+
+	scsi_dh = get_device_handler(name);
+	if (!scsi_dh)
+		return -EINVAL;
+
+	spin_lock_irqsave(q->queue_lock, flags);
+	sdev = q->queuedata;
+	if (!sdev || !get_device(&sdev->sdev_gendev))
+		err = -ENODEV;
+	spin_unlock_irqrestore(q->queue_lock, flags);
+
+	if (!err) {
+		err = scsi_dh_handler_attach(sdev, scsi_dh);
+
+		put_device(&sdev->sdev_gendev);
+	}
+	return err;
+}
+EXPORT_SYMBOL_GPL(scsi_dh_attach);
+
+/*
+ * scsi_dh_handler_detach - Detach device handler
+ * @sdev - sdev the handler should be detached from
+ *
+ * This function will detach the device handler only
+ * if the sdev is not part of the internal list, ie
+ * if it has been attached manually.
+ */
+void scsi_dh_detach(struct request_queue *q)
+{
+	unsigned long flags;
+	struct scsi_device *sdev;
+	struct scsi_device_handler *scsi_dh = NULL;
+	int i, found = 0;
+
+	spin_lock_irqsave(q->queue_lock, flags);
+	sdev = q->queuedata;
+	if (!sdev || !get_device(&sdev->sdev_gendev))
+		sdev = NULL;
+	spin_unlock_irqrestore(q->queue_lock, flags);
+
+	if (!sdev)
+		return;
+
+	if (sdev->scsi_dh_data) {
+		scsi_dh = sdev->scsi_dh_data->scsi_dh;
+		for(i = 0; scsi_dh->devlist[i].vendor; i++) {
+			if (!strncmp(sdev->vendor, scsi_dh->devlist[i].vendor,
+				     strlen(scsi_dh->devlist[i].vendor)) &&
+			    !strncmp(sdev->model, scsi_dh->devlist[i].model,
+				     strlen(scsi_dh->devlist[i].model))) {
+				found = 1;
+				break;
+			}
+		}
+		/* sdev not on internal list, detach */
+		if (!found)
+			scsi_dh_handler_detach(sdev);
+	}
+	put_device(&sdev->sdev_gendev);
+}
+EXPORT_SYMBOL_GPL(scsi_dh_detach);
+
 static struct notifier_block scsi_dh_nb = {
 	.notifier_call = scsi_dh_notifier
 };
diff --git a/include/scsi/scsi_dh.h b/include/scsi/scsi_dh.h
index 5d05ac0..8032e51 100644
--- a/include/scsi/scsi_dh.h
+++ b/include/scsi/scsi_dh.h
@@ -59,3 +59,5 @@ enum {
 
 extern int scsi_dh_activate(struct request_queue *);
 extern int scsi_dh_handler_exist(const char *);
+extern int scsi_dh_attach(struct request_queue *, const char *);
+extern void scsi_dh_detach(struct request_queue *q);
-- 
1.5.2.4

Re: [PATCH 7/7] scsi_dh: attach to hardware handler from dm-mpath

[Chandra Seetharaman]

Hi Hannes,

Now I see why you want this change in dm-multipath. I think I agree with
these changes.

But, it brings another question: what does dh_state provide ? Help to
user to see which hardware handler a device is attached to ?

I thought more about the scsi_dh_detach function (in the context of my
earlier comment), adding it would require more housekeeping to associate
one-to-one mapping between attach and detach. We can leave it the same
way as the module will be detached when the device disappear eventually.

Thanks,

chandra 
On Tue, 2008-05-20 at 14:41 +0200, Hannes Reinecke wrote:
> Hi Chandra,
> 
> Chandra Seetharaman wrote:
> > On Mon, 2008-05-19 at 12:21 +0200, Hannes Reinecke wrote:
> >> Hi Chandra,
> >>
> [ .. ]
> >> Yes, but this only verifies that the _handler_ exist, not that it is
> >> attached to this sdev
> > 
> > Agreed. And it works properly with the original design.
> > 
> > Here is my thinking on how it would work with the new feature (ability
> > to attach any device using dh_state) you added:
> >  - User adds a new device
> >  - User knows which scsi hardware handler their device can attach to,
> >    lets say, "mydev".
> >  - User attaches the device with scsi hardware handler (writing "mydev"
> >    to "dh_state").
> >    Device is now attached to "mydev"
> >  - User updates his/her multipath.conf file with "mydev" for the
> >    specific device.
> >  - User invokes multipath, which checks for the existence of "mydev"
> >    and allows the table insertion.
> >  - When pg_init was required in the future, multipath calls "activate"
> >    for "mydev" correctly.
> > 
> > Wouldn't this work ?
> > 
> > Note: In the absence of this patch (7/7) only.
> > 
> Yes, but only for the first time. After reboot the user would
> have to do the reconfiguration again.
> 
> >>> Hardware handler module is attached to the device itself (thru
> >>> try_module_get() in attach), so, the module will exist as long as the
> >>> device exists.
> >>>
> >> Not quite. It's only attached automatically if the device map has
> >> an entry for this module.
> >> However, given this patchset we can now easily manually attach any
> >> device to the device handler. Or at least try to do so.
> >> It's then up to the device handler to refuse binding if none of
> >> the necessary pages/information etc. was found.
> > 
> > Doesn't dh_state solve this issue ?
> > 
> In theory, yes.
> 
> >>> Hence, there is no need to attach it again from the multipath layer.
> >>>
> >> Yes, you do. The user might have it's own custom configuration file,
> >> covering new/unknown/unhandled/testing devices, which out of necessity
> >> are _not_ hardcoded in the device table of the device handler.
> >> So multipath has to have a way of attaching device handler to those
> >> devices, too.
> > 
> > dh_state allows the user to do this, in which case why do we need to do
> > this in multipath layer ?
> > 
> Because then we (read: I) don't have to modify multipath-tools.
> If we don't take this patch we'll have to modify multipath-tools
> to check for the dh_state attribute explicitly and activate
> the hardware handler directly via sysfs.
> 
> But this also means that existing multipath-tools programs won't
> be able to correctly activate the hardware handler in
> _some_ cases (ie those cases, where the hardware table for the
> device_handler doesn't contain the device in question).
> And I can just imagine the bugs I'll be getting ...
> So if we decide to take that road we should kill the hardware
> handler feature from the device-mapper multipath interface
> altogether to notify the user that the programs won't work
> anymore. Otherwise it's just heading for trouble.
> 
> So I'd rather keep the userland interface stable here and add
> some code to the kernel.
> 
> Cheers,
> 
> Hannes

Re: [PATCH 7/7] scsi_dh: attach to hardware handler from dm-mpath

[Hannes Reinecke]

Chandra Seetharaman wrote:
> Hi Hannes,
> 
> Now I see why you want this change in dm-multipath. I think I agree with
> these changes.
> 
Ah. good.

> But, it brings another question: what does dh_state provide ? Help to
> user to see which hardware handler a device is attached to ?
> 
And allowing to attach to a different hardware handler.
Not everyone is running multipathing, but might be interested in the
having the device handler nevertheless.

> I thought more about the scsi_dh_detach function (in the context of my
> earlier comment), adding it would require more housekeeping to associate
> one-to-one mapping between attach and detach. We can leave it the same
> way as the module will be detached when the device disappear eventually.
> 
Why? You can detach with dh_state, too; just do an

echo detach > /sys/block/sdX/device/dh_state

and the hardware handler will detach.
So no additional attribute is required.

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 7/7] scsi_dh: attach to hardware handler from dm-mpath

[James Bottomley]

On Wed, 2008-05-21 at 08:18 +0200, Hannes Reinecke wrote:
> Chandra Seetharaman wrote:
> > Hi Hannes,
> > 
> > Now I see why you want this change in dm-multipath. I think I agree with
> > these changes.
> > 
> Ah. good.
> 
> > But, it brings another question: what does dh_state provide ? Help to
> > user to see which hardware handler a device is attached to ?
> > 
> And allowing to attach to a different hardware handler.
> Not everyone is running multipathing, but might be interested in the
> having the device handler nevertheless.
> 
> > I thought more about the scsi_dh_detach function (in the context of my
> > earlier comment), adding it would require more housekeeping to associate
> > one-to-one mapping between attach and detach. We can leave it the same
> > way as the module will be detached when the device disappear eventually.
> > 
> Why? You can detach with dh_state, too; just do an
> 
> echo detach > /sys/block/sdX/device/dh_state
> 
> and the hardware handler will detach.
> So no additional attribute is required.

Actually, if you're going down this route, it makes more sense to have
the device handler be a driver ... remember you were the one promising
multiple driver binding at the FS/Storage summit ...  that way we can
use all the generic driver standard interfaces for manual
binding/unbinding.  Plus we can place the attributes as driver attribute
groups.

James

Re: [PATCH 7/7] scsi_dh: attach to hardware handler from dm-mpath

[Chandra Seetharaman]

Looks good to me.

On Tue, 2008-05-20 at 16:05 +0200, Hannes Reinecke wrote:
> multipath keeps a separate device table which may be
> more current than the built-in one.
> So we should make sure to always call ->attach whenever
> a multipath map with hardware handler is instantiated.
> And we should call ->detach on removal, too.
> 
> Signed-off-by: Hannes Reinecke <hare@suse.de>
> ---
>  drivers/md/dm-mpath.c                 |   13 ++++++
>  drivers/scsi/device_handler/scsi_dh.c |   74 +++++++++++++++++++++++++++++++++
>  include/scsi/scsi_dh.h                |    2 +
>  3 files changed, 89 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/md/dm-mpath.c b/drivers/md/dm-mpath.c
> index e8f704a..bd2bcf4 100644
> --- a/drivers/md/dm-mpath.c
> +++ b/drivers/md/dm-mpath.c
> @@ -147,9 +147,12 @@ static struct priority_group *alloc_priority_group(void)
>  static void free_pgpaths(struct list_head *pgpaths, struct dm_target *ti)
>  {
>  	struct pgpath *pgpath, *tmp;
> +	struct multipath *m = (struct multipath *) ti->private;
> 
>  	list_for_each_entry_safe(pgpath, tmp, pgpaths, list) {
>  		list_del(&pgpath->list);
> +		if (m->hw_handler_name)
> +			scsi_dh_detach(bdev_get_queue(pgpath->path.dev->bdev));
>  		dm_put_device(ti, pgpath->path.dev);
>  		free_pgpath(pgpath);
>  	}
> @@ -546,6 +549,7 @@ static struct pgpath *parse_path(struct arg_set *as, struct path_selector *ps,
>  {
>  	int r;
>  	struct pgpath *p;
> +	struct multipath *m = (struct multipath *) ti->private;
> 
>  	/* we need at least a path arg */
>  	if (as->argc < 1) {
> @@ -564,6 +568,15 @@ static struct pgpath *parse_path(struct arg_set *as, struct path_selector *ps,
>  		goto bad;
>  	}
> 
> +	if (m->hw_handler_name) {
> +		r = scsi_dh_attach(bdev_get_queue(p->path.dev->bdev),
> +				   m->hw_handler_name);
> +		if (r < 0) {
> +			dm_put_device(ti, p->path.dev);
> +			goto bad;
> +		}
> +	}
> +
>  	r = ps->type->add_path(ps, &p->path, as->argc, as->argv, &ti->error);
>  	if (r) {
>  		dm_put_device(ti, p->path.dev);
> diff --git a/drivers/scsi/device_handler/scsi_dh.c b/drivers/scsi/device_handler/scsi_dh.c
> index b80fae7..a9404b5 100644
> --- a/drivers/scsi/device_handler/scsi_dh.c
> +++ b/drivers/scsi/device_handler/scsi_dh.c
> @@ -362,6 +362,80 @@ int scsi_dh_handler_exist(const char *name)
>  }
>  EXPORT_SYMBOL_GPL(scsi_dh_handler_exist);
> 
> +/*
> + * scsi_dh_handler_attach - Attach device handler
> + * @sdev - sdev the handler should be attached to
> + * @name - name of the handler to attach
> + */
> +int scsi_dh_attach(struct request_queue *q, const char *name)
> +{
> +	unsigned long flags;
> +	struct scsi_device *sdev;
> +	struct scsi_device_handler *scsi_dh;
> +	int err = 0;
> +
> +	scsi_dh = get_device_handler(name);
> +	if (!scsi_dh)
> +		return -EINVAL;
> +
> +	spin_lock_irqsave(q->queue_lock, flags);
> +	sdev = q->queuedata;
> +	if (!sdev || !get_device(&sdev->sdev_gendev))
> +		err = -ENODEV;
> +	spin_unlock_irqrestore(q->queue_lock, flags);
> +
> +	if (!err) {
> +		err = scsi_dh_handler_attach(sdev, scsi_dh);
> +
> +		put_device(&sdev->sdev_gendev);
> +	}
> +	return err;
> +}
> +EXPORT_SYMBOL_GPL(scsi_dh_attach);
> +
> +/*
> + * scsi_dh_handler_detach - Detach device handler
> + * @sdev - sdev the handler should be detached from
> + *
> + * This function will detach the device handler only
> + * if the sdev is not part of the internal list, ie
> + * if it has been attached manually.
> + */
> +void scsi_dh_detach(struct request_queue *q)
> +{
> +	unsigned long flags;
> +	struct scsi_device *sdev;
> +	struct scsi_device_handler *scsi_dh = NULL;
> +	int i, found = 0;
> +
> +	spin_lock_irqsave(q->queue_lock, flags);
> +	sdev = q->queuedata;
> +	if (!sdev || !get_device(&sdev->sdev_gendev))
> +		sdev = NULL;
> +	spin_unlock_irqrestore(q->queue_lock, flags);
> +
> +	if (!sdev)
> +		return;
> +
> +	if (sdev->scsi_dh_data) {
> +		scsi_dh = sdev->scsi_dh_data->scsi_dh;
> +		for(i = 0; scsi_dh->devlist[i].vendor; i++) {
> +			if (!strncmp(sdev->vendor, scsi_dh->devlist[i].vendor,
> +				     strlen(scsi_dh->devlist[i].vendor)) &&
> +			    !strncmp(sdev->model, scsi_dh->devlist[i].model,
> +				     strlen(scsi_dh->devlist[i].model))) {
> +				found = 1;
> +				break;
> +			}
> +		}
> +		/* sdev not on internal list, detach */
> +		if (!found)
> +			scsi_dh_handler_detach(sdev);
> +	}
> +	put_device(&sdev->sdev_gendev);
> +}
> +EXPORT_SYMBOL_GPL(scsi_dh_detach);
> +
>  static struct notifier_block scsi_dh_nb = {
>  	.notifier_call = scsi_dh_notifier
>  };
> diff --git a/include/scsi/scsi_dh.h b/include/scsi/scsi_dh.h
> index 5d05ac0..8032e51 100644
> --- a/include/scsi/scsi_dh.h
> +++ b/include/scsi/scsi_dh.h
> @@ -59,3 +59,5 @@ enum {
> 
>  extern int scsi_dh_activate(struct request_queue *);
>  extern int scsi_dh_handler_exist(const char *);
> +extern int scsi_dh_attach(struct request_queue *, const char *);
> +extern void scsi_dh_detach(struct request_queue *q);

Re: [PATCH 7/7] scsi_dh: attach to hardware handler from dm-mpath

[Hannes Reinecke]

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

On Thu, May 22, 2008 at 09:14:54AM +0100, James Bottomley wrote:
> On Wed, 2008-05-21 at 08:18 +0200, Hannes Reinecke wrote:
[ .. ]
> > Why? You can detach with dh_state, too; just do an
> > 
> > echo detach > /sys/block/sdX/device/dh_state
> > 
> > and the hardware handler will detach.
> > So no additional attribute is required.
> 
> Actually, if you're going down this route, it makes more sense to have
> the device handler be a driver ... remember you were the one promising
> multiple driver binding at the FS/Storage summit ...  that way we can
> use all the generic driver standard interfaces for manual
> binding/unbinding.  Plus we can place the attributes as driver attribute
> groups.
> 
Hmm. Actually this very discussion I had with Kay Sievers earlier this
week when we tried to make bsg useable for udev.
The problem here is that we really want the device_handler to kick in
_before_ any of the 'normal' SCSI ULD starts it's probing, as we might
want to suppress I/O on this channel.

If we were to use the normal driver binding for this we'd be called
at the same level with the normal ULDs, making it unpredictable at which
time we're called.

And it was actually Greg KH which promised the multiple driver binding,
but seems that he's not getting around doing this anytime soon.
Hmm; maybe I can convince Kay on doing it ...
But want we really want to do is _not_ having these 'special' handlers
called at any time during bus probing, but having a fixed order like:

sdev probing
 - (optional) device handler attach
 - bsg attach
 - SCSI ULD probing

We would like to have bsg available before ULD probing, as then we could
use the bsg device nodes in general in udev (like calling scsi_id on it).
It _might_ be possible to handle these cases with multiple driver binding,
but seeing that there's no-one currently working on it I'd like to stick
with the current approach.

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 7/7] scsi_dh: attach to hardware handler from dm-mpath

[James Bottomley]

On Fri, 2008-05-23 at 13:40 +0200, Hannes Reinecke wrote:
> On Thu, May 22, 2008 at 09:14:54AM +0100, James Bottomley wrote:
> > On Wed, 2008-05-21 at 08:18 +0200, Hannes Reinecke wrote:
> [ .. ]
> > > Why? You can detach with dh_state, too; just do an
> > > 
> > > echo detach > /sys/block/sdX/device/dh_state
> > > 
> > > and the hardware handler will detach.
> > > So no additional attribute is required.
> > 
> > Actually, if you're going down this route, it makes more sense to have
> > the device handler be a driver ... remember you were the one promising
> > multiple driver binding at the FS/Storage summit ...  that way we can
> > use all the generic driver standard interfaces for manual
> > binding/unbinding.  Plus we can place the attributes as driver attribute
> > groups.
> > 
> Hmm. Actually this very discussion I had with Kay Sievers earlier this
> week when we tried to make bsg useable for udev.
> The problem here is that we really want the device_handler to kick in
> _before_ any of the 'normal' SCSI ULD starts it's probing, as we might
> want to suppress I/O on this channel.

That's easily doable provided there's some type of trigger for
registration.  At the moment, it's just a binding free for all since
drivers bind either when they appear (for existing devices) or when the
device appears (for existing drivers).  For the new multiple driver
binding code, I could see a good case for either strictly controlling
the ordering (for binding a new device to existing drivers) or even
making the bindings classification driven.

> If we were to use the normal driver binding for this we'd be called
> at the same level with the normal ULDs, making it unpredictable at which
> time we're called.

Depends how it's done.  Right at the moment with only a single driver
there's no concept of ordering ... if it's list based, then it would be
in list order, but there's no reason the order can't be dictated by the
bus.

> And it was actually Greg KH which promised the multiple driver binding,
> but seems that he's not getting around doing this anytime soon.

Well, since we have the use case, the rules of open source would tend to
dictate that we have the impetus to do it.  Particularly when there's
tricky questions like binding order to be answered.

> Hmm; maybe I can convince Kay on doing it ...
> But want we really want to do is _not_ having these 'special' handlers
> called at any time during bus probing, but having a fixed order like:
> 
> sdev probing
>  - (optional) device handler attach
>  - bsg attach
>  - SCSI ULD probing
> 
> We would like to have bsg available before ULD probing, as then we could
> use the bsg device nodes in general in udev (like calling scsi_id on it).
> It _might_ be possible to handle these cases with multiple driver binding,
> but seeing that there's no-one currently working on it I'd like to stick
> with the current approach.

I think a simple priority based scheme would cope with that.  Say a
single integer in the bus for max priority and then it loops from zero
to max binding all drivers equal to that priority.  We could use 3 (zero
for device handler, one for bsg and 2 for ULD).  We should probably use
1 as the default for everything, giving the ability to bind either
before or after the default (using zero or 2).

James