netdev - Re: [PATCH 3/3] net: hisilicon: new hip04 ethernet 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: <13743364.WZtdi2BZb3@wuerfel>
Date:	Mon, 24 Mar 2014 16:18:01 +0100
From:	Arnd Bergmann <arnd@...db.de>
To:	linux-arm-kernel@...ts.infradead.org
Cc:	Zhangfei Gao <zhangfei.gao@...aro.org>, davem@...emloft.net,
	linux@....linux.org.uk, f.fainelli@...il.com,
	sergei.shtylyov@...entembedded.com, mark.rutland@....com,
	netdev@...r.kernel.org, devicetree@...r.kernel.org
Subject: Re: [PATCH 3/3] net: hisilicon: new hip04 ethernet driver

On Monday 24 March 2014 22:14:56 Zhangfei Gao wrote:

> +
> +static void hip04_tx_reclaim(struct net_device *ndev, bool force)
> +{
> +	struct hip04_priv *priv = netdev_priv(ndev);
> +	unsigned tx_head = priv->tx_head;
> +	unsigned tx_tail = priv->tx_tail;
> +	struct tx_desc *desc = &priv->tx_desc[priv->tx_tail];
> +
> +	while (tx_tail != tx_head) {
> +		if (desc->send_addr != 0) {
> +			if (force)
> +				desc->send_addr = 0;
> +			else
> +				break;
> +		}
> +		if (priv->tx_phys[tx_tail]) {
> +			dma_unmap_single(&ndev->dev, priv->tx_phys[tx_tail],
> +				priv->tx_skb[tx_tail]->len, DMA_TO_DEVICE);
> +			priv->tx_phys[tx_tail] = 0;
> +		}
> +		dev_kfree_skb_irq(priv->tx_skb[tx_tail]);
> +		priv->tx_skb[tx_tail] = NULL;
> +		tx_tail = TX_NEXT(tx_tail);
> +		priv->tx_count--;
> +	}
> +	priv->tx_tail = tx_tail;
> +}

I think you still need to find a solution to ensure that the tx reclaim is
called eventually through a method other than start_xmit.

> +
> +	priv->map = syscon_regmap_lookup_by_compatible("hisilicon,hip04-ppe");
> +	if (IS_ERR(priv->map)) {
> +		dev_warn(d, "no hisilicon,hip04-ppe\n");
> +		ret = PTR_ERR(priv->map);
> +		goto init_fail;
> +	}
> +
> +	n = of_parse_phandle(node, "port-handle", 0);
> +	if (n) {
> +		ret = of_property_read_u32(n, "reg", &priv->port);
> +		if (ret) {
> +			dev_warn(d, "no reg info\n");
> +			goto init_fail;
> +		}
> +
> +		ret = of_property_read_u32(n, "channel", &priv->chan);
> +		if (ret) {
> +			dev_warn(d, "no channel info\n");
> +			goto init_fail;
> +		}
> +	} else {
> +		dev_warn(d, "no port-handle\n");
> +		ret = -EINVAL;
> +		goto init_fail;
> +	}

Doing the lookup by "compatible" string doesn't really help you
at solve the problem of single-instance ppe at all, because that
function will only ever look at the first one. Use
syscon_regmap_lookup_by_phandle instead and pass the phandle
you get from the "port-handle" property.

Also, since you decided to treat the ppe as a dump regmap, I would
recommend moving the 'reg' and 'channel' properties into arguments
of the port-handle link, and retting rid of the child nodes of
the ppe, like:

+       ppe: ppe@...0000 {
+               compatible = "hisilicon,hip04-ppe", "syscon";
+               reg = <0x28c0000 0x10000>;
+       };
+
+       fe: ethernet@...0000 {
+               compatible = "hisilicon,hip04-mac";
+               reg = <0x28b0000 0x10000>;
+               interrupts = <0 413 4>;
+               phy-mode = "mii";
+               port-handle = <&ppe 0xf1 0>;
+       };


	Arnd
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html