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

[Yibo Dong]

On Wed, Jun 17, 2026 at 01:45:05PM -0700, Jakub Kicinski wrote:
> 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?
> 
That's an elegant approach and would work perfectly if the mailbox was
backed by shared RAM. However, the rnpgbe mailbox is implemented with
32-bit registers, not byte-addressable memory.
Unfortunately, memcpy_toio() cannot guarantee 32-bit access width. On
some architectures it decomposes into the widest aligned access
available — e.g., alpha uses __raw_writeq() for 8-byte aligned blocks,
which would corrupt data on a 32-bit register.
For register-based mailboxes, readl()/writel() remain the correct choice
because they guarantee 32-bit access width. The byte-order conversion
then needs to happen at the CPU side via cpu_to_le32()/le32_to_cpu(),
which is what the current fix does.

Should I change to '2 (the union version)' for next version?

Sorry for delay replay.

Thanks for your feedback.