Linux-EDAC Archive on lore.kernel.org
 help / color / Atom feed
From: Borislav Petkov <bp@alien8.de>
To: Isaac Vaughn <isaac.vaughn@Knights.ucf.edu>,
	Yazen Ghannam <yazen.ghannam@amd.com>
Cc: "trivial@kernel.org" <trivial@kernel.org>,
	"linux-edac@vger.kernel.org" <linux-edac@vger.kernel.org>
Subject: Re: [PATCH] amd64_edac - Add Family 17h Model 70h PCI IDs
Date: Wed, 4 Sep 2019 23:53:08 +0200
Message-ID: <20190904215308.GA12374@zn.tnic> (raw)
In-Reply-To: <BN7PR07MB5186DD5732B95B784A9D46BFCEB80@BN7PR07MB5186.namprd07.prod.outlook.com>

Hi Isaac,

On Wed, Sep 04, 2019 at 09:13:47PM +0000, Isaac Vaughn wrote:
> I noticed the current EDAC driver doesn't support the new Zen 2 (model
> 70h) processors, so I patched the new device ids in. The changes are
> minimal, I merely extended the existing enums with information from
> the new models.

Makes sense to me.

@Yazen, ACK?

> This is my first kernel patch and I'm not sure I followed the guide
> (kernel.org/doc/html/v5.2/process/submitting-patches.html) correctly,
> so please let me know if I need to resubmit this email with updated
> formatting prior to review.

Sure, you're almost there but it needs to have a title and a commit
message. If you need an example, look at how a similar patch to yours is
formatted:

6e846239e548 ("EDAC/amd64: Add Family 17h Model 30h PCI IDs")

Here's a web URL too in case you don't know how to work the git thing :)

https://git.kernel.org/linus/6e846239e5487cbb89ac8192d5f11437d010130e

That is, if you want to learn how to do patches at all. Otherwise, I can
fix it up for ya.

Leaving in the rest for Yazen.

> Signed-off-by: Isaac Vaughn <isaac.vaughn@knights.ucf.edu>
> ---
> 
> diff --git a/drivers/edac/amd64_edac.c b/drivers/edac/amd64_edac.c
> index 873437be86d9..a35c97f9100a 100644
> --- a/drivers/edac/amd64_edac.c
> +++ b/drivers/edac/amd64_edac.c
> @@ -2253,6 +2253,15 @@ static struct amd64_family_type family_types[] = {
>                         .dbam_to_cs             = f17_base_addr_to_cs_size,
>                 }
>         },
> + [F17_M70H_CPUS] = {
> +         .ctl_name = F17h_M70h,
> +         .f0_id = PCI_DEVICE_ID_AMD_17H_M70H_DF_F0,
> +         .f6_id = PCI_DEVICE_ID_AMD_17H_M70H_DF_F6,
> +         .ops = {
> +                 .early_channel_count        = f17_early_channel_count,
> +                 .dbam_to_cs         = f17_base_addr_to_cs_size,
> +         }
> + },

You need to fix the tabbing here to look like the other struct
assignments above.

>  };
>  
>  /*
> @@ -3241,6 +3250,10 @@ static struct amd64_family_type *per_family_init(struct amd64_pvt *pvt)
>                         fam_type = &family_types[F17_M30H_CPUS];
>                         pvt->ops = &family_types[F17_M30H_CPUS].ops;
>                         break;
> +         } else if (pvt->model >= 0x70 && pvt->model <= 0x7f) {
> +                 fam_type = &family_types[F17_M70H_CPUS];
> +                 pvt->ops = &family_types[F17_M70H_CPUS].ops;
> +                 break;
>                 }

Here too. Your "else if" needs to have the same indentation as the one above.

>                 /* fall through */
>         case 0x18:
> diff --git a/drivers/edac/amd64_edac.h b/drivers/edac/amd64_edac.h
> index 8f66472f7adc..1adf7ddbf744 100644
> --- a/drivers/edac/amd64_edac.h
> +++ b/drivers/edac/amd64_edac.h
> @@ -119,6 +119,8 @@
>  #define PCI_DEVICE_ID_AMD_17H_M10H_DF_F6 0x15ee
>  #define PCI_DEVICE_ID_AMD_17H_M30H_DF_F0 0x1490
>  #define PCI_DEVICE_ID_AMD_17H_M30H_DF_F6 0x1496
> +#define PCI_DEVICE_ID_AMD_17H_M70H_DF_F0 0x1440
> +#define PCI_DEVICE_ID_AMD_17H_M70H_DF_F6 0x1446
>  
>  /*
>   * Function 1 - Address Map
> @@ -285,6 +287,7 @@ enum amd_families {
>         F17_CPUS,
>         F17_M10H_CPUS,
>         F17_M30H_CPUS,
> + F17_M70H_CPUS,

And that too.

Depending on the text editor, you can show the whitespace characters on each
line and then make sure they're the same as on the lines above it. For example
with vim you do

":set list"

and that would show you:

^I^I} else if (pvt->model >= 0x30 && pvt->model <= 0x3f) {$ 
^I^I^Ifam_type = &family_types[F17_M30H_CPUS];$
^I^I^Ipvt->ops = &family_types[F17_M30H_CPUS].ops;$
^I^I^Ibreak;$
         } else if (pvt->model >= 0x70 && pvt->model <= 0x7f) {$
                 fam_type = &family_types[F17_M70H_CPUS];$
                 pvt->ops = &family_types[F17_M70H_CPUS].ops;$
                 break;$
^I^I}$


and you can immediately see the difference. We use tabs in the kernel,
which appear here as ^I and you have spaces, thus the difference in
indentation.

There's a checker script in the kernel source directory you can use:

$ ./scripts/checkpatch.pl /tmp/isaac.01
WARNING: please, no spaces at the start of a line
#34: FILE: drivers/edac/amd64_edac.c:2256:
+ [F17_M70H_CPUS] = {$

ERROR: code indent should use tabs where possible
#35: FILE: drivers/edac/amd64_edac.c:2257:
+         .ctl_name = F17h_M70h,$

WARNING: please, no spaces at the start of a line
#35: FILE: drivers/edac/amd64_edac.c:2257:
+         .ctl_name = F17h_M70h,$

ERROR: code indent should use tabs where possible
#36: FILE: drivers/edac/amd64_edac.c:2258:
+         .f0_id = PCI_DEVICE_ID_AMD_17H_M70H_DF_F0,$

...

which would show you what is wrong with the patch.

And finally, here some more links which you might find useful:

https://www.kernel.org/doc/html/v5.2/process/coding-style.html
https://kernelnewbies.org/

That is, if you want to get involved in doing the patches thing :-)

Thx and have fun!

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

  reply index

Thread overview: 18+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-09-04 21:13 Isaac Vaughn
2019-09-04 21:53 ` Borislav Petkov [this message]
2019-09-04 22:21   ` Ghannam, Yazen
2019-09-05  1:21   ` Isaac Vaughn
2019-09-05  7:09     ` Borislav Petkov
2019-09-05 13:17       ` Isaac Vaughn
2019-09-05 13:54         ` Borislav Petkov
2019-09-06  1:01           ` [PATCH] Add PCI device IDs for family 17h, model 70h Isaac Vaughn
2019-09-06  1:41             ` Ghannam, Yazen
2019-09-06  1:56           ` Isaac Vaughn
2019-09-06  9:12             ` Borislav Petkov
     [not found]               ` <20190906075729.9e2faf7147da62fc26006833@knights.ucf.edu>
2019-09-06 12:14                 ` Borislav Petkov
2019-09-06 13:02               ` Guenter Roeck
2019-09-06 13:09                 ` Boris Petkov
     [not found]                   ` <B08C8E54-43FA-4E29-8D7D-5F9C4AF20CCF@Knights.ucf.edu>
2019-09-06 14:50                     ` Borislav Petkov
2019-09-06 23:27                       ` Isaac Vaughn
2019-09-06 16:11                   ` Guenter Roeck
2019-09-06 16:22                     ` Borislav Petkov

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20190904215308.GA12374@zn.tnic \
    --to=bp@alien8.de \
    --cc=isaac.vaughn@Knights.ucf.edu \
    --cc=linux-edac@vger.kernel.org \
    --cc=trivial@kernel.org \
    --cc=yazen.ghannam@amd.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-EDAC Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-edac/0 linux-edac/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-edac linux-edac/ https://lore.kernel.org/linux-edac \
		linux-edac@vger.kernel.org linux-edac@archiver.kernel.org
	public-inbox-index linux-edac

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-edac


AGPL code for this site: git clone https://public-inbox.org/ public-inbox