From ac9a96da65f6eae4513654adaad8a457d1c1575c Mon Sep 17 00:00:00 2001 From: Mathias Agopian Date: Fri, 12 Jul 2013 02:01:16 -0700 Subject: [PATCH] fix a dead-lock in sensorservice sensorservice would deadlock if for some reason a sensor failed to enable. simplifed the code a bit, and made it behave a little closer to mr1.1 -- I couldn't convince myself that some changes in how locks were used were correct. Bug: 9794362 Change-Id: I6110f5dbb67e543f1c71d127de2299232badb36a --- services/sensorservice/SensorDevice.cpp | 16 +++++++---- services/sensorservice/SensorDevice.h | 2 +- services/sensorservice/SensorInterface.cpp | 4 +-- services/sensorservice/SensorInterface.h | 8 ++---- services/sensorservice/SensorService.cpp | 33 ++++++++++++---------- services/sensorservice/SensorService.h | 6 ++-- 6 files changed, 38 insertions(+), 31 deletions(-) diff --git a/services/sensorservice/SensorDevice.cpp b/services/sensorservice/SensorDevice.cpp index 16dabe8fe..a12529e17 100644 --- a/services/sensorservice/SensorDevice.cpp +++ b/services/sensorservice/SensorDevice.cpp @@ -111,13 +111,10 @@ ssize_t SensorDevice::poll(sensors_event_t* buffer, size_t count) { return c; } -status_t SensorDevice::resetStateWithoutActuatingHardware(void *ident, int handle) -{ - if (!mSensorDevice) return NO_INIT; - Info& info( mActivationCount.editValueFor(handle)); +void SensorDevice::autoDisable(void *ident, int handle) { + Info& info( mActivationCount.editValueFor(handle) ); Mutex::Autolock _l(mLock); info.rates.removeItem(ident); - return NO_ERROR; } status_t SensorDevice::activate(void* ident, int handle, int enabled) @@ -168,6 +165,15 @@ status_t SensorDevice::activate(void* ident, int handle, int enabled) ALOGE_IF(err, "Error %s sensor %d (%s)", enabled ? "activating" : "disabling", handle, strerror(-err)); + + if (err != NO_ERROR) { + // clean-up on failure + if (enabled) { + // failure when enabling the sensor + Mutex::Autolock _l(mLock); + info.rates.removeItem(ident); + } + } } { // scope for the lock diff --git a/services/sensorservice/SensorDevice.h b/services/sensorservice/SensorDevice.h index c0b357d5c..227dab668 100644 --- a/services/sensorservice/SensorDevice.h +++ b/services/sensorservice/SensorDevice.h @@ -56,7 +56,7 @@ public: ssize_t poll(sensors_event_t* buffer, size_t count); status_t activate(void* ident, int handle, int enabled); status_t setDelay(void* ident, int handle, int64_t ns); - status_t resetStateWithoutActuatingHardware(void *ident, int handle); + void autoDisable(void *ident, int handle); void dump(String8& result, char* buffer, size_t SIZE); }; diff --git a/services/sensorservice/SensorInterface.cpp b/services/sensorservice/SensorInterface.cpp index cf0a11df6..b483b7514 100644 --- a/services/sensorservice/SensorInterface.cpp +++ b/services/sensorservice/SensorInterface.cpp @@ -54,8 +54,8 @@ status_t HardwareSensor::setDelay(void* ident, int handle, int64_t ns) { return mSensorDevice.setDelay(ident, handle, ns); } -status_t HardwareSensor::resetStateWithoutActuatingHardware(void *ident, int handle) { - return mSensorDevice.resetStateWithoutActuatingHardware(ident, handle); +void HardwareSensor::autoDisable(void *ident, int handle) { + mSensorDevice.autoDisable(ident, handle); } Sensor HardwareSensor::getSensor() const { diff --git a/services/sensorservice/SensorInterface.h b/services/sensorservice/SensorInterface.h index 2e709ae4d..2e14e5782 100644 --- a/services/sensorservice/SensorInterface.h +++ b/services/sensorservice/SensorInterface.h @@ -40,11 +40,7 @@ public: virtual status_t setDelay(void* ident, int handle, int64_t ns) = 0; virtual Sensor getSensor() const = 0; virtual bool isVirtual() const = 0; - virtual status_t resetStateWithoutActuatingHardware(void *ident, int handle) { - // Override when you want to clean up for sensors which auto disable - // after trigger, or when enabling sensors fail. - return INVALID_OPERATION; - } + virtual void autoDisable(void *ident, int handle) { } }; // --------------------------------------------------------------------------- @@ -66,7 +62,7 @@ public: virtual status_t setDelay(void* ident, int handle, int64_t ns); virtual Sensor getSensor() const; virtual bool isVirtual() const { return false; } - virtual status_t resetStateWithoutActuatingHardware(void *ident, int handle); + virtual void autoDisable(void *ident, int handle); }; diff --git a/services/sensorservice/SensorService.cpp b/services/sensorservice/SensorService.cpp index ebf5cf0c7..6481584a6 100644 --- a/services/sensorservice/SensorService.cpp +++ b/services/sensorservice/SensorService.cpp @@ -249,10 +249,8 @@ void SensorService::cleanupAutoDisabledSensor(const sp& c if (getSensorType(handle) == SENSOR_TYPE_SIGNIFICANT_MOTION) { if (connection->hasSensor(handle)) { sensor = mSensorMap.valueFor(handle); - err = sensor ?sensor->resetStateWithoutActuatingHardware(connection.get(), handle) - : status_t(BAD_VALUE); - if (err != NO_ERROR) { - ALOGE("Sensor Inteface: Resetting state failed with err: %d", err); + if (sensor != NULL) { + sensor->autoDisable(connection.get(), handle); } cleanupWithoutDisable(connection, handle); } @@ -496,8 +494,12 @@ status_t SensorService::enable(const sp& connection, if (mInitCheck != NO_ERROR) return mInitCheck; - Mutex::Autolock _l(mLock); SensorInterface* sensor = mSensorMap.valueFor(handle); + if (sensor == NULL) { + return BAD_VALUE; + } + + Mutex::Autolock _l(mLock); SensorRecord* rec = mActiveSensors.valueFor(handle); if (rec == 0) { rec = new SensorRecord(connection); @@ -533,16 +535,11 @@ status_t SensorService::enable(const sp& connection, handle, connection.get()); } - // we are setup, now enable the sensor. - status_t err = sensor ? sensor->activate(connection.get(), true) : status_t(BAD_VALUE); - + status_t err = sensor->activate(connection.get(), true); if (err != NO_ERROR) { - // enable has failed, reset state in SensorDevice. - status_t resetErr = sensor ? sensor->resetStateWithoutActuatingHardware(connection.get(), - handle) : status_t(BAD_VALUE); // enable has failed, reset our state. - cleanupWithoutDisable(connection, handle); + cleanupWithoutDisableLocked(connection, handle); } return err; } @@ -553,7 +550,8 @@ status_t SensorService::disable(const sp& connection, if (mInitCheck != NO_ERROR) return mInitCheck; - status_t err = cleanupWithoutDisable(connection, handle); + Mutex::Autolock _l(mLock); + status_t err = cleanupWithoutDisableLocked(connection, handle); if (err == NO_ERROR) { SensorInterface* sensor = mSensorMap.valueFor(handle); err = sensor ? sensor->activate(connection.get(), false) : status_t(BAD_VALUE); @@ -561,9 +559,14 @@ status_t SensorService::disable(const sp& connection, return err; } -status_t SensorService::cleanupWithoutDisable(const sp& connection, - int handle) { +status_t SensorService::cleanupWithoutDisable( + const sp& connection, int handle) { Mutex::Autolock _l(mLock); + return cleanupWithoutDisableLocked(connection, handle); +} + +status_t SensorService::cleanupWithoutDisableLocked( + const sp& connection, int handle) { SensorRecord* rec = mActiveSensors.valueFor(handle); if (rec) { // see if this connection becomes inactive diff --git a/services/sensorservice/SensorService.h b/services/sensorservice/SensorService.h index 25e5f764b..fcdbc7d62 100644 --- a/services/sensorservice/SensorService.h +++ b/services/sensorservice/SensorService.h @@ -115,8 +115,10 @@ class SensorService : static void sortEventBuffer(sensors_event_t* buffer, size_t count); void registerSensor(SensorInterface* sensor); void registerVirtualSensor(SensorInterface* sensor); - status_t cleanupWithoutDisable(const sp& connection, - int handle); + status_t cleanupWithoutDisable( + const sp& connection, int handle); + status_t cleanupWithoutDisableLocked( + const sp& connection, int handle); void cleanupAutoDisabledSensor(const sp& connection, sensors_event_t const* buffer, const int count);