Message-ID: <5a51136c-43d6-10ea-e60d-f8ebf3b19dfc@nfschina.com>
Date:   Mon, 29 May 2023 09:54:09 +0800
From:   Su Hui <suhui@...china.com>
To:     Mauro Carvalho Chehab <mchehab@...nel.org>
Cc:     YongSu Yoo <yongsuyoo0215@...il.com>, linux-media@...r.kernel.org,
        linux-kernel@...r.kernel.org, kernel-janitors@...r.kernel.org,
        Dan Carpenter <dan.carpenter@...aro.org>
Subject: Re: [PATCH] media: dvb_ringbuffer: Return -EFAULT if copy fails


On 2023/5/26 18:45, Mauro Carvalho Chehab wrote:
> 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?
There is no real problem.
I understand, and this patch is wrong.
Sorry to bother you.

Su Hui

>
> Regards,
> Mauro
>

Powered by blists - more mailing lists