* [PATCH] mfd: vexpress: Handle pending config transactions
@ 2013-04-24 16:31 Pawel Moll
2013-04-25 13:02 ` Jon Medhurst (Tixy)
2013-04-25 17:14 ` [PATCH v2] " Pawel Moll
0 siblings, 2 replies; 6+ messages in thread
From: Pawel Moll @ 2013-04-24 16:31 UTC (permalink / raw)
To: linux-arm-kernel
The config transactions "scheduler" was hopelessly broken,
repeating completed transaction instead of picking up
next pending one.
Fixed now. Also improved debug messages.
Signed-off-by: Pawel Moll <pawel.moll@arm.com>
---
drivers/mfd/vexpress-config.c | 31 ++++++++++++++++++++-----------
1 file changed, 20 insertions(+), 11 deletions(-)
diff --git a/drivers/mfd/vexpress-config.c b/drivers/mfd/vexpress-config.c
index 3c1723aa..4991db3 100644
--- a/drivers/mfd/vexpress-config.c
+++ b/drivers/mfd/vexpress-config.c
@@ -184,13 +184,14 @@ static int vexpress_config_schedule(struct vexpress_config_trans *trans)
spin_lock_irqsave(&bridge->transactions_lock, flags);
- vexpress_config_dump_trans("Executing", trans);
-
- if (list_empty(&bridge->transactions))
+ if (list_empty(&bridge->transactions)) {
+ vexpress_config_dump_trans("Executing", trans);
status = bridge->info->func_exec(trans->func->func,
trans->offset, trans->write, trans->data);
- else
+ } else {
+ vexpress_config_dump_trans("Queuing", trans);
status = VEXPRESS_CONFIG_STATUS_WAIT;
+ }
switch (status) {
case VEXPRESS_CONFIG_STATUS_DONE:
@@ -217,20 +218,28 @@ void vexpress_config_complete(struct vexpress_config_bridge *bridge,
trans = list_first_entry(&bridge->transactions,
struct vexpress_config_trans, list);
+ trans->status = status;
vexpress_config_dump_trans("Completed", trans);
- trans->status = status;
list_del(&trans->list);
- if (!list_empty(&bridge->transactions)) {
- vexpress_config_dump_trans("Pending", trans);
+ complete(&trans->completion);
+
+ while (!list_empty(&bridge->transactions)) {
+ trans = list_first_entry(&bridge->transactions,
+ struct vexpress_config_trans, list);
- bridge->info->func_exec(trans->func->func, trans->offset,
- trans->write, trans->data);
+ vexpress_config_dump_trans("Executing pending", trans);
+ status = bridge->info->func_exec(trans->func->func,
+ trans->offset, trans->write, trans->data);
+
+ if (status == VEXPRESS_CONFIG_STATUS_DONE)
+ vexpress_config_dump_trans("Finished pending", trans);
+ else
+ break;
}
- spin_unlock_irqrestore(&bridge->transactions_lock, flags);
- complete(&trans->completion);
+ spin_unlock_irqrestore(&bridge->transactions_lock, flags);
}
EXPORT_SYMBOL(vexpress_config_complete);
--
1.7.10.4
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH] mfd: vexpress: Handle pending config transactions
2013-04-24 16:31 [PATCH] mfd: vexpress: Handle pending config transactions Pawel Moll
@ 2013-04-25 13:02 ` Jon Medhurst (Tixy)
2013-04-25 14:40 ` Pawel Moll
2013-04-25 17:14 ` [PATCH v2] " Pawel Moll
1 sibling, 1 reply; 6+ messages in thread
From: Jon Medhurst (Tixy) @ 2013-04-25 13:02 UTC (permalink / raw)
To: linux-arm-kernel
On Wed, 2013-04-24 at 17:31 +0100, Pawel Moll wrote:
> The config transactions "scheduler" was hopelessly broken,
> repeating completed transaction instead of picking up
> next pending one.
>
> Fixed now. Also improved debug messages.
>
> Signed-off-by: Pawel Moll <pawel.moll@arm.com>
> ---
> drivers/mfd/vexpress-config.c | 31 ++++++++++++++++++++-----------
> 1 file changed, 20 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/mfd/vexpress-config.c b/drivers/mfd/vexpress-config.c
> index 3c1723aa..4991db3 100644
> --- a/drivers/mfd/vexpress-config.c
> +++ b/drivers/mfd/vexpress-config.c
> @@ -184,13 +184,14 @@ static int vexpress_config_schedule(struct vexpress_config_trans *trans)
>
> spin_lock_irqsave(&bridge->transactions_lock, flags);
>
> - vexpress_config_dump_trans("Executing", trans);
> -
> - if (list_empty(&bridge->transactions))
> + if (list_empty(&bridge->transactions)) {
> + vexpress_config_dump_trans("Executing", trans);
> status = bridge->info->func_exec(trans->func->func,
> trans->offset, trans->write, trans->data);
> - else
> + } else {
> + vexpress_config_dump_trans("Queuing", trans);
> status = VEXPRESS_CONFIG_STATUS_WAIT;
> + }
>
> switch (status) {
> case VEXPRESS_CONFIG_STATUS_DONE:
> @@ -217,20 +218,28 @@ void vexpress_config_complete(struct vexpress_config_bridge *bridge,
>
> trans = list_first_entry(&bridge->transactions,
> struct vexpress_config_trans, list);
> + trans->status = status;
> vexpress_config_dump_trans("Completed", trans);
>
> - trans->status = status;
> list_del(&trans->list);
>
> - if (!list_empty(&bridge->transactions)) {
> - vexpress_config_dump_trans("Pending", trans);
> + complete(&trans->completion);
> +
> + while (!list_empty(&bridge->transactions)) {
> + trans = list_first_entry(&bridge->transactions,
> + struct vexpress_config_trans, list);
>
> - bridge->info->func_exec(trans->func->func, trans->offset,
> - trans->write, trans->data);
> + vexpress_config_dump_trans("Executing pending", trans);
> + status = bridge->info->func_exec(trans->func->func,
> + trans->offset, trans->write, trans->data);
> +
> + if (status == VEXPRESS_CONFIG_STATUS_DONE)
> + vexpress_config_dump_trans("Finished pending", trans);
> + else
> + break;
For each transaction we execute in this loop, don't we also need to do
the actions vexpress_config_complete does? E.g.
trans->status = status;
list_del(&trans->list);
complete(&trans->completion);
except in the case status==VEXPRESS_CONFIG_STATUS_WAIT.
Or, does the call to func_exec cause vexpress_config_complete() to be
called later? (It hadn't better be called synchronously or we get a
deadlock).
Actually, if it is called later, there is still a problem because if the
call to func_exec returns VEXPRESS_CONFIG_STATUS_DONE then the
transaction is left on the list and the while loop will try and execute
it again.
> }
> - spin_unlock_irqrestore(&bridge->transactions_lock, flags);
>
> - complete(&trans->completion);
> + spin_unlock_irqrestore(&bridge->transactions_lock, flags);
> }
> EXPORT_SYMBOL(vexpress_config_complete);
>
--
Tixy
^ permalink raw reply [flat|nested] 6+ messages in thread* [PATCH] mfd: vexpress: Handle pending config transactions
2013-04-25 13:02 ` Jon Medhurst (Tixy)
@ 2013-04-25 14:40 ` Pawel Moll
0 siblings, 0 replies; 6+ messages in thread
From: Pawel Moll @ 2013-04-25 14:40 UTC (permalink / raw)
To: linux-arm-kernel
On Thu, 2013-04-25 at 14:02 +0100, Jon Medhurst (Tixy) wrote:
> > + while (!list_empty(&bridge->transactions)) {
> > + trans = list_first_entry(&bridge->transactions,
> > + struct vexpress_config_trans, list);
> >
> > - bridge->info->func_exec(trans->func->func, trans->offset,
> > - trans->write, trans->data);
> > + vexpress_config_dump_trans("Executing pending", trans);
> > + status = bridge->info->func_exec(trans->func->func,
> > + trans->offset, trans->write, trans->data);
> > +
> > + if (status == VEXPRESS_CONFIG_STATUS_DONE)
> > + vexpress_config_dump_trans("Finished pending", trans);
> > + else
> > + break;
>
> For each transaction we execute in this loop, don't we also need to do
> the actions vexpress_config_complete does? E.g.
>
> trans->status = status;
> list_del(&trans->list);
> complete(&trans->completion);
>
> except in the case status==VEXPRESS_CONFIG_STATUS_WAIT.
You're completely right. I shouldn't have write this code as the last
thing in the afternoon...
Will send v2.
Pawe?
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v2] mfd: vexpress: Handle pending config transactions
2013-04-24 16:31 [PATCH] mfd: vexpress: Handle pending config transactions Pawel Moll
2013-04-25 13:02 ` Jon Medhurst (Tixy)
@ 2013-04-25 17:14 ` Pawel Moll
2013-04-26 10:51 ` Jon Medhurst (Tixy)
2013-04-26 14:13 ` Samuel Ortiz
1 sibling, 2 replies; 6+ messages in thread
From: Pawel Moll @ 2013-04-25 17:14 UTC (permalink / raw)
To: linux-arm-kernel
The config transactions "scheduler" was hopelessly broken,
repeating completed transaction instead of picking up
next pending one.
Fixed now. Also improved debug messages.
Signed-off-by: Pawel Moll <pawel.moll@arm.com>
---
drivers/mfd/vexpress-config.c | 35 +++++++++++++++++++++--------------
1 file changed, 21 insertions(+), 14 deletions(-)
diff --git a/drivers/mfd/vexpress-config.c b/drivers/mfd/vexpress-config.c
index 3c1723aa..84ce6b9 100644
--- a/drivers/mfd/vexpress-config.c
+++ b/drivers/mfd/vexpress-config.c
@@ -184,13 +184,14 @@ static int vexpress_config_schedule(struct vexpress_config_trans *trans)
spin_lock_irqsave(&bridge->transactions_lock, flags);
- vexpress_config_dump_trans("Executing", trans);
-
- if (list_empty(&bridge->transactions))
+ if (list_empty(&bridge->transactions)) {
+ vexpress_config_dump_trans("Executing", trans);
status = bridge->info->func_exec(trans->func->func,
trans->offset, trans->write, trans->data);
- else
+ } else {
+ vexpress_config_dump_trans("Queuing", trans);
status = VEXPRESS_CONFIG_STATUS_WAIT;
+ }
switch (status) {
case VEXPRESS_CONFIG_STATUS_DONE:
@@ -212,25 +213,31 @@ void vexpress_config_complete(struct vexpress_config_bridge *bridge,
{
struct vexpress_config_trans *trans;
unsigned long flags;
+ const char *message = "Completed";
spin_lock_irqsave(&bridge->transactions_lock, flags);
trans = list_first_entry(&bridge->transactions,
struct vexpress_config_trans, list);
- vexpress_config_dump_trans("Completed", trans);
-
trans->status = status;
- list_del(&trans->list);
- if (!list_empty(&bridge->transactions)) {
- vexpress_config_dump_trans("Pending", trans);
+ do {
+ vexpress_config_dump_trans(message, trans);
+ list_del(&trans->list);
+ complete(&trans->completion);
- bridge->info->func_exec(trans->func->func, trans->offset,
- trans->write, trans->data);
- }
- spin_unlock_irqrestore(&bridge->transactions_lock, flags);
+ if (list_empty(&bridge->transactions))
+ break;
+
+ trans = list_first_entry(&bridge->transactions,
+ struct vexpress_config_trans, list);
+ vexpress_config_dump_trans("Executing pending", trans);
+ trans->status = bridge->info->func_exec(trans->func->func,
+ trans->offset, trans->write, trans->data);
+ message = "Finished pending";
+ } while (trans->status == VEXPRESS_CONFIG_STATUS_DONE);
- complete(&trans->completion);
+ spin_unlock_irqrestore(&bridge->transactions_lock, flags);
}
EXPORT_SYMBOL(vexpress_config_complete);
--
1.7.10.4
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH v2] mfd: vexpress: Handle pending config transactions
2013-04-25 17:14 ` [PATCH v2] " Pawel Moll
@ 2013-04-26 10:51 ` Jon Medhurst (Tixy)
2013-04-26 14:13 ` Samuel Ortiz
1 sibling, 0 replies; 6+ messages in thread
From: Jon Medhurst (Tixy) @ 2013-04-26 10:51 UTC (permalink / raw)
To: linux-arm-kernel
On Thu, 2013-04-25 at 18:14 +0100, Pawel Moll wrote:
> The config transactions "scheduler" was hopelessly broken,
> repeating completed transaction instead of picking up
> next pending one.
>
> Fixed now. Also improved debug messages.
>
> Signed-off-by: Pawel Moll <pawel.moll@arm.com>
Yes, looks really fixed now. :-)
Reviewed-by: Jon Medhurst <tixy@linaro.org>
> ---
> drivers/mfd/vexpress-config.c | 35 +++++++++++++++++++++--------------
> 1 file changed, 21 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/mfd/vexpress-config.c b/drivers/mfd/vexpress-config.c
> index 3c1723aa..84ce6b9 100644
> --- a/drivers/mfd/vexpress-config.c
> +++ b/drivers/mfd/vexpress-config.c
> @@ -184,13 +184,14 @@ static int vexpress_config_schedule(struct vexpress_config_trans *trans)
>
> spin_lock_irqsave(&bridge->transactions_lock, flags);
>
> - vexpress_config_dump_trans("Executing", trans);
> -
> - if (list_empty(&bridge->transactions))
> + if (list_empty(&bridge->transactions)) {
> + vexpress_config_dump_trans("Executing", trans);
> status = bridge->info->func_exec(trans->func->func,
> trans->offset, trans->write, trans->data);
> - else
> + } else {
> + vexpress_config_dump_trans("Queuing", trans);
> status = VEXPRESS_CONFIG_STATUS_WAIT;
> + }
>
> switch (status) {
> case VEXPRESS_CONFIG_STATUS_DONE:
> @@ -212,25 +213,31 @@ void vexpress_config_complete(struct vexpress_config_bridge *bridge,
> {
> struct vexpress_config_trans *trans;
> unsigned long flags;
> + const char *message = "Completed";
>
> spin_lock_irqsave(&bridge->transactions_lock, flags);
>
> trans = list_first_entry(&bridge->transactions,
> struct vexpress_config_trans, list);
> - vexpress_config_dump_trans("Completed", trans);
> -
> trans->status = status;
> - list_del(&trans->list);
>
> - if (!list_empty(&bridge->transactions)) {
> - vexpress_config_dump_trans("Pending", trans);
> + do {
> + vexpress_config_dump_trans(message, trans);
> + list_del(&trans->list);
> + complete(&trans->completion);
>
> - bridge->info->func_exec(trans->func->func, trans->offset,
> - trans->write, trans->data);
> - }
> - spin_unlock_irqrestore(&bridge->transactions_lock, flags);
> + if (list_empty(&bridge->transactions))
> + break;
> +
> + trans = list_first_entry(&bridge->transactions,
> + struct vexpress_config_trans, list);
> + vexpress_config_dump_trans("Executing pending", trans);
> + trans->status = bridge->info->func_exec(trans->func->func,
> + trans->offset, trans->write, trans->data);
> + message = "Finished pending";
> + } while (trans->status == VEXPRESS_CONFIG_STATUS_DONE);
>
> - complete(&trans->completion);
> + spin_unlock_irqrestore(&bridge->transactions_lock, flags);
> }
> EXPORT_SYMBOL(vexpress_config_complete);
>
^ permalink raw reply [flat|nested] 6+ messages in thread* [PATCH v2] mfd: vexpress: Handle pending config transactions
2013-04-25 17:14 ` [PATCH v2] " Pawel Moll
2013-04-26 10:51 ` Jon Medhurst (Tixy)
@ 2013-04-26 14:13 ` Samuel Ortiz
1 sibling, 0 replies; 6+ messages in thread
From: Samuel Ortiz @ 2013-04-26 14:13 UTC (permalink / raw)
To: linux-arm-kernel
Hi Pawel,
On Thu, Apr 25, 2013 at 06:14:51PM +0100, Pawel Moll wrote:
> The config transactions "scheduler" was hopelessly broken,
> repeating completed transaction instead of picking up
> next pending one.
>
> Fixed now. Also improved debug messages.
>
> Signed-off-by: Pawel Moll <pawel.moll@arm.com>
> ---
> drivers/mfd/vexpress-config.c | 35 +++++++++++++++++++++--------------
> 1 file changed, 21 insertions(+), 14 deletions(-)
Applied and pushed, thanks.
Cheers,
Samuel.
--
Intel Open Source Technology Centre
http://oss.intel.com/
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2013-04-26 14:13 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-04-24 16:31 [PATCH] mfd: vexpress: Handle pending config transactions Pawel Moll
2013-04-25 13:02 ` Jon Medhurst (Tixy)
2013-04-25 14:40 ` Pawel Moll
2013-04-25 17:14 ` [PATCH v2] " Pawel Moll
2013-04-26 10:51 ` Jon Medhurst (Tixy)
2013-04-26 14:13 ` Samuel Ortiz
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).