Re: [PATCH v3.5] dw_dmac: return proper residue value
On Fri, Jan 25, 2013 at 11:30 AM, Viresh Kumar wrote: > On Fri, Jan 25, 2013 at 2:49 PM, Andy Shevchenko > wrote: >> On Fri, Jan 25, 2013 at 11:07 AM, Viresh Kumar >> wrote: >>> Who is going to right on ctlhi/lo? > > Ahh, my English :( (/s/right/write) > >> dwc_do_single_block() >> >>> we write to ctlhi/lo only when we program new >>> transfer. and that is not going to happen while we are in middle of a >>> transfer. >> >> We have got a tasklet running inside tx_status call. Isn't possible? >> tasklet runs scan_descriptors, that continues transfer in soft LLP mode. > > But we have just executed scan_descriptor() before this function and it > doesn't > look possible that transfer wasn't over then and inbetween these calls we got > an > interrupt, scheduled an tasklet and called dwc_do_single_block() :) Yeah, the keyword is "look". 1 per million cases it could be true. I think this discussion is going to the dead end. Anyone else would like to argue for one or the other opinion? I might obey your way with the commentary that with quite lower possibility we could have an issue there. -- With Best Regards, Andy Shevchenko -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3.5] dw_dmac: return proper residue value
On Fri, Jan 25, 2013 at 11:39 AM, Viresh Kumar wrote: > On 25 January 2013 15:07, Andy Shevchenko wrote: >> Yeah, the keyword is "look". 1 per million cases it could be true. I >> think this discussion is going to the dead end. >> Anyone else would like to argue for one or the other opinion? >> >> I might obey your way with the commentary that with quite lower >> possibility we could have an issue there. > > :) > Okay, lets keep your version. > > @Vinod: you are free to pick these patches now ;) I do patch v4 to gather everything together. -- With Best Regards, Andy Shevchenko -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3.5] dw_dmac: return proper residue value
On 25 January 2013 15:07, Andy Shevchenko wrote: > Yeah, the keyword is "look". 1 per million cases it could be true. I > think this discussion is going to the dead end. > Anyone else would like to argue for one or the other opinion? > > I might obey your way with the commentary that with quite lower > possibility we could have an issue there. :) Okay, lets keep your version. @Vinod: you are free to pick these patches now ;) -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3.5] dw_dmac: return proper residue value
On Fri, Jan 25, 2013 at 2:49 PM, Andy Shevchenko wrote: > On Fri, Jan 25, 2013 at 11:07 AM, Viresh Kumar > wrote: >> Who is going to right on ctlhi/lo? Ahh, my English :( (/s/right/write) > dwc_do_single_block() > >> we write to ctlhi/lo only when we program new >> transfer. and that is not going to happen while we are in middle of a >> transfer. > > We have got a tasklet running inside tx_status call. Isn't possible? > tasklet runs scan_descriptors, that continues transfer in soft LLP mode. But we have just executed scan_descriptor() before this function and it doesn't look possible that transfer wasn't over then and inbetween these calls we got an interrupt, scheduled an tasklet and called dwc_do_single_block() :) -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3.5] dw_dmac: return proper residue value
On Fri, Jan 25, 2013 at 11:07 AM, Viresh Kumar wrote: > On 25 January 2013 14:34, Andy Shevchenko wrote: >> Okay, we have to have a protection here because get_sent reads two >> registers consequentially. This means we could end up with scenario >> with threads 1 and 2 >> >> 1. read ctlhi >> 2. write ctlhi >> 2. write ctllo >> 1. read ctllo > > Who is going to right on ctlhi/lo? dwc_do_single_block() > we write to ctlhi/lo only when we program new > transfer. and that is not going to happen while we are in middle of a > transfer. We have got a tasklet running inside tx_status call. Isn't possible? tasklet runs scan_descriptors, that continues transfer in soft LLP mode. -- With Best Regards, Andy Shevchenko -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3.5] dw_dmac: return proper residue value
On 25 January 2013 14:34, Andy Shevchenko wrote: > Okay, we have to have a protection here because get_sent reads two > registers consequentially. This means we could end up with scenario > with threads 1 and 2 > > 1. read ctlhi > 2. write ctlhi > 2. write ctllo > 1. read ctllo Who is going to right on ctlhi/lo? we write to ctlhi/lo only when we program new transfer. and that is not going to happen while we are in middle of a transfer. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3.5] dw_dmac: return proper residue value
On Fri, Jan 25, 2013 at 10:56 AM, Andy Shevchenko
wrote:
> On Fri, 2013-01-25 at 09:43 +0530, Viresh Kumar wrote:
>> On Thu, Jan 24, 2013 at 6:37 PM, Andy Shevchenko
>> wrote:
>> > diff --git a/drivers/dma/dw_dmac.c b/drivers/dma/dw_dmac.c
>> > +static inline u32 dwc_get_residue(struct dw_dma_chan *dwc)
>> > +{
>> > + unsigned long flags;
>> > + u32 residue;
>> > +
>> > + spin_lock_irqsave(>lock, flags);
>> > +
>> > + residue = dwc->residue;
>>
>> you need to release the lock just here.
>>
>> > + if (test_bit(DW_DMA_IS_SOFT_LLP, >flags) && residue)
>> > + residue -= dwc_get_sent(dwc);
>>
>> why do you need lock while reading the control registers? It looks you didn't
>> get what i wanted to say earlier. We are using locks to protect some part for
>> shared data or critical sections. these are playing with dwc transfer
>> lists, etc..
>>
>> Probably we don't need a lock to read the control register as nobody can
>> guarantee that another access is not happening currently. As hardware is
>> changing this register continuously for the transfer.
>>
>> Let me know if i am missing something.
Okay, we have to have a protection here because get_sent reads two
registers consequentially. This means we could end up with scenario
with threads 1 and 2
1. read ctlhi
2. write ctlhi
2. write ctllo
1. read ctllo
--
With Best Regards,
Andy Shevchenko
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3.5] dw_dmac: return proper residue value
On Fri, 2013-01-25 at 09:43 +0530, Viresh Kumar wrote:
> On Thu, Jan 24, 2013 at 6:37 PM, Andy Shevchenko
> wrote:
> > Currently the driver returns full length of the active descriptor which is
> > wrong. We have to go throught the active descriptor and substract the
> > length of
> > each sent children in the chain from the total length along with the actual
> > data in the DMA channel registers.
> >
> > The cyclic case is not handled by this patch due to len field in the
> > descriptor
> > structure is left untouched by the original code.
> >
> > Signed-off-by: Andy Shevchenko
>
> Firstly the issue i raised earlier about not updating residue from
> tasklet or interrupt
> handler was wrong. As i had an older version of code in my mind. This got
> solved
> with following patch:
>
> commit 77bcc497c60ec62dbb84abc809a6e218d53409e9
> Author: Andy Shevchenko
> Date: Fri Jan 18 14:14:15 2013 +0200
>
> dw_dmac: move soft LLP code from tasklet to dwc_scan_descriptors
>
>
> > ---
> > diff --git a/drivers/dma/dw_dmac.c b/drivers/dma/dw_dmac.c
> > +static inline u32 dwc_get_residue(struct dw_dma_chan *dwc)
> > +{
> > + unsigned long flags;
> > + u32 residue;
> > +
> > + spin_lock_irqsave(>lock, flags);
> > +
> > + residue = dwc->residue;
>
> you need to release the lock just here.
>
> > + if (test_bit(DW_DMA_IS_SOFT_LLP, >flags) && residue)
> > + residue -= dwc_get_sent(dwc);
>
> why do you need lock while reading the control registers? It looks you didn't
> get what i wanted to say earlier. We are using locks to protect some part for
> shared data or critical sections. these are playing with dwc transfer
> lists, etc..
>
> Probably we don't need a lock to read the control register as nobody can
> guarantee that another access is not happening currently. As hardware is
> changing this register continuously for the transfer.
>
> Let me know if i am missing something.
I'm trying to find a proof of any of our opinions.
My intuitive understanding that we have to avoid a concurrency during hw
access.
>
> > + spin_unlock_irqrestore(>lock, flags);
> > + return residue;
> > +}
--
Andy Shevchenko
Intel Finland Oy
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3.5] dw_dmac: return proper residue value
On Fri, 2013-01-25 at 09:43 +0530, Viresh Kumar wrote:
On Thu, Jan 24, 2013 at 6:37 PM, Andy Shevchenko
andriy.shevche...@linux.intel.com wrote:
Currently the driver returns full length of the active descriptor which is
wrong. We have to go throught the active descriptor and substract the
length of
each sent children in the chain from the total length along with the actual
data in the DMA channel registers.
The cyclic case is not handled by this patch due to len field in the
descriptor
structure is left untouched by the original code.
Signed-off-by: Andy Shevchenko andriy.shevche...@linux.intel.com
Firstly the issue i raised earlier about not updating residue from
tasklet or interrupt
handler was wrong. As i had an older version of code in my mind. This got
solved
with following patch:
commit 77bcc497c60ec62dbb84abc809a6e218d53409e9
Author: Andy Shevchenko andriy.shevche...@linux.intel.com
Date: Fri Jan 18 14:14:15 2013 +0200
dw_dmac: move soft LLP code from tasklet to dwc_scan_descriptors
---
diff --git a/drivers/dma/dw_dmac.c b/drivers/dma/dw_dmac.c
+static inline u32 dwc_get_residue(struct dw_dma_chan *dwc)
+{
+ unsigned long flags;
+ u32 residue;
+
+ spin_lock_irqsave(dwc-lock, flags);
+
+ residue = dwc-residue;
you need to release the lock just here.
+ if (test_bit(DW_DMA_IS_SOFT_LLP, dwc-flags) residue)
+ residue -= dwc_get_sent(dwc);
why do you need lock while reading the control registers? It looks you didn't
get what i wanted to say earlier. We are using locks to protect some part for
shared data or critical sections. these are playing with dwc transfer
lists, etc..
Probably we don't need a lock to read the control register as nobody can
guarantee that another access is not happening currently. As hardware is
changing this register continuously for the transfer.
Let me know if i am missing something.
I'm trying to find a proof of any of our opinions.
My intuitive understanding that we have to avoid a concurrency during hw
access.
+ spin_unlock_irqrestore(dwc-lock, flags);
+ return residue;
+}
--
Andy Shevchenko andriy.shevche...@linux.intel.com
Intel Finland Oy
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3.5] dw_dmac: return proper residue value
On Fri, Jan 25, 2013 at 10:56 AM, Andy Shevchenko
andriy.shevche...@linux.intel.com wrote:
On Fri, 2013-01-25 at 09:43 +0530, Viresh Kumar wrote:
On Thu, Jan 24, 2013 at 6:37 PM, Andy Shevchenko
andriy.shevche...@linux.intel.com wrote:
diff --git a/drivers/dma/dw_dmac.c b/drivers/dma/dw_dmac.c
+static inline u32 dwc_get_residue(struct dw_dma_chan *dwc)
+{
+ unsigned long flags;
+ u32 residue;
+
+ spin_lock_irqsave(dwc-lock, flags);
+
+ residue = dwc-residue;
you need to release the lock just here.
+ if (test_bit(DW_DMA_IS_SOFT_LLP, dwc-flags) residue)
+ residue -= dwc_get_sent(dwc);
why do you need lock while reading the control registers? It looks you didn't
get what i wanted to say earlier. We are using locks to protect some part for
shared data or critical sections. these are playing with dwc transfer
lists, etc..
Probably we don't need a lock to read the control register as nobody can
guarantee that another access is not happening currently. As hardware is
changing this register continuously for the transfer.
Let me know if i am missing something.
Okay, we have to have a protection here because get_sent reads two
registers consequentially. This means we could end up with scenario
with threads 1 and 2
1. read ctlhi
2. write ctlhi
2. write ctllo
1. read ctllo
--
With Best Regards,
Andy Shevchenko
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3.5] dw_dmac: return proper residue value
On 25 January 2013 14:34, Andy Shevchenko andy.shevche...@gmail.com wrote: Okay, we have to have a protection here because get_sent reads two registers consequentially. This means we could end up with scenario with threads 1 and 2 1. read ctlhi 2. write ctlhi 2. write ctllo 1. read ctllo Who is going to right on ctlhi/lo? we write to ctlhi/lo only when we program new transfer. and that is not going to happen while we are in middle of a transfer. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3.5] dw_dmac: return proper residue value
On Fri, Jan 25, 2013 at 11:07 AM, Viresh Kumar viresh.ku...@linaro.org wrote: On 25 January 2013 14:34, Andy Shevchenko andy.shevche...@gmail.com wrote: Okay, we have to have a protection here because get_sent reads two registers consequentially. This means we could end up with scenario with threads 1 and 2 1. read ctlhi 2. write ctlhi 2. write ctllo 1. read ctllo Who is going to right on ctlhi/lo? dwc_do_single_block() we write to ctlhi/lo only when we program new transfer. and that is not going to happen while we are in middle of a transfer. We have got a tasklet running inside tx_status call. Isn't possible? tasklet runs scan_descriptors, that continues transfer in soft LLP mode. -- With Best Regards, Andy Shevchenko -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3.5] dw_dmac: return proper residue value
On Fri, Jan 25, 2013 at 2:49 PM, Andy Shevchenko andy.shevche...@gmail.com wrote: On Fri, Jan 25, 2013 at 11:07 AM, Viresh Kumar viresh.ku...@linaro.org wrote: Who is going to right on ctlhi/lo? Ahh, my English :( (/s/right/write) dwc_do_single_block() we write to ctlhi/lo only when we program new transfer. and that is not going to happen while we are in middle of a transfer. We have got a tasklet running inside tx_status call. Isn't possible? tasklet runs scan_descriptors, that continues transfer in soft LLP mode. But we have just executed scan_descriptor() before this function and it doesn't look possible that transfer wasn't over then and inbetween these calls we got an interrupt, scheduled an tasklet and called dwc_do_single_block() :) -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3.5] dw_dmac: return proper residue value
On 25 January 2013 15:07, Andy Shevchenko andy.shevche...@gmail.com wrote: Yeah, the keyword is look. 1 per million cases it could be true. I think this discussion is going to the dead end. Anyone else would like to argue for one or the other opinion? I might obey your way with the commentary that with quite lower possibility we could have an issue there. :) Okay, lets keep your version. @Vinod: you are free to pick these patches now ;) -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3.5] dw_dmac: return proper residue value
On Fri, Jan 25, 2013 at 11:39 AM, Viresh Kumar viresh.ku...@linaro.org wrote: On 25 January 2013 15:07, Andy Shevchenko andy.shevche...@gmail.com wrote: Yeah, the keyword is look. 1 per million cases it could be true. I think this discussion is going to the dead end. Anyone else would like to argue for one or the other opinion? I might obey your way with the commentary that with quite lower possibility we could have an issue there. :) Okay, lets keep your version. @Vinod: you are free to pick these patches now ;) I do patch v4 to gather everything together. -- With Best Regards, Andy Shevchenko -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3.5] dw_dmac: return proper residue value
On Fri, Jan 25, 2013 at 11:30 AM, Viresh Kumar viresh.ku...@linaro.org wrote: On Fri, Jan 25, 2013 at 2:49 PM, Andy Shevchenko andy.shevche...@gmail.com wrote: On Fri, Jan 25, 2013 at 11:07 AM, Viresh Kumar viresh.ku...@linaro.org wrote: Who is going to right on ctlhi/lo? Ahh, my English :( (/s/right/write) dwc_do_single_block() we write to ctlhi/lo only when we program new transfer. and that is not going to happen while we are in middle of a transfer. We have got a tasklet running inside tx_status call. Isn't possible? tasklet runs scan_descriptors, that continues transfer in soft LLP mode. But we have just executed scan_descriptor() before this function and it doesn't look possible that transfer wasn't over then and inbetween these calls we got an interrupt, scheduled an tasklet and called dwc_do_single_block() :) Yeah, the keyword is look. 1 per million cases it could be true. I think this discussion is going to the dead end. Anyone else would like to argue for one or the other opinion? I might obey your way with the commentary that with quite lower possibility we could have an issue there. -- With Best Regards, Andy Shevchenko -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3.5] dw_dmac: return proper residue value
On Thu, Jan 24, 2013 at 6:37 PM, Andy Shevchenko
wrote:
> Currently the driver returns full length of the active descriptor which is
> wrong. We have to go throught the active descriptor and substract the length
> of
> each sent children in the chain from the total length along with the actual
> data in the DMA channel registers.
>
> The cyclic case is not handled by this patch due to len field in the
> descriptor
> structure is left untouched by the original code.
>
> Signed-off-by: Andy Shevchenko
Firstly the issue i raised earlier about not updating residue from
tasklet or interrupt
handler was wrong. As i had an older version of code in my mind. This got solved
with following patch:
commit 77bcc497c60ec62dbb84abc809a6e218d53409e9
Author: Andy Shevchenko
Date: Fri Jan 18 14:14:15 2013 +0200
dw_dmac: move soft LLP code from tasklet to dwc_scan_descriptors
> ---
> diff --git a/drivers/dma/dw_dmac.c b/drivers/dma/dw_dmac.c
> +static inline u32 dwc_get_residue(struct dw_dma_chan *dwc)
> +{
> + unsigned long flags;
> + u32 residue;
> +
> + spin_lock_irqsave(>lock, flags);
> +
> + residue = dwc->residue;
you need to release the lock just here.
> + if (test_bit(DW_DMA_IS_SOFT_LLP, >flags) && residue)
> + residue -= dwc_get_sent(dwc);
why do you need lock while reading the control registers? It looks you didn't
get what i wanted to say earlier. We are using locks to protect some part for
shared data or critical sections. these are playing with dwc transfer
lists, etc..
Probably we don't need a lock to read the control register as nobody can
guarantee that another access is not happening currently. As hardware is
changing this register continuously for the transfer.
Let me know if i am missing something.
> + spin_unlock_irqrestore(>lock, flags);
> + return residue;
> +}
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3.5] dw_dmac: return proper residue value
On Thu, Jan 24, 2013 at 6:37 PM, Andy Shevchenko
andriy.shevche...@linux.intel.com wrote:
Currently the driver returns full length of the active descriptor which is
wrong. We have to go throught the active descriptor and substract the length
of
each sent children in the chain from the total length along with the actual
data in the DMA channel registers.
The cyclic case is not handled by this patch due to len field in the
descriptor
structure is left untouched by the original code.
Signed-off-by: Andy Shevchenko andriy.shevche...@linux.intel.com
Firstly the issue i raised earlier about not updating residue from
tasklet or interrupt
handler was wrong. As i had an older version of code in my mind. This got solved
with following patch:
commit 77bcc497c60ec62dbb84abc809a6e218d53409e9
Author: Andy Shevchenko andriy.shevche...@linux.intel.com
Date: Fri Jan 18 14:14:15 2013 +0200
dw_dmac: move soft LLP code from tasklet to dwc_scan_descriptors
---
diff --git a/drivers/dma/dw_dmac.c b/drivers/dma/dw_dmac.c
+static inline u32 dwc_get_residue(struct dw_dma_chan *dwc)
+{
+ unsigned long flags;
+ u32 residue;
+
+ spin_lock_irqsave(dwc-lock, flags);
+
+ residue = dwc-residue;
you need to release the lock just here.
+ if (test_bit(DW_DMA_IS_SOFT_LLP, dwc-flags) residue)
+ residue -= dwc_get_sent(dwc);
why do you need lock while reading the control registers? It looks you didn't
get what i wanted to say earlier. We are using locks to protect some part for
shared data or critical sections. these are playing with dwc transfer
lists, etc..
Probably we don't need a lock to read the control register as nobody can
guarantee that another access is not happening currently. As hardware is
changing this register continuously for the transfer.
Let me know if i am missing something.
+ spin_unlock_irqrestore(dwc-lock, flags);
+ return residue;
+}
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
