Greg KH, on 10/22/2010 09:56 PM wrote: > On Fri, Oct 22, 2010 at 09:30:53PM +0400, Vladislav Bolkhovitin wrote: >> + unsigned int tgt_kobj_initialized:1; > > It's the middle of the merge window, and I'm about to go on vacation, so > I didn't read this patch after this line. > > It's obvious that this patch is wrong, you shouldn't need to worry about > this. And even if you did, you don't need this flag. > > Why are you trying to do something that no one else needs? Why make > things harder than they have to be. I tried to explain that to you in http://lkml.org/lkml/2010/10/14/291 and mentioned there the need to create this flag to track half-initialized kobjects. You agreed (http://lkml.org/lkml/2010/10/14/299) that don't return half-initialized objects is a regular kernel practice, but then requested to strictly bound the larger object freeing to its kobject release(), which means that all SYSFS creating functions now have to return half-initialized SYSFS hierarchy in case of any error. Hence the flag to track it. Simply, any SCST object has a lot of other things to initialize besides its kobject, hence the need either to free it independently from its kobject, or track by a flag if its kobjects initialized. For illustration, here is the simplified code from my previous example, i.e. without this patch. I added to it scst_unregister_target() to make it more complete. void scst_tgt_sysfs_del(struct scst_tgt *tgt) { ... kobject_put(&tgt->tgt_kobj); wait_for_completion(&tgt->tgt_kobj_release_cmpl); } int scst_tgt_sysfs_create(struct scst_tgt *tgt) { init_completion(&tgt->tgt_kobj_release_cmpl); res = kobject_init_and_add(&tgt->tgt_kobj, &tgt_ktype, &tgt->tgtt->tgtt_kobj, tgt->tgt_name); if (res != 0) goto out; res = sysfs_create_file(&tgt->tgt_kobj, &tgt_enable_attr.attr); if (res != 0) goto out_err; ... out: return res; out_err: scst_tgt_sysfs_del(tgt); goto out; } struct scst_tgt *scst_register_target() { struct scst_tgt *tgt; int rc = 0; rc = scst_alloc_tgt(); if (rc != 0) goto out; ... tgt->tgt_name = kmalloc(strlen(target_name) + 1, GFP_KERNEL); if (tgt->tgt_name == NULL) goto out_free_tgt; ... mutex_lock(); rc = scst_tgt_sysfs_create(tgt); if (rc < 0) goto out_unlock; tgt->default_acg = scst_alloc_add_acg(); if (tgt->default_acg == NULL) goto out_sysfs_del; ... out: return tgt; out_sysfs_del: mutex_unlock(); scst_tgt_sysfs_del(tgt) goto out_free_tgt; out_unlock: mutex_unlock(); out_free_tgt: scst_free_tgt(tgt); } void scst_unregister_target(struct scst_tgt *tgt) { ... scst_tgt_sysfs_del(tgt); ... scst_free_tgt(tgt); } You can see complete source code of those functions in the original patch set I sent in this thread (patches 4 and 8). What am I missing and how errors processing should be done with neither freeing tgt not in tgt_kobj release() as above, nor without tgt_kobj_initialized flag as in the patch? Thanks, Vlad -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
