| Message ID | 20190703081327.17505-4-andrew.smirnov@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Herbert Xu |
| Headers | show |
| Series | crypto: caam - Add i.MX8MQ support | expand |
On 7/3/2019 11:14 AM, Andrey Smirnov wrote: > Move tasklet_init() call further down in order to simplify error path > cleanup. No functional change intended. > > diff --git a/drivers/crypto/caam/jr.c b/drivers/crypto/caam/jr.c > index 4b25b2fa3d02..a7ca2bbe243f 100644 > --- a/drivers/crypto/caam/jr.c > +++ b/drivers/crypto/caam/jr.c > @@ -441,15 +441,13 @@ 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 = request_irq(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; > + return error; > } The caam_jr_interrupt handler can schedule the tasklet so it makes sense to have it be initialized ahead of request_irq. In theory it's possible for an interrupt to be triggered immediately when request_irq is called. I'm not very familiar with the CAAM ip, can you ensure no interrupts are pending in HW at probe time? The "no functional change" part is not obvious. -- Regards, Leonard
On Wed, Jul 3, 2019 at 6:51 AM Leonard Crestez <leonard.crestez@nxp.com> wrote: > > On 7/3/2019 11:14 AM, Andrey Smirnov wrote: > > Move tasklet_init() call further down in order to simplify error path > > cleanup. No functional change intended. > > > > diff --git a/drivers/crypto/caam/jr.c b/drivers/crypto/caam/jr.c > > index 4b25b2fa3d02..a7ca2bbe243f 100644 > > --- a/drivers/crypto/caam/jr.c > > +++ b/drivers/crypto/caam/jr.c > > @@ -441,15 +441,13 @@ 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 = request_irq(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; > > + return error; > > } > > The caam_jr_interrupt handler can schedule the tasklet so it makes sense > to have it be initialized ahead of request_irq. In theory it's possible > for an interrupt to be triggered immediately when request_irq is called. > > I'm not very familiar with the CAAM ip, can you ensure no interrupts are > pending in HW at probe time? The "no functional change" part is not obvious. > Said tasklet will use both jrp->outring and jrp->entinfo array initialized after IRQ request call in both versions of the code (before/after this patch). AFAICT, the only case where this patch would change initialization safety of the original code is if JR was scheduled somehow while ORSFx is 0 (no jobs done), which I don't think is possible. Thanks, Andrey Smirnov
On 7/3/2019 8:14 PM, Andrey Smirnov wrote: > On Wed, Jul 3, 2019 at 6:51 AM Leonard Crestez <leonard.crestez@nxp.com> wrote: >> On 7/3/2019 11:14 AM, Andrey Smirnov wrote: >>> Move tasklet_init() call further down in order to simplify error path >>> cleanup. No functional change intended. >>> >>> diff --git a/drivers/crypto/caam/jr.c b/drivers/crypto/caam/jr.c >>> index 4b25b2fa3d02..a7ca2bbe243f 100644 >>> --- a/drivers/crypto/caam/jr.c >>> +++ b/drivers/crypto/caam/jr.c >>> @@ -441,15 +441,13 @@ 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 = request_irq(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; >>> + return error; >>> } >> >> The caam_jr_interrupt handler can schedule the tasklet so it makes sense >> to have it be initialized ahead of request_irq. In theory it's possible >> for an interrupt to be triggered immediately when request_irq is called. >> >> I'm not very familiar with the CAAM ip, can you ensure no interrupts are >> pending in HW at probe time? The "no functional change" part is not obvious. >> > > Said tasklet will use both jrp->outring and jrp->entinfo array > initialized after IRQ request call in both versions of the code > (before/after this patch). AFAICT, the only case where this patch > would change initialization safety of the original code is if JR was > scheduled somehow while ORSFx is 0 (no jobs done), which I don't think > is possible. I took a second look at caam_jr_init and there is apparently a whole bunch of other reset/init stuff done after request_irq. For example caam_reset_hw_jr is done after request_irq and masks interrupts? What I'd expect is that request_irq is done last after all other initialization is performed. But I'm not familiar with how CAAM JRs work so feel free to ignore this. -- Regards, Leonard
On 7/4/2019 2:45 AM, Leonard Crestez wrote: > On 7/3/2019 8:14 PM, Andrey Smirnov wrote: >> On Wed, Jul 3, 2019 at 6:51 AM Leonard Crestez <leonard.crestez@nxp.com> wrote: >>> On 7/3/2019 11:14 AM, Andrey Smirnov wrote: >>>> Move tasklet_init() call further down in order to simplify error path >>>> cleanup. No functional change intended. >>>> >>>> diff --git a/drivers/crypto/caam/jr.c b/drivers/crypto/caam/jr.c >>>> index 4b25b2fa3d02..a7ca2bbe243f 100644 >>>> --- a/drivers/crypto/caam/jr.c >>>> +++ b/drivers/crypto/caam/jr.c >>>> @@ -441,15 +441,13 @@ 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 = request_irq(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; >>>> + return error; >>>> } >>> >>> The caam_jr_interrupt handler can schedule the tasklet so it makes sense >>> to have it be initialized ahead of request_irq. In theory it's possible >>> for an interrupt to be triggered immediately when request_irq is called. >>> >>> I'm not very familiar with the CAAM ip, can you ensure no interrupts are >>> pending in HW at probe time? The "no functional change" part is not obvious. >>> Actually there is a previous report (in i.MX BSP) of CAAM job ring generating an interrupt at probe time, between request_irq() and reset: https://source.codeaurora.org/external/imx/linux-imx/commit/drivers/crypto/caam?h=imx_4.14.98_2.0.0_ga&id=aa7b3f51971ec1f60f41fe8ea71870b215376b8a So yes, there might be cases when interrupts are pending. >> >> Said tasklet will use both jrp->outring and jrp->entinfo array >> initialized after IRQ request call in both versions of the code >> (before/after this patch). AFAICT, the only case where this patch >> would change initialization safety of the original code is if JR was >> scheduled somehow while ORSFx is 0 (no jobs done), which I don't think >> is possible. > > I took a second look at caam_jr_init and there is apparently a whole > bunch of other reset/init stuff done after request_irq. For example > caam_reset_hw_jr is done after request_irq and masks interrupts? > > What I'd expect is that request_irq is done last after all other > initialization is performed. But I'm not familiar with how CAAM JRs work > so feel free to ignore this. > There's some history here... (which is in contradiction with above-mentioned report). Commit 9620fd959fb1 ("crypto: caam - handle interrupt lines shared across rings") moved request_irq() before JR reset: " - resetting a job ring triggers an interrupt, so move request_irq prior to jr_reset to avoid 'got IRQ but nobody cared' messages. " but IIUC that ("resetting a job ring triggers an interrupt") was actually due to disabling the IRQ line using disable_irq() instead of masking the interrupt in HW using JRCFGR_LS[IMSK]. The way JR reset sequence is implemented now - in caam_reset_hw_jr() - should not trigger any interrupt. Thus, it should be safe to move request_irq() after everything is set up, at the end of probing. My suggestion is to move both tasklet_init() and request_irq() at the bottom of the probe callback. However, I'd say this is a fix that should be marked accordingly and should be posted either separately, or at the top of the patch set. Thanks, Horia
diff --git a/drivers/crypto/caam/jr.c b/drivers/crypto/caam/jr.c index 4b25b2fa3d02..a7ca2bbe243f 100644 --- a/drivers/crypto/caam/jr.c +++ b/drivers/crypto/caam/jr.c @@ -441,15 +441,13 @@ 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 = request_irq(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; + return error; } error = caam_reset_hw_jr(dev); @@ -471,6 +469,8 @@ static int caam_jr_init(struct device *dev) if (!jrp->entinfo) goto out_free_outring; + tasklet_init(&jrp->irqtask, caam_jr_dequeue, (unsigned long)dev); + for (i = 0; i < JOBR_DEPTH; i++) jrp->entinfo[i].desc_addr_dma = !0; @@ -504,8 +504,6 @@ static int caam_jr_init(struct device *dev) dev_err(dev, "can't allocate job rings for %d\n", jrp->ridx); out_free_irq: free_irq(jrp->irq, dev); -out_kill_deq: - tasklet_kill(&jrp->irqtask); return error; }
Move tasklet_init() call further down in order to simplify error path cleanup. 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 | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-)