Re: [PATCH net] net: rnpgbe: fix mailbox endianness handling

[Jakub Kicinski]

On Wed, 17 Jun 2026 22:05:30 +0800 Yibo Dong wrote:
> On Wed, Jun 17, 2026 at 02:09:00PM +0200, Andrew Lunn wrote:
> > > My understanding is as follows:
> > > The firmware structures are defined with__le16 / __le32 for wire format,
> > > but the original code cast these struct pointers to u32 * before passing
> > > them to the mailbox read/write routines:
> > > - Send path: (u32 *)&req -> msg buffer -> writel()
> > > - Receive path: readl() -> msg buffer -> (u32 *)&reply
> > > Sparse only sees pure u32 = u32 assignments here, so no type mismatch is
> > > reported.  
> > 
> > Can the code be changed so that it does not need the cast? Casts are
> > bad, as you have just shown. This is something i try to push back on,
> > it makes you think about types and avoid issues like this.
> > 
> > 	Andrew
> >   
> Thinking... Yes. A few possibilities:
> 
> 1. Make all fields __le32, then extract via shifts:
>    struct mbx_fw_cmd_req {
>        __le32 word0;  // [15:0]=flags  [31:16]=opcode
>        __le32 word1;  // [15:0]=datalen [31:16]=ret_value
>        ...
>    };
>    But that's painful — le32_to_cpu(req.word0) >> 16 vs req.opcode.
> 
> 2. Use a union to keep named fields while also exposing __le32[] access:
>    union mbx_fw_cmd_req_u {
>        struct mbx_fw_cmd_req req;
>        __le32 dwords[sizeof(struct mbx_fw_cmd_req) / sizeof(__le32)];
>    };
>    union mbx_fw_cmd_reply_u {
>        struct mbx_fw_cmd_reply reply;
>        __le32 dwords[sizeof(struct mbx_fw_cmd_reply) / sizeof(__le32)];
>    };
> 
>    The transport interface becomes:
>    int mucse_write_mbx_pf(struct mucse_hw *hw, const __le32 *msg, u16 size);
>    int mucse_read_mbx_pf(struct mucse_hw *hw, __le32 *msg, u16 size);
> 
>    Callers would use:
>    union mbx_fw_cmd_req_u cmd = {};
>    cmd.req.opcode = cpu_to_le16(...);
>    cmd.req.flags  = cpu_to_le16(...);
>    mucse_write_mbx_pf(hw, cmd.dwords, sizeof(cmd.req));
> 
>    If the transport layer forgets le32_to_cpu(), sparse would catch it
>    because msg is __le32 * and mbx_data_rd32() returns u32.
> 
>    The downside is an extra union wrapper and an extra level in field
>    access (cmd.req.opcode vs req.opcode) — a minor inconvenience.
> 
> Do you have a preference between these, or another approach?
> 
> Thanks for the feedback.

3. Maybe use memcpy_toio() to transfer the data without any byteswaps?