linux-kernel - Re: [PATCHv1] ARM: imx: Add support for low power suspend on MX51.

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 for Android: free password hash cracker in your pocket

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

Message-ID: <alpine.LFD.2.00.1103022343410.2701@localhost6.localdomain6>
Date:	Thu, 3 Mar 2011 00:51:32 +0100 (CET)
From:	Thomas Gleixner <tglx@...utronix.de>
To:	Uwe Kleine-König 
	<u.kleine-koenig@...gutronix.de>
cc:	Dinh.Nguyen@...escale.com, linux-kernel@...r.kernel.org,
	linux@....linux.org.uk, s.hauer@...gutronix.de,
	xiao-lizhang@...escale.com, linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCHv1] ARM: imx: Add support for low power suspend on MX51.

Uwe,

On Wed, 2 Mar 2011, Uwe Kleine-König wrote:
> On Wed, Mar 02, 2011 at 11:17:58AM -0600, Dinh.Nguyen@...escale.com wrote:
> > From: Dinh Nguyen <Dinh.Nguyen@...escale.com>

> > --- /dev/null
> > +++ b/arch/arm/mach-mx5/pm.c
> I'd like to have that called pm-imx51.c

And I'd like to have a pony.

> > +		ccm_clpcr |= (0x3 << MXC_CCM_CLPCR_STBY_COUNT_OFFSET);
> the parentheses aren't needed here

Could you finally provide a patch to checkpatch.pl or git commit which
resolves that issue once and forever ?

Not to mention the fact, that those parentheses are not disturbing the
readability of that code at all.

> > +		ccm_clpcr |= (0x1 << MXC_CCM_CLPCR_LPM_OFFSET);
> ditto

Ditto.

> > +static int __init mx5_pm_init(void)
> I'd prefer to have that called by imx51_init_early.

And the reason is? 

    1) your personal preference
    2) there is some useful technical reason

If #1, then this comment was just waste of electrons
If #2, you failed to provide some reasonable explanation

Again, I'd like to have a pony.

Seriously, while all of us admire your invaluable skills of running
scripts over patches and kernel code, that kind of review you are
trying to provide is utterly useless.

1) The patch itself has been questioned about its correctness hours
   before you added the output of your secret script. It was already
   reported to be non functional. So what's the value of adding
   scriptable review to it?

2) As long as you do not see the most obvious functional problems with
   a patch please spare your script computing power and the bandwidth
   you are consuming by your futile attempts to gain a profile as a
   patch reviewer.

Just for the record. The obvious bug with this code is this:

> > +	plat_lpc |= MXC_CORTEXA8_PLAT_LPC_DSM
> > +		    | MXC_CORTEXA8_PLAT_LPC_DBG_DSM;
> > +	arm_srpgcr |= MXC_SRPGCR_PCR;
> > +
> > +	__raw_writel(plat_lpc, MXC_CORTEXA8_PLAT_LPC);
> > +	__raw_writel(ccm_clpcr, MXC_CCM_CLPCR);
> > +	__raw_writel(arm_srpgcr, MXC_SRPG_ARM_SRPGCR);
> > +	__raw_writel(arm_srpgcr, MXC_SRPG_NEON_SRPGCR);
> > +
> > +	if (tzic_enable_wake(0) != 0)
> > +		return -EAGAIN;

It happily returns here with -EAGAIN w/o undoing the already committed
changes which are preparatory to the tzic_enable_wake() check.

I might be wrong as usual, but if this is cleaned up by the calling
code magically then the "return -EINVAL"; lacks a big fat
comment. While such an omission is not a triggerable bug it documents
the lack of tought and taste by creating asymetric mechanisms w/o the
courtesy to document them properly. Even if documented, asymetric
interfaces are crap most of the time.

> > +	cpu_do_idle();
> > +	return 0;
> > +}

Thanks,

	tglx