On Fri, Feb 14, 2020 at 09:02:40AM -0800, Greg KH wrote:
> > +/**
> > + * virtbus_dev_register - add a virtual bus device
> > + * @vdev: virtual bus device to add
> > + */
> > +int virtbus_dev_register(struct virtbus_device *vdev)
> > +{
> > + int ret;
> > +
> > + if (!vdev->release) {
> > + dev_err(&vdev->dev, "virtbus_device .release callback NULL\n");
>
> "virtbus_device MUST have a .release callback that does something!\n"
>
> > + return -EINVAL;
> > + }
> > +
> > + device_initialize(&vdev->dev);
> > +
> > + vdev->dev.bus = &virtual_bus_type;
> > + vdev->dev.release = virtbus_dev_release;
> > + /* 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");
> > + put_device(&vdev->dev);
>
> If you allocate the number before device_initialize(), no need to call
> put_device(). Just a minor thing, no big deal.
If *_regster does put_device on error then it must always do
put_device on any error, for instance the above return -EINVAL with
no put_device leaks memory.
Generally I find the design and audit of drivers simpler if the
register doesn't do device_initialize or put_device - have them
distinct and require the caller to manage this.
For instance look at ice_init_peer_devices() and ask who frees
the alloc_ordered_workqueue() if virtbus_dev_register() fails..
It is not all easy to tell if this is right or not..
> > + put_device(&vdev->dev);
> > + ida_simple_remove(&virtbus_dev_ida, vdev->id);
>
> You need to do this before put_device().
Shouldn't it be in the release function? The ida index should not be
re-used until the kref goes to zero..
> > +struct virtbus_device {
> > + struct device dev;
> > + const char *name;
> > + void (*release)(struct virtbus_device *);
> > + int id;
> > + const struct virtbus_dev_id *matched_element;
> > +};
>
> Any reason you need to make "struct virtbus_device" a public structure
> at all?
The general point of this scheme is to do this in a public header:
+struct iidc_virtbus_object {
+ struct virtbus_device vdev;
+ struct iidc_peer_dev *peer_dev;
+};
And then this when the driver binds:
+int irdma_probe(struct virtbus_device *vdev)
+{
+ struct iidc_virtbus_object *vo =
+ container_of(vdev, struct iidc_virtbus_object, vdev);
+ struct iidc_peer_dev *ldev = vo->peer_dev;
So the virtbus_device is in a public header to enable the container_of
construction.
Jason
