On Wed, 14 Mar 2012 09:45:46 +0100
Andreas Westin <andreas.westin@xxxxxxxxxxxxxx> wrote:
> +config CRYPTO_DEV_UX500
> + tristate "Driver for ST-Ericsson UX500 crypto hardware acceleration"
> + #depends on ARCH_U8500
guessing this should be either removed or modified to include u5500?
> +config CRYPTO_DEV_UX500_CRYP
> + tristate "UX500 crypto driver for CRYP block"
> + depends on CRYPTO_DEV_UX500
> + select CRYPTO_DES
> + help
> + This is the driver for the crypto block CRYP.
what will its module be called?
> +config CRYPTO_DEV_UX500_DEBUG
> + bool "Activate ux500 platform debug-mode for crypto and hash block"
> + depends on CRYPTO_DEV_UX500_CRYP || CRYPTO_DEV_UX500_HASH
> + default n
> + help
> + Say Y if you want to add debug prints to ux500_hash and
> + ux500_cryp devices.
there are better ways to do this now - see dynamic-debug-howto.txt.
> + int peripheralID2 = 0;
mixed-case name
> +
> + if (NULL == device_data)
> + return -EINVAL;
> +
> + if (cpu_is_u8500())
> + peripheralID2 = CRYP_PERIPHERAL_ID2_DB8500;
> + else if (cpu_is_u5500())
> + peripheralID2 = CRYP_PERIPHERAL_ID2_DB5500;
> +
> + /* Check Peripheral and Pcell Id Register for CRYP */
> + if ((CRYP_PERIPHERAL_ID0 ==
> + readl_relaxed(&device_data->base->periphId0))
> + && (CRYP_PERIPHERAL_ID1 ==
> + readl_relaxed(&device_data->base->periphId1))
> + && (peripheralID2 ==
> + readl_relaxed(&device_data->base->periphId2))
> + && (CRYP_PERIPHERAL_ID3 ==
> + readl_relaxed(&device_data->base->periphId3))
> + && (CRYP_PCELL_ID0 ==
> + readl_relaxed(&device_data->base->pcellId0))
> + && (CRYP_PCELL_ID1 ==
> + readl_relaxed(&device_data->base->pcellId1))
> + && (CRYP_PCELL_ID2 ==
> + readl_relaxed(&device_data->base->pcellId2))
> + && (CRYP_PCELL_ID3 ==
> + readl_relaxed(&device_data->base->pcellId3))) {
mixed-case names
> + return 0;
> + }
> +
> + return -EPERM;
> +}
> +
> +/**
> + * cryp_activity - This routine enables/disable the cryptography function.
> + * @device_data: Pointer to the device data struct for base address.
> + * @cryp_activity: Enable/Disable functionality
> + */
> +void cryp_activity(struct cryp_device_data *device_data,
> + enum cryp_crypen cryp_crypen)
comment should say @cryp_crypen:
> +void cryp_flush_inoutfifo(struct cryp_device_data *device_data)
> +{
> + /*
> + * We always need to disble the hardware before trying to flush the
disable
> + * After the keyprepartion for decrypting is done you should set
key preparation
> +int cryp_configure_key_values(struct cryp_device_data *device_data,
> + enum cryp_key_reg_index key_reg_index,
> + struct cryp_key_value key_value)
> +{
> + while (cryp_is_logic_busy(device_data))
> + cpu_relax();
> +
> + switch (key_reg_index) {
> + case CRYP_KEY_REG_1:
> + writel_relaxed(key_value.key_value_left,
> + &device_data->base->key_1_l);
alignment issues, presumably after a s/writel/writel_relaxed/g
during development (should make it easy to re-check all occurrences).
> +/**
> + * cryp_write_indata - This routine writes 32 bit data into the data input
> + * register of the cryptography IP.
> + * @device_data: Pointer to the device data struct for base address.
> + * @write_data: Data word to write
> + */
> +int cryp_write_indata(struct cryp_device_data *device_data, u32 write_data)
> +{
> + writel_relaxed(write_data, &device_data->base->din);
> +
> + return 0;
> +}
> +
> +/**
> + * cryp_read_outdata - This routine reads the data from the data output
> + * register of the CRYP logic
> + * @device_data: Pointer to the device data struct for base address.
> + * @read_data: Read the data from the output FIFO.
> + */
> +int cryp_read_outdata(struct cryp_device_data *device_data, u32 *read_data)
> +{
> + *read_data = readl_relaxed(&device_data->base->dout);
> +
> + return 0;
> +}
I would say these should be made void, static, inline, etc., but
they only have one callsite each - are they necessary at all?
> + * Protection configuration structure for setting privilage access
privileged
> + * CRYP power management specifc structure.
specific
> +/**
> + * uint8p_to_uint32_be - 4*uint8 to uint32 big endian
> + * @in: Data to convert.
> + */
> +static inline u32 uint8p_to_uint32_be(u8 *in)
> +{
> + return (u32)in[0]<<24 |
> + ((u32)in[1]<<16) |
> + ((u32)in[2]<<8) |
> + ((u32)in[3]);
> +}
use cpu_to_be32
> +/**
> + * swap_bits_in_byte - mirror the bits in a byte
> + * @b: the byte to be mirrored
> + * The bits are swapped the following way:
> + * Byte b include bits 0-7, nibble 1 (n1) include bits 0-3 and
> + * nibble 2 (n2) bits 4-7.
> + *
> + * Nibble 1 (n1):
> + * (The "old" (moved) bit is replaced with a zero)
> + * 1. Move bit 6 and 7, 4 positions to the left.
> + * 2. Move bit 3 and 5, 2 positions to the left.
> + * 3. Move bit 1-4, 1 position to the left.
> + *
> + * Nibble 2 (n2):
> + * 1. Move bit 0 and 1, 4 positions to the right.
> + * 2. Move bit 2 and 4, 2 positions to the right.
> + * 3. Move bit 3-6, 1 position to the right.
> + *
> + * Combine the two nibbles to a complete and swapped byte.
> + */
> +
> +static inline u8 swap_bits_in_byte(u8 b)
can't exactly tell - is this because the h/w needs its keys
provided in bitwise-reverse order? and for decryption only?
> +{
> +#define R_SHIFT_4_MASK (0xc0) /* Bits 6 and 7, right shift 4 */
> +#define R_SHIFT_2_MASK (0x28) /* (After right shift 4) Bits 3 and 5,
> + right shift 2 */
> +#define R_SHIFT_1_MASK (0x1e) /* (After right shift 2) Bits 1-4,
> + right shift 1 */
> +#define L_SHIFT_4_MASK (0x03) /* Bits 0 and 1, left shift 4 */
> +#define L_SHIFT_2_MASK (0x14) /* (After left shift 4) Bits 2 and 4,
> + left shift 2 */
> +#define L_SHIFT_1_MASK (0x78) /* (After left shift 1) Bits 3-6,
> + left shift 1 */
unnecessary parens
> +static irqreturn_t cryp_interrupt_handler(int irq, void *param)
> +{
> + struct cryp_ctx *ctx;
> + int i;
> + struct cryp_device_data *device_data;
> +
> + if (param == NULL) {
> + BUG_ON(!param);
> + return IRQ_HANDLED;
> + }
this makes no sense - plus, when would !param be true in the first
place?
> +
> + /* The device is coming from the one found in hw_crypt_noxts. */
> + device_data = (struct cryp_device_data *)param;
> +
> + ctx = device_data->current_ctx;
> +
> + if (ctx == NULL) {
> + BUG_ON(!ctx);
> + return IRQ_HANDLED;
> + }
same here
> +
> + dev_dbg(ctx->device->dev, "[%s] (len: %d) %s, ", __func__, ctx->outlen,
> + cryp_pending_irq_src(device_data, CRYP_IRQ_SRC_OUTPUT_FIFO) ?
> + "out" : "in");
> +
> + if (cryp_pending_irq_src(device_data,
> + CRYP_IRQ_SRC_OUTPUT_FIFO)) {
> + if (ctx->outlen / ctx->blocksize > 0) {
> + for (i = 0; i < ctx->blocksize / 4; i++) {
> + cryp_read_outdata(device_data,
> + (u32 *)ctx->outdata);
> + ctx->outdata += 4;
> + ctx->outlen -= 4;
> + }
> +
> + if (ctx->outlen == 0) {
> + cryp_disable_irq_src(device_data,
> + CRYP_IRQ_SRC_OUTPUT_FIFO);
> + }
> + }
> + } else if (cryp_pending_irq_src(device_data,
> + CRYP_IRQ_SRC_INPUT_FIFO)) {
> + if (ctx->datalen / ctx->blocksize > 0) {
> + for (i = 0 ; i < ctx->blocksize / 4; i++) {
> + cryp_write_indata(device_data,
> + *((u32 *)ctx->indata));
> + ctx->indata += 4;
> + ctx->datalen -= 4;
> + }
> +
> + if (ctx->datalen == 0)
> + cryp_disable_irq_src(device_data,
> + CRYP_IRQ_SRC_INPUT_FIFO);
> +
> + if (ctx->config.algomode == CRYP_ALGO_AES_XTS) {
> + CRYP_PUT_BITS(&device_data->base->cr,
> + CRYP_START_ENABLE,
> + CRYP_CR_START_POS,
> + CRYP_CR_START_MASK);
> +
> + cryp_wait_until_done(device_data);
> + }
> + }
> + }
> +
> + return IRQ_HANDLED;
> +}
curious again - why are irq's being disabled in the IRQ handler, and
then only reenabled upon a new request - why can't they stay
enabled? Is this due to the 'polling mode' option somehow not being
mutually exclusive with an interrupt-based request?
> +static int mode_is_aes(enum cryp_algo_mode mode)
> +{
> + return (CRYP_ALGO_AES_ECB == mode) ||
> + (CRYP_ALGO_AES_CBC == mode) ||
> + (CRYP_ALGO_AES_CTR == mode) ||
> + (CRYP_ALGO_AES_XTS == mode);
unnecessary inner parens
> + if (ctx->updated == 0) {
> + cryp_flush_inoutfifo(device_data);
> + if (cfg_keys(ctx) != 0) {
> + dev_err(ctx->device->dev, "[%s]: cfg_keys failed!",
> + __func__);
> + return -EPERM;
-EINVAL?
> + }
> +
> + if ((ctx->iv) &&
> + (CRYP_ALGO_AES_ECB != ctx->config.algomode) &&
> + (CRYP_ALGO_DES_ECB != ctx->config.algomode) &&
> + (CRYP_ALGO_TDES_ECB != ctx->config.algomode)) {
unnecessary inner parens
> + if (!ctx->device->dma.sg_dst_len) {
> + dev_dbg(ctx->device->dev,
> + "[%s]: Could not map the sg list "
> + "(FROM_DEVICE)", __func__);
message text strings are allowed to exceed the 80 char line limit,
for grep-ability reasons.
> + regulator_disable(
> + device_data->pwr_regulator);
join last 2 lines
> + /*
> + * ctx->outlen is decremented in the cryp_interrupt_handler
> + * function. We had to add cpu_relax() (barrier) to make sure
> + * that gcc didn't optimze away this variable.
optimize
> + */
> + while (ctx->outlen > 0)
> + cpu_relax();
do we need a timeout check in case h/w goes in the weeds?
> +static void aes_encrypt(struct crypto_tfm *tfm, u8 *out, const u8 *in)
> +{
> + struct cryp_ctx *ctx = crypto_tfm_ctx(tfm);
> +
> + pr_debug(DEV_DBG_NAME " [%s]", __func__);
> +
> + ctx->blocksize = crypto_tfm_alg_blocksize(tfm);
> +
> + ctx->config.algodir = CRYP_ALGORITHM_ENCRYPT;
> + ctx->config.algomode = CRYP_ALGO_AES_ECB;
> +
> + ctx->indata = in;
> + ctx->outdata = out;
> + ctx->datalen = ctx->blocksize;
> +
> + if (cryp_hw_calculate(ctx))
> + pr_err("ux500_cryp:crypX: [%s]: cryp_hw_calculate() failed!",
> + __func__);
shouldn't this error be propagated to the higher level API?
> +}
> +/**
> + * struct crypto_alg aes_alg
> + */
> +static struct crypto_alg aes_alg = {
> + .cra_name = "aes",
> + .cra_driver_name = "aes-ux500",
> + .cra_priority = 300,
> + .cra_flags = CRYPTO_ALG_TYPE_CIPHER,
> + .cra_blocksize = AES_BLOCK_SIZE,
> + .cra_ctxsize = sizeof(struct cryp_ctx),
> + .cra_alignmask = 3,
> + .cra_module = THIS_MODULE,
> + .cra_list = LIST_HEAD_INIT(aes_alg.cra_list),
> + .cra_u = {
> + .cipher = {
> + .cia_min_keysize = AES_MIN_KEY_SIZE,
> + .cia_max_keysize = AES_MAX_KEY_SIZE,
> + .cia_setkey = aes_setkey,
> + .cia_encrypt = aes_encrypt,
> + .cia_decrypt = aes_decrypt
> + }
> + }
> +};
each of the entry points already assign algodir, algomode, and
blocksize; why not add the corresponding values in a template struct
here, that embeds the crypto_alg structs instead (see struct
talitos_alg_template). The code would be greatly simplified.
Don't know if it would help, but see also commit 4b00434 "crypto:
Add bulk algorithm registration interface" (in Herbert's crypto-2.6
tree).
Kim
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
