On Sat, Dec 03, 2011 at 12:55:51PM +0200, Ivan Klymenko wrote:
> ?? Fri, 2 Dec 2011 21:19:14 +0000 (UTC)
> Marius Strobl <mar...@freebsd.org> ??????????:
> 
> > Author: marius
> > Date: Fri Dec  2 21:19:14 2011
> > New Revision: 228209
> > URL: http://svn.freebsd.org/changeset/base/228209
> > 
> > Log:
> >   - In device_probe_child(9) check the return value of
> > device_set_driver(9) when actually setting a driver as especially
> > ENOMEM is fatal in these cases.
> >   - Annotate other calls to device_set_devclass(9) and
> > device_set_driver(9) without the return value being checked and that
> > are okay to fail. 
> >   Reviewed by:      yongari (slightly earlier version)
> > 
> > Modified:
> >   head/sys/kern/subr_bus.c
> > 
> > Modified: head/sys/kern/subr_bus.c
> > ==============================================================================
> > --- head/sys/kern/subr_bus.c        Fri Dec  2 20:28:45 2011
> > (r228208) +++ head/sys/kern/subr_bus.c      Fri Dec  2 21:19:14
> > 2011        (r228209) @@ -1129,7 +1129,7 @@
> > devclass_driver_deleted(devclass_t buscl dev->parent->devclass ==
> > busclass) { if ((error = device_detach(dev)) != 0)
> >                                     return (error);
> > -                           device_set_driver(dev, NULL);
> > +                           (void)device_set_driver(dev, NULL);
> >                             BUS_PROBE_NOMATCH(dev->parent, dev);
> >                             devnomatch(dev);
> >                             dev->flags |= DF_DONENOMATCH;
> > @@ -2007,19 +2007,22 @@ device_probe_child(device_t dev, device_
> >             for (dl = first_matching_driver(dc, child);
> >                  dl;
> >                  dl = next_matching_driver(dc, child, dl)) {
> > -
> >                     /* If this driver's pass is too high, then
> > ignore it. */ if (dl->pass > bus_current_pass)
> >                             continue;
> >  
> >                     PDEBUG(("Trying %s",
> > DRIVERNAME(dl->driver)));
> > -                   device_set_driver(child, dl->driver);
> > +                   result = device_set_driver(child,
> > dl->driver);
> > +                   if (result == ENOMEM)
> > +                           return (result);
> > +                   else if (result != 0)
> > +                           continue;
> >                     if (!hasclass) {
> >                             if (device_set_devclass(child,
> > dl->driver->name)) { printf("driver bug: Unable to set devclass
> > (devname: %s)\n", (child ? device_get_name(child) :
> >                                             "no device"));
> > -                                   device_set_driver(child,
> > NULL);
> > +
> > (void)device_set_driver(child, NULL); continue;
> >                             }
> >                     }
> > @@ -2033,7 +2036,7 @@ device_probe_child(device_t dev, device_
> >                     /* Reset flags and devclass before the next
> > probe. */ child->devflags = 0;
> >                     if (!hasclass)
> > -                           device_set_devclass(child, NULL);
> > +                           (void)device_set_devclass(child,
> > NULL); 
> >                     /*
> >                      * If the driver returns SUCCESS, there can
> > be @@ -2050,7 +2053,7 @@ device_probe_child(device_t dev, device_
> >                      * certainly doesn't match.
> >                      */
> >                     if (result > 0) {
> > -                           device_set_driver(child, NULL);
> > +                           (void)device_set_driver(child, NULL);
> >                             continue;
> >                     }
> >  
> > @@ -2113,7 +2116,9 @@ device_probe_child(device_t dev, device_
> >                     if (result != 0)
> >                             return (result);
> >             }
> > -           device_set_driver(child, best->driver);
> > +           result = device_set_driver(child, best->driver);
> > +           if (result != 0)
> > +                   return (result);
> >             resource_int_value(best->driver->name, child->unit,
> >                 "flags", &child->devflags);
> >  
> > @@ -2722,8 +2727,8 @@ device_attach(device_t dev)
> >                 dev->driver->name, dev->unit, error);
> >             /* Unset the class; set in device_probe_child */
> >             if (dev->devclass == NULL)
> > -                   device_set_devclass(dev, NULL);
> > -           device_set_driver(dev, NULL);
> > +                   (void)device_set_devclass(dev, NULL);
> > +           (void)device_set_driver(dev, NULL);
> >             device_sysctl_fini(dev);
> >             dev->state = DS_NOTPRESENT;
> >             return (error);
> > @@ -2776,7 +2781,7 @@ device_detach(device_t dev)
> >             devclass_delete_device(dev->devclass, dev);
> >  
> >     dev->state = DS_NOTPRESENT;
> > -   device_set_driver(dev, NULL);
> > +   (void)device_set_driver(dev, NULL);
> >     device_set_desc(dev, NULL);
> >     device_sysctl_fini(dev);
> >  
> > @@ -4613,7 +4618,6 @@ print_driver(driver_t *driver, int inden
> >     print_driver_short(driver, indent);
> >  }
> >  
> > -
> >  static void
> >  print_driver_list(driver_list_t drivers, int indent)
> >  {
> 
> Thank you!
> Now I can again connect a USB external drive...

In what way did it fail before? This change actually doesn't fix any
issue encountered in practice that I'm aware of. I just spotted this
possibility for a problem while trying to debug what appears to be
a race in newbus and I can't think of anything besides a panic due
to trying to use unallocated memory this change would fix. But even
then if device_set_driver(9) fails with ENOMEM the panic would be
just avoided but you'd still end up with no driver being attached ...

> But, nevertheless, remains a problem when you connect an external drive
> via FireWire...

Marius

_______________________________________________
svn-src-all@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"

Reply via email to