Received-SPF: pass (google.com: domain of linux-bluetooth+bounces-3667-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Message-ID: <1657fe63-20d6-4617-813d-99eae7e674be@quicinc.com>
Date: Wed, 17 Apr 2024 17:41:42 +0800
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH] Bluetooth: qca: Fix nullptr dereference for non-serdev
 devices
From: quic_zijuhu <quic_zijuhu@quicinc.com>
To: Johan Hovold <johan@kernel.org>
CC: <luiz.dentz@gmail.com>, <marcel@holtmann.org>,
        <linux-bluetooth@vger.kernel.org>, <johan+linaro@kernel.org>
References: <1713325792-17181-1-git-send-email-quic_zijuhu@quicinc.com>
 <Zh9saGbOl-fXP6SD@hovoldconsulting.com>
 <5e4e62c8-d17f-45fd-9212-088eb026dc7b@quicinc.com>
 <Zh91zq13nZvH3-Yj@hovoldconsulting.com>
 <4dac0caa-629d-4b4d-ad54-f6ab15268524@quicinc.com>
 <Zh-JNzlskY6ZvsSm@hovoldconsulting.com>
 <4313bcc9-8fbf-477d-90f2-013681cdebb9@quicinc.com>
Content-Language: en-US
In-Reply-To: <4313bcc9-8fbf-477d-90f2-013681cdebb9@quicinc.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit

On 4/17/2024 5:38 PM, quic_zijuhu wrote:
> On 4/17/2024 4:32 PM, Johan Hovold wrote:
>> Again, make sure wrap you replies at 72 cols and trim unnecessary
>> context:
>>
>>   https://docs.kernel.org/process/submitting-patches.html#use-trimmed-interleaved-replies-in-email-discussions
>>
>> On Wed, Apr 17, 2024 at 03:32:51PM +0800, quic_zijuhu wrote:
>>> On 4/17/2024 3:10 PM, Johan Hovold wrote:
>>>> On Wed, Apr 17, 2024 at 02:51:38PM +0800, quic_zijuhu wrote:
>>
>>>>> i have submitted below patches to add supports for all other
>>>>> non-serdev controllers.
>>>>
>>>>> https://patchwork.kernel.org/project/bluetooth/list/?series=844120
>>>>
>>>> Ok, you need it for some future changes, but I'm afraid that adding new
>>>> random vendor specific ioctls like you do in that is series is a no-go.
>>
>>> it is a generic ioctl, for QCA, it is used to specific soc_type. it
>>> maybe be used by other vendor driver to set user specified info.
>>
>> In it's current form it's a vendor specific hack that is never going to
>> make it upstream.
>>
> no, i don't think so.
> 
> 1)
> ioctl()'s designed purpose is to complete such non-standard config.
> 
> 2) present ioctl HCIUARTGETPROTO which is not exported is used to specify which vendor protocol is used
>  is it a a vendor specific hack?
> 
> 3)
> hci_ldisc driver don't touch user specified settings and pass it into vendor driver directly
> does it has any problem?
> 
> 4)
> for tool btattach. it does NOT get any board config info from DT|ACPI compared with
> formal BT driver. so i introduce a new ioctl to supplement such info when possible
> to make btattach work.
> 
>  
>> For a start, you don't even make sure that the types becomes part of the
>> ABI, which means that passing, say, type 5 can mean different things
>> depending on the kernel version.
>> it is specified by user and ONLY be parsed by vendor device driver.
> it is user's responsibility to specify the right value. 
> so i also don't check and care about its value and it don't need to change any code
> for further added any new soc_types.
> 
> moreover, tool attach is mainly used before the final product phase. namely, its is mainly used
> by developer and customer's evaluation.
>  
>> Can't you just retrieve the device type from the device itself? If it's
>> already powered, you should not need to know this beforehand1) it is the simplest and lowest risk fix
> 2) different soc_types have different responses when read its IDS as shown by qca_read_soc_version().
> 3) the way you mentioned will involve many changes and it also means high risks for many current soc types.
>> Johan
> 
BTW, it is the simplest and lowest risk fix for tool btattach