Re: [Cocci] [RFC] drop owner assignment from platform_drivers
> > You got me wondering, though, that it could not be correct to call > > platform_driver_register() from the platform core instead of module > > init. I will check tomorrow. Still, this would be a bug independent of > > my series. Although I'd need to respin it if platform_driver_probe() > > needed a fix. > > Right, this seems to be a preexisting bug. platform_create_bundle > and platform_driver_probe will both overwrite the .owner field with > NULL since they live in builtin code. They need to be replaced with > __platform_driver_probe and __platform_driver_register that both > take an extra owner argument passed down from the caller in the driver > module. Yeah, that would be one solution. However, my personal favourite would meanwhile be to revert the commit that Russell mentioned. I think it is cleaner to have the owner explicitly set in the module rather than hidden away by a function call. However, grepping through include/linux, there are a few subsystems hiding it this way. So, it is a pattern somewhow. Oh well... signature.asc Description: Digital signature ___ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [Cocci] [RFC] drop owner assignment from platform_drivers
On Sat, Oct 11, 2014 at 06:56:51PM +0200, Wolfram Sang wrote: > > > > You got me wondering, though, that it could not be correct to call > > > platform_driver_register() from the platform core instead of module > > > init. I will check tomorrow. Still, this would be a bug independent of > > > my series. Although I'd need to respin it if platform_driver_probe() > > > needed a fix. > > > > Right, this seems to be a preexisting bug. platform_create_bundle > > and platform_driver_probe will both overwrite the .owner field with > > NULL since they live in builtin code. They need to be replaced with > > __platform_driver_probe and __platform_driver_register that both > > take an extra owner argument passed down from the caller in the driver > > module. > > Yeah, that would be one solution. However, my personal favourite would > meanwhile be to revert the commit that Russell mentioned. I think it is > cleaner to have the owner explicitly set in the module rather than > hidden away by a function call. However, grepping through include/linux, > there are a few subsystems hiding it this way. So, it is a pattern > somewhow. Oh well... It really /ought/ to be consistent, because inconsistencies like that will be a never-ending source of subtle mistakes. Imagine what it would be like if the kernel was a complete mess of functions with return type "int" where there was no predominant pattern of returning negative errno numbers - where it was random whether int-returning functions returned zero for failure, others returned zero for success. We would have to look up every single function to check it's return style, and it would be a bigger problem when reviewing code. There is a lot of value for saving time and reducing errors to have a consistent, simple and obvious methodology. (That's not to say that it should be enforced draconian style - but there'd better be a good reason to be different, rather than "I think it's better this way" or "my personal style is different".) -- FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up according to speedtest.net. ___ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [Cocci] [RFC] drop owner assignment from platform_drivers
On Sat, Oct 11, 2014 at 06:56:51PM +0200, Wolfram Sang wrote: > > > > You got me wondering, though, that it could not be correct to call > > > platform_driver_register() from the platform core instead of module > > > init. I will check tomorrow. Still, this would be a bug independent of > > > my series. Although I'd need to respin it if platform_driver_probe() > > > needed a fix. > > > > Right, this seems to be a preexisting bug. platform_create_bundle > > and platform_driver_probe will both overwrite the .owner field with > > NULL since they live in builtin code. They need to be replaced with > > __platform_driver_probe and __platform_driver_register that both > > take an extra owner argument passed down from the caller in the driver > > module. > > Yeah, that would be one solution. However, my personal favourite would > meanwhile be to revert the commit that Russell mentioned. I think it is > cleaner to have the owner explicitly set in the module rather than > hidden away by a function call. However, grepping through include/linux, > there are a few subsystems hiding it this way. So, it is a pattern > somewhow. Oh well... The pattern is to not have to manually set MODULE_OWNER, and have the pre-processor do it for you, otherwise you will forget or get it wrong. That is why I accepted this patch to the platform driver interface, as it is in line with many other bus driver apis (pci, usb, etc.). I missed the one code path you pointed out, and that should be fixed, but that doesn't mean that the original patch should be reverted, as it is the way we want things to be, let's just fix up the bug and move on. And again, may I just say how much I hate the platform driver code, one of these days I'm going to lock myself in a room for a week and figure out a way to just delete that stuff... greg k-h ___ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [Cocci] [RFC] drop owner assignment from platform_drivers
> I missed the one code path you pointed out, and that should be fixed, > but that doesn't mean that the original patch should be reverted, as it > is the way we want things to be, let's just fix up the bug and move on. OK, that is a clear statement. So, what is your opinion on the original cleanup series removing unnecessary '.owner = THIS_MODULE' lines in drivers? Helpful? Noise? Thanks, Wolfram signature.asc Description: Digital signature ___ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
