On 29-3-2017 13:23, Rafał Miłecki wrote:
> On 03/28/2017 12:43 PM, Arend van Spriel wrote:
>> From: Franky Lin <[email protected]>
>>
>> Move brcmf_fws_deinit into brcmf_proto_bcdc_detach since it is a bcdc
>> exclusive feature.
>>
>> Signed-off-by: Franky Lin <[email protected]>
>> Change-Id: Iefa5db632b92185a49d538b1cd25b7d8be618ce0
>> Reviewed-on: http://hnd-swgit.sj.broadcom.com:8080/8157
>> Reviewed-by: brcm80211 ci <[email protected]>
>> Reviewed-by: Arend Van Spriel <[email protected]>
>> Signed-off-by: Arend van Spriel <[email protected]>
>> ---
>> drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcdc.c | 1 +
>> drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c | 7 -------
>> 2 files changed, 1 insertion(+), 7 deletions(-)
>>
>> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcdc.c
>> b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcdc.c
>> index bc24b00..67a132c1 100644
>> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcdc.c
>> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcdc.c
>> @@ -464,6 +464,7 @@ int brcmf_proto_bcdc_attach(struct brcmf_pub *drvr)
>>
>> void brcmf_proto_bcdc_detach(struct brcmf_pub *drvr)
>> {
>> + brcmf_fws_deinit(drvr);
>> kfree(drvr->proto->pd);
>> drvr->proto->pd = NULL;
>> }
>
> I'd appreciate you commenting on someones work. I wouldn't mind "Move
> deinit to
> brcmf_proto_bcdc_detach" comment so I could update my
> [PATCH] brcmfmac: wrap brcmf_fws_(de)init into bcdc layer
> if that is so important.
The naming of the functions in fwsignal were poorly chosen (by me). The
common pattern throughout the driver is to use brcmf_<mod>_attach/detach
If fwsignal would have followed that pattern you probably would have
done so.
>> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
>> b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
>> index 9886280..ba4da48 100644
>> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
>> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
>> @@ -32,7 +32,6 @@
>> #include "p2p.h"
>> #include "cfg80211.h"
>> #include "fwil.h"
>> -#include "fwsignal.h"
>> #include "feature.h"
>> #include "proto.h"
>> #include "pcie.h"
>> @@ -1034,10 +1033,6 @@ int brcmf_bus_started(struct device *dev)
>> brcmf_cfg80211_detach(drvr->config);
>> drvr->config = NULL;
>> }
>> - if (drvr->fws) {
>> - brcmf_proto_del_if(ifp->drvr, ifp);
>> - brcmf_fws_deinit(drvr);
>> - }
>> brcmf_net_detach(ifp->ndev, false);
>> if (p2p_ifp)
>> brcmf_net_detach(p2p_ifp->ndev, false);
>
> I don't like this. By removing del_if from error path you assume add_if
> doesn't
> allocate any memory and it never will.
This removal was actually the reason this patch was not submitted
earlier as I asked for more testing.
> It's quite hacky for me. If you init something, then you should also
> deinit it.
> Otherwise it's too easy to introduce an invalid state or add a memory leak.
> Please keep code simple to follow.
The removal here is safe as all interfaces are deleted in brcmf_detach()
using brcmf_remove_interface() which is called when brcmf_bus_started()
fails.
Regards,
Arend
