| 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
| ||
|
Date: Tue, 4 Feb 2020 07:18:24 +0000
From: "Calvin Johnson (OSS)" <calvin.johnson@....nxp.com>
To: Andy Shevchenko <andy.shevchenko@...il.com>
CC: "linux.cj@...il.com" <linux.cj@...il.com>,
Jon Nettleton <jon@...id-run.com>,
Russell King - ARM Linux <linux@...linux.org.uk>,
Makarand Pawagi <makarand.pawagi@....com>,
Cristi Sovaiala <cristian.sovaiala@....com>,
Laurentiu Tudor <laurentiu.tudor@....com>,
Ioana Ciornei <ioana.ciornei@....com>,
Varun Sethi <V.Sethi@....com>,
Pankaj Bansal <pankaj.bansal@....com>,
"Rajesh V. Bikkina" <rajesh.bikkina@....com>,
"Calvin Johnson (OSS)" <calvin.johnson@....nxp.com>,
"David S. Miller" <davem@...emloft.net>,
"Madalin Bucur (OSS)" <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
> -----Original Message-----
> From: Andy Shevchenko <andy.shevchenko@...il.com>
> 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?
Without this change, I get:
---------------------------------------------------------
drivers/net/ethernet/freescale/xgmac_mdio.c: In function 'xgmac_mdio_probe':
drivers/net/ethernet/freescale/xgmac_mdio.c:269:27: error: request for member 'start' in something not a structure or union
(unsigned long long)res.start);
^
scripts/Makefile.build:265: recipe for target 'drivers/net/ethernet/freescale/xgmac_mdio.o' failed
make[4]: *** [drivers/net/ethernet/freescale/xgmac_mdio.o] Error 1
---------------------------------------------------------
On checking other files that calls platform_get_resource, I can see that this is the way they refer 'start'.
>
> ...
>
> > - 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?
devm_ioremap_resource returns non-NULL error values. So, this doesn't look right.
I'll work on it for v2.
> > 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.
Understood. I had got some issues while using fwnode API to handle DT case due to which
DT/ACPI checks were done and both are handled separately. Let me see if I can root cause it.
>
> > + } 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?
I didn't test it on any other platforms other than LX2160ARDB. AFAIU, without
device of this ACPI ID present, the driver won't get probed.
> (Hint: missed terminator)
static const struct acpi_device_id xgmac_mdio_acpi_match[] = {
{ "NXP0006", 0 },
{ }
};
Is this what you meant?
>
> > +};
> > +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.
No compiler warning was observed in both cases.
I can see other drivers using this macro.
drivers/net/ethernet/apm/xgene-v2/main.c:734: .acpi_match_table = ACPI_PTR(xge_acpi_match),
drivers/net/ethernet/apm/xgene/xgene_enet_main.c:2172: .acpi_match_table = ACPI_PTR(xgene_enet_acpi_match),
drivers/net/ethernet/hisilicon/hns/hns_enet.c:2445: .acpi_match_table = ACPI_PTR(hns_enet_acpi_match),
drivers/net/ethernet/hisilicon/hns_mdio.c:566: .acpi_match_table = ACPI_PTR(hns_mdio_acpi_match),
drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c:5997: .acpi_match_table = ACPI_PTR(mvpp2_acpi_match),
drivers/net/ethernet/qualcomm/emac/emac.c:766: .acpi_match_table = ACPI_PTR(emac_acpi_match),
drivers/net/ethernet/smsc/smsc911x.c:2667: .acpi_match_table = ACPI_PTR(smsc911x_acpi_match),
drivers/net/ethernet/socionext/netsec.c:2187: .acpi_match_table = ACPI_PTR(netsec_acpi_ids),
drivers/net/phy/mdio-xgene.c:456: .acpi_match_table = ACPI_PTR(xgene_mdio_acpi_match),
Thanks
Calvin
Powered by blists - more mailing lists