* Re: [PATCH 1/1] mwifiex: remove superfluous condition (2)
From: Kalle Valo @ 2016-07-31 12:03 UTC (permalink / raw)
To: Heinrich Schuchardt
Cc: Amitkumar Karwar, Nishant Sarmukadam, linux-wireless, netdev,
linux-kernel
In-Reply-To: <1469962177-2432-1-git-send-email-xypron.glpk@gmx.de>
Heinrich Schuchardt <xypron.glpk@gmx.de> writes:
> We are using mac as source address in a memcpy.
> In the lines below we can assume mac is not NULL.
>
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
Please try to make the commit titles unique, for example you can use
function names or something else. Adding "(2)" is not the way to do
that.
--
Kalle Valo
^ permalink raw reply
* [PATCH 1/1] mwifiex: remove superfluous condition (2)
From: Heinrich Schuchardt @ 2016-07-31 10:49 UTC (permalink / raw)
To: Amitkumar Karwar, Nishant Sarmukadam, Kalle Valo
Cc: linux-wireless, netdev, linux-kernel, Heinrich Schuchardt
We are using mac as source address in a memcpy.
In the lines below we can assume mac is not NULL.
Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
drivers/net/wireless/marvell/mwifiex/sta_cmd.c | 11 +++--------
1 file changed, 3 insertions(+), 8 deletions(-)
diff --git a/drivers/net/wireless/marvell/mwifiex/sta_cmd.c b/drivers/net/wireless/marvell/mwifiex/sta_cmd.c
index 128add8..0d4f9fe 100644
--- a/drivers/net/wireless/marvell/mwifiex/sta_cmd.c
+++ b/drivers/net/wireless/marvell/mwifiex/sta_cmd.c
@@ -706,15 +706,10 @@ mwifiex_cmd_802_11_key_material_v2(struct mwifiex_private *priv,
(priv->wep_key_curr_index & KEY_INDEX_MASK))
key_info |= KEY_DEFAULT;
} else {
- if (mac) {
- if (is_broadcast_ether_addr(mac))
- key_info |= KEY_MCAST;
- else
- key_info |= KEY_UNICAST |
- KEY_DEFAULT;
- } else {
+ if (is_broadcast_ether_addr(mac))
key_info |= KEY_MCAST;
- }
+ else
+ key_info |= KEY_UNICAST | KEY_DEFAULT;
}
}
km->key_param_set.key_info = cpu_to_le16(key_info);
--
2.8.1
^ permalink raw reply related
* [PATCH 1/1] mwifiex: remove superfluous condition
From: Heinrich Schuchardt @ 2016-07-31 10:39 UTC (permalink / raw)
To: Amitkumar Karwar, Nishant Sarmukadam, Kalle Valo
Cc: linux-wireless, netdev, linux-kernel, Heinrich Schuchardt
for_each_property_of_node is only executed if the
property prop is not NULL.
Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
drivers/net/wireless/marvell/mwifiex/sta_cmd.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/wireless/marvell/mwifiex/sta_cmd.c b/drivers/net/wireless/marvell/mwifiex/sta_cmd.c
index 7897037..128add8 100644
--- a/drivers/net/wireless/marvell/mwifiex/sta_cmd.c
+++ b/drivers/net/wireless/marvell/mwifiex/sta_cmd.c
@@ -1482,7 +1482,7 @@ int mwifiex_dnld_dt_cfgdata(struct mwifiex_private *priv,
continue;
/* property header is 6 bytes, data must fit in cmd buffer */
- if (prop && prop->value && prop->length > 6 &&
+ if (prop->value && prop->length > 6 &&
prop->length <= MWIFIEX_SIZE_OF_CMD_BUFFER - S_DS_GEN) {
ret = mwifiex_send_cmd(priv, HostCmd_CMD_CFG_DATA,
HostCmd_ACT_GEN_SET, 0,
--
2.8.1
^ permalink raw reply related
* [PATCH 1/1] rtlwifi: remove superfluous condition
From: Heinrich Schuchardt @ 2016-07-31 10:30 UTC (permalink / raw)
To: Larry Finger, Chaoming Li, Kalle Valo
Cc: linux-wireless, netdev, linux-kernel, Heinrich Schuchardt
If sta == NULL, the changed line will not be reached.
So no need to check if stat == NULL here.
Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
drivers/net/wireless/realtek/rtlwifi/core.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/wireless/realtek/rtlwifi/core.c b/drivers/net/wireless/realtek/rtlwifi/core.c
index 41f77f8..7aee5ebb1 100644
--- a/drivers/net/wireless/realtek/rtlwifi/core.c
+++ b/drivers/net/wireless/realtek/rtlwifi/core.c
@@ -1135,7 +1135,7 @@ static void rtl_op_bss_info_changed(struct ieee80211_hw *hw,
mac->mode = WIRELESS_MODE_AC_24G;
}
- if (vif->type == NL80211_IFTYPE_STATION && sta)
+ if (vif->type == NL80211_IFTYPE_STATION)
rtlpriv->cfg->ops->update_rate_tbl(hw, sta, 0);
rcu_read_unlock();
--
2.8.1
^ permalink raw reply related
* Re: Dell Venue 8 Pro "Dell Wireless" (Atheros 6k) chipset with SDIO ID 0271:0418 not recognized/supported
From: Kalle Valo @ 2016-07-31 9:19 UTC (permalink / raw)
To: russianneuromancer; +Cc: ath6kl, linux-wireless
In-Reply-To: <14140188.nOOhQX7Q2z@laptop1>
russianneuromancer@ya.ru writes:
> Please look into commentary 50 from bugreport https://bugzilla.kernel.org/show_bug.cgi?id=67921#c50
> There is patch for enabling support of Atheros Wireless adapter in Dell Venue 8 Pro tablet.
>
> Please review if this possible to include this patch into upstream.
Do you mean this ath6kl patch:
https://github.com/AdamWill/baytrail-m/blob/master/kernel/support-Dell-OEM-chipset-found-in-Venue-8-Pro-SDIO-I.patch
That looks good to me, but please submit it officially as defined in the wiki:
https://wireless.wiki.kernel.org/en/users/drivers/ath6kl#hacking_on_ath6kl
I also saw the comment about broken ath.git link, I fixed that now.
--
Kalle Valo
^ permalink raw reply
* Re: [RFC v0 7/8] Input: ims-pcu: use firmware_stat instead of completion
From: Dmitry Torokhov @ 2016-07-31 7:23 UTC (permalink / raw)
To: Luis R. Rodriguez, Arend van Spriel
Cc: Daniel Wagner, Bjorn Andersson, Daniel Wagner, Bastien Nocera,
Greg Kroah-Hartman, Johannes Berg, Kalle Valo, Ohad Ben-Cohen,
linux-input, linux-kselftest, linux-wireless, linux-kernel
In-Reply-To: <20160730165817.GQ3296@wotan.suse.de>
On July 30, 2016 9:58:17 AM PDT, "Luis R. Rodriguez" <mcgrof@kernel.org> wrote:
>On Sat, Jul 30, 2016 at 02:42:41PM +0200, Arend van Spriel wrote:
>> + Luis (again) ;-)
>>
>> On 29-07-16 08:13, Daniel Wagner wrote:
>> > On 07/28/2016 09:01 PM, Bjorn Andersson wrote:
>> >> On Thu 28 Jul 11:33 PDT 2016, Dmitry Torokhov wrote:
>> >>
>> >>> On Thu, Jul 28, 2016 at 09:55:11AM +0200, Daniel Wagner wrote:
>> >>>> From: Daniel Wagner <daniel.wagner@bmw-carit.de>
>> >>>>
>> >> [..]
>> >>>
>> >>> Do not quite like it... I'd rather asynchronous request give out
>a
>> >>> firmware status pointer that could be used later on.
>>
>> Excellent. Why not get rid of the callback function as well and have
>> fw_loading_wait() return result (0 = firmware available, < 0 = fail).
>> Just to confirm, you are proposing a new API function next to
>> request_firmware_nowait(), right?
>
>If proposing new firmware_class patches please bounce / Cc me, I've
>recently asked for me to be added to MAINTAINERS so I get these
>e-mails as I'm working on a new flexible API which would allow us
>to extend the firmware API without having to care about the old
>stupid usermode helper at all.
I am not sure why we started calling usermode helper "stupid". We only had to implement direct kernel firmware loading because udev/stsremd folks had "interesting" ideas how events should be handled; but having userspace to feed us data is not stupid.
If we want to overhaul firmware loading support we need to figure out how to support case when a driver want to [asynchronously] request firmware/config/blob and the rest of the system is not ready. Even if we want kernel to do read/load the data we need userspace to tell kernel when firmware partition is available, until then the kernel should not fail the request.
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [RFC v0 7/8] Input: ims-pcu: use firmware_stat instead of completion
From: Dmitry Torokhov @ 2016-07-31 7:17 UTC (permalink / raw)
To: Arend van Spriel, Daniel Wagner, Bjorn Andersson
Cc: Daniel Wagner, Bastien Nocera, Greg Kroah-Hartman, Johannes Berg,
Kalle Valo, Ohad Ben-Cohen, linux-input, linux-kselftest,
linux-wireless, linux-kernel, Luis R. Rodriguez
In-Reply-To: <ee9a29aa-0d29-a33f-e43a-057dc359cf89@broadcom.com>
On July 30, 2016 5:42:41 AM PDT, Arend van Spriel <arend.vanspriel@broadcom.com> wrote:
>+ Luis (again) ;-)
>
>On 29-07-16 08:13, Daniel Wagner wrote:
>> On 07/28/2016 09:01 PM, Bjorn Andersson wrote:
>>> On Thu 28 Jul 11:33 PDT 2016, Dmitry Torokhov wrote:
>>>
>>>> On Thu, Jul 28, 2016 at 09:55:11AM +0200, Daniel Wagner wrote:
>>>>> From: Daniel Wagner <daniel.wagner@bmw-carit.de>
>>>>>
>>> [..]
>>>>
>>>> Do not quite like it... I'd rather asynchronous request give out a
>>>> firmware status pointer that could be used later on.
>
>Excellent. Why not get rid of the callback function as well and have
>fw_loading_wait() return result (0 = firmware available, < 0 = fail).
>Just to confirm, you are proposing a new API function next to
>request_firmware_nowait(), right?
Yes, that would be a new API call. Maybe we could replace old API with the new at some point.
>>>> pcu->fw_st = request_firmware_async(IMS_PCU_FIRMWARE_NAME,
>>>> - pcu,
>>>> - ims_pcu_process_async_firmware);
> + pcu);
>>>> if (IS_ERR(pcu->fw_st))
>>>> return PTR_ERR(pcu->fw_st);
>>>>
>>>> ....
>>>>
>>>> err = fw_loading_wait(pcu->fw_st);
> if (err)
> return err;
>
> fw = fwstat_get_firmware(pcu->fw_st);
>
>Or whatever consistent prefix it is going to be.
>
>>>>
>>>
>>> In the remoteproc case (patch 6) this would clean up the code,
>rather
>>> than replacing the completion API 1 to 1. I like it!
>>
>> IIRC most drivers do it the same way. So request_firmware_async()
>indeed
>> would be good thing to have. Let me try that.
>
>While the idea behind this series is a good one I am wondering about
>the
>need for these drivers to use the asynchronous API. The historic reason
>might be to avoid timeout caused by user-mode helper, but that may no
>longer apply and these drivers could be better off using
>request_firmware_direct().
Actually systems using this driver rely on usermode helper to provide necessary delay and load the firmware from storage once root partition is mounted. Converting to request_firmware_direct() would break them.
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [RFC v0 7/8] Input: ims-pcu: use firmware_stat instead of completion
From: Luis R. Rodriguez @ 2016-07-30 16:58 UTC (permalink / raw)
To: Arend van Spriel
Cc: Daniel Wagner, Bjorn Andersson, Dmitry Torokhov, Daniel Wagner,
Bastien Nocera, Greg Kroah-Hartman, Johannes Berg, Kalle Valo,
Ohad Ben-Cohen, linux-input, linux-kselftest, linux-wireless,
linux-kernel, Luis R. Rodriguez
In-Reply-To: <ee9a29aa-0d29-a33f-e43a-057dc359cf89@broadcom.com>
On Sat, Jul 30, 2016 at 02:42:41PM +0200, Arend van Spriel wrote:
> + Luis (again) ;-)
>
> On 29-07-16 08:13, Daniel Wagner wrote:
> > On 07/28/2016 09:01 PM, Bjorn Andersson wrote:
> >> On Thu 28 Jul 11:33 PDT 2016, Dmitry Torokhov wrote:
> >>
> >>> On Thu, Jul 28, 2016 at 09:55:11AM +0200, Daniel Wagner wrote:
> >>>> From: Daniel Wagner <daniel.wagner@bmw-carit.de>
> >>>>
> >> [..]
> >>>
> >>> Do not quite like it... I'd rather asynchronous request give out a
> >>> firmware status pointer that could be used later on.
>
> Excellent. Why not get rid of the callback function as well and have
> fw_loading_wait() return result (0 = firmware available, < 0 = fail).
> Just to confirm, you are proposing a new API function next to
> request_firmware_nowait(), right?
If proposing new firmware_class patches please bounce / Cc me, I've
recently asked for me to be added to MAINTAINERS so I get these
e-mails as I'm working on a new flexible API which would allow us
to extend the firmware API without having to care about the old
stupid usermode helper at all.
> >>> pcu->fw_st = request_firmware_async(IMS_PCU_FIRMWARE_NAME,
> >>> - pcu,
> >>> - ims_pcu_process_async_firmware);
> + pcu);
> >>> if (IS_ERR(pcu->fw_st))
> >>> return PTR_ERR(pcu->fw_st);
> >>>
> >>> ....
> >>>
> >>> err = fw_loading_wait(pcu->fw_st);
> if (err)
> return err;
>
> fw = fwstat_get_firmware(pcu->fw_st);
>
> Or whatever consistent prefix it is going to be.
>
> >>>
> >>
> >> In the remoteproc case (patch 6) this would clean up the code, rather
> >> than replacing the completion API 1 to 1. I like it!
> >
> > IIRC most drivers do it the same way. So request_firmware_async() indeed
> > would be good thing to have. Let me try that.
>
> While the idea behind this series is a good one I am wondering about the
> need for these drivers to use the asynchronous API. The historic reason
> might be to avoid timeout caused by user-mode helper, but that may no
> longer apply and these drivers could be better off using
> request_firmware_direct().
BTW I have in my queue for the sysdata API something like firmware_request_direct()
but with async support. The only thing left to do I think is just add the devm
helpers so drivers no longer need to worry about the release of the firmware.
> There have been numerous discussions about the firmware API. Here most
> recent one:
>
> http://www.spinics.net/lists/linux-wireless/index.html#152755
And more importantly, the sysdata API queue:
https://git.kernel.org/cgit/linux/kernel/git/mcgrof/linux-next.git/log/?h=20160616-sysdata-v2
Luis
^ permalink raw reply
* Re: [RFC v0 7/8] Input: ims-pcu: use firmware_stat instead of completion
From: Arend van Spriel @ 2016-07-30 12:42 UTC (permalink / raw)
To: Daniel Wagner, Bjorn Andersson, Dmitry Torokhov
Cc: Daniel Wagner, Bastien Nocera, Greg Kroah-Hartman, Johannes Berg,
Kalle Valo, Ohad Ben-Cohen, linux-input, linux-kselftest,
linux-wireless, linux-kernel, Luis R. Rodriguez
In-Reply-To: <c05a7b98-5ada-701f-9a77-65d81b841c17@bmw-carit.de>
+ Luis (again) ;-)
On 29-07-16 08:13, Daniel Wagner wrote:
> On 07/28/2016 09:01 PM, Bjorn Andersson wrote:
>> On Thu 28 Jul 11:33 PDT 2016, Dmitry Torokhov wrote:
>>
>>> On Thu, Jul 28, 2016 at 09:55:11AM +0200, Daniel Wagner wrote:
>>>> From: Daniel Wagner <daniel.wagner@bmw-carit.de>
>>>>
>> [..]
>>>
>>> Do not quite like it... I'd rather asynchronous request give out a
>>> firmware status pointer that could be used later on.
Excellent. Why not get rid of the callback function as well and have
fw_loading_wait() return result (0 = firmware available, < 0 = fail).
Just to confirm, you are proposing a new API function next to
request_firmware_nowait(), right?
>>> pcu->fw_st = request_firmware_async(IMS_PCU_FIRMWARE_NAME,
>>> - pcu,
>>> - ims_pcu_process_async_firmware);
+ pcu);
>>> if (IS_ERR(pcu->fw_st))
>>> return PTR_ERR(pcu->fw_st);
>>>
>>> ....
>>>
>>> err = fw_loading_wait(pcu->fw_st);
if (err)
return err;
fw = fwstat_get_firmware(pcu->fw_st);
Or whatever consistent prefix it is going to be.
>>>
>>
>> In the remoteproc case (patch 6) this would clean up the code, rather
>> than replacing the completion API 1 to 1. I like it!
>
> IIRC most drivers do it the same way. So request_firmware_async() indeed
> would be good thing to have. Let me try that.
While the idea behind this series is a good one I am wondering about the
need for these drivers to use the asynchronous API. The historic reason
might be to avoid timeout caused by user-mode helper, but that may no
longer apply and these drivers could be better off using
request_firmware_direct().
There have been numerous discussions about the firmware API. Here most
recent one:
http://www.spinics.net/lists/linux-wireless/index.html#152755
Regards,
Arend
> Thanks for the excellent feedback.
>
> cheers,
> daniel
> --
> To unsubscribe from this list: send the line "unsubscribe
> linux-wireless" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH] ath10k: Allow setting coverage class
From: Sebastian Gottschall @ 2016-07-29 16:38 UTC (permalink / raw)
To: Ben Greear, Benjamin Berg, Kalle Valo, linux-wireless
Cc: Mathias Kretschmer, ath10k, Simon Wunderlich
In-Reply-To: <579B71B4.7080206@candelatech.com>
if these sourcecodes would be just up to date with the binaries provided
by qca. but qca doesnt seem to care that much about it.
this is mainly also the reason why there are soo many long term
stability issues with no resolution
Am 29.07.2016 um 17:09 schrieb Ben Greear:
> On 07/29/2016 07:52 AM, Benjamin Berg wrote:
>> On Mi, 2016-07-27 at 10:26 -0700, Ben Greear wrote:
>>> On 07/27/2016 01:33 AM, Benjamin Berg wrote:
>>>>
>>>> Unfortunately ath10k does not generally allow modifying the
>>>> coverage class
>>>> with the stock firmware and Qualcomm has so far refused to
>>>> implement this
>>>> feature so that it can be properly supported in ath10k. If we
>>>> however know
>>>> the registers that need to be modified for proper operation with a
>>>> higher
>>>> coverage class, then we can do these modifications from the driver.
>>>>
>>>> This patch implements this hack for first generation cards which
>>>> are based
>>>> on a core that is similar to ath9k. The registers are modified in
>>>> place and
>>>> need to be re-written every time the firmware sets them. To achieve
>>>> this
>>>> the register status is verified after any event from the firmware.
>>>>
>>>> The coverage class may not be modified temporarily right after the
>>>> card
>>>> re-initializes the registers. This is for example the case during
>>>> scanning.
>>>>
>>>> A warning will be generated if the hack is not supported on the
>>>> card or
>>>> unexpected values are hit. There is no error reporting for userspace
>>>> applications though (this is a limitation in the mac80211 driver
>>>> interface).
>>>>
>>>>>> Thanks to Sebastian Gottschall <s.gottschall@dd-wrt.com> for
>>>>>> initially
>>>> working on a userspace support for this. This patch wouldn't have been
>>>> possible without this documentation.
>>>
>>> I would be concerned about the various resets firmware does to work
>>> around
>>> hardware hangs as well. I don't think any events are generated for
>>> these, unless
>>> you count the dbglog messages?
>>
>> Yeah, I am aware of the fact that the firmware may do internal resets
>> from time to time. The interesting question (and one for which I do not
>> know the answer) is whether we get a wmi or other event under all
>> conditions where the register may be rewritten due to a reset.
>>
>> The current code will re-set the register value after any wmi event
>> including debug messages. If this is not enough, then the only solution
>> might be to periodically poll the register values instead of relying on
>> a received event.
>
> You will get a dbglog event at least most of the time, so maybe that
> will be good enough.
>
> Someone with src code and that cared could audit the code I guess, but
> then
> that person could also just implement the feature properly in the
> firmware
> to begin with...
>
> Thanks,
> Ben
>
--
Mit freundlichen Grüssen / Regards
Sebastian Gottschall / CTO
NewMedia-NET GmbH - DD-WRT
Firmensitz: Berliner Ring 101, 64625 Bensheim
Registergericht: Amtsgericht Darmstadt, HRB 25473
Geschäftsführer: Peter Steinhäuser, Christian Scheele
http://www.dd-wrt.com
email: s.gottschall@dd-wrt.com
Tel.: +496251-582650 / Fax: +496251-5826565
^ permalink raw reply
* Dell Venue 8 Pro "Dell Wireless" (Atheros 6k) chipset with SDIO ID 0271:0418 not recognized/supported
From: russianneuromancer @ 2016-07-29 15:19 UTC (permalink / raw)
To: ath6kl; +Cc: linux-wireless
Hi
Please look into commentary 50 from bugreport https://bugzilla.kernel.org/show_bug.cgi?id=67921#c50
There is patch for enabling support of Atheros Wireless adapter in Dell Venue 8 Pro tablet.
Please review if this possible to include this patch into upstream.
^ permalink raw reply
* Re: [PATCH] ath10k: Allow setting coverage class
From: Ben Greear @ 2016-07-29 15:09 UTC (permalink / raw)
To: Benjamin Berg, Kalle Valo, linux-wireless
Cc: Mathias Kretschmer, Sebastian Gottschall, ath10k,
Simon Wunderlich
In-Reply-To: <1469803947.7306.139.camel@sipsolutions.net>
On 07/29/2016 07:52 AM, Benjamin Berg wrote:
> On Mi, 2016-07-27 at 10:26 -0700, Ben Greear wrote:
>> On 07/27/2016 01:33 AM, Benjamin Berg wrote:
>>>
>>> Unfortunately ath10k does not generally allow modifying the coverage class
>>> with the stock firmware and Qualcomm has so far refused to implement this
>>> feature so that it can be properly supported in ath10k. If we however know
>>> the registers that need to be modified for proper operation with a higher
>>> coverage class, then we can do these modifications from the driver.
>>>
>>> This patch implements this hack for first generation cards which are based
>>> on a core that is similar to ath9k. The registers are modified in place and
>>> need to be re-written every time the firmware sets them. To achieve this
>>> the register status is verified after any event from the firmware.
>>>
>>> The coverage class may not be modified temporarily right after the card
>>> re-initializes the registers. This is for example the case during scanning.
>>>
>>> A warning will be generated if the hack is not supported on the card or
>>> unexpected values are hit. There is no error reporting for userspace
>>> applications though (this is a limitation in the mac80211 driver
>>> interface).
>>>
>>>>> Thanks to Sebastian Gottschall <s.gottschall@dd-wrt.com> for initially
>>> working on a userspace support for this. This patch wouldn't have been
>>> possible without this documentation.
>>
>> I would be concerned about the various resets firmware does to work around
>> hardware hangs as well. I don't think any events are generated for these, unless
>> you count the dbglog messages?
>
> Yeah, I am aware of the fact that the firmware may do internal resets
> from time to time. The interesting question (and one for which I do not
> know the answer) is whether we get a wmi or other event under all
> conditions where the register may be rewritten due to a reset.
>
> The current code will re-set the register value after any wmi event
> including debug messages. If this is not enough, then the only solution
> might be to periodically poll the register values instead of relying on
> a received event.
You will get a dbglog event at least most of the time, so maybe that
will be good enough.
Someone with src code and that cared could audit the code I guess, but then
that person could also just implement the feature properly in the firmware
to begin with...
Thanks,
Ben
--
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc http://www.candelatech.com
^ permalink raw reply
* Re: [PATCH] ath10k: Allow setting coverage class
From: Benjamin Berg @ 2016-07-29 14:52 UTC (permalink / raw)
To: Ben Greear, Kalle Valo, linux-wireless
Cc: Mathias Kretschmer, Sebastian Gottschall, ath10k,
Simon Wunderlich
In-Reply-To: <5798EEC3.5040008@candelatech.com>
[-- Attachment #1: Type: text/plain, Size: 2153 bytes --]
On Mi, 2016-07-27 at 10:26 -0700, Ben Greear wrote:
> On 07/27/2016 01:33 AM, Benjamin Berg wrote:
> >
> > Unfortunately ath10k does not generally allow modifying the coverage class
> > with the stock firmware and Qualcomm has so far refused to implement this
> > feature so that it can be properly supported in ath10k. If we however know
> > the registers that need to be modified for proper operation with a higher
> > coverage class, then we can do these modifications from the driver.
> >
> > This patch implements this hack for first generation cards which are based
> > on a core that is similar to ath9k. The registers are modified in place and
> > need to be re-written every time the firmware sets them. To achieve this
> > the register status is verified after any event from the firmware.
> >
> > The coverage class may not be modified temporarily right after the card
> > re-initializes the registers. This is for example the case during scanning.
> >
> > A warning will be generated if the hack is not supported on the card or
> > unexpected values are hit. There is no error reporting for userspace
> > applications though (this is a limitation in the mac80211 driver
> > interface).
> >
> > > > Thanks to Sebastian Gottschall <s.gottschall@dd-wrt.com> for initially
> > working on a userspace support for this. This patch wouldn't have been
> > possible without this documentation.
>
> I would be concerned about the various resets firmware does to work around
> hardware hangs as well. I don't think any events are generated for these, unless
> you count the dbglog messages?
Yeah, I am aware of the fact that the firmware may do internal resets
from time to time. The interesting question (and one for which I do not
know the answer) is whether we get a wmi or other event under all
conditions where the register may be rewritten due to a reset.
The current code will re-set the register value after any wmi event
including debug messages. If this is not enough, then the only solution
might be to periodically poll the register values instead of relying on
a received event.
Benjamin
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* [PATCH] mwifiex: PCIe8997 chip specific handling
From: Amitkumar Karwar @ 2016-07-29 10:38 UTC (permalink / raw)
To: linux-wireless; +Cc: Cathy Luo, Nishant Sarmukadam, Amitkumar Karwar
The patch corrects the revision id register and uses it along with
magic value and chip version registers to download appropriate firmware
image.
PCIe8997 Z chipset variant code has been removed, as it won't be used in
production.
Signed-off-by: Amitkumar Karwar <akarwar@marvell.com>
---
drivers/net/wireless/marvell/mwifiex/pcie.c | 35 ++++++++++-------------------
drivers/net/wireless/marvell/mwifiex/pcie.h | 14 +++++-------
2 files changed, 18 insertions(+), 31 deletions(-)
diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.c b/drivers/net/wireless/marvell/mwifiex/pcie.c
index 81bceef..9aec9a9 100644
--- a/drivers/net/wireless/marvell/mwifiex/pcie.c
+++ b/drivers/net/wireless/marvell/mwifiex/pcie.c
@@ -2910,7 +2910,7 @@ static int mwifiex_pcie_request_irq(struct mwifiex_adapter *adapter)
static void mwifiex_pcie_get_fw_name(struct mwifiex_adapter *adapter)
{
int revision_id = 0;
- int version;
+ int version, magic;
struct pcie_service_card *card = adapter->card;
switch (card->dev->device) {
@@ -2935,30 +2935,19 @@ static void mwifiex_pcie_get_fw_name(struct mwifiex_adapter *adapter)
}
break;
case PCIE_DEVICE_ID_MARVELL_88W8997:
- mwifiex_read_reg(adapter, 0x0c48, &revision_id);
+ mwifiex_read_reg(adapter, 0x8, &revision_id);
mwifiex_read_reg(adapter, 0x0cd0, &version);
+ mwifiex_read_reg(adapter, 0x0cd4, &magic);
+ revision_id &= 0xff;
version &= 0x7;
- switch (revision_id) {
- case PCIE8997_V2:
- if (version == CHIP_VER_PCIEUART)
- strcpy(adapter->fw_name,
- PCIEUART8997_FW_NAME_V2);
- else
- strcpy(adapter->fw_name,
- PCIEUSB8997_FW_NAME_V2);
- break;
- case PCIE8997_Z:
- if (version == CHIP_VER_PCIEUART)
- strcpy(adapter->fw_name,
- PCIEUART8997_FW_NAME_Z);
- else
- strcpy(adapter->fw_name,
- PCIEUSB8997_FW_NAME_Z);
- break;
- default:
- strcpy(adapter->fw_name, PCIE8997_DEFAULT_FW_NAME);
- break;
- }
+ magic &= 0xff;
+ if (revision_id == PCIE8997_A1 &&
+ magic == CHIP_MAGIC_VALUE &&
+ version == CHIP_VER_PCIEUART)
+ strcpy(adapter->fw_name, PCIEUART8997_FW_NAME_V4);
+ else
+ strcpy(adapter->fw_name, PCIEUSB8997_FW_NAME_V4);
+ break;
default:
break;
}
diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.h b/drivers/net/wireless/marvell/mwifiex/pcie.h
index f6992f0..46f99ca 100644
--- a/drivers/net/wireless/marvell/mwifiex/pcie.h
+++ b/drivers/net/wireless/marvell/mwifiex/pcie.h
@@ -32,12 +32,9 @@
#define PCIE8897_DEFAULT_FW_NAME "mrvl/pcie8897_uapsta.bin"
#define PCIE8897_A0_FW_NAME "mrvl/pcie8897_uapsta_a0.bin"
#define PCIE8897_B0_FW_NAME "mrvl/pcie8897_uapsta.bin"
-#define PCIE8997_DEFAULT_FW_NAME "mrvl/pcieusb8997_combo_v2.bin"
-#define PCIEUART8997_FW_NAME_Z "mrvl/pcieuart8997_combo.bin"
-#define PCIEUART8997_FW_NAME_V2 "mrvl/pcieuart8997_combo_v2.bin"
-#define PCIEUSB8997_FW_NAME_Z "mrvl/pcieusb8997_combo.bin"
-#define PCIEUSB8997_FW_NAME_V2 "mrvl/pcieusb8997_combo_v2.bin"
-#define PCIE8997_DEFAULT_WIFIFW_NAME "mrvl/pcie8997_wlan.bin"
+#define PCIEUART8997_FW_NAME_V4 "mrvl/pcieuart8997_combo_v4.bin"
+#define PCIEUSB8997_FW_NAME_V4 "mrvl/pcieusb8997_combo_v4.bin"
+#define PCIE8997_DEFAULT_WIFIFW_NAME "mrvl/pcie8997_wlan_v4.bin"
#define PCIE_VENDOR_ID_MARVELL (0x11ab)
#define PCIE_VENDOR_ID_V2_MARVELL (0x1b4b)
@@ -47,9 +44,10 @@
#define PCIE8897_A0 0x1100
#define PCIE8897_B0 0x1200
-#define PCIE8997_Z 0x0
-#define PCIE8997_V2 0x471
+#define PCIE8997_A0 0x10
+#define PCIE8997_A1 0x11
#define CHIP_VER_PCIEUART 0x3
+#define CHIP_MAGIC_VALUE 0x24
/* Constants for Buffer Descriptor (BD) rings */
#define MWIFIEX_MAX_TXRX_BD 0x20
--
1.9.1
^ permalink raw reply related
* Re: [RFC v0 7/8] Input: ims-pcu: use firmware_stat instead of completion
From: Daniel Wagner @ 2016-07-29 6:13 UTC (permalink / raw)
To: Bjorn Andersson, Dmitry Torokhov
Cc: Daniel Wagner, Bastien Nocera, Greg Kroah-Hartman, Johannes Berg,
Kalle Valo, Ohad Ben-Cohen, linux-input, linux-kselftest,
linux-wireless, linux-kernel
In-Reply-To: <20160728190151.GV13516@tuxbot>
On 07/28/2016 09:01 PM, Bjorn Andersson wrote:
> On Thu 28 Jul 11:33 PDT 2016, Dmitry Torokhov wrote:
>
>> On Thu, Jul 28, 2016 at 09:55:11AM +0200, Daniel Wagner wrote:
>>> From: Daniel Wagner <daniel.wagner@bmw-carit.de>
>>>
> [..]
>>
>> Do not quite like it... I'd rather asynchronous request give out a
>> firmware status pointer that could be used later on.
>>
>> pcu->fw_st = request_firmware_async(IMS_PCU_FIRMWARE_NAME,
>> pcu,
>> ims_pcu_process_async_firmware);
>> if (IS_ERR(pcu->fw_st))
>> return PTR_ERR(pcu->fw_st);
>>
>> ....
>>
>> fw_loading_wait(pcu->fw_st);
>>
>
> In the remoteproc case (patch 6) this would clean up the code, rather
> than replacing the completion API 1 to 1. I like it!
IIRC most drivers do it the same way. So request_firmware_async() indeed
would be good thing to have. Let me try that.
Thanks for the excellent feedback.
cheers,
daniel
^ permalink raw reply
* Re: [RFC v0 3/8] firmware: Factor out firmware load helpers
From: Daniel Wagner @ 2016-07-29 6:08 UTC (permalink / raw)
To: Dmitry Torokhov, Daniel Wagner
Cc: Bastien Nocera, Bjorn Andersson, Greg Kroah-Hartman,
Johannes Berg, Kalle Valo, Ohad Ben-Cohen, linux-input,
linux-kselftest, linux-wireless, linux-kernel
In-Reply-To: <20160728175706.GB16852@dtor-ws>
On 07/28/2016 07:57 PM, Dmitry Torokhov wrote:
> On Thu, Jul 28, 2016 at 09:55:07AM +0200, Daniel Wagner wrote:
>> +int __firmware_stat_wait(struct firmware_stat *fwst,
>> + long timeout)
>> +{
>> + int err;
>> + err = swait_event_interruptible_timeout(fwst->wq,
>> + is_fw_sync_done(READ_ONCE(fwst->status)),
>> + timeout);
>> + if (err == 0 && fwst->status == FW_STATUS_ABORT)
>> + return -ENOENT;
>> +
>> + return err;
>> +}
>> +EXPORT_SYMBOL(__firmware_stat_wait);
>> +
>> +void __firmware_stat_set(struct firmware_stat *fwst, unsigned long status)
>> +{
>> + WRITE_ONCE(fwst->status, status);
>> + swake_up(&fwst->wq);
>
> Do we need to notify everyone for FW_STATUS_LOADING status?
Hmm, I don't think so. In the end drivers are probably only interested
in the final result which is either success or fail.
cheers,
daniel
^ permalink raw reply
* Re: [RFC v0 3/8] firmware: Factor out firmware load helpers
From: Daniel Wagner @ 2016-07-29 6:07 UTC (permalink / raw)
To: Dan Williams, Daniel Wagner, Bastien Nocera, Bjorn Andersson,
Dmitry Torokhov, Greg Kroah-Hartman, Johannes Berg, Kalle Valo,
Ohad Ben-Cohen
Cc: linux-input, linux-kselftest, linux-wireless, linux-kernel
In-Reply-To: <1469718061.3013.18.camel@redhat.com>
> It's somewhat odd to me that the structure is "firmware_stat" but most
> of the functions are "fw_loading_*". That seems inconsistent for a
> structure that is (currently) only used by these functions.
I agree, my proposal is odd.
> I would personally do either:
>
> a) "struct fw_load_status" and "fw_load_*()"
>
> or
>
> b) "struct firmware_load_stat" and "firmware_load_*()"
>
> I'd also change the function names from "loading" -> "load", similar to
> how userland has read(2), not reading(2).
a) sounds good to me, because fw_load_ should be long enough prefix.
cheers,
daniel
^ permalink raw reply
* Re: brcm4330 fails to load on newer kernels
From: Fabio Estevam @ 2016-07-29 1:05 UTC (permalink / raw)
To: Arend van Spriel
Cc: Arend van Spriel, Hante Meuleman, linux-wireless,
brcm80211-dev-list
In-Reply-To: <814ae6d0-2375-e8bd-f3e3-98ec5f69b376@broadcom.com>
Hi Arend,
On Thu, Jul 28, 2016 at 3:37 PM, Arend van Spriel
<arend.vanspriel@broadcom.com> wrote:
> Hi Fabio,
>
> So this is another fine example of firmware API not able to deliver. I
> think in all these kernels you have the same issue. The problem is that
> the order of events upon kernel boot is not predictable. In this case
> you have rootfs being mounted and brcmfmac getting probed as the two
> competing events. When rootfs is mounted before brcmfmac is being probed
> it works, but if brcmfmac is probed before rootfs is mounted the
> firmware request will fail. So the only reliable option for built-in
> drivers requiring firmware is to built-in the firmware into the kernel
> as well.
Thanks for your explanation.
Tried building brcmfmac as module and after doing 'modprobe brcmfmac'
the firmware is correctly loaded from the rootfs in all the kernels I
tested.
Now I just need it to load brcmfmac module automatically, but this is
a a separate issue I will investigate.
Thanks a lot for your help!
Fabio Estevam
^ permalink raw reply
* Re: [RFC] ath10k: silence firmware file probing warnings
From: Luis R. Rodriguez @ 2016-07-28 23:28 UTC (permalink / raw)
To: Arend van Spriel, Linus Torvalds
Cc: Luis R. Rodriguez, Stanislaw Gruszka, Prarit Bhargava,
Emmanuel Grumbach, Michal Kazior, Kalle Valo, linux-wireless,
ath10k, Arend van Spriel, Greg Kroah-Hartman, Ming Lei, mmarek,
linux-kernel@vger.kernel.org, Takashi Iwai, Tom Gundersen,
Kay Sievers, David Woodhouse, Jej B, Kees Cook, Fengguang Wu,
Julia Lawall, Mimi Zohar, Rusty Russell, Dmitry Torokhov,
Mauro Carvalho Chehab, Laurent Pinchart, Hannes Reinecke,
rafael.j.wysocki
In-Reply-To: <c3bbb2f3-f551-41bf-ba29-c75f78b5b9ed@broadcom.com>
On Thu, Jul 28, 2016 at 09:23:35PM +0200, Arend van Spriel wrote:
> On 23-07-16 00:05, Luis R. Rodriguez wrote:
> > The firmware API is a mess and I've been trying to correct that
> > with a more flexible API.
<-- snip -->
> > Extensions to the fw API are IMHO best done through a newer flexible
> > API, feel free to refer to this development tree if you'd like to
> > contribute:
> >
> > https://git.kernel.org/cgit/linux/kernel/git/mcgrof/linux-next.git/log/?h=20160616-sysdata-v2
>
> So I had a look and noticed commit c8df68e83392 ("firmware: annotate
> thou shalt not request fw on init or probe"). Now this conflicts with
> our wireless driver. The original suggestion a long, long time ago was
> to use IFF_UP as trigger to go and request firmware. However, for that
> we would need to register a netdevice during probe, and consequently we
> should also have a wiphy instance registered. However, that has all kind
> of feature flags for which we need firmware running on the device to
> query what is supported and what not. I can make a fair bet that
> brcmfmac is not the only driver with such a requirement. So how can we
> crack that nut.
Glad you asked.
Despite my latest set of updates on documentation for the firmware_class [0] (not
yet merged), and it being based on the latest discussed and agreed upon we
really have nothing well ironed out for what you describe, so let's try
to figure that out now.
To be clear, folks had originally said using the firmware API on *init* was
dumb, and some of this came up because of the infamous systemd udev timeout.
For completeness, I've documented some of the history of this issue
in great detail [1], mostly because I had to deal with a bug at SUSE about
this, and find a proper solution. Avoiding re-iterating *exactly why* the
timeout for kmod was ill-founded, and assuming you all now buy that,
the summary of facts of the *why* it turns out to be a bad idea to use the
firmware API on init *or* probe:
a) by default the driver model actually calls both init and probe serially and
synchronously
b) we have no deterministic way of being certain we have whatever filesystem
the driver needed as ready, the firmware may live in initramfs, or may only
be available later on the real filesystem, and even after that the system
may be designed to pivot_root.
In terms of solutions, lets discuss, here are a few options I can think of:
1) Because of b) if you know you are going to use the firmware API you should
just stuff firmware that is needed on init or probe as built-in
(CONFIG_EXTRA_FIRMWARE) or stuff it into initramfs. This approach is generally
accepted, however there is no systematic approach to ensure this is done. Now
that we have coccinelle grammar to find these drivers it should be relatively
easy to identify them and require the firmware as part of the distribution's
initramfs or peg them as part of CONFIG_EXTRA_FIRMWARE if a distro prefers that.
The only issue with this approach is its left up to the distribution to resolve.
2) If the driver *knows* that probe may take a while it can request the driver core
to probe the driver asynchronously, it can do so by using:
static struct pci_driver foo_pci_driver = {
...
.driver.probe_type = PROBE_PREFER_ASYNCHRONOUS,
};
This would not really resolve the deterministic issues listed in b) and for
this reason this is not really a firmware-API-on-probe solution -- its an
annotation to help avoid delays boot due to knowing probe may take a while.
3) Userspace that loads modules can / should pass the async probe generic
module parameter "async_probe" (for instance 'modprobe ath10k async_probe')
when loading all modules. This should already be relatively
safe when using on modules. This is what systemd long ago assumed was
being done anyway. Again, also this is not really a firmware-API-on-probe solution,
it can however be used by systemd for instance to help avoid delays on
boot due to module lengthy probe calls
As it stands we have no resolution for the deterministic existential issues of
the firmware but in practice 1-3 should help. 2-3 should probably be done
regardless. I've been meaning to send patches for 3) but haven't had time.
As far as the deterministic existential firmware issue... Since we're just
reading files from the filesystem now (there are exceptions to this, but my
goal is to corner-case this code with the sysdata API), if we really wanted
something rock solid for these drivers I suppose we could consider implementing
an event driven probe if a driver requests for async probe. For instance, if
async probe was requested and the driver has MODULE_FIRMWARE(firmware_name)
we could add a notifier to probe the driver once the firmware is accessible.
For all intents and purposes though -- although I know the real issue here
the deterministic way of knowing when firmware is available, in practice
annotating your driver with PROBE_PREFER_ASYNCHRONOUS should solve most
races. This would not be a resolution, but rather an annotation to the fact
your driver probe does take a while -- and should make most folks happy until
we have a proper deterministic firmware solution, I think.
I welcome other ideas.
[0] https://lkml.kernel.org/r/1466117661-22075-1-git-send-email-mcgrof@kernel.org
[1] http://www.do-not-panic.com/2015/12/linux-asynchronous-probe.html
Luis
^ permalink raw reply
* Re: [RFC] ath10k: silence firmware file probing warnings
From: Arend van Spriel @ 2016-07-28 19:23 UTC (permalink / raw)
To: Luis R. Rodriguez, Stanislaw Gruszka
Cc: Prarit Bhargava, Emmanuel Grumbach, Michal Kazior, Kalle Valo,
linux-wireless, ath10k, Arend van Spriel, Greg Kroah-Hartman,
Ming Lei, mmarek
In-Reply-To: <20160722221523.GM5537@wotan.suse.de>
On 23-07-16 00:15, Luis R. Rodriguez wrote:
> On Fri, Jul 22, 2016 at 12:26:00PM +0200, Stanislaw Gruszka wrote:
>> On Fri, Jul 22, 2016 at 10:38:24AM +0200, Arend Van Spriel wrote:
>>> + Luis
>>>
>>> On 21-7-2016 13:51, Stanislaw Gruszka wrote:
>>>> (cc: firmware and brcmfmac maintainers)
>>>>
>>>> On Thu, Jul 21, 2016 at 06:23:11AM -0400, Prarit Bhargava wrote:
>>>>>
>>>>>
>>>>> On 07/21/2016 04:05 AM, Stanislaw Gruszka wrote:
>>>>>> On Thu, Jul 21, 2016 at 10:36:42AM +0300, Emmanuel Grumbach wrote:
>>>>>>> On Thu, Jul 21, 2016 at 10:09 AM, Stanislaw Gruszka <sgruszka@redhat.com> wrote:
>>>>>>>> On Tue, Jul 19, 2016 at 03:00:37PM +0200, Michal Kazior wrote:
>>>>>>>>> Firmware files are versioned to prevent older
>>>>>>>>> driver instances to load unsupported firmware
>>>>>>>>> blobs. This is reflected with a fallback logic
>>>>>>>>> which attempts to load several firmware files.
>>>>>>>>>
>>>>>>>>> This however produced a lot of unnecessary
>>>>>>>>> warnings sometimes confusing users and leading
>>>>>>>>> them to rename firmware files making things even
>>>>>>>>> more confusing.
>>>>>>>>
>>>>>>>> This happens on kernels configured with
>>>>>>>> CONFIG_FW_LOADER_USER_HELPER_FALLBACK and cause not only ugly warnings,
>>>>>>>> but also 60 seconds delay before loading next firmware version.
>>>>>>>> For some reason RHEL kernel needs above config option, so this
>>>>>>>> patch is very welcome from my perspective.
>>>>>>>>
>>>>>>>
>>>>>>> Sorry for my ignorance but how does the firmware loading work if not
>>>>>>> with udev's help?
>>>>>>
>>>>>> I'm not sure exactly, but I think kernel VFS layer is capable to copy
>>>>>> file data directly from mounted filesystem without user space helper.
>>>>>
>>>>> Here's the situation: request_firmware() waits 60 seconds for udev to do its
>>>>> loading magic via a "usermode helper". This delay is there to allow, for
>>>>> example, userspace to unpack or download a new firmware image or verify the
>>>>> firmware image *in userspace* before providing it to the driver to apply to the HW.
>>>>>
>>>>> Why 60 seconds? It is arbitrary and there is no way for udev & the kernel to
>>>>> handshake on completion.
>>>>>
>>>>>>
>>>>>>> As you can imagine, iwlwifi is suffering from the
>>>>>>> same problem and I would be interested in applying the same change,
>>>>>>> but I'd love to understand a bit more :)
>>>>>>
>>>>>> Yes, iwlwifi (and some other drivers) suffer from this. However this
>>>>>> happen when the newest firmware version is not installed on the system
>>>>>> and CONFIG_FW_LOADER_USER_HELPER_FALLBACK is enabled. What I suppose
>>>>>> it's not common.
>>>>>
>>>>> request_firmware_direct() was introduced at my request because (as you've
>>>>> noticed) when CONFIG_FW_LOADER_USER_HELPER_FALLBACK=y drivers may stall for long
>>>>> periods of time when starting. The bug that this introduced was a 60 second
>>>>> delay per logical cpu when starting a system. On a 64 cpu system that meant the
>>>>> boot would complete in a little over one hour.
>>>>>
>>>>>>
>>>>>> I started to see this currently, because that option was enabled on
>>>>>> RHEL kernel. BTW: I think Prarit iwlwifi thermal_zone problem was
>>>>>> happened because of that, i.e. thermal device was not functional
>>>>>> because f/w wasn't loaded due to big delay.
>>>>>>
>>>>>> I'm not sure if replacing to request_firmware_direct() is a good
>>>>>> fix though. For example I can see this problem also on brcmfmac, which
>>>>>> use request_firmware_nowait(). I think I would rather prefer special
>>>>>> helper for firmware drivers that needs user helper and have
>>>>>> request_firmware() be direct as default.
>>>>>>
>>>>>
>>>>> The difference between request_firmware_direct() and request_firmware() is that
>>>>> the _direct() version does not wait the 60 seconds for udev interaction. The
>>>>> only userspace check performed is to see if the file is there, and if the file
>>>>> does exist it is provided to the driver to be applied to the hardware.
>>>>>
>>>>> So the real question to ask here is whether or not the ath10k, brcmfmac, and
>>>>> iwlwifi require udev to do anything beyond checking for the existence and
>>>>> loading the firmware image. If they don't, then it is better to use
>>>>> request_firmware_direct().
>>>>
>>>> They don't need that, like 99% of the drivers I think, hence changing the
>>>> default seems to be more reasonable. However changing 3 drivers would work
>>>> for me as well, and that change do not introduce risk of broking drivers
>>>> that require udev fw download.
>>>>
>>>> iwlwifi and ath10k are trivial, bcrmfmac is a bit more complex as it
>>>> use request_firmware_nowait(), so it first need to be converted to
>>>> ordinary request_firmware(), but this should be doable and I can do
>>>> that.
>>>
>>> I am going bonkers here. This is the Nth time a discussion pops up on
>>> firmware API usage. I stopped counting N :-( So the first issue was that
>>> the INIT was taking to long as we were requesting firmware during probe
>>> which was executed in the INIT context. So we added a worker and
>>> register the driver from there. There was probably a reason for
>>> switching to _no_wait() as well, but I do not recall the details. The
>>> things is I don't know if I need user-space or not. I just need firmware
>>> to get the device up and running. We have changed our driver a couple of
>>> times now to accommodate something that in my opinion should have been
>>> abstracted behind the firmware API in the first place and now here is
>>> another proposal to change the drivers. Come on!
>>
>> I understand you dislike that :-) Just to clarify the issue here:
>>
>> Some drivers (including brcmfmac) request new firmware images, which are
>> not yet available (i.e. development F/W versions) and then fall-back
>> to older firmware version and works perfectly fine.
>
> The right way to address this of course is to extend the FW API
> so that a batch of firmware files can be hunted for and one can
> easily annotate as a driver developer which are optional and which are
> not. The API then will do everything for you.
That is more or less how I abstracted it in brcmfmac. Our batch only
consists of max two files with binary executable image and rf
calibration data, which may or may not be optional. So we do not have
firmware fallback sequence.
For iwlwifi and ath10k I guess the use-case is that the batch is an
ordered list from newest supported firmware to oldest supported firmware.
> I was considering this as a future extension to the firmware API
> through the new extensible firmware API, the sysdata API. I'd prefer
> to add that as a secondary step, but if someone wants to add support
> for it now feel free to send me some patches against my tree.
>
>> However with CONFIG_FW_LOADER_USER_HELPER_FALLBACK=y configured, in case
>> of missing F/W image, request firmware involve user space helper and
>> waits 60s (loading_timeout value from drivers/base/firmware_class.c),
>> what delays creating network interface and confuse users.
>
> Correct, this has been a holy mess. One way to fix this on userspace
> is to upgrade systemd with an exception to the udev kmod helper,
> and avoid the timeout completely for kmod helper which loads drivers.
>
> CONFIG_FW_LOADER_USER_HELPER_FALLBACK should be avoided.
>
>> For brcmfmac this looks like this:
>>
>> [ 15.160923] brcmfmac 0000:03:00.0: Direct firmware load for brcm/brcmfmac4356-pcie.txt failed with error -2
>> [ 15.170759] brcmfmac 0000:03:00.0: Falling back to user helper
>> <snip>
>> [ 75.709397] brcmfmac: brcmf_c_preinit_dcmds: Firmware version = wl0: Oct 22 2015 06:16:41 version 7.35.180.119 (r594535) FWID 01-1a5c4016
>> [ 75.736941] brcmfmac: brcmf_cfg80211_reg_notifier: not a ISO3166 code (0x30 0x30)
>>
>> Without CONFIG_FW_LOADER_USER_HELPER_FALLBACK first firmware request
>> silently fail and then instantly next F/W image is loaded.
>
> Yup.
>
>> Another option to solve to problem would be stop requesting not
>> available publicly firmware. However, I assume some drivers would
>> like to preserve that option.
>
> No, this seems counter productive given that the firmware is expected
> to land eventually.
>
>>>> However I wonder if changing that will not broke the case when
>>>> driver is build-in in the kernel and f/w is not yet available when
>>>> driver start to initialize. Or maybe nowadays this is not the case
>>>> any longer, i.e. the MODULE_FIRMWARE macros assure proper f/w
>>>> images are build-in in the kernel or copied to initramfs?
>>>
>>> That is a nice idea, but I have not seen any change in that area. Could
>>> have missed it.
>>
>> I believe this is how the things are already done, IOW switching to
>> request_firmware_direct() in the driver should be no harm.
>
> We don't stuff firmware as built-in if the driver used MODULE_FIRMWARE()
> and the driver is built-in. What commit did that?
>
> The right thing to do is distros should avoid
> CONFIG_FW_LOADER_USER_HELPER_FALLBACK and if they are stuck with it, a
> systemd work around is possible. Upstream systemd already increased
> the timeout to 180 seconds. Upstream also added a udevd --event-timeout
> command line option, look into that. Other distros (OpenSUSE) avoids
> the kmod timeout ;)
>
> The timeout thing was simply a mistake, specially for kmod and it wasn't
> well thought out. I've listed more serious implications for it here:
>
> http://www.do-not-panic.com/2015/12/linux-asynchronous-probe.html
Thanks. Will read that.
Regards,
Arend
^ permalink raw reply
* Re: [RFC] ath10k: silence firmware file probing warnings
From: Arend van Spriel @ 2016-07-28 19:23 UTC (permalink / raw)
To: Luis R. Rodriguez
Cc: Stanislaw Gruszka, Prarit Bhargava, Emmanuel Grumbach,
Michal Kazior, Kalle Valo, linux-wireless, ath10k,
Arend van Spriel, Greg Kroah-Hartman, Ming Lei, mmarek
In-Reply-To: <20160722220526.GL5537@wotan.suse.de>
On 23-07-16 00:05, Luis R. Rodriguez wrote:
> On Fri, Jul 22, 2016 at 10:38:24AM +0200, Arend Van Spriel wrote:
>> + Luis
>>
>> On 21-7-2016 13:51, Stanislaw Gruszka wrote:
>>> (cc: firmware and brcmfmac maintainers)
>>>
>>> On Thu, Jul 21, 2016 at 06:23:11AM -0400, Prarit Bhargava wrote:
>>>>
>>>>
>>>> On 07/21/2016 04:05 AM, Stanislaw Gruszka wrote:
>>>>> On Thu, Jul 21, 2016 at 10:36:42AM +0300, Emmanuel Grumbach wrote:
>>>>>> On Thu, Jul 21, 2016 at 10:09 AM, Stanislaw Gruszka <sgruszka@redhat.com> wrote:
>>>>>>> On Tue, Jul 19, 2016 at 03:00:37PM +0200, Michal Kazior wrote:
>>>>>>>> Firmware files are versioned to prevent older
>>>>>>>> driver instances to load unsupported firmware
>>>>>>>> blobs. This is reflected with a fallback logic
>>>>>>>> which attempts to load several firmware files.
>>>>>>>>
>>>>>>>> This however produced a lot of unnecessary
>>>>>>>> warnings sometimes confusing users and leading
>>>>>>>> them to rename firmware files making things even
>>>>>>>> more confusing.
>>>>>>>
>>>>>>> This happens on kernels configured with
>>>>>>> CONFIG_FW_LOADER_USER_HELPER_FALLBACK and cause not only ugly warnings,
>>>>>>> but also 60 seconds delay before loading next firmware version.
>>>>>>> For some reason RHEL kernel needs above config option, so this
>>>>>>> patch is very welcome from my perspective.
>>>>>>>
>>>>>>
>>>>>> Sorry for my ignorance but how does the firmware loading work if not
>>>>>> with udev's help?
>>>>>
>>>>> I'm not sure exactly, but I think kernel VFS layer is capable to copy
>>>>> file data directly from mounted filesystem without user space helper.
>>>>
>>>> Here's the situation: request_firmware() waits 60 seconds for udev to do its
>>>> loading magic via a "usermode helper". This delay is there to allow, for
>>>> example, userspace to unpack or download a new firmware image or verify the
>>>> firmware image *in userspace* before providing it to the driver to apply to the HW.
>>>>
>>>> Why 60 seconds? It is arbitrary and there is no way for udev & the kernel to
>>>> handshake on completion.
>>>>
>>>>>
>>>>>> As you can imagine, iwlwifi is suffering from the
>>>>>> same problem and I would be interested in applying the same change,
>>>>>> but I'd love to understand a bit more :)
>>>>>
>>>>> Yes, iwlwifi (and some other drivers) suffer from this. However this
>>>>> happen when the newest firmware version is not installed on the system
>>>>> and CONFIG_FW_LOADER_USER_HELPER_FALLBACK is enabled. What I suppose
>>>>> it's not common.
>>>>
>>>> request_firmware_direct() was introduced at my request because (as you've
>>>> noticed) when CONFIG_FW_LOADER_USER_HELPER_FALLBACK=y drivers may stall for long
>>>> periods of time when starting. The bug that this introduced was a 60 second
>>>> delay per logical cpu when starting a system. On a 64 cpu system that meant the
>>>> boot would complete in a little over one hour.
>>>>
>>>>>
>>>>> I started to see this currently, because that option was enabled on
>>>>> RHEL kernel. BTW: I think Prarit iwlwifi thermal_zone problem was
>>>>> happened because of that, i.e. thermal device was not functional
>>>>> because f/w wasn't loaded due to big delay.
>>>>>
>>>>> I'm not sure if replacing to request_firmware_direct() is a good
>>>>> fix though. For example I can see this problem also on brcmfmac, which
>>>>> use request_firmware_nowait(). I think I would rather prefer special
>>>>> helper for firmware drivers that needs user helper and have
>>>>> request_firmware() be direct as default.
>>>>>
>>>>
>>>> The difference between request_firmware_direct() and request_firmware() is that
>>>> the _direct() version does not wait the 60 seconds for udev interaction. The
>>>> only userspace check performed is to see if the file is there, and if the file
>>>> does exist it is provided to the driver to be applied to the hardware.
>>>>
>>>> So the real question to ask here is whether or not the ath10k, brcmfmac, and
>>>> iwlwifi require udev to do anything beyond checking for the existence and
>>>> loading the firmware image. If they don't, then it is better to use
>>>> request_firmware_direct().
>>>
>>> They don't need that, like 99% of the drivers I think, hence changing the
>>> default seems to be more reasonable. However changing 3 drivers would work
>>> for me as well, and that change do not introduce risk of broking drivers
>>> that require udev fw download.
>>>
>>> iwlwifi and ath10k are trivial, bcrmfmac is a bit more complex as it
>>> use request_firmware_nowait(), so it first need to be converted to
>>> ordinary request_firmware(), but this should be doable and I can do
>>> that.
>>
>> I am going bonkers here. This is the Nth time a discussion pops up on
>> firmware API usage. I stopped counting N :-( So the first issue was that
>> the INIT was taking to long as we were requesting firmware during probe
>> which was executed in the INIT context. So we added a worker and
>> register the driver from there. There was probably a reason for
>> switching to _no_wait() as well, but I do not recall the details. The
>> things is I don't know if I need user-space or not. I just need firmware
>> to get the device up and running. We have changed our driver a couple of
>> times now to accommodate something that in my opinion should have been
>> abstracted behind the firmware API in the first place and now here is
>> another proposal to change the drivers. Come on!
>
> Its a big mess, but a lot of it has to do with the fact that none of the
> issues have been well documented. Its also not clear what distros, driver
> developers or users should do. I've tried helping with by providing such
> documentation and also providing grammar rules to avoid further issues [0],
> hopefully this series will be merged soon.
>
> [0] https://marc.info/?l=linux-kernel&m=146611775314567
>
> Using grammar rules the hunt with coccinelle as of today's linux-next
> reveals there are only 2 explicit users of the usermode helper, and
> I've vetted for these as well in the above patch series.
>
> Now, other than this users will experience the usermode helper if the
> distribution messed up and build their kernel with the fallback
> usermode helper. If this was done on some old kernel the only way
> to fix that is to fix that kernel build or change drivers to avoid
> the usermode helper explicitly, unfortunately some API calls cannot
> avoid it .... I've documented all this in the above series.
>
>>> However I wonder if changing that will not broke the case when
>>> driver is build-in in the kernel and f/w is not yet available when
>>> driver start to initialize.
>
> Indeed, tons of races are in theory possible here ;) technically since we use a
> common API to read files directly now, a race might also be possible for other
> users of the API on init as well. I have some grammar rules to test for this in
> development, that is to vet that not only the firmware API is checked and we
> avoid on init but also other callers that use the same read API.
>
>>> Or maybe nowadays this is not the case
>>> any longer, i.e. the MODULE_FIRMWARE macros assure proper f/w
>>> images are build-in in the kernel or copied to initramfs?
>
> The firmware API is a mess and I've been trying to correct that
> with a more flexible API.
>
>> That is a nice idea, but I have not seen any change in that area. Could
>> have missed it.
>
> Extensions to the fw API are IMHO best done through a newer flexible
> API, feel free to refer to this development tree if you'd like to
> contribute:
>
> https://git.kernel.org/cgit/linux/kernel/git/mcgrof/linux-next.git/log/?h=20160616-sysdata-v2
So I had a look and noticed commit c8df68e83392 ("firmware: annotate
thou shalt not request fw on init or probe"). Now this conflicts with
our wireless driver. The original suggestion a long, long time ago was
to use IFF_UP as trigger to go and request firmware. However, for that
we would need to register a netdevice during probe, and consequently we
should also have a wiphy instance registered. However, that has all kind
of feature flags for which we need firmware running on the device to
query what is supported and what not. I can make a fair bet that
brcmfmac is not the only driver with such a requirement. So how can we
crack that nut.
Regards,
Arend
^ permalink raw reply
* Re: [RFC v0 7/8] Input: ims-pcu: use firmware_stat instead of completion
From: Bjorn Andersson @ 2016-07-28 19:01 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Daniel Wagner, Bastien Nocera, Greg Kroah-Hartman, Johannes Berg,
Kalle Valo, Ohad Ben-Cohen, linux-input, linux-kselftest,
linux-wireless, linux-kernel, Daniel Wagner
In-Reply-To: <20160728183343.GD16852@dtor-ws>
On Thu 28 Jul 11:33 PDT 2016, Dmitry Torokhov wrote:
> On Thu, Jul 28, 2016 at 09:55:11AM +0200, Daniel Wagner wrote:
> > From: Daniel Wagner <daniel.wagner@bmw-carit.de>
> >
[..]
>
> Do not quite like it... I'd rather asynchronous request give out a
> firmware status pointer that could be used later on.
>
> pcu->fw_st = request_firmware_async(IMS_PCU_FIRMWARE_NAME,
> pcu,
> ims_pcu_process_async_firmware);
> if (IS_ERR(pcu->fw_st))
> return PTR_ERR(pcu->fw_st);
>
> ....
>
> fw_loading_wait(pcu->fw_st);
>
In the remoteproc case (patch 6) this would clean up the code, rather
than replacing the completion API 1 to 1. I like it!
Regards,
Bjorn
^ permalink raw reply
* Re: brcm4330 fails to load on newer kernels
From: Arend van Spriel @ 2016-07-28 18:37 UTC (permalink / raw)
To: Fabio Estevam
Cc: Arend van Spriel, Hante Meuleman, linux-wireless,
brcm80211-dev-list
In-Reply-To: <CAOMZO5CvuumYQZ92YFCjd7Vrmp4i1T1pdYAmGUvRgjNzVpMWyQ@mail.gmail.com>
On 27-07-16 22:57, Fabio Estevam wrote:
> Hi Arend,
>
> On Wed, Jul 27, 2016 at 5:51 PM, Arend van Spriel
> <arend.vanspriel@broadcom.com> wrote:
>> On 27-07-16 00:35, Fabio Estevam wrote:
>>> Hi,
>>>
>>> On a imx6sl-warp board with a brcm4330 I get the following results
>>> depending on the kernel version:
>>>
>>> - Kernel 4.4.15: place brcmfmac4330-sdio.bin and brcmfmac4330-sdio.txt
>>> in the rootfs and the kernel is able to read them correctly. wlan0 is
>>> present. All is fine.
>>>
>>> - Kernel 4.5.7: place brcmfmac4330-sdio.bin brcmfmac4330-sdio.txt in
>>> the rootfs and the kernel fails to load them:
>>>
>>> brcmfmac mmc1:0001:1: Direct firmware load for
>>> brcm/brcmfmac4330-sdio.bin failed with error -2
>>>
>>> Then I build brcmfmac4330-sdio.bin brcmfmac4330-sdio.txt into the
>>> kernel and then firmware is detected and wlan0 appears.
>>>
>>> - Kernel 4.7: I can place the firmware and nvram file into the rootfs
>>> or built-i and the following error is seen:
>>>
>>> brcmfmac: brcmf_sdio_htclk: HT Avail timeout (1000000): clkctl 0x50
>>> brcmfmac: brcmf_sdio_htclk: HT Avail timeout (1000000): clkctl 0x50
>>> brcmfmac: brcmf_sdio_htclk: HT Avail timeout (1000000): clkctl 0x50
>>>
>>> So wlan0 never appears here.
>>>
>>> Does anyone have any suggestions about these different behaviours?
>>
>> So for all kernel you have brcmfmac built-in the kernel or as a module?
>
> In all these tests I have brcmfmac built-in the kernel.
Hi Fabio,
So this is another fine example of firmware API not able to deliver. I
think in all these kernels you have the same issue. The problem is that
the order of events upon kernel boot is not predictable. In this case
you have rootfs being mounted and brcmfmac getting probed as the two
competing events. When rootfs is mounted before brcmfmac is being probed
it works, but if brcmfmac is probed before rootfs is mounted the
firmware request will fail. So the only reliable option for built-in
drivers requiring firmware is to built-in the firmware into the kernel
as well.
Regards,
Arend
^ permalink raw reply
* Re: [RFC v0 7/8] Input: ims-pcu: use firmware_stat instead of completion
From: Dmitry Torokhov @ 2016-07-28 18:33 UTC (permalink / raw)
To: Daniel Wagner
Cc: Bastien Nocera, Bjorn Andersson, Greg Kroah-Hartman,
Johannes Berg, Kalle Valo, Ohad Ben-Cohen, linux-input,
linux-kselftest, linux-wireless, linux-kernel, Daniel Wagner
In-Reply-To: <1469692512-16863-8-git-send-email-wagi@monom.org>
On Thu, Jul 28, 2016 at 09:55:11AM +0200, Daniel Wagner wrote:
> From: Daniel Wagner <daniel.wagner@bmw-carit.de>
>
> Loading firmware is an operation many drivers implement in various ways
> around the completion API. And most of them do it almost in the same
> way. Let's reuse the firmware_stat API which is used also by the
> firmware_class loader. Apart of streamlining the firmware loading states
> we also document it slightly better.
>
> Signed-off-by: Daniel Wagner <daniel.wagner@bmw-carit.de>
> ---
> drivers/input/misc/ims-pcu.c | 10 +++++-----
> 1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/input/misc/ims-pcu.c b/drivers/input/misc/ims-pcu.c
> index 9c0ea36..cda1fbf 100644
> --- a/drivers/input/misc/ims-pcu.c
> +++ b/drivers/input/misc/ims-pcu.c
> @@ -109,7 +109,7 @@ struct ims_pcu {
>
> u32 fw_start_addr;
> u32 fw_end_addr;
> - struct completion async_firmware_done;
> + struct firmware_stat fw_st;
>
> struct ims_pcu_buttons buttons;
> struct ims_pcu_gamepad *gamepad;
> @@ -940,7 +940,7 @@ static void ims_pcu_process_async_firmware(const struct firmware *fw,
> release_firmware(fw);
>
> out:
> - complete(&pcu->async_firmware_done);
> + fw_loading_done(pcu->fw_st);
Why does the driver have to do it? If firmware loader manages this, then
it should let waiters know when callback finishes.
> }
>
> /*********************************************************************
> @@ -1967,7 +1967,7 @@ static int ims_pcu_init_bootloader_mode(struct ims_pcu *pcu)
> ims_pcu_process_async_firmware);
> if (error) {
> /* This error is not fatal, let userspace have another chance */
> - complete(&pcu->async_firmware_done);
> + fw_loading_abort(pcu->fw_st);
Why should the driver signal abort if it does not manage completion in
this case?
> }
>
> return 0;
> @@ -1976,7 +1976,7 @@ static int ims_pcu_init_bootloader_mode(struct ims_pcu *pcu)
> static void ims_pcu_destroy_bootloader_mode(struct ims_pcu *pcu)
> {
> /* Make sure our initial firmware request has completed */
> - wait_for_completion(&pcu->async_firmware_done);
> + fw_loading_wait(pcu->fw_st);
> }
>
> #define IMS_PCU_APPLICATION_MODE 0
> @@ -2000,7 +2000,7 @@ static int ims_pcu_probe(struct usb_interface *intf,
> pcu->bootloader_mode = id->driver_info == IMS_PCU_BOOTLOADER_MODE;
> mutex_init(&pcu->cmd_mutex);
> init_completion(&pcu->cmd_done);
> - init_completion(&pcu->async_firmware_done);
> + firmware_stat_init(&pcu->fw_st);
Do not quite like it... I'd rather asynchronous request give out a
firmware status pointer that could be used later on.
pcu->fw_st = request_firmware_async(IMS_PCU_FIRMWARE_NAME,
pcu,
ims_pcu_process_async_firmware);
if (IS_ERR(pcu->fw_st))
return PTR_ERR(pcu->fw_st);
....
fw_loading_wait(pcu->fw_st);
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [RFC v0 3/8] firmware: Factor out firmware load helpers
From: Dmitry Torokhov @ 2016-07-28 17:57 UTC (permalink / raw)
To: Daniel Wagner
Cc: Bastien Nocera, Bjorn Andersson, Greg Kroah-Hartman,
Johannes Berg, Kalle Valo, Ohad Ben-Cohen, linux-input,
linux-kselftest, linux-wireless, linux-kernel, Daniel Wagner
In-Reply-To: <1469692512-16863-4-git-send-email-wagi@monom.org>
On Thu, Jul 28, 2016 at 09:55:07AM +0200, Daniel Wagner wrote:
> +int __firmware_stat_wait(struct firmware_stat *fwst,
> + long timeout)
> +{
> + int err;
> + err = swait_event_interruptible_timeout(fwst->wq,
> + is_fw_sync_done(READ_ONCE(fwst->status)),
> + timeout);
> + if (err == 0 && fwst->status == FW_STATUS_ABORT)
> + return -ENOENT;
> +
> + return err;
> +}
> +EXPORT_SYMBOL(__firmware_stat_wait);
> +
> +void __firmware_stat_set(struct firmware_stat *fwst, unsigned long status)
> +{
> + WRITE_ONCE(fwst->status, status);
> + swake_up(&fwst->wq);
Do we need to notify everyone for FW_STATUS_LOADING status?
The driver users do not care for sure.
Thanks.
--
Dmitry
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox