Re: [PATCH 2/2] talitos: Freescale integrated security engine (SEC) driver
On Sat, 31 May 2008 13:59:02 +0400 Evgeniy Polyakov [EMAIL PROTECTED] wrote: Hi. On Fri, May 30, 2008 at 05:19:30PM -0500, Kim Phillips ([EMAIL PROTECTED]) wrote: ok, I see what you are saying now; if a channel gets done during talitos_done processing, it'll trigger an interrupt and reset priv-status, leaving the tasklet in the dark as to which channel has done status, depending on how many channel dones it has already processed. I think the only solution here is to call flush_channel on each channel, regardless of the bits in the interrupt status - I'll send out a patch shortly. Out of curiosity, what is number of channels? I had simialar issue with typically four but some parts only have one. HIFN crypto driver and limited number of descriptor to 80 iirc, since with that number HIFN traversal did not show perfromance degradataion on Ghz x86. ok, I've been focusing on correctness for the time being. callback, during that time cached status and priv itself (and tail like in two simultaneous flushes) could change (or not?) I think you're talking about a different 'status' here; flush_channel's local 'status' doesn't resemble priv-status bits in any way, it looks at the descriptor header writeback bits for done status, on a per descriptor basis. It forwards this descriptor done vs. error status to the callback. priv itself won't change; it's uniquely associated to the device. I meant descriptor hdr value accessed via it - can it be checked in tasklet under the lock and in submit path without? Can they correlate somehow? I believe the check for a non-null request-desc (under lock) before the hdr value is accessed ensures this doesn't happen. Kim ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH 2/2] talitos: Freescale integrated security engine (SEC) driver
On Mon, Jun 02, 2008 at 09:27:01AM -0500, Kim Phillips ([EMAIL PROTECTED]) wrote: I meant descriptor hdr value accessed via it - can it be checked in tasklet under the lock and in submit path without? Can they correlate somehow? I believe the check for a non-null request-desc (under lock) before the hdr value is accessed ensures this doesn't happen. But can it be changed? You write to it without lock, but read under the one (different for each channel though), so it attracted attention. -- Evgeniy Polyakov ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH 2/2] talitos: Freescale integrated security engine (SEC) driver
On Mon, 2 Jun 2008 20:00:12 +0400 Evgeniy Polyakov [EMAIL PROTECTED] wrote: On Mon, Jun 02, 2008 at 09:27:01AM -0500, Kim Phillips ([EMAIL PROTECTED]) wrote: I meant descriptor hdr value accessed via it - can it be checked in tasklet under the lock and in submit path without? Can they correlate somehow? I believe the check for a non-null request-desc (under lock) before the hdr value is accessed ensures this doesn't happen. But can it be changed? You write to it without lock, but read under the one (different for each channel though), so it attracted attention. can you point where in the code your concern is? desc is assigned under head lock and cleared under tail lock, both after an smp_wmb. hdr data is assigned before desc is written, and read after desc is found to be !NULL (i.e, hdr access is governed by if (desc)). head and tail indices get advanced each within their corresponding locks. So afaict there shouldn't be a case where data pointed to by desc can be accessed by both the consumer and the producer at any one point in time. Does that help? Kim ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH 2/2] talitos: Freescale integrated security engine (SEC) driver
On Mon, Jun 02, 2008 at 11:50:21AM -0500, Kim Phillips ([EMAIL PROTECTED]) wrote: But can it be changed? You write to it without lock, but read under the one (different for each channel though), so it attracted attention. can you point where in the code your concern is? talitos_submit() writes to hdr (initially I think?) without locks. It is read in flush_channel() under tail lock, but then it is dropped, so I rised a question, if it can be modified during that time, since if it can, status value, calculated from it, can be different and thus error check result can be false. Or this is not an issue? -- Evgeniy Polyakov ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH 2/2] talitos: Freescale integrated security engine (SEC) driver
On Mon, 2 Jun 2008 21:57:51 +0400 Evgeniy Polyakov [EMAIL PROTECTED] wrote: On Mon, Jun 02, 2008 at 11:50:21AM -0500, Kim Phillips ([EMAIL PROTECTED]) wrote: But can it be changed? You write to it without lock, but read under the one (different for each channel though), so it attracted attention. can you point where in the code your concern is? talitos_submit() writes to hdr (initially I think?) without locks. ok, the desc whose hdr is being written was allocated by talitos_submit's caller, and it hasn't been made part of the circular buffer at that point. It becomes a part of the buffer (eligible for contention with the consumer side) when desc is assigned. It is read in flush_channel() under tail lock, but then it is dropped, so I rised a question, if it can be modified during that time, since if it can, status value, calculated from it, can be different and thus error check result can be false. Or this is not an issue? it would be an issue if flush_cannel didn't save off the data required to call the callback with in saved_req. flush_channel does this on purpose to be able to call the callback outside of lock (as is commented). Note desc gets assigned NULL prior to releasing the lock, after copying its contents to saved_req. Kim ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH 2/2] talitos: Freescale integrated security engine (SEC) driver
Hi. On Mon, Jun 02, 2008 at 02:06:03PM -0500, Kim Phillips ([EMAIL PROTECTED]) wrote: it would be an issue if flush_cannel didn't save off the data required to call the callback with in saved_req. flush_channel does this on purpose to be able to call the callback outside of lock (as is commented). Note desc gets assigned NULL prior to releasing the lock, after copying its contents to saved_req. Excellent! It looks like there are no more issues and second version fixed found problem place. -- Evgeniy Polyakov ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH 2/2] talitos: Freescale integrated security engine (SEC) driver
Hi. On Fri, May 30, 2008 at 05:19:30PM -0500, Kim Phillips ([EMAIL PROTECTED]) wrote: ok, I see what you are saying now; if a channel gets done during talitos_done processing, it'll trigger an interrupt and reset priv-status, leaving the tasklet in the dark as to which channel has done status, depending on how many channel dones it has already processed. I think the only solution here is to call flush_channel on each channel, regardless of the bits in the interrupt status - I'll send out a patch shortly. Out of curiosity, what is number of channels? I had simialar issue with HIFN crypto driver and limited number of descriptor to 80 iirc, since with that number HIFN traversal did not show perfromance degradataion on Ghz x86. callback, during that time cached status and priv itself (and tail like in two simultaneous flushes) could change (or not?) I think you're talking about a different 'status' here; flush_channel's local 'status' doesn't resemble priv-status bits in any way, it looks at the descriptor header writeback bits for done status, on a per descriptor basis. It forwards this descriptor done vs. error status to the callback. priv itself won't change; it's uniquely associated to the device. I meant descriptor hdr value accessed via it - can it be checked in tasklet under the lock and in submit path without? Can they correlate somehow? -- Evgeniy Polyakov ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH 2/2] talitos: Freescale integrated security engine (SEC) driver
Hi.
On Thu, May 29, 2008 at 02:12:50PM -0500, Kim Phillips ([EMAIL PROTECTED])
wrote:
+static irqreturn_t talitos_interrupt(int irq, void *data)
+{
+ struct device *dev = data;
+ struct talitos_private *priv = dev_get_drvdata(dev);
+
+ priv-status = in_be32(priv-reg + TALITOS_ISR);
+ priv-status_lo = in_be32(priv-reg + TALITOS_ISR_LO);
+
+ if (unlikely(priv-status ~TALITOS_ISR_CHDONE)) {
+ talitos_error((unsigned long)data);
+ /* ack */
+ out_be32(priv-reg + TALITOS_ICR, priv-status);
+ out_be32(priv-reg + TALITOS_ICR_LO, priv-status_lo);
+ }
+ else
+ {
+ /* ack */
+ out_be32(priv-reg + TALITOS_ICR, priv-status);
+ out_be32(priv-reg + TALITOS_ICR_LO, priv-status_lo);
+
+ if (likely(priv-status TALITOS_ISR_CHDONE))
+ tasklet_schedule(priv-done_task);
+ }
+
+ return (priv-status || priv-status_lo) ? IRQ_HANDLED : IRQ_NONE;
+}
Don't you want to protect against simultaneous access to register space
from different CPUs? Or it is single processor board only?
--
Evgeniy Polyakov
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH 2/2] talitos: Freescale integrated security engine (SEC) driver
On Fri, 30 May 2008 22:09:04 +0400
Evgeniy Polyakov [EMAIL PROTECTED] wrote:
Hi.
On Thu, May 29, 2008 at 02:12:50PM -0500, Kim Phillips ([EMAIL PROTECTED])
wrote:
+static irqreturn_t talitos_interrupt(int irq, void *data)
+{
+ struct device *dev = data;
+ struct talitos_private *priv = dev_get_drvdata(dev);
+
+ priv-status = in_be32(priv-reg + TALITOS_ISR);
+ priv-status_lo = in_be32(priv-reg + TALITOS_ISR_LO);
+
+ if (unlikely(priv-status ~TALITOS_ISR_CHDONE)) {
+ talitos_error((unsigned long)data);
+ /* ack */
+ out_be32(priv-reg + TALITOS_ICR, priv-status);
+ out_be32(priv-reg + TALITOS_ICR_LO, priv-status_lo);
+ }
+ else
+ {
+ /* ack */
+ out_be32(priv-reg + TALITOS_ICR, priv-status);
+ out_be32(priv-reg + TALITOS_ICR_LO, priv-status_lo);
+
+ if (likely(priv-status TALITOS_ISR_CHDONE))
+ tasklet_schedule(priv-done_task);
+ }
+
+ return (priv-status || priv-status_lo) ? IRQ_HANDLED : IRQ_NONE;
+}
Don't you want to protect against simultaneous access to register space
from different CPUs? Or it is single processor board only?
Doesn't linux mask the IRQ line for the interrupt currently being
serviced, and on all processors?
Kim
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH 2/2] talitos: Freescale integrated security engine (SEC) driver
Kim Phillips wrote: On Fri, 30 May 2008 22:09:04 +0400 Evgeniy Polyakov [EMAIL PROTECTED] wrote: Don't you want to protect against simultaneous access to register space from different CPUs? Or it is single processor board only? Doesn't linux mask the IRQ line for the interrupt currently being serviced, and on all processors? Yes. Could there be interference from non-interrupt driver code on another cpu (or interrupted code), though? -Scott ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH 2/2] talitos: Freescale integrated security engine (SEC) driver
On Fri, May 30, 2008 at 02:41:17PM -0500, Scott Wood ([EMAIL PROTECTED]) wrote: Don't you want to protect against simultaneous access to register space from different CPUs? Or it is single processor board only? Doesn't linux mask the IRQ line for the interrupt currently being serviced, and on all processors? Yes. Could there be interference from non-interrupt driver code on another cpu (or interrupted code), though? Yes, that register space can be assigned from non-interrupt path on different cpu. I saw spin_lock_irqsave() is used in some other places, but not in interrupt handler itself. -- Evgeniy Polyakov ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH 2/2] talitos: Freescale integrated security engine (SEC) driver
On Fri, 30 May 2008 14:41:17 -0500 Scott Wood [EMAIL PROTECTED] wrote: Kim Phillips wrote: On Fri, 30 May 2008 22:09:04 +0400 Evgeniy Polyakov [EMAIL PROTECTED] wrote: Don't you want to protect against simultaneous access to register space from different CPUs? Or it is single processor board only? Doesn't linux mask the IRQ line for the interrupt currently being serviced, and on all processors? Yes. Could there be interference from non-interrupt driver code on another cpu (or interrupted code), though? not that I can see - the fetch fifo register writes are protected with per-channel spinlocks. Kim ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH 2/2] talitos: Freescale integrated security engine (SEC) driver
Kim Phillips wrote: On Fri, 30 May 2008 14:41:17 -0500 Scott Wood [EMAIL PROTECTED] wrote: Kim Phillips wrote: On Fri, 30 May 2008 22:09:04 +0400 Evgeniy Polyakov [EMAIL PROTECTED] wrote: Don't you want to protect against simultaneous access to register space from different CPUs? Or it is single processor board only? Doesn't linux mask the IRQ line for the interrupt currently being serviced, and on all processors? Yes. Could there be interference from non-interrupt driver code on another cpu (or interrupted code), though? not that I can see - the fetch fifo register writes are protected with per-channel spinlocks. But you don't take the spinlocks from the interrupt handler. -Scott ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH 2/2] talitos: Freescale integrated security engine (SEC) driver
Kim Phillips wrote: On Fri, 30 May 2008 15:19:43 -0500 Scott Wood [EMAIL PROTECTED] wrote: Kim Phillips wrote: On Fri, 30 May 2008 14:41:17 -0500 Scott Wood [EMAIL PROTECTED] wrote: Kim Phillips wrote: On Fri, 30 May 2008 22:09:04 +0400 Evgeniy Polyakov [EMAIL PROTECTED] wrote: Don't you want to protect against simultaneous access to register space from different CPUs? Or it is single processor board only? Doesn't linux mask the IRQ line for the interrupt currently being serviced, and on all processors? Yes. Could there be interference from non-interrupt driver code on another cpu (or interrupted code), though? not that I can see - the fetch fifo register writes are protected with per-channel spinlocks. But you don't take the spinlocks from the interrupt handler. why can't fetch fifo registers be written the same time the ISR is being accessed? I don't know -- you brought them up. My question was whether there's anything that the ISR touches that is also touched by non-ISR code. -Scott ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH 2/2] talitos: Freescale integrated security engine (SEC) driver
On Fri, 30 May 2008 15:19:43 -0500 Scott Wood [EMAIL PROTECTED] wrote: Kim Phillips wrote: On Fri, 30 May 2008 14:41:17 -0500 Scott Wood [EMAIL PROTECTED] wrote: Kim Phillips wrote: On Fri, 30 May 2008 22:09:04 +0400 Evgeniy Polyakov [EMAIL PROTECTED] wrote: Don't you want to protect against simultaneous access to register space from different CPUs? Or it is single processor board only? Doesn't linux mask the IRQ line for the interrupt currently being serviced, and on all processors? Yes. Could there be interference from non-interrupt driver code on another cpu (or interrupted code), though? not that I can see - the fetch fifo register writes are protected with per-channel spinlocks. But you don't take the spinlocks from the interrupt handler. why can't fetch fifo registers be written the same time the ISR is being accessed? Kim ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH 2/2] talitos: Freescale integrated security engine (SEC) driver
On Fri, 30 May 2008 15:36:50 -0500 Scott Wood [EMAIL PROTECTED] wrote: Kim Phillips wrote: On Fri, 30 May 2008 15:19:43 -0500 Scott Wood [EMAIL PROTECTED] wrote: Kim Phillips wrote: On Fri, 30 May 2008 14:41:17 -0500 Scott Wood [EMAIL PROTECTED] wrote: Kim Phillips wrote: On Fri, 30 May 2008 22:09:04 +0400 Evgeniy Polyakov [EMAIL PROTECTED] wrote: Don't you want to protect against simultaneous access to register space from different CPUs? Or it is single processor board only? Doesn't linux mask the IRQ line for the interrupt currently being serviced, and on all processors? Yes. Could there be interference from non-interrupt driver code on another cpu (or interrupted code), though? not that I can see - the fetch fifo register writes are protected with per-channel spinlocks. But you don't take the spinlocks from the interrupt handler. why can't fetch fifo registers be written the same time the ISR is being accessed? I don't know -- you brought them up. My question was whether there's anything that the ISR touches that is also touched by non-ISR code. sorry, by ISR I meant interrupt status registers. but I can't tell where the suspected simultaneous accesses are. Evgeniy, can you point out the register accesses you're talking about? Kim ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH 2/2] talitos: Freescale integrated security engine (SEC) driver
On Fri, May 30, 2008 at 03:48:20PM -0500, Kim Phillips ([EMAIL PROTECTED]) wrote: sorry, by ISR I meant interrupt status registers. but I can't tell where the suspected simultaneous accesses are. Evgeniy, can you point out the register accesses you're talking about? priv-status is accessed from tasklets, although readonly, but that rises a red flag... Also callback invocation tasklet drops the lock around callback, during that time cached status and priv itself (and tail like in two simultaneous flushes) could change (or not?) -- Evgeniy Polyakov ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH 2/2] talitos: Freescale integrated security engine (SEC) driver
On Sat, 31 May 2008 01:12:08 +0400 Evgeniy Polyakov [EMAIL PROTECTED] wrote: On Fri, May 30, 2008 at 03:48:20PM -0500, Kim Phillips ([EMAIL PROTECTED]) wrote: sorry, by ISR I meant interrupt status registers. but I can't tell where the suspected simultaneous accesses are. Evgeniy, can you point out the register accesses you're talking about? priv-status is accessed from tasklets, although readonly, but that rises a red flag... Also callback invocation tasklet drops the lock around ok, I see what you are saying now; if a channel gets done during talitos_done processing, it'll trigger an interrupt and reset priv-status, leaving the tasklet in the dark as to which channel has done status, depending on how many channel dones it has already processed. I think the only solution here is to call flush_channel on each channel, regardless of the bits in the interrupt status - I'll send out a patch shortly. callback, during that time cached status and priv itself (and tail like in two simultaneous flushes) could change (or not?) I think you're talking about a different 'status' here; flush_channel's local 'status' doesn't resemble priv-status bits in any way, it looks at the descriptor header writeback bits for done status, on a per descriptor basis. It forwards this descriptor done vs. error status to the callback. priv itself won't change; it's uniquely associated to the device. Kim ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
