Received-SPF: pass (google.com: domain of linux-kernel+bounces-184874-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Message-ID: <8432d37c-c3fb-4404-ab74-fb0a9940f0de@redhat.com>
Date: Tue, 21 May 2024 13:22:49 +0200
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [RFC PATCH 11/11] rust: PCI: add BAR request and ioremap
To: Miguel Ojeda <miguel.ojeda.sandonis@gmail.com>, pstanner@redhat.com
Cc: gregkh@linuxfoundation.org, rafael@kernel.org, bhelgaas@google.com,
 ojeda@kernel.org, alex.gaynor@gmail.com, wedsonaf@gmail.com,
 boqun.feng@gmail.com, gary@garyguo.net, bjorn3_gh@protonmail.com,
 benno.lossin@proton.me, a.hindborg@samsung.com, aliceryhl@google.com,
 airlied@gmail.com, fujita.tomonori@gmail.com, lina@asahilina.net,
 ajanulgu@redhat.com, lyude@redhat.com, rust-for-linux@vger.kernel.org,
 linux-kernel@vger.kernel.org, linux-pci@vger.kernel.org
References: <20240520172554.182094-1-dakr@redhat.com>
 <20240520172554.182094-12-dakr@redhat.com>
 <CANiq72kObXP7-YtcXnWQXJpEQ=N+RtcSeM6A+scBK00VkFj5JA@mail.gmail.com>
Content-Language: en-US
From: Danilo Krummrich <dakr@redhat.com>
Organization: RedHat
In-Reply-To: <CANiq72kObXP7-YtcXnWQXJpEQ=N+RtcSeM6A+scBK00VkFj5JA@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit

On 5/21/24 01:27, Miguel Ojeda wrote:
> On Mon, May 20, 2024 at 7:27 PM Danilo Krummrich <dakr@redhat.com> wrote:
>> +        let barlen = pdev.resource_len(num)?;
>> +        if barlen == 0 {
>> +            return Err(ENOMEM);
>> +        }
>> +
>> +        // SAFETY:
>> +        // `pdev` is always valid.
> 
> Please explain why it is always valid -- the point of a `SAFETY`
> comment is not to say something is OK, but why it is so.
> 
>> +        // `barnr` is checked for validity at the top of the function.

I added pci::Device::resource_len(), since I needed to get the VRAM bar size in Nova.

pci::Device::resource_len() also needs to check for a valid  bar index and is used
above, hence the check is implicit. I just forgot to change this comment accordingly.

>> +    /// Returns the size of the given PCI bar resource.
>> +    pub fn resource_len(&self, bar: u8) -> Result<bindings::resource_size_t> {
> 
> Sometimes `bindings::` in signatures of public methods may be
> justified -- is it the case here? Or should a newtype or similar be
> provided instead? I only see this function being called inside the
> module, anyway.

I agree, I just added this function real quick to let Nova report the VRAM bar size
and forgot to clean this up.

> 
>> +    /// Mapps an entire PCI-BAR after performing a region-request on it.
> 
> Typo.
> 
> Cheers,
> Miguel
>