Hi Corentin,

On 21.10.2014 19:25, Corentin LABBE wrote:

On 10/21/14 01:28, Vladimir Zapolskiy wrote:

quoted

Hello LABBE,

On 19.10.2014 17:16, LABBE Corentin wrote:

quoted

Add support for the Security System included in Allwinner SoC A20.
The Security System is a hardware cryptographic accelerator that support AES/MD5/SHA1/DES/3DES/PRNG algorithms.

[]

quoted

quoted

+
+	/* If we have only one SG, we can use kmap_atomic */
+	if (sg_next(in_sg) == NULL && sg_next(out_sg) == NULL)
+		return sunxi_ss_aes_poll_atomic(areq);

for clarity it might be better to move all "mutex_unlock(&ss->lock)"
calls from sunxi_ss_aes_poll_atomic() body right to here.

Ok
I have moved all mutex_unlock/writel(0, SS_CTL) at the end of function, it is cleaner now.

please check that sunxi_ss_aes_poll_atomic() has no more mutex_unlock()
calls inside it.

With best wishes,
Vladimir

quoted

quoted

+
+};
+
+static int sunxi_ss_probe(struct platform_device *pdev)
+{
+	struct resource *res;
+	u32 v;
+	int err;
+	unsigned long cr;
+	const unsigned long cr_ahb = 24 * 1000 * 1000;
+	const unsigned long cr_mod = 150 * 1000 * 1000;
+
+	if (!pdev->dev.of_node)
+		return -ENODEV;
+
+	ss = devm_kzalloc(&pdev->dev, sizeof(*ss), GFP_KERNEL);
+	if (ss == NULL)
+		return -ENOMEM;

Why do you dynamically allocate memory for "struct sunxi_ss_ctx *ss"?
Since you have a single global pointer, it makes sense to declare
"struct sunxi_ss_ctx ss" statically instead.

And even a better solution is to remove a single global pointer.

All other crypto driver I have read use a global structure and it made things easy.
Thanks to M. Ripard that pointed to me the talitos driver that solve the global device pointer by using alg template and container_of().

But since I think there will never 2 Security System at the same time on the same SoC, I do not know if it is worth the cost to add more complexity just to remove a pointer.

quoted

quoted

+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	ss->base = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(ss->base)) {
+		dev_err(&pdev->dev, "Cannot request MMIO\n");
+		return PTR_ERR(ss->base);
+	}
+
+	ss->ssclk = devm_clk_get(&pdev->dev, "mod");
+	if (IS_ERR(ss->ssclk)) {
+		err = PTR_ERR(ss->ssclk);
+		dev_err(&pdev->dev, "Cannot get SS clock err=%d\n", err);
+		return err;
+	}
+	dev_dbg(&pdev->dev, "clock ss acquired\n");
+
+	ss->busclk = devm_clk_get(&pdev->dev, "ahb");
+	if (IS_ERR(ss->busclk)) {
+		err = PTR_ERR(ss->busclk);
+		dev_err(&pdev->dev, "Cannot get AHB SS clock err=%d\n", err);
+		return err;
+	}
+	dev_dbg(&pdev->dev, "clock ahb_ss acquired\n");
+
+	/* Enable both clocks */
+	err = clk_prepare_enable(ss->busclk);
+	if (err != 0) {
+		dev_err(&pdev->dev, "Cannot prepare_enable busclk\n");
+		return err;
+	}
+	err = clk_prepare_enable(ss->ssclk);
+	if (err != 0) {
+		dev_err(&pdev->dev, "Cannot prepare_enable ssclk\n");
+		clk_disable_unprepare(ss->busclk);

goto somewhere to the end of the function?

OK

quoted

quoted

+		return err;
+	}
+
+	/*
+	 * Check that clock have the correct rates gived in the datasheet
+	 * Try to set the clock to the maximum allowed
+	 */
+	err = clk_set_rate(ss->ssclk, cr_mod);
+	if (err != 0) {
+		dev_err(&pdev->dev, "Cannot set clock rate to ssclk\n");
+		clk_disable_unprepare(ss->ssclk);
+		clk_disable_unprepare(ss->busclk);

goto "error_md5"?

Ok

quoted

quoted

+		return err;
+	}
+
+	cr = clk_get_rate(ss->busclk);
+	if (cr >= cr_ahb)
+		dev_dbg(&pdev->dev, "Clock bus %lu (%lu MHz) (must be >= %lu)\n",
+				cr, cr / 1000000, cr_ahb);
+	else
+		dev_warn(&pdev->dev, "Clock bus %lu (%lu MHz) (must be >= %lu)\n",
+				cr, cr / 1000000, cr_ahb);

See next comment.

quoted

+	cr = clk_get_rate(ss->ssclk);
+	if (cr <= cr_mod)
+		if (cr < cr_mod)
+			dev_info(&pdev->dev, "Clock ss %lu (%lu MHz) (must be <= %lu)\n",
+					cr, cr / 1000000, cr_mod);
+		else
+			dev_dbg(&pdev->dev, "Clock ss %lu (%lu MHz) (must be <= %lu)\n",
+					cr, cr / 1000000, cr_mod);
+	else
+		dev_warn(&pdev->dev, "Clock ss is at %lu (%lu MHz) (must be <= %lu)\n",
+				cr, cr / 1000000, cr_mod);

The management of kernel log levels looks pretty strange. As far as I
understand there is no error on any clock rate, I'd recommend to keep
only one information message.

If clock rate are below the recommended value, the only impact I found was bad performance.
So it explain the warn and no error. (yes the info must be warn, ...fixed)

But I will put comment for explain that.

quoted

quoted

+	/*
+	 * Datasheet named it "Die Bonding ID"
+	 * I expect to be a sort of Security System Revision number.
+	 * Since the A80 seems to have an other version of SS
+	 * this info could be useful
+	 */
+	writel(SS_ENABLED, ss->base + SS_CTL);
+	v = readl(ss->base + SS_CTL);
+	v >>= 16;
+	v &= 0x07;
+	dev_info(&pdev->dev, "Die ID %d\n", v);
+	writel(0, ss->base + SS_CTL);
+
+	ss->dev = &pdev->dev;
+
+	mutex_init(&ss->lock);
+	mutex_init(&ss->bufin_lock);
+	mutex_init(&ss->bufout_lock);
+
+	err = crypto_register_ahash(&sunxi_md5_alg);
+	if (err)
+		goto error_md5;
+	err = crypto_register_ahash(&sunxi_sha1_alg);
+	if (err)
+		goto error_sha1;
+	err = crypto_register_algs(sunxi_cipher_algs,
+			ARRAY_SIZE(sunxi_cipher_algs));
+	if (err)
+		goto error_ciphers;
+
+	return 0;
+error_ciphers:
+	crypto_unregister_ahash(&sunxi_sha1_alg);
+error_sha1:
+	crypto_unregister_ahash(&sunxi_md5_alg);
+error_md5:
+	clk_disable_unprepare(ss->ssclk);
+	clk_disable_unprepare(ss->busclk);
+	return err;
+}
+
+static int __exit sunxi_ss_remove(struct platform_device *pdev)
+{
+	if (!pdev->dev.of_node)
+		return 0;

Redundant check.

Ok

quoted

--
With best wishes,
Vladimir

Thanks for the review



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