Re: [PATCH] Bluetooth: qca: Fix nullptr dereference for non-serdev devices

[quic_zijuhu <quic_zijuhu@quicinc.com>]

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