linux-kernel - Re: [PATCH v1 3/7] net/fsl: add ACPI support for mdio bus

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:   Fri, 31 Jan 2020 18:08:58 +0200
From:   Andy Shevchenko <andy.shevchenko@...il.com>
To:     Calvin Johnson <calvin.johnson@....com>
Cc:     linux.cj@...il.com, Jon Nettleton <jon@...id-run.com>,
        Russell King - ARM Linux <linux@...linux.org.uk>,
        Makarand Pawagi <makarand.pawagi@....com>,
        cristian.sovaiala@....com, laurentiu.tudor@....com,
        ioana.ciornei@....com, V.Sethi@....com, pankaj.bansal@....com,
        "Rajesh V . Bikkina" <rajesh.bikkina@....com>,
        Calvin Johnson <calvin.johnson@....nxp.com>,
        "David S. Miller" <davem@...emloft.net>,
        Madalin Bucur <madalin.bucur@....nxp.com>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        netdev <netdev@...r.kernel.org>
Subject: Re: [PATCH v1 3/7] net/fsl: add ACPI support for mdio bus

On Fri, Jan 31, 2020 at 5:37 PM Calvin Johnson <calvin.johnson@....com> wrote:
>
> From: Calvin Johnson <calvin.johnson@....nxp.com>
>
> Add ACPI support for MDIO bus registration while maintaining
> the existing DT support.

...

> -       ret = of_address_to_resource(np, 0, &res);
> -       if (ret) {
> +       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +       if (!res) {
>                 dev_err(&pdev->dev, "could not obtain address\n");
> -               return ret;
> +               return -ENODEV;
>         }

...

> -       snprintf(bus->id, MII_BUS_ID_SIZE, "%llx", (unsigned long long)res.start);
> +       snprintf(bus->id, MII_BUS_ID_SIZE, "%llx",
> +                (unsigned long long)res->start);

Why this has been touched?

...

> -       priv->mdio_base = of_iomap(np, 0);
> +       priv->mdio_base = devm_ioremap_resource(&pdev->dev, res);
>         if (!priv->mdio_base) {

Are you sure the check is correct now?

>                 ret = -ENOMEM;
>                 goto err_ioremap;
>         }

...

>
> -       priv->is_little_endian = of_property_read_bool(pdev->dev.of_node,
> -                                                      "little-endian");
> -
> -       priv->has_a011043 = of_property_read_bool(pdev->dev.of_node,
> -                                                 "fsl,erratum-a011043");
> -
> -       ret = of_mdiobus_register(bus, np);
> -       if (ret) {
> -               dev_err(&pdev->dev, "cannot register MDIO bus\n");

> +       if (is_of_node(pdev->dev.fwnode)) {

> +       } else if (is_acpi_node(pdev->dev.fwnode)) {

Oh, no, this is wrong. Pure approach AFAICS is to use fwnode API or
device property API.

And actually what you need to include is rather <linux/property.h>,
and not acpi.h.

> +       } else {
> +               dev_err(&pdev->dev, "Cannot get cfg data from DT or ACPI\n");
> +               ret = -ENXIO;
>                 goto err_registration;
>         }

> +static const struct acpi_device_id xgmac_mdio_acpi_match[] = {
> +       {"NXP0006", 0}

How did you test this on platforms with the same IP and without device
 of this ACPI ID present?

(Hint: missed terminator)

> +};
> +MODULE_DEVICE_TABLE(acpi, xgmac_mdio_acpi_match);

> +               .acpi_match_table = ACPI_PTR(xgmac_mdio_acpi_match),

ACPI_PTR is not needed otherwise you will get a compiler warning.

-- 
With Best Regards,
Andy Shevchenko