Re: [PATCH 3/5] watchdog: cleanup a bit omap_wdt.c

[Russell King - ARM Linux]

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-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/