Re: [PATCH 1/1] x86/PCI: Name PCI BIOS error code & use FIELD_GET()

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

[-- Attachment #1: Type: text/plain, Size: 3816 bytes --]

On Thu, 26 Oct 2023, Dave Hansen wrote:

> TOn 10/26/23 05:54, Ilpo Järvinen wrote:
> > PCI BIOS returns error code in AH register when carry flag is set. The
> > extraction of the error code is currently set of masking and shifting
> > which makes the code harder to understand than it needs to be.
> 
> That's a really convoluted way of saying that "The error code is in the
> high 8 bits of EAX".  The fact that 'AH' is an alias for the same

"high 8 bits" should odd to me since AH is not exactly at the highest end 
of eax register. But I'll try to figure out something along the lines of 
your suggestion.

> logical thing or that BIOS doesn't fill it in when !CF are rather
> irrelevant to this patch.  In fact, that makes this changelog actively
> confusing because there's no carry flag logic to be seen.  'eax' (the
> variable) universally has a valid error code, it's just zero when
> there's no error.
> 
> > Name the PCI BIOS error code with a define and use FIELD_GET() to
> > access it to improve code readability.
> > 
> > In addition, rely on implicit cast to int and replace zero test
> > with PCIBIOS_SUCCESSFUL.
> 
> It would be really nice if this was something like:
> 
> 	Subject: x86/PCI: Clean up open-coded error code mangling
> 
> That makes it clear that this is a cleanup.  It's kinda obvious from the
> code that it uses FIELD_GET().  You don't need to tell us in the subject.

Okay, I'll try to adapt the rest of the changelog to your suggestions
and make other changes you suggested.

-- 
 i.


> >  arch/x86/pci/pcbios.c | 22 ++++++++++++++++------
> >  1 file changed, 16 insertions(+), 6 deletions(-)
> > 
> > diff --git a/arch/x86/pci/pcbios.c b/arch/x86/pci/pcbios.c
> > index 4f15280732ed..0515e0c05e10 100644
> > --- a/arch/x86/pci/pcbios.c
> > +++ b/arch/x86/pci/pcbios.c
> > @@ -3,6 +3,8 @@
> >   * BIOS32 and PCI BIOS handling.
> >   */
> >  
> > +#include <linux/bits.h>
> > +#include <linux/bitfield.h>
> >  #include <linux/pci.h>
> >  #include <linux/init.h>
> >  #include <linux/slab.h>
> > @@ -29,6 +31,12 @@
> >  #define PCIBIOS_HW_TYPE1_SPEC		0x10
> >  #define PCIBIOS_HW_TYPE2_SPEC		0x20
> >  
> > +/*
> > + * Returned in EAX:
> > + * - AH: return code
> > + */
> > +#define PCIBIOS_RETURN_CODE			GENMASK(15, 8)
> > +
> >  int pcibios_enabled;
> >  
> >  /* According to the BIOS specification at:
> > @@ -154,7 +162,7 @@ static int __init check_pcibios(void)
> >  			: "memory");
> >  		local_irq_restore(flags);
> >  
> > -		status = (eax >> 8) & 0xff;
> > +		status = FIELD_GET(PCIBIOS_RETURN_CODE, eax);
> 
> Nit: This:
> 
> 	FIELD_GET(PCIBIOS_RETURN_CODE, foo)
> 
> pattern is repeated for *EVERY* use of PCIBIOS_RETURN_CODE.  This would
> actually look nicer if you just did a helper like:
> 
> 	static inline u32 get_return_code(u32 eax)
> 	{
> 		return FIELD_GET(PCIBIOS_RETURN_CODE, eax);
> 	}
> 
> although I'm not 100% sure what types you actually want there.
>
> >  		hw_mech = eax & 0xff;
> >  		major_ver = (ebx >> 8) & 0xff;
> >  		minor_ver = ebx & 0xff;
> > @@ -227,7 +235,7 @@ static int pci_bios_read(unsigned int seg, unsigned int bus,
> >  
> >  	raw_spin_unlock_irqrestore(&pci_config_lock, flags);
> >  
> > -	return (int)((result & 0xff00) >> 8);
> > +	return FIELD_GET(PCIBIOS_RETURN_CODE, result);
> >  }
> >  
> >  static int pci_bios_write(unsigned int seg, unsigned int bus,
> > @@ -269,7 +277,7 @@ static int pci_bios_write(unsigned int seg, unsigned int bus,
> >  
> >  	raw_spin_unlock_irqrestore(&pci_config_lock, flags);
> >  
> > -	return (int)((result & 0xff00) >> 8);
> > +	return FIELD_GET(PCIBIOS_RETURN_CODE, result);
> >  }
> 
> For a cleanup like this, it's also nice to add the blurb:
> 
> 	"No functional changes intended."
> 
> Or even (if it's true):
> 
> 	"New code compiles the exact same code as the old code."
>