linux-kernel - Re: [PATCH] SPI LPC information kernel module

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: <20200630085839.GE637809@kroah.com>
Date:   Tue, 30 Jun 2020 10:58:39 +0200
From:   Greg Kroah-Hartman <gregkh@...uxfoundation.org>
To:     Daniel Gutson <daniel.gutson@...ypsium.com>
Cc:     Derek Kiernan <derek.kiernan@...inx.com>,
        Arnd Bergmann <arnd@...db.de>,
        Mauro Carvalho Chehab <mchehab+huawei@...nel.org>,
        linux-kernel@...r.kernel.org, Richard Hughes <hughsient@...il.com>,
        Alex Bazhaniuk <alex@...ypsium.com>
Subject: Re: [PATCH] SPI LPC information kernel module

On Mon, Jun 29, 2020 at 07:59:32PM -0300, Daniel Gutson wrote:
> --- /dev/null
> +++ b/drivers/misc/spi_lpc/bios_data_access.h
> @@ -0,0 +1,181 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * SPI LPC flash platform security driver
> + *
> + * Copyright 2020 (c) Daniel Gutson (daniel.gutson@...ypsium.com)
> + *
> + */
> +#ifndef BIOS_DATA_ACCESS_H
> +#define BIOS_DATA_ACCESS_H

What BIOS?  Is this a UEFI thing?  ACPI?  Uboot?  Fastboot?

> +
> +#include <linux/types.h>
> +
> +enum PCH_Arch {

Why the upper-case name?

> +	pch_none,
> +	pch_6_c200,
> +	pch_7_c210,
> +	pch_c60x_x79,
> +	pch_communications_89xx,
> +	pch_8_c220,
> +	pch_c61x_x99,
> +	pch_5_mobile,
> +	pch_6_mobile,
> +	pch_7_8_mobile,
> +	pch_1xx,
> +	pch_c620,
> +	pch_2xx,
> +	pch_3xx,
> +	pch_4xx,
> +	pch_495,
> +	pch_5xx,
> +	PCH_Archs_count

Do these have real values that the hardware expects?  If so, you need to
assign them numbers, right?  If not, what do all of these mean?

> +};
> +
> +enum CPU_Arch {
> +	cpu_none,
> +	cpu_bdw,
> +	cpu_bdx,
> +	cpu_hsw,
> +	cpu_hsx,
> +	cpu_ivt,
> +	cpu_jkt,
> +	cpu_kbl,
> +	cpu_skl,
> +	cpu_ivb,
> +	cpu_snb,
> +	cpu_avn,
> +	cpu_cfl,
> +	cpu_byt,
> +	cpu_whl,
> +	cpu_cml,
> +	cpu_icl,
> +	cpu_apl,
> +	cpu_glk,
> +	cpu_tgl,
> +	cpu_amd,
> +	CPU_Archs_count
> +};
> +
> +enum RegisterSource { RegSource_PCH, RegSource_CPU };

Again all of the Caps, please don't use, that's not Linux styles (and
checkpatch.pl should have warned you about this, did you run it on your
patch?)

thanks,

greg k-h