From: Hans Verkuil hans.verkuil@cisco.com
While stress testing my CEC Framework v16 patch series found here:
http://www.spinics.net/lists/linux-input/msg44422.html
I discovered a few issues when dealing with HDMI disconnects.
The adv7511 patch fixes a potential race condition (never seen it go wrong, but I feel much safer with the new code). The WARN_ON patch removes a WARN_ON I thought could never happen when in fact it can in a disconnect scenario, and the final patch fixes a nasty kernel oops because the delayed work timer was never cleaned up while the underlying data structure was freed.
I decided not to post a v17 patch series to avoid spamming everyone, instead I'll merge these fixes in my cec pull request for Mauro and update that pull request.
Regards,
Hans
Hans Verkuil (3): adv7511: always update CEC irq mask cec: remove WARN_ON cec: correctly cancel delayed work when the CEC adapter is disabled
drivers/media/i2c/adv7511.c | 4 ++-- drivers/staging/media/cec/cec.c | 29 ++++++++++++++++++++++++----- 2 files changed, 26 insertions(+), 7 deletions(-)
From: Hans Verkuil hans.verkuil@cisco.com
Instead of doing:
if (state->cec_enabled_adap) adv7511_wr_and_or(sd, 0x95, 0xc0, enable ? 0x39 : 0x00);
do:
adv7511_wr_and_or(sd, 0x95, 0xc0, (state->cec_enabled_adap && enable) ? 0x39 : 0x00);
This ensures that the cec irq mask is always updated correctly according to both conditions.
Signed-off-by: Hans Verkuil hans.verkuil@cisco.com --- drivers/media/i2c/adv7511.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/media/i2c/adv7511.c b/drivers/media/i2c/adv7511.c index 002117b..ddcde2d 100644 --- a/drivers/media/i2c/adv7511.c +++ b/drivers/media/i2c/adv7511.c @@ -937,8 +937,8 @@ static void adv7511_set_isr(struct v4l2_subdev *sd, bool enable) else if (adv7511_have_hotplug(sd)) irqs |= MASK_ADV7511_EDID_RDY_INT;
- if (state->cec_enabled_adap) - adv7511_wr_and_or(sd, 0x95, 0xc0, enable ? 0x39 : 0x00); + adv7511_wr_and_or(sd, 0x95, 0xc0, + (state->cec_enabled_adap && enable) ? 0x39 : 0x00);
/* * This i2c write can fail (approx. 1 in 1000 writes). But it
From: Hans Verkuil hans.verkuil@cisco.com
If a transmit is issued and before cec_transmit_done() is called the HDMI cable is unplugged, then it is possible that adap->transmitting == NULL.
So drop the WARN_ON, explain why it can happen and just ignore the tranmit.
Signed-off-by: Hans Verkuil hans.verkuil@cisco.com --- drivers/staging/media/cec/cec.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/drivers/staging/media/cec/cec.c b/drivers/staging/media/cec/cec.c index 3c5f084..9a62aa2 100644 --- a/drivers/staging/media/cec/cec.c +++ b/drivers/staging/media/cec/cec.c @@ -485,9 +485,13 @@ void cec_transmit_done(struct cec_adapter *adap, u8 status, u8 arb_lost_cnt, dprintk(2, "cec_transmit_done %02x\n", status); mutex_lock(&adap->lock); data = adap->transmitting; - if (WARN_ON(data == NULL)) { - /* This is weird and should not happen. Ignore this transmit */ - dprintk(0, "cec_transmit_done without an ongoing transmit!\n"); + if (data == NULL) { + /* + * This can happen if a transmit was issued and the cable is + * unplugged while the transmit is ongoing. Ignore this + * transmit in that case. + */ + dprintk(1, "cec_transmit_done without an ongoing transmit!\n"); goto unlock; }
From: Hans Verkuil hans.verkuil@cisco.com
When cleaning up pending work from the wait_queue list, make sure to cancel the delayed work. Otherwise nasty kernel oopses will occur when the timer goes off and the cec_data struct has disappeared.
Signed-off-by: Hans Verkuil hans.verkuil@cisco.com --- drivers/staging/media/cec/cec.c | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-)
diff --git a/drivers/staging/media/cec/cec.c b/drivers/staging/media/cec/cec.c index 9a62aa2..c2a876e 100644 --- a/drivers/staging/media/cec/cec.c +++ b/drivers/staging/media/cec/cec.c @@ -393,13 +393,28 @@ static int cec_thread_func(void *_adap) struct cec_data, list); cec_data_cancel(data); } + if (adap->transmitting) + cec_data_cancel(adap->transmitting); + + /* + * Cancel the pending timeout work. We have to unlock + * the mutex when flushing the work since + * cec_wait_timeout() will take it. This is OK since + * no new entries can be added to wait_queue as long + * as adap->transmitting is NULL, which it is due to + * the cec_data_cancel() above. + */ while (!list_empty(&adap->wait_queue)) { data = list_first_entry(&adap->wait_queue, struct cec_data, list); + + if (!cancel_delayed_work(&data->work)) { + mutex_unlock(&adap->lock); + flush_scheduled_work(); + mutex_lock(&adap->lock); + } cec_data_cancel(data); } - if (adap->transmitting) - cec_data_cancel(adap->transmitting); goto unlock; }
dri-devel@lists.freedesktop.org
-
Hans Verkuil