Re: brcm4330 fails to load on newer kernels

[Fabio Estevam]

Hi Arend,

On Thu, Jul 28, 2016 at 3:37 PM, Arend van Spriel
 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
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC] ath10k: silence firmware file probing warnings

[Luis R. Rodriguez]

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 

Re: [RFC] ath10k: silence firmware file probing warnings

[Arend van Spriel]

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 
>>>  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 

Re: [RFC] ath10k: silence firmware file probing warnings

[Arend van Spriel]

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 
>>  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 

Re: [RFC v0 7/8] Input: ims-pcu: use firmware_stat instead of completion

[Bjorn Andersson]

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 
> > 
[..]
> 
> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: brcm4330 fails to load on newer kernels

[Arend van Spriel]

On 27-07-16 22:57, Fabio Estevam wrote:
> Hi Arend,
> 
> On Wed, Jul 27, 2016 at 5:51 PM, Arend van Spriel
>  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 (100): clkctl 0x50
>>> brcmfmac: brcmf_sdio_htclk: HT Avail timeout (100): clkctl 0x50
>>> brcmfmac: brcmf_sdio_htclk: HT Avail timeout (100): 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
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC v0 7/8] Input: ims-pcu: use firmware_stat instead of completion

[Dmitry Torokhov]

On Thu, Jul 28, 2016 at 09:55:11AM +0200, Daniel Wagner wrote:
> From: Daniel Wagner 
> 
> 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 
> ---
>  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(>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(>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(>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(>cmd_mutex);
>   init_completion(>cmd_done);
> - init_completion(>async_firmware_done);
> + firmware_stat_init(>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
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC v0 3/8] firmware: Factor out firmware load helpers

[Dmitry Torokhov]

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(>wq);

Do we need to notify everyone for FW_STATUS_LOADING status?

The driver users do not care for sure.

Thanks.

-- 
Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: staging: wilc1000: Reduce scope for a few variables in mac_ioctl()

[SF Markus Elfring]

> Which circumstances do "not any sense at all" imply? 

Should the expression 'strlen("RSSI")' be passed for the parameter 'length' 
instead?


> I suggest to fix this since it is indeed a bug,

We can agree that this function implementation was broken for a while there.


> instead of doing "micro optimizations" - which is the last thing that code in 
> the staging area 
> needs (as IIRC you have already been told by others, including the staging 
> maintainer).

The acceptance might grow a bit more for such software fine-tuning
(like refactoring around variable usage).

Regards,
Markus
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC v0 3/8] firmware: Factor out firmware load helpers

[Dan Williams]

On Thu, 2016-07-28 at 09:55 +0200, Daniel Wagner wrote:
> From: Daniel Wagner 
> 
> Factor out the firmware loading synchronization code in order to
> allow
> drivers to reuse it. This also documents more clearly what is
> happening. This is especial useful the drivers which will be
> converted
> afterwards. Not everyone has to come with yet another way to handle
> it.

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 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).

Dan

> We use swait instead completion. complete() would only wake up one
> waiter, so complete_all() is used. complete_all() wakes max
> MAX_UINT/2
> waiters which is plenty in all cases. Though withh swait we just wait
> until the exptected status shows up and wake any waiter.
> 
> Signed-off-by: Daniel Wagner 
> ---
>  drivers/base/firmware_class.c | 112 +++-
> --
>  include/linux/firmware.h  |  71 ++
>  2 files changed, 122 insertions(+), 61 deletions(-)
> 
> diff --git a/drivers/base/firmware_class.c
> b/drivers/base/firmware_class.c
> index 773fc30..bf1ca70 100644
> --- a/drivers/base/firmware_class.c
> +++ b/drivers/base/firmware_class.c
> @@ -30,6 +30,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  
>  #include 
>  
> @@ -85,11 +86,36 @@ static inline bool fw_is_builtin_firmware(const
> struct firmware *fw)
>  }
>  #endif
>  
> -enum {
> - FW_STATUS_LOADING,
> - FW_STATUS_DONE,
> - FW_STATUS_ABORT,
> -};
> +
> +#if defined(CONFIG_FW_LOADER) || (defined(CONFIG_FW_LOADER_MODULE)
> && defined(MODULE))
> +
> +static inline bool is_fw_sync_done(unsigned long status)
> +{
> + return status == FW_STATUS_LOADED || status ==
> FW_STATUS_ABORT;
> +}
> +
> +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(>wq);
> +}
> +EXPORT_SYMBOL(__firmware_stat_set);
> +
> +#endif
>  
>  static int loading_timeout = 60; /* In seconds */
>  
> @@ -138,9 +164,8 @@ struct firmware_cache {
>  struct firmware_buf {
>   struct kref ref;
>   struct list_head list;
> - struct completion completion;
> + struct firmware_stat fwst;
>   struct firmware_cache *fwc;
> - unsigned long status;
>   void *data;
>   size_t size;
>  #ifdef CONFIG_FW_LOADER_USER_HELPER
> @@ -194,7 +219,7 @@ static struct firmware_buf
> *__allocate_fw_buf(const char *fw_name,
>  
>   kref_init(>ref);
>   buf->fwc = fwc;
> - init_completion(>completion);
> + firmware_stat_init(>fwst);
>  #ifdef CONFIG_FW_LOADER_USER_HELPER
>   INIT_LIST_HEAD(>pending_list);
>  #endif
> @@ -292,15 +317,6 @@ static const char * const fw_path[] = {
>  module_param_string(path, fw_path_para, sizeof(fw_path_para), 0644);
>  MODULE_PARM_DESC(path, "customized firmware image search path with a
> higher priority than default path");
>  
> -static void fw_finish_direct_load(struct device *device,
> -   struct firmware_buf *buf)
> -{
> - mutex_lock(_lock);
> - set_bit(FW_STATUS_DONE, >status);
> - complete_all(>completion);
> - mutex_unlock(_lock);
> -}
> -
>  static int fw_get_filesystem_firmware(struct device *device,
>      struct firmware_buf *buf)
>  {
> @@ -339,7 +355,7 @@ static int fw_get_filesystem_firmware(struct
> device *device,
>   }
>   dev_dbg(device, "direct-loading %s\n", buf->fw_id);
>   buf->size = size;
> - fw_finish_direct_load(device, buf);
> + fw_loading_done(buf->fwst);
>   break;
>   }
>   __putname(path);
> @@ -457,12 +473,11 @@ static void __fw_load_abort(struct firmware_buf
> *buf)
>    * There is a small window in which user can write to
> 'loading'
>    * between loading done and disappearance of 'loading'
>    */
> - if (test_bit(FW_STATUS_DONE, >status))
> + if (is_fw_loading_done(buf->fwst))
>   return;
>  
>   list_del_init(>pending_list);
> - set_bit(FW_STATUS_ABORT, >status);
> - 

Re: [RFC v0 4/8] Input: goodix: use firmware_stat instead of completion

[Daniel Wagner]

Looking at the API, I really don't like the mixing of namespaces.
Either it's fw_ or it's firmware_ but not a mix of both.


I agree, that was not really clever.

struct fw_loading { ... }
__fw_loading_*()
fw_loading_*()

Would that make more sense? firmware_loading_ as prefix is a bit long IMO.


Also looks like fw_loading_start() would do nothing as the struct is
likely zero initialised, and FW_STATUS_LOADING == 0. Maybe you need an
UNSET enum member?


Good point, I cut a corner here a bit :). In the spirit of making things 
more clear fw_loading_start() should be used.



FW_STATUS_ABORT <- FW_STATUS_ABORTED, to match the adjective suffixes
of the other members. Ditto fw_loading_abort() which doesn't abort
firmware loading but sets the status.


Okay.

Thanks a lot for the feedback.

cheers,
daniel
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC v0 4/8] Input: goodix: use firmware_stat instead of completion

[Bastien Nocera]

On Thu, 2016-07-28 at 13:59 +0200, Daniel Wagner wrote:
> On 07/28/2016 01:22 PM, Bastien Nocera wrote:
> > On Thu, 2016-07-28 at 09:55 +0200, Daniel Wagner wrote:
> > > From: Daniel Wagner 
> > > 
> > > 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 
> > 
> > Irina added and tested that feature, so best for her to comment on
> > this, as I don't have any hardware that would use this feature.
> 
> In case you have any comments on the API, let me know. I'll add Irina
> to 
> the Cc list in the next version.

Looking at the API, I really don't like the mixing of namespaces.
Either it's fw_ or it's firmware_ but not a mix of both.

Also looks like fw_loading_start() would do nothing as the struct is
likely zero initialised, and FW_STATUS_LOADING == 0. Maybe you need an
UNSET enum member?

FW_STATUS_ABORT <- FW_STATUS_ABORTED, to match the adjective suffixes
of the other members. Ditto fw_loading_abort() which doesn't abort
firmware loading but sets the status.

Cheers

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC v0 4/8] Input: goodix: use firmware_stat instead of completion

[Daniel Wagner]

On 07/28/2016 01:22 PM, Bastien Nocera wrote:

On Thu, 2016-07-28 at 09:55 +0200, Daniel Wagner wrote:

From: Daniel Wagner 

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 


Irina added and tested that feature, so best for her to comment on
this, as I don't have any hardware that would use this feature.


In case you have any comments on the API, let me know. I'll add Irina to 
the Cc list in the next version.


cheers,
daniel

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/3] staging: wilc1000: One function call less in mac_ioctl() after error detection

[Julian Calaby]

Hi Marcus,

On Mon, Jul 25, 2016 at 6:22 AM, SF Markus Elfring
 wrote:
> From: Markus Elfring 
> Date: Sun, 24 Jul 2016 21:15:23 +0200
>
> The kfree() function was called in two cases by the mac_ioctl() function
> during error handling even if the passed variable did not contain a pointer
> for a valid data item.
>
> Improve this implementation detail by the introduction of another
> jump label.
>
> Signed-off-by: Markus Elfring 

This is pointless micro-optimisation: kfree(NULL) is perfectly valid
and buff is either malloc'd memory or NULL when it's called.

Thanks,

-- 
Julian Calaby

Email: julian.cal...@gmail.com
Profile: http://www.google.com/profiles/julian.calaby/
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Aw: Re: staging: wilc1000: Reduce scope for a few variables in mac_ioctl()

[Lino Sanfilippo]

> Gesendet: Dienstag, 26. Juli 2016 um 08:25 Uhr
> Von: "SF Markus Elfring" 

>
> >> -  if (strncasecmp(buff, "RSSI", length) == 0) {
> >> +  if (strncasecmp(buff, "RSSI", 0) == 0) {
> >> +  s8 rssi;
> >> +
> > 
> > Um, please think a second about if it makes any sense at all to compare 
> > zero chars of two strings.
> 
> Under which circumstances should the variable "length" contain an other
> value than zero?

Which circumstances do "not any sense at all" imply? 

> 
> How can this open issue be fixed better?

The code is not too complicated and I think it is very obvious which 
value/variable
should be passed instead of 0. I suggest to fix this since it is indeed a bug, 
instead of 
doing "micro optimizations" - which is the last thing that code in the staging 
area 
needs (as IIRC you have already been told by others, including the staging 
maintainer).

Regards,
Lino


--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: ath10k: Remove driver log suggesting QCA9887 support is experimental

[Kalle Valo]

Mohammed Shafi Shajakhan  wrote:
> From: Mohammed Shafi Shajakhan 
> 
> Support for QCA9887 is no longer experimental and if there are any issues
> we need to address them
> 
> Signed-off-by: Mohammed Shafi Shajakhan 

I'm planning to push this to 4.8 if no objections.

-- 
Sent by pwcli
https://patchwork.kernel.org/patch/9249941/

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: ath10k: fix get rx_status from htt context

[Kalle Valo]

Ashok Raj Nagarajan  wrote:
> On handling amsdu on rx path, get the rx_status from htt context. Without this
> fix, we are seeing warnings when running DBDC traffic like this.
> 
> WARNING: CPU: 0 PID: 0 at net/mac80211/rx.c:4105 ieee80211_rx_napi+0x88/0x7d8 
> [mac80211]()
> 
> [ 1715.878248] CPU: 0 PID: 0 Comm: swapper/0 Tainted: G W 3.18.21 #1
> [ 1715.878273] [] (unwind_backtrace) from [] 
> (show_stack+0x10/0x14)
> [ 1715.878293] [] (show_stack) from [] 
> (dump_stack+0x70/0xbc)
> [ 1715.878315] [] (dump_stack) from [] 
> (warn_slowpath_common+0x64/0x88)
> [ 1715.878339] [] (warn_slowpath_common) from [] 
> (warn_slowpath_null+0x18/0x20)
> [ 1715.878395] [] (warn_slowpath_null) from [] 
> (ieee80211_rx_napi+0x88/0x7d8 [mac80211])
> [ 1715.878474] [] (ieee80211_rx_napi [mac80211]) from [] 
> (ath10k_htt_t2h_msg_handler+0xb48/0xbfc [ath10k_core])
> [ 1715.878535] [] (ath10k_htt_t2h_msg_handler [ath10k_core]) from 
> [] (ath10k_htt_t2h_msg_handler+0xbf8/0xbfc [ath10k_core])
> [ 1715.878597] [] (ath10k_htt_t2h_msg_handler [ath10k_core]) from 
> [] (ath10k_htt_txrx_compl_task+0xa54/0x1170 [ath10k_core])
> [ 1715.878639] [] (ath10k_htt_txrx_compl_task [ath10k_core]) from 
> [] (tasklet_action+0xb4/0x130)
> [ 1715.878659] [] (tasklet_action) from [] 
> (__do_softirq+0xe0/0x210)
> [ 1715.878678] [] (__do_softirq) from [] 
> (irq_exit+0x84/0xe0)
> [ 1715.878700] [] (irq_exit) from [] 
> (__handle_domain_irq+0x98/0xd0)
> [ 1715.878722] [] (__handle_domain_irq) from [] 
> (gic_handle_irq+0x38/0x5c)
> [ 1715.878741] [] (gic_handle_irq) from [] 
> (__irq_svc+0x40/0x74)
> [ 1715.878753] Exception stack(0xc05f9f50 to 0xc05f9f98)
> [ 1715.878767] 9f40: ffed  00399e1e c000a220
> [ 1715.878786] 9f60:  c05f6780 c05f8000  c05f5db8 ffed 
> c05f8000 c04d1980
> [ 1715.878802] 9f80:  c05f9f98 c0018110 c0018114 6013 
> [ 1715.878822] [] (__irq_svc) from [] 
> (arch_cpu_idle+0x2c/0x50)
> [ 1715.878844] [] (arch_cpu_idle) from [] 
> (cpu_startup_entry+0x108/0x234)
> [ 1715.878866] [] (cpu_startup_entry) from [] 
> (start_kernel+0x33c/0x3b8)
> [ 1715.878879] ---[ end trace 6d5e1cc0fef8ed6a ]---
> [ 1715.878899] [ cut here ]
> 
> Fixes: 18235664e7f9 ("ath10k: cleanup amsdu processing for rx indication")
> Signed-off-by: Ashok Raj Nagarajan 

I'm planning to push this to 4.8 if no objections.

-- 
Sent by pwcli
https://patchwork.kernel.org/patch/9248457/

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[RFC v0 2/8] selftests: firmware: do not clutter output

[Daniel Wagner]

From: Daniel Wagner 

Some of the test are supposed to fail but we get a few ouptput lines:

./fw_filesystem.sh: line 51: printf: write error: Invalid argument
./fw_filesystem.sh: line 56: printf: write error: No such device
./fw_filesystem.sh: line 62: echo: write error: No such file or directory

Let's silence them so that the selftest output is clean if all is fine.

Signed-off-by: Daniel Wagner 
---
 tools/testing/selftests/firmware/fw_filesystem.sh | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/tools/testing/selftests/firmware/fw_filesystem.sh 
b/tools/testing/selftests/firmware/fw_filesystem.sh
index 5c495ad..d8ac9ba 100755
--- a/tools/testing/selftests/firmware/fw_filesystem.sh
+++ b/tools/testing/selftests/firmware/fw_filesystem.sh
@@ -48,18 +48,18 @@ echo "ABCD0123" >"$FW"
 
 NAME=$(basename "$FW")
 
-if printf '\000' >"$DIR"/trigger_request; then
+if printf '\000' >"$DIR"/trigger_request 2> /dev/null; then
echo "$0: empty filename should not succeed" >&2
exit 1
 fi
 
-if printf '\000' >"$DIR"/trigger_async_request; then
+if printf '\000' >"$DIR"/trigger_async_request 2> /dev/null; then
echo "$0: empty filename should not succeed (async)" >&2
exit 1
 fi
 
 # Request a firmware that doesn't exist, it should fail.
-if echo -n "nope-$NAME" >"$DIR"/trigger_request; then
+if echo -n "nope-$NAME" >"$DIR"/trigger_request 2> /dev/null; then
echo "$0: firmware shouldn't have loaded" >&2
exit 1
 fi
-- 
2.7.4
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[RFC v0 5/8] ath9k_htc: use firmware_stat instead of completion

[Daniel Wagner]

From: Daniel Wagner 

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 
---
 drivers/net/wireless/ath/ath9k/hif_usb.c | 10 +-
 drivers/net/wireless/ath/ath9k/hif_usb.h |  2 +-
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/net/wireless/ath/ath9k/hif_usb.c 
b/drivers/net/wireless/ath/ath9k/hif_usb.c
index e1c338c..0a05d68 100644
--- a/drivers/net/wireless/ath/ath9k/hif_usb.c
+++ b/drivers/net/wireless/ath/ath9k/hif_usb.c
@@ -1067,7 +1067,7 @@ static void ath9k_hif_usb_firmware_fail(struct 
hif_device_usb *hif_dev)
struct device *dev = _dev->udev->dev;
struct device *parent = dev->parent;
 
-   complete_all(_dev->fw_done);
+   fw_loading_abort(hif_dev->fw_st);
 
if (parent)
device_lock(parent);
@@ -1192,7 +1192,7 @@ static void ath9k_hif_usb_firmware_cb(const struct 
firmware *fw, void *context)
 
release_firmware(fw);
hif_dev->flags |= HIF_USB_READY;
-   complete_all(_dev->fw_done);
+   fw_loading_done(hif_dev->fw_st);
 
return;
 
@@ -1287,7 +1287,7 @@ static int ath9k_hif_usb_probe(struct usb_interface 
*interface,
 #endif
usb_set_intfdata(interface, hif_dev);
 
-   init_completion(_dev->fw_done);
+   firmware_stat_init(_dev->fw_st);
 
ret = ath9k_hif_request_firmware(hif_dev, true);
if (ret)
@@ -1330,7 +1330,7 @@ static void ath9k_hif_usb_disconnect(struct usb_interface 
*interface)
if (!hif_dev)
return;
 
-   wait_for_completion(_dev->fw_done);
+   fw_loading_wait(hif_dev->fw_st);
 
if (hif_dev->flags & HIF_USB_READY) {
ath9k_htc_hw_deinit(hif_dev->htc_handle, unplugged);
@@ -1363,7 +1363,7 @@ static int ath9k_hif_usb_suspend(struct usb_interface 
*interface,
if (!(hif_dev->flags & HIF_USB_START))
ath9k_htc_suspend(hif_dev->htc_handle);
 
-   wait_for_completion(_dev->fw_done);
+   fw_loading_wait(hif_dev->fw_st);
 
if (hif_dev->flags & HIF_USB_READY)
ath9k_hif_usb_dealloc_urbs(hif_dev);
diff --git a/drivers/net/wireless/ath/ath9k/hif_usb.h 
b/drivers/net/wireless/ath/ath9k/hif_usb.h
index 7c2ef7e..0af9fe4 100644
--- a/drivers/net/wireless/ath/ath9k/hif_usb.h
+++ b/drivers/net/wireless/ath/ath9k/hif_usb.h
@@ -111,7 +111,7 @@ struct hif_device_usb {
const struct usb_device_id *usb_device_id;
const void *fw_data;
size_t fw_size;
-   struct completion fw_done;
+   struct firmware_stat fw_st;
struct htc_target *htc_handle;
struct hif_usb_tx tx;
struct usb_anchor regout_submitted;
-- 
2.7.4
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[RFC v0 8/8] iwl4965: use firmware_stat instead of completion

[Daniel Wagner]

From: Daniel Wagner 

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 
---
 drivers/net/wireless/intel/iwlegacy/4965-mac.c | 8 
 drivers/net/wireless/intel/iwlegacy/common.h   | 3 ++-
 2 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/net/wireless/intel/iwlegacy/4965-mac.c 
b/drivers/net/wireless/intel/iwlegacy/4965-mac.c
index a91d170..d5e5808 100644
--- a/drivers/net/wireless/intel/iwlegacy/4965-mac.c
+++ b/drivers/net/wireless/intel/iwlegacy/4965-mac.c
@@ -5005,7 +5005,7 @@ il4965_ucode_callback(const struct firmware *ucode_raw, 
void *context)
 
/* We have our copies now, allow OS release its copies */
release_firmware(ucode_raw);
-   complete(>_4965.firmware_loading_complete);
+   fw_loading_done(il->_4965.fw_st);
return;
 
 try_again:
@@ -5019,7 +5019,7 @@ err_pci_alloc:
IL_ERR("failed to allocate pci memory\n");
il4965_dealloc_ucode_pci(il);
 out_unbind:
-   complete(>_4965.firmware_loading_complete);
+   fw_loading_done(il->_4965.fw_st);
device_release_driver(>pci_dev->dev);
release_firmware(ucode_raw);
 }
@@ -6678,7 +6678,7 @@ il4965_pci_probe(struct pci_dev *pdev, const struct 
pci_device_id *ent)
 
il_power_initialize(il);
 
-   init_completion(>_4965.firmware_loading_complete);
+   firmware_stat_init(>_4965.fw_st);
 
err = il4965_request_firmware(il, true);
if (err)
@@ -6716,7 +6716,7 @@ il4965_pci_remove(struct pci_dev *pdev)
if (!il)
return;
 
-   wait_for_completion(>_4965.firmware_loading_complete);
+   fw_loading_wait(il->_4965.fw_st);
 
D_INFO("*** UNLOAD DRIVER ***\n");
 
diff --git a/drivers/net/wireless/intel/iwlegacy/common.h 
b/drivers/net/wireless/intel/iwlegacy/common.h
index 726ede3..94af7b7 100644
--- a/drivers/net/wireless/intel/iwlegacy/common.h
+++ b/drivers/net/wireless/intel/iwlegacy/common.h
@@ -32,6 +32,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 
@@ -1357,7 +1358,7 @@ struct il_priv {
bool last_phy_res_valid;
u32 ampdu_ref;
 
-   struct completion firmware_loading_complete;
+   struct firmware_stat fw_st;
 
/*
 * chain noise reset and gain commands are the
-- 
2.7.4
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[RFC v0 6/8] remoteproc: use firmware_stat instead of completion

[Daniel Wagner]

From: Daniel Wagner 

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 
---
 drivers/remoteproc/remoteproc_core.c | 10 +-
 drivers/soc/ti/wkup_m3_ipc.c |  2 +-
 include/linux/remoteproc.h   |  6 --
 3 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/drivers/remoteproc/remoteproc_core.c 
b/drivers/remoteproc/remoteproc_core.c
index db3958b..3b158f7 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -936,7 +936,7 @@ static void rproc_fw_config_virtio(const struct firmware 
*fw, void *context)
 out:
release_firmware(fw);
/* allow rproc_del() contexts, if any, to proceed */
-   complete_all(>firmware_loading_complete);
+   fw_loading_done(rproc->fw_st);
 }
 
 static int rproc_add_virtio_devices(struct rproc *rproc)
@@ -944,7 +944,7 @@ static int rproc_add_virtio_devices(struct rproc *rproc)
int ret;
 
/* rproc_del() calls must wait until async loader completes */
-   init_completion(>firmware_loading_complete);
+   firmware_stat_init(>fw_st);
 
/*
 * We must retrieve early virtio configuration info from
@@ -959,7 +959,7 @@ static int rproc_add_virtio_devices(struct rproc *rproc)
  rproc, rproc_fw_config_virtio);
if (ret < 0) {
dev_err(>dev, "request_firmware_nowait err: %d\n", ret);
-   complete_all(>firmware_loading_complete);
+   fw_loading_abort(rproc->fw_st);
}
 
return ret;
@@ -1089,7 +1089,7 @@ static int __rproc_boot(struct rproc *rproc, bool wait)
 
/* if rproc virtio is not yet configured, wait */
if (wait)
-   wait_for_completion(>firmware_loading_complete);
+   fw_loading_wait(rproc->fw_st);
 
ret = rproc_fw_boot(rproc, firmware_p);
 
@@ -1447,7 +1447,7 @@ int rproc_del(struct rproc *rproc)
return -EINVAL;
 
/* if rproc is just being registered, wait */
-   wait_for_completion(>firmware_loading_complete);
+   fw_loading_wait(rproc->fw_st);
 
/* clean up remote vdev entries */
list_for_each_entry_safe(rvdev, tmp, >rvdevs, node)
diff --git a/drivers/soc/ti/wkup_m3_ipc.c b/drivers/soc/ti/wkup_m3_ipc.c
index 8823cc8..14c6396 100644
--- a/drivers/soc/ti/wkup_m3_ipc.c
+++ b/drivers/soc/ti/wkup_m3_ipc.c
@@ -370,7 +370,7 @@ static void wkup_m3_rproc_boot_thread(struct wkup_m3_ipc 
*m3_ipc)
struct device *dev = m3_ipc->dev;
int ret;
 
-   wait_for_completion(_ipc->rproc->firmware_loading_complete);
+   fw_loading_wait(m3_ipc->rproc->fw_st);
 
init_completion(_ipc->sync_complete);
 
diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
index 1c457a8..b8e7ff4 100644
--- a/include/linux/remoteproc.h
+++ b/include/linux/remoteproc.h
@@ -41,6 +41,7 @@
 #include 
 #include 
 #include 
+#include 
 
 /**
  * struct resource_table - firmware resource table header
@@ -397,7 +398,7 @@ enum rproc_crash_type {
  * @num_traces: number of trace buffers
  * @carveouts: list of physically contiguous memory allocations
  * @mappings: list of iommu mappings we initiated, needed on shutdown
- * @firmware_loading_complete: marks e/o asynchronous firmware loading
+ * @fw_sync: marks e/o asynchronous firmware loading
  * @bootaddr: address of first instruction to boot rproc with (optional)
  * @rvdevs: list of remote virtio devices
  * @notifyids: idr for dynamically assigning rproc-wide unique notify ids
@@ -429,7 +430,7 @@ struct rproc {
int num_traces;
struct list_head carveouts;
struct list_head mappings;
-   struct completion firmware_loading_complete;
+   struct firmware_stat fw_st;
u32 bootaddr;
struct list_head rvdevs;
struct idr notifyids;
@@ -479,6 +480,7 @@ struct rproc_vring {
  * @vring: the vrings for this vdev
  * @rsc_offset: offset of the vdev's resource entry
  */
+
 struct rproc_vdev {
struct list_head node;
struct rproc *rproc;
-- 
2.7.4
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[RFC v0 3/8] firmware: Factor out firmware load helpers

[Daniel Wagner]

From: Daniel Wagner 

Factor out the firmware loading synchronization code in order to allow
drivers to reuse it. This also documents more clearly what is
happening. This is especial useful the drivers which will be converted
afterwards. Not everyone has to come with yet another way to handle it.

We use swait instead completion. complete() would only wake up one
waiter, so complete_all() is used. complete_all() wakes max MAX_UINT/2
waiters which is plenty in all cases. Though withh swait we just wait
until the exptected status shows up and wake any waiter.

Signed-off-by: Daniel Wagner 
---
 drivers/base/firmware_class.c | 112 +++---
 include/linux/firmware.h  |  71 ++
 2 files changed, 122 insertions(+), 61 deletions(-)

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index 773fc30..bf1ca70 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -30,6 +30,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #include 
 
@@ -85,11 +86,36 @@ static inline bool fw_is_builtin_firmware(const struct 
firmware *fw)
 }
 #endif
 
-enum {
-   FW_STATUS_LOADING,
-   FW_STATUS_DONE,
-   FW_STATUS_ABORT,
-};
+
+#if defined(CONFIG_FW_LOADER) || (defined(CONFIG_FW_LOADER_MODULE) && 
defined(MODULE))
+
+static inline bool is_fw_sync_done(unsigned long status)
+{
+   return status == FW_STATUS_LOADED || status == FW_STATUS_ABORT;
+}
+
+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(>wq);
+}
+EXPORT_SYMBOL(__firmware_stat_set);
+
+#endif
 
 static int loading_timeout = 60;   /* In seconds */
 
@@ -138,9 +164,8 @@ struct firmware_cache {
 struct firmware_buf {
struct kref ref;
struct list_head list;
-   struct completion completion;
+   struct firmware_stat fwst;
struct firmware_cache *fwc;
-   unsigned long status;
void *data;
size_t size;
 #ifdef CONFIG_FW_LOADER_USER_HELPER
@@ -194,7 +219,7 @@ static struct firmware_buf *__allocate_fw_buf(const char 
*fw_name,
 
kref_init(>ref);
buf->fwc = fwc;
-   init_completion(>completion);
+   firmware_stat_init(>fwst);
 #ifdef CONFIG_FW_LOADER_USER_HELPER
INIT_LIST_HEAD(>pending_list);
 #endif
@@ -292,15 +317,6 @@ static const char * const fw_path[] = {
 module_param_string(path, fw_path_para, sizeof(fw_path_para), 0644);
 MODULE_PARM_DESC(path, "customized firmware image search path with a higher 
priority than default path");
 
-static void fw_finish_direct_load(struct device *device,
- struct firmware_buf *buf)
-{
-   mutex_lock(_lock);
-   set_bit(FW_STATUS_DONE, >status);
-   complete_all(>completion);
-   mutex_unlock(_lock);
-}
-
 static int fw_get_filesystem_firmware(struct device *device,
   struct firmware_buf *buf)
 {
@@ -339,7 +355,7 @@ static int fw_get_filesystem_firmware(struct device *device,
}
dev_dbg(device, "direct-loading %s\n", buf->fw_id);
buf->size = size;
-   fw_finish_direct_load(device, buf);
+   fw_loading_done(buf->fwst);
break;
}
__putname(path);
@@ -457,12 +473,11 @@ static void __fw_load_abort(struct firmware_buf *buf)
 * There is a small window in which user can write to 'loading'
 * between loading done and disappearance of 'loading'
 */
-   if (test_bit(FW_STATUS_DONE, >status))
+   if (is_fw_loading_done(buf->fwst))
return;
 
list_del_init(>pending_list);
-   set_bit(FW_STATUS_ABORT, >status);
-   complete_all(>completion);
+   fw_loading_abort(buf->fwst);
 }
 
 static void fw_load_abort(struct firmware_priv *fw_priv)
@@ -475,9 +490,6 @@ static void fw_load_abort(struct firmware_priv *fw_priv)
fw_priv->buf = NULL;
 }
 
-#define is_fw_load_aborted(buf)\
-   test_bit(FW_STATUS_ABORT, &(buf)->status)
-
 static LIST_HEAD(pending_fw_head);
 
 /* reboot notifier for avoid deadlock with usermode_lock */
@@ -577,7 +589,7 @@ static ssize_t firmware_loading_show(struct device *dev,
 
mutex_lock(_lock);
if (fw_priv->buf)
-   loading = test_bit(FW_STATUS_LOADING, _priv->buf->status);
+   loading = is_fw_loading(fw_priv->buf->fwst);

[RFC v0 7/8] Input: ims-pcu: use firmware_stat instead of completion

[Daniel Wagner]

From: Daniel Wagner 

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 
---
 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(>async_firmware_done);
+   fw_loading_done(pcu->fw_st);
 }
 
 /*
@@ -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(>async_firmware_done);
+   fw_loading_abort(pcu->fw_st);
}
 
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(>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(>cmd_mutex);
init_completion(>cmd_done);
-   init_completion(>async_firmware_done);
+   firmware_stat_init(>fw_st);
 
error = ims_pcu_parse_cdc_data(intf, pcu);
if (error)
-- 
2.7.4
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[RFC v0 0/8] Reuse firmware loader helpers

[Daniel Wagner]

From: Daniel Wagner 

Hi,

While reviewing all the complete_all() users, I realized there is
recouring pattern how the completion API is used to synchronize the
stages of the firmware loading. Since firmware_class.c contains a
fairly complete implemetation for synching the loading, it worthwhile
to export it and reuse it in drivers, At the same time one
complete_all() user is gone which is a good thing for -rt (*).

The first 2 patches are bug fixes for the test script.

Patch 3 adds the new API, and the following patches update a few
drivers.

I haven't updated all the drivers because I wanted to see first if
this is going into the right direction. Since naming is a difficult, I
am more than please to pick a better name if you have one.

cheers,
daniel

(*) Under -rt waking all waiters via complete_all() is not good. If
the complete_all() call happens in IRQ context we have an unbound
latency there. Therefore the aim is to reduce the complete_all() users
and get rid of them where possible.

Daniel Wagner (8):
  selftests: firmware: do not abort test too early
  selftests: firmware: do not clutter output
  firmware: Factor out firmware load helpers
  Input: goodix: use firmware_stat instead of completion
  ath9k_htc: use firmware_stat instead of completion
  remoteproc: use firmware_stat instead of completion
  Input: ims-pcu: use firmware_stat instead of completion
  iwl4965: use firmware_stat instead of completion

 drivers/base/firmware_class.c | 112 ++
 drivers/input/misc/ims-pcu.c  |  10 +-
 drivers/input/touchscreen/goodix.c|  10 +-
 drivers/net/wireless/ath/ath9k/hif_usb.c  |  10 +-
 drivers/net/wireless/ath/ath9k/hif_usb.h  |   2 +-
 drivers/net/wireless/intel/iwlegacy/4965-mac.c|   8 +-
 drivers/net/wireless/intel/iwlegacy/common.h  |   3 +-
 drivers/remoteproc/remoteproc_core.c  |  10 +-
 drivers/soc/ti/wkup_m3_ipc.c  |   2 +-
 include/linux/firmware.h  |  71 ++
 include/linux/remoteproc.h|   6 +-
 tools/testing/selftests/firmware/fw_filesystem.sh |   6 +-
 tools/testing/selftests/firmware/fw_userhelper.sh |   2 +-
 13 files changed, 158 insertions(+), 94 deletions(-)

-- 
2.7.4
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[RFC v0 1/8] selftests: firmware: do not abort test too early

[Daniel Wagner]

From: Daniel Wagner 

When running the test script you will get:

kselftest/firmware/fw_userhelper.sh: line 69: echo: write error: Resource 
temporarily unavailable

and stops right there. Because the script runs with the '-e' option
which will stop the script at any error.

We should stop there because we are trying to something wrong and get an
error reported back. Instead, ignore the error message and make sure we
do not stop the script by setting the last error code to true.

Signed-off-by: Daniel Wagner 
---
 tools/testing/selftests/firmware/fw_userhelper.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/testing/selftests/firmware/fw_userhelper.sh 
b/tools/testing/selftests/firmware/fw_userhelper.sh
index b9983f8..2d244a7 100755
--- a/tools/testing/selftests/firmware/fw_userhelper.sh
+++ b/tools/testing/selftests/firmware/fw_userhelper.sh
@@ -66,7 +66,7 @@ NAME=$(basename "$FW")
 
 # Test failure when doing nothing (timeout works).
 echo 1 >/sys/class/firmware/timeout
-echo -n "$NAME" >"$DIR"/trigger_request
+echo -n "$NAME" >"$DIR"/trigger_request 2> /dev/null || true
 if diff -q "$FW" /dev/test_firmware >/dev/null ; then
echo "$0: firmware was not expected to match" >&2
exit 1
-- 
2.7.4
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[RFC v0 4/8] Input: goodix: use firmware_stat instead of completion

[Daniel Wagner]

From: Daniel Wagner 

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 
---
 drivers/input/touchscreen/goodix.c | 10 +-
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/input/touchscreen/goodix.c 
b/drivers/input/touchscreen/goodix.c
index 240b16f..67158d3 100644
--- a/drivers/input/touchscreen/goodix.c
+++ b/drivers/input/touchscreen/goodix.c
@@ -47,7 +47,7 @@ struct goodix_ts_data {
u16 id;
u16 version;
const char *cfg_name;
-   struct completion firmware_loading_complete;
+   struct firmware_stat fwst;
unsigned long irq_flags;
 };
 
@@ -683,7 +683,7 @@ static void goodix_config_cb(const struct firmware *cfg, 
void *ctx)
 
 err_release_cfg:
release_firmware(cfg);
-   complete_all(>firmware_loading_complete);
+   fw_loading_done(ts->fwst);
 }
 
 static int goodix_ts_probe(struct i2c_client *client,
@@ -705,7 +705,7 @@ static int goodix_ts_probe(struct i2c_client *client,
 
ts->client = client;
i2c_set_clientdata(client, ts);
-   init_completion(>firmware_loading_complete);
+   firmware_stat_init(>fwst);
 
error = goodix_get_gpio_config(ts);
if (error)
@@ -766,7 +766,7 @@ static int goodix_ts_remove(struct i2c_client *client)
struct goodix_ts_data *ts = i2c_get_clientdata(client);
 
if (ts->gpiod_int && ts->gpiod_rst)
-   wait_for_completion(>firmware_loading_complete);
+   fw_loading_wait(ts->fwst);
 
return 0;
 }
@@ -781,7 +781,7 @@ static int __maybe_unused goodix_suspend(struct device *dev)
if (!ts->gpiod_int || !ts->gpiod_rst)
return 0;
 
-   wait_for_completion(>firmware_loading_complete);
+   fw_loading_wait(ts->fwst);
 
/* Free IRQ as IRQ pin is used as output in the suspend sequence */
goodix_free_irq(ts);
-- 
2.7.4
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

rfkill bound to a switch

[Kai Hendry]

Hi there,

Since laptops typically have a toggle button for killing and enabled
wifi, how does one supposed to bind rfkill like a toggle?

I.e. knowing which state you are in so you can call {block,unblock}
accordingly?

Parsing event or list output seems non-trivial.


Kind regards,
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html