Re: [RFC] Bluetooth: Add new serdev based driver for 3-Wire attached controllers

[Marcel Holtmann <marcel@holtmann.org>]

Hi Jeremy,

>> +static int bt3wire_receive_buf(struct serdev_device *serdev, const u8 *buf,
>> +			      size_t count)
>> +{
>> +	struct bt3wire_dev *bdev = serdev_device_get_drvdata(serdev);
>> +	size_t i;
>> +
>> +	for (i = 0; i < count; i++) {
>> +		const unsigned char *ptr = buf + i;
>> +
>> +		switch (bdev->rx_slip_state) {
>> +		case SLIP_WAIT_DELIM:
>> +			/* The delimiter octet 0xC0 is placed at the start and
>> +			 * end of every packet. To synchronize with the packet
>> +			 * stream, wait for the delimiter octet and drop any
>> +			 * other octets.
>> +			 */
>> +			if (*ptr == SLIP_DELIM) {
>> +				bdev->rx_skb = bt_skb_alloc(MAX_PACKET_SIZE,
>> +							    GFP_ATOMIC);
>> +				if (bdev->rx_skb)
>> +					bdev->rx_slip_state = SLIP_PACKET;
>> +			}
>> +			break;
>> +		case SLIP_PACKET:
>> +			/* Receiving the delimiter octet 0xC0 indicates a
>> +			 * complete packet, the escape octet 0xDB indicates
>> +			 * the switch to an escape sequence, and all other
>> +			 * octets are copied into the result.
>> +			 */
>> +			if (*ptr == SLIP_DELIM) {
>> +				bt3wire_process_pkt(bdev, bdev->rx_skb);
>> +				bdev->rx_skb = NULL;
>> +				bdev->rx_slip_state = SLIP_WAIT_DELIM;
>> +			} else if (*ptr == SLIP_ESC) {
>> +				bdev->rx_slip_state = SLIP_ESCAPE;
>> +			} else {
>> +				if (bdev->rx_skb->len < MAX_PACKET_SIZE) {
>> +					skb_put_u8(bdev->rx_skb, *ptr);
>> +				} else {
>> +					kfree_skb(bdev->rx_skb);
>> +					bdev->rx_skb = NULL;
>> +					bdev->rx_slip_state = SLIP_WAIT_DELIM;
>> +				}
>> +			}
>> +			break;
>> +		case SLIP_ESCAPE:
>> +			/* As part of the escape sequence, only the octets
>> +			 * 0xDC for the delimiter and 0xDD for the escape
>> +			 * sequence are valid. All other octets are causing
>> +			 * re-sychronization with the delimter.
>> +			 */
>> +			if (*ptr == SLIP_ESC_DELIM) {
>> +				if (bdev->rx_skb->len < MAX_PACKET_SIZE) {
>> +					skb_put_u8(bdev->rx_skb, SLIP_DELIM);
>> +				} else {
>> +					kfree_skb(bdev->rx_skb);
>> +					bdev->rx_skb = NULL;
>> +					bdev->rx_slip_state = SLIP_WAIT_DELIM;
>> +				}
>> +				bdev->rx_slip_state = SLIP_PACKET;
> 
> Should this be in "if (bdev->rx_skb->len < MAX_PACKET_SIZE)" block?
> Otherwise a too-large packet is put in the SLIP_PACKET state instead of
> SLIP_WAIT_DELIM.

indeed, this is a bug. Good catch.

Regards

Marcel