linux-kernel - Re: [PATCH v1 1/7] Add Driver for SUNIX PCI(e) I/O expansion board

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]

Message-ID: <391203d8794c2fc5343f2bf991a5497d3d5b7fcd.camel@perches.com>
Date:   Wed, 31 Mar 2021 15:49:19 -0700
From:   Joe Perches <joe@...ches.com>
To:     Moriis Ku <saumah@...il.com>, lee.jones@...aro.org
Cc:     linux-kernel@...r.kernel.org
Subject: Re: [PATCH v1 1/7] Add Driver for SUNIX PCI(e) I/O expansion board

On Tue, 2021-03-30 at 16:23 +0800, Moriis Ku wrote:
> From: Morris <saumah@...il.com>
> 
> Signed-off-by: Morris <saumah@...il.com>
> ---
>  spi_pack.c | 1506 ++++++++++++++++++++++++++++++++++++++++++++++++++++

You might try to use scripts/checkpatch.pl on this and see
if there is anything you want to change to have the code look
more like the rest of the kernel.

Using
	./scripts/checkpatch.pl --strict --fix <patch>
from the top of the kernel tree should help quite a bit with
making the code layout look more like typical kernel style.

> diff --git a/spi_pack.c b/spi_pack.c
[]
> +static void get_info(struct sunix_sdc_spi_channel *spi_chl, unsigned int incomeLength, unsigned int * translateLength)
> +{
[]
> +	do
> +	{
> +		Address = spi_chl->info.phy2_base_start + spi_chl->info.memoffset;
> +
> +	} while (false);

That's an odd and unnecessary use of a do while.

> +	memset(TrBuff, 0, SUNIX_SDC_SPI_BUFF);
> +	TrLength = 0;
> +
> +	pTrHeader->Version = 0x01;
> +	pTrHeader->CmdResponseEventData = pRxHeader->CmdResponseEventData | 0x8000;
> +	pTrHeader->ResponseStatus = nStatus;
> +	pTrHeader->Length = (nStatus == SDCSPI_STATUS_SUCCESS)?(31 + (cib_info->spi_number_of_device * 12)):0;
> +	TrLength = sizeof(SPI_HEADER);

Perhaps reorder the patch submission to define the structs first.

This can't compile as SPI_HEADER isn't defined
SPI_HEADER and PSPI_HEADER should use not use typedefs and use the typical
	struct spi_header
and
	struct spi_header *

> +	if (pTrHeader->ResponseStatus == SDCSPI_STATUS_SUCCESS)

Hungarian isn't generally used in the kernel.
CamelCase isn't generally used either.

> +	{
> +		memcpy(&TrBuff[TrLength], spi_chl->info.model_name, 16);
> +		TrLength += 16;
> +		TrBuff[TrLength++] = spi_chl->info.bus_number;
> +		TrBuff[TrLength++] = spi_chl->info.dev_number;
> +		TrBuff[TrLength++] = spi_chl->info.line;
> +		TrBuff[TrLength++] = (unsigned char)((Address & 0xff000000) >> 24);
> +		TrBuff[TrLength++] = (unsigned char)((Address & 0x00ff0000) >> 16);
> +		TrBuff[TrLength++] = (unsigned char)((Address & 0x0000ff00) >> 8);
> +		TrBuff[TrLength++] = (unsigned char)((Address & 0x000000ff));
> +		TrBuff[TrLength++] = (unsigned char)(spi_chl->info.irq);

You might consider using a single char * and increment that and not use
TrLength at all and use p - TrBuff when necessary.

		*p++ = spi_chl->info.bus_number;
		*p++ = spi_chl->info.dev_number;
		...

> +		TrBuff[TrLength++] = (unsigned char)((cib_info->spi_significand_of_clock & 0xff000000) >> 24);
> +		TrBuff[TrLength++] = (unsigned char)((cib_info->spi_significand_of_clock & 0x00ff0000) >> 16);
> +		TrBuff[TrLength++] = (unsigned char)((cib_info->spi_significand_of_clock & 0x0000ff00) >> 8);
> +		TrBuff[TrLength++] = (unsigned char)((cib_info->spi_significand_of_clock & 0x000000ff));

Perhaps

		put_unaligned_le32(cib_info->spi_significant_of_clock, p);
		p += 4;

etc...