| Message ID | 20170103161140.16919-1-zajec5@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
On 3-1-2017 17:11, Rafał Miłecki wrote: > From: Rafał Miłecki <rafal@milecki.pl> > > Every new firmware API will most likely require changes in our code to > support it. Right now we support 2 versions only. Refuse to init if we > detect newer version. > > Signed-off-by: Rafał Miłecki <rafal@milecki.pl> > --- > Hi Arend, > > I think you were concerned about possible firmware API changes. Please > review this patch, I hope it's a proper check for running unsupported > firmware version which could result in broken communication between host > driver and a device. > --- > drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c > index 0babfc7..c69ae84 100644 > --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c > +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c > @@ -6816,6 +6816,11 @@ struct brcmf_cfg80211_info *brcmf_cfg80211_attach(struct brcmf_pub *drvr, > brcmf_err("Failed to get D11 version (%d)\n", err); > goto priv_out; > } > + if (io_type > BRCMU_D11AC_IOTYPE) { > + brcmf_err("Unsupported IO version %d\n", io_type); > + goto priv_out; > + } I prefer to have this in brcmu_d11_attach() and have it return an error. Now this actually does not cover the API version in its entirety. The broadcom firmware API is a bit more complicated. Firmware commands may use structured data where we may only add fields at the end to keep it backward compatible and they are versioned if that can not be avoided. As example see recent patch [1]. Regards, Arend [1] https://patchwork.kernel.org/patch/9442873/ > cfg->d11inf.io_type = (u8)io_type; > brcmu_d11_attach(&cfg->d11inf); > >
On 5 January 2017 at 10:50, Arend Van Spriel <arend.vanspriel@broadcom.com> wrote: > On 3-1-2017 17:11, Rafał Miłecki wrote: >> From: Rafał Miłecki <rafal@milecki.pl> >> >> Every new firmware API will most likely require changes in our code to >> support it. Right now we support 2 versions only. Refuse to init if we >> detect newer version. >> >> Signed-off-by: Rafał Miłecki <rafal@milecki.pl> >> --- >> Hi Arend, >> >> I think you were concerned about possible firmware API changes. Please >> review this patch, I hope it's a proper check for running unsupported >> firmware version which could result in broken communication between host >> driver and a device. >> --- >> drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c | 5 +++++ >> 1 file changed, 5 insertions(+) >> >> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c >> index 0babfc7..c69ae84 100644 >> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c >> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c >> @@ -6816,6 +6816,11 @@ struct brcmf_cfg80211_info *brcmf_cfg80211_attach(struct brcmf_pub *drvr, >> brcmf_err("Failed to get D11 version (%d)\n", err); >> goto priv_out; >> } >> + if (io_type > BRCMU_D11AC_IOTYPE) { >> + brcmf_err("Unsupported IO version %d\n", io_type); >> + goto priv_out; >> + } > > I prefer to have this in brcmu_d11_attach() and have it return an error. My too, but since you made brcmu_d11_attach part of *utils* and new IO version support may require driver changes, I didn't want to mess these two. If it was up to me, I would keep brcmu_d11_attach in brcmfmac code and then add a proper check in this function indeed. Is there any reason you made brcmu_d11_attach part of utils?
On 5-1-2017 11:06, Rafał Miłecki wrote: > On 5 January 2017 at 10:50, Arend Van Spriel > <arend.vanspriel@broadcom.com> wrote: >> On 3-1-2017 17:11, Rafał Miłecki wrote: >>> From: Rafał Miłecki <rafal@milecki.pl> >>> >>> Every new firmware API will most likely require changes in our code to >>> support it. Right now we support 2 versions only. Refuse to init if we >>> detect newer version. >>> >>> Signed-off-by: Rafał Miłecki <rafal@milecki.pl> >>> --- >>> Hi Arend, >>> >>> I think you were concerned about possible firmware API changes. Please >>> review this patch, I hope it's a proper check for running unsupported >>> firmware version which could result in broken communication between host >>> driver and a device. >>> --- >>> drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c | 5 +++++ >>> 1 file changed, 5 insertions(+) >>> >>> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c >>> index 0babfc7..c69ae84 100644 >>> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c >>> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c >>> @@ -6816,6 +6816,11 @@ struct brcmf_cfg80211_info *brcmf_cfg80211_attach(struct brcmf_pub *drvr, >>> brcmf_err("Failed to get D11 version (%d)\n", err); >>> goto priv_out; >>> } >>> + if (io_type > BRCMU_D11AC_IOTYPE) { >>> + brcmf_err("Unsupported IO version %d\n", io_type); >>> + goto priv_out; >>> + } >> >> I prefer to have this in brcmu_d11_attach() and have it return an error. > > My too, but since you made brcmu_d11_attach part of *utils* and new IO > version support may require driver changes, I didn't want to mess > these two. > > If it was up to me, I would keep brcmu_d11_attach in brcmfmac code and > then add a proper check in this function indeed. > > Is there any reason you made brcmu_d11_attach part of utils? The plan was/is to add 11ac support in brcmsmac so it would need the brcmu_d11_*() functions. Regards, Arend
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c index 0babfc7..c69ae84 100644 --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c @@ -6816,6 +6816,11 @@ struct brcmf_cfg80211_info *brcmf_cfg80211_attach(struct brcmf_pub *drvr, brcmf_err("Failed to get D11 version (%d)\n", err); goto priv_out; } + if (io_type > BRCMU_D11AC_IOTYPE) { + brcmf_err("Unsupported IO version %d\n", io_type); + goto priv_out; + } + cfg->d11inf.io_type = (u8)io_type; brcmu_d11_attach(&cfg->d11inf);