Danilo Krummrich
2025-Aug-26 20:45 UTC
[PATCH v6 2/5] rust: pci: provide access to PCI Vendor values
On 8/26/25 10:38 PM, John Hubbard wrote:> 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.Sorry, that's on me being not precise. When I said private I meant private to the parent module. The diff looks good, thanks! Please also make sure to add #[inline] where appropriate and rebase onto driver-core-next.
John Hubbard
2025-Aug-26 20:58 UTC
[PATCH v6 2/5] rust: pci: provide access to PCI Vendor values
On 8/26/25 1:45 PM, Danilo Krummrich wrote:> On 8/26/25 10:38 PM, John Hubbard wrote: >> On 8/25/25 5:47 AM, Danilo Krummrich wrote: >>> On Mon Aug 25, 2025 at 2:33 PM CEST, Alexandre Courbot wrote: >> ... > Sorry, that's on me being not precise. When I said private I meant private to > the parent module. > > The diff looks good, thanks!Huge relief! :)> > Please also make sure to add #[inline] where appropriate and rebase onto > driver-core-next.I have no idea "where appropriate" is, here. These are not hot paths, and the existing pci.rs methods such as Device::vendor_id() are not inlined, and so my initial approach is to just not inline any of this... thanks, -- John Hubbard