On Thu, Apr 18, 2024, at 17:15, Heiko Carstens wrote: >> > > > - /* >> > > > - * The release function could be called after the >> > > > - * module has been unloaded. It's _only_ task is to >> > > > - * free the struct. Therefore, we specify kfree() >> > > > - * directly here. (Probably a little bit obfuscating >> > > > - * but legitime ...). >> > > > - */ >> > > >> > > Why is the comment not relevant after this change? Or better: why is it not >> > > valid before this change, which is why the code was introduced a very long >> > > time ago? Any reference? >> > > >> > > I've seen the warning since quite some time, but didn't change the code >> > > before sure that this doesn't introduce the bug described in the comment. >> > >> > From only 20 years ago: >> > >> > https://lore.kernel.org/all/20040316170812.GA14971@xxxxxxxxx/ >> > >> > The particular code (zfcp) was changed, so it doesn't have this code >> > (or never did?) anymore, but for the rest this may or may not still >> > be valid. >> >> I guess relevant may not have been the correct word. Maybe obvious? I >> can keep the comment but I do not really see what it adds, although >> reading the above thread, I suppose it was added as justification for >> calling kfree() as ->release() for a 'struct device'? Kind of seems like >> that ship has sailed since I see this all over the place as a >> ->release() function. I do not see how this patch could have a function >> change beyond that but I may be misreading or misinterpreting your full >> comment. > > That doesn't answer my question what prevents the release function > from being called after the module has been unloaded. > > At least back then when the code was added it was a real bug. I think the way this should work is to have the allocation and the release function in the iucv bus driver, with a function roughly like struct device *iucv_alloc_device(char *name, const struct attribute_group *attrs, void *priv) { dev = kzalloc(sizeof(struct device), GFP_KERNEL); if (!dev) return NULL; dev_set_name(dev, "%s", name); dev->bus = &iucv_bus; dev->parent = iucv_root; dev->groups = attrs; dev_set_drvdata(dev, priv); dev->release = iucv_free_dev; return dev; } Now the release function cannot go away as long as any module is loaded that links against it, and those modules cannot go away as long as the devices are in use. I don't remember how iucv works, but if there is a way to detect which system services exist, then the actual device creation should also be separate from the driver using those services, with another driver responsible for enumerating the existing services and creating those devices. Arnd
