netdev - Re: [PATCH 2/4] octeon_ep: add support for ndo ops.

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Sun, 13 Feb 2022 17:16:22 +0200
From:   Leon Romanovsky <leon@...nel.org>
To:     Veerasenareddy Burru <vburru@...vell.com>
Cc:     davem@...emloft.net, kuba@...nel.org, corbet@....net,
        netdev@...r.kernel.org, linux-doc@...r.kernel.org,
        linux-kernel@...r.kernel.org,
        Abhijit Ayarekar <aayarekar@...vell.com>,
        Satananda Burla <sburla@...vell.com>
Subject: Re: [PATCH 2/4] octeon_ep: add support for ndo ops.

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@...vell.com>
> Signed-off-by: Abhijit Ayarekar <aayarekar@...vell.com>
> Signed-off-by: Satananda Burla <sburla@...vell.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