[PATCH 1/2] spi/bitbang: check for setup_transfer during initialization

[PATCH 1/2] spi/bitbang: check for setup_transfer during initialization

[Uwe Kleine-König]

From: Sascha Hauer <s.hauer@pengutronix.de>

setup_transfer is mandatory if spi_bitbang_transfer is used, so
check for it during initialization and not each time during runtime.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/spi/spi_bitbang.c |   13 ++++---------
 1 files changed, 4 insertions(+), 9 deletions(-)

diff --git a/drivers/spi/spi_bitbang.c b/drivers/spi/spi_bitbang.c
index 8b55724..14a63f6 100644
--- a/drivers/spi/spi_bitbang.c
+++ b/drivers/spi/spi_bitbang.c
@@ -259,10 +259,6 @@ static void bitbang_work(struct work_struct *work)
 	struct spi_bitbang	*bitbang =
 		container_of(work, struct spi_bitbang, work);
 	unsigned long		flags;
-	int			(*setup_transfer)(struct spi_device *,
-					struct spi_transfer *);
-
-	setup_transfer = bitbang->setup_transfer;
 
 	spin_lock_irqsave(&bitbang->lock, flags);
 	bitbang->busy = 1;
@@ -300,11 +296,7 @@ static void bitbang_work(struct work_struct *work)
 
 			/* init (-1) or override (1) transfer params */
 			if (do_setup != 0) {
-				if (!setup_transfer) {
-					status = -ENOPROTOOPT;
-					break;
-				}
-				status = setup_transfer(spi, t);
+				status = bitbang->setup_transfer(spi, t);
 				if (status < 0)
 					break;
 				if (do_setup == -1)
@@ -465,6 +457,9 @@ int spi_bitbang_start(struct spi_bitbang *bitbang)
 		}
 	} else if (!bitbang->master->setup)
 		return -EINVAL;
+	if (bitbang->master->transfer == spi_bitbang_transfer &&
+			!bitbang->setup_transfer)
+		return -EINVAL;
 
 	/* this task is the only thing to touch the SPI bits */
 	bitbang->busy = 0;
-- 
1.7.2.3


------------------------------------------------------------------------------
The ultimate all-in-one performance toolkit: Intel(R) Parallel Studio XE:
Pinpoint memory and threading errors before they happen.
Find and fix more than 250 security defects in the development cycle.
Locate bottlenecks in serial and parallel code that limit performance.
http://p.sf.net/sfu/intel-dev2devfeb
_______________________________________________
spi-devel-general mailing list
spi-devel-general@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/spi-devel-general

[PATCH 2/2] [RFC] spi/bitbang: Use a kthread worker instead of workqueue

[Uwe Kleine-König]

From: Sascha Hauer <s.hauer@pengutronix.de>

Since the concurrency managed workqueues are merged we often have
several microseconds delays in spi transfers. This patch resolves
this by using a kthread worker in spi_bitbang instead.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/spi/spi_bitbang.c       |   17 ++++++++++-------
 include/linux/spi/spi_bitbang.h |    7 ++++---
 2 files changed, 14 insertions(+), 10 deletions(-)

diff --git a/drivers/spi/spi_bitbang.c b/drivers/spi/spi_bitbang.c
index 14a63f6..e2a874a 100644
--- a/drivers/spi/spi_bitbang.c
+++ b/drivers/spi/spi_bitbang.c
@@ -254,7 +254,7 @@ static int spi_bitbang_bufs(struct spi_device *spi, struct spi_transfer *t)
  * Drivers can provide word-at-a-time i/o primitives, or provide
  * transfer-at-a-time ones to leverage dma or fifo hardware.
  */
-static void bitbang_work(struct work_struct *work)
+static void bitbang_work(struct kthread_work *work)
 {
 	struct spi_bitbang	*bitbang =
 		container_of(work, struct spi_bitbang, work);
@@ -396,7 +396,7 @@ int spi_bitbang_transfer(struct spi_device *spi, struct spi_message *m)
 		status = -ENETDOWN;
 	else {
 		list_add_tail(&m->queue, &bitbang->queue);
-		queue_work(bitbang->workqueue, &bitbang->work);
+		queue_kthread_work(&bitbang->worker, &bitbang->work);
 	}
 	spin_unlock_irqrestore(&bitbang->lock, flags);
 
@@ -432,11 +432,12 @@ EXPORT_SYMBOL_GPL(spi_bitbang_transfer);
 int spi_bitbang_start(struct spi_bitbang *bitbang)
 {
 	int	status;
+	struct sched_param param = { .sched_priority = 49 };
 
 	if (!bitbang->master || !bitbang->chipselect)
 		return -EINVAL;
 
-	INIT_WORK(&bitbang->work, bitbang_work);
+	init_kthread_work(&bitbang->work, bitbang_work);
 	spin_lock_init(&bitbang->lock);
 	INIT_LIST_HEAD(&bitbang->queue);
 
@@ -463,12 +464,14 @@ int spi_bitbang_start(struct spi_bitbang *bitbang)
 
 	/* this task is the only thing to touch the SPI bits */
 	bitbang->busy = 0;
-	bitbang->workqueue = create_singlethread_workqueue(
+	init_kthread_worker(&bitbang->worker);
+	bitbang->worker_task = kthread_run(kthread_worker_fn, &bitbang->worker,
 			dev_name(bitbang->master->dev.parent));
-	if (bitbang->workqueue == NULL) {
+	if (bitbang->worker_task == NULL) {
 		status = -EBUSY;
 		goto err1;
 	}
+	sched_setscheduler(bitbang->worker_task, SCHED_FIFO, &param);
 
 	/* driver may get busy before register() returns, especially
 	 * if someone registered boardinfo for devices
@@ -480,7 +483,7 @@ int spi_bitbang_start(struct spi_bitbang *bitbang)
 	return status;
 
 err2:
-	destroy_workqueue(bitbang->workqueue);
+	kthread_stop(bitbang->worker_task);
 err1:
 	return status;
 }
@@ -495,7 +498,7 @@ int spi_bitbang_stop(struct spi_bitbang *bitbang)
 
 	WARN_ON(!list_empty(&bitbang->queue));
 
-	destroy_workqueue(bitbang->workqueue);
+	kthread_stop(bitbang->worker_task);
 
 	return 0;
 }
diff --git a/include/linux/spi/spi_bitbang.h b/include/linux/spi/spi_bitbang.h
index f987a2b..36bb3be 100644
--- a/include/linux/spi/spi_bitbang.h
+++ b/include/linux/spi/spi_bitbang.h
@@ -1,11 +1,12 @@
 #ifndef	__SPI_BITBANG_H
 #define	__SPI_BITBANG_H
 
-#include <linux/workqueue.h>
+#include <linux/kthread.h>
 
 struct spi_bitbang {
-	struct workqueue_struct	*workqueue;
-	struct work_struct	work;
+	struct kthread_worker	worker;
+	struct task_struct	*worker_task;
+	struct kthread_work	work;
 
 	spinlock_t		lock;
 	struct list_head	queue;
-- 
1.7.2.3


------------------------------------------------------------------------------
The ultimate all-in-one performance toolkit: Intel(R) Parallel Studio XE:
Pinpoint memory and threading errors before they happen.
Find and fix more than 250 security defects in the development cycle.
Locate bottlenecks in serial and parallel code that limit performance.
http://p.sf.net/sfu/intel-dev2devfeb
_______________________________________________
spi-devel-general mailing list
spi-devel-general@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/spi-devel-general

Re: [PATCH 1/2] spi/bitbang: check for setup_transfer during initialization

[Grant Likely]

On Tue, Feb 08, 2011 at 10:46:14AM +0100, Uwe Kleine-König wrote:
> From: Sascha Hauer <s.hauer-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> 
> setup_transfer is mandatory if spi_bitbang_transfer is used, so
> check for it during initialization and not each time during runtime.
> 
> Signed-off-by: Sascha Hauer <s.hauer-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>

Applied, thanks.

g.

> ---
>  drivers/spi/spi_bitbang.c |   13 ++++---------
>  1 files changed, 4 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/spi/spi_bitbang.c b/drivers/spi/spi_bitbang.c
> index 8b55724..14a63f6 100644
> --- a/drivers/spi/spi_bitbang.c
> +++ b/drivers/spi/spi_bitbang.c
> @@ -259,10 +259,6 @@ static void bitbang_work(struct work_struct *work)
>  	struct spi_bitbang	*bitbang =
>  		container_of(work, struct spi_bitbang, work);
>  	unsigned long		flags;
> -	int			(*setup_transfer)(struct spi_device *,
> -					struct spi_transfer *);
> -
> -	setup_transfer = bitbang->setup_transfer;
>  
>  	spin_lock_irqsave(&bitbang->lock, flags);
>  	bitbang->busy = 1;
> @@ -300,11 +296,7 @@ static void bitbang_work(struct work_struct *work)
>  
>  			/* init (-1) or override (1) transfer params */
>  			if (do_setup != 0) {
> -				if (!setup_transfer) {
> -					status = -ENOPROTOOPT;
> -					break;
> -				}
> -				status = setup_transfer(spi, t);
> +				status = bitbang->setup_transfer(spi, t);
>  				if (status < 0)
>  					break;
>  				if (do_setup == -1)
> @@ -465,6 +457,9 @@ int spi_bitbang_start(struct spi_bitbang *bitbang)
>  		}
>  	} else if (!bitbang->master->setup)
>  		return -EINVAL;
> +	if (bitbang->master->transfer == spi_bitbang_transfer &&
> +			!bitbang->setup_transfer)
> +		return -EINVAL;
>  
>  	/* this task is the only thing to touch the SPI bits */
>  	bitbang->busy = 0;
> -- 
> 1.7.2.3
> 
> 
> ------------------------------------------------------------------------------
> The ultimate all-in-one performance toolkit: Intel(R) Parallel Studio XE:
> Pinpoint memory and threading errors before they happen.
> Find and fix more than 250 security defects in the development cycle.
> Locate bottlenecks in serial and parallel code that limit performance.
> http://p.sf.net/sfu/intel-dev2devfeb
> _______________________________________________
> spi-devel-general mailing list
> spi-devel-general-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
> https://lists.sourceforge.net/lists/listinfo/spi-devel-general

------------------------------------------------------------------------------
The ultimate all-in-one performance toolkit: Intel(R) Parallel Studio XE:
Pinpoint memory and threading errors before they happen.
Find and fix more than 250 security defects in the development cycle.
Locate bottlenecks in serial and parallel code that limit performance.
http://p.sf.net/sfu/intel-dev2devfeb

Re: [PATCH 2/2] [RFC] spi/bitbang: Use a kthread worker instead of workqueue

[Grant Likely]

On Tue, Feb 08, 2011 at 10:46:15AM +0100, Uwe Kleine-König wrote:
> From: Sascha Hauer <s.hauer@pengutronix.de>
> 
> Since the concurrency managed workqueues are merged we often have
> several microseconds delays in spi transfers. This patch resolves
> this by using a kthread worker in spi_bitbang instead.
> 
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Hmmm, I could use some input here from folks with a better
understanding of the concurrency managed workqueues.  Is switching to
a kthread worker the right thing to do, or is it working around a bug
in the workqueue implementation that should be fixed instead?

Anyone care to chime in?

g.

> ---
>  drivers/spi/spi_bitbang.c       |   17 ++++++++++-------
>  include/linux/spi/spi_bitbang.h |    7 ++++---
>  2 files changed, 14 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/spi/spi_bitbang.c b/drivers/spi/spi_bitbang.c
> index 14a63f6..e2a874a 100644
> --- a/drivers/spi/spi_bitbang.c
> +++ b/drivers/spi/spi_bitbang.c
> @@ -254,7 +254,7 @@ static int spi_bitbang_bufs(struct spi_device *spi, struct spi_transfer *t)
>   * Drivers can provide word-at-a-time i/o primitives, or provide
>   * transfer-at-a-time ones to leverage dma or fifo hardware.
>   */
> -static void bitbang_work(struct work_struct *work)
> +static void bitbang_work(struct kthread_work *work)
>  {
>  	struct spi_bitbang	*bitbang =
>  		container_of(work, struct spi_bitbang, work);
> @@ -396,7 +396,7 @@ int spi_bitbang_transfer(struct spi_device *spi, struct spi_message *m)
>  		status = -ENETDOWN;
>  	else {
>  		list_add_tail(&m->queue, &bitbang->queue);
> -		queue_work(bitbang->workqueue, &bitbang->work);
> +		queue_kthread_work(&bitbang->worker, &bitbang->work);
>  	}
>  	spin_unlock_irqrestore(&bitbang->lock, flags);
>  
> @@ -432,11 +432,12 @@ EXPORT_SYMBOL_GPL(spi_bitbang_transfer);
>  int spi_bitbang_start(struct spi_bitbang *bitbang)
>  {
>  	int	status;
> +	struct sched_param param = { .sched_priority = 49 };
>  
>  	if (!bitbang->master || !bitbang->chipselect)
>  		return -EINVAL;
>  
> -	INIT_WORK(&bitbang->work, bitbang_work);
> +	init_kthread_work(&bitbang->work, bitbang_work);
>  	spin_lock_init(&bitbang->lock);
>  	INIT_LIST_HEAD(&bitbang->queue);
>  
> @@ -463,12 +464,14 @@ int spi_bitbang_start(struct spi_bitbang *bitbang)
>  
>  	/* this task is the only thing to touch the SPI bits */
>  	bitbang->busy = 0;
> -	bitbang->workqueue = create_singlethread_workqueue(
> +	init_kthread_worker(&bitbang->worker);
> +	bitbang->worker_task = kthread_run(kthread_worker_fn, &bitbang->worker,
>  			dev_name(bitbang->master->dev.parent));
> -	if (bitbang->workqueue == NULL) {
> +	if (bitbang->worker_task == NULL) {
>  		status = -EBUSY;
>  		goto err1;
>  	}
> +	sched_setscheduler(bitbang->worker_task, SCHED_FIFO, &param);
>  
>  	/* driver may get busy before register() returns, especially
>  	 * if someone registered boardinfo for devices
> @@ -480,7 +483,7 @@ int spi_bitbang_start(struct spi_bitbang *bitbang)
>  	return status;
>  
>  err2:
> -	destroy_workqueue(bitbang->workqueue);
> +	kthread_stop(bitbang->worker_task);
>  err1:
>  	return status;
>  }
> @@ -495,7 +498,7 @@ int spi_bitbang_stop(struct spi_bitbang *bitbang)
>  
>  	WARN_ON(!list_empty(&bitbang->queue));
>  
> -	destroy_workqueue(bitbang->workqueue);
> +	kthread_stop(bitbang->worker_task);
>  
>  	return 0;
>  }
> diff --git a/include/linux/spi/spi_bitbang.h b/include/linux/spi/spi_bitbang.h
> index f987a2b..36bb3be 100644
> --- a/include/linux/spi/spi_bitbang.h
> +++ b/include/linux/spi/spi_bitbang.h
> @@ -1,11 +1,12 @@
>  #ifndef	__SPI_BITBANG_H
>  #define	__SPI_BITBANG_H
>  
> -#include <linux/workqueue.h>
> +#include <linux/kthread.h>
>  
>  struct spi_bitbang {
> -	struct workqueue_struct	*workqueue;
> -	struct work_struct	work;
> +	struct kthread_worker	worker;
> +	struct task_struct	*worker_task;
> +	struct kthread_work	work;
>  
>  	spinlock_t		lock;
>  	struct list_head	queue;
> -- 
> 1.7.2.3
> 
> 
> ------------------------------------------------------------------------------
> The ultimate all-in-one performance toolkit: Intel(R) Parallel Studio XE:
> Pinpoint memory and threading errors before they happen.
> Find and fix more than 250 security defects in the development cycle.
> Locate bottlenecks in serial and parallel code that limit performance.
> http://p.sf.net/sfu/intel-dev2devfeb
> _______________________________________________
> spi-devel-general mailing list
> spi-devel-general@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/spi-devel-general