* [PATCH] scsi: ensure bsg device is available before announcing scsi device
@ 2011-03-02 19:02 David Zeuthen
2011-03-03 17:56 ` James Bottomley
0 siblings, 1 reply; 6+ messages in thread
From: David Zeuthen @ 2011-03-02 19:02 UTC (permalink / raw)
To: James.Bottomley; +Cc: linux-scsi, kay.sievers
From: David Zeuthen <davidz@redhat.com>
udev should be able to examine a scsi device (e.g. retrieving VPD like
the make/model and serial number) before announcing it to the rest of
user space. Currently this is not possible because the scsi device has
no device node of its own. Instead, user space will have to use either
the bsg device or sg device but these are not available at uevent add
time since both are children of the scsi device
This patch ensures that the bsg device is created before user space is
notified of add event for the scsi device. With devtmpfs, this thus
enables udev to use the /dev/bsg/$kernel device node to examine the
scsi device.
Related udev commits:
http://git.kernel.org/?p=linux/hotplug/udev.git;a=commitdiff;h=2938220037862b7698df091a1e5cd45f44132d73
http://git.kernel.org/?p=linux/hotplug/udev.git;a=commitdiff;h=310f99d33595afdc1962611a913c429785f9ad40
Signed-off-by: David Zeuthen <davidz@redhat.com>
Acked-by: Kay Sievers <kay.sievers@vrfy.org>
---
block/bsg.c | 53 +++++++++++++++++++++++++++++++++++-
drivers/scsi/scsi_sysfs.c | 27 +++++++++++++-----
drivers/scsi/scsi_transport_fc.c | 4 +-
drivers/scsi/scsi_transport_sas.c | 2 +-
include/linux/bsg.h | 6 +++-
5 files changed, 77 insertions(+), 15 deletions(-)
diff --git a/block/bsg.c b/block/bsg.c
index 0c8b64a..7148b4c 100644
--- a/block/bsg.c
+++ b/block/bsg.c
@@ -993,8 +993,48 @@ void bsg_unregister_queue(struct request_queue *q)
}
EXPORT_SYMBOL_GPL(bsg_unregister_queue);
+static void bsg_release_class_device(struct device *dev)
+{
+ pr_debug("device: '%s': %s\n", dev_name(dev), __func__);
+ kfree(dev);
+}
+
+static struct device *bsg_create_class_device(struct class *class,
+ struct device *parent,
+ dev_t devt,
+ const char *name)
+{
+ struct device *dev = NULL;
+ int retval = -ENODEV;
+
+ if (class == NULL || IS_ERR(class))
+ goto error;
+
+ dev = kzalloc(sizeof(*dev), GFP_KERNEL);
+ if (!dev) {
+ retval = -ENOMEM;
+ goto error;
+ }
+
+ dev->devt = devt;
+ dev->class = class;
+ dev->parent = parent;
+ dev->release = bsg_release_class_device;
+
+ retval = kobject_set_name(&dev->kobj, name);
+ if (retval)
+ goto error;
+
+ return dev;
+
+error:
+ put_device(dev);
+ return ERR_PTR(retval);
+}
+
int bsg_register_queue(struct request_queue *q, struct device *parent,
- const char *name, void (*release)(struct device *))
+ const char *name, void (*release)(struct device *),
+ bool suppress_add_uevent)
{
struct bsg_class_device *bcd;
dev_t dev;
@@ -1040,11 +1080,20 @@ int bsg_register_queue(struct request_queue *q, struct device *parent,
bcd->release = release;
kref_init(&bcd->ref);
dev = MKDEV(bsg_major, bcd->minor);
- class_dev = device_create(bsg_class, parent, dev, NULL, "%s", devname);
+ class_dev = bsg_create_class_device(bsg_class, parent, dev, devname);
if (IS_ERR(class_dev)) {
ret = PTR_ERR(class_dev);
goto put_dev;
}
+ if (suppress_add_uevent) {
+ dev_set_uevent_suppress(class_dev, true);
+ ret = device_register(class_dev);
+ dev_set_uevent_suppress(class_dev, false);
+ } else {
+ ret = device_register(class_dev);
+ }
+ if (ret)
+ goto put_dev;
bcd->class_dev = class_dev;
if (q->kobj.sd) {
diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
index 490ce21..fd8c43f 100644
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -862,12 +862,31 @@ int scsi_sysfs_add_sdev(struct scsi_device *sdev)
*/
scsi_autopm_get_device(sdev);
+ /* Delay add uevent for device until bsg device has been
+ * created - this way user space can use the bsg device node
+ * when handling the uevent
+ */
+ dev_set_uevent_suppress(&sdev->sdev_gendev, true);
error = device_add(&sdev->sdev_gendev);
+ dev_set_uevent_suppress(&sdev->sdev_gendev, false);
if (error) {
sdev_printk(KERN_INFO, sdev,
"failed to add device: %d\n", error);
return error;
}
+
+ error = bsg_register_queue(rq, &sdev->sdev_gendev, NULL, NULL, true);
+ if (error) {
+ /* we're treating error on bsg register as non-fatal,
+ * so pretend nothing went wrong */
+ sdev_printk(KERN_INFO, sdev,
+ "Failed to register bsg queue, errno=%d\n", error);
+ kobject_uevent(&sdev->sdev_gendev.kobj, KOBJ_ADD);
+ } else {
+ kobject_uevent(&sdev->sdev_gendev.kobj, KOBJ_ADD);
+ kobject_uevent(&rq->bsg_dev.class_dev->kobj, KOBJ_ADD);
+ }
+
device_enable_async_suspend(&sdev->sdev_dev);
error = device_add(&sdev->sdev_dev);
if (error) {
@@ -898,14 +917,6 @@ int scsi_sysfs_add_sdev(struct scsi_device *sdev)
if (error)
return error;
- error = bsg_register_queue(rq, &sdev->sdev_gendev, NULL, NULL);
-
- if (error)
- /* we're treating error on bsg register as non-fatal,
- * so pretend nothing went wrong */
- sdev_printk(KERN_INFO, sdev,
- "Failed to register bsg queue, errno=%d\n", error);
-
/* add additional host specific attributes */
if (sdev->host->hostt->sdev_attrs) {
for (i = 0; sdev->host->hostt->sdev_attrs[i]; i++) {
diff --git a/drivers/scsi/scsi_transport_fc.c b/drivers/scsi/scsi_transport_fc.c
index 998c01b..6066b9a7 100644
--- a/drivers/scsi/scsi_transport_fc.c
+++ b/drivers/scsi/scsi_transport_fc.c
@@ -4037,7 +4037,7 @@ fc_bsg_hostadd(struct Scsi_Host *shost, struct fc_host_attrs *fc_host)
blk_queue_rq_timed_out(q, fc_bsg_job_timeout);
blk_queue_rq_timeout(q, FC_DEFAULT_BSG_TIMEOUT);
- err = bsg_register_queue(q, dev, bsg_name, NULL);
+ err = bsg_register_queue(q, dev, bsg_name, NULL, false);
if (err) {
printk(KERN_ERR "fc_host%d: bsg interface failed to "
"initialize - register queue\n",
@@ -4083,7 +4083,7 @@ fc_bsg_rportadd(struct Scsi_Host *shost, struct fc_rport *rport)
blk_queue_rq_timed_out(q, fc_bsg_job_timeout);
blk_queue_rq_timeout(q, BLK_DEFAULT_SG_TIMEOUT);
- err = bsg_register_queue(q, dev, NULL, NULL);
+ err = bsg_register_queue(q, dev, NULL, NULL, false);
if (err) {
printk(KERN_ERR "%s: bsg interface failed to "
"initialize - register queue\n",
diff --git a/drivers/scsi/scsi_transport_sas.c b/drivers/scsi/scsi_transport_sas.c
index 927e99c..0da85fe 100644
--- a/drivers/scsi/scsi_transport_sas.c
+++ b/drivers/scsi/scsi_transport_sas.c
@@ -241,7 +241,7 @@ static int sas_bsg_initialize(struct Scsi_Host *shost, struct sas_rphy *rphy)
if (!q)
return -ENOMEM;
- error = bsg_register_queue(q, dev, name, release);
+ error = bsg_register_queue(q, dev, name, release, false);
if (error) {
blk_cleanup_queue(q);
return -ENOMEM;
diff --git a/include/linux/bsg.h b/include/linux/bsg.h
index ecb4730..999f3e9 100644
--- a/include/linux/bsg.h
+++ b/include/linux/bsg.h
@@ -75,12 +75,14 @@ struct bsg_class_device {
extern int bsg_register_queue(struct request_queue *q,
struct device *parent, const char *name,
- void (*release)(struct device *));
+ void (*release)(struct device *),
+ bool suppress_add_uevent);
extern void bsg_unregister_queue(struct request_queue *);
#else
static inline int bsg_register_queue(struct request_queue *q,
struct device *parent, const char *name,
- void (*release)(struct device *))
+ void (*release)(struct device *),
+ bool suppress_add_uevent)
{
return 0;
}
--
1.7.4.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] scsi: ensure bsg device is available before announcing scsi device
2011-03-02 19:02 [PATCH] scsi: ensure bsg device is available before announcing scsi device David Zeuthen
@ 2011-03-03 17:56 ` James Bottomley
2011-03-04 14:32 ` David Zeuthen
0 siblings, 1 reply; 6+ messages in thread
From: James Bottomley @ 2011-03-03 17:56 UTC (permalink / raw)
To: David Zeuthen; +Cc: linux-scsi, kay.sievers
On Wed, 2011-03-02 at 14:02 -0500, David Zeuthen wrote:
> From: David Zeuthen <davidz@redhat.com>
>
> udev should be able to examine a scsi device (e.g. retrieving VPD like
> the make/model and serial number) before announcing it to the rest of
> user space. Currently this is not possible because the scsi device has
> no device node of its own. Instead, user space will have to use either
> the bsg device or sg device but these are not available at uevent add
> time since both are children of the scsi device
This premise sounds a bit wrong. SCSI devices go through several
distinct phases: create, add and then configure. We send the initial
probe before the add phase, but no user should really poke at the device
before the configure phase because that's where we set the
characteristics and other stuff. The signal to udev that the configure
phase is over is when the ULD binding completes. That's when the device
is running and it's safe to poke at. It also means it's been identified
internally and you'll know whether it's a disk, a tape or whatever.
So you should see two distinct events: an ADD for the sdev, meaning we
think there's something there, but at this point there's no real device
for a user to see followed by the add for whatever the ULD major device
binding is ... at that point, we guarantee the device is up, its bsg
queue is registered, there's actually a user block or character device
corresponding to it and we've finished configuring it.
What exactly are you trying to do here? It's really not that safe to be
running user level poking in parallel with the configuration scanning
we'll be doing as the ULD binds.
James
> This patch ensures that the bsg device is created before user space is
> notified of add event for the scsi device. With devtmpfs, this thus
> enables udev to use the /dev/bsg/$kernel device node to examine the
> scsi device.
>
> Related udev commits:
>
> http://git.kernel.org/?p=linux/hotplug/udev.git;a=commitdiff;h=2938220037862b7698df091a1e5cd45f44132d73
> http://git.kernel.org/?p=linux/hotplug/udev.git;a=commitdiff;h=310f99d33595afdc1962611a913c429785f9ad40
>
> Signed-off-by: David Zeuthen <davidz@redhat.com>
> Acked-by: Kay Sievers <kay.sievers@vrfy.org>
> ---
> block/bsg.c | 53 +++++++++++++++++++++++++++++++++++-
> drivers/scsi/scsi_sysfs.c | 27 +++++++++++++-----
> drivers/scsi/scsi_transport_fc.c | 4 +-
> drivers/scsi/scsi_transport_sas.c | 2 +-
> include/linux/bsg.h | 6 +++-
> 5 files changed, 77 insertions(+), 15 deletions(-)
>
> diff --git a/block/bsg.c b/block/bsg.c
> index 0c8b64a..7148b4c 100644
> --- a/block/bsg.c
> +++ b/block/bsg.c
> @@ -993,8 +993,48 @@ void bsg_unregister_queue(struct request_queue *q)
> }
> EXPORT_SYMBOL_GPL(bsg_unregister_queue);
>
> +static void bsg_release_class_device(struct device *dev)
> +{
> + pr_debug("device: '%s': %s\n", dev_name(dev), __func__);
> + kfree(dev);
> +}
> +
> +static struct device *bsg_create_class_device(struct class *class,
> + struct device *parent,
> + dev_t devt,
> + const char *name)
> +{
> + struct device *dev = NULL;
> + int retval = -ENODEV;
> +
> + if (class == NULL || IS_ERR(class))
> + goto error;
> +
> + dev = kzalloc(sizeof(*dev), GFP_KERNEL);
> + if (!dev) {
> + retval = -ENOMEM;
> + goto error;
> + }
> +
> + dev->devt = devt;
> + dev->class = class;
> + dev->parent = parent;
> + dev->release = bsg_release_class_device;
> +
> + retval = kobject_set_name(&dev->kobj, name);
> + if (retval)
> + goto error;
> +
> + return dev;
> +
> +error:
> + put_device(dev);
> + return ERR_PTR(retval);
> +}
> +
> int bsg_register_queue(struct request_queue *q, struct device *parent,
> - const char *name, void (*release)(struct device *))
> + const char *name, void (*release)(struct device *),
> + bool suppress_add_uevent)
> {
> struct bsg_class_device *bcd;
> dev_t dev;
> @@ -1040,11 +1080,20 @@ int bsg_register_queue(struct request_queue *q, struct device *parent,
> bcd->release = release;
> kref_init(&bcd->ref);
> dev = MKDEV(bsg_major, bcd->minor);
> - class_dev = device_create(bsg_class, parent, dev, NULL, "%s", devname);
> + class_dev = bsg_create_class_device(bsg_class, parent, dev, devname);
> if (IS_ERR(class_dev)) {
> ret = PTR_ERR(class_dev);
> goto put_dev;
> }
> + if (suppress_add_uevent) {
> + dev_set_uevent_suppress(class_dev, true);
> + ret = device_register(class_dev);
> + dev_set_uevent_suppress(class_dev, false);
> + } else {
> + ret = device_register(class_dev);
> + }
> + if (ret)
> + goto put_dev;
> bcd->class_dev = class_dev;
>
> if (q->kobj.sd) {
> diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
> index 490ce21..fd8c43f 100644
> --- a/drivers/scsi/scsi_sysfs.c
> +++ b/drivers/scsi/scsi_sysfs.c
> @@ -862,12 +862,31 @@ int scsi_sysfs_add_sdev(struct scsi_device *sdev)
> */
> scsi_autopm_get_device(sdev);
>
> + /* Delay add uevent for device until bsg device has been
> + * created - this way user space can use the bsg device node
> + * when handling the uevent
> + */
> + dev_set_uevent_suppress(&sdev->sdev_gendev, true);
> error = device_add(&sdev->sdev_gendev);
> + dev_set_uevent_suppress(&sdev->sdev_gendev, false);
> if (error) {
> sdev_printk(KERN_INFO, sdev,
> "failed to add device: %d\n", error);
> return error;
> }
> +
> + error = bsg_register_queue(rq, &sdev->sdev_gendev, NULL, NULL, true);
> + if (error) {
> + /* we're treating error on bsg register as non-fatal,
> + * so pretend nothing went wrong */
> + sdev_printk(KERN_INFO, sdev,
> + "Failed to register bsg queue, errno=%d\n", error);
> + kobject_uevent(&sdev->sdev_gendev.kobj, KOBJ_ADD);
> + } else {
> + kobject_uevent(&sdev->sdev_gendev.kobj, KOBJ_ADD);
> + kobject_uevent(&rq->bsg_dev.class_dev->kobj, KOBJ_ADD);
> + }
> +
> device_enable_async_suspend(&sdev->sdev_dev);
> error = device_add(&sdev->sdev_dev);
> if (error) {
> @@ -898,14 +917,6 @@ int scsi_sysfs_add_sdev(struct scsi_device *sdev)
> if (error)
> return error;
>
> - error = bsg_register_queue(rq, &sdev->sdev_gendev, NULL, NULL);
> -
> - if (error)
> - /* we're treating error on bsg register as non-fatal,
> - * so pretend nothing went wrong */
> - sdev_printk(KERN_INFO, sdev,
> - "Failed to register bsg queue, errno=%d\n", error);
> -
> /* add additional host specific attributes */
> if (sdev->host->hostt->sdev_attrs) {
> for (i = 0; sdev->host->hostt->sdev_attrs[i]; i++) {
> diff --git a/drivers/scsi/scsi_transport_fc.c b/drivers/scsi/scsi_transport_fc.c
> index 998c01b..6066b9a7 100644
> --- a/drivers/scsi/scsi_transport_fc.c
> +++ b/drivers/scsi/scsi_transport_fc.c
> @@ -4037,7 +4037,7 @@ fc_bsg_hostadd(struct Scsi_Host *shost, struct fc_host_attrs *fc_host)
> blk_queue_rq_timed_out(q, fc_bsg_job_timeout);
> blk_queue_rq_timeout(q, FC_DEFAULT_BSG_TIMEOUT);
>
> - err = bsg_register_queue(q, dev, bsg_name, NULL);
> + err = bsg_register_queue(q, dev, bsg_name, NULL, false);
> if (err) {
> printk(KERN_ERR "fc_host%d: bsg interface failed to "
> "initialize - register queue\n",
> @@ -4083,7 +4083,7 @@ fc_bsg_rportadd(struct Scsi_Host *shost, struct fc_rport *rport)
> blk_queue_rq_timed_out(q, fc_bsg_job_timeout);
> blk_queue_rq_timeout(q, BLK_DEFAULT_SG_TIMEOUT);
>
> - err = bsg_register_queue(q, dev, NULL, NULL);
> + err = bsg_register_queue(q, dev, NULL, NULL, false);
> if (err) {
> printk(KERN_ERR "%s: bsg interface failed to "
> "initialize - register queue\n",
> diff --git a/drivers/scsi/scsi_transport_sas.c b/drivers/scsi/scsi_transport_sas.c
> index 927e99c..0da85fe 100644
> --- a/drivers/scsi/scsi_transport_sas.c
> +++ b/drivers/scsi/scsi_transport_sas.c
> @@ -241,7 +241,7 @@ static int sas_bsg_initialize(struct Scsi_Host *shost, struct sas_rphy *rphy)
> if (!q)
> return -ENOMEM;
>
> - error = bsg_register_queue(q, dev, name, release);
> + error = bsg_register_queue(q, dev, name, release, false);
> if (error) {
> blk_cleanup_queue(q);
> return -ENOMEM;
> diff --git a/include/linux/bsg.h b/include/linux/bsg.h
> index ecb4730..999f3e9 100644
> --- a/include/linux/bsg.h
> +++ b/include/linux/bsg.h
> @@ -75,12 +75,14 @@ struct bsg_class_device {
>
> extern int bsg_register_queue(struct request_queue *q,
> struct device *parent, const char *name,
> - void (*release)(struct device *));
> + void (*release)(struct device *),
> + bool suppress_add_uevent);
> extern void bsg_unregister_queue(struct request_queue *);
> #else
> static inline int bsg_register_queue(struct request_queue *q,
> struct device *parent, const char *name,
> - void (*release)(struct device *))
> + void (*release)(struct device *),
> + bool suppress_add_uevent)
> {
> return 0;
> }
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] scsi: ensure bsg device is available before announcing scsi device
2011-03-03 17:56 ` James Bottomley
@ 2011-03-04 14:32 ` David Zeuthen
2011-03-04 15:41 ` James Bottomley
0 siblings, 1 reply; 6+ messages in thread
From: David Zeuthen @ 2011-03-04 14:32 UTC (permalink / raw)
To: James Bottomley; +Cc: linux-scsi, kay.sievers
Hey,
On Thu, 2011-03-03 at 11:56 -0600, James Bottomley wrote:
> On Wed, 2011-03-02 at 14:02 -0500, David Zeuthen wrote:
> > From: David Zeuthen <davidz@redhat.com>
> >
> > udev should be able to examine a scsi device (e.g. retrieving VPD like
> > the make/model and serial number) before announcing it to the rest of
> > user space. Currently this is not possible because the scsi device has
> > no device node of its own. Instead, user space will have to use either
> > the bsg device or sg device but these are not available at uevent add
> > time since both are children of the scsi device
>
> This premise sounds a bit wrong. SCSI devices go through several
> distinct phases: create, add and then configure. We send the initial
> probe before the add phase, but no user should really poke at the device
> before the configure phase because that's where we set the
> characteristics and other stuff. The signal to udev that the configure
> phase is over is when the ULD binding completes. That's when the device
> is running and it's safe to poke at. It also means it's been identified
> internally and you'll know whether it's a disk, a tape or whatever.
>
> So you should see two distinct events: an ADD for the sdev, meaning we
> think there's something there, but at this point there's no real device
> for a user to see followed by the add for whatever the ULD major device
> binding is ... at that point, we guarantee the device is up, its bsg
> queue is registered, there's actually a user block or character device
> corresponding to it and we've finished configuring it.
>
> What exactly are you trying to do here? It's really not that safe to be
> running user level poking in parallel with the configuration scanning
> we'll be doing as the ULD binds.
So there are basically two things I'd like to achieve here
1. Have bsg-symlinks for SCSI devices like this
/dev/bsg/by-id/scsi-0x05-SEAGATE-ST3300657SS-3SJ1S7K600009051M0CE
/dev/bsg/by-id/scsi-0x0d-PROMISE-2U-SAS-12-D2-BP-35000155350e0133e
(e.g. scsi-<type>-<vendor>-<model>-<t83-serial>)
2. Some RAID HBAs / drivers (I've seen this with a LSI1068 using the
mptsas driver) expose the disks that are part of a HW RAID as
scsi device but there are no block devices for them. This allows
user space to send SMART commands (using the bsg node) and nag the
user if a disk is failing.
Note that we can already do all this today but since the sysfs layout
looks like this
0:0:0:0
|-- block
│ +-- sdb
| | ...
|
|-- bsg
| +-- 0:0:0:0
| | ...
This setup is rather unfortunate because the bsg device and block device
are siblings and, thus, cannot share information with each other. This
is because only children can import information from their parents -
anything else is prone to races.
Now, since we currently run scsi_id + friends on the block device it
would mean running these tools _again_ on the bsg device. Which,
honestly is a waste. So the idea was simply to always run it on the scsi
device (using the bsg node) and simply just import the data to the block
device and bsg devices (both are children of the scsi device).
David
--
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] scsi: ensure bsg device is available before announcing scsi device
2011-03-04 14:32 ` David Zeuthen
@ 2011-03-04 15:41 ` James Bottomley
2011-03-04 16:50 ` David Zeuthen
0 siblings, 1 reply; 6+ messages in thread
From: James Bottomley @ 2011-03-04 15:41 UTC (permalink / raw)
To: David Zeuthen; +Cc: linux-scsi, kay.sievers
On Fri, 2011-03-04 at 09:32 -0500, David Zeuthen wrote:
> Hey,
>
> On Thu, 2011-03-03 at 11:56 -0600, James Bottomley wrote:
> > On Wed, 2011-03-02 at 14:02 -0500, David Zeuthen wrote:
> > > From: David Zeuthen <davidz@redhat.com>
> > >
> > > udev should be able to examine a scsi device (e.g. retrieving VPD like
> > > the make/model and serial number) before announcing it to the rest of
> > > user space. Currently this is not possible because the scsi device has
> > > no device node of its own. Instead, user space will have to use either
> > > the bsg device or sg device but these are not available at uevent add
> > > time since both are children of the scsi device
> >
> > This premise sounds a bit wrong. SCSI devices go through several
> > distinct phases: create, add and then configure. We send the initial
> > probe before the add phase, but no user should really poke at the device
> > before the configure phase because that's where we set the
> > characteristics and other stuff. The signal to udev that the configure
> > phase is over is when the ULD binding completes. That's when the device
> > is running and it's safe to poke at. It also means it's been identified
> > internally and you'll know whether it's a disk, a tape or whatever.
> >
> > So you should see two distinct events: an ADD for the sdev, meaning we
> > think there's something there, but at this point there's no real device
> > for a user to see followed by the add for whatever the ULD major device
> > binding is ... at that point, we guarantee the device is up, its bsg
> > queue is registered, there's actually a user block or character device
> > corresponding to it and we've finished configuring it.
> >
> > What exactly are you trying to do here? It's really not that safe to be
> > running user level poking in parallel with the configuration scanning
> > we'll be doing as the ULD binds.
>
> So there are basically two things I'd like to achieve here
>
> 1. Have bsg-symlinks for SCSI devices like this
>
> /dev/bsg/by-id/scsi-0x05-SEAGATE-ST3300657SS-3SJ1S7K600009051M0CE
> /dev/bsg/by-id/scsi-0x0d-PROMISE-2U-SAS-12-D2-BP-35000155350e0133e
>
> (e.g. scsi-<type>-<vendor>-<model>-<t83-serial>)
>
> 2. Some RAID HBAs / drivers (I've seen this with a LSI1068 using the
> mptsas driver) expose the disks that are part of a HW RAID as
> scsi device but there are no block devices for them. This allows
> user space to send SMART commands (using the bsg node) and nag the
> user if a disk is failing.
This really doesn't sound like such a good idea. The smart commands
have to be ATA_16/12 encapsulated. There's already a pile of bugs
saying that this fails (including causing crashes) because various
internal firmwares don't process the encapsulation correctly. The LSI
firmware, is the majority culprit, by the way.
> Note that we can already do all this today but since the sysfs layout
> looks like this
>
> 0:0:0:0
> |-- block
> │ +-- sdb
> | | ...
> |
> |-- bsg
> | +-- 0:0:0:0
> | | ...
>
> This setup is rather unfortunate because the bsg device and block device
> are siblings and, thus, cannot share information with each other. This
> is because only children can import information from their parents -
> anything else is prone to races.
>
> Now, since we currently run scsi_id + friends on the block device it
> would mean running these tools _again_ on the bsg device. Which,
> honestly is a waste. So the idea was simply to always run it on the scsi
> device (using the bsg node) and simply just import the data to the block
> device and bsg devices (both are children of the scsi device).
OK, so why not simply an event on BSG device creation then? To make
what you want to do work, it sounds like we should be moving BSG binding
farther back in the configure sequence (to the same place where we'd be
binding an ordinary ULD). It also perhaps sounds like we don't need any
events on the sdev. The only reason we might find them useful is if we
were going to do configuration from user space (which was a proposal
once upon a time). The sdev was never designed to be any sort of thing
the user saw (well, except as an identifying label). That's why you
need to bind a driver (including bsg) to speak to it.
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] scsi: ensure bsg device is available before announcing scsi device
2011-03-04 15:41 ` James Bottomley
@ 2011-03-04 16:50 ` David Zeuthen
2011-03-04 17:10 ` James Bottomley
0 siblings, 1 reply; 6+ messages in thread
From: David Zeuthen @ 2011-03-04 16:50 UTC (permalink / raw)
To: James Bottomley; +Cc: linux-scsi, kay.sievers
On Fri, 2011-03-04 at 09:41 -0600, James Bottomley wrote:
> > Note that we can already do all this today but since the sysfs layout
> > looks like this
> >
> > 0:0:0:0
> > |-- block
> > │ +-- sdb
> > | | ...
> > |
> > |-- bsg
> > | +-- 0:0:0:0
> > | | ...
> >
> > This setup is rather unfortunate because the bsg device and block device
> > are siblings and, thus, cannot share information with each other. This
> > is because only children can import information from their parents -
> > anything else is prone to races.
> >
> > Now, since we currently run scsi_id + friends on the block device it
> > would mean running these tools _again_ on the bsg device. Which,
> > honestly is a waste. So the idea was simply to always run it on the scsi
> > device (using the bsg node) and simply just import the data to the block
> > device and bsg devices (both are children of the scsi device).
>
> OK, so why not simply an event on BSG device creation then? To make
> what you want to do work, it sounds like we should be moving BSG binding
> farther back in the configure sequence (to the same place where we'd be
> binding an ordinary ULD). It also perhaps sounds like we don't need any
> events on the sdev. The only reason we might find them useful is if we
> were going to do configuration from user space (which was a proposal
> once upon a time). The sdev was never designed to be any sort of thing
> the user saw (well, except as an identifying label). That's why you
> need to bind a driver (including bsg) to speak to it.
Not entirely sure what you exactly are proposing here but note that the
root problem is that the block device and bsg device are siblings... the
only way this can work without races in user space is to have their
common ancestor do the work (e.g. run /lib/udev/scsi_id + friends) on
the 'add' uevent, not on some 'change' uevent later... there is just no
way it will work otherwise.
If we didn't care about backward compat, maybe it could look like this
0:0:0:0
+-- dev <-- this is dev file for the bsg device
|
+-- block
| +-- sdb
| | +-- dev <-- this is the dev file for the block device
so we get the bsg device as the first uevent and the block device with
the other one. With this setup we'd run /lib/udev/scsi_id + friends on
the bsg device node and then we'd import it for the block device node
and everything would be good.
That said, I don't think we can make such a change as it would probably
break some parts of user space...
David
--
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] scsi: ensure bsg device is available before announcing scsi device
2011-03-04 16:50 ` David Zeuthen
@ 2011-03-04 17:10 ` James Bottomley
0 siblings, 0 replies; 6+ messages in thread
From: James Bottomley @ 2011-03-04 17:10 UTC (permalink / raw)
To: David Zeuthen; +Cc: linux-scsi, kay.sievers
On Fri, 2011-03-04 at 11:50 -0500, David Zeuthen wrote:
> On Fri, 2011-03-04 at 09:41 -0600, James Bottomley wrote:
> > > Note that we can already do all this today but since the sysfs layout
> > > looks like this
> > >
> > > 0:0:0:0
> > > |-- block
> > > │ +-- sdb
> > > | | ...
> > > |
> > > |-- bsg
> > > | +-- 0:0:0:0
> > > | | ...
> > >
> > > This setup is rather unfortunate because the bsg device and block device
> > > are siblings and, thus, cannot share information with each other. This
> > > is because only children can import information from their parents -
> > > anything else is prone to races.
> > >
> > > Now, since we currently run scsi_id + friends on the block device it
> > > would mean running these tools _again_ on the bsg device. Which,
> > > honestly is a waste. So the idea was simply to always run it on the scsi
> > > device (using the bsg node) and simply just import the data to the block
> > > device and bsg devices (both are children of the scsi device).
> >
> > OK, so why not simply an event on BSG device creation then? To make
> > what you want to do work, it sounds like we should be moving BSG binding
> > farther back in the configure sequence (to the same place where we'd be
> > binding an ordinary ULD). It also perhaps sounds like we don't need any
> > events on the sdev. The only reason we might find them useful is if we
> > were going to do configuration from user space (which was a proposal
> > once upon a time). The sdev was never designed to be any sort of thing
> > the user saw (well, except as an identifying label). That's why you
> > need to bind a driver (including bsg) to speak to it.
>
> Not entirely sure what you exactly are proposing here but note that the
> root problem is that the block device and bsg device are siblings...
Right, but that's a consequence of the fact that we allow multiple upper
layer driver bindings.
What I'm telling you is that you can't trigger off the ADD event on the
sdev because the device isn't ready at that point. What you need to do
is trigger off the event for binding the ULD(s) ... but I'm also warning
that the multiple binding nature of ULDs always makes this racy. If you
shift to using the bsg device, we'll have to make sure it isn't
advertised until the system is ready to receive packets down it ... and
I suspect we might be creating it a bit early in the current setup,
simply because no-one has used it like this before.
You have a guarantee that there will always be a bsg device ... so if
you want to use that for a probe, fine, cache the results on it. I
don't think we can ever guarantee that the other ULD will (or won't) be
there at the same time, so surely you can make udev on the other device
wait until the bsg based probe completes?
James
> the
> only way this can work without races in user space is to have their
> common ancestor do the work (e.g. run /lib/udev/scsi_id + friends) on
> the 'add' uevent, not on some 'change' uevent later... there is just no
> way it will work otherwise.
>
> If we didn't care about backward compat, maybe it could look like this
>
> 0:0:0:0
> +-- dev <-- this is dev file for the bsg device
> |
> +-- block
> | +-- sdb
> | | +-- dev <-- this is the dev file for the block device
>
> so we get the bsg device as the first uevent and the block device with
> the other one. With this setup we'd run /lib/udev/scsi_id + friends on
> the bsg device node and then we'd import it for the block device node
> and everything would be good.
>
> That said, I don't think we can make such a change as it would probably
> break some parts of user space...
>
> David
>
>
> --
> 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
--
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2011-03-04 17:10 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-03-02 19:02 [PATCH] scsi: ensure bsg device is available before announcing scsi device David Zeuthen
2011-03-03 17:56 ` James Bottomley
2011-03-04 14:32 ` David Zeuthen
2011-03-04 15:41 ` James Bottomley
2011-03-04 16:50 ` David Zeuthen
2011-03-04 17:10 ` James Bottomley
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox