Re: [PATCH 4/5] HID: apple-ibridge: Add Apple iBridge HID driver for T1 chip.

  Hi Jonathan,

On Sun, Feb 28, 2021 at 03:02:39PM +0000, Jonathan Cameron wrote:

On Sat, 27 Feb 2021 17:26:42 -0800
Ronald Tschal?r [off-list ref] wrote:

[snip]

quoted

+#ifdef CONFIG_PM
+/**
+ * appleib_forward_int_op() - Forward a hid-driver callback to all drivers on
+ * all virtual HID devices attached to the given real HID device.
+ * @hdev the real hid-device
+ * @forward a function that calls the callback on the given driver
+ * @args arguments for the forward function
+ *
+ * This is for callbacks that return a status as an int.
+ *
+ * Returns: 0 on success, or the first error returned by the @forward function.
+ */
+static int appleib_forward_int_op(struct hid_device *hdev,
+				  int (*forward)(struct hid_driver *,
+						 struct hid_device *, void *),
+				  void *args)
+{
+	struct appleib_hid_dev_info *hdev_info = hid_get_drvdata(hdev);
+	struct hid_device *sub_hdev;
+	int rc = 0;
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(hdev_info->sub_hdevs); i++) {
+		sub_hdev = hdev_info->sub_hdevs[i];
+		if (sub_hdev->driver) {
+			rc = forward(sub_hdev->driver, sub_hdev, args);
+			if (rc)
+				break;

return rc; here would be cleaner.

quoted

+		}
+
+		break;

This is unusual.  It's a for loop but as far as I can see only first iteration
can ever run as we exit the loop at this break if we haven't done so earlier.
What is the intent here?

quoted

+	}
+
+	return rc;
+}

Ho boy, good catch! This is simply a mistake. As you say, it should
(and does now) read:

	for (i = 0; i < ARRAY_SIZE(hdev_info->sub_hdevs); i++) {
		sub_hdev = hdev_info->sub_hdevs[i];
		if (sub_hdev->driver) {
			rc = forward(sub_hdev->driver, sub_hdev, args);
			if (rc)
				return rc;
		}
	}

	return rc;

Thanks.


  Cheers,

  Ronald