Re: [PATCH net-next v04 1/1] hinic3: module initialization and tx/rx logic

[Gur Stavi <gur.stavi@huawei.com>]

> > Auxiliary driver registration.
> > Net device_ops registration but open/stop are empty stubs.
> > tx/rx logic.
>
> Take care for spelling: Tx/Rx; HW (just below).

Please elaborate. Spelling of what?

>
> >
> > All major data structures of the driver are fully introduced with the
> > code that uses them but without their initialization code that requires
> > management interface with the hw.
> >
> > Submitted-by: Gur Stavi <gur.stavi@huawei.com>
>
> this tag is not needed
>
> > Signed-off-by: Gur Stavi <gur.stavi@huawei.com>
>
> your tag should be last
>
> > Signed-off-by: Xin Guo <guoxin09@huawei.com>
>
> no idea what Xin did, if you want to give credit to someone
> you could use Co-developed-by: tag (put it just above the SoB)
>
> > Signed-off-by: gongfan <gongfan1@huawei.com>
>
> It would be much appricieated if you will spell the full name
> for gongfan, at least two parts (like you did for the rest),
> especially for someone you put into the MAINTAINERS file.
> OTOH, please keep in mind that maintainer should be active
> on the mailing list (regarding this driver).
>

Ack

> > +
> > +Global function ID
> > +------------------
> > +
> > +Every function, PF or VF, has a unique ordinal identification within the device.
> > +Many commands to management (mbox or cmdq) contain this ID so HW can apply the
>
> s/commands to management/management commands/
>

Ack

> > +L:	netdev@vger.kernel.org
> > +S:	Supported
>
> there is a (relatively new) requirement that the device needs to report
> to the netdev CI to have an "S" here, so for now you should use "M"
>

Ack

> > +	# Fields of HW and management structures are little endian and are
> > +	# currently not converted
> > +	depends on !CPU_BIG_ENDIAN
> > +	depends on X86 || ARM64 || COMPILE_TEST
> > +	depends on PCI_MSI && 64BIT
> > +	select AUXILIARY_BUS
> > +	help
> > +	  This driver supports HiNIC PCIE Ethernet cards.
> > +	  To compile this driver as part of the kernel, choose Y here.
> > +	  If unsure, choose N.
> > +	  The default is N.
>
> the last 3 lines are very much obvious/redundant
>

Ack

> > diff --git a/drivers/net/ethernet/huawei/hinic3/Makefile b/drivers/net/ethernet/huawei/hinic3/Makefile
> > new file mode 100644
> > index 000000000000..02656853f629
> > --- /dev/null
> > +++ b/drivers/net/ethernet/huawei/hinic3/Makefile
> > @@ -0,0 +1,21 @@
> > +# SPDX-License-Identifier: GPL-2.0
> > +# Copyright (c) Huawei Technologies Co., Ltd. 2024. All rights reserved.
>
> too late
>

Ack

> > +
> > +obj-$(CONFIG_HINIC3) += hinic3.o
> > +
> > +hinic3-objs := hinic3_hwdev.o \
> > +	       hinic3_lld.o \
> > +	       hinic3_common.o \
> > +	       hinic3_hwif.o \
> > +	       hinic3_hw_cfg.o \
> > +	       hinic3_queue_common.o \
> > +	       hinic3_mbox.o \
> > +	       hinic3_hw_comm.o \
> > +	       hinic3_wq.o \
> > +	       hinic3_nic_io.o \
> > +	       hinic3_nic_cfg.o \
> > +	       hinic3_tx.o \
> > +	       hinic3_rx.o \
> > +	       hinic3_netdev_ops.o \
> > +	       hinic3_rss.o \
> > +	       hinic3_main.o
>
> in general, any list of random things could be sorted (to ease out git
> merges in the future)
>

Ack

> > +	(((u32)(hwdev)->cfg_mgmt->svc_cap.chip_svc_type) & BIT(HINIC3_SERVICE_T_NIC))
> > +
> > +bool hinic3_support_nic(struct hinic3_hwdev *hwdev)
> > +{
> > +	if (!IS_NIC_TYPE(hwdev))
> > +		return false;
> > +
> > +	return true;
>
> this is just:
> 	return IS_NIC_TYPE(hwdev);
>

Ack

> > +static int comm_msg_to_mgmt_sync(struct hinic3_hwdev *hwdev, u16 cmd, const void *buf_in,
> > +				 u32 in_size, void *buf_out, u32 *out_size)
> > +{
> > +	return hinic3_send_mbox_to_mgmt(hwdev, HINIC3_MOD_COMM, cmd, buf_in,
> > +					in_size, buf_out, out_size, 0);
> > +}
> > +
> > +int hinic3_func_reset(struct hinic3_hwdev *hwdev, u16 func_id, u64 reset_flag)
> > +{
> > +	struct comm_cmd_func_reset func_reset;
>
>   = {}
>
> > +	u32 out_size = sizeof(func_reset);
> > +	int err;
> > +
> > +	memset(&func_reset, 0, sizeof(func_reset));
>
> with "= {}" above, memset() could be eliminated
>
> > +	func_reset.func_id = func_id;
> > +	func_reset.reset_flag = reset_flag;
>
> alternative would be
> 	struct comm_cmd_func_reset func_reset = {
> 		.func_id = func_id,
> 		.reset_flag = reset_flag,
> 	};
>

Ack

> > +++ b/drivers/net/ethernet/huawei/hinic3/hinic3_hw_intf.h
> > @@ -0,0 +1,88 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/* Copyright (c) Huawei Technologies Co., Ltd. 2024. All rights reserved. */
> > +
> > +#ifndef HINIC3_HW_INTF_H
>
> please add two underscores to the header guards
>

Ack

> > +#define HINIC3_HW_INTF_H