On Thu, Oct 01, 2020 at 09:02:12AM -0300, Jason Gunthorpe wrote:
> On Thu, Oct 01, 2020 at 01:54:02PM +0200, Greg KH wrote:
> > On Thu, Oct 01, 2020 at 08:46:08AM -0300, Jason Gunthorpe wrote:
> > > On Thu, Oct 01, 2020 at 01:01:20PM +0200, Greg KH wrote:
> > > > On Wed, Sep 30, 2020 at 03:50:46PM -0700, Dave Ertman wrote:
> > > > > +int ancillary_device_initialize(struct ancillary_device *ancildev)
> > > > > +{
> > > > > + struct device *dev = &ancildev->dev;
> > > > > +
> > > > > + dev->bus = &ancillary_bus_type;
> > > > > +
> > > > > + if (WARN_ON(!dev->parent) || WARN_ON(!ancildev->name) ||
> > > > > + WARN_ON(!(dev->type && dev->type->release) && !dev->release))
> > > > > + return -EINVAL;
> > > >
> > > > You have a lot of WARN_ON() calls in this patch. That blows up anyone
> > > > who runs with panic-on-warn, right?
> > >
> > > AFAIK this is the standard pattern to code a "can't happen"
> > > assertion. Linus has been clear not to use BUG_ON, but to try and
> > > recover. The WARN_ON directly points to the faulty driver so it can be
> > > fixed.
> >
> > Printing an error and returning an error value also does the same exact
> > thing, the developer will not have a working system.
> >
> > Please don't abuse WARN_ON() for things that should just be normal error
> > checking logic of api calls.
>
> This is not normal error checking, it is precondition
> assertion. Something has gone badly wrong if it ever triggers.
>
> If you don't want to use WARN_ON for assertions then when should it be
> used?
>
> pr_err is not the same thing, it doesn't trigger reports from fuzzers.
fuzzers shouldn't be messing with device registration functions :)
just do a normal pr_err() and all is fine, again, this is like any other
in-kernel api that is trying to check for valid values being passed to
it.
thanks,
grteg k-h
