lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Fri, 22 Mar 2019 02:47:59 +0100 From: Tim Schumacher <timschumi@....de> To: Kalle Valo <kvalo@...eaurora.org> Cc: QCA ath9k Development <ath9k-devel@....qualcomm.com>, "David S. Miller" <davem@...emloft.net>, linux-wireless@...r.kernel.org, netdev@...r.kernel.org, linux-kernel@...r.kernel.org Subject: Re: [PATCH] ath9k: Check for errors when reading SREV register On 21.03.19 11:02, Kalle Valo wrote: > Tim Schumacher <timschumi@....de> writes: > >> Right now, if an error is encountered during the SREV register >> read (i.e. an EIO in ath9k_regread()), that error code gets >> passed all the way to __ath9k_hw_init(), where it is visible >> during the "Chip rev not supported" message. >> >> ath9k_htc 1-1.4:1.0: ath9k_htc: HTC initialized with 33 credits >> ath: phy2: Mac Chip Rev 0x0f.3 is not supported by this driver >> ath: phy2: Unable to initialize hardware; initialization status: -95 >> ath: phy2: Unable to initialize hardware; initialization status: -95 >> ath9k_htc: Failed to initialize the device >> >> Check for -EIO explicitly in ath9k_hw_read_revisions() and return >> a boolean based on the success of the operation. Check for that in >> __ath9k_hw_init() and abort with a more debugging-friendly message >> if reading the revisions wasn't successful. >> >> ath9k_htc 1-1.4:1.0: ath9k_htc: HTC initialized with 33 credits >> ath: phy2: Failed to read SREV register >> ath: phy2: Could not read hardware revision >> ath: phy2: Unable to initialize hardware; initialization status: -95 >> ath: phy2: Unable to initialize hardware; initialization status: -95 >> ath9k_htc: Failed to initialize the device >> >> This helps when debugging by directly showing the first point of >> failure and it could prevent possible errors if a 0x0f.3 revision >> is ever supported. >> >> Signed-off-by: Tim Schumacher <timschumi@....de> > > [...] > >> - val = REG_READ(ah, AR_SREV) & AR_SREV_ID; >> + srev = REG_READ(ah, AR_SREV); >> + >> + if (srev == -EIO) { >> + ath_err(ath9k_hw_common(ah), >> + "Failed to read SREV register"); >> + return false; >> + } > > I really don't like how the error handling is implemented in REG_READ(). > If the register has value 0xfffffffb (= -EIO ==-5) won't this interpret > that as an error? > If the register had that value, it would indeed report an error. However (at least if I read the current code and the data sheet correctly), to make use of the higher 24 bits of the register, the "small"/old version of the SREV_ID (i.e. the rightmost 8 bit) need to be set to 0xFF. Therefore, a register read of 0xfffffffb should never happen in this register. But the error handling is indeed a bit weird. Making the return value a pure status indicator and saving the value from the register by passing a reference would probably be the best solution to fixing this up.
Powered by blists - more mailing lists