On 2024-03-01 16:12, Xu Yilun wrote: > On Thu, Feb 29, 2024 at 11:37:10AM +0100, Marco Pagani wrote: >> >> On 2024-02-28 08:10, Xu Yilun wrote: >>> On Tue, Feb 27, 2024 at 12:49:06PM +0100, Marco Pagani wrote: >>>> >>>> >>>> On 2024-02-21 15:37, Xu Yilun wrote: >>>>> On Tue, Feb 20, 2024 at 12:11:26PM +0100, Marco Pagani wrote: >>>>>> >>>>>> >>>>>> On 2024-02-18 11:05, Xu Yilun wrote: >>>>>>> On Mon, Feb 05, 2024 at 06:47:34PM +0100, Marco Pagani wrote: >>>>>>>> >>>>>>>> >>>>>>>> On 2024-02-04 06:15, Xu Yilun wrote: >>>>>>>>> On Fri, Feb 02, 2024 at 06:44:01PM +0100, Marco Pagani wrote: >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> On 2024-01-30 05:31, Xu Yilun wrote: >>>>>>>>>>>> +#define fpga_mgr_register_full(parent, info) \ >>>>>>>>>>>> + __fpga_mgr_register_full(parent, info, THIS_MODULE) >>>>>>>>>>>> struct fpga_manager * >>>>>>>>>>>> -fpga_mgr_register_full(struct device *parent, const struct fpga_manager_info *info); >>>>>>>>>>>> +__fpga_mgr_register_full(struct device *parent, const struct fpga_manager_info *info, >>>>>>>>>>>> + struct module *owner); >>>>>>>>>>>> >>>>>>>>>>>> +#define fpga_mgr_register(parent, name, mops, priv) \ >>>>>>>>>>>> + __fpga_mgr_register(parent, name, mops, priv, THIS_MODULE) >>>>>>>>>>>> struct fpga_manager * >>>>>>>>>>>> -fpga_mgr_register(struct device *parent, const char *name, >>>>>>>>>>>> - const struct fpga_manager_ops *mops, void *priv); >>>>>>>>>>>> +__fpga_mgr_register(struct device *parent, const char *name, >>>>>>>>>>>> + const struct fpga_manager_ops *mops, void *priv, struct module *owner); >>>>>>>>>>>> + >>>>>>>>>>>> void fpga_mgr_unregister(struct fpga_manager *mgr); >>>>>>>>>>>> >>>>>>>>>>>> +#define devm_fpga_mgr_register_full(parent, info) \ >>>>>>>>>>>> + __devm_fpga_mgr_register_full(parent, info, THIS_MODULE) >>>>>>>>>>>> struct fpga_manager * >>>>>>>>>>>> -devm_fpga_mgr_register_full(struct device *parent, const struct fpga_manager_info *info); >>>>>>>>>>>> +__devm_fpga_mgr_register_full(struct device *parent, const struct fpga_manager_info *info, >>>>>>>>>>>> + struct module *owner); >>>>>>>>>>> >>>>>>>>>>> Add a line here. I can do it myself if you agree. >>>>>>>>>> >>>>>>>>>> Sure, that is fine by me. I also spotted a typo in the commit log body >>>>>>>>>> (in taken -> is taken). Do you want me to send a v6, or do you prefer >>>>>>>>>> to fix that in place? >>>>>>>>> >>>>>>>>> No need, I can fix it. >>>>>>>>> >>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> There is still a RFC prefix for this patch. Are you ready to get it merged? >>>>>>>>>>> If yes, Acked-by: Xu Yilun <yilun.xu@xxxxxxxxx> >>>>>>>>>> >>>>>>>>>> I'm ready for the patch to be merged. However, I recently sent an RFC >>>>>>>>>> to propose a safer implementation of try_module_get() that would >>>>>>>>>> simplify the code and may also benefit other subsystems. What do you >>>>>>>>>> think? >>>>>>>>>> >>>>>>>>>> https://lore.kernel.org/linux-modules/20240130193614.49772-1-marpagan@xxxxxxxxxx/ >>>>>>>>> >>>>>>>>> I suggest take your fix to linux-fpga/for-next now. If your try_module_get() >>>>>>>>> proposal is applied before the end of this cycle, we could re-evaluate >>>>>>>>> this patch. >>>>>>>> >>>>>>>> That's fine by me. >>>>>>> >>>>>>> Sorry, I still found issues about this solution. >>>>>>> >>>>>>> void fpga_mgr_unregister(struct fpga_manager *mgr) >>>>>>> { >>>>>>> dev_info(&mgr->dev, "%s %s\n", __func__, mgr->name); >>>>>>> >>>>>>> /* >>>>>>> * If the low level driver provides a method for putting fpga into >>>>>>> * a desired state upon unregister, do it. >>>>>>> */ >>>>>>> fpga_mgr_fpga_remove(mgr); >>>>>>> >>>>>>> mutex_lock(&mgr->mops_mutex); >>>>>>> >>>>>>> mgr->mops = NULL; >>>>>>> >>>>>>> mutex_unlock(&mgr->mops_mutex); >>>>>>> >>>>>>> device_unregister(&mgr->dev); >>>>>>> } >>>>>>> >>>>>>> Note that fpga_mgr_unregister() doesn't have to be called in module_exit(). >>>>>>> So if we do fpga_mgr_get() then fpga_mgr_unregister(), We finally had a >>>>>>> fpga_manager dev without mops, this is not what the user want and cause >>>>>>> problem when using this fpga_manager dev for other FPGA APIs. >>>>>> >>>>>> How about moving mgr->mops = NULL from fpga_mgr_unregister() to >>>>>> class->dev_release()? In that way, mops will be set to NULL only when the >>>>>> manager dev refcount reaches 0. >>>>> >>>>> I'm afraid it doesn't help. The lifecycle of the module and the fpga >>>>> mgr dev is different. >>>>> >>>>> We use mops = NULL to indicate module has been freed or will be freed in no >>>>> time. On the other hand mops != NULL means module is still there, so >>>>> that try_module_get() could be safely called. It is possible someone >>>>> has got fpga mgr dev but not the module yet, at that time the module is >>>>> unloaded, then try_module_get() triggers crash. >>>>> >>>>>> >>>>>> If fpga_mgr_unregister() is called from module_exit(), we are sure that nobody >>>>>> got the manager dev earlier using fpga_mgr_get(), or it would have bumped up >>>>> >>>>> No, someone may get the manager dev but not the module yet, and been >>>>> scheduled out. >>>>> >>>> >>>> You are right. Overall, it's a bad idea. How about then using an additional >>>> bool flag instead of "overloading" the mops pointer? Something like: >>>> >>>> get: >>>> if (!mgr->owner_valid || !try_module_get(mgr->mops_owner)) >>>> >>>> remove: >>>> mgr->owner_valid = false; >>> >>> I'm not quite sure which function is actually mentioned by "remove". I >>> assume it should be fpga_mgr_unregister(). >> >> Yes, I was referring to fpga_mgr_unregister(). >> >>> IIUC this flag means no more reference to fpga mgr, but existing >>> references are still valid. >> >> Yes. >> >>> >>> It works for me. But the name of this flag could be reconsidered to >>> avoid misunderstanding. The owner is still valid (we still need to put >>> the owner) but allows no more reference. Maybe "owner_inactive"? >> >> Right, owner_valid might be misleading. How about removing any >> reference to the owner module and name the flag unreg? > > the full name "unregistered" is better. That's fine by me. > >> >> __fpga_mgr_get: >> if (mgr->unreg || !try_module_get(mgr->mops_owner)) >> mgr = ERR_PTR(-ENODEV); >> >> fpga_mgr_unregister: >> mgr->unreg = true; >> >>> I still wanna this owner reference change been splitted, so that >>> we could simply revert it when the try_module_get_safe() got accepted. >> >> I guess it may take some time to have try_module_get_safe() accepted. >> What do you prefer to do with the bridge and the region in the >> meantime? > > This issue could happen in little chance. I actually don't have much > preference, either way is good to me. > Okay, I'll also send the patch for the region then. Thanks, Marco
