Re: [bug report] mt76: implement functions to get the response skb for MCU calls

[Dan Carpenter <dan.carpenter@oracle.com>]

On Thu, Oct 13, 2022 at 06:25:54PM +0200, Lorenzo Bianconi wrote:
> > I would like to revisit this question.  Last time I complained about
> > this Johannes responded but he misread what mt76_mcu_send_and_get_msg()
> > does.  I have looked at it as well and I also cannot explain what is
> > going on in that function.
> > 
> > I have looked at the callers and my first instinct is that maybe this
> > is dead stub code?  But then when I look at mt76x02u_mcu_send_msg() I
> > think "No, this is not stub code.  This should be returning the newly
> > allocated skb to the caller."
> > 
> > But then I think, surely at some point someone tested this code???  It
> > must be stub code.
> > 
> > Could we get some clarity on this?
> 
> for mt76x2 and mt76x0 we do not care of ret_skb (in fact we do not run
> mt76_mcu_send_and_get_msg() directly but we rely on mt76_mcu_send_msg()).
> For mt7921 we set mcu_skb_send_msg function pointer and not mcu_send_msg.

Ah thanks...  It's easy enough to silence the warning in Smatch but I
was never sure if it wasn't a bug.

> Moreover mt7921_mcu_get_eeprom() has been remove a while back.
> Am I missing something?

There are 12 callers for mt76_mcu_send_and_get_msg() and 11 of them
assume that the "ret_skb" is initialized (i.e. they assume that
the ->mcu_send_msg op is not used) so I get 11 Smatch warnings from
this...

Why not just do something like below?  It moves the ->mcu_send_msg()
call to the only place where it won't cause a crash.

regards,
dan carpenter

diff --git a/drivers/net/wireless/mediatek/mt76/mcu.c b/drivers/net/wireless/mediatek/mt76/mcu.c
index a8cafa39a56d..6bf0b7d8daee 100644
--- a/drivers/net/wireless/mediatek/mt76/mcu.c
+++ b/drivers/net/wireless/mediatek/mt76/mcu.c
@@ -58,9 +58,6 @@ int mt76_mcu_send_and_get_msg(struct mt76_dev *dev, int cmd, const void *data,
 {
 	struct sk_buff *skb;
 
-	if (dev->mcu_ops->mcu_send_msg)
-		return dev->mcu_ops->mcu_send_msg(dev, cmd, data, len, wait_resp);
-
 	skb = mt76_mcu_msg_alloc(dev, data, len);
 	if (!skb)
 		return -ENOMEM;
diff --git a/drivers/net/wireless/mediatek/mt76/mt76.h b/drivers/net/wireless/mediatek/mt76/mt76.h
index 87db9498dea4..99f931c08da9 100644
--- a/drivers/net/wireless/mediatek/mt76/mt76.h
+++ b/drivers/net/wireless/mediatek/mt76/mt76.h
@@ -1383,6 +1383,9 @@ static inline int
 mt76_mcu_send_msg(struct mt76_dev *dev, int cmd, const void *data, int len,
 		  bool wait_resp)
 {
+	if (dev->mcu_ops->mcu_send_msg)
+		return dev->mcu_ops->mcu_send_msg(dev, cmd, data, len, wait_resp);
+
 	return mt76_mcu_send_and_get_msg(dev, cmd, data, len, wait_resp, NULL);
 }