> -----Original Message-----
> From: Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx>
> Sent: Thursday, October 1, 2020 5:16 AM
> To: Jason Gunthorpe <jgg@xxxxxxxxxx>
> Cc: Ertman, David M <david.m.ertman@xxxxxxxxx>; alsa-devel@alsa-
> project.org; tiwai@xxxxxxx; broonie@xxxxxxxxxx; pierre-
> louis.bossart@xxxxxxxxxxxxxxx; Sridharan, Ranjani
> <ranjani.sridharan@xxxxxxxxx>; parav@xxxxxxxxxx; Patil, Kiran
> <kiran.patil@xxxxxxxxx>; Ranjani Sridharan
> <ranjani.sridharan@xxxxxxxxxxxxxxx>; Fred Oh <fred.oh@xxxxxxxxxxxxxxx>;
> Saleem, Shiraz <shiraz.saleem@xxxxxxxxx>; Parav Pandit
> <parav@xxxxxxxxxxxx>; Williams, Dan J <dan.j.williams@xxxxxxxxx>
> Subject: Re: [PATCH 1/6] Add ancillary bus support
>
> 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.
I will remove these for next version.
-DaveE
>
> thanks,
>
> grteg k-h
