| Message ID | 20210729073503.187-11-xieyongji@bytedance.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v10,01/17] iova: Export alloc_iova_fast() and free_iova_fast() | expand |
在 2021/7/29 下午3:34, Xie Yongji 写道: > The device reset may fail in virtio-vdpa case now, so add checks to > its return value and fail the register_virtio_device(). So the reset() would be called by the driver during remove as well, or is it sufficient to deal only with the reset during probe? Thanks > > Signed-off-by: Xie Yongji <xieyongji@bytedance.com> > --- > drivers/virtio/virtio.c | 15 ++++++++++----- > 1 file changed, 10 insertions(+), 5 deletions(-) > > diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c > index a15beb6b593b..8df75425fb43 100644 > --- a/drivers/virtio/virtio.c > +++ b/drivers/virtio/virtio.c > @@ -349,7 +349,9 @@ int register_virtio_device(struct virtio_device *dev) > > /* We always start by resetting the device, in case a previous > * driver messed it up. This also tests that code path a little. */ > - dev->config->reset(dev); > + err = dev->config->reset(dev); > + if (err) > + goto err_reset; > > /* Acknowledge that we've seen the device. */ > virtio_add_status(dev, VIRTIO_CONFIG_S_ACKNOWLEDGE); > @@ -362,10 +364,13 @@ int register_virtio_device(struct virtio_device *dev) > */ > err = device_add(&dev->dev); > if (err) > - ida_simple_remove(&virtio_index_ida, dev->index); > -out: > - if (err) > - virtio_add_status(dev, VIRTIO_CONFIG_S_FAILED); > + goto err_add; > + > + return 0; > +err_add: > + virtio_add_status(dev, VIRTIO_CONFIG_S_FAILED); > +err_reset: > + ida_simple_remove(&virtio_index_ida, dev->index); > return err; > } > EXPORT_SYMBOL_GPL(register_virtio_device);
On Tue, Aug 3, 2021 at 4:09 PM Jason Wang <jasowang@redhat.com> wrote: > > > 在 2021/7/29 下午3:34, Xie Yongji 写道: > > The device reset may fail in virtio-vdpa case now, so add checks to > > its return value and fail the register_virtio_device(). > > > So the reset() would be called by the driver during remove as well, or > is it sufficient to deal only with the reset during probe? > Actually there is no way to handle failure during removal. And it should be safe with the protection of software IOTLB even if the reset() fails. Thanks, Yongji
在 2021/8/3 下午5:38, Yongji Xie 写道: > On Tue, Aug 3, 2021 at 4:09 PM Jason Wang <jasowang@redhat.com> wrote: >> >> 在 2021/7/29 下午3:34, Xie Yongji 写道: >>> The device reset may fail in virtio-vdpa case now, so add checks to >>> its return value and fail the register_virtio_device(). >> >> So the reset() would be called by the driver during remove as well, or >> is it sufficient to deal only with the reset during probe? >> > Actually there is no way to handle failure during removal. And it > should be safe with the protection of software IOTLB even if the > reset() fails. > > Thanks, > Yongji If this is true, does it mean we don't even need to care about reset failure? Thanks
On Wed, Aug 4, 2021 at 4:32 PM Jason Wang <jasowang@redhat.com> wrote: > > > 在 2021/8/3 下午5:38, Yongji Xie 写道: > > On Tue, Aug 3, 2021 at 4:09 PM Jason Wang <jasowang@redhat.com> wrote: > >> > >> 在 2021/7/29 下午3:34, Xie Yongji 写道: > >>> The device reset may fail in virtio-vdpa case now, so add checks to > >>> its return value and fail the register_virtio_device(). > >> > >> So the reset() would be called by the driver during remove as well, or > >> is it sufficient to deal only with the reset during probe? > >> > > Actually there is no way to handle failure during removal. And it > > should be safe with the protection of software IOTLB even if the > > reset() fails. > > > > Thanks, > > Yongji > > > If this is true, does it mean we don't even need to care about reset > failure? > But we need to handle the failure in the vhost-vdpa case, isn't it? Thanks, Yongji
在 2021/8/4 下午4:50, Yongji Xie 写道: > On Wed, Aug 4, 2021 at 4:32 PM Jason Wang <jasowang@redhat.com> wrote: >> >> 在 2021/8/3 下午5:38, Yongji Xie 写道: >>> On Tue, Aug 3, 2021 at 4:09 PM Jason Wang <jasowang@redhat.com> wrote: >>>> 在 2021/7/29 下午3:34, Xie Yongji 写道: >>>>> The device reset may fail in virtio-vdpa case now, so add checks to >>>>> its return value and fail the register_virtio_device(). >>>> So the reset() would be called by the driver during remove as well, or >>>> is it sufficient to deal only with the reset during probe? >>>> >>> Actually there is no way to handle failure during removal. And it >>> should be safe with the protection of software IOTLB even if the >>> reset() fails. >>> >>> Thanks, >>> Yongji >> >> If this is true, does it mean we don't even need to care about reset >> failure? >> > But we need to handle the failure in the vhost-vdpa case, isn't it? Yes, but: - This patch is for virtio not for vhost, if we don't care virtio, we can avoid the changes - For vhost, there could be two ways probably: 1) let the set_status to report error 2) require userspace to re-read for status It looks to me you want to go with 1) and I'm not sure whether or not it's too late to go with 2). Thanks > > Thanks, > Yongji >
On Wed, Aug 4, 2021 at 4:54 PM Jason Wang <jasowang@redhat.com> wrote: > > > 在 2021/8/4 下午4:50, Yongji Xie 写道: > > On Wed, Aug 4, 2021 at 4:32 PM Jason Wang <jasowang@redhat.com> wrote: > >> > >> 在 2021/8/3 下午5:38, Yongji Xie 写道: > >>> On Tue, Aug 3, 2021 at 4:09 PM Jason Wang <jasowang@redhat.com> wrote: > >>>> 在 2021/7/29 下午3:34, Xie Yongji 写道: > >>>>> The device reset may fail in virtio-vdpa case now, so add checks to > >>>>> its return value and fail the register_virtio_device(). > >>>> So the reset() would be called by the driver during remove as well, or > >>>> is it sufficient to deal only with the reset during probe? > >>>> > >>> Actually there is no way to handle failure during removal. And it > >>> should be safe with the protection of software IOTLB even if the > >>> reset() fails. > >>> > >>> Thanks, > >>> Yongji > >> > >> If this is true, does it mean we don't even need to care about reset > >> failure? > >> > > But we need to handle the failure in the vhost-vdpa case, isn't it? > > > Yes, but: > > - This patch is for virtio not for vhost, if we don't care virtio, we > can avoid the changes > - For vhost, there could be two ways probably: > > 1) let the set_status to report error > 2) require userspace to re-read for status > > It looks to me you want to go with 1) and I'm not sure whether or not > it's too late to go with 2). > Looks like 2) can't work if reset failure happens in vhost_vdpa_release() and vhost_vdpa_open(). Thanks, Yongji
在 2021/8/4 下午5:07, Yongji Xie 写道: > On Wed, Aug 4, 2021 at 4:54 PM Jason Wang <jasowang@redhat.com> wrote: >> >> 在 2021/8/4 下午4:50, Yongji Xie 写道: >>> On Wed, Aug 4, 2021 at 4:32 PM Jason Wang <jasowang@redhat.com> wrote: >>>> 在 2021/8/3 下午5:38, Yongji Xie 写道: >>>>> On Tue, Aug 3, 2021 at 4:09 PM Jason Wang <jasowang@redhat.com> wrote: >>>>>> 在 2021/7/29 下午3:34, Xie Yongji 写道: >>>>>>> The device reset may fail in virtio-vdpa case now, so add checks to >>>>>>> its return value and fail the register_virtio_device(). >>>>>> So the reset() would be called by the driver during remove as well, or >>>>>> is it sufficient to deal only with the reset during probe? >>>>>> >>>>> Actually there is no way to handle failure during removal. And it >>>>> should be safe with the protection of software IOTLB even if the >>>>> reset() fails. >>>>> >>>>> Thanks, >>>>> Yongji >>>> If this is true, does it mean we don't even need to care about reset >>>> failure? >>>> >>> But we need to handle the failure in the vhost-vdpa case, isn't it? >> >> Yes, but: >> >> - This patch is for virtio not for vhost, if we don't care virtio, we >> can avoid the changes >> - For vhost, there could be two ways probably: >> >> 1) let the set_status to report error >> 2) require userspace to re-read for status >> >> It looks to me you want to go with 1) and I'm not sure whether or not >> it's too late to go with 2). >> > Looks like 2) can't work if reset failure happens in > vhost_vdpa_release() and vhost_vdpa_open(). Yes, you're right. Thanks > > Thanks, > Yongji >
diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c index a15beb6b593b..8df75425fb43 100644 --- a/drivers/virtio/virtio.c +++ b/drivers/virtio/virtio.c @@ -349,7 +349,9 @@ int register_virtio_device(struct virtio_device *dev) /* We always start by resetting the device, in case a previous * driver messed it up. This also tests that code path a little. */ - dev->config->reset(dev); + err = dev->config->reset(dev); + if (err) + goto err_reset; /* Acknowledge that we've seen the device. */ virtio_add_status(dev, VIRTIO_CONFIG_S_ACKNOWLEDGE); @@ -362,10 +364,13 @@ int register_virtio_device(struct virtio_device *dev) */ err = device_add(&dev->dev); if (err) - ida_simple_remove(&virtio_index_ida, dev->index); -out: - if (err) - virtio_add_status(dev, VIRTIO_CONFIG_S_FAILED); + goto err_add; + + return 0; +err_add: + virtio_add_status(dev, VIRTIO_CONFIG_S_FAILED); +err_reset: + ida_simple_remove(&virtio_index_ida, dev->index); return err; } EXPORT_SYMBOL_GPL(register_virtio_device);
The device reset may fail in virtio-vdpa case now, so add checks to its return value and fail the register_virtio_device(). Signed-off-by: Xie Yongji <xieyongji@bytedance.com> --- drivers/virtio/virtio.c | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-)