On Fri, Nov 10, 2023 at 11:58:37PM +0100, Marco Pagani wrote: > > > On 2023-11-08 16:52, Xu Yilun wrote: > > On Fri, Nov 03, 2023 at 09:31:02PM +0100, Marco Pagani wrote: > >> > >> > >> On 2023-10-30 09:32, Xu Yilun wrote: > >>> On Fri, Oct 27, 2023 at 05:29:27PM +0200, Marco Pagani wrote: > >>>> Remove unnecessary module reference counting from the core components > >>>> of the subsystem. Low-level driver modules cannot be removed before > >>>> core modules since they use their exported symbols. > >>> > >>> Could you help show the code for this conclusion? > >>> > >>> This is different from what I remember, a module cannot be removed when > >>> its exported symbols are being used by other modules. IOW, the core > >>> modules cannot be removed when there exist related low-level driver > >>> modules. But the low-level driver modules could be removed freely > >>> without other protecting mechanism. > >>> > >> > >> My understanding was that we wanted to remove module reference counting > >> from the fpga core and ease it from the responsibility of preventing > >> low-level driver modules from being unloaded. > > > > FPGA core needs to prevent low-level driver module unloading sometimes, > > e.g. when region reprograming is in progress. That's why we get fpga > > region driver modules & bridge modules in fpga_region_program_fpga(). > > > > But we try best to get them only necessary. Blindly geting them all the > > time results in no way to unload all modules (core & low level modules). > > > >> > >> If we want to keep reference counting in the fpga core, we could add a > >> struct module *owner field in the struct fpga_manager_ops (and others > >> core *_ops) so that the low-level driver can set it to THIS_MODULE. > >> In this way, we can later use it in fpga_mgr_register() to bump up the > > > > Yes, we should pass the module owner in fpga_mgr_register(), but could > > not bump up its refcount at once. > > > >> refcount of the low-level driver module by calling > >> try_module_get(mgr->mops->owner) directly when it registers the manager. > >> Finally, fpga_mgr_unregister() would call module_put(mgr->mops->owner) > >> to allow unloading the low-level driver module. > > > > As mentioned above, that makes problem. Most of the low level driver > > modules call fpga_mgr_unregister() on module_exit(), but bumping up > > their module refcount prevents module_exit() been executed. That came > > out to be a dead lock. > > > > Initially, I considered calling try_module_get(mgr->mops->owner) > in fpga_mgr_get(). But then, the new kernel-doc description of > try_module_get() (1) made me question the safety of that approach. > My concern is that the low-level driver could be removed right when > someone is calling fpga_mgr_get() and hasn't yet reached > try_module_get(mgr->mops->owner). Can that really happen? This shouldn't be a real issue, but normally this should only be needed on an open() like call, don't tie a module reference to a device reference, those are two different things. > In that case, the struct mops > (along with the entire low-level driver module) and the manager dev > would "disappear" under the feet of fpga_mgr_get(). You should have a lock for handling this anyway, this feels odd that it's a problem, but I haven't looked at the code in a long time. try it and see! greg k-h
