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

[Simon Horman <horms@kernel.org>]

On Wed, Jan 15, 2025 at 06:22:46PM +0800, Xin Tian wrote:
> Enable cmd queue to support driver-firmware communication.
> Hardware control will be performed through cmdq mostly.
> 
> Co-developed-by: Honggang Wei <weihg@yunsilicon.com>
> Signed-off-by: Honggang Wei <weihg@yunsilicon.com>
> Co-developed-by: Lei Yan <jacky@yunsilicon.com>
> Signed-off-by: Lei Yan <jacky@yunsilicon.com>
> Signed-off-by: Xin Tian <tianx@yunsilicon.com>

...

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

...

> +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

...

> +static void xsc_cmd_comp_handler(struct xsc_core_device *xdev, u8 idx, struct xsc_rsp_layout *rsp)
> +{
> +	struct xsc_cmd *cmd = &xdev->cmd;
> +	struct xsc_cmd_work_ent *ent;
> +	struct xsc_inbox_hdr *hdr;
> +
> +	if (idx > cmd->max_reg_cmds || (cmd->bitmask & (1 << idx))) {
> +		pci_err(xdev->pdev, "idx[%d] exceed max cmds, or has no relative request.\n", idx);
> +		return;
> +	}
> +	ent = cmd->ent_arr[idx];
> +	ent->rsp_lay = rsp;
> +	ktime_get_ts64(&ent->ts2);
> +
> +	memcpy(ent->out->first.data, ent->rsp_lay->out, sizeof(ent->rsp_lay->out));
> +	if (!cmd->checksum_disabled)
> +		ent->ret = verify_signature(ent);
> +	else
> +		ent->ret = 0;
> +	ent->status = 0;
> +
> +	hdr = (struct xsc_inbox_hdr *)ent->in->first.data;

nit: hdr is set but otherwise unused in this function.
     Perhaps it can be removed (and added in a later patch if needed there)?

     Flagged by W=1 builds.

> +	free_ent(cmd, ent->idx);
> +	complete(&ent->done);
> +	up(&cmd->sem);
> +}

...