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

[Johan Hovold <johan@kernel.org>]

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.

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.
 
> 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.
 
> 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.

> > 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.

> 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.

Probing the device type should result in a better user experience, which
I'm sure your customers will appreciate.

Johan