Re: [PATCH 1/2] spi: Fix DMA mapping ownership on partial map failure

[Honghui Jiang]

Thanks for the review.

On Thu, Aug 06, 2026 at 10:34:16PM +0300, Andy Shevchenko wrote:
> The Fixes tag refers to the commit that moved from per message to per
> transfer mapping flags. Can you elaborate why it was no problem
> before? (Not sure if we need the answer to be included in the commit
> message, probably the cover letter is the best choice for this info.)

The partial-failure handling was already incomplete before that commit,
but the failure mode was different. __spi_unmap_msg() was gated by
ctlr->cur_msg_mapped, which was set only after the whole message had
been mapped successfully. A partial failure therefore skipped the
normal unmap path: it could leak mappings made for earlier transfers,
but it could not unmap them again using an unpublished DMA device.

The per-transfer conversion removed that message-wide gate. The
per-transfer flags can now remain set after a partial failure, while
cur_{tx,rx}_dma_dev are still published only after the whole mapping
loop succeeds. The subsequent cleanup can therefore unmap those
transfers using a NULL or stale device. That is the regression referred
to by the Fixes tag.

Agreed. I'll add this explanation to the v2 cover letter.

> For a fix this is probably okay to introduce, but can we get rid of
> forward declaration by moving the unmapping routine above? If so, can
> you also add another patch for that?

Yes. I'll keep the forward declaration in the fix so that it remains
independently buildable and backportable. I'll then add a follow-up
cleanup patch moving __spi_unmap_msg() above __spi_map_msg() and
removing the declaration.

> Does unwind routine nullify them? If not, there is a chance that some
> code (maybe in the future) might use stale pointers.

It currently does not.

I checked the current users. The three users in the SPI core only use a
device when the corresponding *_sg_mapped flag is set. The accesses in
spi-amlogic-spisg also occur before spi_finalize_current_message() on
every path which reaches them.

Nevertheless, I agree that the pointers should not survive the message.
I'll clear them in a separate follow-up cleanup patch, in
spi_unmap_msg() after __spi_unmap_msg() returns, rather than inside
__spi_unmap_msg() itself. The latter is also used for the in-message
DMA-to-PIO fallback and therefore does not mark the end of the message
lifetime.

> Since you touched these lines, perhaps it makes sense to drop ' != 0'
> parts to make it follow the regular pattern.

Will do. Both checks will use plain if (ret) in v2.

Thanks,
Honghui