On Saturday 29 November 2008 23:22:19 David Brownell wrote: > On Saturday 29 November 2008, Hans Verkuil wrote: > > > > +void v4l2_device_register(struct device *dev, struct > > > > v4l2_device *v4l2_dev) +{ > > > > + BUG_ON(!dev || !v4l2_dev || dev_get_drvdata(dev)); > > > > > > Ouch. Better to return -EINVAL, like most register() calls, > > > than *ever* use a BUG_ON() for bad parameters. Same applies > > > every other place you use BUG_ON, from a quick scan ... > > > > Are there some documented guidelines on when to use BUG_ON? > > Maybe there should be. I know I've seen flames from Linus on > the topic. Basically, treat it like a panic() where the system > must stop operation lest it catch fire or scribble all over the > (not-backed-up) disk ... if the system can keep running sanely, > then BUG() and friends are inappropriate.
I think it would be good to have some document about this, since from what I've seen from a quick scan I'm not the only one who uses it incorrectly. There is no documentation in the asm-generic/bug.h header and there is also no documentation on this in the Documentation directory. > > I see it used in other places in this way. > > I tend to submit patches fixing bugs like that, when I have time. > > > My reasoning was that returning an > > error makes sense if external causes can result in an error, but > > this test is more the equivalent of an assert(), i.e. catching a > > programming bug early. > > In which case a WARN() is better. But in most cases I wouldn't > even do that. The kernel's design center is closer to "run > robustly" than "make developers' lives easier". Programmers > who don't check return values for critical operations like > registering core resources deserve what they get. And if you > want to nudge them, the __must_check annotation helps catch > such goofage even earlier: compile time, not run time. I've replaced it as follows (and with __must_check in the header): int v4l2_device_register(struct device *dev, struct v4l2_device *v4l2_dev) { if (dev == NULL || v4l2_dev == NULL) return -EINVAL; /* Warn if we apparently re-register a device */ WARN_ON(dev_get_drvdata(dev)); INIT_LIST_HEAD(&v4l2_dev->subdevs); spin_lock_init(&v4l2_dev->lock); v4l2_dev->dev = dev; snprintf(v4l2_dev->name, sizeof(v4l2_dev->name), "%s %s", dev->driver->name, dev->bus_id); dev_set_drvdata(dev, v4l2_dev); return 0; } EXPORT_SYMBOL_GPL(v4l2_device_register); void v4l2_device_unregister(struct v4l2_device *v4l2_dev) { struct v4l2_subdev *sd, *next; if (v4l2_dev == NULL || v4l2_dev->dev == NULL) return; dev_set_drvdata(v4l2_dev->dev, NULL); /* unregister subdevs */ list_for_each_entry_safe(sd, next, &v4l2_dev->subdevs, list) v4l2_device_unregister_subdev(sd); v4l2_dev->dev = NULL; } EXPORT_SYMBOL_GPL(v4l2_device_unregister); int v4l2_device_register_subdev(struct v4l2_device *dev, struct v4l2_subdev *sd) { /* Check for valid input */ if (dev == NULL || sd == NULL || !sd->name[0]) return -EINVAL; /* Warn if we apparently re-register a subdev */ WARN_ON(sd->dev); if (!try_module_get(sd->owner)) return -ENODEV; sd->dev = dev; spin_lock(&dev->lock); list_add_tail(&sd->list, &dev->subdevs); spin_unlock(&dev->lock); return 0; } EXPORT_SYMBOL_GPL(v4l2_device_register_subdev); void v4l2_device_unregister_subdev(struct v4l2_subdev *sd) { /* return if it isn't registered */ if (sd == NULL || sd->dev == NULL) return; spin_lock(&sd->dev->lock); list_del(&sd->list); spin_unlock(&sd->dev->lock); sd->dev = NULL; module_put(sd->owner); } EXPORT_SYMBOL_GPL(v4l2_device_unregister_subdev); Thanks for the review! Hans -- Hans Verkuil - video4linux developer - sponsored by TANDBERG -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html