linux-kernel - Re: [PATCH V4 3/6] misc/pvpanic: add API for pvpanic driver framework

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:   Thu, 24 Jan 2019 15:54:11 +0200
From:   Andy Shevchenko <andy.shevchenko@...il.com>
To:     Peng Hao <peng.hao2@....com.cn>
Cc:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Arnd Bergmann <arnd@...db.de>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH V4 3/6] misc/pvpanic: add API for pvpanic driver framework

On Thu, Jan 24, 2019 at 10:17 AM Peng Hao <peng.hao2@....com.cn> wrote:
>
> Add pvpanic_add/remove_device API. Follow-up patches will use them to
> add/remove specific drivers into framework.

I'm not sure this is the best way to instantiate the drivers.

Code with platfrom_device_add*() is related to platform which
instantiates the necessary set of the drivers.
In case of OF it should be done in Device Tree, in ACPI case you would
have some device description for that in DSDT table.

> +int pvpanic_add_device(struct device *dev, struct resource *res)
> +{
> +       struct platform_device *pdev;
> +       int ret;
> +
> +       pdev = platform_device_alloc("pvpanic", -1);
> +       if (!pdev)
> +               return -ENOMEM;
> +
> +       pdev->dev.parent = dev;
> +
> +       ret = platform_device_add_resources(pdev, res, 1);
> +       if (ret)
> +               goto err;
> +
> +       ret = platform_device_add(pdev);
> +       if (ret)
> +               goto err;
> +       pvpanic_data.pdev = pdev;
> +
> +       return 0;
> +err:
> +       platform_device_put(pdev);

> +       return -1;

It should return proper %-ERRNO code. I think to achieve this the
following can be used

return ret;

> +}

>  static int pvpanic_platform_probe (struct platform_device *pdev)
>  {
>         struct device *dev = &pdev->dev;
>         struct resource *res;
> +       void __iomem *base;
>
>         res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>         if (res) {
> @@ -59,8 +96,10 @@ static int pvpanic_platform_probe (struct platform_device *pdev)
>                 base = ioport_map(res->start, resource_size(res));
>                 if (!base)
>                         return -ENODEV;

> +               pvpanic_data.is_ioport = true;

You better allocate this on a heap and put as a pointer to the device
driver private data.

>          }
>
> +       pvpanic_data.base = base;

Ditto for entire pvpanic_data instance.

-- 
With Best Regards,
Andy Shevchenko