Message ID | 1451248920-4935-1-git-send-email-smoch@web.de (mailing list archive) |
---|---|
State | Superseded, archived |
Headers |
Received: from mail.tu-berlin.de ([130.149.7.33]) by www.linuxtv.org with esmtp (Exim 4.84) (envelope-from <linux-media-owner@vger.kernel.org>) id 1aDIAY-0007CK-RM; Sun, 27 Dec 2015 20:43:58 +0000 X-tubIT-Incoming-IP: 209.132.180.67 Received: from vger.kernel.org ([209.132.180.67]) by mail.tu-berlin.de (exim-4.76/mailfrontend-8) with esmtp id 1aDIAW-00063I-k5; Sun, 27 Dec 2015 21:43:58 +0100 Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753700AbbL0Umx (ORCPT <rfc822;mkrufky@linuxtv.org> + 1 other); Sun, 27 Dec 2015 15:42:53 -0500 Received: from mout.web.de ([212.227.17.11]:54125 "EHLO mout.web.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751695AbbL0Umv (ORCPT <rfc822;linux-media@vger.kernel.org>); Sun, 27 Dec 2015 15:42:51 -0500 Received: from localhost.localdomain ([217.190.10.6]) by smtp.web.de (mrweb102) with ESMTPSA (Nemesis) id 0MQNiS-1agwUj3Bca-00TlvI; Sun, 27 Dec 2015 21:42:45 +0100 From: Soeren Moch <smoch@web.de> To: Mauro Carvalho Chehab <mchehab@osg.samsung.com> Cc: Soeren Moch <smoch@web.de>, linux-media@vger.kernel.org, linux-kernel@vger.kernel.org Subject: [PATCH] media: dvb_ringbuffer: Add memory barriers Date: Sun, 27 Dec 2015 21:41:56 +0100 Message-Id: <1451248920-4935-1-git-send-email-smoch@web.de> X-Mailer: git-send-email 1.9.1 X-Provags-ID: V03:K0:SixnNuYNbw9kL561DlNnoRc3FVuxh97qTPCxkLWeNQ263Ic8+Pu lJHCI6hH/ZIkDEgLmEovN42SPn5hVJkZf7L9thPIQ+1kzjCTIqIyK0yJPgQ8I8BuPJKizgC zf0iQeKhaixnTwvbHC1DC46JrlKP60LTAC9CSAmcNv2smYJ6GKw8oiI149rLbK5HsMjz8Mp Gwwg8+sqNyAca1isvV5IA== X-UI-Out-Filterresults: notjunk:1; V01:K0:iLeGBtKSjDg=:1ZNKP1edeVb1y5U7tOq9pm 10kb5y7whIxkdnGWzhQKRwFl+8TNtZ6fMiR4JF3CCWCT7UTD15iCQoYEzikXV+WeBnfnAYvmS UiGhWMGIADuZBwUzlZcPUNB5pd590tQRqod6G1pz/fFy/5D6OPuqLyMlBWvkqMOomZGGRfN3S zHFczggRSMwzaNdgi812xImdHXy3xwX2FAS3ODBz++rN6EX0cgXsT7F8PK8j6S0XMVPMieUZv DWSTK2IVRrcAT5a75GVnh361+UA3KghMjaWLaP7aLzwfY/J135/DWUjsWtn+w/xBuxr3dz2gN sA2ap7Mv7ZihNr7ItFn42BgcVlVixW64iMojP70/APbVq8RuqU40EszYg/YBsGbl1ZQBd3Nu+ 2UWomSBEKseY1cs0Cx48SoOlUt8ufJDaOFW29qtryxiPiUCcNsY3PMLLTNz8gk3BYBHKtt+oN wxTmfi6gg/LRAJ9qlQz4mSQmpT19SwkrjAkUVrp4AMGraucgPv7mzy3KLHt9MVdlA/bfNu0rp ENoiI3VDUPO6xA/b484HajJDLsW6mWVrhuPNmaN37rGu0n9sOpGZCL3dqUNYR95PifMeRh5mD 9aOhb3wS75p2wxeYHmdRxm6jdUfveDHv88aJ4bJEeywT5OlCEvMsesEdPOyxTx5o4zaHA2TM0 9EcDlwA/c3QMcooj5dgaBqaHa1ypd1R/RwYsbuO8Fe83tVXc/phlGJP8VHxlGFjq1TaeG26CG CFIQS7REvqQMXdGU5yWFSuXroc2OxXSrbhuCdLobKj5ixt47dgqVkWqDlGU= Sender: linux-media-owner@vger.kernel.org Precedence: bulk List-ID: <linux-media.vger.kernel.org> X-Mailing-List: linux-media@vger.kernel.org X-PMX-Version: 6.0.0.2142326, Antispam-Engine: 2.7.2.2107409, Antispam-Data: 2015.12.27.202716 X-PMX-Spam: Gauge=IIIIIIII, Probability=8%, Report=' MULTIPLE_RCPTS 0.1, HTML_00_01 0.05, HTML_00_10 0.05, BODY_SIZE_4000_4999 0, BODY_SIZE_5000_LESS 0, BODY_SIZE_7000_LESS 0, NO_URI_HTTPS 0, SINGLE_URI_IN_BODY 0, URI_ENDS_IN_HTML 0, __ANY_URI 0, __CP_URI_IN_BODY 0, __HAS_FROM 0, __HAS_MSGID 0, __HAS_X_MAILER 0, __HAS_X_MAILING_LIST 0, __MIME_TEXT_ONLY 0, __MULTIPLE_RCPTS_CC_X2 0, __SANE_MSGID 0, __SINGLE_URI_TEXT 0, __SUBJ_ALPHA_END 0, __TO_MALFORMED_2 0, __URI_IN_BODY 0, __URI_NO_WWW 0, __URI_NS ' |
Commit Message
Soeren Moch
Dec. 27, 2015, 8:41 p.m. UTC
Implement memory barriers according to Documentation/circular-buffers.txt:
- use smp_store_release() to update ringbuffer read/write pointers
- use smp_load_acquire() to load write pointer on reader side
- use ACCESS_ONCE() to load read pointer on writer side
This fixes data stream corruptions observed e.g. on an ARM Cortex-A9
quad core system with different types (PCI, USB) of DVB tuners.
Signed-off-by: Soeren Moch <smoch@web.de>
Cc: stable@vger.kernel.org # 3.14+
---
Cc: Mauro Carvalho Chehab <mchehab@osg.samsung.com>
Cc: linux-media@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Since smp_store_release() and smp_load_acquire() were introduced in linux-3.14,
a 3.14+ stable tag was added. Is it desired to apply a similar patch to older
stable kernels?
---
drivers/media/dvb-core/dvb_ringbuffer.c | 27 ++++++++++++++-------------
1 file changed, 14 insertions(+), 13 deletions(-)
Comments
On 27.12.2015 21:41, Soeren Moch wrote: > Implement memory barriers according to Documentation/circular-buffers.txt: > - use smp_store_release() to update ringbuffer read/write pointers > - use smp_load_acquire() to load write pointer on reader side > - use ACCESS_ONCE() to load read pointer on writer side > > This fixes data stream corruptions observed e.g. on an ARM Cortex-A9 > quad core system with different types (PCI, USB) of DVB tuners. > > Signed-off-by: Soeren Moch <smoch@web.de> > Cc: stable@vger.kernel.org # 3.14+ Mauro, any news or comments on this? Since this is a real fix for broken behaviour, can you pick this up, please? Regards, Soeren > --- > Cc: Mauro Carvalho Chehab <mchehab@osg.samsung.com> > Cc: linux-media@vger.kernel.org > Cc: linux-kernel@vger.kernel.org > > Since smp_store_release() and smp_load_acquire() were introduced in linux-3.14, > a 3.14+ stable tag was added. Is it desired to apply a similar patch to older > stable kernels? > --- > drivers/media/dvb-core/dvb_ringbuffer.c | 27 ++++++++++++++------------- > 1 file changed, 14 insertions(+), 13 deletions(-) > > diff --git a/drivers/media/dvb-core/dvb_ringbuffer.c b/drivers/media/dvb-core/dvb_ringbuffer.c > index 1100e98..58b5968 100644 > --- a/drivers/media/dvb-core/dvb_ringbuffer.c > +++ b/drivers/media/dvb-core/dvb_ringbuffer.c > @@ -55,7 +55,7 @@ void dvb_ringbuffer_init(struct dvb_ringbuffer *rbuf, void *data, size_t len) > > int dvb_ringbuffer_empty(struct dvb_ringbuffer *rbuf) > { > - return (rbuf->pread==rbuf->pwrite); > + return (rbuf->pread == smp_load_acquire(&rbuf->pwrite)); > } > > > @@ -64,7 +64,7 @@ ssize_t dvb_ringbuffer_free(struct dvb_ringbuffer *rbuf) > { > ssize_t free; > > - free = rbuf->pread - rbuf->pwrite; > + free = ACCESS_ONCE(rbuf->pread) - rbuf->pwrite; > if (free <= 0) > free += rbuf->size; > return free-1; > @@ -76,7 +76,7 @@ ssize_t dvb_ringbuffer_avail(struct dvb_ringbuffer *rbuf) > { > ssize_t avail; > > - avail = rbuf->pwrite - rbuf->pread; > + avail = smp_load_acquire(&rbuf->pwrite) - rbuf->pread; > if (avail < 0) > avail += rbuf->size; > return avail; > @@ -86,14 +86,15 @@ ssize_t dvb_ringbuffer_avail(struct dvb_ringbuffer *rbuf) > > void dvb_ringbuffer_flush(struct dvb_ringbuffer *rbuf) > { > - rbuf->pread = rbuf->pwrite; > + smp_store_release(&rbuf->pread, smp_load_acquire(&rbuf->pwrite)); > rbuf->error = 0; > } > EXPORT_SYMBOL(dvb_ringbuffer_flush); > > void dvb_ringbuffer_reset(struct dvb_ringbuffer *rbuf) > { > - rbuf->pread = rbuf->pwrite = 0; > + smp_store_release(&rbuf->pread, 0); > + smp_store_release(&rbuf->pwrite, 0); > rbuf->error = 0; > } > > @@ -119,12 +120,12 @@ ssize_t dvb_ringbuffer_read_user(struct dvb_ringbuffer *rbuf, u8 __user *buf, si > return -EFAULT; > buf += split; > todo -= split; > - rbuf->pread = 0; > + smp_store_release(&rbuf->pread, 0); > } > if (copy_to_user(buf, rbuf->data+rbuf->pread, todo)) > return -EFAULT; > > - rbuf->pread = (rbuf->pread + todo) % rbuf->size; > + smp_store_release(&rbuf->pread, (rbuf->pread + todo) % rbuf->size); > > return len; > } > @@ -139,11 +140,11 @@ void dvb_ringbuffer_read(struct dvb_ringbuffer *rbuf, u8 *buf, size_t len) > memcpy(buf, rbuf->data+rbuf->pread, split); > buf += split; > todo -= split; > - rbuf->pread = 0; > + smp_store_release(&rbuf->pread, 0); > } > memcpy(buf, rbuf->data+rbuf->pread, todo); > > - rbuf->pread = (rbuf->pread + todo) % rbuf->size; > + smp_store_release(&rbuf->pread, (rbuf->pread + todo) % rbuf->size); > } > > > @@ -158,10 +159,10 @@ ssize_t dvb_ringbuffer_write(struct dvb_ringbuffer *rbuf, const u8 *buf, size_t > memcpy(rbuf->data+rbuf->pwrite, buf, split); > buf += split; > todo -= split; > - rbuf->pwrite = 0; > + smp_store_release(&rbuf->pwrite, 0); > } > memcpy(rbuf->data+rbuf->pwrite, buf, todo); > - rbuf->pwrite = (rbuf->pwrite + todo) % rbuf->size; > + smp_store_release(&rbuf->pwrite, (rbuf->pwrite + todo) % rbuf->size); > > return len; > } > @@ -181,12 +182,12 @@ ssize_t dvb_ringbuffer_write_user(struct dvb_ringbuffer *rbuf, > return len - todo; > buf += split; > todo -= split; > - rbuf->pwrite = 0; > + smp_store_release(&rbuf->pwrite, 0); > } > status = copy_from_user(rbuf->data+rbuf->pwrite, buf, todo); > if (status) > return len - todo; > - rbuf->pwrite = (rbuf->pwrite + todo) % rbuf->size; > + smp_store_release(&rbuf->pwrite, (rbuf->pwrite + todo) % rbuf->size); > > return len; > } -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Soeren, Em Sun, 7 Feb 2016 20:22:36 +0100 Soeren Moch <smoch@web.de> escreveu: > On 27.12.2015 21:41, Soeren Moch wrote: > > Implement memory barriers according to Documentation/circular-buffers.txt: > > - use smp_store_release() to update ringbuffer read/write pointers > > - use smp_load_acquire() to load write pointer on reader side > > - use ACCESS_ONCE() to load read pointer on writer side > > > > This fixes data stream corruptions observed e.g. on an ARM Cortex-A9 > > quad core system with different types (PCI, USB) of DVB tuners. > > > > Signed-off-by: Soeren Moch <smoch@web.de> > > Cc: stable@vger.kernel.org # 3.14+ > > Mauro, > > any news or comments on this? > Since this is a real fix for broken behaviour, can you pick this up, please? The problem here is that I'm very reluctant to touch at the DVB core without doing some tests myself, as things like locking can be very sensible. I'll try to find some time to take a look on it for Kernel 4.8, but I'd like to reproduce the bug locally. Could you please provide me enough info to reproduce it (and eventually some test MPEG-TS where you know this would happen)? I have two DekTek RF generators here, so I should be able to play such TS and see what happens with and without the patch on x86, arm32 and arm64. Regards, Mauro > > Regards, > Soeren > > > --- > > Cc: Mauro Carvalho Chehab <mchehab@osg.samsung.com> > > Cc: linux-media@vger.kernel.org > > Cc: linux-kernel@vger.kernel.org > > > > Since smp_store_release() and smp_load_acquire() were introduced in linux-3.14, > > a 3.14+ stable tag was added. Is it desired to apply a similar patch to older > > stable kernels? > > --- > > drivers/media/dvb-core/dvb_ringbuffer.c | 27 ++++++++++++++------------- > > 1 file changed, 14 insertions(+), 13 deletions(-) > > > > diff --git a/drivers/media/dvb-core/dvb_ringbuffer.c b/drivers/media/dvb-core/dvb_ringbuffer.c > > index 1100e98..58b5968 100644 > > --- a/drivers/media/dvb-core/dvb_ringbuffer.c > > +++ b/drivers/media/dvb-core/dvb_ringbuffer.c > > @@ -55,7 +55,7 @@ void dvb_ringbuffer_init(struct dvb_ringbuffer *rbuf, void *data, size_t len) > > > > int dvb_ringbuffer_empty(struct dvb_ringbuffer *rbuf) > > { > > - return (rbuf->pread==rbuf->pwrite); > > + return (rbuf->pread == smp_load_acquire(&rbuf->pwrite)); > > } > > > > > > @@ -64,7 +64,7 @@ ssize_t dvb_ringbuffer_free(struct dvb_ringbuffer *rbuf) > > { > > ssize_t free; > > > > - free = rbuf->pread - rbuf->pwrite; > > + free = ACCESS_ONCE(rbuf->pread) - rbuf->pwrite; > > if (free <= 0) > > free += rbuf->size; > > return free-1; > > @@ -76,7 +76,7 @@ ssize_t dvb_ringbuffer_avail(struct dvb_ringbuffer *rbuf) > > { > > ssize_t avail; > > > > - avail = rbuf->pwrite - rbuf->pread; > > + avail = smp_load_acquire(&rbuf->pwrite) - rbuf->pread; > > if (avail < 0) > > avail += rbuf->size; > > return avail; > > @@ -86,14 +86,15 @@ ssize_t dvb_ringbuffer_avail(struct dvb_ringbuffer *rbuf) > > > > void dvb_ringbuffer_flush(struct dvb_ringbuffer *rbuf) > > { > > - rbuf->pread = rbuf->pwrite; > > + smp_store_release(&rbuf->pread, smp_load_acquire(&rbuf->pwrite)); > > rbuf->error = 0; > > } > > EXPORT_SYMBOL(dvb_ringbuffer_flush); > > > > void dvb_ringbuffer_reset(struct dvb_ringbuffer *rbuf) > > { > > - rbuf->pread = rbuf->pwrite = 0; > > + smp_store_release(&rbuf->pread, 0); > > + smp_store_release(&rbuf->pwrite, 0); > > rbuf->error = 0; > > } > > > > @@ -119,12 +120,12 @@ ssize_t dvb_ringbuffer_read_user(struct dvb_ringbuffer *rbuf, u8 __user *buf, si > > return -EFAULT; > > buf += split; > > todo -= split; > > - rbuf->pread = 0; > > + smp_store_release(&rbuf->pread, 0); > > } > > if (copy_to_user(buf, rbuf->data+rbuf->pread, todo)) > > return -EFAULT; > > > > - rbuf->pread = (rbuf->pread + todo) % rbuf->size; > > + smp_store_release(&rbuf->pread, (rbuf->pread + todo) % rbuf->size); > > > > return len; > > } > > @@ -139,11 +140,11 @@ void dvb_ringbuffer_read(struct dvb_ringbuffer *rbuf, u8 *buf, size_t len) > > memcpy(buf, rbuf->data+rbuf->pread, split); > > buf += split; > > todo -= split; > > - rbuf->pread = 0; > > + smp_store_release(&rbuf->pread, 0); > > } > > memcpy(buf, rbuf->data+rbuf->pread, todo); > > > > - rbuf->pread = (rbuf->pread + todo) % rbuf->size; > > + smp_store_release(&rbuf->pread, (rbuf->pread + todo) % rbuf->size); > > } > > > > > > @@ -158,10 +159,10 @@ ssize_t dvb_ringbuffer_write(struct dvb_ringbuffer *rbuf, const u8 *buf, size_t > > memcpy(rbuf->data+rbuf->pwrite, buf, split); > > buf += split; > > todo -= split; > > - rbuf->pwrite = 0; > > + smp_store_release(&rbuf->pwrite, 0); > > } > > memcpy(rbuf->data+rbuf->pwrite, buf, todo); > > - rbuf->pwrite = (rbuf->pwrite + todo) % rbuf->size; > > + smp_store_release(&rbuf->pwrite, (rbuf->pwrite + todo) % rbuf->size); > > > > return len; > > } > > @@ -181,12 +182,12 @@ ssize_t dvb_ringbuffer_write_user(struct dvb_ringbuffer *rbuf, > > return len - todo; > > buf += split; > > todo -= split; > > - rbuf->pwrite = 0; > > + smp_store_release(&rbuf->pwrite, 0); > > } > > status = copy_from_user(rbuf->data+rbuf->pwrite, buf, todo); > > if (status) > > return len - todo; > > - rbuf->pwrite = (rbuf->pwrite + todo) % rbuf->size; > > + smp_store_release(&rbuf->pwrite, (rbuf->pwrite + todo) % rbuf->size); > > > > return len; > > } > > > -- > To unsubscribe from this list: send the line "unsubscribe linux-media" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
Em Sat, 7 May 2016 10:22:35 -0300 Mauro Carvalho Chehab <mchehab@osg.samsung.com> escreveu: > Hi Soeren, > > Em Sun, 7 Feb 2016 20:22:36 +0100 > Soeren Moch <smoch@web.de> escreveu: > > > On 27.12.2015 21:41, Soeren Moch wrote: > > > Implement memory barriers according to Documentation/circular-buffers.txt: > > > - use smp_store_release() to update ringbuffer read/write pointers > > > - use smp_load_acquire() to load write pointer on reader side > > > - use ACCESS_ONCE() to load read pointer on writer side > > > > > > This fixes data stream corruptions observed e.g. on an ARM Cortex-A9 > > > quad core system with different types (PCI, USB) of DVB tuners. > > > > > > Signed-off-by: Soeren Moch <smoch@web.de> > > > Cc: stable@vger.kernel.org # 3.14+ > > > > Mauro, > > > > any news or comments on this? > > Since this is a real fix for broken behaviour, can you pick this up, please? > > The problem here is that I'm very reluctant to touch at the DVB core > without doing some tests myself, as things like locking can be > very sensible. In addition, it is good if other DVB developers could also test it. Even being sent for some time, until now, nobody else tested it. > > I'll try to find some time to take a look on it for Kernel 4.8, > but I'd like to reproduce the bug locally. > > Could you please provide me enough info to reproduce it (and > eventually some test MPEG-TS where you know this would happen)? > > I have two DekTek RF generators here, so I should be able to > play such TS and see what happens with and without the patch > on x86, arm32 and arm64. Ah, forgot to mention, but checkpatch.pl wants comments for the memory barriers: WARNING: memory barrier without comment #52: FILE: drivers/media/dvb-core/dvb_ringbuffer.c:58: + return (rbuf->pread == smp_load_acquire(&rbuf->pwrite)); WARNING: memory barrier without comment #70: FILE: drivers/media/dvb-core/dvb_ringbuffer.c:79: + avail = smp_load_acquire(&rbuf->pwrite) - rbuf->pread; WARNING: memory barrier without comment #79: FILE: drivers/media/dvb-core/dvb_ringbuffer.c:89: + smp_store_release(&rbuf->pread, smp_load_acquire(&rbuf->pwrite)); WARNING: memory barrier without comment #87: FILE: drivers/media/dvb-core/dvb_ringbuffer.c:96: + smp_store_release(&rbuf->pread, 0); WARNING: memory barrier without comment #88: FILE: drivers/media/dvb-core/dvb_ringbuffer.c:97: + smp_store_release(&rbuf->pwrite, 0); WARNING: memory barrier without comment #97: FILE: drivers/media/dvb-core/dvb_ringbuffer.c:123: + smp_store_release(&rbuf->pread, 0); WARNING: memory barrier without comment #103: FILE: drivers/media/dvb-core/dvb_ringbuffer.c:128: + smp_store_release(&rbuf->pread, (rbuf->pread + todo) % rbuf->size); WARNING: memory barrier without comment #112: FILE: drivers/media/dvb-core/dvb_ringbuffer.c:143: + smp_store_release(&rbuf->pread, 0); WARNING: memory barrier without comment #117: FILE: drivers/media/dvb-core/dvb_ringbuffer.c:147: + smp_store_release(&rbuf->pread, (rbuf->pread + todo) % rbuf->size); WARNING: memory barrier without comment #126: FILE: drivers/media/dvb-core/dvb_ringbuffer.c:162: + smp_store_release(&rbuf->pwrite, 0); WARNING: memory barrier without comment #130: FILE: drivers/media/dvb-core/dvb_ringbuffer.c:165: + smp_store_release(&rbuf->pwrite, (rbuf->pwrite + todo) % rbuf->size); WARNING: memory barrier without comment #139: FILE: drivers/media/dvb-core/dvb_ringbuffer.c:185: + smp_store_release(&rbuf->pwrite, 0); WARNING: memory barrier without comment #145: FILE: drivers/media/dvb-core/dvb_ringbuffer.c:190: + smp_store_release(&rbuf->pwrite, (rbuf->pwrite + todo) % rbuf->size); Thanks, Mauro -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Mauro, thanks for looking after this patch. On 05/07/16 15:22, Mauro Carvalho Chehab wrote: > Hi Soeren, > > Em Sun, 7 Feb 2016 20:22:36 +0100 > Soeren Moch <smoch@web.de> escreveu: > >> On 27.12.2015 21:41, Soeren Moch wrote: >>> Implement memory barriers according to Documentation/circular-buffers.txt: >>> - use smp_store_release() to update ringbuffer read/write pointers >>> - use smp_load_acquire() to load write pointer on reader side >>> - use ACCESS_ONCE() to load read pointer on writer side >>> >>> This fixes data stream corruptions observed e.g. on an ARM Cortex-A9 >>> quad core system with different types (PCI, USB) of DVB tuners. >>> >>> Signed-off-by: Soeren Moch <smoch@web.de> >>> Cc: stable@vger.kernel.org # 3.14+ >> >> Mauro, >> >> any news or comments on this? >> Since this is a real fix for broken behaviour, can you pick this up, please? > > The problem here is that I'm very reluctant to touch at the DVB core > without doing some tests myself, as things like locking can be > very sensible. I agree. But this patch adds memory barriers (no locks) according to Documentation/circular-buffers.txt. It should not be dangerous to follow these guidelines. Nevertheless, independent review and testing is always a good idea, especially in core code. > I'll try to find some time to take a look on it for Kernel 4.8, Thanks. > but I'd like to reproduce the bug locally. > > Could you please provide me enough info to reproduce it (and > eventually some test MPEG-TS where you know this would happen)? I used vdr with different types of DVB tuners on a TBS2910 board (quad core ARM Cortex-A9, see arch/arm/boot/dts/imx6q-tbs2910.dts). With more than one active cpu core I occasionally see data stream corruptions in recorded ts streams. This is not the case for one active cpu. The problem occurs when dvb_dmxdev_buffer_write() and dvb_dmxdev_buffer_read() are running simultaneously on different cpu cores on architectures without strong write ordering (e.g. on arm, not on x86). Here the write data pointer (written in dvb_ringbuffer_write() ) can become visible to the other cpu core (in dvb_ringbuffer_avail() ), before the actual ts packet data is visible. dvb_ringbuffer_read_user() then can read old ts data, although new ts data is already written into the ringbuffer by the other cpu core. With smp_store_release() and smp_load_acquire() the correct write ordering is maintained, ts packet data is visible on the reading cpu core before the updated write pointer. > I have two DekTek RF generators here, so I should be able to > play such TS and see what happens with and without the patch > on x86, arm32 and arm64. On x86 you should not see any difference, since smp_store_release() and smp_load_acquire() expand to simple stores and loads there. On multi-core arm systems you may see broken ts streams without patch, especially when reading the dvr device very fast, so that the dvb_ringbuffer is always close to empty. Regards, Soeren > Regards, > Mauro > >> >> Regards, >> Soeren >> >>> --- >>> Cc: Mauro Carvalho Chehab <mchehab@osg.samsung.com> >>> Cc: linux-media@vger.kernel.org >>> Cc: linux-kernel@vger.kernel.org >>> >>> Since smp_store_release() and smp_load_acquire() were introduced in linux-3.14, >>> a 3.14+ stable tag was added. Is it desired to apply a similar patch to older >>> stable kernels? >>> --- >>> drivers/media/dvb-core/dvb_ringbuffer.c | 27 ++++++++++++++------------- >>> 1 file changed, 14 insertions(+), 13 deletions(-) >>> >>> diff --git a/drivers/media/dvb-core/dvb_ringbuffer.c b/drivers/media/dvb-core/dvb_ringbuffer.c >>> index 1100e98..58b5968 100644 >>> --- a/drivers/media/dvb-core/dvb_ringbuffer.c >>> +++ b/drivers/media/dvb-core/dvb_ringbuffer.c >>> @@ -55,7 +55,7 @@ void dvb_ringbuffer_init(struct dvb_ringbuffer *rbuf, void *data, size_t len) >>> >>> int dvb_ringbuffer_empty(struct dvb_ringbuffer *rbuf) >>> { >>> - return (rbuf->pread==rbuf->pwrite); >>> + return (rbuf->pread == smp_load_acquire(&rbuf->pwrite)); >>> } >>> >>> >>> @@ -64,7 +64,7 @@ ssize_t dvb_ringbuffer_free(struct dvb_ringbuffer *rbuf) >>> { >>> ssize_t free; >>> >>> - free = rbuf->pread - rbuf->pwrite; >>> + free = ACCESS_ONCE(rbuf->pread) - rbuf->pwrite; >>> if (free <= 0) >>> free += rbuf->size; >>> return free-1; >>> @@ -76,7 +76,7 @@ ssize_t dvb_ringbuffer_avail(struct dvb_ringbuffer *rbuf) >>> { >>> ssize_t avail; >>> >>> - avail = rbuf->pwrite - rbuf->pread; >>> + avail = smp_load_acquire(&rbuf->pwrite) - rbuf->pread; >>> if (avail < 0) >>> avail += rbuf->size; >>> return avail; >>> @@ -86,14 +86,15 @@ ssize_t dvb_ringbuffer_avail(struct dvb_ringbuffer *rbuf) >>> >>> void dvb_ringbuffer_flush(struct dvb_ringbuffer *rbuf) >>> { >>> - rbuf->pread = rbuf->pwrite; >>> + smp_store_release(&rbuf->pread, smp_load_acquire(&rbuf->pwrite)); >>> rbuf->error = 0; >>> } >>> EXPORT_SYMBOL(dvb_ringbuffer_flush); >>> >>> void dvb_ringbuffer_reset(struct dvb_ringbuffer *rbuf) >>> { >>> - rbuf->pread = rbuf->pwrite = 0; >>> + smp_store_release(&rbuf->pread, 0); >>> + smp_store_release(&rbuf->pwrite, 0); >>> rbuf->error = 0; >>> } >>> >>> @@ -119,12 +120,12 @@ ssize_t dvb_ringbuffer_read_user(struct dvb_ringbuffer *rbuf, u8 __user *buf, si >>> return -EFAULT; >>> buf += split; >>> todo -= split; >>> - rbuf->pread = 0; >>> + smp_store_release(&rbuf->pread, 0); >>> } >>> if (copy_to_user(buf, rbuf->data+rbuf->pread, todo)) >>> return -EFAULT; >>> >>> - rbuf->pread = (rbuf->pread + todo) % rbuf->size; >>> + smp_store_release(&rbuf->pread, (rbuf->pread + todo) % rbuf->size); >>> >>> return len; >>> } >>> @@ -139,11 +140,11 @@ void dvb_ringbuffer_read(struct dvb_ringbuffer *rbuf, u8 *buf, size_t len) >>> memcpy(buf, rbuf->data+rbuf->pread, split); >>> buf += split; >>> todo -= split; >>> - rbuf->pread = 0; >>> + smp_store_release(&rbuf->pread, 0); >>> } >>> memcpy(buf, rbuf->data+rbuf->pread, todo); >>> >>> - rbuf->pread = (rbuf->pread + todo) % rbuf->size; >>> + smp_store_release(&rbuf->pread, (rbuf->pread + todo) % rbuf->size); >>> } >>> >>> >>> @@ -158,10 +159,10 @@ ssize_t dvb_ringbuffer_write(struct dvb_ringbuffer *rbuf, const u8 *buf, size_t >>> memcpy(rbuf->data+rbuf->pwrite, buf, split); >>> buf += split; >>> todo -= split; >>> - rbuf->pwrite = 0; >>> + smp_store_release(&rbuf->pwrite, 0); >>> } >>> memcpy(rbuf->data+rbuf->pwrite, buf, todo); >>> - rbuf->pwrite = (rbuf->pwrite + todo) % rbuf->size; >>> + smp_store_release(&rbuf->pwrite, (rbuf->pwrite + todo) % rbuf->size); >>> >>> return len; >>> } >>> @@ -181,12 +182,12 @@ ssize_t dvb_ringbuffer_write_user(struct dvb_ringbuffer *rbuf, >>> return len - todo; >>> buf += split; >>> todo -= split; >>> - rbuf->pwrite = 0; >>> + smp_store_release(&rbuf->pwrite, 0); >>> } >>> status = copy_from_user(rbuf->data+rbuf->pwrite, buf, todo); >>> if (status) >>> return len - todo; >>> - rbuf->pwrite = (rbuf->pwrite + todo) % rbuf->size; >>> + smp_store_release(&rbuf->pwrite, (rbuf->pwrite + todo) % rbuf->size); >>> >>> return len; >>> } >> >> >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-media" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html > > -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 05/07/16 15:26, Mauro Carvalho Chehab wrote: > Em Sat, 7 May 2016 10:22:35 -0300 > Mauro Carvalho Chehab <mchehab@osg.samsung.com> escreveu: > >> Hi Soeren, >> >> Em Sun, 7 Feb 2016 20:22:36 +0100 >> Soeren Moch <smoch@web.de> escreveu: >> >>> On 27.12.2015 21:41, Soeren Moch wrote: >>>> Implement memory barriers according to Documentation/circular-buffers.txt: >>>> - use smp_store_release() to update ringbuffer read/write pointers >>>> - use smp_load_acquire() to load write pointer on reader side >>>> - use ACCESS_ONCE() to load read pointer on writer side >>>> >>>> This fixes data stream corruptions observed e.g. on an ARM Cortex-A9 >>>> quad core system with different types (PCI, USB) of DVB tuners. >>>> >>>> Signed-off-by: Soeren Moch <smoch@web.de> >>>> Cc: stable@vger.kernel.org # 3.14+ >>> >>> Mauro, >>> >>> any news or comments on this? >>> Since this is a real fix for broken behaviour, can you pick this up, please? >> >> The problem here is that I'm very reluctant to touch at the DVB core >> without doing some tests myself, as things like locking can be >> very sensible. > > In addition, it is good if other DVB developers could also test it. > Even being sent for some time, until now, nobody else tested it. I know that people from the german vdrportal.de also use this patch for quite some time now. Unfortunately they are not active on the linux-media mailing list. >> >> I'll try to find some time to take a look on it for Kernel 4.8, >> but I'd like to reproduce the bug locally. >> >> Could you please provide me enough info to reproduce it (and >> eventually some test MPEG-TS where you know this would happen)? >> >> I have two DekTek RF generators here, so I should be able to >> play such TS and see what happens with and without the patch >> on x86, arm32 and arm64. > > Ah, forgot to mention, but checkpatch.pl wants comments for the memory > barriers: > OK, when I wrote this patch (linux-4.4-rc6) checkpatch.pl did not complain about missing comments. I will send a version 2 of this patch to address these warnings. Regards, Soeren > WARNING: memory barrier without comment > #52: FILE: drivers/media/dvb-core/dvb_ringbuffer.c:58: > + return (rbuf->pread == smp_load_acquire(&rbuf->pwrite)); > > WARNING: memory barrier without comment > #70: FILE: drivers/media/dvb-core/dvb_ringbuffer.c:79: > + avail = smp_load_acquire(&rbuf->pwrite) - rbuf->pread; > > WARNING: memory barrier without comment > #79: FILE: drivers/media/dvb-core/dvb_ringbuffer.c:89: > + smp_store_release(&rbuf->pread, smp_load_acquire(&rbuf->pwrite)); > > WARNING: memory barrier without comment > #87: FILE: drivers/media/dvb-core/dvb_ringbuffer.c:96: > + smp_store_release(&rbuf->pread, 0); > > WARNING: memory barrier without comment > #88: FILE: drivers/media/dvb-core/dvb_ringbuffer.c:97: > + smp_store_release(&rbuf->pwrite, 0); > > WARNING: memory barrier without comment > #97: FILE: drivers/media/dvb-core/dvb_ringbuffer.c:123: > + smp_store_release(&rbuf->pread, 0); > > WARNING: memory barrier without comment > #103: FILE: drivers/media/dvb-core/dvb_ringbuffer.c:128: > + smp_store_release(&rbuf->pread, (rbuf->pread + todo) % rbuf->size); > > WARNING: memory barrier without comment > #112: FILE: drivers/media/dvb-core/dvb_ringbuffer.c:143: > + smp_store_release(&rbuf->pread, 0); > > WARNING: memory barrier without comment > #117: FILE: drivers/media/dvb-core/dvb_ringbuffer.c:147: > + smp_store_release(&rbuf->pread, (rbuf->pread + todo) % rbuf->size); > > WARNING: memory barrier without comment > #126: FILE: drivers/media/dvb-core/dvb_ringbuffer.c:162: > + smp_store_release(&rbuf->pwrite, 0); > > WARNING: memory barrier without comment > #130: FILE: drivers/media/dvb-core/dvb_ringbuffer.c:165: > + smp_store_release(&rbuf->pwrite, (rbuf->pwrite + todo) % rbuf->size); > > WARNING: memory barrier without comment > #139: FILE: drivers/media/dvb-core/dvb_ringbuffer.c:185: > + smp_store_release(&rbuf->pwrite, 0); > > WARNING: memory barrier without comment > #145: FILE: drivers/media/dvb-core/dvb_ringbuffer.c:190: > + smp_store_release(&rbuf->pwrite, (rbuf->pwrite + todo) % rbuf->size); > > Thanks, > Mauro > -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/media/dvb-core/dvb_ringbuffer.c b/drivers/media/dvb-core/dvb_ringbuffer.c index 1100e98..58b5968 100644 --- a/drivers/media/dvb-core/dvb_ringbuffer.c +++ b/drivers/media/dvb-core/dvb_ringbuffer.c @@ -55,7 +55,7 @@ void dvb_ringbuffer_init(struct dvb_ringbuffer *rbuf, void *data, size_t len) int dvb_ringbuffer_empty(struct dvb_ringbuffer *rbuf) { - return (rbuf->pread==rbuf->pwrite); + return (rbuf->pread == smp_load_acquire(&rbuf->pwrite)); } @@ -64,7 +64,7 @@ ssize_t dvb_ringbuffer_free(struct dvb_ringbuffer *rbuf) { ssize_t free; - free = rbuf->pread - rbuf->pwrite; + free = ACCESS_ONCE(rbuf->pread) - rbuf->pwrite; if (free <= 0) free += rbuf->size; return free-1; @@ -76,7 +76,7 @@ ssize_t dvb_ringbuffer_avail(struct dvb_ringbuffer *rbuf) { ssize_t avail; - avail = rbuf->pwrite - rbuf->pread; + avail = smp_load_acquire(&rbuf->pwrite) - rbuf->pread; if (avail < 0) avail += rbuf->size; return avail; @@ -86,14 +86,15 @@ ssize_t dvb_ringbuffer_avail(struct dvb_ringbuffer *rbuf) void dvb_ringbuffer_flush(struct dvb_ringbuffer *rbuf) { - rbuf->pread = rbuf->pwrite; + smp_store_release(&rbuf->pread, smp_load_acquire(&rbuf->pwrite)); rbuf->error = 0; } EXPORT_SYMBOL(dvb_ringbuffer_flush); void dvb_ringbuffer_reset(struct dvb_ringbuffer *rbuf) { - rbuf->pread = rbuf->pwrite = 0; + smp_store_release(&rbuf->pread, 0); + smp_store_release(&rbuf->pwrite, 0); rbuf->error = 0; } @@ -119,12 +120,12 @@ ssize_t dvb_ringbuffer_read_user(struct dvb_ringbuffer *rbuf, u8 __user *buf, si return -EFAULT; buf += split; todo -= split; - rbuf->pread = 0; + smp_store_release(&rbuf->pread, 0); } if (copy_to_user(buf, rbuf->data+rbuf->pread, todo)) return -EFAULT; - rbuf->pread = (rbuf->pread + todo) % rbuf->size; + smp_store_release(&rbuf->pread, (rbuf->pread + todo) % rbuf->size); return len; } @@ -139,11 +140,11 @@ void dvb_ringbuffer_read(struct dvb_ringbuffer *rbuf, u8 *buf, size_t len) memcpy(buf, rbuf->data+rbuf->pread, split); buf += split; todo -= split; - rbuf->pread = 0; + smp_store_release(&rbuf->pread, 0); } memcpy(buf, rbuf->data+rbuf->pread, todo); - rbuf->pread = (rbuf->pread + todo) % rbuf->size; + smp_store_release(&rbuf->pread, (rbuf->pread + todo) % rbuf->size); } @@ -158,10 +159,10 @@ ssize_t dvb_ringbuffer_write(struct dvb_ringbuffer *rbuf, const u8 *buf, size_t memcpy(rbuf->data+rbuf->pwrite, buf, split); buf += split; todo -= split; - rbuf->pwrite = 0; + smp_store_release(&rbuf->pwrite, 0); } memcpy(rbuf->data+rbuf->pwrite, buf, todo); - rbuf->pwrite = (rbuf->pwrite + todo) % rbuf->size; + smp_store_release(&rbuf->pwrite, (rbuf->pwrite + todo) % rbuf->size); return len; } @@ -181,12 +182,12 @@ ssize_t dvb_ringbuffer_write_user(struct dvb_ringbuffer *rbuf, return len - todo; buf += split; todo -= split; - rbuf->pwrite = 0; + smp_store_release(&rbuf->pwrite, 0); } status = copy_from_user(rbuf->data+rbuf->pwrite, buf, todo); if (status) return len - todo; - rbuf->pwrite = (rbuf->pwrite + todo) % rbuf->size; + smp_store_release(&rbuf->pwrite, (rbuf->pwrite + todo) % rbuf->size); return len; }