[PATCH 5.10 0/1] mt76: fix mt7615_init_tx_queues() return value

[PATCH 5.10 0/1] mt76: fix mt7615_init_tx_queues() return value

[Nikita Zhandarovich]

Fix mt7615_init_tx_queues() so that it returns result value of final call
to mt7615_init_tx_queue() instead of default 0. Otherwise, failure in
mt7615_init_tx_queue() is not tracked by the parent function. The patch
can be cleanly applied to the 5.10 branch.

This issue is addressed in b671da33d1c5 upstream. I decided to refrain
from backporting it whole for now.

[PATCH 5.10 1/1] mt76: fix mt7615_init_tx_queues() return value

[Nikita Zhandarovich]

mt7615_init_tx_queues() returns 0 regardless of how final
mt7615_init_tx_queue() performs. If mt7615_init_tx_queue() fails (due to
memory issues, for instance), parent function will still erroneously
return 0.

This change takes into account ret value of mt7615_init_tx_queue()
when finishing up mt7615_init_tx_queues().

Fixes: 04b8e65922f6 ("mt76: add mac80211 driver for MT7615 PCIe-based chipsets")
Signed-off-by: Nikita Zhandarovich <n.zhandarovich@fintech.ru>

 drivers/net/wireless/mediatek/mt76/mt7615/dma.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/wireless/mediatek/mt76/mt7615/dma.c b/drivers/net/wireless/mediatek/mt76/mt7615/dma.c
index bf8ae14121db..47922c1dd6e3 100644
--- a/drivers/net/wireless/mediatek/mt76/mt7615/dma.c
+++ b/drivers/net/wireless/mediatek/mt76/mt7615/dma.c
@@ -82,7 +82,7 @@ mt7615_init_tx_queues(struct mt7615_dev *dev)
 
 	ret = mt7615_init_tx_queue(dev, MT_TXQ_MCU, MT7615_TXQ_MCU,
 				   MT7615_TX_MCU_RING_SIZE);
-	return 0;
+	return ret;
 }
 
 static int mt7615_poll_tx(struct napi_struct *napi, int budget)

Re: [PATCH 5.10 1/1] mt76: fix mt7615_init_tx_queues() return value

[Greg Kroah-Hartman]

On Mon, Jan 30, 2023 at 04:36:55AM -0800, Nikita Zhandarovich wrote:
> mt7615_init_tx_queues() returns 0 regardless of how final
> mt7615_init_tx_queue() performs. If mt7615_init_tx_queue() fails (due to
> memory issues, for instance), parent function will still erroneously
> return 0.
> 
> This change takes into account ret value of mt7615_init_tx_queue()
> when finishing up mt7615_init_tx_queues().
> 
> Fixes: 04b8e65922f6 ("mt76: add mac80211 driver for MT7615 PCIe-based chipsets")
> Signed-off-by: Nikita Zhandarovich <n.zhandarovich@fintech.ru>
> 
>  drivers/net/wireless/mediatek/mt76/mt7615/dma.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

What is the git commit id of this upstream?

And I can't apply this as-is for the obvious reason it would mess up the
changelog, how did you create this?

confused,

greg k-h

RE: [PATCH 5.10 1/1] mt76: fix mt7615_init_tx_queues() return value

[Жандарович Никита Игоревич]

> What is the git commit id of this upstream?
> 
> And I can't apply this as-is for the obvious reason it would mess up the
> changelog, how did you create this?
> 
> confused,
> 
> greg k-h

Commit in question is b671da33d1c5973f90f098ff66a91953691df582 upstream. I wasn't certain it makes sense to backport the whole patch as only a small portion of it pertains to the fault at question.

Would be extremely grateful for directions how to proceed from here.

somewhat embarrassed,

Nikita

Re: [PATCH 5.10 1/1] mt76: fix mt7615_init_tx_queues() return value

[Greg Kroah-Hartman]

On Mon, Jan 30, 2023 at 01:27:26PM +0000, Жандарович Никита Игоревич wrote:
> > What is the git commit id of this upstream?
> > 
> > And I can't apply this as-is for the obvious reason it would mess up the
> > changelog, how did you create this?
> > 
> > confused,
> > 
> > greg k-h
> 
> Commit in question is b671da33d1c5973f90f098ff66a91953691df582
> upstream. I wasn't certain it makes sense to backport the whole patch
> as only a small portion of it pertains to the fault at question.

What is the "fault"?

And why not take the whole thing?  What's wrong with that?  We almost
always want to take whatever is in Linus's tree because when we do not,
we almost always cause bugs or other problems (later merge issues.)

So always take the original fix please.

thanks,

greg k-h

RE: [PATCH 5.10 1/1] mt76: fix mt7615_init_tx_queues() return value

[Жандарович Никита Игоревич]

> On Mon, Jan 30, 2023 at 01:27:26PM +0000, Жандарович Никита Игоревич
> wrote:
> > > What is the git commit id of this upstream?
> > >
> > > And I can't apply this as-is for the obvious reason it would mess up
> > > the changelog, how did you create this?
> > >
> > > confused,
> > >
> > > greg k-h
> >
> > Commit in question is b671da33d1c5973f90f098ff66a91953691df582
> > upstream. I wasn't certain it makes sense to backport the whole patch
> > as only a small portion of it pertains to the fault at question.
> 
> What is the "fault"?

In 5.10.y "mt7615_init_tx_queues() returns 0 regardless of how final
mt7615_init_tx_queue() performs. If mt7615_init_tx_queue() fails (due to
memory issues, for instance), parent function will still erroneously
return 0."

This was fixed upstream, although that particular commit's scope was broader.

> And why not take the whole thing?  What's wrong with that?  We almost
> always want to take whatever is in Linus's tree because when we do not, we
> almost always cause bugs or other problems (later merge issues.)
> 
> So always take the original fix please.
> 
> thanks,
> 
> greg k-h

That makes sense, of course. Thanks for your patience, will work toward backporting the whole thing.

regards,

Nikita

Re: [PATCH 5.10 1/1] mt76: fix mt7615_init_tx_queues() return value

[Greg Kroah-Hartman]

On Mon, Jan 30, 2023 at 01:48:24PM +0000, Жандарович Никита Игоревич wrote:
> > On Mon, Jan 30, 2023 at 01:27:26PM +0000, Жандарович Никита Игоревич
> > wrote:
> > > > What is the git commit id of this upstream?
> > > >
> > > > And I can't apply this as-is for the obvious reason it would mess up
> > > > the changelog, how did you create this?
> > > >
> > > > confused,
> > > >
> > > > greg k-h
> > >
> > > Commit in question is b671da33d1c5973f90f098ff66a91953691df582
> > > upstream. I wasn't certain it makes sense to backport the whole patch
> > > as only a small portion of it pertains to the fault at question.
> > 
> > What is the "fault"?
> 
> In 5.10.y "mt7615_init_tx_queues() returns 0 regardless of how final
> mt7615_init_tx_queue() performs. If mt7615_init_tx_queue() fails (due to
> memory issues, for instance), parent function will still erroneously
> return 0."

And how can memory issues actually be triggered in a real system?  Is
this a fake problem or something you can validate and verify works
properly?

Don't worry about fake issues for stable backports please.

thanks,

greg k-h

RE: [PATCH 5.10 1/1] mt76: fix mt7615_init_tx_queues() return value

[Жандарович Никита Игоревич]

> > > What is the "fault"?
> >
> > In 5.10.y "mt7615_init_tx_queues() returns 0 regardless of how final
> > mt7615_init_tx_queue() performs. If mt7615_init_tx_queue() fails (due
> > to memory issues, for instance), parent function will still
> > erroneously return 0."
> 
> And how can memory issues actually be triggered in a real system?  Is this a
> fake problem or something you can validate and verify works properly?
> 
> Don't worry about fake issues for stable backports please.
> 
> thanks,
> 
> greg k-h

mt7615_init_tx_queue() calls devm_kzalloc() (which can throw -ENOMEM) and mt76_queue_alloc() (which can also fail). It's hard for me to gauge how probable these failures can be. But I feel like at the very least it's a logical sanity check. 

@@ -82,7 +82,7 @@ mt7615_init_tx_queues(struct mt7615_dev *dev)
 	
        ret = mt7615_init_tx_queue(dev, MT_TXQ_MCU, MT7615_TXQ_MCU,
                                   MT7615_TX_MCU_RING_SIZE);
       return 0;

There is no special reason  for mt7615_init_tx_queues() to ignore last 'ret'. If last mt7615_init_tx_queue(), so should mt7615_init_tx_queues(). And upstream patch (b671da33d1c5973f90f098ff66a91953691df582) addresses this as well. 
If you feel differently, I will of course back down.

regards,

Nikita

Re: [PATCH 5.10 1/1] mt76: fix mt7615_init_tx_queues() return value

[Greg Kroah-Hartman]

On Mon, Jan 30, 2023 at 04:13:11PM +0000, Жандарович Никита Игоревич wrote:
> > > > What is the "fault"?
> > >
> > > In 5.10.y "mt7615_init_tx_queues() returns 0 regardless of how final
> > > mt7615_init_tx_queue() performs. If mt7615_init_tx_queue() fails (due
> > > to memory issues, for instance), parent function will still
> > > erroneously return 0."
> > 
> > And how can memory issues actually be triggered in a real system?  Is this a
> > fake problem or something you can validate and verify works properly?
> > 
> > Don't worry about fake issues for stable backports please.
> > 
> > thanks,
> > 
> > greg k-h
> 
> mt7615_init_tx_queue() calls devm_kzalloc() (which can throw -ENOMEM) and mt76_queue_alloc() (which can also fail). It's hard for me to gauge how probable these failures can be. But I feel like at the very least it's a logical sanity check. 

Again, how can those allocations really fail?  Or the queue allocation?
Can you test this?  If not, it's not a real failure that you need to
backport.  Otherwise all of the little tiny "fix up this potential
failure path" patches would need to be backported, and that's just crazy
if they can not be hit in normal operation.

And please line-wrap your emails :(

thanks,

greg k-h