| Message ID | 20190509105533.24275-2-kernel@martin.sperl.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | spi: core: correct ordering of unprepare and resources | expand |
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>
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 >
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;