Date: Mon, 2 Mar 2009 21:57:27 +0100
From: Ingo Molnar <mingo@elte.hu>
To: Yinghai Lu <yinghai@kernel.org>
Cc: "Kevin O'Connor" <kevin@koconnor.net>, "Rafael J. Wysocki" <rjw@sisk.pl>,
       Stefan Reinauer <stepan@coresystems.de>, linux-kernel@vger.kernel.org,
       coreboot@coreboot.org
Subject: Re: [PATCH] x86: ioremap mptable -v2
Message-ID: <20090302205727.GA20228@elte.hu>
References: <20090222225847.GB1649@elte.hu> <49A23EE0.8000102@kernel.org> <20090228234127.GA29964@morn.localdomain> <49A9FC9A.2030707@kernel.org> <20090301180420.GA27040@morn.localdomain> <49AB511E.9010507@kernel.org> <20090302101823.GJ20897@elte.hu> <49AC3C8C.8070702@kernel.org> <20090302202918.GA14471@elte.hu> <49AC459F.10606@kernel.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <49AC459F.10606@kernel.org>
User-Agent: Mutt/1.5.18 (2008-05-17)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 1946
Lines: 56


* Yinghai Lu <yinghai@kernel.org> wrote:

> Ingo Molnar wrote:
> > * Yinghai Lu <yinghai@kernel.org> wrote:
> > 
> >> V3: according to Ingo, seperate get_mpc_size()
> > 
> > No, that was not my suggestion. My suggestion was to separate 
> > this whole 'else if' branch:
> > 
> >>  	} else if (mpf->physptr) {
> >> +		struct mpc_table *mpc;
> >> +		unsigned long size;
> >>  
> >> +		size = get_mpc_size(mpf->physptr);
> >> +		mpc = early_ioremap(mpf->physptr, size);
> >>  		/*
> >>  		 * Read the physical hardware table.  Anything here will
> >>  		 * override the defaults.
> >>  		 */
> >> -		if (!smp_read_mpc(phys_to_virt(mpf->physptr), early)) {
> >> +		if (!smp_read_mpc(mpc, early)) {
> >>  #ifdef CONFIG_X86_LOCAL_APIC
> >>  			smp_found_config = 0;
> >>  #endif
> > 
> > ... into a helper function - if that improves the code.
> oh, i missed it
> > Your patch does early_ioremap, iounmap then ioremap and iounmap - 
> > quite pointlessly.
> try to get exact mpc size.
> > 
> > You should resist cleanup suggestions that make the code worse, 
> > even if it comes from a maintainer :-)
> 
> we could do that later. to make __get_smp_config smaller and readable.

No, do it in two separate patches please: _first_ do the whole 
cleanup of these functions - on the assumption and expectation 
that it wont break anything. Then add the early_ioremap() change 
in a second patch - on top of the cleanup patch.

If we do a cleanup _after_ a functional change then we make the 
feature patch harder to revert and harder to fix as well. We'd 
always have to 'see through' the cleanup patch when considering 
breakages caused by the functional patch.

Like i suggested in my first reply ;-)

	Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/