Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit 08994417 authored by Hsin-chen Chuang's avatar Hsin-chen Chuang
Browse files

floss: mgmt: Wait until btadapterd completely stopped before restart 

It's common to see btmanagerd is trying to restart btadapterd but
actually nothing happens, because the restart could be rejected by
upstart if btadapterd process is still alive (btmanagerd considers it as
stopped as soon as the pid file is removed, which is a bit earlier than
upstart).

Wait until btadapterd is completely stopped to avoid a redundant wait.

Also revise the NativeInvoker to match the new definition of the
ProcessManager API. For systemd and upstart invokers, comment is updated
and no behavior change.

Bug: 374017641
Tag: #floss
Test: mmm packages/modules/Bluetooth
Test: modprobe -r btusb; modprobe btusb  # btmanagerd waits for
      btadapterd to completely stop, and then restarts it.
Flag: EXEMPT, Floss-only change
Change-Id: I99192d3db64cd7ae29726425ae1c4315ef738514
parent b00fba0c

system/gd/rust/linux/mgmt/src/state_machine.rs

+58 −44
Original line number Diff line number Diff line
@@ -946,6 +946,9 @@ pub trait ProcessManager { 


    /// Stop the adapter process.

    ///

    /// This should block the thread until the btadapterd process is completely stopped,

    /// that is, another btadapterd process with the same index is ready to be |start|.

    ///

    /// # Args

    /// * `virtual_hci` - Virtual index of adapter used for apis.

    /// * `real_hci` - Real index of the adapter on the system.
@@ -961,8 +964,7 @@ pub enum Invoker { 


#[derive(Default)]

pub struct NativeInvoker {

    process_container: Option<Child>,

    bluetooth_pid: u32,

    process_container: HashMap<VirtualHciIndex, Child>,

}



impl NativeInvoker {
@@ -973,23 +975,28 @@ impl NativeInvoker { 


impl ProcessManager for NativeInvoker {

    fn start(&mut self, virtual_hci: VirtualHciIndex, real_hci: RealHciIndex) {

        let new_process = Command::new("/usr/bin/btadapterd")

        if self.process_container.contains_key(&virtual_hci) {

            return;

        }

        match Command::new("/usr/bin/btadapterd")

            .arg(format!("INDEX={} HCI={}", virtual_hci.to_i32(), real_hci.to_i32()))

            .stdout(Stdio::piped())

            .spawn()

            .expect("cannot open");

        self.bluetooth_pid = new_process.id();

        self.process_container = Some(new_process);

        {

            Ok(p) => {

                self.process_container.insert(virtual_hci, p);

            }

    fn stop(&mut self, _virtual_hci: VirtualHciIndex, _real_hci: RealHciIndex) {

        match self.process_container {

            Some(ref mut _p) => {

                signal::kill(Pid::from_raw(self.bluetooth_pid as i32), Signal::SIGTERM).unwrap();

                self.process_container = None;

            Err(e) => error!("Failed to start btadapterd: {}", e),

        }

            None => {

                warn!("Process doesn't exist");

    }

    fn stop(&mut self, virtual_hci: VirtualHciIndex, _real_hci: RealHciIndex) {

        if let Some(mut p) = self.process_container.remove(&virtual_hci) {

            if let Err(e) = signal::kill(Pid::from_raw(p.id() as i32), Signal::SIGTERM) {

                warn!("Failed to send signal, process could have exited: {}", e);

            }

            let _ = p.wait();

        } else {

            warn!("Process doesn't exist");

        }

    }

}
@@ -1018,14 +1025,14 @@ impl ProcessManager for UpstartInvoker { 
        }

    }



    fn stop(&mut self, virtual_hci: VirtualHciIndex, real_hci: RealHciIndex) {

    fn stop(&mut self, virtual_hci: VirtualHciIndex, _real_hci: RealHciIndex) {

        // Currently in the upstart script, only INDEX is used for identifying the instance. Thus,

        // intentionally NOT passing HCI to upstart, to avoid the following confusing situation:

        //   1. UpstartInvoker: start btadapterd INDEX=0 HCI=0

        //   2. Kernel: The HCI0 crashed, and became HCI1

        //   3. UpstartInvoker: stop btadapterd INDEX=0 HCI=1  <---- This is confusing

        if let Err(e) = Command::new("initctl")

            .args([

                "stop",

                "btadapterd",

                format!("INDEX={}", virtual_hci.to_i32()).as_str(),

                format!("HCI={}", real_hci.to_i32()).as_str(),

            ])

            .args(["stop", "btadapterd", format!("INDEX={}", virtual_hci.to_i32()).as_str()])

            .output()

        {

            error!("Failed to stop btadapterd: {}", e);
@@ -1055,6 +1062,9 @@ impl ProcessManager for SystemdInvoker { 
    }



    fn stop(&mut self, virtual_hci: VirtualHciIndex, real_hci: RealHciIndex) {

        // FIXME(b/307625503): If the real index changed (could be caused by FW crash or USB issues)

        // then this function would be broken. Need a re-design of the virtual/real index management

        // that is compatible other invokers.

        Command::new("systemctl")

            .args([

                "stop",
@@ -1530,14 +1540,20 @@ impl StateMachineInternal { 
                let restart_count =

                    self.get_state(hci, |a: &AdapterState| Some(a.restart_count)).unwrap_or(0);



                // This is an unexpectedly stop, which means the ProcessManager might not yet be

                // ready to restart the same instance. Explicitly call stop to make sure we can move

                // on to the next step.

                warn!(

                    "{} stopped unexpectedly, first wait for the process to completely exit.",

                    hci

                );

                self.process_manager.stop(hci, self.get_real_hci_by_virtual_id(hci));



                if !present {

                    // If the index doesn't present, we have nothing to do - We can't even trigger

                    // the hardware reset because the sysfs reset entry would disappear as well.

                    // After the index presents, we shall try restarting.

                    warn!(

                        "{} stopped unexpectedly. After {} restarts, index disappeared.",

                        hci, restart_count

                    );

                    warn!("{} exited. After {} restarts, index disappeared.", hci, restart_count);

                    self.modify_state(hci, |s: &mut AdapterState| {

                        s.state = ProcessState::Off;

                        s.restart_count = 0;
@@ -1547,7 +1563,7 @@ impl StateMachineInternal { 
                    // If we've restarted a number of times, attempt to use the reset mechanism

                    // instead of retrying a start.

                    warn!(

                        "{} stopped unexpectedly. After {} restarts, trying a reset recovery.",

                        "{} exited. After {} restarts, trying a reset recovery.",

                        hci, restart_count

                    );

                    // Reset the restart count since we're attempting a reset now.
@@ -1561,11 +1577,7 @@ impl StateMachineInternal { 
                    self.reset_hci(real_hci);

                    (ProcessState::Off, CommandTimeoutAction::CancelTimer)

                } else {

                    warn!(

                        "{} stopped unexpectedly, try restarting (attempt #{})",

                        hci,

                        restart_count + 1

                    );

                    warn!("{} exited. Try restarting (attempt #{})", hci, restart_count + 1);

                    self.modify_state(hci, |s: &mut AdapterState| {

                        s.state = ProcessState::TurningOn;

                        s.restart_count += 1;
@@ -1612,25 +1624,29 @@ impl StateMachineInternal { 
                let restart_count =

                    self.get_state(hci, |a: &AdapterState| Some(a.restart_count)).unwrap_or(0);



                // Explicitly call stop to make sure the ProcessManager is ready to restart the

                // same instance.

                warn!(

                    "{} timed out while starting, first wait for the process to completely exit.",

                    hci

                );

                self.process_manager.stop(hci, self.get_real_hci_by_virtual_id(hci));



                if !present {

                    // If the index doesn't present, we have nothing to do - We can't even trigger

                    // the hardware reset because the sysfs reset entry would disappear as well.

                    // After the index presents, we shall try restarting.

                    warn!(

                        "{} timed out while starting. After {} restarts, index disappeared.",

                        hci, restart_count

                    );

                    warn!("{} exited. After {} restarts, index disappeared.", hci, restart_count);

                    self.modify_state(hci, |s: &mut AdapterState| {

                        s.state = ProcessState::Off;

                        s.restart_count = 0;

                    });

                    self.process_manager.stop(hci, self.get_real_hci_by_virtual_id(hci));

                    StateMachineTimeoutActions::Noop

                } else if restart_count >= RESET_ON_RESTART_COUNT {

                    // If we've restarted a number of times, attempt to use the reset mechanism

                    // instead of retrying a start.

                    warn!(

                        "{} timed out while starting. After {} restarts, trying a reset recovery.",

                        "{} exited. After {} restarts, trying a reset recovery.",

                        hci, restart_count

                    );

                    // Reset the restart count since we're attempting a reset now.
@@ -1644,16 +1660,11 @@ impl StateMachineInternal { 
                    self.reset_hci(real_hci);

                    StateMachineTimeoutActions::Noop

                } else {

                    warn!(

                        "{} timed out while starting, try restarting (attempt #{})",

                        hci,

                        restart_count + 1

                    );

                    warn!("{} exited. Try restarting (attempt #{})", hci, restart_count + 1);

                    self.modify_state(hci, |s: &mut AdapterState| {

                        s.state = ProcessState::TurningOn;

                        s.restart_count += 1;

                    });

                    self.process_manager.stop(hci, self.get_real_hci_by_virtual_id(hci));

                    self.process_manager.start(hci, self.get_real_hci_by_virtual_id(hci));

                    StateMachineTimeoutActions::RetryStart

                }
@@ -1946,7 +1957,8 @@ mod tests { 
        tokio::runtime::Runtime::new().unwrap().block_on(async {

            let mut process_manager = MockProcessManager::new();

            process_manager.expect_start();

            // Expect to start again

            // Expect to wait for stopped and start again

            process_manager.expect_stop();

            process_manager.expect_start();

            let mut state_machine = make_state_machine(process_manager);

            state_machine.action_on_hci_presence_changed(DEFAULT_ADAPTER, true);
@@ -1964,6 +1976,8 @@ mod tests { 
        tokio::runtime::Runtime::new().unwrap().block_on(async {

            let mut process_manager = MockProcessManager::new();

            process_manager.expect_start();

            // Expect to wait for stopped

            process_manager.expect_stop();

            let mut state_machine = make_state_machine(process_manager);

            state_machine.action_on_hci_presence_changed(DEFAULT_ADAPTER, true);

            state_machine.set_config_enabled(DEFAULT_ADAPTER, true);