[PATCH 0/1] Fix NULL pointer dereference

[PATCH 0/1] Fix NULL pointer dereference

[Pelle Nilsson]

As I reported earlier, spi-altera driver is broken and causes a kernel
panic due to a NULL pointer dereference during first SPI
transaction. On a closer look, it turned out that the setup_transfer()
bitbang callback is mandatory when the txrx_bufs() callback is
present. It was therefore an error to remove it in commit
30af9b558a56. This patch simply adds back an empty implementation of
the callback.

Pelle Nilsson (1):
  spi: altera: Add empty implementation of setup_transfer callback

 drivers/spi/spi-altera.c | 6 ++++++
 1 file changed, 6 insertions(+)

-- 
1.9.1

[PATCH 1/1] spi: altera: Add empty implementation of setup_transfer callback

[Pelle Nilsson]

This callback is mandatory since txrx_bufs callback is defined. The lack of
it causes a kernel panic on first SPI transaction.

Signed-off-by: Pelle Nilsson <per.nilsson@xelmo.com>
---
 drivers/spi/spi-altera.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/spi/spi-altera.c b/drivers/spi/spi-altera.c
index b95010e..6d72b1d 100644
--- a/drivers/spi/spi-altera.c
+++ b/drivers/spi/spi-altera.c
@@ -197,6 +197,11 @@ static irqreturn_t altera_spi_irq(int irq, void *dev)
 	return IRQ_HANDLED;
 }
 
+static int altera_spi_setupxfer(struct spi_device *spi, struct spi_transfer *t)
+{
+	return 0;
+}
+
 static int altera_spi_probe(struct platform_device *pdev)
 {
 	struct altera_spi *hw;
@@ -220,6 +225,7 @@ static int altera_spi_probe(struct platform_device *pdev)
 
 	/* setup the state for the bitbang driver */
 	hw->bitbang.master = master;
+	hw->bitbang.setup_transfer = altera_spi_setupxfer;
 	hw->bitbang.chipselect = altera_spi_chipsel;
 	hw->bitbang.txrx_bufs = altera_spi_txrx;
 
-- 
1.9.1

Re: [PATCH 1/1] spi: altera: Add empty implementation of setup_transfer callback

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 260 bytes --]

On Thu, Apr 09, 2015 at 05:03:42PM +0200, Pelle Nilsson wrote:

> This callback is mandatory since txrx_bufs callback is defined. The lack of
> it causes a kernel panic on first SPI transaction.

Why is the callback mandatory if an empty implementation is OK?

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 1/1] spi: altera: Add empty implementation of setup_transfer callback

[Pelle Nilsson]

On 2015-04-09 17:40, Mark Brown wrote:
> Why is the callback mandatory if an empty implementation is OK?

Ask the author of spi-bitbang. :-)

In spi_bitbang_start() we have this chunk of code:

	if (!bitbang->txrx_bufs) {
		bitbang->use_dma = 0;
		bitbang->txrx_bufs = spi_bitbang_bufs;
		if (!master->setup) {
			if (!bitbang->setup_transfer)
				bitbang->setup_transfer =
					 spi_bitbang_setup_transfer;
			master->setup = spi_bitbang_setup;
			master->cleanup = spi_bitbang_cleanup;
		}
	}

As can be seen here, if setup_transfer is NULL (not set by the
specific driver), it is filled in with the default callback function
spi_bitbang_setup_transfer(), but only if txrx_bufs is also NULL,
which is not the case here.

There is a comment in spi-xilinx also stating this fact (though their
implementation isn't actually empty anymore):

/* spi_bitbang requires custom setup_transfer() to be defined if there
is a
 * custom txrx_bufs().
 */

Re: [PATCH 1/1] spi: altera: Add empty implementation of setup_transfer callback

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 380 bytes --]

On Thu, Apr 09, 2015 at 05:50:52PM +0200, Pelle Nilsson wrote:
> On 2015-04-09 17:40, Mark Brown wrote:
> > Why is the callback mandatory if an empty implementation is OK?

> Ask the author of spi-bitbang. :-)

What I'm trying to suggest is that you're perhaps fixing the wrong
problem - make the callback optional if there's a good reason for it.
Why is this patch the best fix?

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 1/1] spi: altera: Add empty implementation of setup_transfer callback

[Pelle Nilsson]

On 2015-04-09 18:10, Mark Brown wrote:
> On Thu, Apr 09, 2015 at 05:50:52PM +0200, Pelle Nilsson wrote: What
> I'm trying to suggest is that you're perhaps fixing the wrong 
> problem - make the callback optional if there's a good reason for
> it.

Yes, the callback can be made optional, I'll send a new patch.

[PATCH] spi: bitbang: Make setup_transfer callback optional

[Pelle Nilsson]

spi-altera doesn't register this callback, and doesn't have a need for it,
but currently causes a NULL dereference when callback is called.
---
 drivers/spi/spi-bitbang.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/drivers/spi/spi-bitbang.c b/drivers/spi/spi-bitbang.c
index 5ef6638..d02057d 100644
--- a/drivers/spi/spi-bitbang.c
+++ b/drivers/spi/spi-bitbang.c
@@ -180,7 +180,7 @@ int spi_bitbang_setup(struct spi_device *spi)
 {
 	struct spi_bitbang_cs	*cs = spi->controller_state;
 	struct spi_bitbang	*bitbang;
-	int			retval;
+	int			retval = 0;
 	unsigned long		flags;
 
 	bitbang = spi_master_get_devdata(spi->master);
@@ -197,7 +197,8 @@ int spi_bitbang_setup(struct spi_device *spi)
 	if (!cs->txrx_word)
 		return -EINVAL;
 
-	retval = bitbang->setup_transfer(spi, NULL);
+	if (bitbang->setup_transfer)
+		retval = bitbang->setup_transfer(spi, NULL);
 	if (retval < 0)
 		return retval;
 
@@ -295,9 +296,11 @@ static int spi_bitbang_transfer_one(struct spi_master *master,
 
 		/* init (-1) or override (1) transfer params */
 		if (do_setup != 0) {
-			status = bitbang->setup_transfer(spi, t);
-			if (status < 0)
-				break;
+			if (bitbang->setup_transfer) {
+				status = bitbang->setup_transfer(spi, t);
+				if (status < 0)
+					break;
+			}
 			if (do_setup == -1)
 				do_setup = 0;
 		}
-- 
1.9.1

Re: [Nios2-dev] [PATCH] spi: bitbang: Make setup_transfer callback optional

[Ezequiel Garcia]

Hi Pelle,

Just some minor nitpicks.

On 04/13/2015 07:08 AM, Pelle Nilsson wrote:
> spi-altera doesn't register this callback, and doesn't have a need for it,
> but currently causes a NULL dereference when callback is called.

This commit needs a Signed-off-by tag.

Also, maybe you can avoid talking about spi-altera in the commit, since
this change is not related to it. Instead you can explain that some SPI
controllers don't register the callback (e.g. spi-altera) and so this
commit makes it optional to prevent a NULL dereference.

You explained in the cover that you are fixing commit 30af9b558a56 so
you probably want to add a Fixes tag (Documentation/SubmittingPatches
explains this procedure). The stable team will use the Fixes tag to
backport the change if needed.

> ---
>  drivers/spi/spi-bitbang.c | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/spi/spi-bitbang.c b/drivers/spi/spi-bitbang.c
> index 5ef6638..d02057d 100644
> --- a/drivers/spi/spi-bitbang.c
> +++ b/drivers/spi/spi-bitbang.c
> @@ -180,7 +180,7 @@ int spi_bitbang_setup(struct spi_device *spi)
>  {
>  	struct spi_bitbang_cs	*cs = spi->controller_state;
>  	struct spi_bitbang	*bitbang;
> -	int			retval;
> +	int			retval = 0;
>  	unsigned long		flags;
>  
>  	bitbang = spi_master_get_devdata(spi->master);
> @@ -197,7 +197,8 @@ int spi_bitbang_setup(struct spi_device *spi)
>  	if (!cs->txrx_word)
>  		return -EINVAL;
>  
> -	retval = bitbang->setup_transfer(spi, NULL);
> +	if (bitbang->setup_transfer)

retval is only used in this scope, so you can declare it only here:

        if (bitbang->setup_transfer) {
                int retval = bitbang->setup_transfer(spi, NULL);
                if (retval < 0)
                        return retval;
        }

> +		retval = bitbang->setup_transfer(spi, NULL);
>  	if (retval < 0)
>  		return retval;
>  
> @@ -295,9 +296,11 @@ static int spi_bitbang_transfer_one(struct spi_master *master,
>  
>  		/* init (-1) or override (1) transfer params */
>  		if (do_setup != 0) {
> -			status = bitbang->setup_transfer(spi, t);
> -			if (status < 0)
> -				break;
> +			if (bitbang->setup_transfer) {
> +				status = bitbang->setup_transfer(spi, t);
> +				if (status < 0)
> +					break;
> +			}
>  			if (do_setup == -1)
>  				do_setup = 0;
>  		}
> 

Other than that, the commit looks good.
-- 
Ezequiel Garcia, VanguardiaSur
www.vanguardiasur.com.ar

[PATCH v2] spi: bitbang: Make setup_transfer() callback optional

[Pelle Nilsson]

Some controller drivers have no need of this callback (spi-altera even
causes a NULL pointer dereference because it doesn't register the callback,
falsely assuming that it is already optional).

Fixes: 30af9b558a56 ("spi/bitbang: Drop empty setup() functions")
Signed-off-by: Pelle Nilsson <per.nilsson@xelmo.com>
---
 drivers/spi/spi-bitbang.c | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/drivers/spi/spi-bitbang.c b/drivers/spi/spi-bitbang.c
index 5ef6638..840a498 100644
--- a/drivers/spi/spi-bitbang.c
+++ b/drivers/spi/spi-bitbang.c
@@ -180,7 +180,6 @@ int spi_bitbang_setup(struct spi_device *spi)
 {
 	struct spi_bitbang_cs	*cs = spi->controller_state;
 	struct spi_bitbang	*bitbang;
-	int			retval;
 	unsigned long		flags;
 
 	bitbang = spi_master_get_devdata(spi->master);
@@ -197,9 +196,11 @@ int spi_bitbang_setup(struct spi_device *spi)
 	if (!cs->txrx_word)
 		return -EINVAL;
 
-	retval = bitbang->setup_transfer(spi, NULL);
-	if (retval < 0)
-		return retval;
+	if (bitbang->setup_transfer) {
+		int retval = bitbang->setup_transfer(spi, NULL);
+		if (retval < 0)
+			return retval;
+	}
 
 	dev_dbg(&spi->dev, "%s, %u nsec/bit\n", __func__, 2 * cs->nsecs);
 
@@ -295,9 +296,11 @@ static int spi_bitbang_transfer_one(struct spi_master *master,
 
 		/* init (-1) or override (1) transfer params */
 		if (do_setup != 0) {
-			status = bitbang->setup_transfer(spi, t);
-			if (status < 0)
-				break;
+			if (bitbang->setup_transfer) {
+				status = bitbang->setup_transfer(spi, t);
+				if (status < 0)
+					break;
+			}
 			if (do_setup == -1)
 				do_setup = 0;
 		}
-- 
1.9.1

Re: [PATCH v2] spi: bitbang: Make setup_transfer() callback optional

[Ezequiel Garcia]

On 04/14/2015 10:40 AM, Pelle Nilsson wrote:
> Some controller drivers have no need of this callback (spi-altera even
> causes a NULL pointer dereference because it doesn't register the callback,
> falsely assuming that it is already optional).
> 
> Fixes: 30af9b558a56 ("spi/bitbang: Drop empty setup() functions")
> Signed-off-by: Pelle Nilsson <per.nilsson@xelmo.com>

Reviewed-by: Ezequiel Garcia <ezequiel.garcia@vanguardiasur.com.ar>

Please note the merge window is open, so don't expect Mark to pick
your change soon.
-- 
Ezequiel Garcia, VanguardiaSur
www.vanguardiasur.com.ar

Re: [PATCH v2] spi: bitbang: Make setup_transfer() callback optional

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 411 bytes --]

On Tue, Apr 14, 2015 at 03:40:17PM +0200, Pelle Nilsson wrote:
> Some controller drivers have no need of this callback (spi-altera even
> causes a NULL pointer dereference because it doesn't register the callback,
> falsely assuming that it is already optional).

I've applied this but really for something like this the commit message
should be explaining *why* it's reasonable for the callback to be
omitted.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 0/1] Fix NULL pointer dereference

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 450 bytes --]

On Thu, Apr 09, 2015 at 05:03:41PM +0200, Pelle Nilsson wrote:
> As I reported earlier, spi-altera driver is broken and causes a kernel
> panic due to a NULL pointer dereference during first SPI

Please don't send cover letters for single patches, it tends to mean
you've not written enough in the commit message.  Either the mail is
redundant and shouldn't be sent or there should be more in the patch
either in the commit message or after the ---.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[PATCH v2] spi: altera: Add empty implementation of setup_transfer callback

[Pelle Nilsson]

spi-altera driver is broken and causes a kernel panic due to a NULL
pointer dereference during first SPI transaction. The setup_transfer()
bitbang callback is mandatory when the txrx_bufs() callback is
present. It was therefore an error to remove it in commit
30af9b558a56. This patch simply adds back an empty implementation of
the callback.

Signed-off-by: Pelle Nilsson <per.nilsson@xelmo.com>
---
 drivers/spi/spi-altera.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/spi/spi-altera.c b/drivers/spi/spi-altera.c
index b95010e..6d72b1d 100644
--- a/drivers/spi/spi-altera.c
+++ b/drivers/spi/spi-altera.c
@@ -197,6 +197,11 @@ static irqreturn_t altera_spi_irq(int irq, void *dev)
 	return IRQ_HANDLED;
 }
 
+static int altera_spi_setupxfer(struct spi_device *spi, struct spi_transfer *t)
+{
+	return 0;
+}
+
 static int altera_spi_probe(struct platform_device *pdev)
 {
 	struct altera_spi *hw;
@@ -220,6 +225,7 @@ static int altera_spi_probe(struct platform_device *pdev)
 
 	/* setup the state for the bitbang driver */
 	hw->bitbang.master = master;
+	hw->bitbang.setup_transfer = altera_spi_setupxfer;
 	hw->bitbang.chipselect = altera_spi_chipsel;
 	hw->bitbang.txrx_bufs = altera_spi_txrx;
 
-- 
1.9.1

Re: [PATCH v2] spi: altera: Add empty implementation of setup_transfer callback

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 346 bytes --]

On Thu, Apr 09, 2015 at 05:54:13PM +0200, Pelle Nilsson wrote:

> present. It was therefore an error to remove it in commit
> 30af9b558a56. This patch simply adds back an empty implementation of

Please always provide a human readable description of commits as well as
the IDs.  As I said in reply to your previous mail why is this the best
fix?

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]