[PATCH V3] input: Fix USB autosuspend on bcm5974

[PATCH V3] input: Fix USB autosuspend on bcm5974

[Matthew Garrett]

The bcm5974 code takes a USB autosuspend reference on device open and
releases it on device close. This means that the hardware won't sleep when
anything holds it open. This is sensible for input devices that don't
support remote wakeups on normal use (like most mice), but this hardware
trigger wakeups on touch and so can suspend transparently to the user. Doing
so allows the USB host controller to sleep when the machine is idle, giving
measurable power savings.

Signed-off-by: Matthew Garrett <mjg@redhat.com>
---

Correctly flag needs_remote_wake.

 drivers/input/mouse/bcm5974.c |   10 ++++++----
 1 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/input/mouse/bcm5974.c b/drivers/input/mouse/bcm5974.c
index 5ec617e..4ca5523 100644
--- a/drivers/input/mouse/bcm5974.c
+++ b/drivers/input/mouse/bcm5974.c
@@ -631,6 +631,7 @@ static void bcm5974_irq_button(struct urb *urb)
 
 	switch (urb->status) {
 	case 0:
+		usb_mark_last_busy(dev->udev);
 		break;
 	case -EOVERFLOW:
 	case -ECONNRESET:
@@ -660,6 +661,7 @@ static void bcm5974_irq_trackpad(struct urb *urb)
 
 	switch (urb->status) {
 	case 0:
+		usb_mark_last_busy(dev->udev);
 		break;
 	case -EOVERFLOW:
 	case -ECONNRESET:
@@ -764,8 +766,7 @@ static int bcm5974_open(struct input_dev *input)
 
 	mutex_unlock(&dev->pm_mutex);
 
-	if (error)
-		usb_autopm_put_interface(dev->intf);
+	usb_autopm_put_interface(dev->intf);
 
 	return error;
 }
@@ -780,8 +781,6 @@ static void bcm5974_close(struct input_dev *input)
 	dev->opened = 0;
 
 	mutex_unlock(&dev->pm_mutex);
-
-	usb_autopm_put_interface(dev->intf);
 }
 
 static int bcm5974_suspend(struct usb_interface *iface, pm_message_t message)
@@ -870,6 +869,9 @@ static int bcm5974_probe(struct usb_interface *iface,
 			 dev->tp_data, dev->cfg.tp_datalen,
 			 bcm5974_irq_trackpad, dev, 1);
 
+	/* Required for autosuspend */
+	iface->needs_remote_wakeup = 1;
+
 	/* create bcm5974 device */
 	usb_make_path(udev, dev->phys, sizeof(dev->phys));
 	strlcat(dev->phys, "/input0", sizeof(dev->phys));
-- 
1.7.6.4

Re: [PATCH V3] input: Fix USB autosuspend on bcm5974

[Henrik Rydberg]

Hi Matthew,

> The bcm5974 code takes a USB autosuspend reference on device open and
> releases it on device close. This means that the hardware won't sleep when
> anything holds it open. This is sensible for input devices that don't
> support remote wakeups on normal use (like most mice), but this hardware
> trigger wakeups on touch and so can suspend transparently to the user. Doing
> so allows the USB host controller to sleep when the machine is idle, giving
> measurable power savings.
> 
> Signed-off-by: Matthew Garrett <mjg@redhat.com>
> ---
> 
> Correctly flag needs_remote_wake.

With this patch, autosuspend seems to work correctly after boot, while
keeping a device file open. When all device files are closed, the
looping suspend/resume behavior is still there. Moreover, when coming
back from a user suspend, rpm_suspend fails with EBUSY on my machine,
and the device will not autosuspend thereafter.

IOW, getting closer, but still no cigar, sorry.

Cheers,
Henrik

Re: [PATCH V3] input: Fix USB autosuspend on bcm5974

[Matthew Garrett]

On Mon, Oct 10, 2011 at 11:05:34PM +0200, Henrik Rydberg wrote:

> With this patch, autosuspend seems to work correctly after boot, while
> keeping a device file open. When all device files are closed, the
> looping suspend/resume behavior is still there. Moreover, when coming
> back from a user suspend, rpm_suspend fails with EBUSY on my machine,
> and the device will not autosuspend thereafter.

Is this with the patch I sent for usbhid earlier applied? Right now the 
keyboard will fail to suspend after system suspend, which will keep the 
entire composite device awake. I'm looking into your other issue now.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH V3] input: Fix USB autosuspend on bcm5974

[Henrik Rydberg]

On Mon, Oct 10, 2011 at 10:12:31PM +0100, Matthew Garrett wrote:
> On Mon, Oct 10, 2011 at 11:05:34PM +0200, Henrik Rydberg wrote:
> 
> > With this patch, autosuspend seems to work correctly after boot, while
> > keeping a device file open. When all device files are closed, the
> > looping suspend/resume behavior is still there. Moreover, when coming
> > back from a user suspend, rpm_suspend fails with EBUSY on my machine,
> > and the device will not autosuspend thereafter.
> 
> Is this with the patch I sent for usbhid earlier applied? Right now the 
> keyboard will fail to suspend after system suspend, which will keep the 
> entire composite device awake. I'm looking into your other issue now.

Ahh, yes, the suspend failure was due to the keyboard interface,
thanks. The loop-on-closed is still present, though (as expected,
since there is no usb_mark_last_busy() call).

Since the autosuspend removes the special handling of an open device,
we could simply the patch further, removing open/close altogether. The
patch below seems to work for all corner cases tested.

Cheers,
Henrik

>From 8a1e46bf45c6d43070447472757519a909fcaeb5 Mon Sep 17 00:00:00 2001
From: Matthew Garrett <mjg@redhat.com>
Date: Mon, 10 Oct 2011 11:47:10 -0400
Subject: [PATCH v3] input: Fix USB autosuspend on bcm5974

The bcm5974 code takes a USB autosuspend reference on device open and
releases it on device close. This means that the hardware won't sleep when
anything holds it open. This is sensible for input devices that don't
support remote wakeups on normal use (like most mice), but this hardware
trigger wakeups on touch and so can suspend transparently to the user. Doing
so allows the USB host controller to sleep when the machine is idle, giving
measurable power savings.

[rydberg@euromail.se: removal of open/close]
Signed-off-by: Matthew Garrett <mjg@redhat.com>
Signed-off-by: Henrik Rydberg <rydberg@euromail.se>
---
 drivers/input/mouse/bcm5974.c |   74 +++++++++++------------------------------
 1 files changed, 20 insertions(+), 54 deletions(-)

diff --git a/drivers/input/mouse/bcm5974.c b/drivers/input/mouse/bcm5974.c
index 5ec617e..178be2b 100644
--- a/drivers/input/mouse/bcm5974.c
+++ b/drivers/input/mouse/bcm5974.c
@@ -221,7 +221,7 @@ struct bcm5974 {
 	struct input_dev *input;	/* input dev */
 	struct bcm5974_config cfg;	/* device configuration */
 	struct mutex pm_mutex;		/* serialize access to open/suspend */
-	int opened;			/* 1: opened, 0: closed */
+	int started;			/* 1: started, 0: paused */
 	struct urb *bt_urb;		/* button usb request block */
 	struct bt_data *bt_data;	/* button transferred data */
 	struct urb *tp_urb;		/* trackpad usb request block */
@@ -631,6 +631,7 @@ static void bcm5974_irq_button(struct urb *urb)
 
 	switch (urb->status) {
 	case 0:
+		usb_mark_last_busy(dev->udev);
 		break;
 	case -EOVERFLOW:
 	case -ECONNRESET:
@@ -660,6 +661,7 @@ static void bcm5974_irq_trackpad(struct urb *urb)
 
 	switch (urb->status) {
 	case 0:
+		usb_mark_last_busy(dev->udev);
 		break;
 	case -EOVERFLOW:
 	case -ECONNRESET:
@@ -708,6 +710,9 @@ static int bcm5974_start_traffic(struct bcm5974 *dev)
 {
 	int error;
 
+	if (dev->started)
+		return 0;
+
 	error = bcm5974_wellspring_mode(dev, true);
 	if (error) {
 		dprintk(1, "bcm5974: mode switch failed\n");
@@ -722,6 +727,7 @@ static int bcm5974_start_traffic(struct bcm5974 *dev)
 	if (error)
 		goto err_kill_bt;
 
+	dev->started = 1;
 	return 0;
 
 err_kill_bt:
@@ -734,54 +740,12 @@ err_out:
 
 static void bcm5974_pause_traffic(struct bcm5974 *dev)
 {
+	if (!dev->started)
+		return;
 	usb_kill_urb(dev->tp_urb);
 	usb_kill_urb(dev->bt_urb);
 	bcm5974_wellspring_mode(dev, false);
-}
-
-/*
- * The code below implements open/close and manual suspend/resume.
- * All functions may be called in random order.
- *
- * Opening a suspended device fails with EACCES - permission denied.
- *
- * Failing a resume leaves the device resumed but closed.
- */
-static int bcm5974_open(struct input_dev *input)
-{
-	struct bcm5974 *dev = input_get_drvdata(input);
-	int error;
-
-	error = usb_autopm_get_interface(dev->intf);
-	if (error)
-		return error;
-
-	mutex_lock(&dev->pm_mutex);
-
-	error = bcm5974_start_traffic(dev);
-	if (!error)
-		dev->opened = 1;
-
-	mutex_unlock(&dev->pm_mutex);
-
-	if (error)
-		usb_autopm_put_interface(dev->intf);
-
-	return error;
-}
-
-static void bcm5974_close(struct input_dev *input)
-{
-	struct bcm5974 *dev = input_get_drvdata(input);
-
-	mutex_lock(&dev->pm_mutex);
-
-	bcm5974_pause_traffic(dev);
-	dev->opened = 0;
-
-	mutex_unlock(&dev->pm_mutex);
-
-	usb_autopm_put_interface(dev->intf);
+	dev->started = 0;
 }
 
 static int bcm5974_suspend(struct usb_interface *iface, pm_message_t message)
@@ -790,8 +754,7 @@ static int bcm5974_suspend(struct usb_interface *iface, pm_message_t message)
 
 	mutex_lock(&dev->pm_mutex);
 
-	if (dev->opened)
-		bcm5974_pause_traffic(dev);
+	bcm5974_pause_traffic(dev);
 
 	mutex_unlock(&dev->pm_mutex);
 
@@ -801,12 +764,11 @@ static int bcm5974_suspend(struct usb_interface *iface, pm_message_t message)
 static int bcm5974_resume(struct usb_interface *iface)
 {
 	struct bcm5974 *dev = usb_get_intfdata(iface);
-	int error = 0;
+	int error;
 
 	mutex_lock(&dev->pm_mutex);
 
-	if (dev->opened)
-		error = bcm5974_start_traffic(dev);
+	error = bcm5974_start_traffic(dev);
 
 	mutex_unlock(&dev->pm_mutex);
 
@@ -870,6 +832,9 @@ static int bcm5974_probe(struct usb_interface *iface,
 			 dev->tp_data, dev->cfg.tp_datalen,
 			 bcm5974_irq_trackpad, dev, 1);
 
+	/* Required for autosuspend */
+	iface->needs_remote_wakeup = 1;
+
 	/* create bcm5974 device */
 	usb_make_path(udev, dev->phys, sizeof(dev->phys));
 	strlcat(dev->phys, "/input0", sizeof(dev->phys));
@@ -883,9 +848,6 @@ static int bcm5974_probe(struct usb_interface *iface,
 
 	input_set_drvdata(input_dev, dev);
 
-	input_dev->open = bcm5974_open;
-	input_dev->close = bcm5974_close;
-
 	setup_events_to_report(input_dev, cfg);
 
 	error = input_register_device(dev->input);
@@ -895,6 +857,10 @@ static int bcm5974_probe(struct usb_interface *iface,
 	/* save our data pointer in this interface device */
 	usb_set_intfdata(iface, dev);
 
+	error = bcm5974_start_traffic(dev);
+	if (error)
+		goto err_free_buffer;
+
 	return 0;
 
 err_free_buffer:
-- 
1.7.7

Re: [PATCH V3] input: Fix USB autosuspend on bcm5974

[Matthew Garrett]

On Tue, Oct 11, 2011 at 12:44:31AM +0200, Henrik Rydberg wrote:
> On Mon, Oct 10, 2011 at 10:12:31PM +0100, Matthew Garrett wrote:
> > Is this with the patch I sent for usbhid earlier applied? Right now the 
> > keyboard will fail to suspend after system suspend, which will keep the 
> > entire composite device awake. I'm looking into your other issue now.
> 
> Ahh, yes, the suspend failure was due to the keyboard interface,
> thanks. The loop-on-closed is still present, though (as expected,
> since there is no usb_mark_last_busy() call).

Ah, right, sorry - I wasn't trying to use the device after it had been 
closed. In an ideal world we'd really want to be able to indicate that 
we don't want a remote wakeup in that situation. Otherwise, the patch 
looks good. Thank you!

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH V3] input: Fix USB autosuspend on bcm5974

[Oliver Neukum]

Am Dienstag, 11. Oktober 2011, 02:11:03 schrieb Matthew Garrett:

> Ah, right, sorry - I wasn't trying to use the device after it had been 
> closed. In an ideal world we'd really want to be able to indicate that 
> we don't want a remote wakeup in that situation. Otherwise, the patch 
> looks good. Thank you!

It looks like we had exactly that capability until
48826626263d4a61d06fd8c5805da31f925aefa0
removed it.

	Regards
		Oliver

Re: [PATCH V3] input: Fix USB autosuspend on bcm5974

[Henrik Rydberg]

On Tue, Oct 11, 2011 at 06:10:12AM +0200, Oliver Neukum wrote:
> Am Dienstag, 11. Oktober 2011, 02:11:03 schrieb Matthew Garrett:
> 
> > Ah, right, sorry - I wasn't trying to use the device after it had been 
> > closed. In an ideal world we'd really want to be able to indicate that 
> > we don't want a remote wakeup in that situation. Otherwise, the patch 
> > looks good. Thank you!
> 
> It looks like we had exactly that capability until
> 48826626263d4a61d06fd8c5805da31f925aefa0
> removed it.

Hm. Throwing in another related question below.

Problem: the hid keyboard driver refuses to suspend while a key is
pressed, which is fine per se, but there is no indication presented to the power layer,
resulting in a failed suspend and (due to complex reasons it seems) disabled autosuspend.

Question:  If the hid interrupt handler were to actually prevent
suspend during keypress, what would be the natural way to do it?

Henrik

Re: [PATCH V3] input: Fix USB autosuspend on bcm5974

[Oliver Neukum]

Am Dienstag, 11. Oktober 2011, 13:04:21 schrieb Henrik Rydberg:
> On Tue, Oct 11, 2011 at 06:10:12AM +0200, Oliver Neukum wrote:
> > Am Dienstag, 11. Oktober 2011, 02:11:03 schrieb Matthew Garrett:
> > 
> > > Ah, right, sorry - I wasn't trying to use the device after it had been 
> > > closed. In an ideal world we'd really want to be able to indicate that 
> > > we don't want a remote wakeup in that situation. Otherwise, the patch 
> > > looks good. Thank you!
> > 
> > It looks like we had exactly that capability until
> > 48826626263d4a61d06fd8c5805da31f925aefa0
> > removed it.
> 
> Hm. Throwing in another related question below.
> 
> Problem: the hid keyboard driver refuses to suspend while a key is
> pressed, which is fine per se, but there is no indication presented to the power layer,
> resulting in a failed suspend and (due to complex reasons it seems) disabled autosuspend.

That is a bug. Autosuspend at a later date should happen.

> Question:  If the hid interrupt handler were to actually prevent
> suspend during keypress, what would be the natural way to do it?

Firstly we must prevent autosuspension when a key is pressed
because most keyboards do not generate a wakeup upon key release
which is then lost.

Secondly a key pres triggers the mark_busy. We could do an async
get on every key press and an async put on every key release. But you
cannot beat the principal race here because the operation happen
in interrupt and must therefore be async. You must be prepared for
the window to be hit and return an error code.

	Regards
		Oliver

Re: [PATCH V3] input: Fix USB autosuspend on bcm5974

[Matthew Garrett]

On Tue, Oct 11, 2011 at 01:09:21PM +0200, Oliver Neukum wrote:
> Am Dienstag, 11. Oktober 2011, 13:04:21 schrieb Henrik Rydberg:
> > Problem: the hid keyboard driver refuses to suspend while a key is
> > pressed, which is fine per se, but there is no indication presented to the power layer,
> > resulting in a failed suspend and (due to complex reasons it seems) disabled autosuspend.
> 
> That is a bug. Autosuspend at a later date should happen.

Yeah, there's definitely something broken here. If I hold down a key on 
a USB keyboard for more than two seconds then it attempts to suspend but 
fails. Releasing the key triggers another usb_mark_busy() but the core 
never tries to suspend it again. What /does/ trigger a second attempt at 
suspend is toggling an LED on and then off.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH V3] input: Fix USB autosuspend on bcm5974

[Oliver Neukum]

Am Dienstag, 11. Oktober 2011, 22:42:50 schrieb Matthew Garrett:
> On Tue, Oct 11, 2011 at 01:09:21PM +0200, Oliver Neukum wrote:
> > Am Dienstag, 11. Oktober 2011, 13:04:21 schrieb Henrik Rydberg:
> > > Problem: the hid keyboard driver refuses to suspend while a key is
> > > pressed, which is fine per se, but there is no indication presented to the power layer,
> > > resulting in a failed suspend and (due to complex reasons it seems) disabled autosuspend.
> > 
> > That is a bug. Autosuspend at a later date should happen.
> 
> Yeah, there's definitely something broken here. If I hold down a key on 
> a USB keyboard for more than two seconds then it attempts to suspend but 
> fails. Releasing the key triggers another usb_mark_busy() but the core 
> never tries to suspend it again. What /does/ trigger a second attempt at 
> suspend is toggling an LED on and then off.

It seems to me that rpm_suspend() in drivers/base/power has already called
pm_runtime_cancel_pending(dev) unconditionally before calling rpm_callback()
And afterwards the timer is not restarted.

	Regards
		Oliver

Re: [PATCH V3] input: Fix USB autosuspend on bcm5974

[Alan Stern]

On Wed, 12 Oct 2011, Oliver Neukum wrote:

> Am Dienstag, 11. Oktober 2011, 22:42:50 schrieb Matthew Garrett:
> > On Tue, Oct 11, 2011 at 01:09:21PM +0200, Oliver Neukum wrote:
> > > Am Dienstag, 11. Oktober 2011, 13:04:21 schrieb Henrik Rydberg:
> > > > Problem: the hid keyboard driver refuses to suspend while a key is
> > > > pressed, which is fine per se, but there is no indication presented to the power layer,
> > > > resulting in a failed suspend and (due to complex reasons it seems) disabled autosuspend.
> > > 
> > > That is a bug. Autosuspend at a later date should happen.
> > 
> > Yeah, there's definitely something broken here. If I hold down a key on 
> > a USB keyboard for more than two seconds then it attempts to suspend but 
> > fails. Releasing the key triggers another usb_mark_busy() but the core 
> > never tries to suspend it again. What /does/ trigger a second attempt at 
> > suspend is toggling an LED on and then off.
> 
> It seems to me that rpm_suspend() in drivers/base/power has already called
> pm_runtime_cancel_pending(dev) unconditionally before calling rpm_callback()
> And afterwards the timer is not restarted.

I admit to not being very familiar with the way usbhid handles 
autosuspend.  However in general, if a suspend fails because the driver 
is busy, then when the driver stops being busy it should initiate the 
next round of autosuspend.  In this case, when the key release has been 
fully processed, the driver should call usb_put_interface_async() or 
something similar, which would restart the timer.

Alan Stern

Re: [PATCH V3] input: Fix USB autosuspend on bcm5974

[Oliver Neukum]

Am Mittwoch, 12. Oktober 2011, 16:33:13 schrieb Alan Stern:

> I admit to not being very familiar with the way usbhid handles 
> autosuspend.  However in general, if a suspend fails because the driver 
> is busy, then when the driver stops being busy it should initiate the 
> next round of autosuspend.  In this case, when the key release has been 
> fully processed, the driver should call usb_put_interface_async() or 
> something similar, which would restart the timer.

I see. I can do this although it is not very elegant, because
the pm counters at this point ought to be 0 already.
Arguably a driver is the wrong layer to do this.

	Regards
		Oliver

Re: [PATCH V3] input: Fix USB autosuspend on bcm5974

[Alan Stern]

On Wed, 12 Oct 2011, Oliver Neukum wrote:

> Am Mittwoch, 12. Oktober 2011, 16:33:13 schrieb Alan Stern:
> 
> > I admit to not being very familiar with the way usbhid handles 
> > autosuspend.  However in general, if a suspend fails because the driver 
> > is busy, then when the driver stops being busy it should initiate the 
> > next round of autosuspend.  In this case, when the key release has been 
> > fully processed, the driver should call usb_put_interface_async() or 
> > something similar, which would restart the timer.
> 
> I see. I can do this although it is not very elegant, because
> the pm counters at this point ought to be 0 already.

Well, if the usage counter is 0 then you wouldn't want to call
usb_put_interface_async().  On the other hand, if the usage counter is
0 then by definition the device isn't in use, so you shouldn't need to
fail an autosuspend request -- not unless it races with an I/O event.

To anticipate your next question, how should such a race be hanlded?
Here's one way:

	I/O event occurs; interrupt handler is called.

	Handler sets a device_busy flag.

					Autosuspend attempt occurs 
					because the usage counter is 0.

					Suspend routine returns -EBUSY
					because device_busy is set.

	Handler calls usb_get_interface_async().

	The device is active, so the handler goes on to process the 
	event.

	Handler calls usb_put_interface_async().

> Arguably a driver is the wrong layer to do this.

Are you suggesting the usb_runtime_suspend() should call 
usb_mark_last_busy() and pm_runtime_autosuspend() if usb_suspend_both() 
returns -EBUSY or -EAGAIN?

This would allow a driver to block autosuspend attempts while leaving 
the usage counter set to 0.

Alan Stern

Re: [PATCH V3] input: Fix USB autosuspend on bcm5974

[Matthew Garrett]

On Wed, Oct 12, 2011 at 11:28:06AM -0400, Alan Stern wrote:

> Well, if the usage counter is 0 then you wouldn't want to call
> usb_put_interface_async().  On the other hand, if the usage counter is
> 0 then by definition the device isn't in use, so you shouldn't need to
> fail an autosuspend request -- not unless it races with an I/O event.

Yeah. Plausibly the model we have in hid is wrong at the moment - rather 
than set flags that block suspend, it might make more sense to take 
references. It'd complicate things a little, in that we'd have to take 
more care in terms of keeping track of the state.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH V3] input: Fix USB autosuspend on bcm5974

[Oliver Neukum]

Am Mittwoch, 12. Oktober 2011, 18:16:51 schrieb Matthew Garrett:
> On Wed, Oct 12, 2011 at 11:28:06AM -0400, Alan Stern wrote:
> 
> > Well, if the usage counter is 0 then you wouldn't want to call
> > usb_put_interface_async().  On the other hand, if the usage counter is
> > 0 then by definition the device isn't in use, so you shouldn't need to
> > fail an autosuspend request -- not unless it races with an I/O event.
> 
> Yeah. Plausibly the model we have in hid is wrong at the moment - rather 
> than set flags that block suspend, it might make more sense to take 
> references. It'd complicate things a little, in that we'd have to take 
> more care in terms of keeping track of the state.

I am afraid references are very hard to take there because it needs to be done
in interrupt.

	Regards
		Oliver

Re: [PATCH V3] input: Fix USB autosuspend on bcm5974

[Matthew Garrett]

On Wed, Oct 12, 2011 at 06:56:32PM +0200, Oliver Neukum wrote:
> Am Mittwoch, 12. Oktober 2011, 18:16:51 schrieb Matthew Garrett:
> > Yeah. Plausibly the model we have in hid is wrong at the moment - rather 
> > than set flags that block suspend, it might make more sense to take 
> > references. It'd complicate things a little, in that we'd have to take 
> > more care in terms of keeping track of the state.
> 
> I am afraid references are very hard to take there because it needs to be done
> in interrupt.

True. That makes things rather more awkward.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH V3] input: Fix USB autosuspend on bcm5974

[Alan Stern]

On Wed, 12 Oct 2011, Matthew Garrett wrote:

> On Wed, Oct 12, 2011 at 06:56:32PM +0200, Oliver Neukum wrote:
> > Am Mittwoch, 12. Oktober 2011, 18:16:51 schrieb Matthew Garrett:
> > > Yeah. Plausibly the model we have in hid is wrong at the moment - rather 
> > > than set flags that block suspend, it might make more sense to take 
> > > references. It'd complicate things a little, in that we'd have to take 
> > > more care in terms of keeping track of the state.
> > 
> > I am afraid references are very hard to take there because it needs to be done
> > in interrupt.
> 
> True. That makes things rather more awkward.

What about the second idea I mentioned?  If autosuspend fails with
-EBUSY or -EAGAIN, usbcore could schedule another autosuspend attempt.

Alan Stern

Re: [PATCH V3] input: Fix USB autosuspend on bcm5974

[Matthew Garrett]

On Wed, Oct 12, 2011 at 01:24:42PM -0400, Alan Stern wrote:
> On Wed, 12 Oct 2011, Matthew Garrett wrote:
> > True. That makes things rather more awkward.
> 
> What about the second idea I mentioned?  If autosuspend fails with
> -EBUSY or -EAGAIN, usbcore could schedule another autosuspend attempt.

Yeah, that sounds better.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH V3] input: Fix USB autosuspend on bcm5974

[Alan Stern]

On Wed, 12 Oct 2011, Matthew Garrett wrote:

> On Wed, Oct 12, 2011 at 01:24:42PM -0400, Alan Stern wrote:
> > On Wed, 12 Oct 2011, Matthew Garrett wrote:
> > > True. That makes things rather more awkward.
> > 
> > What about the second idea I mentioned?  If autosuspend fails with
> > -EBUSY or -EAGAIN, usbcore could schedule another autosuspend attempt.
> 
> Yeah, that sounds better.

Does this patch help?

Alan Stern


 drivers/usb/core/driver.c |    7 +++++++
 1 file changed, 7 insertions(+)

Index: usb-3.1/drivers/usb/core/driver.c
===================================================================
--- usb-3.1.orig/drivers/usb/core/driver.c
+++ usb-3.1/drivers/usb/core/driver.c
@@ -1667,6 +1667,13 @@ int usb_runtime_suspend(struct device *d
 		return -EAGAIN;
 
 	status = usb_suspend_both(udev, PMSG_AUTO_SUSPEND);
+
+	/* Retry if autosuspend failed temporarily */
+	if (status == -EBUSY || status == -EAGAIN) {
+		usb_mark_last_busy(udev);
+		pm_request_autosuspend(dev);
+	}
+
 	/* The PM core reacts badly unless the return code is 0,
 	 * -EAGAIN, or -EBUSY, so always return -EBUSY on an error.
 	 */

Re: [PATCH V3] input: Fix USB autosuspend on bcm5974

[Henrik Rydberg]

On Wed, Oct 12, 2011 at 05:59:05PM +0100, Matthew Garrett wrote:
> On Wed, Oct 12, 2011 at 06:56:32PM +0200, Oliver Neukum wrote:
> > Am Mittwoch, 12. Oktober 2011, 18:16:51 schrieb Matthew Garrett:
> > > Yeah. Plausibly the model we have in hid is wrong at the moment - rather 
> > > than set flags that block suspend, it might make more sense to take 
> > > references. It'd complicate things a little, in that we'd have to take 
> > > more care in terms of keeping track of the state.
> > 
> > I am afraid references are very hard to take there because it needs to be done
> > in interrupt.
> 
> True. That makes things rather more awkward.

I was looking at usb_autopm_get_interface_no_resume() yesterday for
the same reason, it seemed light-weight enough. No?

Btw, I wonder if the patch below would make sense or if calling put*()
is always a bug at zero usage count.

diff --git a/drivers/usb/core/driver.c b/drivers/usb/core/driver.c
index 34e3da5..4857e76 100644
--- a/drivers/usb/core/driver.c
+++ b/drivers/usb/core/driver.c
@@ -1516,7 +1516,7 @@ void usb_autopm_put_interface_no_suspend(struct usb_interface *intf)
        struct usb_device       *udev = interface_to_usbdev(intf);
 
        usb_mark_last_busy(udev);
-       atomic_dec(&intf->pm_usage_cnt);
+       atomic_add_unless(&intf->pm_usage_cnt, -1, 0);
        pm_runtime_put_noidle(&intf->dev);
 }
 EXPORT_SYMBOL_GPL(usb_autopm_put_interface_no_suspend);


Henrik

Re: [PATCH V3] input: Fix USB autosuspend on bcm5974

[Alan Stern]

On Wed, 12 Oct 2011, Henrik Rydberg wrote:

> On Wed, Oct 12, 2011 at 05:59:05PM +0100, Matthew Garrett wrote:
> > On Wed, Oct 12, 2011 at 06:56:32PM +0200, Oliver Neukum wrote:
> > > Am Mittwoch, 12. Oktober 2011, 18:16:51 schrieb Matthew Garrett:
> > > > Yeah. Plausibly the model we have in hid is wrong at the moment - rather 
> > > > than set flags that block suspend, it might make more sense to take 
> > > > references. It'd complicate things a little, in that we'd have to take 
> > > > more care in terms of keeping track of the state.
> > > 
> > > I am afraid references are very hard to take there because it needs to be done
> > > in interrupt.
> > 
> > True. That makes things rather more awkward.
> 
> I was looking at usb_autopm_get_interface_no_resume() yesterday for
> the same reason, it seemed light-weight enough. No?
> 
> Btw, I wonder if the patch below would make sense or if calling put*()
> is always a bug at zero usage count.

It's always a bug.  However, the patch I sent to Matthew should fix the 
immediate problem.

In fact, the original autosuspend design intended to allow drivers to 
do this -- keep devices awake by failing suspend requests even though 
the usage count is 0.  Under some conditions, that's the approach with 
the least overhead.  But the code to handle this got lost by mistake, 
so now it needs to be added back.

Alan Stern

Re: [PATCH V3] input: Fix USB autosuspend on bcm5974

[Henrik Rydberg]

> It's always a bug.  However, the patch I sent to Matthew should fix the 
> immediate problem.
> 
> In fact, the original autosuspend design intended to allow drivers to 
> do this -- keep devices awake by failing suspend requests even though 
> the usage count is 0.  Under some conditions, that's the approach with 
> the least overhead.  But the code to handle this got lost by mistake, 
> so now it needs to be added back.

I tried the patch out, unfortunately it did not work. The second
suspend request returns with a -EINPROGRESS, here:

	if (dev->power.runtime_status == RPM_SUSPENDING) {
		DEFINE_WAIT(wait);

		if (rpmflags & (RPM_ASYNC | RPM_NOWAIT)) {
			retval = -EINPROGRESS;
			goto out;
		}

		/* Wait for the other suspend running in parallel with us. */
		for (;;) {

The reason is above my horizon, although RPM_ASYNC looks suspicious.
In general, we are re-entering rpm_suspend from within the driver
handler, so the exit condition of rpm_suspend may not have been
satisfied yet.

Thanks,
Henrik

Re: [PATCH V3] input: Fix USB autosuspend on bcm5974

[Alan Stern]

On Thu, 13 Oct 2011, Henrik Rydberg wrote:

> > It's always a bug.  However, the patch I sent to Matthew should fix the 
> > immediate problem.
> > 
> > In fact, the original autosuspend design intended to allow drivers to 
> > do this -- keep devices awake by failing suspend requests even though 
> > the usage count is 0.  Under some conditions, that's the approach with 
> > the least overhead.  But the code to handle this got lost by mistake, 
> > so now it needs to be added back.
> 
> I tried the patch out, unfortunately it did not work. The second
> suspend request returns with a -EINPROGRESS, here:
> 
> 	if (dev->power.runtime_status == RPM_SUSPENDING) {
> 		DEFINE_WAIT(wait);
> 
> 		if (rpmflags & (RPM_ASYNC | RPM_NOWAIT)) {
> 			retval = -EINPROGRESS;
> 			goto out;
> 		}
> 
> 		/* Wait for the other suspend running in parallel with us. */
> 		for (;;) {
> 
> The reason is above my horizon, although RPM_ASYNC looks suspicious.
> In general, we are re-entering rpm_suspend from within the driver
> handler, so the exit condition of rpm_suspend may not have been
> satisfied yet.

Ah yes, I vaguely remember thinking that another part would have to be
fixed.  But it's not just the part you identified; the error actually
occurred higher up.  The patch below ought to help.

Alan Stern


 drivers/base/power/runtime.c |   18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

Index: usb-3.1/drivers/base/power/runtime.c
===================================================================
--- usb-3.1.orig/drivers/base/power/runtime.c
+++ usb-3.1/drivers/base/power/runtime.c
@@ -278,8 +278,9 @@ static int rpm_callback(int (*cb)(struct
  * @rpmflags: Flag bits.
  *
  * Check if the device's runtime PM status allows it to be suspended.  If
- * another suspend has been started earlier, either return immediately or wait
- * for it to finish, depending on the RPM_NOWAIT and RPM_ASYNC flags.  Cancel a
+ * another suspend has been started earlier and the RPM_ASYNC flag isn't set,
+ * either return immediately or wait for it to finish, depending on the
+ * RPM_NOWAIT flag.  Cancel a
  * pending idle notification.  If the RPM_ASYNC flag is set then queue a
  * suspend request; otherwise run the ->runtime_suspend() callback directly.
  * If a deferred resume was requested while the callback was running then carry
@@ -311,8 +312,7 @@ static int rpm_suspend(struct device *de
 		goto out;
 
 	/* If the autosuspend_delay time hasn't expired yet, reschedule. */
-	if ((rpmflags & RPM_AUTO)
-	    && dev->power.runtime_status != RPM_SUSPENDING) {
+	if (rpmflags & RPM_AUTO) {
 		unsigned long expires = pm_runtime_autosuspend_expiration(dev);
 
 		if (expires != 0) {
@@ -339,10 +339,16 @@ static int rpm_suspend(struct device *de
 	/* Other scheduled or pending requests need to be canceled. */
 	pm_runtime_cancel_pending(dev);
 
-	if (dev->power.runtime_status == RPM_SUSPENDING) {
+	/*
+	 * If the request is synchronous, wait for a concurrent suspend attempt
+	 * to finish.  Allow asynchronous requests to go through; they will be
+	 * cancelled if the concurrent suspend succeeds.
+	 */
+	if (dev->power.runtime_status == RPM_SUSPENDING &&
+			!(rpmflags & RPM_ASYNC)) {
 		DEFINE_WAIT(wait);
 
-		if (rpmflags & (RPM_ASYNC | RPM_NOWAIT)) {
+		if (rpmflags & RPM_NOWAIT) {
 			retval = -EINPROGRESS;
 			goto out;
 		}

Re: [PATCH V3] input: Fix USB autosuspend on bcm5974

[Henrik Rydberg]

> > I tried the patch out, unfortunately it did not work. The second
> > suspend request returns with a -EINPROGRESS, here:
> > 
> > 	if (dev->power.runtime_status == RPM_SUSPENDING) {
> > 		DEFINE_WAIT(wait);
> > 
> > 		if (rpmflags & (RPM_ASYNC | RPM_NOWAIT)) {
> > 			retval = -EINPROGRESS;
> > 			goto out;
> > 		}
> > 
> > 		/* Wait for the other suspend running in parallel with us. */
> > 		for (;;) {
> > 
> > The reason is above my horizon, although RPM_ASYNC looks suspicious.
> > In general, we are re-entering rpm_suspend from within the driver
> > handler, so the exit condition of rpm_suspend may not have been
> > satisfied yet.
> 
> Ah yes, I vaguely remember thinking that another part would have to be
> fixed.  But it's not just the part you identified; the error actually
> occurred higher up.  The patch below ought to help.

Yes, this works nicely, thank you. I attached my tested-by at the end,
presuming there will be a formal patch. :-)

>  drivers/base/power/runtime.c |   18 ++++++++++++------
>  1 file changed, 12 insertions(+), 6 deletions(-)
> 
> Index: usb-3.1/drivers/base/power/runtime.c
> ===================================================================
> --- usb-3.1.orig/drivers/base/power/runtime.c
> +++ usb-3.1/drivers/base/power/runtime.c
> @@ -278,8 +278,9 @@ static int rpm_callback(int (*cb)(struct
>   * @rpmflags: Flag bits.
>   *
>   * Check if the device's runtime PM status allows it to be suspended.  If
> - * another suspend has been started earlier, either return immediately or wait
> - * for it to finish, depending on the RPM_NOWAIT and RPM_ASYNC flags.  Cancel a
> + * another suspend has been started earlier and the RPM_ASYNC flag isn't set,
> + * either return immediately or wait for it to finish, depending on the
> + * RPM_NOWAIT flag.  Cancel a
>   * pending idle notification.  If the RPM_ASYNC flag is set then queue a
>   * suspend request; otherwise run the ->runtime_suspend() callback directly.
>   * If a deferred resume was requested while the callback was running then carry
> @@ -311,8 +312,7 @@ static int rpm_suspend(struct device *de
>  		goto out;
>  
>  	/* If the autosuspend_delay time hasn't expired yet, reschedule. */
> -	if ((rpmflags & RPM_AUTO)
> -	    && dev->power.runtime_status != RPM_SUSPENDING) {
> +	if (rpmflags & RPM_AUTO) {
>  		unsigned long expires = pm_runtime_autosuspend_expiration(dev);
>  
>  		if (expires != 0) {
> @@ -339,10 +339,16 @@ static int rpm_suspend(struct device *de
>  	/* Other scheduled or pending requests need to be canceled. */
>  	pm_runtime_cancel_pending(dev);
>  
> -	if (dev->power.runtime_status == RPM_SUSPENDING) {
> +	/*
> +	 * If the request is synchronous, wait for a concurrent suspend attempt
> +	 * to finish.  Allow asynchronous requests to go through; they will be
> +	 * cancelled if the concurrent suspend succeeds.
> +	 */
> +	if (dev->power.runtime_status == RPM_SUSPENDING &&
> +			!(rpmflags & RPM_ASYNC)) {
>  		DEFINE_WAIT(wait);
>  
> -		if (rpmflags & (RPM_ASYNC | RPM_NOWAIT)) {
> +		if (rpmflags & RPM_NOWAIT) {
>  			retval = -EINPROGRESS;
>  			goto out;
>  		}
> 

    Tested-by: Henrik Rydberg <rydberg@euromail.se>

Thanks,
Henrik

Re: [PATCH V3] input: Fix USB autosuspend on bcm5974

[Oliver Neukum]

Am Mittwoch, 12. Oktober 2011, 17:28:06 schrieb Alan Stern:
> > Arguably a driver is the wrong layer to do this.
> 
> Are you suggesting the usb_runtime_suspend() should call 
> usb_mark_last_busy() and pm_runtime_autosuspend() if usb_suspend_both() 
> returns -EBUSY or -EAGAIN?

Yes.

	Regards
		Oliver

Re: [PATCH V3] input: Fix USB autosuspend on bcm5974

[Alan Stern]

On Wed, 12 Oct 2011, Oliver Neukum wrote:

> Am Mittwoch, 12. Oktober 2011, 17:28:06 schrieb Alan Stern:
> > > Arguably a driver is the wrong layer to do this.
> > 
> > Are you suggesting the usb_runtime_suspend() should call 
> > usb_mark_last_busy() and pm_runtime_autosuspend() if usb_suspend_both() 
> > returns -EBUSY or -EAGAIN?
> 
> Yes.

I just sent a trial patch to Matthew.  And after a little checking, I
found the reason for the problem.

Originally usbcore did this automatically.  When we switched over to 
the runtime PM framework, the PM core did it.  But the code to retry 
failed autosuspends was removed by Rafael Wysocki in commit 
f71648d73c1650b8b4aceb3856bebbde6daa3b86 (PM / Runtime: Remove idle 
notification after failing suspend), and the USB subsystem wasn't 
updated accordingly.  I even Acked that patch -- apparently I had 
forgotten about the need for this mechanism.

So it looks like putting it back into usbcore is the right thing to do.

Alan Stern

Re: [PATCH V3] input: Fix USB autosuspend on bcm5974

[Dmitry Torokhov]

On Tue, Oct 11, 2011 at 06:10:12AM +0200, Oliver Neukum wrote:
> Am Dienstag, 11. Oktober 2011, 02:11:03 schrieb Matthew Garrett:
> 
> > Ah, right, sorry - I wasn't trying to use the device after it had been 
> > closed. In an ideal world we'd really want to be able to indicate that 
> > we don't want a remote wakeup in that situation. Otherwise, the patch 
> > looks good. Thank you!
> 
> It looks like we had exactly that capability until
> 48826626263d4a61d06fd8c5805da31f925aefa0
> removed it.
> 

Hmm, reading Alan's comment I can see why wakeup might be beneficial
when system is in the sleep state, but while it is running and there is
no driver or driver wants to disable wakeups I think we should
accommodate it.

Thanks.

-- 
Dmitry

Re: [PATCH V3] input: Fix USB autosuspend on bcm5974

[Alan Stern]

On Tue, 11 Oct 2011, Dmitry Torokhov wrote:

> On Tue, Oct 11, 2011 at 06:10:12AM +0200, Oliver Neukum wrote:
> > Am Dienstag, 11. Oktober 2011, 02:11:03 schrieb Matthew Garrett:
> > 
> > > Ah, right, sorry - I wasn't trying to use the device after it had been 
> > > closed. In an ideal world we'd really want to be able to indicate that 
> > > we don't want a remote wakeup in that situation. Otherwise, the patch 
> > > looks good. Thank you!
> > 
> > It looks like we had exactly that capability until
> > 48826626263d4a61d06fd8c5805da31f925aefa0
> > removed it.
> > 
> 
> Hmm, reading Alan's comment I can see why wakeup might be beneficial
> when system is in the sleep state, but while it is running and there is
> no driver or driver wants to disable wakeups I think we should
> accommodate it.

Currently, wakeup is enabled for USB devices during autosuspend if and
only if one or more of the interface drivers has set the
intf->needs_remote_wakeup flag.  Therefore all that usbhid should need 
to do is clear that flag when the device file is closed.

Alan Stern

Re: [PATCH V3] input: Fix USB autosuspend on bcm5974

[Oliver Neukum]

Am Dienstag, 11. Oktober 2011, 18:43:55 schrieb Alan Stern:
> On Tue, 11 Oct 2011, Dmitry Torokhov wrote:
 
> > Hmm, reading Alan's comment I can see why wakeup might be beneficial
> > when system is in the sleep state, but while it is running and there is
> > no driver or driver wants to disable wakeups I think we should
> > accommodate it.
> 
> Currently, wakeup is enabled for USB devices during autosuspend if and
> only if one or more of the interface drivers has set the
> intf->needs_remote_wakeup flag.  Therefore all that usbhid should need 
> to do is clear that flag when the device file is closed.

Now I am confused because Matthew reported that his device generated
remote wakeups without setting needs_remote_wakeup at all.
Matthew, could you clarify?

	Regards
		Oliver

Re: [PATCH V3] input: Fix USB autosuspend on bcm5974

[Matthew Garrett]

On Wed, Oct 12, 2011 at 09:03:00AM +0200, Oliver Neukum wrote:
> Am Dienstag, 11. Oktober 2011, 18:43:55 schrieb Alan Stern:
> > On Tue, 11 Oct 2011, Dmitry Torokhov wrote:
>  
> > > Hmm, reading Alan's comment I can see why wakeup might be beneficial
> > > when system is in the sleep state, but while it is running and there is
> > > no driver or driver wants to disable wakeups I think we should
> > > accommodate it.
> > 
> > Currently, wakeup is enabled for USB devices during autosuspend if and
> > only if one or more of the interface drivers has set the
> > intf->needs_remote_wakeup flag.  Therefore all that usbhid should need 
> > to do is clear that flag when the device file is closed.
> 
> Now I am confused because Matthew reported that his device generated
> remote wakeups without setting needs_remote_wakeup at all.
> Matthew, could you clarify?

I /think/ Alan's wrong here - do_remote_wakeup is set to
device_may_wakeup(), which will be true if the device can generate 
remote wakeups even if nothing's asked for them. I'm basing this on 
autosuspend_check() and choose_wakeup(). Alan, am I misinterpreting this 
code?

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH V3] input: Fix USB autosuspend on bcm5974

[Alan Stern]

On Wed, 12 Oct 2011, Matthew Garrett wrote:

> On Wed, Oct 12, 2011 at 09:03:00AM +0200, Oliver Neukum wrote:
> > Am Dienstag, 11. Oktober 2011, 18:43:55 schrieb Alan Stern:
> > > On Tue, 11 Oct 2011, Dmitry Torokhov wrote:
> >  
> > > > Hmm, reading Alan's comment I can see why wakeup might be beneficial
> > > > when system is in the sleep state, but while it is running and there is
> > > > no driver or driver wants to disable wakeups I think we should
> > > > accommodate it.
> > > 
> > > Currently, wakeup is enabled for USB devices during autosuspend if and
> > > only if one or more of the interface drivers has set the
> > > intf->needs_remote_wakeup flag.  Therefore all that usbhid should need 
> > > to do is clear that flag when the device file is closed.
> > 
> > Now I am confused because Matthew reported that his device generated
> > remote wakeups without setting needs_remote_wakeup at all.
> > Matthew, could you clarify?
> 
> I /think/ Alan's wrong here - do_remote_wakeup is set to
> device_may_wakeup(), which will be true if the device can generate 
> remote wakeups even if nothing's asked for them. I'm basing this on 
> autosuspend_check() and choose_wakeup(). Alan, am I misinterpreting this 
> code?

We may be talking about different things.  choose_wakeup() is called
for system suspend, whereas autosuspend_check() is called (obviously!)
for autosuspend.

Yes, choose_wakeup() uses device_may_wakeup().  But I was referring
only to autosuspend (as was Dmitry).  In autosuspend_check(),
do_remote_wakeup is set to w, which is the result of or'ing all the
intf->needs_remote_wakeup values.

Alan Stern

Re: [PATCH V3] input: Fix USB autosuspend on bcm5974

[Matthew Garrett]

Sorry, yes, you're right. In that case it's probably working for me 
because the keyboard is another interface on the same device, so the 
wakeup bit is being set from there.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH V3] input: Fix USB autosuspend on bcm5974

[Oliver Neukum]

Am Mittwoch, 12. Oktober 2011, 16:26:21 schrieb Matthew Garrett:
> Sorry, yes, you're right. In that case it's probably working for me 
> because the keyboard is another interface on the same device, so the 
> wakeup bit is being set from there.

Very good. A mystery is solved. Indeed you may have found
a deficiency in the USB spec.

	Regards
		Oliver

Re: [PATCH V3] input: Fix USB autosuspend on bcm5974

[Henrik Rydberg]

On Wed, Oct 12, 2011 at 04:38:38PM +0200, Oliver Neukum wrote:
> Am Mittwoch, 12. Oktober 2011, 16:26:21 schrieb Matthew Garrett:
> > Sorry, yes, you're right. In that case it's probably working for me 
> > because the keyboard is another interface on the same device, so the 
> > wakeup bit is being set from there.
> 
> Very good. A mystery is solved. Indeed you may have found
> a deficiency in the USB spec.

Glad you sorted this out. So we can really only treat a device as one
(auto)power unit, due to remote wakeup being requested at the device level,
rather than at the interface level.

Thanks,
Henrik

Re: [PATCH V3] input: Fix USB autosuspend on bcm5974

[Oliver Neukum]

Am Mittwoch, 12. Oktober 2011, 20:50:03 schrieb Henrik Rydberg:
> On Wed, Oct 12, 2011 at 04:38:38PM +0200, Oliver Neukum wrote:
> > Am Mittwoch, 12. Oktober 2011, 16:26:21 schrieb Matthew Garrett:
> > > Sorry, yes, you're right. In that case it's probably working for me 
> > > because the keyboard is another interface on the same device, so the 
> > > wakeup bit is being set from there.
> > 
> > Very good. A mystery is solved. Indeed you may have found
> > a deficiency in the USB spec.
> 
> Glad you sorted this out. So we can really only treat a device as one
> (auto)power unit, due to remote wakeup being requested at the device level,
> rather than at the interface level.

Yes, when in doubt we must request it, even at the cost of unnecessary wakeups.
This also means that only Matthew's latest patch is correct. We cannot depend
on the usbhid always be loaded and requesting remote wakeups.

	Regards
		Oliver

Re: [PATCH V3] input: Fix USB autosuspend on bcm5974

[Matthew Garrett]

Hmm. lsof agrees that I have no input devices open, but I see the 
bcm5974 device being suspended and then staying that way.

-- 
Matthew Garrett | mjg59@srcf.ucam.org