[PATCH 1/4] ARM: PL330: Don't call the callbacks if there isn't any active transfer

[PATCH 1/4] ARM: PL330: Don't call the callbacks if there isn't any active transfer

[Javi Merino]

If there are no transactions running it is not only wrong but
potentially dangerous to call the callbacks. There can be a stale
struct pl330_req pointer from a previous run, but the memory may be free
already. This patch fixes this, and the callbacks are only called if
there were active transfers when pl330_release_channel() is called.

Signed-off-by: Javi Merino <javi.merino@arm.com>
Cc: Jassi Brar <jassi.brar@samsung.com>
---
 arch/arm/common/pl330.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/arch/arm/common/pl330.c b/arch/arm/common/pl330.c
index 7129cfb..842bab0 100644
--- a/arch/arm/common/pl330.c
+++ b/arch/arm/common/pl330.c
@@ -1691,8 +1691,10 @@ void pl330_release_channel(void *ch_id)
 
 	_stop(thrd);
 
-	_callback(thrd->req[1 - thrd->lstenq].r, PL330_ERR_ABORT);
-	_callback(thrd->req[thrd->lstenq].r, PL330_ERR_ABORT);
+	if (!IS_FREE(&thrd->req[1 - thrd->lstenq]))
+		_callback(thrd->req[1 - thrd->lstenq].r, PL330_ERR_ABORT);
+	if (!IS_FREE(&thrd->req[thrd->lstenq]))
+		_callback(thrd->req[thrd->lstenq].r, PL330_ERR_ABORT);
 
 	pl330 = thrd->dmac;
 
-- 
1.7.0.4

[PATCH 2/4] ARM: PL330: Only register usable channels

[Javi Merino]

When the manager is running non-secure, the only channels that can
issue interrupts are the ones that have a 1 in their corresponding bit
in Configuration Register 3. The other ones will generate an abort
when trying to signal the end of the transaction so they are useless
in non-secure mode.

Signed-off-by: Javi Merino <javi.merino@arm.com>
Cc: Jassi Brar <jassi.brar@samsung.com>
---
 arch/arm/common/pl330.c |    8 +++++++-
 1 files changed, 7 insertions(+), 1 deletions(-)

diff --git a/arch/arm/common/pl330.c b/arch/arm/common/pl330.c
index 842bab0..590fd57 100644
--- a/arch/arm/common/pl330.c
+++ b/arch/arm/common/pl330.c
@@ -1623,6 +1623,11 @@ static inline int _alloc_event(struct pl330_thread *thrd)
 	return -1;
 }
 
+static bool is_non_secure_chan(const struct pl330_info *pi, int i)
+{
+	return pi->pcfg.irq_ns & (1 << i);
+}
+
 /* Upon success, returns IdentityToken for the
  * allocated channel, NULL otherwise.
  */
@@ -1647,7 +1652,8 @@ void *pl330_request_channel(const struct pl330_info *pi)
 
 	for (i = 0; i < chans; i++) {
 		thrd = &pl330->channels[i];
-		if (thrd->free) {
+		if ((thrd->free) && (!_manager_ns(thrd) ||
+					is_non_secure_chan(pi, i))) {
 			thrd->ev = _alloc_event(thrd);
 			if (thrd->ev >= 0) {
 				thrd->free = false;
-- 
1.7.0.4

[PATCH 3/4] ARM: PL330: Fix the size of the dst_cache_ctrl field

[Javi Merino]

dst_cache_ctrl affects bits 3, 1 and 0 of AWCACHE but it is a 3-bit
field in the Channel Control Register (see Table 3-21 of the DMA-330
Technical Reference Manual) and should be programmed as such.

Signed-off-by: Javi Merino <javi.merino@arm.com>
Cc: Jassi Brar <jassi.brar@samsung.com>
---
 arch/arm/include/asm/hardware/pl330.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/arch/arm/include/asm/hardware/pl330.h b/arch/arm/include/asm/hardware/pl330.h
index 575fa81..c182138 100644
--- a/arch/arm/include/asm/hardware/pl330.h
+++ b/arch/arm/include/asm/hardware/pl330.h
@@ -41,7 +41,7 @@ enum pl330_dstcachectrl {
 	DCCTRL1, /* Bufferable only */
 	DCCTRL2, /* Cacheable, but do not allocate */
 	DCCTRL3, /* Cacheable and bufferable, but do not allocate */
-	DINVALID1 = 8,
+	DINVALID1,              /* AWCACHE = 0x1000 */
 	DINVALID2,
 	DCCTRL6, /* Cacheable write-through, allocate on writes only */
 	DCCTRL7, /* Cacheable write-back, allocate on writes only */
-- 
1.7.0.4

[PATCH 4/4] ARM: PL330: Fix typo in _prepare_ccr()

[Javi Merino]

scctl should be shifted by CC_SRCCTRL_SHFT and dcctl by
CC_DSTCCTRL_SHFT, not the other way round.

Signed-off-by: Javi Merino <javi.merino@arm.com>
Cc: Jassi Brar <jassi.brar@samsung.com>
---
 arch/arm/common/pl330.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/arm/common/pl330.c b/arch/arm/common/pl330.c
index 590fd57..1817963 100644
--- a/arch/arm/common/pl330.c
+++ b/arch/arm/common/pl330.c
@@ -1211,8 +1211,8 @@ static inline u32 _prepare_ccr(const struct pl330_reqcfg *rqc)
 	ccr |= (rqc->brst_size << CC_SRCBRSTSIZE_SHFT);
 	ccr |= (rqc->brst_size << CC_DSTBRSTSIZE_SHFT);
 
-	ccr |= (rqc->dcctl << CC_SRCCCTRL_SHFT);
-	ccr |= (rqc->scctl << CC_DSTCCTRL_SHFT);
+	ccr |= (rqc->scctl << CC_SRCCCTRL_SHFT);
+	ccr |= (rqc->dcctl << CC_DSTCCTRL_SHFT);
 
 	ccr |= (rqc->swap << CC_SWAP_SHFT);
 
-- 
1.7.0.4

[PATCH 1/4] ARM: PL330: Don't call the callbacks if there isn't any active transfer

[Jassi Brar]

On 2 November 2011 20:00, Javi Merino <javi.merino@arm.com> wrote:
> If there are no transactions running it is not only wrong but
> potentially dangerous to call the callbacks.
>
The idea was to have dmac drivers not touch the xfer after it has been submitted
and before the callback is made upon successful transmission or abort/flush.

> There can be a stale struct pl330_req pointer from a previous run, but the
> memory may be free already.
>
Sorry I am unable to fathom the scenario. The pl330_request_channel resets
both pl330_req pointers. Maybe some real failure you saw, could help
me understand.
Thnx.

[PATCH 2/4] ARM: PL330: Only register usable channels

[Jassi Brar]

On 2 November 2011 20:00, Javi Merino <javi.merino@arm.com> wrote:
> When the manager is running non-secure, the only channels that can
> issue interrupts are the ones that have a 1 in their corresponding bit
> in Configuration Register 3. The other ones will generate an abort
> when trying to signal the end of the transaction so they are useless
> in non-secure mode.
>
> Signed-off-by: Javi Merino <javi.merino@arm.com>
> Cc: Jassi Brar <jassi.brar@samsung.com>
> ---
> ?arch/arm/common/pl330.c | ? ?8 +++++++-
> ?1 files changed, 7 insertions(+), 1 deletions(-)
>
> diff --git a/arch/arm/common/pl330.c b/arch/arm/common/pl330.c
> index 842bab0..590fd57 100644
> --- a/arch/arm/common/pl330.c
> +++ b/arch/arm/common/pl330.c
> @@ -1623,6 +1623,11 @@ static inline int _alloc_event(struct pl330_thread *thrd)
> ? ? ? ?return -1;
> ?}
>
> +static bool is_non_secure_chan(const struct pl330_info *pi, int i)
> +{
> + ? ? ? return pi->pcfg.irq_ns & (1 << i);
> +}
> +
> ?/* Upon success, returns IdentityToken for the
> ?* allocated channel, NULL otherwise.
> ?*/
> @@ -1647,7 +1652,8 @@ void *pl330_request_channel(const struct pl330_info *pi)
>
> ? ? ? ?for (i = 0; i < chans; i++) {
> ? ? ? ? ? ? ? ?thrd = &pl330->channels[i];
> - ? ? ? ? ? ? ? if (thrd->free) {
> + ? ? ? ? ? ? ? if ((thrd->free) && (!_manager_ns(thrd) ||
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? is_non_secure_chan(pi, i))) {
> ? ? ? ? ? ? ? ? ? ? ? ?thrd->ev = _alloc_event(thrd);
> ? ? ? ? ? ? ? ? ? ? ? ?if (thrd->ev >= 0) {
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?thrd->free = false;
>
Acked-by: Jassi Brar <jassisinghbrar@gmail.com>

Though _chan_ns(pi, i) would have looked better than is_non_secure_chan(pi, i)

[PATCH 3/4] ARM: PL330: Fix the size of the dst_cache_ctrl field

[Jassi Brar]

On 2 November 2011 20:00, Javi Merino <javi.merino@arm.com> wrote:
> dst_cache_ctrl affects bits 3, 1 and 0 of AWCACHE but it is a 3-bit
> field in the Channel Control Register (see Table 3-21 of the DMA-330
> Technical Reference Manual) and should be programmed as such.
>
> Signed-off-by: Javi Merino <javi.merino@arm.com>
> Cc: Jassi Brar <jassi.brar@samsung.com>
> ---
> ?arch/arm/include/asm/hardware/pl330.h | ? ?2 +-
> ?1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/arch/arm/include/asm/hardware/pl330.h b/arch/arm/include/asm/hardware/pl330.h
> index 575fa81..c182138 100644
> --- a/arch/arm/include/asm/hardware/pl330.h
> +++ b/arch/arm/include/asm/hardware/pl330.h
> @@ -41,7 +41,7 @@ enum pl330_dstcachectrl {
> ? ? ? ?DCCTRL1, /* Bufferable only */
> ? ? ? ?DCCTRL2, /* Cacheable, but do not allocate */
> ? ? ? ?DCCTRL3, /* Cacheable and bufferable, but do not allocate */
> - ? ? ? DINVALID1 = 8,
> + ? ? ? DINVALID1, ? ? ? ? ? ? ?/* AWCACHE = 0x1000 */
> ? ? ? ?DINVALID2,
> ? ? ? ?DCCTRL6, /* Cacheable write-through, allocate on writes only */
> ? ? ? ?DCCTRL7, /* Cacheable write-back, allocate on writes only */
>
Acked-by: Jassi Brar <jassisinghbrar@gmail.com>

[PATCH 4/4] ARM: PL330: Fix typo in _prepare_ccr()

[Jassi Brar]

On 2 November 2011 20:00, Javi Merino <javi.merino@arm.com> wrote:
> scctl should be shifted by CC_SRCCTRL_SHFT and dcctl by
> CC_DSTCCTRL_SHFT, not the other way round.
>
> Signed-off-by: Javi Merino <javi.merino@arm.com>
> Cc: Jassi Brar <jassi.brar@samsung.com>
> ---
> ?arch/arm/common/pl330.c | ? ?4 ++--
> ?1 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm/common/pl330.c b/arch/arm/common/pl330.c
> index 590fd57..1817963 100644
> --- a/arch/arm/common/pl330.c
> +++ b/arch/arm/common/pl330.c
> @@ -1211,8 +1211,8 @@ static inline u32 _prepare_ccr(const struct pl330_reqcfg *rqc)
> ? ? ? ?ccr |= (rqc->brst_size << CC_SRCBRSTSIZE_SHFT);
> ? ? ? ?ccr |= (rqc->brst_size << CC_DSTBRSTSIZE_SHFT);
>
> - ? ? ? ccr |= (rqc->dcctl << CC_SRCCCTRL_SHFT);
> - ? ? ? ccr |= (rqc->scctl << CC_DSTCCTRL_SHFT);
> + ? ? ? ccr |= (rqc->scctl << CC_SRCCCTRL_SHFT);
> + ? ? ? ccr |= (rqc->dcctl << CC_DSTCCTRL_SHFT);
>
> ? ? ? ?ccr |= (rqc->swap << CC_SWAP_SHFT);
>
Acked-by: Jassi Brar <jassisinghbrar@gmail.com>

[PATCH 1/4] ARM: PL330: Don't call the callbacks if there isn't any active transfer

[Javi Merino]

On 15/11/11 07:51, Jassi Brar wrote:
> On 2 November 2011 20:00, Javi Merino <javi.merino@arm.com> wrote:
>> If there are no transactions running it is not only wrong but
>> potentially dangerous to call the callbacks.
>>
> The idea was to have dmac drivers not touch the xfer after it has been submitted
> and before the callback is made upon successful transmission or abort/flush.

Right.

>> There can be a stale struct pl330_req pointer from a previous run, but the
>> memory may be free already.
>>
> Sorry I am unable to fathom the scenario. The pl330_request_channel resets
> both pl330_req pointers. Maybe some real failure you saw, could help
> me understand.

You request a channel, submit a xfer, it finishes successfully and
pl330_update() is called, which calls r->xfer_cb() and marks the request
as done by making req->mc_len = 0 .  Then, if you call
pl330_release_channel(), _callback(thrd->req[0].r, PL330_ERR_ABORT) is
called and, as thrd_req[0].r and thrd_req[0].r->xfer_cb are not null,
r->xfer_cb() is called again.  That's wrong, the xfer finished (maybe
long time ago) and the callback was called back then.  You shouldn't
call it again with an error.  You should only do so if the transfer was
running or scheduled to run, that's why I suggest calling _callback() if
!IS_FREE(req).

I'm not sure if this was clear enough, but I always hit this when
releasing channels that I've used.  Cheers,
Javi

-- IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium.  Thank you.

[PATCH 1/4] ARM: PL330: Don't call the callbacks if there isn't any active transfer

[Jassi Brar]

On Tue, Nov 15, 2011 at 9:21 PM, Javi Merino <javi.merino@arm.com> wrote:
> On 15/11/11 07:51, Jassi Brar wrote:
>>> There can be a stale struct pl330_req pointer from a previous run, but the
>>> memory may be free already.
>>>
>> Sorry I am unable to fathom the scenario. The pl330_request_channel resets
>> both pl330_req pointers. Maybe some real failure you saw, could help
>> me understand.
>
> You request a channel, submit a xfer, it finishes successfully and
> pl330_update() is called, which calls r->xfer_cb() and marks the request
> as done by making req->mc_len = 0 . ?Then, if you call
> pl330_release_channel(), _callback(thrd->req[0].r, PL330_ERR_ABORT) is
> called and, as thrd_req[0].r and thrd_req[0].r->xfer_cb are not null,
> r->xfer_cb() is called again. ?That's wrong, the xfer finished (maybe
> long time ago) and the callback was called back then. ?You shouldn't
> call it again with an error. ?You should only do so if the transfer was
> running or scheduled to run, that's why I suggest calling _callback() if
> !IS_FREE(req).
>
Usually flush is called before releasing the channel, which would
have taken care of it. But I agree, flush shouldn't be necessary.

MARK_FREE(_pl330_req)  with _pl330_req.r != NULL   doesn't look neat.
So maybe we should move   _pl330_req.r = NULL   into MARK_FREE macro.
Which would require moving 'struct list_head rqd' from _pl330_req to
pl330_req, that should be ok though I think.

[PATCH 1/4] ARM: PL330: Don't call the callbacks if there isn't any active transfer

[Jassi Brar]

On 15 November 2011 22:30, Jassi Brar <jassisinghbrar@gmail.com> wrote:
> On Tue, Nov 15, 2011 at 9:21 PM, Javi Merino <javi.merino@arm.com> wrote:
>> On 15/11/11 07:51, Jassi Brar wrote:
>>>> There can be a stale struct pl330_req pointer from a previous run, but the
>>>> memory may be free already.
>>>>
>>> Sorry I am unable to fathom the scenario. The pl330_request_channel resets
>>> both pl330_req pointers. Maybe some real failure you saw, could help
>>> me understand.
>>
>> You request a channel, submit a xfer, it finishes successfully and
>> pl330_update() is called, which calls r->xfer_cb() and marks the request
>> as done by making req->mc_len = 0 . ?Then, if you call
>> pl330_release_channel(), _callback(thrd->req[0].r, PL330_ERR_ABORT) is
>> called and, as thrd_req[0].r and thrd_req[0].r->xfer_cb are not null,
>> r->xfer_cb() is called again. ?That's wrong, the xfer finished (maybe
>> long time ago) and the callback was called back then. ?You shouldn't
>> call it again with an error. ?You should only do so if the transfer was
>> running or scheduled to run, that's why I suggest calling _callback() if
>> !IS_FREE(req).
>>
> Usually flush is called before releasing the channel, which would
> have taken care of it. But I agree, flush shouldn't be necessary.
>
> MARK_FREE(_pl330_req) ?with _pl330_req.r != NULL ? doesn't look neat.
> So maybe we should move ? _pl330_req.r = NULL ? into MARK_FREE macro.
> Which would require moving 'struct list_head rqd' from _pl330_req to
> pl330_req, that should be ok though I think.
>
I think the following strikes just the right balance and should fix
the issue ...

diff --git a/arch/arm/common/pl330.c b/arch/arm/common/pl330.c
index 7129cfb..ecbca3f 100644
--- a/arch/arm/common/pl330.c
+++ b/arch/arm/common/pl330.c
@@ -1480,13 +1480,19 @@ int pl330_update(const struct pl330_info *pi)

 	/* Now that we are in no hurry, do the callbacks */
 	while (!list_empty(&pl330->req_done)) {
+		struct pl330_req *r;
+
 		rqdone = container_of(pl330->req_done.next,
 					struct _pl330_req, rqd);

 		list_del_init(&rqdone->rqd);

+		/* Detach the req */
+		r = rqdone->r;
+		rqdone->r = NULL;
+
 		spin_unlock_irqrestore(&pl330->lock, flags);
-		_callback(rqdone->r, PL330_ERR_NONE);
+		_callback(r, PL330_ERR_NONE);
 		spin_lock_irqsave(&pl330->lock, flags);
 	}
---
Thanks
-j

[PATCH 1/4] ARM: PL330: Don't call the callbacks if there isn't any active transfer

[Javi Merino]

On 16/11/11 06:24, Jassi Brar wrote:
> On 15 November 2011 22:30, Jassi Brar <jassisinghbrar@gmail.com> wrote:
>> On Tue, Nov 15, 2011 at 9:21 PM, Javi Merino <javi.merino@arm.com> wrote:
>>> On 15/11/11 07:51, Jassi Brar wrote:
>>>>> There can be a stale struct pl330_req pointer from a previous run, but the
>>>>> memory may be free already.
>>>>>
>>>> Sorry I am unable to fathom the scenario. The pl330_request_channel resets
>>>> both pl330_req pointers. Maybe some real failure you saw, could help
>>>> me understand.
>>>
>>> You request a channel, submit a xfer, it finishes successfully and
>>> pl330_update() is called, which calls r->xfer_cb() and marks the request
>>> as done by making req->mc_len = 0 .  Then, if you call
>>> pl330_release_channel(), _callback(thrd->req[0].r, PL330_ERR_ABORT) is
>>> called and, as thrd_req[0].r and thrd_req[0].r->xfer_cb are not null,
>>> r->xfer_cb() is called again.  That's wrong, the xfer finished (maybe
>>> long time ago) and the callback was called back then.  You shouldn't
>>> call it again with an error.  You should only do so if the transfer was
>>> running or scheduled to run, that's why I suggest calling _callback() if
>>> !IS_FREE(req).
>>>
>> Usually flush is called before releasing the channel, which would
>> have taken care of it. But I agree, flush shouldn't be necessary.
>>
>> MARK_FREE(_pl330_req)  with _pl330_req.r != NULL   doesn't look neat.
>> So maybe we should move   _pl330_req.r = NULL   into MARK_FREE macro.
>> Which would require moving 'struct list_head rqd' from _pl330_req to
>> pl330_req, that should be ok though I think.
>>
> I think the following strikes just the right balance and should fix
> the issue ...
> 
> diff --git a/arch/arm/common/pl330.c b/arch/arm/common/pl330.c
> index 7129cfb..ecbca3f 100644
> --- a/arch/arm/common/pl330.c
> +++ b/arch/arm/common/pl330.c
> @@ -1480,13 +1480,19 @@ int pl330_update(const struct pl330_info *pi)
> 
>  	/* Now that we are in no hurry, do the callbacks */
>  	while (!list_empty(&pl330->req_done)) {
> +		struct pl330_req *r;
> +
>  		rqdone = container_of(pl330->req_done.next,
>  					struct _pl330_req, rqd);
> 
>  		list_del_init(&rqdone->rqd);
> 
> +		/* Detach the req */
> +		r = rqdone->r;
> +		rqdone->r = NULL;
> +
>  		spin_unlock_irqrestore(&pl330->lock, flags);
> -		_callback(rqdone->r, PL330_ERR_NONE);
> +		_callback(r, PL330_ERR_NONE);
>  		spin_lock_irqsave(&pl330->lock, flags);
>  	}

Right, and there's no need to patch pl330_chan_ctrl() in PL330_OP_ABORT
because _pl330_req.r is set to NULL in there already.

I'll try this patch and if it works we can submit it instead of my
original one.

Thanks,
Javi

[PATCH 1/4] ARM: PL330: Don't call the callbacks if there isn't any active transfer

[Javi Merino]

Hi Jassi,

On 22/11/11 17:16, Javi Merino wrote:
> On 16/11/11 06:24, Jassi Brar wrote:
>> On 15 November 2011 22:30, Jassi Brar <jassisinghbrar@gmail.com> wrote:
>> I think the following strikes just the right balance and should fix
>> the issue ...
>>
>> diff --git a/arch/arm/common/pl330.c b/arch/arm/common/pl330.c
>> index 7129cfb..ecbca3f 100644
>> --- a/arch/arm/common/pl330.c
>> +++ b/arch/arm/common/pl330.c
>> @@ -1480,13 +1480,19 @@ int pl330_update(const struct pl330_info *pi)
>>
>>  	/* Now that we are in no hurry, do the callbacks */
>>  	while (!list_empty(&pl330->req_done)) {
>> +		struct pl330_req *r;
>> +
>>  		rqdone = container_of(pl330->req_done.next,
>>  					struct _pl330_req, rqd);
>>
>>  		list_del_init(&rqdone->rqd);
>>
>> +		/* Detach the req */
>> +		r = rqdone->r;
>> +		rqdone->r = NULL;
>> +
>>  		spin_unlock_irqrestore(&pl330->lock, flags);
>> -		_callback(rqdone->r, PL330_ERR_NONE);
>> +		_callback(r, PL330_ERR_NONE);
>>  		spin_lock_irqsave(&pl330->lock, flags);
>>  	}
> 
> I'll try this patch and if it works we can submit it instead of my
> original one.

It worked!  Can you please send it to Russell's patch system?  You can
add a:

Tested-by: Javi Merino <javi.merino@arm.com>

Cheers,
Javi