netdev - Re: [PATCH] [v7] 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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Tue, 9 Aug 2016 18:25:26 -0700
From:	Florian Fainelli <f.fainelli@...il.com>
To:	Timur Tabi <timur@...eaurora.org>,
	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
Subject: Re: [PATCH] [v7] net: emac: emac gigabit ethernet controller driver

On 08/09/2016 06:09 PM, Timur Tabi wrote:
> 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);

Humm, it kind of depends, but I would err on the side of strictly lesser
than as a better behavior where you may still allow a full fragmented
SKB to make it through on the next xmit call (I know the code you quote
is exactly not doing that).

> 
> 
>>> 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.

Correct, and possibly other HW settings that you have may have to
program to tell the MAC what the maximum packet length should be.

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

napi_disable() should take care of that.
-- 
Florian