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 ? > _______________________________________________ > Virtualization mailing list > Virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx > https://lists.linuxfoundation.org/mailman/listinfo/virtualization _______________________________________________ Virtualization mailing list Virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx https://lists.linuxfoundation.org/mailman/listinfo/virtualization