[PATCH] mfd: cros ec: spi: Fix "in progress" error signaling
For host commands that take a long time to process, cros ec can return
early by signaling a EC_RES_IN_PROGRESS result. The host must then poll
status with EC_CMD_GET_COMMS_STATUS until completion of the command.
None of the above applies when data link errors are encountered. When
errors such as EC_SPI_PAST_END are encountered during command
transmission, it usually means the command was not received by the EC.
Treating such errors as if they were 'EC_RES_IN_PROGRESS' results is
almost always the wrong decision, and can result in host commands
silently being lost.
Reported-and-tested-by: Jon Hunter <jonath...@nvidia.com>
Signed-off-by: Shawn Nematbakhsh <sha...@chromium.org>
---
drivers/mfd/cros_ec_spi.c | 52 ++-
1 file changed, 24 insertions(+), 28 deletions(-)
diff --git a/drivers/mfd/cros_ec_spi.c b/drivers/mfd/cros_ec_spi.c
index c9714072e224..d019b5b00b67 100644
--- a/drivers/mfd/cros_ec_spi.c
+++ b/drivers/mfd/cros_ec_spi.c
@@ -377,6 +377,7 @@ static int cros_ec_pkt_xfer_spi(struct cros_ec_device
*ec_dev,
u8 *ptr;
u8 *rx_buf;
u8 sum;
+ u8 rx_byte;
int ret = 0, final_ret;
len = cros_ec_prepare_tx(ec_dev, ec_msg);
@@ -421,25 +422,22 @@ static int cros_ec_pkt_xfer_spi(struct cros_ec_device
*ec_dev,
if (!ret) {
/* Verify that EC can process command */
for (i = 0; i < len; i++) {
- switch (rx_buf[i]) {
- case EC_SPI_PAST_END:
- case EC_SPI_RX_BAD_DATA:
- case EC_SPI_NOT_READY:
- ret = -EAGAIN;
- ec_msg->result = EC_RES_IN_PROGRESS;
- default:
+ rx_byte = rx_buf[i];
+ if (rx_byte == EC_SPI_PAST_END ||
+ rx_byte == EC_SPI_RX_BAD_DATA ||
+ rx_byte == EC_SPI_NOT_READY) {
+ ret = -EREMOTEIO;
break;
}
- if (ret)
- break;
}
- if (!ret)
- ret = cros_ec_spi_receive_packet(ec_dev,
- ec_msg->insize + sizeof(*response));
- } else {
- dev_err(ec_dev->dev, "spi transfer failed: %d\n", ret);
}
+ if (!ret)
+ ret = cros_ec_spi_receive_packet(ec_dev,
+ ec_msg->insize + sizeof(*response));
+ else
+ dev_err(ec_dev->dev, "spi transfer failed: %d\n", ret);
+
final_ret = terminate_request(ec_dev);
spi_bus_unlock(ec_spi->spi->master);
@@ -508,6 +506,7 @@ static int cros_ec_cmd_xfer_spi(struct cros_ec_device
*ec_dev,
int i, len;
u8 *ptr;
u8 *rx_buf;
+ u8 rx_byte;
int sum;
int ret = 0, final_ret;
@@ -544,25 +543,22 @@ static int cros_ec_cmd_xfer_spi(struct cros_ec_device
*ec_dev,
if (!ret) {
/* Verify that EC can process command */
for (i = 0; i < len; i++) {
- switch (rx_buf[i]) {
- case EC_SPI_PAST_END:
- case EC_SPI_RX_BAD_DATA:
- case EC_SPI_NOT_READY:
- ret = -EAGAIN;
- ec_msg->result = EC_RES_IN_PROGRESS;
- default:
+ rx_byte = rx_buf[i];
+ if (rx_byte == EC_SPI_PAST_END ||
+ rx_byte == EC_SPI_RX_BAD_DATA ||
+ rx_byte == EC_SPI_NOT_READY) {
+ ret = -EREMOTEIO;
break;
}
- if (ret)
- break;
}
- if (!ret)
- ret = cros_ec_spi_receive_response(ec_dev,
- ec_msg->insize + EC_MSG_TX_PROTO_BYTES);
- } else {
- dev_err(ec_dev->dev, "spi transfer failed: %d\n", ret);
}
+ if (!ret)
+ ret = cros_ec_spi_receive_response(ec_dev,
+ ec_msg->insize + EC_MSG_TX_PROTO_BYTES);
+ else
+ dev_err(ec_dev->dev, "spi transfer failed: %d\n", ret);
+
final_ret = terminate_request(ec_dev);
spi_bus_unlock(ec_spi->spi->master);
--
2.12.2
[PATCH] mfd: cros ec: spi: Fix "in progress" error signaling
For host commands that take a long time to process, cros ec can return
early by signaling a EC_RES_IN_PROGRESS result. The host must then poll
status with EC_CMD_GET_COMMS_STATUS until completion of the command.
None of the above applies when data link errors are encountered. When
errors such as EC_SPI_PAST_END are encountered during command
transmission, it usually means the command was not received by the EC.
Treating such errors as if they were 'EC_RES_IN_PROGRESS' results is
almost always the wrong decision, and can result in host commands
silently being lost.
Reported-and-tested-by: Jon Hunter
Signed-off-by: Shawn Nematbakhsh
---
drivers/mfd/cros_ec_spi.c | 52 ++-
1 file changed, 24 insertions(+), 28 deletions(-)
diff --git a/drivers/mfd/cros_ec_spi.c b/drivers/mfd/cros_ec_spi.c
index c9714072e224..d019b5b00b67 100644
--- a/drivers/mfd/cros_ec_spi.c
+++ b/drivers/mfd/cros_ec_spi.c
@@ -377,6 +377,7 @@ static int cros_ec_pkt_xfer_spi(struct cros_ec_device
*ec_dev,
u8 *ptr;
u8 *rx_buf;
u8 sum;
+ u8 rx_byte;
int ret = 0, final_ret;
len = cros_ec_prepare_tx(ec_dev, ec_msg);
@@ -421,25 +422,22 @@ static int cros_ec_pkt_xfer_spi(struct cros_ec_device
*ec_dev,
if (!ret) {
/* Verify that EC can process command */
for (i = 0; i < len; i++) {
- switch (rx_buf[i]) {
- case EC_SPI_PAST_END:
- case EC_SPI_RX_BAD_DATA:
- case EC_SPI_NOT_READY:
- ret = -EAGAIN;
- ec_msg->result = EC_RES_IN_PROGRESS;
- default:
+ rx_byte = rx_buf[i];
+ if (rx_byte == EC_SPI_PAST_END ||
+ rx_byte == EC_SPI_RX_BAD_DATA ||
+ rx_byte == EC_SPI_NOT_READY) {
+ ret = -EREMOTEIO;
break;
}
- if (ret)
- break;
}
- if (!ret)
- ret = cros_ec_spi_receive_packet(ec_dev,
- ec_msg->insize + sizeof(*response));
- } else {
- dev_err(ec_dev->dev, "spi transfer failed: %d\n", ret);
}
+ if (!ret)
+ ret = cros_ec_spi_receive_packet(ec_dev,
+ ec_msg->insize + sizeof(*response));
+ else
+ dev_err(ec_dev->dev, "spi transfer failed: %d\n", ret);
+
final_ret = terminate_request(ec_dev);
spi_bus_unlock(ec_spi->spi->master);
@@ -508,6 +506,7 @@ static int cros_ec_cmd_xfer_spi(struct cros_ec_device
*ec_dev,
int i, len;
u8 *ptr;
u8 *rx_buf;
+ u8 rx_byte;
int sum;
int ret = 0, final_ret;
@@ -544,25 +543,22 @@ static int cros_ec_cmd_xfer_spi(struct cros_ec_device
*ec_dev,
if (!ret) {
/* Verify that EC can process command */
for (i = 0; i < len; i++) {
- switch (rx_buf[i]) {
- case EC_SPI_PAST_END:
- case EC_SPI_RX_BAD_DATA:
- case EC_SPI_NOT_READY:
- ret = -EAGAIN;
- ec_msg->result = EC_RES_IN_PROGRESS;
- default:
+ rx_byte = rx_buf[i];
+ if (rx_byte == EC_SPI_PAST_END ||
+ rx_byte == EC_SPI_RX_BAD_DATA ||
+ rx_byte == EC_SPI_NOT_READY) {
+ ret = -EREMOTEIO;
break;
}
- if (ret)
- break;
}
- if (!ret)
- ret = cros_ec_spi_receive_response(ec_dev,
- ec_msg->insize + EC_MSG_TX_PROTO_BYTES);
- } else {
- dev_err(ec_dev->dev, "spi transfer failed: %d\n", ret);
}
+ if (!ret)
+ ret = cros_ec_spi_receive_response(ec_dev,
+ ec_msg->insize + EC_MSG_TX_PROTO_BYTES);
+ else
+ dev_err(ec_dev->dev, "spi transfer failed: %d\n", ret);
+
final_ret = terminate_request(ec_dev);
spi_bus_unlock(ec_spi->spi->master);
--
2.12.2
[PATCH 2/2] power: supply: sbs-battery: Don't needlessly set CAPACITY_MODE
According to the smart battery spec (1), the CAPACITY_MODE bit does not
influence the value read from RelativeStateOfCharge(), so don't bother
changing CAPACITY_MODE when doing such a read.
(1) - Smart Battery Data Specification, Rev 1.1, Dec. 11, 1998
Signed-off-by: Shawn Nematbakhsh <sha...@chromium.org>
---
drivers/power/supply/sbs-battery.c | 14 +++---
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/drivers/power/supply/sbs-battery.c
b/drivers/power/supply/sbs-battery.c
index d7e68fd97099..4bae210d5a8a 100644
--- a/drivers/power/supply/sbs-battery.c
+++ b/drivers/power/supply/sbs-battery.c
@@ -439,6 +439,11 @@ static int sbs_get_battery_property(struct i2c_client
*client,
} else {
if (psp == POWER_SUPPLY_PROP_STATUS)
val->intval = POWER_SUPPLY_STATUS_UNKNOWN;
+ else if (psp == POWER_SUPPLY_PROP_CAPACITY)
+ /* sbs spec says that this can be >100 %
+* even if max value is 100 %
+*/
+ val->intval = min(ret, 100);
else
val->intval = 0;
}
@@ -549,12 +554,7 @@ static int sbs_get_battery_capacity(struct i2c_client
*client,
if (ret < 0)
return ret;
- if (psp == POWER_SUPPLY_PROP_CAPACITY) {
- /* sbs spec says that this can be >100 %
- * even if max value is 100 % */
- val->intval = min(ret, 100);
- } else
- val->intval = ret;
+ val->intval = ret;
ret = sbs_set_battery_mode(client, mode);
if (ret < 0)
@@ -619,7 +619,6 @@ static int sbs_get_property(struct power_supply *psy,
case POWER_SUPPLY_PROP_CHARGE_NOW:
case POWER_SUPPLY_PROP_CHARGE_FULL:
case POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN:
- case POWER_SUPPLY_PROP_CAPACITY:
ret = sbs_get_property_index(client, psp);
if (ret < 0)
break;
@@ -647,6 +646,7 @@ static int sbs_get_property(struct power_supply *psy,
case POWER_SUPPLY_PROP_TIME_TO_FULL_AVG:
case POWER_SUPPLY_PROP_VOLTAGE_MIN_DESIGN:
case POWER_SUPPLY_PROP_VOLTAGE_MAX_DESIGN:
+ case POWER_SUPPLY_PROP_CAPACITY:
ret = sbs_get_property_index(client, psp);
if (ret < 0)
break;
--
2.13.1.508.gb3defc5cc-goog
[PATCH 2/2] power: supply: sbs-battery: Don't needlessly set CAPACITY_MODE
According to the smart battery spec (1), the CAPACITY_MODE bit does not
influence the value read from RelativeStateOfCharge(), so don't bother
changing CAPACITY_MODE when doing such a read.
(1) - Smart Battery Data Specification, Rev 1.1, Dec. 11, 1998
Signed-off-by: Shawn Nematbakhsh
---
drivers/power/supply/sbs-battery.c | 14 +++---
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/drivers/power/supply/sbs-battery.c
b/drivers/power/supply/sbs-battery.c
index d7e68fd97099..4bae210d5a8a 100644
--- a/drivers/power/supply/sbs-battery.c
+++ b/drivers/power/supply/sbs-battery.c
@@ -439,6 +439,11 @@ static int sbs_get_battery_property(struct i2c_client
*client,
} else {
if (psp == POWER_SUPPLY_PROP_STATUS)
val->intval = POWER_SUPPLY_STATUS_UNKNOWN;
+ else if (psp == POWER_SUPPLY_PROP_CAPACITY)
+ /* sbs spec says that this can be >100 %
+* even if max value is 100 %
+*/
+ val->intval = min(ret, 100);
else
val->intval = 0;
}
@@ -549,12 +554,7 @@ static int sbs_get_battery_capacity(struct i2c_client
*client,
if (ret < 0)
return ret;
- if (psp == POWER_SUPPLY_PROP_CAPACITY) {
- /* sbs spec says that this can be >100 %
- * even if max value is 100 % */
- val->intval = min(ret, 100);
- } else
- val->intval = ret;
+ val->intval = ret;
ret = sbs_set_battery_mode(client, mode);
if (ret < 0)
@@ -619,7 +619,6 @@ static int sbs_get_property(struct power_supply *psy,
case POWER_SUPPLY_PROP_CHARGE_NOW:
case POWER_SUPPLY_PROP_CHARGE_FULL:
case POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN:
- case POWER_SUPPLY_PROP_CAPACITY:
ret = sbs_get_property_index(client, psp);
if (ret < 0)
break;
@@ -647,6 +646,7 @@ static int sbs_get_property(struct power_supply *psy,
case POWER_SUPPLY_PROP_TIME_TO_FULL_AVG:
case POWER_SUPPLY_PROP_VOLTAGE_MIN_DESIGN:
case POWER_SUPPLY_PROP_VOLTAGE_MAX_DESIGN:
+ case POWER_SUPPLY_PROP_CAPACITY:
ret = sbs_get_property_index(client, psp);
if (ret < 0)
break;
--
2.13.1.508.gb3defc5cc-goog
[PATCH 1/2] power: supply: sbs-battery: Prevent CAPACITY_MODE races
A subset of smart battery commands return charge or energy depending on
the CAPACITY_MODE bit setting of BatteryMode(). In order to
unambiguously read a charge or energy value, it is necessary to ensure
that CAPACITY_MODE is set as desired, and not changed for the duration
of the attribute read.
Signed-off-by: Shawn Nematbakhsh <sha...@chromium.org>
---
drivers/power/supply/sbs-battery.c | 8
1 file changed, 8 insertions(+)
diff --git a/drivers/power/supply/sbs-battery.c
b/drivers/power/supply/sbs-battery.c
index e3a114e60f1a..d7e68fd97099 100644
--- a/drivers/power/supply/sbs-battery.c
+++ b/drivers/power/supply/sbs-battery.c
@@ -171,6 +171,7 @@ struct sbs_info {
u32 i2c_retry_count;
u32 poll_retry_count;
struct delayed_work work;
+ struct mutexmode_lock;
};
static char model_name[I2C_SMBUS_BLOCK_MAX + 1];
@@ -623,7 +624,13 @@ static int sbs_get_property(struct power_supply *psy,
if (ret < 0)
break;
+ /* sbs_get_battery_capacity() will change the battery mode
+* temporarily to read the requested attribute. Ensure we stay
+* in the desired mode for the duration of the attribute read.
+*/
+ mutex_lock(>mode_lock);
ret = sbs_get_battery_capacity(client, ret, psp, val);
+ mutex_unlock(>mode_lock);
break;
case POWER_SUPPLY_PROP_SERIAL_NUMBER:
@@ -808,6 +815,7 @@ static int sbs_probe(struct i2c_client *client,
psy_cfg.of_node = client->dev.of_node;
psy_cfg.drv_data = chip;
chip->last_state = POWER_SUPPLY_STATUS_UNKNOWN;
+ mutex_init(>mode_lock);
/* use pdata if available, fall back to DT properties,
* or hardcoded defaults if not
--
2.13.1.508.gb3defc5cc-goog
[PATCH 1/2] power: supply: sbs-battery: Prevent CAPACITY_MODE races
A subset of smart battery commands return charge or energy depending on
the CAPACITY_MODE bit setting of BatteryMode(). In order to
unambiguously read a charge or energy value, it is necessary to ensure
that CAPACITY_MODE is set as desired, and not changed for the duration
of the attribute read.
Signed-off-by: Shawn Nematbakhsh
---
drivers/power/supply/sbs-battery.c | 8
1 file changed, 8 insertions(+)
diff --git a/drivers/power/supply/sbs-battery.c
b/drivers/power/supply/sbs-battery.c
index e3a114e60f1a..d7e68fd97099 100644
--- a/drivers/power/supply/sbs-battery.c
+++ b/drivers/power/supply/sbs-battery.c
@@ -171,6 +171,7 @@ struct sbs_info {
u32 i2c_retry_count;
u32 poll_retry_count;
struct delayed_work work;
+ struct mutexmode_lock;
};
static char model_name[I2C_SMBUS_BLOCK_MAX + 1];
@@ -623,7 +624,13 @@ static int sbs_get_property(struct power_supply *psy,
if (ret < 0)
break;
+ /* sbs_get_battery_capacity() will change the battery mode
+* temporarily to read the requested attribute. Ensure we stay
+* in the desired mode for the duration of the attribute read.
+*/
+ mutex_lock(>mode_lock);
ret = sbs_get_battery_capacity(client, ret, psp, val);
+ mutex_unlock(>mode_lock);
break;
case POWER_SUPPLY_PROP_SERIAL_NUMBER:
@@ -808,6 +815,7 @@ static int sbs_probe(struct i2c_client *client,
psy_cfg.of_node = client->dev.of_node;
psy_cfg.drv_data = chip;
chip->last_state = POWER_SUPPLY_STATUS_UNKNOWN;
+ mutex_init(>mode_lock);
/* use pdata if available, fall back to DT properties,
* or hardcoded defaults if not
--
2.13.1.508.gb3defc5cc-goog
[PATCH v2] usb: xhci: Disable runtime PM suspend for quirky controllers
If a USB controller with XHCI_RESET_ON_RESUME goes to runtime suspend,
a reset will be performed upon runtime resume. Any previously suspended
devices attached to the controller will be re-enumerated at this time.
This will cause problems, for example, if an open system call on the
device triggered the resume (the open call will fail).
Note that this change is only relevant when persist_enabled is not set
for USB devices.
Signed-off-by: Shawn Nematbakhsh
---
Changes since v1:
- Only disable runtime suspend when devices are connected.
- Skip this change if CONFIG_USB_DEFAULT_PERSIST is enabled.
---
Change-Id: I964e5bdfec09d03fac8656aaf566bc48d34ef0f9
---
drivers/usb/host/xhci.c | 22 ++
1 file changed, 22 insertions(+)
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index 9478caa..8adbab2 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -3581,10 +3581,21 @@ void xhci_free_dev(struct usb_hcd *hcd, struct
usb_device *udev)
{
struct xhci_hcd *xhci = hcd_to_xhci(hcd);
struct xhci_virt_device *virt_dev;
+ struct device *dev = hcd->self.controller;
unsigned long flags;
u32 state;
int i, ret;
+#ifndef CONFIG_USB_DEFAULT_PERSIST
+ /*
+* We called pm_runtime_get_noresume when the device was attached.
+* Decrement the counter here to allow controller to runtime suspend
+* if no devices remain.
+*/
+ if (xhci->quirks & XHCI_RESET_ON_RESUME)
+ pm_runtime_put_noidle(dev);
+#endif
+
ret = xhci_check_args(hcd, udev, NULL, 0, true, __func__);
/* If the host is halted due to driver unload, we still need to free the
* device.
@@ -3656,6 +3667,7 @@ static int xhci_reserve_host_control_ep_resources(struct
xhci_hcd *xhci)
int xhci_alloc_dev(struct usb_hcd *hcd, struct usb_device *udev)
{
struct xhci_hcd *xhci = hcd_to_xhci(hcd);
+ struct device *dev = hcd->self.controller;
unsigned long flags;
int timeleft;
int ret;
@@ -3708,6 +3720,16 @@ int xhci_alloc_dev(struct usb_hcd *hcd, struct
usb_device *udev)
goto disable_slot;
}
udev->slot_id = xhci->slot_id;
+
+#ifndef CONFIG_USB_DEFAULT_PERSIST
+ /*
+* If resetting upon resume, we can't put the controller into runtime
+* suspend if there is a device attached.
+*/
+ if (xhci->quirks & XHCI_RESET_ON_RESUME)
+ pm_runtime_get_noresume(dev);
+#endif
+
/* Is this a LS or FS device under a HS hub? */
/* Hub or peripherial? */
return 1;
--
1.7.12.4
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH v2] usb: xhci: Disable runtime PM suspend for quirky controllers
If a USB controller with XHCI_RESET_ON_RESUME goes to runtime suspend,
a reset will be performed upon runtime resume. Any previously suspended
devices attached to the controller will be re-enumerated at this time.
This will cause problems, for example, if an open system call on the
device triggered the resume (the open call will fail).
Note that this change is only relevant when persist_enabled is not set
for USB devices.
Signed-off-by: Shawn Nematbakhsh sha...@chromium.org
---
Changes since v1:
- Only disable runtime suspend when devices are connected.
- Skip this change if CONFIG_USB_DEFAULT_PERSIST is enabled.
---
Change-Id: I964e5bdfec09d03fac8656aaf566bc48d34ef0f9
---
drivers/usb/host/xhci.c | 22 ++
1 file changed, 22 insertions(+)
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index 9478caa..8adbab2 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -3581,10 +3581,21 @@ void xhci_free_dev(struct usb_hcd *hcd, struct
usb_device *udev)
{
struct xhci_hcd *xhci = hcd_to_xhci(hcd);
struct xhci_virt_device *virt_dev;
+ struct device *dev = hcd-self.controller;
unsigned long flags;
u32 state;
int i, ret;
+#ifndef CONFIG_USB_DEFAULT_PERSIST
+ /*
+* We called pm_runtime_get_noresume when the device was attached.
+* Decrement the counter here to allow controller to runtime suspend
+* if no devices remain.
+*/
+ if (xhci-quirks XHCI_RESET_ON_RESUME)
+ pm_runtime_put_noidle(dev);
+#endif
+
ret = xhci_check_args(hcd, udev, NULL, 0, true, __func__);
/* If the host is halted due to driver unload, we still need to free the
* device.
@@ -3656,6 +3667,7 @@ static int xhci_reserve_host_control_ep_resources(struct
xhci_hcd *xhci)
int xhci_alloc_dev(struct usb_hcd *hcd, struct usb_device *udev)
{
struct xhci_hcd *xhci = hcd_to_xhci(hcd);
+ struct device *dev = hcd-self.controller;
unsigned long flags;
int timeleft;
int ret;
@@ -3708,6 +3720,16 @@ int xhci_alloc_dev(struct usb_hcd *hcd, struct
usb_device *udev)
goto disable_slot;
}
udev-slot_id = xhci-slot_id;
+
+#ifndef CONFIG_USB_DEFAULT_PERSIST
+ /*
+* If resetting upon resume, we can't put the controller into runtime
+* suspend if there is a device attached.
+*/
+ if (xhci-quirks XHCI_RESET_ON_RESUME)
+ pm_runtime_get_noresume(dev);
+#endif
+
/* Is this a LS or FS device under a HS hub? */
/* Hub or peripherial? */
return 1;
--
1.7.12.4
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] usb: xhci: Disable runtime PM suspend for quirky controllers.
Hi Sarah, I will resubmit the patch with these changes shortly. On Fri, Aug 9, 2013 at 10:22 AM, Sarah Sharp wrote: > Hi Shawn, > > I noticed that the ChromeOS kernel tree is still using this particular > patch, and thought it was probably time to revisit it. > > On Sat, May 25, 2013 at 09:57:57AM -0700, Shawn Nematbakhsh wrote: >> Hi Sarah and Alan, >> >> Thanks for the comments. I will make the following revisions: >> >> 1. Call pm_runtime_get_noresume only when the first device is connected, >> and call pm_runtime_put when the last device is disconnected. >> 2. Wrap the calls in an ifdef CONFIG_USB_DEFAULT_PERSIST. >> 3. Make sure that all pm_runtime_get_noresume calls have a corresponding >> pm_runtime_put somewhere (originally the intent was to disable runtime >> suspend "forever", but no longer). >> >> In principle, would the patch be acceptable with these revisions? > > The thread petered off with all other options turning out to be > dead-ends, so yes, if you made those changes, you could get that patch > upstream. I would like the ChromeOS kernel to be as close to upstream > as possible, so please resubmit this patch with those changes. > > Thanks, > Sarah Sharp > >> >> On Sat, May 25, 2013 at 7:11 AM, Alan Stern wrote: >> >> > On Fri, 24 May 2013, Sarah Sharp wrote: >> > >> > > On Fri, May 24, 2013 at 11:12:57AM -0700, Shawn Nematbakhsh wrote: >> > > > If a USB controller with XHCI_RESET_ON_RESUME goes to runtime suspend, >> > > > a reset will be performed upon runtime resume. Any previously suspended >> > > > devices attached to the controller will be re-enumerated at this time. >> > > > This will cause problems, for example, if an open system call on the >> > > > device triggered the resume (the open call will fail). >> > > >> > > That's ugly, but disabling runtime PM is going to have a big impact on >> > > the power consumption of those systems. >> > > >> > > Alan, do you think this is really the right thing to be doing here? It >> > > feels like userspace should just be able to deal with devices >> > > disconnecting on resume. After all, there are lots of USB devices that >> > > can't handle USB device suspend at all. >> > >> > This is a complicated issue. It depends on the runtime PM settings for >> > both the device and the host controller. >> > >> > As just one aspect, consider the fact that if it wants to, userspace >> > can already prevent the controller from going into runtime suspend. >> > Always preventing this at the kernel level, even when no devices are >> > plugged in, does seem too heavy-handed. >> > >> > > Shouldn't userspace just disable runtime PM for the USB device classes >> > > that don't have a reset resume callback? >> > >> > That's not so easy, because the kernel changes over time. Userspace >> > has no general way to tell which drivers have reset-resume support. >> > >> > > > Note that this change is only relevant when persist_enabled is not set >> > > > for USB devices. >> > > >> > > Could we at least wrap the call in an ifdef CONFIG_USB_DEFAULT_PERSIST? >> > > That way if people have USB persist turned off in their configuration, >> > > their host will still be able to suspend. >> > >> > Not just that; the patch is incorrect on the face of it... >> > >> > > > @@ -4687,6 +4687,12 @@ int xhci_gen_setup(struct usb_hcd *hcd, >> > xhci_get_quirks_t get_quirks) >> > > > >> > > > get_quirks(dev, xhci); >> > > > >> > > > + /* If we are resetting upon resume, we must disable runtime PM. >> > > > +* Otherwise, an open() syscall to a device on our runtime >> > suspended >> > > > +* controller will trigger controller reset and device >> > re-enumeration */ >> > > > + if (xhci->quirks & XHCI_RESET_ON_RESUME) >> > > > + pm_runtime_get_noresume(dev); >> > > > + >> > >> > It adds a pm_runtime_get call with no corresponding pm_runtime_put. >> > >> > Alan Stern >> > >> > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] usb: xhci: Disable runtime PM suspend for quirky controllers.
Hi Sarah, I will resubmit the patch with these changes shortly. On Fri, Aug 9, 2013 at 10:22 AM, Sarah Sharp sarah.a.sh...@linux.intel.com wrote: Hi Shawn, I noticed that the ChromeOS kernel tree is still using this particular patch, and thought it was probably time to revisit it. On Sat, May 25, 2013 at 09:57:57AM -0700, Shawn Nematbakhsh wrote: Hi Sarah and Alan, Thanks for the comments. I will make the following revisions: 1. Call pm_runtime_get_noresume only when the first device is connected, and call pm_runtime_put when the last device is disconnected. 2. Wrap the calls in an ifdef CONFIG_USB_DEFAULT_PERSIST. 3. Make sure that all pm_runtime_get_noresume calls have a corresponding pm_runtime_put somewhere (originally the intent was to disable runtime suspend forever, but no longer). In principle, would the patch be acceptable with these revisions? The thread petered off with all other options turning out to be dead-ends, so yes, if you made those changes, you could get that patch upstream. I would like the ChromeOS kernel to be as close to upstream as possible, so please resubmit this patch with those changes. Thanks, Sarah Sharp On Sat, May 25, 2013 at 7:11 AM, Alan Stern st...@rowland.harvard.eduwrote: On Fri, 24 May 2013, Sarah Sharp wrote: On Fri, May 24, 2013 at 11:12:57AM -0700, Shawn Nematbakhsh wrote: If a USB controller with XHCI_RESET_ON_RESUME goes to runtime suspend, a reset will be performed upon runtime resume. Any previously suspended devices attached to the controller will be re-enumerated at this time. This will cause problems, for example, if an open system call on the device triggered the resume (the open call will fail). That's ugly, but disabling runtime PM is going to have a big impact on the power consumption of those systems. Alan, do you think this is really the right thing to be doing here? It feels like userspace should just be able to deal with devices disconnecting on resume. After all, there are lots of USB devices that can't handle USB device suspend at all. This is a complicated issue. It depends on the runtime PM settings for both the device and the host controller. As just one aspect, consider the fact that if it wants to, userspace can already prevent the controller from going into runtime suspend. Always preventing this at the kernel level, even when no devices are plugged in, does seem too heavy-handed. Shouldn't userspace just disable runtime PM for the USB device classes that don't have a reset resume callback? That's not so easy, because the kernel changes over time. Userspace has no general way to tell which drivers have reset-resume support. Note that this change is only relevant when persist_enabled is not set for USB devices. Could we at least wrap the call in an ifdef CONFIG_USB_DEFAULT_PERSIST? That way if people have USB persist turned off in their configuration, their host will still be able to suspend. Not just that; the patch is incorrect on the face of it... @@ -4687,6 +4687,12 @@ int xhci_gen_setup(struct usb_hcd *hcd, xhci_get_quirks_t get_quirks) get_quirks(dev, xhci); + /* If we are resetting upon resume, we must disable runtime PM. +* Otherwise, an open() syscall to a device on our runtime suspended +* controller will trigger controller reset and device re-enumeration */ + if (xhci-quirks XHCI_RESET_ON_RESUME) + pm_runtime_get_noresume(dev); + It adds a pm_runtime_get call with no corresponding pm_runtime_put. Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] usb: xhci: Disable runtime PM suspend for quirky controllers.
Hi Sarah and Alan, Thanks for the comments. I will make the following revisions: 1. Call pm_runtime_get_noresume only when the first device is connected, and call pm_runtime_put when the last device is disconnected. 2. Wrap the calls in an ifdef CONFIG_USB_DEFAULT_PERSIST. 3. Make sure that all pm_runtime_get_noresume calls have a corresponding pm_runtime_put somewhere (originally the intent was to disable runtime suspend "forever", but no longer). In principle, would the patch be acceptable with these revisions? On Sat, May 25, 2013 at 7:11 AM, Alan Stern wrote: > On Fri, 24 May 2013, Sarah Sharp wrote: > >> On Fri, May 24, 2013 at 11:12:57AM -0700, Shawn Nematbakhsh wrote: >> > If a USB controller with XHCI_RESET_ON_RESUME goes to runtime suspend, >> > a reset will be performed upon runtime resume. Any previously suspended >> > devices attached to the controller will be re-enumerated at this time. >> > This will cause problems, for example, if an open system call on the >> > device triggered the resume (the open call will fail). >> >> That's ugly, but disabling runtime PM is going to have a big impact on >> the power consumption of those systems. >> >> Alan, do you think this is really the right thing to be doing here? It >> feels like userspace should just be able to deal with devices >> disconnecting on resume. After all, there are lots of USB devices that >> can't handle USB device suspend at all. > > This is a complicated issue. It depends on the runtime PM settings for > both the device and the host controller. > > As just one aspect, consider the fact that if it wants to, userspace > can already prevent the controller from going into runtime suspend. > Always preventing this at the kernel level, even when no devices are > plugged in, does seem too heavy-handed. > >> Shouldn't userspace just disable runtime PM for the USB device classes >> that don't have a reset resume callback? > > That's not so easy, because the kernel changes over time. Userspace > has no general way to tell which drivers have reset-resume support. > >> > Note that this change is only relevant when persist_enabled is not set >> > for USB devices. >> >> Could we at least wrap the call in an ifdef CONFIG_USB_DEFAULT_PERSIST? >> That way if people have USB persist turned off in their configuration, >> their host will still be able to suspend. > > Not just that; the patch is incorrect on the face of it... > >> > @@ -4687,6 +4687,12 @@ int xhci_gen_setup(struct usb_hcd *hcd, >> > xhci_get_quirks_t get_quirks) >> > >> > get_quirks(dev, xhci); >> > >> > + /* If we are resetting upon resume, we must disable runtime PM. >> > +* Otherwise, an open() syscall to a device on our runtime suspended >> > +* controller will trigger controller reset and device re-enumeration >> > */ >> > + if (xhci->quirks & XHCI_RESET_ON_RESUME) >> > + pm_runtime_get_noresume(dev); >> > + > > It adds a pm_runtime_get call with no corresponding pm_runtime_put. > > Alan Stern > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] usb: xhci: Disable runtime PM suspend for quirky controllers.
Hi Sarah and Alan, Thanks for the comments. I will make the following revisions: 1. Call pm_runtime_get_noresume only when the first device is connected, and call pm_runtime_put when the last device is disconnected. 2. Wrap the calls in an ifdef CONFIG_USB_DEFAULT_PERSIST. 3. Make sure that all pm_runtime_get_noresume calls have a corresponding pm_runtime_put somewhere (originally the intent was to disable runtime suspend forever, but no longer). In principle, would the patch be acceptable with these revisions? On Sat, May 25, 2013 at 7:11 AM, Alan Stern st...@rowland.harvard.edu wrote: On Fri, 24 May 2013, Sarah Sharp wrote: On Fri, May 24, 2013 at 11:12:57AM -0700, Shawn Nematbakhsh wrote: If a USB controller with XHCI_RESET_ON_RESUME goes to runtime suspend, a reset will be performed upon runtime resume. Any previously suspended devices attached to the controller will be re-enumerated at this time. This will cause problems, for example, if an open system call on the device triggered the resume (the open call will fail). That's ugly, but disabling runtime PM is going to have a big impact on the power consumption of those systems. Alan, do you think this is really the right thing to be doing here? It feels like userspace should just be able to deal with devices disconnecting on resume. After all, there are lots of USB devices that can't handle USB device suspend at all. This is a complicated issue. It depends on the runtime PM settings for both the device and the host controller. As just one aspect, consider the fact that if it wants to, userspace can already prevent the controller from going into runtime suspend. Always preventing this at the kernel level, even when no devices are plugged in, does seem too heavy-handed. Shouldn't userspace just disable runtime PM for the USB device classes that don't have a reset resume callback? That's not so easy, because the kernel changes over time. Userspace has no general way to tell which drivers have reset-resume support. Note that this change is only relevant when persist_enabled is not set for USB devices. Could we at least wrap the call in an ifdef CONFIG_USB_DEFAULT_PERSIST? That way if people have USB persist turned off in their configuration, their host will still be able to suspend. Not just that; the patch is incorrect on the face of it... @@ -4687,6 +4687,12 @@ int xhci_gen_setup(struct usb_hcd *hcd, xhci_get_quirks_t get_quirks) get_quirks(dev, xhci); + /* If we are resetting upon resume, we must disable runtime PM. +* Otherwise, an open() syscall to a device on our runtime suspended +* controller will trigger controller reset and device re-enumeration */ + if (xhci-quirks XHCI_RESET_ON_RESUME) + pm_runtime_get_noresume(dev); + It adds a pm_runtime_get call with no corresponding pm_runtime_put. Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] usb: xhci: Disable runtime PM suspend for quirky controllers.
If a USB controller with XHCI_RESET_ON_RESUME goes to runtime suspend, a reset will be performed upon runtime resume. Any previously suspended devices attached to the controller will be re-enumerated at this time. This will cause problems, for example, if an open system call on the device triggered the resume (the open call will fail). Note that this change is only relevant when persist_enabled is not set for USB devices. Signed-off-by: Shawn Nematbakhsh --- drivers/usb/host/xhci.c | 6 ++ 1 file changed, 6 insertions(+) diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c index b4aa79d..7455156 100644 --- a/drivers/usb/host/xhci.c +++ b/drivers/usb/host/xhci.c @@ -4687,6 +4687,12 @@ int xhci_gen_setup(struct usb_hcd *hcd, xhci_get_quirks_t get_quirks) get_quirks(dev, xhci); + /* If we are resetting upon resume, we must disable runtime PM. +* Otherwise, an open() syscall to a device on our runtime suspended +* controller will trigger controller reset and device re-enumeration */ + if (xhci->quirks & XHCI_RESET_ON_RESUME) + pm_runtime_get_noresume(dev); + /* Make sure the HC is halted. */ retval = xhci_halt(xhci); if (retval) -- 1.7.12.4 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] usb: xhci: Disable runtime PM suspend for quirky controllers.
If a USB controller with XHCI_RESET_ON_RESUME goes to runtime suspend, a reset will be performed upon runtime resume. Any previously suspended devices attached to the controller will be re-enumerated at this time. This will cause problems, for example, if an open system call on the device triggered the resume (the open call will fail). Note that this change is only relevant when persist_enabled is not set for USB devices. Signed-off-by: Shawn Nematbakhsh sha...@chromium.org --- drivers/usb/host/xhci.c | 6 ++ 1 file changed, 6 insertions(+) diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c index b4aa79d..7455156 100644 --- a/drivers/usb/host/xhci.c +++ b/drivers/usb/host/xhci.c @@ -4687,6 +4687,12 @@ int xhci_gen_setup(struct usb_hcd *hcd, xhci_get_quirks_t get_quirks) get_quirks(dev, xhci); + /* If we are resetting upon resume, we must disable runtime PM. +* Otherwise, an open() syscall to a device on our runtime suspended +* controller will trigger controller reset and device re-enumeration */ + if (xhci-quirks XHCI_RESET_ON_RESUME) + pm_runtime_get_noresume(dev); + /* Make sure the HC is halted. */ retval = xhci_halt(xhci); if (retval) -- 1.7.12.4 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] [media] uvcvideo: Retry usb_submit_urb on -EPERM return
Hi Laurent,
Thanks for the changes! I agree that your synchronization logic is
correct. Just two small comments:
On Mon, Apr 29, 2013 at 1:34 PM, Laurent Pinchart
wrote:
> Hi Shawn,
>
> Thank you for the patch.
>
> On Tuesday 23 April 2013 17:42:32 Shawn Nematbakhsh wrote:
>> While usb_kill_urb is in progress, calls to usb_submit_urb will fail
>> with -EPERM (documented in Documentation/usb/URB.txt). The UVC driver
>> does not correctly handle this case -- there is no synchronization
>> between uvc_v4l2_open / uvc_status_start and uvc_v4l2_release /
>> uvc_status_stop.
>
> Wouldn't it be better to synchronize status operations in open/release ?
> Something like the following patch:
>
> From 9285d678ed2f823bb215f6bdec3ca1a9e1cac977 Mon Sep 17 00:00:00 2001
> From: Laurent Pinchart
> Date: Fri, 26 Apr 2013 03:28:51 +0200
> Subject: [PATCH] uvcvideo: Fix open/close race condition
>
> Maintaining the users count using an atomic variable makes sure that
> access to the counter won't be racy, but doesn't serialize access to the
> operations protected by the counter. This creates a race condition that
> could result in the status URB being submitted multiple times.
>
> Use a mutex to protect the users count and serialize access to the
> status start and stop operations.
>
> Reported-by: Shawn Nematbakhsh
> Signed-off-by: Laurent Pinchart
> ---
> drivers/media/usb/uvc/uvc_driver.c | 22 --
> drivers/media/usb/uvc/uvc_status.c | 21 ++---
> drivers/media/usb/uvc/uvc_v4l2.c | 14 ++
> drivers/media/usb/uvc/uvcvideo.h | 7 +++
> 4 files changed, 31 insertions(+), 33 deletions(-)
>
> diff --git a/drivers/media/usb/uvc/uvc_driver.c
> b/drivers/media/usb/uvc/uvc_driver.c
> index e68fa53..b638037 100644
> --- a/drivers/media/usb/uvc/uvc_driver.c
> +++ b/drivers/media/usb/uvc/uvc_driver.c
> @@ -1836,8 +1836,8 @@ static int uvc_probe(struct usb_interface *intf,
> INIT_LIST_HEAD(>chains);
> INIT_LIST_HEAD(>streams);
> atomic_set(>nstreams, 0);
> - atomic_set(>users, 0);
I think dev->users is uninitialized now? Probably we should initialize
to 0 here.
> atomic_set(>nmappings, 0);
> + mutex_init(>lock);
>
> dev->udev = usb_get_dev(udev);
> dev->intf = usb_get_intf(intf);
> @@ -1953,8 +1953,12 @@ static int uvc_suspend(struct usb_interface *intf,
> pm_message_t message)
>
> /* Controls are cached on the fly so they don't need to be saved. */
> if (intf->cur_altsetting->desc.bInterfaceSubClass ==
> - UVC_SC_VIDEOCONTROL)
> - return uvc_status_suspend(dev);
> + UVC_SC_VIDEOCONTROL) {
> + mutex_lock(>lock);
> + if (dev->users)
> + uvc_status_stop(dev);
> + mutex_unlock(>lock);
To keep the same control flow, should we return here?
> + }
>
> list_for_each_entry(stream, >streams, list) {
> if (stream->intf == intf)
> @@ -1976,14 +1980,20 @@ static int __uvc_resume(struct usb_interface *intf,
> int reset)
>
> if (intf->cur_altsetting->desc.bInterfaceSubClass ==
> UVC_SC_VIDEOCONTROL) {
> - if (reset) {
> - int ret = uvc_ctrl_resume_device(dev);
> + int ret = 0;
>
> + if (reset) {
> + ret = uvc_ctrl_resume_device(dev);
> if (ret < 0)
> return ret;
> }
>
> - return uvc_status_resume(dev);
> + mutex_lock(>lock);
> + if (dev->users)
> + ret = uvc_status_start(dev, GFP_NOIO);
> + mutex_unlock(>lock);
> +
> + return ret;
> }
>
> list_for_each_entry(stream, >streams, list) {
> diff --git a/drivers/media/usb/uvc/uvc_status.c
> b/drivers/media/usb/uvc/uvc_status.c
> index b749277..f552ab9 100644
> --- a/drivers/media/usb/uvc/uvc_status.c
> +++ b/drivers/media/usb/uvc/uvc_status.c
> @@ -206,32 +206,15 @@ void uvc_status_cleanup(struct uvc_device *dev)
> uvc_input_cleanup(dev);
> }
>
> -int uvc_status_start(struct uvc_device *dev)
> +int uvc_status_start(struct uvc_device *dev, gfp_t flags)
> {
> if (dev->int_urb == NULL)
> return 0;
>
> - return usb_submit_urb(dev->int_urb, GFP_KERNEL);
> + return usb_submit_urb(dev->int_urb, flags);
> }
>
> void uvc_status_stop(struct uvc_device *dev)
> {
> usb_kill_urb(de
Re: [PATCH] [media] uvcvideo: Retry usb_submit_urb on -EPERM return
Hi Laurent,
Thanks for the changes! I agree that your synchronization logic is
correct. Just two small comments:
On Mon, Apr 29, 2013 at 1:34 PM, Laurent Pinchart
laurent.pinch...@ideasonboard.com wrote:
Hi Shawn,
Thank you for the patch.
On Tuesday 23 April 2013 17:42:32 Shawn Nematbakhsh wrote:
While usb_kill_urb is in progress, calls to usb_submit_urb will fail
with -EPERM (documented in Documentation/usb/URB.txt). The UVC driver
does not correctly handle this case -- there is no synchronization
between uvc_v4l2_open / uvc_status_start and uvc_v4l2_release /
uvc_status_stop.
Wouldn't it be better to synchronize status operations in open/release ?
Something like the following patch:
From 9285d678ed2f823bb215f6bdec3ca1a9e1cac977 Mon Sep 17 00:00:00 2001
From: Laurent Pinchart laurent.pinch...@ideasonboard.com
Date: Fri, 26 Apr 2013 03:28:51 +0200
Subject: [PATCH] uvcvideo: Fix open/close race condition
Maintaining the users count using an atomic variable makes sure that
access to the counter won't be racy, but doesn't serialize access to the
operations protected by the counter. This creates a race condition that
could result in the status URB being submitted multiple times.
Use a mutex to protect the users count and serialize access to the
status start and stop operations.
Reported-by: Shawn Nematbakhsh sha...@chromium.org
Signed-off-by: Laurent Pinchart laurent.pinch...@ideasonboard.com
---
drivers/media/usb/uvc/uvc_driver.c | 22 --
drivers/media/usb/uvc/uvc_status.c | 21 ++---
drivers/media/usb/uvc/uvc_v4l2.c | 14 ++
drivers/media/usb/uvc/uvcvideo.h | 7 +++
4 files changed, 31 insertions(+), 33 deletions(-)
diff --git a/drivers/media/usb/uvc/uvc_driver.c
b/drivers/media/usb/uvc/uvc_driver.c
index e68fa53..b638037 100644
--- a/drivers/media/usb/uvc/uvc_driver.c
+++ b/drivers/media/usb/uvc/uvc_driver.c
@@ -1836,8 +1836,8 @@ static int uvc_probe(struct usb_interface *intf,
INIT_LIST_HEAD(dev-chains);
INIT_LIST_HEAD(dev-streams);
atomic_set(dev-nstreams, 0);
- atomic_set(dev-users, 0);
I think dev-users is uninitialized now? Probably we should initialize
to 0 here.
atomic_set(dev-nmappings, 0);
+ mutex_init(dev-lock);
dev-udev = usb_get_dev(udev);
dev-intf = usb_get_intf(intf);
@@ -1953,8 +1953,12 @@ static int uvc_suspend(struct usb_interface *intf,
pm_message_t message)
/* Controls are cached on the fly so they don't need to be saved. */
if (intf-cur_altsetting-desc.bInterfaceSubClass ==
- UVC_SC_VIDEOCONTROL)
- return uvc_status_suspend(dev);
+ UVC_SC_VIDEOCONTROL) {
+ mutex_lock(dev-lock);
+ if (dev-users)
+ uvc_status_stop(dev);
+ mutex_unlock(dev-lock);
To keep the same control flow, should we return here?
+ }
list_for_each_entry(stream, dev-streams, list) {
if (stream-intf == intf)
@@ -1976,14 +1980,20 @@ static int __uvc_resume(struct usb_interface *intf,
int reset)
if (intf-cur_altsetting-desc.bInterfaceSubClass ==
UVC_SC_VIDEOCONTROL) {
- if (reset) {
- int ret = uvc_ctrl_resume_device(dev);
+ int ret = 0;
+ if (reset) {
+ ret = uvc_ctrl_resume_device(dev);
if (ret 0)
return ret;
}
- return uvc_status_resume(dev);
+ mutex_lock(dev-lock);
+ if (dev-users)
+ ret = uvc_status_start(dev, GFP_NOIO);
+ mutex_unlock(dev-lock);
+
+ return ret;
}
list_for_each_entry(stream, dev-streams, list) {
diff --git a/drivers/media/usb/uvc/uvc_status.c
b/drivers/media/usb/uvc/uvc_status.c
index b749277..f552ab9 100644
--- a/drivers/media/usb/uvc/uvc_status.c
+++ b/drivers/media/usb/uvc/uvc_status.c
@@ -206,32 +206,15 @@ void uvc_status_cleanup(struct uvc_device *dev)
uvc_input_cleanup(dev);
}
-int uvc_status_start(struct uvc_device *dev)
+int uvc_status_start(struct uvc_device *dev, gfp_t flags)
{
if (dev-int_urb == NULL)
return 0;
- return usb_submit_urb(dev-int_urb, GFP_KERNEL);
+ return usb_submit_urb(dev-int_urb, flags);
}
void uvc_status_stop(struct uvc_device *dev)
{
usb_kill_urb(dev-int_urb);
}
-
-int uvc_status_suspend(struct uvc_device *dev)
-{
- if (atomic_read(dev-users))
- usb_kill_urb(dev-int_urb);
-
- return 0;
-}
-
-int uvc_status_resume(struct uvc_device *dev)
-{
- if (dev-int_urb == NULL || atomic_read(dev-users) == 0)
- return 0;
-
- return usb_submit_urb(dev-int_urb, GFP_NOIO);
-}
-
diff --git a/drivers
[PATCH] [media] uvcvideo: Retry usb_submit_urb on -EPERM return
From: Shawn Nematbakhsh
While usb_kill_urb is in progress, calls to usb_submit_urb will fail
with -EPERM (documented in Documentation/usb/URB.txt). The UVC driver
does not correctly handle this case -- there is no synchronization
between uvc_v4l2_open / uvc_status_start and uvc_v4l2_release /
uvc_status_stop.
This patch adds a retry / timeout when uvc_status_open / usb_submit_urb
returns -EPERM. This usually means that usb_kill_urb is in progress, and
we just need to wait a while.
Signed-off-by: Shawn Nematbakhsh
---
drivers/media/usb/uvc/uvc_v4l2.c | 10 +-
drivers/media/usb/uvc/uvcvideo.h | 1 +
2 files changed, 10 insertions(+), 1 deletion(-)
diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
index b2dc326..f1498a8 100644
--- a/drivers/media/usb/uvc/uvc_v4l2.c
+++ b/drivers/media/usb/uvc/uvc_v4l2.c
@@ -479,6 +479,7 @@ static int uvc_v4l2_open(struct file *file)
{
struct uvc_streaming *stream;
struct uvc_fh *handle;
+ unsigned long timeout;
int ret = 0;
uvc_trace(UVC_TRACE_CALLS, "uvc_v4l2_open\n");
@@ -499,7 +500,14 @@ static int uvc_v4l2_open(struct file *file)
}
if (atomic_inc_return(>dev->users) == 1) {
- ret = uvc_status_start(stream->dev);
+ timeout = jiffies + msecs_to_jiffies(UVC_STATUS_START_TIMEOUT);
+ /* -EPERM means stop in progress, wait for completion */
+ do {
+ ret = uvc_status_start(stream->dev);
+ if (ret == -EPERM)
+ usleep_range(5000, 6000);
+ } while (ret == -EPERM && time_before(jiffies, timeout));
+
if (ret < 0) {
atomic_dec(>dev->users);
usb_autopm_put_interface(stream->dev->intf);
diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
index af505fd..a47e1d3 100644
--- a/drivers/media/usb/uvc/uvcvideo.h
+++ b/drivers/media/usb/uvc/uvcvideo.h
@@ -122,6 +122,7 @@
#define UVC_CTRL_CONTROL_TIMEOUT 300
#define UVC_CTRL_STREAMING_TIMEOUT 5000
+#define UVC_STATUS_START_TIMEOUT 100
/* Maximum allowed number of control mappings per device */
#define UVC_MAX_CONTROL_MAPPINGS 1024
--
1.7.12.4
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] [media] uvcvideo: Retry usb_submit_urb on -EPERM return
While usb_kill_urb is in progress, calls to usb_submit_urb will fail
with -EPERM (documented in Documentation/usb/URB.txt). The UVC driver
does not correctly handle this case -- there is no synchronization
between uvc_v4l2_open / uvc_status_start and uvc_v4l2_release /
uvc_status_stop.
This patch adds a retry / timeout when uvc_status_open / usb_submit_urb
returns -EPERM. This usually means that usb_kill_urb is in progress, and
we just need to wait a while.
Signed-off-by: Shawn Nematbakhsh
---
drivers/media/usb/uvc/uvc_v4l2.c | 10 +-
drivers/media/usb/uvc/uvcvideo.h | 1 +
2 files changed, 10 insertions(+), 1 deletion(-)
diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
index b2dc326..f1498a8 100644
--- a/drivers/media/usb/uvc/uvc_v4l2.c
+++ b/drivers/media/usb/uvc/uvc_v4l2.c
@@ -479,6 +479,7 @@ static int uvc_v4l2_open(struct file *file)
{
struct uvc_streaming *stream;
struct uvc_fh *handle;
+ unsigned long timeout;
int ret = 0;
uvc_trace(UVC_TRACE_CALLS, "uvc_v4l2_open\n");
@@ -499,7 +500,14 @@ static int uvc_v4l2_open(struct file *file)
}
if (atomic_inc_return(>dev->users) == 1) {
- ret = uvc_status_start(stream->dev);
+ timeout = jiffies + msecs_to_jiffies(UVC_STATUS_START_TIMEOUT);
+ /* -EPERM means stop in progress, wait for completion */
+ do {
+ ret = uvc_status_start(stream->dev);
+ if (ret == -EPERM)
+ usleep_range(5000, 6000);
+ } while (ret == -EPERM && time_before(jiffies, timeout));
+
if (ret < 0) {
atomic_dec(>dev->users);
usb_autopm_put_interface(stream->dev->intf);
diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
index af505fd..a47e1d3 100644
--- a/drivers/media/usb/uvc/uvcvideo.h
+++ b/drivers/media/usb/uvc/uvcvideo.h
@@ -122,6 +122,7 @@
#define UVC_CTRL_CONTROL_TIMEOUT 300
#define UVC_CTRL_STREAMING_TIMEOUT 5000
+#define UVC_STATUS_START_TIMEOUT 100
/* Maximum allowed number of control mappings per device */
#define UVC_MAX_CONTROL_MAPPINGS 1024
--
1.7.12.4
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] [media] uvcvideo: Retry usb_submit_urb on -EPERM return
While usb_kill_urb is in progress, calls to usb_submit_urb will fail
with -EPERM (documented in Documentation/usb/URB.txt). The UVC driver
does not correctly handle this case -- there is no synchronization
between uvc_v4l2_open / uvc_status_start and uvc_v4l2_release /
uvc_status_stop.
This patch adds a retry / timeout when uvc_status_open / usb_submit_urb
returns -EPERM. This usually means that usb_kill_urb is in progress, and
we just need to wait a while.
Signed-off-by: Shawn Nematbakhsh sha...@chromium.org
---
drivers/media/usb/uvc/uvc_v4l2.c | 10 +-
drivers/media/usb/uvc/uvcvideo.h | 1 +
2 files changed, 10 insertions(+), 1 deletion(-)
diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
index b2dc326..f1498a8 100644
--- a/drivers/media/usb/uvc/uvc_v4l2.c
+++ b/drivers/media/usb/uvc/uvc_v4l2.c
@@ -479,6 +479,7 @@ static int uvc_v4l2_open(struct file *file)
{
struct uvc_streaming *stream;
struct uvc_fh *handle;
+ unsigned long timeout;
int ret = 0;
uvc_trace(UVC_TRACE_CALLS, uvc_v4l2_open\n);
@@ -499,7 +500,14 @@ static int uvc_v4l2_open(struct file *file)
}
if (atomic_inc_return(stream-dev-users) == 1) {
- ret = uvc_status_start(stream-dev);
+ timeout = jiffies + msecs_to_jiffies(UVC_STATUS_START_TIMEOUT);
+ /* -EPERM means stop in progress, wait for completion */
+ do {
+ ret = uvc_status_start(stream-dev);
+ if (ret == -EPERM)
+ usleep_range(5000, 6000);
+ } while (ret == -EPERM time_before(jiffies, timeout));
+
if (ret 0) {
atomic_dec(stream-dev-users);
usb_autopm_put_interface(stream-dev-intf);
diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
index af505fd..a47e1d3 100644
--- a/drivers/media/usb/uvc/uvcvideo.h
+++ b/drivers/media/usb/uvc/uvcvideo.h
@@ -122,6 +122,7 @@
#define UVC_CTRL_CONTROL_TIMEOUT 300
#define UVC_CTRL_STREAMING_TIMEOUT 5000
+#define UVC_STATUS_START_TIMEOUT 100
/* Maximum allowed number of control mappings per device */
#define UVC_MAX_CONTROL_MAPPINGS 1024
--
1.7.12.4
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] [media] uvcvideo: Retry usb_submit_urb on -EPERM return
From: Shawn Nematbakhsh sha...@chromium.org
While usb_kill_urb is in progress, calls to usb_submit_urb will fail
with -EPERM (documented in Documentation/usb/URB.txt). The UVC driver
does not correctly handle this case -- there is no synchronization
between uvc_v4l2_open / uvc_status_start and uvc_v4l2_release /
uvc_status_stop.
This patch adds a retry / timeout when uvc_status_open / usb_submit_urb
returns -EPERM. This usually means that usb_kill_urb is in progress, and
we just need to wait a while.
Signed-off-by: Shawn Nematbakhsh sha...@chromium.org
---
drivers/media/usb/uvc/uvc_v4l2.c | 10 +-
drivers/media/usb/uvc/uvcvideo.h | 1 +
2 files changed, 10 insertions(+), 1 deletion(-)
diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
index b2dc326..f1498a8 100644
--- a/drivers/media/usb/uvc/uvc_v4l2.c
+++ b/drivers/media/usb/uvc/uvc_v4l2.c
@@ -479,6 +479,7 @@ static int uvc_v4l2_open(struct file *file)
{
struct uvc_streaming *stream;
struct uvc_fh *handle;
+ unsigned long timeout;
int ret = 0;
uvc_trace(UVC_TRACE_CALLS, uvc_v4l2_open\n);
@@ -499,7 +500,14 @@ static int uvc_v4l2_open(struct file *file)
}
if (atomic_inc_return(stream-dev-users) == 1) {
- ret = uvc_status_start(stream-dev);
+ timeout = jiffies + msecs_to_jiffies(UVC_STATUS_START_TIMEOUT);
+ /* -EPERM means stop in progress, wait for completion */
+ do {
+ ret = uvc_status_start(stream-dev);
+ if (ret == -EPERM)
+ usleep_range(5000, 6000);
+ } while (ret == -EPERM time_before(jiffies, timeout));
+
if (ret 0) {
atomic_dec(stream-dev-users);
usb_autopm_put_interface(stream-dev-intf);
diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
index af505fd..a47e1d3 100644
--- a/drivers/media/usb/uvc/uvcvideo.h
+++ b/drivers/media/usb/uvc/uvcvideo.h
@@ -122,6 +122,7 @@
#define UVC_CTRL_CONTROL_TIMEOUT 300
#define UVC_CTRL_STREAMING_TIMEOUT 5000
+#define UVC_STATUS_START_TIMEOUT 100
/* Maximum allowed number of control mappings per device */
#define UVC_MAX_CONTROL_MAPPINGS 1024
--
1.7.12.4
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Input: trackpoint - Optimize trackpoint init to use power-on reset
Hi Dmitry,
On Tue, Apr 16, 2013 at 4:09 PM, Dmitry Torokhov
wrote:
> Hi Shawn,
>
> On Tue, Apr 09, 2013 at 02:53:44PM -0700, Shawn Nematbakhsh wrote:
>> Hi Dmitry,
>>
>> Thanks for the review. Comments in-line.
>>
>> On Wed, Mar 27, 2013 at 10:32 PM, Dmitry Torokhov
>> wrote:
>> > Hi Shawn,
>> >
>> > On Tue, Mar 26, 2013 at 12:36:41PM -0700, Shawn Nematbakhsh wrote:
>> >> The trackpoint driver sets various parameter default values, all of
>> >> which happen to be power-on defaults (Source: IBM TrackPoint Engineering
>> >> Specification, Version 4.0. Also confirmed by empirical data).
>> >>
>> >> By sending the power-on reset command to reset all parameters to
>> >> power-on state, we can skip the lengthy process of programming all
>> >> parameters. In testing, ~2.5 secs of time writing parameters was reduced
>> >> to .35 seconds waiting for power-on reset to complete.
>> >>
>> >> Signed-off-by: Shawn Nematbakhsh
>> >> ---
>> >> drivers/input/mouse/trackpoint.c | 223
>> >> +--
>> >> drivers/input/mouse/trackpoint.h | 7 +-
>> >> 2 files changed, 149 insertions(+), 81 deletions(-)
>> >>
>> >> diff --git a/drivers/input/mouse/trackpoint.c
>> >> b/drivers/input/mouse/trackpoint.c
>> >> index f310249..17e9b36 100644
>> >> --- a/drivers/input/mouse/trackpoint.c
>> >> +++ b/drivers/input/mouse/trackpoint.c
>> >> @@ -20,6 +20,26 @@
>> >> #include "trackpoint.h"
>> >>
>> >> /*
>> >> + * Power-on Reset: Resets all trackpoint parameters, including RAM
>> >> values,
>> >> + * to defaults.
>> >> + * Returns zero on success, non-zero on failure.
>> >> + */
>> >> +static int trackpoint_power_on_reset(struct ps2dev *ps2dev)
>> >> +{
>> >> + unsigned char results[2];
>> >> +
>> >> + if (ps2_command(ps2dev, NULL, MAKE_PS2_CMD(0, 0, TP_COMMAND)) ||
>> >> + ps2_command(ps2dev, results, MAKE_PS2_CMD(0, 2, TP_POR))) {
>> >> + return -1;
>> >> + }
>> >> +
>> >> + /* POR response should be 0xAA00 or 0xFC00 */
>> >> + if ((results[0] != 0xAA && results[0] != 0xFC) || results[1] !=
>> >> 0x00)
>> >> + return -1;
>> >
>> > I am a bit concerned about this. 0xfc00 indicates POR failure. In this
>> > case shouldn't we try to reset the device, issue another POR and bail if
>> > it fails again?
>> >
>>
>> Yes, you are correct. I just now posted v2 to fix this. Now, on
>> 0xfc00, we attempt one retry. On subsequent 0xfc00, we fail POR and
>> attempt to set parameters manually.
>>
>> >>
>> >> static struct attribute *trackpoint_attrs[] = {
>> >> _attr_sensitivity.dattr.attr,
>> >> @@ -179,6 +241,9 @@ static struct attribute *trackpoint_attrs[] = {
>> >> _attr_press_to_select.dattr.attr,
>> >> _attr_skipback.dattr.attr,
>> >> _attr_ext_dev.dattr.attr,
>> >> + _attr_twohand.dattr.attr,
>> >> + _attr_source_tag.dattr.attr,
>> >> + _attr_mb.dattr.attr,
>> >
>> > What is the benefit of adding these 3 new attributes?
>> >
>>
>> Previously, these attributes were handled in a special way -- the
>> corresponding TP values were set to default, but the attributes were
>> not exported. Now, they are set to default and exported. It makes for
>> cleaner code.
>>
>> I see no good use for these attributes other than driver hacking. I
>> can remove them if you think it's best.
>
>
> Yes, I think that even though having them as attributes makes the code
> look nice we should not be exposing them as they do break driver
> behavior.
>
> Does the version of the patch below work for you?
>
This version looks + tests good, with one small change: Since we no
longer export the three extra parameters, I removed the corresponding
trackpoint_data struct members. Ex.
< @@ -136,10 +138,13 @@ struct trackpoint_data
---
> @@ -136,9 +138,9 @@ struct trackpoint_data
386,388d385
< + unsigned char twohand;
< + unsigned char source_tag;
< + unsigned char mb;
See complete revised patch below.
Input: trackpoint - Optimize trackpoint init to use power-on reset
From: Shawn Nematbakhsh
The trackpoint driver sets various parameter
Re: [PATCH] Input: trackpoint - Optimize trackpoint init to use power-on reset
Hi Dmitry,
On Tue, Apr 16, 2013 at 4:09 PM, Dmitry Torokhov
dmitry.torok...@gmail.com wrote:
Hi Shawn,
On Tue, Apr 09, 2013 at 02:53:44PM -0700, Shawn Nematbakhsh wrote:
Hi Dmitry,
Thanks for the review. Comments in-line.
On Wed, Mar 27, 2013 at 10:32 PM, Dmitry Torokhov
dmitry.torok...@gmail.com wrote:
Hi Shawn,
On Tue, Mar 26, 2013 at 12:36:41PM -0700, Shawn Nematbakhsh wrote:
The trackpoint driver sets various parameter default values, all of
which happen to be power-on defaults (Source: IBM TrackPoint Engineering
Specification, Version 4.0. Also confirmed by empirical data).
By sending the power-on reset command to reset all parameters to
power-on state, we can skip the lengthy process of programming all
parameters. In testing, ~2.5 secs of time writing parameters was reduced
to .35 seconds waiting for power-on reset to complete.
Signed-off-by: Shawn Nematbakhsh sha...@chromium.org
---
drivers/input/mouse/trackpoint.c | 223
+--
drivers/input/mouse/trackpoint.h | 7 +-
2 files changed, 149 insertions(+), 81 deletions(-)
diff --git a/drivers/input/mouse/trackpoint.c
b/drivers/input/mouse/trackpoint.c
index f310249..17e9b36 100644
--- a/drivers/input/mouse/trackpoint.c
+++ b/drivers/input/mouse/trackpoint.c
@@ -20,6 +20,26 @@
#include trackpoint.h
/*
+ * Power-on Reset: Resets all trackpoint parameters, including RAM
values,
+ * to defaults.
+ * Returns zero on success, non-zero on failure.
+ */
+static int trackpoint_power_on_reset(struct ps2dev *ps2dev)
+{
+ unsigned char results[2];
+
+ if (ps2_command(ps2dev, NULL, MAKE_PS2_CMD(0, 0, TP_COMMAND)) ||
+ ps2_command(ps2dev, results, MAKE_PS2_CMD(0, 2, TP_POR))) {
+ return -1;
+ }
+
+ /* POR response should be 0xAA00 or 0xFC00 */
+ if ((results[0] != 0xAA results[0] != 0xFC) || results[1] !=
0x00)
+ return -1;
I am a bit concerned about this. 0xfc00 indicates POR failure. In this
case shouldn't we try to reset the device, issue another POR and bail if
it fails again?
Yes, you are correct. I just now posted v2 to fix this. Now, on
0xfc00, we attempt one retry. On subsequent 0xfc00, we fail POR and
attempt to set parameters manually.
static struct attribute *trackpoint_attrs[] = {
psmouse_attr_sensitivity.dattr.attr,
@@ -179,6 +241,9 @@ static struct attribute *trackpoint_attrs[] = {
psmouse_attr_press_to_select.dattr.attr,
psmouse_attr_skipback.dattr.attr,
psmouse_attr_ext_dev.dattr.attr,
+ psmouse_attr_twohand.dattr.attr,
+ psmouse_attr_source_tag.dattr.attr,
+ psmouse_attr_mb.dattr.attr,
What is the benefit of adding these 3 new attributes?
Previously, these attributes were handled in a special way -- the
corresponding TP values were set to default, but the attributes were
not exported. Now, they are set to default and exported. It makes for
cleaner code.
I see no good use for these attributes other than driver hacking. I
can remove them if you think it's best.
Yes, I think that even though having them as attributes makes the code
look nice we should not be exposing them as they do break driver
behavior.
Does the version of the patch below work for you?
This version looks + tests good, with one small change: Since we no
longer export the three extra parameters, I removed the corresponding
trackpoint_data struct members. Ex.
@@ -136,10 +138,13 @@ struct trackpoint_data
---
@@ -136,9 +138,9 @@ struct trackpoint_data
386,388d385
+ unsigned char twohand;
+ unsigned char source_tag;
+ unsigned char mb;
See complete revised patch below.
Input: trackpoint - Optimize trackpoint init to use power-on reset
From: Shawn Nematbakhsh sha...@chromium.org
The trackpoint driver sets various parameter default values, all of
which happen to be power-on defaults (Source: IBM TrackPoint Engineering
Specification, Version 4.0. Also confirmed by empirical data).
By sending the power-on reset command to reset all parameters to
power-on state, we can skip the lengthy process of programming all
parameters. In testing, ~2.5 secs of time writing parameters was reduced
to .35 seconds waiting for power-on reset to complete.
Signed-off-by: Shawn Nematbakhsh sha...@chromium.org
Signed-off-by: Dmitry Torokhov dmitry.torok...@gmail.com
---
drivers/input/mouse/trackpoint.c | 249 +--
drivers/input/mouse/trackpoint.h | 4 +-
2 files changed, 166 insertions(+), 87 deletions(-)
diff --git a/drivers/input/mouse/trackpoint.c b/drivers/input/mouse/trackpoint.c
index f310249..ca843b6 100644
--- a/drivers/input/mouse/trackpoint.c
+++ b/drivers/input/mouse/trackpoint.c
@@ -20,9 +20,34 @@
#include trackpoint.h
/*
+ * Power-on Reset: Resets all trackpoint parameters, including RAM values,
+ * to defaults.
+ * Returns zero on success, non-zero on failure
Re: [PATCH] Input: trackpoint - Optimize trackpoint init to use power-on reset
Hi Dmitry,
Thanks for the review. Comments in-line.
On Wed, Mar 27, 2013 at 10:32 PM, Dmitry Torokhov
wrote:
> Hi Shawn,
>
> On Tue, Mar 26, 2013 at 12:36:41PM -0700, Shawn Nematbakhsh wrote:
>> The trackpoint driver sets various parameter default values, all of
>> which happen to be power-on defaults (Source: IBM TrackPoint Engineering
>> Specification, Version 4.0. Also confirmed by empirical data).
>>
>> By sending the power-on reset command to reset all parameters to
>> power-on state, we can skip the lengthy process of programming all
>> parameters. In testing, ~2.5 secs of time writing parameters was reduced
>> to .35 seconds waiting for power-on reset to complete.
>>
>> Signed-off-by: Shawn Nematbakhsh
>> ---
>> drivers/input/mouse/trackpoint.c | 223
>> +--
>> drivers/input/mouse/trackpoint.h | 7 +-
>> 2 files changed, 149 insertions(+), 81 deletions(-)
>>
>> diff --git a/drivers/input/mouse/trackpoint.c
>> b/drivers/input/mouse/trackpoint.c
>> index f310249..17e9b36 100644
>> --- a/drivers/input/mouse/trackpoint.c
>> +++ b/drivers/input/mouse/trackpoint.c
>> @@ -20,6 +20,26 @@
>> #include "trackpoint.h"
>>
>> /*
>> + * Power-on Reset: Resets all trackpoint parameters, including RAM values,
>> + * to defaults.
>> + * Returns zero on success, non-zero on failure.
>> + */
>> +static int trackpoint_power_on_reset(struct ps2dev *ps2dev)
>> +{
>> + unsigned char results[2];
>> +
>> + if (ps2_command(ps2dev, NULL, MAKE_PS2_CMD(0, 0, TP_COMMAND)) ||
>> + ps2_command(ps2dev, results, MAKE_PS2_CMD(0, 2, TP_POR))) {
>> + return -1;
>> + }
>> +
>> + /* POR response should be 0xAA00 or 0xFC00 */
>> + if ((results[0] != 0xAA && results[0] != 0xFC) || results[1] != 0x00)
>> + return -1;
>
> I am a bit concerned about this. 0xfc00 indicates POR failure. In this
> case shouldn't we try to reset the device, issue another POR and bail if
> it fails again?
>
Yes, you are correct. I just now posted v2 to fix this. Now, on
0xfc00, we attempt one retry. On subsequent 0xfc00, we fail POR and
attempt to set parameters manually.
>>
>> static struct attribute *trackpoint_attrs[] = {
>> _attr_sensitivity.dattr.attr,
>> @@ -179,6 +241,9 @@ static struct attribute *trackpoint_attrs[] = {
>> _attr_press_to_select.dattr.attr,
>> _attr_skipback.dattr.attr,
>> _attr_ext_dev.dattr.attr,
>> + _attr_twohand.dattr.attr,
>> + _attr_source_tag.dattr.attr,
>> + _attr_mb.dattr.attr,
>
> What is the benefit of adding these 3 new attributes?
>
Previously, these attributes were handled in a special way -- the
corresponding TP values were set to default, but the attributes were
not exported. Now, they are set to default and exported. It makes for
cleaner code.
I see no good use for these attributes other than driver hacking. I
can remove them if you think it's best.
> Thanks.
>
> --
> Dmitry
Thanks,
Shawn
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH v2] Input: trackpoint - Optimize trackpoint init to use power-on reset
From: Shawn Nematbakhsh
The trackpoint driver sets various parameter default values, all of
which happen to be power-on defaults (Source: IBM TrackPoint Engineering
Specification, Version 4.0. Also confirmed by empirical data).
By sending the power-on reset command to reset all parameters to
power-on state, we can skip the lengthy process of programming all
parameters. In testing, ~2.5 secs of time writing parameters was reduced
to .35 seconds waiting for power-on reset to complete.
Signed-off-by: Shawn Nematbakhsh
---
Changes since v1:
- Changed handling of 0xfc00 reply to POR.
---
drivers/input/mouse/trackpoint.c | 224 +--
drivers/input/mouse/trackpoint.h | 7 +-
2 files changed, 150 insertions(+), 81 deletions(-)
diff --git a/drivers/input/mouse/trackpoint.c b/drivers/input/mouse/trackpoint.c
index f310249..42964d4 100644
--- a/drivers/input/mouse/trackpoint.c
+++ b/drivers/input/mouse/trackpoint.c
@@ -20,6 +20,30 @@
#include "trackpoint.h"
/*
+ * Power-on Reset: Resets all trackpoint parameters, including RAM values,
+ * to defaults.
+ * Returns zero on success, non-zero on failure.
+ */
+static int trackpoint_power_on_reset(struct ps2dev *ps2dev)
+{
+ unsigned char results[2];
+ int tries = 0;
+
+ /* Issue POR command, and repeat up to once if 0xFC00 received */
+ do {
+ if (ps2_command(ps2dev, NULL, MAKE_PS2_CMD(0, 0, TP_COMMAND)) ||
+ ps2_command(ps2dev, results, MAKE_PS2_CMD(0, 2, TP_POR)))
+ return -1;
+ } while (results[0] == 0xFC && results[1] == 0x00 && ++tries < 2);
+
+ /* Check for success response -- 0xAA00 */
+ if (results[0] != 0xAA || results[1] != 0x00)
+ return -1;
+
+ return 0;
+}
+
+/*
* Device IO: read, write and toggle bit
*/
static int trackpoint_read(struct ps2dev *ps2dev, unsigned char loc, unsigned
char *results)
@@ -69,6 +93,7 @@ struct trackpoint_attr_data {
unsigned char command;
unsigned char mask;
unsigned char inverted;
+ unsigned char power_on_default;
};
static ssize_t trackpoint_show_int_attr(struct psmouse *psmouse, void *data,
char *buf)
@@ -102,10 +127,11 @@ static ssize_t trackpoint_set_int_attr(struct psmouse
*psmouse, void *data,
return count;
}
-#define TRACKPOINT_INT_ATTR(_name, _command)
\
+#define TRACKPOINT_INT_ATTR(_name, _command, _default)
\
static struct trackpoint_attr_data trackpoint_attr_##_name = {
\
.field_offset = offsetof(struct trackpoint_data, _name),
\
.command = _command,
\
+ .power_on_default = _default,
\
};
\
PSMOUSE_DEFINE_ATTR(_name, S_IWUSR | S_IRUGO,
\
_attr_##_name,
\
@@ -139,31 +165,68 @@ static ssize_t trackpoint_set_bit_attr(struct psmouse
*psmouse, void *data,
}
-#define TRACKPOINT_BIT_ATTR(_name, _command, _mask, _inv)
\
+#define TRACKPOINT_BIT_ATTR(_name, _command, _mask, _inv, _default)
\
static struct trackpoint_attr_data trackpoint_attr_##_name = {
\
- .field_offset = offsetof(struct trackpoint_data, _name),
\
- .command= _command,
\
- .mask = _mask,
\
- .inverted = _inv,
\
+ .field_offset = offsetof(struct trackpoint_data,
\
+ _name),
\
+ .command= _command,
\
+ .mask = _mask,
\
+ .inverted = _inv,
\
+ .power_on_default = _default,
\
};
\
PSMOUSE_DEFINE_ATTR(_name, S_IWUSR | S_IRUGO,
\
_attr_##_name,
\
trackpoint_show_int_attr, trackpoint_set_bit_attr)
-TRACKPOINT_INT_ATTR(sensitivity, TP_SENS);
-TRACKPOINT_INT_ATTR(speed, TP_SPEED);
-TRACKPOINT_INT_ATTR(inertia, TP_INERTIA);
-TRACKPOINT_INT_ATTR(reach, TP_REACH);
-TRACKPOINT_INT_ATTR(draghys, TP_DRAGHYS);
-TRACKPOINT_INT_ATTR(mindrag, TP_MINDRAG);
-TRACKPOINT_INT_ATTR(thresh, TP_THRESH);
-TRACKPOINT_INT_ATTR(upthresh, TP_UP_THRESH);
-TRACKPOINT_INT_ATTR(ztime, TP_Z_TIME
[PATCH v2] Input: trackpoint - Optimize trackpoint init to use power-on reset
From: Shawn Nematbakhsh sha...@chromium.org
The trackpoint driver sets various parameter default values, all of
which happen to be power-on defaults (Source: IBM TrackPoint Engineering
Specification, Version 4.0. Also confirmed by empirical data).
By sending the power-on reset command to reset all parameters to
power-on state, we can skip the lengthy process of programming all
parameters. In testing, ~2.5 secs of time writing parameters was reduced
to .35 seconds waiting for power-on reset to complete.
Signed-off-by: Shawn Nematbakhsh sha...@chromium.org
---
Changes since v1:
- Changed handling of 0xfc00 reply to POR.
---
drivers/input/mouse/trackpoint.c | 224 +--
drivers/input/mouse/trackpoint.h | 7 +-
2 files changed, 150 insertions(+), 81 deletions(-)
diff --git a/drivers/input/mouse/trackpoint.c b/drivers/input/mouse/trackpoint.c
index f310249..42964d4 100644
--- a/drivers/input/mouse/trackpoint.c
+++ b/drivers/input/mouse/trackpoint.c
@@ -20,6 +20,30 @@
#include trackpoint.h
/*
+ * Power-on Reset: Resets all trackpoint parameters, including RAM values,
+ * to defaults.
+ * Returns zero on success, non-zero on failure.
+ */
+static int trackpoint_power_on_reset(struct ps2dev *ps2dev)
+{
+ unsigned char results[2];
+ int tries = 0;
+
+ /* Issue POR command, and repeat up to once if 0xFC00 received */
+ do {
+ if (ps2_command(ps2dev, NULL, MAKE_PS2_CMD(0, 0, TP_COMMAND)) ||
+ ps2_command(ps2dev, results, MAKE_PS2_CMD(0, 2, TP_POR)))
+ return -1;
+ } while (results[0] == 0xFC results[1] == 0x00 ++tries 2);
+
+ /* Check for success response -- 0xAA00 */
+ if (results[0] != 0xAA || results[1] != 0x00)
+ return -1;
+
+ return 0;
+}
+
+/*
* Device IO: read, write and toggle bit
*/
static int trackpoint_read(struct ps2dev *ps2dev, unsigned char loc, unsigned
char *results)
@@ -69,6 +93,7 @@ struct trackpoint_attr_data {
unsigned char command;
unsigned char mask;
unsigned char inverted;
+ unsigned char power_on_default;
};
static ssize_t trackpoint_show_int_attr(struct psmouse *psmouse, void *data,
char *buf)
@@ -102,10 +127,11 @@ static ssize_t trackpoint_set_int_attr(struct psmouse
*psmouse, void *data,
return count;
}
-#define TRACKPOINT_INT_ATTR(_name, _command)
\
+#define TRACKPOINT_INT_ATTR(_name, _command, _default)
\
static struct trackpoint_attr_data trackpoint_attr_##_name = {
\
.field_offset = offsetof(struct trackpoint_data, _name),
\
.command = _command,
\
+ .power_on_default = _default,
\
};
\
PSMOUSE_DEFINE_ATTR(_name, S_IWUSR | S_IRUGO,
\
trackpoint_attr_##_name,
\
@@ -139,31 +165,68 @@ static ssize_t trackpoint_set_bit_attr(struct psmouse
*psmouse, void *data,
}
-#define TRACKPOINT_BIT_ATTR(_name, _command, _mask, _inv)
\
+#define TRACKPOINT_BIT_ATTR(_name, _command, _mask, _inv, _default)
\
static struct trackpoint_attr_data trackpoint_attr_##_name = {
\
- .field_offset = offsetof(struct trackpoint_data, _name),
\
- .command= _command,
\
- .mask = _mask,
\
- .inverted = _inv,
\
+ .field_offset = offsetof(struct trackpoint_data,
\
+ _name),
\
+ .command= _command,
\
+ .mask = _mask,
\
+ .inverted = _inv,
\
+ .power_on_default = _default,
\
};
\
PSMOUSE_DEFINE_ATTR(_name, S_IWUSR | S_IRUGO,
\
trackpoint_attr_##_name,
\
trackpoint_show_int_attr, trackpoint_set_bit_attr)
-TRACKPOINT_INT_ATTR(sensitivity, TP_SENS);
-TRACKPOINT_INT_ATTR(speed, TP_SPEED);
-TRACKPOINT_INT_ATTR(inertia, TP_INERTIA);
-TRACKPOINT_INT_ATTR(reach, TP_REACH);
-TRACKPOINT_INT_ATTR(draghys, TP_DRAGHYS);
-TRACKPOINT_INT_ATTR(mindrag, TP_MINDRAG);
-TRACKPOINT_INT_ATTR(thresh, TP_THRESH);
-TRACKPOINT_INT_ATTR(upthresh, TP_UP_THRESH
Re: [PATCH] Input: trackpoint - Optimize trackpoint init to use power-on reset
Hi Dmitry,
Thanks for the review. Comments in-line.
On Wed, Mar 27, 2013 at 10:32 PM, Dmitry Torokhov
dmitry.torok...@gmail.com wrote:
Hi Shawn,
On Tue, Mar 26, 2013 at 12:36:41PM -0700, Shawn Nematbakhsh wrote:
The trackpoint driver sets various parameter default values, all of
which happen to be power-on defaults (Source: IBM TrackPoint Engineering
Specification, Version 4.0. Also confirmed by empirical data).
By sending the power-on reset command to reset all parameters to
power-on state, we can skip the lengthy process of programming all
parameters. In testing, ~2.5 secs of time writing parameters was reduced
to .35 seconds waiting for power-on reset to complete.
Signed-off-by: Shawn Nematbakhsh sha...@chromium.org
---
drivers/input/mouse/trackpoint.c | 223
+--
drivers/input/mouse/trackpoint.h | 7 +-
2 files changed, 149 insertions(+), 81 deletions(-)
diff --git a/drivers/input/mouse/trackpoint.c
b/drivers/input/mouse/trackpoint.c
index f310249..17e9b36 100644
--- a/drivers/input/mouse/trackpoint.c
+++ b/drivers/input/mouse/trackpoint.c
@@ -20,6 +20,26 @@
#include trackpoint.h
/*
+ * Power-on Reset: Resets all trackpoint parameters, including RAM values,
+ * to defaults.
+ * Returns zero on success, non-zero on failure.
+ */
+static int trackpoint_power_on_reset(struct ps2dev *ps2dev)
+{
+ unsigned char results[2];
+
+ if (ps2_command(ps2dev, NULL, MAKE_PS2_CMD(0, 0, TP_COMMAND)) ||
+ ps2_command(ps2dev, results, MAKE_PS2_CMD(0, 2, TP_POR))) {
+ return -1;
+ }
+
+ /* POR response should be 0xAA00 or 0xFC00 */
+ if ((results[0] != 0xAA results[0] != 0xFC) || results[1] != 0x00)
+ return -1;
I am a bit concerned about this. 0xfc00 indicates POR failure. In this
case shouldn't we try to reset the device, issue another POR and bail if
it fails again?
Yes, you are correct. I just now posted v2 to fix this. Now, on
0xfc00, we attempt one retry. On subsequent 0xfc00, we fail POR and
attempt to set parameters manually.
static struct attribute *trackpoint_attrs[] = {
psmouse_attr_sensitivity.dattr.attr,
@@ -179,6 +241,9 @@ static struct attribute *trackpoint_attrs[] = {
psmouse_attr_press_to_select.dattr.attr,
psmouse_attr_skipback.dattr.attr,
psmouse_attr_ext_dev.dattr.attr,
+ psmouse_attr_twohand.dattr.attr,
+ psmouse_attr_source_tag.dattr.attr,
+ psmouse_attr_mb.dattr.attr,
What is the benefit of adding these 3 new attributes?
Previously, these attributes were handled in a special way -- the
corresponding TP values were set to default, but the attributes were
not exported. Now, they are set to default and exported. It makes for
cleaner code.
I see no good use for these attributes other than driver hacking. I
can remove them if you think it's best.
Thanks.
--
Dmitry
Thanks,
Shawn
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] Input: trackpoint - Optimize trackpoint init to use power-on reset
The trackpoint driver sets various parameter default values, all of
which happen to be power-on defaults (Source: IBM TrackPoint Engineering
Specification, Version 4.0. Also confirmed by empirical data).
By sending the power-on reset command to reset all parameters to
power-on state, we can skip the lengthy process of programming all
parameters. In testing, ~2.5 secs of time writing parameters was reduced
to .35 seconds waiting for power-on reset to complete.
Signed-off-by: Shawn Nematbakhsh
---
drivers/input/mouse/trackpoint.c | 223 +--
drivers/input/mouse/trackpoint.h | 7 +-
2 files changed, 149 insertions(+), 81 deletions(-)
diff --git a/drivers/input/mouse/trackpoint.c b/drivers/input/mouse/trackpoint.c
index f310249..17e9b36 100644
--- a/drivers/input/mouse/trackpoint.c
+++ b/drivers/input/mouse/trackpoint.c
@@ -20,6 +20,26 @@
#include "trackpoint.h"
/*
+ * Power-on Reset: Resets all trackpoint parameters, including RAM values,
+ * to defaults.
+ * Returns zero on success, non-zero on failure.
+ */
+static int trackpoint_power_on_reset(struct ps2dev *ps2dev)
+{
+ unsigned char results[2];
+
+ if (ps2_command(ps2dev, NULL, MAKE_PS2_CMD(0, 0, TP_COMMAND)) ||
+ ps2_command(ps2dev, results, MAKE_PS2_CMD(0, 2, TP_POR))) {
+ return -1;
+ }
+
+ /* POR response should be 0xAA00 or 0xFC00 */
+ if ((results[0] != 0xAA && results[0] != 0xFC) || results[1] != 0x00)
+ return -1;
+ return 0;
+}
+
+/*
* Device IO: read, write and toggle bit
*/
static int trackpoint_read(struct ps2dev *ps2dev, unsigned char loc, unsigned
char *results)
@@ -69,6 +89,7 @@ struct trackpoint_attr_data {
unsigned char command;
unsigned char mask;
unsigned char inverted;
+ unsigned char power_on_default;
};
static ssize_t trackpoint_show_int_attr(struct psmouse *psmouse, void *data,
char *buf)
@@ -102,10 +123,11 @@ static ssize_t trackpoint_set_int_attr(struct psmouse
*psmouse, void *data,
return count;
}
-#define TRACKPOINT_INT_ATTR(_name, _command)
\
+#define TRACKPOINT_INT_ATTR(_name, _command, _default)
\
static struct trackpoint_attr_data trackpoint_attr_##_name = {
\
.field_offset = offsetof(struct trackpoint_data, _name),
\
.command = _command,
\
+ .power_on_default = _default,
\
};
\
PSMOUSE_DEFINE_ATTR(_name, S_IWUSR | S_IRUGO,
\
_attr_##_name,
\
@@ -139,31 +161,71 @@ static ssize_t trackpoint_set_bit_attr(struct psmouse
*psmouse, void *data,
}
-#define TRACKPOINT_BIT_ATTR(_name, _command, _mask, _inv)
\
+#define TRACKPOINT_BIT_ATTR(_name, _command, _mask, _inv, _default)
\
static struct trackpoint_attr_data trackpoint_attr_##_name = {
\
- .field_offset = offsetof(struct trackpoint_data, _name),
\
- .command= _command,
\
- .mask = _mask,
\
- .inverted = _inv,
\
+ .field_offset = offsetof(struct trackpoint_data,
\
+ _name),
\
+ .command= _command,
\
+ .mask = _mask,
\
+ .inverted = _inv,
\
+ .power_on_default = _default,
\
};
\
PSMOUSE_DEFINE_ATTR(_name, S_IWUSR | S_IRUGO,
\
_attr_##_name,
\
trackpoint_show_int_attr, trackpoint_set_bit_attr)
-TRACKPOINT_INT_ATTR(sensitivity, TP_SENS);
-TRACKPOINT_INT_ATTR(speed, TP_SPEED);
-TRACKPOINT_INT_ATTR(inertia, TP_INERTIA);
-TRACKPOINT_INT_ATTR(reach, TP_REACH);
-TRACKPOINT_INT_ATTR(draghys, TP_DRAGHYS);
-TRACKPOINT_INT_ATTR(mindrag, TP_MINDRAG);
-TRACKPOINT_INT_ATTR(thresh, TP_THRESH);
-TRACKPOINT_INT_ATTR(upthresh, TP_UP_THRESH);
-TRACKPOINT_INT_ATTR(ztime, TP_Z_TIME);
-TRACKPOINT_INT_ATTR(jenks, TP_JENKS_CURV);
-
-TRACKPOINT_BIT_ATTR(press_to_select, TP_TOGGLE_PTSON, TP_MASK_PTSON, 0);
-TRACKPOINT_BIT_ATTR(skipback, TP_TOGGLE_SKIPBACK, TP_MASK_SKIPBACK, 0);
-TRACKPOINT_BIT_ATTR(ext_dev, TP_TOGGLE_EXT_DEV, TP_MASK_EXT
[PATCH] Input: trackpoint - Optimize trackpoint init to use power-on reset
The trackpoint driver sets various parameter default values, all of
which happen to be power-on defaults (Source: IBM TrackPoint Engineering
Specification, Version 4.0. Also confirmed by empirical data).
By sending the power-on reset command to reset all parameters to
power-on state, we can skip the lengthy process of programming all
parameters. In testing, ~2.5 secs of time writing parameters was reduced
to .35 seconds waiting for power-on reset to complete.
Signed-off-by: Shawn Nematbakhsh sha...@chromium.org
---
drivers/input/mouse/trackpoint.c | 223 +--
drivers/input/mouse/trackpoint.h | 7 +-
2 files changed, 149 insertions(+), 81 deletions(-)
diff --git a/drivers/input/mouse/trackpoint.c b/drivers/input/mouse/trackpoint.c
index f310249..17e9b36 100644
--- a/drivers/input/mouse/trackpoint.c
+++ b/drivers/input/mouse/trackpoint.c
@@ -20,6 +20,26 @@
#include trackpoint.h
/*
+ * Power-on Reset: Resets all trackpoint parameters, including RAM values,
+ * to defaults.
+ * Returns zero on success, non-zero on failure.
+ */
+static int trackpoint_power_on_reset(struct ps2dev *ps2dev)
+{
+ unsigned char results[2];
+
+ if (ps2_command(ps2dev, NULL, MAKE_PS2_CMD(0, 0, TP_COMMAND)) ||
+ ps2_command(ps2dev, results, MAKE_PS2_CMD(0, 2, TP_POR))) {
+ return -1;
+ }
+
+ /* POR response should be 0xAA00 or 0xFC00 */
+ if ((results[0] != 0xAA results[0] != 0xFC) || results[1] != 0x00)
+ return -1;
+ return 0;
+}
+
+/*
* Device IO: read, write and toggle bit
*/
static int trackpoint_read(struct ps2dev *ps2dev, unsigned char loc, unsigned
char *results)
@@ -69,6 +89,7 @@ struct trackpoint_attr_data {
unsigned char command;
unsigned char mask;
unsigned char inverted;
+ unsigned char power_on_default;
};
static ssize_t trackpoint_show_int_attr(struct psmouse *psmouse, void *data,
char *buf)
@@ -102,10 +123,11 @@ static ssize_t trackpoint_set_int_attr(struct psmouse
*psmouse, void *data,
return count;
}
-#define TRACKPOINT_INT_ATTR(_name, _command)
\
+#define TRACKPOINT_INT_ATTR(_name, _command, _default)
\
static struct trackpoint_attr_data trackpoint_attr_##_name = {
\
.field_offset = offsetof(struct trackpoint_data, _name),
\
.command = _command,
\
+ .power_on_default = _default,
\
};
\
PSMOUSE_DEFINE_ATTR(_name, S_IWUSR | S_IRUGO,
\
trackpoint_attr_##_name,
\
@@ -139,31 +161,71 @@ static ssize_t trackpoint_set_bit_attr(struct psmouse
*psmouse, void *data,
}
-#define TRACKPOINT_BIT_ATTR(_name, _command, _mask, _inv)
\
+#define TRACKPOINT_BIT_ATTR(_name, _command, _mask, _inv, _default)
\
static struct trackpoint_attr_data trackpoint_attr_##_name = {
\
- .field_offset = offsetof(struct trackpoint_data, _name),
\
- .command= _command,
\
- .mask = _mask,
\
- .inverted = _inv,
\
+ .field_offset = offsetof(struct trackpoint_data,
\
+ _name),
\
+ .command= _command,
\
+ .mask = _mask,
\
+ .inverted = _inv,
\
+ .power_on_default = _default,
\
};
\
PSMOUSE_DEFINE_ATTR(_name, S_IWUSR | S_IRUGO,
\
trackpoint_attr_##_name,
\
trackpoint_show_int_attr, trackpoint_set_bit_attr)
-TRACKPOINT_INT_ATTR(sensitivity, TP_SENS);
-TRACKPOINT_INT_ATTR(speed, TP_SPEED);
-TRACKPOINT_INT_ATTR(inertia, TP_INERTIA);
-TRACKPOINT_INT_ATTR(reach, TP_REACH);
-TRACKPOINT_INT_ATTR(draghys, TP_DRAGHYS);
-TRACKPOINT_INT_ATTR(mindrag, TP_MINDRAG);
-TRACKPOINT_INT_ATTR(thresh, TP_THRESH);
-TRACKPOINT_INT_ATTR(upthresh, TP_UP_THRESH);
-TRACKPOINT_INT_ATTR(ztime, TP_Z_TIME);
-TRACKPOINT_INT_ATTR(jenks, TP_JENKS_CURV);
-
-TRACKPOINT_BIT_ATTR(press_to_select, TP_TOGGLE_PTSON, TP_MASK_PTSON, 0);
-TRACKPOINT_BIT_ATTR(skipback, TP_TOGGLE_SKIPBACK, TP_MASK_SKIPBACK, 0);
-TRACKPOINT_BIT_ATTR(ext_dev, TP_TOGGLE_EXT_DEV
Re: [PATCH v2] atkbd: Fix multi-char scancode handling on reconnect.
Hi Dmitry,
On Sun, Jan 6, 2013 at 1:10 AM, Dmitry Torokhov
wrote:
> Hi Shawn,
>
> On Thu, Dec 20, 2012 at 06:33:11PM -0800, Shawn Nematbakhsh wrote:
>> On resume from suspend there is a possibility for multi-byte scancodes
>> to be handled incorrectly. atkbd_reconnect disables the processing of
>> scancodes in software by calling atkbd_disable, but the keyboard may
>> still be active because no disconnect command was sent. Later, software
>> handling is re-enabled. If a multi-byte scancode sent from the keyboard
>> straddles the re-enable, only the latter byte(s) will be handled.
>>
>> In practice, this leads to cases where multi-byte break codes (ex. "e0
>> 4d" - break code for right-arrow) are misread as make codes ("4d" - make
>> code for numeric 6), leading to one or more unwanted, untyped characters
>> being interpreted.
>>
>> The solution implemented here involves sending command f5 (reset
>> disable) to the keyboard prior to disabling software handling of codes.
>> Later, the command to re-enable the keyboard is sent only after we are
>> prepared to handle scancodes.
>
> The core tries to avoid disturbing devices that are not keyboards, so I
> believe we should check the device ID first and if it is keyboard, then
> do the reset. We should also reset the internal state (emul and xl_bit)
> when re-enabling the device.
>
> Does the version of the patch below work for you?
>
> Thanks.
>
> --
> Dmitry
Thanks for the revision. This version of the patch tests good. I agree
with your changes, it makes sense to deactivate the keyboard from both
connect and reconnect, just in case the keyboard is able to send
scancodes at this point.
--
Shawn
>
> Input: atkbd - fix multi-byte scancode handling on reconnect
>
> From: Shawn Nematbakhsh
>
> On resume from suspend there is a possibility for multi-byte scancodes
> to be handled incorrectly. atkbd_reconnect disables the processing of
> scancodes in software by calling atkbd_disable, but the keyboard may
> still be active because no disconnect command was sent. Later, software
> handling is re-enabled. If a multi-byte scancode sent from the keyboard
> straddles the re-enable, only the latter byte(s) will be handled.
>
> In practice, this leads to cases where multi-byte break codes (ex. "e0
> 4d" - break code for right-arrow) are misread as make codes ("4d" - make
> code for numeric 6), leading to one or more unwanted, untyped characters
> being interpreted.
>
> The solution implemented here involves sending command f5 (reset
> disable) to the keyboard prior to disabling software handling of codes.
> Later, the command to re-enable the keyboard is sent only after we are
> prepared to handle scancodes.
>
> Signed-off-by: Shawn Nematbakhsh
> Signed-off-by: Dmitry Torokhov
> ---
> drivers/input/keyboard/atkbd.c | 72
>
> 1 file changed, 51 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/input/keyboard/atkbd.c b/drivers/input/keyboard/atkbd.c
> index 33d0fcd..2626773 100644
> --- a/drivers/input/keyboard/atkbd.c
> +++ b/drivers/input/keyboard/atkbd.c
> @@ -676,6 +676,39 @@ static inline void atkbd_disable(struct atkbd *atkbd)
> serio_continue_rx(atkbd->ps2dev.serio);
> }
>
> +static int atkbd_activate(struct atkbd *atkbd)
> +{
> + struct ps2dev *ps2dev = >ps2dev;
> +
> +/*
> + * Enable the keyboard to receive keystrokes.
> + */
> +
> + if (ps2_command(ps2dev, NULL, ATKBD_CMD_ENABLE)) {
> + dev_err(>serio->dev,
> + "Failed to enable keyboard on %s\n",
> + ps2dev->serio->phys);
> + return -1;
> + }
> +
> + return 0;
> +}
> +
> +/*
> + * atkbd_deactivate() resets and disables the keyboard from sending
> + * keystrokes.
> + */
> +
> +static void atkbd_deactivate(struct atkbd *atkbd)
> +{
> + struct ps2dev *ps2dev = >ps2dev;
> +
> + if (ps2_command(ps2dev, NULL, ATKBD_CMD_RESET_DIS))
> + dev_err(>serio->dev,
> + "Failed to deactivate keyboard on %s\n",
> + ps2dev->serio->phys);
> +}
> +
> /*
> * atkbd_probe() probes for an AT keyboard on a serio port.
> */
> @@ -731,6 +764,12 @@ static int atkbd_probe(struct atkbd *atkbd)
> return -1;
> }
>
> +/*
> + * Make sure nothing is coming from the keyboard and disturbs our
> + * internal state.
> + */
> + atkbd_deactivate(atkbd);
> +
> return 0;
> }
>
> @@ -825,2
Re: [PATCH v2] atkbd: Fix multi-char scancode handling on reconnect.
Hi Dmitry,
On Sun, Jan 6, 2013 at 1:10 AM, Dmitry Torokhov
dmitry.torok...@gmail.com wrote:
Hi Shawn,
On Thu, Dec 20, 2012 at 06:33:11PM -0800, Shawn Nematbakhsh wrote:
On resume from suspend there is a possibility for multi-byte scancodes
to be handled incorrectly. atkbd_reconnect disables the processing of
scancodes in software by calling atkbd_disable, but the keyboard may
still be active because no disconnect command was sent. Later, software
handling is re-enabled. If a multi-byte scancode sent from the keyboard
straddles the re-enable, only the latter byte(s) will be handled.
In practice, this leads to cases where multi-byte break codes (ex. e0
4d - break code for right-arrow) are misread as make codes (4d - make
code for numeric 6), leading to one or more unwanted, untyped characters
being interpreted.
The solution implemented here involves sending command f5 (reset
disable) to the keyboard prior to disabling software handling of codes.
Later, the command to re-enable the keyboard is sent only after we are
prepared to handle scancodes.
The core tries to avoid disturbing devices that are not keyboards, so I
believe we should check the device ID first and if it is keyboard, then
do the reset. We should also reset the internal state (emul and xl_bit)
when re-enabling the device.
Does the version of the patch below work for you?
Thanks.
--
Dmitry
Thanks for the revision. This version of the patch tests good. I agree
with your changes, it makes sense to deactivate the keyboard from both
connect and reconnect, just in case the keyboard is able to send
scancodes at this point.
--
Shawn
Input: atkbd - fix multi-byte scancode handling on reconnect
From: Shawn Nematbakhsh sha...@chromium.org
On resume from suspend there is a possibility for multi-byte scancodes
to be handled incorrectly. atkbd_reconnect disables the processing of
scancodes in software by calling atkbd_disable, but the keyboard may
still be active because no disconnect command was sent. Later, software
handling is re-enabled. If a multi-byte scancode sent from the keyboard
straddles the re-enable, only the latter byte(s) will be handled.
In practice, this leads to cases where multi-byte break codes (ex. e0
4d - break code for right-arrow) are misread as make codes (4d - make
code for numeric 6), leading to one or more unwanted, untyped characters
being interpreted.
The solution implemented here involves sending command f5 (reset
disable) to the keyboard prior to disabling software handling of codes.
Later, the command to re-enable the keyboard is sent only after we are
prepared to handle scancodes.
Signed-off-by: Shawn Nematbakhsh sha...@chromium.org
Signed-off-by: Dmitry Torokhov dmitry.torok...@gmail.com
---
drivers/input/keyboard/atkbd.c | 72
1 file changed, 51 insertions(+), 21 deletions(-)
diff --git a/drivers/input/keyboard/atkbd.c b/drivers/input/keyboard/atkbd.c
index 33d0fcd..2626773 100644
--- a/drivers/input/keyboard/atkbd.c
+++ b/drivers/input/keyboard/atkbd.c
@@ -676,6 +676,39 @@ static inline void atkbd_disable(struct atkbd *atkbd)
serio_continue_rx(atkbd-ps2dev.serio);
}
+static int atkbd_activate(struct atkbd *atkbd)
+{
+ struct ps2dev *ps2dev = atkbd-ps2dev;
+
+/*
+ * Enable the keyboard to receive keystrokes.
+ */
+
+ if (ps2_command(ps2dev, NULL, ATKBD_CMD_ENABLE)) {
+ dev_err(ps2dev-serio-dev,
+ Failed to enable keyboard on %s\n,
+ ps2dev-serio-phys);
+ return -1;
+ }
+
+ return 0;
+}
+
+/*
+ * atkbd_deactivate() resets and disables the keyboard from sending
+ * keystrokes.
+ */
+
+static void atkbd_deactivate(struct atkbd *atkbd)
+{
+ struct ps2dev *ps2dev = atkbd-ps2dev;
+
+ if (ps2_command(ps2dev, NULL, ATKBD_CMD_RESET_DIS))
+ dev_err(ps2dev-serio-dev,
+ Failed to deactivate keyboard on %s\n,
+ ps2dev-serio-phys);
+}
+
/*
* atkbd_probe() probes for an AT keyboard on a serio port.
*/
@@ -731,6 +764,12 @@ static int atkbd_probe(struct atkbd *atkbd)
return -1;
}
+/*
+ * Make sure nothing is coming from the keyboard and disturbs our
+ * internal state.
+ */
+ atkbd_deactivate(atkbd);
+
return 0;
}
@@ -825,24 +864,6 @@ static int atkbd_reset_state(struct atkbd *atkbd)
return 0;
}
-static int atkbd_activate(struct atkbd *atkbd)
-{
- struct ps2dev *ps2dev = atkbd-ps2dev;
-
-/*
- * Enable the keyboard to receive keystrokes.
- */
-
- if (ps2_command(ps2dev, NULL, ATKBD_CMD_ENABLE)) {
- dev_err(ps2dev-serio-dev,
- Failed to enable keyboard on %s\n,
- ps2dev-serio-phys);
- return -1;
- }
-
- return 0
[PATCH v2] atkbd: Fix multi-char scancode handling on reconnect.
On resume from suspend there is a possibility for multi-byte scancodes
to be handled incorrectly. atkbd_reconnect disables the processing of
scancodes in software by calling atkbd_disable, but the keyboard may
still be active because no disconnect command was sent. Later, software
handling is re-enabled. If a multi-byte scancode sent from the keyboard
straddles the re-enable, only the latter byte(s) will be handled.
In practice, this leads to cases where multi-byte break codes (ex. "e0
4d" - break code for right-arrow) are misread as make codes ("4d" - make
code for numeric 6), leading to one or more unwanted, untyped characters
being interpreted.
The solution implemented here involves sending command f5 (reset
disable) to the keyboard prior to disabling software handling of codes.
Later, the command to re-enable the keyboard is sent only after we are
prepared to handle scancodes.
Signed-off-by: Shawn Nematbakhsh
---
Changes since v1:
- Removed unused return values from atkbd_deactivate.
drivers/input/keyboard/atkbd.c | 22 --
1 files changed, 20 insertions(+), 2 deletions(-)
diff --git a/drivers/input/keyboard/atkbd.c b/drivers/input/keyboard/atkbd.c
index add5ffd..44dcf83 100644
--- a/drivers/input/keyboard/atkbd.c
+++ b/drivers/input/keyboard/atkbd.c
@@ -844,6 +844,20 @@ static int atkbd_activate(struct atkbd *atkbd)
}
/*
+ * atkbd_deactivate() resets and disables the keyboard from sending
+ * keystrokes.
+ */
+static void atkbd_deactivate(struct atkbd *atkbd)
+{
+ struct ps2dev *ps2dev = >ps2dev;
+
+ if (ps2_command(ps2dev, NULL, ATKBD_CMD_RESET_DIS))
+ dev_err(>serio->dev,
+ "Failed to deactivate keyboard on %s\n",
+ ps2dev->serio->phys);
+}
+
+/*
* atkbd_cleanup() restores the keyboard state so that BIOS is happy after a
* reboot.
*/
@@ -1199,6 +1213,9 @@ static int atkbd_reconnect(struct serio *serio)
mutex_lock(>mutex);
+ if (atkbd->write)
+ atkbd_deactivate(atkbd);
+
atkbd_disable(atkbd);
if (atkbd->write) {
@@ -1208,8 +1225,6 @@ static int atkbd_reconnect(struct serio *serio)
if (atkbd->set != atkbd_select_set(atkbd, atkbd->set,
atkbd->extra))
goto out;
- atkbd_activate(atkbd);
-
/*
* Restore LED state and repeat rate. While input core
* will do this for us at resume time reconnect may happen
@@ -1224,6 +1239,9 @@ static int atkbd_reconnect(struct serio *serio)
}
atkbd_enable(atkbd);
+ if (atkbd->write)
+ atkbd_activate(atkbd);
+
retval = 0;
out:
--
1.7.7.3
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH v2] atkbd: Fix multi-char scancode handling on reconnect.
On resume from suspend there is a possibility for multi-byte scancodes
to be handled incorrectly. atkbd_reconnect disables the processing of
scancodes in software by calling atkbd_disable, but the keyboard may
still be active because no disconnect command was sent. Later, software
handling is re-enabled. If a multi-byte scancode sent from the keyboard
straddles the re-enable, only the latter byte(s) will be handled.
In practice, this leads to cases where multi-byte break codes (ex. e0
4d - break code for right-arrow) are misread as make codes (4d - make
code for numeric 6), leading to one or more unwanted, untyped characters
being interpreted.
The solution implemented here involves sending command f5 (reset
disable) to the keyboard prior to disabling software handling of codes.
Later, the command to re-enable the keyboard is sent only after we are
prepared to handle scancodes.
Signed-off-by: Shawn Nematbakhsh sha...@chromium.org
---
Changes since v1:
- Removed unused return values from atkbd_deactivate.
drivers/input/keyboard/atkbd.c | 22 --
1 files changed, 20 insertions(+), 2 deletions(-)
diff --git a/drivers/input/keyboard/atkbd.c b/drivers/input/keyboard/atkbd.c
index add5ffd..44dcf83 100644
--- a/drivers/input/keyboard/atkbd.c
+++ b/drivers/input/keyboard/atkbd.c
@@ -844,6 +844,20 @@ static int atkbd_activate(struct atkbd *atkbd)
}
/*
+ * atkbd_deactivate() resets and disables the keyboard from sending
+ * keystrokes.
+ */
+static void atkbd_deactivate(struct atkbd *atkbd)
+{
+ struct ps2dev *ps2dev = atkbd-ps2dev;
+
+ if (ps2_command(ps2dev, NULL, ATKBD_CMD_RESET_DIS))
+ dev_err(ps2dev-serio-dev,
+ Failed to deactivate keyboard on %s\n,
+ ps2dev-serio-phys);
+}
+
+/*
* atkbd_cleanup() restores the keyboard state so that BIOS is happy after a
* reboot.
*/
@@ -1199,6 +1213,9 @@ static int atkbd_reconnect(struct serio *serio)
mutex_lock(atkbd-mutex);
+ if (atkbd-write)
+ atkbd_deactivate(atkbd);
+
atkbd_disable(atkbd);
if (atkbd-write) {
@@ -1208,8 +1225,6 @@ static int atkbd_reconnect(struct serio *serio)
if (atkbd-set != atkbd_select_set(atkbd, atkbd-set,
atkbd-extra))
goto out;
- atkbd_activate(atkbd);
-
/*
* Restore LED state and repeat rate. While input core
* will do this for us at resume time reconnect may happen
@@ -1224,6 +1239,9 @@ static int atkbd_reconnect(struct serio *serio)
}
atkbd_enable(atkbd);
+ if (atkbd-write)
+ atkbd_activate(atkbd);
+
retval = 0;
out:
--
1.7.7.3
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] atkbd: Fix multi-char scancode handling on reconnect.
On resume from suspend there is a possibility for multi-byte scancodes
to be handled incorrectly. atkbd_reconnect disables the processing of
scancodes in software by calling atkbd_disable, but the keyboard may
still be active because no disconnect command was sent. Later, software
handling is re-enabled. If a multi-byte scancode sent from the keyboard
straddles the re-enable, only the latter byte(s) will be handled.
In practice, this leads to cases where multi-byte break codes (ex. "e0
4d" - break code for right-arrow) are misread as make codes ("4d" - make
code for numeric 6), leading to one or more unwanted, untyped characters
being interpreted.
The solution implemented here involves sending command f5 (reset
disable) to the keyboard prior to disabling software handling of codes.
Later, the command to re-enable the keyboard is sent only after we are
prepared to handle scancodes.
Signed-off-by: Shawn Nematbakhsh
---
drivers/input/keyboard/atkbd.c | 26 --
1 files changed, 24 insertions(+), 2 deletions(-)
diff --git a/drivers/input/keyboard/atkbd.c b/drivers/input/keyboard/atkbd.c
index add5ffd..da49e8b 100644
--- a/drivers/input/keyboard/atkbd.c
+++ b/drivers/input/keyboard/atkbd.c
@@ -844,6 +844,24 @@ static int atkbd_activate(struct atkbd *atkbd)
}
/*
+ * atkbd_deactivate() resets and disables the keyboard from sending
+ * keystrokes.
+ */
+static int atkbd_deactivate(struct atkbd *atkbd)
+{
+ struct ps2dev *ps2dev = >ps2dev;
+
+ if (ps2_command(ps2dev, NULL, ATKBD_CMD_RESET_DIS)) {
+ dev_err(>serio->dev,
+ "Failed to deactivate keyboard on %s\n",
+ ps2dev->serio->phys);
+ return -1;
+ }
+
+ return 0;
+}
+
+/*
* atkbd_cleanup() restores the keyboard state so that BIOS is happy after a
* reboot.
*/
@@ -1199,6 +1217,9 @@ static int atkbd_reconnect(struct serio *serio)
mutex_lock(>mutex);
+ if (atkbd->write)
+ atkbd_deactivate(atkbd);
+
atkbd_disable(atkbd);
if (atkbd->write) {
@@ -1208,8 +1229,6 @@ static int atkbd_reconnect(struct serio *serio)
if (atkbd->set != atkbd_select_set(atkbd, atkbd->set,
atkbd->extra))
goto out;
- atkbd_activate(atkbd);
-
/*
* Restore LED state and repeat rate. While input core
* will do this for us at resume time reconnect may happen
@@ -1224,6 +1243,9 @@ static int atkbd_reconnect(struct serio *serio)
}
atkbd_enable(atkbd);
+ if (atkbd->write)
+ atkbd_activate(atkbd);
+
retval = 0;
out:
--
1.7.8.6
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] atkbd: Fix multi-char scancode handling on reconnect.
On resume from suspend there is a possibility for multi-byte scancodes
to be handled incorrectly. atkbd_reconnect disables the processing of
scancodes in software by calling atkbd_disable, but the keyboard may
still be active because no disconnect command was sent. Later, software
handling is re-enabled. If a multi-byte scancode sent from the keyboard
straddles the re-enable, only the latter byte(s) will be handled.
In practice, this leads to cases where multi-byte break codes (ex. e0
4d - break code for right-arrow) are misread as make codes (4d - make
code for numeric 6), leading to one or more unwanted, untyped characters
being interpreted.
The solution implemented here involves sending command f5 (reset
disable) to the keyboard prior to disabling software handling of codes.
Later, the command to re-enable the keyboard is sent only after we are
prepared to handle scancodes.
Signed-off-by: Shawn Nematbakhsh sha...@chromium.org
---
drivers/input/keyboard/atkbd.c | 26 --
1 files changed, 24 insertions(+), 2 deletions(-)
diff --git a/drivers/input/keyboard/atkbd.c b/drivers/input/keyboard/atkbd.c
index add5ffd..da49e8b 100644
--- a/drivers/input/keyboard/atkbd.c
+++ b/drivers/input/keyboard/atkbd.c
@@ -844,6 +844,24 @@ static int atkbd_activate(struct atkbd *atkbd)
}
/*
+ * atkbd_deactivate() resets and disables the keyboard from sending
+ * keystrokes.
+ */
+static int atkbd_deactivate(struct atkbd *atkbd)
+{
+ struct ps2dev *ps2dev = atkbd-ps2dev;
+
+ if (ps2_command(ps2dev, NULL, ATKBD_CMD_RESET_DIS)) {
+ dev_err(ps2dev-serio-dev,
+ Failed to deactivate keyboard on %s\n,
+ ps2dev-serio-phys);
+ return -1;
+ }
+
+ return 0;
+}
+
+/*
* atkbd_cleanup() restores the keyboard state so that BIOS is happy after a
* reboot.
*/
@@ -1199,6 +1217,9 @@ static int atkbd_reconnect(struct serio *serio)
mutex_lock(atkbd-mutex);
+ if (atkbd-write)
+ atkbd_deactivate(atkbd);
+
atkbd_disable(atkbd);
if (atkbd-write) {
@@ -1208,8 +1229,6 @@ static int atkbd_reconnect(struct serio *serio)
if (atkbd-set != atkbd_select_set(atkbd, atkbd-set,
atkbd-extra))
goto out;
- atkbd_activate(atkbd);
-
/*
* Restore LED state and repeat rate. While input core
* will do this for us at resume time reconnect may happen
@@ -1224,6 +1243,9 @@ static int atkbd_reconnect(struct serio *serio)
}
atkbd_enable(atkbd);
+ if (atkbd-write)
+ atkbd_activate(atkbd);
+
retval = 0;
out:
--
1.7.8.6
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
