Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Sun, 13 Feb 2022 17:16:22 +0200
From:   Leon Romanovsky <leon@kernel.org>
To:     Veerasenareddy Burru <vburru@marvell.com>
Cc:     davem@davemloft.net, kuba@kernel.org, corbet@lwn.net,
        netdev@vger.kernel.org, linux-doc@vger.kernel.org,
        linux-kernel@vger.kernel.org,
        Abhijit Ayarekar <aayarekar@marvell.com>,
        Satananda Burla <sburla@marvell.com>
Subject: Re: [PATCH 2/4] octeon_ep: add support for ndo ops.
Message-ID: <YgkgxhXrXPmdAuM2@unreal>
References: <20220210213306.3599-1-vburru@marvell.com>
 <20220210213306.3599-3-vburru@marvell.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220210213306.3599-3-vburru@marvell.com>
Precedence: bulk

On Thu, Feb 10, 2022 at 01:33:04PM -0800, Veerasenareddy Burru wrote:
> Add support for ndo ops to set MAC address, change MTU, get stats.
> Add control path support to set MAC address, change MTU, get stats,
> set speed, get and set link mode.
> 
> Signed-off-by: Veerasenareddy Burru <vburru@marvell.com>
> Signed-off-by: Abhijit Ayarekar <aayarekar@marvell.com>
> Signed-off-by: Satananda Burla <sburla@marvell.com>
> ---
>  .../marvell/octeon_ep/octep_ctrl_net.c        | 105 ++++++++++++++++++
>  .../ethernet/marvell/octeon_ep/octep_main.c   |  67 +++++++++++
>  2 files changed, 172 insertions(+)

Please don't put "." in end of patch title.

> 
> diff --git a/drivers/net/ethernet/marvell/octeon_ep/octep_ctrl_net.c b/drivers/net/ethernet/marvell/octeon_ep/octep_ctrl_net.c
> index 1f0d8ba3c8ee..be9b0f31c754 100644
> --- a/drivers/net/ethernet/marvell/octeon_ep/octep_ctrl_net.c
> +++ b/drivers/net/ethernet/marvell/octeon_ep/octep_ctrl_net.c
> @@ -87,3 +87,108 @@ int octep_get_mac_addr(struct octep_device *oct, u8 *addr)
>  
>  	return 0;
>  }
> +
> +int octep_set_mac_addr(struct octep_device *oct, u8 *addr)
> +{
> +	struct octep_ctrl_mbox_msg msg = { 0 };
> +	struct octep_ctrl_net_h2f_req req = { 0 };

It is enough to write {} without 0.

> +
> +	req.hdr.cmd = OCTEP_CTRL_NET_H2F_CMD_MAC;
> +	req.mac.cmd = OCTEP_CTRL_NET_CMD_SET;
> +	memcpy(&req.mac.addr, addr, ETH_ALEN);
> +
> +	msg.hdr.flags = OCTEP_CTRL_MBOX_MSG_HDR_FLAG_REQ;
> +	msg.hdr.sizew = OCTEP_CTRL_NET_H2F_MAC_REQ_SZW;
> +	msg.msg = &req;
> +	return octep_ctrl_mbox_send(&oct->ctrl_mbox, &msg);
> +}
> +
> +int octep_set_mtu(struct octep_device *oct, int mtu)
> +{
> +	struct octep_ctrl_mbox_msg msg = { 0 };
> +	struct octep_ctrl_net_h2f_req req = { 0 };
> +
> +	req.hdr.cmd = OCTEP_CTRL_NET_H2F_CMD_MTU;
> +	req.mtu.cmd = OCTEP_CTRL_NET_CMD_SET;
> +	req.mtu.val = mtu;
> +
> +	msg.hdr.flags = OCTEP_CTRL_MBOX_MSG_HDR_FLAG_REQ;
> +	msg.hdr.sizew = OCTEP_CTRL_NET_H2F_MTU_REQ_SZW;
> +	msg.msg = &req;
> +	return octep_ctrl_mbox_send(&oct->ctrl_mbox, &msg);
> +}
> +
> +int octep_get_if_stats(struct octep_device *oct)
> +{
> +	struct octep_ctrl_mbox_msg msg = { 0 };
> +	struct octep_ctrl_net_h2f_req req = { 0 };
> +	struct octep_iface_rx_stats *iface_rx_stats;
> +	struct octep_iface_tx_stats *iface_tx_stats;
> +	int err;

Reversed Christmas tree, in all functions.

> +
> +	req.hdr.cmd = OCTEP_CTRL_NET_H2F_CMD_GET_IF_STATS;
> +	req.mac.cmd = OCTEP_CTRL_NET_CMD_GET;
> +	req.get_stats.offset = oct->ctrl_mbox_ifstats_offset;
> +
> +	msg.hdr.flags = OCTEP_CTRL_MBOX_MSG_HDR_FLAG_REQ;
> +	msg.hdr.sizew = OCTEP_CTRL_NET_H2F_GET_STATS_REQ_SZW;
> +	msg.msg = &req;
> +	err = octep_ctrl_mbox_send(&oct->ctrl_mbox, &msg);
> +	if (!err) {

Please use success oriented approach, in all places.

if (err)
   return err;

....

> +		iface_rx_stats = (struct octep_iface_rx_stats *)(oct->ctrl_mbox.barmem +
> +								 oct->ctrl_mbox_ifstats_offset);
> +		iface_tx_stats = (struct octep_iface_tx_stats *)(oct->ctrl_mbox.barmem +
> +								 oct->ctrl_mbox_ifstats_offset +
> +								 sizeof(struct octep_iface_rx_stats)
> +								);
> +		memcpy(&oct->iface_rx_stats, iface_rx_stats, sizeof(struct octep_iface_rx_stats));
> +		memcpy(&oct->iface_tx_stats, iface_tx_stats, sizeof(struct octep_iface_tx_stats));
> +	}
> +
> +	return 0;
> +}
> +

Thanks