On Tue, Jan 08, 2019 at 04:57:40PM -0800, Bart Van Assche wrote:
Hi Bart,
Sorry to reply so late.
> On Mon, 2019-01-07 at 20:52 +0800, Weiping Zhang wrote:
> > If the low level driver has no timerout handler, the
> ^^^^^^^^
> timeout?
>
OK, I'll fix this spelling mistake.
> > +static umode_t queue_attr_visible(struct kobject *kobj, struct attribute
> > *attr,
> > + int n)
> > +{
> > + struct request_queue *q =
> > + container_of(kobj, struct request_queue, kobj);
> > +
> > + if (attr == &queue_io_timeout_entry.attr) {
> > + if (!q->mq_ops || !q->mq_ops->timeout)
> > + return 0;
> > + }
>
> Are there any legacy block drivers left? Do we really need the mq_ops test?
As request_fn removed, now we can use mq_ops->timeout directly.
>
> Additionally, please combine the two nested if-statements into a single
> if-statement.
OK, it's more clear.
>
> > @@ -942,6 +961,14 @@ int blk_register_queue(struct gendisk *disk)
> > goto unlock;
> > }
> >
> > + ret = sysfs_create_group(&q->kobj, &queue_attr_group);
> > + if (ret) {
> > + kobject_del(&q->kobj);
> > + blk_trace_remove_sysfs(dev);
> > + kobject_put(&dev->kobj);
> > + goto unlock;
> > + }
>
> Are you sure the "goto unlock" is OK here? Shouldn't kobject_del() be called
> to undo the kobject_add() call if sysfs_create_group() fails?
Sorry, can you tell me why it's may be not safe, if goto unlock here,
if failed to call sysfs_create_group, I think we should call
kobject_del.
>
> > if (queue_is_mq(q)) {
> > __blk_mq_register_dev(dev, q);
> > blk_mq_debugfs_register(q);
> > @@ -958,6 +985,7 @@ int blk_register_queue(struct gendisk *disk)
> > if (ret) {
> > mutex_unlock(&q->sysfs_lock);
> > kobject_uevent(&q->kobj, KOBJ_REMOVE);
> > + sysfs_remove_group(&q->kobj, &queue_attr_group);
> > kobject_del(&q->kobj);
> > blk_trace_remove_sysfs(dev);
> > kobject_put(&dev->kobj);
> > @@ -1006,6 +1034,7 @@ void blk_unregister_queue(struct gendisk *disk)
> > blk_mq_unregister_dev(disk_to_dev(disk), q);
> > mutex_unlock(&q->sysfs_lock);
> >
> > + sysfs_remove_group(&q->kobj, &queue_attr_group);
> > kobject_uevent(&q->kobj, KOBJ_REMOVE);
> > kobject_del(&q->kobj);
> > blk_trace_remove_sysfs(disk_to_dev(disk));
>
> Is it necessary to call sysfs_remove_group() explicitly? Isn't this something
> kobject_del() does automatically?
I check kobject_del, it's same as you said,
kobject_del->sysfs_remove_dir->kernfs_remove, it will remove all files
and subdirectories belong this kobject directory.
>
> Thanks,
>
Thanks
Weiping
> Bart.
>
