RE: [PATCH 1/6] Add ancillary bus support

From: "Ertman, David M" <david.m.ertman@intel.com>
To: Greg KH <gregkh@linuxfoundation.org>, Jason Gunthorpe <jgg@nvidia.com>
Cc: "alsa-devel@alsa-project.org" <alsa-devel@alsa-project.org>,
	"Patil, Kiran" <kiran.patil@intel.com>,
	"tiwai@suse.de" <tiwai@suse.de>,
	"Sridharan, Ranjani" <ranjani.sridharan@intel.com>,
	"pierre-louis.bossart@linux.intel.com"
	<pierre-louis.bossart@linux.intel.com>,
	Ranjani Sridharan <ranjani.sridharan@linux.intel.com>,
	Fred Oh <fred.oh@linux.intel.com>,
	"broonie@kernel.org" <broonie@kernel.org>,
	"parav@nvidia.com" <parav@nvidia.com>,
	"Williams, Dan J" <dan.j.williams@intel.com>,
	"Saleem, Shiraz" <shiraz.saleem@intel.com>,
	Parav Pandit <parav@mellanox.com>
Subject: RE: [PATCH 1/6] Add ancillary bus support
Date: Thu, 1 Oct 2020 18:26:29 +0000	[thread overview]
Message-ID: <DM6PR11MB284104012D31D2A3EBC3E7D7DD300@DM6PR11MB2841.namprd11.prod.outlook.com> (raw)
In-Reply-To: <20201001121541.GB2375307@kroah.com>

> -----Original Message-----
> From: Greg KH <gregkh@linuxfoundation.org>
> Sent: Thursday, October 1, 2020 5:16 AM
> To: Jason Gunthorpe <jgg@nvidia.com>
> Cc: Ertman, David M <david.m.ertman@intel.com>; alsa-devel@alsa-
> project.org; tiwai@suse.de; broonie@kernel.org; pierre-
> louis.bossart@linux.intel.com; Sridharan, Ranjani
> <ranjani.sridharan@intel.com>; parav@nvidia.com; Patil, Kiran
> <kiran.patil@intel.com>; Ranjani Sridharan
> <ranjani.sridharan@linux.intel.com>; Fred Oh <fred.oh@linux.intel.com>;
> Saleem, Shiraz <shiraz.saleem@intel.com>; Parav Pandit
> <parav@mellanox.com>; Williams, Dan J <dan.j.williams@intel.com>
> 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