Received-SPF: pass (google.com: domain of linux-bluetooth+bounces-3705-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Message-ID: <0d7c43dd-6301-465f-b881-5efef708a96c@quicinc.com>
Date: Thu, 18 Apr 2024 09:26:24 +0800
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v2 4/4] Bluetooth: qca: Fix wrong soc type returned for
 tool btattach
To: Luiz Augusto von Dentz <luiz.dentz@gmail.com>
CC: <luiz.von.dentz@intel.com>, <marcel@holtmann.org>,
        <linux-bluetooth@vger.kernel.org>
References: <1710914907-30012-1-git-send-email-quic_zijuhu@quicinc.com>
 <1713358336-29619-1-git-send-email-quic_zijuhu@quicinc.com>
 <1713358336-29619-5-git-send-email-quic_zijuhu@quicinc.com>
 <CABBYNZLRyZu5mEtMjfoQA0VFZrBBbNK5KSYGM4uUrQ5EgmZS6A@mail.gmail.com>
Content-Language: en-US
From: quic_zijuhu <quic_zijuhu@quicinc.com>
In-Reply-To: <CABBYNZLRyZu5mEtMjfoQA0VFZrBBbNK5KSYGM4uUrQ5EgmZS6A@mail.gmail.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 8bit

On 4/18/2024 5:39 AM, Luiz Augusto von Dentz wrote:
> Hi Zijun,
> 
> On Wed, Apr 17, 2024 at 8:53 AM Zijun Hu <quic_zijuhu@quicinc.com> wrote:
>>
>> qca_soc_type() returns wrong soc type QCA_ROME when use tool btattach
>> for any other soc type such as QCA_QCA2066, and wrong soc type will
>> cause later initialization failure, fixed by using user specified
>> soc type for hci_uart derived from tool btattach.
> 
> Then we have to fix qca_soc_type or explain what is going on that it
> can't detect the soc_type if initialized via btattach?
> 
perhaps, my commit message is not precise and cause some mistook.

for tool btattach, only default QCA_ROME is used, there are no way to 
get right soc type for other BT controllers. so i introduce a ioctl to let user specify
soc type info used. so i fix qca_soc_type() to use user specified soc type for tool btattach
case.

1) different soc types have different responses for VSC which is used to detect soc type
as shown by. so soc_type is can't be detected and it  is needed by config by DT|ACPI or user specified.
int qca_read_soc_version(struct hci_dev *hdev, struct qca_btsoc_version *ver,
			 enum qca_btsoc_type soc_type)

2) soc type is a critical info, and it is used everywhere by hci_qca driver, it is also used to
decide which BT firmware to download as shown qca_uart_setup(), it soc type is not right. it will download
error BT firmware and cause serious results.

i will correct commit message for this patch.

>>
>> Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
>> ---
>>  drivers/bluetooth/btqca.h   | 1 +
>>  drivers/bluetooth/hci_qca.c | 8 +++++++-
>>  2 files changed, 8 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/bluetooth/btqca.h b/drivers/bluetooth/btqca.h
>> index dc31984f71dc..a148d4c4e1bd 100644
>> --- a/drivers/bluetooth/btqca.h
>> +++ b/drivers/bluetooth/btqca.h
>> @@ -153,6 +153,7 @@ enum qca_btsoc_type {
>>         QCA_WCN6750,
>>         QCA_WCN6855,
>>         QCA_WCN7850,
>> +       QCA_MAX,
>>  };
>>
>>  #if IS_ENABLED(CONFIG_BT_QCA)
>> diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
>> index c04b97332bca..7c3577a4887c 100644
>> --- a/drivers/bluetooth/hci_qca.c
>> +++ b/drivers/bluetooth/hci_qca.c
>> @@ -238,12 +238,17 @@ static void qca_dmp_hdr(struct hci_dev *hdev, struct sk_buff *skb);
>>
>>  static enum qca_btsoc_type qca_soc_type(struct hci_uart *hu)
>>  {
>> +       /* For Non-serdev device, hu->proto_data records soc type
>> +        * set by ioctl HCIUARTSETPROTODATA.
>> +        */
>> +       int proto_data = (int)hu->proto_data;
>>         enum qca_btsoc_type soc_type;
>>
>>         if (hu->serdev) {
>>                 struct qca_serdev *qsd = serdev_device_get_drvdata(hu->serdev);
>> -
>>                 soc_type = qsd->btsoc_type;
>> +       } else if ((proto_data > 0) && (proto_data < QCA_MAX)) {
>> +               soc_type = (enum qca_btsoc_type)proto_data;
> 
> Like I said a vendor specific ioctl will not gonna fly with me,
> specially since each vendor may need a different size to describe
> their controller version, etc,
> 
i have comments about this part of this question in reply for  [PATCH v2 3/4]

hci_uart->proto_data is a protocol specified unsigned long data, it is parsed
by specific protocol, for protocol, it is parsed as soc type. so force cast to 
(enum qca_btsoc_type).

hci_uart->proto_data is mostly similar as @data of struct struct of_device_id defined by
below header file. it is assigned with misc data type and explained by specific device driver.
include/linux/mod_devicetable.h:
struct of_device_id {
	char	name[32];
	char	type[32];
	char	compatible[128];
	const void *data;
};

  
>>         } else {
>>                 soc_type = QCA_ROME;
>>         }
>> @@ -2281,6 +2286,7 @@ static int qca_serdev_probe(struct serdev_device *serdev)
>>                 return -ENOMEM;
>>
>>         qcadev->serdev_hu.serdev = serdev;
>> +       qcadev->serdev_hu.proto_data = 0;
>>         data = device_get_match_data(&serdev->dev);
>>         serdev_device_set_drvdata(serdev, qcadev);
>>         device_property_read_string(&serdev->dev, "firmware-name",
>> --
>> 2.7.4
>>
> 
>