netdev - Re: [net-next PATCH v4 2/6] net: phy: introduce device_mdiobus

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, 9 Jul 2020 23:39:46 +0300
From:   Andy Shevchenko <andy.shevchenko@...il.com>
To:     Calvin Johnson <calvin.johnson@....nxp.com>
Cc:     Jeremy Linton <jeremy.linton@....com>,
        Russell King - ARM Linux admin <linux@...linux.org.uk>,
        Jon <jon@...id-run.com>,
        Cristi Sovaiala <cristian.sovaiala@....com>,
        Ioana Ciornei <ioana.ciornei@....com>,
        Andrew Lunn <andrew@...n.ch>,
        Florian Fainelli <f.fainelli@...il.com>,
        Madalin Bucur <madalin.bucur@....nxp.com>,
        ACPI Devel Maling List <linux-acpi@...r.kernel.org>,
        linux.cj@...il.com, netdev <netdev@...r.kernel.org>
Subject: Re: [net-next PATCH v4 2/6] net: phy: introduce device_mdiobus_register()

On Thu, Jul 9, 2020 at 8:57 PM Calvin Johnson
<calvin.johnson@....nxp.com> wrote:
>
> Introduce device_mdiobus_register() to register mdiobus
> in cases of either DT or ACPI.

...

> +/**
> + * device_mdiobus_register - bring up all the PHYs on a given bus and
> + * attach them to bus. This handles both DT and ACPI methods.

This is usually one line summary and description goes...

> + * @bus: target mii_bus
> + * @dev: given MDIO device
> + *

...somewhere here.

> + * Returns 0 on success or < 0 on error.

This would be nicer to read as '...or negative error code' or alike.

> + */
> +int device_mdiobus_register(struct mii_bus *bus,
> +                           struct device *dev)
> +{

> +       if (dev->of_node) {
> +               return of_mdiobus_register(bus, dev->of_node);
> +       } else if (dev_fwnode(dev)) {
> +               bus->dev.fwnode = dev_fwnode(dev);
> +               return mdiobus_register(bus);

All these 'else' are redundant, but the main confusion here is the use
of dev_fwnode() vs. dev->of_node.

I would rather see something like

struct fwnode_handle *fwnode = dev_fwnode(dev);
...

if (is_of_node(fwnode))
  return ...(..., to_of_node(fwnode));
if (fwnode) {
  ...
  return ...
}
return -ENODEV;

(Okay, 'else':s may be left if you think it's better to read)

> +       } else {
> +               return -ENODEV;
> +       }
> +}

-- 
With Best Regards,
Andy Shevchenko