Re: [PATCH net-next v7 1/5] rust: core abstractions for network PHY drivers

FUJITA Tomonori
2023-10-29T04:21:15Z

On Sat, 28 Oct 2023 18:45:40 +0000
Benno Lossin <benno.lossin@proton.me> wrote:

> On 28.10.23 20:23, Andrew Lunn wrote:
>> On Sat, Oct 28, 2023 at 04:37:53PM +0000, Benno Lossin wrote:
>>> On 28.10.23 11:27, FUJITA Tomonori wrote:
>>>> On Fri, 27 Oct 2023 21:19:38 +0000
>>>> Benno Lossin <benno.lossin@proton.me> wrote:
>>>>> I did not notice this before, but this means we cannot use the `link`
>>>>> function from bindgen, since that takes `&self`. We would need a
>>>>> function that takes `*const Self` instead.
>>>>
>>>> Implementing functions to access to a bitfield looks tricky so we need
>>>> to add such feature to bindgen or we add getters to the C side?
>>>
>>> Indeed, I just opened an issue [1] on the bindgen repo.
>>>
>>> [1]: https://github.com/rust-lang/rust-bindgen/issues/2674
>>
>> Please could you help me understand the consequences here. Are you
>> saying the rust toolchain is fatally broken here, it cannot generate
>> valid code at the moment? As a result we need to wait for a new
>> version of bindgen?
> This only affects bitfields, since they require special accessor functions
> generated by bindgen, so I would not say that the toolchain is fatally broken.
> It also is theoretically possible to manually access the bitfields in a correct
> manner, but that is error prone (which is why we use the accessor functions
> provided by bindgen).
>
> In this particular case we have three options:
> 1. wait until bindgen provides a raw accessor function that allows to use
> only raw pointers.
> 2. create some C helper functions for the bitfield access that will be replaced
> by the bindgen functions once bindgen has updated.
> 3. Since for the `phy_device` bindings, we only ever call functions while holding
> the `phy_device.lock` lock (at least I think that this is correct) we might be
> able to get away with creating a reference to the object and use the current
> accessor functions anyway.
>
> But for point 3 I will have to consult the others.

The current code is fine from Rust perspective because the current
code copies phy_driver on stack and makes a reference to the copy, if
I undertand correctly.

It's not nice to create an 500-bytes object on stack. It turned out
that it's not so simple to avoid it.