[PATCH 4/7] spi/pl022: move device disable to workqueue thread

[PATCH 4/7] spi/pl022: move device disable to workqueue thread

[Linus Walleij]

From: Chris Blair <chris.blair-0IS4wlFg1OjSUeElwK9/Pw@public.gmane.org>

Moves the disabling of the device and clocks to the same thread in
which the device and clocks are enabled. This avoids SMP issues where
the device can be enabled for a transfer by one thread and then
disabled by the completion of the previous transfer in another thread.

Signed-off-by: Chris Blair <chris.blair-0IS4wlFg1OjSUeElwK9/Pw@public.gmane.org>
Signed-off-by: Linus Walleij <linus.walleij-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
ChangeLog v1->v2:
- Moved this patch before the block disable patch at request from
  Viresh.
- Insert blank line...
---
 drivers/spi/spi-pl022.c |   26 +++++++++++++++++---------
 1 files changed, 17 insertions(+), 9 deletions(-)

diff --git a/drivers/spi/spi-pl022.c b/drivers/spi/spi-pl022.c
index b4038f9..8cdf052 100644
--- a/drivers/spi/spi-pl022.c
+++ b/drivers/spi/spi-pl022.c
@@ -512,8 +512,6 @@ static void giveback(struct pl022 *pl022)
 	msg->state = NULL;
 	if (msg->complete)
 		msg->complete(msg->context);
-	/* This message is completed, so let's turn off the clocks & power */
-	pm_runtime_put(&pl022->adev->dev);
 }
 
 /**
@@ -1509,14 +1507,19 @@ static void pump_messages(struct work_struct *work)
 	struct pl022 *pl022 =
 		container_of(work, struct pl022, pump_messages);
 	unsigned long flags;
+	bool was_busy = false;
 
 	/* Lock queue and check for queue work */
 	spin_lock_irqsave(&pl022->queue_lock, flags);
 	if (list_empty(&pl022->queue) || !pl022->running) {
+		if (pl022->busy) {
+			pm_runtime_put(&pl022->adev->dev);
+		}
 		pl022->busy = false;
 		spin_unlock_irqrestore(&pl022->queue_lock, flags);
 		return;
 	}
+
 	/* Make sure we are not already running a message */
 	if (pl022->cur_msg) {
 		spin_unlock_irqrestore(&pl022->queue_lock, flags);
@@ -1527,7 +1530,10 @@ static void pump_messages(struct work_struct *work)
 	    list_entry(pl022->queue.next, struct spi_message, queue);
 
 	list_del_init(&pl022->cur_msg->queue);
-	pl022->busy = true;
+	if (pl022->busy)
+		was_busy = true;
+	else
+		pl022->busy = true;
 	spin_unlock_irqrestore(&pl022->queue_lock, flags);
 
 	/* Initial message state */
@@ -1537,12 +1543,14 @@ static void pump_messages(struct work_struct *work)
 
 	/* Setup the SPI using the per chip configuration */
 	pl022->cur_chip = spi_get_ctldata(pl022->cur_msg->spi);
-	/*
-	 * We enable the core voltage and clocks here, then the clocks
-	 * and core will be disabled when giveback() is called in each method
-	 * (poll/interrupt/DMA)
-	 */
-	pm_runtime_get_sync(&pl022->adev->dev);
+	if (!was_busy)
+		/*
+		 * We enable the core voltage and clocks here, then the clocks
+		 * and core will be disabled when this workqueue is run again
+		 * and there is no more work to be done.
+		 */
+		pm_runtime_get_sync(&pl022->adev->dev);
+
 	restore_state(pl022);
 	flush(pl022);
 
-- 
1.7.3.2


------------------------------------------------------------------------------
All the data continuously generated in your IT infrastructure 
contains a definitive record of customers, application performance, 
security threats, fraudulent activity, and more. Splunk takes this 
data and makes sense of it. IT sense. And common sense.
http://p.sf.net/sfu/splunk-novd2d

[PATCH 4/7] spi/pl022: move device disable to workqueue thread

[Linus Walleij]

From: Chris Blair <chris.blair@stericsson.com>

Moves the disabling of the device and clocks to the same thread in
which the device and clocks are enabled. This avoids SMP issues where
the device can be enabled for a transfer by one thread and then
disabled by the completion of the previous transfer in another thread.

Signed-off-by: Chris Blair <chris.blair@stericsson.com>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
ChangeLog v1->v2:
- Moved this patch before the block disable patch at request from
  Viresh.
- Insert blank line...
---
 drivers/spi/spi-pl022.c |   26 +++++++++++++++++---------
 1 files changed, 17 insertions(+), 9 deletions(-)

diff --git a/drivers/spi/spi-pl022.c b/drivers/spi/spi-pl022.c
index b4038f9..8cdf052 100644
--- a/drivers/spi/spi-pl022.c
+++ b/drivers/spi/spi-pl022.c
@@ -512,8 +512,6 @@ static void giveback(struct pl022 *pl022)
 	msg->state = NULL;
 	if (msg->complete)
 		msg->complete(msg->context);
-	/* This message is completed, so let's turn off the clocks & power */
-	pm_runtime_put(&pl022->adev->dev);
 }
 
 /**
@@ -1509,14 +1507,19 @@ static void pump_messages(struct work_struct *work)
 	struct pl022 *pl022 =
 		container_of(work, struct pl022, pump_messages);
 	unsigned long flags;
+	bool was_busy = false;
 
 	/* Lock queue and check for queue work */
 	spin_lock_irqsave(&pl022->queue_lock, flags);
 	if (list_empty(&pl022->queue) || !pl022->running) {
+		if (pl022->busy) {
+			pm_runtime_put(&pl022->adev->dev);
+		}
 		pl022->busy = false;
 		spin_unlock_irqrestore(&pl022->queue_lock, flags);
 		return;
 	}
+
 	/* Make sure we are not already running a message */
 	if (pl022->cur_msg) {
 		spin_unlock_irqrestore(&pl022->queue_lock, flags);
@@ -1527,7 +1530,10 @@ static void pump_messages(struct work_struct *work)
 	    list_entry(pl022->queue.next, struct spi_message, queue);
 
 	list_del_init(&pl022->cur_msg->queue);
-	pl022->busy = true;
+	if (pl022->busy)
+		was_busy = true;
+	else
+		pl022->busy = true;
 	spin_unlock_irqrestore(&pl022->queue_lock, flags);
 
 	/* Initial message state */
@@ -1537,12 +1543,14 @@ static void pump_messages(struct work_struct *work)
 
 	/* Setup the SPI using the per chip configuration */
 	pl022->cur_chip = spi_get_ctldata(pl022->cur_msg->spi);
-	/*
-	 * We enable the core voltage and clocks here, then the clocks
-	 * and core will be disabled when giveback() is called in each method
-	 * (poll/interrupt/DMA)
-	 */
-	pm_runtime_get_sync(&pl022->adev->dev);
+	if (!was_busy)
+		/*
+		 * We enable the core voltage and clocks here, then the clocks
+		 * and core will be disabled when this workqueue is run again
+		 * and there is no more work to be done.
+		 */
+		pm_runtime_get_sync(&pl022->adev->dev);
+
 	restore_state(pl022);
 	flush(pl022);
 
-- 
1.7.3.2

Re: [PATCH 4/7] spi/pl022: move device disable to workqueue thread

[Viresh Kumar]

On 11/22/2011 1:55 PM, Linus WALLEIJ wrote:
>  	if (list_empty(&pl022->queue) || !pl022->running) {
> +		if (pl022->busy) {
> +			pm_runtime_put(&pl022->adev->dev);
> +		}

We used to get warnings from checkpatch, for single line code inside {}.
Don't we get them anymore?

Reviewed-by: Viresh Kumar <viresh.kumar-qxv4g6HH51o@public.gmane.org>

-- 
viresh

------------------------------------------------------------------------------
All the data continuously generated in your IT infrastructure 
contains a definitive record of customers, application performance, 
security threats, fraudulent activity, and more. Splunk takes this 
data and makes sense of it. IT sense. And common sense.
http://p.sf.net/sfu/splunk-novd2d

[PATCH 4/7] spi/pl022: move device disable to workqueue thread

[Viresh Kumar]

On 11/22/2011 1:55 PM, Linus WALLEIJ wrote:
>  	if (list_empty(&pl022->queue) || !pl022->running) {
> +		if (pl022->busy) {
> +			pm_runtime_put(&pl022->adev->dev);
> +		}

We used to get warnings from checkpatch, for single line code inside {}.
Don't we get them anymore?

Reviewed-by: Viresh Kumar <viresh.kumar@st.com>

-- 
viresh

Re: [PATCH 4/7] spi/pl022: move device disable to workqueue thread

[Linus Walleij]

On Tue, Nov 22, 2011 at 9:28 AM, Viresh Kumar <viresh.kumar-qxv4g6HH51o@public.gmane.org> wrote:
> On 11/22/2011 1:55 PM, Linus WALLEIJ wrote:
>>       if (list_empty(&pl022->queue) || !pl022->running) {
>> +             if (pl022->busy) {
>> +                     pm_runtime_put(&pl022->adev->dev);
>> +             }
>
> We used to get warnings from checkpatch, for single line code inside {}.
> Don't we get them anymore?

It's driven by heuristics, I think it only warns on the first level since
elsewhere you may want braces for readability.

But I'll take it out when I add you Reviewed-by...

Linus

------------------------------------------------------------------------------
All the data continuously generated in your IT infrastructure 
contains a definitive record of customers, application performance, 
security threats, fraudulent activity, and more. Splunk takes this 
data and makes sense of it. IT sense. And common sense.
http://p.sf.net/sfu/splunk-novd2d

[PATCH 4/7] spi/pl022: move device disable to workqueue thread

[Linus Walleij]

On Tue, Nov 22, 2011 at 9:28 AM, Viresh Kumar <viresh.kumar@st.com> wrote:
> On 11/22/2011 1:55 PM, Linus WALLEIJ wrote:
>> ? ? ? if (list_empty(&pl022->queue) || !pl022->running) {
>> + ? ? ? ? ? ? if (pl022->busy) {
>> + ? ? ? ? ? ? ? ? ? ? pm_runtime_put(&pl022->adev->dev);
>> + ? ? ? ? ? ? }
>
> We used to get warnings from checkpatch, for single line code inside {}.
> Don't we get them anymore?

It's driven by heuristics, I think it only warns on the first level since
elsewhere you may want braces for readability.

But I'll take it out when I add you Reviewed-by...

Linus

Re: [PATCH 4/7] spi/pl022: move device disable to workqueue thread

[Wolfram Sang]

[-- Attachment #1.1: Type: text/plain, Size: 1037 bytes --]

Hi Linus,

On Tue, Nov 22, 2011 at 10:28:44AM +0100, Linus Walleij wrote:
> On Tue, Nov 22, 2011 at 9:28 AM, Viresh Kumar <viresh.kumar@st.com> wrote:
> > On 11/22/2011 1:55 PM, Linus WALLEIJ wrote:
> >>       if (list_empty(&pl022->queue) || !pl022->running) {
> >> +             if (pl022->busy) {
> >> +                     pm_runtime_put(&pl022->adev->dev);
> >> +             }
> >
> > We used to get warnings from checkpatch, for single line code inside {}.
> > Don't we get them anymore?
> 
> It's driven by heuristics, I think it only warns on the first level since
> elsewhere you may want braces for readability.
> 
> But I'll take it out when I add you Reviewed-by...

Does that imply you will send out a V3 of this series somewhen and I
don't need to add this right now to the for-grant branch I am currently
preparing?

Thanks,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 4/7] spi/pl022: move device disable to workqueue thread

[Wolfram Sang]

Hi Linus,

On Tue, Nov 22, 2011 at 10:28:44AM +0100, Linus Walleij wrote:
> On Tue, Nov 22, 2011 at 9:28 AM, Viresh Kumar <viresh.kumar@st.com> wrote:
> > On 11/22/2011 1:55 PM, Linus WALLEIJ wrote:
> >> ? ? ? if (list_empty(&pl022->queue) || !pl022->running) {
> >> + ? ? ? ? ? ? if (pl022->busy) {
> >> + ? ? ? ? ? ? ? ? ? ? pm_runtime_put(&pl022->adev->dev);
> >> + ? ? ? ? ? ? }
> >
> > We used to get warnings from checkpatch, for single line code inside {}.
> > Don't we get them anymore?
> 
> It's driven by heuristics, I think it only warns on the first level since
> elsewhere you may want braces for readability.
> 
> But I'll take it out when I add you Reviewed-by...

Does that imply you will send out a V3 of this series somewhen and I
don't need to add this right now to the for-grant branch I am currently
preparing?

Thanks,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20111207/b856cd69/attachment.sig>

Re: [PATCH 4/7] spi/pl022: move device disable to workqueue thread

[Linus Walleij]

On Wed, Dec 7, 2011 at 9:22 PM, Wolfram Sang <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org> wrote:

>> But I'll take it out when I add you Reviewed-by...
>
> Does that imply you will send out a V3 of this series somewhen and I
> don't need to add this right now to the for-grant branch I am currently
> preparing?

No I just added them to my branch and waited for Grant to get
back online.

The patches and ACKs are accumulated on this branch:

git://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-stericsson.git pl022

If you want to make a huge bundle to send to Grant, please pull
this into your tree.

Thanks,
Linus Walleij

------------------------------------------------------------------------------
Cloud Services Checklist: Pricing and Packaging Optimization
This white paper is intended to serve as a reference, checklist and point of 
discussion for anyone considering optimizing the pricing and packaging model 
of a cloud services business. Read Now!
http://www.accelacomm.com/jaw/sfnl/114/51491232/

[PATCH 4/7] spi/pl022: move device disable to workqueue thread

[Linus Walleij]

On Wed, Dec 7, 2011 at 9:22 PM, Wolfram Sang <w.sang@pengutronix.de> wrote:

>> But I'll take it out when I add you Reviewed-by...
>
> Does that imply you will send out a V3 of this series somewhen and I
> don't need to add this right now to the for-grant branch I am currently
> preparing?

No I just added them to my branch and waited for Grant to get
back online.

The patches and ACKs are accumulated on this branch:

git://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-stericsson.git pl022

If you want to make a huge bundle to send to Grant, please pull
this into your tree.

Thanks,
Linus Walleij