Re: [PATCH] virtio: add support for SIZE_MAX & SEG_MAX features

[Tom Rini <trini@konsulko.com>]

[-- Attachment #1: Type: text/plain, Size: 3079 bytes --]

On Thu, Apr 02, 2026 at 01:41:11PM -0600, Simon Glass wrote:
> Hi Christian,
> 
> On 2026-03-27T08:18:58, Christian Pötzsch
> <christian.poetzsch@kernkonzept.com> wrote:
> > virtio: add support for SIZE_MAX & SEG_MAX features
> > virtio: add support for SIZE_MAX & SEG_MAX features
> >
> > Some virtio implementations may forward the virtio requests directly to
> > the underlying hw. The hw may have some restrictions in how many and how
> > big the requests can be. Therefore, the corresponding virtio device will
> > announce this limitations with the SIZE_MAX & SEG_MAX feature.
> >
> > Add support for those features. Split an io request into multiple virtio
> > requests if more than seg_max segments would be used. Also split a
> > single buffer request into multiple segments if the buffer is bigger
> > then size_max.
> >
> > Signed-off-by: Christian Pötzsch <christian.poetzsch@kernkonzept.com>
> > Signed-off-by: Adam Lackorzynski <adam@l4re.org>
> 
> > diff --git a/drivers/virtio/virtio_blk.c b/drivers/virtio/virtio_blk.c
> > @@ -119,7 +145,38 @@ static ulong virtio_blk_do_req(...)
> > +err_free:
> > +     for (i = 0; i < seg_cnt + 2; ++i)
> > +             free(sgs[i]);
> > +     free(sgs);
> > +
> > +     return status == VIRTIO_BLK_S_OK ? blkcnt : -EIO;
> 
> The err_free label is reached from both the success path (fall-through
> after virtqueue_get_buf()) and error paths (goto from malloc()
> failure, virtqueue_add() failure, or unknown type). On error paths,
> status is uninitialised, so the final check has undefined behaviour.
> Please can you separate the success path from the error cleanup, e.g.
> with an explicit return before the err_free label.
> 
> > diff --git a/drivers/virtio/virtio_blk.c b/drivers/virtio/virtio_blk.c
> > @@ -145,38 +145,38 @@ static ulong virtio_blk_do_req(...)
> > +     while (i < blkcnt) {
> > +             u32 blk_per_sg = min(blkcnt - i, seg_sec_cnt * priv->seg_max);
> > +
> > +             ret = virtio_blk_do_single_req(dev, sector + i, blk_per_sg,
> > +                                            buffer + i * 512, type);
> > +             if (!ret)
> > +                     return ret;
> 
> The error check is inverted. virtio_blk_do_single_req() returns blkcnt
> (positive) on success and -EIO on failure, so if (!ret) only triggers
> when ret is zero. I suspect you want if (ret < 0) here.
> 
> BTW please add a function comment for virtio_blk_do_single_req() so
> all this is clearer.

This is all true, and useful feedback.

> > diff --git a/drivers/virtio/virtio_blk.c b/drivers/virtio/virtio_blk.c
> > @@ -67,50 +74,69 @@ static void virtio_blk_init_data_sg(...)
> > +     // The virtio device may have constrains on the maximum segment size.
> 
> U-Boot follows the Linux kernel coding style which prefers /* */
> comments over //. Please can you convert these.

This hasn't been true for a number of years, Linux Kernel nor here.
We've accepted C99-style comments (when not needing to be kernel-doc)
for a long time now.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]