Re: [PATCH 10/24] staging: wilc1000: move wilc_multicast_mac_addr_list to 'wilc_vif' struct

[Ajay Singh <ajay.kathat@microchip.com>]

On Thu, 23 Aug 2018 11:10:48 +0300
Claudiu Beznea <Claudiu.Beznea@microchip.com> wrote:

> On 14.08.2018 09:50, Ajay Singh wrote:
> > Instead of using 'wilc_multicast_mac_addr_list' as global variable
> > move it part of wilc_vif struct. Rename
> > 'wilc_multicast_mac_addr_list' variable to 'mc_mac_addr_list' as
> > its now part of 'wilc_vif' struct.
> > 
> > Signed-off-by: Ajay Singh <ajay.kathat@microchip.com>
> > ---
> >  drivers/staging/wilc1000/host_interface.c     |  4 +---
> >  drivers/staging/wilc1000/host_interface.h     |  1 -
> >  drivers/staging/wilc1000/linux_wlan.c         | 14 +++++++-------
> >  drivers/staging/wilc1000/wilc_wfi_netdevice.h |  1 +
> >  4 files changed, 9 insertions(+), 11 deletions(-)
> > 
> > diff --git a/drivers/staging/wilc1000/host_interface.c
> > b/drivers/staging/wilc1000/host_interface.c index d930f06..642c314
> > 100644 --- a/drivers/staging/wilc1000/host_interface.c
> > +++ b/drivers/staging/wilc1000/host_interface.c
> > @@ -193,8 +193,6 @@ static struct mutex hif_deinit_lock;
> >  static struct timer_list periodic_rssi;
> >  static struct wilc_vif *periodic_rssi_vif;
> >  
> > -u8
> > wilc_multicast_mac_addr_list[WILC_MULTICAST_TABLE_SIZE][ETH_ALEN]; -
> >  static u8 rcv_assoc_resp[MAX_ASSOC_RESP_FRAME_SIZE];
> >  
> >  static u8 set_ip[2][4];
> > @@ -2588,7 +2586,7 @@ static void handle_set_mcast_filter(struct
> > work_struct *work) *cur_byte++ = ((hif_set_mc->cnt >> 24) & 0xFF);
> >  
> >  	if (hif_set_mc->cnt > 0)
> > -		memcpy(cur_byte, wilc_multicast_mac_addr_list,
> > +		memcpy(cur_byte, vif->mc_mac_addr_list,  
> 
> A locking mechanism should be used for vif->mc_mac_addr_list. It is
> read here.

Below is my understanding. 

wilc_set_multicast_list() is called from 'ndo_set_rx_mode' callback.
 
wilc_set_multicast_list() calls wilc_setup_multicast_filter() which
only read the value from the array and don't write to it when below
if conditions pass.

if (dev->flags &
IFF_ALLMULTI || dev->mc.count > WILC_MULTICAST_TABLE_SIZE) 

if (dev->mc.count == 0)
 
For the scenario when above 'if' conditions fails then value will be
write to array first followed by read operation which will happen
in different context(worker thread).

Unless ndo_set_rx_mode() gets called quickly I don't think there is any
issue here. 

If required instead of adding the lock how about making
handle_set_mcast_filter() a sync call in further patches, so that it can
complete handle_set_mcast_filter() operation before coming out of
ndo_set_rx_mode() callback.

Actually, I am also worried if its right to add 'lock' at this
point without reproducing the issue and only based on the code
observation.

Regards,
Ajay

> 
> >  		       ((hif_set_mc->cnt) * ETH_ALEN));
> >  
> >  	result = wilc_send_config_pkt(vif, SET_CFG, &wid, 1,
> > diff --git a/drivers/staging/wilc1000/host_interface.h
> > b/drivers/staging/wilc1000/host_interface.h index d026f44..4a84dd2
> > 100644 --- a/drivers/staging/wilc1000/host_interface.h
> > +++ b/drivers/staging/wilc1000/host_interface.h
> > @@ -362,7 +362,6 @@ int wilc_set_tx_power(struct wilc_vif *vif, u8
> > tx_power); int wilc_get_tx_power(struct wilc_vif *vif, u8
> > *tx_power); 
> >  extern u8 wilc_connected_ssid[6];
> > -extern u8
> > wilc_multicast_mac_addr_list[WILC_MULTICAST_TABLE_SIZE][ETH_ALEN]; 
> >  extern int wilc_connecting;
> >  
> > diff --git a/drivers/staging/wilc1000/linux_wlan.c
> > b/drivers/staging/wilc1000/linux_wlan.c index 283bb74..bbaa653
> > 100644 --- a/drivers/staging/wilc1000/linux_wlan.c
> > +++ b/drivers/staging/wilc1000/linux_wlan.c
> > @@ -822,14 +822,14 @@ static void wilc_set_multicast_list(struct
> > net_device *dev) }
> >  
> >  	netdev_for_each_mc_addr(ha, dev) {
> > -		memcpy(wilc_multicast_mac_addr_list[i], ha->addr,
> > ETH_ALEN);
> > +		memcpy(vif->mc_mac_addr_list[i], ha->addr,
> > ETH_ALEN);  
> 
> and set here. The contexts are different. If not in this patch, then
> in a future one.
> 
> >  		netdev_dbg(dev, "Entry[%d]: %x:%x:%x:%x:%x:%x\n",
> > i,
> > -			   wilc_multicast_mac_addr_list[i][0],
> > -			   wilc_multicast_mac_addr_list[i][1],
> > -			   wilc_multicast_mac_addr_list[i][2],
> > -			   wilc_multicast_mac_addr_list[i][3],
> > -			   wilc_multicast_mac_addr_list[i][4],
> > -			   wilc_multicast_mac_addr_list[i][5]);
> > +			   vif->mc_mac_addr_list[i][0],
> > +			   vif->mc_mac_addr_list[i][1],
> > +			   vif->mc_mac_addr_list[i][2],
> > +			   vif->mc_mac_addr_list[i][3],
> > +			   vif->mc_mac_addr_list[i][4],
> > +			   vif->mc_mac_addr_list[i][5]);
> >  		i++;
> >  	}
> >  
> > diff --git a/drivers/staging/wilc1000/wilc_wfi_netdevice.h
> > b/drivers/staging/wilc1000/wilc_wfi_netdevice.h index
> > 8cccbbc..ee8eda7 100644 ---
> > a/drivers/staging/wilc1000/wilc_wfi_netdevice.h +++
> > b/drivers/staging/wilc1000/wilc_wfi_netdevice.h @@ -120,6 +120,7 @@
> > struct wilc_vif { u8 ifc_id;
> >  	struct timer_list during_ip_timer;
> >  	bool obtaining_ip;
> > +	u8 mc_mac_addr_list[WILC_MULTICAST_TABLE_SIZE][ETH_ALEN];
> >  };
> >  
> >  struct wilc {
> >