* Re: [PATCH v2 0/7] ath9k: EEPROM swapping improvements
From: Martin Blumenstingl @ 2016-12-17 14:40 UTC (permalink / raw)
To: Adrian Chadd
Cc: Valo, Kalle, ath9k-devel, linux-wireless@vger.kernel.org,
ath9k-devel@lists.ath9k.org, devicetree@vger.kernel.org,
arnd@arndb.de, chunkeey@googlemail.com, nbd@nbd.name
In-Reply-To: <CAJ-Vmo=3zox7QkFUA-3yxtvSTzPT4GiFkoOUU3cPTXSN4xV8vQ@mail.gmail.com>
Hi Adrian,
On Wed, Dec 14, 2016 at 7:45 AM, Adrian Chadd <adrian@freebsd.org> wrote:
> hi,
>
> On 12 December 2016 at 12:05, Martin Blumenstingl
> <martin.blumenstingl@googlemail.com> wrote:
>
>>
>> It seems that there are a few devices out there where the whole EEPROM
>> is swab16'ed which switches the position of the 1-byte fields
>> opCapFlags and eepMisc.
>> those still work fine with the new code, however I had a second patch
>> in LEDE [0] which results in ath9k_platform_data.endian_check NOT
>> being set anymore.
>> that endian_check flag was used before to swab16 the whole EEPROM, to
>> correct the position of the 1-byte fields again.
>> Currently we are fixing this in the firmware hotplug script: [1]
>> This is definitely not a blocker for this series though (if we want to
>> have a devicetree replacement for "ath9k_platform_data.endian_check"
>> then I'd work on that within a separate series, but I somewhat
>> consider these EEPROMs as "broken" so fixing them in
>> userspace/firmware hotplug script is fine for me)
>
> As a reference - the reference driver has been doign this for a while.
> It attempts to detect the endianness by looking at the 0xa55a
> signature endian and figuring out which endian the actual contents are
> in.
>
> So just FYI yeah, this is a "thing" for reasons I don't quite know.
on all devices I have seen so far (all customer devices, no
development boards) these two magic bytes *can* be used to detect the
endianness of the data that is written to the PCI memory (and thus
whether swapping of that data is required or not).
however, there are many devices (roughly 50% of the ones I've seen)
where the magic bytes cannot be used to swap the actual EEPROM (=
calibration) data because they are "inverted". reading the eepMisc
byte works fine on all devices I've seen so far *if* the manufacturer
did not swab16 all data (PCI memory and EEPROM data).
on the other hand you are right: all four devices which were broken
had the correct magic bytes at the start, but as long as this is not
the case for all devices we cannot use it without some feature-flag.
as an (unrelated) side-note: I've also some EEPROMs where the length
matches neither the "magic bytes endianness" nor the "eepMisc
endianness". I consider these broken as well, but fortunately ath9k
has a fallback for this issue.
Regards,
Martin
^ permalink raw reply
* MAC address in wl1251 NVS data (Was: Re: wl1251 NVS calibration data format)
From: Pali Rohár @ 2016-12-17 13:10 UTC (permalink / raw)
To: Sebastian Reichel
Cc: Gery Kahn, Shahar Lev, Kalle Valo, linux-wireless, linux-kernel,
Pavel Machek, Ivaylo Dimitrov
In-Reply-To: <20161217120350.phnlfhklwfqqgbjr@earth>
[-- Attachment #1: Type: Text/Plain, Size: 2381 bytes --]
> On Sat, Dec 17, 2016 at 12:14:50PM +0100, Pali Rohár wrote:
> > > [1] http://notaz.gp2x.de/misc/pnd/wl1251/nvs_map.txt
> > In that description is something about STA mac address:
> >
019 02 //length
> > 01a 6d //STA_ADDR_L Register Address. (STA MAC
> > Address)
> > 01b 54 //
> > 01c 00 //STA_ADDR_L Register
> > 01d 00 //
> > 01e 32 //
> > 01f 28 //
> > 020 00 //STA_ADDR_H Register Data.
021 08 //
022 00 //
023 00 //
So... above data means:
019 - number of words
01a - low bits of offset applied with mask 0xfe
01b - high bits of offset
01c-01f first word
020-023 second word
Interpreted as: at address offset 0x536c are written two words
0x28320000 and 0x00000800
wl1271 driver has in linux/drivers/net/wireless/ti/wlcore/boot.c this:
/* update current MAC address to NVS */
nvs_ptr[11] = wl->addresses[0].addr[0];
nvs_ptr[10] = wl->addresses[0].addr[1];
nvs_ptr[6] = wl->addresses[0].addr[2];
nvs_ptr[5] = wl->addresses[0].addr[3];
nvs_ptr[4] = wl->addresses[0].addr[4];
nvs_ptr[3] = wl->addresses[0].addr[5];
Looking at wl1271-nvs.bin file (which is "modified" in kernel by boot.c)
000: 01
001: 6d
002: 54
003: 00
004: 00
005: ef
006: be
Means: at address offset 0x536c is written one word 0xBEEF0000
007: 01
008: 71
009: 54
00a: ad
00b: de
00c: 00
00d: 00
Means: at address offset 0x5371 is written one word 0x0000DEAD
Above boot.c kernel code updates those data to MAC address, so at
address offset 0x536c is written four low bytes of MAC address and to
0x5371 are written remaining two bytes. So 00:00:DE:AD:BE:EF
So conclusion: address offset for wl1271 (where is written MAC address)
is exactly same as for wl1251 which is marked in that documentation as
STA_ADDR_L Register.
Btw, in our wl1251-nvs.bin found in Maemo rootfs, which is exactly same
as in linux-firmware.git tree there are those data:
019: 02
01a: 6d
01b: 54
01c: 09
01d: 03
01e: 07
01f: 20
020: 00
021: 00
022: 00
023: 00
So hardcoded MAC address in wl1251-nvs.bin is: 00:00:20:07:03:09. Which
is assigned to DIAB. Strange that it is not TI...
--
Pali Rohár
pali.rohar@gmail.com
[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]
^ permalink raw reply
* Re: wl1251 NVS calibration data format
From: Sebastian Reichel @ 2016-12-17 12:03 UTC (permalink / raw)
To: Pali Rohár
Cc: Gery Kahn, Shahar Lev, Kalle Valo, linux-wireless, linux-kernel,
Pavel Machek, Ivaylo Dimitrov
In-Reply-To: <201612171214.50820@pali>
[-- Attachment #1: Type: text/plain, Size: 2595 bytes --]
Hi,
On Sat, Dec 17, 2016 at 12:14:50PM +0100, Pali Rohár wrote:
> On Saturday 17 December 2016 10:37:05 Sebastian Reichel wrote:
> > On Fri, Dec 16, 2016 at 12:01:48PM +0100, Pali Rohár wrote:
> > > Hi! Do you know format of wl1251 NVS calibration data file?
> > >
> > > I found that there is tool for changing NVS file for wl1271 and
> > > newer chips (so not for wl1251!) at:
> > > https://github.com/gxk/ti-utils
> > >
> > > And wl1271 has in NVS data already place for MAC address. And in
> > > wlcore (for wl1271 and newer) there is really kernel code which is
> > > doing something with MAC address in NVS, see:
> > > https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tre
> > > e/drivers/net/wireless/ti/wlcore/boot.c#n352
> > >
> > > So... I would like to know if in wl1251 NVS calibration file is
> > > also some place for MAC address or not.
> > >
> > > Default wl1251 NVS calibration file is available in linux-firmware:
> > > https://git.kernel.org/cgit/linux/kernel/git/firmware/linux-firmwar
> > > e.git/tree/ti-connectivity/wl1251-nvs.bin
> >
> > Pandora people [0] have a description of the format at [1].
> >
> > [0] https://pandorawiki.org/WiFi
> > [1] http://notaz.gp2x.de/misc/pnd/wl1251/nvs_map.txt
>
> Thank you very very much!
You are welcome.
> I tried to search for something, but I have not find anything.
> In that description is something about STA mac address:
>
> 01a 6d //STA_ADDR_L Register Address. (STA MAC Address)
> 01b 54 //
> 01c 00 //STA_ADDR_L Register
> 01d 00 //
> 01e 32 //
> 01f 28 //
> 020 00 //STA_ADDR_H Register Data.
>
> STA would be abbreviation for station and so it should be really set to
> mac address of that chip?
Yes, STA is a common abbreviation:
https://en.wikipedia.org/wiki/Station_(networking)
> If yes, that could allow us to set permanent MAC address at time when
> loading & sending NVS calibration data... Exactly same as wl1271 and new
> drivers are working.
>
> I will try to play with driver if it is really truth!
Thanks for your work.
> I already looked into original TI's multiplatform HAL driver for wl1251
> chip (big mess) and found there that there is wl1251 command to read mac
> address from chip. It could be done by this wl1251 function:
>
> wl1251_cmd_interrogate(wl, DOT11_STATION_ID, mac, sizeof(*mac))
>
> (same id as for setting permanent mac address, but opposite to read it)
-- Sebastian
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: wl1251 NVS calibration data format
From: Pali Rohár @ 2016-12-17 11:14 UTC (permalink / raw)
To: Sebastian Reichel
Cc: Gery Kahn, Shahar Lev, Kalle Valo, linux-wireless, linux-kernel,
Pavel Machek, Ivaylo Dimitrov
In-Reply-To: <20161217093705.p64yzumqlu3u5aq7@earth>
[-- Attachment #1: Type: Text/Plain, Size: 2326 bytes --]
On Saturday 17 December 2016 10:37:05 Sebastian Reichel wrote:
> Hi,
>
> On Fri, Dec 16, 2016 at 12:01:48PM +0100, Pali Rohár wrote:
> > Hi! Do you know format of wl1251 NVS calibration data file?
> >
> > I found that there is tool for changing NVS file for wl1271 and
> > newer chips (so not for wl1251!) at:
> > https://github.com/gxk/ti-utils
> >
> > And wl1271 has in NVS data already place for MAC address. And in
> > wlcore (for wl1271 and newer) there is really kernel code which is
> > doing something with MAC address in NVS, see:
> > https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tre
> > e/drivers/net/wireless/ti/wlcore/boot.c#n352
> >
> > So... I would like to know if in wl1251 NVS calibration file is
> > also some place for MAC address or not.
> >
> > Default wl1251 NVS calibration file is available in linux-firmware:
> > https://git.kernel.org/cgit/linux/kernel/git/firmware/linux-firmwar
> > e.git/tree/ti-connectivity/wl1251-nvs.bin
>
> Pandora people [0] have a description of the format at [1].
>
> [0] https://pandorawiki.org/WiFi
> [1] http://notaz.gp2x.de/misc/pnd/wl1251/nvs_map.txt
Thank you very very much!
I tried to search for something, but I have not find anything.
In that description is something about STA mac address:
01a 6d //STA_ADDR_L Register Address. (STA MAC Address)
01b 54 //
01c 00 //STA_ADDR_L Register
01d 00 //
01e 32 //
01f 28 //
020 00 //STA_ADDR_H Register Data.
STA would be abbreviation for station and so it should be really set to
mac address of that chip?
If yes, that could allow us to set permanent MAC address at time when
loading & sending NVS calibration data... Exactly same as wl1271 and new
drivers are working.
I will try to play with driver if it is really truth!
I already looked into original TI's multiplatform HAL driver for wl1251
chip (big mess) and found there that there is wl1251 command to read mac
address from chip. It could be done by this wl1251 function:
wl1251_cmd_interrogate(wl, DOT11_STATION_ID, mac, sizeof(*mac))
(same id as for setting permanent mac address, but opposite to read it)
--
Pali Rohár
pali.rohar@gmail.com
[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]
^ permalink raw reply
* Re: wl1251 NVS calibration data format
From: Sebastian Reichel @ 2016-12-17 9:37 UTC (permalink / raw)
To: Pali Rohár
Cc: Gery Kahn, Shahar Lev, Kalle Valo, linux-wireless, linux-kernel,
Pavel Machek, Ivaylo Dimitrov
In-Reply-To: <201612161201.48356@pali>
[-- Attachment #1: Type: text/plain, Size: 1056 bytes --]
Hi,
On Fri, Dec 16, 2016 at 12:01:48PM +0100, Pali Rohár wrote:
> Hi! Do you know format of wl1251 NVS calibration data file?
>
> I found that there is tool for changing NVS file for wl1271 and newer
> chips (so not for wl1251!) at: https://github.com/gxk/ti-utils
>
> And wl1271 has in NVS data already place for MAC address. And in wlcore
> (for wl1271 and newer) there is really kernel code which is doing
> something with MAC address in NVS, see:
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/net/wireless/ti/wlcore/boot.c#n352
>
> So... I would like to know if in wl1251 NVS calibration file is also
> some place for MAC address or not.
>
> Default wl1251 NVS calibration file is available in linux-firmware:
> https://git.kernel.org/cgit/linux/kernel/git/firmware/linux-firmware.git/tree/ti-connectivity/wl1251-nvs.bin
Pandora people [0] have a description of the format at [1].
[0] https://pandorawiki.org/WiFi
[1] http://notaz.gp2x.de/misc/pnd/wl1251/nvs_map.txt
-- Sebastian
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [Patch] NFC: trf7970a:
From: Mark Greer @ 2016-12-16 20:35 UTC (permalink / raw)
To: Geoff Lansberry
Cc: linux-wireless, Lauro Ramos Venancio, Aloisio Almeida Jr,
Samuel Ortiz, Justin Bronder
In-Reply-To: <20161216045210.GA29196@animalcreek.com>
On Thu, Dec 15, 2016 at 09:52:10PM -0700, Mark Greer wrote:
> On Wed, Dec 14, 2016 at 03:31:23PM -0700, Mark Greer wrote:
> > I'll start on this
> > tonight but won't likely get far until tomorrow. In the meantime,
> > if you and/or your contractor make progress, please share.
>
> Geoff,
>
> Which version of neard are you using? 0.16?
Also, the flood.py script doesn't work well at all for me. At best,
it works successfully for one iteration and then fails continually for
all other iterations. This is true when using the trf7970a and pn533
drivers.
I've tweaked it a but but still no success. I haven't looked all that
closely at it but since you said you were persuing this, I'll wait to
hear more from you.
Mark
--
^ permalink raw reply
* Re: [PATCH 03/14 V2] rtlwifi: rtl8821ae: Remove all instances of DBG_EMERG
From: Joe Perches @ 2016-12-16 19:34 UTC (permalink / raw)
To: Larry Finger, kvalo; +Cc: devel, linux-wireless, Ping-Ke Shih
In-Reply-To: <20161215182310.13713-4-Larry.Finger@lwfinger.net>
On Thu, 2016-12-15 at 12:22 -0600, Larry Finger wrote:
> This is a step toward eliminating the RT_TRACE macros. Those calls that
> have DBG_EMERG as the level are always logged, and they represent error
> conditions, thus they are replaced with pr_err().
OK,
> diff --git a/drivers/net/wireless/realtek/rtlwifi/rtl8821ae/fw.c b/drivers/net/wireless/realtek/rtlwifi/rtl8821ae/fw.c
[]
> @@ -162,8 +161,8 @@ static int _rtl8821ae_fw_free_to_go(struct ieee80211_hw *hw)
> goto exit;
> }
>
> - RT_TRACE(rtlpriv, COMP_FW, DBG_EMERG,
> - "Checksum report OK ! REG_MCUFWDL:0x%08x .\n", value32);
> + pr_err("Checksum report OK! REG_MCUFWDL:0x%08x\n",
> + value32);
>
> value32 = rtl_read_dword(rtlpriv, REG_MCUFWDL);
> value32 |= MCUFWDL_RDY;
> @@ -186,9 +184,8 @@ static int _rtl8821ae_fw_free_to_go(struct ieee80211_hw *hw)
> udelay(FW_8821AE_POLLING_DELAY);
> } while (counter++ < FW_8821AE_POLLING_TIMEOUT_COUNT);
>
> - RT_TRACE(rtlpriv, COMP_ERR, DBG_EMERG,
> - "Polling FW ready fail!! REG_MCUFWDL:0x%08x .\n",
> - value32);
> + pr_err("Polling FW ready fail!! REG_MCUFWDL:0x%08x .\n",
> + value32);
It's odd to fix / remove " .\n" above but here and
the wrapping comment on the first patch applies too.
I didn't look at the rest and I won't comment on
other uses in any further patches in the series.
Thanks, Joe
^ permalink raw reply
* Re: [PATCH 02/14 V2] rtlwifi: Remove RT_TRACE messages that use DBG_EMERG
From: Joe Perches @ 2016-12-16 19:31 UTC (permalink / raw)
To: Larry Finger, kvalo; +Cc: devel, linux-wireless, Ping-Ke Shih
In-Reply-To: <20161215182310.13713-3-Larry.Finger@lwfinger.net>
On Thu, 2016-12-15 at 12:22 -0600, Larry Finger wrote:
> These messages are always logged and represent error conditions, thus
> we can use pr_err().
OK and some trivialities:
> diff --git a/drivers/net/wireless/realtek/rtlwifi/base.c b/drivers/net/wireless/realtek/rtlwifi/base.c
[]
> @@ -389,8 +388,8 @@ static void _rtl_init_mac80211(struct ieee80211_hw *hw)
> /* <4> set mac->sband to wiphy->sband */
> hw->wiphy->bands[NL80211_BAND_5GHZ] = sband;
> } else {
> - RT_TRACE(rtlpriv, COMP_INIT, DBG_EMERG, "Err BAND %d\n",
> - rtlhal->current_bandtype);
> + pr_err("Err BAND %d\n",
> + rtlhal->current_bandtype);
It's nice to rewrap lines to 80 columns where possible.
> @@ -1886,8 +1883,7 @@ void rtl_phy_scan_operation_backup(struct ieee80211_hw *hw, u8 operation)
> (u8 *)&iotype);
> break;
> default:
> - RT_TRACE(rtlpriv, COMP_ERR, DBG_EMERG,
> - "Unknown Scan Backup operation.\n");
> + pr_err("Unknown Scan Backup operation.\n");
And it's also nice to remove unnecessary periods for
output message consistency. Most don't use it.
> diff --git a/drivers/net/wireless/realtek/rtlwifi/cam.c b/drivers/net/wireless/realtek/rtlwifi/cam.c
[]
> @@ -285,8 +285,7 @@ u8 rtl_cam_get_free_entry(struct ieee80211_hw *hw, u8 *sta_addr)
> u8 i, *addr;
>
> if (NULL == sta_addr) {
> - RT_TRACE(rtlpriv, COMP_SEC, DBG_EMERG,
> - "sta_addr is NULL.\n");
> + pr_err("sta_addr is NULL.\n");
etc...
^ permalink raw reply
* "rfkill: Add rfkill-any LED trigger" causes deadlock
From: Mike Krinkin @ 2016-12-16 17:09 UTC (permalink / raw)
To: kernel, johannes.berg, linux-wireless, davem, netdev
In-Reply-To: <CACpa5=_chGuU3zVcVR3nkNySwz1cGAQTs39v+BjwcDNSvOqhTw@mail.gmail.com>
On Fri, Dec 16, 2016 at 07:46:06PM +0300, Михаил Кринкин wrote:
> Hi,
>
> with recent i can't load my thinkpad with recent linux-next, bisect points
> at the commit 73f4f76a196d7adb ("rfkill: Add rfkill-any LED trigger").
>
> Problem occurs because thinkapd_acpi rfkill set_block handler
> tpacpi_rfk_hook_set_block calls rfkill_set_sw_state, which in turn calls
> new rfkill_any_led_trigger_event. So when rfkill_set_block called from
> rfkill_register we have deadlock.
>
> I added WARN to __rfkill_any_led_trigger_event to see how deadlock occurs,
> here is backtrace:
>
> [ 6.090079] WARNING: CPU: 2 PID: 307 at net/rfkill/core.c:184
> rfkill_any_led_trigger_event+0x2d/0x40
> [ 6.090080] reached __rfkill_any_led_trigger_event
> [ 6.090081] Modules linked in:
> [ 6.090082] snd_pcm sg thinkpad_acpi(+) mei_me nvram snd_seq mei
> idma64 virt_dma intel_pch_thermal ucsi intel_lpss_pci snd_seq_device
> hci_uart snd_timer snd btbcm soundcore btqca led_class btintel
> bluetooth i8042 rtc_cmos serio evdev intel_lpss_acpi intel_lpss
> acpi_pad tpm_infineon mfd_core kvm_intel kvm irqbypass ipv6 autofs4
> ext4 jbd2 mbcache sd_mod i915 i2c_algo_bit drm_kms_helper syscopyarea
> sysfillrect sysimgblt fb_sys_fops drm ahci libahci video
> pinctrl_sunrisepoint pinctrl_intel
> [ 6.090112] CPU: 2 PID: 307 Comm: systemd-udevd Tainted: G W
> 4.9.0-01741-g73f4f76-dirty #106
> [ 6.090113] Hardware name: LENOVO 20GJ004ERT/20GJ004ERT, BIOS
> R0CET21W (1.09 ) 03/07/2016
> [ 6.090114] ffffb199c023f998 ffffffffb4334b6b ffffb199c023f9e8
> 0000000000000000
> [ 6.090117] ffffb199c023f9d8 ffffffffb40658ab 000000b84dd75d28
> 0000000080000006
> [ 6.090119] ffff99ea4dd75d28 0000000000000001 0000000000000001
> 0000000000000000
> [ 6.090122] Call Trace:
> [ 6.090126] [<ffffffffb4334b6b>] dump_stack+0x4d/0x72
> [ 6.090128] [<ffffffffb40658ab>] __warn+0xcb/0xf0
> [ 6.090130] [<ffffffffb406592f>] warn_slowpath_fmt+0x5f/0x80
> [ 6.090132] [<ffffffffb4631bfd>] rfkill_any_led_trigger_event+0x2d/0x40
> [ 6.090135] [<ffffffffb46322d9>] rfkill_set_sw_state+0x89/0xd0
> [ 6.090142] [<ffffffffc06c9921>]
> tpacpi_rfk_update_swstate+0x31/0x50 [thinkpad_acpi]
> [ 6.090148] [<ffffffffc06c9972>]
> tpacpi_rfk_hook_set_block+0x32/0x70 [thinkpad_acpi]
> [ 6.090150] [<ffffffffb46327f0>] rfkill_set_block+0x90/0x160
> [ 6.090152] [<ffffffffb4632930>] __rfkill_switch_all+0x70/0xa0
> [ 6.090154] [<ffffffffb4633028>] rfkill_register+0x278/0x2b0
> [ 6.090159] [<ffffffffc06e1f0e>] tpacpi_new_rfkill+0xef/0x15d
> [thinkpad_acpi]
> [ 6.090164] [<ffffffffc06e2407>] bluetooth_init+0x15e/0x1a9 [thinkpad_acpi]
> [ 6.090169] [<ffffffffc06e2fb5>]
> thinkpad_acpi_module_init.part.47+0x5e7/0x996 [thinkpad_acpi]
> [ 6.090174] [<ffffffffc06e3364>] ?
> thinkpad_acpi_module_init.part.47+0x996/0x996 [thinkpad_acpi]
> [ 6.090179] [<ffffffffc06e36af>]
> thinkpad_acpi_module_init+0x34b/0xc9c [thinkpad_acpi]
> [ 6.090181] [<ffffffffb4000450>] do_one_initcall+0x50/0x180
> [ 6.090184] [<ffffffffb4177e12>] ? do_init_module+0x27/0x1ee
> [ 6.090186] [<ffffffffb41cc4b6>] ? kmem_cache_alloc_trace+0x156/0x1a0
> [ 6.090188] [<ffffffffb4177e4a>] do_init_module+0x5f/0x1ee
> [ 6.090191] [<ffffffffb40ed562>] load_module+0x22c2/0x28d0
> [ 6.090192] [<ffffffffb40ea0f0>] ? __symbol_put+0x60/0x60
> [ 6.090195] [<ffffffffb42ee15d>] ? ima_post_read_file+0x7d/0xa0
> [ 6.090198] [<ffffffffb42a809b>] ? security_kernel_post_read_file+0x6b/0x80
> [ 6.090200] [<ffffffffb40eddcf>] SYSC_finit_module+0xdf/0x110
> [ 6.090202] [<ffffffffb40ede1e>] SyS_finit_module+0xe/0x10
> [ 6.090204] [<ffffffffb463d524>] entry_SYSCALL_64_fastpath+0x17/0x98
> [ 6.090206] ---[ end trace ea6da61c1ec208d1 ]---
> [ 6.090207] rfkill_any_led_trigger unlocked
> [ 6.091664] ------------[ cut here ]------------
^ permalink raw reply
* "rfkill: Add rfkill-any LED trigger" causes deadlock
From: Михаил Кринкин @ 2016-12-16 16:46 UTC (permalink / raw)
To: kernel, johannes.berg, linux-wireless, davem, netdev
In-Reply-To: <20161216163707.GA2629@gmail.com>
Hi,
with recent i can't load my thinkpad with recent linux-next, bisect points
at the commit 73f4f76a196d7adb ("rfkill: Add rfkill-any LED trigger").
Problem occurs because thinkapd_acpi rfkill set_block handler
tpacpi_rfk_hook_set_block calls rfkill_set_sw_state, which in turn calls
new rfkill_any_led_trigger_event. So when rfkill_set_block called from
rfkill_register we have deadlock.
I added WARN to __rfkill_any_led_trigger_event to see how deadlock occurs,
here is backtrace:
[ 6.090079] WARNING: CPU: 2 PID: 307 at net/rfkill/core.c:184
rfkill_any_led_trigger_event+0x2d/0x40
[ 6.090080] reached __rfkill_any_led_trigger_event
[ 6.090081] Modules linked in:
[ 6.090082] snd_pcm sg thinkpad_acpi(+) mei_me nvram snd_seq mei
idma64 virt_dma intel_pch_thermal ucsi intel_lpss_pci snd_seq_device
hci_uart snd_timer snd btbcm soundcore btqca led_class btintel
bluetooth i8042 rtc_cmos serio evdev intel_lpss_acpi intel_lpss
acpi_pad tpm_infineon mfd_core kvm_intel kvm irqbypass ipv6 autofs4
ext4 jbd2 mbcache sd_mod i915 i2c_algo_bit drm_kms_helper syscopyarea
sysfillrect sysimgblt fb_sys_fops drm ahci libahci video
pinctrl_sunrisepoint pinctrl_intel
[ 6.090112] CPU: 2 PID: 307 Comm: systemd-udevd Tainted: G W
4.9.0-01741-g73f4f76-dirty #106
[ 6.090113] Hardware name: LENOVO 20GJ004ERT/20GJ004ERT, BIOS
R0CET21W (1.09 ) 03/07/2016
[ 6.090114] ffffb199c023f998 ffffffffb4334b6b ffffb199c023f9e8
0000000000000000
[ 6.090117] ffffb199c023f9d8 ffffffffb40658ab 000000b84dd75d28
0000000080000006
[ 6.090119] ffff99ea4dd75d28 0000000000000001 0000000000000001
0000000000000000
[ 6.090122] Call Trace:
[ 6.090126] [<ffffffffb4334b6b>] dump_stack+0x4d/0x72
[ 6.090128] [<ffffffffb40658ab>] __warn+0xcb/0xf0
[ 6.090130] [<ffffffffb406592f>] warn_slowpath_fmt+0x5f/0x80
[ 6.090132] [<ffffffffb4631bfd>] rfkill_any_led_trigger_event+0x2d/0x40
[ 6.090135] [<ffffffffb46322d9>] rfkill_set_sw_state+0x89/0xd0
[ 6.090142] [<ffffffffc06c9921>]
tpacpi_rfk_update_swstate+0x31/0x50 [thinkpad_acpi]
[ 6.090148] [<ffffffffc06c9972>]
tpacpi_rfk_hook_set_block+0x32/0x70 [thinkpad_acpi]
[ 6.090150] [<ffffffffb46327f0>] rfkill_set_block+0x90/0x160
[ 6.090152] [<ffffffffb4632930>] __rfkill_switch_all+0x70/0xa0
[ 6.090154] [<ffffffffb4633028>] rfkill_register+0x278/0x2b0
[ 6.090159] [<ffffffffc06e1f0e>] tpacpi_new_rfkill+0xef/0x15d
[thinkpad_acpi]
[ 6.090164] [<ffffffffc06e2407>] bluetooth_init+0x15e/0x1a9 [thinkpad_acpi]
[ 6.090169] [<ffffffffc06e2fb5>]
thinkpad_acpi_module_init.part.47+0x5e7/0x996 [thinkpad_acpi]
[ 6.090174] [<ffffffffc06e3364>] ?
thinkpad_acpi_module_init.part.47+0x996/0x996 [thinkpad_acpi]
[ 6.090179] [<ffffffffc06e36af>]
thinkpad_acpi_module_init+0x34b/0xc9c [thinkpad_acpi]
[ 6.090181] [<ffffffffb4000450>] do_one_initcall+0x50/0x180
[ 6.090184] [<ffffffffb4177e12>] ? do_init_module+0x27/0x1ee
[ 6.090186] [<ffffffffb41cc4b6>] ? kmem_cache_alloc_trace+0x156/0x1a0
[ 6.090188] [<ffffffffb4177e4a>] do_init_module+0x5f/0x1ee
[ 6.090191] [<ffffffffb40ed562>] load_module+0x22c2/0x28d0
[ 6.090192] [<ffffffffb40ea0f0>] ? __symbol_put+0x60/0x60
[ 6.090195] [<ffffffffb42ee15d>] ? ima_post_read_file+0x7d/0xa0
[ 6.090198] [<ffffffffb42a809b>] ? security_kernel_post_read_file+0x6b/0x80
[ 6.090200] [<ffffffffb40eddcf>] SYSC_finit_module+0xdf/0x110
[ 6.090202] [<ffffffffb40ede1e>] SyS_finit_module+0xe/0x10
[ 6.090204] [<ffffffffb463d524>] entry_SYSCALL_64_fastpath+0x17/0x98
[ 6.090206] ---[ end trace ea6da61c1ec208d1 ]---
[ 6.090207] rfkill_any_led_trigger unlocked
[ 6.091664] ------------[ cut here ]------------
^ permalink raw reply
* Re: [RFC V3 04/11] nl80211: add driver api for gscan notifications
From: Johannes Berg @ 2016-12-16 12:36 UTC (permalink / raw)
To: Arend Van Spriel; +Cc: linux-wireless
In-Reply-To: <9715e731-efcc-0252-ed7b-5ca67e2c0b5b@broadcom.com>
On Fri, 2016-12-16 at 13:17 +0100, Arend Van Spriel wrote:
>
> > I have no problem introducing a common storage for this, if
> > necessary with some fields/nl attributes being optional, but I
> > suspect this is actually a necessary part of gscan, otherwise
> > you're not able to report all the necessary data?
>
> If you just look at the gscan api in wifihal then yes. I was just
> wondering whether "all the necessary data" really comprises all these
> from a use-case perspective. As an example the api also has rtt
> fields, but both brcm and intel solutions do not report that.
Yeah, no idea. Sorry - my wording wasn't quite precise. By "this is
actually a necessary part of gscan" I meant the partial history
reporting itself, not any particular field thereof.
johannes
^ permalink raw reply
* pull-request: mac80211 2016-12-16
From: Johannes Berg @ 2016-12-16 12:39 UTC (permalink / raw)
To: David Miller; +Cc: netdev, linux-wireless
Hi Dave,
Since you seem to be updating net, I thought I'd send you a few fixes.
These aren't really all that important though, so if you want to let
them wait for a bit I can live with that.
Please pull and let me know if there's any problem.
Thanks,
johannes
The following changes since commit 8fa3b6f9392bf6d90cb7b908e07bd90166639f0a:
Merge tag 'cris-for-4.10' of git://git.kernel.org/pub/scm/linux/kernel/git/jesper/cris (2016-12-12 09:06:38 -0800)
are available in the git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/jberg/mac80211.git tags/mac80211-for-davem-2016-12-16
for you to fetch changes up to a17d93ff3a950fefaea40e4a4bf3669b9137c533:
mac80211: fix legacy and invalid rx-rate report (2016-12-15 10:54:48 +0100)
----------------------------------------------------------------
Three fixes:
* avoid a WARN_ON() when trying to use WEP with AP_VLANs
* ensure enough headroom on mesh forwarding packets
* don't report unknown/invalid rates to userspace
----------------------------------------------------------------
Ben Greear (1):
mac80211: fix legacy and invalid rx-rate report
Cedric Izoard (1):
mac80211: Ensure enough headroom when forwarding mesh pkt
Johannes Berg (1):
mac80211: don't call drv_set_default_unicast_key() for VLANs
net/mac80211/key.c | 3 ++-
net/mac80211/rx.c | 2 +-
net/mac80211/sta_info.c | 14 ++++++++------
3 files changed, 11 insertions(+), 8 deletions(-)
^ permalink raw reply
* Re: [PATCH] nl80211: better describe field in struct nl80211_bss_select_rssi_adjust
From: Johannes Berg @ 2016-12-16 12:33 UTC (permalink / raw)
To: Arend van Spriel; +Cc: linux-wireless
In-Reply-To: <1481890554-18019-1-git-send-email-arend.vanspriel@broadcom.com>
On Fri, 2016-12-16 at 12:15 +0000, Arend van Spriel wrote:
> The two fields in struct nl80211_bss_select_rssi_adjust did not state
> their type or unit. Adding documentation.
>
Applied, thanks :)
johannes
^ permalink raw reply
* Re: [PATCH] nl80211: rework {sched_,}scan event related functions
From: Johannes Berg @ 2016-12-16 12:34 UTC (permalink / raw)
To: Arend van Spriel; +Cc: linux-wireless
In-Reply-To: <1481887314-9471-1-git-send-email-arend.vanspriel@broadcom.com>
On Fri, 2016-12-16 at 11:21 +0000, Arend van Spriel wrote:
> A couple of functions used with scan events were named with
> term "send" although they were only preparing the the event
> message so renamed those.
Applied - I added a mention of nl80211_send_sched_scan_results() to the
commit log since I was confused about that at first :)
johannes
^ permalink raw reply
* Re: [RFC V3 03/11] nl80211: add support for gscan
From: Arend Van Spriel @ 2016-12-16 12:21 UTC (permalink / raw)
To: Johannes Berg; +Cc: linux-wireless
In-Reply-To: <1481883237.27953.22.camel@sipsolutions.net>
On 16-12-2016 11:13, Johannes Berg wrote:
> On Wed, 2016-12-14 at 10:01 +0100, Arend Van Spriel wrote:
>
>> Had to look for "> 16" ;-)
>
> Sorry.
>
>> Here an instance of the tab vs. space issue you mentioned. Will go
>> over the patch and fix that.
>
> There were a few, not really interesting though - git would probably
> flag it anyway, or checkpatch :)
>
>>> + if (num_chans > 16)
>>> + return -EINVAL;
>>
>> I suspect this is the restriction you were referring to.
>
> Yes.
>
>> There is no
>> reason for this although the android wifi hal has max 16 channels in
>> a bucket so I might have picked that up.
>
> I thought I saw something with a u16 bitmap that seemed related, but I
> don't see that now so I'm probably just confused.
>
>> So could a driver have a similar limit and should we add such to the
>> gscan capabilities? For instance our firmware api has a nasty
>> restriction of 64 channels for all buckets together, eg. can do 4
>> buckets of 16 channels each.
>
> We do have a limit of the maximum scan buckets, which seems to be 16
> right now. We also have a limit on the number of channels per bucket,
> which is also 16, but no combined limit afaict (so 16x16 seems fine).
>
> Maybe we do need some advertisement in that area then? Right now,
> wifihal seems to be able to read as capabilities the number of buckets
> (wifi_gscan_capabilities), but assumes the number of channels:
>
> const unsigned MAX_CHANNELS = 16;
> const unsigned MAX_BUCKETS = 16;
>
> I guess we took that and combined it, and you had more negotiation with
> Google ;-)
I was not so much involved with the initial gscan effort, but I guess
for brcm it might be true.
> We may then have to actually advertise the limit you have ("64 channels
> combined over all buckets"), unless you can get away with just
> advertising 4 buckets (and us saying 16 channels per bucket is enough?)
>
> I'm a bit tempted to make this more forward compatible though and not
> hard-limit the number of channels per bucket in the code.
Indeed so I will remove it.
Regards,
Arend
^ permalink raw reply
* Re: [RFC V3 04/11] nl80211: add driver api for gscan notifications
From: Arend Van Spriel @ 2016-12-16 12:17 UTC (permalink / raw)
To: Johannes Berg; +Cc: linux-wireless
In-Reply-To: <1481882578.27953.20.camel@sipsolutions.net>
On 16-12-2016 11:02, Johannes Berg wrote:
>
>> Not sure what is meant by "through the buckets".
>
> TBH, I was handwaving because I don't understand this part of gscan
> well :-)
>
>> Referring to your
>> remark/question in the "Unversal scan proposal" thread:
>>
>> """
>> I'm much more worried about the "bucket reporting" since that doesn't
>> fit into the current full BSS reporting model at all. What's your
>> suggestion for this?
>> """
>>
>> So this is exactly the dilemma I considered so I decided to stick
>> with the full BSS reporting model for gscan as well merely to get it
>> discussed so glad you brought it up ;-).
>
> Heh.
>
> Ok, so I missed that. Somehow I thought hidden in the buckets was a
> partial reporting :-)
>
>> The problem here is that gscan is a vehicle that serves a number of
>> use-cases. So ignoring hotlists, ePNO, etc. the gscan configuration
>> still hold several notification types:
>>
>> - report after completing M scans capturing N best APs or a
>> percentage of (M * N).
>> - report after completing a scan include a specific bucket.
>> - report full scan results.
>>
>> The first two notification trigger retrieval of gscan results which
>> are historic, ie. partial scan results for max M scans.
>>
>> As said earlier the universal scan proposal has some similarities to
>> gscan. Guess you share that as you are using the term "bucket
>> reporting" in that discussion ;-). The historic results are needed
>> for location (if I am not mistaken) so the full BSS reporting model
>> does not fit that. Question is what particular attribute in the
>> historic results is needed for location (suspecting only rssi and
>> possibly the timestamp, but would like to see that confirmed). I was
>> thinking about have historic storage in cfg80211 so we do not need a
>> per-driver solution.
>
> Ok, now I'm starting to understand this better, I guess.
>
> As far as I can tell from our code, for cached results we're reporting
> the following data:
>
> * some flags
> * a scan ID
> * and for each AP:
> * RSSI
> * timestamp
> * channel
> * BSSID
> * SSID (which internally we even have a separate table for and each
> AP just has an index to it, to save memory I guess)
> * beacon period
> * capability field
>
> No IEs and similar things like differentiating probe response/beacon,
> so we can't use the full reporting for this.
>
> I have no problem introducing a common storage for this, if necessary
> with some fields/nl attributes being optional, but I suspect this is
> actually a necessary part of gscan, otherwise you're not able to report
> all the necessary data?
If you just look at the gscan api in wifihal then yes. I was just
wondering whether "all the necessary data" really comprises all these
from a use-case perspective. As an example the api also has rtt fields,
but both brcm and intel solutions do not report that.
Regards,
Arend
^ permalink raw reply
* [PATCH] nl80211: better describe field in struct nl80211_bss_select_rssi_adjust
From: Arend van Spriel @ 2016-12-16 12:15 UTC (permalink / raw)
To: Johannes Berg; +Cc: linux-wireless, Arend van Spriel
The two fields in struct nl80211_bss_select_rssi_adjust did not state
their type or unit. Adding documentation.
Reported-by: Jouni Malinen <j@w1.fi>
Signed-off-by: Arend van Spriel <arend.vanspriel@broadcom.com>
---
include/uapi/linux/nl80211.h | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/include/uapi/linux/nl80211.h b/include/uapi/linux/nl80211.h
index 6b76e3b..d74e10b 100644
--- a/include/uapi/linux/nl80211.h
+++ b/include/uapi/linux/nl80211.h
@@ -4964,8 +4964,9 @@ enum nl80211_sched_scan_plan {
/**
* struct nl80211_bss_select_rssi_adjust - RSSI adjustment parameters.
*
- * @band: band of BSS that must match for RSSI value adjustment.
- * @delta: value used to adjust the RSSI value of matching BSS.
+ * @band: band of BSS that must match for RSSI value adjustment. The value
+ * of this field is according to &enum nl80211_band.
+ * @delta: value used to adjust the RSSI value of matching BSS in dB.
*/
struct nl80211_bss_select_rssi_adjust {
__u8 band;
--
1.9.1
^ permalink raw reply related
* [PATCH] nl80211: rework {sched_,}scan event related functions
From: Arend van Spriel @ 2016-12-16 11:21 UTC (permalink / raw)
To: Johannes Berg; +Cc: linux-wireless, Arend van Spriel
A couple of functions used with scan events were named with
term "send" although they were only preparing the the event
message so renamed those.
Signed-off-by: Arend van Spriel <arend.vanspriel@broadcom.com>
---
net/wireless/nl80211.c | 34 ++++++++--------------------------
net/wireless/nl80211.h | 6 ++----
net/wireless/scan.c | 9 +++++----
3 files changed, 15 insertions(+), 34 deletions(-)
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index 3df85a7..727ca50 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -12810,7 +12810,7 @@ static int nl80211_add_scan_req(struct sk_buff *msg,
return -ENOBUFS;
}
-static int nl80211_send_scan_msg(struct sk_buff *msg,
+static int nl80211_prep_scan_msg(struct sk_buff *msg,
struct cfg80211_registered_device *rdev,
struct wireless_dev *wdev,
u32 portid, u32 seq, int flags,
@@ -12841,7 +12841,7 @@ static int nl80211_send_scan_msg(struct sk_buff *msg,
}
static int
-nl80211_send_sched_scan_msg(struct sk_buff *msg,
+nl80211_prep_sched_scan_msg(struct sk_buff *msg,
struct cfg80211_registered_device *rdev,
struct net_device *netdev,
u32 portid, u32 seq, int flags, u32 cmd)
@@ -12873,7 +12873,7 @@ void nl80211_send_scan_start(struct cfg80211_registered_device *rdev,
if (!msg)
return;
- if (nl80211_send_scan_msg(msg, rdev, wdev, 0, 0, 0,
+ if (nl80211_prep_scan_msg(msg, rdev, wdev, 0, 0, 0,
NL80211_CMD_TRIGGER_SCAN) < 0) {
nlmsg_free(msg);
return;
@@ -12892,7 +12892,7 @@ struct sk_buff *nl80211_build_scan_msg(struct cfg80211_registered_device *rdev,
if (!msg)
return NULL;
- if (nl80211_send_scan_msg(msg, rdev, wdev, 0, 0, 0,
+ if (nl80211_prep_scan_msg(msg, rdev, wdev, 0, 0, 0,
aborted ? NL80211_CMD_SCAN_ABORTED :
NL80211_CMD_NEW_SCAN_RESULTS) < 0) {
nlmsg_free(msg);
@@ -12902,31 +12902,13 @@ struct sk_buff *nl80211_build_scan_msg(struct cfg80211_registered_device *rdev,
return msg;
}
-void nl80211_send_scan_result(struct cfg80211_registered_device *rdev,
- struct sk_buff *msg)
-{
- if (!msg)
- return;
-
- genlmsg_multicast_netns(&nl80211_fam, wiphy_net(&rdev->wiphy), msg, 0,
- NL80211_MCGRP_SCAN, GFP_KERNEL);
-}
-
-void nl80211_send_sched_scan_results(struct cfg80211_registered_device *rdev,
- struct net_device *netdev)
+/* send message created by nl80211_build_scan_msg() */
+void nl80211_send_scan_msg(struct cfg80211_registered_device *rdev,
+ struct sk_buff *msg)
{
- struct sk_buff *msg;
-
- msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
if (!msg)
return;
- if (nl80211_send_sched_scan_msg(msg, rdev, netdev, 0, 0, 0,
- NL80211_CMD_SCHED_SCAN_RESULTS) < 0) {
- nlmsg_free(msg);
- return;
- }
-
genlmsg_multicast_netns(&nl80211_fam, wiphy_net(&rdev->wiphy), msg, 0,
NL80211_MCGRP_SCAN, GFP_KERNEL);
}
@@ -12940,7 +12922,7 @@ void nl80211_send_sched_scan(struct cfg80211_registered_device *rdev,
if (!msg)
return;
- if (nl80211_send_sched_scan_msg(msg, rdev, netdev, 0, 0, 0, cmd) < 0) {
+ if (nl80211_prep_sched_scan_msg(msg, rdev, netdev, 0, 0, 0, cmd) < 0) {
nlmsg_free(msg);
return;
}
diff --git a/net/wireless/nl80211.h b/net/wireless/nl80211.h
index 7e3821d..75f8252 100644
--- a/net/wireless/nl80211.h
+++ b/net/wireless/nl80211.h
@@ -14,12 +14,10 @@ void nl80211_send_scan_start(struct cfg80211_registered_device *rdev,
struct wireless_dev *wdev);
struct sk_buff *nl80211_build_scan_msg(struct cfg80211_registered_device *rdev,
struct wireless_dev *wdev, bool aborted);
-void nl80211_send_scan_result(struct cfg80211_registered_device *rdev,
- struct sk_buff *msg);
+void nl80211_send_scan_msg(struct cfg80211_registered_device *rdev,
+ struct sk_buff *msg);
void nl80211_send_sched_scan(struct cfg80211_registered_device *rdev,
struct net_device *netdev, u32 cmd);
-void nl80211_send_sched_scan_results(struct cfg80211_registered_device *rdev,
- struct net_device *netdev);
void nl80211_common_reg_change_event(enum nl80211_commands cmd_id,
struct regulatory_request *request);
diff --git a/net/wireless/scan.c b/net/wireless/scan.c
index b5bd58d..76cdd1d 100644
--- a/net/wireless/scan.c
+++ b/net/wireless/scan.c
@@ -176,7 +176,7 @@ void ___cfg80211_scan_done(struct cfg80211_registered_device *rdev,
ASSERT_RTNL();
if (rdev->scan_msg) {
- nl80211_send_scan_result(rdev, rdev->scan_msg);
+ nl80211_send_scan_msg(rdev, rdev->scan_msg);
rdev->scan_msg = NULL;
return;
}
@@ -222,7 +222,7 @@ void ___cfg80211_scan_done(struct cfg80211_registered_device *rdev,
if (!send_message)
rdev->scan_msg = msg;
else
- nl80211_send_scan_result(rdev, msg);
+ nl80211_send_scan_msg(rdev, msg);
}
void __cfg80211_scan_done(struct work_struct *wk)
@@ -270,7 +270,8 @@ void __cfg80211_sched_scan_results(struct work_struct *wk)
spin_unlock_bh(&rdev->bss_lock);
request->scan_start = jiffies;
}
- nl80211_send_sched_scan_results(rdev, request->dev);
+ nl80211_send_sched_scan(rdev, request->dev,
+ NL80211_CMD_SCHED_SCAN_RESULTS);
}
rtnl_unlock();
@@ -1078,7 +1079,7 @@ struct cfg80211_bss *
else
rcu_assign_pointer(tmp.pub.beacon_ies, ies);
rcu_assign_pointer(tmp.pub.ies, ies);
-
+
memcpy(tmp.pub.bssid, mgmt->bssid, ETH_ALEN);
tmp.pub.channel = channel;
tmp.pub.scan_width = data->scan_width;
--
1.9.1
^ permalink raw reply related
* Re: [RFC v2 11/11] ath10k: Added sdio support
From: Valo, Kalle @ 2016-12-16 11:21 UTC (permalink / raw)
To: Erik Stromdahl; +Cc: linux-wireless@vger.kernel.org, ath10k@lists.infradead.org
In-Reply-To: <1479496971-19174-12-git-send-email-erik.stromdahl@gmail.com>
Erik Stromdahl <erik.stromdahl@gmail.com> writes:
> Initial HIF sdio/mailbox implementation.
>
> Signed-off-by: Erik Stromdahl <erik.stromdahl@gmail.com>
I know most of this coming from ath6kl but I think we should still
improve the code. Lots of comments will follow, don't get scared :)
> +#define CALC_TXRX_PADDED_LEN(ar_sdio, len) \
> + (__ALIGN_MASK((len), (ar_sdio)->mbox_info.block_mask))
I think this could be a proper static inline function. Andrew Morton
once said: "Write in C, not CPP" (or something like that), I think
that's a really good point.
> +static int ath10k_sdio_read_write_sync(struct ath10k *ar, u32 addr, u8 *=
buf,
> + u32 len, u32 request);
> +static int ath10k_sdio_hif_diag_read(struct ath10k *ar, u32 address, voi=
d *buf,
> + size_t buf_len);
> +static int ath10k_sdio_hif_diag_read32(struct ath10k *ar, u32 address,
> + u32 *value);
We prefer to avoid forward declarations if at all possible. I didn't
check but if there's a clean way to avoid these please remove them.
> +/* HIF mbox interrupt handling */
> +
> +static int ath10k_sdio_mbox_rx_process_packet(struct ath10k_sdio *ar_sdi=
o,
> + struct ath10k_sdio_rx_data *pkt,
> + u32 *lookaheads,
> + int *n_lookaheads)
> +{
So the style in ath10k is that all functions (of course this doesn't
apply to callbacks etc) have "struct ath10k *ar" as the first parameter.
This way there's no need to check if a function takes ar or ar_sdio.
> + int status =3D 0;
In ath10k we prefer to use ret. And avoid initialising it, please.
> + struct ath10k_htc *htc =3D &ar_sdio->ar->htc;
> + struct sk_buff *skb =3D pkt->skb;
> + struct ath10k_htc_hdr *htc_hdr =3D (struct ath10k_htc_hdr *)skb->data;
> + bool trailer_present =3D htc_hdr->flags & ATH10K_HTC_FLAG_TRAILER_PRESE=
NT;
> + u16 payload_len;
> +
> + payload_len =3D le16_to_cpu(htc_hdr->len);
> +
> + if (trailer_present) {
> + u8 *trailer;
> + enum ath10k_htc_ep_id eid;
> +
> + trailer =3D skb->data + sizeof(struct ath10k_htc_hdr) +
> + payload_len - htc_hdr->trailer_len;
> +
> + eid =3D (enum ath10k_htc_ep_id)htc_hdr->eid;
A some kind of mapping function for ep id would be nice, it makes it
more visible how it works.
> +static int ath10k_sdio_mbox_rx_process_packets(struct ath10k_sdio *ar_sd=
io,
> + u32 lookaheads[],
> + int *n_lookahead)
> +{
> + struct ath10k *ar =3D ar_sdio->ar;
> + struct ath10k_htc *htc =3D &ar->htc;
> + struct ath10k_sdio_rx_data *pkt;
> + int status =3D 0, i;
> +
> + for (i =3D 0; i < ar_sdio->n_rx_pkts; i++) {
> + struct ath10k_htc_ep *ep;
> + enum ath10k_htc_ep_id id;
> + u32 *lookaheads_local =3D lookaheads;
> + int *n_lookahead_local =3D n_lookahead;
> +
> + id =3D ((struct ath10k_htc_hdr *)&lookaheads[i])->eid;
> +
> + if (id >=3D ATH10K_HTC_EP_COUNT) {
> + ath10k_err(ar, "Invalid endpoint in look-ahead: %d\n",
> + id);
In ath10k we use ath10k_err() for errors from which can't survive and
ath10k_warn() for errors where we try to continue. So ath10k_warn()
would be more approriate here and most of other cases in sdio.c
> + status =3D -ENOMEM;
> + goto out;
> + }
> +
> + ep =3D &htc->endpoint[id];
> +
> + if (ep->service_id =3D=3D 0) {
> + ath10k_err(ar, "ep %d is not connected !\n", id);
> + status =3D -ENOMEM;
> + goto out;
> + }
> +
> + pkt =3D &ar_sdio->rx_pkts[i];
> +
> + if (pkt->part_of_bundle && !pkt->last_in_bundle) {
> + /* Only read lookahead's from RX trailers
> + * for the last packet in a bundle.
> + */
> + lookaheads_local =3D NULL;
> + n_lookahead_local =3D NULL;
> + }
> +
> + status =3D ath10k_sdio_mbox_rx_process_packet(ar_sdio,
> + pkt,
> + lookaheads_local,
> + n_lookahead_local);
> + if (status)
> + goto out;
> +
> + ep->ep_ops.ep_rx_complete(ar_sdio->ar, pkt->skb);
> + /* The RX complete handler now owns the skb...*/
> + pkt->skb =3D NULL;
> + pkt->alloc_len =3D 0;
> + }
> +
> +out:
> + /* Free all packets that was not passed on to the RX completion
> + * handler...
> + */
> + for (; i < ar_sdio->n_rx_pkts; i++)
> + ath10k_sdio_mbox_free_rx_pkt(&ar_sdio->rx_pkts[i]);
I got a bit fooled by not initialising i here and only then realised
why. I guess it's ok but a bit of so and so
> +
> + return status;
> +}
> +
> +static int alloc_pkt_bundle(struct ath10k *ar,
> + struct ath10k_sdio_rx_data *rx_pkts,
> + struct ath10k_htc_hdr *htc_hdr,
> + size_t full_len, size_t act_len, size_t *bndl_cnt)
> +{
> + int i, status =3D 0;
> +
> + *bndl_cnt =3D (htc_hdr->flags & ATH10K_HTC_FLAG_BUNDLE_MASK) >>
> + ATH10K_HTC_FLAG_BUNDLE_LSB;
We recently got FIELD_GET() and FIELD_PREP() to kernel for handling
bitmasks. ath10k is not yet converted (patches welcome!) but it would be
good to use those already in sdio.c. Also SM() could be replaced with
those.
> +int ath10k_sdio_mbox_rxmsg_pending_handler(struct ath10k_sdio *ar_sdio,
> + u32 msg_lookahead, bool *done)
> +{
> + struct ath10k *ar =3D ar_sdio->ar;
> + int status =3D 0;
> + u32 lookaheads[ATH10K_SDIO_MAX_RX_MSGS];
> + int n_lookaheads =3D 1;
> +
> + *done =3D true;
> +
> + /* Copy the lookahead obtained from the HTC register table into our
> + * temp array as a start value.
> + */
> + lookaheads[0] =3D msg_lookahead;
> +
> + for (;;) {
Iternal loops in kernel can be dangerous. Better to add some sort of
timeout check with a warning message, something like:
while ((time_before(jiffies, timeout)) {
}
if (timed out)
ath10k_warn("timeout in foo");
> + /* Try to allocate as many HTC RX packets indicated by
> + * n_lookaheads.
> + */
> + status =3D ath10k_sdio_mbox_rx_alloc(ar_sdio, lookaheads,
> + n_lookaheads);
> + if (status)
> + break;
> +
> + if (ar_sdio->n_rx_pkts >=3D 2)
> + /* A recv bundle was detected, force IRQ status
> + * re-check again.
> + */
> + *done =3D false;
> +
> + status =3D ath10k_sdio_mbox_rx_fetch(ar_sdio);
> +
> + /* Process fetched packets. This will potentially update
> + * n_lookaheads depending on if the packets contain lookahead
> + * reports.
> + */
> + n_lookaheads =3D 0;
> + status =3D ath10k_sdio_mbox_rx_process_packets(ar_sdio,
> + lookaheads,
> + &n_lookaheads);
> +
> + if (!n_lookaheads || status)
> + break;
> +
> + /* For SYNCH processing, if we get here, we are running
> + * through the loop again due to updated lookaheads. Set
> + * flag that we should re-check IRQ status registers again
> + * before leaving IRQ processing, this can net better
> + * performance in high throughput situations.
> + */
> + *done =3D false;
> + }
> +
> + if (status && (status !=3D -ECANCELED))
> + ath10k_err(ar, "failed to get pending recv messages: %d\n",
> + status);
> +
> + if (atomic_read(&ar_sdio->stopping)) {
> + ath10k_warn(ar, "host is going to stop. Turning of RX\n");
> + ath10k_sdio_hif_rx_control(ar_sdio, false);
> + }
I'm always skeptic when I use atomic variables used like this, I doubt
it's really correct.
> +
> + return status;
> +}
> +
> +static int ath10k_sdio_mbox_proc_dbg_intr(struct ath10k_sdio *ar_sdio)
> +{
> + int ret;
> + u32 dummy;
> + struct ath10k *ar =3D ar_sdio->ar;
> +
> + ath10k_warn(ar, "firmware crashed\n");
We have firmware crash dump support in ath10k. You could add a "TODO:"
comment about implementing that later.
> +static int ath10k_sdio_mbox_proc_err_intr(struct ath10k_sdio *ar_sdio)
> +{
> + int status;
> + u8 error_int_status;
> + u8 reg_buf[4];
> + struct ath10k_sdio_irq_data *irq_data =3D &ar_sdio->irq_data;
> + struct ath10k *ar =3D ar_sdio->ar;
> +
> + ath10k_dbg(ar, ATH10K_DBG_SDIO, "error interrupt\n");
> +
> + error_int_status =3D irq_data->irq_proc_reg.error_int_status & 0x0F;
> + if (!error_int_status) {
> + WARN_ON(1);
> + return -EIO;
> + }
> +
> + ath10k_dbg(ar, ATH10K_DBG_SDIO,
> + "valid interrupt source(s) in ERROR_INT_STATUS: 0x%x\n",
> + error_int_status);
> +
> + if (MS(error_int_status, MBOX_ERROR_INT_STATUS_WAKEUP))
> + ath10k_dbg(ar, ATH10K_DBG_SDIO, "error : wakeup\n");
> +
> + if (MS(error_int_status, MBOX_ERROR_INT_STATUS_RX_UNDERFLOW))
> + ath10k_err(ar, "rx underflow\n");
> +
> + if (MS(error_int_status, MBOX_ERROR_INT_STATUS_TX_OVERFLOW))
> + ath10k_err(ar, "tx overflow\n");
> +
> + /* Clear the interrupt */
> + irq_data->irq_proc_reg.error_int_status &=3D ~error_int_status;
> +
> + /* set W1C value to clear the interrupt, this hits the register first *=
/
> + reg_buf[0] =3D error_int_status;
> + reg_buf[1] =3D 0;
> + reg_buf[2] =3D 0;
> + reg_buf[3] =3D 0;
> +
> + status =3D ath10k_sdio_read_write_sync(ar,
> + MBOX_ERROR_INT_STATUS_ADDRESS,
> + reg_buf, 4, HIF_WR_SYNC_BYTE_FIX);
> +
> + WARN_ON(status);
This is a bit dangerous, in worst case it can spam the kernel log and
force a host reboot due watchdog timing out etc.
Better to replace with standard warning message:
ret =3D ath10k_sdio_read_write_sync(ar,
MBOX_ERROR_INT_STATUS_ADDRESS,
reg_buf, 4, HIF_WR_SYNC_BYTE_FIX);
if (ret) {
ath10k_warn("failed to read interrupr status: %d", ret);
return ret;
}
> +static int ath10k_sdio_mbox_proc_cpu_intr(struct ath10k_sdio *ar_sdio)
> +{
> + int status;
> + struct ath10k_sdio_irq_data *irq_data =3D &ar_sdio->irq_data;
> + struct ath10k *ar =3D ar_sdio->ar;
> + u8 cpu_int_status, reg_buf[4];
> +
> + cpu_int_status =3D irq_data->irq_proc_reg.cpu_int_status &
> + irq_data->irq_en_reg.cpu_int_status_en;
> + if (!cpu_int_status) {
> + WARN_ON(1);
> + return -EIO;
> + }
Ditto about WARN_ON(), ath10k_warn() is much better.
> +/* process pending interrupts synchronously */
> +static int ath10k_sdio_mbox_proc_pending_irqs(struct ath10k_sdio *ar_sdi=
o,
> + bool *done)
> +{
> + struct ath10k_sdio_irq_data *irq_data =3D &ar_sdio->irq_data;
> + struct ath10k *ar =3D ar_sdio->ar;
> + struct ath10k_sdio_irq_proc_registers *rg;
> + int status =3D 0;
> + u8 host_int_status =3D 0;
> + u32 lookahead =3D 0;
> + u8 htc_mbox =3D 1 << ATH10K_HTC_MAILBOX;
> +
> + /* NOTE: HIF implementation guarantees that the context of this
> + * call allows us to perform SYNCHRONOUS I/O, that is we can block,
> + * sleep or call any API that can block or switch thread/task
> + * contexts. This is a fully schedulable context.
> + */
> +
> + /* Process pending intr only when int_status_en is clear, it may
> + * result in unnecessary bus transaction otherwise. Target may be
> + * unresponsive at the time.
> + */
> + if (irq_data->irq_en_reg.int_status_en) {
> + /* Read the first sizeof(struct ath10k_irq_proc_registers)
> + * bytes of the HTC register table. This
> + * will yield us the value of different int status
> + * registers and the lookahead registers.
> + */
> + status =3D ath10k_sdio_read_write_sync(
> + ar,
> + MBOX_HOST_INT_STATUS_ADDRESS,
> + (u8 *)&irq_data->irq_proc_reg,
> + sizeof(irq_data->irq_proc_reg),
> + HIF_RD_SYNC_BYTE_INC);
> + if (status)
> + goto out;
> +
> + /* Update only those registers that are enabled */
> + host_int_status =3D irq_data->irq_proc_reg.host_int_status &
> + irq_data->irq_en_reg.int_status_en;
> +
> + /* Look at mbox status */
> + if (host_int_status & htc_mbox) {
> + /* Mask out pending mbox value, we use look ahead as
> + * the real flag for mbox processing.
> + */
> + host_int_status &=3D ~htc_mbox;
> + if (irq_data->irq_proc_reg.rx_lookahead_valid &
> + htc_mbox) {
> + rg =3D &irq_data->irq_proc_reg;
> + lookahead =3D le32_to_cpu(
> + rg->rx_lookahead[ATH10K_HTC_MAILBOX]);
> + if (!lookahead)
> + ath10k_err(ar, "lookahead is zero!\n");
> + }
> + }
> + }
> +
> + if (!host_int_status && !lookahead) {
> + *done =3D true;
> + goto out;
> + }
> +
> + if (lookahead) {
> + ath10k_dbg(ar, ATH10K_DBG_SDIO,
> + "pending mailbox msg, lookahead: 0x%08X\n",
> + lookahead);
> +
> + status =3D ath10k_sdio_mbox_rxmsg_pending_handler(ar_sdio,
> + lookahead,
> + done);
> + if (status)
> + goto out;
> + }
> +
> + /* now, handle the rest of the interrupts */
> + ath10k_dbg(ar, ATH10K_DBG_SDIO,
> + "valid interrupt source(s) for other interrupts: 0x%x\n",
> + host_int_status);
> +
> + if (MS(host_int_status, MBOX_HOST_INT_STATUS_CPU)) {
> + /* CPU Interrupt */
> + status =3D ath10k_sdio_mbox_proc_cpu_intr(ar_sdio);
> + if (status)
> + goto out;
> + }
> +
> + if (MS(host_int_status, MBOX_HOST_INT_STATUS_ERROR)) {
> + /* Error Interrupt */
> + status =3D ath10k_sdio_mbox_proc_err_intr(ar_sdio);
> + if (status)
> + goto out;
> + }
> +
> + if (MS(host_int_status, MBOX_HOST_INT_STATUS_COUNTER))
> + /* Counter Interrupt */
> + status =3D ath10k_sdio_mbox_proc_counter_intr(ar_sdio);
> +
> +out:
> + /* An optimization to bypass reading the IRQ status registers
> + * unecessarily which can re-wake the target, if upper layers
> + * determine that we are in a low-throughput mode, we can rely on
> + * taking another interrupt rather than re-checking the status
> + * registers which can re-wake the target.
> + *
> + * NOTE : for host interfaces that makes use of detecting pending
> + * mbox messages at hif can not use this optimization due to
> + * possible side effects, SPI requires the host to drain all
> + * messages from the mailbox before exiting the ISR routine.
> + */
> +
> + ath10k_dbg(ar, ATH10K_DBG_SDIO,
> + "%s: (done:%d, status=3D%d)\n", __func__, *done, status);
We try to follow this kind of format for debug messages:
"sdio pending irqs done %d status %d"
So start with the debug level name followed by the debug separated with spa=
ces.
And IIRC no need for "\n", the macro adds that automatically.
> +
> + return status;
> +}
> +
> +/* Macro to check if DMA buffer is WORD-aligned and DMA-able.
> + * Most host controllers assume the buffer is DMA'able and will
> + * bug-check otherwise (i.e. buffers on the stack). virt_addr_valid
> + * check fails on stack memory.
> + */
> +static inline bool buf_needs_bounce(u8 *buf)
> +{
> + return ((unsigned long)buf & 0x3) || !virt_addr_valid(buf);
> +}
IS_ALIGNED()? And this is super ugly, do we really need this? I would
much prefer that we would directly use struct sk_buff, more of that
later.
> +static inline enum ath10k_htc_ep_id pipe_id_to_eid(u8 pipe_id)
> +{
> + return (enum ath10k_htc_ep_id)pipe_id;
> +}
Oh, we already have a this kind of mapping function? Can't this be used
earlier?
> +static void ath10k_sdio_set_mbox_info(struct ath10k_sdio *ar_sdio)
> +{
> + struct ath10k_mbox_info *mbox_info =3D &ar_sdio->mbox_info;
> + u16 device =3D ar_sdio->func->device;
> +
> + mbox_info->htc_addr =3D ATH10K_HIF_MBOX_BASE_ADDR;
> + mbox_info->block_size =3D ATH10K_HIF_MBOX_BLOCK_SIZE;
> + mbox_info->block_mask =3D ATH10K_HIF_MBOX_BLOCK_SIZE - 1;
> + mbox_info->gmbox_addr =3D ATH10K_HIF_GMBOX_BASE_ADDR;
> + mbox_info->gmbox_sz =3D ATH10K_HIF_GMBOX_WIDTH;
> +
> + mbox_info->ext_info[0].htc_ext_addr =3D ATH10K_HIF_MBOX0_EXT_BASE_ADDR;
> +
> + if ((device & ATH10K_MANUFACTURER_ID_REV_MASK) < 4)
> + mbox_info->ext_info[0].htc_ext_sz =3D ATH10K_HIF_MBOX0_EXT_WIDTH;
> + else
> + /* from rome 2.0(0x504), the width has been extended
> + * to 56K
> + */
> + mbox_info->ext_info[0].htc_ext_sz =3D
> + ATH10K_HIF_MBOX0_EXT_WIDTH_ROME_2_0;
> +
> + mbox_info->ext_info[1].htc_ext_addr =3D
> + mbox_info->ext_info[0].htc_ext_addr +
> + mbox_info->ext_info[0].htc_ext_sz +
> + ATH10K_HIF_MBOX_DUMMY_SPACE_SIZE;
> + mbox_info->ext_info[1].htc_ext_sz =3D ATH10K_HIF_MBOX1_EXT_WIDTH;
> +}
> +
> +static inline void ath10k_sdio_set_cmd52_arg(u32 *arg, u8 write, u8 raw,
> + unsigned int address,
> + unsigned char val)
> +{
> + const u8 func =3D 0;
> +
> + *arg =3D ((write & 1) << 31) |
> + ((func & 0x7) << 28) |
> + ((raw & 1) << 27) |
> + (1 << 26) |
> + ((address & 0x1FFFF) << 9) |
> + (1 << 8) |
> + (val & 0xFF);
> +}
Quite ugly. FIELD_PREP() & co?
> +
> +static int ath10k_sdio_func0_cmd52_wr_byte(struct mmc_card *card,
> + unsigned int address,
> + unsigned char byte)
> +{
> + struct mmc_command io_cmd;
> +
> + memset(&io_cmd, 0, sizeof(io_cmd));
> + ath10k_sdio_set_cmd52_arg(&io_cmd.arg, 1, 0, address, byte);
> + io_cmd.opcode =3D SD_IO_RW_DIRECT;
> + io_cmd.flags =3D MMC_RSP_R5 | MMC_CMD_AC;
> +
> + return mmc_wait_for_cmd(card->host, &io_cmd, 0);
> +}
> +
> +static int ath10k_sdio_func0_cmd52_rd_byte(struct mmc_card *card,
> + unsigned int address,
> + unsigned char *byte)
> +{
> + int ret;
> + struct mmc_command io_cmd;
> +
> + memset(&io_cmd, 0, sizeof(io_cmd));
> + ath10k_sdio_set_cmd52_arg(&io_cmd.arg, 0, 0, address, 0);
> + io_cmd.opcode =3D SD_IO_RW_DIRECT;
> + io_cmd.flags =3D MMC_RSP_R5 | MMC_CMD_AC;
> +
> + ret =3D mmc_wait_for_cmd(card->host, &io_cmd, 0);
> + if (!ret)
> + *byte =3D io_cmd.resp[0];
> +
> + return ret;
> +}
> +
> +static int ath10k_sdio_io(struct ath10k_sdio *ar_sdio, u32 request, u32 =
addr,
> + u8 *buf, u32 len)
> +{
> + int ret =3D 0;
Avoid these kind of unnecessary initialisations.
> + struct sdio_func *func =3D ar_sdio->func;
> + struct ath10k *ar =3D ar_sdio->ar;
> +
> + sdio_claim_host(func);
> +
> + if (request & HIF_WRITE) {
> + if (request & HIF_FIXED_ADDRESS)
> + ret =3D sdio_writesb(func, addr, buf, len);
> + else
> + ret =3D sdio_memcpy_toio(func, addr, buf, len);
> + } else {
> + if (request & HIF_FIXED_ADDRESS)
> + ret =3D sdio_readsb(func, buf, addr, len);
> + else
> + ret =3D sdio_memcpy_fromio(func, buf, addr, len);
> + }
> +
> + sdio_release_host(func);
> +
> + ath10k_dbg(ar, ATH10K_DBG_SDIO, "%s addr 0x%x%s buf 0x%p len %d\n",
> + request & HIF_WRITE ? "wr" : "rd", addr,
> + request & HIF_FIXED_ADDRESS ? " (fixed)" : "", buf, len);
> + ath10k_dbg_dump(ar, ATH10K_DBG_SDIO_DUMP, NULL,
> + request & HIF_WRITE ? "sdio wr " : "sdio rd ",
> + buf, len);
> +
> + return ret;
> +}
> +
> +static struct ath10k_sdio_bus_request
> +*ath10k_sdio_alloc_busreq(struct ath10k_sdio *ar_sdio)
> +{
> + struct ath10k_sdio_bus_request *bus_req;
> +
> + spin_lock_bh(&ar_sdio->lock);
> +
> + if (list_empty(&ar_sdio->bus_req_freeq)) {
> + spin_unlock_bh(&ar_sdio->lock);
> + return NULL;
> + }
> +
> + bus_req =3D list_first_entry(&ar_sdio->bus_req_freeq,
> + struct ath10k_sdio_bus_request, list);
> + list_del(&bus_req->list);
> +
> + spin_unlock_bh(&ar_sdio->lock);
> +
> + return bus_req;
> +}
> +
> +static void ath10k_sdio_free_bus_req(struct ath10k_sdio *ar_sdio,
> + struct ath10k_sdio_bus_request *bus_req)
> +{
> + spin_lock_bh(&ar_sdio->lock);
> + list_add_tail(&bus_req->list, &ar_sdio->bus_req_freeq);
> + spin_unlock_bh(&ar_sdio->lock);
> +}
> +
> +static int ath10k_sdio_read_write_sync(struct ath10k *ar, u32 addr, u8 *=
buf,
> + u32 len, u32 request)
> +{
> + struct ath10k_sdio *ar_sdio =3D ath10k_sdio_priv(ar);
> + u8 *tbuf =3D NULL;
Extra space after u8?
> + int ret;
> + bool bounced =3D false;
> +
> + if (request & HIF_BLOCK_BASIS)
> + len =3D round_down(len, ar_sdio->mbox_info.block_size);
> +
> + if (buf_needs_bounce(buf)) {
> + if (!ar_sdio->dma_buffer)
> + return -ENOMEM;
> + /* FIXME: I am not sure if it is always correct to assume
> + * that the SDIO irq is a "fake" irq and sleep is possible.
> + * (this function will get called from
> + * ath10k_sdio_irq_handler
> + */
> + mutex_lock(&ar_sdio->dma_buffer_mutex);
> + tbuf =3D ar_sdio->dma_buffer;
> +
> + if (request & HIF_WRITE)
> + memcpy(tbuf, buf, len);
> +
> + bounced =3D true;
> + } else {
> + tbuf =3D buf;
> + }
So I really hate that ar_sdio->dma_buffer, do we really need it? What if
we just get rid of it and allocate struct sk_buff and pass that around?
No need to do extra copying then, I hope :)
> +
> + ret =3D ath10k_sdio_io(ar_sdio, request, addr, tbuf, len);
> + if ((request & HIF_READ) && bounced)
> + memcpy(buf, tbuf, len);
> +
> + if (bounced)
> + mutex_unlock(&ar_sdio->dma_buffer_mutex);
> +
> + return ret;
> +}
> +
> +static void __ath10k_sdio_write_async(struct ath10k_sdio *ar_sdio,
> + struct ath10k_sdio_bus_request *req)
> +{
> + int status;
> + struct ath10k_htc_ep *ep;
> + struct sk_buff *skb;
> +
> + skb =3D req->skb;
> + status =3D ath10k_sdio_read_write_sync(ar_sdio->ar, req->address,
> + skb->data, req->len,
> + req->request);
> + ep =3D &ar_sdio->ar->htc.endpoint[req->eid];
> + ath10k_htc_notify_tx_completion(ep, skb);
> + ath10k_sdio_free_bus_req(ar_sdio, req);
> +}
> +
> +static void ath10k_sdio_write_async_work(struct work_struct *work)
> +{
> + struct ath10k_sdio *ar_sdio;
> + struct ath10k_sdio_bus_request *req, *tmp_req;
> +
> + ar_sdio =3D container_of(work, struct ath10k_sdio, wr_async_work);
> +
> + spin_lock_bh(&ar_sdio->wr_async_lock);
> + list_for_each_entry_safe(req, tmp_req, &ar_sdio->wr_asyncq, list) {
> + list_del(&req->list);
> + spin_unlock_bh(&ar_sdio->wr_async_lock);
> + __ath10k_sdio_write_async(ar_sdio, req);
> + spin_lock_bh(&ar_sdio->wr_async_lock);
> + }
> + spin_unlock_bh(&ar_sdio->wr_async_lock);
> +}
> +
> +static void ath10k_sdio_irq_handler(struct sdio_func *func)
> +{
> + int status =3D 0;
> + unsigned long timeout;
> + struct ath10k_sdio *ar_sdio;
> + bool done =3D false;
> +
> + ar_sdio =3D sdio_get_drvdata(func);
> + atomic_set(&ar_sdio->irq_handling, 1);
> +
> + /* Release the host during interrupts so we can pick it back up when
> + * we process commands.
> + */
> + sdio_release_host(ar_sdio->func);
> +
> + timeout =3D jiffies + ATH10K_SDIO_HIF_COMMUNICATION_TIMEOUT_HZ;
> + while (time_before(jiffies, timeout) && !done) {
> + status =3D ath10k_sdio_mbox_proc_pending_irqs(ar_sdio, &done);
> + if (status)
> + break;
> + }
> +
> + sdio_claim_host(ar_sdio->func);
> +
> + atomic_set(&ar_sdio->irq_handling, 0);
> + wake_up(&ar_sdio->irq_wq);
> +
> + WARN_ON(status && status !=3D -ECANCELED);
> +}
Questionable use of an atomic variable again, looks like badly implement
poor man's locking to me. And instead of wake_up() we should workqueues
or threaded irqs (if sdio supports that).
> +
> +static int ath10k_sdio_hif_disable_intrs(struct ath10k_sdio *ar_sdio)
> +{
> + int ret;
> + struct ath10k_sdio_irq_enable_reg regs;
> + struct ath10k_sdio_irq_data *irq_data =3D &ar_sdio->irq_data;
> +
> + memset(®s, 0, sizeof(regs));
> +
> + ret =3D ath10k_sdio_read_write_sync(ar_sdio->ar,
> + MBOX_INT_STATUS_ENABLE_ADDRESS,
> + ®s.int_status_en, sizeof(regs),
> + HIF_WR_SYNC_BYTE_INC);
> + if (ret) {
> + ath10k_err(ar_sdio->ar, "Unable to disable sdio interrupts\n");
> + return ret;
> + }
> +
> + spin_lock_bh(&irq_data->lock);
> + irq_data->irq_en_reg =3D regs;
> + spin_unlock_bh(&irq_data->lock);
> +
> + return 0;
> +}
> +
> +static int ath10k_sdio_hif_power_up(struct ath10k *ar)
> +{
> + struct ath10k_sdio *ar_sdio =3D ath10k_sdio_priv(ar);
> + struct sdio_func *func =3D ar_sdio->func;
> + int ret =3D 0;
> +
> + if (!ar_sdio->is_disabled)
> + return 0;
> +
> + ath10k_dbg(ar, ATH10K_DBG_BOOT, "sdio power on\n");
> +
> + sdio_claim_host(func);
> +
> + ret =3D sdio_enable_func(func);
> + if (ret) {
> + ath10k_err(ar, "Unable to enable sdio func: %d)\n", ret);
> + sdio_release_host(func);
> + return ret;
> + }
> +
> + sdio_release_host(func);
> +
> + /* Wait for hardware to initialise. It should take a lot less than
> + * 20 ms but let's be conservative here.
> + */
> + msleep(20);
> +
> + ar_sdio->is_disabled =3D false;
> +
> + ret =3D ath10k_sdio_hif_disable_intrs(ar_sdio);
> +
> + return ret;
> +}
> +
> +static void ath10k_sdio_hif_power_down(struct ath10k *ar)
> +{
> + struct ath10k_sdio *ar_sdio =3D ath10k_sdio_priv(ar);
> + int ret;
> +
> + if (ar_sdio->is_disabled)
> + return;
> +
> + ath10k_dbg(ar, ATH10K_DBG_BOOT, "sdio power off\n");
> +
> + /* Disable the card */
> + sdio_claim_host(ar_sdio->func);
> + ret =3D sdio_disable_func(ar_sdio->func);
> + sdio_release_host(ar_sdio->func);
> +
> + if (ret)
> + ath10k_dbg(ar, ATH10K_DBG_BOOT,
> + "Unable to disable sdio: %d\n", ret);
Shouldn't this be ath10k_warn()?
> +
> + ar_sdio->is_disabled =3D true;
> +}
> +
> +int ath10k_sdio_hif_tx_sg(struct ath10k *ar, u8 pipe_id,
> + struct ath10k_hif_sg_item *items, int n_items)
> +{
> + int i;
> + u32 address;
> + struct ath10k_sdio *ar_sdio =3D ath10k_sdio_priv(ar);
> + struct ath10k_sdio_bus_request *bus_req;
> +
> + bus_req =3D ath10k_sdio_alloc_busreq(ar_sdio);
> +
> + if (WARN_ON_ONCE(!bus_req))
> + return -ENOMEM;
Is ath10k_warn() more approriate?
> + for (i =3D 0; i < n_items; i++) {
> + bus_req->skb =3D items[i].transfer_context;
> + bus_req->request =3D HIF_WRITE;
> + bus_req->eid =3D pipe_id_to_eid(pipe_id);
> + /* Write TX data to the end of the mbox address space */
> + address =3D ar_sdio->mbox_addr[bus_req->eid] +
> + ar_sdio->mbox_size[bus_req->eid] - bus_req->skb->len;
> + bus_req->address =3D address;
> + bus_req->len =3D CALC_TXRX_PADDED_LEN(ar_sdio, bus_req->skb->len);
> +
> + spin_lock_bh(&ar_sdio->wr_async_lock);
> + list_add_tail(&bus_req->list, &ar_sdio->wr_asyncq);
> + spin_unlock_bh(&ar_sdio->wr_async_lock);
> + }
> +
> + queue_work(ar_sdio->workqueue, &ar_sdio->wr_async_work);
> +
> + return 0;
> +}
> +
> +static int ath10k_sdio_hif_enable_intrs(struct ath10k_sdio *ar_sdio)
> +{
> + struct ath10k_sdio_irq_enable_reg regs;
> + int status;
> + struct ath10k_sdio_irq_data *irq_data =3D &ar_sdio->irq_data;
> +
> + memset(®s, 0, sizeof(regs));
> +
> + /* Enable all but CPU interrupts */
> + regs.int_status_en =3D SM(0x01, MBOX_INT_STATUS_ENABLE_ERROR) |
> + SM(0x01, MBOX_INT_STATUS_ENABLE_CPU) |
> + SM(0x01, MBOX_INT_STATUS_ENABLE_COUNTER);
> +
> + /* NOTE: There are some cases where HIF can do detection of
> + * pending mbox messages which is disabled now.
> + */
> + regs.int_status_en |=3D SM(0x01, MBOX_INT_STATUS_ENABLE_MBOX_DATA);
> +
> + /* Set up the CPU Interrupt status Register */
> + regs.cpu_int_status_en =3D 0;
> +
> + /* Set up the Error Interrupt status Register */
> + regs.err_int_status_en =3D
> + SM(0x01, MBOX_ERROR_STATUS_ENABLE_RX_UNDERFLOW) |
> + SM(0x01, MBOX_ERROR_STATUS_ENABLE_TX_OVERFLOW);
> +
> + /* Enable Counter interrupt status register to get fatal errors for
> + * debugging.
> + */
> + regs.cntr_int_status_en =3D SM(ATH10K_SDIO_TARGET_DEBUG_INTR_MASK,
> + MBOX_COUNTER_INT_STATUS_ENABLE_BIT);
> +
> + status =3D ath10k_sdio_read_write_sync(ar_sdio->ar,
> + MBOX_INT_STATUS_ENABLE_ADDRESS,
> + ®s.int_status_en, sizeof(regs),
> + HIF_WR_SYNC_BYTE_INC);
> + if (status) {
> + ath10k_err(ar_sdio->ar,
> + "failed to update interrupt ctl reg err: %d\n",
> + status);
> + return status;
> + }
> +
> + spin_lock_bh(&irq_data->lock);
> + irq_data->irq_en_reg =3D regs;
> + spin_unlock_bh(&irq_data->lock);
> +
> + return 0;
> +}
> +
> +static int ath10k_sdio_hif_start(struct ath10k *ar)
> +{
> + int ret;
> + struct ath10k_sdio *ar_sdio =3D ath10k_sdio_priv(ar);
> + u32 addr, val;
> +
> + addr =3D host_interest_item_address(HI_ITEM(hi_acs_flags));
> +
> + ret =3D ath10k_sdio_hif_diag_read32(ar, addr, &val);
> + if (ret) {
> + ath10k_err(ar, "Unable to read diag mem: %d\n", ret);
> + goto out;
> + }
> +
> + if (val & HI_ACS_FLAGS_SDIO_SWAP_MAILBOX_FW_ACK) {
> + ath10k_dbg(ar, ATH10K_DBG_SDIO,
> + "Mailbox SWAP Service is enabled\n");
> + ar_sdio->swap_mbox =3D true;
> + }
> +
> + /* eid 0 always uses the lower part of the extended mailbox address
> + * space (ext_info[0].htc_ext_addr).
> + */
> + ar_sdio->mbox_addr[0] =3D ar_sdio->mbox_info.ext_info[0].htc_ext_addr;
> + ar_sdio->mbox_size[0] =3D ar_sdio->mbox_info.ext_info[0].htc_ext_sz;
> +
> + sdio_claim_host(ar_sdio->func);
> +
> + /* Register the isr */
> + ret =3D sdio_claim_irq(ar_sdio->func, ath10k_sdio_irq_handler);
> + if (ret) {
> + ath10k_err(ar, "Failed to claim sdio irq: %d\n", ret);
> + sdio_release_host(ar_sdio->func);
> + goto out;
> + }
> +
> + sdio_release_host(ar_sdio->func);
> +
> + ret =3D ath10k_sdio_hif_enable_intrs(ar_sdio);
> + if (ret)
> + ath10k_err(ar, "Failed to enable sdio interrupts: %d\n", ret);
> +
> +out:
> + return ret;
> +}
> +
> +static bool ath10k_sdio_is_on_irq(struct ath10k *ar)
> +{
> + struct ath10k_sdio *ar_sdio =3D ath10k_sdio_priv(ar);
> +
> + return !atomic_read(&ar_sdio->irq_handling);
> +}
Yikes.
> +
> +static void ath10k_sdio_irq_disable(struct ath10k *ar)
> +{
> + struct ath10k_sdio *ar_sdio =3D ath10k_sdio_priv(ar);
> + int ret;
> +
> + sdio_claim_host(ar_sdio->func);
> +
> + atomic_set(&ar_sdio->stopping, 1);
> +
> + if (atomic_read(&ar_sdio->irq_handling)) {
> + sdio_release_host(ar_sdio->func);
> +
> + ret =3D wait_event_interruptible(ar_sdio->irq_wq,
> + ath10k_sdio_is_on_irq(ar));
> + if (ret)
> + return;
> +
> + sdio_claim_host(ar_sdio->func);
> + }
There has to be a better way to implement this, now it feels that it's
just reimplementing the wheel. We should have proper way to wait for
interrupt handlers and workqueues to end etc.
> + ret =3D sdio_release_irq(ar_sdio->func);
> + if (ret)
> + ath10k_err(ar, "Failed to release sdio irq: %d\n", ret);
> +
> + sdio_release_host(ar_sdio->func);
> +}
> +
> +static int ath10k_sdio_config(struct ath10k_sdio *ar_sdio)
> +{
> + struct ath10k *ar =3D ar_sdio->ar;
> + struct sdio_func *func =3D ar_sdio->func;
> + unsigned char byte, asyncintdelay =3D 2;
> + int ret;
> +
> + ath10k_dbg(ar, ATH10K_DBG_BOOT, "SDIO configuration\n");
> +
> + sdio_claim_host(func);
> +
> + byte =3D 0;
> + ret =3D ath10k_sdio_func0_cmd52_rd_byte(func->card,
> + SDIO_CCCR_DRIVE_STRENGTH,
> + &byte);
> +
> + byte =3D (byte & (~(SDIO_DRIVE_DTSx_MASK << SDIO_DRIVE_DTSx_SHIFT))) |
> + SDIO_DTSx_SET_TYPE_D;
FIELD_PREP(). There are lots of places where that an be used.
> +static void ath10k_sdio_write32(struct ath10k *ar, u32 offset, u32 value=
)
> +{
> +}
> +
> +static u32 ath10k_sdio_read32(struct ath10k *ar, u32 offset)
> +{
> + return 0;
> +}
Somekind of FIXME/TODO comments for write32() and read32()? What
functionality are we going to miss when we don't implement these?
> +static void ath10k_sdio_hif_send_complete_check(struct ath10k *ar,
> + u8 pipe, int force)
> +{
> +}
> +
> +static u16 ath10k_sdio_hif_get_free_queue_number(struct ath10k *ar, u8 p=
ipe)
> +{
> + return 0;
> +}
Similar comments here also.
> +
> +static const struct ath10k_hif_ops ath10k_sdio_hif_ops =3D {
> + .tx_sg =3D ath10k_sdio_hif_tx_sg,
> + .diag_read =3D ath10k_sdio_hif_diag_read,
> + .diag_write =3D ath10k_sdio_diag_write_mem,
> + .exchange_bmi_msg =3D ath10k_sdio_hif_exchange_bmi_msg,
> + .start =3D ath10k_sdio_hif_start,
> + .stop =3D ath10k_sdio_hif_stop,
> + .map_service_to_pipe =3D ath10k_sdio_hif_map_service_to_pipe,
> + .get_default_pipe =3D ath10k_sdio_hif_get_default_pipe,
> + .send_complete_check =3D ath10k_sdio_hif_send_complete_check,
> + .get_free_queue_number =3D ath10k_sdio_hif_get_free_queue_number,
> + .power_up =3D ath10k_sdio_hif_power_up,
> + .power_down =3D ath10k_sdio_hif_power_down,
> + .read32 =3D ath10k_sdio_read32,
> + .write32 =3D ath10k_sdio_write32,
> +#ifdef CONFIG_PM
> + .suspend =3D ath10k_sdio_hif_suspend,
> + .resume =3D ath10k_sdio_hif_resume,
> +#endif
> + .fetch_cal_eeprom =3D ath10k_sdio_hif_fetch_cal_eeprom,
> +};
> +
> +#ifdef CONFIG_PM_SLEEP
> +
> +/* Empty handlers so that mmc subsystem doesn't remove us entirely durin=
g
> + * suspend. We instead follow cfg80211 suspend/resume handlers.
> + */
> +static int ath10k_sdio_pm_suspend(struct device *device)
> +{
> + return 0;
> +}
> +
> +static int ath10k_sdio_pm_resume(struct device *device)
> +{
> + return 0;
> +}
> +
> +static SIMPLE_DEV_PM_OPS(ath10k_sdio_pm_ops, ath10k_sdio_pm_suspend,
> + ath10k_sdio_pm_resume);
> +
> +#define ATH10K_SDIO_PM_OPS (&ath10k_sdio_pm_ops)
> +
> +#else
> +
> +#define ATH10K_SDIO_PM_OPS NULL
> +
> +#endif /* CONFIG_PM_SLEEP */
> +
> +static int ath10k_sdio_probe(struct sdio_func *func,
> + const struct sdio_device_id *id)
> +{
> + int ret;
> + struct ath10k_sdio *ar_sdio;
> + struct ath10k *ar;
> + int i;
> + enum ath10k_hw_rev hw_rev;
> +
> + hw_rev =3D ATH10K_HW_QCA6174;
This needs a comment, at least to remind if ever add something else than
QCA6174 based SDIO design.
> +
> + ar =3D ath10k_core_create(sizeof(*ar_sdio), &func->dev, ATH10K_BUS_SDIO=
,
> + hw_rev, &ath10k_sdio_hif_ops);
> + if (!ar) {
> + dev_err(&func->dev, "failed to allocate core\n");
> + return -ENOMEM;
> + }
> +
> + ath10k_dbg(ar, ATH10K_DBG_BOOT,
> + "sdio new func %d vendor 0x%x device 0x%x block 0x%x/0x%x\n",
> + func->num, func->vendor, func->device,
> + func->max_blksize, func->cur_blksize);
> +
> + ar_sdio =3D ath10k_sdio_priv(ar);
> +
> + ar_sdio->dma_buffer =3D kzalloc(ATH10K_HIF_DMA_BUFFER_SIZE, GFP_KERNEL)=
;
> + if (!ar_sdio->dma_buffer) {
> + ret =3D -ENOMEM;
> + goto err_core_destroy;
> + }
> +
> + ar_sdio->func =3D func;
> + sdio_set_drvdata(func, ar_sdio);
> +
> + ar_sdio->is_disabled =3D true;
> + ar_sdio->ar =3D ar;
> +
> + spin_lock_init(&ar_sdio->lock);
> + spin_lock_init(&ar_sdio->wr_async_lock);
> + spin_lock_init(&ar_sdio->irq_data.lock);
> + mutex_init(&ar_sdio->dma_buffer_mutex);
> +
> + INIT_LIST_HEAD(&ar_sdio->bus_req_freeq);
> + INIT_LIST_HEAD(&ar_sdio->wr_asyncq);
> +
> + INIT_WORK(&ar_sdio->wr_async_work, ath10k_sdio_write_async_work);
> + ar_sdio->workqueue =3D create_singlethread_workqueue("ath10k_sdio_wq");
> + if (!ar_sdio->workqueue)
> + goto err;
> +
> + init_waitqueue_head(&ar_sdio->irq_wq);
> +
> + for (i =3D 0; i < ATH10K_SDIO_BUS_REQUEST_MAX_NUM; i++)
> + ath10k_sdio_free_bus_req(ar_sdio, &ar_sdio->bus_req[i]);
> +
> + ar->dev_id =3D id->device;
> + ar->id.vendor =3D id->vendor;
> + ar->id.device =3D id->device;
> +
> + ath10k_sdio_set_mbox_info(ar_sdio);
> +
> + ret =3D ath10k_sdio_config(ar_sdio);
> + if (ret) {
> + ath10k_err(ar, "Failed to config sdio: %d\n", ret);
> + goto err;
> + }
> +
> + ret =3D ath10k_core_register(ar, 0/*chip_id is not applicaple for SDIO*=
/);
> + if (ret) {
> + ath10k_err(ar, "failed to register driver core: %d\n", ret);
> + goto err;
> + }
I would assume that chip_id is applicaple also with SDIO, there has to
be a register where to get it. Also this kind of comment style is
preferred:
/* TODO: don't know yet how to get chip_id with SDIO */
chip_id =3D 0;
ret =3D ath10k_core_register(ar, chip_id);
> +
> + return ret;
> +
> +err:
> + kfree(ar_sdio->dma_buffer);
> +err_core_destroy:
> + ath10k_core_destroy(ar);
> +
> + return ret;
> +}
> +
> +static void ath10k_sdio_remove(struct sdio_func *func)
> +{
> + struct ath10k_sdio *ar_sdio;
> + struct ath10k *ar;
> +
> + ar_sdio =3D sdio_get_drvdata(func);
> + ar =3D ar_sdio->ar;
> +
> + ath10k_dbg(ar, ATH10K_DBG_BOOT,
> + "sdio removed func %d vendor 0x%x device 0x%x\n",
> + func->num, func->vendor, func->device);
> +
> + (void)ath10k_sdio_hif_disable_intrs(ar_sdio);
> + cancel_work_sync(&ar_sdio->wr_async_work);
> + ath10k_core_unregister(ar);
> + ath10k_core_destroy(ar);
> +
> + kfree(ar_sdio->dma_buffer);
> +}
> +
> +static const struct sdio_device_id ath10k_sdio_devices[] =3D {
> + {SDIO_DEVICE(ATH10K_MANUFACTURER_CODE,
> + (ATH10K_MANUFACTURER_ID_AR6005_BASE | 0xA))},
> + {},
> +};
I suspect there's a more sensible way to create the device table than
this, just no time to check it now. Anyone know?
The naming "ath10k manufacturer" is also wrong, it should be QCA or
Qualcomm.
--=20
Kalle Valo=
^ permalink raw reply
* Re: wl1251 NVS calibration data format
From: Pali Rohár @ 2016-12-16 11:12 UTC (permalink / raw)
To: Gery Kahn, Shahar Lev, Kalle Valo, linux-wireless, linux-kernel,
Sebastian Reichel, Pavel Machek, Ivaylo Dimitrov
In-Reply-To: <201612161201.48356@pali>
[-- Attachment #1: Type: Text/Plain, Size: 1133 bytes --]
Resending email to new Gery's address...
On Friday 16 December 2016 12:01:48 Pali Rohár wrote:
> Hi! Do you know format of wl1251 NVS calibration data file?
>
> I found that there is tool for changing NVS file for wl1271 and newer
> chips (so not for wl1251!) at: https://github.com/gxk/ti-utils
>
> And wl1271 has in NVS data already place for MAC address. And in wlcore
> (for wl1271 and newer) there is really kernel code which is doing
> something with MAC address in NVS, see:
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/net/wireless/ti/wlcore/boot.c#n352
Also, there is parsing MAC address from NVS wl1271 data:
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/net/wireless/ti/wlcore/main.c#n6009
> So... I would like to know if in wl1251 NVS calibration file is also
> some place for MAC address or not.
>
> Default wl1251 NVS calibration file is available in linux-firmware:
> https://git.kernel.org/cgit/linux/kernel/git/firmware/linux-firmware.git/tree/ti-connectivity/wl1251-nvs.bin
--
Pali Rohár
pali.rohar@gmail.com
[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]
^ permalink raw reply
* wl1251 NVS calibration data format
From: Pali Rohár @ 2016-12-16 11:01 UTC (permalink / raw)
To: Gery Kahn, Shahar Lev, Kalle Valo, linux-wireless, linux-kernel,
Sebastian Reichel, Pavel Machek, Ivaylo Dimitrov
[-- Attachment #1: Type: Text/Plain, Size: 831 bytes --]
Hi! Do you know format of wl1251 NVS calibration data file?
I found that there is tool for changing NVS file for wl1271 and newer
chips (so not for wl1251!) at: https://github.com/gxk/ti-utils
And wl1271 has in NVS data already place for MAC address. And in wlcore
(for wl1271 and newer) there is really kernel code which is doing
something with MAC address in NVS, see:
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/net/wireless/ti/wlcore/boot.c#n352
So... I would like to know if in wl1251 NVS calibration file is also
some place for MAC address or not.
Default wl1251 NVS calibration file is available in linux-firmware:
https://git.kernel.org/cgit/linux/kernel/git/firmware/linux-firmware.git/tree/ti-connectivity/wl1251-nvs.bin
--
Pali Rohár
pali.rohar@gmail.com
[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]
^ permalink raw reply
* Re: wl1251 & mac address & calibration data
From: Pali Rohár @ 2016-12-16 10:40 UTC (permalink / raw)
To: Daniel Wagner
Cc: Luis R. Rodriguez, Arend Van Spriel, Tom Gundersen, Johannes Berg,
Ming Lei, Mimi Zohar, Bjorn Andersson, Rafał Miłecki,
Kalle Valo, Sebastian Reichel, Pavel Machek, Michal Kazior,
Ivaylo Dimitrov, Aaro Koskinen, Tony Lindgren, linux-wireless,
Network Development, linux-kernel@vger.kernel.org,
David Woodhouse, Takashi Iwai, Josh Boyer, Dmitry Torokhov
In-Reply-To: <07be7fc8-6c7f-6e03-379d-fa781b248259@monom.org>
[-- Attachment #1: Type: Text/Plain, Size: 2061 bytes --]
On Friday 16 December 2016 08:25:44 Daniel Wagner wrote:
> On 12/16/2016 03:03 AM, Luis R. Rodriguez wrote:
> > For the new API a solution for "fallback mechanisms" should be
> > clean though and I am looking to stay as far as possible from the
> > existing mess. A solution to help both the old API and new API is
> > possible for the "fallback mechanism" though -- but for that I can
> > only refer you at this point to some of Daniel Wagner and Tom
> > Gunderson's firmwared deamon prospect. It should help pave the way
> > for a clean solution and help address other stupid issues.
>
> The firmwared project is hosted here
>
> https://github.com/teg/firmwared
>
> As Luis pointed out, firmwared relies on
> FW_LOADER_USER_HELPER_FALLBACK, which is not enabled by default.
I know. But it does not mean that I cannot enable this option at kernel
compile time.
Bigger problem is that currently request_firmware() first try to load
firmware directly from VFS and after that (if fails) fallback to user
helper.
So I would need to extend kernel firmware code with new function (or
flag) to not use VFS and try only user mode helper.
> I
> don't see any reason why firmwared should not also support loading
> calibration data. If we find a sound way to do this.
It can, but why should I use another daemon for firmware loading as
non-systemd version of udev (and eudev fork) support firmware loading?
I think I stay with udev/eudev.
> As you can see from the commit history it is a pretty young project
> and more ore less reanimation of the old udev firmware loader
> feature. We are getting int into shape, adding integration tests
> etc.
>
> The main motivation for this project is the get movement back in
> stuck discussion on the firmware loader API. Luis was very busy
> writing up all the details on the current situation and purely from
> the amount of documentation need to describe the API you can tell
> something is awry.
>
> Thanks,
> Daniel
--
Pali Rohár
pali.rohar@gmail.com
[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]
^ permalink raw reply
* Re: [RFC V3 04/11] nl80211: add driver api for gscan notifications
From: Johannes Berg @ 2016-12-16 10:02 UTC (permalink / raw)
To: Arend Van Spriel; +Cc: linux-wireless
In-Reply-To: <69ec7e9f-cd46-c46d-140c-0b30343cc0f7@broadcom.com>
> Not sure what is meant by "through the buckets".
TBH, I was handwaving because I don't understand this part of gscan
well :-)
> Referring to your
> remark/question in the "Unversal scan proposal" thread:
>
> """
> I'm much more worried about the "bucket reporting" since that doesn't
> fit into the current full BSS reporting model at all. What's your
> suggestion for this?
> """
>
> So this is exactly the dilemma I considered so I decided to stick
> with the full BSS reporting model for gscan as well merely to get it
> discussed so glad you brought it up ;-).
Heh.
Ok, so I missed that. Somehow I thought hidden in the buckets was a
partial reporting :-)
> The problem here is that gscan is a vehicle that serves a number of
> use-cases. So ignoring hotlists, ePNO, etc. the gscan configuration
> still hold several notification types:
>
> - report after completing M scans capturing N best APs or a
> percentage of (M * N).
> - report after completing a scan include a specific bucket.
> - report full scan results.
>
> The first two notification trigger retrieval of gscan results which
> are historic, ie. partial scan results for max M scans.
>
> As said earlier the universal scan proposal has some similarities to
> gscan. Guess you share that as you are using the term "bucket
> reporting" in that discussion ;-). The historic results are needed
> for location (if I am not mistaken) so the full BSS reporting model
> does not fit that. Question is what particular attribute in the
> historic results is needed for location (suspecting only rssi and
> possibly the timestamp, but would like to see that confirmed). I was
> thinking about have historic storage in cfg80211 so we do not need a
> per-driver solution.
Ok, now I'm starting to understand this better, I guess.
As far as I can tell from our code, for cached results we're reporting
the following data:
* some flags
* a scan ID
* and for each AP:
* RSSI
* timestamp
* channel
* BSSID
* SSID (which internally we even have a separate table for and each
AP just has an index to it, to save memory I guess)
* beacon period
* capability field
No IEs and similar things like differentiating probe response/beacon,
so we can't use the full reporting for this.
I have no problem introducing a common storage for this, if necessary
with some fields/nl attributes being optional, but I suspect this is
actually a necessary part of gscan, otherwise you're not able to report
all the necessary data?
johannes
^ permalink raw reply
* Re: wl1251 & mac address & calibration data
From: Pali Rohár @ 2016-12-16 10:35 UTC (permalink / raw)
To: Luis R. Rodriguez
Cc: Arend Van Spriel, Tom Gundersen, Daniel Wagner, Johannes Berg,
Ming Lei, Mimi Zohar, Bjorn Andersson, Rafał Miłecki,
Kalle Valo, Sebastian Reichel, Pavel Machek, Michal Kazior,
Ivaylo Dimitrov, Aaro Koskinen, Tony Lindgren, linux-wireless,
Network Development, linux-kernel@vger.kernel.org,
David Woodhouse, Takashi Iwai, Josh Boyer, Dmitry Torokhov
In-Reply-To: <CAB=NE6VHF+U7DzapENvLMKtN4Dgi9J0qMoeUJ8o4vSwc2-rRFQ@mail.gmail.com>
[-- Attachment #1: Type: Text/Plain, Size: 8476 bytes --]
On Friday 16 December 2016 03:03:19 Luis R. Rodriguez wrote:
> On Thu, Dec 15, 2016 at 2:12 PM, Arend Van Spriel
>
> <arend.vanspriel@broadcom.com> wrote:
> > On 15-12-2016 16:33, Pali Rohár wrote:
> >> On Thu Dec 15 09:18:44 2016 Kalle Valo <kvalo@codeaurora.org>
> >> wrote:
> >>> (Adding Luis because he has been working on request_firmware()
> >>> lately)
> >>>
> >>> Pali Rohár <pali.rohar@gmail.com> writes:
> >>>>>> So no, there is no argument against... request_firmware() in
> >>>>>> fallback mode with userspace helper is by design blocking and
> >>>>>> waiting for userspace. But waiting for some change in DTS in
> >>>>>> kernel is just nonsense.
> >>>>>
> >>>>> I would just mark the wlan device with status = "disabled" and
> >>>>> enable it in the overlay together with adding the NVS & MAC
> >>>>> info.
> >>>>
> >>>> So if you think that this solution make sense, we can wait what
> >>>> net wireless maintainers say about it...
> >>>>
> >>>> For me it looks like that solution can be:
> >>>>
> >>>> extending request_firmware() to use only userspace helper
> >>>
> >>> I haven't followed the discussion very closely but this is my
> >>> preference what drivers should do:
> >>>
> >>> 1) First the driver should do try to get the calibration data and
> >>> mac
> >>>
> >>> address from the device tree.
> >>
> >> Ok, but there is no (dynamic, device specific) data in DTS for
> >> N900. So 1) is noop.
> >
> > Uhm. What do you mean? You can propose a patch to the DT bindings
> > [1] to get it in there and create your N900 DTB or am I missing
> > something here. Are there hardware restrictions that do not allow
> > you to boot with your own DTB.
> >
> >>> 2) If they are not in DT the driver should retrieve the
> >>> calibration data
> >>>
> >>> with request_firmware(). BUT with an option for user space
> >>> to implement that with a helper script so that the data
> >>> can be created dynamically, which I believe openwrt does
> >>> with ath10k calibration data right now.
> >>
> >> Currently there is flag for request_firmware() that it should
> >> fallback to user helper if direct VFS access not find needed
> >> firmware.
> >>
> >> But this flag is not suitable as /lib/firmware already provides
> >> default (not device specific) calibration data.
> >>
> >> So I would suggest to add another flag/function which will primary
> >> use user helper.
> >
> > I recall Luis saying that user-mode helper (fallback) should be
> > discouraged, because there is no assurance that there is a
> > user-mode helper so you might just be pissing in the wind.
>
> There's tons of issues with the current status quo of the so called
> "firmware usermode helper". To start off with its a complete
> misnomer, the kernel's usermode helper infrastructure is implemented
> in lib/kmod.c and it provides an API to help call out to userspace
> some helper for you. That's the real kernel usermode helper. In so
> far as the firmware_class.c driver is concerned -- it only makes use
> of the kernel user mode helper as an option if you have
> CONFIG_UEVENT_HELPER_PATH set to help address kobject uevents. Most
> distributions do not use this, and back in the day systemd udev, and
> prior to that hotplug used to process firmware kobject uevents.
> systemd udev is long gone. Gone. This kobject uevents really are a
> "fallback mechanism" for firmware only -- if cached firmware,
> built-in firmware, or direct filesystem lookup fails. These each are
> their own beast. Finally kobject uevents are only one of the
> fallback
> mechanisms, "custom fallback mechanisms" are the other option and its
> what you and others seem to describe to want for these sorts of
> custom things.
>
> There are issues with the existing request_firmware() API in that
> everyone and their mother keeps extending it with stupid small API
> extensions to do yet-another-tweak, and then having to go and change
> tons of drivers. Or a new API call added for just one custom knob.
> Naturally this is all plain dumb, so yet-another-API call or new
> argument is not going to help us. We don't have "flags" persi in this
> API, that was one issue with the design of the API, but there are
> much more. The entire change in mechanism of "firmware fallback
> mechanism" depending on which kernel config options you have is
> another. Its a big web of gum put together. All this needs to end.
>
> I've recently proposed a new patch to first help with understanding
> each of the individual items I've listed above with as much new
> information as is possible. I myself need to re-read it to just keep
> tabs on what the hell is going on at times. After this I have a new
> API proposal which I've been working on for a long time now which
> adds an extensible API with only 2 calls: sync, async, and lets us
> modify attributes through a parameters struct. This is what we
> should use in the future for further extensions.
>
> For the new API a solution for "fallback mechanisms" should be clean
> though and I am looking to stay as far as possible from the existing
> mess. A solution to help both the old API and new API is possible for
> the "fallback mechanism" though -- but for that I can only refer you
> at this point to some of Daniel Wagner and Tom Gunderson's firmwared
> deamon prospect. It should help pave the way for a clean solution and
> help address other stupid issues.
>
> I'll note -- the "custom fallback mechanism", part of the old API is
> just a terrible idea for many reasons. I've documented issues in my
> documentation revamp, I suggest to read that, refer to this thread:
>
> https://lkml.kernel.org/r/20161213030828.17820-4-mcgrof@kernel.org
>
> > The idea was to have a
> > dedicated API call that explicitly does the request towards
> > user-space.
>
> In so far as this driver example that was mentioned in this thread my
> recommendation is to use the default existing MAC address /
> calibration data on /lib/firmware/ and then use another request to
> help override for the custom thing. The only issue of course is that
> the timeout for the custom thing is 60 seconds, but if your platforms
> are controlled -- just reduce this to 1 second or so given that udev
> is long gone and it would seem you'd only use a custom fw request to
> depend on. You could also wrap this thing into a kconfig option for
> now, and have the filename specified, if empty then no custom request
> is sent. If set then you have it request it.
>
> Please note the other patches in my series for the custom fallback
> though. We have only 2 users upstream -- and from recent discussions
> it would seem it might be possible to address what you need with
> firmwared in a clean way without this custom old fallback crap thing.
> Johannes at least has a similar requirement and is convinced a
> "custom" thing can be done without even adding an extra custom
> kobject uevent attribute as from what I recall he hinted, drivers
> have no business to be involved in this. If you do end up using the
> custom fallback mechanism be sure to add super crystal clear
> documentation for it (see my other patches for the declarer for this
> documentation). Since we have only 2 drivers using this custom thing
> its hard to get folks to commit to writing the docs, write it now
> and be sure you think of the future as well.
>
> Oh also the "custom firmware fallback" was also broken on recent
> kernels for many kernel revisions, it just did not work until
> recently a fix which went in, so your users wills need this fix
> cherry picked. Hey I tell you, the custom fw thing was a terrible
> incarnation. Only 2 drivers use it. There are good reasons to not
> like it (be sure to read the suspend issue I documented).
>
> > By the way, are we talking here about wl1251 device or driver as
> > you also mentioned wl12xx? I did not read the entire thread.
>
> Luis
Hi Luis! wl1251 already uses request_firmware for loading calibration
data from VFS. And because /lib/firmware/ contains preinstalled default
calibration data it uses it -- which is wrong as wireless has bad
quality.
In my setup I'm going to use udev with firmware loading support
(probably fork eudev), so it should be systemd-independent.
--
Pali Rohár
pali.rohar@gmail.com
[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]
^ permalink raw reply
* Re: wl1251 & mac address & calibration data
From: Pali Rohár @ 2016-12-16 10:26 UTC (permalink / raw)
To: Arend Van Spriel
Cc: Kalle Valo, Sebastian Reichel, Pavel Machek, Michal Kazior,
Ivaylo Dimitrov, Aaro Koskinen, Tony Lindgren, linux-wireless,
Network Development, linux-kernel, Luis R. Rodriguez
In-Reply-To: <76365770-f5ba-9565-3fca-710518f64d81@broadcom.com>
[-- Attachment #1: Type: Text/Plain, Size: 3509 bytes --]
On Thursday 15 December 2016 21:12:47 Arend Van Spriel wrote:
> On 15-12-2016 16:33, Pali Rohár wrote:
> > On Thu Dec 15 09:18:44 2016 Kalle Valo <kvalo@codeaurora.org> wrote:
> >> (Adding Luis because he has been working on request_firmware()
> >> lately)
> >>
> >> Pali Rohár <pali.rohar@gmail.com> writes:
> >>>>> So no, there is no argument against... request_firmware() in
> >>>>> fallback mode with userspace helper is by design blocking and
> >>>>> waiting for userspace. But waiting for some change in DTS in
> >>>>> kernel is just nonsense.
> >>>>
> >>>> I would just mark the wlan device with status = "disabled" and
> >>>> enable it in the overlay together with adding the NVS & MAC
> >>>> info.
> >>>
> >>> So if you think that this solution make sense, we can wait what
> >>> net wireless maintainers say about it...
> >>>
> >>> For me it looks like that solution can be:
> >>>
> >>> extending request_firmware() to use only userspace helper
> >>
> >> I haven't followed the discussion very closely but this is my
> >> preference what drivers should do:
> >>
> >> 1) First the driver should do try to get the calibration data and
> >> mac
> >>
> >> address from the device tree.
> >
> > Ok, but there is no (dynamic, device specific) data in DTS for
> > N900. So 1) is noop.
>
> Uhm. What do you mean? You can propose a patch to the DT bindings [1]
> to get it in there and create your N900 DTB or am I missing
> something here. Are there hardware restrictions that do not allow
> you to boot with your own DTB.
What is [1]?
N900's bootloader does not support DTB and it does not pass any DTB. It
boots kernel in ATAGs mode. What we are doing is appending DTB compiled
from kernel sources to end of zImage.
But that appended DTB cannot contains device specific nodes (e.g.
calibration data for device) as zImage is there for any N900 device, not
just only one.
> >> 2) If they are not in DT the driver should retrieve the
> >> calibration data
> >>
> >> with request_firmware(). BUT with an option for user space
> >> to implement that with a helper script so that the data
> >> can be created dynamically, which I believe openwrt does
> >> with ath10k calibration data right now.
> >
> > Currently there is flag for request_firmware() that it should
> > fallback to user helper if direct VFS access not find needed
> > firmware.
> >
> > But this flag is not suitable as /lib/firmware already provides
> > default (not device specific) calibration data.
> >
> > So I would suggest to add another flag/function which will primary
> > use user helper.
>
> I recall Luis saying that user-mode helper (fallback) should be
> discouraged, because there is no assurance that there is a user-mode
> helper so you might just be pissing in the wind. The idea was to have
> a dedicated API call that explicitly does the request towards
> user-space.
>
> By the way, are we talking here about wl1251 device or driver as you
> also mentioned wl12xx? I did not read the entire thread.
Yes, we are talking about wl1251 device, which is in Nokia N900 and
wl1251.ko and wl1251_spi.ko drivers.
I mentioned wl12xx as it already uses similar approach with mac address
via request_firmware(). And as those drivers are very similar plus from
one manufactor and in same kernel folder I mentioned similar API for
consistency...
--
Pali Rohár
pali.rohar@gmail.com
[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]
^ 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