Re: [PATCH v3 02/14] net-next/yunsilicon: Enable CMDQ

On Fri, Jan 17, 2025 at 11:42:22AM +0800, tianx wrote:

On 2025/1/16 21:55, Simon Horman wrote:

quoted

On Wed, Jan 15, 2025 at 06:22:46PM +0800, Xin Tian wrote:

quoted

Enable cmd queue to support driver-firmware communication.
Hardware control will be performed through cmdq mostly.

Co-developed-by: Honggang Wei <redacted>
Signed-off-by: Honggang Wei <redacted>
Co-developed-by: Lei Yan <redacted>
Signed-off-by: Lei Yan <redacted>
Signed-off-by: Xin Tian <redacted>

...

quoted

diff --git a/drivers/net/ethernet/yunsilicon/xsc/pci/cmdq.c b/drivers/net/ethernet/yunsilicon/xsc/pci/cmdq.c

...

quoted

+static void cmd_work_handler(struct work_struct *work)
+{
+	struct xsc_cmd_work_ent *ent = container_of(work, struct xsc_cmd_work_ent, work);
+	struct xsc_cmd *cmd = ent->cmd;
+	struct xsc_core_device *xdev = container_of(cmd, struct xsc_core_device, cmd);
+	struct xsc_cmd_layout *lay;
+	struct semaphore *sem;
+	unsigned long flags;

Hi Xin Tian,

Please consider arranging local variables in reverse xmas tree order -
longest line to shortest - as is preferred in Networking code.
Separating initialisation from declarations as needed.

And also, please consider limiting lines to 80 columns wide or less,
where it can be achieved without reducing readability. This is also
preferred in Networking code.

I think that in this case that both could be achieved like this
(completely untested):

	struct xsc_cmd_work_ent *ent;
	struct xsc_core_device *xdev;
	struct xsc_cmd_layout *lay;
	struct semaphore *sem;
	struct xsc_cmd *cmd;
	unsigned long flags;

	ent = container_of(work, struct xsc_cmd_work_ent, work);
	cmd = ent->cmd;
	xdev = container_of(cmd, struct xsc_core_device, cmd);

With regards to reverse xmas tree ordering, this tool can be useful:
https://github.com/ecree-solarflare/xmastree

...

Thank you for the tool. I will address the same issue across the entire 
patch set.

Thanks. I forgot to mention that the 80 column preference can
be tested using ./scripts/checkpatch.pl --max-line-length=80

...