virtqueue_resize_packed() or virtqueue_resize_split() can return
error code if failed, so add a check for this.

Signed-off-by: Su Hui <suhui@nfschina.com>
---

I'm not sure that return directly is right or not,
maybe there are some process should do before return.

 drivers/virtio/virtio_ring.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index 51d8f3299c10..cf662c3a755b 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -2759,6 +2759,9 @@ int virtqueue_resize(struct virtqueue *_vq, u32 num,
 	else
 		err = virtqueue_resize_split(_vq, num);
 
+	if (err)
+		return err;
+
 	return virtqueue_enable_after_reset(_vq);
 }
 EXPORT_SYMBOL_GPL(virtqueue_resize);
-- 
2.30.2

On Fri, Oct 20, 2023 at 05:23:21PM +0800, Su Hui wrote:
> virtqueue_resize_packed() or virtqueue_resize_split() can return
> error code if failed, so add a check for this.
> 
> Signed-off-by: Su Hui <suhui@nfschina.com>
> ---
> 
> I'm not sure that return directly is right or not,
> maybe there are some process should do before return.

yes - presizely what virtqueue_enable_after_reset does.

Error handling in virtqueue_enable_after_reset is really weird BTW.
For some reason it overrides the error code returned.





>  drivers/virtio/virtio_ring.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> index 51d8f3299c10..cf662c3a755b 100644
> --- a/drivers/virtio/virtio_ring.c
> +++ b/drivers/virtio/virtio_ring.c
> @@ -2759,6 +2759,9 @@ int virtqueue_resize(struct virtqueue *_vq, u32 num,
>  	else
>  		err = virtqueue_resize_split(_vq, num);
>  
> +	if (err)
> +		return err;
> +
>  	return virtqueue_enable_after_reset(_vq);

So I think it should be something like:

	int err_reset = virtqueue_enable_after_reset(_vq);
	BUG_ON(err_reset);

	return err;



>  }
>  EXPORT_SYMBOL_GPL(virtqueue_resize);
> -- 
> 2.30.2

On Fri, 20 Oct 2023 05:34:32 -0400, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Fri, Oct 20, 2023 at 05:23:21PM +0800, Su Hui wrote:
> > virtqueue_resize_packed() or virtqueue_resize_split() can return
> > error code if failed, so add a check for this.
> >
> > Signed-off-by: Su Hui <suhui@nfschina.com>
> > ---
> >
> > I'm not sure that return directly is right or not,
> > maybe there are some process should do before return.
>
> yes - presizely what virtqueue_enable_after_reset does.
>
> Error handling in virtqueue_enable_after_reset is really weird BTW.
> For some reason it overrides the error code returned.
>
>
>
>
>
> >  drivers/virtio/virtio_ring.c | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> > index 51d8f3299c10..cf662c3a755b 100644
> > --- a/drivers/virtio/virtio_ring.c
> > +++ b/drivers/virtio/virtio_ring.c
> > @@ -2759,6 +2759,9 @@ int virtqueue_resize(struct virtqueue *_vq, u32 num,
> >  	else
> >  		err = virtqueue_resize_split(_vq, num);
> >
> > +	if (err)
> > +		return err;
> > +
> >  	return virtqueue_enable_after_reset(_vq);
>
> So I think it should be something like:
>
> 	int err_reset = virtqueue_enable_after_reset(_vq);
> 	BUG_ON(err_reset);
>
> 	return err;
>

How about WARN and vq->broken?

Thanks.


>
>
> >  }
> >  EXPORT_SYMBOL_GPL(virtqueue_resize);
> > --
> > 2.30.2
>

On Fri, Oct 20, 2023 at 05:36:41PM +0800, Xuan Zhuo wrote:
> On Fri, 20 Oct 2023 05:34:32 -0400, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > On Fri, Oct 20, 2023 at 05:23:21PM +0800, Su Hui wrote:
> > > virtqueue_resize_packed() or virtqueue_resize_split() can return
> > > error code if failed, so add a check for this.
> > >
> > > Signed-off-by: Su Hui <suhui@nfschina.com>
> > > ---
> > >
> > > I'm not sure that return directly is right or not,
> > > maybe there are some process should do before return.
> >
> > yes - presizely what virtqueue_enable_after_reset does.
> >
> > Error handling in virtqueue_enable_after_reset is really weird BTW.
> > For some reason it overrides the error code returned.
> >
> >
> >
> >
> >
> > >  drivers/virtio/virtio_ring.c | 3 +++
> > >  1 file changed, 3 insertions(+)
> > >
> > > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> > > index 51d8f3299c10..cf662c3a755b 100644
> > > --- a/drivers/virtio/virtio_ring.c
> > > +++ b/drivers/virtio/virtio_ring.c
> > > @@ -2759,6 +2759,9 @@ int virtqueue_resize(struct virtqueue *_vq, u32 num,
> > >  	else
> > >  		err = virtqueue_resize_split(_vq, num);
> > >
> > > +	if (err)
> > > +		return err;
> > > +
> > >  	return virtqueue_enable_after_reset(_vq);
> >
> > So I think it should be something like:
> >
> > 	int err_reset = virtqueue_enable_after_reset(_vq);
> > 	BUG_ON(err_reset);
> >
> > 	return err;
> >
> 
> How about WARN and vq->broken?
> 
> Thanks.

Well, what are the cases where it can happen practically?

> 
> >
> >
> > >  }
> > >  EXPORT_SYMBOL_GPL(virtqueue_resize);
> > > --
> > > 2.30.2
> >

On Fri, 20 Oct 2023 05:42:14 -0400, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Fri, Oct 20, 2023 at 05:36:41PM +0800, Xuan Zhuo wrote:
> > On Fri, 20 Oct 2023 05:34:32 -0400, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > On Fri, Oct 20, 2023 at 05:23:21PM +0800, Su Hui wrote:
> > > > virtqueue_resize_packed() or virtqueue_resize_split() can return
> > > > error code if failed, so add a check for this.
> > > >
> > > > Signed-off-by: Su Hui <suhui@nfschina.com>
> > > > ---
> > > >
> > > > I'm not sure that return directly is right or not,
> > > > maybe there are some process should do before return.
> > >
> > > yes - presizely what virtqueue_enable_after_reset does.
> > >
> > > Error handling in virtqueue_enable_after_reset is really weird BTW.
> > > For some reason it overrides the error code returned.
> > >
> > >
> > >
> > >
> > >
> > > >  drivers/virtio/virtio_ring.c | 3 +++
> > > >  1 file changed, 3 insertions(+)
> > > >
> > > > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> > > > index 51d8f3299c10..cf662c3a755b 100644
> > > > --- a/drivers/virtio/virtio_ring.c
> > > > +++ b/drivers/virtio/virtio_ring.c
> > > > @@ -2759,6 +2759,9 @@ int virtqueue_resize(struct virtqueue *_vq, u32 num,
> > > >  	else
> > > >  		err = virtqueue_resize_split(_vq, num);
> > > >
> > > > +	if (err)
> > > > +		return err;
> > > > +
> > > >  	return virtqueue_enable_after_reset(_vq);
> > >
> > > So I think it should be something like:
> > >
> > > 	int err_reset = virtqueue_enable_after_reset(_vq);
> > > 	BUG_ON(err_reset);
> > >
> > > 	return err;
> > >
> >
> > How about WARN and vq->broken?
> >
> > Thanks.
>
> Well, what are the cases where it can happen practically?

Device error. Such as vp_active_vq()

Thanks.


>
> >
> > >
> > >
> > > >  }
> > > >  EXPORT_SYMBOL_GPL(virtqueue_resize);
> > > > --
> > > > 2.30.2
> > >
>

On Fri, Oct 20, 2023 at 05:50:22PM +0800, Xuan Zhuo wrote:
> On Fri, 20 Oct 2023 05:42:14 -0400, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > On Fri, Oct 20, 2023 at 05:36:41PM +0800, Xuan Zhuo wrote:
> > > On Fri, 20 Oct 2023 05:34:32 -0400, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > > On Fri, Oct 20, 2023 at 05:23:21PM +0800, Su Hui wrote:
> > > > > virtqueue_resize_packed() or virtqueue_resize_split() can return
> > > > > error code if failed, so add a check for this.
> > > > >
> > > > > Signed-off-by: Su Hui <suhui@nfschina.com>
> > > > > ---
> > > > >
> > > > > I'm not sure that return directly is right or not,
> > > > > maybe there are some process should do before return.
> > > >
> > > > yes - presizely what virtqueue_enable_after_reset does.
> > > >
> > > > Error handling in virtqueue_enable_after_reset is really weird BTW.
> > > > For some reason it overrides the error code returned.
> > > >
> > > >
> > > >
> > > >
> > > >
> > > > >  drivers/virtio/virtio_ring.c | 3 +++
> > > > >  1 file changed, 3 insertions(+)
> > > > >
> > > > > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> > > > > index 51d8f3299c10..cf662c3a755b 100644
> > > > > --- a/drivers/virtio/virtio_ring.c
> > > > > +++ b/drivers/virtio/virtio_ring.c
> > > > > @@ -2759,6 +2759,9 @@ int virtqueue_resize(struct virtqueue *_vq, u32 num,
> > > > >  	else
> > > > >  		err = virtqueue_resize_split(_vq, num);
> > > > >
> > > > > +	if (err)
> > > > > +		return err;
> > > > > +
> > > > >  	return virtqueue_enable_after_reset(_vq);
> > > >
> > > > So I think it should be something like:
> > > >
> > > > 	int err_reset = virtqueue_enable_after_reset(_vq);
> > > > 	BUG_ON(err_reset);
> > > >
> > > > 	return err;
> > > >
> > >
> > > How about WARN and vq->broken?
> > >
> > > Thanks.
> >
> > Well, what are the cases where it can happen practically?
> 
> Device error. Such as vp_active_vq()
> 
> Thanks.

Hmm interesting. OK. But do callers know to recover?

> 
> >
> > >
> > > >
> > > >
> > > > >  }
> > > > >  EXPORT_SYMBOL_GPL(virtqueue_resize);
> > > > > --
> > > > > 2.30.2
> > > >
> >

On Fri, 20 Oct 2023 06:08:06 -0400, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Fri, Oct 20, 2023 at 05:50:22PM +0800, Xuan Zhuo wrote:
> > On Fri, 20 Oct 2023 05:42:14 -0400, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > On Fri, Oct 20, 2023 at 05:36:41PM +0800, Xuan Zhuo wrote:
> > > > On Fri, 20 Oct 2023 05:34:32 -0400, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > > > On Fri, Oct 20, 2023 at 05:23:21PM +0800, Su Hui wrote:
> > > > > > virtqueue_resize_packed() or virtqueue_resize_split() can return
> > > > > > error code if failed, so add a check for this.
> > > > > >
> > > > > > Signed-off-by: Su Hui <suhui@nfschina.com>
> > > > > > ---
> > > > > >
> > > > > > I'm not sure that return directly is right or not,
> > > > > > maybe there are some process should do before return.
> > > > >
> > > > > yes - presizely what virtqueue_enable_after_reset does.
> > > > >
> > > > > Error handling in virtqueue_enable_after_reset is really weird BTW.
> > > > > For some reason it overrides the error code returned.
> > > > >
> > > > >
> > > > >
> > > > >
> > > > >
> > > > > >  drivers/virtio/virtio_ring.c | 3 +++
> > > > > >  1 file changed, 3 insertions(+)
> > > > > >
> > > > > > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> > > > > > index 51d8f3299c10..cf662c3a755b 100644
> > > > > > --- a/drivers/virtio/virtio_ring.c
> > > > > > +++ b/drivers/virtio/virtio_ring.c
> > > > > > @@ -2759,6 +2759,9 @@ int virtqueue_resize(struct virtqueue *_vq, u32 num,
> > > > > >  	else
> > > > > >  		err = virtqueue_resize_split(_vq, num);
> > > > > >
> > > > > > +	if (err)
> > > > > > +		return err;
> > > > > > +
> > > > > >  	return virtqueue_enable_after_reset(_vq);
> > > > >
> > > > > So I think it should be something like:
> > > > >
> > > > > 	int err_reset = virtqueue_enable_after_reset(_vq);
> > > > > 	BUG_ON(err_reset);
> > > > >
> > > > > 	return err;
> > > > >
> > > >
> > > > How about WARN and vq->broken?
> > > >
> > > > Thanks.
> > >
> > > Well, what are the cases where it can happen practically?
> >
> > Device error. Such as vp_active_vq()
> >
> > Thanks.
>
> Hmm interesting. OK. But do callers know to recover?


No.

So I think WARN + broken is suitable.

Thanks.


>
> >
> > >
> > > >
> > > > >
> > > > >
> > > > > >  }
> > > > > >  EXPORT_SYMBOL_GPL(virtqueue_resize);
> > > > > > --
> > > > > > 2.30.2
> > > > >
> > >
>
> _______________________________________________
> Virtualization mailing list
> Virtualization@lists.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/virtualization

On 2023/10/23 10:26, Xuan Zhuo wrote:
>>>> Well, what are the cases where it can happen practically?
>>> Device error. Such as vp_active_vq()
>>>
>>> Thanks.
>> Hmm interesting. OK. But do callers know to recover?
>
> No.
>
> So I think WARN + broken is suitable.
>
> Thanks.
Sorry for the late, is the following code okay?

@@ -2739,7 +2739,7 @@ int virtqueue_resize(struct virtqueue *_vq, u32 num,
                      void (*recycle)(struct virtqueue *vq, void *buf))
  {
         struct vring_virtqueue *vq = to_vvq(_vq);
-       int err;
+       int err, err_reset;
  
         if (num > vq->vq.num_max)
                 return -E2BIG;
@@ -2759,7 +2759,15 @@ int virtqueue_resize(struct virtqueue *_vq, u32 num,
         else
                 err = virtqueue_resize_split(_vq, num);

-       return virtqueue_enable_after_reset(_vq);
+       err_reset = virtqueue_enable_after_reset(_vq);
+
+       if (err) {
+               vq->broken = true;
+               WARN_ON(err_reset);
+               return err;
+       }
+
+       return err_reset;
  }

Thanks.
Su Hui

On Mon, 23 Oct 2023 10:51:59 +0800, Su Hui <suhui@nfschina.com> wrote:
> On 2023/10/23 10:26, Xuan Zhuo wrote:
> >>>> Well, what are the cases where it can happen practically?
> >>> Device error. Such as vp_active_vq()
> >>>
> >>> Thanks.
> >> Hmm interesting. OK. But do callers know to recover?
> >
> > No.
> >
> > So I think WARN + broken is suitable.
> >
> > Thanks.
> Sorry for the late, is the following code okay?
>
> @@ -2739,7 +2739,7 @@ int virtqueue_resize(struct virtqueue *_vq, u32 num,
>                       void (*recycle)(struct virtqueue *vq, void *buf))
>   {
>          struct vring_virtqueue *vq = to_vvq(_vq);
> -       int err;
> +       int err, err_reset;
>
>          if (num > vq->vq.num_max)
>                  return -E2BIG;
> @@ -2759,7 +2759,15 @@ int virtqueue_resize(struct virtqueue *_vq, u32 num,
>          else
>                  err = virtqueue_resize_split(_vq, num);
>
> -       return virtqueue_enable_after_reset(_vq);
> +       err_reset = virtqueue_enable_after_reset(_vq);
> +
> +       if (err) {

No err.

err is not important.
You can remove that.

Thanks.


> +               vq->broken = true;
> +               WARN_ON(err_reset);
> +               return err;
> +       }
> +
> +       return err_reset;
>   }
>
> Thanks.
> Su Hui
>

On 2023/10/23 10:53, Xuan Zhuo wrote:
> On Mon, 23 Oct 2023 10:51:59 +0800, Su Hui <suhui@nfschina.com> wrote:
>> On 2023/10/23 10:26, Xuan Zhuo wrote:
>>>>>> Well, what are the cases where it can happen practically?
>>>>> Device error. Such as vp_active_vq()
>>>>>
>>>>> Thanks.
>>>> Hmm interesting. OK. But do callers know to recover?
>>> No.
>>>
>>> So I think WARN + broken is suitable.
>>>
>>> Thanks.
>> Sorry for the late, is the following code okay?
>>
>> @@ -2739,7 +2739,7 @@ int virtqueue_resize(struct virtqueue *_vq, u32 num,
>>                        void (*recycle)(struct virtqueue *vq, void *buf))
>>    {
>>           struct vring_virtqueue *vq = to_vvq(_vq);
>> -       int err;
>> +       int err, err_reset;
>>
>>           if (num > vq->vq.num_max)
>>                   return -E2BIG;
>> @@ -2759,7 +2759,15 @@ int virtqueue_resize(struct virtqueue *_vq, u32 num,
>>           else
>>                   err = virtqueue_resize_split(_vq, num);
>>
>> -       return virtqueue_enable_after_reset(_vq);
>> +       err_reset = virtqueue_enable_after_reset(_vq);
>> +
>> +       if (err) {
> No err.
>
> err is not important.
> You can remove that.

Emm, I'm a little confused that which code should I remove ?

> Thanks.
>
>
>> +               vq->broken = true;
>> +               WARN_ON(err_reset);
>> +               return err;
>> +       }
>> +
>> +       return err_reset;
>>    }
>>
>> Thanks.
>> Su Hui
>>

On Mon, 23 Oct 2023 11:06:48 +0800, Su Hui <suhui@nfschina.com> wrote:
>
> On 2023/10/23 10:53, Xuan Zhuo wrote:
> > On Mon, 23 Oct 2023 10:51:59 +0800, Su Hui <suhui@nfschina.com> wrote:
> >> On 2023/10/23 10:26, Xuan Zhuo wrote:
> >>>>>> Well, what are the cases where it can happen practically?
> >>>>> Device error. Such as vp_active_vq()
> >>>>>
> >>>>> Thanks.
> >>>> Hmm interesting. OK. But do callers know to recover?
> >>> No.
> >>>
> >>> So I think WARN + broken is suitable.
> >>>
> >>> Thanks.
> >> Sorry for the late, is the following code okay?
> >>
> >> @@ -2739,7 +2739,7 @@ int virtqueue_resize(struct virtqueue *_vq, u32 num,
> >>                        void (*recycle)(struct virtqueue *vq, void *buf))
> >>    {
> >>           struct vring_virtqueue *vq = to_vvq(_vq);
> >> -       int err;
> >> +       int err, err_reset;
> >>
> >>           if (num > vq->vq.num_max)
> >>                   return -E2BIG;
> >> @@ -2759,7 +2759,15 @@ int virtqueue_resize(struct virtqueue *_vq, u32 num,
> >>           else
> >>                   err = virtqueue_resize_split(_vq, num);
> >>
> >> -       return virtqueue_enable_after_reset(_vq);
> >> +       err_reset = virtqueue_enable_after_reset(_vq);
> >> +
> >> +       if (err) {
> > No err.
> >
> > err is not important.
> > You can remove that.
>
> Emm, I'm a little confused that which code should I remove ?


like this:
	if (vq->packed_ring)
		virtqueue_resize_packed(_vq, num);
	else
		virtqueue_resize_split(_vq, num);

And we should set broken and warn inside virtqueue_enable_after_reset()?

Thanks.




>
> > Thanks.
> >
> >
> >> +               vq->broken = true;
> >> +               WARN_ON(err_reset);
> >> +               return err;
> >> +       }
> >> +
> >> +       return err_reset;
> >>    }
> >>
> >> Thanks.
> >> Su Hui
> >>

On 2023/10/23 13:46, Xuan Zhuo wrote:
>>>>>>>> Well, what are the cases where it can happen practically?
>>>>>>> Device error. Such as vp_active_vq()
>>>>>>>
>>>>>>> Thanks.
>>>>>> Hmm interesting. OK. But do callers know to recover?
>>>>> No.
>>>>>
>>>>> So I think WARN + broken is suitable.
>>>>>
>>>>> Thanks.
>>>> Sorry for the late, is the following code okay?
>>>>
>>>> @@ -2739,7 +2739,7 @@ int virtqueue_resize(struct virtqueue *_vq, u32 num,
>>>>                         void (*recycle)(struct virtqueue *vq, void *buf))
>>>>     {
>>>>            struct vring_virtqueue *vq = to_vvq(_vq);
>>>> -       int err;
>>>> +       int err, err_reset;
>>>>
>>>>            if (num > vq->vq.num_max)
>>>>                    return -E2BIG;
>>>> @@ -2759,7 +2759,15 @@ int virtqueue_resize(struct virtqueue *_vq, u32 num,
>>>>            else
>>>>                    err = virtqueue_resize_split(_vq, num);
>>>>
>>>> -       return virtqueue_enable_after_reset(_vq);
>>>> +       err_reset = virtqueue_enable_after_reset(_vq);
>>>> +
>>>> +       if (err) {
>>> No err.
>>>
>>> err is not important.
>>> You can remove that.
>> Emm, I'm a little confused that which code should I remove ?
>>
>>
>> like this:
>> 	if (vq->packed_ring)
>> 		virtqueue_resize_packed(_vq, num);
>> 	else
>> 		virtqueue_resize_split(_vq, num);
>>
>> And we should set broken and warn inside virtqueue_enable_after_reset()?

In my opinion, we should return the error code of virtqueue_resize_packed() / virtqueue_resize_split().
But if this err is not important, this patch makes no sense.
Maybe I misunderstand somewhere...
If you think it's worth sending a patch, you can send it :).(I'm not familiar with this code).

Thanks,
Su Hui

On Mon, 23 Oct 2023 17:50:46 +0800, Su Hui <suhui@nfschina.com> wrote:
> On 2023/10/23 13:46, Xuan Zhuo wrote:
> >>>>>>>> Well, what are the cases where it can happen practically?
> >>>>>>> Device error. Such as vp_active_vq()
> >>>>>>>
> >>>>>>> Thanks.
> >>>>>> Hmm interesting. OK. But do callers know to recover?
> >>>>> No.
> >>>>>
> >>>>> So I think WARN + broken is suitable.
> >>>>>
> >>>>> Thanks.
> >>>> Sorry for the late, is the following code okay?
> >>>>
> >>>> @@ -2739,7 +2739,7 @@ int virtqueue_resize(struct virtqueue *_vq, u32 num,
> >>>>                         void (*recycle)(struct virtqueue *vq, void *buf))
> >>>>     {
> >>>>            struct vring_virtqueue *vq = to_vvq(_vq);
> >>>> -       int err;
> >>>> +       int err, err_reset;
> >>>>
> >>>>            if (num > vq->vq.num_max)
> >>>>                    return -E2BIG;
> >>>> @@ -2759,7 +2759,15 @@ int virtqueue_resize(struct virtqueue *_vq, u32 num,
> >>>>            else
> >>>>                    err = virtqueue_resize_split(_vq, num);
> >>>>
> >>>> -       return virtqueue_enable_after_reset(_vq);
> >>>> +       err_reset = virtqueue_enable_after_reset(_vq);
> >>>> +
> >>>> +       if (err) {
> >>> No err.
> >>>
> >>> err is not important.
> >>> You can remove that.
> >> Emm, I'm a little confused that which code should I remove ?
> >>
> >>
> >> like this:
> >> 	if (vq->packed_ring)
> >> 		virtqueue_resize_packed(_vq, num);
> >> 	else
> >> 		virtqueue_resize_split(_vq, num);
> >>
> >> And we should set broken and warn inside virtqueue_enable_after_reset()?
>
> In my opinion, we should return the error code of virtqueue_resize_packed() / virtqueue_resize_split().
> But if this err is not important, this patch makes no sense.
> Maybe I misunderstand somewhere...
> If you think it's worth sending a patch, you can send it :).(I'm not familiar with this code).

OK.

Thanks.


>
> Thanks,
> Su Hui
>

On Mon, Oct 23, 2023 at 05:52:02PM +0800, Xuan Zhuo wrote:
> On Mon, 23 Oct 2023 17:50:46 +0800, Su Hui <suhui@nfschina.com> wrote:
> > On 2023/10/23 13:46, Xuan Zhuo wrote:
> > >>>>>>>> Well, what are the cases where it can happen practically?
> > >>>>>>> Device error. Such as vp_active_vq()
> > >>>>>>>
> > >>>>>>> Thanks.
> > >>>>>> Hmm interesting. OK. But do callers know to recover?
> > >>>>> No.
> > >>>>>
> > >>>>> So I think WARN + broken is suitable.
> > >>>>>
> > >>>>> Thanks.
> > >>>> Sorry for the late, is the following code okay?
> > >>>>
> > >>>> @@ -2739,7 +2739,7 @@ int virtqueue_resize(struct virtqueue *_vq, u32 num,
> > >>>>                         void (*recycle)(struct virtqueue *vq, void *buf))
> > >>>>     {
> > >>>>            struct vring_virtqueue *vq = to_vvq(_vq);
> > >>>> -       int err;
> > >>>> +       int err, err_reset;
> > >>>>
> > >>>>            if (num > vq->vq.num_max)
> > >>>>                    return -E2BIG;
> > >>>> @@ -2759,7 +2759,15 @@ int virtqueue_resize(struct virtqueue *_vq, u32 num,
> > >>>>            else
> > >>>>                    err = virtqueue_resize_split(_vq, num);
> > >>>>
> > >>>> -       return virtqueue_enable_after_reset(_vq);
> > >>>> +       err_reset = virtqueue_enable_after_reset(_vq);
> > >>>> +
> > >>>> +       if (err) {
> > >>> No err.
> > >>>
> > >>> err is not important.
> > >>> You can remove that.
> > >> Emm, I'm a little confused that which code should I remove ?
> > >>
> > >>
> > >> like this:
> > >> 	if (vq->packed_ring)
> > >> 		virtqueue_resize_packed(_vq, num);
> > >> 	else
> > >> 		virtqueue_resize_split(_vq, num);
> > >>
> > >> And we should set broken and warn inside virtqueue_enable_after_reset()?
> >
> > In my opinion, we should return the error code of virtqueue_resize_packed() / virtqueue_resize_split().
> > But if this err is not important, this patch makes no sense.
> > Maybe I misunderstand somewhere...
> > If you think it's worth sending a patch, you can send it :).(I'm not familiar with this code).
> 
> OK.
> 
> Thanks.

I would first try to recover by re-enabling.
If that fails we can set broken.


> 
> >
> > Thanks,
> > Su Hui
> >

On Mon, 23 Oct 2023 17:52:02 +0800, Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote:
> On Mon, 23 Oct 2023 17:50:46 +0800, Su Hui <suhui@nfschina.com> wrote:
> > On 2023/10/23 13:46, Xuan Zhuo wrote:
> > >>>>>>>> Well, what are the cases where it can happen practically?
> > >>>>>>> Device error. Such as vp_active_vq()
> > >>>>>>>
> > >>>>>>> Thanks.
> > >>>>>> Hmm interesting. OK. But do callers know to recover?
> > >>>>> No.
> > >>>>>
> > >>>>> So I think WARN + broken is suitable.
> > >>>>>
> > >>>>> Thanks.
> > >>>> Sorry for the late, is the following code okay?
> > >>>>
> > >>>> @@ -2739,7 +2739,7 @@ int virtqueue_resize(struct virtqueue *_vq, u32 num,
> > >>>>                         void (*recycle)(struct virtqueue *vq, void *buf))
> > >>>>     {
> > >>>>            struct vring_virtqueue *vq = to_vvq(_vq);
> > >>>> -       int err;
> > >>>> +       int err, err_reset;
> > >>>>
> > >>>>            if (num > vq->vq.num_max)
> > >>>>                    return -E2BIG;
> > >>>> @@ -2759,7 +2759,15 @@ int virtqueue_resize(struct virtqueue *_vq, u32 num,
> > >>>>            else
> > >>>>                    err = virtqueue_resize_split(_vq, num);
> > >>>>
> > >>>> -       return virtqueue_enable_after_reset(_vq);
> > >>>> +       err_reset = virtqueue_enable_after_reset(_vq);
> > >>>> +
> > >>>> +       if (err) {
> > >>> No err.
> > >>>
> > >>> err is not important.
> > >>> You can remove that.
> > >> Emm, I'm a little confused that which code should I remove ?
> > >>
> > >>
> > >> like this:
> > >> 	if (vq->packed_ring)
> > >> 		virtqueue_resize_packed(_vq, num);
> > >> 	else
> > >> 		virtqueue_resize_split(_vq, num);
> > >>
> > >> And we should set broken and warn inside virtqueue_enable_after_reset()?
> >
> > In my opinion, we should return the error code of virtqueue_resize_packed() / virtqueue_resize_split().
> > But if this err is not important, this patch makes no sense.
> > Maybe I misunderstand somewhere...
> > If you think it's worth sending a patch, you can send it :).(I'm not familiar with this code).
>
> OK.

Hi Michael,

The queue reset code is wrote with the CONFIG_VIRTIO_HARDEN_NOTIFICATION.

When we disable the vq, the broken is true until we re-enable it.

So when we re-enable it fail, the vq is broken status.

Normally, this just happens on the buggy device.
So I think that is enough.

Thanks.


	static int vp_modern_disable_vq_and_reset(struct virtqueue *vq)
	{
		[...]

		vp_modern_set_queue_reset(mdev, vq->index);

		[...]

	#ifdef CONFIG_VIRTIO_HARDEN_NOTIFICATION
->>		__virtqueue_break(vq);
	#endif

		[...]
	}

	static int vp_modern_enable_vq_after_reset(struct virtqueue *vq)
	{
		[...]

		if (vp_modern_get_queue_reset(mdev, index))
			return -EBUSY;

		if (vp_modern_get_queue_enable(mdev, index))
			return -EBUSY;

		err = vp_active_vq(vq, info->msix_vector);
		if (err)
			return err;

		}

		[...]

	#ifdef CONFIG_VIRTIO_HARDEN_NOTIFICATION
->>		__virtqueue_unbreak(vq);
	#endif

		[...]
	}




>
> Thanks.
>
>
> >
> > Thanks,
> > Su Hui
> >

On Mon, Oct 23, 2023 at 06:52:34PM +0800, Xuan Zhuo wrote:
> On Mon, 23 Oct 2023 17:52:02 +0800, Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote:
> > On Mon, 23 Oct 2023 17:50:46 +0800, Su Hui <suhui@nfschina.com> wrote:
> > > On 2023/10/23 13:46, Xuan Zhuo wrote:
> > > >>>>>>>> Well, what are the cases where it can happen practically?
> > > >>>>>>> Device error. Such as vp_active_vq()
> > > >>>>>>>
> > > >>>>>>> Thanks.
> > > >>>>>> Hmm interesting. OK. But do callers know to recover?
> > > >>>>> No.
> > > >>>>>
> > > >>>>> So I think WARN + broken is suitable.
> > > >>>>>
> > > >>>>> Thanks.
> > > >>>> Sorry for the late, is the following code okay?
> > > >>>>
> > > >>>> @@ -2739,7 +2739,7 @@ int virtqueue_resize(struct virtqueue *_vq, u32 num,
> > > >>>>                         void (*recycle)(struct virtqueue *vq, void *buf))
> > > >>>>     {
> > > >>>>            struct vring_virtqueue *vq = to_vvq(_vq);
> > > >>>> -       int err;
> > > >>>> +       int err, err_reset;
> > > >>>>
> > > >>>>            if (num > vq->vq.num_max)
> > > >>>>                    return -E2BIG;
> > > >>>> @@ -2759,7 +2759,15 @@ int virtqueue_resize(struct virtqueue *_vq, u32 num,
> > > >>>>            else
> > > >>>>                    err = virtqueue_resize_split(_vq, num);
> > > >>>>
> > > >>>> -       return virtqueue_enable_after_reset(_vq);
> > > >>>> +       err_reset = virtqueue_enable_after_reset(_vq);
> > > >>>> +
> > > >>>> +       if (err) {
> > > >>> No err.
> > > >>>
> > > >>> err is not important.
> > > >>> You can remove that.
> > > >> Emm, I'm a little confused that which code should I remove ?
> > > >>
> > > >>
> > > >> like this:
> > > >> 	if (vq->packed_ring)
> > > >> 		virtqueue_resize_packed(_vq, num);
> > > >> 	else
> > > >> 		virtqueue_resize_split(_vq, num);
> > > >>
> > > >> And we should set broken and warn inside virtqueue_enable_after_reset()?
> > >
> > > In my opinion, we should return the error code of virtqueue_resize_packed() / virtqueue_resize_split().
> > > But if this err is not important, this patch makes no sense.
> > > Maybe I misunderstand somewhere...
> > > If you think it's worth sending a patch, you can send it :).(I'm not familiar with this code).
> >
> > OK.
> 
> Hi Michael,
> 
> The queue reset code is wrote with the CONFIG_VIRTIO_HARDEN_NOTIFICATION.
> 
> When we disable the vq, the broken is true until we re-enable it.
> 
> So when we re-enable it fail, the vq is broken status.
> 
> Normally, this just happens on the buggy device.
> So I think that is enough.
> 
> Thanks.

I don't know what to do about CONFIG_VIRTIO_HARDEN_NOTIFICATION.
It's known to be broken and it does not look like there's
active effort to revive it - should we just drop it for now?


> 
> 	static int vp_modern_disable_vq_and_reset(struct virtqueue *vq)
> 	{
> 		[...]
> 
> 		vp_modern_set_queue_reset(mdev, vq->index);
> 
> 		[...]
> 
> 	#ifdef CONFIG_VIRTIO_HARDEN_NOTIFICATION
> ->>		__virtqueue_break(vq);
> 	#endif
> 
> 		[...]
> 	}
> 
> 	static int vp_modern_enable_vq_after_reset(struct virtqueue *vq)
> 	{
> 		[...]
> 
> 		if (vp_modern_get_queue_reset(mdev, index))
> 			return -EBUSY;
> 
> 		if (vp_modern_get_queue_enable(mdev, index))
> 			return -EBUSY;
> 
> 		err = vp_active_vq(vq, info->msix_vector);
> 		if (err)
> 			return err;
> 
> 		}
> 
> 		[...]
> 
> 	#ifdef CONFIG_VIRTIO_HARDEN_NOTIFICATION
> ->>		__virtqueue_unbreak(vq);
> 	#endif
> 
> 		[...]
> 	}
> 
> 
> 
> 
> >
> > Thanks.
> >
> >
> > >
> > > Thanks,
> > > Su Hui
> > >

If any error happens, this function should restore to the old status.

So, whether the err is true, we should goto the virtqueue_enable_after_reset().

If the err is true, that mean the resize new quuee failed, but the queue
status has restored to the old status.

We should ignore the return value of the virtuqueue_resize_xxx().

Do you find other error in the virtuqueue_resize_xxx().

Thanks.


On Fri, 20 Oct 2023 17:23:21 +0800, Su Hui <suhui@nfschina.com> wrote:
> virtqueue_resize_packed() or virtqueue_resize_split() can return
> error code if failed, so add a check for this.
>
> Signed-off-by: Su Hui <suhui@nfschina.com>
> ---
>
> I'm not sure that return directly is right or not,
> maybe there are some process should do before return.
>
>  drivers/virtio/virtio_ring.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> index 51d8f3299c10..cf662c3a755b 100644
> --- a/drivers/virtio/virtio_ring.c
> +++ b/drivers/virtio/virtio_ring.c
> @@ -2759,6 +2759,9 @@ int virtqueue_resize(struct virtqueue *_vq, u32 num,
>  	else
>  		err = virtqueue_resize_split(_vq, num);
>
> +	if (err)
> +		return err;
> +
>  	return virtqueue_enable_after_reset(_vq);
>  }
>  EXPORT_SYMBOL_GPL(virtqueue_resize);
> --
> 2.30.2
>