[PATCH 1/2] spi/spi_s3c64xx: Make probe more robust against missing board config

[PATCH 1/2] spi/spi_s3c64xx: Make probe more robust against missing board config

[Mark Brown]

The S3C64xx SPI driver requires the machine to call s3c64xx_spi_set_info()
to select a few options, including the clock to use for the SPI controller.
If this is not done then a NULL will be passed as the clock name for
clk_get(), causing an obscure crash. Guard against this and other missing
configuration by validating that the clock name has been filled in in
the platform data that ets passed in.

Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
---
 drivers/spi/spi_s3c64xx.c |    9 +++++++--
 1 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/spi/spi_s3c64xx.c b/drivers/spi/spi_s3c64xx.c
index 9736581..a0b63b7 100644
--- a/drivers/spi/spi_s3c64xx.c
+++ b/drivers/spi/spi_s3c64xx.c
@@ -919,6 +919,13 @@ static int __init s3c64xx_spi_probe(struct platform_device *pdev)
 		return -ENODEV;
 	}
 
+	sci = pdev->dev.platform_data;
+	if (!sci->src_clk_name) {
+		dev_err(&pdev->dev,
+			"Board init must call s3c64xx_spi_set_info()\n");
+		return -EINVAL;
+	}
+
 	/* Check for availability of necessary resource */
 
 	dmatx_res = platform_get_resource(pdev, IORESOURCE_DMA, 0);
@@ -946,8 +953,6 @@ static int __init s3c64xx_spi_probe(struct platform_device *pdev)
 		return -ENOMEM;
 	}
 
-	sci = pdev->dev.platform_data;
-
 	platform_set_drvdata(pdev, master);
 
 	sdd = spi_master_get_devdata(master);
-- 
1.7.1

[PATCH 2/2] spi/spi_s3c64xx: Staticise non-exported functions

[Mark Brown]

Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
---
 drivers/spi/spi_s3c64xx.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/spi/spi_s3c64xx.c b/drivers/spi/spi_s3c64xx.c
index a0b63b7..7e627f7 100644
--- a/drivers/spi/spi_s3c64xx.c
+++ b/drivers/spi/spi_s3c64xx.c
@@ -447,8 +447,8 @@ static void s3c64xx_spi_config(struct s3c64xx_spi_driver_data *sdd)
 	writel(val, regs + S3C64XX_SPI_CLK_CFG);
 }
 
-void s3c64xx_spi_dma_rxcb(struct s3c2410_dma_chan *chan, void *buf_id,
-				int size, enum s3c2410_dma_buffresult res)
+static void s3c64xx_spi_dma_rxcb(struct s3c2410_dma_chan *chan, void *buf_id,
+				 int size, enum s3c2410_dma_buffresult res)
 {
 	struct s3c64xx_spi_driver_data *sdd = buf_id;
 	unsigned long flags;
@@ -467,8 +467,8 @@ void s3c64xx_spi_dma_rxcb(struct s3c2410_dma_chan *chan, void *buf_id,
 	spin_unlock_irqrestore(&sdd->lock, flags);
 }
 
-void s3c64xx_spi_dma_txcb(struct s3c2410_dma_chan *chan, void *buf_id,
-				int size, enum s3c2410_dma_buffresult res)
+static void s3c64xx_spi_dma_txcb(struct s3c2410_dma_chan *chan, void *buf_id,
+				 int size, enum s3c2410_dma_buffresult res)
 {
 	struct s3c64xx_spi_driver_data *sdd = buf_id;
 	unsigned long flags;
-- 
1.7.1

Re: [PATCH 2/2] spi/spi_s3c64xx: Staticise non-exported functions

[Jassi Brar]

On Sat, Aug 21, 2010 at 1:17 AM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:
> Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
> ---
>  drivers/spi/spi_s3c64xx.c |    8 ++++----
>  1 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/spi/spi_s3c64xx.c b/drivers/spi/spi_s3c64xx.c
> index a0b63b7..7e627f7 100644
> --- a/drivers/spi/spi_s3c64xx.c
> +++ b/drivers/spi/spi_s3c64xx.c
> @@ -447,8 +447,8 @@ static void s3c64xx_spi_config(struct s3c64xx_spi_driver_data *sdd)
>        writel(val, regs + S3C64XX_SPI_CLK_CFG);
>  }
>
> -void s3c64xx_spi_dma_rxcb(struct s3c2410_dma_chan *chan, void *buf_id,
> -                               int size, enum s3c2410_dma_buffresult res)
> +static void s3c64xx_spi_dma_rxcb(struct s3c2410_dma_chan *chan, void *buf_id,
> +                                int size, enum s3c2410_dma_buffresult res)
>  {
>        struct s3c64xx_spi_driver_data *sdd = buf_id;
>        unsigned long flags;
> @@ -467,8 +467,8 @@ void s3c64xx_spi_dma_rxcb(struct s3c2410_dma_chan *chan, void *buf_id,
>        spin_unlock_irqrestore(&sdd->lock, flags);
>  }
>
> -void s3c64xx_spi_dma_txcb(struct s3c2410_dma_chan *chan, void *buf_id,
> -                               int size, enum s3c2410_dma_buffresult res)
> +static void s3c64xx_spi_dma_txcb(struct s3c2410_dma_chan *chan, void *buf_id,
> +                                int size, enum s3c2410_dma_buffresult res)
>  {
>        struct s3c64xx_spi_driver_data *sdd = buf_id;
>        unsigned long flags;

Oh, yes. What was I thinking ?!

Acked-by: Jassi Brar <jassisinghbrar@gmail.com>

------------------------------------------------------------------------------
This SF.net email is sponsored by 

Make an app they can't live without
Enter the BlackBerry Developer Challenge
http://p.sf.net/sfu/RIM-dev2dev 
_______________________________________________
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/spi_s3c64xx: Make probe more robust against missing board config

[Grant Likely]

On Fri, Aug 20, 2010 at 05:17:07PM +0100, Mark Brown wrote:
> The S3C64xx SPI driver requires the machine to call s3c64xx_spi_set_info()
> to select a few options, including the clock to use for the SPI controller.
> If this is not done then a NULL will be passed as the clock name for
> clk_get(), causing an obscure crash. Guard against this and other missing
> configuration by validating that the clock name has been filled in in
> the platform data that ets passed in.
> 
> Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com>

Both patches applied, thanks.

g.

> ---
>  drivers/spi/spi_s3c64xx.c |    9 +++++++--
>  1 files changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/spi/spi_s3c64xx.c b/drivers/spi/spi_s3c64xx.c
> index 9736581..a0b63b7 100644
> --- a/drivers/spi/spi_s3c64xx.c
> +++ b/drivers/spi/spi_s3c64xx.c
> @@ -919,6 +919,13 @@ static int __init s3c64xx_spi_probe(struct platform_device *pdev)
>  		return -ENODEV;
>  	}
>  
> +	sci = pdev->dev.platform_data;
> +	if (!sci->src_clk_name) {
> +		dev_err(&pdev->dev,
> +			"Board init must call s3c64xx_spi_set_info()\n");
> +		return -EINVAL;
> +	}
> +
>  	/* Check for availability of necessary resource */
>  
>  	dmatx_res = platform_get_resource(pdev, IORESOURCE_DMA, 0);
> @@ -946,8 +953,6 @@ static int __init s3c64xx_spi_probe(struct platform_device *pdev)
>  		return -ENOMEM;
>  	}
>  
> -	sci = pdev->dev.platform_data;
> -
>  	platform_set_drvdata(pdev, master);
>  
>  	sdd = spi_master_get_devdata(master);
> -- 
> 1.7.1
> 

Re: [spi-devel-general] [PATCH 1/2] spi/spi_s3c64xx: Make probe more robust against missing board config

[Jassi Brar]

On Sat, Aug 21, 2010 at 1:17 AM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:
> The S3C64xx SPI driver requires the machine to call s3c64xx_spi_set_info()
> to select a few options, including the clock to use for the SPI controller.
> If this is not done then a NULL will be passed as the clock name for
> clk_get(), causing an obscure crash. Guard against this and other missing
> configuration by validating that the clock name has been filled in in
> the platform data that ets passed in.

     The movement of sci assignment and check doesn't make any
difference because
      we already check for presence of platform_data and DMA-Tx,Rx and
IO base is
      set irrespective of calling s3c64xx_spi_set_info()

    Also, I think !sci->num_cs might be an even better check because
the samsung clock
   api might be changed (IIRC Ben was already working it out) to make
it redundant
   to pass clock name strings to clk_get. If that is the case, we might end up
   adding another foolproof check like !sci->num_cs

    what do you guys think?

regards

Re: [spi-devel-general] [PATCH 1/2] spi/spi_s3c64xx: Make probe more robust against missing board config

[Mark Brown]

On Sat, Aug 21, 2010 at 10:45:56AM +0900, Jassi Brar wrote:
> On Sat, Aug 21, 2010 at 1:17 AM, Mark Brown
> <broonie@opensource.wolfsonmicro.com> wrote:
> > The S3C64xx SPI driver requires the machine to call s3c64xx_spi_set_info()
> > to select a few options, including the clock to use for the SPI controller.
> > If this is not done then a NULL will be passed as the clock name for
> > clk_get(), causing an obscure crash. Guard against this and other missing
> > configuration by validating that the clock name has been filled in in
> > the platform data that ets passed in.

>      The movement of sci assignment and check doesn't make any
> difference because
>       we already check for presence of platform_data and DMA-Tx,Rx and
> IO base is
>       set irrespective of calling s3c64xx_spi_set_info()

While it does check for those things for at least the 6410 they're all
unconditionally set up by dev-spi.c so the tests all pass and we make it
down into to the clk_get() which then falls over horribly.

>     Also, I think !sci->num_cs might be an even better check because
> the samsung clock
>    api might be changed (IIRC Ben was already working it out) to make
> it redundant
>    to pass clock name strings to clk_get. If that is the case, we might end up
>    adding another foolproof check like !sci->num_cs

The problem with num_cs is that it gets interpreted by a custom function
provided by the board driver so we really can't say anything about what
it does.  If the clock API gets enhanced then we can always cope with
things then, but given the pace of development there I'd expect that
we'd need to continue checking for a while.

TBH I'm a bit surprised that the driver has to do custom stuff to
support gpiolib chip selects - my first thought when I saw that stuff
was that it seemed like something that lots of SPI controllers would be
able to share but I didn't look at the overall SPI code for long enough
to figure out what was going on there.

Re: [PATCH 1/2] spi/spi_s3c64xx: Make probe more robust against missing board config

[Jassi Brar]

On Sat, Aug 21, 2010 at 7:08 PM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:
> On Sat, Aug 21, 2010 at 10:45:56AM +0900, Jassi Brar wrote:
>> On Sat, Aug 21, 2010 at 1:17 AM, Mark Brown
>> <broonie@opensource.wolfsonmicro.com> wrote:
>> > The S3C64xx SPI driver requires the machine to call s3c64xx_spi_set_info()
>> > to select a few options, including the clock to use for the SPI controller.
>> > If this is not done then a NULL will be passed as the clock name for
>> > clk_get(), causing an obscure crash. Guard against this and other missing
>> > configuration by validating that the clock name has been filled in in
>> > the platform data that ets passed in.
>
>>      The movement of sci assignment and check doesn't make any
>> difference because
>>       we already check for presence of platform_data and DMA-Tx,Rx and
>> IO base is
>>       set irrespective of calling s3c64xx_spi_set_info()
>
> While it does check for those things for at least the 6410 they're all
> unconditionally set up by dev-spi.c so the tests all pass and we make it
> down into to the clk_get() which then falls over horribly.
Those parameters are SoC specific and hence will be always available to
platform devices.
Clock selection and num_cs(number of slaves attached to the SPI bus) are
machine specific and hence responsibility of the machine developer to
set appropriately via s3c64xx_spi_set_info()


>>     Also, I think !sci->num_cs might be an even better check because
>> the samsung clock
>>    api might be changed (IIRC Ben was already working it out) to make
>> it redundant
>>    to pass clock name strings to clk_get. If that is the case, we might end up
>>    adding another foolproof check like !sci->num_cs
>
> The problem with num_cs is that it gets interpreted by a custom function
> provided by the board driver so we really can't say anything about what
> it does.
num_cs is passed on as such to the SPI core to master->num_chipselect
which is strictly checked for before a master is created.

>  If the clock API gets enhanced then we can always cope with
> things then, but given the pace of development there I'd expect that
> we'd need to continue checking for a while.
yes, so do i think.

> TBH I'm a bit surprised that the driver has to do custom stuff to
> support gpiolib chip selects - my first thought when I saw that stuff
> was that it seemed like something that lots of SPI controllers would be
> able to share but I didn't look at the overall SPI code for long enough
> to figure out what was going on there.
we have common spi bitbanging driver that takes only four gpio pins
and work like a charm.
In order to enable users use multiple CS per master, in spi_s3c64xx.c
the single CS feature provided by the controller is deliberately left unused
and the driver accepts a gpio per slave and manually controls it. Though
the callbacks and arguments are designed so that a simple gpio number
and gpio_set can be assigned by the machine code.

Anyways, I think we can keep the patch as it is, if it's already
applied in Grant's
tree.

------------------------------------------------------------------------------
This SF.net email is sponsored by 

Make an app they can't live without
Enter the BlackBerry Developer Challenge
http://p.sf.net/sfu/RIM-dev2dev 
_______________________________________________
spi-devel-general mailing list
spi-devel-general@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/spi-devel-general

Re: [spi-devel-general] [PATCH 1/2] spi/spi_s3c64xx: Make probe more robust against missing board config

[Mark Brown]

On Sat, Aug 21, 2010 at 10:58:36PM +0900, Jassi Brar wrote:
> On Sat, Aug 21, 2010 at 7:08 PM, Mark Brown
> > On Sat, Aug 21, 2010 at 10:45:56AM +0900, Jassi Brar wrote:

> >>      The movement of sci assignment and check doesn't make any
> >> difference because
> >>       we already check for presence of platform_data and DMA-Tx,Rx and
> >> IO base is
> >>       set irrespective of calling s3c64xx_spi_set_info()

> > While it does check for those things for at least the 6410 they're all
> > unconditionally set up by dev-spi.c so the tests all pass and we make it
> > down into to the clk_get() which then falls over horribly.

> Those parameters are SoC specific and hence will be always available to
> platform devices.
> Clock selection and num_cs(number of slaves attached to the SPI bus) are
> machine specific and hence responsibility of the machine developer to
> set appropriately via s3c64xx_spi_set_info()

Sure, the split does make sense - it's just that this means that we need
to do something to make sure that s3c64xx_spi_set_info() got called.

> > The problem with num_cs is that it gets interpreted by a custom function
> > provided by the board driver so we really can't say anything about what
> > it does.

> num_cs is passed on as such to the SPI core to master->num_chipselect
> which is strictly checked for before a master is created.

Ah, too many variables with similar functions.

> > support gpiolib chip selects - my first thought when I saw that stuff
> > was that it seemed like something that lots of SPI controllers would be
> > able to share but I didn't look at the overall SPI code for long enough
> > to figure out what was going on there.

> we have common spi bitbanging driver that takes only four gpio pins
> and work like a charm.

Though it does burn CPU rather a lot which makes them unsuitable for
some applications where SPI is used for bulk data transfers or CPU is
otherwise tight.  In general if you've got an actual SPI controller it's
much nicer to use it.

> In order to enable users use multiple CS per master, in spi_s3c64xx.c
> the single CS feature provided by the controller is deliberately left unused
> and the driver accepts a gpio per slave and manually controls it. Though
> the callbacks and arguments are designed so that a simple gpio number
> and gpio_set can be assigned by the machine code.

It did occur to me that a nice way of dealing with this would be to have
the driver default to using the built in chip select (even if driven as
a GPIO for the sake of code simplicity) but leave the current method
there as an override.  That way if people are using the IP block in the
"natural" manner they'd have less to set up.

Re: [spi-devel-general] [PATCH 1/2] spi/spi_s3c64xx: Make probe more robust against missing board config

[Jassi Brar]

On Sun, Aug 22, 2010 at 4:30 AM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:
> On Sat, Aug 21, 2010 at 10:58:36PM +0900, Jassi Brar wrote:
>> On Sat, Aug 21, 2010 at 7:08 PM, Mark Brown
>> > On Sat, Aug 21, 2010 at 10:45:56AM +0900, Jassi Brar wrote:
>
>> >>      The movement of sci assignment and check doesn't make any
>> >> difference because
>> >>       we already check for presence of platform_data and DMA-Tx,Rx and
>> >> IO base is
>> >>       set irrespective of calling s3c64xx_spi_set_info()
>
>> > While it does check for those things for at least the 6410 they're all
>> > unconditionally set up by dev-spi.c so the tests all pass and we make it
>> > down into to the clk_get() which then falls over horribly.
>
>> Those parameters are SoC specific and hence will be always available to
>> platform devices.
>> Clock selection and num_cs(number of slaves attached to the SPI bus) are
>> machine specific and hence responsibility of the machine developer to
>> set appropriately via s3c64xx_spi_set_info()
>
> Sure, the split does make sense - it's just that this means that we need
> to do something to make sure that s3c64xx_spi_set_info() got called.
An immediate kernel crash ? :)
I mean if the developer didn't even run-check the board init code, he ought
to face a kernel crash.
On a more lenient note, probably a check like yours or !sci->num_cs
could be added.

>> > The problem with num_cs is that it gets interpreted by a custom function
>> > provided by the board driver so we really can't say anything about what
>> > it does.
>
>> num_cs is passed on as such to the SPI core to master->num_chipselect
>> which is strictly checked for before a master is created.
>
> Ah, too many variables with similar functions.
Can't think of a way to go around it.

>> > support gpiolib chip selects - my first thought when I saw that stuff
>> > was that it seemed like something that lots of SPI controllers would be
>> > able to share but I didn't look at the overall SPI code for long enough
>> > to figure out what was going on there.
>
>> we have common spi bitbanging driver that takes only four gpio pins
>> and work like a charm.
>
> Though it does burn CPU rather a lot which makes them unsuitable for
> some applications where SPI is used for bulk data transfers or CPU is
> otherwise tight.  In general if you've got an actual SPI controller it's
> much nicer to use it.
of course, and we do use whenever the CLK, MISO and MOSI signals could
be driven by the controller.

>> In order to enable users use multiple CS per master, in spi_s3c64xx.c
>> the single CS feature provided by the controller is deliberately left unused
>> and the driver accepts a gpio per slave and manually controls it. Though
>> the callbacks and arguments are designed so that a simple gpio number
>> and gpio_set can be assigned by the machine code.
>
> It did occur to me that a nice way of dealing with this would be to have
> the driver default to using the built in chip select (even if driven as
> a GPIO for the sake of code simplicity) but leave the current method
> there as an override.  That way if people are using the IP block in the
> "natural" manner they'd have less to set up.
I thought about it but decided against it, for that would complicate the
driver by having to switch between two mechanism in runtime and there
are some peculiarities in the controller about clocking and CS'ing.

Re: [PATCH 1/2] spi/spi_s3c64xx: Make probe more robust against missing board config

[Mark Brown]

On Sun, Aug 22, 2010 at 12:37:25PM +0900, Jassi Brar wrote:

> An immediate kernel crash ? :)
> I mean if the developer didn't even run-check the board init code, he ought
> to face a kernel crash.
> On a more lenient note, probably a check like yours or !sci->num_cs
> could be added.

I'd personally not actually be that upset with a BUG_ON(), my main
reason for changing the code was that it was non-obvious what the source
of the error was rather than the fact that things fell over.

> > It did occur to me that a nice way of dealing with this would be to have
> > the driver default to using the built in chip select (even if driven as
> > a GPIO for the sake of code simplicity) but leave the current method
> > there as an override.  That way if people are using the IP block in the
> > "natural" manner they'd have less to set up.

> I thought about it but decided against it, for that would complicate the
> driver by having to switch between two mechanism in runtime and there
> are some peculiarities in the controller about clocking and CS'ing.

Yeah, that's why I suggested driving it as a GPIO for simplicity - from
the user point of view it doesn't matter if that's what the controller
does so long as the driver figures out the chip select without external
help.

------------------------------------------------------------------------------
This SF.net email is sponsored by 

Make an app they can't live without
Enter the BlackBerry Developer Challenge
http://p.sf.net/sfu/RIM-dev2dev