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

[Andy Shevchenko]

On Wed, Aug 5, 2026 at 6:15 PM Honghui Jiang <jiang_hh2019@xxxxxxx> wrote:
>
> If RX mapping fails after TX mapping succeeds, __spi_map_msg() unmaps
> TX but leaves tx_sg_mapped set.  If TX mapping fails on a later
> transfer, mappings created for earlier transfers remain active.
>
> In both cases, cur_{tx,rx}_dma_dev have not yet been updated because they
> are assigned only after every transfer has been mapped.  The subsequent
> spi_unmap_msg() may therefore unmap the TX mapping again or release
> earlier mappings using a NULL or stale device.  An empty SG table does
> not prevent the NULL dereference because dma_unmap_sg_attrs() accesses
> the device before checking the entry count.
>
> Publish both mapping devices before mapping starts and unwind all
> failures through __spi_unmap_msg().  This clears the mapping flags and
> releases each mapping once with the device that created it.

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

...

> +static int __spi_unmap_msg(struct spi_controller *ctlr, struct spi_message *msg);

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?

>  static int __spi_map_msg(struct spi_controller *ctlr, struct spi_message *msg)

...

> -       ret = -ENOMSG;
> +       /*
> +        * Store the devices before mapping so partial failures can be unwound
> +        * with the device that created each mapping.
> +        */
> +       ctlr->cur_tx_dma_dev = tx_dev;
> +       ctlr->cur_rx_dma_dev = rx_dev;

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

...

>                         if (ret != 0)
> -                               return ret;
> +                               goto unwind;

> +                       if (ret != 0)
> +                               goto unwind;

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


--
With Best Regards,
Andy Shevchenko