On Dec 10, 2007 8:48 PM, Eric W. Biederman <[email protected]> wrote:
> Neil Horman <[email protected]> writes:
>
> Almost there.
>
>
>
> > On Mon, Dec 10, 2007 at 06:08:03PM -0700, Eric W. Biederman wrote:
> >> Neil Horman <[email protected]> writes:
> >>
> > <snip>
> >>
> >> Ok. This test is broken. Please remove the == 1. You are looking
> >> for == (1 << 18). So just saying: "if (htcfg & (1 << 18))" should be clearer.
> >>
> > Fixed. Thanks!
> >
> >> > + printk(KERN_INFO "Detected use of extended apic ids on hypertransport
> > bus\n");
> >> > + if ((htcfg & (1 << 17)) == 0) {
> >> > + printk(KERN_INFO "Enabling hypertransport extended apic interrupt
> >> > broadcast\n");
> >> > + htcfg |= (1 << 17);
> >> > + write_pci_config(num, slot, func, 0x68, htcfg);
> >> > + }
> >> > + }
> >> > +
> >> > +}
> >>
> >> The rest of this quirk looks fine, include the fact it is only intended
> >> to be applied to PCI_VENDOR_ID_AMD PCI_DEVICE_ID_AMD_K8_NB.
> >>
> > Copy that.
> >
> >>
> >> For what is below I don't like the way the infrastructure has been
> >> extended as what you are doing quickly devolves into a big mess.
> >>
> >> Please extend struct chipset to be something like:
> >> struct chipset {
> >> u16 vendor;
> >> u16 device;
> >> u32 class, class_mask;
> >> void (*f)(void);
> >> };
> >>
> >> And then the test for matching the chipset can be something like:
> >> if ((id->vendor == PCI_ANY_ID || id->vendor == dev->vendor) &&
> >> (id->device == PCI_ANY_ID || id->device == dev->device) &&
> >> !((id->class ^ dev->class) & id->class_mask))
> >>
> >> Essentially a subset of pci_match_one_device from drivers/pci/pci.h
> >>
> >> That way you don't need to increase the number of tables or the
> >> number of passes through the pci busses, just update the early_qrk
> >> table with a few more bits of information.
> >>
> > copy that. Fixed. Thanks!
> >
> >> The extended form should be much more maintainable in the long
> >> run. Given that we may want this before we enable the timer
> >> which is very early doing this in the pci early quirks seems
> >> to make sense.
> >>
> >> Eric
> >
> >
> > New patch attached, with suggestions incorporated.
> >
> > Thanks & regards
> > Neil
> >
> > Signed-off-by: Neil Horman <[email protected]>
> >
> >
> > early-quirks.c | 82 ++++++++++++++++++++++++++++++++++++++++++++++++++-------
> > 1 file changed, 73 insertions(+), 9 deletions(-)
> >
> >
> >
> > diff --git a/arch/x86/kernel/early-quirks.c b/arch/x86/kernel/early-quirks.c
> > index 88bb83e..4b0cee1 100644
> > --- a/arch/x86/kernel/early-quirks.c
> > +++ b/arch/x86/kernel/early-quirks.c
> > @@ -44,6 +44,50 @@ static int __init nvidia_hpet_check(struct acpi_table_header
> > *header)
> > #endif /* CONFIG_X86_IO_APIC */
> > #endif /* CONFIG_ACPI */
> >
> > +static void __init fix_hypertransport_config(int num, int slot, int func)
> > +{
> > + u32 htcfg;
> > + /*
> > + *we found a hypertransport bus
> > + *make sure that are broadcasting
> > + *interrupts to all cpus on the ht bus
> > + *if we're using extended apic ids
> > + */
> > + htcfg = read_pci_config(num, slot, func, 0x68);
> > + if (htcfg & (1 << 18)) {
> > + printk(KERN_INFO "Detected use of extended apic ids on hypertransport bus\n");
> > + if ((htcfg & (1 << 17)) == 0) {
> > + printk(KERN_INFO "Enabling hypertransport extended apic interrupt
> > broadcast\n");
> > + htcfg |= (1 << 17);
> > + write_pci_config(num, slot, func, 0x68, htcfg);
> > + }
> > + }
> > +
> > +}
> > +
> > +static void __init check_hypertransport_config()
> > +{
> > + int num, slot, func;
> > + u32 device, vendor;
> > + func = 0;
> > + for (num = 0; num < 32; num++) {
> > + for (slot = 0; slot < 32; slot++) {
> > + vendor = read_pci_config(num,slot,func,
> > + PCI_VENDOR_ID);
> > + device = read_pci_config(num,slot,func,
> > + PCI_DEVICE_ID);
> > + vendor &= 0x0000ffff;
> > + device >>= 16;
> > + if ((vendor == PCI_VENDOR_ID_AMD) &&
> > + (device == PCI_DEVICE_ID_AMD_K8_NB))
> > + fix_hypertransport_config(num,slot,func);
> > + }
> > + }
> > +
> > + return;
> > +
> > +}
>
> We should not need check_hypertransport_config as the generic loop
> now does the work for us.
> > +
> > static void __init nvidia_bugs(void)
> > {
> > #ifdef CONFIG_ACPI
> > @@ -83,15 +127,25 @@ static void __init ati_bugs(void)
> > #endif
> > }
> >
> > +static void __init amd_host_bugs(void)
> > +{
> > + printk(KERN_CRIT "IN AMD_HOST_BUGS\n");
> > + check_hypertransport_config();
> > +}
>
> Likewise this function is unneeded and the printk is likely confusing
> for users.
>
> > struct chipset {
> > u16 vendor;
> > + u16 device;
> > + u32 class;
> > + u32 class_mask;
> > void (*f)(void);
> > };
> >
> > static struct chipset early_qrk[] __initdata = {
> > - { PCI_VENDOR_ID_NVIDIA, nvidia_bugs },
> > - { PCI_VENDOR_ID_VIA, via_bugs },
> > - { PCI_VENDOR_ID_ATI, ati_bugs },
> > + { PCI_VENDOR_ID_NVIDIA, PCI_ANY_ID, PCI_CLASS_BRIDGE_PCI, PCI_ANY_ID,
> > nvidia_bugs },
> > + { PCI_VENDOR_ID_VIA, PCI_ANY_ID, PCI_CLASS_BRIDGE_PCI, PCI_ANY_ID, via_bugs },
> > + { PCI_VENDOR_ID_ATI, PCI_ANY_ID, PCI_CLASS_BRIDGE_PCI, PCI_ANY_ID, ati_bugs },
> > + { PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_K8_NB, PCI_CLASS_BRIDGE_HOST,
> > PCI_ANY_ID, amd_host_bugs },
> > {}
> So make that fix_hypertransport_config and we should be good.
Agreed.
struct chipset {
u16 vendor;
u16 device;
u32 class;
+ u32 class_mask;
void (*f)(void); ============> int (*f) (int num, int slot, int func);
};
> > };
> >
> > @@ -108,25 +162,35 @@ void __init early_quirks(void)
> > for (func = 0; func < 8; func++) {
> > u32 class;
> > u32 vendor;
> > + u32 device;
> > u8 type;
> > int i;
> > +
> > class = read_pci_config(num,slot,func,
> > PCI_CLASS_REVISION);
> > if (class == 0xffffffff)
> > break;
> >
> > - if ((class >> 16) != PCI_CLASS_BRIDGE_PCI)
> > - continue;
> > + class >>= 16;
>
> >
> > vendor = read_pci_config(num, slot, func,
> > PCI_VENDOR_ID);
> > vendor &= 0xffff;
> >
> > - for (i = 0; early_qrk[i].f; i++)
> > - if (early_qrk[i].vendor == vendor) {
> > - early_qrk[i].f();
> > - return;
> > + device = read_pci_config(num, slot, func,
> > + PCI_DEVICE_ID);
> > + device >>= 16;
>
> We don't need to shift device. Although we can do:
> device_vendor = read_pci_config(num, slot, func, PCI_VENDOR_ID);
> device = device_vendor >> 16;
> vendor = device_vendor & 0xffff;
>
> Assuming the early read_pci_config is limited to 32bit reads.
>
>
> > + for(i=0;early_qrk[i].f != NULL;i++) {
> > + if (((early_qrk[i].vendor == PCI_ANY_ID) ||
> > + (early_qrk[i].vendor == vendor)) &&
> > + ((early_qrk[i].device == PCI_ANY_ID) ||
> > + (early_qrk[i].device == device)) &&
> > + ((early_qrk[i].class ^ class) &
> > + early_qrk[i].class_mask)) {
> > + early_qrk[i].f();
> > }
> > + early_qrk[i].f();
===>
int status;
status = early_qrk[i].f(num, slot, bus);
if (status == 1)
break;
else if (status == 2)
return;
YH
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[Index of Archives]
[Kernel Newbies]
[Netfilter]
[Bugtraq]
[Photo]
[Stuff]
[Gimp]
[Yosemite News]
[MIPS Linux]
[ARM Linux]
[Linux Security]
[Linux RAID]
[Video 4 Linux]
[Linux for the blind]
[Linux Resources]
