On Sat, Sep 20, 2008 at 08:32:14AM -0700, David Brownell wrote: > On Saturday 20 September 2008, Russell King - ARM Linux wrote: > > On Fri, Sep 19, 2008 at 05:41:44PM -0700, David Brownell wrote: > > > On Friday 19 September 2008, Felipe Balbi wrote: > > > > static int omap_wdt_open(struct inode *inode, struct file *file) > > > > { > > > > - struct omap_wdt_dev *wdev; > > > > - void __iomem *base; > > > > - wdev = platform_get_drvdata(omap_wdt_dev); > > > > - base = wdev->base; > > > > + struct omap_wdt_dev *wdev = platform_get_drvdata(omap_wdt_dev); > > > > + void __iomem *base = wdev->base; > > > > + > > > > > > Oh, I see where "omap_wdt_dev" (global) gets used. The normal > > > way to do stuff like that is using void* pointers placed in the > > > inode and file structures for exactly that purpose. > > > > You don't have an inode or a file structure until open() is called - > > at which point it _is_ placed in file->private_data. So this driver > > is doing the right thing. > > Well, the conventional thing for misc drivers, at any rate. In > various other drivers, inode->i_private is set up earlier, just > to avoid such a need for globals (or equivalent).
None that I know about - generally other drivers look up their private data in some kind of array and assign to file->private_data in their open() method - in much the same way that watchdog and misc drivers do. See __ptmx_open, __tty_open, pp_open, apm_open, hpet_open, mbcs_open, raw_open, etc. If you look at data structure lifetimes, the lifetime of 'file' is for the duration that any one particular instance of the file is open, and when closed, it's destroyed. It is not shared between separate opens of the same node. The 'inode' is shared between separate opens, and can be discarded when the node is not open by anyone - in other words, it's not persistent. So the only time that an inode structure is guaranteed to be present is just before the node is opened - useless for passing private pointers from the registration function through to the open() function. So, you need to store the private data pointer somewhere no matter what. The simplest solution is as the watchdog drivers are doing. You can only have one watchdog driver anyway, so there's no problem having a single static global device pointer to allow you to carry your private data across to the open() function. And anyway, the point of these patches is not to fix issues like this. It's to get what's in mainline updated to what's in the OMAP tree so stuff can move forwards. So, let's not go down rabbit warrens trying to find obscure new issues which lots of other code already "suffers" from. We're into the third day on this one driver. If every OMAP driver takes this long, we're still going to be struggling with this beyond Christmas, probably no further forward since other stuff will have regressed, and we'll have to start over again. Yes, make sure what we're submitting is correct. But don't introduce any _new_ unnecessary changes while we're trying to merge stuff upstream. Do that only once it's upstream and send those changes upstream. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html