> -----Original Message----- > From: Ertman, David M > Sent: Wednesday, October 7, 2020 12:54 PM > To: 'Leon Romanovsky' <leon@xxxxxxxxxx> > Cc: alsa-devel@xxxxxxxxxxxxxxxx; parav@xxxxxxxxxxxx; tiwai@xxxxxxx; > netdev@xxxxxxxxxxxxxxx; ranjani.sridharan@xxxxxxxxxxxxxxx; Pierre-Louis > Bossart <pierre-louis.bossart@xxxxxxxxxxxxxxx>; fred.oh@xxxxxxxxxxxxxxx; > linux-rdma@xxxxxxxxxxxxxxx; dledford@xxxxxxxxxx; broonie@xxxxxxxxxx; > jgg@xxxxxxxxxx; gregkh@xxxxxxxxxxxxxxxxxxx; kuba@xxxxxxxxxx; Williams, > Dan J <dan.j.williams@xxxxxxxxx>; Saleem, Shiraz > <shiraz.saleem@xxxxxxxxx>; davem@xxxxxxxxxxxxx; Patil, Kiran > <kiran.patil@xxxxxxxxx> > Subject: RE: [PATCH v2 1/6] Add ancillary bus support > > > -----Original Message----- > > From: Alsa-devel <alsa-devel-bounces@xxxxxxxxxxxxxxxx> On Behalf Of > Leon > > Romanovsky > > Sent: Wednesday, October 7, 2020 12:26 PM > > To: Ertman, David M <david.m.ertman@xxxxxxxxx> > > Cc: alsa-devel@xxxxxxxxxxxxxxxx; parav@xxxxxxxxxxxx; tiwai@xxxxxxx; > > netdev@xxxxxxxxxxxxxxx; ranjani.sridharan@xxxxxxxxxxxxxxx; Pierre-Louis > > Bossart <pierre-louis.bossart@xxxxxxxxxxxxxxx>; fred.oh@xxxxxxxxxxxxxxx; > > linux-rdma@xxxxxxxxxxxxxxx; dledford@xxxxxxxxxx; broonie@xxxxxxxxxx; > > jgg@xxxxxxxxxx; gregkh@xxxxxxxxxxxxxxxxxxx; kuba@xxxxxxxxxx; Williams, > > Dan J <dan.j.williams@xxxxxxxxx>; Saleem, Shiraz > > <shiraz.saleem@xxxxxxxxx>; davem@xxxxxxxxxxxxx; Patil, Kiran > > <kiran.patil@xxxxxxxxx> > > Subject: Re: [PATCH v2 1/6] Add ancillary bus support > > > > On Wed, Oct 07, 2020 at 06:06:30PM +0000, Ertman, David M wrote: > > > > -----Original Message----- > > > > From: Leon Romanovsky <leon@xxxxxxxxxx> > > > > Sent: Tuesday, October 6, 2020 10:03 AM > > > > To: Pierre-Louis Bossart <pierre-louis.bossart@xxxxxxxxxxxxxxx> > > > > Cc: Ertman, David M <david.m.ertman@xxxxxxxxx>; alsa-devel@alsa- > > > > project.org; parav@xxxxxxxxxxxx; tiwai@xxxxxxx; > > netdev@xxxxxxxxxxxxxxx; > > > > ranjani.sridharan@xxxxxxxxxxxxxxx; fred.oh@xxxxxxxxxxxxxxx; linux- > > > > rdma@xxxxxxxxxxxxxxx; dledford@xxxxxxxxxx; broonie@xxxxxxxxxx; > > > > jgg@xxxxxxxxxx; gregkh@xxxxxxxxxxxxxxxxxxx; kuba@xxxxxxxxxx; > > Williams, > > > > Dan J <dan.j.williams@xxxxxxxxx>; Saleem, Shiraz > > > > <shiraz.saleem@xxxxxxxxx>; davem@xxxxxxxxxxxxx; Patil, Kiran > > > > <kiran.patil@xxxxxxxxx> > > > > Subject: Re: [PATCH v2 1/6] Add ancillary bus support > > > > > > > > On Tue, Oct 06, 2020 at 10:18:07AM -0500, Pierre-Louis Bossart wrote: > > > > > Thanks for the review Leon. > > > > > > > > > > > > Add support for the Ancillary Bus, ancillary_device and > > ancillary_driver. > > > > > > > It enables drivers to create an ancillary_device and bind an > > > > > > > ancillary_driver to it. > > > > > > > > > > > > I was under impression that this name is going to be changed. > > > > > > > > > > It's part of the opens stated in the cover letter. > > > > > > > > ok, so what are the variants? > > > > system bus (sysbus), sbsystem bus (subbus), crossbus ? > > > > > > > > > > > > > > [...] > > > > > > > > > > > > + const struct my_driver my_drv = { > > > > > > > + .ancillary_drv = { > > > > > > > + .driver = { > > > > > > > + .name = "myancillarydrv", > > > > > > > > > > > > Why do we need to give control over driver name to the driver > > authors? > > > > > > It can be problematic if author puts name that already exists. > > > > > > > > > > Good point. When I used the ancillary_devices for my own > SoundWire > > test, > > > > the > > > > > driver name didn't seem specifically meaningful but needed to be set > to > > > > > something, what mattered was the id_table. Just thinking aloud, > maybe > > we > > > > can > > > > > add prefixing with KMOD_BUILD, as we've done already to avoid > > collisions > > > > > between device names? > > > > > > > > IMHO, it shouldn't be controlled by the drivers at all and need to have > > > > kernel module name hardwired. Users will use it later for various > > > > bind/unbind/autoprobe tricks and it will give predictability for them. > > > > > > > > > > > > > > [...] > > > > > > > > > > > > +int __ancillary_device_add(struct ancillary_device *ancildev, > const > > > > char *modname) > > > > > > > +{ > > > > > > > + struct device *dev = &ancildev->dev; > > > > > > > + int ret; > > > > > > > + > > > > > > > + if (!modname) { > > > > > > > + pr_err("ancillary device modname is NULL\n"); > > > > > > > + return -EINVAL; > > > > > > > + } > > > > > > > + > > > > > > > + ret = dev_set_name(dev, "%s.%s.%d", modname, ancildev- > > >name, > > > > ancildev->id); > > > > > > > + if (ret) { > > > > > > > + pr_err("ancillary device dev_set_name failed: %d\n", > > ret); > > > > > > > + return ret; > > > > > > > + } > > > > > > > + > > > > > > > + ret = device_add(dev); > > > > > > > + if (ret) > > > > > > > + dev_err(dev, "adding ancillary device failed!: %d\n", > > ret); > > > > > > > + > > > > > > > + return ret; > > > > > > > +} > > > > > > > > > > > > Sorry, but this is very strange API that requires users to put > > > > > > internal call to "dev" that is buried inside "struct ancillary_device". > > > > > > > > > > > > For example in your next patch, you write this "put_device(&cdev- > > > > >ancildev.dev);" > > > > > > > > > > > > I'm pretty sure that the amount of bugs in error unwind will be > > > > > > astonishing, so if you are doing wrappers over core code, better do > > not > > > > > > pass complexity to the users. > > > > > > > > > > In initial reviews, there was pushback on adding wrappers that don't > do > > > > > anything except for a pointer indirection. > > > > > > > > > > Others had concerns that the API wasn't balanced and blurring layers. > > > > > > > > Are you talking about internal review or public? > > > > If it is public, can I get a link to it? > > > > > > > > > > > > > > Both points have merits IMHO. Do we want wrappers for everything > > and > > > > > completely hide the low-level device? > > > > > > > > This API is partially obscures low level driver-core code and needs to > > > > provide clear and proper abstractions without need to remember about > > > > put_device. There is already _add() interface why don't you do > > > > put_device() in it? > > > > > > > > > > The pushback Pierre is referring to was during our mid-tier internal > review. > > It was > > > primarily a concern of Parav as I recall, so he can speak to his reasoning. > > > > > > What we originally had was a single API call (ancillary_device_register) > that > > started > > > with a call to device_initialize(), and every error path out of the function > > performed > > > a put_device(). > > > > > > Is this the model you have in mind? > > > > I don't like this flow: > > ancillary_device_initialize() > > if (ancillary_ancillary_device_add()) { > > put_device(....) > > ancillary_device_unregister() > > return err; > > } > > > > And prefer this flow: > > ancillary_device_initialize() > > if (ancillary_device_add()) { > > ancillary_device_unregister() > > return err; > > } > > > > In this way, the ancillary users won't need to do non-intuitive put_device(); > > Isn't there a problem calling device_unregister() if device_add() fails? > device_unregister() does a device_del() and if the device_add() failed there > is > nothing to delete? Sorry, hit send there unintentionally. So, would it be best to split the unregister API into two calls as well. ancillary_device_del() ancillary_device_put() ? -DaveE > > -DaveE > > > > > Thanks