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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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
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ł
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 <[email protected]>
---
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
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 <[email protected]>
Yes, looks really fixed now. :-)
Reviewed-by: Jon Medhurst <[email protected]>
> ---
> 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);
>
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 <[email protected]>
> ---
> 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/