Re: [PATCH] fix RTC_CLASS regression with PARISC

[David Miller <davem@davemloft.net>]

From: David Brownell <david-b@pacbell.net>
Date: Mon, 8 Sep 2008 20:17:23 -0700

> On Monday 08 September 2008, David Miller wrote:
> > 
> > > > int update_persistent_clock(struct timespec now)
> > > > {
> > > >     struct rtc_device *rtc = rtc_class_open("rtc0");
> 
> One more point:  that should probably use CONFIG_RTC_HCTOSYS_DEVICE
> instead of hard-wiring to "rtc0".  Yeah, I'm sure your SPARCs have
> lots of RTCs to choose from -- not! -- but I'd like to see you end
> up with code that many folk can reuse/recycle/pirate.  ;)

Can you be more specific?  Oh, you want me to use the string defined
by that config option.  Ok :-)

But as far as I can tell this will only be set of RTC_HCTOSYS and
users currently are allowed to not set that.

If this code goes somewhere generic you would need to ifdef test on
that, depending upon where you'd want to put it and how it would
be provided generically.

> > > 
> > > I'd be tempted to cache that ... notice how you never
> > > close it, too.  That will goof lots of refcounts...
> > 
> > Well if I cache it then we'll hold it forever and that's not
> > so nice right?
> 
> Why wouldn't it be, so long as it's eventually closed
> to prevent leakage?  Other code can rtc_class_open() too;
> unlike a userspace open("/dev/rtc0", ...) this isn't an
> exclusive operation.

When would be "eventually closed" if I open it here and remember
the pointer in a static local variable, and don't close it?

I guess you need to be more specific about what you mean by
caching :)

> If you're concerned about stuff like "rmmod my-i2c-rtc-driver"
> losing (or "rmmod my-i2c-rtc-driver's-i2c-adapter") ... what's
> supposed to happen is that you start getting an -ENODEV return
> from your rtc_set_mmss() call, and then you close and null your
> cached handle to free up its memory.

I see... god that's ugly.  If you want to do this in the generic
RTC layer helper routines, that's fine, but I don't feel like
adding all sorts of stuff like that to the sparc specific routine
at the moment.

I'm trying to do things that are practical and that I can check
into sparc-next-2.6 right now.