Re: [PATCH] Input: xpad - power off wireless 360 controllers on suspend

On 07/27/2016 02:39 PM, Dmitry Torokhov wrote:

On Wed, Jul 27, 2016 at 02:32:22PM -0700, Dmitry Torokhov wrote:

quoted

On Mon, Jul 25, 2016 at 10:35:08PM -0700, Cameron Gutman wrote:

quoted

When the USB wireless adapter is suspended, the controllers
lose their connection. This causes them to start flashing
their LED rings and searching for the wireless adapter
again, wasting the controller's battery power.

Instead, we will tell the controllers to power down when
we suspend. This mirrors the behavior of the controllers
when connected to the console itself and how the official
Xbox One wireless adapter behaves on Windows.

Signed-off-by: Cameron Gutman <redacted>
---
This patch is independent of the other xpad patch [0] that I
submitted (and decided to wait on). It applies against
unmodified xpad.c in master.

[0] http://www.spinics.net/lists/linux-input/msg46062.html
---
 drivers/input/joystick/xpad.c | 43 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 43 insertions(+)

diff --git a/drivers/input/joystick/xpad.c b/drivers/input/joystick/xpad.c
index a529a45..3408019 100644
--- a/drivers/input/joystick/xpad.c
+++ b/drivers/input/joystick/xpad.c

@@ -115,6 +115,10 @@ static bool sticks_to_null;
 module_param(sticks_to_null, bool, S_IRUGO);
 MODULE_PARM_DESC(sticks_to_null, "Do not map sticks at all for unknown pads");
 
+static bool disable_auto_poweroff;
+module_param(disable_auto_poweroff, bool, S_IRUGO);
+MODULE_PARM_DESC(disable_auto_poweroff, "Do not power off wireless controllers on suspend");

Why negating? Why not do

static bool xpad_auto_poweroff = true;

?

(No need to resubmit if agree/disagree, I can fix up on my side).

Yeah, sounds fine to make it default Y and get rid of the negation.

I'm fine with it if you want to make that change and apply it. See my
comments below about the other issues.

By the way, I think we can allow root writing to it, there is nothing
that stops us from changing behavior at runtime.

Yep, I was following the convention of the existing module params. They
can probably all be changed to allow root writing.

quoted

quoted

+
 static const struct xpad_device {
 	u16 idVendor;
 	u16 idProduct;

@@ -1248,6 +1252,36 @@ static void xpad_stop_input(struct usb_xpad *xpad)
 	usb_kill_urb(xpad->irq_in);
 }
 
+static void xpad360w_poweroff_controller(struct usb_xpad *xpad)
+{
+	unsigned long flags;
+	struct xpad_output_packet *packet =
+			&xpad->out_packets[XPAD_OUT_CMD_IDX];
+
+	spin_lock_irqsave(&xpad->odata_lock, flags);
+
+	packet->data[0] = 0x00;
+	packet->data[1] = 0x00;
+	packet->data[2] = 0x08;
+	packet->data[3] = 0xC0;
+	packet->data[4] = 0x00;
+	packet->data[5] = 0x00;
+	packet->data[6] = 0x00;
+	packet->data[7] = 0x00;
+	packet->data[8] = 0x00;
+	packet->data[9] = 0x00;
+	packet->data[10] = 0x00;
+	packet->data[11] = 0x00;
+	packet->len = 12;
+	packet->pending = true;

I wonder of we don't want to convert commands to something like that:

	static const u8 power_off_cmd[] = {
		0x00, 0x00, 0x08, 0xc0, 0x00, 0x00, 0x00, 0x00,
		0x00, 0x00, 0x00, 0x00,
	};

	...

	memcpy(packet->data, power_off_cmd, sizeof(power_off_cmd));
	// if we need to change something
	// packet->data[3] += command;
	// packet->data[6] = id;
	packet->len = sizeof(power_off_cmd);
	...

This should be a separate patch though.

I was actually planning to submit a series to clean up all of those
unnecessary XTYPE_UNKNOWN checks scattered all over and add some
WARN_ONs in pad-specific code to make sure we never get there with
the wrong pad type. I can add a patch to convert the commands over to
the nicer format, and another to loosen the permissions on those
module params to allow root writing.

I would also like to track down this URB hang I'm sometimes seeing on
resume. I'll probably do that before starting the cleanup work.

quoted

Thanks.

-- 
Dmitry