* Re: [bug report] mt76: implement functions to get the response skb for MCU calls
@ 2021-10-08 14:27 ` Dan Carpenter
0 siblings, 0 replies; 12+ messages in thread
From: Dan Carpenter @ 2021-10-08 14:27 UTC (permalink / raw)
To: Johannes Berg; +Cc: nbd, linux-wireless, linux-mediatek
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:
> >
> > 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
_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [bug report] mt76: implement functions to get the response skb for MCU calls
2021-10-08 14:27 ` Dan Carpenter
@ 2021-10-08 14:35 ` Dan Carpenter
-1 siblings, 0 replies; 12+ messages in thread
From: Dan Carpenter @ 2021-10-08 14:35 UTC (permalink / raw)
To: Johannes Berg; +Cc: nbd, linux-wireless, linux-mediatek
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:
> > 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)?
> >
>
> 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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [bug report] mt76: implement functions to get the response skb for MCU calls
@ 2021-10-08 14:35 ` Dan Carpenter
0 siblings, 0 replies; 12+ messages in thread
From: Dan Carpenter @ 2021-10-08 14:35 UTC (permalink / raw)
To: Johannes Berg; +Cc: nbd, linux-wireless, linux-mediatek
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:
> > 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)?
> >
>
> 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
_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [bug report] mt76: implement functions to get the response skb for MCU calls
2021-10-08 14:27 ` Dan Carpenter
(?)
(?)
@ 2022-10-13 13:04 ` Dan Carpenter
2022-10-13 16:25 ` Lorenzo Bianconi
-1 siblings, 1 reply; 12+ messages in thread
From: Dan Carpenter @ 2022-10-13 13:04 UTC (permalink / raw)
To: Felix Fietkau, Johannes Berg; +Cc: linux-wireless, linux-mediatek
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:
> > 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)?
> >
>
> 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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [bug report] mt76: implement functions to get the response skb for MCU calls
2022-10-13 13:04 ` Dan Carpenter
@ 2022-10-13 16:25 ` Lorenzo Bianconi
2022-10-14 7:30 ` Dan Carpenter
0 siblings, 1 reply; 12+ messages in thread
From: Lorenzo Bianconi @ 2022-10-13 16:25 UTC (permalink / raw)
To: Dan Carpenter
Cc: Felix Fietkau, Johannes Berg, linux-wireless, linux-mediatek
[-- Attachment #1: Type: text/plain, Size: 3240 bytes --]
> 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:
> > 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:
> > > >
> > > > 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
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [bug report] mt76: implement functions to get the response skb for MCU calls
2022-10-13 16:25 ` Lorenzo Bianconi
@ 2022-10-14 7:30 ` Dan Carpenter
2022-10-14 8:11 ` Lorenzo Bianconi
0 siblings, 1 reply; 12+ messages in thread
From: Dan Carpenter @ 2022-10-14 7:30 UTC (permalink / raw)
To: Lorenzo Bianconi
Cc: Felix Fietkau, Johannes Berg, linux-wireless, linux-mediatek
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);
}
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [bug report] mt76: implement functions to get the response skb for MCU calls
2022-10-14 7:30 ` Dan Carpenter
@ 2022-10-14 8:11 ` Lorenzo Bianconi
0 siblings, 0 replies; 12+ messages in thread
From: Lorenzo Bianconi @ 2022-10-14 8:11 UTC (permalink / raw)
To: Dan Carpenter
Cc: Felix Fietkau, Johannes Berg, linux-wireless, linux-mediatek
[-- Attachment #1: Type: text/plain, Size: 2944 bytes --]
> 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);
> }
>
This patch seems correct since we run mcu_send_msg just for mt76x0 and mt76x2.
@Felix: what do you think?
Regards,
Lorenzo
>
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply [flat|nested] 12+ messages in thread