Linux-ARM-Kernel Archive on lore.kernel.org
 help / color / Atom feed
From: Maxime Ripard <mripard@kernel.org>
To: Corentin Labbe <clabbe.montjoie@gmail.com>
Cc: mark.rutland@arm.com, devicetree@vger.kernel.org,
	herbert@gondor.apana.org.au, linux-sunxi@googlegroups.com,
	linux@armlinux.org.uk, linux-kernel@vger.kernel.org,
	wens@csie.org, robh+dt@kernel.org, linux-crypto@vger.kernel.org,
	davem@davemloft.net, linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH 2/9] crypto: Add Allwinner sun8i-ce Crypto Engine
Date: Sat, 7 Sep 2019 11:19:51 +0300
Message-ID: <20190907081951.v2huvhm44jfprfop@flea> (raw)
In-Reply-To: <20190906184551.17858-3-clabbe.montjoie@gmail.com>

Hi,

I can't really comment on the crypto side, so my review is going to be
pretty boring.

On Fri, Sep 06, 2019 at 08:45:44PM +0200, Corentin Labbe wrote:
> +static const struct ce_variant ce_h3_variant = {
> +	.alg_cipher = { CE_ID_NOTSUPP, CE_ALG_AES, CE_ALG_DES, CE_ALG_3DES,
> +		CE_ID_NOTSUPP,
> +	},

As far as I can see, it's always the same value, so I'm not sure why
it's a parameter.

> +	.op_mode = { CE_ID_NOTSUPP, CE_OP_ECB, CE_OP_CBC
> +	},

Ditto

> +	.intreg = CE_ISR,

Ditto

> +	.maxflow = 4,

Ditto

> +	.ce_clks = {
> +		{ "ahb", 200000000 },

This is the default IIRC, and the clock driver will ignore any clock
rate change on it anyway, so the clock rate is pretty much useless
there.

> +		{ "mod", 48000000 },

48MHz seems pretty slow, especially compared to the other rates you
have listed there. Is that on purpose?

Also, I'm not sure what is the point of having the clocks names be
parameters there as well. It's constant across all the compatibles,
the only thing that isn't is the number of clocks and the module clock
rate. It's what you should have in there.

> +		}
> +};
> +
> +static const struct ce_variant ce_h5_variant = {
> +	.alg_cipher = { CE_ID_NOTSUPP, CE_ALG_AES, CE_ALG_DES, CE_ALG_3DES,
> +		CE_ID_NOTSUPP,
> +	},
> +	.op_mode = { CE_ID_NOTSUPP, CE_OP_ECB, CE_OP_CBC
> +	},
> +	.intreg = CE_ISR,
> +	.maxflow = 4,
> +	.ce_clks = {
> +		{ "ahb", 200000000 },
> +		{ "mod", 300000000 },
> +		}
> +};
> +
> +static const struct ce_variant ce_h6_variant = {
> +	.alg_cipher = { CE_ID_NOTSUPP, CE_ALG_AES, CE_ALG_DES, CE_ALG_3DES,
> +		CE_ALG_RAES,
> +	},
> +	.op_mode = { CE_ID_NOTSUPP, CE_OP_ECB, CE_OP_CBC
> +	},
> +	.model = CE_v2,

Can't that be derived from the version register and / or the
compatible? This seems to be redundant with each.

> +	.intreg = CE_ISR,
> +	.maxflow = 4,
> +	.ce_clks = {
> +		{ "ahb", 200000000 },
> +		{ "mod", 300000000 },
> +		{ "mbus", 400000000 },

That rate is going to be ignored as well.

> +		}
> +};
> +
> +static const struct ce_variant ce_a64_variant = {
> +	.alg_cipher = { CE_ID_NOTSUPP, CE_ALG_AES, CE_ALG_DES, CE_ALG_3DES,
> +		CE_ID_NOTSUPP,
> +	},
> +	.op_mode = { CE_ID_NOTSUPP, CE_OP_ECB, CE_OP_CBC
> +	},
> +	.intreg = CE_ISR,
> +	.maxflow = 4,
> +	.ce_clks = {
> +		{ "ahb", 200000000 },
> +		{ "mod", 300000000 },
> +		}
> +};

You should order your variants by alphabetical order.

> +static const struct ce_variant ce_r40_variant = {
> +	.alg_cipher = { CE_ID_NOTSUPP, CE_ALG_AES, CE_ALG_DES, CE_ALG_3DES,
> +		CE_ID_NOTSUPP,
> +	},
> +	.op_mode = { CE_ID_NOTSUPP, CE_OP_ECB, CE_OP_CBC
> +	},
> +	.intreg = CE_ISR,
> +	.maxflow = 4,
> +	.ce_clks = {
> +		{ "ahb", 200000000 },
> +		{ "mod", 300000000 },
> +		}
> +};
> +

...

> +int sun8i_ce_get_engine_number(struct sun8i_ce_dev *ce)
> +{
> +	return atomic_inc_return(&ce->flow) % ce->variant->maxflow;
> +}

I'm not sure what this is supposed to be doing, but that mod there
seems pretty dangerous.

...

> +static int sun8i_ce_probe(struct platform_device *pdev)
> +{
> +	struct resource *res;
> +	u32 v;
> +	int err, i, ce_method, id, irq;
> +	unsigned long cr;
> +	struct sun8i_ce_dev *ce;
> +
> +	if (!pdev->dev.of_node)
> +		return -ENODEV;

This is pretty much guaranteed already

> +	ce = devm_kzalloc(&pdev->dev, sizeof(*ce), GFP_KERNEL);
> +	if (!ce)
> +		return -ENOMEM;
> +
> +	ce->variant = of_device_get_match_data(&pdev->dev);
> +	if (!ce->variant) {
> +		dev_err(&pdev->dev, "Missing Crypto Engine variant\n");
> +		return -EINVAL;
> +	}
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	ce->base = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(ce->base)) {
> +		err = PTR_ERR(ce->base);
> +		dev_err(&pdev->dev, "Cannot request MMIO err=%d\n", err);
> +		return err;

ioremap_resource already prints an error message on failure, so this
is redundant.

> +	}
> +
> +	for (i = 0; i < CE_MAX_CLOCKS; i++) {
> +		if (!ce->variant->ce_clks[i].name)
> +			continue;
> +		dev_info(&pdev->dev, "Get %s clock\n", ce->variant->ce_clks[i].name);

There's no reason to print this at the info level

> +		ce->ceclks[i] = devm_clk_get(&pdev->dev, ce->variant->ce_clks[i].name);
> +		if (IS_ERR(ce->ceclks[i])) {
> +			err = PTR_ERR(ce->ceclks[i]);
> +			dev_err(&pdev->dev, "Cannot get %s CE clock err=%d\n",
> +				ce->variant->ce_clks[i].name, err);
> +		}
> +		cr = clk_get_rate(ce->ceclks[i]);

So on error you'd call clk_get_rate on the clock still? That seems
pretty fragile, you should return there, it's a hard error.

> +		if (ce->variant->ce_clks[i].freq) {
> +			dev_info(&pdev->dev, "Set %s clock to %lu (%lu Mhz) from %lu (%lu Mhz)\n",
> +				 ce->variant->ce_clks[i].name,
> +				 ce->variant->ce_clks[i].freq,
> +				 ce->variant->ce_clks[i].freq / 1000000,
> +				 cr,
> +				 cr / 1000000);

Same remark about that message than the previous one.

> +			err = clk_set_rate(ce->ceclks[i], ce->variant->ce_clks[i].freq);
> +			if (err)
> +				dev_err(&pdev->dev, "Fail to set %s clk speed to %lu\n",
> +					ce->variant->ce_clks[i].name,
> +					ce->variant->ce_clks[i].freq);
> +		} else {
> +			dev_info(&pdev->dev, "%s run at %lu\n",
> +				 ce->variant->ce_clks[i].name, cr);

Ditto.

> +		}
> +		err = clk_prepare_enable(ce->ceclks[i]);

Do you really need this right now though?

> +		if (err) {
> +			dev_err(&pdev->dev, "Cannot prepare_enable %s\n",
> +				ce->variant->ce_clks[i].name);
> +			return err;
> +		}
> +	}
> +
> +	/* Get Non Secure IRQ */
> +	irq = platform_get_irq(pdev, 0);
> +	if (irq < 0) {
> +		dev_err(ce->dev, "Cannot get NS IRQ\n");
> +		return irq;
> +	}
> +
> +	err = devm_request_irq(&pdev->dev, irq, ce_irq_handler, 0,
> +			       "sun8i-ce-ns", ce);
> +	if (err < 0) {
> +		dev_err(ce->dev, "Cannot request NS IRQ\n");
> +		return err;
> +	}
> +
> +	ce->reset = devm_reset_control_get_optional(&pdev->dev, "ahb");
> +	if (IS_ERR(ce->reset)) {
> +		if (PTR_ERR(ce->reset) == -EPROBE_DEFER)
> +			return PTR_ERR(ce->reset);
> +		dev_info(&pdev->dev, "No reset control found\n");

It's not optional though.

> +		ce->reset = NULL;
> +	}
> +
> +	err = reset_control_deassert(ce->reset);
> +	if (err) {
> +		dev_err(&pdev->dev, "Cannot deassert reset control\n");
> +		goto error_clk;
> +	}

Again, you don't really need this at this moment. Using runtime_pm
would make more sense.

> +	v = readl(ce->base + CE_CTR);
> +	v >>= 16;
> +	v &= 0x07;

This should be in a define

> +	dev_info(&pdev->dev, "CE_NS Die ID %x\n", v);

And if that really makes sense to print it, the error message should
be made less cryptic.

> +
> +	ce->dev = &pdev->dev;
> +	platform_set_drvdata(pdev, ce);
> +
> +	mutex_init(&ce->mlock);
> +
> +	ce->chanlist = devm_kcalloc(ce->dev, ce->variant->maxflow,
> +				    sizeof(struct sun8i_ce_flow), GFP_KERNEL);
> +	if (!ce->chanlist) {
> +		err = -ENOMEM;
> +		goto error_flow;
> +	}
> +
> +	for (i = 0; i < ce->variant->maxflow; i++) {
> +		init_completion(&ce->chanlist[i].complete);
> +		mutex_init(&ce->chanlist[i].lock);
> +
> +		ce->chanlist[i].engine = crypto_engine_alloc_init(ce->dev, true);
> +		if (!ce->chanlist[i].engine) {
> +			dev_err(ce->dev, "Cannot allocate engine\n");
> +			i--;
> +			goto error_engine;
> +		}
> +		err = crypto_engine_start(ce->chanlist[i].engine);
> +		if (err) {
> +			dev_err(ce->dev, "Cannot start engine\n");
> +			goto error_engine;
> +		}
> +		ce->chanlist[i].tl = dma_alloc_coherent(ce->dev,
> +							sizeof(struct ce_task),
> +							&ce->chanlist[i].t_phy,
> +							GFP_KERNEL);
> +		if (!ce->chanlist[i].tl) {
> +			dev_err(ce->dev, "Cannot get DMA memory for task %d\n",
> +				i);
> +			err = -ENOMEM;
> +			goto error_engine;
> +		}
> +	}

All this initialization should be done before calling
request_irq. You're using some of those fields in your handler.

> +
> +#ifdef CONFIG_CRYPTO_DEV_SUN8I_CE_DEBUG
> +	ce->dbgfs_dir = debugfs_create_dir("sun8i-ce", NULL);
> +	if (IS_ERR_OR_NULL(ce->dbgfs_dir)) {
> +		dev_err(ce->dev, "Fail to create debugfs dir");
> +		err = -ENOMEM;
> +		goto error_engine;
> +	}
> +	ce->dbgfs_stats = debugfs_create_file("stats", 0444,
> +					      ce->dbgfs_dir, ce,
> +					      &sun8i_ce_debugfs_fops);
> +	if (IS_ERR_OR_NULL(ce->dbgfs_stats)) {
> +		dev_err(ce->dev, "Fail to create debugfs stat");
> +		err = -ENOMEM;
> +		goto error_debugfs;
> +	}
> +#endif
> +	for (i = 0; i < ARRAY_SIZE(ce_algs); i++) {
> +		ce_algs[i].ce = ce;
> +		switch (ce_algs[i].type) {
> +		case CRYPTO_ALG_TYPE_SKCIPHER:
> +			id = ce_algs[i].ce_algo_id;
> +			ce_method = ce->variant->alg_cipher[id];
> +			if (ce_method == CE_ID_NOTSUPP) {
> +				dev_info(ce->dev,
> +					 "DEBUG: Algo of %s not supported\n",
> +					 ce_algs[i].alg.skcipher.base.cra_name);
> +				ce_algs[i].ce = NULL;
> +				break;
> +			}
> +			id = ce_algs[i].ce_blockmode;
> +			ce_method = ce->variant->op_mode[id];
> +			if (ce_method == CE_ID_NOTSUPP) {
> +				dev_info(ce->dev, "DEBUG: Blockmode of %s not supported\n",
> +					 ce_algs[i].alg.skcipher.base.cra_name);
> +				ce_algs[i].ce = NULL;
> +				break;
> +			}
> +			dev_info(ce->dev, "DEBUG: Register %s\n",
> +				 ce_algs[i].alg.skcipher.base.cra_name);
> +			err = crypto_register_skcipher(&ce_algs[i].alg.skcipher);
> +			if (err) {
> +				dev_err(ce->dev, "Fail to register %s\n",
> +					ce_algs[i].alg.skcipher.base.cra_name);
> +				ce_algs[i].ce = NULL;
> +				goto error_alg;
> +			}
> +			break;
> +		default:
> +			dev_err(ce->dev, "ERROR: tryed to register an unknown algo\n");
> +		}
> +	}
> +
> +	return 0;
> +error_alg:
> +	i--;
> +	for (; i >= 0; i--) {
> +		switch (ce_algs[i].type) {
> +		case CRYPTO_ALG_TYPE_SKCIPHER:
> +			if (ce_algs[i].ce)
> +				crypto_unregister_skcipher(&ce_algs[i].alg.skcipher);
> +			break;
> +		}
> +	}
> +#ifdef CONFIG_CRYPTO_DEV_SUN8I_CE_DEBUG
> +error_debugfs:
> +	debugfs_remove_recursive(ce->dbgfs_dir);
> +#endif
> +	i = ce->variant->maxflow;
> +error_engine:
> +	while (i >= 0) {
> +		crypto_engine_exit(ce->chanlist[i].engine);
> +		if (ce->chanlist[i].tl)
> +			dma_free_coherent(ce->dev, sizeof(struct ce_task),
> +					  ce->chanlist[i].tl,
> +					  ce->chanlist[i].t_phy);
> +		i--;
> +	}
> +error_flow:
> +	reset_control_assert(ce->reset);
> +error_clk:
> +	for (i = 0; i < CE_MAX_CLOCKS; i++)
> +		clk_disable_unprepare(ce->ceclks[i]);
> +	return err;
> +}

So that function takes around 200-250 LoC, this is definitely too
much and should be split into multiple functions.

Maxime

--
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

  reply index

Thread overview: 32+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-09-06 18:45 [PATCH 0/9] crypto: add sun8i-ce driver for Allwinner crypto engine Corentin Labbe
2019-09-06 18:45 ` [PATCH 1/9] crypto: Add allwinner subdirectory Corentin Labbe
2019-09-07  3:54   ` Maxime Ripard
2019-09-07 17:53     ` Corentin Labbe
2019-09-06 18:45 ` [PATCH 2/9] crypto: Add Allwinner sun8i-ce Crypto Engine Corentin Labbe
2019-09-07  8:19   ` Maxime Ripard [this message]
2019-09-07 19:04     ` Corentin Labbe
2019-09-09 11:38       ` Maxime Ripard
2019-09-09 13:19         ` Corentin Labbe
2019-09-09 13:59           ` Maxime Ripard
2019-09-06 18:45 ` [PATCH 3/9] dt-bindings: crypto: Add DT bindings documentation for " Corentin Labbe
2019-09-07  4:01   ` Maxime Ripard
2019-09-11 18:31     ` Corentin Labbe
2019-09-12  9:37       ` Maxime Ripard
2019-09-12 20:26         ` Chen-Yu Tsai
2019-09-12 20:33           ` Maxime Ripard
2019-09-12 20:37             ` Chen-Yu Tsai
2019-09-13 12:11               ` Maxime Ripard
2019-09-06 18:45 ` [PATCH 4/9] ARM: dts: sun8i: r40: add crypto engine node Corentin Labbe
2019-09-07  4:02   ` Maxime Ripard
2019-09-07 17:54     ` Corentin Labbe
2019-09-06 18:45 ` [PATCH 5/9] ARM: dts: sun8i: h3: Add Crypto Engine node Corentin Labbe
2019-09-06 18:45 ` [PATCH 6/9] ARM64: dts: allwinner: sun50i: Add Crypto Engine node on A64 Corentin Labbe
2019-09-07  4:02   ` Maxime Ripard
2019-09-07 17:54     ` Corentin Labbe
2019-09-06 18:45 ` [PATCH 7/9] ARM64: dts: allwinner: sun50i: Add crypto engine node on H5 Corentin Labbe
2019-09-06 18:45 ` [PATCH 8/9] ARM64: dts: allwinner: sun50i: Add Crypto Engine node on H6 Corentin Labbe
2019-09-06 18:45 ` [PATCH 9/9] sunxi_defconfig: add new crypto options Corentin Labbe
2019-09-07  4:03   ` Maxime Ripard
2019-09-07 17:55     ` Corentin Labbe
2019-09-13  8:15     ` Corentin Labbe
2019-09-13 12:10       ` Maxime Ripard

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20190907081951.v2huvhm44jfprfop@flea \
    --to=mripard@kernel.org \
    --cc=clabbe.montjoie@gmail.com \
    --cc=davem@davemloft.net \
    --cc=devicetree@vger.kernel.org \
    --cc=herbert@gondor.apana.org.au \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-crypto@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-sunxi@googlegroups.com \
    --cc=linux@armlinux.org.uk \
    --cc=mark.rutland@arm.com \
    --cc=robh+dt@kernel.org \
    --cc=wens@csie.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-ARM-Kernel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-arm-kernel/0 linux-arm-kernel/git/0.git
	git clone --mirror https://lore.kernel.org/linux-arm-kernel/1 linux-arm-kernel/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-arm-kernel linux-arm-kernel/ https://lore.kernel.org/linux-arm-kernel \
		linux-arm-kernel@lists.infradead.org
	public-inbox-index linux-arm-kernel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.infradead.lists.linux-arm-kernel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git