netdev - Re: [PATCH 3/3] net: hisilicon: Add Fast Ethernet MAC 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: <4004629.ilZU8AvAjF@wuerfel>
Date:	Mon, 13 Jun 2016 11:06:05 +0200
From:	Arnd Bergmann <arnd@...db.de>
To:	Dongpo Li <lidongpo@...ilicon.com>
Cc:	f.fainelli@...il.com, robh+dt@...nel.org, mark.rutland@....com,
	davem@...emloft.net, xuejiancheng@...ilicon.com,
	netdev@...r.kernel.org, devicetree@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 3/3] net: hisilicon: Add Fast Ethernet MAC driver

On Monday, June 13, 2016 2:07:56 PM CEST Dongpo Li wrote:

> +- reset-names: should contain the reset signal name "mac_reset"(required)
> +	and "phy_reset"(optional).

Maybe just name the resets 'mac' and 'phy'? The '_reset' part is
implied by the property.

I gave the driver a brief review and basically everything looks
great, very nice work!

There are two small things that I noticed:

> +
> +       do {
> +               hisi_femac_xmit_reclaim(dev);
> +               num = hisi_femac_rx(dev, task);
> +               work_done += num;
> +               task -= num;
> +               if ((work_done >= budget) || (num == 0))
> +                       break;
> +
> +               ints = readl(priv->glb_base + GLB_IRQ_STAT);
> +               writel(ints & DEF_INT_MASK,
> +                      priv->glb_base + GLB_IRQ_RAW);
> +       } while (ints & DEF_INT_MASK);
> +
> +       if (work_done < budget) {
> +               napi_complete(napi);
> +               hisi_femac_irq_enable(priv, DEF_INT_MASK);
> +       }

You tx function uses BQL to optimize the queue length, and that
is great. You also check xmit reclaim for rx interrupts, so
as long as you have both rx and tx traffic, this should work
great.

However, I notice that you only have a 'tx fifo empty'
interrupt triggering the napi poll, so I guess on a tx-only
workload you will always end up pushing packets into the
queue until BQL throttles tx, and then get the interrupt
after all packets have been sent, which will cause BQL to
make the queue longer up to the maximum queue size, and that
negates the effect of BQL.

Is there any way you can get a tx interrupt earlier than
this in order to get a more balanced queue, or is it ok
to just rely on rx packets to come in occasionally, and
just use the tx fifo empty interrupt as a fallback?

> +	priv->phy_mode = of_get_phy_mode(node);
> +	if (priv->phy_mode < 0) {
> +		dev_err(dev, "not find phy-mode\n");
> +		ret = -EINVAL;
> +		goto out_disable_clk;
> +	}
> +
> +	priv->phy_node = of_parse_phandle(node, "phy-handle", 0);
> +	if (!priv->phy_node) {
> +		dev_err(dev, "not find phy-handle\n");
> +		ret = -EINVAL;
> +		goto out_disable_clk;
> +	}
> +
> +	priv->phy = of_phy_connect(ndev, priv->phy_node,
> +				   hisi_femac_adjust_link, 0, priv->phy_mode);
> +	if (!(priv->phy) || IS_ERR(priv->phy)) {
> +		dev_err(dev, "connect to PHY failed!\n");
> +		ret = -ENODEV;
> +		goto out_phy_node;
> +	}

I wonder if we could generalize this set of three calls, I
get the impression that we duplicate this across several
drivers that shouldn't need to bother with the specific
phy-handle and phy-mode properties.

	Arnd