> 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:
signature.asc
Description: PGP signature
