Message-ID: <20230526114539.0520dcbf@sal.lan>
Date:   Fri, 26 May 2023 11:45:39 +0100
From:   Mauro Carvalho Chehab <mchehab@...nel.org>
To:     Dan Carpenter <dan.carpenter@...aro.org>
Cc:     Su Hui <suhui@...china.com>, YongSu Yoo <yongsuyoo0215@...il.com>,
        linux-media@...r.kernel.org, linux-kernel@...r.kernel.org,
        kernel-janitors@...r.kernel.org
Subject: Re: [PATCH] media: dvb_ringbuffer: Return -EFAULT if copy fails

Em Wed, 24 May 2023 10:20:38 +0300
Dan Carpenter <dan.carpenter@...aro.org> escreveu:

> On Wed, May 24, 2023 at 01:20:27PM +0800, Su Hui wrote:
> > It's confusing about the comment on function declaration.
> > 
> >     /**
> >      * dvb_ringbuffer_write_user - Writes a buffer received via a user
> > pointer
> > 
> >     ..........
> > 
> >      * Return: number of bytes transferred or -EFAULT
> > 
> > But the function Only returns  the number of bytes transferred.
> > 
> > Maybe the comment should be modified because it never returns -EFAULT.  
> 
> To be honest, I think that -EFAULT is probably a better return.  But
> there is no way we could apply the patch with that commit message.  The
> commit message doesn't explain the problem for the user or why returning
> the number of bytes copied is not correct in this case.
> 
> I think that maybe it's not too late to change this to return -EFAULT,
> but it would have been easier to make the change in 2014 before there
> were many users.  Also it would be easier if you were testing this on
> real hardware.

It is too late to change the API here, as this could break userspace.

Basically, DVB subsystem normally works with a Kernel-implemented ringbuffer
that transfers MPEG TS data between kernelspace/userspace. The size is
set via an ioctl (DMX_SET_BUFFER_SIZE). By the way, such uAPI is older 
than 2014. It was added upstream on Kernel 2.6.

The buffer size is usually big. For instance, dvbv5-zap uses:

	#define DVB_BUF_SIZE      (4096 * 8 * 188)

The normal operation is that data will be received from a MPEG-TS
stream, although it is also possible to send data on cable TV, when
using dvb net interface.

While on several boards, the hardware<->kernel transfer happens on
188-bytes packages, there are some hardware out there where the
data passed from/to kernel is not 188-bytes aligned.

The normal operation (receiving a TV broadcast) means that the Kernel 
will be filling a ringbuffer containing the data passed from the
hardware. The size of the such buffer is adjusted via DMX_SET_BUFFER_SIZE
and contains MPEG TS packets of 188-bytes. Userspace will be in an
endless loop that will be waiting for data to arrive at the ringbuffer,
copying received data its own userspace buffer. If the buffer is not set
to a multiple of 188, it should be up to userspace to handle incomplete
frames. The same occurs if the data is 204-bytes aligned. Btw, userspace
can detect the packet size, based on the frame content.

On such example, if a ringbuffer transfer would be passing 1554 bytes,
it means that 8 MPEG-TS frames are complete, and that 50 bytes of the
next frame was also transfered from/to userspace.

It should be up to userspace to ensure that those extra 50 bytes will
be probably taken into account by the application and ensure that the
remaining 138 bytes will be handled at the next from/to userspace
data transfer.

Not the best API, but any change there will break userspace.

In particular, this patch will completely break transfers if the
buffer size is not 188-bytes aligned.

so,

NACK.

Su,

Did you find any real problem with this? On what hardware/application?

Regards,
Mauro

Powered by blists - more mailing lists