netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Jason Gunthorpe <jgg@ziepe.ca>
To: "Ertman, David M" <david.m.ertman@intel.com>
Cc: "Kirsher, Jeffrey T" <jeffrey.t.kirsher@intel.com>,
	"davem@davemloft.net" <davem@davemloft.net>,
	"gregkh@linuxfoundation.org" <gregkh@linuxfoundation.org>,
	"netdev@vger.kernel.org" <netdev@vger.kernel.org>,
	"linux-rdma@vger.kernel.org" <linux-rdma@vger.kernel.org>,
	"nhorman@redhat.com" <nhorman@redhat.com>,
	"sassmann@redhat.com" <sassmann@redhat.com>,
	"parav@mellanox.com" <parav@mellanox.com>,
	"galpress@amazon.com" <galpress@amazon.com>,
	"selvin.xavier@broadcom.com" <selvin.xavier@broadcom.com>,
	"sriharsha.basavapatna@broadcom.com" 
	<sriharsha.basavapatna@broadcom.com>,
	"benve@cisco.com" <benve@cisco.com>,
	"bharat@chelsio.com" <bharat@chelsio.com>,
	"xavier.huwei@huawei.com" <xavier.huwei@huawei.com>,
	"yishaih@mellanox.com" <yishaih@mellanox.com>,
	"leonro@mellanox.com" <leonro@mellanox.com>,
	"mkalderon@marvell.com" <mkalderon@marvell.com>,
	"aditr@vmware.com" <aditr@vmware.com>,
	"ranjani.sridharan@linux.intel.com" 
	<ranjani.sridharan@linux.intel.com>,
	"pierre-louis.bossart@linux.intel.com" 
	<pierre-louis.bossart@linux.intel.com>,
	"Patil, Kiran" <kiran.patil@intel.com>,
	"Bowers, AndrewX" <andrewx.bowers@intel.com>
Subject: Re: [net-next 1/9] Implementation of Virtual Bus
Date: Mon, 20 Apr 2020 21:44:54 -0300	[thread overview]
Message-ID: <20200421004454.GP26002@ziepe.ca> (raw)
In-Reply-To: <DM6PR11MB284111B69E966E68EBC2C508DDD40@DM6PR11MB2841.namprd11.prod.outlook.com>

On Mon, Apr 20, 2020 at 11:16:38PM +0000, Ertman, David M wrote:
> > > +/**
> > > + * virtbus_register_device - add a virtual bus device
> > > + * @vdev: virtual bus device to add
> > > + */
> > > +int virtbus_register_device(struct virtbus_device *vdev)
> > > +{
> > > +	int ret;
> > > +
> > > +	/* Do this first so that all error paths perform a put_device */
> > > +	device_initialize(&vdev->dev);
> > > +
> > > +	if (!vdev->release) {
> > > +		ret = -EINVAL;
> > > +		dev_err(&vdev->dev, "virtbus_device MUST have a .release
> > callback that does something.\n");
> > > +		goto device_pre_err;
> > 
> > This does put_device but the release() hasn't been set yet. Doesn't it
> > leak memory?
> 
> The KO registering the virtbus_device is responsible for allocating
> and freeing the memory for the virtbus_device (which should be done
> in the release() function).  If there is no release function
> defined, then the originating KO needs to handle this.  We are
> trying to not recreate the platform_bus, so the design philosophy
> behind virtual_bus is minimalist.

Oh, a precondition assertion should just be written as something like:

   if (WARN_ON(!vdev->release))
       return -EINVAL;

And done before device_initialize

But I wouldn't bother, things will just reliably crash on null pointer
exceptions if a driver mis-uses the API.

> > > +	}
> > > +
> > > +	/* All device IDs are automatically allocated */
> > > +	ret = ida_simple_get(&virtbus_dev_ida, 0, 0, GFP_KERNEL);
> > > +	if (ret < 0) {
> > > +		dev_err(&vdev->dev, "get IDA idx for virtbus device failed!\n");
> > > +		goto device_pre_err;
> > 
> > Do this before device_initialize()
>
> The memory for virtbus device is allocated by the KO registering the
> virtbus_device before it calls virtbus_register_device().  If this
> function is exiting on an error, then we have to do a put_device()
> so that the release is called (if it exists) to clean up the memory.

put_device() must call virtbus_release_device(), which does
ida_simple_remove() on vdev->id which hasn't been set yet.

Also ->release wasn't initialized at this point so its leaks memory..

> The ida_simple_get is not used until later in the function when
> setting the vdev->id.  It doesn't matter what IDA it is used, as
> long as it is unique.  So, since we cannot have the error state
> before the device_initialize, there is no reason to have the
> ida_sinple_get before the device_initialization.

I was a bit wrong on this advice because no matter what you have to do
put_device(), so you need to add some kind of flag that the vdev->id
is not valid.

It is ugly. It is nicer to arrange thing so initialization is done
after kalloc but before device_initialize. For instance look how
vdpa_alloc_device() and vdpa_register() work, very clean, very simple
goto error unwinds everywhere.

> GregKH was pretty insistent that all error paths out of this
> function go through a put_device() when possible.

After device_initialize() is called all error paths must go through
put_device.

> > Can't understand why vdev->name is being passed in with the struct,
> > why not just a function argument?
> 
> This avoids having the calling KO have to manage a separate piece of memory
> to hold the name during the call to virtbus_device_regsiter.  It is a cleaner
> memory model to just store it once in the virtbus_device itself.  This name is
> the abbreviated name without the ID appended on the end, which will be used
> for matching drivers and devices.

Your other explanation was better. This would be less confusing if it
was called match_name/device_label/driver_key or something, as it is
not the 'name'.

> > > + * virtbus_unregister_device - remove a virtual bus device
> > > + * @vdev: virtual bus device we are removing
> > > + */
> > > +void virtbus_unregister_device(struct virtbus_device *vdev)
> > > +{
> > > +	device_del(&vdev->dev);
> > > +	put_device(&vdev->dev);
> > > +}
> > > +EXPORT_SYMBOL_GPL(virtbus_unregister_device);
> > 
> > Just inline this as wrapper around device_unregister
> 
> I thought that EXPORT_SYMBOL makes inline meaningless?
> But, putting device_unregister here is a good catch.

I mean move it to the header file and inline it

Jason

  reply	other threads:[~2020-04-21  0:44 UTC|newest]

Thread overview: 22+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-04-17 17:10 [net-next 0/9][pull request] 100GbE Intel Wired LAN Driver Updates 2020-04-17 Jeff Kirsher
2020-04-17 17:10 ` [net-next 1/9] Implementation of Virtual Bus Jeff Kirsher
2020-04-17 19:14   ` Pierre-Louis Bossart
2020-04-17 19:51   ` Jason Gunthorpe
2020-04-20 23:16     ` Ertman, David M
2020-04-21  0:44       ` Jason Gunthorpe [this message]
2020-04-21 23:27         ` Ertman, David M
2020-04-18 12:50   ` Greg KH
2020-04-20 22:59     ` Ertman, David M
2020-04-21  6:52       ` Greg KH
2020-04-20 23:46   ` Ranjani Sridharan
2020-04-17 17:10 ` [net-next 2/9] ice: Create and register virtual bus for RDMA Jeff Kirsher
2020-04-17 19:17   ` Jason Gunthorpe
2020-04-20 23:47     ` Kirsher, Jeffrey T
2020-04-17 23:49   ` Jason Gunthorpe
2020-04-17 17:10 ` [net-next 3/9] ice: Complete RDMA peer registration Jeff Kirsher
2020-04-17 17:10 ` [net-next 4/9] ice: Support resource allocation requests Jeff Kirsher
2020-04-17 17:10 ` [net-next 5/9] ice: Enable event notifications Jeff Kirsher
2020-04-17 17:10 ` [net-next 6/9] ice: Allow reset operations Jeff Kirsher
2020-04-17 17:10 ` [net-next 7/9] ice: Pass through communications to VF Jeff Kirsher
2020-04-17 17:10 ` [net-next 8/9] i40e: Move client header location Jeff Kirsher
2020-04-17 17:10 ` [net-next 9/9] i40e: Register a virtbus device to provide RDMA Jeff Kirsher

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200421004454.GP26002@ziepe.ca \
    --to=jgg@ziepe.ca \
    --cc=aditr@vmware.com \
    --cc=andrewx.bowers@intel.com \
    --cc=benve@cisco.com \
    --cc=bharat@chelsio.com \
    --cc=davem@davemloft.net \
    --cc=david.m.ertman@intel.com \
    --cc=galpress@amazon.com \
    --cc=gregkh@linuxfoundation.org \
    --cc=jeffrey.t.kirsher@intel.com \
    --cc=kiran.patil@intel.com \
    --cc=leonro@mellanox.com \
    --cc=linux-rdma@vger.kernel.org \
    --cc=mkalderon@marvell.com \
    --cc=netdev@vger.kernel.org \
    --cc=nhorman@redhat.com \
    --cc=parav@mellanox.com \
    --cc=pierre-louis.bossart@linux.intel.com \
    --cc=ranjani.sridharan@linux.intel.com \
    --cc=sassmann@redhat.com \
    --cc=selvin.xavier@broadcom.com \
    --cc=sriharsha.basavapatna@broadcom.com \
    --cc=xavier.huwei@huawei.com \
    --cc=yishaih@mellanox.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).