linux-kernel - Re: [PATCH] PCAP RTC driver (for 2.6.32).

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Fri, 26 Jun 2009 20:57:38 -0300
From:	Daniel Ribeiro <drwyrm@...il.com>
To:	Andrew Morton <akpm@...ux-foundation.org>
Cc:	a.zummo@...ertech.it, rtc-linux@...glegroups.com,
	linux-kernel@...r.kernel.org, openezx-devel@...ts.openezx.org,
	sameo@...ux.intel.com
Subject: Re: [PATCH] PCAP RTC driver (for 2.6.32).

Hi Andrew,

Em Sex, 2009-06-26 às 13:23 -0700, Andrew Morton escreveu:
> This could be coded more simply:
> 
> 	unsigned long rtc_events;
> 
> 	if (irq == pcap_to_irq(pcap_rtc->pcap, PCAP_IRQ_1HZ))
> 		rtc_events = RTC_IRQF | RTC_UF;
> 	else if (irq == pcap_to_irq(pcap_rtc->pcap, PCAP_IRQ_TODA))
> 		rtc_events = RTC_IRQF | RTC_AF;

This would make gcc complain about rtc_events being used uninitialized.

> SEC_PER_DAY is defined in include/linux/mfd/ezx-pcap.h.  It should not
> be, because it is not specific to that driver and can be used elsewhere
> in the kernel.
> 
> I'd suggest that we
> 
> - define SECS_PER_DAY in include/linux/time.h
> 
> - remove the private definitions of SECS_PER_DAY from
>   arch/m68k/mac/misc, arch/parisc/include/asm/rtc.h,
>   arch/ia64/hp/sim/boot/fw-emu.c, fs/udf/udftime.c, fs/fat/misc.c and
>   wherever else it appears, make those files use the common definition
> 
> - migrate rtc-pcap.c from SEC_PER_DAY over to the common SECS_PER_DAY.
> 
> - Then do it all again for SECS_PER_MIN and SECS_PER_HOUR.
> 
> 
> What a mess.

I can't work on this right now, but i will be happy to reserve some
hours for this next month if nobody else does it before.

> "tmp" is a poor identifier.  We had an opportunity here to use an
> identifier which would communicate useful information to the reader. 
> But we blew it and used the information-free "tmp" instead.
> 
> Something like
> 
> 	u32 time_of_day;	/* In seconds since midnight */
> 
> would be nice.  If that is indeed what the variable contains.  How
> would I know?  It's called 'tmp" and is undescribed!

Hum.. I used tmp because the variable is reused and it may be "days
since 1/1/1970" or "seconds since today's 00:00:00". I will add some
comments to make this clear.

> > +static inline int pcap_rtc_irq_enable(struct device *dev, int pirq,
> > +							unsigned int en)
> > +{
> Inlining this function probably made the code larger and slower.

Right, i will amend this.


I see that you have already added it to the -mm tree. Do you prefer an
incremental patch to address your comments or should I send a patch to
replace the current version?

Thanks for the review! :)

-- 
Daniel Ribeiro

Download attachment "signature.asc" of type "application/pgp-signature" (198 bytes)