Re: [PATCH V10 11/15] rust: cpufreq: Add initial abstractions for cpufreq framework

[Danilo Krummrich]

On Wed, Apr 16, 2025 at 03:07:20PM +0530, Viresh Kumar wrote:
> On 16-04-25, 11:14, Danilo Krummrich wrote:
> > On Wed, Apr 16, 2025 at 12:09:28PM +0530, Viresh Kumar wrote:
> 
> > > +    pub unsafe fn data(&self, index: usize) -> u32 {
> > > +        // SAFETY: By the type invariant, the pointer stored in `self` is valid and `index` is
> > > +        // guaranteed to be valid by the safety requirements of the function.
> > > +        unsafe { (*self.as_raw().add(index)).driver_data }
> > > +    }
> > 
> > Those three functions above look like they're supposed to be used directly by
> > drivers, but are unsafe. :(
> > 
> > It looks like the reason for them being unsafe is that with only the pointer to
> > the struct cpufreq_frequency_table array we don't know the length of the array.
> 
> Yes.
> 
> > However, a Table instance seems to come from TableBox, which *does* know the
> > length of the KVec<bindings::cpufreq_frequency_table>. Why can't we just preserve the
> > length and provide a safe API?
> 
> The Table is also created from a raw pointer, when it is received from
> the C callbacks. Also the Table can be created from the OPP table,
> where again we receive a raw pointer from the C code.
> 
> I tried to do this differently earlier and finalized on current
> version after some discussions on the list:
> 
> https://lore.kernel.org/all/2025011327-cubbyhole-idealness-d4cc@gregkh/

I skimmed over your explanation from the link and got stuck at:

> - The cpufreq core then calls cpufreq driver's callbacks and passes an
>   index to the freq-table, which the drivers don't need to verify
>   against table length, since the index came from the core itself.

This sounds like you could just abstract the index passed through the callback
in some trusted type (e.g. cpufreq::TableIndex) and let the cpufreq::Table
methods take this trusted index type, rather than a raw usize, which would also
make the methods safe.

- Danilo