linux-kernel - Re: [PATCH][V2] gpio: add a driver for GPIO pins found on AMD-8111 south bridge chips

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] [day] [month] [year] [list]

Date:	Mon, 28 May 2012 21:40:44 +0400
From:	Dmitry Eremin-Solenikov <dbaryshkov@...il.com>
To:	Linus Walleij <linus.walleij@...aro.org>
Cc:	Grant Likely <grant.likely@...retlab.ca>,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH][V2] gpio: add a driver for GPIO pins found on AMD-8111
 south bridge chips

Hello,

On Mon, May 28, 2012 at 8:19 PM, Linus Walleij <linus.walleij@...aro.org> wrote:
> On Mon, May 28, 2012 at 2:48 PM, Dmitry Eremin-Solenikov
> <dbaryshkov@...il.com> wrote:
>
>> Add a driver to use GPIO pins available on several AMD south bridges
>> (currently only AMD 8111 is supported).
>>
>> Signed-off-by: Dmitry Eremin-Solenikov <dbaryshkov@...il.com>
>> ---
>>
>> Changes since V1:
>> * Replaced magic numbers in register access with named values
>
> This is looking a lot better, some nitpicking:

Oh, my, I forgot to change module_init/module_exit function names.
So anyway I'll have to send V3

>
>> +#define AMD_GPIO_MODE_ALTFN    0x08 /* Or 0x09 */
>
> Hm Hm I wonder what this register does... looks a lot like a mux.
> (Just philisophizing...)

As the name says, it is an "alternate function". Some (most) of these pins
can have alternative functions like "clock output", IRQ, SMBus, etc. Setting
this ALTFN mode enables special logic for that pin.

>
>> +static int __init mod_init(void)
>> +{
>> +       int err = -ENODEV;
>> +       struct pci_dev *pdev = NULL;
>> +       const struct pci_device_id *ent;
>> +
>> +       for_each_pci_dev(pdev) {
>> +               ent = pci_match_id(pci_tbl, pdev);
>> +               if (ent)
>> +                       goto found;
>
> It's not like I know how PCI abstractions really work, but what happens
> here if there would be two instances of the device? It looks like you will
> only find the first?

Hmm. I don't know a system with two amd 8111 south bridges in it.
This part is mostly a c&p from amd hw_random driver.

>
>> +       }
>> +       /* Device not found. */
>> +       goto out;
>
> Can't you just return -ENODEV here? Why jump around...

OK.

>
> Now as I said I'm not a PCI expert, but isn't the proper way to do this
> to use this stuff:
>
> static struct pci_driver amd_gpio_driver = {
>        .name =         "amd-gpio",
>        .id_table =     pci_ids,
>        .probe =        amd_gpio_probe,
>        .remove =       __devexit_p(amd_gpio_remove),
> };
>
> static int __init amd_gpio_init(void)
> {
>        return pci_register_driver(&amd_gpio_driver);
> }
>
> static void __exit amd_gpio_exit(void)
> {
>        pci_unregister_driver(&amd_gpio_driver);
> }
>
> module_init(amd_gpio_init);
> module_exit(amd_gpio_exit);
>
> Then start working from the probe function insteaf of go and
> iterate over the PCI bus yourself? I was thinking there was a
> reason for but couldn't find any.

It would be the usual way. There is however a problem with that way.
The System Management devices (that the driver looks for):

1) Is a kind of placeholder. If you check, the driver get's the IO address
    not via usual pci_resource_start()/_end(), which end up with the data
    from BARs from pci device, but does special register access magic

2) Is a multifunction device. The IO space that is provided by the device
   is used for HW random generator, I2C (well, SMBus) controller, now
   the GPIO pins driver. Also it covers some power management/SMI/SCI
   magic, which usually nobody should touch. So, in the ideal world,
   we should register an MFD driver over that PCI device and then
   register all other child sub-devices. However this looks like a
huger overkill
   to me. In the end, it's just a GPIO part of south bridge, which should
   not be touched by normal people :)

-- 
With best wishes
Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/