Re: linux-next: build warning after merge of the leds tree

[Michal Kazior]

On Thu, Feb 14, 2019 at 4:46 AM Stephen Rothwell  wrote:
>
> Hi Jacek,
>
> After merging the leds tree, today's linux-next build (x86_64
> allmodconfig) produced this warning:
>
> drivers/leds/leds-lp55xx-common.c: In function 'lp55xx_firmware_loaded':
> drivers/leds/leds-lp55xx-common.c:217:1: warning: label 'out' defined but not 
> used [-Wunused-label]
>  out:
>  ^~~
>
> Introduced by commit
>
>   905c2157dd19 ("leds: lp55xx: fix null deref on firmware load failure")

I'm sorry guys, that's my fault. Let me fix that.


Michał

Re: [PATCH 2/6] wl1251: Use request_firmware_prefer_user() for loading NVS calibration data

[Michal Kazior]

On 1 February 2017 at 09:33, Pali Rohár  wrote:
> On Tuesday 31 January 2017 07:59:18 Tony Lindgren wrote:
>> * Kalle Valo  [170130 22:36]:
[...]
>> > * before distro updates linux-firmware create yours own deb/rpm/whatever
>> >   package "wl1251-firmware" which installs your flavor of nvs file (or
>> >   the user fallback helper if more dynamic functionality is preferred)
>>
>> And that won't work when using the same file system on other machines.
>>
>> Think NFSroot for example. At least I'm using the same NFSroot across
>> about 15 different machines including one n900 macro board with smc91x
>> Ethernet.
>
> Exactly problem which we already discussed in previous emails. You
> cannot serve one file (loaded by direct request_firmware) when your
> rootfs is readonly, e.g. comes via NFS shared for more devices...

You can extract the nvs blob, put it in tmpfs and bind-mount (or
symlink) it to /lib/firmware/ via modprobe install hook (or init
scripts).


Michał

Re: [PATCH 2/6] wl1251: Use request_firmware_prefer_user() for loading NVS calibration data

[Michal Kazior]

On 1 February 2017 at 09:33, Pali Rohár  wrote:
> On Tuesday 31 January 2017 07:59:18 Tony Lindgren wrote:
>> * Kalle Valo  [170130 22:36]:
[...]
>> > * before distro updates linux-firmware create yours own deb/rpm/whatever
>> >   package "wl1251-firmware" which installs your flavor of nvs file (or
>> >   the user fallback helper if more dynamic functionality is preferred)
>>
>> And that won't work when using the same file system on other machines.
>>
>> Think NFSroot for example. At least I'm using the same NFSroot across
>> about 15 different machines including one n900 macro board with smc91x
>> Ethernet.
>
> Exactly problem which we already discussed in previous emails. You
> cannot serve one file (loaded by direct request_firmware) when your
> rootfs is readonly, e.g. comes via NFS shared for more devices...

You can extract the nvs blob, put it in tmpfs and bind-mount (or
symlink) it to /lib/firmware/ via modprobe install hook (or init
scripts).


Michał

Re: wl1251 & mac address & calibration data

[Michal Kazior]

On 22 November 2016 at 16:31, Pali Rohár <pali.ro...@gmail.com> wrote:
> On Tuesday 22 November 2016 16:22:57 Michal Kazior wrote:
>> On 21 November 2016 at 16:51, Pali Rohár <pali.ro...@gmail.com> wrote:
>> > On Friday 11 November 2016 18:20:50 Pali Rohár wrote:
>> >> Hi! I will open discussion about mac address and calibration data for
>> >> wl1251 wireless chip again...
>> >>
>> >> Problem: Mac address & calibration data for wl1251 chip on Nokia N900
>> >> are stored on second nand partition (mtd1) in special proprietary format
>> >> which is used only for Nokia N900 (probably on N8x0 and N9 too).
>> >> Wireless driver wl1251.ko cannot work without mac address and
>> >> calibration data.
>>
>> Same problem applies to some ath9k/ath10k supported routers. Some even
>> carry mac address as implicit offset from ethernet mac address. As far
>> as I understand OpenWRT cooks cal blobs on first boot prior to loading
>> modules.
>
> So... wl1251 on Nokia N900 is not alone and this problem is there for
> more drivers and devices. Which means we should come up with some
> generic solution.

This isn't particularly a problem for ath9k/ath10k.

Let me give you more background on ath10k.

ath10k devices can come with caldata and macaddr stored in their
OTP/EEPROM. In that case a generic "template" board file is used.
Userspace doesn't need to do anything special.

Some vendors however decide to use flash partition to store caldata.
In that case ath10k expects userspace to prepare cal-$bus-$devname.bin
files, each for a different radio (you can have multiple radios on a
system).

Now translating this for wl1251 I would expect it should also use
something like wl1251-nvs-sdio-0x0001.bin for devices like N900 that
have caldata on flash partition (instead of the generic
wl1251-nvs.bin). I'm not sure if wl1251-nvs.bin is something
comparable to (the generic) board.bin ath10k has though. Maybe the
entire idea behind wl1251-nvs.bin is flawed as it's supposed to be
device specific and is oblivious to possibility of having multiple
wl1251 radios on one system (probably sane assumption from practical
standpoint but still).


>> >> Absence of mac address cause that driver generates random mac address at
>> >> every kernel boot which has couple of problems (unstable identifier of
>> >> wireless device due to udev permanent storage rules; unpredictable
>> >> behaviour for dhcp mac address assignment, mac address filtering, ...).
>> >>
>> >> Currently there is no way to set (permanent) mac address for network
>> >> interface from userspace. And it does not make sense to implement in
>> >> linux kernel large parser for proprietary format of second nand
>> >> partition where is mac address stored only for one device -- Nokia N900.
>> >>
>> >> Driver wl1251.ko loads calibration data via request_firmware() for file
>> >> wl1251-nvs.bin. There are some "example" calibration file in linux-
>> >> firmware repository, but it is not suitable for normal usage as real
>> >> calibration data are per-device specific.
>>
>> You could hook up a script that cooks up the cal/mac file via
>> modprobe's install hook, no?
>
> Via modprobe hook I can either pass custom module parameter or call any
> other system (shell) commands.
>
> As wl1251.ko does not accept mac_address as module parameter, such
> modprobe hook does not help -- as there is absolutely no way from
> userspace to set or change (permanent) mac address.

Quoting modprobe.d manual:

>   install modulename command...
>   This command instructs modprobe to run your
>   command instead of inserting the module in the
>   kernel as normal. The command can be any shell
>   command: this allows you to do any kind of
>   complex processing you might wish. [...]

You can hook up a script that cooks up wl1251-nvs.bin (caldata,
macaddr) and then insmod the actual wl1251.ko module. Or you can just
cook up the nvs on first device boot and store it in /lib/firmware
(possibly overwriting the "generic" wl1251 from linux-firmware).


Michal

Re: wl1251 & mac address & calibration data

[Michal Kazior]

On 22 November 2016 at 16:31, Pali Rohár  wrote:
> On Tuesday 22 November 2016 16:22:57 Michal Kazior wrote:
>> On 21 November 2016 at 16:51, Pali Rohár  wrote:
>> > On Friday 11 November 2016 18:20:50 Pali Rohár wrote:
>> >> Hi! I will open discussion about mac address and calibration data for
>> >> wl1251 wireless chip again...
>> >>
>> >> Problem: Mac address & calibration data for wl1251 chip on Nokia N900
>> >> are stored on second nand partition (mtd1) in special proprietary format
>> >> which is used only for Nokia N900 (probably on N8x0 and N9 too).
>> >> Wireless driver wl1251.ko cannot work without mac address and
>> >> calibration data.
>>
>> Same problem applies to some ath9k/ath10k supported routers. Some even
>> carry mac address as implicit offset from ethernet mac address. As far
>> as I understand OpenWRT cooks cal blobs on first boot prior to loading
>> modules.
>
> So... wl1251 on Nokia N900 is not alone and this problem is there for
> more drivers and devices. Which means we should come up with some
> generic solution.

This isn't particularly a problem for ath9k/ath10k.

Let me give you more background on ath10k.

ath10k devices can come with caldata and macaddr stored in their
OTP/EEPROM. In that case a generic "template" board file is used.
Userspace doesn't need to do anything special.

Some vendors however decide to use flash partition to store caldata.
In that case ath10k expects userspace to prepare cal-$bus-$devname.bin
files, each for a different radio (you can have multiple radios on a
system).

Now translating this for wl1251 I would expect it should also use
something like wl1251-nvs-sdio-0x0001.bin for devices like N900 that
have caldata on flash partition (instead of the generic
wl1251-nvs.bin). I'm not sure if wl1251-nvs.bin is something
comparable to (the generic) board.bin ath10k has though. Maybe the
entire idea behind wl1251-nvs.bin is flawed as it's supposed to be
device specific and is oblivious to possibility of having multiple
wl1251 radios on one system (probably sane assumption from practical
standpoint but still).


>> >> Absence of mac address cause that driver generates random mac address at
>> >> every kernel boot which has couple of problems (unstable identifier of
>> >> wireless device due to udev permanent storage rules; unpredictable
>> >> behaviour for dhcp mac address assignment, mac address filtering, ...).
>> >>
>> >> Currently there is no way to set (permanent) mac address for network
>> >> interface from userspace. And it does not make sense to implement in
>> >> linux kernel large parser for proprietary format of second nand
>> >> partition where is mac address stored only for one device -- Nokia N900.
>> >>
>> >> Driver wl1251.ko loads calibration data via request_firmware() for file
>> >> wl1251-nvs.bin. There are some "example" calibration file in linux-
>> >> firmware repository, but it is not suitable for normal usage as real
>> >> calibration data are per-device specific.
>>
>> You could hook up a script that cooks up the cal/mac file via
>> modprobe's install hook, no?
>
> Via modprobe hook I can either pass custom module parameter or call any
> other system (shell) commands.
>
> As wl1251.ko does not accept mac_address as module parameter, such
> modprobe hook does not help -- as there is absolutely no way from
> userspace to set or change (permanent) mac address.

Quoting modprobe.d manual:

>   install modulename command...
>   This command instructs modprobe to run your
>   command instead of inserting the module in the
>   kernel as normal. The command can be any shell
>   command: this allows you to do any kind of
>   complex processing you might wish. [...]

You can hook up a script that cooks up wl1251-nvs.bin (caldata,
macaddr) and then insmod the actual wl1251.ko module. Or you can just
cook up the nvs on first device boot and store it in /lib/firmware
(possibly overwriting the "generic" wl1251 from linux-firmware).


Michal

Re: wl1251 & mac address & calibration data

[Michal Kazior]

On 21 November 2016 at 16:51, Pali Rohár  wrote:
> On Friday 11 November 2016 18:20:50 Pali Rohár wrote:
>> Hi! I will open discussion about mac address and calibration data for
>> wl1251 wireless chip again...
>>
>> Problem: Mac address & calibration data for wl1251 chip on Nokia N900
>> are stored on second nand partition (mtd1) in special proprietary format
>> which is used only for Nokia N900 (probably on N8x0 and N9 too).
>> Wireless driver wl1251.ko cannot work without mac address and
>> calibration data.

Same problem applies to some ath9k/ath10k supported routers. Some even
carry mac address as implicit offset from ethernet mac address. As far
as I understand OpenWRT cooks cal blobs on first boot prior to loading
modules.


>> Absence of mac address cause that driver generates random mac address at
>> every kernel boot which has couple of problems (unstable identifier of
>> wireless device due to udev permanent storage rules; unpredictable
>> behaviour for dhcp mac address assignment, mac address filtering, ...).
>>
>> Currently there is no way to set (permanent) mac address for network
>> interface from userspace. And it does not make sense to implement in
>> linux kernel large parser for proprietary format of second nand
>> partition where is mac address stored only for one device -- Nokia N900.
>>
>> Driver wl1251.ko loads calibration data via request_firmware() for file
>> wl1251-nvs.bin. There are some "example" calibration file in linux-
>> firmware repository, but it is not suitable for normal usage as real
>> calibration data are per-device specific.

You could hook up a script that cooks up the cal/mac file via
modprobe's install hook, no?


Michał

Re: wl1251 & mac address & calibration data

[Michal Kazior]

On 21 November 2016 at 16:51, Pali Rohár  wrote:
> On Friday 11 November 2016 18:20:50 Pali Rohár wrote:
>> Hi! I will open discussion about mac address and calibration data for
>> wl1251 wireless chip again...
>>
>> Problem: Mac address & calibration data for wl1251 chip on Nokia N900
>> are stored on second nand partition (mtd1) in special proprietary format
>> which is used only for Nokia N900 (probably on N8x0 and N9 too).
>> Wireless driver wl1251.ko cannot work without mac address and
>> calibration data.

Same problem applies to some ath9k/ath10k supported routers. Some even
carry mac address as implicit offset from ethernet mac address. As far
as I understand OpenWRT cooks cal blobs on first boot prior to loading
modules.


>> Absence of mac address cause that driver generates random mac address at
>> every kernel boot which has couple of problems (unstable identifier of
>> wireless device due to udev permanent storage rules; unpredictable
>> behaviour for dhcp mac address assignment, mac address filtering, ...).
>>
>> Currently there is no way to set (permanent) mac address for network
>> interface from userspace. And it does not make sense to implement in
>> linux kernel large parser for proprietary format of second nand
>> partition where is mac address stored only for one device -- Nokia N900.
>>
>> Driver wl1251.ko loads calibration data via request_firmware() for file
>> wl1251-nvs.bin. There are some "example" calibration file in linux-
>> firmware repository, but it is not suitable for normal usage as real
>> calibration data are per-device specific.

You could hook up a script that cooks up the cal/mac file via
modprobe's install hook, no?


Michał

Re: [PATCH 2/3 RFC] ath10k: wmi: match wait_for_completion_timeout return type

[Michal Kazior]

On 12 March 2015 at 16:49, Nicholas Mc Guire  wrote:
> Return type of wait_for_completion_timeout is unsigned long not int.
> An appropriately named unsigned long is added and the assignments fixed up.
> Rather than returning 0 (timeout) or a more or less random remaining time
> (completion success) this return 0 or 1 which also resolves the type of the
> functions being int.
>
> Signed-off-by: Nicholas Mc Guire 
> ---
>
> Checking the call-sites of ath10k_wmi_wait_for_unified_ready and
> ath10k_wmi_wait_for_service_ready the positive return value (remaining
> time in jiffies) is never passed up the call-chain nor used so it is
> cleaner to treat this like a boolean success/fail only (actually the two
> functions should probably be of type bool - but that does not seem to be
> common practice in the ath10k code base)

It'd make sense to have these functions return 0 or -ETIMEDOUT. In
that case both call sites would need to be adjusted to treat "< 0" or
"!x" as an error (instead of the current "<= 0") condition and not set
-ETIMEDOUT themselves.


Michał
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 2/3 RFC] ath10k: wmi: match wait_for_completion_timeout return type

[Michal Kazior]

On 12 March 2015 at 16:49, Nicholas Mc Guire hof...@osadl.org wrote:
 Return type of wait_for_completion_timeout is unsigned long not int.
 An appropriately named unsigned long is added and the assignments fixed up.
 Rather than returning 0 (timeout) or a more or less random remaining time
 (completion success) this return 0 or 1 which also resolves the type of the
 functions being int.

 Signed-off-by: Nicholas Mc Guire hof...@osadl.org
 ---

 Checking the call-sites of ath10k_wmi_wait_for_unified_ready and
 ath10k_wmi_wait_for_service_ready the positive return value (remaining
 time in jiffies) is never passed up the call-chain nor used so it is
 cleaner to treat this like a boolean success/fail only (actually the two
 functions should probably be of type bool - but that does not seem to be
 common practice in the ath10k code base)

It'd make sense to have these functions return 0 or -ETIMEDOUT. In
that case both call sites would need to be adjusted to treat  0 or
!x as an error (instead of the current = 0) condition and not set
-ETIMEDOUT themselves.


Michał
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] ath:Release resources for structure pointer, ar if error pointing device in the function, ath10k_core_register_work

[Michal Kazior]

On 3 March 2015 at 03:36, Nicholas Krause  wrote:
> Releases resources and deregisters the stucture pointer ar passed by the 
> caller to the function, ath10k_core_register_work
> if unable to probe the structure pointer successfully with a call to 
> ath10k_core_probe_fw. Further more if this happerns
> we must first jump to the label err for the goto statement required to jump 
> to handle this particular error in the function,
> ath10k_core_register_work.  After we are in the correct error section we must 
> free the resources for the structure pointer,ar
> with a call to the function,  ath10k_core_unregister to free resources 
> allocated for the structure pointer,ar.
>
> Signed-off-by: Nicholas Krause 
> ---
>  drivers/net/wireless/ath/ath10k/core.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/drivers/net/wireless/ath/ath10k/core.c 
> b/drivers/net/wireless/ath/ath10k/core.c
> index 310e12b..8b2ca25 100644
> --- a/drivers/net/wireless/ath/ath10k/core.c
> +++ b/drivers/net/wireless/ath/ath10k/core.c
> @@ -1307,9 +1307,7 @@ err_unregister_mac:
>  err_release_fw:
> ath10k_core_free_firmware_files(ar);
>  err:
> -   /* TODO: It's probably a good idea to release device from the driver
> -* but calling device_release_driver() here will cause a deadlock.
> -*/
> +   ath10k_core_unregister(ar);
> return;
>  }

Did you test this? This will deadlock. ath10k_core_unregister() tries
to cancel ar->register_work. This won't work if you call it from the
worker itself. Moreover if I ignore the deadlock
ath10k_core_unregister() would do nothing else in this context because
ATH10K_FLAG_CORE_REGISTERED wouldn't be even set.

If you're interested in dealing with this TODO I suggest you read
through the original thread which led to the current state of affairs:

  http://www.spinics.net/lists/linux-wireless/msg124004.html


Michał
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] ath:Release resources for structure pointer, ar if error pointing device in the function, ath10k_core_register_work

[Michal Kazior]

On 3 March 2015 at 03:36, Nicholas Krause xerofo...@gmail.com wrote:
 Releases resources and deregisters the stucture pointer ar passed by the 
 caller to the function, ath10k_core_register_work
 if unable to probe the structure pointer successfully with a call to 
 ath10k_core_probe_fw. Further more if this happerns
 we must first jump to the label err for the goto statement required to jump 
 to handle this particular error in the function,
 ath10k_core_register_work.  After we are in the correct error section we must 
 free the resources for the structure pointer,ar
 with a call to the function,  ath10k_core_unregister to free resources 
 allocated for the structure pointer,ar.

 Signed-off-by: Nicholas Krause xerofo...@gmail.com
 ---
  drivers/net/wireless/ath/ath10k/core.c | 4 +---
  1 file changed, 1 insertion(+), 3 deletions(-)

 diff --git a/drivers/net/wireless/ath/ath10k/core.c 
 b/drivers/net/wireless/ath/ath10k/core.c
 index 310e12b..8b2ca25 100644
 --- a/drivers/net/wireless/ath/ath10k/core.c
 +++ b/drivers/net/wireless/ath/ath10k/core.c
 @@ -1307,9 +1307,7 @@ err_unregister_mac:
  err_release_fw:
 ath10k_core_free_firmware_files(ar);
  err:
 -   /* TODO: It's probably a good idea to release device from the driver
 -* but calling device_release_driver() here will cause a deadlock.
 -*/
 +   ath10k_core_unregister(ar);
 return;
  }

Did you test this? This will deadlock. ath10k_core_unregister() tries
to cancel ar-register_work. This won't work if you call it from the
worker itself. Moreover if I ignore the deadlock
ath10k_core_unregister() would do nothing else in this context because
ATH10K_FLAG_CORE_REGISTERED wouldn't be even set.

If you're interested in dealing with this TODO I suggest you read
through the original thread which led to the current state of affairs:

  http://www.spinics.net/lists/linux-wireless/msg124004.html


Michał
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/