| Message ID | 1490697808-5538-3-git-send-email-arend.vanspriel@broadcom.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 8f9dd1a974380ebe2d7bf82df4e6ba6bfb89c575 |
| Delegated to: | Kalle Valo |
| Headers | show |
On 03/28/2017 12:43 PM, Arend van Spriel wrote: > From: Franky Lin <franky.lin@broadcom.com> > > Move brcmf_fws_deinit into brcmf_proto_bcdc_detach since it is a bcdc > exclusive feature. > > Signed-off-by: Franky Lin <franky.lin@broadcom.com> > Change-Id: Iefa5db632b92185a49d538b1cd25b7d8be618ce0 > Reviewed-on: http://hnd-swgit.sj.broadcom.com:8080/8157 > Reviewed-by: brcm80211 ci <brcm80211-ci@broadcom.com> > Reviewed-by: Arend Van Spriel <arend.vanspriel@broadcom.com> > Signed-off-by: Arend van Spriel <arend.vanspriel@broadcom.com> > --- > 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. > 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. 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.
On 29-3-2017 13:23, Rafał Miłecki wrote: > On 03/28/2017 12:43 PM, Arend van Spriel wrote: >> From: Franky Lin <franky.lin@broadcom.com> >> >> Move brcmf_fws_deinit into brcmf_proto_bcdc_detach since it is a bcdc >> exclusive feature. >> >> Signed-off-by: Franky Lin <franky.lin@broadcom.com> >> Change-Id: Iefa5db632b92185a49d538b1cd25b7d8be618ce0 >> Reviewed-on: http://hnd-swgit.sj.broadcom.com:8080/8157 >> Reviewed-by: brcm80211 ci <brcm80211-ci@broadcom.com> >> Reviewed-by: Arend Van Spriel <arend.vanspriel@broadcom.com> >> Signed-off-by: Arend van Spriel <arend.vanspriel@broadcom.com> >> --- >> 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
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; } 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); @@ -1103,8 +1098,6 @@ void brcmf_detach(struct device *dev) brcmf_cfg80211_detach(drvr->config); - brcmf_fws_deinit(drvr); - brcmf_bus_stop(drvr->bus_if); brcmf_proto_detach(drvr);