From: Florian Fainelli <f.fainelli@gmail•com>
To: "Rafał Miłecki" <zajec5@gmail•com>,
"Florian Fainelli" <f.fainelli@gmail•com>,
"David S . Miller" <davem@davemloft•net>,
"Eric Dumazet" <edumazet@google•com>,
"Jakub Kicinski" <kuba@kernel•org>,
"Paolo Abeni" <pabeni@redhat•com>
Cc: netdev@vger•kernel.org, bcm-kernel-feedback-list@broadcom•com,
"Rafał Miłecki" <rafal@milecki•pl>
Subject: Re: [PATCH] net: broadcom: bcm4908_enet: report queued and transmitted bytes
Date: Wed, 26 Oct 2022 12:53:50 -0700 [thread overview]
Message-ID: <f0b2383e-fa08-c488-ec00-b0804d22c86d@gmail.com> (raw)
In-Reply-To: <bc15d5e0-1e48-d353-fc90-680c8039bf4f@gmail.com>
On 10/26/22 08:12, Rafał Miłecki wrote:
> On 26.10.2022 16:58, Florian Fainelli wrote:
>> On 10/26/2022 7:26 AM, Rafał Miłecki wrote:
>>> From: Rafał Miłecki <rafal@milecki•pl>
>>>
>>> This allows BQL to operate avoiding buffer bloat and reducing latency.
>>>
>>> Signed-off-by: Rafał Miłecki <rafal@milecki•pl>
>>> ---
>>> drivers/net/ethernet/broadcom/bcm4908_enet.c | 7 +++++++
>>> 1 file changed, 7 insertions(+)
>>>
>>> diff --git a/drivers/net/ethernet/broadcom/bcm4908_enet.c
>>> b/drivers/net/ethernet/broadcom/bcm4908_enet.c
>>> index 93ccf549e2ed..e672a9ef4444 100644
>>> --- a/drivers/net/ethernet/broadcom/bcm4908_enet.c
>>> +++ b/drivers/net/ethernet/broadcom/bcm4908_enet.c
>>> @@ -495,6 +495,7 @@ static int bcm4908_enet_stop(struct net_device
>>> *netdev)
>>> netif_carrier_off(netdev);
>>> napi_disable(&rx_ring->napi);
>>> napi_disable(&tx_ring->napi);
>>> + netdev_reset_queue(netdev);
>>> bcm4908_enet_dma_rx_ring_disable(enet, &enet->rx_ring);
>>> bcm4908_enet_dma_tx_ring_disable(enet, &enet->tx_ring);
>>> @@ -564,6 +565,8 @@ static netdev_tx_t bcm4908_enet_start_xmit(struct
>>> sk_buff *skb, struct net_devic
>>> enet->netdev->stats.tx_bytes += skb->len;
>>> enet->netdev->stats.tx_packets++;
>>> + netdev_sent_queue(enet->netdev, skb->len);
>>
>> There is an opportunity for fixing an use after free here, after you
>> call bcm4908_enet_dma_tx_ring_enable() the hardware can start
>> transmission right away and also call the TX completion handler, so
>> you could be de-referencing a freed skb reference at this point. Also,
>> to ensure that DMA is actually functional, it is recommended to
>> increase TX stats in the TX completion handler, since that indicates
>> that you have a functional completion process.
>
> I see the problem, thanks!
>
> Actually hw may start transmission even earlier - right after filling
> buf_desc coherent struct.
Not familiar with that hardware, but in premise yes, I suppose once you
write a proper address and length the DMA can notice and start
transmitting. Also even though you are using non-coherent memory, there
appears to be a missing dma_wmb() between the store to buf_desc->ctl and
buf_desc->addr. There is no explicit dependency between those two stores
and subsequent loads or stores, so the processor write buffer could
re-order those in theory. Unlikely to happen because this used on a
Cortex-A53 IIRC, but better safe than sorry.
>
>
>> So long story short, if you record the skb length *before* calling
>> bcm4908_enet_dma_tx_ring_enable() and use that for reporting sent
>> bytes, you should be good.
>
> I may still end up calling netdev_completed_queue() for data for which
> I didn't call netdev_sent_queue() yet. Is that safe?
>
> Maybe I just just call netdev_sent_queue() before updating the buf_desc?
You would want it to be as close a possible from when you hand the
buffer to the hardware, but I see no locking between
bcm4908_start_xmit() and bcm4908_enet_irq_handler() so you already have
a race don't you?
--
Florian
next prev parent reply other threads:[~2022-10-26 19:54 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-10-26 14:26 [PATCH] net: broadcom: bcm4908_enet: report queued and transmitted bytes Rafał Miłecki
2022-10-26 14:58 ` Florian Fainelli
2022-10-26 15:12 ` Rafał Miłecki
2022-10-26 19:53 ` Florian Fainelli [this message]
2022-10-26 20:15 ` Rafał Miłecki
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=f0b2383e-fa08-c488-ec00-b0804d22c86d@gmail.com \
--to=f.fainelli@gmail$(echo .)com \
--cc=bcm-kernel-feedback-list@broadcom$(echo .)com \
--cc=davem@davemloft$(echo .)net \
--cc=edumazet@google$(echo .)com \
--cc=kuba@kernel$(echo .)org \
--cc=netdev@vger$(echo .)kernel.org \
--cc=pabeni@redhat$(echo .)com \
--cc=rafal@milecki$(echo .)pl \
--cc=zajec5@gmail$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox