From 4cc40bd16c89fb469c2f61fc80fddf181217f490 Mon Sep 17 00:00:00 2001 From: Jerry Xie Date: Sun, 21 Dec 2025 18:19:10 -0600 Subject: [PATCH 1/4] Audit panics path in embedded-service type-c * Add additional error type for event port * Make event port methods return result instead of panicking for invalid port --- embedded-service/src/type_c/controller.rs | 6 +-- embedded-service/src/type_c/event.rs | 59 ++++++++++++++++------- type-c-service/src/wrapper/mod.rs | 8 ++- type-c-service/src/wrapper/vdm.rs | 4 +- 4 files changed, 52 insertions(+), 25 deletions(-) diff --git a/embedded-service/src/type_c/controller.rs b/embedded-service/src/type_c/controller.rs index 83cafced..4d3b5587 100644 --- a/embedded-service/src/type_c/controller.rs +++ b/embedded-service/src/type_c/controller.rs @@ -475,11 +475,7 @@ impl<'a> Device<'a> { /// Covert a local port ID to a global port ID pub fn lookup_global_port(&self, port: LocalPortId) -> Result { - if port.0 >= self.num_ports as u8 { - return Err(PdError::InvalidParams); - } - - Ok(self.ports[port.0 as usize]) + Ok(*self.ports.get(port.0 as usize).ok_or(PdError::InvalidParams)?) } /// Convert a global port ID to a local port ID diff --git a/embedded-service/src/type_c/event.rs b/embedded-service/src/type_c/event.rs index 4748bc06..9961cb2a 100644 --- a/embedded-service/src/type_c/event.rs +++ b/embedded-service/src/type_c/event.rs @@ -33,6 +33,14 @@ bitfield! { pub u8, pd_hard_reset, set_pd_hard_reset: 8, 8; } +/// Event errors +#[derive(Clone, Copy, Debug)] +#[cfg_attr(feature = "defmt", derive(defmt::Format))] +pub enum Error { + /// Invalid data + InvalidPort, +} + /// Port status change events /// This is a type-safe wrapper around the raw bitfield /// These events are related to the overall port state and typically need to be considered together. @@ -405,25 +413,36 @@ impl PortPending { } /// Marks the given port as pending - pub fn pend_port(&mut self, port: usize) { + pub fn pend_port(&mut self, port: usize) -> Result<(), Error> { + if port >= self.0.len() { + return Err(Error::InvalidPort); + } self.0.set(port, true); + + Ok(()) } /// Marks the indexes given by the iterator as pending - pub fn pend_ports>(&mut self, iter: I) { + pub fn pend_ports>(&mut self, iter: I) -> Result<(), Error> { for port in iter { - self.pend_port(port); + self.pend_port(port)?; } + Ok(()) } /// Clears the pending status of the given port - pub fn clear_port(&mut self, port: usize) { + pub fn clear_port(&mut self, port: usize) -> Result<(), Error> { + if port >= self.0.len() { + return Err(Error::InvalidPort); + } + self.0.set(port, false); + Ok(()) } /// Returns true if the given port is pending - pub fn is_pending(&self, port: usize) -> bool { - self.0[port] + pub fn is_pending(&self, port: usize) -> Result { + Ok(*self.0.get(port).ok_or(Error::InvalidPort)?) } /// Returns a combination of the current pending ports and other @@ -453,8 +472,11 @@ impl Default for PortPending { impl FromIterator for PortPending { fn from_iter>(iter: T) -> Self { let mut flags = PortPending::none(); - flags.pend_ports(iter); - flags + if flags.pend_ports(iter).is_ok() { + flags + } else { + PortPending::none() + } } } @@ -474,9 +496,12 @@ impl Iterator for PortPendingIter { while self.index < self.flags.len() { let port_index = self.index; self.index += 1; - if self.flags.is_pending(port_index) { - self.flags.clear_port(port_index); - return Some(port_index); + if self.flags.is_pending(port_index).unwrap_or(false) { + if self.flags.clear_port(port_index).is_ok() { + return Some(port_index); + } else { + continue; + } } } None @@ -500,12 +525,12 @@ mod tests { fn test_port_event_flags_iter() { let mut pending = PortPending::none(); - pending.pend_port(0); - pending.pend_port(1); - pending.pend_port(2); - pending.pend_port(10); - pending.pend_port(23); - pending.pend_port(31); + pending.pend_port(0).unwrap(); + pending.pend_port(1).unwrap(); + pending.pend_port(2).unwrap(); + pending.pend_port(10).unwrap(); + pending.pend_port(23).unwrap(); + pending.pend_port(31).unwrap(); let mut iter = pending.into_iter(); assert_eq!(iter.next(), Some(0)); diff --git a/type-c-service/src/wrapper/mod.rs b/type-c-service/src/wrapper/mod.rs index 37f60649..38dcb170 100644 --- a/type-c-service/src/wrapper/mod.rs +++ b/type-c-service/src/wrapper/mod.rs @@ -299,7 +299,9 @@ where if events != PortEvent::none() { let mut pending = PortPending::none(); - pending.pend_port(global_port_id.0 as usize); + pending + .pend_port(global_port_id.0 as usize) + .map_err(|_| Error::Pd(PdError::InvalidPort))?; self.registration.pd_controller.notify_ports(pending); trace!("P{}: Notified service for events: {:#?}", global_port_id.0, events); } @@ -333,7 +335,9 @@ where // Pend this port let mut pending = PortPending::none(); - pending.pend_port(global_port_id.0 as usize); + pending + .pend_port(global_port_id.0 as usize) + .map_err(|_| Error::Pd(PdError::InvalidPort))?; self.registration.pd_controller.notify_ports(pending); Ok(()) } diff --git a/type-c-service/src/wrapper/vdm.rs b/type-c-service/src/wrapper/vdm.rs index d0968608..0eb01fa5 100644 --- a/type-c-service/src/wrapper/vdm.rs +++ b/type-c-service/src/wrapper/vdm.rs @@ -55,7 +55,9 @@ where } let mut pending = PortPending::none(); - pending.pend_port(global_port_id.0 as usize); + pending + .pend_port(global_port_id.0 as usize) + .map_err(|_| PdError::InvalidPort)?; self.registration.pd_controller.notify_ports(pending); Ok(()) } From 9ebec619e1d2502399f4c604d084a2b67a4019e3 Mon Sep 17 00:00:00 2001 From: Jerry Xie Date: Sun, 21 Dec 2025 18:27:25 -0600 Subject: [PATCH 2/4] Fix broken type-c-service tests --- type-c-service/src/lib.rs | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/type-c-service/src/lib.rs b/type-c-service/src/lib.rs index 99e9e2a5..4dfea19f 100644 --- a/type-c-service/src/lib.rs +++ b/type-c-service/src/lib.rs @@ -148,9 +148,9 @@ mod tests { #[tokio::test] async fn test_port_status_changed() { let mut pending_ports = PortPending::none(); - pending_ports.pend_port(0); - pending_ports.pend_port(2); - pending_ports.pend_port(3); + pending_ports.pend_port(0).unwrap(); + pending_ports.pend_port(2).unwrap(); + pending_ports.pend_port(3).unwrap(); let mut streamer = PortEventStreamer::new(pending_ports.into_iter()); @@ -197,7 +197,7 @@ mod tests { #[tokio::test] async fn test_port_notification() { let mut pending_ports = PortPending::none(); - pending_ports.pend_port(0); + pending_ports.pend_port(0).unwrap(); let mut streamer = PortEventStreamer::new(pending_ports.into_iter()); let event = streamer @@ -229,7 +229,7 @@ mod tests { #[tokio::test] async fn test_last_notifications() { let mut pending_ports = PortPending::none(); - pending_ports.pend_port(0); + pending_ports.pend_port(0).unwrap(); let mut streamer = PortEventStreamer::new(pending_ports.into_iter()); @@ -252,8 +252,8 @@ mod tests { #[tokio::test] async fn test_port_event() { let mut pending_ports = PortPending::none(); - pending_ports.pend_port(0); - pending_ports.pend_port(6); + pending_ports.pend_port(0).unwrap(); + pending_ports.pend_port(6).unwrap(); let mut streamer = PortEventStreamer::new(pending_ports.into_iter()); @@ -321,7 +321,7 @@ mod tests { #[tokio::test] async fn test_empty_event() { let mut pending_ports = PortPending::none(); - pending_ports.pend_port(0); + pending_ports.pend_port(0).unwrap(); let mut streamer = PortEventStreamer::new(pending_ports.into_iter()); let event = streamer.next::<(), _, _>(async |_| Ok(PortEvent::none())).await; @@ -332,8 +332,8 @@ mod tests { #[tokio::test] async fn test_skip_no_pending() { let mut pending_ports = PortPending::none(); - pending_ports.pend_port(0); - pending_ports.pend_port(1); + pending_ports.pend_port(0).unwrap(); + pending_ports.pend_port(1).unwrap(); let mut streamer = PortEventStreamer::new(pending_ports.into_iter()); let event = streamer From 59f5970e1b6587b604aaf5445d2010d8320d46fe Mon Sep 17 00:00:00 2001 From: Jerry Xie Date: Mon, 22 Dec 2025 12:49:14 -0600 Subject: [PATCH 3/4] Address review comments * Rename error type and add more info to error * Make pend_ports() swallow the invalid port and log error instead of shortcircuiting and return an error --- embedded-service/src/type_c/event.rs | 41 +++++++++++++++------------- embedded-service/src/type_c/mod.rs | 1 + 2 files changed, 23 insertions(+), 19 deletions(-) diff --git a/embedded-service/src/type_c/event.rs b/embedded-service/src/type_c/event.rs index 9961cb2a..2ffeb38a 100644 --- a/embedded-service/src/type_c/event.rs +++ b/embedded-service/src/type_c/event.rs @@ -4,6 +4,7 @@ //! Processing these events typically requires acessing similar registers so they are grouped together. //! [`PortNotification`] contains events that are typically more message-like (PD alerts, VDMs, etc) and can be processed independently. //! Consequently [`PortNotification`] implements iterator traits to allow for processing these events as a stream. +use super::error; use bitfield::bitfield; use bitvec::BitArr; @@ -33,12 +34,12 @@ bitfield! { pub u8, pd_hard_reset, set_pd_hard_reset: 8, 8; } -/// Event errors -#[derive(Clone, Copy, Debug)] +/// Port pending errors +#[derive(Clone, Copy, Debug, PartialEq)] #[cfg_attr(feature = "defmt", derive(defmt::Format))] -pub enum Error { - /// Invalid data - InvalidPort, +pub enum PortPendingError { + /// Invalid port + InvalidPort(usize), } /// Port status change events @@ -413,9 +414,9 @@ impl PortPending { } /// Marks the given port as pending - pub fn pend_port(&mut self, port: usize) -> Result<(), Error> { + pub fn pend_port(&mut self, port: usize) -> Result<(), PortPendingError> { if port >= self.0.len() { - return Err(Error::InvalidPort); + return Err(PortPendingError::InvalidPort(port)); } self.0.set(port, true); @@ -423,17 +424,18 @@ impl PortPending { } /// Marks the indexes given by the iterator as pending - pub fn pend_ports>(&mut self, iter: I) -> Result<(), Error> { + pub fn pend_ports>(&mut self, iter: I) { for port in iter { - self.pend_port(port)?; + if let Err(e) = self.pend_port(port) { + error!("Error pending port {}", e); + } } - Ok(()) } /// Clears the pending status of the given port - pub fn clear_port(&mut self, port: usize) -> Result<(), Error> { + pub fn clear_port(&mut self, port: usize) -> Result<(), PortPendingError> { if port >= self.0.len() { - return Err(Error::InvalidPort); + return Err(PortPendingError::InvalidPort(port)); } self.0.set(port, false); @@ -441,8 +443,8 @@ impl PortPending { } /// Returns true if the given port is pending - pub fn is_pending(&self, port: usize) -> Result { - Ok(*self.0.get(port).ok_or(Error::InvalidPort)?) + pub fn is_pending(&self, port: usize) -> Result { + Ok(*self.0.get(port).ok_or(PortPendingError::InvalidPort(port))?) } /// Returns a combination of the current pending ports and other @@ -472,11 +474,8 @@ impl Default for PortPending { impl FromIterator for PortPending { fn from_iter>(iter: T) -> Self { let mut flags = PortPending::none(); - if flags.pend_ports(iter).is_ok() { - flags - } else { - PortPending::none() - } + flags.pend_ports(iter); + flags } } @@ -532,6 +531,10 @@ mod tests { pending.pend_port(23).unwrap(); pending.pend_port(31).unwrap(); + let result = pending.pend_port(32); + let expected = Err(PortPendingError::InvalidPort(32)); + assert_eq!(expected, result); + let mut iter = pending.into_iter(); assert_eq!(iter.next(), Some(0)); assert_eq!(iter.next(), Some(1)); diff --git a/embedded-service/src/type_c/mod.rs b/embedded-service/src/type_c/mod.rs index 8169b055..2943acdd 100644 --- a/embedded-service/src/type_c/mod.rs +++ b/embedded-service/src/type_c/mod.rs @@ -2,6 +2,7 @@ use embedded_usb_pd::pdo::{Common, Contract}; use embedded_usb_pd::type_c; +use crate::error; use crate::power::policy; pub mod comms; From 5a255c7f49286e3fded37195c2850b826cee491b Mon Sep 17 00:00:00 2001 From: Jerry Xie Date: Mon, 22 Dec 2025 12:58:10 -0600 Subject: [PATCH 4/4] Fix clippy warning about error --- embedded-service/src/type_c/event.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/embedded-service/src/type_c/event.rs b/embedded-service/src/type_c/event.rs index 2ffeb38a..aef14e02 100644 --- a/embedded-service/src/type_c/event.rs +++ b/embedded-service/src/type_c/event.rs @@ -426,8 +426,8 @@ impl PortPending { /// Marks the indexes given by the iterator as pending pub fn pend_ports>(&mut self, iter: I) { for port in iter { - if let Err(e) = self.pend_port(port) { - error!("Error pending port {}", e); + if self.pend_port(port).is_err() { + error!("Error pending port {}", port); } } }