linux-kernel - Re: [PATCH v2] v4l2_device/v4l2

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <200811300006.22080.hverkuil@xs4all.nl>
Date:	Sun, 30 Nov 2008 00:06:21 +0100
From:	Hans Verkuil <hverkuil@...all.nl>
To:	David Brownell <david-b@...bell.net>
Cc:	Linux and Kernel Video <video4linux-list@...hat.com>,
	"linux-omap@...r.kernel.org" <linux-omap@...r.kernel.org>,
	"davinci-linux-open-source@...ux.davincidsp.com" 
	<davinci-linux-open-source@...ux.davincidsp.com>,
	linux-kernel@...r.kernel.org,
	Laurent Pinchart <laurent.pinchart@...net.be>
Subject: Re: [PATCH v2] v4l2_device/v4l2_subdev: final (?) version

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-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/