diff mbox

spi: core: Don't destroy master queue if we fail to create it

Message ID 1398966567-5182-1-git-send-email-broonie@kernel.org
State New
Headers show

Commit Message

Mark Brown May 1, 2014, 5:49 p.m. UTC 
From: Mark Brown <broonie@linaro.org>

If we fail to create the master queue for some reason we should not attempt
to clean it up since attempting to stop a kthread that was not created will
hang and it's just generally bad practice. Unfortunately at present we call
spi_destroy_queue() even in cases where the creation fails.

Fix this by fixing the error handling in spi_master_initialize_queue() so
that we only flag the master as queued or destroy the queue if creation
succeeded. The change to the flag is done since the general master
cleanup uses this to destroy the queue.

Reported-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
Signed-off-by: Mark Brown <broonie@linaro.org>
---

This has been compile tested only but it should hopefully be more robust
in the long term than just skipping the queue deletion inside the
destructor.

 drivers/spi/spi.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Geert Uytterhoeven May 1, 2014, 6:55 p.m. UTC | #1 
On Thu, May 1, 2014 at 7:49 PM, Mark Brown <broonie@kernel.org> wrote:
> From: Mark Brown <broonie@linaro.org>
>
> If we fail to create the master queue for some reason we should not attempt
> to clean it up since attempting to stop a kthread that was not created will
> hang and it's just generally bad practice. Unfortunately at present we call
> spi_destroy_queue() even in cases where the creation fails.
>
> Fix this by fixing the error handling in spi_master_initialize_queue() so
> that we only flag the master as queued or destroy the queue if creation
> succeeded. The change to the flag is done since the general master
> cleanup uses this to destroy the queue.
>
> Reported-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
> Signed-off-by: Mark Brown <broonie@linaro.org>
> ---
>
> This has been compile tested only but it should hopefully be more robust
> in the long term than just skipping the queue deletion inside the
> destructor.
>
>  drivers/spi/spi.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
> index 38ba75a..bc42e27 100644
> --- a/drivers/spi/spi.c
> +++ b/drivers/spi/spi.c
> @@ -1151,7 +1151,6 @@ static int spi_master_initialize_queue(struct spi_master *master)
>  {
>         int ret;
>
> -       master->queued = true;
>         master->transfer = spi_queued_transfer;
>         if (!master->transfer_one_message)
>                 master->transfer_one_message = spi_transfer_one_message;
> @@ -1162,6 +1161,7 @@ static int spi_master_initialize_queue(struct spi_master *master)
>                 dev_err(&master->dev, "problem initializing queue\n");
>                 goto err_init_queue;

You can just "return ret;" here...

>         }
> +       master->queued = true;
>         ret = spi_start_queue(master);
>         if (ret) {
>                 dev_err(&master->dev, "problem starting queue\n");
> @@ -1171,8 +1171,8 @@ static int spi_master_initialize_queue(struct spi_master *master)
>         return 0;
>
>  err_start_queue:
> -err_init_queue:
>         spi_destroy_queue(master);
> +err_init_queue:

... and kill the err_init_queue label.

>         return ret;
>  }

Apart from that:
Acked-by: Geert Uytterhoeven <geert@linux-m68k.org>

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mark Brown May 1, 2014, 7:03 p.m. UTC | #2 
On Thu, May 01, 2014 at 08:55:42PM +0200, Geert Uytterhoeven wrote:
> On Thu, May 1, 2014 at 7:49 PM, Mark Brown <broonie@kernel.org> wrote:
> > From: Mark Brown <broonie@linaro.org>

> > @@ -1162,6 +1161,7 @@ static int spi_master_initialize_queue(struct spi_master *master)
> >                 dev_err(&master->dev, "problem initializing queue\n");
> >                 goto err_init_queue;

> You can just "return ret;" here...

> >  err_start_queue:
> > -err_init_queue:
> >         spi_destroy_queue(master);
> > +err_init_queue:

> ... and kill the err_init_queue label.

Yeah, I know.  It's a stylistic thing - I tend to prefer to always use
labels if they're used at all in a function even if we could return
directly since it's more consistent.  Not that I'm 100% consistent about
that.

> Apart from that:
> Acked-by: Geert Uytterhoeven <geert@linux-m68k.org>

Thanks.
diff mbox

Patch

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 38ba75a..bc42e27 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -1151,7 +1151,6 @@  static int spi_master_initialize_queue(struct spi_master *master)
 {
 	int ret;
 
-	master->queued = true;
 	master->transfer = spi_queued_transfer;
 	if (!master->transfer_one_message)
 		master->transfer_one_message = spi_transfer_one_message;
@@ -1162,6 +1161,7 @@  static int spi_master_initialize_queue(struct spi_master *master)
 		dev_err(&master->dev, "problem initializing queue\n");
 		goto err_init_queue;
 	}
+	master->queued = true;
 	ret = spi_start_queue(master);
 	if (ret) {
 		dev_err(&master->dev, "problem starting queue\n");
@@ -1171,8 +1171,8 @@  static int spi_master_initialize_queue(struct spi_master *master)
 	return 0;
 
 err_start_queue:
-err_init_queue:
 	spi_destroy_queue(master);
+err_init_queue:
 	return ret;
 }