From: Hans Verkuil hans.verkuil@cisco.com
This patch series replaces patches 1-3 of:
https://www.spinics.net/lists/linux-media/msg141216.html
Patches 4 & 5 of that series remain as-is and are omap4 bug fixes.
This patch series can be applied to the media subsystem since it has no drm changes.
Changes since the previous patch series are:
- Added a new patch improving the documentation of the cec_transmit_done function to hopefully avoid the omap4 bug in the future.
- Add a new patch that fixes a bug in the Signal Free Time calculation of the CEC framework. This affects hardware that relies on the framework for this value. This bug could basically cause a 'denial of service' situation on the CEC bus. Found with the cec-compliance adapter test, which does really nasty things :-)
- Improve cec-gpio to fixup the Signal Free Time if a transmit is delayed because a new message was received first (thus requiring a lower SFT).
All patches are meant for 4.20, and patches 2-5 will get a CC to stable for 4.18 and up.
The adv patches could go back to older kernels, but it's not very important.
I'll post a pull request for this series soon.
Regards,
Hans
Hans Verkuil (6): cec-core.rst: improve cec_transmit_done documentation cec: add new tx/rx status bits to detect aborts/timeouts adv7604: when the EDID is cleared, unconfigure CEC as well adv7842: when the EDID is cleared, unconfigure CEC as well cec: fix the Signal Free Time calculation cec-gpio: select correct Signal Free Time
Documentation/media/kapi/cec-core.rst | 4 + .../media/uapi/cec/cec-ioc-receive.rst | 25 ++++- drivers/media/cec/cec-adap.c | 92 +++++-------------- drivers/media/cec/cec-pin.c | 20 ++++ drivers/media/i2c/adv7604.c | 4 +- drivers/media/i2c/adv7842.c | 4 +- include/media/cec.h | 2 +- include/uapi/linux/cec.h | 3 + 8 files changed, 82 insertions(+), 72 deletions(-)
From: Hans Verkuil hans.verkuil@cisco.com
Clarify that calling cec_transmit_done can start a new transmit and that you should put the hardware in a state that allows for a new transmit before calling this function.
Signed-off-by: Hans Verkuil hans.verkuil@cisco.com --- Documentation/media/kapi/cec-core.rst | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/Documentation/media/kapi/cec-core.rst b/Documentation/media/kapi/cec-core.rst index 1d989c544370..bca1d9d1d223 100644 --- a/Documentation/media/kapi/cec-core.rst +++ b/Documentation/media/kapi/cec-core.rst @@ -268,6 +268,10 @@ to 1, if the hardware does support retry then either set these counters to 0 if the hardware provides no feedback of which errors occurred and how many times, or fill in the correct values as reported by the hardware.
+Be aware that calling these functions can immediately start a new transmit +if there is one pending in the queue. So make sure that the hardware is in +a state where new transmits can be started *before* calling these functions. + The cec_transmit_attempt_done() function is a helper for cases where the hardware never retries, so the transmit is always for just a single attempt. It will call cec_transmit_done() in turn, filling in 1 for the
From: Hans Verkuil hans.verkuil@cisco.com
If the HDMI cable is disconnected or the CEC adapter is manually unconfigured, then all pending transmits and wait-for-replies are aborted. Signal this with new status bits (CEC_RX/TX_STATUS_ABORTED).
If due to (usually) a driver bug a transmit never ends (i.e. the transmit_done was never called by the driver), then when this times out the message is marked with CEC_TX_STATUS_TIMEOUT.
This should not happen and is an indication of a driver bug.
Without a separate status bit for this it was impossible to detect this from userspace.
The 'transmit timed out' kernel message is now a warning, so this should be more prominent in the kernel log as well.
Signed-off-by: Hans Verkuil hans.verkuil@cisco.com --- .../media/uapi/cec/cec-ioc-receive.rst | 25 ++++++- drivers/media/cec/cec-adap.c | 66 +++++-------------- include/uapi/linux/cec.h | 3 + 3 files changed, 44 insertions(+), 50 deletions(-)
diff --git a/Documentation/media/uapi/cec/cec-ioc-receive.rst b/Documentation/media/uapi/cec/cec-ioc-receive.rst index e964074cd15b..b25e48afaa08 100644 --- a/Documentation/media/uapi/cec/cec-ioc-receive.rst +++ b/Documentation/media/uapi/cec/cec-ioc-receive.rst @@ -16,10 +16,10 @@ CEC_RECEIVE, CEC_TRANSMIT - Receive or transmit a CEC message Synopsis ========
-.. c:function:: int ioctl( int fd, CEC_RECEIVE, struct cec_msg *argp ) +.. c:function:: int ioctl( int fd, CEC_RECEIVE, struct cec_msg *argp ) :name: CEC_RECEIVE
-.. c:function:: int ioctl( int fd, CEC_TRANSMIT, struct cec_msg *argp ) +.. c:function:: int ioctl( int fd, CEC_TRANSMIT, struct cec_msg *argp ) :name: CEC_TRANSMIT
Arguments @@ -272,6 +272,19 @@ View On' messages from initiator 0xf ('Unregistered') to destination 0 ('TV'). - The transmit failed after one or more retries. This status bit is mutually exclusive with :ref:`CEC_TX_STATUS_OK <CEC-TX-STATUS-OK>`. Other bits can still be set to explain which failures were seen. + * .. _`CEC-TX-STATUS-ABORTED`: + + - ``CEC_TX_STATUS_ABORTED`` + - 0x40 + - The transmit was aborted due to an HDMI disconnect, or the adapter + was unconfigured, or a transmit was interrupted, or the driver + returned an error when attempting to start a transmit. + * .. _`CEC-TX-STATUS-TIMEOUT`: + + - ``CEC_TX_STATUS_TIMEOUT`` + - 0x80 + - The transmit timed out. This should not normally happen and this + indicates a driver problem.
.. tabularcolumns:: |p{5.6cm}|p{0.9cm}|p{11.0cm}| @@ -300,6 +313,14 @@ View On' messages from initiator 0xf ('Unregistered') to destination 0 ('TV'). - The message was received successfully but the reply was ``CEC_MSG_FEATURE_ABORT``. This status is only set if this message was the reply to an earlier transmitted message. + * .. _`CEC-RX-STATUS-ABORTED`: + + - ``CEC_RX_STATUS_ABORTED`` + - 0x08 + - The wait for a reply to an earlier transmitted message was aborted + because the HDMI cable was disconnected, the adapter was unconfigured + or the :ref:`CEC_TRANSMIT <CEC_RECEIVE>` that waited for a + reply was interrupted.
diff --git a/drivers/media/cec/cec-adap.c b/drivers/media/cec/cec-adap.c index 829878356e1e..e6e82b504e56 100644 --- a/drivers/media/cec/cec-adap.c +++ b/drivers/media/cec/cec-adap.c @@ -354,7 +354,7 @@ static void cec_data_completed(struct cec_data *data) * * This function is called with adap->lock held. */ -static void cec_data_cancel(struct cec_data *data) +static void cec_data_cancel(struct cec_data *data, u8 tx_status) { /* * It's either the current transmit, or it is a pending @@ -369,13 +369,11 @@ static void cec_data_cancel(struct cec_data *data) }
if (data->msg.tx_status & CEC_TX_STATUS_OK) { - /* Mark the canceled RX as a timeout */ data->msg.rx_ts = ktime_get_ns(); - data->msg.rx_status = CEC_RX_STATUS_TIMEOUT; + data->msg.rx_status = CEC_RX_STATUS_ABORTED; } else { - /* Mark the canceled TX as an error */ data->msg.tx_ts = ktime_get_ns(); - data->msg.tx_status |= CEC_TX_STATUS_ERROR | + data->msg.tx_status |= tx_status | CEC_TX_STATUS_MAX_RETRIES; data->msg.tx_error_cnt++; data->attempts = 0; @@ -403,15 +401,15 @@ static void cec_flush(struct cec_adapter *adap) while (!list_empty(&adap->transmit_queue)) { data = list_first_entry(&adap->transmit_queue, struct cec_data, list); - cec_data_cancel(data); + cec_data_cancel(data, CEC_TX_STATUS_ABORTED); } if (adap->transmitting) - cec_data_cancel(adap->transmitting); + cec_data_cancel(adap->transmitting, CEC_TX_STATUS_ABORTED);
/* Cancel the pending timeout work. */ list_for_each_entry_safe(data, n, &adap->wait_queue, list) { if (cancel_delayed_work(&data->work)) - cec_data_cancel(data); + cec_data_cancel(data, CEC_TX_STATUS_OK); /* * If cancel_delayed_work returned false, then * the cec_wait_timeout function is running, @@ -487,12 +485,13 @@ int cec_thread_func(void *_adap) * so much traffic on the bus that the adapter was * unable to transmit for CEC_XFER_TIMEOUT_MS (2.1s). */ - dprintk(1, "%s: message %*ph timed out\n", __func__, + pr_warn("cec-%s: message %*ph timed out\n", adap->name, adap->transmitting->msg.len, adap->transmitting->msg.msg); adap->tx_timeouts++; /* Just give up on this. */ - cec_data_cancel(adap->transmitting); + cec_data_cancel(adap->transmitting, + CEC_TX_STATUS_TIMEOUT); goto unlock; }
@@ -543,7 +542,7 @@ int cec_thread_func(void *_adap) /* Tell the adapter to transmit, cancel on error */ if (adap->ops->adap_transmit(adap, data->attempts, signal_free_time, &data->msg)) - cec_data_cancel(data); + cec_data_cancel(data, CEC_TX_STATUS_ABORTED);
unlock: mutex_unlock(&adap->lock); @@ -715,8 +714,6 @@ int cec_transmit_msg_fh(struct cec_adapter *adap, struct cec_msg *msg, { struct cec_data *data; u8 last_initiator = 0xff; - unsigned int timeout; - int res = 0;
msg->rx_ts = 0; msg->tx_ts = 0; @@ -858,48 +855,21 @@ int cec_transmit_msg_fh(struct cec_adapter *adap, struct cec_msg *msg, if (!block) return 0;
- /* - * If we don't get a completion before this time something is really - * wrong and we time out. - */ - timeout = CEC_XFER_TIMEOUT_MS; - /* Add the requested timeout if we have to wait for a reply as well */ - if (msg->timeout) - timeout += msg->timeout; - /* * Release the lock and wait, retake the lock afterwards. */ mutex_unlock(&adap->lock); - res = wait_for_completion_killable_timeout(&data->c, - msecs_to_jiffies(timeout)); + wait_for_completion_killable(&data->c); mutex_lock(&adap->lock);
- if (data->completed) { - /* The transmit completed (possibly with an error) */ - *msg = data->msg; - kfree(data); - return 0; - } - /* - * The wait for completion timed out or was interrupted, so mark this - * as non-blocking and disconnect from the filehandle since it is - * still 'in flight'. When it finally completes it will just drop the - * result silently. - */ - data->blocking = false; - if (data->fh) - list_del(&data->xfer_list); - data->fh = NULL; + /* Cancel the transmit if it was interrupted */ + if (!data->completed) + cec_data_cancel(data, CEC_TX_STATUS_ABORTED);
- if (res == 0) { /* timed out */ - /* Check if the reply or the transmit failed */ - if (msg->timeout && (msg->tx_status & CEC_TX_STATUS_OK)) - msg->rx_status = CEC_RX_STATUS_TIMEOUT; - else - msg->tx_status = CEC_TX_STATUS_MAX_RETRIES; - } - return res > 0 ? 0 : res; + /* The transmit completed (possibly with an error) */ + *msg = data->msg; + kfree(data); + return 0; }
/* Helper function to be used by drivers and this framework. */ diff --git a/include/uapi/linux/cec.h b/include/uapi/linux/cec.h index 097fcd812471..3094af68b6e7 100644 --- a/include/uapi/linux/cec.h +++ b/include/uapi/linux/cec.h @@ -152,10 +152,13 @@ static inline void cec_msg_set_reply_to(struct cec_msg *msg, #define CEC_TX_STATUS_LOW_DRIVE (1 << 3) #define CEC_TX_STATUS_ERROR (1 << 4) #define CEC_TX_STATUS_MAX_RETRIES (1 << 5) +#define CEC_TX_STATUS_ABORTED (1 << 6) +#define CEC_TX_STATUS_TIMEOUT (1 << 7)
#define CEC_RX_STATUS_OK (1 << 0) #define CEC_RX_STATUS_TIMEOUT (1 << 1) #define CEC_RX_STATUS_FEATURE_ABORT (1 << 2) +#define CEC_RX_STATUS_ABORTED (1 << 3)
static inline int cec_msg_status_is_ok(const struct cec_msg *msg) {
From: Hans Verkuil hans.verkuil@cisco.com
When there is no EDID the CEC adapter should be unconfigured as well. So call cec_phys_addr_invalidate() when this happens.
Signed-off-by: Hans Verkuil hans.verkuil@cisco.com --- drivers/media/i2c/adv7604.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/media/i2c/adv7604.c b/drivers/media/i2c/adv7604.c index 1c89959b1509..9eb7c70a7712 100644 --- a/drivers/media/i2c/adv7604.c +++ b/drivers/media/i2c/adv7604.c @@ -2284,8 +2284,10 @@ static int adv76xx_set_edid(struct v4l2_subdev *sd, struct v4l2_edid *edid) state->aspect_ratio.numerator = 16; state->aspect_ratio.denominator = 9;
- if (!state->edid.present) + if (!state->edid.present) { state->edid.blocks = 0; + cec_phys_addr_invalidate(state->cec_adap); + }
v4l2_dbg(2, debug, sd, "%s: clear EDID pad %d, edid.present = 0x%x\n", __func__, edid->pad, state->edid.present);
From: Hans Verkuil hans.verkuil@cisco.com
When there is no EDID the CEC adapter should be unconfigured as well. So call cec_phys_addr_invalidate() when this happens.
Signed-off-by: Hans Verkuil hans.verkuil@cisco.com --- drivers/media/i2c/adv7842.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/media/i2c/adv7842.c b/drivers/media/i2c/adv7842.c index cd63cc6564e9..4721d49dcf0f 100644 --- a/drivers/media/i2c/adv7842.c +++ b/drivers/media/i2c/adv7842.c @@ -786,8 +786,10 @@ static int edid_write_hdmi_segment(struct v4l2_subdev *sd, u8 port) /* Disable I2C access to internal EDID ram from HDMI DDC ports */ rep_write_and_or(sd, 0x77, 0xf3, 0x00);
- if (!state->hdmi_edid.present) + if (!state->hdmi_edid.present) { + cec_phys_addr_invalidate(state->cec_adap); return 0; + }
pa = v4l2_get_edid_phys_addr(edid, 256, &spa_loc); err = v4l2_phys_addr_validate(pa, &pa, NULL);
From: Hans Verkuil hans.verkuil@cisco.com
The calculation of the Signal Free Time in the framework was not correct. If a message was received, then the next transmit should be considered a New Initiator and use a shorter SFT value.
This was not done with the result that if both sides where continually sending messages, they both could use the same SFT value and one side could deny the other side access to the bus.
Note that this fix does not take the corner case into account where a receive is in progress when you call adap_transmit.
Signed-off-by: Hans Verkuil hans.verkuil@cisco.com --- drivers/media/cec/cec-adap.c | 26 +++++++------------------- include/media/cec.h | 2 +- 2 files changed, 8 insertions(+), 20 deletions(-)
diff --git a/drivers/media/cec/cec-adap.c b/drivers/media/cec/cec-adap.c index e6e82b504e56..0c0d9107383e 100644 --- a/drivers/media/cec/cec-adap.c +++ b/drivers/media/cec/cec-adap.c @@ -526,9 +526,11 @@ int cec_thread_func(void *_adap) if (data->attempts) { /* should be >= 3 data bit periods for a retry */ signal_free_time = CEC_SIGNAL_FREE_TIME_RETRY; - } else if (data->new_initiator) { + } else if (adap->last_initiator != + cec_msg_initiator(&data->msg)) { /* should be >= 5 data bit periods for new initiator */ signal_free_time = CEC_SIGNAL_FREE_TIME_NEW_INITIATOR; + adap->last_initiator = cec_msg_initiator(&data->msg); } else { /* * should be >= 7 data bit periods for sending another @@ -713,7 +715,6 @@ int cec_transmit_msg_fh(struct cec_adapter *adap, struct cec_msg *msg, struct cec_fh *fh, bool block) { struct cec_data *data; - u8 last_initiator = 0xff;
msg->rx_ts = 0; msg->tx_ts = 0; @@ -823,23 +824,6 @@ int cec_transmit_msg_fh(struct cec_adapter *adap, struct cec_msg *msg, data->adap = adap; data->blocking = block;
- /* - * Determine if this message follows a message from the same - * initiator. Needed to determine the free signal time later on. - */ - if (msg->len > 1) { - if (!(list_empty(&adap->transmit_queue))) { - const struct cec_data *last; - - last = list_last_entry(&adap->transmit_queue, - const struct cec_data, list); - last_initiator = cec_msg_initiator(&last->msg); - } else if (adap->transmitting) { - last_initiator = - cec_msg_initiator(&adap->transmitting->msg); - } - } - data->new_initiator = last_initiator != cec_msg_initiator(msg); init_completion(&data->c); INIT_DELAYED_WORK(&data->work, cec_wait_timeout);
@@ -1027,6 +1011,8 @@ void cec_received_msg_ts(struct cec_adapter *adap, mutex_lock(&adap->lock); dprintk(2, "%s: %*ph\n", __func__, msg->len, msg->msg);
+ adap->last_initiator = 0xff; + /* Check if this message was for us (directed or broadcast). */ if (!cec_msg_is_broadcast(msg)) valid_la = cec_has_log_addr(adap, msg_dest); @@ -1489,6 +1475,8 @@ void __cec_s_phys_addr(struct cec_adapter *adap, u16 phys_addr, bool block) }
mutex_lock(&adap->devnode.lock); + adap->last_initiator = 0xff; + 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 9f382f0c2970..254a610b9aa5 100644 --- a/include/media/cec.h +++ b/include/media/cec.h @@ -63,7 +63,6 @@ struct cec_data { struct delayed_work work; struct completion c; u8 attempts; - bool new_initiator; bool blocking; bool completed; }; @@ -174,6 +173,7 @@ struct cec_adapter { bool is_configuring; bool is_configured; bool cec_pin_is_high; + u8 last_initiator; u32 monitor_all_cnt; u32 monitor_pin_cnt; u32 follower_cnt;
From: Hans Verkuil hans.verkuil@cisco.com
If a receive is in progress or starts before the transmit has a chance, then lower the Signal Free Time of the upcoming transmit to no more than CEC_SIGNAL_FREE_TIME_NEW_INITIATOR.
This is per the specification requirements.
Signed-off-by: Hans Verkuil hans.verkuil@cisco.com --- drivers/media/cec/cec-pin.c | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+)
diff --git a/drivers/media/cec/cec-pin.c b/drivers/media/cec/cec-pin.c index 6e311424f0dc..635db8e70ead 100644 --- a/drivers/media/cec/cec-pin.c +++ b/drivers/media/cec/cec-pin.c @@ -935,6 +935,17 @@ static enum hrtimer_restart cec_pin_timer(struct hrtimer *timer) /* Start bit, switch to receive state */ pin->ts = ts; pin->state = CEC_ST_RX_START_BIT_LOW; + /* + * If a transmit is pending, then that transmit should + * use a signal free time of no more than + * CEC_SIGNAL_FREE_TIME_NEW_INITIATOR since it will + * have a new initiator due to the receive that is now + * starting. + */ + if (pin->tx_msg.len && pin->tx_signal_free_time > + CEC_SIGNAL_FREE_TIME_NEW_INITIATOR) + pin->tx_signal_free_time = + CEC_SIGNAL_FREE_TIME_NEW_INITIATOR; break; } if (ktime_to_ns(pin->ts) == 0) @@ -1157,6 +1168,15 @@ static int cec_pin_adap_transmit(struct cec_adapter *adap, u8 attempts, { struct cec_pin *pin = adap->pin;
+ /* + * If a receive is in progress, then this transmit should use + * a signal free time of max CEC_SIGNAL_FREE_TIME_NEW_INITIATOR + * since when it starts transmitting it will have a new initiator. + */ + if (pin->state != CEC_ST_IDLE && + signal_free_time > CEC_SIGNAL_FREE_TIME_NEW_INITIATOR) + signal_free_time = CEC_SIGNAL_FREE_TIME_NEW_INITIATOR; + pin->tx_signal_free_time = signal_free_time; pin->tx_extra_bytes = 0; pin->tx_msg = *msg;
dri-devel@lists.freedesktop.org
-
Hans Verkuil