Messages in this thread | | | Date | Fri, 10 Jan 2014 07:24:02 -0800 | From | Greg Kroah-Hartman <> | Subject | Re: Freeing of dev->p |
| |
On Fri, Jan 10, 2014 at 03:39:07PM +0100, Jean Delvare wrote: > + * @driver_data: Private pointer for driver specific info. Will turn into a > + * list soon.
Ah, this comment reminds me of why I originally did this. I was working on moving for a way to have multiple drivers bound to the same device, as people needed that type of thing for something that I can't remember at the moment.
As it's been years now with no real movement forward on that idea, I guess it's not going to happen :)
> * @power: For device power management. > * See Documentation/power/devices.txt for details. > * @pm_domain: Provide callbacks that are executed during system suspend, > @@ -737,6 +739,8 @@ struct device { > device */ > void *platform_data; /* Platform specific data, device > core doesn't touch it */ > + void *driver_data; /* Driver data, set and get with > + dev_set/get_drvdata */ > struct dev_pm_info power; > struct dev_pm_domain *pm_domain; > > > For performance I'd even question the point of the dev check in > dev_get_drvdata(), especially when there is no such check in > dev_set_drvdata() which presumably is always called first.
It's nice to not oops if a NULL pointer is passed in :)
> Plus dev_set_drvdata() can no longer fail (something only 3 drivers in > the whole kernel tree were checking for anyway) so it could return > void instead of int.
True.
> Then I suppose we could inline both functions > again, for performance. Well, put in short, really revering > b4028437876866aba4747a655ede00f892089e14 would be the way to go IMHO.
Almost, the copyright lines should stay :)
> Really, while I understand your envy to protect driver core internals > from unwanted access, the cost here was simply too high IMHO, both in > terms of getting things right and performance. Some drivers are calling > dev_get_drvdata() directly or indirectly repeatedly at run-time. They > had no reason not to as this used to be so fast, and now it is no > longer an inline function, it has conditionals and a double pointer > indirection... > > Plus, I can't think of anything really bad that could result from > accessing driver_data directly, contrary to the other members of struct > device_private.
See first response above for why I did this, it wasn't to just make things "harder" to mess up, I actually had a reason to do it (imagine that!)
Thanks for the detailed response, I think I'll just revert most of that patch and see if it's still workable.
greg k-h
| |