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."
>
