| Message ID | 20170328095814.3734615-2-arnd@arndb.de (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Herbert Xu |
| Headers | show |
Ack. Didn't reply all.... Sorry, Arnd. There was a krobot warning about this and I submitted a patch just now. (I thought) my mistake was (in this function) not handling the structure elements in the same manner as other functions. My patch rectifies that. On 03/28/2017 04:58 AM, Arnd Bergmann wrote: > The driver causes a warning when built as big-endian: > > drivers/crypto/ccp/ccp-dev-v5.c: In function 'ccp5_perform_des3': > include/uapi/linux/byteorder/big_endian.h:32:26: error: large integer > implicitly truncated to unsigned type [-Werror=overflow] > #define __cpu_to_le32(x) ((__force __le32)__swab32((x))) > ^ > include/linux/byteorder/generic.h:87:21: note: in expansion of macro > '__cpu_to_le32' > #define cpu_to_le32 __cpu_to_le32 > ^~~~~~~~~~~~~ > drivers/crypto/ccp/ccp-dev-v5.c:436:28: note: in expansion of macro > 'cpu_to_le32' > CCP5_CMD_KEY_MEM(&desc) = cpu_to_le32(CCP_MEMTYPE_SB); > > The warning is correct, doing a 32-bit byte swap on a value that gets > assigned into a bit field cannot work, since we would only write zeroes > in this case, regardless of the input. Yes, this was all wrong. > In fact, the use of bit fields in hardware defined data structures is > not portable to start with, so until all these bit fields get replaced > by something else, the driver cannot work on big-endian machines, and > I'm adding an annotation here to prevent it from being selected. This is a driver that talks to hardware, a device which, AFAIK, has no plan to be implemented in a big endian flavor. I clearly need to be more diligent in building with various checkers enabled. I'd prefer my fix over your suggested refusal to compile, if that's okay. > The CCPv3 code seems to not suffer from this problem, only v5 uses > bitfields. Yes, I took a different approach when I wrote the code. IMO (arguably) more readable. Same result: words full of hardware-dependent bit patterns. Please help me understand what I could do better.
On Tue, Mar 28, 2017 at 4:08 PM, Gary R Hook <ghook@amd.com> wrote: >> In fact, the use of bit fields in hardware defined data structures is >> not portable to start with, so until all these bit fields get replaced >> by something else, the driver cannot work on big-endian machines, and >> I'm adding an annotation here to prevent it from being selected. > > > This is a driver that talks to hardware, a device which, AFAIK, has no > plan to be implemented in a big endian flavor. I clearly need to be more > diligent in building with various checkers enabled. I'd prefer my fix > over your suggested refusal to compile, if that's okay. It's hard to predict the future. If this device ever makes it into an ARM based chip, the chances are relatively high that someone will eventually run a big-endian kernel on it. As long as it's guaranteed to be x86-only, the risk of anyone running into the bug is close to zero, but we normally still try to write device drivers in portable C code to prevent it from getting copied incorrectly into another driver. >> The CCPv3 code seems to not suffer from this problem, only v5 uses >> bitfields. > > > Yes, I took a different approach when I wrote the code. IMO (arguably) > more readable. Same result: words full of hardware-dependent bit patterns. > > Please help me understand what I could do better. The rule for portable drivers is that you must not use bitfields in structures that can be accessed by the hardware. I think you can do this in a more readable way by removing the CCP5_CMD_* macros etc completely and just accessing the members of the structure as __le32 words. The main advantage for readability here is that you can grep for the struct members and see where they are used without following the macros. If it helps, you can also encapsulate the generation of the word inside of an inline function, like: static inline __le32 ccp5_cmd_dw0(bool soc, bool ioc, bool init, bool eom, u32 engine) { u32 dw0 = (soc ? CCP5_WORD0_SOC : 0) | (ioc ? CCP5_WORD0_IOC : 0) | (init ? CCP5_WORD0_INIT : 0) | (eom ? CCP5_WORD0_EOM : 0) | CCP5_WORD0_ENGINE(engine); return __cpu_to_le32(dw0); } ... desc->dw0 = ccp5_cmd_dw0(op->soc, 0, op->init, op->oem, op->engine); Arnd
On 03/28/2017 09:59 AM, Arnd Bergmann wrote: > On Tue, Mar 28, 2017 at 4:08 PM, Gary R Hook <ghook@amd.com> wrote: > >>> In fact, the use of bit fields in hardware defined data structures is >>> not portable to start with, so until all these bit fields get replaced >>> by something else, the driver cannot work on big-endian machines, and >>> I'm adding an annotation here to prevent it from being selected. >> >> >> This is a driver that talks to hardware, a device which, AFAIK, has no >> plan to be implemented in a big endian flavor. I clearly need to be more >> diligent in building with various checkers enabled. I'd prefer my fix >> over your suggested refusal to compile, if that's okay. > > It's hard to predict the future. If this device ever makes it into an > ARM based chip, the chances are relatively high that someone > will eventually run a big-endian kernel on it. As long as it's guaranteed > to be x86-only, the risk of anyone running into the bug is close to > zero, but we normally still try to write device drivers in portable C > code to prevent it from getting copied incorrectly into another driver. Understood, and I had surmised as such. Totally agree. >>> The CCPv3 code seems to not suffer from this problem, only v5 uses >>> bitfields. >> >> >> Yes, I took a different approach when I wrote the code. IMO (arguably) >> more readable. Same result: words full of hardware-dependent bit patterns. >> >> Please help me understand what I could do better. > > The rule for portable drivers is that you must not use bitfields in > structures > that can be accessed by the hardware. I think you can do this in a more > readable way by removing the CCP5_CMD_* macros etc completely > and just accessing the members of the structure as __le32 words. > The main advantage for readability here is that you can grep for the > struct members and see where they are used without following the > macros. If it helps, you can also encapsulate the generation of the > word inside of an inline function, like: > Please see my follow-on patch.
diff --git a/drivers/crypto/ccp/Kconfig b/drivers/crypto/ccp/Kconfig index 2238f77aa248..07af9ece84f9 100644 --- a/drivers/crypto/ccp/Kconfig +++ b/drivers/crypto/ccp/Kconfig @@ -1,6 +1,7 @@ config CRYPTO_DEV_CCP_DD tristate "Cryptographic Coprocessor device driver" depends on CRYPTO_DEV_CCP + depends on !CPU_BIG_ENDIAN || BROKEN default m select HW_RANDOM select DMA_ENGINE
The driver causes a warning when built as big-endian: drivers/crypto/ccp/ccp-dev-v5.c: In function 'ccp5_perform_des3': include/uapi/linux/byteorder/big_endian.h:32:26: error: large integer implicitly truncated to unsigned type [-Werror=overflow] #define __cpu_to_le32(x) ((__force __le32)__swab32((x))) ^ include/linux/byteorder/generic.h:87:21: note: in expansion of macro '__cpu_to_le32' #define cpu_to_le32 __cpu_to_le32 ^~~~~~~~~~~~~ drivers/crypto/ccp/ccp-dev-v5.c:436:28: note: in expansion of macro 'cpu_to_le32' CCP5_CMD_KEY_MEM(&desc) = cpu_to_le32(CCP_MEMTYPE_SB); The warning is correct, doing a 32-bit byte swap on a value that gets assigned into a bit field cannot work, since we would only write zeroes in this case, regardless of the input. In fact, the use of bit fields in hardware defined data structures is not portable to start with, so until all these bit fields get replaced by something else, the driver cannot work on big-endian machines, and I'm adding an annotation here to prevent it from being selected. The CCPv3 code seems to not suffer from this problem, only v5 uses bitfields. Fixes: 4b394a232df7 ("crypto: ccp - Let a v5 CCP provide the same function as v3") Signed-off-by: Arnd Bergmann <arnd@arndb.de> --- drivers/crypto/ccp/Kconfig | 1 + 1 file changed, 1 insertion(+)