media: cec: keep track of outstanding transmits

BugLink: https://bugs.launchpad.net/bugs/1837257
commit 32804fcb612bf867034a093f459415e485cf044b upstream.

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>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Kamal Mostafa <kamal@canonical.com>
Signed-off-by: Khalid Elmously <khalid.elmously@canonical.com>

diff --git a/drivers/media/cec/cec-adap.c b/drivers/media/cec/cec-adap.c
index 98f88c43f62cb14b6ea6e06f34d3f826cdfe2782..de73ab3b9a14cd72a8f95f9860ce9269652b8377 100644 (file)
--- a/drivers/media/cec/cec-adap.c
+++ b/drivers/media/cec/cec-adap.c
@@ -431,7 +431,7 @@ int cec_thread_func(void *_adap)
                                (adap->needs_hpd &&
                                 (!adap->is_configured && !adap->is_configuring)) ||
                                kthread_should_stop() ||
-                               (!adap->transmitting &&
+                               (!adap->transmit_in_progress &&
                                 !list_empty(&adap->transmit_queue)),
                                msecs_to_jiffies(CEC_XFER_TIMEOUT_MS));
                        timeout = err == 0;
@@ -439,7 +439,7 @@ int cec_thread_func(void *_adap)
                        /* Otherwise we just wait for something to happen. */
                        wait_event_interruptible(adap->kthread_waitq,
                                kthread_should_stop() ||
-                               (!adap->transmitting &&
+                               (!adap->transmit_in_progress &&
                                 !list_empty(&adap->transmit_queue)));
                }
 
@@ -464,6 +464,7 @@ int cec_thread_func(void *_adap)
                        dprintk(1, "%s: message %*ph timed out\n", __func__,
                                adap->transmitting->msg.len,
                                adap->transmitting->msg.msg);
+                       adap->transmit_in_progress = false;
                        adap->tx_timeouts++;
                        /* Just give up on this. */
                        cec_data_cancel(adap->transmitting);
@@ -474,7 +475,7 @@ int cec_thread_func(void *_adap)
                 * If we are still transmitting, or there is nothing new to
                 * transmit, then just continue waiting.
                 */
-               if (adap->transmitting || list_empty(&adap->transmit_queue))
+               if (adap->transmit_in_progress || list_empty(&adap->transmit_queue))
                        goto unlock;
 
                /* Get a new message to transmit */
@@ -518,6 +519,8 @@ int cec_thread_func(void *_adap)
                if (adap->ops->adap_transmit(adap, data->attempts,
                                             signal_free_time, &data->msg))
                        cec_data_cancel(data);
+               else
+                       adap->transmit_in_progress = true;
 
 unlock:
                mutex_unlock(&adap->lock);
@@ -548,14 +551,17 @@ void cec_transmit_done_ts(struct cec_adapter *adap, u8 status,
        data = adap->transmitting;
        if (!data) {
                /*
-                * This can happen if a transmit was issued and the cable is
+                * This might happen if a transmit was issued and the cable is
                 * unplugged while the transmit is ongoing. Ignore this
                 * transmit in that case.
                 */
-               dprintk(1, "%s was called without an ongoing transmit!\n",
-                       __func__);
-               goto unlock;
+               if (!adap->transmit_in_progress)
+                       dprintk(1, "%s was called without an ongoing transmit!\n",
+                               __func__);
+               adap->transmit_in_progress = false;
+               goto wake_thread;
        }
+       adap->transmit_in_progress = false;
 
        msg = &data->msg;
 
@@ -621,7 +627,6 @@ wake_thread:
         * for transmitting or to retry the current message.
         */
        wake_up_interruptible(&adap->kthread_waitq);
-unlock:
        mutex_unlock(&adap->lock);
 }
 EXPORT_SYMBOL_GPL(cec_transmit_done_ts);
@@ -1483,14 +1488,18 @@ void __cec_s_phys_addr(struct cec_adapter *adap, u16 phys_addr, bool block)
                if (adap->monitor_all_cnt)
                        WARN_ON(call_op(adap, adap_monitor_all_enable, false));
                mutex_lock(&adap->devnode.lock);
-               if (adap->needs_hpd || list_empty(&adap->devnode.fhs))
+               if (adap->needs_hpd || list_empty(&adap->devnode.fhs)) {
                        WARN_ON(adap->ops->adap_enable(adap, false));
+                       adap->transmit_in_progress = false;
+                       wake_up_interruptible(&adap->kthread_waitq);
+               }
                mutex_unlock(&adap->devnode.lock);
                if (phys_addr == CEC_PHYS_ADDR_INVALID)
                        return;
        }
 
        mutex_lock(&adap->devnode.lock);
+       adap->transmit_in_progress = false;
        if ((adap->needs_hpd || list_empty(&adap->devnode.fhs)) &&
            adap->ops->adap_enable(adap, true)) {
                mutex_unlock(&adap->devnode.lock);

diff --git a/include/media/cec.h b/include/media/cec.h
index a60fed96b2d61a252e5d1b2fa80f26d071a75dcd..dac9f36549d85ceb6a470e0fcd3687b67d298b65 100644 (file)
--- a/include/media/cec.h
+++ b/include/media/cec.h
@@ -163,6 +163,7 @@ struct cec_adapter {
        unsigned int transmit_queue_sz;
        struct list_head wait_queue;
        struct cec_data *transmitting;
+       bool transmit_in_progress;
 
        struct task_struct *kthread_config;
        struct completion config_completion;