Received-SPF: pass (google.com: domain of linux-bluetooth+bounces-3749-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Message-ID: <fbe5722b-1e45-4ccb-a050-20a473a823c8@quicinc.com>
Date: Fri, 19 Apr 2024 07:04:11 +0800
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH] Bluetooth: qca: Fix nullptr dereference for non-serdev
 devices
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>
 <ZiFDWeW_-PpI38GQ@hovoldconsulting.com>
Content-Language: en-US
From: quic_zijuhu <quic_zijuhu@quicinc.com>
In-Reply-To: <ZiFDWeW_-PpI38GQ@hovoldconsulting.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit

On 4/18/2024 11:59 PM, Johan Hovold wrote:
> For the third time, wrap your replies at 72 cols.
> 
> I've reflown your reply below manually again, but you need to fix mail
> setup and habits so you can communicate with upstream using the mailing
> lists.
> 
thanks for your reminder. will do it for further mail.
> On Wed, Apr 17, 2024 at 05:38:59PM +0800, quic_zijuhu wrote:
>> On 4/17/2024 4:32 PM, Johan Hovold wrote:
> 
>>>>>> https://patchwork.kernel.org/project/bluetooth/list/?series=844120
> 
>>> In it's current form it's a vendor specific hack that is never going to
>>> make it upstream.
> 
>> 1)
>> ioctl()'s designed purpose is to complete such non-standard config.
> 
> That's irrelevant.
> 
>> 2) present ioctl HCIUARTGETPROTO which is not exported is used to
>> specify which vendor protocol is used is it a a vendor specific hack?
> 
> That's an existing interface, that's ABI and has clearly defined
> semantics, unlike what you are proposing.
> 
> Those protocol values can never change once they've been added.
>  
for new introduced ioctl, who use it will define semantics and ensure it has
unchanged value as HCIUARTGETPROTO.

for QCA protocol, it specify soc_type and soc_type is defined clearly.

hope i can find some present usage or example from kernel.
>> 3)
>> hci_ldisc driver don't touch user specified settings and pass it into
>> vendor driver directly does it has any problem?
> 
> No, because the protocol values will never change, unlike the random
> data you're shuffling into the driver.
>  
for new introduced ioctl, it is not random value. it is soc_type.
hci_qca driver will check its availability.
>> 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.
> 
> I understand why you want this. I still think it's the wrong approach
> and in any case the interface in it's current form is not acceptable.
> 
do you have any better propose for how to specify driver specific settings ?

>>> 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.
> 
> That's not how Linux works, sorry. We never break user space so your
> type data would have to be well-defined and can never change (you can
> only add new types).
> 
>> moreover, tool attach is mainly used before the final product phase.
>> namely, its is mainly used by developer and customer's evaluation.
> 
> Also irrelevant. You still don't get to add random new ioctl() that
> violates the Linux ABI contract.
> 
>>> Can't you just retrieve the device type from the device itself? If it's
>>> already powered, you should not need to know this beforehand
> 
>> 1) it is the simplest and lowest risk fix
> 
> No, it's a quick and dirty hack.
this solution maybe look ugly. but it actually is the simplest and the lowest
risky fix.
> 
>> 2) different soc_types have different responses when read its IDS as
>> shown by qca_read_soc_version().
> 
> I'm sure that can be managed. You claim that these device have a common
> protocol (qca) so it should be possible to probe for such differences.
> 
>> 3) the way you mentioned will involve many changes and it also means
>> high risks for many current soc types.
> 
> There's no risk as hardly anyone uses the line discipline interface
> anymore and it can currently only be used for the old ROME devices.
> Just make sure ROME still works after your change.
> 
no, as explained in cover letter. tool btattch is very useful,
vendor and customer often need to use it even if it still use ldisc interfaces.

vendor and customer often need to attach vendor's BT module to generic linux
machine, then use tool btattach to enable BT and  complete many tasks. as explained
by
https://lore.kernel.org/all/5bcf5034-fea4-43c6-ac7d-db6f24b88512@quicinc.com/

so i need to add tool btattach support for many other QCA soc types.

> Probing the device type should result in a better user experience, which
> I'm sure your customers will appreciate.
>i agree with you. as you known, soc type is embedded in everywhere of
hci_qca driver. since i ever considered auto detection soc_type and it will
involve many of changes and perhaps introduce high ricky broken for present
many qca BT socs. so i drop the solution. let me try to go back auto detection
solution.
thanks
> Johan