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.

quoted

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.

quoted

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.