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

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] [thread-next>] [day] [month] [year] [list]

Message-Id: <0ed84d3f-9571-4bd9-831e-7aff64de3eb8@yunsilicon.com>
Date: Fri, 17 Jan 2025 11:42:22 +0800
From: "tianx" <tianx@...silicon.com>
To: "Simon Horman" <horms@...nel.org>
Cc: <netdev@...r.kernel.org>, <leon@...nel.org>, <andrew+netdev@...n.ch>, 
	<kuba@...nel.org>, <pabeni@...hat.com>, <edumazet@...gle.com>, 
	<davem@...emloft.net>, <jeff.johnson@....qualcomm.com>, 
	<przemyslaw.kitszel@...el.com>, <weihg@...silicon.com>, 
	<wanry@...silicon.com>
Subject: Re: [PATCH v3 02/14] net-next/yunsilicon: Enable CMDQ

On 2025/1/16 21:55, Simon Horman wrote:
> 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@...silicon.com>
>> Signed-off-by: Honggang Wei <weihg@...silicon.com>
>> Co-developed-by: Lei Yan <jacky@...silicon.com>
>> Signed-off-by: Lei Yan <jacky@...silicon.com>
>> Signed-off-by: Xin Tian <tianx@...silicon.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
>
> ...
Thank you for the tool. I will address the same issue across the entire 
patch set.
>> +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.
Ack, will remove.
>> +	free_ent(cmd, ent->idx);
>> +	complete(&ent->done);
>> +	up(&cmd->sem);
>> +}
> ...