[PATCH 29/29] staging: wilc1000: return exact error of register_netdev() from wilc_netdev_init()
Modified wilc_netdev_init() to return the error code received from register_netdev() during the failure condition. Earlier discussion link [1]. https://www.spinics.net/lists/linux-wireless/msg177304.html Suggested-by: Claudiu Beznea Signed-off-by: Ajay Singh --- drivers/staging/wilc1000/linux_wlan.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/drivers/staging/wilc1000/linux_wlan.c b/drivers/staging/wilc1000/linux_wlan.c index 29c1317..75abaf9 100644 --- a/drivers/staging/wilc1000/linux_wlan.c +++ b/drivers/staging/wilc1000/linux_wlan.c @@ -1056,14 +1056,14 @@ static const struct net_device_ops wilc_netdev_ops = { int wilc_netdev_init(struct wilc **wilc, struct device *dev, int io_type, const struct wilc_hif_func *ops) { - int i, ret; + int i, ret = -ENOMEM; struct wilc_vif *vif; struct net_device *ndev; struct wilc *wl; wl = kzalloc(sizeof(*wl), GFP_KERNEL); if (!wl) - return -ENOMEM; + return ret; if (wilc_wlan_cfg_init(wl)) goto free_wl; @@ -1151,7 +1151,7 @@ int wilc_netdev_init(struct wilc **wilc, struct device *dev, int io_type, wilc_wlan_cfg_deinit(wl); free_wl: kfree(wl); - return -ENOMEM; + return ret; } EXPORT_SYMBOL_GPL(wilc_netdev_init); -- 2.7.4
Re: [PATCH 29/29] staging: wilc1000: return exact error of register_netdev() from wilc_netdev_init()
I was waiting for you to send this like a spider waits for flies. You fell directly into my trap. Mwuahahahahaha. drivers/staging/wilc1000/linux_wlan.c 1056 int wilc_netdev_init(struct wilc **wilc, struct device *dev, int io_type, 1057 const struct wilc_hif_func *ops) 1058 { 1059 int i, ret = -ENOMEM; 1060 struct wilc_vif *vif; 1061 struct net_device *ndev; 1062 struct wilc *wl; 1063 1064 wl = kzalloc(sizeof(*wl), GFP_KERNEL); 1065 if (!wl) 1066 return ret; ^^^ It's cleaner to return -ENOMEM so that we don't have to glance up to the declaration block. This is especially true when "ret" is zero, btw, because that can indicate a reversed test. if (!ret) return ret; In this theoretically example it was supposed to be: if (ret) return ret; Normally, reversed conditions are caught in testing, but for the kernel, no one has the hardware to test everything so we do get reversed conditions from time to time. 1067 1068 if (wilc_wlan_cfg_init(wl)) 1069 goto free_wl; 1070 1071 *wilc = wl; 1072 wl->io_type = io_type; 1073 wl->hif_func = ops; 1074 wl->enable_ps = true; 1075 wl->chip_ps_state = CHIP_WAKEDUP; 1076 INIT_LIST_HEAD(&wl->txq_head.list); 1077 INIT_LIST_HEAD(&wl->rxq_head.list); 1078 1079 wl->hif_workqueue = create_singlethread_workqueue("WILC_wq"); 1080 if (!wl->hif_workqueue) 1081 goto free_cfg; 1082 1083 register_inetaddr_notifier(&g_dev_notifier); 1084 1085 for (i = 0; i < NUM_CONCURRENT_IFC; i++) { 1086 struct wireless_dev *wdev; 1087 1088 ndev = alloc_etherdev(sizeof(struct wilc_vif)); 1089 if (!ndev) 1090 goto free_ndev; ^^^ ret is zero on the second iteration through the loop. 1091 1092 vif = netdev_priv(ndev); 1093 memset(vif, 0, sizeof(struct wilc_vif)); 1094 1095 if (i == 0) { 1096 strcpy(ndev->name, "wlan%d"); 1097 vif->ifc_id = 1; 1098 } else { 1099 strcpy(ndev->name, "p2p%d"); 1100 vif->ifc_id = 0; 1101 } 1102 vif->wilc = *wilc; 1103 vif->ndev = ndev; 1104 wl->vif[i] = vif; 1105 wl->vif_num = i; 1106 vif->idx = wl->vif_num; 1107 1108 ndev->netdev_ops = &wilc_netdev_ops; 1109 1110 wdev = wilc_create_wiphy(ndev, dev); if (!wdev) { 1112 netdev_err(ndev, "Can't register WILC Wiphy\n"); 1113 goto free_ndev; ^^^ Here too. 1114 } 1115 1116 SET_NETDEV_DEV(ndev, dev); 1117 1118 vif->ndev->ieee80211_ptr = wdev; 1119 vif->ndev->ml_priv = vif; 1120 wdev->netdev = vif->ndev; 1121 vif->netstats.rx_packets = 0; 1122 vif->netstats.tx_packets = 0; 1123 vif->netstats.rx_bytes = 0; 1124 vif->netstats.tx_bytes = 0; 1125 1126 ret = register_netdev(ndev); 1127 if (ret) 1128 goto free_ndev; ret is cleared on the first iteration through the loop. 1129 1130 vif->iftype = STATION_MODE; 1131 vif->mac_opened = 0; 1132 } 1133 1134 return 0; regards, dan carpenter
Re: [PATCH 29/29] staging: wilc1000: return exact error of register_netdev() from wilc_netdev_init()
Hi Dan, Thanks your reviewing the patch. On Wed, 19 Sep 2018 12:41:32 +0300 Dan Carpenter wrote: > I was waiting for you to send this like a spider waits for flies. You > fell directly into my trap. Mwuahahahahaha. Oops!!! I missed seeing it coming :) > > drivers/staging/wilc1000/linux_wlan.c > 1056 int wilc_netdev_init(struct wilc **wilc, struct device *dev, > int io_type, 1057 const struct wilc_hif_func > *ops) 1058 { > 1059 int i, ret = -ENOMEM; > 1060 struct wilc_vif *vif; > 1061 struct net_device *ndev; > 1062 struct wilc *wl; > 1063 > 1064 wl = kzalloc(sizeof(*wl), GFP_KERNEL); > 1065 if (!wl) > 1066 return ret; >^^^ > It's cleaner to return -ENOMEM so that we don't have to glance up to > the declaration block. This is especially true when "ret" is zero, > btw, because that can indicate a reversed test. I will change it to directly return -ENOMEM value. > > if (!ret) > return ret; > > In this theoretically example it was supposed to be: > > if (ret) > return ret; > > Normally, reversed conditions are caught in testing, but for the > kernel, no one has the hardware to test everything so we do get > reversed conditions from time to time. > > 1067 > 1068 if (wilc_wlan_cfg_init(wl)) I will set 'ret' value to -ENOMEM here also. > 1069 goto free_wl; > 1070 > 1071 *wilc = wl; > 1072 wl->io_type = io_type; > 1073 wl->hif_func = ops; > 1074 wl->enable_ps = true; > 1075 wl->chip_ps_state = CHIP_WAKEDUP; > 1076 INIT_LIST_HEAD(&wl->txq_head.list); > 1077 INIT_LIST_HEAD(&wl->rxq_head.list); > 1078 > 1079 wl->hif_workqueue = > create_singlethread_workqueue("WILC_wq"); 1080 if > (!wl->hif_workqueue) 1081 goto free_cfg; > 1082 > 1083 register_inetaddr_notifier(&g_dev_notifier); > 1084 > 1085 for (i = 0; i < NUM_CONCURRENT_IFC; i++) { > 1086 struct wireless_dev *wdev; > 1087 > 1088 ndev = alloc_etherdev(sizeof(struct > wilc_vif)); 1089 if (!ndev) > 1090 goto free_ndev; > ^^^ > ret is zero on the second iteration through the loop. I will set 'ret' to -ENOMEM before 'goto', which should handle this scenario. > > 1091 > 1092 vif = netdev_priv(ndev); > 1093 memset(vif, 0, sizeof(struct wilc_vif)); > 1094 > 1095 if (i == 0) { > 1096 strcpy(ndev->name, "wlan%d"); > 1097 vif->ifc_id = 1; > 1098 } else { > 1099 strcpy(ndev->name, "p2p%d"); > 1100 vif->ifc_id = 0; > 1101 } > 1102 vif->wilc = *wilc; > 1103 vif->ndev = ndev; > 1104 wl->vif[i] = vif; > 1105 wl->vif_num = i; > 1106 vif->idx = wl->vif_num; > 1107 > 1108 ndev->netdev_ops = &wilc_netdev_ops; > 1109 > 1110 wdev = wilc_create_wiphy(ndev, dev); > if (!wdev) { > 1112 netdev_err(ndev, "Can't register WILC > Wiphy\n"); 1113 goto free_ndev; > ^^^ > Here too. Same here, i.e setting ret = -ENOMEM should handle the condition. Also I will remove the default setting of 'ret' value to -ENOMEM because after modification the error scenarios will set 'ret' explicitly. Regards, Ajay