lkml.org 
[lkml]   [2009]   [Feb]   [4]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [RFC] [PATCH] PCI mmconfig without ACPI

    * Ed Swierk <eswierk@aristanetworks.com> wrote:

    > Make it possible to use memory-mapped PCI configuration space on systems
    > with a supported PCI host bridge without CONFIG_ACPI.
    >
    > The acpi_mcfg_allocation struct serves double duty, as a template for
    > parsing the ACPI MCFG table and also to store the mmconfig data, which
    > doesn't necessarily come from ACPI. Should I leave the struct in
    > acpi/actbl1.h for ACPI parsing, and create a new one for storing mmconfig
    > data?

    ok, that's certainly a nice cleanup and restructuring of this code.

    A few comments:

    > config PCI_MMCONFIG
    > def_bool y
    > - depends on X86_32 && PCI && ACPI && (PCI_GOMMCONFIG || PCI_GOANY)
    > + depends on X86_32 && PCI && (PCI_GOMMCONFIG || PCI_GOANY)

    ( nice - increasing a PCI feature's reach and decoupling it from hardware
    enumeration methods such as ACPI is always good news! )

    > +#ifdef CONFIG_ACPI
    > +
    > static acpi_status __init check_mcfg_resource(struct acpi_resource *res,
    > void *data)

    An even cleaner approach would be to create a new file:

    arch/x86/pci/mmconfig-acpi.c, and move this block of 5 functions there - and
    add a obj-$(CONFIG_ACPI) rule to arch/x86/pci/Makefile to build it.

    The interfacing to arch/x86/pci/mmconfig-shared.c could be simplified too,
    instead of this two-pass thing:

    if (!known_bridge) {
    acpi_table_parse(ACPI_SIG_MCFG, acpi_parse_mcfg);
    pci_mmcfg_reject_broken(early);
    }

    a single:

    if (!known_bridge)
    pci_detect_acpi_mmcfg(early);

    interface could be used. In the !CONFIG_ACPI case this interface would be an
    inline do-nothing wrapper, in a pci x86 header file:

    static inline void pci_detect_acpi_mmcfg(int early) { }

    A few currently local symbols in the file would have to be made explicit and
    moved into the header - but it should be rather straightforward i think.

    That way we avoid the ugly #ifdef and clean up the general code structure
    and modularization a bit.

    this #ifdef would go away as well:

    > +#ifdef CONFIG_ACPI
    > if (!known_bridge) {
    > acpi_table_parse(ACPI_SIG_MCFG, acpi_parse_mcfg);
    > pci_mmcfg_reject_broken(early);
    > }
    > +#endif


    > +#ifdef CONFIG_PCI_MMCONFIG
    > +
    > +struct acpi_mcfg_allocation {
    > + u64 address; /* Base address, processor-relative */
    > + u16 pci_segment; /* PCI segment group number */
    > + u8 start_bus_number; /* Starting PCI Bus number */
    > + u8 end_bus_number; /* Final PCI Bus number */
    > + u32 reserved;
    > +};

    Please rename this to "struct pci_mcfg_allocation" - there's nothing ACPI
    about it anymore - mmcfg is a PCI feature and ACPI is an enumeration method.

    Also, while touching it, please also use the opportunity to align structure
    fields vertically:

    struct pci_mcfg_allocation {
    u64 address; /* Base address, processor-relative */
    u16 pci_segment; /* PCI segment group number */
    u8 start_bus_number; /* Starting PCI Bus number */
    u8 end_bus_number; /* Final PCI Bus number */
    u32 __reserved;
    };

    The whole layout of this structure becomes easier to read and nicer to look
    at as well.

    Another small detail: note how i renamed reserved to __reserved - that is a
    standard way to de-emphasise the signficance of a structure field.

    The reserved field there is for future expansion and to pad the structure to
    16 bytes - it doesnt really mean much and the underscores move it a bit out
    of the default line of sight.

    With a 'reserved' field people end up wondering whether it's perhaps some
    _semantic_ 'reserved area' kind of thing (like for e820 maps, etc.) - so
    it's never bad to make that distinction explicit via the double underscores.

    But again, nice patch and it would be nice to see this concept hit mainline.

    Ingo


    \
     
     \ /
      Last update: 2009-02-04 19:21    [W:0.027 / U:6.008 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site