From 3a614244eec068e1c65781638b34f1b5e1b15a70 Mon Sep 17 00:00:00 2001 From: Marijn Suijten Date: Fri, 1 Nov 2024 17:28:18 +0100 Subject: [PATCH 1/2] node: Fix mask-or operator precedence by replacing `+` with `|` A request for using masks and shifts on the known `0-1-2` constants defining the minor type of a DRM node resulted in the expression to maintain the original `+` from first subtracting the current node type followed by adding the requested node type. As the subtract of the current node type was replaced by a mask, the resulting `old_id & ID_MASK + minor_base` expression is interpreted as `old_id & (ID_MASK + minor_base)` because of operator precedence, while it would have been correctly interpreted as `(old_id & ID_MASK) | minor_base` when the correct `|` OR operator is used. --- src/node/mod.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/node/mod.rs b/src/node/mod.rs index 5fcecb3..dc86100 100644 --- a/src/node/mod.rs +++ b/src/node/mod.rs @@ -330,8 +330,8 @@ pub fn dev_path(dev: dev_t, ty: NodeType) -> io::Result { if let Some(dev_name) = devname(dev) { let suffix = dev_name.trim_start_matches(|c: char| !c.is_numeric()); if let Ok(old_id) = suffix.parse::() { - let id_mask = 0b11_1111; - let id = old_id & id_mask + ty.minor_base(); + const ID_MASK: u32 = 0b11_1111; + let id = old_id & ID_MASK | ty.minor_base(); let path = PathBuf::from(format!("/dev/dri/{}{}", ty.minor_name_prefix(), id)); if path.exists() { return Ok(path); @@ -363,8 +363,8 @@ pub fn dev_path(dev: dev_t, ty: NodeType) -> io::Result { } let old_id = minor(dev); - let id_mask = 0b11_1111; - let id = old_id & id_mask + ty.minor_base(); + const ID_MASK: u32 = 0b11_1111; + let id = old_id & ID_MASK | ty.minor_base(); let path = PathBuf::from(format!("/dev/dri/{}{}", ty.minor_name_prefix(), id)); if path.exists() { return Ok(path); From 4b08084ad56d2a42d1f0e4822e01c0f3ea87b3aa Mon Sep 17 00:00:00 2001 From: Marijn Suijten Date: Fri, 1 Nov 2024 17:38:12 +0100 Subject: [PATCH 2/2] node: Put NodeType <-> dev_t conversions closer together Group all this logic together in functions inside `impl NodeType`. --- src/node/mod.rs | 48 +++++++++++++++++++++++++++++++----------------- 1 file changed, 31 insertions(+), 17 deletions(-) diff --git a/src/node/mod.rs b/src/node/mod.rs index dc86100..964f5a8 100644 --- a/src/node/mod.rs +++ b/src/node/mod.rs @@ -44,16 +44,7 @@ impl DrmNode { return Err(CreateDrmNodeError::NotDrmNode); } - // The type of the DRM node is determined by the minor number ranges: - // 0 - 63 -> Primary - // 64 - 127 -> Control - // 128 - 255 -> Render - let ty = match minor(dev) >> 6 { - 0 => NodeType::Primary, - 1 => NodeType::Control, - 2 => NodeType::Render, - _ => return Err(CreateDrmNodeError::NotDrmNode), - }; + let ty = NodeType::from_dev_id(dev)?; Ok(DrmNode { dev, ty }) } @@ -140,6 +131,12 @@ pub enum NodeType { } impl NodeType { + /// Bit-offset of [`NodeType`] inside [`minor()`] + const MINOR_OFFSET: u32 = 6; + /// Mask of [`NodeType`] inside [`minor()`] + #[cfg(not(target_os = "linux"))] + const MINOR_MASK: u32 = 0b11 << Self::MINOR_OFFSET; + /// Returns a string representing the prefix of a minor device's name. /// /// For example, on Linux with a primary node, the returned string would be `card`. @@ -151,14 +148,33 @@ impl NodeType { } } + fn from_dev_id(dev: dev_t) -> Result { + // The type of the DRM node is determined by the minor number ranges: + // 0 - 63 -> Primary + // 64 - 127 -> Control + // 128 - 255 -> Render + Ok(match minor(dev) >> Self::MINOR_OFFSET { + 0 => Self::Primary, + 1 => Self::Control, + 2 => Self::Render, + _ => return Err(CreateDrmNodeError::NotDrmNode), + }) + } + #[cfg(not(target_os = "linux"))] - fn minor_base(&self) -> u32 { + fn minor_index(&self) -> u32 { match self { NodeType::Primary => 0, - NodeType::Control => 64, - NodeType::Render => 128, + NodeType::Control => 1, + NodeType::Render => 2, } } + + /// Returns the value to place at [`Self::MINOR_MASK`] + #[cfg(not(target_os = "linux"))] + fn minor_base(&self) -> u32 { + self.minor_index() << Self::MINOR_OFFSET + } } impl Display for NodeType { @@ -330,8 +346,7 @@ pub fn dev_path(dev: dev_t, ty: NodeType) -> io::Result { if let Some(dev_name) = devname(dev) { let suffix = dev_name.trim_start_matches(|c: char| !c.is_numeric()); if let Ok(old_id) = suffix.parse::() { - const ID_MASK: u32 = 0b11_1111; - let id = old_id & ID_MASK | ty.minor_base(); + let id = old_id & !NodeType::MINOR_MASK | ty.minor_base(); let path = PathBuf::from(format!("/dev/dri/{}{}", ty.minor_name_prefix(), id)); if path.exists() { return Ok(path); @@ -363,8 +378,7 @@ pub fn dev_path(dev: dev_t, ty: NodeType) -> io::Result { } let old_id = minor(dev); - const ID_MASK: u32 = 0b11_1111; - let id = old_id & ID_MASK | ty.minor_base(); + let id = old_id & !NodeType::MINOR_MASK | ty.minor_base(); let path = PathBuf::from(format!("/dev/dri/{}{}", ty.minor_name_prefix(), id)); if path.exists() { return Ok(path);