From d3be501437efd5f176e7541cace20b8db467eb47 Mon Sep 17 00:00:00 2001 From: Roshan Pius Date: Thu, 3 May 2018 18:26:23 -0700 Subject: WifiDiagnostics: Fix alert handling This is currently broken because |mWifiStateMachine| is always null in WifiDiagnostics (was broken in a refactor of WifiDiagnostics in early O). Changes in the CL: a) Construct WifiDiagnostics directly in WifiInjector's constructor (No need for makeWifiDiagnostics). b) Directly handle the alert callback. This is already guaranteed to be invoked from WifiStateMachine thread. There is no need for the extra hop through WSM message handling. c) Don't create 2 instances of WifiDiagnostics in WifiStateMachine & WifiStateMachinePrime. Some other related cleanups 1) Removed an unused binder object in WifiInjector. 2) Don't cache JavaRuntime instance in WifiInjector. Bug: 79222338 Test: Unit tests Test: Triggered an alert (each disconnect triggers an alert) and verified that: i) metrics are incremented, and ii) dumpsys contains the alert data. Change-Id: I59b157d57cc7ba234efa2dfe20fc59cc5834df2c --- .../com/android/server/wifi/WifiDiagnostics.java | 22 +++++++++------- .../java/com/android/server/wifi/WifiInjector.java | 30 ++++++++++------------ .../com/android/server/wifi/WifiStateMachine.java | 13 +--------- .../android/server/wifi/WifiStateMachinePrime.java | 2 +- .../com/android/server/wifi/WifiVendorHal.java | 2 +- 5 files changed, 30 insertions(+), 39 deletions(-) (limited to 'service') diff --git a/service/java/com/android/server/wifi/WifiDiagnostics.java b/service/java/com/android/server/wifi/WifiDiagnostics.java index 23e6ebe93..412394388 100644 --- a/service/java/com/android/server/wifi/WifiDiagnostics.java +++ b/service/java/com/android/server/wifi/WifiDiagnostics.java @@ -16,6 +16,7 @@ package com.android.server.wifi; +import android.annotation.NonNull; import android.content.Context; import android.util.Base64; @@ -101,30 +102,30 @@ class WifiDiagnostics extends BaseWifiDiagnostics { private boolean mIsLoggingEventHandlerRegistered; private WifiNative.RingBufferStatus[] mRingBuffers; private WifiNative.RingBufferStatus mPerPacketRingBuffer; - private WifiStateMachine mWifiStateMachine; private final BuildProperties mBuildProperties; private final WifiLog mLog; private final LastMileLogger mLastMileLogger; private final Runtime mJavaRuntime; + private final WifiMetrics mWifiMetrics; private int mMaxRingBufferSizeBytes; private WifiInjector mWifiInjector; public WifiDiagnostics(Context context, WifiInjector wifiInjector, - WifiStateMachine wifiStateMachine, WifiNative wifiNative, - BuildProperties buildProperties, LastMileLogger lastMileLogger) { + WifiNative wifiNative, BuildProperties buildProperties, + LastMileLogger lastMileLogger) { super(wifiNative); RING_BUFFER_BYTE_LIMIT_SMALL = context.getResources().getInteger( R.integer.config_wifi_logger_ring_buffer_default_size_limit_kb) * 1024; RING_BUFFER_BYTE_LIMIT_LARGE = context.getResources().getInteger( R.integer.config_wifi_logger_ring_buffer_verbose_size_limit_kb) * 1024; - mWifiStateMachine = wifiStateMachine; mBuildProperties = buildProperties; mIsLoggingEventHandlerRegistered = false; mMaxRingBufferSizeBytes = RING_BUFFER_BYTE_LIMIT_SMALL; mLog = wifiInjector.makeLog(TAG); mLastMileLogger = lastMileLogger; mJavaRuntime = wifiInjector.getJavaRuntime(); + mWifiMetrics = wifiInjector.getWifiMetrics(); mWifiInjector = wifiInjector; } @@ -409,11 +410,9 @@ class WifiDiagnostics extends BaseWifiDiagnostics { } } - synchronized void onWifiAlert(int errorCode, byte[] buffer) { - if (mWifiStateMachine != null) { - mWifiStateMachine.sendMessage( - WifiStateMachine.CMD_FIRMWARE_ALERT, errorCode, 0, buffer); - } + synchronized void onWifiAlert(int errorCode, @NonNull byte[] buffer) { + captureAlertData(errorCode, buffer); + mWifiMetrics.incrementAlertReasonCount(errorCode); } private boolean isVerboseLoggingEnabled() { @@ -550,6 +549,11 @@ class WifiDiagnostics extends BaseWifiDiagnostics { return mLastBugReports; } + @VisibleForTesting + LimitedCircularArray getAlertReports() { + return mLastAlerts; + } + private String compressToBase64(byte[] input) { String result; //compress diff --git a/service/java/com/android/server/wifi/WifiInjector.java b/service/java/com/android/server/wifi/WifiInjector.java index 7fc49ece2..358abd546 100644 --- a/service/java/com/android/server/wifi/WifiInjector.java +++ b/service/java/com/android/server/wifi/WifiInjector.java @@ -132,12 +132,12 @@ public class WifiInjector { private HalDeviceManager mHalDeviceManager; private final IBatteryStats mBatteryStats; private final WifiStateTracker mWifiStateTracker; - private final Runtime mJavaRuntime; private final SelfRecovery mSelfRecovery; private final WakeupController mWakeupController; private final INetworkManagementService mNwManagementService; private final ScanRequestProxy mScanRequestProxy; private final SarManager mSarManager; + private final BaseWifiDiagnostics mWifiDiagnostics; private final boolean mUseRealLogger; @@ -251,16 +251,20 @@ public class WifiInjector { (ActivityManager) mContext.getSystemService(Context.ACTIVITY_SERVICE), this, mWifiConfigManager, mWifiPermissionsUtil, mWifiMetrics, mClock); - // mWifiStateMachine has an implicit dependency on mJavaRuntime due to WifiDiagnostics. - mJavaRuntime = Runtime.getRuntime(); mSarManager = new SarManager(mContext, makeTelephonyManager(), wifiStateMachineLooper, mWifiNative); + if (mUseRealLogger) { + mWifiDiagnostics = new WifiDiagnostics( + mContext, this, mWifiNative, mBuildProperties, + new LastMileLogger(this)); + } else { + mWifiDiagnostics = new BaseWifiDiagnostics(mWifiNative); + } mWifiStateMachine = new WifiStateMachine(mContext, mFrameworkFacade, wifiStateMachineLooper, UserManager.get(mContext), this, mBackupManagerProxy, mCountryCode, mWifiNative, new WrongPasswordNotifier(mContext, mFrameworkFacade), mSarManager); - IBinder b = mFrameworkFacade.getService(Context.NETWORKMANAGEMENT_SERVICE); mWifiStateMachinePrime = new WifiStateMachinePrime(this, mContext, wifiStateMachineLooper, mWifiNative, new DefaultModeManager(mContext, wifiStateMachineLooper), mBatteryStats); @@ -493,14 +497,8 @@ public class WifiInjector { return new LogcatLog(tag); } - public BaseWifiDiagnostics makeWifiDiagnostics(WifiNative wifiNative) { - if (mUseRealLogger) { - return new WifiDiagnostics( - mContext, this, mWifiStateMachine, wifiNative, mBuildProperties, - new LastMileLogger(this)); - } else { - return new BaseWifiDiagnostics(wifiNative); - } + public BaseWifiDiagnostics getWifiDiagnostics() { + return mWifiDiagnostics; } /** @@ -578,10 +576,6 @@ public class WifiInjector { return mHalDeviceManager; } - public Runtime getJavaRuntime() { - return mJavaRuntime; - } - public WifiNative getWifiNative() { return mWifiNative; } @@ -610,6 +604,10 @@ public class WifiInjector { return mScanRequestProxy; } + public Runtime getJavaRuntime() { + return Runtime.getRuntime(); + } + public ActivityManagerService getActivityManagerService() { return (ActivityManagerService) ActivityManager.getService(); } diff --git a/service/java/com/android/server/wifi/WifiStateMachine.java b/service/java/com/android/server/wifi/WifiStateMachine.java index 5161a3117..5e49a4c6f 100644 --- a/service/java/com/android/server/wifi/WifiStateMachine.java +++ b/service/java/com/android/server/wifi/WifiStateMachine.java @@ -495,9 +495,6 @@ public class WifiStateMachine extends StateMachine { /* Get matching network */ static final int CMD_GET_MATCHING_CONFIG = BASE + 99; - /* alert from firmware */ - static final int CMD_FIRMWARE_ALERT = BASE + 100; - /* SIM is removed; reset any cached data for it */ static final int CMD_RESET_SIM_NETWORKS = BASE + 101; @@ -801,7 +798,7 @@ public class WifiStateMachine extends StateMachine { mPasspointManager = mWifiInjector.getPasspointManager(); mWifiMonitor = mWifiInjector.getWifiMonitor(); - mWifiDiagnostics = mWifiInjector.makeWifiDiagnostics(mWifiNative); + mWifiDiagnostics = mWifiInjector.getWifiDiagnostics(); mScanRequestProxy = mWifiInjector.getScanRequestProxy(); mWifiPermissionsWrapper = mWifiInjector.getWifiPermissionsWrapper(); @@ -3422,14 +3419,6 @@ public class WifiStateMachine extends StateMachine { int featureSet = mWifiNative.getSupportedFeatureSet(mInterfaceName); replyToMessage(message, message.what, featureSet); break; - case CMD_FIRMWARE_ALERT: - if (mWifiDiagnostics != null) { - byte[] buffer = (byte[])message.obj; - int alertReason = message.arg1; - mWifiDiagnostics.captureAlertData(alertReason, buffer); - mWifiMetrics.incrementAlertReasonCount(alertReason); - } - break; case CMD_GET_LINK_LAYER_STATS: // Not supported hence reply with error message replyToMessage(message, message.what, null); diff --git a/service/java/com/android/server/wifi/WifiStateMachinePrime.java b/service/java/com/android/server/wifi/WifiStateMachinePrime.java index fdacd1460..3c0a4825b 100644 --- a/service/java/com/android/server/wifi/WifiStateMachinePrime.java +++ b/service/java/com/android/server/wifi/WifiStateMachinePrime.java @@ -137,7 +137,7 @@ public class WifiStateMachinePrime { mDefaultModeManager = defaultModeManager; mBatteryStats = batteryStats; mSelfRecovery = mWifiInjector.getSelfRecovery(); - mWifiDiagnostics = mWifiInjector.makeWifiDiagnostics(mWifiNative); + mWifiDiagnostics = mWifiInjector.getWifiDiagnostics(); mScanRequestProxy = mWifiInjector.getScanRequestProxy(); mModeStateMachine = new ModeStateMachine(); mWifiNativeStatusListener = new WifiNativeStatusListener(); diff --git a/service/java/com/android/server/wifi/WifiVendorHal.java b/service/java/com/android/server/wifi/WifiVendorHal.java index 08d03d7de..1e622ca1b 100644 --- a/service/java/com/android/server/wifi/WifiVendorHal.java +++ b/service/java/com/android/server/wifi/WifiVendorHal.java @@ -2886,7 +2886,7 @@ public class WifiVendorHal { @Override public void onDebugErrorAlert(int errorCode, java.util.ArrayList debugData) { - mVerboseLog.d("onDebugErrorAlert " + errorCode); + mLog.w("onDebugErrorAlert " + errorCode); mHalEventHandler.post(() -> { WifiNative.WifiLoggerEventHandler eventHandler; synchronized (sLock) { -- cgit v1.2.3