linux-kernel - Re: R: [PATCH v3 1/2] net: mdio: add ipq8064 mdio driver

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]

Message-ID: <20200221115248.GH25745@shell.armlinux.org.uk>
Date:   Fri, 21 Feb 2020 11:52:48 +0000
From:   Russell King - ARM Linux admin <linux@...linux.org.uk>
To:     ansuelsmth@...il.com
Cc:     'Andy Gross' <agross@...nel.org>,
        'Bjorn Andersson' <bjorn.andersson@...aro.org>,
        "'David S. Miller'" <davem@...emloft.net>,
        'Rob Herring' <robh+dt@...nel.org>,
        'Mark Rutland' <mark.rutland@....com>,
        'Andrew Lunn' <andrew@...n.ch>,
        'Florian Fainelli' <f.fainelli@...il.com>,
        'Heiner Kallweit' <hkallweit1@...il.com>,
        linux-arm-msm@...r.kernel.org, netdev@...r.kernel.org,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: R: [PATCH v3 1/2] net: mdio: add ipq8064 mdio driver

On Fri, Feb 21, 2020 at 01:49:17AM +0100, ansuelsmth@...il.com wrote:
> > On Fri, Feb 21, 2020 at 12:26:21AM +0100, Ansuel Smith wrote:
> > > +static int
> > > +ipq8064_mdio_probe(struct platform_device *pdev)
> > > +{
> > > +	struct device_node *np = pdev->dev.of_node;
> > > +	struct ipq8064_mdio *priv;
> > > +	struct mii_bus *bus;
> > > +	int ret;
> > > +
> > > +	bus = devm_mdiobus_alloc_size(&pdev->dev, sizeof(*priv));
> > > +	if (!bus)
> > > +		return -ENOMEM;
> > > +
> > > +	bus->name = "ipq8064_mdio_bus";
> > > +	bus->read = ipq8064_mdio_read;
> > > +	bus->write = ipq8064_mdio_write;
> > > +	snprintf(bus->id, MII_BUS_ID_SIZE, "%s-mii", dev_name(&pdev-
> > >dev));
> > > +	bus->parent = &pdev->dev;
> > > +
> > > +	priv = bus->priv;
> > > +	priv->base = syscon_node_to_regmap(np);
> > > +	if (IS_ERR_OR_NULL(priv->base)) {
> > > +		priv->base = syscon_regmap_lookup_by_phandle(np,
> > "master");
> > > +		if (IS_ERR_OR_NULL(priv->base)) {
> > > +			dev_err(&pdev->dev, "master phandle not
> > found\n");
> > > +			return -EINVAL;
> > > +		}
> > > +	}
> > 
> > I'm curious why you've kept this as-is given my comments?
> > 
> > If you don't agree with them, it would be helpful to reply to the
> > review email giving the reasons why.
>
> I read your command and now I understand what you mean. Since they both
> never return NULL the IS_ERR_OR_NULL is wrong and only IS_ERR should be
> used. Correct me if I'm wrong.
> About the error propagation, should I return the
> syscon_regmap_lookup_by_phandle
> error or I can keep the EINVAL error? 

Hi,

You probably want something like:

	priv->base = syscon_node_to_regmap(np);
	if (IS_ERR(priv->base) && priv->base != ERR_PTR(-EPROBE_DEFER))
		priv->base = syscon_regmap_lookup_by_phandle(np, "master");

	if (priv->base == ERR_PTR(-EPROBE_DEFER)) {
		return -EPROBE_DEFER;
	} else if (IS_ERR(priv->base)) {
		dev_err(&pdev->dev, "error getting syscon regmap, error=%pe\n",
			priv->base);
		return PTR_ERR(priv->base);
	}

Please ensure that you test the above, including the case where you
should fall through to syscon_regmap_lookup_by_phandle().

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up