From: Andy Walls <awalls@md.metrocast.net>
To: Jean Delvare <khali@linux-fr.org>
Cc: linux-media@vger.kernel.org, Mike Isely <isely@isely.net>,
Kenney Phillisjr <kphillisjr@gmail.com>,
Hans Verkuil <hverkuil@xs4all.nl>
Subject: Re: [PATCH 04/17] cx25840: Make cx25840 i2c register read transactions atomic
Date: Tue, 20 Jul 2010 08:35:26 -0400 [thread overview]
Message-ID: <1279629326.2714.31.camel@morgan.silverblock.net> (raw)
In-Reply-To: <20100720084217.4ab937a7@hyperion.delvare>
On Tue, 2010-07-20 at 08:42 +0200, Jean Delvare wrote:
> Hi Andy,
>
> On Mon, 19 Jul 2010 21:11:46 -0400, Andy Walls wrote:
> > There was a small window between writing the cx25840 register
> > address over the i2c bus and reading the register contents back from the
> > cx25840 device that the i2c adapter lock was released. This change ensures the
> > adapter lock is not released until the register read is done.
> >
> > Signed-off-by: Andy Walls <awalls@md.metrocast.net>
>
> Good catch.
Thanks.
> Acked-by: Jean Delvare <khali@linux-fr.org>
>
> Note that cx25840_and_or() still has a (smaller and less dangerous)
> race window. If several calls to cx25840_and_or() happen in parallel on
> the same register, some of the changes may be lost. I don't know if
> this can be a problem in practice though. If it is, then additional
> locking around cx25840_and_or() would be needed.
Ah, thank you for pointing that out.
So, please bear with me while I think out loud:
1. There are many explicit cases of read-modify-write on a register in
the cx25840 module, this is not the only one.
2. The bridge driver historically has always serialized access to the
cx25840 module so races have never previously been an issue.
3. I have added a work handler in the cx23885 module that calls the
cx25840 module's interrupt handler. Calls by the work handler are
serialized with respect to themselves, but not with respect to
serialized calls in #2.
4. IIRC, registers written to by the cx25840 interrupt handler are never
written to by the other cx25840 module functions; and vice-versa. I
will have to audit the cx25840 module code to be sure.
5. There is always a race on r-m-w on *some* registers in the
0x800-0x9ff range with the audio microcontroller that is built into the
chip. The only way to provide locking for those is to halt the
microcontroller. I've just looked at Patch 12/17 again. The interrupt
handler only reads the CX23885_AUD_MC_INT_MASK_REG, which is used by the
audio micrcontroller. The interrupt handler does do a r-m-w on the
CX25840_AUD_INT_STAT_REG, but that register is not used by the
microcontroller.
So, I think I'm OK with just not dropping the i2c adapter lock in a
cx25840 register read transaction until it is complete.
Thanks for making me think that one through. :)
Regards,
Andy
next prev parent reply other threads:[~2010-07-20 12:36 UTC|newest]
Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <cover.1279586511.git.awalls@md.metrocast.net>
2010-07-20 1:08 ` [PATCH 01/17] cx23885: Return -ENXIO on slave nack Andy Walls
2010-07-20 1:09 ` [PATCH 02/17] cx23885: Check for slave nack on all transactions Andy Walls
2010-07-20 1:10 ` [PATCH 03/17] cx23885: i2c_wait_done returns 0 or 1, don't check for < 0 return value Andy Walls
2010-07-20 1:11 ` [PATCH 04/17] cx25840: Make cx25840 i2c register read transactions atomic Andy Walls
2010-07-20 6:42 ` Jean Delvare
2010-07-20 12:35 ` Andy Walls [this message]
2010-07-20 12:46 ` Jean Delvare
2010-07-20 1:12 ` [PATCH 05/17] cx23885: Add correct detection of the HVR-1250 model 79501 Andy Walls
2010-07-20 1:12 ` [PATCH 06/17] cx23885: Add a VIDIOC_LOG_STATUS ioctl function for analog video devices Andy Walls
2010-07-20 1:16 ` [PATCH 07/17] v4l2_subdev: Add s_io_pin_config to v4l2_subdev_core_ops Andy Walls
2010-07-20 1:18 ` [PATCH 08/17] cx25840: Add s_io_pin_config core subdev ops for the CX2388[578] Andy Walls
2010-07-20 1:18 ` [PATCH 09/17] v4l2_subdev, cx23885: Differentiate IR carrier sense and I/O pin inversion Andy Walls
2010-07-20 1:19 ` [PATCH 10/17] cx23885: For CX23888 IR, configure the IO pin mux IR pins explcitly Andy Walls
2010-07-20 1:20 ` [PATCH 11/17] v4l2_subdev: Move interrupt_service_routine ptr to v4l2_subdev_core_ops Andy Walls
2010-07-20 1:22 ` [PATCH 12/17] cx25840: Add support for CX2388[57] A/V core integrated IR controllers Andy Walls
2010-07-20 1:22 ` [PATCH 13/17] cx23885: Add a v4l2_subdev group id for the CX2388[578] integrated AV core Andy Walls
2010-07-20 1:23 ` [PATCH 14/17] cx23885: Add preliminary IR Rx support for the HVR-1250 and TeVii S470 Andy Walls
2010-07-20 1:25 ` [PATCH 15/17] cx23885: Protect PCI interrupt mask manipulations with a spinlock Andy Walls
2010-07-20 1:25 ` [PATCH 16/17] cx23885: Move AV Core irq handling to a work handler Andy Walls
2010-07-20 1:26 ` [PATCH 17/17] cx23885: Require user to explicitly enable CX2388[57] IR via module param Andy Walls
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1279629326.2714.31.camel@morgan.silverblock.net \
--to=awalls@md.metrocast.net \
--cc=hverkuil@xs4all.nl \
--cc=isely@isely.net \
--cc=khali@linux-fr.org \
--cc=kphillisjr@gmail.com \
--cc=linux-media@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox