On Fri, Dec 15, 2017 at 09:38:42AM +0800, weiping zhang wrote:
> 2017-12-15 3:13 GMT+08:00 Michael S. Tsirkin <mst@xxxxxxxxxx>:
> > On Tue, Dec 12, 2017 at 09:24:02PM +0800, weiping zhang wrote:
> >> As mentioned at drivers/base/core.c:
> >> /*
> >> * NOTE: _Never_ directly free @dev after calling this function, even
> >> * if it returned an error! Always use put_device() to give up the
> >> * reference initialized in this function instead.
> >> */
> >> so we don't free vp_dev until vp_dev->vdev.dev.release be called.
> >
> > seeing as 5739411acbaa63a6c22c91e340fdcdbcc7d82a51 adding these
> > annotations went to stable, should this go there too?
> >
> just let people know the detail reason of using put_device.
> >> Signed-off-by: weiping zhang <zhangweiping@xxxxxxxxxxxxxxx>
> >> Reviewed-by: Cornelia Huck <cohuck@xxxxxxxxxx>
> >
> > OK but this relies on users knowing that register_virtio_device
> > calls device_register. I think we want to add a comment
> > to register_virtio_device.
> >
> > Also the cleanup is uglified.
> >
> > I really think the right thing would be to change device_register making
> > it safe to kfree. People have the right to expect register on failure to
> > have no effect.
> >
> > That just might be too hard to implement though.
> >
> > For now, my suggestion - add a variable.
> >
> >> ---
> >> drivers/virtio/virtio_pci_common.c | 17 +++++++++--------
> >> 1 file changed, 9 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c
> >> index 1c4797e..91d20f7 100644
> >> --- a/drivers/virtio/virtio_pci_common.c
> >> +++ b/drivers/virtio/virtio_pci_common.c
> >> @@ -551,16 +551,17 @@ static int virtio_pci_probe(struct pci_dev *pci_dev,
> >> pci_set_master(pci_dev);
> >>
> >> rc = register_virtio_device(&vp_dev->vdev);
> >> - if (rc)
> >> - goto err_register;
> >> + if (rc) {
> >> + if (vp_dev->ioaddr)
> >> + virtio_pci_legacy_remove(vp_dev);
> >> + else
> >> + virtio_pci_modern_remove(vp_dev);
> >> + pci_disable_device(pci_dev);
> >> + put_device(&vp_dev->vdev.dev);
> >> + }
> >>
> >> - return 0;
> >> + return rc;
> >>
> >> -err_register:
> >> - if (vp_dev->ioaddr)
> >> - virtio_pci_legacy_remove(vp_dev);
> >> - else
> >> - virtio_pci_modern_remove(vp_dev);
> >> err_probe:
> >> pci_disable_device(pci_dev);
> >> err_enable_device:
> >> --
> >> 2.9.4
> >
> > I'd prefer something like the below.
> >
> > --->
> >
> > virtio_pci: don't kfree device on register failure
> >
> > Signed-off-by: Michael S. Tsirkin <mst@xxxxxxxxxx>
> >
> > ---
> >
> > diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c
> > index 1c4797e..995ab03 100644
> > --- a/drivers/virtio/virtio_pci_common.c
> > +++ b/drivers/virtio/virtio_pci_common.c
> > @@ -513,7 +513,7 @@ static void virtio_pci_release_dev(struct device *_d)
> > static int virtio_pci_probe(struct pci_dev *pci_dev,
> > const struct pci_device_id *id)
> > {
> > - struct virtio_pci_device *vp_dev;
> > + struct virtio_pci_device *vp_dev, *reg_dev = NULL;
> > int rc;
> >
> > /* allocate our structure and fill it out */
> > @@ -551,6 +551,8 @@ static int virtio_pci_probe(struct pci_dev *pci_dev,
> > pci_set_master(pci_dev);
> >
> > rc = register_virtio_device(&vp_dev->vdev);
> > + /* NOTE: device is considered registered even if register failed. */
> > + reg_dev = vp_dev;
> > if (rc)
> > goto err_register;
> >
> > @@ -564,7 +566,10 @@ static int virtio_pci_probe(struct pci_dev *pci_dev,
> > err_probe:
> > pci_disable_device(pci_dev);
> > err_enable_device:
> > - kfree(vp_dev);
> > + if (reg_dev)
> > + put_device(dev);
> > + else
> > + kfree(vp_dev);
> > return rc;
> > }
> looks more cleaner and same coding style.
> Need I send V3 or apply your patch directly ?
Pls post v3 updating all patches to this style.
Also just to make sure, none of this is a regression and none
of this causes actual known issues right?
I think it's preferrable to defer to next merge cycle unless this
is a regression.
> > _______________________________________________
> > Virtualization mailing list
> > Virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx
> > https://lists.linuxfoundation.org/mailman/listinfo/virtualization
_______________________________________________
Virtualization mailing list
Virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
