[v6,04/14] crypto: caam - request JR IRQ as the last step
diff mbox series

Message ID 20190717152458.22337-5-andrew.smirnov@gmail.com
State New
Headers show
Series
  • crypto: caam - Add i.MX8MQ support
Related show

Commit Message

Andrey Smirnov July 17, 2019, 3:24 p.m. UTC
In order to avoid any risk of JR IRQ request being handled while some
of the resources used for that are not yet allocated move the code
requesting said IRQ to the endo of caam_jr_init(). No functional
change intended.

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
Cc: Chris Spencer <christopher.spencer@sea.co.uk>
Cc: Cory Tusar <cory.tusar@zii.aero>
Cc: Chris Healy <cphealy@gmail.com>
Cc: Lucas Stach <l.stach@pengutronix.de>
Cc: Horia Geantă <horia.geanta@nxp.com>
Cc: Aymen Sghaier <aymen.sghaier@nxp.com>
Cc: Leonard Crestez <leonard.crestez@nxp.com>
Cc: linux-crypto@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
---
 drivers/crypto/caam/jr.c | 34 ++++++++++++++++------------------
 1 file changed, 16 insertions(+), 18 deletions(-)

Comments

Horia Geanta July 23, 2019, 4:02 p.m. UTC | #1
On 7/17/2019 6:25 PM, Andrey Smirnov wrote:
> In order to avoid any risk of JR IRQ request being handled while some
> of the resources used for that are not yet allocated move the code
> requesting said IRQ to the endo of caam_jr_init(). No functional
                             ^ typo
> change intended.
> 
What qualifies as a "functional change"?
I've seen this comment in several commits.

>  	error = caam_reset_hw_jr(dev);
>  	if (error)
> -		goto out_kill_deq;
> +		return error;
>  
>  	error = -ENOMEM;
>  	jrp->inpring = dmam_alloc_coherent(dev, sizeof(*jrp->inpring) *
>  					   JOBR_DEPTH, &inpbusaddr,
>  					   GFP_KERNEL);
>  	if (!jrp->inpring)
> -		goto out_kill_deq;
> +		return -ENOMEM;
Above there's "error = -ENOMEM;", so why not "return err;" here and
in all the other cases below?

>  
>  	jrp->outring = dmam_alloc_coherent(dev, sizeof(*jrp->outring) *
>  					   JOBR_DEPTH, &outbusaddr,
>  					   GFP_KERNEL);
>  	if (!jrp->outring)
> -		goto out_kill_deq;
> +		return -ENOMEM;
>  
>  	jrp->entinfo = devm_kcalloc(dev, JOBR_DEPTH, sizeof(*jrp->entinfo),
>  				    GFP_KERNEL);
>  	if (!jrp->entinfo)
> -		goto out_kill_deq;
> +		return -ENOMEM;
>  
>  	for (i = 0; i < JOBR_DEPTH; i++)
>  		jrp->entinfo[i].desc_addr_dma = !0;
> @@ -483,10 +472,19 @@ static int caam_jr_init(struct device *dev)
>  		      (JOBR_INTC_COUNT_THLD << JRCFG_ICDCT_SHIFT) |
>  		      (JOBR_INTC_TIME_THLD << JRCFG_ICTT_SHIFT));
>  
> +	tasklet_init(&jrp->irqtask, caam_jr_dequeue, (unsigned long)dev);
> +
> +	/* Connect job ring interrupt handler. */
> +	error = devm_request_irq(dev, jrp->irq, caam_jr_interrupt, IRQF_SHARED,
> +				 dev_name(dev), dev);
> +	if (error) {
> +		dev_err(dev, "can't connect JobR %d interrupt (%d)\n",
> +			jrp->ridx, jrp->irq);
> +		tasklet_kill(&jrp->irqtask);
> +		return error;
"return error;" should be moved out the if block.

> +	}
> +
>  	return 0;
> -out_kill_deq:
> -	tasklet_kill(&jrp->irqtask);
> -	return error;
>  }

Horia
Andrey Smirnov Aug. 12, 2019, 5:59 p.m. UTC | #2
On Tue, Jul 23, 2019 at 9:02 AM Horia Geanta <horia.geanta@nxp.com> wrote:
>
> On 7/17/2019 6:25 PM, Andrey Smirnov wrote:
> > In order to avoid any risk of JR IRQ request being handled while some
> > of the resources used for that are not yet allocated move the code
> > requesting said IRQ to the endo of caam_jr_init(). No functional
>                              ^ typo
> > change intended.
> >
> What qualifies as a "functional change"?
> I've seen this comment in several commits.
>

My intent was to mark refactoring only changes as such. Probably not
appropriate for this commit. Will drop in v7.

> >       error = caam_reset_hw_jr(dev);
> >       if (error)
> > -             goto out_kill_deq;
> > +             return error;
> >
> >       error = -ENOMEM;
> >       jrp->inpring = dmam_alloc_coherent(dev, sizeof(*jrp->inpring) *
> >                                          JOBR_DEPTH, &inpbusaddr,
> >                                          GFP_KERNEL);
> >       if (!jrp->inpring)
> > -             goto out_kill_deq;
> > +             return -ENOMEM;
> Above there's "error = -ENOMEM;", so why not "return err;" here and
> in all the other cases below?
>

I was going to remove that "error = -ENOMEM;", but forgot. Will do in v7.

> >
> >       jrp->outring = dmam_alloc_coherent(dev, sizeof(*jrp->outring) *
> >                                          JOBR_DEPTH, &outbusaddr,
> >                                          GFP_KERNEL);
> >       if (!jrp->outring)
> > -             goto out_kill_deq;
> > +             return -ENOMEM;
> >
> >       jrp->entinfo = devm_kcalloc(dev, JOBR_DEPTH, sizeof(*jrp->entinfo),
> >                                   GFP_KERNEL);
> >       if (!jrp->entinfo)
> > -             goto out_kill_deq;
> > +             return -ENOMEM;
> >
> >       for (i = 0; i < JOBR_DEPTH; i++)
> >               jrp->entinfo[i].desc_addr_dma = !0;
> > @@ -483,10 +472,19 @@ static int caam_jr_init(struct device *dev)
> >                     (JOBR_INTC_COUNT_THLD << JRCFG_ICDCT_SHIFT) |
> >                     (JOBR_INTC_TIME_THLD << JRCFG_ICTT_SHIFT));
> >
> > +     tasklet_init(&jrp->irqtask, caam_jr_dequeue, (unsigned long)dev);
> > +
> > +     /* Connect job ring interrupt handler. */
> > +     error = devm_request_irq(dev, jrp->irq, caam_jr_interrupt, IRQF_SHARED,
> > +                              dev_name(dev), dev);
> > +     if (error) {
> > +             dev_err(dev, "can't connect JobR %d interrupt (%d)\n",
> > +                     jrp->ridx, jrp->irq);
> > +             tasklet_kill(&jrp->irqtask);
> > +             return error;
> "return error;" should be moved out the if block.
>

Sure, will do in v7.

Thanks,
Andrey Smirnov

Patch
diff mbox series

diff --git a/drivers/crypto/caam/jr.c b/drivers/crypto/caam/jr.c
index ea02f7774f7c..98e0a504322f 100644
--- a/drivers/crypto/caam/jr.c
+++ b/drivers/crypto/caam/jr.c
@@ -428,38 +428,27 @@  static int caam_jr_init(struct device *dev)
 
 	jrp = dev_get_drvdata(dev);
 
-	tasklet_init(&jrp->irqtask, caam_jr_dequeue, (unsigned long)dev);
-
-	/* Connect job ring interrupt handler. */
-	error = devm_request_irq(dev, jrp->irq, caam_jr_interrupt, IRQF_SHARED,
-				 dev_name(dev), dev);
-	if (error) {
-		dev_err(dev, "can't connect JobR %d interrupt (%d)\n",
-			jrp->ridx, jrp->irq);
-		goto out_kill_deq;
-	}
-
 	error = caam_reset_hw_jr(dev);
 	if (error)
-		goto out_kill_deq;
+		return error;
 
 	error = -ENOMEM;
 	jrp->inpring = dmam_alloc_coherent(dev, sizeof(*jrp->inpring) *
 					   JOBR_DEPTH, &inpbusaddr,
 					   GFP_KERNEL);
 	if (!jrp->inpring)
-		goto out_kill_deq;
+		return -ENOMEM;
 
 	jrp->outring = dmam_alloc_coherent(dev, sizeof(*jrp->outring) *
 					   JOBR_DEPTH, &outbusaddr,
 					   GFP_KERNEL);
 	if (!jrp->outring)
-		goto out_kill_deq;
+		return -ENOMEM;
 
 	jrp->entinfo = devm_kcalloc(dev, JOBR_DEPTH, sizeof(*jrp->entinfo),
 				    GFP_KERNEL);
 	if (!jrp->entinfo)
-		goto out_kill_deq;
+		return -ENOMEM;
 
 	for (i = 0; i < JOBR_DEPTH; i++)
 		jrp->entinfo[i].desc_addr_dma = !0;
@@ -483,10 +472,19 @@  static int caam_jr_init(struct device *dev)
 		      (JOBR_INTC_COUNT_THLD << JRCFG_ICDCT_SHIFT) |
 		      (JOBR_INTC_TIME_THLD << JRCFG_ICTT_SHIFT));
 
+	tasklet_init(&jrp->irqtask, caam_jr_dequeue, (unsigned long)dev);
+
+	/* Connect job ring interrupt handler. */
+	error = devm_request_irq(dev, jrp->irq, caam_jr_interrupt, IRQF_SHARED,
+				 dev_name(dev), dev);
+	if (error) {
+		dev_err(dev, "can't connect JobR %d interrupt (%d)\n",
+			jrp->ridx, jrp->irq);
+		tasklet_kill(&jrp->irqtask);
+		return error;
+	}
+
 	return 0;
-out_kill_deq:
-	tasklet_kill(&jrp->irqtask);
-	return error;
 }