[PATCH 0/3] HID: sony: Various cleanups and fixes for the sony module

[PATCH 0/3] HID: sony: Various cleanups and fixes for the sony module

[Frank Praznik]

Since the new ll_driver functions allow for reports to be sent on the control
channel the LEDs and force-feedback can be enabled for the Sixaxis/Dualshock 3.

The second patch only sets the force-feedback flags for devices that actually
support force feedback.  It also moves the cancel_work_sync function out of the
ff shutdown function since the worker functions are initialized and used for
the LEDs even when force-feedback support is disabled at compile time.

The final patch is an updated version of the duplicate controller detection code
that was originally part of the Bluetooth patch set.  It now works on both the
Sixaxis and Dualshock 4.  On USB the MAC can be retrieved via feature reports.
Unfortunately, neither controller supports retrieving the MAC address via a
feature report when connected via Bluetooth so it needs to be parsed from the
uniq string where HIDP stores it.  Based on previous discussions
(http://www.spinics.net/lists/linux-input/msg29530.html), uniq should provide a
stable way for retrieving a Bluetooth MAC address under normal circumstances,
unless the behavior in HIDP changes for some reason.  If a MAC cannot be parsed
from the uniq string the duplicate check will be skipped and the connection will
proceed, so even in the case of a uHID device or the behavior of HIDP changing
in the future, a user's controller will still work.

[PATCH 1/3] HID: sony: Enable LED controls and rumble for the Sixaxis on Bluetooth.

[Frank Praznik]

Add a SIXAXIS_CONTROLLER macro to simplify conditionals where the
connection type is irrelevant.

Enable the LED and force feedback controls for Sixaxis controllers connected via
Bluetooth.

Send Sixaxis Bluetooth output reports on the control channel.

Signed-off-by: Frank Praznik <frank.praznik@oh.rr.com>
---
 drivers/hid/hid-sony.c     | 24 +++++++++++++-----------
 1 file changed, 13 insertions(+), 11 deletions(-)

diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c
index f88d1ae..81917eb 100644
--- a/drivers/hid/hid-sony.c
+++ b/drivers/hid/hid-sony.c
@@ -44,12 +44,12 @@
 #define DUALSHOCK4_CONTROLLER_USB BIT(5)
 #define DUALSHOCK4_CONTROLLER_BT  BIT(6)
 
+#define SIXAXIS_CONTROLLER (SIXAXIS_CONTROLLER_USB | SIXAXIS_CONTROLLER_BT)
 #define DUALSHOCK4_CONTROLLER (DUALSHOCK4_CONTROLLER_USB |\
 				DUALSHOCK4_CONTROLLER_BT)
-#define SONY_LED_SUPPORT (SIXAXIS_CONTROLLER_USB | BUZZ_CONTROLLER |\
-				DUALSHOCK4_CONTROLLER)
-#define SONY_BATTERY_SUPPORT (SIXAXIS_CONTROLLER_USB | SIXAXIS_CONTROLLER_BT |\
+#define SONY_LED_SUPPORT (SIXAXIS_CONTROLLER | BUZZ_CONTROLLER |\
 				DUALSHOCK4_CONTROLLER)
+#define SONY_BATTERY_SUPPORT (SIXAXIS_CONTROLLER | DUALSHOCK4_CONTROLLER)
 
 #define MAX_LEDS 4
 
@@ -935,8 +935,7 @@ static int sony_raw_event(struct hid_device *hdev, struct hid_report *report,
 	/* Sixaxis HID report has acclerometers/gyro with MSByte first, this
 	 * has to be BYTE_SWAPPED before passing up to joystick interface
 	 */
-	if ((sc->quirks & (SIXAXIS_CONTROLLER_USB | SIXAXIS_CONTROLLER_BT)) &&
-			rd[0] == 0x01 && size == 49) {
+	if ((sc->quirks & SIXAXIS_CONTROLLER) && rd[0] == 0x01 && size == 49) {
 		swap(rd[41], rd[42]);
 		swap(rd[43], rd[44]);
 		swap(rd[45], rd[46]);
@@ -1096,8 +1095,7 @@ static void sony_set_leds(struct hid_device *hdev, const __u8 *leds, int count)
 
 	if (drv_data->quirks & BUZZ_CONTROLLER && count == 4) {
 		buzz_set_leds(hdev, leds);
-	} else if ((drv_data->quirks & SIXAXIS_CONTROLLER_USB) ||
-		   (drv_data->quirks & DUALSHOCK4_CONTROLLER)) {
+	} else {
 		for (n = 0; n < count; n++)
 			drv_data->led_state[n] = leds[n];
 		schedule_work(&drv_data->state_worker);
@@ -1285,7 +1283,11 @@ static void sixaxis_state_worker(struct work_struct *work)
 	buf[10] |= sc->led_state[2] << 3;
 	buf[10] |= sc->led_state[3] << 4;
 
-	hid_output_raw_report(sc->hdev, buf, sizeof(buf), HID_OUTPUT_REPORT);
+	if (sc->quirks & SIXAXIS_CONTROLLER_USB)
+		hid_output_raw_report(sc->hdev, buf, sizeof(buf), HID_OUTPUT_REPORT);
+	else
+		hid_hw_raw_request(sc->hdev, 0x01, buf, sizeof(buf),
+				HID_OUTPUT_REPORT, HID_REQ_SET_REPORT);
 }
 
 static void dualshock4_state_worker(struct work_struct *work)
@@ -1520,10 +1522,10 @@ static int sony_probe(struct hid_device *hdev, const struct hid_device_id *id)
 		hdev->hid_output_raw_report = sixaxis_usb_output_raw_report;
 		ret = sixaxis_set_operational_usb(hdev);
 		INIT_WORK(&sc->state_worker, sixaxis_state_worker);
-	}
-	else if (sc->quirks & SIXAXIS_CONTROLLER_BT)
+	} else if (sc->quirks & SIXAXIS_CONTROLLER_BT) {
 		ret = sixaxis_set_operational_bt(hdev);
-	else if (sc->quirks & DUALSHOCK4_CONTROLLER) {
+		INIT_WORK(&sc->state_worker, sixaxis_state_worker);
+	} else if (sc->quirks & DUALSHOCK4_CONTROLLER) {
 		if (sc->quirks & DUALSHOCK4_CONTROLLER_BT) {
 			ret = dualshock4_set_operational_bt(hdev);
 			if (ret < 0) {
-- 
1.8.5.3

Re: [PATCH 1/3] HID: sony: Enable LED controls and rumble for the Sixaxis on Bluetooth.

[David Herrmann]

Hi

On Tue, Feb 18, 2014 at 11:22 PM, Frank Praznik <frank.praznik@oh.rr.com> wrote:
> Add a SIXAXIS_CONTROLLER macro to simplify conditionals where the
> connection type is irrelevant.
>
> Enable the LED and force feedback controls for Sixaxis controllers connected via
> Bluetooth.
>
> Send Sixaxis Bluetooth output reports on the control channel.
>
> Signed-off-by: Frank Praznik <frank.praznik@oh.rr.com>
> ---
>  drivers/hid/hid-sony.c     | 24 +++++++++++++-----------
>  1 file changed, 13 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c
> index f88d1ae..81917eb 100644
> --- a/drivers/hid/hid-sony.c
> +++ b/drivers/hid/hid-sony.c
> @@ -44,12 +44,12 @@
>  #define DUALSHOCK4_CONTROLLER_USB BIT(5)
>  #define DUALSHOCK4_CONTROLLER_BT  BIT(6)
>
> +#define SIXAXIS_CONTROLLER (SIXAXIS_CONTROLLER_USB | SIXAXIS_CONTROLLER_BT)
>  #define DUALSHOCK4_CONTROLLER (DUALSHOCK4_CONTROLLER_USB |\
>                                 DUALSHOCK4_CONTROLLER_BT)
> -#define SONY_LED_SUPPORT (SIXAXIS_CONTROLLER_USB | BUZZ_CONTROLLER |\
> -                               DUALSHOCK4_CONTROLLER)
> -#define SONY_BATTERY_SUPPORT (SIXAXIS_CONTROLLER_USB | SIXAXIS_CONTROLLER_BT |\
> +#define SONY_LED_SUPPORT (SIXAXIS_CONTROLLER | BUZZ_CONTROLLER |\
>                                 DUALSHOCK4_CONTROLLER)
> +#define SONY_BATTERY_SUPPORT (SIXAXIS_CONTROLLER | DUALSHOCK4_CONTROLLER)
>
>  #define MAX_LEDS 4
>
> @@ -935,8 +935,7 @@ static int sony_raw_event(struct hid_device *hdev, struct hid_report *report,
>         /* Sixaxis HID report has acclerometers/gyro with MSByte first, this
>          * has to be BYTE_SWAPPED before passing up to joystick interface
>          */
> -       if ((sc->quirks & (SIXAXIS_CONTROLLER_USB | SIXAXIS_CONTROLLER_BT)) &&
> -                       rd[0] == 0x01 && size == 49) {
> +       if ((sc->quirks & SIXAXIS_CONTROLLER) && rd[0] == 0x01 && size == 49) {
>                 swap(rd[41], rd[42]);
>                 swap(rd[43], rd[44]);
>                 swap(rd[45], rd[46]);
> @@ -1096,8 +1095,7 @@ static void sony_set_leds(struct hid_device *hdev, const __u8 *leds, int count)
>
>         if (drv_data->quirks & BUZZ_CONTROLLER && count == 4) {
>                 buzz_set_leds(hdev, leds);
> -       } else if ((drv_data->quirks & SIXAXIS_CONTROLLER_USB) ||
> -                  (drv_data->quirks & DUALSHOCK4_CONTROLLER)) {
> +       } else {
>                 for (n = 0; n < count; n++)
>                         drv_data->led_state[n] = leds[n];
>                 schedule_work(&drv_data->state_worker);
> @@ -1285,7 +1283,11 @@ static void sixaxis_state_worker(struct work_struct *work)
>         buf[10] |= sc->led_state[2] << 3;
>         buf[10] |= sc->led_state[3] << 4;
>
> -       hid_output_raw_report(sc->hdev, buf, sizeof(buf), HID_OUTPUT_REPORT);
> +       if (sc->quirks & SIXAXIS_CONTROLLER_USB)
> +               hid_output_raw_report(sc->hdev, buf, sizeof(buf), HID_OUTPUT_REPORT);
> +       else
> +               hid_hw_raw_request(sc->hdev, 0x01, buf, sizeof(buf),
> +                               HID_OUTPUT_REPORT, HID_REQ_SET_REPORT);
>  }
>
>  static void dualshock4_state_worker(struct work_struct *work)
> @@ -1520,10 +1522,10 @@ static int sony_probe(struct hid_device *hdev, const struct hid_device_id *id)
>                 hdev->hid_output_raw_report = sixaxis_usb_output_raw_report;
>                 ret = sixaxis_set_operational_usb(hdev);
>                 INIT_WORK(&sc->state_worker, sixaxis_state_worker);
> -       }
> -       else if (sc->quirks & SIXAXIS_CONTROLLER_BT)
> +       } else if (sc->quirks & SIXAXIS_CONTROLLER_BT) {

We usually send coding-style cleanups as separate patches so reverts
will not affect them. Anyhow, no need to resend imho, so:

Reviewed-by: David Herrmann <dh.herrmann@gmail.com>

Thanks
David

>                 ret = sixaxis_set_operational_bt(hdev);
> -       else if (sc->quirks & DUALSHOCK4_CONTROLLER) {
> +               INIT_WORK(&sc->state_worker, sixaxis_state_worker);
> +       } else if (sc->quirks & DUALSHOCK4_CONTROLLER) {
>                 if (sc->quirks & DUALSHOCK4_CONTROLLER_BT) {
>                         ret = dualshock4_set_operational_bt(hdev);
>                         if (ret < 0) {
> --
> 1.8.5.3
>

Re: [PATCH 1/3] HID: sony: Enable LED controls and rumble for the Sixaxis on Bluetooth.

[Jiri Kosina]

On Tue, 18 Feb 2014, Frank Praznik wrote:

> Add a SIXAXIS_CONTROLLER macro to simplify conditionals where the
> connection type is irrelevant.
> 
> Enable the LED and force feedback controls for Sixaxis controllers connected via
> Bluetooth.
> 
> Send Sixaxis Bluetooth output reports on the control channel.
> 
> Signed-off-by: Frank Praznik <frank.praznik@oh.rr.com>

Queued for 3.15, thanks.

-- 
Jiri Kosina
SUSE Labs

[PATCH 2/3] HID: sony: Force-feedback cleanup

[Frank Praznik]

Only initialize force feedback for devices that actually support it (Sixaxis and
Dualshock 4).

Move the cancel_work_sync() call out of sony_destroy_ff() since the state worker
is used for the LEDs even when force-feedback is disabled.

Remove the sony_destroy_ff() function since it is no longer used.

Signed-off-by: Frank Praznik <frank.praznik@oh.rr.com>
---
 drivers/hid/hid-sony.c | 21 +++++++--------------
 1 file changed, 7 insertions(+), 14 deletions(-)

diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c
index 81917eb..ad1cebd 100644
--- a/drivers/hid/hid-sony.c
+++ b/drivers/hid/hid-sony.c
@@ -50,6 +50,7 @@
 #define SONY_LED_SUPPORT (SIXAXIS_CONTROLLER | BUZZ_CONTROLLER |\
 				DUALSHOCK4_CONTROLLER)
 #define SONY_BATTERY_SUPPORT (SIXAXIS_CONTROLLER | DUALSHOCK4_CONTROLLER)
+#define SONY_FF_SUPPORT (SIXAXIS_CONTROLLER | DUALSHOCK4_CONTROLLER)
 
 #define MAX_LEDS 4
 
@@ -1354,22 +1355,12 @@ static int sony_init_ff(struct hid_device *hdev)
 	return input_ff_create_memless(input_dev, NULL, sony_play_effect);
 }
 
-static void sony_destroy_ff(struct hid_device *hdev)
-{
-	struct sony_sc *sc = hid_get_drvdata(hdev);
-
-	cancel_work_sync(&sc->state_worker);
-}
-
 #else
 static int sony_init_ff(struct hid_device *hdev)
 {
 	return 0;
 }
 
-static void sony_destroy_ff(struct hid_device *hdev)
-{
-}
 #endif
 
 static int sony_battery_get_property(struct power_supply *psy,
@@ -1567,9 +1558,11 @@ static int sony_probe(struct hid_device *hdev, const struct hid_device_id *id)
 		}
 	}
 
-	ret = sony_init_ff(hdev);
-	if (ret < 0)
-		goto err_close;
+	if (sc->quirks & SONY_FF_SUPPORT) {
+		ret = sony_init_ff(hdev);
+		if (ret < 0)
+			goto err_close;
+	}
 
 	return 0;
 err_close:
@@ -1595,7 +1588,7 @@ static void sony_remove(struct hid_device *hdev)
 		sony_battery_remove(sc);
 	}
 
-	sony_destroy_ff(hdev);
+	cancel_work_sync(&sc->state_worker);
 
 	hid_hw_stop(hdev);
 }
-- 
1.8.5.3

Re: [PATCH 2/3] HID: sony: Force-feedback cleanup

[David Herrmann]

Hi

On Tue, Feb 18, 2014 at 11:22 PM, Frank Praznik <frank.praznik@oh.rr.com> wrote:
> Only initialize force feedback for devices that actually support it (Sixaxis and
> Dualshock 4).
>
> Move the cancel_work_sync() call out of sony_destroy_ff() since the state worker
> is used for the LEDs even when force-feedback is disabled.
>
> Remove the sony_destroy_ff() function since it is no longer used.
>
> Signed-off-by: Frank Praznik <frank.praznik@oh.rr.com>
> ---
>  drivers/hid/hid-sony.c | 21 +++++++--------------
>  1 file changed, 7 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c
> index 81917eb..ad1cebd 100644
> --- a/drivers/hid/hid-sony.c
> +++ b/drivers/hid/hid-sony.c
> @@ -50,6 +50,7 @@
>  #define SONY_LED_SUPPORT (SIXAXIS_CONTROLLER | BUZZ_CONTROLLER |\
>                                 DUALSHOCK4_CONTROLLER)
>  #define SONY_BATTERY_SUPPORT (SIXAXIS_CONTROLLER | DUALSHOCK4_CONTROLLER)
> +#define SONY_FF_SUPPORT (SIXAXIS_CONTROLLER | DUALSHOCK4_CONTROLLER)
>
>  #define MAX_LEDS 4
>
> @@ -1354,22 +1355,12 @@ static int sony_init_ff(struct hid_device *hdev)
>         return input_ff_create_memless(input_dev, NULL, sony_play_effect);
>  }
>
> -static void sony_destroy_ff(struct hid_device *hdev)
> -{
> -       struct sony_sc *sc = hid_get_drvdata(hdev);
> -
> -       cancel_work_sync(&sc->state_worker);
> -}
> -
>  #else
>  static int sony_init_ff(struct hid_device *hdev)
>  {
>         return 0;
>  }
>
> -static void sony_destroy_ff(struct hid_device *hdev)
> -{
> -}
>  #endif
>
>  static int sony_battery_get_property(struct power_supply *psy,
> @@ -1567,9 +1558,11 @@ static int sony_probe(struct hid_device *hdev, const struct hid_device_id *id)
>                 }
>         }
>
> -       ret = sony_init_ff(hdev);
> -       if (ret < 0)
> -               goto err_close;
> +       if (sc->quirks & SONY_FF_SUPPORT) {
> +               ret = sony_init_ff(hdev);
> +               if (ret < 0)
> +                       goto err_close;
> +       }
>
>         return 0;
>  err_close:
> @@ -1595,7 +1588,7 @@ static void sony_remove(struct hid_device *hdev)
>                 sony_battery_remove(sc);
>         }
>
> -       sony_destroy_ff(hdev);
> +       cancel_work_sync(&sc->state_worker);

Ugh, that's a bugfix! This can trigger if the led-worker runs during
unplug. You should mark it for stable next time you send it (adding:
Cc: <stable@vger.kernel.org>)

Reviewed-by: David Herrmann <dh.herrmann@gmail.com>

Thanks
David

>
>         hid_hw_stop(hdev);
>  }
> --
> 1.8.5.3
>

[PATCH 3/3] HID: sony: Prevent duplicate controller connections.

[Frank Praznik]

If a Sixaxis or Dualshock 4 controller is connected via USB while already
connected via Bluetooth it will cause duplicate devices to be added to the
input device list.

To prevent this a global list of controllers and their MAC addresses is
maintained and new controllers are checked against this list.  If a duplicate
is found, the probe function with exit with -EEXIST.

On USB the MAC is retrieved via a feature report.  On Bluetooth neither
controller reports the MAC address in a feature report so the MAC is parsed from
the uniq string.  As uniq cannot be guaranteed to be a MAC address in every case
(uHID or the behavior of HIDP changing) a parsing failure will not prevent the
connection.

Signed-off-by: Frank Praznik <frank.praznik@oh.rr.com>
---
 drivers/hid/hid-sony.c | 165 +++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 159 insertions(+), 6 deletions(-)

diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c
index ad1cebd..c25f0d7 100644
--- a/drivers/hid/hid-sony.c
+++ b/drivers/hid/hid-sony.c
@@ -713,8 +713,12 @@ static enum power_supply_property sony_battery_props[] = {
 	POWER_SUPPLY_PROP_STATUS,
 };
 
+static spinlock_t sony_dev_list_lock;
+static LIST_HEAD(sony_device_list);
+
 struct sony_sc {
 	spinlock_t lock;
+	struct list_head list_node;
 	struct hid_device *hdev;
 	struct led_classdev *leds[MAX_LEDS];
 	unsigned long quirks;
@@ -726,6 +730,7 @@ struct sony_sc {
 	__u8 right;
 #endif
 
+	__u8 mac_address[6];
 	__u8 cable_state;
 	__u8 battery_charging;
 	__u8 battery_capacity;
@@ -1473,6 +1478,137 @@ static int sony_register_touchpad(struct sony_sc *sc, int touch_count,
 	return 0;
 }
 
+/* If a controller is plugged in via USB while already connected via Bluetooth
+ * it will show up as two devices. A global list of connected controllers and
+ * their MAC addresses is maintained to ensure that a device is only connected
+ * once.
+ */
+static int sony_check_add_dev_list(struct sony_sc *sc)
+{
+	struct sony_sc *entry;
+	struct list_head *pos;
+	unsigned long flags;
+	int ret;
+
+	spin_lock_irqsave(&sony_dev_list_lock, flags);
+
+	list_for_each(pos, &sony_device_list) {
+		entry = list_entry(pos, struct sony_sc, list_node);
+		ret = memcmp(sc->mac_address, entry->mac_address,
+				sizeof(sc->mac_address));
+		if (!ret) {
+			if (sc->hdev->bus != entry->hdev->bus) {
+				ret = -EEXIST;
+				hid_info(sc->hdev, "controller with MAC address %pMR already connected\n",
+					sc->mac_address);
+			} else {
+				/* Duplicate found, but on the same bus as the
+				 * original. Allow the connection in this case.
+				 */
+				ret = 0;
+			}
+
+			spin_unlock_irqrestore(&sony_dev_list_lock, flags);
+			return ret;
+		}
+	}
+
+	list_add(&(sc->list_node), &sony_device_list);
+	spin_unlock_irqrestore(&sony_dev_list_lock, flags);
+
+	return 0;
+}
+
+static void sony_remove_dev_list(struct sony_sc *sc)
+{
+	unsigned long flags;
+
+	if (sc->list_node.next) {
+		spin_lock_irqsave(&sony_dev_list_lock, flags);
+		list_del(&(sc->list_node));
+		spin_unlock_irqrestore(&sony_dev_list_lock, flags);
+	}
+}
+
+static int sony_get_bt_devaddr(struct sony_sc *sc)
+{
+	int ret, n;
+	unsigned int mac_addr[6];
+
+	/* HIDP stores the device MAC address as a string in the uniq field. */
+	ret = strlen(sc->hdev->uniq);
+	if (ret != 17)
+		return -EINVAL;
+
+	ret = sscanf(sc->hdev->uniq, "%02x:%02x:%02x:%02x:%02x:%02x",
+		&mac_addr[5], &mac_addr[4], &mac_addr[3], &mac_addr[2],
+		&mac_addr[1], &mac_addr[0]);
+
+	if (ret != 6)
+		return -EINVAL;
+
+	for (n = 0; n < 6; n++)
+		sc->mac_address[n] = (__u8)mac_addr[n];
+
+	return 0;
+}
+
+static int sony_check_add(struct sony_sc *sc)
+{
+	int n, ret;
+
+	if ((sc->quirks & DUALSHOCK4_CONTROLLER_BT) ||
+	    (sc->quirks & SIXAXIS_CONTROLLER_BT)) {
+		/* sony_get_bt_devaddr() attempts to parse the Bluetooth MAC
+		 * address from the uniq string where HIDP stores it.
+		 * As uniq cannot be guaranteed to be a MAC address in all cases
+		 * a failure of this function should not prevent the connection.
+		 */
+		if (sony_get_bt_devaddr(sc) < 0) {
+			hid_warn(sc->hdev, "UNIQ does not contain a MAC address; duplicate check skipped\n");
+			return 0;
+		}
+	} else if (sc->quirks & DUALSHOCK4_CONTROLLER_USB) {
+		__u8 buf[7];
+
+		/* The MAC address of a DS4 controller connected via USB can be
+		 * retrieved with feature report 0x81. The address begins at
+		 * offset 1.
+		 */
+		ret = hid_hw_raw_request(sc->hdev, 0x81, buf, sizeof(buf),
+				HID_FEATURE_REPORT, HID_REQ_GET_REPORT);
+
+		if (ret != 7) {
+			hid_err(sc->hdev, "failed to retrieve feature report 0x81 with the DualShock 4 MAC address\n");
+			return ret < 0 ? ret : -EINVAL;
+		}
+
+		memcpy(sc->mac_address, &buf[1], sizeof(sc->mac_address));
+	} else if (sc->quirks & SIXAXIS_CONTROLLER_USB) {
+		__u8 buf[18];
+
+		/* The MAC address of a Sixaxis controller connected via USB can
+		 * be retrieved with feature report 0xf2. The address begins at
+		 * offset 4.
+		 */
+		ret = hid_hw_raw_request(sc->hdev, 0xf2, buf, sizeof(buf),
+				HID_FEATURE_REPORT, HID_REQ_GET_REPORT);
+
+		if (ret != 18) {
+			hid_err(sc->hdev, "failed to retrieve feature report 0xf2 with the Sixaxis MAC address\n");
+			return ret < 0 ? ret : -EINVAL;
+		}
+
+		/* The Sixaxis device MAC in the report is in reverse order */
+		for (n = 0; n < 6; n++)
+			sc->mac_address[5-n] = buf[4+n];
+	} else {
+		return 0;
+	}
+
+	return sony_check_add_dev_list(sc);
+}
+
 static int sony_probe(struct hid_device *hdev, const struct hid_device_id *id)
 {
 	int ret;
@@ -1509,12 +1645,22 @@ static int sony_probe(struct hid_device *hdev, const struct hid_device_id *id)
 		return ret;
 	}
 
-	if (sc->quirks & SIXAXIS_CONTROLLER_USB) {
-		hdev->hid_output_raw_report = sixaxis_usb_output_raw_report;
-		ret = sixaxis_set_operational_usb(hdev);
-		INIT_WORK(&sc->state_worker, sixaxis_state_worker);
-	} else if (sc->quirks & SIXAXIS_CONTROLLER_BT) {
-		ret = sixaxis_set_operational_bt(hdev);
+	if (sc->quirks & SIXAXIS_CONTROLLER) {
+		if (sc->quirks & SIXAXIS_CONTROLLER_USB) {
+			hdev->hid_output_raw_report = sixaxis_usb_output_raw_report;
+			ret = sixaxis_set_operational_usb(hdev);
+		} else
+			ret = sixaxis_set_operational_bt(hdev);
+
+		if (ret < 0) {
+			hid_err(hdev, "failed to set the Sixaxis operational mode\n");
+			goto err_stop;
+		}
+
+		ret = sony_check_add(sc);
+		if (ret < 0)
+			goto err_stop;
+
 		INIT_WORK(&sc->state_worker, sixaxis_state_worker);
 	} else if (sc->quirks & DUALSHOCK4_CONTROLLER) {
 		if (sc->quirks & DUALSHOCK4_CONTROLLER_BT) {
@@ -1524,6 +1670,11 @@ static int sony_probe(struct hid_device *hdev, const struct hid_device_id *id)
 				goto err_stop;
 			}
 		}
+
+		ret = sony_check_add(sc);
+		if (ret < 0)
+			goto err_stop;
+
 		/* The Dualshock 4 touchpad supports 2 touches and has a
 		 * resolution of 1920x940.
 		 */
@@ -1588,6 +1739,8 @@ static void sony_remove(struct hid_device *hdev)
 		sony_battery_remove(sc);
 	}
 
+	sony_remove_dev_list(sc);
+
 	cancel_work_sync(&sc->state_worker);
 
 	hid_hw_stop(hdev);
-- 
1.8.5.3

Re: [PATCH 3/3] HID: sony: Prevent duplicate controller connections.

[David Herrmann]

Hi

On Tue, Feb 18, 2014 at 11:22 PM, Frank Praznik <frank.praznik@oh.rr.com> wrote:
> If a Sixaxis or Dualshock 4 controller is connected via USB while already
> connected via Bluetooth it will cause duplicate devices to be added to the
> input device list.
>
> To prevent this a global list of controllers and their MAC addresses is
> maintained and new controllers are checked against this list.  If a duplicate
> is found, the probe function with exit with -EEXIST.
>
> On USB the MAC is retrieved via a feature report.  On Bluetooth neither
> controller reports the MAC address in a feature report so the MAC is parsed from
> the uniq string.  As uniq cannot be guaranteed to be a MAC address in every case
> (uHID or the behavior of HIDP changing) a parsing failure will not prevent the
> connection.
>
> Signed-off-by: Frank Praznik <frank.praznik@oh.rr.com>
> ---
>  drivers/hid/hid-sony.c | 165 +++++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 159 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c
> index ad1cebd..c25f0d7 100644
> --- a/drivers/hid/hid-sony.c
> +++ b/drivers/hid/hid-sony.c
> @@ -713,8 +713,12 @@ static enum power_supply_property sony_battery_props[] = {
>         POWER_SUPPLY_PROP_STATUS,
>  };
>
> +static spinlock_t sony_dev_list_lock;
> +static LIST_HEAD(sony_device_list);

Please include <linux/list.h>

> +
>  struct sony_sc {
>         spinlock_t lock;
> +       struct list_head list_node;
>         struct hid_device *hdev;
>         struct led_classdev *leds[MAX_LEDS];
>         unsigned long quirks;
> @@ -726,6 +730,7 @@ struct sony_sc {
>         __u8 right;
>  #endif
>
> +       __u8 mac_address[6];
>         __u8 cable_state;
>         __u8 battery_charging;
>         __u8 battery_capacity;
> @@ -1473,6 +1478,137 @@ static int sony_register_touchpad(struct sony_sc *sc, int touch_count,
>         return 0;
>  }
>
> +/* If a controller is plugged in via USB while already connected via Bluetooth
> + * it will show up as two devices. A global list of connected controllers and
> + * their MAC addresses is maintained to ensure that a device is only connected
> + * once.
> + */

We usually use one of those comment-styles:

/*
 * some
 * comment
 */

/* some
 * comment */

..but I use my own style all the time, so who am I to judge.. You
might get screamed at in other subsystems, though.

> +static int sony_check_add_dev_list(struct sony_sc *sc)
> +{
> +       struct sony_sc *entry;
> +       struct list_head *pos;
> +       unsigned long flags;
> +       int ret;
> +
> +       spin_lock_irqsave(&sony_dev_list_lock, flags);
> +
> +       list_for_each(pos, &sony_device_list) {
> +               entry = list_entry(pos, struct sony_sc, list_node);

You can use "list_for_each_entry()" here.

> +               ret = memcmp(sc->mac_address, entry->mac_address,
> +                               sizeof(sc->mac_address));
> +               if (!ret) {
> +                       if (sc->hdev->bus != entry->hdev->bus) {
> +                               ret = -EEXIST;
> +                               hid_info(sc->hdev, "controller with MAC address %pMR already connected\n",
> +                                       sc->mac_address);
> +                       } else {
> +                               /* Duplicate found, but on the same bus as the
> +                                * original. Allow the connection in this case.
> +                                */
> +                               ret = 0;

Please drop that. Imagine some device uses BT-LE at some point and
thus is managed via uHID. If the device is connected via BT *and*
BT-LE, both will have BUS_BLUETOOTH set but are the same device. I
think the mac-comparison is enough. I don't know why a single device
would be connected twice and be managed by hid-sony..

> +                       }
> +
> +                       spin_unlock_irqrestore(&sony_dev_list_lock, flags);
> +                       return ret;

Usually we use:

                r = -ESOMETHING;
                goto unlock;
...
        r = 0;

unlock:
        spin_unlock_irqrestore(&sony_dev_list_lock, flags);
        return r;

This way the locking is symmetric and easier to review.

> +               }
> +       }
> +
> +       list_add(&(sc->list_node), &sony_device_list);
> +       spin_unlock_irqrestore(&sony_dev_list_lock, flags);
> +
> +       return 0;
> +}
> +
> +static void sony_remove_dev_list(struct sony_sc *sc)
> +{
> +       unsigned long flags;
> +
> +       if (sc->list_node.next) {
> +               spin_lock_irqsave(&sony_dev_list_lock, flags);
> +               list_del(&(sc->list_node));
> +               spin_unlock_irqrestore(&sony_dev_list_lock, flags);
> +       }
> +}
> +
> +static int sony_get_bt_devaddr(struct sony_sc *sc)
> +{
> +       int ret, n;
> +       unsigned int mac_addr[6];
> +
> +       /* HIDP stores the device MAC address as a string in the uniq field. */
> +       ret = strlen(sc->hdev->uniq);

Are you sure "uniq" cannot be NULL?

> +       if (ret != 17)
> +               return -EINVAL;
> +
> +       ret = sscanf(sc->hdev->uniq, "%02x:%02x:%02x:%02x:%02x:%02x",
> +               &mac_addr[5], &mac_addr[4], &mac_addr[3], &mac_addr[2],
> +               &mac_addr[1], &mac_addr[0]);

%02hhx so you can avoid the temporary copy?

> +
> +       if (ret != 6)
> +               return -EINVAL;
> +
> +       for (n = 0; n < 6; n++)
> +               sc->mac_address[n] = (__u8)mac_addr[n];
> +
> +       return 0;
> +}
> +
> +static int sony_check_add(struct sony_sc *sc)
> +{
> +       int n, ret;
> +
> +       if ((sc->quirks & DUALSHOCK4_CONTROLLER_BT) ||
> +           (sc->quirks & SIXAXIS_CONTROLLER_BT)) {
> +               /* sony_get_bt_devaddr() attempts to parse the Bluetooth MAC
> +                * address from the uniq string where HIDP stores it.
> +                * As uniq cannot be guaranteed to be a MAC address in all cases
> +                * a failure of this function should not prevent the connection.
> +                */
> +               if (sony_get_bt_devaddr(sc) < 0) {
> +                       hid_warn(sc->hdev, "UNIQ does not contain a MAC address; duplicate check skipped\n");
> +                       return 0;
> +               }
> +       } else if (sc->quirks & DUALSHOCK4_CONTROLLER_USB) {
> +               __u8 buf[7];
> +
> +               /* The MAC address of a DS4 controller connected via USB can be
> +                * retrieved with feature report 0x81. The address begins at
> +                * offset 1.
> +                */
> +               ret = hid_hw_raw_request(sc->hdev, 0x81, buf, sizeof(buf),
> +                               HID_FEATURE_REPORT, HID_REQ_GET_REPORT);
> +
> +               if (ret != 7) {
> +                       hid_err(sc->hdev, "failed to retrieve feature report 0x81 with the DualShock 4 MAC address\n");
> +                       return ret < 0 ? ret : -EINVAL;
> +               }
> +
> +               memcpy(sc->mac_address, &buf[1], sizeof(sc->mac_address));
> +       } else if (sc->quirks & SIXAXIS_CONTROLLER_USB) {
> +               __u8 buf[18];
> +
> +               /* The MAC address of a Sixaxis controller connected via USB can
> +                * be retrieved with feature report 0xf2. The address begins at
> +                * offset 4.
> +                */
> +               ret = hid_hw_raw_request(sc->hdev, 0xf2, buf, sizeof(buf),
> +                               HID_FEATURE_REPORT, HID_REQ_GET_REPORT);
> +
> +               if (ret != 18) {
> +                       hid_err(sc->hdev, "failed to retrieve feature report 0xf2 with the Sixaxis MAC address\n");
> +                       return ret < 0 ? ret : -EINVAL;
> +               }
> +
> +               /* The Sixaxis device MAC in the report is in reverse order */

"reverse" sounds weird here.. addresses are usually stored in
network-byte-order (BigEndian), so lets be clear here and say the
input is little-endian so you have to swap it. Or am I wrong?

> +               for (n = 0; n < 6; n++)
> +                       sc->mac_address[5-n] = buf[4+n];
> +       } else {
> +               return 0;
> +       }
> +
> +       return sony_check_add_dev_list(sc);
> +}
> +
>  static int sony_probe(struct hid_device *hdev, const struct hid_device_id *id)
>  {
>         int ret;
> @@ -1509,12 +1645,22 @@ static int sony_probe(struct hid_device *hdev, const struct hid_device_id *id)
>                 return ret;
>         }
>
> -       if (sc->quirks & SIXAXIS_CONTROLLER_USB) {
> -               hdev->hid_output_raw_report = sixaxis_usb_output_raw_report;
> -               ret = sixaxis_set_operational_usb(hdev);
> -               INIT_WORK(&sc->state_worker, sixaxis_state_worker);
> -       } else if (sc->quirks & SIXAXIS_CONTROLLER_BT) {
> -               ret = sixaxis_set_operational_bt(hdev);
> +       if (sc->quirks & SIXAXIS_CONTROLLER) {
> +               if (sc->quirks & SIXAXIS_CONTROLLER_USB) {
> +                       hdev->hid_output_raw_report = sixaxis_usb_output_raw_report;
> +                       ret = sixaxis_set_operational_usb(hdev);
> +               } else
> +                       ret = sixaxis_set_operational_bt(hdev);
> +
> +               if (ret < 0) {
> +                       hid_err(hdev, "failed to set the Sixaxis operational mode\n");
> +                       goto err_stop;
> +               }
> +
> +               ret = sony_check_add(sc);
> +               if (ret < 0)
> +                       goto err_stop;
> +

You check the quirks in sony_check_add() again, so why not leave all
this code as is and call sony_check_add() below for *all* devices?

Could you add a line to net/hidp/core.c where we write UNIQ that some
devices depend on this? Other than that, the patch looks good.

Thanks
David

>                 INIT_WORK(&sc->state_worker, sixaxis_state_worker);
>         } else if (sc->quirks & DUALSHOCK4_CONTROLLER) {
>                 if (sc->quirks & DUALSHOCK4_CONTROLLER_BT) {
> @@ -1524,6 +1670,11 @@ static int sony_probe(struct hid_device *hdev, const struct hid_device_id *id)
>                                 goto err_stop;
>                         }
>                 }
> +
> +               ret = sony_check_add(sc);
> +               if (ret < 0)
> +                       goto err_stop;
> +
>                 /* The Dualshock 4 touchpad supports 2 touches and has a
>                  * resolution of 1920x940.
>                  */
> @@ -1588,6 +1739,8 @@ static void sony_remove(struct hid_device *hdev)
>                 sony_battery_remove(sc);
>         }
>
> +       sony_remove_dev_list(sc);
> +
>         cancel_work_sync(&sc->state_worker);
>
>         hid_hw_stop(hdev);
> --
> 1.8.5.3
>