DriverDev-Devel Archive on lore.kernel.org
 help / color / Atom feed
* [bug report] staging: wilc1000: added support to dynamically add/remove interfaces
@ 2019-11-13 18:33 Dan Carpenter
       [not found] ` <BN6PR11MB3985D361E1AAA5CE959C3828E3710@BN6PR11MB3985.namprd11.prod.outlook.com>
  0 siblings, 1 reply; 2+ messages in thread
From: Dan Carpenter @ 2019-11-13 18:33 UTC (permalink / raw)
  To: ajay.kathat; +Cc: devel

[ When we renamed the files, then all the old warnings showed up as
  new warnings again.  - dan ]

Hello Ajay Singh,

The patch 9bc061e88054: "staging: wilc1000: added support to
dynamically add/remove interfaces" from Jun 26, 2019, leads to the
following static checker warning:

	drivers/staging/wilc1000/wlan.c:497 wilc_wlan_handle_txq()
	warn: missing error code here? 'wilc_wlan_txq_get_first()' failed.

drivers/staging/wilc1000/wlan.c
   474  int wilc_wlan_handle_txq(struct wilc *wilc, u32 *txq_count)
   475  {
   476          int i, entries = 0;
   477          u32 sum;
   478          u32 reg;
   479          u32 offset = 0;
   480          int vmm_sz = 0;
   481          struct txq_entry_t *tqe;
   482          int ret = 0;
   483          int counter;
   484          int timeout;
   485          u32 vmm_table[WILC_VMM_TBL_SIZE];
   486          const struct wilc_hif_func *func;
   487          u8 *txb = wilc->tx_buffer;
   488          struct net_device *dev;
   489          struct wilc_vif *vif;
   490  
   491          if (wilc->quit)
   492                  goto out;
                        ^^^^^^^^

One of my coding hints is that "goto out;" is always wrong.  In the
best case the name is too ambiguous so it doesn't tell what the goto
does.  But quite often the goto does too much.  For example, it could do
kfree(foo->bar); where foo is NULL so it's a NULL dereference.  Always
always distrust code which does a goto out.

In this case we are unlocking a lock but we aren't holding the lock.
Is this a success path?  That's complicated to say and becomes even
more complicated when we review the rest of this function.  It appears
that this function returns -ENOBUFS or random meaningless nonsense.

   493  
   494          mutex_lock(&wilc->txq_add_to_head_cs);
                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Lock.

   495          tqe = wilc_wlan_txq_get_first(wilc);
   496          if (!tqe)
   497                  goto out;
                        ^^^^^^^^^
In this case, Smatch complains that maybe the error code is wrong.
Are we setting "*txq_count" to the correct value?  It's hard to say.

   498          dev = tqe->vif->ndev;
   499          wilc_wlan_txq_filter_dup_tcp_ack(dev);
   500          i = 0;
   501          sum = 0;
   502          do {
   503                  if (tqe && (i < (WILC_VMM_TBL_SIZE - 1))) {
   504                          if (tqe->type == WILC_CFG_PKT)
   505                                  vmm_sz = ETH_CONFIG_PKT_HDR_OFFSET;
   506  
   507                          else if (tqe->type == WILC_NET_PKT)
   508                                  vmm_sz = ETH_ETHERNET_HDR_OFFSET;

[ snip ]

   666          ret = func->hif_clear_int_ext(wilc, ENABLE_TX_VMM);
   667          if (!ret)
   668                  goto out_release_bus;
                        ^^^^^^^^^^^^^^^^^^^^
These functions return 1 on success and 0 on failure.  We should set the
error code here.  There are several other similar places in this
function where we return zero on error.

   669  
   670          ret = func->hif_block_tx_ext(wilc, 0, txb, offset);
   671  
   672  out_release_bus:
   673          release_bus(wilc, WILC_BUS_RELEASE_ALLOW_SLEEP);
   674  
   675  out:
   676          mutex_unlock(&wilc->txq_add_to_head_cs);
   677  
   678          *txq_count = wilc->txq_entries;
   679          return ret;
   680  }

regards,
dan carpenter
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

^ permalink raw reply	[flat|nested] 2+ messages in thread

* Re: FW: [bug report] staging: wilc1000: added support to dynamically add/remove interfaces
       [not found] ` <BN6PR11MB3985D361E1AAA5CE959C3828E3710@BN6PR11MB3985.namprd11.prod.outlook.com>
@ 2019-11-18 23:28   ` " Adham.Abozaeid
  0 siblings, 0 replies; 2+ messages in thread
From: Adham.Abozaeid @ 2019-11-18 23:28 UTC (permalink / raw)
  To: dan.carpenter; +Cc: devel, Ajay.Kathat


> -----Original Message-----
> From: Dan Carpenter <dan.carpenter@oracle.com> 
> Sent: Thursday, November 14, 2019 12:03 AM
> To: Ajay Kathat - I15481 <Ajay.Kathat@microchip.com>
> Cc: devel@driverdev.osuosl.org
> Subject: [bug report] staging: wilc1000: added support to dynamically add/remove interfaces
>
>
>
> [ When we renamed the files, then all the old warnings showed up as
>   new warnings again.  - dan ]
Hi Dan

Thanks for your feedback. We will target these comments in the upcoming patches.

Thanks,
Adham
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

^ permalink raw reply	[flat|nested] 2+ messages in thread

end of thread, back to index

Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-11-13 18:33 [bug report] staging: wilc1000: added support to dynamically add/remove interfaces Dan Carpenter
     [not found] ` <BN6PR11MB3985D361E1AAA5CE959C3828E3710@BN6PR11MB3985.namprd11.prod.outlook.com>
2019-11-18 23:28   ` FW: " Adham.Abozaeid

DriverDev-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/driverdev-devel/0 driverdev-devel/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 driverdev-devel driverdev-devel/ https://lore.kernel.org/driverdev-devel \
		driverdev-devel@linuxdriverproject.org devel@driverdev.osuosl.org
	public-inbox-index driverdev-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.linuxdriverproject.driverdev-devel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git