On 28 January 2016 at 18:09, P J P <ppan...@redhat.com> wrote:
> Yes, this is exactly same case, except that 'bounce.buffer' is NULL; It does
> not point to a valid address.
>
> 1. For first address_space_map() everything goes well and 'bounce.buffer' is
> allocated.
OK
> 2. For second address_space_map(), it returns NULL, because 'bounce.buffer' is
> already in_use=true.
OK
>
> ahci_port_write
> ahci_cond_start_engines
> ahci_map_fis_address
> map_page
> dma_memory_map
> address_space_map <== returns NULL
>
> 3. For first address_space_unmap() everything goes well and 'bounce.buffer' is
> set to NULL and 'bounce.in_use' is set to false.
OK
> 4. For the second address_space_unmap(), the 'buffer' parameter is NULL
> because second address_space_map() returned NULL.
This is where something has gone wrong -- a NULL return means that
address_space_map() *failed*. It is not a valid mapping and the
ahci code should never be passing it to address_space_unmap()
(or indeed doing anything with it at all).
Instead it needs to handle it as an error case. But it looks like
ahci_cond_start_engines() already does that:
if (ahci_map_fis_address(ad)) {
pr->cmd |= PORT_CMD_FIS_ON;
} else {
error_report("AHCI: Failed to start FIS receive engine: "
"bad FIS receive buffer address");
return -1;
}
so perhaps the problem is that it's not correctly tracking
whether the mapping succeeded in order to avoid unmapping
something that was never mapped.
I suspect that the correct fix to this is that
ahci_unmap_fis_address() should only call dma_memory_unmap()
if ad->res_fis is not NULL. (Other calls to dma_memory_unmap()
in this file also need checking to see if they should have
similar guards.)
thanks
-- PMM
