Re: 160 mhz crash

[Sven Eckelmann <sven.eckelmann@openmesh.com>]

[-- Attachment #1.1: Type: text/plain, Size: 7541 bytes --]

On Donnerstag, 28. Juni 2018 11:56:39 CEST Sebastian Gottschall wrote:
> btw. openwrt imported my patches recently, but with some openwrt 
> specific modifications

Thanks, just tested these [1]. It doesn't crash anymore and at least the basic 
functionality seems to work.

On Donnerstag, 28. Juni 2018 11:56:02 CEST Sebastian Gottschall wrote:
> i understand that. but i dont see a problem with the code style 

Your commit message contains things which don't belong there (the changelog of 
the different version) and you don't follow the rules about line length in the 
commit message or subject. Also checkpatch.pl panics when seeing your 
changes [2,3]:

    ------------------------------------------------------------------------------------------
    ath10k-160mhz/v1-ath10k-fix-band_center_freq-handling-for-VHT160-in-recent-firmwares.patch
    ------------------------------------------------------------------------------------------
    WARNING: Possible unwrapped commit description (prefer a maximum 75 chars per line)
    #17: 
    starting with firmware 10.4.3.4.x series QCA changed the handling of the channel property band_center_freq1 and band_center_freq2 in vht160 operation mode
    
    WARNING: 'compatiblity' may be misspelled - perhaps 'compatibility'?
    #18: 
    likelly for backward compatiblity with vht80 only capable clients.
    
    WARNING: line over 80 characters
    #62: FILE: drivers/net/wireless/ath/ath10k/wmi.c:1681:
    +                       ch->band_center_freq1 = __cpu_to_le32(arg->band_center_freq1 - 40);
    
    ERROR: trailing whitespace
    #64: FILE: drivers/net/wireless/ath/ath10k/wmi.c:1683:
    +^I^I^Ich->band_center_freq1 = __cpu_to_le32(arg->band_center_freq1 + 40);^I^I$
    
    WARNING: line over 80 characters
    #64: FILE: drivers/net/wireless/ath/ath10k/wmi.c:1683:
    +                       ch->band_center_freq1 = __cpu_to_le32(arg->band_center_freq1 + 40);
    
    ERROR: Missing Signed-off-by: line(s)
    
    total: 2 errors, 4 warnings, 0 checks, 34 lines checked
    
    NOTE: For some of the reported defects, checkpatch may be able to
          mechanically convert to the typical style using --fix or --fix-inplace.
    
    NOTE: Whitespace errors detected.
          You may wish to use scripts/cleanpatch or scripts/cleanfile
    
    ath10k-160mhz/v1-ath10k-fix-band_center_freq-handling-for-VHT160-in-recent-firmwares.patch has style problems, please review.
    ----------------------------------------------------------------------------------------------------------------------------
    ath10k-160mhz/v7-ath10k-fix-crash-in-recent-3.5.3-9984-firmware-due-wrong-handling-of-peer_bw_rxnss_override-parameter.patch
    ----------------------------------------------------------------------------------------------------------------------------
    WARNING: Possible unwrapped commit description (prefer a maximum 75 chars per line)
    #17: 
    current handling of peer_bw_rxnss_override parameter is based on guessing the VHT160/8080 capability by rx rate. this is wrong and may lead
    
    WARNING: line over 80 characters
    #73: FILE: drivers/net/wireless/ath/ath10k/mac.c:2534:
    +       /* only 4x4 configuration do support 2x2 for VHT160, everything else must use 1x1 */
    
    WARNING: line over 80 characters
    #75: FILE: drivers/net/wireless/ath/ath10k/mac.c:2536:
    +               nss160 = arg->peer_num_spatial_streams <= 2 ? arg->peer_num_spatial_streams : 2;
    
    WARNING: line over 80 characters
    #77: FILE: drivers/net/wireless/ath/ath10k/mac.c:2538:
    +       /* in case if peer is connected with vht160 or vht80+80, we need to properly adjust rxnss parameters otherwise firmware will raise a assert */
    
    ERROR: space required before the open parenthesis '('
    #78: FILE: drivers/net/wireless/ath/ath10k/mac.c:2539:
    +       switch(arg->peer_phymode) {
    
    WARNING: line over 80 characters
    #90: FILE: drivers/net/wireless/ath/ath10k/mac.c:2551:
    +                  sta->addr, arg->peer_max_mpdu, arg->peer_flags, arg->peer_bw_rxnss_override);
    
    WARNING: line over 80 characters
    #119: FILE: drivers/net/wireless/ath/ath10k/wmi.c:7214:
    +       cmd->peer_bw_rxnss_override = __cpu_to_le32(arg->peer_bw_rxnss_override);
    
    CHECK: Prefer using the BIT macro
    #132: FILE: drivers/net/wireless/ath/ath10k/wmi.h:6383:
    +#define BW_NSS_FWCONF_MAP_ENABLE             (1 << 31)
    
    WARNING: line over 80 characters
    #139: FILE: drivers/net/wireless/ath/ath10k/wmi.h:6390:
    +#define GET_BW_NSS_FWCONF_160(x)             ((((x) & BW_NSS_FWCONF_MAP_160MHZ_M) >> BW_NSS_FWCONF_MAP_160MHZ_S) + 1)
    
    WARNING: line over 80 characters
    #140: FILE: drivers/net/wireless/ath/ath10k/wmi.h:6391:
    +#define GET_BW_NSS_FWCONF_80_80(x)           ((((x) & BW_NSS_FWCONF_MAP_80_80MHZ_M) >> BW_NSS_FWCONF_MAP_80_80MHZ_S) + 1)
    
    WARNING: line over 80 characters
    #143: FILE: drivers/net/wireless/ath/ath10k/wmi.h:6394:
    +#define BW_NSS_FWCONF_160(x)          (BW_NSS_FWCONF_MAP_ENABLE | (((x - 1) << BW_NSS_FWCONF_MAP_160MHZ_S) & BW_NSS_FWCONF_MAP_160MHZ_M))
    
    CHECK: Macro argument 'x' may be better as '(x)' to avoid precedence issues
    #143: FILE: drivers/net/wireless/ath/ath10k/wmi.h:6394:
    +#define BW_NSS_FWCONF_160(x)          (BW_NSS_FWCONF_MAP_ENABLE | (((x - 1) << BW_NSS_FWCONF_MAP_160MHZ_S) & BW_NSS_FWCONF_MAP_160MHZ_M))
    
    WARNING: line over 80 characters
    #144: FILE: drivers/net/wireless/ath/ath10k/wmi.h:6395:
    +#define BW_NSS_FWCONF_80_80(x)        (BW_NSS_FWCONF_MAP_ENABLE | (((x - 1) << BW_NSS_FWCONF_MAP_80_80MHZ_S) & BW_NSS_FWCONF_MAP_80_80MHZ_M))
    
    CHECK: Macro argument 'x' may be better as '(x)' to avoid precedence issues
    #144: FILE: drivers/net/wireless/ath/ath10k/wmi.h:6395:
    +#define BW_NSS_FWCONF_80_80(x)        (BW_NSS_FWCONF_MAP_ENABLE | (((x - 1) << BW_NSS_FWCONF_MAP_80_80MHZ_S) & BW_NSS_FWCONF_MAP_80_80MHZ_M))
    
    total: 1 errors, 10 warnings, 3 checks, 94 lines checked
    
    NOTE: For some of the reported defects, checkpatch may be able to
          mechanically convert to the typical style using --fix or --fix-inplace.
    
    ath10k-160mhz/v7-ath10k-fix-crash-in-recent-3.5.3-9984-firmware-due-wrong-handling-of-peer_bw_rxnss_override-parameter.patch has style problems, please review.
    
    NOTE: If any of the errors are false positives, please report
          them to the maintainer, see CHECKPATCH in MAINTAINERS.

There are also general coding style problems [4]. Look for example at the way 
you format a switch:

    +       /* in case if peer is connected with vht160 or vht80+80, we need to properly adjust rxnss parameters otherwise firmware will raise a assert */
    +       switch(arg->peer_phymode) {
    +       case MODE_11AC_VHT80_80:
    +               arg->peer_bw_rxnss_override = BW_NSS_FWCONF_80_80(nss160);
    +       /* fall through */
    +       case MODE_11AC_VHT160:
    +               arg->peer_bw_rxnss_override |= BW_NSS_FWCONF_160(nss160);
    +       break;
    +       default:
    +       break;
            }

Everything after (and including) the first break seems to be misaligned.


I hope it helps to improve the patches.


Kind regards,
	Sven

[1] https://github.com/openwrt/openwrt/commit/134e832814f1986c7ee06ac00806ebb6e762fd15
[2] https://patchwork.kernel.org/patch/10365117/
[3] https://patchwork.kernel.org/patch/10372949/
[4] https://www.kernel.org/doc/html/v4.10/process/coding-style.html#indentation

[-- Attachment #1.2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 146 bytes --]

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k