Re: [PATCH V1 1/3] spi: core: resource: fix memory leak on error and place in "correct" sequence

[Noralf Trønnes <noralf@xxxxxxxxxxx>]

Den 09.05.2019 12.55, skrev kernel@xxxxxxxxxxxxxxxx:
> From: Martin Sperl <kernel@xxxxxxxxxxxxxxxx>
> 
> 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@xxxxxxxxxxxxxxxx>
> ---

Thanks for sorting this out Martin.

Tested-by: Noralf Trønnes <noralf@xxxxxxxxxxx>

>  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
>