Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1561031

Re: [PATCH 11/13] x86/microcode/AMD: Unify load_ucode_amd_ap()

Show all headers | View raw

On Tue, 17 Jan 2017, Thomas Gleixner wrote:

> On Tue, 17 Jan 2017, Borislav Petkov wrote:
> >  void load_ucode_amd_ap(unsigned int cpuid_1_eax)
> >  {
> >  	struct equiv_cpu_entry *eq;
> >  	struct microcode_amd *mc;
> > +	struct cont_desc *desc;
> >  	u16 eq_id;
> >  
> > +	if (IS_ENABLED(CONFIG_X86_32)) {
> > +		mc   = (struct microcode_amd *)__pa_nodebug(amd_ucode_patch);
> > +		desc = (struct cont_desc *)__pa_nodebug(&cont);
> > +	} else {
> > +		mc   = (struct microcode_amd *)amd_ucode_patch;
> > +		desc = &cont;
> 
> Bah! Now I realize that 'cont' is not a local variable as I assumed when
> looking at the other patch. 'cont' is a pretty bad name for a (file) global
> variable. Can we please use a more obvious name ?
> 
> While at it please make that thing static as there cant be a user outside
> of that file.
> 
> > +	}
> > +
> >  	/* First AP hasn't cached it yet, go through the blob. */
> > -	if (!cont.data) {
> > -		struct cpio_data cp;
> > +	if (!desc->data) {
> > +		struct cpio_data cp = { };
> >  
> > -		if (cont.size == -1)
> > +		if (desc->size == -1)
> >  			return;
> 
> I'm not really fond of abusing the size member for this. And that '-1'
> seems to have different meanings depending on other members. Really not
> intuitive.
> 
> Please introduce a proper state member which tells what the descriptor
> struct contains, i.e. EMPTY, VALID, INVALID ....

Reading further through the series I see that you remove all that cruft at
the end. Dammit, I never start reading books in the final chapter, maybe I
should do that with patch series :)

It might be nice nevertheless to have an initial patch which cleans up the
name of that 'cont' variable and that status thing, but it might not be
worth the trouble as you remove it at the end anyway, which makes a lot of
sense btw. Your call.

Thanks,

	tglx

Back to linux.kernel | Previous | Next — Previous in thread | Next in thread | Find similar | Unroll thread

Thread

[PATCH 00/13] x86/microcode: 4.11 queue Borislav Petkov <bp@alien8.de> - 2017-01-17 18:50 +0100
  [PATCH 12/13] x86/microcode/AMD: Simplify saving from initrd Borislav Petkov <bp@alien8.de> - 2017-01-17 18:50 +0100
    Re: [PATCH 12/13] x86/microcode/AMD: Simplify saving from initrd Thomas Gleixner <tglx@linutronix.de> - 2017-01-17 22:20 +0100
  [PATCH 11/13] x86/microcode/AMD: Unify load_ucode_amd_ap() Borislav Petkov <bp@alien8.de> - 2017-01-17 18:50 +0100
    Re: [PATCH 11/13] x86/microcode/AMD: Unify load_ucode_amd_ap() Thomas Gleixner <tglx@linutronix.de> - 2017-01-17 22:00 +0100
      Re: [PATCH 11/13] x86/microcode/AMD: Unify load_ucode_amd_ap() Thomas Gleixner <tglx@linutronix.de> - 2017-01-17 22:20 +0100
  [PATCH 05/13] x86/microcode/AMD: Extend the container struct Borislav Petkov <bp@alien8.de> - 2017-01-17 18:50 +0100
    Re: [PATCH 05/13] x86/microcode/AMD: Extend the container struct Thomas Gleixner <tglx@linutronix.de> - 2017-01-17 21:50 +0100
  [PATCH 09/13] x86/microcode/AMD: Use find_microcode_in_initrd() Borislav Petkov <bp@alien8.de> - 2017-01-17 18:50 +0100
    Re: [PATCH 09/13] x86/microcode/AMD: Use  find_microcode_in_initrd() Thomas Gleixner <tglx@linutronix.de> - 2017-01-17 22:20 +0100
  [PATCH 04/13] x86/microcode/AMD: Shorten function parameter's name Borislav Petkov <bp@alien8.de> - 2017-01-17 18:50 +0100
    Re: [PATCH 04/13] x86/microcode/AMD: Shorten function parameter's  name Thomas Gleixner <tglx@linutronix.de> - 2017-01-17 21:10 +0100
  [PATCH 10/13] x86/microcode/AMD: Check patch level only on the BSP Borislav Petkov <bp@alien8.de> - 2017-01-17 18:50 +0100
    Re: [PATCH 10/13] x86/microcode/AMD: Check patch level only on the  BSP Thomas Gleixner <tglx@linutronix.de> - 2017-01-17 21:50 +0100

csiph-web