mt76: implement functions to get the response skb for MCU calls

bug report Dan Carpenter 3 participants 8 messages Oct 8, 2021 – Oct 14, 2022

Subsystems (2)

Arm MediaTek MediaTek MT76 wireless LAN driver

lore

Share & tools

lore

Johannes Berg

2021-10-08 14:03 UTC #1 lore raw

Quoted 11 lines · Dan Carpenter

    1146 ret = mt76_mcu_send_and_get_msg(&dev->mt76, MCU_EXT_CMD_EFUSE_ACCESS, &req,
    1147 sizeof(req), true, &skb);
If mt76_mcu_send_and_get_msg() calls the dev->mcu_ops->mcu_send_msg() then "skb" is not initialized.
    1148 if (ret)
    1149 return ret;
    1150
--> 1151 res = (struct mt7921_mcu_eeprom_info *)skb->data;

Looks like possibly 'skb' is always initialized if mt76_mcu_send_and_get_msg() returns 0 (success)?

But I guess it'd be nicer to write that with ERR_PTR() and actually *return* the pointer (or ERR_PTR), rather than the output parameter.

johannes

Dan Carpenter

author

2021-10-08 14:28 UTC #2 lore raw

Quoted 8 lines · Johannes Berg

Quoted 7 lines · Dan Carpenter
1146 ret = mt76_mcu_send_and_get_msg(&dev->mt76, MCU_EXT_CMD_EFUSE_ACCESS, &req,
1147 sizeof(req), true, &skb);
If mt76_mcu_send_and_get_msg() calls the dev->mcu_ops->mcu_send_msg() then "skb" is not initialized.

Quoted 8 lines

Quoted 4 lines
    1148 if (ret)
    1149 return ret;
    1150
--> 1151 res = (struct mt7921_mcu_eeprom_info *)skb->data;
Looks like possibly 'skb' is always initialized if mt76_mcu_send_and_get_msg() returns 0 (success)?

This build is with cross function analysis enabled so Smatch looks for that.

The problem is that the caller has to know if dev->mcu_ops->mcu_send_msg is NULL or not because if it's non-NULL "skb" is not set. Perhaps that means it should be separated into two functions and we pick which one to call depending on whether the pointer is set.

drivers/net/wireless/mediatek/mt76/mcu.c
54 int mt76_mcu_send_and_get_msg(struct mt76_dev *dev, int cmd, const void *data,
55 int len, bool wait_resp, struct sk_buff **ret_skb)
^^^^^^^
This is the parameter.

    56  {
    57          struct sk_buff *skb;
    58  
    59          if (dev->mcu_ops->mcu_send_msg)
    60                  return dev->mcu_ops->mcu_send_msg(dev, cmd, data, len, wait_resp);

The function pointer doesn't set *ret_skb at all.

    61  
    62          skb = mt76_mcu_msg_alloc(dev, data, len);
    63          if (!skb)
    64                  return -ENOMEM;
    65  
    66          return mt76_mcu_skb_send_and_get_msg(dev, skb, cmd, wait_resp, ret_skb);

But this does.

    67  }

regards,
dan carpenter

Dan Carpenter

author

2021-10-08 14:36 UTC #3 lore raw

Quoted 14 lines · Dan Carpenter

Quoted 13 lines · Johannes Berg
Quoted 11 lines · Dan Carpenter
    1146 ret = mt76_mcu_send_and_get_msg(&dev->mt76, MCU_EXT_CMD_EFUSE_ACCESS, &req,
    1147 sizeof(req), true, &skb);
If mt76_mcu_send_and_get_msg() calls the dev->mcu_ops->mcu_send_msg() then "skb" is not initialized.
    1148 if (ret)
    1149 return ret;
    1150
--> 1151 res = (struct mt7921_mcu_eeprom_info *)skb->data;

Quoted 6 lines

Quoted 3 lines
Looks like possibly 'skb' is always initialized if mt76_mcu_send_and_get_msg() returns 0 (success)?
This build is with cross function analysis enabled so Smatch looks for that.

Btw, it turns out I basically completely disabled the Smatch check for uninitialized variables a while back.

I've fixed it now so it's warning again, but I'm going through and manually fixing stuff and adding hack arounds to silence false positives. So hopefully, I'll be able to enable it in the published code soonish.

regards,
dan carpenter

Dan Carpenter

author

2022-10-13 13:04 UTC #4 lore raw

In reply to Dan Carpenter

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?

regards,
dan carpenter

Quoted 53 lines · Dan Carpenter

Quoted 8 lines · Johannes Berg
Quoted 7 lines · Dan Carpenter
    1146 ret = mt76_mcu_send_and_get_msg(&dev->mt76, MCU_EXT_CMD_EFUSE_ACCESS, &req,
    1147 sizeof(req), true, &skb);
If mt76_mcu_send_and_get_msg() calls the dev->mcu_ops->mcu_send_msg() then "skb" is not initialized.
Quoted 8 lines
Quoted 4 lines
    1148 if (ret)
    1149 return ret;
    1150
--> 1151 res = (struct mt7921_mcu_eeprom_info *)skb->data;
Looks like possibly 'skb' is always initialized if mt76_mcu_send_and_get_msg() returns 0 (success)?
This build is with cross function analysis enabled so Smatch looks for that.
The problem is that the caller has to know if dev->mcu_ops->mcu_send_msg is NULL or not because if it's non-NULL "skb" is not set. Perhaps that means it should be separated into two functions and we pick which one to call depending on whether the pointer is set.
drivers/net/wireless/mediatek/mt76/mcu.c
54 int mt76_mcu_send_and_get_msg(struct mt76_dev *dev, int cmd, const void *data,
55 int len, bool wait_resp, struct sk_buff **ret_skb)
^^^^^^^
This is the parameter.
    56  {
    57          struct sk_buff *skb;
    58  
    59          if (dev->mcu_ops->mcu_send_msg)
    60                  return dev->mcu_ops->mcu_send_msg(dev, cmd, data, len, wait_resp);
The function pointer doesn't set *ret_skb at all.
    61  
    62          skb = mt76_mcu_msg_alloc(dev, data, len);
    63          if (!skb)
    64                  return -ENOMEM;
    65  
    66          return mt76_mcu_skb_send_and_get_msg(dev, skb, cmd, wait_resp, ret_skb);
But this does.
    67  }
regards,
dan carpenter

Lorenzo Bianconi

maintainer

2022-10-13 16:26 UTC #5 lore raw

Quoted 10 lines

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

Quoted 4 lines

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. Moreover mt7921_mcu_get_eeprom() has been remove a while back. Am I missing something?

Regards,
Lorenzo

Quoted 58 lines

regards,
dan carpenter
Quoted 53 lines · Dan Carpenter
Quoted 8 lines · Johannes Berg
Quoted 7 lines · Dan Carpenter
    1146 ret = mt76_mcu_send_and_get_msg(&dev->mt76, MCU_EXT_CMD_EFUSE_ACCESS, &req,
    1147 sizeof(req), true, &skb);
If mt76_mcu_send_and_get_msg() calls the dev->mcu_ops->mcu_send_msg() then "skb" is not initialized.
Quoted 8 lines
Quoted 4 lines
    1148 if (ret)
    1149 return ret;
    1150
--> 1151 res = (struct mt7921_mcu_eeprom_info *)skb->data;
Looks like possibly 'skb' is always initialized if mt76_mcu_send_and_get_msg() returns 0 (success)?
This build is with cross function analysis enabled so Smatch looks for that.
The problem is that the caller has to know if dev->mcu_ops->mcu_send_msg is NULL or not because if it's non-NULL "skb" is not set. Perhaps that means it should be separated into two functions and we pick which one to call depending on whether the pointer is set.
drivers/net/wireless/mediatek/mt76/mcu.c
54 int mt76_mcu_send_and_get_msg(struct mt76_dev *dev, int cmd, const void *data,
55 int len, bool wait_resp, struct sk_buff **ret_skb)
^^^^^^^
This is the parameter.
    56  {
    57          struct sk_buff *skb;
    58  
    59          if (dev->mcu_ops->mcu_send_msg)
    60                  return dev->mcu_ops->mcu_send_msg(dev, cmd, data, len, wait_resp);
The function pointer doesn't set *ret_skb at all.
    61  
    62          skb = mt76_mcu_msg_alloc(dev, data, len);
    63          if (!skb)
    64                  return -ENOMEM;
    65  
    66          return mt76_mcu_skb_send_and_get_msg(dev, skb, cmd, wait_resp, ret_skb);
But this does.
    67  }
regards,
dan carpenter

Dan Carpenter

author

2022-10-14 07:30 UTC #6 lore raw

Quoted 10 lines · Lorenzo Bianconi

Quoted 10 lines
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."

Quoted 8 lines

Quoted 4 lines
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.

Quoted 2 lines

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

drivers/net/wireless/mediatek/mt76/mcu.c +0 -3

index a8cafa39a56d..6bf0b7d8daee 100644
@@ -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;

drivers/net/wireless/mediatek/mt76/mt76.h +3 -0

index 87db9498dea4..99f931c08da9 100644
@@ -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);
 }

Lorenzo Bianconi

maintainer

2022-10-14 08:11 UTC #7 lore raw

Quoted 56 lines · Lorenzo Bianconi

Quoted 10 lines
Quoted 10 lines
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."
Quoted 8 lines
Quoted 4 lines
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.
Quoted 2 lines
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
drivers/net/wireless/mediatek/mt76/mcu.c +0 -3
index a8cafa39a56d..6bf0b7d8daee 100644
@@ -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;
drivers/net/wireless/mediatek/mt76/mt76.h +0 -0
index 87db9498dea4..99f931c08da9 100644

drivers/net/wireless/mediatek/mt76/mt76.h static inline int · line 1391

Quoted 9 lines

drivers/net/wireless/mediatek/mt76/mt76.h +3 -0

@@ -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);
 }

This patch seems correct since we run mcu_send_msg just for mt76x0 and mt76x2. @Felix: what do you think?

Regards,
Lorenzo

Quoted 2 lines