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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Johan,

On Thu, Apr 18, 2024 at 11:59 AM Johan Hovold <johan@xxxxxxxxxx> 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.
>
> 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.

+1, thanks a lot for putting in the effort to explain in such detail.

-- 
Luiz Augusto von Dentz





[Index of Archives]     [Bluez Devel]     [Linux Wireless Networking]     [Linux Wireless Personal Area Networking]     [Linux ATH6KL]     [Linux USB Devel]     [Linux Media Drivers]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Big List of Linux Books]

  Powered by Linux