linux-kernel - Re: [PATCH 1/1] Bluetooth: btusb: Record debug log for Mediatek Chip.

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [day] [month] [year] [list]

Message-Id: <73A590F6-4EAF-44AC-8803-769F7919712C@holtmann.org>
Date:   Sat, 26 Jun 2021 07:59:30 +0200
From:   Marcel Holtmann <marcel@...tmann.org>
To:     Mark-YW.Chen@...iatek.com
Cc:     Johan Hedberg <johan.hedberg@...il.com>, chris.lu@...iatek.com,
        will-cy.lee@...iatek.com, Sean Wang <sean.wang@...iatek.com>,
        Bluetooth Kernel Mailing List 
        <linux-bluetooth@...r.kernel.org>,
        linux-mediatek@...ts.infradead.org,
        open list <linux-kernel@...r.kernel.org>,
        michaelfsun@...gle.com, shawnku@...gle.com, jemele@...gle.com
Subject: Re: [PATCH 1/1] Bluetooth: btusb: Record debug log for Mediatek Chip.

Hi Mark,

> Mediatek Bluetooth Controller send the fw log via EP2, this patch
> create callback(data->recv_acl) for processing acl packet.
> 
> 1. create callback(data->recv_acl) for processing acl packet.
> 2. Add btusb_recv_acl_mtk to dispatch acl packet.
> 3. Send mediatek debug log and coredump via hci_recv_diag channel.
> 4. The upper layerd can use hci_channel_minitor to receive
>   these packets.
> 
> Signed-off-by: mark-yw.chen <mark-yw.chen@...iatek.com>
> ---
> drivers/bluetooth/btusb.c | 23 ++++++++++++++++++++++-
> 1 file changed, 22 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> index b015dcecfb13..0a86713f496b 100644
> --- a/drivers/bluetooth/btusb.c
> +++ b/drivers/bluetooth/btusb.c
> @@ -569,6 +569,7 @@ struct btusb_data {
> 	int suspend_count;
> 
> 	int (*recv_event)(struct hci_dev *hdev, struct sk_buff *skb);
> +	int (*recv_acl)(struct hci_dev *hdev, struct sk_buff *skb);
> 	int (*recv_bulk)(struct btusb_data *data, void *buffer, int count);
> 
> 	int (*setup_on_usb)(struct hci_dev *hdev);
> @@ -776,7 +777,7 @@ static int btusb_recv_bulk(struct btusb_data *data, void *buffer, int count)
> 
> 		if (!hci_skb_expect(skb)) {
> 			/* Complete frame */
> -			hci_recv_frame(data->hdev, skb);
> +			data->recv_acl(data->hdev, skb);
> 			skb = NULL;
> 		}
> 	}
> @@ -3851,6 +3852,24 @@ static int btusb_mtk_shutdown(struct hci_dev *hdev)
> 	return 0;
> }
> 
> +static int btusb_recv_acl_mtk(struct hci_dev *hdev, struct sk_buff *skb)
> +{
> +	struct btusb_data *data = hci_get_drvdata(hdev);
> +	int err;
> +

since this is guaranteed to be a complete ACL packet now, use the ACL header struct and compare the handle. Which also means you can nicely use a switch statement here.

> +	if (skb->data[0] == 0x6f && skb->data[1] == 0xfc) {

This needs a comment on why you disable auto-suspend here.

> +		usb_disable_autosuspend(data->udev);
> +		err = hci_recv_diag(data->hdev, skb);
> +	} else if ((skb->data[0] == 0xff || skb->data[0] == 0xfe) &&
> +		   skb->data[1] == 0x05) {
> +		err = hci_recv_diag(data->hdev, skb);

I would also comment on what type of diagnostic message each of these are.

> +	} else {
> +		err = hci_recv_frame(data->hdev, skb);
> +	}
> +
> +	return err;
> +}
> +
> MODULE_FIRMWARE(FIRMWARE_MT7663);
> MODULE_FIRMWARE(FIRMWARE_MT7668);
> 
> @@ -4542,6 +4561,7 @@ static int btusb_probe(struct usb_interface *intf,
> 		data->recv_event = hci_recv_frame;
> 		data->recv_bulk = btusb_recv_bulk;
> 	}

Extra empty line before this statement.

> +	data->recv_acl = hci_recv_frame;
> 
> 	hdev = hci_alloc_dev();
> 	if (!hdev)
> @@ -4669,6 +4689,7 @@ static int btusb_probe(struct usb_interface *intf,
> 		hdev->setup = btusb_mtk_setup;
> 		hdev->shutdown = btusb_mtk_shutdown;
> 		hdev->manufacturer = 70;
> +		data->recv_acl = btusb_recv_acl_mtk;
> 		set_bit(HCI_QUIRK_NON_PERSISTENT_SETUP, &hdev->quirks);
> 	}

Regards

Marcel