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

Hi

On Tue, Feb 18, 2014 at 11:22 PM, Frank Praznik [off-list ref] wrote:

Please include <linux/list.h>

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?

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