On Sat, Apr 22, 2000 at 02:40:07PM +0200, Manfred Spraul wrote:
> Cesar Eduardo Barros wrote:
> >
> > (not on the list, please CC: replies to me)
> >
> > Using atomic_* seems to cause a false sense of security in some people.
> > atomic_set(...,atomic_read(...)) is not atomic, and rtc.c did it everywhere.
> > Also, testing for a bit and later enabling it (like in rtc_open) causes races
> > (two proccesses in SMP could open the rtc at the same time if they got the
> > right timing). I patched it to use a spinlock instead.
> >
>
> rtc_release, rtc_open and rtc_ioctl are protected by lock_kernel(), so
> they are protected from SMP races.
>
I'm a newbie at kernel hacking, and it shows =)
> >
> > if (rtc_async_queue)
> > + /* Is this OK on interrupt? */
> > kill_fasync (rtc_async_queue, SIGIO, POLL_IN);
> >
>
> yes.
>
Thanks for this one =)
> > @@ -699,8 +717,13 @@
> > {
> > /* interrupts and maybe timer disabled at this point by rtc_release */
> >
> > - if (atomic_read(&rtc_status) & RTC_TIMER_ON)
> > + /* FIXME: how rtc_open here could be avoided? */
>
> the magic of lock_kernel() helps:
> rtc_exit() and rtc_open() both run with the big kernel lock.
>
Nothing is worse than undocumented magic =)
> I'm far more concerned about the xchg() in rtc_read:
> that xchg() and rtc_interrupt() aren't synchronized, the rest could be
> fixed with test_and_set(), atomic_mask(),...
Well, I got the result I wanted: to show rtc.c was doing something fishy. I
only looked at the horrible rtc_status atomic_set(...,atomic_read(...)) mess,
though. I think the problem is that rtc_irq_data is holding two different
values for two different purposes. I'll look at it and try to remake my patch
(this time without spinlocks I hope).
-- Cesar Eduardo Barros cesarb@web4u.com.br cesarb@dcc.ufrj.br- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.rutgers.edu Please read the FAQ at http://www.tux.org/lkml/
This archive was generated by hypermail 2b29 : Sun Apr 23 2000 - 21:00:21 EST