diff mbox

[2/3] spi: s3c64xx: validate s3c64xx_spi_csinfo before using

Message ID 1405426860-18404-3-git-send-email-ch.naveen@samsung.com (mailing list archive)
State New, archived
Headers show

Commit Message

Naveen Krishna Chatradhi July 15, 2014, 12:20 p.m. UTC
This patch validates the cs->line (Chip select gpio) and
struct s3c64xx_spi_csinfo *cs object for both DT and NON-DT
platforms before using in .setup().

Also, check gpio_is_valid(spi->cs_gpio) in cleanup() before
freeing up.

Signed-off-by: Naveen Krishna Chatradhi <ch.naveen@samsung.com>
Cc: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
Cc: Doug Anderson <dianders@chromium.org>
---
 drivers/spi/spi-s3c64xx.c |   15 ++++-----------
 1 file changed, 4 insertions(+), 11 deletions(-)

Comments

Tomasz Figa July 15, 2014, 5:49 p.m. UTC | #1
Hi Naveen,

Please see my comments inline.

On 15.07.2014 14:20, Naveen Krishna Chatradhi wrote:
> This patch validates the cs->line (Chip select gpio) and
> struct s3c64xx_spi_csinfo *cs object for both DT and NON-DT
> platforms before using in .setup().
> 
> Also, check gpio_is_valid(spi->cs_gpio) in cleanup() before
> freeing up.

Missing reason of the change.

> 
> Signed-off-by: Naveen Krishna Chatradhi <ch.naveen@samsung.com>
> Cc: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
> Cc: Doug Anderson <dianders@chromium.org>
> ---
>  drivers/spi/spi-s3c64xx.c |   15 ++++-----------
>  1 file changed, 4 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
> index 72bfba6..8971076 100644
> --- a/drivers/spi/spi-s3c64xx.c
> +++ b/drivers/spi/spi-s3c64xx.c
> @@ -773,12 +773,6 @@ static struct s3c64xx_spi_csinfo *s3c64xx_get_slave_ctrldata(
>  	/* The CS line is asserted/deasserted by the gpio pin */
>  	cs->line = spi->cs_gpio;
>  
> -	if (!gpio_is_valid(cs->line)) {
> -		dev_err(&spi->dev, "chip select gpio is not specified or invalid\n");
> -		kfree(cs);
> -		return ERR_PTR(-EINVAL);
> -	}
> -
>  	data_np = of_get_child_by_name(slave_np, "controller-data");
>  	if (!data_np) {
>  		dev_err(&spi->dev, "child node 'controller-data' not found\n");
> @@ -805,15 +799,14 @@ static int s3c64xx_spi_setup(struct spi_device *spi)
>  	int err;
>  
>  	sdd = spi_master_get_devdata(spi->master);
> -	if (!cs && spi->dev.of_node) {
> +	if (spi->dev.of_node)

This check is equivalent, i.e. cs will always be NULL whenever
spi->dev.of_node is not.

>  		cs = s3c64xx_get_slave_ctrldata(spi);
> -		spi->controller_data = cs;
> -	}
>  
> -	if (IS_ERR_OR_NULL(cs)) {
> +	if (IS_ERR_OR_NULL(cs) || !gpio_is_valid(cs->line)) {

I'm not sure this is correct. It will error out even if hardware chip
select is used.

Otherwise you need to free cs here if wrong GPIO was the cause of
entering this block, so if this extra check is to stay, I'd suggest
splitting this into two separate if blocks.

>  		dev_err(&spi->dev, "No CS for SPI(%d)\n", spi->chip_select);
>  		return -ENODEV;
>  	}
> +	spi->controller_data = cs;

I'm not sure what's the point of moving this assignment here.

>  
>  	if (!spi_get_ctldata(spi)) {
>  		/* Request gpio only if cs line is asserted by gpio pins */
> @@ -898,7 +891,7 @@ static void s3c64xx_spi_cleanup(struct spi_device *spi)
>  	struct s3c64xx_spi_driver_data *sdd;
>  
>  	sdd = spi_master_get_devdata(spi->master);
> -	if (spi->cs_gpio) {
> +	if (gpio_is_valid(spi->cs_gpio)) {
>  		gpio_free(spi->cs_gpio);
>  		if (spi->dev.of_node)
>  			kfree(cs);

I believe this is completely wrong. cs is allocated even if GPIO chip
select is not used, so the only thing that should be done conditionally
is gpio_free().

In general, I liked previous version of this series much more, as it was
doing what it should as opposed to this one.

Best regards,
Tomasz
Javier Martinez Canillas July 15, 2014, 8 p.m. UTC | #2
Hello Naveen,

On 07/15/2014 07:49 PM, Tomasz Figa wrote:
> 
> In general, I liked previous version of this series much more, as it was
> doing what it should as opposed to this one.
> 
> Best regards,
> Tomasz
> 

I agree with Tomasz. I think version v6 of your series was (almost) correct
while this is one is not. You only had to address Mark's concerns about using
gpio_is_valid(spi->cs_gpio).

Also, you need to work out your commit messages since I they are still not
clearly explaining the motivation for the changes. You are explaining what the
patches are changing but you have to explain why the changes are needed in the
first place.

Best regards,
Javier
diff mbox

Patch

diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
index 72bfba6..8971076 100644
--- a/drivers/spi/spi-s3c64xx.c
+++ b/drivers/spi/spi-s3c64xx.c
@@ -773,12 +773,6 @@  static struct s3c64xx_spi_csinfo *s3c64xx_get_slave_ctrldata(
 	/* The CS line is asserted/deasserted by the gpio pin */
 	cs->line = spi->cs_gpio;
 
-	if (!gpio_is_valid(cs->line)) {
-		dev_err(&spi->dev, "chip select gpio is not specified or invalid\n");
-		kfree(cs);
-		return ERR_PTR(-EINVAL);
-	}
-
 	data_np = of_get_child_by_name(slave_np, "controller-data");
 	if (!data_np) {
 		dev_err(&spi->dev, "child node 'controller-data' not found\n");
@@ -805,15 +799,14 @@  static int s3c64xx_spi_setup(struct spi_device *spi)
 	int err;
 
 	sdd = spi_master_get_devdata(spi->master);
-	if (!cs && spi->dev.of_node) {
+	if (spi->dev.of_node)
 		cs = s3c64xx_get_slave_ctrldata(spi);
-		spi->controller_data = cs;
-	}
 
-	if (IS_ERR_OR_NULL(cs)) {
+	if (IS_ERR_OR_NULL(cs) || !gpio_is_valid(cs->line)) {
 		dev_err(&spi->dev, "No CS for SPI(%d)\n", spi->chip_select);
 		return -ENODEV;
 	}
+	spi->controller_data = cs;
 
 	if (!spi_get_ctldata(spi)) {
 		/* Request gpio only if cs line is asserted by gpio pins */
@@ -898,7 +891,7 @@  static void s3c64xx_spi_cleanup(struct spi_device *spi)
 	struct s3c64xx_spi_driver_data *sdd;
 
 	sdd = spi_master_get_devdata(spi->master);
-	if (spi->cs_gpio) {
+	if (gpio_is_valid(spi->cs_gpio)) {
 		gpio_free(spi->cs_gpio);
 		if (spi->dev.of_node)
 			kfree(cs);