Em Wed, 24 May 2023 10:20:38 +0300 Dan Carpenter <dan.carpenter@xxxxxxxxxx> 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
