Re: [EXT] [PATCH v6 3/4] crypto: caam - add in-kernel interface for blob generator

[Ahmad Fatoum <a.fatoum@pengutronix.de>]

Hello Pankaj,

On 22.03.22 08:32, Ahmad Fatoum wrote:
> Hello Pankaj,
> 
> On 22.03.22 07:25, Pankaj Gupta wrote:
>> Hi Ahmad,
>>
>> Suggested to define macro with more details.
>> Please find comments in-line.
>>
> 
>> len = 4 + (4 + ALIGN(keymod_len, 4)) + 2*(4 + 4 + 
>>>>>> + CAAM_PTR_SZ_MAX) + 4;
>>
>>> +/* header + (key mod immediate) + 2x seq_intlen pointers + op */
>>> +#define CAAM_BLOB_DESC_BYTES_MAX \
>>> +       (CAAM_CMD_SZ + \
>>> +        CAAM_CMD_SZ + CAAM_BLOB_KEYMOD_LENGTH + \
>>> +        2 * (CAAM_CMD_SZ + CAAM_PTR_SZ_MAX) + \
>>> +        CAAM_CMD_SZ)
>>> +
>>
>> Suggested to replace the above macro like below:
>>
>> +#define CAAM_BLOB_DESC_BYTES_MAX \			
>> +       (CAAM_CMD_SZ + \					/* Command to initialize & stating length of  descriptor */
>> +        CAAM_CMD_SZ + CAAM_BLOB_KEYMOD_LENGTH + \	/* Command to append the key-modifier + followed by the key-modifier data */
>> +        (CAAM_CMD_SZ + CAAM_PTR_SZ_MAX) + \		/* Command to include input plain key and pointer to the input key */
>> +        (CAAM_CMD_SZ + CAAM_PTR_SZ_MAX) + \		/* Command to include output-key blob and pointer to the output-key blob */
>> +        CAAM_CMD_SZ)						/* Command describing the Operation to perform */
> 
> 
> Sure thing, will do for v7. Otherwise, if all looks good to you,
> can I have your Reviewed-by?
This doesn't compile as-is and it leads to quite long lines.
The description isn't accurate also, because what's plain and what's blob
changes depending on whether we encapsulate or decapsulate.

Here's my revised macro version:

#define CAAM_BLOB_DESC_BYTES_MAX                                        \
        /* Command to initialize & stating length of descriptor */      \
        (CAAM_CMD_SZ +                                                  \
        /* Command to append the key-modifier + key-modifier data */    \
         CAAM_CMD_SZ + CAAM_BLOB_KEYMOD_LENGTH +                        \
        /* Command to include input key + pointer to the input key */   \
         CAAM_CMD_SZ + CAAM_PTR_SZ_MAX +                                \
        /* Command to include output key + pointer to the output key */ \
         CAAM_CMD_SZ + CAAM_PTR_SZ_MAX +                                \
        /* Command describing the Operation to perform */               \
         CAAM_CMD_SZ)

Alternatively, I can change it back into a function:

static inline u32 *caam_blob_desc_alloc(void)
{
        size_t size = 0;

        /* Command to initialize & stating length of descriptor */
        size += CAAM_CMD_SZ;
        /* Command to append the key-modifier + key-modifier data */
        size += CAAM_CMD_SZ + CAAM_BLOB_KEYMOD_LENGTH;
        /* Command to include input plain key + pointer to the input key */
        size += CAAM_CMD_SZ + CAAM_PTR_SZ_MAX;
        /* Command to include output-key blob + pointer to the output key */
        size += CAAM_CMD_SZ + CAAM_PTR_SZ_MAX;
        /* Command describing the Operation to perform */
        size += CAAM_CMD_SZ;

        return kzalloc(size, GFP_KERNEL | GFP_DMA);
}

Let me know what works better for you.

Cheers,
Ahmad

> 
> Thanks,
> 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 |