Re: [Qemu-devel] [PATCHv3] dma/i82374: avoid double creation of i82374 device

[Eduardo Otubo]

On 16/03/2018 - 11:46:57, Thomas Huth wrote:
> On 27.11.2017 09:40, Eduardo Otubo wrote:
> > On Fri, Nov 24, 2017 at 06:44:59PM +0100, Thomas Huth wrote:
> >>  Hi Eduardo,
> >>
> >> On 24.11.2017 14:46, Eduardo Otubo wrote:
> >>> v3:
> >>>  * Removed all unecessary local_err
> >>>  * Change return of isa_bus_dma() and DMA_init() from void to int8_t,
> >>>    returning -EBUSY on error and 0 on success
> >>>  * Added qdev_cleanup_nofail() in case isa_bus_dma() returns error. The
> >>>    cleanup looks safe, but please review if I didn't miss any detail
> >>>
> >>> v2:
> >>>  * Removed user_creatable=false and replaced by error handling using
> >>>    Error **errp and error_propagate();
> >>
> >> Version changelog should go below the "---" separator, otherwise it will
> >> be included in the git changelog as well, which is kind of ugly.
> >>
> >>> QEMU fails when used with the following command line:
> >>>
> >>>     ./ppc64-softmmu/qemu-system-ppc64 -S -machine 40p,accel=tcg -device 
> >>> i82374
> >>>     qemu-system-ppc64: hw/isa/isa-bus.c:110: isa_bus_dma: Assertion 
> >>> `!bus->dma[0] && !bus->dma[1]' failed.
> >>>     Aborted (core dumped)
> >>>
> >>> The 40p machine type already created the device i82374. If specified in 
> >>> the
> >>> command line, it will try to create it again, hence generating the error. 
> >>> The
> >>> function isa_bus_dma() isn't supposed to be called twice for the same 
> >>> bus. One
> >>> way to avoid this problem is to set user_creatable=false.
> >>
> >> You don't do that user_creatable=false here anymore, so please remove it
> >> from the description.
> >>
> >>> A possible fix in a near future would be making
> >>> isa_bus_dma()/DMA_init()/i82374_realize() return an error instead of 
> >>> asserting
> >>> as well.
> >>
> >> You should rephrase that sentence as well.
> >>
> > 
> > Well, I think one mistake lead to another here. I've always put the 
> > changelog
> > before the --- and that's why the old commit message. For example:
> > 2f668be77501c0232a84aafb6a066c9915987f0e. But I guess on that context it 
> > made
> > sense to use the changelog since the commit message was too simplistic. I'm
> > gonna fix this on the v3 then, among other thigs that I need to fix. Thanks 
> > for
> > the heads up :)
> 
>  Hi,
> 
> did you ever send a v4? The problem seems still to persist...
> 

I didn't, I guess I put this on the bottom of my priority list and never got to
it.

I'll send a new version right away.
Thanks for the reminder.

-- 
Eduardo Otubo