Linux-ARM-Kernel Archive on lore.kernel.org
 help / color / Atom feed
From: Corentin Labbe <clabbe.montjoie@gmail.com>
To: Maxime Ripard <mripard@kernel.org>
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: Mon, 9 Sep 2019 15:19:06 +0200
Message-ID: <20190909131906.GA12882@Red> (raw)
In-Reply-To: <20190909113837.vrnqdfgzhsiymfpm@flea>

On Mon, Sep 09, 2019 at 01:38:37PM +0200, Maxime Ripard wrote:
> On Sat, Sep 07, 2019 at 09:04:08PM +0200, Corentin Labbe wrote:
> > > 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.
> >
> > Since the datasheet give some max frequency, I think I will add a
> > max_freq and add a check to verify if the clock is in the right
> > range
> 
> It's a bit pointless. What are you going to do if it's not correct?
> What are you trying to fix / report with this?

I thinked to print a warning.
If someone want to play with overclocking for example, the driver should said that probably some result could be invalid.

> 
> > > > +		}
> > > > +};
> > > > +
> > > > +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.
> >
> > I could use the compatible, but I want to avoid a string comparison
> > on each request.
> 
> Well, this is specifically what this structure is for then, right? So
> instead of having the model, just add the information that you want
> there.
> 

ok, I will change to a "bool all_size_in_bytes"

> > > > +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.
> > >
> > > ...
> >
> > This mod do a round robin on each channel.
> > I dont see why it is dangerous.
> 
> Well, you're using the atomic API here which is most commonly used for
> refcounting, while you're using a mod.
> 
> Plus, while the increment is atomic, the modulo isn't, so you can end
> up in a case where you would be preempted between the
> atomic_inc_return and the mod, which is dangerous.
> 
> Again, I'm not sure what this function is doing (which is also a
> problem in itself). I guess you should just make it clearer what it
> does, and then we can discuss it properly.

Each request need to be assigned to a channel.
Each channel are identified by a number from 1 to 4.

So this function return the channel to use, 1 then 2 then 3 then 4 then 1...
Note that this is uncritical. If, due to anything, two request are assigned to the same channel, nothing will break.

> 
> > > > +			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?
> >
> > Not sure to understand, why I shouldnt do it now ?
> > Does it is related to your pm_runtime remark below ?
> >
> > My feeling was to submit the driver without PM and convert it after.
> 
> runtime_pm would be pretty cheap to add though judging by what you're
> doing there.
> 

I will try to add runtime_pm

> > > > +		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.
> >
> > I dont understand why.
> 
> On all the SoCs, you need that reset line to be deasserted, otherwise
> the IP (and therefore the driver) will be non-functional. It's not an
> option to run without it.

Currently all the SoC have a reset, but nothing prevent a new SoC with CE without reset.
Anyway, I will made the reset mandatory for the moment.

> 
> > > > +		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
> > >
> >
> > Will fix.
> >
> > > > +	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.
> > >
> >
> > Will fix.
> >
> > > > +
> > > > +	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.
> >
> > No interrupt could fire, since algorithms are still not registred.
> 
> That's not true. Spurious interrupts are a thing, the engine could
> have been left in a weird state by the bootloader / kexec / reboot
> with some pending interrupts, etc.
> 
> You have registered that handler already, you should expect it to be
> called at any point in time.
> 

Ok will fix.

Thanks for your review.

_______________________________________________
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
2019-09-07 19:04     ` Corentin Labbe
2019-09-09 11:38       ` Maxime Ripard
2019-09-09 13:19         ` Corentin Labbe [this message]
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=20190909131906.GA12882@Red \
    --to=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=mripard@kernel.org \
    --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