Re: [PATCH net-next v7 2/5] rust: net::phy add module_phy_driver macro

Alice Ryhl
2023-11-17T09:39:16Z

FUJITA Tomonori <fujita.tomonori@gmail.com> writes:
> This macro creates an array of kernel's `struct phy_driver` and
> registers it. This also corresponds to the kernel's
> `MODULE_DEVICE_TABLE` macro, which embeds the information for module
> loading into the module binary file.
>
> A PHY driver should use this macro.
>
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>

Reviewed-by: Alice Ryhl <aliceryhl@google.com>

A few minor nits:

> + (drivers: [$($driver:ident),+], device_table: [$($dev:expr),+], $($f:tt)*) => {

Here, you can add $(,)? to allow trailing commas when the macro is used.
Like this:

(drivers: [$($driver:ident),+ $(,)?], device_table: [$($dev:expr),+ $(,)?], $($f:tt)*) => {

> + ::kernel::bindings::mdio_device_id {

Here, I recommend `$crate` instead of `::kernel`.

> +/// #[no_mangle]
> +/// static __mod_mdio__phydev_device_table: [::kernel::bindings::mdio_device_id; 2] = [
> +/// ::kernel::bindings::mdio_device_id {
> +/// phy_id: 0x003b1861,
> +/// phy_id_mask: 0xffffffff,
> +/// },
> +/// ::kernel::bindings::mdio_device_id {
> +/// phy_id: 0,
> +/// phy_id_mask: 0,
> +/// },
> +/// ];

I'd probably put a safety comment on the `#[no_mangle]` invocation to say that
"C will not read off the end of this constant since the last element is zero".

Alice