Re: [PATCH v6 2/5] rust: pci: provide access to PCI Vendor values

[John Hubbard <jhubbard@xxxxxxxxxx>]

On 8/25/25 5:47 AM, Danilo Krummrich wrote:
> On Mon Aug 25, 2025 at 2:33 PM CEST, Alexandre Courbot wrote:
...
>> Naive question from someone with a device tree background and almost no
>> PCI experience: one consequence of using `From` here is that if I create
>> an non-registered Vendor value (e.g. `let vendor =
>> Vendor::from(0xf0f0)`), then do `vendor.as_raw()`, I won't get the value
>> passed initially but the one for `UNKNOWN`, e.g. `0xffff`. Are we ok
>> with this?
> 
> I think that's fine, since we shouldn't actually hit this. Drivers should only
> ever use the pre-defined constants of Vendor; consequently the
> Device::vendor_id() can't return UNKNOWN either.
> 
> So, I think the From impl is not ideal, since we can't limit its visibility. In
> order to improve this, I suggest to use Vendor::new() directly in the macro, and
> make Vendor::new() private. The same goes for Class, I guess.

Correction: when I went to implement this, I discovered that there is a better
way, which addresses both Alex's and your concerns. 

The incremental diff below shows how. It provides:

a) .from_raw(), which in this case matches conventions slightly better
   than new(). (I'm still learning that the Rust way is a bit different
   that the C++ way! haha).

b) Only the parent module (in this case, that's pci:: ) can call
   Class::from_raw(). This is exactly what we need. Fully private methods
   wouldn't work, but leaving it open for any caller to construct a
   Class item is also a problem.

c) Restored infallible operations, and with it, Alex's request for a
   reasonable behavior here now works once again:

       from_raw(0x10de).as_raw() == 0x10de
       

diff --git a/rust/kernel/pci.rs b/rust/kernel/pci.rs
index 0faec49bf8a2..40047a7433b1 100644
--- a/rust/kernel/pci.rs
+++ b/rust/kernel/pci.rs
@@ -418,7 +418,7 @@ pub fn resource_len(&self, bar: u32) -> Result<bindings::resource_size_t> {
     /// Returns the PCI class as a `Class` struct.
     pub fn pci_class(&self) -> Class {
         // SAFETY: `self.as_raw` is a valid pointer to a `struct pci_dev`.
-        Class::new(unsafe { (*self.as_raw()).class })
+        Class::from_raw(unsafe { (*self.as_raw()).class })
     }
 }
 
diff --git a/rust/kernel/pci/id.rs b/rust/kernel/pci/id.rs
index 1291553b4e15..399436ffaab9 100644
--- a/rust/kernel/pci/id.rs
+++ b/rust/kernel/pci/id.rs
@@ -51,24 +51,15 @@ impl Class {
                 pub const $variant: Self = Self(Self::to_24bit_class($binding));
             )+
         }
-
-        /// Convert a raw 24-bit class code value to a `Class`.
-        impl From<u32> for Class {
-            fn from(value: u32) -> Self {
-                match value {
-                    $(x if x == Self::$variant.0 => Self::$variant,)+
-                    _ => Self::UNKNOWN,
-                }
-            }
-        }
     };
 }
 
 /// Once constructed, a `Class` contains a valid PCI Class code.
 impl Class {
-    /// Create a new Class from a raw 24-bit class code.
-    pub fn new(class_code: u32) -> Self {
-        Self::from(class_code)
+    /// Create a Class from a raw 24-bit class code.
+    /// Only accessible from the parent pci module.
+    pub(super) fn from_raw(class_code: u32) -> Self {
+        Self(class_code)
     }
 
     /// Get the raw 24-bit class code value.
@@ -235,5 +226,4 @@ fn try_from(value: u32) -> Result<Self, Self::Error> {
     ACCELERATOR_PROCESSING     = bindings::PCI_CLASS_ACCELERATOR_PROCESSING,     // 0x120000
 
     OTHERS                     = bindings::PCI_CLASS_OTHERS,                     // 0xff0000
-    UNKNOWN                    = 0xffffff,
 }


thanks,
-- 
John Hubbard