netdev - Re: [PATCH] [v6] net: emac: emac gigabit ethernet controller 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: <57A0DF6D.6040207@codeaurora.org>
Date:	Tue, 2 Aug 2016 12:59:09 -0500
From:	Timur Tabi <timur@...eaurora.org>
To:	Lino Sanfilippo <LinoSanfilippo@....de>, netdev@...r.kernel.org,
	devicetree@...r.kernel.org, linux-arm-msm@...r.kernel.org,
	sdharia@...eaurora.org, shankerd@...eaurora.org,
	vikrams@...eaurora.org, cov@...eaurora.org, gavidov@...eaurora.org,
	robh+dt@...nel.org, andrew@...n.ch, bjorn.andersson@...aro.org,
	mlangsdo@...hat.com, jcm@...hat.com, agross@...eaurora.org,
	davem@...emloft.net, f.fainelli@...il.com
Subject: Re: [PATCH] [v6] net: emac: emac gigabit ethernet controller driver

Lino Sanfilippo wrote:

> By looking closer to the code, the lock seems to serve the protection of a list of skbs that
> are queued to be timestamped. However there is nothing that ever enqueues those skbs, is it?
> I assume that this is a leftover of a previous version of that driver. Code to be merged into
> mailine has to be completely cleaned up and must not contains any functions which are never
> called. So either you implement the timestamping feature completely or you remove the concerning
> code altogether for the initial mainline driver version. You can add that feature any time later.

I will remove it.  It's not enabled by default on my platform anyway.  I 
didn't realize it wasn't properly implemented.

>> So you're saying that if NETIF_F_RXCSUM is not set, then
>> napi_gro_receive() should not be called?
>
> This requirement seems indeed to be obsolete now so you can ignore my former complaint and
> leave it as it is.

Ok.

> No, how should it? There still is nothing that wakes up the queue once it is stopped. Stopping and
> restarting/waking up a queue is up to the driver, since the network stack cant know if the hw is
> ready to queue another transmission or not. Usually the queue is stopped in the xmit function
> as soon as there are not enough descs left. Stopping the queue tells the network stack not to
> call the xmit function any more. When there are enough descs available again the driver
> has to wake up the queue. This is normally done in the tx completion handler (emac_mac_tx_process()
> in your case) as soon as enough free list elements are available again. Take a look at other
> drivers and when they call netif_wake_queue (or one of its variants).

Something must have gotten deleted by accident.  The internal version of 
the driver has this:

if (netif_queue_stopped(adpt->netdev) &&
     netif_carrier_ok(adpt->netdev) &&
     (emac_get_num_free_tpdescs(txque) >= (txque->tpd.count / 8)))
	netif_wake_queue(adpt->netdev);

My version replaces this with:

	netdev_completed_queue(adpt->netdev, pkts_compl, bytes_compl);

I don't know why this change was made.  However, if I comment out this 
line, the transmit queue times out, so obviously it's necessary.

I notice that *some* drivers, follow that with some variant of:

	if (netif_queue_stopped(bgmac->net_dev))
		netif_wake_queue(bgmac->net_dev);

Is there a good way to test my code? ping and iperf appear to send no 
more than 3 packets at a time, which comes nowhere close to filling the 
queue (which holds 512 normally).  netif_queue_stopped() never returns 
true, no matter what I do.

>> Should I just move the "netdev->mtu = new_mtu" line outside of the
>> if-statement?
>
> You can do that, but take care to ajdust dpt->rxbuf_size to the correct
> value as soon as the interface is brought up. (The same applies to the
> initialization of the mac with the new mtu value of course).

Is there any reason this doesn't work:

	netdev->mtu = new_mtu;
	adpt->rxbuf_size = new_mtu > EMAC_DEF_RX_BUF_SIZE ?
		ALIGN(max_frame, 8) : EMAC_DEF_RX_BUF_SIZE;

	if (netif_running(netdev))
		return emac_reinit_locked(adpt);

I set rxbuf_size regardless.  If the interface is up, it will 
reinitialize and load the new value.  If the interface is down, 
rxbuf_size will be ignored until emac_mac_up() is called.

-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.