On Wed, Sep 17, 2025 at 11:45:31AM +0100, Vadim Fedorenko wrote:
> On 16/09/2025 12:29, Dong Yibo wrote:
> > Add fundamental firmware (FW) communication operations via PF-FW
> > mailbox, including:
> > - FW sync (via HW info query with retries)
> > - HW reset (post FW command to reset hardware)
> > - MAC address retrieval (request FW for port-specific MAC)
> > - Power management (powerup/powerdown notification to FW)
> >
> > Signed-off-by: Dong Yibo <dong...@mucse.com>
>
> Reviewed-by: Vadim Fedorenko <vadim.fedore...@linux.dev>
>
> small nits below
>
>
> > +static void build_get_hw_info_req(struct mbx_fw_cmd_req *req)
> > +{
> > + req->flags = 0;
> > + req->opcode = cpu_to_le16(GET_HW_INFO);
> > + req->datalen = cpu_to_le16(MUCSE_MBX_REQ_HDR_LEN);
> > + req->reply_lo = 0;
> > + req->reply_hi = 0;
> > +}
>
> All these build*() functions re-init flags and reply to 0, but all
> mbx_fw_cmd_req are zero-inited on the stack. Might be better clean
> things assignments, but no strong opinion because the code is explicit
>
> If you will think of refactoring this part, it might be a good idea to
> avoid build*() functions at all and do proper initialization of
> mbx_fw_cmd_req in callers?
>
> > +
> > +/**
> > + * mucse_mbx_get_info - Get hw info from fw
> > + * @hw: pointer to the HW structure
> > + *
> > + * mucse_mbx_get_info tries to get hw info from hw.
> > + *
> > + * Return: 0 on success, negative errno on failure
> > + **/
> > +static int mucse_mbx_get_info(struct mucse_hw *hw)
> > +{
> > + struct mbx_fw_cmd_reply reply = {};
> > + struct mbx_fw_cmd_req req = {};
>
> something like:
>
> struct mbx_fw_cmd_req req =
> {
> .opcode = cpu_to_le16(GET_HW_INFO),
> .datalen = cpu_to_le16(MUCSE_MBX_REQ_HDR_LEN),
> }
>
>
>
That's a good idea! That makes the code more compact.
I think I should update this as your suggestion.
Regarding adding your "Reviewed-by" tag in the next version:
Would it be acceptable to include it when I submit the updated patch (with
the initialization logic adjusted), or should I wait for your further
review of the modified code first?
Thanks for your feedback.
