Message-ID: <gbia6rqppcc53vmel5q5jvgdri3cmeowb64mxfk7jzo6ncuz2f@6kd7acqii62x>
Date: Tue, 24 Sep 2024 11:36:09 -0500
From: Andrew Halaney <ahalaney@...hat.com>
To: Sarosh Hasan <quic_sarohasa@...cinc.com>
Cc: Suraj Jaiswal <jsuraj@....qualcomm.com>, 
	"Suraj Jaiswal (QUIC)" <quic_jsuraj@...cinc.com>, Vinod Koul <vkoul@...nel.org>, 
	"bhupesh.sharma@...aro.org" <bhupesh.sharma@...aro.org>, Andy Gross <agross@...nel.org>, 
	Bjorn Andersson <andersson@...nel.org>, Konrad Dybcio <konrad.dybcio@...aro.org>, 
	"David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, 
	Jakub Kicinski <kuba@...nel.org>, Rob Herring <robh+dt@...nel.org>, 
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>, Conor Dooley <conor+dt@...nel.org>, 
	Alexandre Torgue <alexandre.torgue@...s.st.com>, Jose Abreu <joabreu@...opsys.com>, 
	Maxime Coquelin <mcoquelin.stm32@...il.com>, "netdev@...r.kernel.org" <netdev@...r.kernel.org>, 
	"linux-arm-msm@...r.kernel.org" <linux-arm-msm@...r.kernel.org>, "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>, 
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>, 
	"linux-stm32@...md-mailman.stormreply.com" <linux-stm32@...md-mailman.stormreply.com>, Prasad Sodagudi <psodagud@...cinc.com>, 
	Rob Herring <robh@...nel.org>, kernel <kernel@...cinc.com>
Subject: Re: [PATCH net] net: stmmac: Stop using a single dma_map() for
 multiple descriptors

On Tue, Sep 24, 2024 at 04:36:59PM GMT, Sarosh Hasan wrote:
> 
> 
> On 9/10/2024 7:34 PM, Andrew Halaney wrote:
> > Hey Suraj,
> > 
> > Your email client didn't seem to quote my response in your latest reply,
> > so its difficult to figure out what you're writing vs me below. It also
> > seems to have messed with the line breaks so I'm manually redoing those.
> > 
> > Please see if you can figure out how to make that happen for further
> > replies!
> > 
> > More comments below...
> > 
> > On Tue, Sep 10, 2024 at 12:47:08PM GMT, Suraj Jaiswal wrote:
> >>
> >>
> >> -----Original Message-----
> >> From: Andrew Halaney <ahalaney@...hat.com> 
> >> Sent: Wednesday, September 4, 2024 3:47 AM
> >> To: Suraj Jaiswal (QUIC) <quic_jsuraj@...cinc.com>
> >> Cc: Vinod Koul <vkoul@...nel.org>; bhupesh.sharma@...aro.org; Andy Gross <agross@...nel.org>; Bjorn Andersson <andersson@...nel.org>; Konrad Dybcio <konrad.dybcio@...aro.org>; David S. Miller <davem@...emloft.net>; Eric Dumazet <edumazet@...gle.com>; Jakub Kicinski <kuba@...nel.org>; Rob Herring <robh+dt@...nel.org>; Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>; Conor Dooley <conor+dt@...nel.org>; Alexandre Torgue <alexandre.torgue@...s.st.com>; Jose Abreu <joabreu@...opsys.com>; Maxime Coquelin <mcoquelin.stm32@...il.com>; netdev@...r.kernel.org; linux-arm-msm@...r.kernel.org; devicetree@...r.kernel.org; linux-kernel@...r.kernel.org; linux-stm32@...md-mailman.stormreply.com; Prasad Sodagudi <psodagud@...cinc.com>; Rob Herring <robh@...nel.org>; kernel <kernel@...cinc.com>
> >> Subject: Re: [PATCH net] net: stmmac: Stop using a single dma_map() for multiple descriptors
> >>
> >> WARNING: This email originated from outside of Qualcomm. Please be wary of any links or attachments, and do not enable macros.
> >>
> >> On Mon, Sep 02, 2024 at 03:24:36PM GMT, Suraj Jaiswal wrote:
> >>> Currently same page address is shared
> >>> between multiple buffer addresses and causing smmu fault for other 
> >>> descriptor if address hold by one descriptor got cleaned.
> >>> Allocate separate buffer address for each descriptor for TSO path so 
> >>> that if one descriptor cleared it should not clean other descriptor 
> >>> address.
> > 
> > snip...
> > 
> >>>
> >>>  static void stmmac_flush_tx_descriptors(struct stmmac_priv *priv, int 
> >>> queue) @@ -4351,25 +4380,17 @@ static netdev_tx_t stmmac_tso_xmit(struct sk_buff *skb, struct net_device *dev)
> >>>               pay_len = 0;
> >>>       }
> >>>
> >>> -     stmmac_tso_allocator(priv, des, tmp_pay_len, (nfrags == 0), queue);
> >>> +     if (stmmac_tso_allocator(priv, (skb->data + proto_hdr_len),
> >>> +                              tmp_pay_len, nfrags == 0, queue, false))
> >>> +             goto dma_map_err;
> >>
> >> Changing the second argument here is subtly changing the dma_cap.addr64 <= 32
> >> case right before this. Is that intentional?
> >>
> >> i.e., prior, pretend des = 0 (side note but des is a very confusing variable
> >> name for "dma address" when there's also mentions of desc meaning "descriptor"
> >> in the DMA ring). In the <= 32 case, we'd call stmmac_tso_allocator(priv, 0)
> >> and in the else case we'd call stmmac_tso_allocator(priv, 0 + proto_hdr_len).
> >>
> >> With this change in both cases its called with the (not-yet-dma-mapped)
> >> skb->data + proto_hdr_len always (i.e. like the else case).
> >>
> >> Honestly, the <= 32 case reads weird to me without this patch. It seems some
> >> of the buffer is filled but des is not properly incremented?
> >>
> >> I don't know how this hardware is supposed to be programmed (no databook
> >> access) but that seems fishy (and like a separate bug, which would be nice to
> >> squash if so in its own patch). Would you be able to explain the logic there
> >> to me if it does make sense to you?
> >>
> > 
> >> <Suraj> des can not be 0 . des 0 means dma_map_single() failed and it will return.
> >> If we see if des calculation (first->des1 = cpu_to_le32(des + proto_hdr_len);)
> >> and else case des calculator ( des += proto_hdr_len;) it is adding proto_hdr_len
> >> to the memory that we after mapping skb->data using dma_map_single.
> >> Same way we added proto_hdr_len in second argument . 
> > 
> > 
> > 0 was just an example, and a confusing one, sorry. Let me paste the original
> > fishy code that I think you've modified the behavior for. Here's the
> > original:
> > 
> > 	if (priv->dma_cap.addr64 <= 32) {
> > 		first->des0 = cpu_to_le32(des);
> > 
> > 		/* Fill start of payload in buff2 of first descriptor */
> > 		if (pay_len)
> > 			first->des1 = cpu_to_le32(des + proto_hdr_len);
> > 
> > 		/* If needed take extra descriptors to fill the remaining payload */
> > 		tmp_pay_len = pay_len - TSO_MAX_BUFF_SIZE;
> > 	} else {
> > 		stmmac_set_desc_addr(priv, first, des);
> > 		tmp_pay_len = pay_len;
> > 		des += proto_hdr_len;
> > 		pay_len = 0;
> > 	}
> > 
> > 	stmmac_tso_allocator(priv, des, tmp_pay_len, (nfrags == 0), queue);
> > 
> > Imagine the <= 32 case. Let's say des is address 0 (just for simplicity
> > sake, let's assume that's valid). That means:
> > 
> >     first->des0 = des;
> >     first->des1 = des + proto_hdr_len;
> >     stmmac_tso_allocator(priv, des, tmp_pay_len, (nfrags == 0), queue)
> > 
> >     if des is 0, proto_hdr_len is 64, then that means
> > 
> >     first->des0 = 0
> >     first->des1 = 64
> >     stmmac_tso_allocator(priv, 0, tmp_pay_len, (nfrags == 0), queue)
> > 
> > That seems fishy to me. We setup up the first descriptor with the
> > beginning of des, and then the code goes and sets up more descriptors
> > (stmmac_tso_allocator()) starting with the same des again?
> tso_alloc is checking if more descriptor needed for packet . it is adding offset to get next
> descriptor (curr_addr = des + (total_len - tmp_len)) and storing in des of next descriptor.

Yes, so in stmmac_tso_allocator() we currently have:

	static void stmmac_tso_allocator(struct stmmac_priv *priv, dma_addr_t des,
					 int total_len, bool last_segment, u32 queue)
	{
		struct stmmac_tx_queue *tx_q = &priv->dma_conf.tx_queue[queue];
		struct dma_desc *desc;
		u32 buff_size;
		int tmp_len;

		tmp_len = total_len;

		while (tmp_len > 0) {
			dma_addr_t curr_addr;

			tx_q->cur_tx = STMMAC_GET_ENTRY(tx_q->cur_tx,
							priv->dma_conf.dma_tx_size);
			...
			curr_addr = des + (total_len - tmp_len);
			if (priv->dma_cap.addr64 <= 32)
				desc->des0 = cpu_to_le32(curr_addr);

so on the first loop you've got:
	tmp_len = total_len
	...
	curr_addr = des + total_len - temp_len
	i.e.
	curr_addr = des
meaning with the "first" handling I've highlighted we've got
	first->des0 = des
	"next"->des0 = des

where "next" is the cur_tx descriptor in the first loop of
stmmac_tso_allocator() (essentially the second descriptor).
That seems broken to me, and was that way prior to this patch.

You've modified the behavior in this patch unintentionally. I think it
needs modifying, but it should be done so explicitly in its own patch
prior to this one. I also think the current modification in this patch
isn't a fix. See prior reply below where I highlighted the programming as I
understand it with this patch applied, which would result in something
like.

first->des0 = des
first->des1 = des + proto_hdr_len
"next"->des0 = des + proto_hdr_len

Which again seems wrong, two descriptors pointing to the same address
isn't making sense to me.

Sorry to sound like a broken record, but I want to make sure we're on
the same page! Sounds like you're looking into it based on the below
comment, but some of these comments here made me think I didn't explain
the situation well enough.

> > 
> > Should we be adding the payload length (TSO_MAX_BUFF_SIZE I suppose
> > based on the tmp_pay_len = pay_len - TSO_MAX_BUFFSIZE above)? It seems
> > that <= 32 results in duplicate data in both the "first" descriptor
> > programmed above, and in the "second" descriptor programmed in
> > stmmac_tso_allocator().
> curr_addr = des + (total_len - tmp_len) is used in while loop in  tso_alloc to get address of all required descriptor . 
> descriptor address will be updated finally in tso_alloc by below call .
>  
> if (priv->dma_cap.addr64 <= 32)
>                                                desc->des0 = cpu_to_le32(curr_addr);
>                                else
>                                                stmmac_set_desc_addr(priv, desc, curr_addr);
> 
>  Also, since tmp_pay_len is decremented, but des
> > isn't, it seems that stmmac_tso_allocator() would not put all of the
> > buffer in the descriptors and would leave the last TSO_MAX_BUFF_SIZE
> > bytes out?
> > 
> > I highlight all of this because with your change here we get the
> > following now in the <= 32 case:
> > 
> >     first->des0 = des
> >     first->des1 = des + proto_hdr_len
> >     stmmac_tso_allocator(priv, des + proto_hdr_len, ...)
> > 
> > which is a subtle change in the call to stmmac_tso_allocator, meaning
> > a subtle change in the descriptor programming.
> > 
> > Both seem wrong for the <= 32 case, but I'm "reading between the lines"
> > with how these descriptors are programmed (I don't have the docs to back
> > this up, I'm inferring from the code). It seems to me that in the <= 32
> > case we should have:
> > 
> >     first->des0 = des
> >     first->des1 = des + proto_hdr_len
> >     stmmac_tso_allocator(priv, des + TSO_MAX_BUF_SIZE, ...)
> 
> let me check <=32 case only on setup and get back.
> > 
> > or similar depending on if that really makes sense with how des0/des1 is
> > used (the handling is different in stmmac_tso_allocator() for <= 32,
> > only des0 is used so I'm having a tough time figuring out how much of
> > the des is actually programmed in des0 + des1 above without knowing the
> > hardware better).
> > 
> > Does that make sense? The prior code seems fishy to me, your change
> > seems to unintentionally change that fhsy part, but it still seems fishy
> > to me. I don't think you should be changing that code's behavior in that
> > patch, if you think it's right then we should continue with the current
> > behavior prior to your patch, and if you think its wrong we should
> > probably fix that *prior* to this patch in your series.
> > 
> > Thanks,
> > Andrew
> > 
> 

Powered by blists - more mailing lists