Hi Dan,
Thanks your reviewing the patch.
On Wed, 19 Sep 2018 12:41:32 +0300
Dan Carpenter <dan.carpenter@xxxxxxxxxx> 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);
> 1111 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
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
