From: Johannes Berg <johannes@sipsolutions.net> Date: 2021-10-08 14:03:20
On Fri, 2021-10-08 at 16:00 +0300, Dan Carpenter wrote:
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
From: Dan Carpenter <hidden> Date: 2021-10-08 14:28:07
On Fri, Oct 08, 2021 at 04:03:10PM +0200, Johannes Berg wrote:
On Fri, 2021-10-08 at 16:00 +0300, Dan Carpenter wrote:
quoted
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)?
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
From: Dan Carpenter <hidden> Date: 2021-10-08 14:36:17
On Fri, Oct 08, 2021 at 05:27:35PM +0300, Dan Carpenter wrote:
On Fri, Oct 08, 2021 at 04:03:10PM +0200, Johannes Berg wrote:
quoted
On Fri, 2021-10-08 at 16:00 +0300, Dan Carpenter wrote:
quoted
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)?
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
From: Dan Carpenter <hidden> Date: 2022-10-13 13:05:02
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
On Fri, Oct 08, 2021 at 05:27:35PM +0300, Dan Carpenter wrote:
On Fri, Oct 08, 2021 at 04:03:10PM +0200, Johannes Berg wrote:
quoted
On Fri, 2021-10-08 at 16:00 +0300, Dan Carpenter wrote:
quoted
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)?
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
From: Lorenzo Bianconi <lorenzo@kernel.org> Date: 2022-10-13 16:26:04
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.
Moreover mt7921_mcu_get_eeprom() has been remove a while back.
Am I missing something?
Regards,
Lorenzo
regards,
dan carpenter
On Fri, Oct 08, 2021 at 05:27:35PM +0300, Dan Carpenter wrote:
quoted
On Fri, Oct 08, 2021 at 04:03:10PM +0200, Johannes Berg wrote:
quoted
On Fri, 2021-10-08 at 16:00 +0300, Dan Carpenter wrote:
quoted
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)?
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
From: Dan Carpenter <hidden> Date: 2022-10-14 07:30:41
On Thu, Oct 13, 2022 at 06:25:54PM +0200, Lorenzo Bianconi wrote:
quoted
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
From: Lorenzo Bianconi <lorenzo@kernel.org> Date: 2022-10-14 08:11:34
quoted hunk
On Thu, Oct 13, 2022 at 06:25:54PM +0200, Lorenzo Bianconi wrote:
quoted
quoted
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.
quoted
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