Hello Jarkko, On 28.02.22 13:14, Jarkko Sakkinen wrote: > On Wed, 2022-02-23 at 17:20 +0100, Ahmad Fatoum wrote: >> On 23.02.22 16:41, Jarkko Sakkinen wrote: >>> On Tue, Feb 22, 2022 at 08:58:18PM +0100, Ahmad Fatoum wrote: >>>> + /* header + (key mod immediate) + 2x pointers + op */ >>>> + len = 4 + (4 + ALIGN(keymod_len, 4)) + 2*(4 + 4 + CAAM_PTR_SZ_MAX) + 4; >>> >>> Nit: the amount of magic numbers is overwhelming here. I neither understand >>> the statement nor how that comment should help me to understand it. >> >> header = 4 >> (key mod immediate) = (4 + ALIGN(keymod_len, 4)) >> 2x pointer = 2 * (4 + 4 + CAAM_PTR_SZ_MAX) >> op = 4 > > Please create a struct with the associated fields instead and then > it is just sizeof that. The CAAM descriptor construction code writes the current length to the first word and then macros are used to append fields to the descriptor while incrementing the length. The key modifier is in the middle of the descriptor, so this isn't representable as a C struct without using VLAs, which are banned in the kernel. Even if it worked, it's arguably not good style to define a struct just to compute the size and then cast it to a u32 * to use the same macros all other CAAM code uses. I thought this over a bit and figured that we don't really need to compute the length dynamically as it's at most 44 or 52 bytes (depending on DMA address width) anyway, so we can just use a fixed value. I still keep the addition, because I believe there is a value in seeing how we arrive at the 44 or 52 bytes instead of just hardcoding it. Just sent out v6 with these changes. Let me know what you think. Cheers, Ahmad -- Pengutronix e.K. | | Steuerwalder Str. 21 | http://www.pengutronix.de/ | 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
