From: Rafał Miłecki <[email protected]>
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 <[email protected]>
---
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;
+ }
+
cfg->d11inf.io_type = (u8)io_type;
brcmu_d11_attach(&cfg->d11inf);
--
2.10.1
On 5-1-2017 11:06, Rafał Miłecki wrote:
> On 5 January 2017 at 10:50, Arend Van Spriel
> <[email protected]> wrote:
>> On 3-1-2017 17:11, Rafał Miłecki wrote:
>>> From: Rafał Miłecki <[email protected]>
>>>
>>> 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 <[email protected]>
>>> ---
>>> 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
On 5 January 2017 at 10:50, Arend Van Spriel
<[email protected]> wrote:
> On 3-1-2017 17:11, Rafa=C5=82 Mi=C5=82ecki wrote:
>> From: Rafa=C5=82 Mi=C5=82ecki <[email protected]>
>>
>> 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=C5=82 Mi=C5=82ecki <[email protected]>
>> ---
>> 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 3-1-2017 17:11, Rafał Miłecki wrote:
> From: Rafał Miłecki <[email protected]>
>
> 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 <[email protected]>
> ---
> 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);
>
>