Bluetooth: msft: __hci_cmd_sync() doesn't return NULL

Commit Message

Dan Carpenter Oct. 5, 2023, 11:19 a.m. UTC

The __hci_cmd_sync() function doesn't return NULL.  Checking for NULL
doesn't make the code safer, it just confuses people.

When a function returns both error pointers and NULL then generally the
NULL is a kind of success case.  For example, maybe we look up an item
then errors mean we ran out of memory but NULL means the item is not
found.  Or if we request a feature, then error pointers mean that there
was an error but NULL means that the feature has been deliberately
turned off.

In this code it's different.  The NULL is handled as if there is a bug
in __hci_cmd_sync() where it accidentally returns NULL instead of a
proper error code.  This was done consistently until commit 9e14606d8f38
("Bluetooth: msft: Extended monitor tracking by address filter") which
deleted the work around for the potential future bug and treated NULL as
success.

Predicting potential future bugs is complicated, but we should just fix
them instead of working around them.  Instead of debating whether NULL
is failure or success, let's just say it's currently impossible and
delete the dead code.

Signed-off-by: Dan Carpenter <[e-mail]>
---
 net/bluetooth/msft.c | 20 +++++++-------------
 1 file changed, 7 insertions(+), 13 deletions(-)

Comments

[e-mail] Oct. 5, 2023, 3:03 p.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=790270

---Test result---

Test Summary:
CheckPatch                    PASS      0.68 seconds
GitLint                       PASS      0.30 seconds
SubjectPrefix                 PASS      0.10 seconds
BuildKernel                   PASS      34.61 seconds
CheckAllWarning               PASS      37.11 seconds
CheckSparse                   PASS      42.41 seconds
CheckSmatch                   PASS      113.72 seconds
BuildKernel32                 PASS      32.68 seconds
TestRunnerSetup               PASS      501.17 seconds
TestRunner_l2cap-tester       PASS      31.09 seconds
TestRunner_iso-tester         PASS      52.47 seconds
TestRunner_bnep-tester        PASS      10.49 seconds
TestRunner_mgmt-tester        PASS      221.38 seconds
TestRunner_rfcomm-tester      PASS      16.03 seconds
TestRunner_sco-tester         PASS      19.58 seconds
TestRunner_ioctl-tester       PASS      18.45 seconds
TestRunner_mesh-tester        PASS      13.57 seconds
TestRunner_smp-tester         PASS      14.41 seconds
TestRunner_userchan-tester    PASS      11.19 seconds
IncrementalBuild              PASS      31.16 seconds



---
Regards,
Linux Bluetooth

[e-mail] Oct. 9, 2023, 10:20 p.m. UTC | #2

Hello:

This patch was applied to bluetooth/bluetooth-next.git (master)
by Luiz Augusto von Dentz <[e-mail]>:

On Thu, 5 Oct 2023 14:19:23 +0300 you wrote:
> The __hci_cmd_sync() function doesn't return NULL.  Checking for NULL
> doesn't make the code safer, it just confuses people.
> 
> When a function returns both error pointers and NULL then generally the
> NULL is a kind of success case.  For example, maybe we look up an item
> then errors mean we ran out of memory but NULL means the item is not
> found.  Or if we request a feature, then error pointers mean that there
> was an error but NULL means that the feature has been deliberately
> turned off.
> 
> [...]

Here is the summary with links:
  - Bluetooth: msft: __hci_cmd_sync() doesn't return NULL
    https://git.kernel.org/bluetooth/bluetooth-next/c/99976bf519f2

You are awesome, thank you!

Patch

diff --git a/net/bluetooth/msft.c b/net/bluetooth/msft.c
index abbafa6194ca..630e3023273b 100644
--- a/net/bluetooth/msft.c
+++ b/net/bluetooth/msft.c
@@ -150,10 +150,7 @@  static bool read_supported_features(struct hci_dev *hdev,
 
 	skb = __hci_cmd_sync(hdev, hdev->msft_opcode, sizeof(cp), &cp,
 			     HCI_CMD_TIMEOUT);
-	if (IS_ERR_OR_NULL(skb)) {
-		if (!skb)
-			skb = ERR_PTR(-EIO);
-
+	if (IS_ERR(skb)) {
 		bt_dev_err(hdev, "Failed to read MSFT supported features (%ld)",
 			   PTR_ERR(skb));
 		return false;
@@ -353,7 +350,7 @@  static void msft_remove_addr_filters_sync(struct hci_dev *hdev, u8 handle)
 
 		skb = __hci_cmd_sync(hdev, hdev->msft_opcode, sizeof(cp), &cp,
 				     HCI_CMD_TIMEOUT);
-		if (IS_ERR_OR_NULL(skb)) {
+		if (IS_ERR(skb)) {
 			kfree(address_filter);
 			continue;
 		}
@@ -442,11 +439,8 @@  static int msft_remove_monitor_sync(struct hci_dev *hdev,
 
 	skb = __hci_cmd_sync(hdev, hdev->msft_opcode, sizeof(cp), &cp,
 			     HCI_CMD_TIMEOUT);
-	if (IS_ERR_OR_NULL(skb)) {
-		if (!skb)
-			return -EIO;
+	if (IS_ERR(skb))
 		return PTR_ERR(skb);
-	}
 
 	return msft_le_cancel_monitor_advertisement_cb(hdev, hdev->msft_opcode,
 						       monitor, skb);
@@ -559,7 +553,7 @@  static int msft_add_monitor_sync(struct hci_dev *hdev,
 	skb = __hci_cmd_sync(hdev, hdev->msft_opcode, total_size, cp,
 			     HCI_CMD_TIMEOUT);
 
-	if (IS_ERR_OR_NULL(skb)) {
+	if (IS_ERR(skb)) {
 		err = PTR_ERR(skb);
 		goto out_free;
 	}
@@ -740,10 +734,10 @@  static int msft_cancel_address_filter_sync(struct hci_dev *hdev, void *data)
 
 	skb = __hci_cmd_sync(hdev, hdev->msft_opcode, sizeof(cp), &cp,
 			     HCI_CMD_TIMEOUT);
-	if (IS_ERR_OR_NULL(skb)) {
+	if (IS_ERR(skb)) {
 		bt_dev_err(hdev, "MSFT: Failed to cancel address (%pMR) filter",
 			   &address_filter->bdaddr);
-		err = -EIO;
+		err = PTR_ERR(skb);
 		goto done;
 	}
 	kfree_skb(skb);
@@ -893,7 +887,7 @@  static int msft_add_address_filter_sync(struct hci_dev *hdev, void *data)
 
 	skb = __hci_cmd_sync(hdev, hdev->msft_opcode, size, cp,
 			     HCI_CMD_TIMEOUT);
-	if (IS_ERR_OR_NULL(skb)) {
+	if (IS_ERR(skb)) {
 		bt_dev_err(hdev, "Failed to enable address %pMR filter",
 			   &address_filter->bdaddr);
 		skb = NULL;