Bluetooth: qca: Fix nullptr dereference for non-serdev devices

Commit Message

quic_zijuhu April 17, 2024, 3:49 a.m. UTC

hu->serdev is nullptr and will cause nullptr dereference if qca_setup()
is called by non-serdev device, fixed by null check before access.

Fixes: 77f45cca8bc5 ("Bluetooth: qca: fix device-address endianness")
Signed-off-by: Zijun Hu <[e-mail]>
---
 drivers/bluetooth/hci_qca.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

Comments

[e-mail] April 17, 2024, 4:34 a.m. UTC | #1

This is automated email and please do not reply to this email!

Dear submitter,

Thank you for submitting the patches to the linux bluetooth mailing list.
This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=845272

---Test result---

Test Summary:
CheckPatch                    PASS      0.49 seconds
GitLint                       PASS      0.20 seconds
SubjectPrefix                 PASS      0.07 seconds
BuildKernel                   PASS      29.66 seconds
CheckAllWarning               PASS      32.54 seconds
CheckSparse                   PASS      37.94 seconds
CheckSmatch                   FAIL      36.06 seconds
BuildKernel32                 PASS      28.89 seconds
TestRunnerSetup               PASS      518.82 seconds
TestRunner_l2cap-tester       PASS      22.47 seconds
TestRunner_iso-tester         PASS      30.86 seconds
TestRunner_bnep-tester        PASS      4.60 seconds
TestRunner_mgmt-tester        PASS      110.61 seconds
TestRunner_rfcomm-tester      PASS      7.15 seconds
TestRunner_sco-tester         PASS      14.91 seconds
TestRunner_ioctl-tester       PASS      7.52 seconds
TestRunner_mesh-tester        FAIL      5.96 seconds
TestRunner_smp-tester         PASS      6.67 seconds
TestRunner_userchan-tester    PASS      5.16 seconds
IncrementalBuild              PASS      27.78 seconds

Details
##############################
Test: CheckSmatch - FAIL
Desc: Run smatch tool with source
Output:

Segmentation fault (core dumped)
make[4]: *** [scripts/Makefile.build:244: net/bluetooth/hci_core.o] Error 139
make[4]: *** Deleting file 'net/bluetooth/hci_core.o'
make[3]: *** [scripts/Makefile.build:485: net/bluetooth] Error 2
make[2]: *** [scripts/Makefile.build:485: net] Error 2
make[2]: *** Waiting for unfinished jobs....
Segmentation fault (core dumped)
make[4]: *** [scripts/Makefile.build:244: drivers/bluetooth/bcm203x.o] Error 139
make[4]: *** Deleting file 'drivers/bluetooth/bcm203x.o'
make[4]: *** Waiting for unfinished jobs....
Segmentation fault (core dumped)
make[4]: *** [scripts/Makefile.build:244: drivers/bluetooth/bpa10x.o] Error 139
make[4]: *** Deleting file 'drivers/bluetooth/bpa10x.o'
make[3]: *** [scripts/Makefile.build:485: drivers/bluetooth] Error 2
make[2]: *** [scripts/Makefile.build:485: drivers] Error 2
make[1]: *** [/github/workspace/src/src/Makefile:1919: .] Error 2
make: *** [Makefile:240: __sub-make] Error 2
##############################
Test: TestRunner_mesh-tester - FAIL
Desc: Run mesh-tester with test-runner
Output:
Total: 10, Passed: 9 (90.0%), Failed: 1, Not Run: 0

Failed Test Cases
Mesh - Send cancel - 2                               Failed       0.099 seconds


---
Regards,
Linux Bluetooth

Johan Hovold April 17, 2024, 6:30 a.m. UTC | #2

On Wed, Apr 17, 2024 at 11:49:52AM +0800, Zijun Hu wrote:
> hu->serdev is nullptr and will cause nullptr dereference if qca_setup()
> is called by non-serdev device, fixed by null check before access.

No, this patch is not correct.
 
> Fixes: 77f45cca8bc5 ("Bluetooth: qca: fix device-address endianness")
> Signed-off-by: Zijun Hu <[e-mail]>
> ---
>  drivers/bluetooth/hci_qca.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
> index 92fa20f5ac7d..9c6573c727e1 100644
> --- a/drivers/bluetooth/hci_qca.c
> +++ b/drivers/bluetooth/hci_qca.c
> @@ -1905,10 +1905,11 @@ static int qca_setup(struct hci_uart *hu)
>  	case QCA_WCN6750:
>  	case QCA_WCN6855:
>  	case QCA_WCN7850:
> -		qcadev = serdev_device_get_drvdata(hu->serdev);

Non-serdev controllers have type QCA_ROME (see qca_soc_type()) so will
never end up in this path.

I verified this when I wrote the patch and also fixed up a couple of
real non-serdev bugs here:

	https://lore.kernel.org/lkml/[e-mail]/

> -		if (qcadev->bdaddr_property_broken)
> -			set_bit(HCI_QUIRK_BDADDR_PROPERTY_BROKEN, &hdev->quirks);
> -
> +		if (hu->serdev) {
> +			qcadev = serdev_device_get_drvdata(hu->serdev);
> +			if (qcadev->bdaddr_property_broken)
> +				set_bit(HCI_QUIRK_BDADDR_PROPERTY_BROKEN, &hdev->quirks);
> +		}
>  		hci_set_aosp_capable(hdev);
>  
>  		ret = qca_read_soc_version(hdev, &ver, soc_type);

Johan

quic_zijuhu April 17, 2024, 6:51 a.m. UTC | #3

On 4/17/2024 2:30 PM, Johan Hovold wrote:
> On Wed, Apr 17, 2024 at 11:49:52AM +0800, Zijun Hu wrote:
>> hu->serdev is nullptr and will cause nullptr dereference if qca_setup()
>> is called by non-serdev device, fixed by null check before access.
> 
> No, this patch is not correct.
>  
i don't think so, nullptr checking for hu->serdev has been performed within qca_setup()
everywhere when need to access serdev related members since this function will be called 
by both serdev and none-serdev. so suggest add such checking.

>> Fixes: 77f45cca8bc5 ("Bluetooth: qca: fix device-address endianness")
>> Signed-off-by: Zijun Hu <[e-mail]>
>> ---
>>  drivers/bluetooth/hci_qca.c | 9 +++++----
>>  1 file changed, 5 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
>> index 92fa20f5ac7d..9c6573c727e1 100644
>> --- a/drivers/bluetooth/hci_qca.c
>> +++ b/drivers/bluetooth/hci_qca.c
>> @@ -1905,10 +1905,11 @@ static int qca_setup(struct hci_uart *hu)
>>  	case QCA_WCN6750:
>>  	case QCA_WCN6855:
>>  	case QCA_WCN7850:
>> -		qcadev = serdev_device_get_drvdata(hu->serdev);
> 
> Non-serdev controllers have type QCA_ROME (see qca_soc_type()) so will
> never end up in this path.
> 
i have submitted below patches to add supports for all other non-serdev controllers.
https://patchwork.kernel.org/project/bluetooth/list/?series=844120

> I verified this when I wrote the patch and also fixed up a couple of
> real non-serdev bugs here:
> 
> 	https://lore.kernel.org/lkml/[e-mail]/
>
actually, i have submitted below fix for this issue earlier.
https://lore.kernel.org/all/[e-mail]/

>> -		if (qcadev->bdaddr_property_broken)
>> -			set_bit(HCI_QUIRK_BDADDR_PROPERTY_BROKEN, &hdev->quirks);
>> -
>> +		if (hu->serdev) {
>> +			qcadev = serdev_device_get_drvdata(hu->serdev);
>> +			if (qcadev->bdaddr_property_broken)
>> +				set_bit(HCI_QUIRK_BDADDR_PROPERTY_BROKEN, &hdev->quirks);
>> +		}
>>  		hci_set_aosp_capable(hdev);
>>  
>>  		ret = qca_read_soc_version(hdev, &ver, soc_type);
> 
> Johan

Johan Hovold April 17, 2024, 7:10 a.m. UTC | #4

[ Please wrap you mails at 72 columns or so and trim unnecessary context
  when replying. ]

On Wed, Apr 17, 2024 at 02:51:38PM +0800, quic_zijuhu wrote:
> On 4/17/2024 2:30 PM, Johan Hovold wrote:
> > On Wed, Apr 17, 2024 at 11:49:52AM +0800, Zijun Hu wrote:
> >> hu->serdev is nullptr and will cause nullptr dereference if qca_setup()
> >> is called by non-serdev device, fixed by null check before access.
> > 
> > No, this patch is not correct.

> i don't think so, nullptr checking for hu->serdev has been performed
> within qca_setup() everywhere when need to access serdev related
> members since this function will be called by both serdev and
> none-serdev. so suggest add such checking.

Your patch is not correct since you claim that this path can trigger a
NULL pointer dereference. As I point out below that is currently not
possible.

If you need this for some future change you need to say so in the commit
message and drop the bogus Fixes tag.

> >> Fixes: 77f45cca8bc5 ("Bluetooth: qca: fix device-address endianness")
> >> Signed-off-by: Zijun Hu <[e-mail]>

> >> @@ -1905,10 +1905,11 @@ static int qca_setup(struct hci_uart *hu)
> >>  	case QCA_WCN6750:
> >>  	case QCA_WCN6855:
> >>  	case QCA_WCN7850:
> >> -		qcadev = serdev_device_get_drvdata(hu->serdev);
> > 
> > Non-serdev controllers have type QCA_ROME (see qca_soc_type()) so will
> > never end up in this path.

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

Why are you trying to revive the old line-discipline when we have
serdev?

In any case, a change like this one would should be included in that
series so that it's clear that it is only needed for your proposed
further changes.
 
> > I verified this when I wrote the patch and also fixed up a couple of
> > real non-serdev bugs here:
> > 
> > 	https://lore.kernel.org/lkml/[e-mail]/

> actually, i have submitted below fix for this issue earlier.
> https://lore.kernel.org/all/[e-mail]/

Ok.

Johan

quic_zijuhu April 17, 2024, 7:32 a.m. UTC | #5

On 4/17/2024 3:10 PM, Johan Hovold wrote:
> [ Please wrap you mails at 72 columns or so and trim unnecessary context
>   when replying. ]
> 
> On Wed, Apr 17, 2024 at 02:51:38PM +0800, quic_zijuhu wrote:
>> On 4/17/2024 2:30 PM, Johan Hovold wrote:
>>> On Wed, Apr 17, 2024 at 11:49:52AM +0800, Zijun Hu wrote:
>>>> hu->serdev is nullptr and will cause nullptr dereference if qca_setup()
>>>> is called by non-serdev device, fixed by null check before access.
>>>
>>> No, this patch is not correct.
> 
>> i don't think so, nullptr checking for hu->serdev has been performed
>> within qca_setup() everywhere when need to access serdev related
>> members since this function will be called by both serdev and
>> none-serdev. so suggest add such checking.
> 
> Your patch is not correct since you claim that this path can trigger a
> NULL pointer dereference. As I point out below that is currently not
> possible.
> 
> If you need this for some future change you need to say so in the commit
> message and drop the bogus Fixes tag.
> 
will add this change in to btattach support serials.
>>>> Fixes: 77f45cca8bc5 ("Bluetooth: qca: fix device-address endianness")
>>>> Signed-off-by: Zijun Hu <[e-mail]>
> 
>>>> @@ -1905,10 +1905,11 @@ static int qca_setup(struct hci_uart *hu)
>>>>  	case QCA_WCN6750:
>>>>  	case QCA_WCN6855:
>>>>  	case QCA_WCN7850:
>>>> -		qcadev = serdev_device_get_drvdata(hu->serdev);
>>>
>>> Non-serdev controllers have type QCA_ROME (see qca_soc_type()) so will
>>> never end up in this path.
> 
>> 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.
 
> Why are you trying to revive the old line-discipline when we have
> serdev?
> 

we usually need to use tool btattach which will generate non-serdev devices to attach a BT module to generic PC to
complete various development and verification, customer also have requirements
to use tool btattach as explained by below link
https://patchwork.kernel.org/project/bluetooth/patch/[e-mail]/

> In any case, a change like this one would should be included in that
> series so that it's clear that it is only needed for your proposed
> further changes.
>  okay, it has been included in the updated patch serials, will update the serials to include change
in question.
https://patchwork.kernel.org/project/bluetooth/list/?series=844120
>>> I verified this when I wrote the patch and also fixed up a couple of
>>> real non-serdev bugs here:
>>>
>>> 	https://lore.kernel.org/lkml/[e-mail]/
> 
>> actually, i have submitted below fix for this issue earlier.
>> https://lore.kernel.org/all/[e-mail]/
> 
> Ok.
> 
> Johan

Johan Hovold April 17, 2024, 8:32 a.m. UTC | #6

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.

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.

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

quic_zijuhu April 17, 2024, 9:38 a.m. UTC | #7

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

quic_zijuhu April 17, 2024, 9:41 a.m. UTC | #8

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

Johan Hovold April 18, 2024, 3:59 p.m. UTC | #9

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

Luiz Augusto von Dentz April 18, 2024, 5:07 p.m. UTC | #10

Hi Johan,

On Thu, Apr 18, 2024 at 11:59 AM Johan Hovold <[e-mail]> 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.

quic_zijuhu April 18, 2024, 11:04 p.m. UTC | #11

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/[e-mail]/

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

Patch

diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
index 92fa20f5ac7d..9c6573c727e1 100644
--- a/drivers/bluetooth/hci_qca.c
+++ b/drivers/bluetooth/hci_qca.c
@@ -1905,10 +1905,11 @@  static int qca_setup(struct hci_uart *hu)
 	case QCA_WCN6750:
 	case QCA_WCN6855:
 	case QCA_WCN7850:
-		qcadev = serdev_device_get_drvdata(hu->serdev);
-		if (qcadev->bdaddr_property_broken)
-			set_bit(HCI_QUIRK_BDADDR_PROPERTY_BROKEN, &hdev->quirks);
-
+		if (hu->serdev) {
+			qcadev = serdev_device_get_drvdata(hu->serdev);
+			if (qcadev->bdaddr_property_broken)
+				set_bit(HCI_QUIRK_BDADDR_PROPERTY_BROKEN, &hdev->quirks);
+		}
 		hci_set_aosp_capable(hdev);
 
 		ret = qca_read_soc_version(hdev, &ver, soc_type);