linux-kernel - Re: [PATCH v7] i2c: Add PCI and platform drivers for the AMD MP2 I2C controller

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Sun, 4 Nov 2018 13:43:36 -0300
From:   Elie Morisse <syniurge@...il.com>
To:     helgaas@...nel.org
Cc:     linux-i2c@...r.kernel.org, Wolfram Sang <wsa@...-dreams.de>,
        Nehal-bakulchandra.Shah@....com, Shyam-sundar.S-k@....com,
        sandeep.singh@....com, linux-kernel@...r.kernel.org,
        rjw@...ysocki.net, lenb@...nel.org, linux-acpi@...r.kernel.org
Subject: Re: [PATCH v7] i2c: Add PCI and platform drivers for the AMD MP2 I2C controller

Le mar. 30 oct. 2018 à 17:56, Bjorn Helgaas <helgaas@...nel.org> a écrit :
> I'm dubious about this two-driver structure.  If I understand
> correctly (and it's very possible that I don't), the PCI driver
> (amd_mp2_pci_probe()) is the real owner of the i2c adapter: it
> claims the PCI device, claims its BARs, and requests an IRQ.
>
> The i2c_amd_probe() code *looks* like a platform driver that claims
> AMDI0011 devices from the ACPI namespace, but I don't think it's
> really a driver.  It looks like it exists mainly to extract some
> information (bus speed and maybe a bus number?) from the namespace,
> then to call i2c_add_adapter().
>
> It looks like i2c_amd_probe() must run *after* amd_mp2_pci_probe(),
> but there's no way to really enforce that ordering.
>
> And i2c-amd-plat-mp2 contains the i2c_amd_algorithm functions, which
> operate on the PCI device, which requires exported interfaces
> (amd_mp2_read(), amd_mp2_write()) that are implemented in the PCI
> driver but called from the platform part.

> It seems like there should be a way to put the ACPI lookups into
> i2c-amd-pci-mp2 so there's only one driver.

I merged the two drivers into one module in v8 I just submitted, but this
doesn't enforce that the platform probe occur after the PCI probe.

(and from looking at the drivers/acpi/ code _DEP doesn't seem to help)

> _DEP is for operation region dependencies.  I don't know enough about
> ACPI to know why you're using it to find the PCI device related to an
> ACPI device.  That doesn't really seem like an op region thing.

For the Yoga 530 that seems to be the only available hint at which MP2
device those ACPI devices are related to.

I attached the DSDT of the Yoga 530-14ARR
(also pasted here: https://paste.kde.org/pjobniftq)

Relevant parts are:

    Scope (_SB)
    {
        Device (PCI0)
        {
// ....
            Device (GP17)
            {
// ...
                Device (MP2C)
                {
                    Name (_ADR, 0x07)  // _ADR: Address
                }
// ...
            }
        }
    }

    Scope (_SB)
    {
        Device (I2CA)
        {
            Name (_HID, "AMDI0011")  // _HID: Hardware ID
            Name (_UID, Zero)  // _UID: Unique ID
            Name (_ADR, Zero)  // _ADR: Address
            Name (_CRS, ResourceTemplate ()  // _CRS: Current Resource Settings
            {
                IRQ (Edge, ActiveHigh, Exclusive, )
                    {10}
                Memory32Fixed (ReadWrite,
                    0xFEDC2000,         // Address Base
                    0x00001000,         // Address Length
                    )
            })
            Name (_DEP, Package (0x01)  // _DEP: Dependencies
            {
                ^PCI0.GP17.MP2C
            })
            Method (_STA, 0, NotSerialized)  // _STA: Status
            {
                Return (0x0F)
            }

            Method (RSET, 0, NotSerialized)
            {
                SRAD (0x05, 0xC8)
            }
        }

        Device (I2CB)
        {
            Name (_HID, "AMDI0011")  // _HID: Hardware ID
            Name (_UID, One)  // _UID: Unique ID
            Name (_ADR, One)  // _ADR: Address
            Name (_CRS, ResourceTemplate ()  // _CRS: Current Resource Settings
            {
                IRQ (Edge, ActiveHigh, Exclusive, )
                    {11}
                Memory32Fixed (ReadWrite,
                    0xFEDC3000,         // Address Base
                    0x00001000,         // Address Length
                    )
            })
            Name (_DEP, Package (0x01)  // _DEP: Dependencies
            {
                ^PCI0.GP17.MP2C
            })
            Method (_STA, 0, NotSerialized)  // _STA: Status
            {
                Return (0x0F)
            }

            Method (RSET, 0, NotSerialized)
            {
                SRAD (0x06, 0xC8)
            }
        }
    }

View attachment "dsdt.dsl" of type "text/x-dsl" (268997 bytes)