From: javi.merino@arm•com (Javi Merino)
To: linux-arm-kernel@lists•infradead.org
Subject: [PATCH 1/4] ARM: PL330: Don't call the callbacks if there isn't any active transfer
Date: Tue, 22 Nov 2011 17:16:22 +0000 [thread overview]
Message-ID: <4ECBD8E6.3010605@arm.com> (raw)
In-Reply-To: <CAJe_ZhftO+481BfL0ErEcM_brfmSuTXkTEniLRYxxM2T7OM2QA@mail.gmail.com>
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
next prev parent reply other threads:[~2011-11-22 17:16 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2011-11-02 14:30 [PATCH 1/4] ARM: PL330: Don't call the callbacks if there isn't any active transfer Javi Merino
2011-11-02 14:30 ` [PATCH 2/4] ARM: PL330: Only register usable channels Javi Merino
2011-11-15 8:48 ` Jassi Brar
2011-11-02 14:30 ` [PATCH 3/4] ARM: PL330: Fix the size of the dst_cache_ctrl field Javi Merino
2011-11-15 9:09 ` Jassi Brar
2011-11-02 14:30 ` [PATCH 4/4] ARM: PL330: Fix typo in _prepare_ccr() Javi Merino
2011-11-15 9:11 ` Jassi Brar
2011-11-15 7:51 ` [PATCH 1/4] ARM: PL330: Don't call the callbacks if there isn't any active transfer Jassi Brar
2011-11-15 15:51 ` Javi Merino
2011-11-15 17:00 ` Jassi Brar
2011-11-16 6:24 ` Jassi Brar
2011-11-22 17:16 ` Javi Merino [this message]
2011-11-23 9:46 ` Javi Merino
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=4ECBD8E6.3010605@arm.com \
--to=javi.merino@arm$(echo .)com \
--cc=linux-arm-kernel@lists$(echo .)infradead.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox