Re: [PATCH] [v7] net: emac: emac gigabit ethernet controller driver

[Timur Tabi <timur-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>]

Florian Fainelli wrote:

> nr_frags can't be bigger than MAX_SKB_FRAGS, hence these checks all
> other drivers do against 1 + MAX_SKB_FRAGS.

Doh, I just realized something.  emac_mac_tx_buf_send() just needs to 
make sure that there's enough room for ONE skb.  For some reason I 
thought it had to make sure there's enough room for multiple SKBs.

Now it makes a lot more sense.  Thank you.

So it looks like a given SKB can occupy 3 + nr_frags descriptors.  So I 
need to change that line to:

	if (emac_tpd_num_free_descs(tx_q) < (MAX_SKB_FRAGS + 3))
		netif_stop_queue(adpt->netdev);

Question, some  drivers do <= instead of just <, like this:

	if (ring->free_bds <= (MAX_SKB_FRAGS + 1))
		netif_tx_stop_queue(txq);

Is it necessary to stop the queue if there exactly enough descriptors to 
hold an SKB?  Shouldn't the above be this instead:

	if (ring->free_bds < (MAX_SKB_FRAGS + 1))
		netif_tx_stop_queue(txq);


>> However, I'm confused about one thing.  Almost every other driver just
>> sets "netdev->mtu = new_mtu" and does nothing else.  I can't find any
>> other driver that actually stops the RX path, reprograms the hardware,
>> and then restarts the RX path.  I know this is a stupid question, but
>> why is my driver doing that?
>
> Most drivers allocate RX buffer sizes that are usually bigger than the
> MTU, but would probably silently fail or expose transient behavior once
> the MTU changes to greater than the size pre-defined.

So it looks like the real problem is a race condition between

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

and

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


That is, if the interface is running, I set rxbuf_size.  If suddenly I 
receive some packets, then the driver will use the wrong buffer size.

Is there an easy way for me to stop the RX path before I set rxbuf_size? 
  Some netif_xxx function I can call?


-- 
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.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html