[PATCH v2 5/7] drm/i2c: tda9950: add CEC driver

[Russell King - ARM Linux]

On Wed, Dec 06, 2017 at 03:11:43PM +0100, Hans Verkuil wrote:
> Hi Russell,
> 
> Some small comments below:
> 
> On 12/06/17 13:35, Russell King wrote:
> > +	/*
> > +	 * This should never happen: the data sheet says that there will
> > +	 * always be a valid message if the interrupt line is asserted.
> > +	 */
> > +	if (buf[0] == 0) {
> 
> Checking for <= 2 is safer. See also my comment below.
> 
> > +		dev_warn(&priv->client->dev, "interrupt pending, but no message?\n");
> > +		return IRQ_NONE;
> > +	}
> > +
> > +	switch (buf[1]) {
> > +	case CDR1_CNF: /* transmit result */
> > +		arb_lost_cnt = nack_cnt = err_cnt = 0;
> > +		switch (buf[2]) {
> > +		case CDR2_CNF_SUCCESS:
> > +			tx_status = CEC_TX_STATUS_OK;
> > +			break;
> > +
> > +		case CDR2_CNF_ARB_ERROR:
> > +			tx_status = CEC_TX_STATUS_ARB_LOST;
> > +			arb_lost_cnt = cconr;
> > +			break;
> > +
> > +		case CDR2_CNF_NACK_ADDR:
> > +			tx_status = CEC_TX_STATUS_NACK;
> > +			nack_cnt = cconr;
> > +			break;
> > +
> > +		default: /* some other error, refer to TDA9950 docs */
> > +			dev_err(&priv->client->dev, "CNF reply error 0x%02x\n",
> > +				buf[2]);
> > +			tx_status = CEC_TX_STATUS_ERROR;
> > +			err_cnt = cconr;
> > +			break;
> > +		}
> > +		/* TDA9950 executes all retries for us */
> > +		tx_status |= CEC_TX_STATUS_MAX_RETRIES;
> > +		cec_transmit_done(priv->adap, tx_status, arb_lost_cnt,
> > +				  nack_cnt, 0, err_cnt);
> > +		break;
> > +
> > +	case CDR1_IND:
> > +		priv->rx_msg.len = buf[0] - 2;
> 
> Does the datasheet give any guarantees that buf[0] is always between 3 and 18?

"When reading, values are read starting at the register currently
addressed by the Address Pointer Register. The pointer auto-increments
after each read. If the host should read past register 19h, or read more
bytes than indicated by the FrameByteCount in register CDR[0] (address
07h), the value FFh will be returned.

...

Notes on reading the CEC Data Registers
• The CEC Data Registers only contain a valid message when the INT line
  is set and the INT bit in the TDA9950 Status Register is set.
• If CEC Data Registers are read when the INT line is not set, the first
  CEC Data Register will contain 0, indicating that there are no bytes
  to read. Any further reads before a STOP condition will return the
  value FFh.

...

The maximum length of a frame is 19 bytes."

> Note: it is possible for devices to send more than 16 bytes in a CEC message.
> Not allowed, mind you, but it can be done and I suspect some do in certain
> situations. So if the hardware just keeps counting you can get a rx_msg.len > 16
> and then the memcpy below would overwrite memory. You want to clamp the length
> to max 16.

Clamping to 16 is probably a good idea in any case, and actually ends up
catching the case where buf[0] < 2 - since priv->rx_msg.len is unsigned,
it'll become a very large positive number in that case.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up