| Message ID | 50D2AB1D.9050602@intel.com |
|---|---|
| State | Not Applicable |
| Delegated to: | David Miller |
| Headers | show |
Hello, Aaron. On Thu, Dec 20, 2012 at 02:07:25PM +0800, Aaron Lu wrote: > +static int is_gendisk_part0(struct device *dev, void *data) > +{ > + struct device **child = data; > + > + if (dev->class == &block_class && dev->type == &disk_type) { > + *child = dev; > + return 1; > + } else > + return 0; > +} > + > +/** > + * disk_from_device - Get the gendisk pointer for this device. > + * @dev: the device this gendisk is created for, i.e. gendisk->driverfs_dev > + * > + * LLD sometimes need to play with the gendisk without HLD's aware, > + * this routine gives LLD the required access to gendisk. > + * > + * CONTEXT: > + * Don't care. > + */ > +struct gendisk *disk_from_device(struct device *dev) > +{ > + struct device *child; > + > + if (device_for_each_child(dev, &child, is_gendisk_part0)) > + return dev_to_disk(child); > + else > + return NULL; > +} > +EXPORT_SYMBOL(disk_from_device); This is really a round-about way to find out the matching device and it wouldn't work if the disk device nests deeper. Doesn't really look like a good idea to me. > Then together with disk_try_block_events and disk_unblock_events, we can > avoid touching SCSI layer to let ODD stay in zero power state. Also, I'd much prefer something along the line of block_events_nowait() instead of try_block. Thanks.
On 12/26/2012 01:17 AM, Tejun Heo wrote: > Hello, Aaron. > > On Thu, Dec 20, 2012 at 02:07:25PM +0800, Aaron Lu wrote: >> +static int is_gendisk_part0(struct device *dev, void *data) >> +{ >> + struct device **child = data; >> + >> + if (dev->class == &block_class && dev->type == &disk_type) { >> + *child = dev; >> + return 1; >> + } else >> + return 0; >> +} >> + >> +/** >> + * disk_from_device - Get the gendisk pointer for this device. >> + * @dev: the device this gendisk is created for, i.e. gendisk->driverfs_dev >> + * >> + * LLD sometimes need to play with the gendisk without HLD's aware, >> + * this routine gives LLD the required access to gendisk. >> + * >> + * CONTEXT: >> + * Don't care. >> + */ >> +struct gendisk *disk_from_device(struct device *dev) >> +{ >> + struct device *child; >> + >> + if (device_for_each_child(dev, &child, is_gendisk_part0)) >> + return dev_to_disk(child); >> + else >> + return NULL; >> +} >> +EXPORT_SYMBOL(disk_from_device); > > This is really a round-about way to find out the matching device and > it wouldn't work if the disk device nests deeper. Doesn't really look > like a good idea to me. I don't quite understand the 'disk device nests deeper' case, can you please elaborate? My understanding is, as long as the disk's part0 device has a parent, this function should work. For LLDs want to take advantage of this function, it should pass the device that is the parent of part0 as the param, this function itself doesn't try to dig further. The only problem I can see is when there are multiple gendisks created for a single device, which I don't know if there is such a case? > >> Then together with disk_try_block_events and disk_unblock_events, we can >> avoid touching SCSI layer to let ODD stay in zero power state. > > Also, I'd much prefer something along the line of > block_events_nowait() instead of try_block. Sure, no problem. Thanks, Aaron -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello, On Wed, Dec 26, 2012 at 09:42:14AM +0800, Aaron Lu wrote: > > This is really a round-about way to find out the matching device and > > it wouldn't work if the disk device nests deeper. Doesn't really look > > like a good idea to me. > > I don't quite understand the 'disk device nests deeper' case, can you > please elaborate? My understanding is, as long as the disk's part0 > device has a parent, this function should work. For LLDs want to take Hmmm, maybe I misread but it looked like it wouldn't work if there are intermediate nodes between the parent device and part0. It might not happen for sata but I don't think it's a good idea to assume that the part0 and hardware device are connected directly. In general, it's a quite roundabout way to do it. Let's just push it through SCSI. That's how everything else is routed after all. It's confusing to do this one differently. Thanks.
On 12/29/2012 05:16 AM, Tejun Heo wrote: > Hello, > > On Wed, Dec 26, 2012 at 09:42:14AM +0800, Aaron Lu wrote: >>> This is really a round-about way to find out the matching device and >>> it wouldn't work if the disk device nests deeper. Doesn't really look >>> like a good idea to me. >> >> I don't quite understand the 'disk device nests deeper' case, can you >> please elaborate? My understanding is, as long as the disk's part0 >> device has a parent, this function should work. For LLDs want to take > > Hmmm, maybe I misread but it looked like it wouldn't work if there are > intermediate nodes between the parent device and part0. It might not > happen for sata but I don't think it's a good idea to assume that the > part0 and hardware device are connected directly. > > In general, it's a quite roundabout way to do it. Let's just push it > through SCSI. That's how everything else is routed after all. It's > confusing to do this one differently. OK, thanks for the suggestion. I'll prepare v11 using the previous demonstrated way(adding the event_driven flag to scsi_device) to silence the poll. Best regards, Aaron -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/block/genhd.c b/block/genhd.c index 9a289d7..448b201 100644 --- a/block/genhd.c +++ b/block/genhd.c @@ -1619,6 +1619,38 @@ static void disk_events_workfn(struct work_struct *work) kobject_uevent_env(&disk_to_dev(disk)->kobj, KOBJ_CHANGE, envp); } +static int is_gendisk_part0(struct device *dev, void *data) +{ + struct device **child = data; + + if (dev->class == &block_class && dev->type == &disk_type) { + *child = dev; + return 1; + } else + return 0; +} + +/** + * disk_from_device - Get the gendisk pointer for this device. + * @dev: the device this gendisk is created for, i.e. gendisk->driverfs_dev + * + * LLD sometimes need to play with the gendisk without HLD's aware, + * this routine gives LLD the required access to gendisk. + * + * CONTEXT: + * Don't care. + */ +struct gendisk *disk_from_device(struct device *dev) +{ + struct device *child; + + if (device_for_each_child(dev, &child, is_gendisk_part0)) + return dev_to_disk(child); + else + return NULL; +} +EXPORT_SYMBOL(disk_from_device); + /* * A disk events enabled device has the following sysfs nodes under * its /sys/block/X/ directory. diff --git a/include/linux/genhd.h b/include/linux/genhd.h index 79b8bba..e8002c0 100644 --- a/include/linux/genhd.h +++ b/include/linux/genhd.h @@ -427,6 +427,7 @@ extern void disk_block_events(struct gendisk *disk); extern void disk_unblock_events(struct gendisk *disk); extern void disk_flush_events(struct gendisk *disk, unsigned int mask); extern unsigned int disk_clear_events(struct gendisk *disk, unsigned int mask); +extern struct gendisk *disk_from_device(struct device *dev); /* drivers/char/random.c */ extern void add_disk_randomness(struct gendisk *disk);