Re: [PATCH net-next v4 05/10] enic: define MBOX message types and header structures

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
enic: define MBOX message types and header structures

This commit defines the mailbox protocol used for PF-VF communication over
the admin channel. It introduces the request/reply message types and the
structures representing the message headers and payloads.

> --- /dev/null
> +++ b/drivers/net/ethernet/cisco/enic/enic_mbox.h
> @@ -0,0 +1,75 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/* Copyright 2025 Cisco Systems, Inc.  All rights reserved. */
> +
> +#ifndef _ENIC_MBOX_H_
> +#define _ENIC_MBOX_H_

This isn't a bug, but should this header include <linux/types.h> and
<linux/bits.h>? It uses fixed-width types like __le16, __le32, u8 and the
BIT() macro. Without these includes, it might cause compilation errors if
included in an isolated context.

[ ... ]

> +struct enic_mbox_generic_reply {
> +	__le16 ret_major;
> +	__le16 ret_minor;
> +};

[ ... ]

> +struct enic_mbox_vf_capability_reply_msg {
> +	struct enic_mbox_generic_reply reply;
> +	__le32 version;
> +	__le32 reserved[32];
> +};

Does this structure introduce misaligned memory accesses by violating
structure alignment inheritance rules?

struct enic_mbox_generic_reply consists only of __le16 fields, giving it
a natural 2-byte alignment. struct enic_mbox_vf_capability_reply_msg
embeds this reply struct and subsequently adds __le32 version and
__le32 reserved[32], which require 4-byte alignment.

If the outer structure is cast from a buffer that only guarantees the inner
structure's 2-byte alignment, the 4-byte aligned fields could cause
misaligned memory accesses.

Should the wider 32-bit fields be split into smaller __le16 fields to
strictly preserve the inherited 2-byte alignment?