* 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