[PATCH] rust: pci: fix documentation related to Device instances

[PATCH] rust: pci: fix documentation related to Device instances

[Rahul Rameshbabu]

Device instances in the pci crate represent a valid struct pci_dev, not a struct
device.

Signed-off-by: Rahul Rameshbabu <sergeantsagara@protonmail.com>
---

Notes:
    Notes:
    
    I noticed this while working on my HID abstraction work and figured it would be
    a small fixup I could send afterwards.
    
    Link: https://lore.kernel.org/rust-for-linux/20250629045031.92358-2-sergeantsagara@protonmail.com/

 rust/kernel/pci.rs | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/rust/kernel/pci.rs b/rust/kernel/pci.rs
index 6b94fd7a3ce9..af25a3fe92e5 100644
--- a/rust/kernel/pci.rs
+++ b/rust/kernel/pci.rs
@@ -254,7 +254,7 @@ pub trait Driver: Send {
 ///
 /// # Invariants
 ///
-/// A [`Device`] instance represents a valid `struct device` created by the C portion of the kernel.
+/// A [`Device`] instance represents a valid `struct pci_dev` created by the C portion of the kernel.
 #[repr(transparent)]
 pub struct Device<Ctx: device::DeviceContext = device::Normal>(
     Opaque<bindings::pci_dev>,
-- 
2.49.0

Re: [PATCH] rust: pci: fix documentation related to Device instances

[Wren Turkal]

On 6/28/25 10:57 PM, Rahul Rameshbabu wrote:
> Device instances in the pci crate represent a valid struct pci_dev, not a struct
> device.
> 
> Signed-off-by: Rahul Rameshbabu <sergeantsagara@protonmail.com>
> ---
> 
> Notes:
>      Notes:
>      
>      I noticed this while working on my HID abstraction work and figured it would be
>      a small fixup I could send afterwards.
>      
>      Link: https://lore.kernel.org/rust-for-linux/20250629045031.92358-2-sergeantsagara@protonmail.com/
> 
>   rust/kernel/pci.rs | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/rust/kernel/pci.rs b/rust/kernel/pci.rs
> index 6b94fd7a3ce9..af25a3fe92e5 100644
> --- a/rust/kernel/pci.rs
> +++ b/rust/kernel/pci.rs
> @@ -254,7 +254,7 @@ pub trait Driver: Send {
>   ///
>   /// # Invariants
>   ///
> -/// A [`Device`] instance represents a valid `struct device` created by the C portion of the kernel.
> +/// A [`Device`] instance represents a valid `struct pci_dev` created by the C portion of the kernel.

Should this not just be a "a valid pci device" and let the type in the 
function definition speak for the type instead of duplicating the type 
name in the  doc comment?

>   #[repr(transparent)]
>   pub struct Device<Ctx: device::DeviceContext = device::Normal>(
>       Opaque<bindings::pci_dev>,

-- 
You're more amazing than you think!

Re: [PATCH] rust: pci: fix documentation related to Device instances

[Rahul Rameshbabu]

On Sun, 29 Jun, 2025 00:14:18 -0700 "Wren Turkal" <wt@penguintechs.org> wrote:
> On 6/28/25 10:57 PM, Rahul Rameshbabu wrote:
>> Device instances in the pci crate represent a valid struct pci_dev, not a struct
>> device.
>>
>> Signed-off-by: Rahul Rameshbabu <sergeantsagara@protonmail.com>
>> ---
>>
>> Notes:
>>      Notes:
>>
>>      I noticed this while working on my HID abstraction work and figured it would be
>>      a small fixup I could send afterwards.
>>
>>      Link: https://lore.kernel.org/rust-for-linux/20250629045031.92358-2-sergeantsagara@protonmail.com/
>>
>>   rust/kernel/pci.rs | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/rust/kernel/pci.rs b/rust/kernel/pci.rs
>> index 6b94fd7a3ce9..af25a3fe92e5 100644
>> --- a/rust/kernel/pci.rs
>> +++ b/rust/kernel/pci.rs
>> @@ -254,7 +254,7 @@ pub trait Driver: Send {
>>   ///
>>   /// # Invariants
>>   ///
>> -/// A [`Device`] instance represents a valid `struct device` created by the C portion of the kernel.
>> +/// A [`Device`] instance represents a valid `struct pci_dev` created by the C portion of the kernel.
>
> Should this not just be a "a valid pci device" and let the type in the
> function definition speak for the type instead of duplicating the type
> name in the  doc comment?
>

My theory is that this comment is explicitly done for folks reading this
code from the C side. Rust tuple structs are not exactly a common
semantic in other programming languages. That said, I am open to
changing the comments if Danillo or others think that would be better as
well. Either way, I appreciate you taking the time to respond.

>>   #[repr(transparent)]
>>   pub struct Device<Ctx: device::DeviceContext = device::Normal>(
>>       Opaque<bindings::pci_dev>,

-- 
Thanks,

Rahul Rameshbabu

Re: [PATCH] rust: pci: fix documentation related to Device instances

[Danilo Krummrich]

On 6/29/25 9:14 AM, Wren Turkal wrote:
> On 6/28/25 10:57 PM, Rahul Rameshbabu wrote:
>>   ///
>>   /// # Invariants
>>   ///
>> -/// A [`Device`] instance represents a valid `struct device` created by the C 
>> portion of the kernel.
>> +/// A [`Device`] instance represents a valid `struct pci_dev` created by the 
>> C portion of the kernel.
> 
> Should this not just be a "a valid pci device" and let the type in the function 
> definition speak for the type instead of duplicating the type name in the  doc 
> comment?

The type is relevant for the invariant.

Re: [PATCH] rust: pci: fix documentation related to Device instances

[Wren Turkal]

On 6/28/25 10:57 PM, Rahul Rameshbabu wrote:
> Device instances in the pci crate represent a valid struct pci_dev, not a struct
> device.
> 
> Signed-off-by: Rahul Rameshbabu <sergeantsagara@protonmail.com>
> ---
> 
> Notes:
>      Notes:
>      
>      I noticed this while working on my HID abstraction work and figured it would be
>      a small fixup I could send afterwards.
>      
>      Link: https://lore.kernel.org/rust-for-linux/20250629045031.92358-2-sergeantsagara@protonmail.com/
> 
>   rust/kernel/pci.rs | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/rust/kernel/pci.rs b/rust/kernel/pci.rs
> index 6b94fd7a3ce9..af25a3fe92e5 100644
> --- a/rust/kernel/pci.rs
> +++ b/rust/kernel/pci.rs
> @@ -254,7 +254,7 @@ pub trait Driver: Send {
>   ///
>   /// # Invariants
>   ///
> -/// A [`Device`] instance represents a valid `struct device` created by the C portion of the kernel.
> +/// A [`Device`] instance represents a valid `struct pci_dev` created by the C portion of the kernel.

Should this not just be a "a valid pci device" and let the type in the 
function definition speak for the type instead of duplicating the type 
name in the  doc comment?

>   #[repr(transparent)]
>   pub struct Device<Ctx: device::DeviceContext = device::Normal>(
>       Opaque<bindings::pci_dev>,

-- 
You're more amazing than you think!

Re: [PATCH] rust: pci: fix documentation related to Device instances

[Wren Turkal]

Apologies on the double send. I thought the first didn't go through. 
Please ignore this message and reply to the other.

wt

On 6/29/25 12:15 AM, Wren Turkal wrote:
> On 6/28/25 10:57 PM, Rahul Rameshbabu wrote:
>> Device instances in the pci crate represent a valid struct pci_dev, 
>> not a struct
>> device.
>>
>> Signed-off-by: Rahul Rameshbabu <sergeantsagara@protonmail.com>
>> ---
>>
>> Notes:
>>      Notes:
>>      I noticed this while working on my HID abstraction work and 
>> figured it would be
>>      a small fixup I could send afterwards.
>>      Link: https://lore.kernel.org/rust-for- 
>> linux/20250629045031.92358-2-sergeantsagara@protonmail.com/
>>
>>   rust/kernel/pci.rs | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/rust/kernel/pci.rs b/rust/kernel/pci.rs
>> index 6b94fd7a3ce9..af25a3fe92e5 100644
>> --- a/rust/kernel/pci.rs
>> +++ b/rust/kernel/pci.rs
>> @@ -254,7 +254,7 @@ pub trait Driver: Send {
>>   ///
>>   /// # Invariants
>>   ///
>> -/// A [`Device`] instance represents a valid `struct device` created 
>> by the C portion of the kernel.
>> +/// A [`Device`] instance represents a valid `struct pci_dev` created 
>> by the C portion of the kernel.
> 
> Should this not just be a "a valid pci device" and let the type in the 
> function definition speak for the type instead of duplicating the type 
> name in the  doc comment?
> 
>>   #[repr(transparent)]
>>   pub struct Device<Ctx: device::DeviceContext = device::Normal>(
>>       Opaque<bindings::pci_dev>,
> 

-- 
You're more amazing than you think!

Re: [PATCH] rust: pci: fix documentation related to Device instances

[Danilo Krummrich]

On Sun, Jun 29, 2025 at 05:57:56AM +0000, Rahul Rameshbabu wrote:
> Device instances in the pci crate represent a valid struct pci_dev, not a struct
> device.
> 
> Signed-off-by: Rahul Rameshbabu <sergeantsagara@protonmail.com>

This commit has two checkpatch warnings, can you please fix them, add a 'Fixes:'
tag and send a v2?

Re: [PATCH] rust: pci: fix documentation related to Device instances

[Rahul Rameshbabu]

On Sat, 05 Jul, 2025 13:14:18 +0200 "Danilo Krummrich" <dakr@kernel.org> wrote:
> On Sun, Jun 29, 2025 at 05:57:56AM +0000, Rahul Rameshbabu wrote:
>> Device instances in the pci crate represent a valid struct pci_dev, not a struct
>> device.
>>
>> Signed-off-by: Rahul Rameshbabu <sergeantsagara@protonmail.com>
>
> This commit has two checkpatch warnings, can you please fix them, add a 'Fixes:'
> tag and send a v2?

Sorry about that. I was traveling with a new laptop when I drafted this
patch, so my setup was not correct. Should have run checkpatch either
way before sending this out.

v2: https://lore.kernel.org/rust-for-linux/20250706035944.18442-3-sergeantsagara@protonmail.com/

Thanks,
Rahul Rameshbabu