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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
