On Fri, Mar 30, 2018 at 11:15:03AM +0800, Jia-Ju Bai wrote: > > > On 2018/3/30 10:44, Ji-Hun Kim wrote: > >@@ -1165,10 +1205,18 @@ static int vnt_start(struct ieee80211_hw *hw) > > } > > dev_dbg(&priv->pcid->dev, "call device init rd0 ring\n"); > >- device_init_rd0_ring(priv); > >- device_init_rd1_ring(priv); > >- device_init_td0_ring(priv); > >- device_init_td1_ring(priv); > >+ ret = device_init_rd0_ring(priv); > >+ if (ret) > >+ goto error; > >+ ret = device_init_rd1_ring(priv); > >+ if (ret) > >+ goto error; > >+ ret = device_init_td0_ring(priv); > >+ if (ret) > >+ goto error; > >+ ret = device_init_td1_ring(priv); > >+ if (ret) > >+ goto error; > > device_init_registers(priv); > >@@ -1178,6 +1226,8 @@ static int vnt_start(struct ieee80211_hw *hw) > > ieee80211_wake_queues(hw); > > return 0; > >+error: > >+ return ret; > > } > > This code will lead to memory leaks when device_init_rd1_ring() > fails, because the memory allocated by device_init_rd0_ring() is not > freed. > > I think this one will be better: > ret = device_init_rd0_ring(priv); > if (ret) > goto error_init_rd0_ring; > ret = device_init_rd1_ring(priv); > if (ret) > goto error_init_rd1_ring; > ret = device_init_td0_ring(priv); > if (ret) > goto error_init_td0_ring; > ret = device_init_td1_ring(priv); > if (ret) > goto error_init_td1_ring; > ...... > error_init_td1_ring: > device_free_td0_ring(priv); > error_init_td0_ring: > device_free_rd1_ring(priv); > error_init_rd1_ring: > device_free_rd0_ring(priv); > error_init_rd0_ring: > return ret; > > > Best wishes, > Jia-Ju Bai > > But, those freeing function are already placed in the each device_init functions for allocation fail like below.
@@ -550,20 +554,29 @@ static void device_init_rd0_ring(struct vnt_private *priv) + return 0; +error: + device_free_rd0_ring(priv); + return ret; Would freeing in the vnt_start() be better instead of freeing in device_init_rd0_ring function? Best regards, Ji-Hun
