On Tue, Feb 09, 2021 at 01:13:17PM -0800, Minchan Kim wrote:

On Tue, Feb 09, 2021 at 12:11:20PM -0800, John Hubbard wrote:

quoted

On 2/9/21 9:49 AM, Greg KH wrote:

quoted

quoted

quoted

That's fine if you want to add it to the parent.  If so, then the
kobject controls the lifetime of the structure, nothing else can.

The problem was parent object(i.e., struct cma cma_areas) is
static arrary so kobj->release function will be NULL or just
dummy. Is it okay? I thought it was one of the what you wanted
to avoid it.

No, that is not ok.

quoted

quoted

Either is fine with me, what is "forbidden" is having a kobject and
somehow thinking that it does not control the lifetime of the structure.

Since parent object is static arrary, there is no need to control the
lifetime so I am curious if parent object approach is okay from kobject
handling point of view.

So the array is _NEVER_ freed?  If not, fine, don't provide a release
function for the kobject, but ick, just make a dynamic kobject I don't
see the problem for something so tiny and not very many...

Yeah, I wasn't trying to generate so much discussion, I initially thought it
would be a minor comment: "just use an embedded struct and avoid some extra
code", at first.

quoted

I worry that any static kobject might be copied/pasted as someone might
think this is an ok thing to do.  And it's not an ok thing to do.

Overall, then, we're seeing that there is a small design hole: in order
to use sysfs most naturally, you either much provide a dynamically allocated
item for it, or you must use the static kobject, and the second one sets a
bad example.

I think we should just use a static kobject, with a cautionary comment to
would-be copy-pasters, that explains the design constraints above. That way,
we still get a nice, less-code implementation, a safe design, and it only
really costs us a single carefully written comment.

thanks,

Agreed. How about this for the warning part?

+
+/*
+ * note: kobj_type should provide a release function to free dynamically
+ * allocated object since kobject is responsible for controlling lifespan
+ * of the object. However, cma_area is static object so technially, it
+ * doesn't need release function. It's very exceptional case so pleaes
+ * do not follow this model.
+ */
 static struct kobj_type cma_ktype = {
        .sysfs_ops = &kobj_sysfs_ops,
        .default_groups = cma_groups
+       .release = NULL, /* do not follow. See above */
 };

No, please no.  Just do it the correct way, what is the objection to
creating a few dynamic kobjects from the heap?  How many of these are
you going to have that it will somehow be "wasteful"?

Please do it properly.

thanks,

greg k-h