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

[Eduardo Habkost]

On Thu, Sep 07, 2017 at 10:38:29AM +0200, Eduardo Otubo wrote:
> On Fri, Sep 01, 2017 at 12:44:30PM -0300, Eduardo Habkost wrote:
> > On Fri, Sep 01, 2017 at 05:34:34PM +0200, Markus Armbruster wrote:
> > > Eduardo Habkost <address@hidden> writes:
> > > > On Fri, Sep 01, 2017 at 01:03:32PM +0200, Eduardo Otubo wrote:
[...]
> > > >> diff --git a/hw/dma/i82374.c b/hw/dma/i82374.c
> > > >> index 6c0f975df0..5275d822e0 100644
> > > >> --- a/hw/dma/i82374.c
> > > >> +++ b/hw/dma/i82374.c
> > > >> @@ -139,6 +139,7 @@ static void i82374_class_init(ObjectClass *klass, 
> > > >> void *data)
> > > >>      dc->realize = i82374_realize;
> > > >>      dc->vmsd = &vmstate_i82374;
> > > >>      dc->props = i82374_properties;
> > > >> +    dc->user_creatable = false;
> > > >
> > > > A "Reason:" comment explaining why user_creatable=false is
> > > > mandatory.  See the comment above user_creatable declaration in
> > > > qdev-core.h for reference.
> > > >
> > > > I suggest the following:
> > > >
> > > >     /*
> > > >      * Reason: i82374_realize() crashes (assertion failure inside 
> > > > isa_bus_dma()
> > > >      *         if the device is instantiated twice.
> > > >      */
> 
> I agree with the comment above. If there's nothing left to fix/add
> I'll just send a v2 for this shortly.

I suggest amending it with the additional info below, explaining
that isa_bus_dma() isn't supposed to be called twice, and that we
need to make isa_bus_dma()/DMA_init()/i82374_realize() return an
appropriate error if the device is instantiated twice.


> 
> > > 
> > > We need to find out *why* it crashes.  Once we know, we can likely write
> > > a better comment.
> > 
> > It crashes because isa_bus_dma() isn't supposed to be called
> > twice for the same bus.
> > 
> > Making isa_bus_dma()/DMA_init()/i82374_realize() return an error
> > instead of asserting would be even better than setting
> > user_creatable=false.
> > 
> > -- 
> > Eduardo
> > 
> 
> -- 
> Eduardo Otubo
> Senior Software Engineer @ RedHat

-- 
Eduardo