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

[Lorenzo Bianconi <lorenzo@kernel.org>]

[-- 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 --]