Linux kernel mirror (for testing) git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
kernel os linux

media: cec: keep track of outstanding transmits

I noticed that repeatedly running 'cec-ctl --playback' would occasionally
select 'Playback Device 2' instead of 'Playback Device 1', even though there
were no other Playback devices in the HDMI topology. This happened both with
'real' hardware and with the vivid CEC emulation, suggesting that this was an
issue in the core code that claims a logical address.

What 'cec-ctl --playback' does is to first clear all existing logical addresses,
and immediately after that configure the new desired device type.

The core code will poll the logical addresses trying to find a free address.
When found it will issue a few standard messages as per the CEC spec and return.
Those messages are queued up and will be transmitted asynchronously.

What happens is that if you run two 'cec-ctl --playback' commands in quick
succession, there is still a message of the first cec-ctl command being transmitted
when you reconfigure the adapter again in the second cec-ctl command.

When the logical addresses are cleared, then all information about outstanding
transmits inside the CEC core is also cleared, and the core is no longer aware
that there is still a transmit in flight.

When the hardware finishes the transmit it calls transmit_done and the CEC core
thinks it is actually in response of a POLL messages that is trying to find a
free logical address. The result of all this is that the core thinks that the
logical address for Playback Device 1 is in use, when it is really an earlier
transmit that ended.

The main transmit thread looks at adap->transmitting to check if a transmit
is in progress, but that is set to NULL when the adapter is unconfigured.
adap->transmitting represents the view of userspace, not that of the hardware.
So when unconfiguring the adapter the message is marked aborted from the point
of view of userspace, but seen from the PoV of the hardware it is still ongoing.

So introduce a new bool transmit_in_progress that represents the hardware state
and use that instead of adap->transmitting. Now the CEC core waits until the
hardware finishes the transmit before starting a new transmit.

Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
Cc: <stable@vger.kernel.org> # for v4.18 and up
Signed-off-by: Mauro Carvalho Chehab <mchehab+samsung@kernel.org>

authored by

Hans Verkuil and committed by
Mauro Carvalho Chehab
32804fcb db07c5ca

+19 -9
+18 -9
drivers/media/cec/cec-adap.c
··· 455 455 (adap->needs_hpd && 456 456 (!adap->is_configured && !adap->is_configuring)) || 457 457 kthread_should_stop() || 458 - (!adap->transmitting && 458 + (!adap->transmit_in_progress && 459 459 !list_empty(&adap->transmit_queue)), 460 460 msecs_to_jiffies(CEC_XFER_TIMEOUT_MS)); 461 461 timeout = err == 0; ··· 463 463 /* Otherwise we just wait for something to happen. */ 464 464 wait_event_interruptible(adap->kthread_waitq, 465 465 kthread_should_stop() || 466 - (!adap->transmitting && 466 + (!adap->transmit_in_progress && 467 467 !list_empty(&adap->transmit_queue))); 468 468 } 469 469 ··· 488 488 pr_warn("cec-%s: message %*ph timed out\n", adap->name, 489 489 adap->transmitting->msg.len, 490 490 adap->transmitting->msg.msg); 491 + adap->transmit_in_progress = false; 491 492 adap->tx_timeouts++; 492 493 /* Just give up on this. */ 493 494 cec_data_cancel(adap->transmitting, ··· 500 499 * If we are still transmitting, or there is nothing new to 501 500 * transmit, then just continue waiting. 502 501 */ 503 - if (adap->transmitting || list_empty(&adap->transmit_queue)) 502 + if (adap->transmit_in_progress || list_empty(&adap->transmit_queue)) 504 503 goto unlock; 505 504 506 505 /* Get a new message to transmit */ ··· 546 545 if (adap->ops->adap_transmit(adap, data->attempts, 547 546 signal_free_time, &data->msg)) 548 547 cec_data_cancel(data, CEC_TX_STATUS_ABORTED); 548 + else 549 + adap->transmit_in_progress = true; 549 550 550 551 unlock: 551 552 mutex_unlock(&adap->lock); ··· 578 575 data = adap->transmitting; 579 576 if (!data) { 580 577 /* 581 - * This can happen if a transmit was issued and the cable is 578 + * This might happen if a transmit was issued and the cable is 582 579 * unplugged while the transmit is ongoing. Ignore this 583 580 * transmit in that case. 584 581 */ 585 - dprintk(1, "%s was called without an ongoing transmit!\n", 586 - __func__); 587 - goto unlock; 582 + if (!adap->transmit_in_progress) 583 + dprintk(1, "%s was called without an ongoing transmit!\n", 584 + __func__); 585 + adap->transmit_in_progress = false; 586 + goto wake_thread; 588 587 } 588 + adap->transmit_in_progress = false; 589 589 590 590 msg = &data->msg; 591 591 ··· 654 648 * for transmitting or to retry the current message. 655 649 */ 656 650 wake_up_interruptible(&adap->kthread_waitq); 657 - unlock: 658 651 mutex_unlock(&adap->lock); 659 652 } 660 653 EXPORT_SYMBOL_GPL(cec_transmit_done_ts); ··· 1508 1503 if (adap->monitor_all_cnt) 1509 1504 WARN_ON(call_op(adap, adap_monitor_all_enable, false)); 1510 1505 mutex_lock(&adap->devnode.lock); 1511 - if (adap->needs_hpd || list_empty(&adap->devnode.fhs)) 1506 + if (adap->needs_hpd || list_empty(&adap->devnode.fhs)) { 1512 1507 WARN_ON(adap->ops->adap_enable(adap, false)); 1508 + adap->transmit_in_progress = false; 1509 + wake_up_interruptible(&adap->kthread_waitq); 1510 + } 1513 1511 mutex_unlock(&adap->devnode.lock); 1514 1512 if (phys_addr == CEC_PHYS_ADDR_INVALID) 1515 1513 return; ··· 1520 1512 1521 1513 mutex_lock(&adap->devnode.lock); 1522 1514 adap->last_initiator = 0xff; 1515 + adap->transmit_in_progress = false; 1523 1516 1524 1517 if ((adap->needs_hpd || list_empty(&adap->devnode.fhs)) && 1525 1518 adap->ops->adap_enable(adap, true)) {
+1
include/media/cec.h
··· 155 155 unsigned int transmit_queue_sz; 156 156 struct list_head wait_queue; 157 157 struct cec_data *transmitting; 158 + bool transmit_in_progress; 158 159 159 160 struct task_struct *kthread_config; 160 161 struct completion config_completion;