[V1,1/3] spi: core: resource: fix memory leak on error and place in "correct" sequence
diff mbox series

Message ID 20190509105533.24275-2-kernel@martin.sperl.org
State New, archived
Headers show
Series
  • spi: core: correct ordering of unprepare and resources
Related show

Commit Message

Martin Sperl May 9, 2019, 10:55 a.m. UTC
From: Martin Sperl <kernel@martin.sperl.org>

When an error occurs in ctlr->prepare_message or spi_map_msg
then spi_resources is not cleared leaving unexpected entries and
memory.

Also there is an ordering issue:
On ititialization:
* prepare_message
* spi_map_msg

and when releasing:
* spi_res_release (outside of finalize)
  -> this restores the spi structures
* spi_unmap_msg
* unprepare_message

So the ordering is wrong in the case that prepare_message is
modifying the spi_message and spi_message.resources.

Especially the dma unmapping of all the translations are not done.

There is still an ambiguity where is the "best" place where to place
spi_res_release - it definitely has to be after spi_unmap_msg,
but if it should be before or after unprepare message is not well
defined.

Ideally this dma unmap and unprepare_message would be executed
by spi_res_release and thus in the guaranteed order they were applied.

This incidently implements a better way for the reverted
commit c9ba7a16d0f1 ("spi: Release spi_res after finalizing message")

Signed-off-by: Martin Sperl <kernel@martin.sperl.org>
---
 drivers/spi/spi.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

--
2.11.0

Comments

Nicolas Saenz Julienne May 9, 2019, 11:17 a.m. UTC | #1
On Thu, 2019-05-09 at 10:55 +0000, kernel@martin.sperl.org wrote:
> From: Martin Sperl <kernel@martin.sperl.org>
> 
> When an error occurs in ctlr->prepare_message or spi_map_msg
> then spi_resources is not cleared leaving unexpected entries and
> memory.
> 
> Also there is an ordering issue:
> On ititialization:
> * prepare_message
> * spi_map_msg
> 
> and when releasing:
> * spi_res_release (outside of finalize)
>   -> this restores the spi structures
> * spi_unmap_msg
> * unprepare_message
> 
> So the ordering is wrong in the case that prepare_message is
> modifying the spi_message and spi_message.resources.
> 
> Especially the dma unmapping of all the translations are not done.
> 
> There is still an ambiguity where is the "best" place where to place
> spi_res_release - it definitely has to be after spi_unmap_msg,
> but if it should be before or after unprepare message is not well
> defined.
> 
> Ideally this dma unmap and unprepare_message would be executed
> by spi_res_release and thus in the guaranteed order they were applied.
> 
> This incidently implements a better way for the reverted
> commit c9ba7a16d0f1 ("spi: Release spi_res after finalizing message")
> 
> Signed-off-by: Martin Sperl <kernel@martin.sperl.org>
> ---
>  drivers/spi/spi.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
> index 8eb7460dd744..1dfb19140bbe 100644
> --- a/drivers/spi/spi.c
> +++ b/drivers/spi/spi.c
> @@ -1181,8 +1181,6 @@ static int spi_transfer_one_message(struct
> spi_controller *ctlr,
>  	if (msg->status && ctlr->handle_err)
>  		ctlr->handle_err(ctlr, msg);
> 
> -	spi_res_release(ctlr, msg);
> -
>  	spi_finalize_current_message(ctlr);
> 
>  	return ret;
> @@ -1448,6 +1446,15 @@ void spi_finalize_current_message(struct spi_controller
> *ctlr)
>  		}
>  	}
> 
> +	/* where to put the release is a slight nightmare because
> +	 * ctlr->prepare_message may add to resources as well.
> +	 * so the question is: call it before unprepare or after?
> +	 * for now leave it after - the asumption here is that
> +	 * if prepare_message is using spi_res for callbacks,
> +	 * then no unprepare_message is used
> +	 */
> +	spi_res_release(ctlr, mesg);
> +
>  	spin_lock_irqsave(&ctlr->queue_lock, flags);
>  	ctlr->cur_msg = NULL;
>  	ctlr->cur_msg_prepared = false;
> --
> 2.11.0

Tested-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>
Noralf Trønnes May 9, 2019, 1:53 p.m. UTC | #2
Den 09.05.2019 12.55, skrev kernel@martin.sperl.org:
> From: Martin Sperl <kernel@martin.sperl.org>
> 
> When an error occurs in ctlr->prepare_message or spi_map_msg
> then spi_resources is not cleared leaving unexpected entries and
> memory.
> 
> Also there is an ordering issue:
> On ititialization:
> * prepare_message
> * spi_map_msg
> 
> and when releasing:
> * spi_res_release (outside of finalize)
>   -> this restores the spi structures
> * spi_unmap_msg
> * unprepare_message
> 
> So the ordering is wrong in the case that prepare_message is
> modifying the spi_message and spi_message.resources.
> 
> Especially the dma unmapping of all the translations are not done.
> 
> There is still an ambiguity where is the "best" place where to place
> spi_res_release - it definitely has to be after spi_unmap_msg,
> but if it should be before or after unprepare message is not well
> defined.
> 
> Ideally this dma unmap and unprepare_message would be executed
> by spi_res_release and thus in the guaranteed order they were applied.
> 
> This incidently implements a better way for the reverted
> commit c9ba7a16d0f1 ("spi: Release spi_res after finalizing message")
> 
> Signed-off-by: Martin Sperl <kernel@martin.sperl.org>
> ---

Thanks for sorting this out Martin.

Tested-by: Noralf Trønnes <noralf@tronnes.org>

>  drivers/spi/spi.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
> index 8eb7460dd744..1dfb19140bbe 100644
> --- a/drivers/spi/spi.c
> +++ b/drivers/spi/spi.c
> @@ -1181,8 +1181,6 @@ static int spi_transfer_one_message(struct spi_controller *ctlr,
>  	if (msg->status && ctlr->handle_err)
>  		ctlr->handle_err(ctlr, msg);
> 
> -	spi_res_release(ctlr, msg);
> -
>  	spi_finalize_current_message(ctlr);
> 
>  	return ret;
> @@ -1448,6 +1446,15 @@ void spi_finalize_current_message(struct spi_controller *ctlr)
>  		}
>  	}
> 
> +	/* where to put the release is a slight nightmare because
> +	 * ctlr->prepare_message may add to resources as well.
> +	 * so the question is: call it before unprepare or after?
> +	 * for now leave it after - the asumption here is that
> +	 * if prepare_message is using spi_res for callbacks,
> +	 * then no unprepare_message is used
> +	 */
> +	spi_res_release(ctlr, mesg);
> +
>  	spin_lock_irqsave(&ctlr->queue_lock, flags);
>  	ctlr->cur_msg = NULL;
>  	ctlr->cur_msg_prepared = false;
> --
> 2.11.0
>

Patch
diff mbox series

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 8eb7460dd744..1dfb19140bbe 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -1181,8 +1181,6 @@  static int spi_transfer_one_message(struct spi_controller *ctlr,
 	if (msg->status && ctlr->handle_err)
 		ctlr->handle_err(ctlr, msg);

-	spi_res_release(ctlr, msg);
-
 	spi_finalize_current_message(ctlr);

 	return ret;
@@ -1448,6 +1446,15 @@  void spi_finalize_current_message(struct spi_controller *ctlr)
 		}
 	}

+	/* where to put the release is a slight nightmare because
+	 * ctlr->prepare_message may add to resources as well.
+	 * so the question is: call it before unprepare or after?
+	 * for now leave it after - the asumption here is that
+	 * if prepare_message is using spi_res for callbacks,
+	 * then no unprepare_message is used
+	 */
+	spi_res_release(ctlr, mesg);
+
 	spin_lock_irqsave(&ctlr->queue_lock, flags);
 	ctlr->cur_msg = NULL;
 	ctlr->cur_msg_prepared = false;