svn commit: r300149 - in head/sys: arm/allwinner arm/allwinner/a10 arm/arm arm/broadcom/bcm2835 arm/mv arm/nvidia arm/ti arm/ti/omap4 arm64/arm64 kern mips/mediatek mips/mips sys

[Zbigniew Bodek]

Hello Andrew,

Thanks for your reply. Please see my comments in-line.

Kind regards
zbb

2016-05-18 17:24 GMT+02:00 Andrew Turner <andrew at fubar.geek.nz>:

> On Wed, 18 May 2016 17:15:10 +0200
> Zbigniew Bodek <zbb at semihalf.com> wrote:
>
> > 2016-05-18 17:05 GMT+02:00 Andrew Turner <andrew at freebsd.org>:
> ...
> > >  #ifdef INTRNG
> > >         xref = OF_xref_from_node(ofw_bus_get_node(dev));
> > > -       if (intr_pic_register(dev, xref) != 0) {
> > > +       if (intr_pic_register(dev, xref) == NULL) {
> > >                 device_printf(dev, "could not register PIC\n");
> > >                 goto error;
> > >         }
> > > @@ -172,7 +172,7 @@ error:
> > >         /* Failure so free resources */
> > >         gic_v3_detach(dev);
> > >
> > > -       return (err);
> > > +       return (ENXIO);
> > >
> >
> >
> > Few line above we have:
> > err = gic_v3_attach(dev);
> > if (err != 0)
> > goto error;
> >
> > So now we would not return error code from gic_v3_attach() but always
> > ENXIO...
>
> The error value doesn't matter, as long as it's non-zero. This also
> fixes the return value in the intrng case if either of
> intr_pic_register or intr_pic_claim_root fail. You might get a little
> information from the return value being printed, however it would be
> more useful to have verbose logging to print an error message.
>
>
So if you wanted to fix INTRNG part why didn't you just set an error value
in case of intr_pic_register() failure before "goto error"?
Simply:

 +       if (intr_pic_register(dev, xref) == NULL) {
                 device_printf(dev, "could not register PIC\n");
+               err = ENXIO;
                 goto error;
        }

Because now we set an error value and in case of real error (not when
gic_v3_attach() returns 0) we print ENXIO while doing nothing with the err
variable...



> In all of these failure cases there isn't much we can do as there is no
> root interrupt controller, the boot will fail later on when we enable
> interrupts.
>

I know but that is not the explanation for breaking the logic here.


>
> Andrew
>