Re: [PATCH net-next v12 4/5] net: rnpgbe: Add basic mbx_fw support

[Vadim Fedorenko <vadim.fedorenko@linux.dev>]

On 17/09/2025 12:05, Yibo Dong wrote:
> 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 <dong100@mucse.com>
>>
>> Reviewed-by: Vadim Fedorenko <vadim.fedorenko@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?

If you will submit another version with this refactoring, I'll better do
another review.