Re: [PATCH 0/3] Detect suspicious indentation after conditional

[Julius Werner]

*friendly ping*

Hi Andy, Joe,

Any comments on this patch series? Are you guys the right point of
contact for checkpatch changes?

On Thu, Mar 25, 2021 at 8:50 PM Julius Werner  wrote:
>
> This patch series is adding functionality to checkpatch.pl to test for
> incorrect code indentation after a conditional statement, like this:
>
>  if (a)
>b;
>c;
>
> (Indentation implies that `c;` was guarded by the conditional, but it
> isn't.) The main part is re-sending a patch from Ivo Sieben that was
> already proposed in 2014 [1]. I don't know why it was never merged --
> it seems that there was no discussion on it. I hope that it was only
> overlooked, because it works great, and I think this is a very important
> class of common error to catch.
>
> I have tested it extensively on the kernel tree and in the course of
> that found a few more edge cases that get fixed by the other two
> patches. With all these applied, the vast majority of hits I get from
> this check on the kernel tree are actual indentation errors or other
> code style violations (e.g. case label and statement on the same line).
> The only significant remaining group of false positives I found are
> cases of macros being defined within a function, which are overall very
> rare. I think the benefit of adding this check would far outweigh the
> remaining amount of noise.
>
> [1]: https://lore.kernel.org/patchwork/patch/465116
>
> Ivo Sieben (1):
>   Suspicious indentation detection after conditional statement
>
> Julius Werner (2):
>   checkpatch: ctx_statement_block: Fix preprocessor guard tracking
>   checkpatch: Ignore labels when checking indentation
>
>  scripts/checkpatch.pl | 56 +++
>  1 file changed, 52 insertions(+), 4 deletions(-)
>
> --
> 2.29.2
>

[PATCH 3/3] checkpatch: Ignore labels when checking indentation

[Julius Werner]

Goto labels are commonly written in the leftmost column (sometimes with
one space in front), regardless of indentation level. Sometimes they're
on a line of their own, but sometimes the same line is shared with a
normal code statement that then starts at the expected indentation
level. When checking indentation, we should check where that normal
piece of code starts, not where the label starts (there's a separate
INDENTED_LABEL test to check the label itself). Therefore, the
line_stats() function that is used to get indentation level should treat
goto labels like whitespace. The SUSPICIOUS_CODE_INDENT test also needs
to explicitly ignore labels to make sure it doesn't get confused by
them.

Signed-off-by: Julius Werner 
---
 scripts/checkpatch.pl | 9 -
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index c1dfc0107be41d..d89367a59e7d37 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -1396,8 +1396,12 @@ sub copy_spacing {
 sub line_stats {
my ($line) = @_;
 
-   # Drop the diff line leader and expand tabs
+   # Drop the diff line leader
$line =~ s/^.//;
+
+   # Treat labels like whitespace when counting indentation
+   $line =~ s/^( ?$Ident:)/" " x length($1)/e;
+
$line = expand_tabs($line);
 
# Pick the indent from the front of the line.
@@ -4197,6 +4201,9 @@ sub process {
# Remove any comments
$s_next =~ s/$;//g;
 
+   # Remove any leading labels
+   $s_next =~ s/\n( ?$Ident:)/"\n" . " " x length($1)/eg;
+
# Skip this check for in case next statement starts 
with 'else'
if ($s_next !~ /\s*\belse\b/) {
 
-- 
2.29.2

[PATCH 2/3] Suspicious indentation detection after conditional statement

[Julius Werner]

From: Ivo Sieben 

Raise a SUSPICIOUS_CODE_INDENT warning when unexpected indentation is found
after a conditional statement. This can be used to find missing braces or
wrong indentation in/after a conditional statement.

For example the following error is caught;

if (foo)
bar();
return;

Which can be either intended by the programmer as:

if (foo)
bar();
return;

or
if (foo) {
bar();
return;
}

Signed-off-by: Ivo Sieben 
---
 scripts/checkpatch.pl | 41 +
 1 file changed, 41 insertions(+)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index ffccbd2033e579..c1dfc0107be41d 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -4185,6 +4185,47 @@ sub process {
WARN("SUSPECT_CODE_INDENT",
 "suspect code indent for conditional 
statements ($indent, $sindent)\n" . $herecurr . "$stat_real\n");
}
+
+# Also check if the next statement after the previous condition has the same 
indent
+   my ($stat_next, undef, $line_nr_next_next) =
+   ctx_statement_block($line_nr_next, 
$remain_next, $off_next);
+   my $s_next = $stat_next;
+
+   # Remove line prefixes
+   $s_next =~ s/\n./\n/gs;
+
+   # Remove any comments
+   $s_next =~ s/$;//g;
+
+   # Skip this check for in case next statement starts 
with 'else'
+   if ($s_next !~ /\s*\belse\b/) {
+
+   # Remove while that belongs to a do {} while
+   if ($stat =~ /\bdo\b/) {
+   $s_next =~ 
s/^.*\bwhile\b\s*($balanced_parens)\s*?//;
+   }
+
+   # Remove blank lines
+   $s_next =~ s/\s*\\?\n//g;
+
+   # Get the real next lines
+   my $next_nof_lines = $line_nr_next_next - 
$line_nr_next;
+   my $stat_next_real = raw_line($line_nr_next, 
$next_nof_lines);
+   if (!defined($stat_next_real)) {
+   $stat_next_real = "";
+   } elsif ($next_nof_lines > 1) {
+   $stat_next_real = 
"[...]\n$stat_next_real";
+   }
+   my (undef, $nindent) = line_stats('+' . 
$s_next);
+
+   #print "stat_next<$stat_next> stat<$stat> 
indent<$indent> nindent<$nindent> s_next<$s_next> 
stat_next_real<$stat_next_real>\n";
+
+   if ($nindent > $indent) {
+   WARN("SUSPICIOUS_CODE_INDENT",
+"suspicious code indentation after 
conditional statements\n" .
+$herecurr . 
"$stat_real\n$stat_next_real\n");
+   }
+   }
}
 
# Track the 'values' across context and added lines.
-- 
2.29.2

[PATCH 1/3] checkpatch: ctx_statement_block: Fix preprocessor guard tracking

[Julius Werner]

The preprocessor guard tracking in ctx_statement_block() is (and seems
to have always been) subtly broken whenever tracking over an #else: the
code is supposed to restore state from the current top of the stack
(like and #endif just without removing it). However, it indexes the
stack at [$#stack - 1]. In Perl, $# does not give you the length of an
array, it gives you the highest valid index. Therefore, the correct
index should just be [$#stack].

The preprocessor guard tracking also gets confused when
ctx_statement_block() was called on a line that is already inside a
preprocessor guard, and steps out of it within the same statement. This
happens commonly with constructs like this:

 #if CONFIG_XXX
   for (a = first_a(); !a_finished(); a = next_a()) {
 #else
   for (b = first_b(); !b_finished(); b = next_b()) {
 #endif
 ... loop body ...

The best course of action in this case is to not try to restore any
previous state (which we don't have) at all, so we should just keep our
current state if $#stack is already 0.

Signed-off-by: Julius Werner 
---
 scripts/checkpatch.pl | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index df8b23dc1eb0af..ffccbd2033e579 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -1572,9 +1572,9 @@ sub ctx_statement_block {
# Handle nested #if/#else.
if ($remainder =~ /^#\s*(?:ifndef|ifdef|if)\s/) {
push(@stack, [ $type, $level ]);
-   } elsif ($remainder =~ /^#\s*(?:else|elif)\b/) {
-   ($type, $level) = @{$stack[$#stack - 1]};
-   } elsif ($remainder =~ /^#\s*endif\b/) {
+   } elsif ($remainder =~ /^#\s*(?:else|elif)\b/ && $#stack > 0) {
+   ($type, $level) = @{$stack[$#stack]};
+   } elsif ($remainder =~ /^#\s*endif\b/ && $#stack > 0) {
($type, $level) = @{pop(@stack)};
}
 
-- 
2.29.2

[PATCH 0/3] Detect suspicious indentation after conditional

[Julius Werner]

This patch series is adding functionality to checkpatch.pl to test for
incorrect code indentation after a conditional statement, like this:

 if (a)
   b;
   c;

(Indentation implies that `c;` was guarded by the conditional, but it
isn't.) The main part is re-sending a patch from Ivo Sieben that was
already proposed in 2014 [1]. I don't know why it was never merged --
it seems that there was no discussion on it. I hope that it was only
overlooked, because it works great, and I think this is a very important
class of common error to catch.

I have tested it extensively on the kernel tree and in the course of
that found a few more edge cases that get fixed by the other two
patches. With all these applied, the vast majority of hits I get from
this check on the kernel tree are actual indentation errors or other
code style violations (e.g. case label and statement on the same line).
The only significant remaining group of false positives I found are
cases of macros being defined within a function, which are overall very
rare. I think the benefit of adding this check would far outweigh the
remaining amount of noise.

[1]: https://lore.kernel.org/patchwork/patch/465116

Ivo Sieben (1):
  Suspicious indentation detection after conditional statement

Julius Werner (2):
  checkpatch: ctx_statement_block: Fix preprocessor guard tracking
  checkpatch: Ignore labels when checking indentation

 scripts/checkpatch.pl | 56 +++
 1 file changed, 52 insertions(+), 4 deletions(-)

-- 
2.29.2

[PATCH] dt-bindings: ddr: Add optional manufacturer and revision ID to LPDDR3

[Julius Werner]

On some platforms, DDR parts are multi-sourced and the exact part number
used is not know to either kernel or firmware at build time. Firmware
can read identifying information from DDR mode registers at boot time
but needs a way to communicate this information to kernel and/or
userspace. This patch adds optional properties for this information to
the existing "jedec,lpddr3" device tree binding to be used for that
purpose.

Signed-off-by: Julius Werner 
---
 Documentation/devicetree/bindings/ddr/lpddr3.txt | 5 +
 1 file changed, 5 insertions(+)

diff --git a/Documentation/devicetree/bindings/ddr/lpddr3.txt 
b/Documentation/devicetree/bindings/ddr/lpddr3.txt
index a0eda35a86eef9..b221e653d3845e 100644
--- a/Documentation/devicetree/bindings/ddr/lpddr3.txt
+++ b/Documentation/devicetree/bindings/ddr/lpddr3.txt
@@ -12,6 +12,9 @@ Required properties:
 
 Optional properties:
 
+- manufacturer-id :  Manufacturer ID value read from Mode Register 5
+- revision-id :  Revision IDs read from Mode Registers 6 and 7
+
 The following optional properties represent the minimum value of some AC
 timing parameters of the DDR device in terms of number of clock cycles.
 These values shall be obtained from the device data-sheet.
@@ -49,6 +52,8 @@ samsung_K3QF2F20DB: lpddr3 {
compatible  = "samsung,K3QF2F20DB", "jedec,lpddr3";
density = <16384>;
io-width= <32>;
+   manufacturer-id = <1>;
+   revision-id = <123 234>;
#address-cells  = <1>;
#size-cells = <0>;
 
-- 
2.29.2

Re: [PATCH] ALSA: hda/realtek: Add quirk for Clevo NH55RZQ

[Werner Sembach]

Hello Takashi,
> The patch isn't cleanly applicable.  Could you try to submit via
> git-send-email instead?

I'm sorry the editor + mailer i used replaced tabs with spaces and made a html 
mail.

Already resend the patch with tabs, but only after that I realized that even 
the "plain" mails are converted to html.

It's probably still broken so don't waste your time with it, i will read the 
man page of git send-mail now ^^.

> Cc to stable is worthwhile.  You can simply add
>   Cc: 
>
> around your signed-off-by lines in the patch.

Ok (also forgot that in my resend patch)

Kind regards,

Werner Sembach

[PATCH] ALSA: hda/realtek: Add quirk for Intel NUC 10

[Werner Sembach]

ALSA: hda/realtek: Add quirk for Intel NUC 10

This adds a new SND_PCI_QUIRK(...) and applies it to the Intel NUC 10
devices. This fixes the issue of the devices not having audio input and
output on the headset jack because the kernel does not recognize when
something is plugged in.

The new quirk was inspired by the quirk for the Intel NUC 8 devices, but
it turned out that the NUC 10 uses another pin. This information was
acquired by black box testing likely pins.

Co-developed-by: Eckhart Mohr 
Signed-off-by: Eckhart Mohr 
Signed-off-by: Werner Sembach 
Cc: 
---
Forgot to add the "From"-line

>From d281364b8ca6c054a0e5ce20caa599bf7d08160d Mon Sep 17 00:00:00 2001
From: Werner Sembach 
Date: Fri, 26 Feb 2021 13:54:30 +0100
Subject: [PATCH] Fix Intel NUC10 no output and input on headset jack

---
 sound/pci/hda/patch_realtek.c | 11 +++
 1 file changed, 11 insertions(+)

diff --git a/sound/pci/hda/patch_realtek.c b/sound/pci/hda/patch_realtek.c
index 290645516313..c14d624dbaf1 100644
--- a/sound/pci/hda/patch_realtek.c
+++ b/sound/pci/hda/patch_realtek.c
@@ -6362,6 +6362,7 @@ enum {
ALC269_FIXUP_LEMOTE_A1802,
ALC269_FIXUP_LEMOTE_A190X,
ALC256_FIXUP_INTEL_NUC8_RUGGED,
+   ALC256_FIXUP_INTEL_NUC10,
ALC255_FIXUP_XIAOMI_HEADSET_MIC,
ALC274_FIXUP_HP_MIC,
ALC274_FIXUP_HP_HEADSET_MIC,
@@ -7744,6 +7745,15 @@ static const struct hda_fixup alc269_fixups[] = {
.chained = true,
.chain_id = ALC269_FIXUP_HEADSET_MODE
},
+   [ALC256_FIXUP_INTEL_NUC10] = {
+   .type = HDA_FIXUP_PINS,
+   .v.pins = (const struct hda_pintbl[]) {
+   { 0x19, 0x01a1913c }, /* use as headset mic, without 
its own jack detect */
+   { }
+   },
+   .chained = true,
+   .chain_id = ALC269_FIXUP_HEADSET_MODE
+   },
[ALC255_FIXUP_XIAOMI_HEADSET_MIC] = {
.type = HDA_FIXUP_VERBS,
.v.verbs = (const struct hda_verb[]) {
@@ -8183,6 +8193,7 @@ static const struct snd_pci_quirk alc269_fixup_tbl[] = {
SND_PCI_QUIRK(0x1c06, 0x2013, "Lemote A1802", 
ALC269_FIXUP_LEMOTE_A1802),
SND_PCI_QUIRK(0x1c06, 0x2015, "Lemote A190X", 
ALC269_FIXUP_LEMOTE_A190X),
SND_PCI_QUIRK(0x8086, 0x2080, "Intel NUC 8 Rugged", 
ALC256_FIXUP_INTEL_NUC8_RUGGED),
+   SND_PCI_QUIRK(0x8086, 0x2081, "Intel NUC 10", ALC256_FIXUP_INTEL_NUC10),
 
 #if 0
/* Below is a quirk table taken from the old code.
-- 
2.25.1

Re: [PATCH] ALSA: hda/realtek: Add quirk for Clevo NH55RZQ

[Werner Sembach]

> Thanks, now I could apply it.
Missed your reply: I have now sent it a third time using git send-email.
One change: I added Cc: stable this time.
> Could you resubmit the NUC10 patch as well? Takashi 
Done.
Hope everything is correct now.

[PATCH] ALSA: hda/realtek: Add quirk for Intel NUC 10

[Werner Sembach]

ALSA: hda/realtek: Add quirk for Intel NUC 10

This adds a new SND_PCI_QUIRK(...) and applies it to the Intel NUC 10
devices. This fixes the issue of the devices not having audio input and
output on the headset jack because the kernel does not recognize when
something is plugged in.

The new quirk was inspired by the quirk for the Intel NUC 8 devices, but
it turned out that the NUC 10 uses another pin. This information was
acquired by black box testing likely pins.

Co-developed-by: Eckhart Mohr 
Signed-off-by: Eckhart Mohr 
Signed-off-by: Werner Sembach 
Cc: 
---
Resend of this patch with git send-email, because last patch got tabs replaced 
with spaces.

>From d281364b8ca6c054a0e5ce20caa599bf7d08160d Mon Sep 17 00:00:00 2001
From: Werner Sembach 
Date: Fri, 26 Feb 2021 13:54:30 +0100
Subject: [PATCH] Fix Intel NUC10 no output and input on headset jack

---
 sound/pci/hda/patch_realtek.c | 11 +++
 1 file changed, 11 insertions(+)

diff --git a/sound/pci/hda/patch_realtek.c b/sound/pci/hda/patch_realtek.c
index 290645516313..c14d624dbaf1 100644
--- a/sound/pci/hda/patch_realtek.c
+++ b/sound/pci/hda/patch_realtek.c
@@ -6362,6 +6362,7 @@ enum {
ALC269_FIXUP_LEMOTE_A1802,
ALC269_FIXUP_LEMOTE_A190X,
ALC256_FIXUP_INTEL_NUC8_RUGGED,
+   ALC256_FIXUP_INTEL_NUC10,
ALC255_FIXUP_XIAOMI_HEADSET_MIC,
ALC274_FIXUP_HP_MIC,
ALC274_FIXUP_HP_HEADSET_MIC,
@@ -7744,6 +7745,15 @@ static const struct hda_fixup alc269_fixups[] = {
.chained = true,
.chain_id = ALC269_FIXUP_HEADSET_MODE
},
+   [ALC256_FIXUP_INTEL_NUC10] = {
+   .type = HDA_FIXUP_PINS,
+   .v.pins = (const struct hda_pintbl[]) {
+   { 0x19, 0x01a1913c }, /* use as headset mic, without 
its own jack detect */
+   { }
+   },
+   .chained = true,
+   .chain_id = ALC269_FIXUP_HEADSET_MODE
+   },
[ALC255_FIXUP_XIAOMI_HEADSET_MIC] = {
.type = HDA_FIXUP_VERBS,
.v.verbs = (const struct hda_verb[]) {
@@ -8183,6 +8193,7 @@ static const struct snd_pci_quirk alc269_fixup_tbl[] = {
SND_PCI_QUIRK(0x1c06, 0x2013, "Lemote A1802", 
ALC269_FIXUP_LEMOTE_A1802),
SND_PCI_QUIRK(0x1c06, 0x2015, "Lemote A190X", 
ALC269_FIXUP_LEMOTE_A190X),
SND_PCI_QUIRK(0x8086, 0x2080, "Intel NUC 8 Rugged", 
ALC256_FIXUP_INTEL_NUC8_RUGGED),
+   SND_PCI_QUIRK(0x8086, 0x2081, "Intel NUC 10", ALC256_FIXUP_INTEL_NUC10),
 
 #if 0
/* Below is a quirk table taken from the old code.
-- 
2.25.1

[PATCH] ALSA: hda/realtek: Add quirk for Clevo NH55RZQ

[Werner Sembach]

From: Eckhart Mohr 

ALSA: hda/realtek: Add quirk for Clevo NH55RZQ

This applies a SND_PCI_QUIRK(...) to the Clevo NH55RZQ barebone. This
fixes the issue of the device not recognizing a pluged in microphone.

The device has both, a microphone only jack, and a speaker + microphone
combo jack. The combo jack already works. The microphone-only jack does
not recognize when a device is pluged in without this patch.

Signed-off-by: Eckhart Mohr 
Co-developed-by: Werner Sembach 
Signed-off-by: Werner Sembach 
Cc: 
---
Third time's the charm, now using git send-email, I'm really sorry for the spam.

>From 2835edd753fd19c72a644dccb7e941cfc0ecdf8e Mon Sep 17 00:00:00 2001
From: Werner Sembach 
Date: Fri, 26 Feb 2021 13:50:15 +0100
Subject: [PATCH] Fix device detection on microphone jack of Clevo NH55RZQ

---
 sound/pci/hda/patch_realtek.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/sound/pci/hda/patch_realtek.c b/sound/pci/hda/patch_realtek.c
index 290645516313..8014e80d72c3 100644
--- a/sound/pci/hda/patch_realtek.c
+++ b/sound/pci/hda/patch_realtek.c
@@ -8089,6 +8089,7 @@ static const struct snd_pci_quirk alc269_fixup_tbl[] = {
SND_PCI_QUIRK(0x1558, 0x8551, "System76 Gazelle (gaze14)", 
ALC293_FIXUP_SYSTEM76_MIC_NO_PRESENCE),
SND_PCI_QUIRK(0x1558, 0x8560, "System76 Gazelle (gaze14)", 
ALC269_FIXUP_HEADSET_MIC),
SND_PCI_QUIRK(0x1558, 0x8561, "System76 Gazelle (gaze14)", 
ALC269_FIXUP_HEADSET_MIC),
+   SND_PCI_QUIRK(0x1558, 0x8562, "Clevo NH[5|7][0-9]RZ[Q]", 
ALC269_FIXUP_DMIC),
SND_PCI_QUIRK(0x1558, 0x8668, "Clevo NP50B[BE]", 
ALC293_FIXUP_SYSTEM76_MIC_NO_PRESENCE),
SND_PCI_QUIRK(0x1558, 0x8680, "Clevo NJ50LU", 
ALC293_FIXUP_SYSTEM76_MIC_NO_PRESENCE),
SND_PCI_QUIRK(0x1558, 0x8686, "Clevo NH50[CZ]U", 
ALC293_FIXUP_SYSTEM76_MIC_NO_PRESENCE),
-- 
2.25.1

[PATCH] ALSA: hda/realtek: Add quirk for Clevo NH55RZQ

[Werner Sembach]

From: Eckhart Mohr 

ALSA: hda/realtek: Add quirk for Clevo NH55RZQ

This applies a SND_PCI_QUIRK(...) to the Clevo NH55RZQ barebone. This
fixes the issue of the device not recognizing a pluged in microphone.

The device has both, a microphone only jack, and a speaker + microphone
combo jack. The combo jack already works. The microphone-only jack does
not recognize when a device is pluged in without this patch.

Signed-off-by: Eckhart Mohr 
Co-developed-by: Werner Sembach 
Signed-off-by: Werner Sembach 
---
This is a resend of the patch because I missed that the editor I used to write 
the commit message replaced tabs with spaces.

>From 2835edd753fd19c72a644dccb7e941cfc0ecdf8e Mon Sep 17 00:00:00 2001
From: Werner Sembach 
Date: Fri, 26 Feb 2021 13:50:15 +0100
Subject: [PATCH] Fix device detection on microphone jack of Clevo NH55RZQ

---
 sound/pci/hda/patch_realtek.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/sound/pci/hda/patch_realtek.c b/sound/pci/hda/patch_realtek.c
index 290645516313..8014e80d72c3 100644
--- a/sound/pci/hda/patch_realtek.c
+++ b/sound/pci/hda/patch_realtek.c
@@ -8089,6 +8089,7 @@ static const struct snd_pci_quirk alc269_fixup_tbl[] = {
SND_PCI_QUIRK(0x1558, 0x8551, "System76 Gazelle (gaze14)", 
ALC293_FIXUP_SYSTEM76_MIC_NO_PRESENCE),
SND_PCI_QUIRK(0x1558, 0x8560, "System76 Gazelle (gaze14)", 
ALC269_FIXUP_HEADSET_MIC),
SND_PCI_QUIRK(0x1558, 0x8561, "System76 Gazelle (gaze14)", 
ALC269_FIXUP_HEADSET_MIC),
+   SND_PCI_QUIRK(0x1558, 0x8562, "Clevo NH[5|7][0-9]RZ[Q]", 
ALC269_FIXUP_DMIC),
SND_PCI_QUIRK(0x1558, 0x8668, "Clevo NP50B[BE]", 
ALC293_FIXUP_SYSTEM76_MIC_NO_PRESENCE),
SND_PCI_QUIRK(0x1558, 0x8680, "Clevo NJ50LU", 
ALC293_FIXUP_SYSTEM76_MIC_NO_PRESENCE),
SND_PCI_QUIRK(0x1558, 0x8686, "Clevo NH50[CZ]U", 
ALC293_FIXUP_SYSTEM76_MIC_NO_PRESENCE),
-- 
2.25.1

[PATCH] ALSA: hda/realtek: Add quirk for Clevo NH55RZQ

[Werner Sembach]

From: Eckhart Mohr 

ALSA: hda/realtek: Add quirk for Clevo NH55RZQ

This applies a SND_PCI_QUIRK(...) to the Clevo NH55RZQ barebone. This
fixes the issue of the device not recognizing a pluged in microphone.

The device has both, a microphone only jack, and a speaker + microphone
combo jack. The combo jack already works. The microphone-only jack does
not recognize when a device is pluged in without this patch.

Signed-off-by: Eckhart Mohr 
Co-developed-by: Werner Sembach 
Signed-off-by: Werner Sembach 
---
Hi,
this is my first ever submitted kernel patch, feel free to criticise me if I 
made an error or missed a best practise bullet point.
Also: I'm unsure if this would fit the requirements for sta...@vger.kernel.org 
and/or triv...@kernel.org, but I think not (correct me if I'm wrong).
Kind regards
Werner Sembach

>From 2835edd753fd19c72a644dccb7e941cfc0ecdf8e Mon Sep 17 00:00:00 2001
From: Werner Sembach 
Date: Fri, 26 Feb 2021 13:50:15 +0100
Subject: [PATCH] Fix device detection on microphone jack of Clevo NH55RZQ

---
 sound/pci/hda/patch_realtek.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/sound/pci/hda/patch_realtek.c b/sound/pci/hda/patch_realtek.c
index 290645516313..8014e80d72c3 100644
--- a/sound/pci/hda/patch_realtek.c
+++ b/sound/pci/hda/patch_realtek.c
@@ -8089,6 +8089,7 @@ static const struct snd_pci_quirk alc269_fixup_tbl[] = {
 SND_PCI_QUIRK(0x1558, 0x8551, "System76 Gazelle (gaze14)", 
ALC293_FIXUP_SYSTEM76_MIC_NO_PRESENCE),
 SND_PCI_QUIRK(0x1558, 0x8560, "System76 Gazelle (gaze14)", 
ALC269_FIXUP_HEADSET_MIC),
 SND_PCI_QUIRK(0x1558, 0x8561, "System76 Gazelle (gaze14)", 
ALC269_FIXUP_HEADSET_MIC),
+    SND_PCI_QUIRK(0x1558, 0x8562, "Clevo NH[5|7][0-9]RZ[Q]", 
ALC269_FIXUP_DMIC),
 SND_PCI_QUIRK(0x1558, 0x8668, "Clevo NP50B[BE]", 
ALC293_FIXUP_SYSTEM76_MIC_NO_PRESENCE),
 SND_PCI_QUIRK(0x1558, 0x8680, "Clevo NJ50LU", 
ALC293_FIXUP_SYSTEM76_MIC_NO_PRESENCE),
 SND_PCI_QUIRK(0x1558, 0x8686, "Clevo NH50[CZ]U", 
ALC293_FIXUP_SYSTEM76_MIC_NO_PRESENCE),
-- 
2.25.1

[PATCH] ALSA: hda/realtek: Add quirk for Intel NUC 10

[Werner Sembach]

From: Werner Sembach 

ALSA: hda/realtek: Add quirk for Intel NUC 10

This adds a new SND_PCI_QUIRK(...) and applies it to the Intel NUC 10
devices. This fixes the issue of the devices not having audio input and
output on the headset jack because the kernel does not recognize when
something is plugged in.

The new quirk was inspired by the quirk for the Intel NUC 8 devices, but
it turned out that the NUC 10 uses another pin. This information was
acquired by black box testing likely pins.

Co-developed-by: Eckhart Mohr 
Signed-off-by: Eckhart Mohr 
Signed-off-by: Werner Sembach 
---
Hi,
this is my second ever submitted kernel patch with the first one send just some 
minutes ago, feel free to criticise me if I made an error or missed a best 
practice bullet point.
Also: I'm unsure if this would fit the requirements for sta...@vger.kernel.org 
and/or triv...@kernel.org, but I think not (correct me if I'm wrong).
Kind regards
Werner Sembach

>From d281364b8ca6c054a0e5ce20caa599bf7d08160d Mon Sep 17 00:00:00 2001
From: Werner Sembach 
Date: Fri, 26 Feb 2021 13:54:30 +0100
Subject: [PATCH] Fix Intel NUC10 no output and input on headset jack

---
 sound/pci/hda/patch_realtek.c | 11 +++
 1 file changed, 11 insertions(+)

diff --git a/sound/pci/hda/patch_realtek.c b/sound/pci/hda/patch_realtek.c
index 290645516313..c14d624dbaf1 100644
--- a/sound/pci/hda/patch_realtek.c
+++ b/sound/pci/hda/patch_realtek.c
@@ -6362,6 +6362,7 @@ enum {
 ALC269_FIXUP_LEMOTE_A1802,
 ALC269_FIXUP_LEMOTE_A190X,
 ALC256_FIXUP_INTEL_NUC8_RUGGED,
+    ALC256_FIXUP_INTEL_NUC10,
 ALC255_FIXUP_XIAOMI_HEADSET_MIC,
 ALC274_FIXUP_HP_MIC,
 ALC274_FIXUP_HP_HEADSET_MIC,
@@ -7744,6 +7745,15 @@ static const struct hda_fixup alc269_fixups[] = {
     .chained = true,
     .chain_id = ALC269_FIXUP_HEADSET_MODE
 },
+    [ALC256_FIXUP_INTEL_NUC10] = {
+        .type = HDA_FIXUP_PINS,
+        .v.pins = (const struct hda_pintbl[]) {
+            { 0x19, 0x01a1913c }, /* use as headset mic, without its own jack 
detect */
+            { }
+        },
+        .chained = true,
+        .chain_id = ALC269_FIXUP_HEADSET_MODE
+    },
 [ALC255_FIXUP_XIAOMI_HEADSET_MIC] = {
     .type = HDA_FIXUP_VERBS,
     .v.verbs = (const struct hda_verb[]) {
@@ -8183,6 +8193,7 @@ static const struct snd_pci_quirk alc269_fixup_tbl[] = {
 SND_PCI_QUIRK(0x1c06, 0x2013, "Lemote A1802", ALC269_FIXUP_LEMOTE_A1802),
 SND_PCI_QUIRK(0x1c06, 0x2015, "Lemote A190X", ALC269_FIXUP_LEMOTE_A190X),
 SND_PCI_QUIRK(0x8086, 0x2080, "Intel NUC 8 Rugged", 
ALC256_FIXUP_INTEL_NUC8_RUGGED),
+    SND_PCI_QUIRK(0x8086, 0x2081, "Intel NUC 10", ALC256_FIXUP_INTEL_NUC10),
 
 #if 0
 /* Below is a quirk table taken from the old code.
-- 
2.25.1

Re: [SPECIFICATION RFC] The firmware and bootloader log specification

[Julius Werner]

Standardizing in-memory logging sounds like an interesting idea,
especially with regards to components that can run on top of different
firmware stacks (things like GRUB or TF-A). But I would be a bit wary
of creating a "new standard to rule them all" and then expecting all
projects to switch what they have over to that. I think we all know
https://xkcd.com/927/.

Have you looked around and evaluated existing solutions that already
have some proliferation first? I think it would be much easier to
convince people to standardize on something that already has existing
users and drivers available in multiple projects.

In coreboot we're using a very simple character ring buffer that only
has two 4-byte header fields: total size and cursor (i.e. current
position where you would write the next character). The top 4 bits of
the cursor field are reserved for flags, one of which is the
"overflow" flag that tells you whether the ring-buffer already
overflowed or not (so readers know whether to print the whole ring
buffer, or only from the start to the current cursor). We try to
dimension the buffers so they don't overflow on a single boot, but
this approach allows us to log multiple boots on platforms that can
persist memory across reboots, which sometimes comes in handy.

The disadvantages of that approach compared to your proposal are lack
of some features, like the facilities field (although one can still
just print a tag like "<0>" or "<4>" behind each newline) or
timestamps (coreboot instead has separate timestamp logging). But I
think a really big advantage is size: in early firmware environments
before DDR training, space is often very precious and we struggle to
find more than a couple of kilobytes for the log buffer. If I look at
the structure you proposed, that's already 24 bytes of overhead per
individual message. If we were hooking that up to our normal printk()
facility in coreboot (such that each invocation creates a new message
header), that would probably waste about a third of the whole log
buffer on overhead. I think a complicated, syslog-style logging
structure that stores individual message blocks instead of a
continuous character string isn't really suitable for firmware
logging.

FWIW the coreboot console has existing support in Linux
(https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/firmware/google/memconsole-coreboot.c),
SeaBIOS 
(https://github.com/coreboot/seabios/blob/master/src/fw/coreboot.c#L219),
TF-A 
(https://github.com/ARM-software/arm-trusted-firmware/blob/master/drivers/coreboot/cbmem_console/aarch64/cbmem_console.S),
GRUB 
(https://git.savannah.gnu.org/cgit/grub.git/tree/grub-core/term/i386/coreboot/cbmemc.c),
U-Boot 
(https://github.com/u-boot/u-boot/blob/master/drivers/misc/cbmem_console.c)
and probably a couple of others I'm not aware of. And the code to add
support (especially when only appending) is so simple that it just
takes a couple of lines to implement (binary code size to implement
the format is also always a concern for firmware environments).

On Wed, Nov 18, 2020 at 7:04 AM Heinrich Schuchardt  wrote:
>
> On 14.11.20 00:52, Daniel Kiper wrote:
> > Hey,
> >
> > This is next attempt to create firmware and bootloader log specification.
> > Due to high interest among industry it is an extension to the initial
> > bootloader log only specification. It takes into the account most of the
> > comments which I got up until now.
> >
> > The goal is to pass all logs produced by various boot components to the
> > running OS. The OS kernel should expose these logs to the user space
> > and/or process them internally if needed. The content of these logs
> > should be human readable. However, they should also contain the
> > information which allows admins to do e.g. boot time analysis.
> >
> > The log specification should be as much as possible platform agnostic
> > and self contained. The final version of this spec should be merged into
> > existing specifications, e.g. UEFI, ACPI, Multiboot2, or be a standalone
> > spec, e.g. as a part of OASIS Standards. The former seems better but is
> > not perfect too...
> >
> > Here is the description (pseudocode) of the structures which will be
> > used to store the log data.
>
> Hello Daniel,
>
> thanks for your suggestion which makes good sense to me.
>
> Why can't we simply use the message format defined in "The Syslog
> Protocol", https://tools.ietf.org/html/rfc5424?
>
> >
> >   struct bf_log
> >   {
> > uint32_t   version;
> > char   producer[64];
> > uint64_t   flags;
> > uint64_t   next_bf_log_addr;
> > uint32_t   next_msg_off;
> > bf_log_msg msgs[];
>
> As bf_log_msg is does not have defined length msgs[] cannot be an array.
>
> >   }
> >
> >   struct bf_log_msg
> >   {
> > uint32_t size;
> > uint64_t ts_nsec;
> > uint32_t level;
> > uint32_t facility;
> > uint32_t msg_off;
> > char strings[];
> >   }
> >
> > The members of struct bf_log:
> >   

AW: [PATCH v2 2/3] rtc: rx6110: add ACPI bindings to I2C

[werner....@siemens.com]

Hi Henning.

We had a short discussion with Andy on coreboot Gerrit 
(https://review.coreboot.org/c/coreboot/+/47235) regarding this issue.
We have agreed that we will give Epson a period of two weeks for reaction. If 
we will not have a valid HID by then, I will push a  patch which will use a 
non-valid HID instead (something like  as proposed by Andy).
I will clarify in the meantime when the next coreboot release will happen and 
prevent this wrong ID from getting part of the release.

Werner


-Ursprüngliche Nachricht-
Von: Henning Schild  
Gesendet: Mittwoch, 18. November 2020 11:04
An: Andy Shevchenko 
Cc: Hahn, Johannes (DI FA CTR PLC PRC3) ; Rafael J. 
Wysocki ; ACPI Devel Maling List 
; Brown, Len ; Simon Glass 
; Simon Glass ; val.kru...@erd.epson.com; 
Claudius Heine ; Alessandro Zummo ; 
Alexandre Belloni ; linux-...@vger.kernel.org; 
linux-kernel@vger.kernel.org; Zeh, Werner (DI MC MTS R HW 1) 
; Andy Shevchenko ; 
Mantel, Martin (DI FA CTR PLC PO) 
Betreff: Re: [PATCH v2 2/3] rtc: rx6110: add ACPI bindings to I2C

We are trying to reach out to Epson to find valid IDs, but will do that 
off-list for now.

In the meantime we will propose just the I2C patches, without ACPI.

We have Werner Zeh here, who pushed the coreboot change. In coreboot nobody 
objected, maybe they have not been aware of the processes or it was missed in 
their review.

Werner i think the coreboot change should be reverted so it does not enter a 
release and will become legacy that needs to be supported. Let us revisit it 
once we have the proper IDs. If that fails we have to look into the other 
options that Andy proposed. There is no point having the coreboot support 
without the Linux-user ... 

regards,
Henning

Am Tue, 17 Nov 2020 13:41:08 +0200
schrieb Andy Shevchenko :

> +Cc: Simon
> 
> Simon, this is an issue with ACPI IDs and I think JFYI would be an 
> interesting topic since this may happen in the future in U-Boot or 
> other projects. Also, you may know people from coreboot to figure out 
> what to do with this case and how to prevent something similar from 
> happening in the future.
> 
> On Tue, Nov 17, 2020 at 1:33 PM Andy Shevchenko 
>  wrote:
> >
> > On Tue, Nov 17, 2020 at 11:51 AM johannes-h...@siemens.com 
> >  wrote:
> > >
> > > Hello Val,
> > >
> > > my name is Johannes Hahn from Siemens AG in Germany.
> > > Our product Open Controller II (OCII)[1] uses the Realtime Clock 
> > > RX6110SA from SEIKO EPSON.
> >
> > Nice to hear from you!
> >  
> > > Currently there is a merge request ongoing for the Linux Kernel 
> > > master branch[2] which adds I²C and ACPI support to your original 
> > > driver implementation.
> > >
> > > Simultaneously there is an already merged patch-set for 
> > > coreboot[3] available creating the ACPI (SSDT) table entries for 
> > > the RX6110SA.
> >
> > Thanks for pointers, I commented there. The ACPI ID change must be 
> > reverted!
> > > The OCII uses coreboot for firmware initialization.
> > >
> > > During the merge request the eligible objection arose that the 
> > > ACPI ID used in the Linux driver patch is not conforming the ACPI 
> > > Specification. Indeed it does not. But when searching for a 
> > > product identifier of RX6110SA I was not able to find a sufficient 
> > > one with respect to the ACPI Specification (see [4] chapter 6.1.5 
> > > _HID (Hardware ID),[5]).
> >
> > Unfortunately many vendors, even being registered in the ACPI/PNP 
> > registry, are still neglecting the process.
> >  
> > > According to the fact that there are other Linux RTC drivers on 
> > > the Kernel mainline[6] which support ACPI matching that also do 
> > > not have ACPI Specification compatible IDs we used that as an 
> > > example for our first patch attempt.
> >
> > I answered this in previous mail.
> >  
> > > A PNP ID for SEIKO EPSON is already registered at UEFI 
> > > database[7].
> > >
> > > What I kindly ask your for is an ACPI Specification conforming 
> > > Product Identifier for the RX6110SA RTC ? According to [5] this 
> > > Product Identifier should be "... always four-character 
> > > hexadecimal numbers (0-9 and A-F)".
> > >
> > > In case you do not know it our can not acquire/create one could 
> > > you please redirect me to someone from SEIKO EPSON who can help me 
> > > with that demand ?
> >
> > So, to be on the constructive page (I thought initially you are from 
> > G company, but anyway) you may do the following:
> >
> > - (for prototyping only) you may use the P

Re: [PATCH v4 1/2] firmware: google: Expose CBMEM over sysfs

[Julius Werner]

> Ok. Regardless of the concern of the physical address is there any usage
> of this attribute by userspace? The description makes it sound like it's
> a pure debug feature, which implies that it should be in debugfs and not
> in sysfs.

I'll leave that up to Patrick. I doubt we'd want to create a whole
separate debugfs hierarchy just for this. Like I said you can just
read it out of the log too, this would just make it a little bit more
convenient. It's not like it would be the only informational attribute
in sysfs...

Re: [PATCH v4 1/2] firmware: google: Expose CBMEM over sysfs

[Julius Werner]

> > +What:  /sys/bus/coreboot/devices/.../cbmem_attributes/address
> > +Date:  Apr 2020
> > +KernelVersion: 5.6
> > +Contact:   Patrick Rudolph 
> > +Description:
> > +   coreboot device directory can contain a file named
> > +   cbmem_attributes/address if the device corresponds to a 
> > CBMEM
> > +   buffer.
> > +   The file holds an ASCII representation of the physical 
> > address
> > +   of the CBMEM buffer in hex (e.g. 0x8000d000) and 
> > should
> > +   be used for debugging only.
>
> If this is for debugging purposes only perhaps it should go into
> debugfs. We try to not leak information about physical addresses to
> userspace and this would let an attacker understand where memory may be.
> That's not ideal and should be avoided.

This is memory allocated by firmware and not subject to (k)ASLR, so
nothing valuable can be leaked here. The same addresses could already
be parsed out of /sys/firmware/log. Before this interface we usually
accessed this stuff via /dev/mem (and tools that want to remain
backwards-compatible will probably want to keep doing that), so having
a quick shorthand to grab physical addresses can be convenient.

Re: [PATCH] tracing/boottime: Fix kprobe multiple events

[Maximilian Werner]

We are a group of students from Leibniz University Hannover and
this patch is part of a project of ours. That's why both of us
signed this off.

Should we have added Masami to Cc? He didn't appear in the
get_maintainer script.

-- Maximilian & Sascha

On 17.06.20 17:06, Steven Rostedt wrote:

On Wed, 17 Jun 2020 11:05:21 -0400
Steven Rostedt  wrote:


On Wed, 17 Jun 2020 16:08:17 +0200
Sascha Ortmann  wrote:


Fix boottime kprobe events to add multiple events even if one fails
and report probe generation failures.

As an example, when we try to set multiprobe kprobe events in
bootconfig like this:

ftrace.event.kprobes.vfsevents {
probes = "vfs_read $arg1 $arg2,,
  !error! not reported;?", // leads to error
 "vfs_write $arg1 $arg2"
}

this will not work like expected. After commit
da0f1f4167e3af69e1d8b32d6d65195ddd2bfb64 ("tracing/boottime:
Fix kprobe event API usage"), the function
trace_boot_add_kprobe_event will not produce any error message,
aborting the function and stopping subsequent probes from getting
installed when adding a probe fails at kprobe_event_gen_cmd_start.
Furthermore, probes continue when kprobe_event_gen_cmd_end fails
(and kprobe_event_gen_cmd_start did not fail). In this case the
function even returns successfully when the last call to
kprobe_event_gen_cmd_end is successful.

The behaviour of reporting and aborting after failures is not
consistent.

The function trace_boot_add_kprobe_event now continues even when
one of the multiple events fails. Each failure is now reported
individually. Since the function can only return one result to the
caller, the function returns now the last failure (or none, if
nothing fails).

Cc: linux-ker...@i4.cs.fau.de
Signed-off-by: Maximilian Werner 
Signed-off-by: Sascha Ortmann 


Why the double signed off by?

Masami, I'm fine with this, but needs your review.


[ It appears that Masami wasn't in the Cc ]




-- Steve


---
  kernel/trace/trace_boot.c | 16 +++-
  1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/kernel/trace/trace_boot.c b/kernel/trace/trace_boot.c
index 9de29bb45a27..dbb50184e060 100644
--- a/kernel/trace/trace_boot.c
+++ b/kernel/trace/trace_boot.c
@@ -95,18 +95,24 @@ trace_boot_add_kprobe_event(struct xbc_node *node, const 
char *event)
struct xbc_node *anode;
char buf[MAX_BUF_LEN];
const char *val;
+   int error = 0;
int ret = 0;
  
  	xbc_node_for_each_array_value(node, "probes", anode, val) {

kprobe_event_cmd_init(, buf, MAX_BUF_LEN);
  
-		ret = kprobe_event_gen_cmd_start(, event, val);

-   if (ret)
-   break;
+   error = kprobe_event_gen_cmd_start(, event, val);
+   if (error) {
+   pr_err("Failed to generate probe: %s\n", buf);
+   ret = error;
+   continue;
+   }
  
-		ret = kprobe_event_gen_cmd_end();

-   if (ret)
+   error = kprobe_event_gen_cmd_end();
+   if (error) {
pr_err("Failed to add probe: %s\n", buf);
+   ret = error;
+   }
}
  
  	return ret;

[PATCH] dh key: Missing a blank line after declarations

[Frank Werner-Krippendorf]

This patch fixes a "WARNING: Missing a blank line after declarations" issue 
found by checkpatch.pl

Signed-off-by: Frank Werner-Krippendorf 
---
 security/keys/dh.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/security/keys/dh.c b/security/keys/dh.c
index c4c629bb1c03..5515f51e62db 100644
--- a/security/keys/dh.c
+++ b/security/keys/dh.c
@@ -161,6 +161,7 @@ static int kdf_ctr(struct kdf_sdesc *sdesc, const u8 *src, 
unsigned int slen,
if (zlen && h) {
u8 tmpbuffer[32];
size_t chunk = min_t(size_t, zlen, sizeof(tmpbuffer));
+
memset(tmpbuffer, 0, chunk);
 
do {
-- 
2.20.1

[PATCH] Do not assign in if condition wg_noise_handshake_consume_initiation()

[Frank Werner-Krippendorf]

Fixes an error condition reported by checkpatch.pl which caused by
assigning a variable in an if condition in
wg_noise_handshake_consume_initiation().

Signed-off-by: Frank Werner-Krippendorf 
---
 drivers/net/wireguard/noise.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/wireguard/noise.c b/drivers/net/wireguard/noise.c
index 626433690abb..9524a15a62a6 100644
--- a/drivers/net/wireguard/noise.c
+++ b/drivers/net/wireguard/noise.c
@@ -617,8 +617,9 @@ wg_noise_handshake_consume_initiation(struct 
message_handshake_initiation *src,
memcpy(handshake->hash, hash, NOISE_HASH_LEN);
memcpy(handshake->chaining_key, chaining_key, NOISE_HASH_LEN);
handshake->remote_index = src->sender_index;
+   initiation_consumption = ktime_get_coarse_boottime_ns();
if ((s64)(handshake->last_initiation_consumption -
-   (initiation_consumption = ktime_get_coarse_boottime_ns())) < 0)
+   initiation_consumption) < 0)
handshake->last_initiation_consumption = initiation_consumption;
handshake->state = HANDSHAKE_CONSUMED_INITIATION;
up_write(>lock);
-- 
2.20.1

Re: [PATCH] MAINTAINERS: rectify entry in ARM SMC WATCHDOG DRIVER

[Julius Werner]

Reviewed-by: Julius Werner 

Re: [PATCH v6 2/2] watchdog: Add new arm_smc_wdt watchdog driver

[Julius Werner]

Reviewed-by: Julius Werner 

Re: [PATCH v5 2/2] watchdog: Add new arm_smc_wdt watchdog driver

[Julius Werner]

> I think I have misunderstood the device tree json-schema spec.
> My intention was for the device tree to fill in a default value in the dtb for
> arm,smc-id if it was omitted in the dts. But now I see that does not seem to
> happen, I cannot really find any documentation of `default`, so I will just 
> put
> a documentation string in instead and force the default in the driver.

The bindings in Documentation/device-tree are just for informational
purposes, they do not have any direct effect on anything. If you want
there to be a default, you'll have to write the kernel code to fill it
in.

Re: [PATCH 2/2] firmware: coreboot: Export active CBFS partition

[Julius Werner]

> I don't know why we need to draw a line in the sand and say that if the
> kernel doesn't need to know about it then it shouldn't parse it. I want
> there to be a consistent userspace ABI that doesn't just move things
> straight from memory to userspace in some binary format. I'd rather we
> have an ABI that decodes and exposes information about the coreboot
> tables through existing frameworks/subsystems where possible and makes
> up new ones otherwise.

Okay... I'm just saying this might grow to become a lot of stuff as
people start having more and more use cases they want to support. But
if you think the kernel should be the one parsing all that, I'm happy
to defer to your expertise there (I'm not really a kernel guy after
all).

> One concern I have is endianness of the binary data. Is it big endian or
> little endian or CPU native endian? The kernel can boot into big or
> little endian on ARM platforms and userspace can be different vs. the
> bootloader too. Userspace shouldn't need to know this detail, the kernel
> should know and do the conversions and expose it somehow. That's why I'm
> suggesting in this case we describe fmap as a sysfs class. I don't see
> how we could export that information otherwise, besides in a binary blob
> that falls into traps like this.

Right now it's just always CPU byte order of what coreboot happened to
run at, and it's not exporting that info in any way either. We don't
really have support for big-endian architectures anyway so it's not
something we really thought about yet. If it ever comes to that, I
assume the byte order of the table header's magic number could be used
to tell the difference.

> Right now we make devices for all the coreboot table entries, which is
> pretty weird considering that some table entries are things like
> LB_TAG_LINKER. That isn't a device. It's some information about how
> coreboot was linked. We should probably blacklist tags so we don't make
> devices and capture these ones in the bus code and expose them in
> /sys/firware/coreboot/ somehow. We should also add device randomness
> from the serial numbers, etc. that coreboot has stashed away in the
> tables.

I mean... should any of them be devices, then? All table entries are
just "some information", where are you defining the difference there?
I'm not sure if the current representation is the right one, but I
think they should probably all be handled in a consistent way.

Re: [PATCH 2/2] firmware: coreboot: Export active CBFS partition

[Julius Werner]

> > I'll expose the coreboot tables using a sysfs driver, which then can be
> > used by coreboot tools instead of accessing /dev/mem. As it holds the
> > FMAP and "boot media params" that's all I need for now.
> >
> > The downside is that the userspace tools need to be keep in sync with
> > the binary interface the coreboot tables are providing.

Well, in the other version the kernel needs to be kept in sync
instead. No matter where you do the parsing, something needs to be
kept in sync. I think userspace would be easier, especially if we
would host a small userspace library in the coreboot repository that
other tools could just link.

> I'd rather we export this information in sysfs via some coreboot_fmap
> class and then make the "boot media params" another property of one of
> the fmap devices. Then userspace can search through all the fmap devices
> and find the "boot media params" one. Is anything else needed?

Okay, this is the fundamental question we need to answer first... do
you really think it's better to add a separate interface for each of
these, Stephen? The coreboot table[1] currently contains 49 entries
with all sorts of random firmware information, and most of these will
never be interesting to the kernel. Do we want to establish a pattern
where we add a new sysfs interface for each of them whenever someone
has a use case for reading it from userspace? I think this might be a
good point to implement a generic interface to read any coreboot table
entry from userspace instead, and say that if the kernel doesn't need
the information in a specific entry itself, it shouldn't need to know
how to parse it.

[1] 
https://review.coreboot.org/cgit/coreboot.git/tree/src/commonlib/include/commonlib/coreboot_tables.h

Re: [PATCH 2/2] firmware: coreboot: Export active CBFS partition

[Julius Werner]

> Somehow we've gotten /sys/firmware/log to be the coreboot log, and quite
> frankly that blows my mind that this path was accepted upstream.
> Userspace has to know it's running on coreboot firmware to know that
> /sys/firmware/log is actually the coreboot log.

Not really sure I understand your concern here? That's the generic
node for the log from the mainboard firmware, whatever it is. It was
originally added for non-coreboot firmware and that use is still
supported. If some other non-coreboot firmware wants to join in, it's
welcome to do so -- the interface is separated out enough to make it
easy to add more backends.

I do agree that if we want to add other, more coreboot-specific nodes,
they should be explicitly namespaced.

> But I also wonder why this is being exposed by the kernel at all?

I share Stephen's concern that I'm not sure this belongs in the kernel
at all. There are existing ways for userspace to access this
information like the cbmem utility does... if you want it accessible
from fwupd, it could chain-call into cbmem or we could factor that
functionality out into a library. If you want to get away from using
/dev/mem for this, we could maybe add a driver that exports CBMEM or
coreboot table areas via sysfs... but then I think that should be a
generic driver which makes them all accessible in one go, rather than
having to add yet another driver whenever someone needs to parse
another coreboot table blob for some reason. We could design an
interface like /sys/firmware/coreboot/table/ where every entry in
the table gets exported as a binary file.

I think a specific sysfs driver only makes sense for things that are
human readable and that you'd actually expect a human to want to go
read directly, like the log. Maybe exporting FMAP entries one by one
like Stephen suggests could be such a case, but I doubt that there's a
common enough need for that since there are plenty of existing ways to
show FMAP entries from userspace (and if there was a generic interface
like /sys/firmware/coreboot/table/37 to access it, we could just add a
new flag to the dump_fmap utility to read it from there).

Re: [PATCH] usb: storage: Add ums-cros-aoa driver

[Julius Werner]

FWIW, I found a suitable workaround now to get my use case working
with existing kernels: I can do the mode switch from userspace, then
after the device reenumerates I can manually disable any interfaces I
don't like by writing 0 to their 'authorized' node, and then I write
the VID/PID to usb-storage/new_id.

I still think it would be nice in general to be able to force a driver
to bind a specific device (rather than a VID/PID match), but it's not
a pressing need for me anymore.

Re: [PATCH] usb: storage: Add ums-cros-aoa driver

[Julius Werner]

> USB drivers only bind to interfaces, are you saying that your device has
> multiple interfaces on it?

Yes, I have a case where the device has two interfaces which both have
interface class 0xff (although they do differ in subclass and
protocol). I only want the usb-storage driver to bind to one of them
(if it binds to the other it will eventually cause a timeout error
that resets the whole device).

I tried doing a userspace mode switch and using
/sys/bus/usb/drivers/usb-storage/new_id to bind the device now, which
*almost* works, but I can't prevent it from binding to both
interfaces. Unfortunately new_id can only take an interface class, not
a subclass or protocol.

> In fact, there already is a way to do this in the kernel: write to the
> sysfs "bind" file.  The difficulty is that you can't force a driver to
> bind to an interface if it doesn't believe it is compatible with the
> interface.  And if the driver believes it is compatible, it will
> automatically attempt to bind with all such interfaces regardless of
> their path.
>
> Perhaps what you need is a usb_device_id flag to indicate that the
> entry should never be used for automatic matches -- only for matches
> made by the user via the "bind" file.  Greg KH would probably be
> willing to accept a new USB_DEVICE_ID_MATCH_NO_AUTO flag, which
> could be included in your unusual_devs.h entries.

This is an interesting idea, but I don't quite see how it can work as
you described? When I write to 'bind', the driver core calls
driver_match_device(), which ends up calling usb_device_match()
(right?), which is the same path that it would call for automatic
matching. It still ends up in usb_match_one_id(), and if I check for
the NO_AUTO flag there it would abort just as if it was an auto-match
attempt. I see no way to pass the information that this is an
explicit, user-requested "bind" rather than an automatic match across
the bus->match() callback into the USB code. (I could change the
signature of the match() callback, but that would require changing
code for all device busses in Linux, which I assume is something we
wouldn't want to do? I could also add a flag to struct device to
communicate "this is currently trying to match for a user-initiated
bind", but that seems hacky and not really the right place to put
that.)

I could instead add a new sysfs node 'force_bind' to the driver core,
that would work like 'bind' except for skipping the
driver_match_device() call entirely and forcing a probe(). Do you
think that would be acceptable? Or is that too big of a hammer to make
available for all drivers in Linux? Maybe if I do the same thing but
only for usb drivers, or even only for the usb-storage driver
specifically, would that be acceptable?

If none of this works, I could also extend the new_id interface to
allow subclass/protocol matches instead. I don't like that as much
because it doesn't allow me to control the devpath of the device I'm
matching, but I think it would be enough for my use case (I can't make
the usb-storage driver bind all AOA devices at all times, but at the
times where I do want to use it for my one device, I don't expect any
other AOA devices to be connected). The problem with this is that the
order of arguments for new ID is already set in stone (vendor,
product, interface class, refVendor, refProduct), and I don't think I
can use the refVendor/refProduct for my case so I can't just append
more numbers behind that. I could maybe instead change it so that it
also accepts a key-value style line (like "idVendor=abcd
idProduct=efgh bInterfaceSubClass=ff"), while still being
backwards-compatible to the old format if you only give it numbers?
What do you think?

Thanks for your advice!

Re: [PATCH] usb: storage: Add ums-cros-aoa driver

[Julius Werner]

(Thanks for the reviews... I'll get back to the kernel code details
after double-checking if this can be done from userspace.)

> > Besides, what's wrong with binding to devices that weren't switched
> > into AOA mode?  Would that just provoke a bunch of unnecessary error
> > messages?

It's not about devices that aren't switched into AOA mode... it's
about devices that are switched into AOA mode for other reasons
(connecting to other Android apps). I don't think a kernel driver like
that exists today, but it could be added, or people could use libusb
to talk to an AOA device. AOA is just a general mechanism to talk to
an Android app for whatever you want, and the descriptors sent during
mode switch clarify what app it's talking to (and thereby what
protocol it is using... it could be mass storage or it could be
something entirely different). But a device switched into AOA mode for
whatever app will always use that same well-known VID/PID (18d1:2d00).
So if I just add that VID/PID to the IDs bound by the usb-storage
driver, it would also grab a device that was mode-switched by
userspace to talk to a completely different app. I need some way to
make sure it only grabs the intended device, and there's no good
identifier for that other than comparing the dev path to what you
originally mode switched.

> > > + /*
> > > +  * Only interface 0 connects to the AOA app. Android devices that 
> > > have
> > > +  * ADB enabled also export an interface 1. We don't want it.
> > > +  */
> > > + if (us->pusb_intf->cur_altsetting->desc.bInterfaceNumber != 0)
> > > + return -ENODEV;
> >
> > Do you really need this test?  What would go wrong if you don't do it?

Yes, otherwise two separate usb-storage instances bind to the two
interfaces. The second interface is meant for a special ADB debugging
protocol and will not respond at all to USB mass storage packets, so
eventually the first request to it times out and
usb_stor_invoke_transport() will do a port reset to recover. That also
kills the first interface asynchronously even though it was working
fine.

> > IMO the userspace approach would be better, unless you can provide a
> > really compelling argument for why it won't suffice.

Well... okay, let me think through that again. I just found the new_id
sysfs API that I wasn't aware of before, maybe I could leverage that
to bind this from userspace after doing the mode switch. But it looks
like that only operates on whole devices... is there any way to force
it to only bind one particular interface?

Re: [GIT PULL net-next, resend] isdn: deprecate non-mISDN drivers

[Werner Cornelius]

Am Freitag, 31. Mai 2019 schrieb Arnd Bergmann:

Just for information about the history and maybe state of parts of the old 
ISDN drivers.
There are still some large private ISDN networks available and will stay 
available for several years. I am still serving solutions there, but without 
i4l or capi.
I know one customer of mine that might use i4l for this purpose only. But 
maybe they switched over to a totally different solution, as  ISDN was the 
third level of fallback there.
I didn't receive any requests for service or migration from any outside party 
for years and so there has nothing been done on this drivers.

I am the author of the complete supplementary services (diversion...) the 
complete hysdn and parts of the Hisax  and i4l.

Hysdn was used by many shop companies and banks, but as the company producing 
the needed pci cards became bankrupt nearly 15 years ago and the company which 
bought the remaining rest does no longer exist too, I assume that there will 
be no or few users of this driver part only.
  
Its a bit sad that the software will disappear, but when there is no need any 
longer and of course support and test are difficult its understandable that it 
should phase out.

Best regards

Werner



> [resending, rebased on top of today's net-next]
> 
> The following changes since commit
>  7b3ed2a137b077bc0967352088b0adb6049eed20:
> 
>   Merge branch '100GbE' of
> git://git.kernel.org/pub/scm/linux/kernel/git/jkirsher/next-queue
> (2019-05-30 15:17:05 -0700)
> 
> are available in the Git repository at:
> 
>  https://git.kernel.org/pub/scm/linux/kernel/git/arnd/playground.git
> tags/isdn-removal
> 
> for you to fetch changes up to 6d97985072dc270032dc7a08631080bfd6253e82:
> 
>   isdn: move capi drivers to staging (2019-05-31 11:17:41 +0200)
> 
> 
> isdn: deprecate non-mISDN drivers
> 
> When isdn4linux came up in the context of another patch series, I
> remembered that we had discussed removing it a while ago.
> 
> It turns out that the suggestion from Karsten Keil wa to remove I4L
> in 2018 after the last public ISDN networks are shut down. This has
> happened now (with a very small number of exceptions), so I guess it's
> time to try again.
> 
> We currently have three ISDN stacks in the kernel: the original
> isdn4linux (with the hisax driver), the newer CAPI (with four drivers),
> and finally the mISDN stack (supporting roughly the same hardware as
> hisax).
> 
> As far as I can tell, anyone using ISDN with mainline kernel drivers in
> the past few years uses mISDN, and this is typically used for voice-only
> PBX installations that don't require a public network.
> 
> The older stacks support additional features for data networks, but those
> typically make no sense any more if there is no network to connect to.
> 
> My proposal for this time is to kill off isdn4linux entirely, as it seems
> to have been unusable for quite a while. This code has been abandoned
> for many years and it does cause problems for treewide maintenance as
> it tends to do everything that we try to stop doing.
> Birger Harzenetter mentioned that is is still using i4l in order to
> make use of the 'divert' feature that is not part of mISDN, but has
> otherwise moved on to mISDN for normal operation, like apparently
> everyone else.
> 
> CAPI in turn is not quite as obsolete, but two of the drivers (avm
> and hysdn) don't seem to be used at all, while another one (gigaset)
> will stop being maintained as Paul Bolle is no longer able to
> test it after the network gets shut down in September.
> All three are now moved into drivers/staging to let others speak
> up in case there are remaining users.
> This leaves Bluetooth CMTP as the only remaining user of CAPI, but
> Marcel Holtmann wishes to keep maintaining it.
> 
> For the discussion on version 1, see [2]
> Unfortunately, Karsten Keil as the maintainer has not participated in
> the discussion.
> 
>   Arnd
> 
> [1] https://patchwork.kernel.org/patch/8484861/#17900371
> [2]
>  https://listserv.isdn4linux.de/pipermail/isdn4linux/2019-April/thread.html
> 
> 
> 
> Arnd Bergmann (5):
>   isdn: gigaset: remove i4l support
>   isdn: remove hisax driver
>   isdn: remove isdn4linux
>   isdn: hdlc: move into mISDN
>   isdn: move capi drivers to staging
> 
>  Documentation/isdn/HiSax.cert  |   96 -
>  Documentation/isdn/INTERFACE   |  759 
>  Documentation/isdn/INTERFACE.fax   |  163 -
>  Documentation/isdn/README  |  599 
>  Documentation/isdn/README.FAQ  |   26 -
>

Re: [PATCH 0/5] Misc Google coreboot driver fixes/cleanups

[Julius Werner]

> Here's some minor fixes and cleanups for the Google coreboot drivers
> that I've had lying around in my tree for a little bit. They
> tighten up the code a bit and get rid of some boiler plate.
>
> Stephen Boyd (5):
>   firmware: google: Add a module_coreboot_driver() macro and use it
>   firmware: google: memconsole: Use devm_memremap()
>   firmware: google: memconsole: Drop __iomem on memremap memory
>   firmware: google: memconsole: Drop global func pointer
>   firmware: google: coreboot: Drop unnecessary headers

Thanks, these all look good to me.

Reviewed-by: Julius Werner 

Re: [PATCH v4 0/6] firmware: coreboot: Fix probe and simplify code

[Julius Werner]

Thanks for all the clean-up, looks great now!

For the whole series:
Reviewed-by: Julius Werner 

Re: [PATCH v4 0/6] firmware: coreboot: Fix probe and simplify code

[Julius Werner]

Thanks for all the clean-up, looks great now!

For the whole series:
Reviewed-by: Julius Werner 

Re: [PATCH v3 7/7] firmware: coreboot: Request table region for exclusive access

[Julius Werner]

Actually, looking at what IO_STRICT_DEVMEM really does, would it
really prevent userspace accesses to these areas? Because it seems
that it only prevents accesses to areas marked as IORESOURCE_BUSY, and
while I can't fully follow how the kernel assigns that, comments
suggest that this is only set when "Driver has marked this resource
busy".

So after you make the change to the other patch where we immediately
unmap the coreboot table again at the end of the probe() function,
shouldn't it become available to userspace again even with
IO_STRICT_DEVMEM set?
On Thu, Aug 9, 2018 at 4:37 PM Julius Werner  wrote:
>
> > Furthermore, I see that my system RAM excludes this coreboot table so it
> > doesn't fall into the bucket that CONFIG_STRICT_DEVMEM would find.
>
> Yes, that is intentional. We don't want the kernel to try to use that
> memory for anything else (since we want those tables to survive), so
> we mark them as reserved in the e820 map.
>
> > > (I guess an alternative would be to rewrite 'cbmem' to use
> > > /sys/bus/coreboot/devices if available to get its coreboot table
> > > information. But we'd still need to maintain the old path for
> > > backwards compatibility anyway, so that would really just make it more
> > > complicated.)
> >
> > This sounds like a good idea. Userspace reaching into /dev/mem is not
> > good from a kernel hardening perspective. That's why those strict devmem
> > configs exist. Can cbmem be updated to query information from device
> > drivers instead, so that we can enable CONFIG_IO_STRICT_DEVMEM as well?
>
> Well... problem is that cbmem doesn't just access the coreboot tables,
> it accesses more stuff. There is actually a larger memory region
> called CBMEM (that's what the utility is named after) which contains
> all sorts of random memory allocations that coreboot wanted to survive
> for the lifetime of the system. The coreboot table is one section in
> there, and it sort of serves as a directory for some of the others
> (although there's also just a general CBMEM directory... there's some
> redundancy there). But cbmem can also print some of the other CBMEM
> sections which it finds by querying the coreboot table, such as the
> firmware log or the boot timestamps.
>
> So the question is how we can get to that content if /dev/mem isn't
> available anymore. One option would be to just write separate kernel
> drivers to completely replace the cbmem utility (we already have one
> for the log, for example), but I think Linux generally doesn't want to
> have too much logic and parsing and stuff in kernel drivers. Another
> option is to add a driver that just exposes a sysfs file through which
> you could read (we don't need to write) the CBMEM area... but then
> we'd essentially want that to take absolute addresses because that's
> what the coreboot table pointers contain, so we would've just built
> /dev/mem by another name (for a restricted range).
>
> The nicest thing, really, would be if there was a way for a kernel
> driver to mark specific regions as "allowed" by /dev/mem. I don't
> suppose we'd be willing to introduce a mechanism like that?

Re: [PATCH v3 7/7] firmware: coreboot: Request table region for exclusive access

[Julius Werner]

Actually, looking at what IO_STRICT_DEVMEM really does, would it
really prevent userspace accesses to these areas? Because it seems
that it only prevents accesses to areas marked as IORESOURCE_BUSY, and
while I can't fully follow how the kernel assigns that, comments
suggest that this is only set when "Driver has marked this resource
busy".

So after you make the change to the other patch where we immediately
unmap the coreboot table again at the end of the probe() function,
shouldn't it become available to userspace again even with
IO_STRICT_DEVMEM set?
On Thu, Aug 9, 2018 at 4:37 PM Julius Werner  wrote:
>
> > Furthermore, I see that my system RAM excludes this coreboot table so it
> > doesn't fall into the bucket that CONFIG_STRICT_DEVMEM would find.
>
> Yes, that is intentional. We don't want the kernel to try to use that
> memory for anything else (since we want those tables to survive), so
> we mark them as reserved in the e820 map.
>
> > > (I guess an alternative would be to rewrite 'cbmem' to use
> > > /sys/bus/coreboot/devices if available to get its coreboot table
> > > information. But we'd still need to maintain the old path for
> > > backwards compatibility anyway, so that would really just make it more
> > > complicated.)
> >
> > This sounds like a good idea. Userspace reaching into /dev/mem is not
> > good from a kernel hardening perspective. That's why those strict devmem
> > configs exist. Can cbmem be updated to query information from device
> > drivers instead, so that we can enable CONFIG_IO_STRICT_DEVMEM as well?
>
> Well... problem is that cbmem doesn't just access the coreboot tables,
> it accesses more stuff. There is actually a larger memory region
> called CBMEM (that's what the utility is named after) which contains
> all sorts of random memory allocations that coreboot wanted to survive
> for the lifetime of the system. The coreboot table is one section in
> there, and it sort of serves as a directory for some of the others
> (although there's also just a general CBMEM directory... there's some
> redundancy there). But cbmem can also print some of the other CBMEM
> sections which it finds by querying the coreboot table, such as the
> firmware log or the boot timestamps.
>
> So the question is how we can get to that content if /dev/mem isn't
> available anymore. One option would be to just write separate kernel
> drivers to completely replace the cbmem utility (we already have one
> for the log, for example), but I think Linux generally doesn't want to
> have too much logic and parsing and stuff in kernel drivers. Another
> option is to add a driver that just exposes a sysfs file through which
> you could read (we don't need to write) the CBMEM area... but then
> we'd essentially want that to take absolute addresses because that's
> what the coreboot table pointers contain, so we would've just built
> /dev/mem by another name (for a restricted range).
>
> The nicest thing, really, would be if there was a way for a kernel
> driver to mark specific regions as "allowed" by /dev/mem. I don't
> suppose we'd be willing to introduce a mechanism like that?

Re: [PATCH v3 7/7] firmware: coreboot: Request table region for exclusive access

[Julius Werner]

> Furthermore, I see that my system RAM excludes this coreboot table so it
> doesn't fall into the bucket that CONFIG_STRICT_DEVMEM would find.

Yes, that is intentional. We don't want the kernel to try to use that
memory for anything else (since we want those tables to survive), so
we mark them as reserved in the e820 map.

> > (I guess an alternative would be to rewrite 'cbmem' to use
> > /sys/bus/coreboot/devices if available to get its coreboot table
> > information. But we'd still need to maintain the old path for
> > backwards compatibility anyway, so that would really just make it more
> > complicated.)
>
> This sounds like a good idea. Userspace reaching into /dev/mem is not
> good from a kernel hardening perspective. That's why those strict devmem
> configs exist. Can cbmem be updated to query information from device
> drivers instead, so that we can enable CONFIG_IO_STRICT_DEVMEM as well?

Well... problem is that cbmem doesn't just access the coreboot tables,
it accesses more stuff. There is actually a larger memory region
called CBMEM (that's what the utility is named after) which contains
all sorts of random memory allocations that coreboot wanted to survive
for the lifetime of the system. The coreboot table is one section in
there, and it sort of serves as a directory for some of the others
(although there's also just a general CBMEM directory... there's some
redundancy there). But cbmem can also print some of the other CBMEM
sections which it finds by querying the coreboot table, such as the
firmware log or the boot timestamps.

So the question is how we can get to that content if /dev/mem isn't
available anymore. One option would be to just write separate kernel
drivers to completely replace the cbmem utility (we already have one
for the log, for example), but I think Linux generally doesn't want to
have too much logic and parsing and stuff in kernel drivers. Another
option is to add a driver that just exposes a sysfs file through which
you could read (we don't need to write) the CBMEM area... but then
we'd essentially want that to take absolute addresses because that's
what the coreboot table pointers contain, so we would've just built
/dev/mem by another name (for a restricted range).

The nicest thing, really, would be if there was a way for a kernel
driver to mark specific regions as "allowed" by /dev/mem. I don't
suppose we'd be willing to introduce a mechanism like that?

Re: [PATCH v3 7/7] firmware: coreboot: Request table region for exclusive access

[Julius Werner]

> Furthermore, I see that my system RAM excludes this coreboot table so it
> doesn't fall into the bucket that CONFIG_STRICT_DEVMEM would find.

Yes, that is intentional. We don't want the kernel to try to use that
memory for anything else (since we want those tables to survive), so
we mark them as reserved in the e820 map.

> > (I guess an alternative would be to rewrite 'cbmem' to use
> > /sys/bus/coreboot/devices if available to get its coreboot table
> > information. But we'd still need to maintain the old path for
> > backwards compatibility anyway, so that would really just make it more
> > complicated.)
>
> This sounds like a good idea. Userspace reaching into /dev/mem is not
> good from a kernel hardening perspective. That's why those strict devmem
> configs exist. Can cbmem be updated to query information from device
> drivers instead, so that we can enable CONFIG_IO_STRICT_DEVMEM as well?

Well... problem is that cbmem doesn't just access the coreboot tables,
it accesses more stuff. There is actually a larger memory region
called CBMEM (that's what the utility is named after) which contains
all sorts of random memory allocations that coreboot wanted to survive
for the lifetime of the system. The coreboot table is one section in
there, and it sort of serves as a directory for some of the others
(although there's also just a general CBMEM directory... there's some
redundancy there). But cbmem can also print some of the other CBMEM
sections which it finds by querying the coreboot table, such as the
firmware log or the boot timestamps.

So the question is how we can get to that content if /dev/mem isn't
available anymore. One option would be to just write separate kernel
drivers to completely replace the cbmem utility (we already have one
for the log, for example), but I think Linux generally doesn't want to
have too much logic and parsing and stuff in kernel drivers. Another
option is to add a driver that just exposes a sysfs file through which
you could read (we don't need to write) the CBMEM area... but then
we'd essentially want that to take absolute addresses because that's
what the coreboot table pointers contain, so we would've just built
/dev/mem by another name (for a restricted range).

The nicest thing, really, would be if there was a way for a kernel
driver to mark specific regions as "allowed" by /dev/mem. I don't
suppose we'd be willing to introduce a mechanism like that?

Re: [PATCH v3 7/7] firmware: coreboot: Request table region for exclusive access

[Julius Werner]

On Thu, Aug 9, 2018 at 10:17 AM Stephen Boyd  wrote:
>
> Call request_mem_region() on the entire coreboot table to make sure
> other devices don't attempt to map the coreboot table in their drivers.
> If drivers need that support, it would be better to provide bus APIs
> they can use to do that through the mapping created in this file.
>

Does this prevent userspace from mapping this region via /dev/mem? If
so, let's please not do it to not break compatibility with existing
tools. (I guess an alternative would be to rewrite 'cbmem' to use
/sys/bus/coreboot/devices if available to get its coreboot table
information. But we'd still need to maintain the old path for
backwards compatibility anyway, so that would really just make it more
complicated.)

Re: [PATCH v3 7/7] firmware: coreboot: Request table region for exclusive access

[Julius Werner]

On Thu, Aug 9, 2018 at 10:17 AM Stephen Boyd  wrote:
>
> Call request_mem_region() on the entire coreboot table to make sure
> other devices don't attempt to map the coreboot table in their drivers.
> If drivers need that support, it would be better to provide bus APIs
> they can use to do that through the mapping created in this file.
>

Does this prevent userspace from mapping this region via /dev/mem? If
so, let's please not do it to not break compatibility with existing
tools. (I guess an alternative would be to rewrite 'cbmem' to use
/sys/bus/coreboot/devices if available to get its coreboot table
information. But we'd still need to maintain the old path for
backwards compatibility anyway, so that would really just make it more
complicated.)

Re: [PATCH v3 6/7] firmware: coreboot: Only populate devices in coreboot_table_init()

[Julius Werner]

On Thu, Aug 9, 2018 at 10:17 AM Stephen Boyd  wrote:
>
> This function checks the header for sanity, registers a bus, and
> populates devices for each coreboot table entry. Let's just populate
> devices here and pull the other bits up into the caller so that this
> function can be repurposed for pure device creation and registration. We
> can devm()ify the memory mapping at the same time to keep error paths
> simpler.
>
> Cc: Wei-Ning Huang 
> Cc: Julius Werner 
> Cc: Brian Norris 
> Cc: Samuel Holland 
> Suggested-by: Julius Werner 
> Signed-off-by: Stephen Boyd 
> ---
>  drivers/firmware/google/coreboot_table.c | 66 +++-
>  1 file changed, 29 insertions(+), 37 deletions(-)
>
> diff --git a/drivers/firmware/google/coreboot_table.c 
> b/drivers/firmware/google/coreboot_table.c
> index f343dbe86448..814913606d22 100644
> --- a/drivers/firmware/google/coreboot_table.c
> +++ b/drivers/firmware/google/coreboot_table.c
> @@ -32,8 +32,6 @@
>  #define CB_DEV(d) container_of(d, struct coreboot_device, dev)
>  #define CB_DRV(d) container_of(d, struct coreboot_driver, drv)
>
> -static struct coreboot_table_header *ptr_header;
> -
>  static int coreboot_bus_match(struct device *dev, struct device_driver *drv)
>  {
> struct coreboot_device *device = CB_DEV(dev);
> @@ -94,35 +92,21 @@ void coreboot_driver_unregister(struct coreboot_driver 
> *driver)
>  }
>  EXPORT_SYMBOL(coreboot_driver_unregister);
>
> -static int coreboot_table_init(struct device *dev, void *ptr)
> +static int coreboot_table_populate(struct device *dev, void *ptr)
>  {
> int i, ret;
> void *ptr_entry;
> struct coreboot_device *device;
> struct coreboot_table_entry *entry;
> -   struct coreboot_table_header *header;
> -
> -   ptr_header = ptr;
> -   header = ptr;
> -
> -   if (strncmp(header->signature, "LBIO", sizeof(header->signature))) {
> -   pr_warn("coreboot_table: coreboot table missing or 
> corrupt!\n");
> -   return -ENODEV;
> -   }
> -
> -   ret = bus_register(_bus_type);
> -   if (ret)
> -   return ret;
> +   struct coreboot_table_header *header = ptr;
>
> -   ptr_entry = ptr_header + header->header_bytes;
> +   ptr_entry = ptr + header->header_bytes;
> for (i = 0; i < header->table_entries; i++) {
> entry = ptr_entry;
>
> device = kzalloc(sizeof(struct device) + entry->size, 
> GFP_KERNEL);
> -   if (!device) {
> -   ret = -ENOMEM;
> -   break;
> -   }
> +   if (!device)
> +   return -ENOMEM;
>
> dev_set_name(>dev, "coreboot%d", i);
> device->dev.parent = dev;
> @@ -133,18 +117,13 @@ static int coreboot_table_init(struct device *dev, void 
> *ptr)
> ret = device_register(>dev);
> if (ret) {
> put_device(>dev);
> -   break;
> +   return ret;
> }
>
> ptr_entry += entry->size;
> }
>
> -   if (ret) {
> -   bus_unregister(_bus_type);
> -   memunmap(ptr);
> -   }
> -
> -   return ret;
> +   return 0;
>  }
>
>  static int coreboot_table_probe(struct platform_device *pdev)
> @@ -152,7 +131,9 @@ static int coreboot_table_probe(struct platform_device 
> *pdev)
> resource_size_t len;
> struct coreboot_table_header *header;
> struct resource *res;
> +   struct device *dev = >dev;
> void *ptr;
> +   int ret;
>
> res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> if (!res)
> @@ -162,26 +143,37 @@ static int coreboot_table_probe(struct platform_device 
> *pdev)
> if (!res->start || !len)
> return -EINVAL;
>
> +   /* Map and check just the header first to make sure things are sane */
> header = memremap(res->start, sizeof(*header), MEMREMAP_WB);
> -   if (header == NULL)
> +   if (!header)
> return -ENOMEM;
>
> -   ptr = memremap(res->start, header->header_bytes + header->table_bytes,
> -  MEMREMAP_WB);
> +   if (strncmp(header->signature, "LBIO", sizeof(header->signature))) {
> +   dev_warn(dev, "coreboot table missing or corrupt!\n");
> +   return -ENODEV;

Leaking the mapping here.

> +   }
> +
> +   

Re: [PATCH v3 6/7] firmware: coreboot: Only populate devices in coreboot_table_init()

[Julius Werner]

On Thu, Aug 9, 2018 at 10:17 AM Stephen Boyd  wrote:
>
> This function checks the header for sanity, registers a bus, and
> populates devices for each coreboot table entry. Let's just populate
> devices here and pull the other bits up into the caller so that this
> function can be repurposed for pure device creation and registration. We
> can devm()ify the memory mapping at the same time to keep error paths
> simpler.
>
> Cc: Wei-Ning Huang 
> Cc: Julius Werner 
> Cc: Brian Norris 
> Cc: Samuel Holland 
> Suggested-by: Julius Werner 
> Signed-off-by: Stephen Boyd 
> ---
>  drivers/firmware/google/coreboot_table.c | 66 +++-
>  1 file changed, 29 insertions(+), 37 deletions(-)
>
> diff --git a/drivers/firmware/google/coreboot_table.c 
> b/drivers/firmware/google/coreboot_table.c
> index f343dbe86448..814913606d22 100644
> --- a/drivers/firmware/google/coreboot_table.c
> +++ b/drivers/firmware/google/coreboot_table.c
> @@ -32,8 +32,6 @@
>  #define CB_DEV(d) container_of(d, struct coreboot_device, dev)
>  #define CB_DRV(d) container_of(d, struct coreboot_driver, drv)
>
> -static struct coreboot_table_header *ptr_header;
> -
>  static int coreboot_bus_match(struct device *dev, struct device_driver *drv)
>  {
> struct coreboot_device *device = CB_DEV(dev);
> @@ -94,35 +92,21 @@ void coreboot_driver_unregister(struct coreboot_driver 
> *driver)
>  }
>  EXPORT_SYMBOL(coreboot_driver_unregister);
>
> -static int coreboot_table_init(struct device *dev, void *ptr)
> +static int coreboot_table_populate(struct device *dev, void *ptr)
>  {
> int i, ret;
> void *ptr_entry;
> struct coreboot_device *device;
> struct coreboot_table_entry *entry;
> -   struct coreboot_table_header *header;
> -
> -   ptr_header = ptr;
> -   header = ptr;
> -
> -   if (strncmp(header->signature, "LBIO", sizeof(header->signature))) {
> -   pr_warn("coreboot_table: coreboot table missing or 
> corrupt!\n");
> -   return -ENODEV;
> -   }
> -
> -   ret = bus_register(_bus_type);
> -   if (ret)
> -   return ret;
> +   struct coreboot_table_header *header = ptr;
>
> -   ptr_entry = ptr_header + header->header_bytes;
> +   ptr_entry = ptr + header->header_bytes;
> for (i = 0; i < header->table_entries; i++) {
> entry = ptr_entry;
>
> device = kzalloc(sizeof(struct device) + entry->size, 
> GFP_KERNEL);
> -   if (!device) {
> -   ret = -ENOMEM;
> -   break;
> -   }
> +   if (!device)
> +   return -ENOMEM;
>
> dev_set_name(>dev, "coreboot%d", i);
> device->dev.parent = dev;
> @@ -133,18 +117,13 @@ static int coreboot_table_init(struct device *dev, void 
> *ptr)
> ret = device_register(>dev);
> if (ret) {
> put_device(>dev);
> -   break;
> +   return ret;
> }
>
> ptr_entry += entry->size;
> }
>
> -   if (ret) {
> -   bus_unregister(_bus_type);
> -   memunmap(ptr);
> -   }
> -
> -   return ret;
> +   return 0;
>  }
>
>  static int coreboot_table_probe(struct platform_device *pdev)
> @@ -152,7 +131,9 @@ static int coreboot_table_probe(struct platform_device 
> *pdev)
> resource_size_t len;
> struct coreboot_table_header *header;
> struct resource *res;
> +   struct device *dev = >dev;
> void *ptr;
> +   int ret;
>
> res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> if (!res)
> @@ -162,26 +143,37 @@ static int coreboot_table_probe(struct platform_device 
> *pdev)
> if (!res->start || !len)
> return -EINVAL;
>
> +   /* Map and check just the header first to make sure things are sane */
> header = memremap(res->start, sizeof(*header), MEMREMAP_WB);
> -   if (header == NULL)
> +   if (!header)
> return -ENOMEM;
>
> -   ptr = memremap(res->start, header->header_bytes + header->table_bytes,
> -  MEMREMAP_WB);
> +   if (strncmp(header->signature, "LBIO", sizeof(header->signature))) {
> +   dev_warn(dev, "coreboot table missing or corrupt!\n");
> +   return -ENODEV;

Leaking the mapping here.

> +   }
> +
> +   

Re: [PATCH v3 5/7] firmware: coreboot: Remap RAM with memremap() instead of ioremap()

[Julius Werner]

One thing to note is that we still want this space to be mappable by
userspace applications via /dev/mem, so we need to make sure that
there's no weird memory type mismatch that causes problems with that.
Adding Aaron to see if he has any concerns here, since I think he's
seen something like that in the past (not sure if it was related to
what this kernel driver does).

Can you please test this on an x86 Chromebook and run the 'cbmem'
userspace utility, make sure it doesn't fail after this?

Also, stupid question after taking a step back and looking at this
again: why do we keep a mapping alive for the lifetime of the driver
at all? It used to be necessary when this driver was
find-entry-on-demand, but nowadays it just goes through all entries
once at probe time and immediately memcpy_fromio()s out all the
relevant information into (struct coreboot_device)s. After that we're
done accessing the "real" coreboot table, forever. Why not just unmap
it again at the end of coreboot_table_init()?
On Thu, Aug 9, 2018 at 10:17 AM Stephen Boyd  wrote:
>
> This is all system memory, so we shouldn't be mapping this all with
> ioremap() as these aren't I/O regions. Instead, they're memory regions
> so we should use memremap(). Pick MEMREMAP_WB so we can map memory from
> RAM directly if that's possible, otherwise it falls back to
> ioremap_cache() like is being done here already. This also nicely
> silences the sparse warnings in this code and reduces the need to copy
> anything around anymore.
>
> Cc: Wei-Ning Huang 
> Cc: Julius Werner 
> Cc: Brian Norris 
> Cc: Samuel Holland 
> Signed-off-by: Stephen Boyd 
> ---
>  drivers/firmware/google/coreboot_table.c | 42 +++-
>  1 file changed, 20 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/firmware/google/coreboot_table.c 
> b/drivers/firmware/google/coreboot_table.c
> index feb31502f64b..f343dbe86448 100644
> --- a/drivers/firmware/google/coreboot_table.c
> +++ b/drivers/firmware/google/coreboot_table.c
> @@ -32,7 +32,7 @@
>  #define CB_DEV(d) container_of(d, struct coreboot_device, dev)
>  #define CB_DRV(d) container_of(d, struct coreboot_driver, drv)
>
> -static struct coreboot_table_header __iomem *ptr_header;
> +static struct coreboot_table_header *ptr_header;
>
>  static int coreboot_bus_match(struct device *dev, struct device_driver *drv)
>  {
> @@ -94,18 +94,18 @@ void coreboot_driver_unregister(struct coreboot_driver 
> *driver)
>  }
>  EXPORT_SYMBOL(coreboot_driver_unregister);
>
> -static int coreboot_table_init(struct device *dev, void __iomem *ptr)
> +static int coreboot_table_init(struct device *dev, void *ptr)
>  {
> int i, ret;
> void *ptr_entry;
> struct coreboot_device *device;
> -   struct coreboot_table_entry entry;
> -   struct coreboot_table_header header;
> +   struct coreboot_table_entry *entry;
> +   struct coreboot_table_header *header;
>
> ptr_header = ptr;
> -   memcpy_fromio(, ptr_header, sizeof(header));
> +   header = ptr;
>
> -   if (strncmp(header.signature, "LBIO", sizeof(header.signature))) {
> +   if (strncmp(header->signature, "LBIO", sizeof(header->signature))) {
> pr_warn("coreboot_table: coreboot table missing or 
> corrupt!\n");
> return -ENODEV;
> }
> @@ -114,11 +114,11 @@ static int coreboot_table_init(struct device *dev, void 
> __iomem *ptr)
> if (ret)
> return ret;
>
> -   ptr_entry = (void *)ptr_header + header.header_bytes;
> -   for (i = 0; i < header.table_entries; i++) {
> -   memcpy_fromio(, ptr_entry, sizeof(entry));
> +   ptr_entry = ptr_header + header->header_bytes;
> +   for (i = 0; i < header->table_entries; i++) {
> +   entry = ptr_entry;
>
> -   device = kzalloc(sizeof(struct device) + entry.size, 
> GFP_KERNEL);
> +   device = kzalloc(sizeof(struct device) + entry->size, 
> GFP_KERNEL);
> if (!device) {
> ret = -ENOMEM;
> break;
> @@ -128,7 +128,7 @@ static int coreboot_table_init(struct device *dev, void 
> __iomem *ptr)
> device->dev.parent = dev;
> device->dev.bus = _bus_type;
> device->dev.release = coreboot_device_release;
> -   memcpy_fromio(>entry, ptr_entry, entry.size);
> +   memcpy(>entry, ptr_entry, entry->size);
>
> ret = device_register(>dev);
> if (ret) {
> @@ -136,12 +136,12 @@ static int coreboot_table_init(struct device *dev, void 
> __iomem *ptr)
>

Re: [PATCH v3 5/7] firmware: coreboot: Remap RAM with memremap() instead of ioremap()

[Julius Werner]

One thing to note is that we still want this space to be mappable by
userspace applications via /dev/mem, so we need to make sure that
there's no weird memory type mismatch that causes problems with that.
Adding Aaron to see if he has any concerns here, since I think he's
seen something like that in the past (not sure if it was related to
what this kernel driver does).

Can you please test this on an x86 Chromebook and run the 'cbmem'
userspace utility, make sure it doesn't fail after this?

Also, stupid question after taking a step back and looking at this
again: why do we keep a mapping alive for the lifetime of the driver
at all? It used to be necessary when this driver was
find-entry-on-demand, but nowadays it just goes through all entries
once at probe time and immediately memcpy_fromio()s out all the
relevant information into (struct coreboot_device)s. After that we're
done accessing the "real" coreboot table, forever. Why not just unmap
it again at the end of coreboot_table_init()?
On Thu, Aug 9, 2018 at 10:17 AM Stephen Boyd  wrote:
>
> This is all system memory, so we shouldn't be mapping this all with
> ioremap() as these aren't I/O regions. Instead, they're memory regions
> so we should use memremap(). Pick MEMREMAP_WB so we can map memory from
> RAM directly if that's possible, otherwise it falls back to
> ioremap_cache() like is being done here already. This also nicely
> silences the sparse warnings in this code and reduces the need to copy
> anything around anymore.
>
> Cc: Wei-Ning Huang 
> Cc: Julius Werner 
> Cc: Brian Norris 
> Cc: Samuel Holland 
> Signed-off-by: Stephen Boyd 
> ---
>  drivers/firmware/google/coreboot_table.c | 42 +++-
>  1 file changed, 20 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/firmware/google/coreboot_table.c 
> b/drivers/firmware/google/coreboot_table.c
> index feb31502f64b..f343dbe86448 100644
> --- a/drivers/firmware/google/coreboot_table.c
> +++ b/drivers/firmware/google/coreboot_table.c
> @@ -32,7 +32,7 @@
>  #define CB_DEV(d) container_of(d, struct coreboot_device, dev)
>  #define CB_DRV(d) container_of(d, struct coreboot_driver, drv)
>
> -static struct coreboot_table_header __iomem *ptr_header;
> +static struct coreboot_table_header *ptr_header;
>
>  static int coreboot_bus_match(struct device *dev, struct device_driver *drv)
>  {
> @@ -94,18 +94,18 @@ void coreboot_driver_unregister(struct coreboot_driver 
> *driver)
>  }
>  EXPORT_SYMBOL(coreboot_driver_unregister);
>
> -static int coreboot_table_init(struct device *dev, void __iomem *ptr)
> +static int coreboot_table_init(struct device *dev, void *ptr)
>  {
> int i, ret;
> void *ptr_entry;
> struct coreboot_device *device;
> -   struct coreboot_table_entry entry;
> -   struct coreboot_table_header header;
> +   struct coreboot_table_entry *entry;
> +   struct coreboot_table_header *header;
>
> ptr_header = ptr;
> -   memcpy_fromio(, ptr_header, sizeof(header));
> +   header = ptr;
>
> -   if (strncmp(header.signature, "LBIO", sizeof(header.signature))) {
> +   if (strncmp(header->signature, "LBIO", sizeof(header->signature))) {
> pr_warn("coreboot_table: coreboot table missing or 
> corrupt!\n");
> return -ENODEV;
> }
> @@ -114,11 +114,11 @@ static int coreboot_table_init(struct device *dev, void 
> __iomem *ptr)
> if (ret)
> return ret;
>
> -   ptr_entry = (void *)ptr_header + header.header_bytes;
> -   for (i = 0; i < header.table_entries; i++) {
> -   memcpy_fromio(, ptr_entry, sizeof(entry));
> +   ptr_entry = ptr_header + header->header_bytes;
> +   for (i = 0; i < header->table_entries; i++) {
> +   entry = ptr_entry;
>
> -   device = kzalloc(sizeof(struct device) + entry.size, 
> GFP_KERNEL);
> +   device = kzalloc(sizeof(struct device) + entry->size, 
> GFP_KERNEL);
> if (!device) {
> ret = -ENOMEM;
> break;
> @@ -128,7 +128,7 @@ static int coreboot_table_init(struct device *dev, void 
> __iomem *ptr)
> device->dev.parent = dev;
> device->dev.bus = _bus_type;
> device->dev.release = coreboot_device_release;
> -   memcpy_fromio(>entry, ptr_entry, entry.size);
> +   memcpy(>entry, ptr_entry, entry->size);
>
> ret = device_register(>dev);
> if (ret) {
> @@ -136,12 +136,12 @@ static int coreboot_table_init(struct device *dev, void 
> __iomem *ptr)
>

Re: [PATCH v3 3/7] firmware: coreboot: Make bus registration symmetric

[Julius Werner]

> @@ -138,8 +136,10 @@ int coreboot_table_init(struct device *dev, void __iomem 
> *ptr)
> ptr_entry += entry.size;
> }
>
> -   if (ret)
> +   if (ret) {
> +   bus_unregister(_bus_type);
> iounmap(ptr);
> +   }

nit: maybe cleaner to just do if (ret) coreboot_table_exit(); here?
You're essentially writing the same code again.

Re: [PATCH v3 3/7] firmware: coreboot: Make bus registration symmetric

[Julius Werner]

> @@ -138,8 +136,10 @@ int coreboot_table_init(struct device *dev, void __iomem 
> *ptr)
> ptr_entry += entry.size;
> }
>
> -   if (ret)
> +   if (ret) {
> +   bus_unregister(_bus_type);
> iounmap(ptr);
> +   }

nit: maybe cleaner to just do if (ret) coreboot_table_exit(); here?
You're essentially writing the same code again.

Re: [PATCH v2 2/2] firmware: coreboot: Collapse platform drivers into bus core

[Julius Werner]

> +config GOOGLE_COREBOOT_TABLE_ACPI
> +   tristate
> +   default GOOGLE_COREBOOT_TABLE

I don't think this helps in upgrading (as your commit message says)
unless you also keep the 'select GOOGLE_COREBOOT_TABLE' here, right?

> -int coreboot_table_init(struct device *dev, void __iomem *ptr)
> +static int coreboot_table_init(struct device *dev, void __iomem *ptr)

nit: There's little reason to keep coreboot_table_init() a separate
function now. Could maybe compact the code a little more if you merge
it into probe()? (Also could then do the signature sanity check before
trusting the length values to map the whole thing, which is probably a
good idea.)

> if (ptr_header) {
> bus_unregister(_bus_type);
> iounmap(ptr_header);

Could ptr_header be handled by devm now, somehow? Also, don't you have
two bus_unregister() now (here and in coreboot_exit())? Or is that
intentional?

> +static struct platform_driver coreboot_table_driver = {
> +   .probe = coreboot_table_probe,
> +   .remove = coreboot_table_remove,
> +   .driver = {
> +   .name = "coreboot_table",
> +   .acpi_match_table = ACPI_PTR(cros_coreboot_acpi_match),
> +   .of_match_table = of_match_ptr(coreboot_of_match),

Who takes precedence if they both exist? Will we have two
coreboot_table busses? (That would probably not be so good...)

Re: [PATCH v2 2/2] firmware: coreboot: Collapse platform drivers into bus core

[Julius Werner]

> +config GOOGLE_COREBOOT_TABLE_ACPI
> +   tristate
> +   default GOOGLE_COREBOOT_TABLE

I don't think this helps in upgrading (as your commit message says)
unless you also keep the 'select GOOGLE_COREBOOT_TABLE' here, right?

> -int coreboot_table_init(struct device *dev, void __iomem *ptr)
> +static int coreboot_table_init(struct device *dev, void __iomem *ptr)

nit: There's little reason to keep coreboot_table_init() a separate
function now. Could maybe compact the code a little more if you merge
it into probe()? (Also could then do the signature sanity check before
trusting the length values to map the whole thing, which is probably a
good idea.)

> if (ptr_header) {
> bus_unregister(_bus_type);
> iounmap(ptr_header);

Could ptr_header be handled by devm now, somehow? Also, don't you have
two bus_unregister() now (here and in coreboot_exit())? Or is that
intentional?

> +static struct platform_driver coreboot_table_driver = {
> +   .probe = coreboot_table_probe,
> +   .remove = coreboot_table_remove,
> +   .driver = {
> +   .name = "coreboot_table",
> +   .acpi_match_table = ACPI_PTR(cros_coreboot_acpi_match),
> +   .of_match_table = of_match_ptr(coreboot_of_match),

Who takes precedence if they both exist? Will we have two
coreboot_table busses? (That would probably not be so good...)

Re: [PATCH] firmware: coreboot: Let OF core populate platform device

[Julius Werner]

Thanks for the quick fix!

Reviewed-by: Julius Werner 

Re: [PATCH] firmware: coreboot: Let OF core populate platform device

[Julius Werner]

Thanks for the quick fix!

Reviewed-by: Julius Werner 

[PATCH] Staging:rtl8712: Style - Removed inline block comment to fix "Statements should start on a tapstop"

[Frank Werner-Krippendorf]

Removed 5 inline comments "/*volatile*/" rtl87x_event.h, to fix
a coding style issue "Statements should start on a tabstop"
detected by checkpatch.pl script.

Signed-off-by: Frank Werner-Krippendorf 
---
 drivers/staging/rtl8712/rtl871x_event.h | 10 +-
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/staging/rtl8712/rtl871x_event.h 
b/drivers/staging/rtl8712/rtl871x_event.h
index 5171379..2e59e8e 100644
--- a/drivers/staging/rtl8712/rtl871x_event.h
+++ b/drivers/staging/rtl8712/rtl871x_event.h
@@ -90,13 +90,13 @@ struct event_node {
unsigned char *node;
unsigned char evt_code;
unsigned short evt_sz;
-   /*volatile*/ int *caller_ff_tail;
+   int *caller_ff_tail;
int caller_ff_sz;
 };
 
 struct c2hevent_queue {
-   /*volatile*/ inthead;
-   /*volatile*/ inttail;
+   int head;
+   int tail;
struct  event_node  nodes[C2HEVENT_SZ];
unsigned char   seq;
 };
@@ -104,8 +104,8 @@ struct c2hevent_queue {
 #define NETWORK_QUEUE_SZ   4
 
 struct network_queue {
-   /*volatile*/ inthead;
-   /*volatile*/ inttail;
+   int head;
+   int tail;
struct wlan_bssid_ex networks[NETWORK_QUEUE_SZ];
 };
 
-- 
2.7.4

[PATCH] Staging:rtl8712: Style - Removed inline block comment to fix "Statements should start on a tapstop"

[Frank Werner-Krippendorf]

Removed 5 inline comments "/*volatile*/" rtl87x_event.h, to fix
a coding style issue "Statements should start on a tabstop"
detected by checkpatch.pl script.

Signed-off-by: Frank Werner-Krippendorf 
---
 drivers/staging/rtl8712/rtl871x_event.h | 10 +-
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/staging/rtl8712/rtl871x_event.h 
b/drivers/staging/rtl8712/rtl871x_event.h
index 5171379..2e59e8e 100644
--- a/drivers/staging/rtl8712/rtl871x_event.h
+++ b/drivers/staging/rtl8712/rtl871x_event.h
@@ -90,13 +90,13 @@ struct event_node {
unsigned char *node;
unsigned char evt_code;
unsigned short evt_sz;
-   /*volatile*/ int *caller_ff_tail;
+   int *caller_ff_tail;
int caller_ff_sz;
 };
 
 struct c2hevent_queue {
-   /*volatile*/ inthead;
-   /*volatile*/ inttail;
+   int head;
+   int tail;
struct  event_node  nodes[C2HEVENT_SZ];
unsigned char   seq;
 };
@@ -104,8 +104,8 @@ struct c2hevent_queue {
 #define NETWORK_QUEUE_SZ   4
 
 struct network_queue {
-   /*volatile*/ inthead;
-   /*volatile*/ inttail;
+   int head;
+   int tail;
struct wlan_bssid_ex networks[NETWORK_QUEUE_SZ];
 };
 
-- 
2.7.4

[PATCH] Staging: rtl8712: rtl871x: removed unused code, to fix a coding style issue

[Frank Werner-Krippendorf]

Fixed a coding style issue.

Signed-off-by: Frank Werner-Krippendorf 
---
 drivers/staging/rtl8712/rtl871x_event.h   | 10 +-
 drivers/staging/rtl8712/rtl871x_io.h  |  2 +-
 drivers/staging/rtl8712/rtl871x_pwrctrl.h | 10 +-
 drivers/staging/rtl8712/rtl871x_xmit.h| 14 +++---
 4 files changed, 18 insertions(+), 18 deletions(-)

diff --git a/drivers/staging/rtl8712/rtl871x_event.h 
b/drivers/staging/rtl8712/rtl871x_event.h
index 5171379..2e59e8e 100644
--- a/drivers/staging/rtl8712/rtl871x_event.h
+++ b/drivers/staging/rtl8712/rtl871x_event.h
@@ -90,13 +90,13 @@ struct event_node {
unsigned char *node;
unsigned char evt_code;
unsigned short evt_sz;
-   /*volatile*/ int *caller_ff_tail;
+   int *caller_ff_tail;
int caller_ff_sz;
 };
 
 struct c2hevent_queue {
-   /*volatile*/ inthead;
-   /*volatile*/ inttail;
+   int head;
+   int tail;
struct  event_node  nodes[C2HEVENT_SZ];
unsigned char   seq;
 };
@@ -104,8 +104,8 @@ struct c2hevent_queue {
 #define NETWORK_QUEUE_SZ   4
 
 struct network_queue {
-   /*volatile*/ inthead;
-   /*volatile*/ inttail;
+   int head;
+   int tail;
struct wlan_bssid_ex networks[NETWORK_QUEUE_SZ];
 };
 
diff --git a/drivers/staging/rtl8712/rtl871x_io.h 
b/drivers/staging/rtl8712/rtl871x_io.h
index dd054d7..61d9e41 100644
--- a/drivers/staging/rtl8712/rtl871x_io.h
+++ b/drivers/staging/rtl8712/rtl871x_io.h
@@ -113,7 +113,7 @@ struct  _io_ops {
 struct io_req {
struct list_head list;
u32 addr;
-   /*volatile*/ u32val;
+   u32 val;
u32 command;
u32 status;
u8  *pbuf;
diff --git a/drivers/staging/rtl8712/rtl871x_pwrctrl.h 
b/drivers/staging/rtl8712/rtl871x_pwrctrl.h
index bd2c3a2..6451beb 100644
--- a/drivers/staging/rtl8712/rtl871x_pwrctrl.h
+++ b/drivers/staging/rtl8712/rtl871x_pwrctrl.h
@@ -89,14 +89,14 @@ struct reportpwrstate_parm {
 
 struct pwrctrl_priv {
struct mutex mutex_lock;
-   /*volatile*/ u8 rpwm; /* requested power state for fw */
+   u8 rpwm; /* requested power state for fw */
/* fw current power state. updated when 1. read from HCPWM or
 * 2. driver lowers power level
 */
-   /*volatile*/ u8 cpwm;
-   /*volatile*/ u8 tog; /* toggling */
-   /*volatile*/ u8 cpwm_tog; /* toggling */
-   /*volatile*/ u8 tgt_rpwm; /* wanted power state */
+   u8 cpwm;
+   u8 tog; /* toggling */
+   u8 cpwm_tog; /* toggling */
+   u8 tgt_rpwm; /* wanted power state */
uint pwr_mode;
uint smart_ps;
uint alives;
diff --git a/drivers/staging/rtl8712/rtl871x_xmit.h 
b/drivers/staging/rtl8712/rtl871x_xmit.h
index 4092727..aac0e14 100644
--- a/drivers/staging/rtl8712/rtl871x_xmit.h
+++ b/drivers/staging/rtl8712/rtl871x_xmit.h
@@ -160,8 +160,8 @@ struct xmit_frame {
_pkt *pkt;
int frame_tag;
struct _adapter *padapter;
-u8 *buf_addr;
-struct xmit_buf *pxmitbuf;
+   u8 *buf_addr;
+   struct xmit_buf *pxmitbuf;
u8 *mem_addr;
u16 sz[8];
struct urb *pxmit_urb[8];
@@ -194,11 +194,11 @@ struct sta_xmit_priv {
 };
 
 struct hw_txqueue {
-   /*volatile*/ sint   head;
-   /*volatile*/ sint   tail;
-   /*volatile*/ sint   free_sz;/*in units of 64 bytes*/
-   /*volatile*/ sint  free_cmdsz;
-   /*volatile*/ sinttxsz[8];
+   sinthead;
+   sinttail;
+   sintfree_sz;/*in units of 64 bytes*/
+   sintfree_cmdsz;
+   sinttxsz[8];
uintff_hwaddr;
uintcmd_hwaddr;
sintac_tag;
-- 
2.7.4

[PATCH] Staging: rtl8712: rtl871x: removed unused code, to fix a coding style issue

[Frank Werner-Krippendorf]

Fixed a coding style issue.

Signed-off-by: Frank Werner-Krippendorf 
---
 drivers/staging/rtl8712/rtl871x_event.h   | 10 +-
 drivers/staging/rtl8712/rtl871x_io.h  |  2 +-
 drivers/staging/rtl8712/rtl871x_pwrctrl.h | 10 +-
 drivers/staging/rtl8712/rtl871x_xmit.h| 14 +++---
 4 files changed, 18 insertions(+), 18 deletions(-)

diff --git a/drivers/staging/rtl8712/rtl871x_event.h 
b/drivers/staging/rtl8712/rtl871x_event.h
index 5171379..2e59e8e 100644
--- a/drivers/staging/rtl8712/rtl871x_event.h
+++ b/drivers/staging/rtl8712/rtl871x_event.h
@@ -90,13 +90,13 @@ struct event_node {
unsigned char *node;
unsigned char evt_code;
unsigned short evt_sz;
-   /*volatile*/ int *caller_ff_tail;
+   int *caller_ff_tail;
int caller_ff_sz;
 };
 
 struct c2hevent_queue {
-   /*volatile*/ inthead;
-   /*volatile*/ inttail;
+   int head;
+   int tail;
struct  event_node  nodes[C2HEVENT_SZ];
unsigned char   seq;
 };
@@ -104,8 +104,8 @@ struct c2hevent_queue {
 #define NETWORK_QUEUE_SZ   4
 
 struct network_queue {
-   /*volatile*/ inthead;
-   /*volatile*/ inttail;
+   int head;
+   int tail;
struct wlan_bssid_ex networks[NETWORK_QUEUE_SZ];
 };
 
diff --git a/drivers/staging/rtl8712/rtl871x_io.h 
b/drivers/staging/rtl8712/rtl871x_io.h
index dd054d7..61d9e41 100644
--- a/drivers/staging/rtl8712/rtl871x_io.h
+++ b/drivers/staging/rtl8712/rtl871x_io.h
@@ -113,7 +113,7 @@ struct  _io_ops {
 struct io_req {
struct list_head list;
u32 addr;
-   /*volatile*/ u32val;
+   u32 val;
u32 command;
u32 status;
u8  *pbuf;
diff --git a/drivers/staging/rtl8712/rtl871x_pwrctrl.h 
b/drivers/staging/rtl8712/rtl871x_pwrctrl.h
index bd2c3a2..6451beb 100644
--- a/drivers/staging/rtl8712/rtl871x_pwrctrl.h
+++ b/drivers/staging/rtl8712/rtl871x_pwrctrl.h
@@ -89,14 +89,14 @@ struct reportpwrstate_parm {
 
 struct pwrctrl_priv {
struct mutex mutex_lock;
-   /*volatile*/ u8 rpwm; /* requested power state for fw */
+   u8 rpwm; /* requested power state for fw */
/* fw current power state. updated when 1. read from HCPWM or
 * 2. driver lowers power level
 */
-   /*volatile*/ u8 cpwm;
-   /*volatile*/ u8 tog; /* toggling */
-   /*volatile*/ u8 cpwm_tog; /* toggling */
-   /*volatile*/ u8 tgt_rpwm; /* wanted power state */
+   u8 cpwm;
+   u8 tog; /* toggling */
+   u8 cpwm_tog; /* toggling */
+   u8 tgt_rpwm; /* wanted power state */
uint pwr_mode;
uint smart_ps;
uint alives;
diff --git a/drivers/staging/rtl8712/rtl871x_xmit.h 
b/drivers/staging/rtl8712/rtl871x_xmit.h
index 4092727..aac0e14 100644
--- a/drivers/staging/rtl8712/rtl871x_xmit.h
+++ b/drivers/staging/rtl8712/rtl871x_xmit.h
@@ -160,8 +160,8 @@ struct xmit_frame {
_pkt *pkt;
int frame_tag;
struct _adapter *padapter;
-u8 *buf_addr;
-struct xmit_buf *pxmitbuf;
+   u8 *buf_addr;
+   struct xmit_buf *pxmitbuf;
u8 *mem_addr;
u16 sz[8];
struct urb *pxmit_urb[8];
@@ -194,11 +194,11 @@ struct sta_xmit_priv {
 };
 
 struct hw_txqueue {
-   /*volatile*/ sint   head;
-   /*volatile*/ sint   tail;
-   /*volatile*/ sint   free_sz;/*in units of 64 bytes*/
-   /*volatile*/ sint  free_cmdsz;
-   /*volatile*/ sinttxsz[8];
+   sinthead;
+   sinttail;
+   sintfree_sz;/*in units of 64 bytes*/
+   sintfree_cmdsz;
+   sinttxsz[8];
uintff_hwaddr;
uintcmd_hwaddr;
sintac_tag;
-- 
2.7.4

Re: [PATCH v2] firmware: Update Kconfig help text for Google firmware

[Julius Werner]

LGTM

Reviewed-by: Julius Werner 
On Mon, Jun 18, 2018 at 3:55 PM Ben Hutchings  wrote:
>
> The help text for GOOGLE_FIRMWARE states that it should only be
> enabled when building a kernel for Google's own servers.  However,
> many of the drivers dependent on it are also useful on Chromebooks or
> on any platform using coreboot.
>
> Update the help text to reflect this double duty.
>
> Fixes: d384d6f43d1e ("firmware: google memconsole: Add coreboot support")
> Signed-off-by: Ben Hutchings 
> ---
> v2: Mention coreboot, and don't touch GSMI help text
>
>  drivers/firmware/google/Kconfig | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/firmware/google/Kconfig b/drivers/firmware/google/Kconfig
> index f16b381a569c..ca049ecf5cfd 100644
> --- a/drivers/firmware/google/Kconfig
> +++ b/drivers/firmware/google/Kconfig
> @@ -2,9 +2,9 @@ menuconfig GOOGLE_FIRMWARE
> bool "Google Firmware Drivers"
> default n
> help
> - These firmware drivers are used by Google's servers.  They are
> - only useful if you are working directly on one of their
> - proprietary servers.  If in doubt, say "N".
> + These firmware drivers are used by Google servers,
> + Chromebooks and other devices using coreboot firmware.
> + If in doubt, say "N".
>
>  if GOOGLE_FIRMWARE
>

Re: [PATCH v2] firmware: Update Kconfig help text for Google firmware

[Julius Werner]

LGTM

Reviewed-by: Julius Werner 
On Mon, Jun 18, 2018 at 3:55 PM Ben Hutchings  wrote:
>
> The help text for GOOGLE_FIRMWARE states that it should only be
> enabled when building a kernel for Google's own servers.  However,
> many of the drivers dependent on it are also useful on Chromebooks or
> on any platform using coreboot.
>
> Update the help text to reflect this double duty.
>
> Fixes: d384d6f43d1e ("firmware: google memconsole: Add coreboot support")
> Signed-off-by: Ben Hutchings 
> ---
> v2: Mention coreboot, and don't touch GSMI help text
>
>  drivers/firmware/google/Kconfig | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/firmware/google/Kconfig b/drivers/firmware/google/Kconfig
> index f16b381a569c..ca049ecf5cfd 100644
> --- a/drivers/firmware/google/Kconfig
> +++ b/drivers/firmware/google/Kconfig
> @@ -2,9 +2,9 @@ menuconfig GOOGLE_FIRMWARE
> bool "Google Firmware Drivers"
> default n
> help
> - These firmware drivers are used by Google's servers.  They are
> - only useful if you are working directly on one of their
> - proprietary servers.  If in doubt, say "N".
> + These firmware drivers are used by Google servers,
> + Chromebooks and other devices using coreboot firmware.
> + If in doubt, say "N".
>
>  if GOOGLE_FIRMWARE
>

Re: [PATCH] firmware: Update Kconfig help text for Google firmware

[Julius Werner]

On Sat, Jun 16, 2018 at 3:05 PM Ben Hutchings  wrote:
>
> The help text for GOOGLE_FIRMWARE states that it should only be
> enabled when building a kernel for Google's own servers.  However, it
> is now also a dependency for various Chromebook firmware drivers.
>
> Update the help text to reflect this double duty.  Add the qualifier
> "server" to the help text for GOOGLE_SMI, which doesn't appear to be
> useful on Chromebooks.
>
> Fixes: d384d6f43d1e ("firmware: google memconsole: Add coreboot support")
> Signed-off-by: Ben Hutchings 
> ---
>  drivers/firmware/google/Kconfig | 9 -
>  1 file changed, 4 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/firmware/google/Kconfig b/drivers/firmware/google/Kconfig
> index f16b381a569c..6fda2c5c69e4 100644
> --- a/drivers/firmware/google/Kconfig
> +++ b/drivers/firmware/google/Kconfig
> @@ -2,9 +2,8 @@ menuconfig GOOGLE_FIRMWARE
> bool "Google Firmware Drivers"
> default n
> help
> - These firmware drivers are used by Google's servers.  They are
> - only useful if you are working directly on one of their
> - proprietary servers.  If in doubt, say "N".
> + These firmware drivers are used by Google servers and
> + Chromebooks.  If in doubt, say "N".

nit: Technically, most of the stuff is useful on all coreboot devices,
not just Chromebooks. (Ideally we'd want to factor that out into a
separate drivers/firmware/coreboot, but it's a bit tricky because the
same memconsole frontend is used for both coreboot and a proprietary
Google server BIOS, so it's a little tricky to decide where everything
should go. Nobody had the time to go untangle it yet.)

>  if GOOGLE_FIRMWARE
>
> @@ -14,8 +13,8 @@ config GOOGLE_SMI
> select EFI_VARS
> help
>   Say Y here if you want to enable SMI callbacks for Google
> - platforms.  This provides an interface for writing to and
> - clearing the EFI event log and reading and writing NVRAM
> + server platforms.  This provides an interface for writing to
> + and clearing the EFI event log and reading and writing NVRAM
>   variables.

GSMI is also used by Chromebooks AFAIK. (I think it's sort of become a
general coreboot interface by now as well, although the name still
says Google.)

Re: [PATCH] firmware: Update Kconfig help text for Google firmware

[Julius Werner]

On Sat, Jun 16, 2018 at 3:05 PM Ben Hutchings  wrote:
>
> The help text for GOOGLE_FIRMWARE states that it should only be
> enabled when building a kernel for Google's own servers.  However, it
> is now also a dependency for various Chromebook firmware drivers.
>
> Update the help text to reflect this double duty.  Add the qualifier
> "server" to the help text for GOOGLE_SMI, which doesn't appear to be
> useful on Chromebooks.
>
> Fixes: d384d6f43d1e ("firmware: google memconsole: Add coreboot support")
> Signed-off-by: Ben Hutchings 
> ---
>  drivers/firmware/google/Kconfig | 9 -
>  1 file changed, 4 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/firmware/google/Kconfig b/drivers/firmware/google/Kconfig
> index f16b381a569c..6fda2c5c69e4 100644
> --- a/drivers/firmware/google/Kconfig
> +++ b/drivers/firmware/google/Kconfig
> @@ -2,9 +2,8 @@ menuconfig GOOGLE_FIRMWARE
> bool "Google Firmware Drivers"
> default n
> help
> - These firmware drivers are used by Google's servers.  They are
> - only useful if you are working directly on one of their
> - proprietary servers.  If in doubt, say "N".
> + These firmware drivers are used by Google servers and
> + Chromebooks.  If in doubt, say "N".

nit: Technically, most of the stuff is useful on all coreboot devices,
not just Chromebooks. (Ideally we'd want to factor that out into a
separate drivers/firmware/coreboot, but it's a bit tricky because the
same memconsole frontend is used for both coreboot and a proprietary
Google server BIOS, so it's a little tricky to decide where everything
should go. Nobody had the time to go untangle it yet.)

>  if GOOGLE_FIRMWARE
>
> @@ -14,8 +13,8 @@ config GOOGLE_SMI
> select EFI_VARS
> help
>   Say Y here if you want to enable SMI callbacks for Google
> - platforms.  This provides an interface for writing to and
> - clearing the EFI event log and reading and writing NVRAM
> + server platforms.  This provides an interface for writing to
> + and clearing the EFI event log and reading and writing NVRAM
>   variables.

GSMI is also used by Chromebooks AFAIK. (I think it's sort of become a
general coreboot interface by now as well, although the name still
says Google.)

Re: [PATCH 0/5] coreboot table bus and framebuffer driver

[Julius Werner]

[resend in plain text]

> It would be great to get some of the google developers to ack these, as
> this touches their code...

 From the coreboot point of view I guess we're fine with it since it claims
to maintain all of the existing functionality. It's just changing the
kernel-level plumbing for these drivers and I don't really have the
expertise to comment on whether this is better or worse than the old code
(maybe Dmitry or Guenter will?). It seems a little odd to me to call this a
"bus", but if we think that's the most fitting abstraction the kernel has
for it, I'm okay with that. All I care about is that it will work (in all
combinations... e.g. regardless of probe order and even if some parts are
compiled as modules and loaded/unloaded at runtime).

Re: [PATCH 0/5] coreboot table bus and framebuffer driver

[Julius Werner]

[resend in plain text]

> It would be great to get some of the google developers to ack these, as
> this touches their code...

 From the coreboot point of view I guess we're fine with it since it claims
to maintain all of the existing functionality. It's just changing the
kernel-level plumbing for these drivers and I don't really have the
expertise to comment on whether this is better or worse than the old code
(maybe Dmitry or Guenter will?). It seems a little odd to me to call this a
"bus", but if we think that's the most fitting abstraction the kernel has
for it, I'm okay with that. All I care about is that it will work (in all
combinations... e.g. regardless of probe order and even if some parts are
compiled as modules and loaded/unloaded at runtime).

[PATCH] drivers: char: mem: Fix wraparound check to allow mappings up to the end

[Julius Werner]

A recent fix to /dev/mem prevents mappings from wrapping around the end
of physical address space. However, the check was written in a way that
also prevents a mapping reaching just up to the end of physical address
space, which may be a valid use case (especially on 32-bit systems).
This patch fixes it by checking the last mapped address (instead of the
first address behind that) for overflow.

Fixes: b299cde245 ("drivers: char: mem: Check for address space wraparound with 
mmap()")
Cc: <sta...@vger.kernel.org>
Reported-by: Nico Huber <nic...@gmx.de>
Signed-off-by: Julius Werner <jwer...@chromium.org>
---
 drivers/char/mem.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/char/mem.c b/drivers/char/mem.c
index 6e0cbe092220..593a8818aca9 100644
--- a/drivers/char/mem.c
+++ b/drivers/char/mem.c
@@ -343,7 +343,7 @@ static int mmap_mem(struct file *file, struct 
vm_area_struct *vma)
phys_addr_t offset = (phys_addr_t)vma->vm_pgoff << PAGE_SHIFT;
 
/* It's illegal to wrap around the end of the physical address space. */
-   if (offset + (phys_addr_t)size < offset)
+   if (offset + (phys_addr_t)size - 1 < offset)
return -EINVAL;
 
if (!valid_mmap_phys_addr_range(vma->vm_pgoff, size))
-- 
2.12.2

[PATCH] drivers: char: mem: Fix wraparound check to allow mappings up to the end

[Julius Werner]

A recent fix to /dev/mem prevents mappings from wrapping around the end
of physical address space. However, the check was written in a way that
also prevents a mapping reaching just up to the end of physical address
space, which may be a valid use case (especially on 32-bit systems).
This patch fixes it by checking the last mapped address (instead of the
first address behind that) for overflow.

Fixes: b299cde245 ("drivers: char: mem: Check for address space wraparound with 
mmap()")
Cc: 
Reported-by: Nico Huber 
Signed-off-by: Julius Werner 
---
 drivers/char/mem.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/char/mem.c b/drivers/char/mem.c
index 6e0cbe092220..593a8818aca9 100644
--- a/drivers/char/mem.c
+++ b/drivers/char/mem.c
@@ -343,7 +343,7 @@ static int mmap_mem(struct file *file, struct 
vm_area_struct *vma)
phys_addr_t offset = (phys_addr_t)vma->vm_pgoff << PAGE_SHIFT;
 
/* It's illegal to wrap around the end of the physical address space. */
-   if (offset + (phys_addr_t)size < offset)
+   if (offset + (phys_addr_t)size - 1 < offset)
return -EINVAL;
 
if (!valid_mmap_phys_addr_range(vma->vm_pgoff, size))
-- 
2.12.2

Re: [PATCH 7/8] firmware: vpd: remove platform driver

[Julius Werner]

I'm not a kernel expert so maybe I don't understand this right, but...
I think this might have been done this way to ensure that the driver
can get initialized correctly regardless of probe ordering.
coreboot_table_find() may fail with -EPROBE_DEFER if the
coreboot_table driver and its dependent (coreboot_table-acpi or
coreboot_table-of) haven't been probed yet. In that case we want the
VPD driver to wait and try again later, after they've been probed. I
believe with the way this used to be written the driver core did that
automatically for us, but with your change I'm not sure if it still
does.

On Wed, May 24, 2017 at 10:22 AM, Guenter Roeck  wrote:
> On Tue, May 23, 2017 at 5:07 PM, Dmitry Torokhov
>  wrote:
>> There is no reason why VPD should register platform device and driver,
>> given that we do not use their respective kobjects to attach attributes,
>> nor do we need suspend/resume hooks, or any other features of device
>> core.
>>
>> Signed-off-by: Dmitry Torokhov 
>
> One reason might be to use devm_ functions for memory allocations to
> simplify error handling and cleanup. Without that, I agree.
>
> Reviewed-by: Guenter Roeck 
>
>> ---
>>  drivers/firmware/google/vpd.c | 44 
>> ---
>>  1 file changed, 16 insertions(+), 28 deletions(-)
>>
>> diff --git a/drivers/firmware/google/vpd.c b/drivers/firmware/google/vpd.c
>> index 78945729388e..f4766bf6785a 100644
>> --- a/drivers/firmware/google/vpd.c
>> +++ b/drivers/firmware/google/vpd.c
>> @@ -22,8 +22,6 @@
>>  #include 
>>  #include 
>>  #include 
>> -#include 
>> -#include 
>>  #include 
>>  #include 
>>
>> @@ -279,47 +277,37 @@ static int vpd_sections_init(phys_addr_t physaddr)
>> ret = vpd_section_init("rw", _vpd,
>>physaddr + sizeof(struct vpd_cbmem) +
>>header.ro_size, header.rw_size);
>> -   if (ret)
>> +   if (ret) {
>> +   vpd_section_destroy(_vpd);
>> return ret;
>> +   }
>> }
>>
>> return 0;
>>  }
>>
>> -static int vpd_probe(struct platform_device *pdev)
>> -{
>> -   int ret;
>> -   struct lb_cbmem_ref entry;
>> -
>> -   ret = coreboot_table_find(CB_TAG_VPD, , sizeof(entry));
>> -   if (ret)
>> -   return ret;
>> -
>> -   return vpd_sections_init(entry.cbmem_addr);
>> -}
>> -
>> -static struct platform_driver vpd_driver = {
>> -   .probe = vpd_probe,
>> -   .driver = {
>> -   .name = "vpd",
>> -   },
>> -};
>> -
>>  static int __init vpd_platform_init(void)
>>  {
>> -   struct platform_device *pdev;
>> -
>> -   pdev = platform_device_register_simple("vpd", -1, NULL, 0);
>> -   if (IS_ERR(pdev))
>> -   return PTR_ERR(pdev);
>> +   struct lb_cbmem_ref entry;
>> +   int err;
>>
>> vpd_kobj = kobject_create_and_add("vpd", firmware_kobj);
>> if (!vpd_kobj)
>> return -ENOMEM;
>>
>> -   platform_driver_register(_driver);
>> +   err = coreboot_table_find(CB_TAG_VPD, , sizeof(entry));
>> +   if (err)
>> +   goto err_kobject_put;
>> +
>> +   err = vpd_sections_init(entry.cbmem_addr);
>> +   if (err)
>> +   goto err_kobject_put;
>>
>> return 0;
>> +
>> +err_kobject_put:
>> +   kobject_put(vpd_kobj);
>> +   return err;
>>  }
>>
>>  static void __exit vpd_platform_exit(void)
>> --
>> 2.13.0.219.gdb65acc882-goog
>>


smime.p7s
Description: S/MIME Cryptographic Signature

Re: [PATCH 7/8] firmware: vpd: remove platform driver

[Julius Werner]

I'm not a kernel expert so maybe I don't understand this right, but...
I think this might have been done this way to ensure that the driver
can get initialized correctly regardless of probe ordering.
coreboot_table_find() may fail with -EPROBE_DEFER if the
coreboot_table driver and its dependent (coreboot_table-acpi or
coreboot_table-of) haven't been probed yet. In that case we want the
VPD driver to wait and try again later, after they've been probed. I
believe with the way this used to be written the driver core did that
automatically for us, but with your change I'm not sure if it still
does.

On Wed, May 24, 2017 at 10:22 AM, Guenter Roeck  wrote:
> On Tue, May 23, 2017 at 5:07 PM, Dmitry Torokhov
>  wrote:
>> There is no reason why VPD should register platform device and driver,
>> given that we do not use their respective kobjects to attach attributes,
>> nor do we need suspend/resume hooks, or any other features of device
>> core.
>>
>> Signed-off-by: Dmitry Torokhov 
>
> One reason might be to use devm_ functions for memory allocations to
> simplify error handling and cleanup. Without that, I agree.
>
> Reviewed-by: Guenter Roeck 
>
>> ---
>>  drivers/firmware/google/vpd.c | 44 
>> ---
>>  1 file changed, 16 insertions(+), 28 deletions(-)
>>
>> diff --git a/drivers/firmware/google/vpd.c b/drivers/firmware/google/vpd.c
>> index 78945729388e..f4766bf6785a 100644
>> --- a/drivers/firmware/google/vpd.c
>> +++ b/drivers/firmware/google/vpd.c
>> @@ -22,8 +22,6 @@
>>  #include 
>>  #include 
>>  #include 
>> -#include 
>> -#include 
>>  #include 
>>  #include 
>>
>> @@ -279,47 +277,37 @@ static int vpd_sections_init(phys_addr_t physaddr)
>> ret = vpd_section_init("rw", _vpd,
>>physaddr + sizeof(struct vpd_cbmem) +
>>header.ro_size, header.rw_size);
>> -   if (ret)
>> +   if (ret) {
>> +   vpd_section_destroy(_vpd);
>> return ret;
>> +   }
>> }
>>
>> return 0;
>>  }
>>
>> -static int vpd_probe(struct platform_device *pdev)
>> -{
>> -   int ret;
>> -   struct lb_cbmem_ref entry;
>> -
>> -   ret = coreboot_table_find(CB_TAG_VPD, , sizeof(entry));
>> -   if (ret)
>> -   return ret;
>> -
>> -   return vpd_sections_init(entry.cbmem_addr);
>> -}
>> -
>> -static struct platform_driver vpd_driver = {
>> -   .probe = vpd_probe,
>> -   .driver = {
>> -   .name = "vpd",
>> -   },
>> -};
>> -
>>  static int __init vpd_platform_init(void)
>>  {
>> -   struct platform_device *pdev;
>> -
>> -   pdev = platform_device_register_simple("vpd", -1, NULL, 0);
>> -   if (IS_ERR(pdev))
>> -   return PTR_ERR(pdev);
>> +   struct lb_cbmem_ref entry;
>> +   int err;
>>
>> vpd_kobj = kobject_create_and_add("vpd", firmware_kobj);
>> if (!vpd_kobj)
>> return -ENOMEM;
>>
>> -   platform_driver_register(_driver);
>> +   err = coreboot_table_find(CB_TAG_VPD, , sizeof(entry));
>> +   if (err)
>> +   goto err_kobject_put;
>> +
>> +   err = vpd_sections_init(entry.cbmem_addr);
>> +   if (err)
>> +   goto err_kobject_put;
>>
>> return 0;
>> +
>> +err_kobject_put:
>> +   kobject_put(vpd_kobj);
>> +   return err;
>>  }
>>
>>  static void __exit vpd_platform_exit(void)
>> --
>> 2.13.0.219.gdb65acc882-goog
>>


smime.p7s
Description: S/MIME Cryptographic Signature

Re: [PATCH] firmware: google: memconsole: Prevent overrun attack on coreboot console

[Julius Werner]

Sorry. Resent.

Re: [PATCH] firmware: google: memconsole: Prevent overrun attack on coreboot console

[Julius Werner]

Sorry. Resent.

[PATCH v2] firmware: google: memconsole: Prevent overrun attack on coreboot console

[Julius Werner]

The recent coreboot memory console update (firmware: google: memconsole:
Adapt to new coreboot ring buffer format) introduced a small security
issue in the driver: The new driver implementation parses the memory
console structure again on every access. This is intentional so that
additional lines added concurrently by runtime firmware can be read out.

However, if an attacker can write to the structure, they could increase
the size value to a point where the driver would read potentially
sensitive memory areas from outside the original console buffer during
the next access. This can be done through /dev/mem, since the console
buffer usually resides in firmware-reserved memory that is not covered
by STRICT_DEVMEM.

This patch resolves that problem by reading the buffer's size value only
once during boot (where we can still trust the structure). Other parts
of the structure can still be modified at runtime, but the driver's
bounds checks make sure that it will never read outside the buffer.

Fixes: a5061d028 ("firmware: google: memconsole: Adapt to new coreboot
ring buffer format")
Signed-off-by: Julius Werner <jwer...@chromium.org>
---
 drivers/firmware/google/memconsole-coreboot.c | 9 ++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/firmware/google/memconsole-coreboot.c 
b/drivers/firmware/google/memconsole-coreboot.c
index 7d39f4ef5d9e..52738887735c 100644
--- a/drivers/firmware/google/memconsole-coreboot.c
+++ b/drivers/firmware/google/memconsole-coreboot.c
@@ -26,7 +26,7 @@
 
 /* CBMEM firmware console log descriptor. */
 struct cbmem_cons {
-   u32 size;
+   u32 size_dont_access_after_boot;
u32 cursor;
u8  body[0];
 } __packed;
@@ -35,6 +35,7 @@ struct cbmem_cons {
 #define OVERFLOW (1 << 31)
 
 static struct cbmem_cons __iomem *cbmem_console;
+static u32 cbmem_console_size;
 
 /*
  * The cbmem_console structure is read again on every access because it may
@@ -47,7 +48,7 @@ static ssize_t memconsole_coreboot_read(char *buf, loff_t 
pos, size_t count)
 {
u32 cursor = cbmem_console->cursor & CURSOR_MASK;
u32 flags = cbmem_console->cursor & ~CURSOR_MASK;
-   u32 size = cbmem_console->size;
+   u32 size = cbmem_console_size;
struct seg {/* describes ring buffer segments in logical order */
u32 phys;   /* physical offset from start of mem buffer */
u32 len;/* length of segment */
@@ -81,8 +82,10 @@ static int memconsole_coreboot_init(phys_addr_t physaddr)
if (!tmp_cbmc)
return -ENOMEM;
 
+   /* Read size only once to prevent overrun attack through /dev/mem. */
+   cbmem_console_size = tmp_cbmc->size_dont_access_after_boot;
cbmem_console = memremap(physaddr,
-tmp_cbmc->size + sizeof(*cbmem_console),
+cbmem_console_size + sizeof(*cbmem_console),
 MEMREMAP_WB);
memunmap(tmp_cbmc);
 
-- 
2.12.2

[PATCH v2] firmware: google: memconsole: Prevent overrun attack on coreboot console

[Julius Werner]

The recent coreboot memory console update (firmware: google: memconsole:
Adapt to new coreboot ring buffer format) introduced a small security
issue in the driver: The new driver implementation parses the memory
console structure again on every access. This is intentional so that
additional lines added concurrently by runtime firmware can be read out.

However, if an attacker can write to the structure, they could increase
the size value to a point where the driver would read potentially
sensitive memory areas from outside the original console buffer during
the next access. This can be done through /dev/mem, since the console
buffer usually resides in firmware-reserved memory that is not covered
by STRICT_DEVMEM.

This patch resolves that problem by reading the buffer's size value only
once during boot (where we can still trust the structure). Other parts
of the structure can still be modified at runtime, but the driver's
bounds checks make sure that it will never read outside the buffer.

Fixes: a5061d028 ("firmware: google: memconsole: Adapt to new coreboot
ring buffer format")
Signed-off-by: Julius Werner 
---
 drivers/firmware/google/memconsole-coreboot.c | 9 ++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/firmware/google/memconsole-coreboot.c 
b/drivers/firmware/google/memconsole-coreboot.c
index 7d39f4ef5d9e..52738887735c 100644
--- a/drivers/firmware/google/memconsole-coreboot.c
+++ b/drivers/firmware/google/memconsole-coreboot.c
@@ -26,7 +26,7 @@
 
 /* CBMEM firmware console log descriptor. */
 struct cbmem_cons {
-   u32 size;
+   u32 size_dont_access_after_boot;
u32 cursor;
u8  body[0];
 } __packed;
@@ -35,6 +35,7 @@ struct cbmem_cons {
 #define OVERFLOW (1 << 31)
 
 static struct cbmem_cons __iomem *cbmem_console;
+static u32 cbmem_console_size;
 
 /*
  * The cbmem_console structure is read again on every access because it may
@@ -47,7 +48,7 @@ static ssize_t memconsole_coreboot_read(char *buf, loff_t 
pos, size_t count)
 {
u32 cursor = cbmem_console->cursor & CURSOR_MASK;
u32 flags = cbmem_console->cursor & ~CURSOR_MASK;
-   u32 size = cbmem_console->size;
+   u32 size = cbmem_console_size;
struct seg {/* describes ring buffer segments in logical order */
u32 phys;   /* physical offset from start of mem buffer */
u32 len;/* length of segment */
@@ -81,8 +82,10 @@ static int memconsole_coreboot_init(phys_addr_t physaddr)
if (!tmp_cbmc)
return -ENOMEM;
 
+   /* Read size only once to prevent overrun attack through /dev/mem. */
+   cbmem_console_size = tmp_cbmc->size_dont_access_after_boot;
cbmem_console = memremap(physaddr,
-tmp_cbmc->size + sizeof(*cbmem_console),
+cbmem_console_size + sizeof(*cbmem_console),
 MEMREMAP_WB);
memunmap(tmp_cbmc);
 
-- 
2.12.2

Re: [PATCH] firmware: google: memconsole: Prevent overrun attack on coreboot console

[Julius Werner]

Fixes: a5061d028 ("firmware: google: memconsole: Adapt to new coreboot
ring buffer format")

On Sat, May 20, 2017 at 1:32 AM, Greg Kroah-Hartman
<gre...@linuxfoundation.org> wrote:
> On Fri, May 19, 2017 at 02:44:38PM -0700, Julius Werner wrote:
>> The recent coreboot memory console update (firmware: google: memconsole:
>> Adapt to new coreboot ring buffer format) introduced a small security
>> issue in the driver: The new driver implementation parses the memory
>> console structure again on every access. This is intentional so that
>> additional lines added concurrently by runtime firmware can be read out.
>>
>> However, if an attacker can write to the structure, they could increase
>> the size value to a point where the driver would read potentially
>> sensitive memory areas from outside the original console buffer during
>> the next access. This can be done through /dev/mem, since the console
>> buffer usually resides in firmware-reserved memory that is not covered
>> by STRICT_DEVMEM.
>>
>> This patch resolves that problem by reading the buffer's size value only
>> once during boot (where we can still trust the structure). Other parts
>> of the structure can still be modified at runtime, but the driver's
>> bounds checks make sure that it will never read outside the buffer.
>>
>> Signed-off-by: Julius Werner <jwer...@chromium.org>
>
> Care to provide a "Fixes: SHA" type line here saying what commit this
> fixes the issue for, to allow people to know what is going on.
>
> thanks,
>
> greg k-h

Re: [PATCH] firmware: google: memconsole: Prevent overrun attack on coreboot console

[Julius Werner]

Fixes: a5061d028 ("firmware: google: memconsole: Adapt to new coreboot
ring buffer format")

On Sat, May 20, 2017 at 1:32 AM, Greg Kroah-Hartman
 wrote:
> On Fri, May 19, 2017 at 02:44:38PM -0700, Julius Werner wrote:
>> The recent coreboot memory console update (firmware: google: memconsole:
>> Adapt to new coreboot ring buffer format) introduced a small security
>> issue in the driver: The new driver implementation parses the memory
>> console structure again on every access. This is intentional so that
>> additional lines added concurrently by runtime firmware can be read out.
>>
>> However, if an attacker can write to the structure, they could increase
>> the size value to a point where the driver would read potentially
>> sensitive memory areas from outside the original console buffer during
>> the next access. This can be done through /dev/mem, since the console
>> buffer usually resides in firmware-reserved memory that is not covered
>> by STRICT_DEVMEM.
>>
>> This patch resolves that problem by reading the buffer's size value only
>> once during boot (where we can still trust the structure). Other parts
>> of the structure can still be modified at runtime, but the driver's
>> bounds checks make sure that it will never read outside the buffer.
>>
>> Signed-off-by: Julius Werner 
>
> Care to provide a "Fixes: SHA" type line here saying what commit this
> fixes the issue for, to allow people to know what is going on.
>
> thanks,
>
> greg k-h

[PATCH] firmware: google: memconsole: Prevent overrun attack on coreboot console

[Julius Werner]

The recent coreboot memory console update (firmware: google: memconsole:
Adapt to new coreboot ring buffer format) introduced a small security
issue in the driver: The new driver implementation parses the memory
console structure again on every access. This is intentional so that
additional lines added concurrently by runtime firmware can be read out.

However, if an attacker can write to the structure, they could increase
the size value to a point where the driver would read potentially
sensitive memory areas from outside the original console buffer during
the next access. This can be done through /dev/mem, since the console
buffer usually resides in firmware-reserved memory that is not covered
by STRICT_DEVMEM.

This patch resolves that problem by reading the buffer's size value only
once during boot (where we can still trust the structure). Other parts
of the structure can still be modified at runtime, but the driver's
bounds checks make sure that it will never read outside the buffer.

Signed-off-by: Julius Werner <jwer...@chromium.org>
---
 drivers/firmware/google/memconsole-coreboot.c | 9 ++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/firmware/google/memconsole-coreboot.c 
b/drivers/firmware/google/memconsole-coreboot.c
index 7d39f4ef5d9e..52738887735c 100644
--- a/drivers/firmware/google/memconsole-coreboot.c
+++ b/drivers/firmware/google/memconsole-coreboot.c
@@ -26,7 +26,7 @@
 
 /* CBMEM firmware console log descriptor. */
 struct cbmem_cons {
-   u32 size;
+   u32 size_dont_access_after_boot;
u32 cursor;
u8  body[0];
 } __packed;
@@ -35,6 +35,7 @@ struct cbmem_cons {
 #define OVERFLOW (1 << 31)
 
 static struct cbmem_cons __iomem *cbmem_console;
+static u32 cbmem_console_size;
 
 /*
  * The cbmem_console structure is read again on every access because it may
@@ -47,7 +48,7 @@ static ssize_t memconsole_coreboot_read(char *buf, loff_t 
pos, size_t count)
 {
u32 cursor = cbmem_console->cursor & CURSOR_MASK;
u32 flags = cbmem_console->cursor & ~CURSOR_MASK;
-   u32 size = cbmem_console->size;
+   u32 size = cbmem_console_size;
struct seg {/* describes ring buffer segments in logical order */
u32 phys;   /* physical offset from start of mem buffer */
u32 len;/* length of segment */
@@ -81,8 +82,10 @@ static int memconsole_coreboot_init(phys_addr_t physaddr)
if (!tmp_cbmc)
return -ENOMEM;
 
+   /* Read size only once to prevent overrun attack through /dev/mem. */
+   cbmem_console_size = tmp_cbmc->size_dont_access_after_boot;
cbmem_console = memremap(physaddr,
-tmp_cbmc->size + sizeof(*cbmem_console),
+cbmem_console_size + sizeof(*cbmem_console),
 MEMREMAP_WB);
memunmap(tmp_cbmc);
 
-- 
2.12.2

[PATCH] firmware: google: memconsole: Prevent overrun attack on coreboot console

[Julius Werner]

The recent coreboot memory console update (firmware: google: memconsole:
Adapt to new coreboot ring buffer format) introduced a small security
issue in the driver: The new driver implementation parses the memory
console structure again on every access. This is intentional so that
additional lines added concurrently by runtime firmware can be read out.

However, if an attacker can write to the structure, they could increase
the size value to a point where the driver would read potentially
sensitive memory areas from outside the original console buffer during
the next access. This can be done through /dev/mem, since the console
buffer usually resides in firmware-reserved memory that is not covered
by STRICT_DEVMEM.

This patch resolves that problem by reading the buffer's size value only
once during boot (where we can still trust the structure). Other parts
of the structure can still be modified at runtime, but the driver's
bounds checks make sure that it will never read outside the buffer.

Signed-off-by: Julius Werner 
---
 drivers/firmware/google/memconsole-coreboot.c | 9 ++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/firmware/google/memconsole-coreboot.c 
b/drivers/firmware/google/memconsole-coreboot.c
index 7d39f4ef5d9e..52738887735c 100644
--- a/drivers/firmware/google/memconsole-coreboot.c
+++ b/drivers/firmware/google/memconsole-coreboot.c
@@ -26,7 +26,7 @@
 
 /* CBMEM firmware console log descriptor. */
 struct cbmem_cons {
-   u32 size;
+   u32 size_dont_access_after_boot;
u32 cursor;
u8  body[0];
 } __packed;
@@ -35,6 +35,7 @@ struct cbmem_cons {
 #define OVERFLOW (1 << 31)
 
 static struct cbmem_cons __iomem *cbmem_console;
+static u32 cbmem_console_size;
 
 /*
  * The cbmem_console structure is read again on every access because it may
@@ -47,7 +48,7 @@ static ssize_t memconsole_coreboot_read(char *buf, loff_t 
pos, size_t count)
 {
u32 cursor = cbmem_console->cursor & CURSOR_MASK;
u32 flags = cbmem_console->cursor & ~CURSOR_MASK;
-   u32 size = cbmem_console->size;
+   u32 size = cbmem_console_size;
struct seg {/* describes ring buffer segments in logical order */
u32 phys;   /* physical offset from start of mem buffer */
u32 len;/* length of segment */
@@ -81,8 +82,10 @@ static int memconsole_coreboot_init(phys_addr_t physaddr)
if (!tmp_cbmc)
return -ENOMEM;
 
+   /* Read size only once to prevent overrun attack through /dev/mem. */
+   cbmem_console_size = tmp_cbmc->size_dont_access_after_boot;
cbmem_console = memremap(physaddr,
-tmp_cbmc->size + sizeof(*cbmem_console),
+cbmem_console_size + sizeof(*cbmem_console),
 MEMREMAP_WB);
memunmap(tmp_cbmc);
 
-- 
2.12.2

[PATCH] drivers: char: mem: Check for address space wraparound with mmap()

[Julius Werner]

/dev/mem currently allows mmap() mappings that wrap around the end of
the physical address space, which should probably be illegal. It
circumvents the existing STRICT_DEVMEM permission check because the loop
immediately terminates (as the start address is already higher than the
end address). On the x86_64 architecture it will then cause a panic
(from the BUG(start >= end) in arch/x86/mm/pat.c:reserve_memtype()).

This patch adds an explicit check to make sure offset + size will not
wrap around in the physical address type.

Signed-off-by: Julius Werner <jwer...@chromium.org>
---
 drivers/char/mem.c | 5 +
 1 file changed, 5 insertions(+)

diff --git a/drivers/char/mem.c b/drivers/char/mem.c
index 7e4a9d1296bb..6e0cbe092220 100644
--- a/drivers/char/mem.c
+++ b/drivers/char/mem.c
@@ -340,6 +340,11 @@ static const struct vm_operations_struct mmap_mem_ops = {
 static int mmap_mem(struct file *file, struct vm_area_struct *vma)
 {
size_t size = vma->vm_end - vma->vm_start;
+   phys_addr_t offset = (phys_addr_t)vma->vm_pgoff << PAGE_SHIFT;
+
+   /* It's illegal to wrap around the end of the physical address space. */
+   if (offset + (phys_addr_t)size < offset)
+   return -EINVAL;
 
if (!valid_mmap_phys_addr_range(vma->vm_pgoff, size))
return -EINVAL;
-- 
2.12.2

[PATCH] drivers: char: mem: Check for address space wraparound with mmap()

[Julius Werner]

/dev/mem currently allows mmap() mappings that wrap around the end of
the physical address space, which should probably be illegal. It
circumvents the existing STRICT_DEVMEM permission check because the loop
immediately terminates (as the start address is already higher than the
end address). On the x86_64 architecture it will then cause a panic
(from the BUG(start >= end) in arch/x86/mm/pat.c:reserve_memtype()).

This patch adds an explicit check to make sure offset + size will not
wrap around in the physical address type.

Signed-off-by: Julius Werner 
---
 drivers/char/mem.c | 5 +
 1 file changed, 5 insertions(+)

diff --git a/drivers/char/mem.c b/drivers/char/mem.c
index 7e4a9d1296bb..6e0cbe092220 100644
--- a/drivers/char/mem.c
+++ b/drivers/char/mem.c
@@ -340,6 +340,11 @@ static const struct vm_operations_struct mmap_mem_ops = {
 static int mmap_mem(struct file *file, struct vm_area_struct *vma)
 {
size_t size = vma->vm_end - vma->vm_start;
+   phys_addr_t offset = (phys_addr_t)vma->vm_pgoff << PAGE_SHIFT;
+
+   /* It's illegal to wrap around the end of the physical address space. */
+   if (offset + (phys_addr_t)size < offset)
+   return -EINVAL;
 
if (!valid_mmap_phys_addr_range(vma->vm_pgoff, size))
return -EINVAL;
-- 
2.12.2

[PATCH 2/2] firmware: google: memconsole: Adapt to new coreboot ring buffer format

[Julius Werner]

The upstream coreboot implementation of memconsole was enhanced from a
single-boot console to a persistent ring buffer
(https://review.coreboot.org/#/c/18301). This patch changes the kernel
memconsole driver to be able to read the new format in all cases.

Signed-off-by: Julius Werner <jwer...@chromium.org>
---
 drivers/firmware/google/memconsole-coreboot.c | 47 ++-
 1 file changed, 39 insertions(+), 8 deletions(-)

diff --git a/drivers/firmware/google/memconsole-coreboot.c 
b/drivers/firmware/google/memconsole-coreboot.c
index b6234e61004d..23dc363b8519 100644
--- a/drivers/firmware/google/memconsole-coreboot.c
+++ b/drivers/firmware/google/memconsole-coreboot.c
@@ -26,19 +26,50 @@
 
 /* CBMEM firmware console log descriptor. */
 struct cbmem_cons {
-   u32 buffer_size;
-   u32 buffer_cursor;
-   u8  buffer_body[0];
+   u32 size;
+   u32 cursor;
+   u8  body[0];
 } __packed;
 
+#define CURSOR_MASK ((1 << 28) - 1)
+#define OVERFLOW (1 << 31)
+
 static struct cbmem_cons __iomem *cbmem_console;
 
+/*
+ * The cbmem_console structure is read again on every access because it may
+ * change at any time if runtime firmware logs new messages. This may rarely
+ * lead to race conditions where the firmware overwrites the beginning of the
+ * ring buffer with more lines after we have already read |cursor|. It should 
be
+ * rare and harmless enough that we don't spend extra effort working around it.
+ */
 static ssize_t memconsole_coreboot_read(char *buf, loff_t pos, size_t count)
 {
-   return memory_read_from_buffer(buf, count, ,
-  cbmem_console->buffer_body,
-  min(cbmem_console->buffer_cursor,
-  cbmem_console->buffer_size));
+   u32 cursor = cbmem_console->cursor & CURSOR_MASK;
+   u32 flags = cbmem_console->cursor & ~CURSOR_MASK;
+   u32 size = cbmem_console->size;
+   struct seg {/* describes ring buffer segments in logical order */
+   u32 phys;   /* physical offset from start of mem buffer */
+   u32 len;/* length of segment */
+   } seg[2] = { {0}, {0} };
+   size_t done = 0;
+   int i;
+
+   if (flags & OVERFLOW) {
+   if (cursor > size)  /* Shouldn't really happen, but... */
+   cursor = 0;
+   seg[0] = (struct seg){.phys = cursor, .len = size - cursor};
+   seg[1] = (struct seg){.phys = 0, .len = cursor};
+   } else {
+   seg[0] = (struct seg){.phys = 0, .len = min(cursor, size)};
+   }
+
+   for (i = 0; i < ARRAY_SIZE(seg) && count > done; i++) {
+   done += memory_read_from_buffer(buf + done, count - done, ,
+   cbmem_console->body + seg[i].phys, seg[i].len);
+   pos -= seg[i].len;
+   }
+   return done;
 }
 
 static int memconsole_coreboot_init(phys_addr_t physaddr)
@@ -51,7 +82,7 @@ static int memconsole_coreboot_init(phys_addr_t physaddr)
return -ENOMEM;
 
cbmem_console = memremap(physaddr,
-tmp_cbmc->buffer_size + sizeof(*cbmem_console),
+tmp_cbmc->size + sizeof(*cbmem_console),
 MEMREMAP_WB);
memunmap(tmp_cbmc);
 
-- 
2.12.2

[PATCH 2/2] firmware: google: memconsole: Adapt to new coreboot ring buffer format

[Julius Werner]

The upstream coreboot implementation of memconsole was enhanced from a
single-boot console to a persistent ring buffer
(https://review.coreboot.org/#/c/18301). This patch changes the kernel
memconsole driver to be able to read the new format in all cases.

Signed-off-by: Julius Werner 
---
 drivers/firmware/google/memconsole-coreboot.c | 47 ++-
 1 file changed, 39 insertions(+), 8 deletions(-)

diff --git a/drivers/firmware/google/memconsole-coreboot.c 
b/drivers/firmware/google/memconsole-coreboot.c
index b6234e61004d..23dc363b8519 100644
--- a/drivers/firmware/google/memconsole-coreboot.c
+++ b/drivers/firmware/google/memconsole-coreboot.c
@@ -26,19 +26,50 @@
 
 /* CBMEM firmware console log descriptor. */
 struct cbmem_cons {
-   u32 buffer_size;
-   u32 buffer_cursor;
-   u8  buffer_body[0];
+   u32 size;
+   u32 cursor;
+   u8  body[0];
 } __packed;
 
+#define CURSOR_MASK ((1 << 28) - 1)
+#define OVERFLOW (1 << 31)
+
 static struct cbmem_cons __iomem *cbmem_console;
 
+/*
+ * The cbmem_console structure is read again on every access because it may
+ * change at any time if runtime firmware logs new messages. This may rarely
+ * lead to race conditions where the firmware overwrites the beginning of the
+ * ring buffer with more lines after we have already read |cursor|. It should 
be
+ * rare and harmless enough that we don't spend extra effort working around it.
+ */
 static ssize_t memconsole_coreboot_read(char *buf, loff_t pos, size_t count)
 {
-   return memory_read_from_buffer(buf, count, ,
-  cbmem_console->buffer_body,
-  min(cbmem_console->buffer_cursor,
-  cbmem_console->buffer_size));
+   u32 cursor = cbmem_console->cursor & CURSOR_MASK;
+   u32 flags = cbmem_console->cursor & ~CURSOR_MASK;
+   u32 size = cbmem_console->size;
+   struct seg {/* describes ring buffer segments in logical order */
+   u32 phys;   /* physical offset from start of mem buffer */
+   u32 len;/* length of segment */
+   } seg[2] = { {0}, {0} };
+   size_t done = 0;
+   int i;
+
+   if (flags & OVERFLOW) {
+   if (cursor > size)  /* Shouldn't really happen, but... */
+   cursor = 0;
+   seg[0] = (struct seg){.phys = cursor, .len = size - cursor};
+   seg[1] = (struct seg){.phys = 0, .len = cursor};
+   } else {
+   seg[0] = (struct seg){.phys = 0, .len = min(cursor, size)};
+   }
+
+   for (i = 0; i < ARRAY_SIZE(seg) && count > done; i++) {
+   done += memory_read_from_buffer(buf + done, count - done, ,
+   cbmem_console->body + seg[i].phys, seg[i].len);
+   pos -= seg[i].len;
+   }
+   return done;
 }
 
 static int memconsole_coreboot_init(phys_addr_t physaddr)
@@ -51,7 +82,7 @@ static int memconsole_coreboot_init(phys_addr_t physaddr)
return -ENOMEM;
 
cbmem_console = memremap(physaddr,
-tmp_cbmc->buffer_size + sizeof(*cbmem_console),
+tmp_cbmc->size + sizeof(*cbmem_console),
 MEMREMAP_WB);
memunmap(tmp_cbmc);
 
-- 
2.12.2

[PATCH 1/2] firmware: google: memconsole: Make memconsole interface more flexible

[Julius Werner]

This patch redesigns the interface between the generic memconsole driver
and its implementations to become more flexible than a flat memory
buffer with unchanging bounds. This allows memconsoles like coreboot's
to include lines that were added by runtime firmware after the driver
was initialized. Since the console log size is thus no longer static,
this means that the /sys/firmware/log file has to become unseekable.

Signed-off-by: Julius Werner <jwer...@chromium.org>
---
 drivers/firmware/google/memconsole-coreboot.c   | 12 +---
 drivers/firmware/google/memconsole-x86-legacy.c | 18 +++---
 drivers/firmware/google/memconsole.c| 14 ++
 drivers/firmware/google/memconsole.h|  7 ---
 4 files changed, 34 insertions(+), 17 deletions(-)

diff --git a/drivers/firmware/google/memconsole-coreboot.c 
b/drivers/firmware/google/memconsole-coreboot.c
index 21210144def7..b6234e61004d 100644
--- a/drivers/firmware/google/memconsole-coreboot.c
+++ b/drivers/firmware/google/memconsole-coreboot.c
@@ -33,6 +33,14 @@ struct cbmem_cons {
 
 static struct cbmem_cons __iomem *cbmem_console;
 
+static ssize_t memconsole_coreboot_read(char *buf, loff_t pos, size_t count)
+{
+   return memory_read_from_buffer(buf, count, ,
+  cbmem_console->buffer_body,
+  min(cbmem_console->buffer_cursor,
+  cbmem_console->buffer_size));
+}
+
 static int memconsole_coreboot_init(phys_addr_t physaddr)
 {
struct cbmem_cons __iomem *tmp_cbmc;
@@ -50,9 +58,7 @@ static int memconsole_coreboot_init(phys_addr_t physaddr)
if (!cbmem_console)
return -ENOMEM;
 
-   memconsole_setup(cbmem_console->buffer_body,
-   min(cbmem_console->buffer_cursor, cbmem_console->buffer_size));
-
+   memconsole_setup(memconsole_coreboot_read);
return 0;
 }
 
diff --git a/drivers/firmware/google/memconsole-x86-legacy.c 
b/drivers/firmware/google/memconsole-x86-legacy.c
index 529078c62488..1b556e944599 100644
--- a/drivers/firmware/google/memconsole-x86-legacy.c
+++ b/drivers/firmware/google/memconsole-x86-legacy.c
@@ -49,6 +49,15 @@ struct biosmemcon_ebda {
};
 } __packed;
 
+static char *memconsole_baseaddr;
+static size_t memconsole_length;
+
+static ssize_t memconsole_read(char *buf, loff_t pos, size_t count)
+{
+   return memory_read_from_buffer(buf, count, , memconsole_baseaddr,
+  memconsole_length);
+}
+
 static void found_v1_header(struct biosmemcon_ebda *hdr)
 {
pr_info("memconsole: BIOS console v1 EBDA structure found at %p\n",
@@ -57,7 +66,9 @@ static void found_v1_header(struct biosmemcon_ebda *hdr)
hdr->v1.buffer_addr, hdr->v1.start,
hdr->v1.end, hdr->v1.num_chars);
 
-   memconsole_setup(phys_to_virt(hdr->v1.buffer_addr), hdr->v1.num_chars);
+   memconsole_baseaddr = phys_to_virt(hdr->v1.buffer_addr);
+   memconsole_length = hdr->v1.num_chars;
+   memconsole_setup(memconsole_read);
 }
 
 static void found_v2_header(struct biosmemcon_ebda *hdr)
@@ -68,8 +79,9 @@ static void found_v2_header(struct biosmemcon_ebda *hdr)
hdr->v2.buffer_addr, hdr->v2.start,
hdr->v2.end, hdr->v2.num_bytes);
 
-   memconsole_setup(phys_to_virt(hdr->v2.buffer_addr + hdr->v2.start),
-hdr->v2.end - hdr->v2.start);
+   memconsole_baseaddr = phys_to_virt(hdr->v2.buffer_addr + hdr->v2.start);
+   memconsole_length = hdr->v2.end - hdr->v2.start;
+   memconsole_setup(memconsole_read);
 }
 
 /*
diff --git a/drivers/firmware/google/memconsole.c 
b/drivers/firmware/google/memconsole.c
index 94e200ddb4fa..166f07c68c02 100644
--- a/drivers/firmware/google/memconsole.c
+++ b/drivers/firmware/google/memconsole.c
@@ -22,15 +22,15 @@
 
 #include "memconsole.h"
 
-static char *memconsole_baseaddr;
-static size_t memconsole_length;
+static ssize_t (*memconsole_read_func)(char *, loff_t, size_t);
 
 static ssize_t memconsole_read(struct file *filp, struct kobject *kobp,
   struct bin_attribute *bin_attr, char *buf,
   loff_t pos, size_t count)
 {
-   return memory_read_from_buffer(buf, count, , memconsole_baseaddr,
-  memconsole_length);
+   if (WARN_ON_ONCE(!memconsole_read_func))
+   return -EIO;
+   return memconsole_read_func(buf, pos, count);
 }
 
 static struct bin_attribute memconsole_bin_attr = {
@@ -38,16 +38,14 @@ static struct bin_attribute memconsole_bin_attr = {
.read = memconsole_read,
 };
 
-void memconsole_setup(void *baseaddr, size_t length)
+void memconsole_setup(ssize_t (*read_func)(char *, loff_t, size_t))
 {
-   memconsole_baseaddr = baseaddr;
-   

[PATCH v3 0/2] Memconsole changes for new coreboot format

[Julius Werner]

This series enhances the memconsole driver to work well with the new
persistent ring buffer console introduced in coreboot with
https://review.coreboot.org/#/c/18301. It needs to be applied on top of
the other memconsole patches currently queued up in char-misc-next
(ending in 049a59db34e).

Julius Werner (2):
  firmware: google: memconsole: Make memconsole interface more flexible
  firmware: google: memconsole: Adapt to new coreboot ring buffer format

 drivers/firmware/google/memconsole-coreboot.c   | 51 +
 drivers/firmware/google/memconsole-x86-legacy.c | 18 +++--
 drivers/firmware/google/memconsole.c| 14 +++
 drivers/firmware/google/memconsole.h|  7 ++--
 4 files changed, 69 insertions(+), 21 deletions(-)

-- 
2.12.2

[PATCH 1/2] firmware: google: memconsole: Make memconsole interface more flexible

[Julius Werner]

This patch redesigns the interface between the generic memconsole driver
and its implementations to become more flexible than a flat memory
buffer with unchanging bounds. This allows memconsoles like coreboot's
to include lines that were added by runtime firmware after the driver
was initialized. Since the console log size is thus no longer static,
this means that the /sys/firmware/log file has to become unseekable.

Signed-off-by: Julius Werner 
---
 drivers/firmware/google/memconsole-coreboot.c   | 12 +---
 drivers/firmware/google/memconsole-x86-legacy.c | 18 +++---
 drivers/firmware/google/memconsole.c| 14 ++
 drivers/firmware/google/memconsole.h|  7 ---
 4 files changed, 34 insertions(+), 17 deletions(-)

diff --git a/drivers/firmware/google/memconsole-coreboot.c 
b/drivers/firmware/google/memconsole-coreboot.c
index 21210144def7..b6234e61004d 100644
--- a/drivers/firmware/google/memconsole-coreboot.c
+++ b/drivers/firmware/google/memconsole-coreboot.c
@@ -33,6 +33,14 @@ struct cbmem_cons {
 
 static struct cbmem_cons __iomem *cbmem_console;
 
+static ssize_t memconsole_coreboot_read(char *buf, loff_t pos, size_t count)
+{
+   return memory_read_from_buffer(buf, count, ,
+  cbmem_console->buffer_body,
+  min(cbmem_console->buffer_cursor,
+  cbmem_console->buffer_size));
+}
+
 static int memconsole_coreboot_init(phys_addr_t physaddr)
 {
struct cbmem_cons __iomem *tmp_cbmc;
@@ -50,9 +58,7 @@ static int memconsole_coreboot_init(phys_addr_t physaddr)
if (!cbmem_console)
return -ENOMEM;
 
-   memconsole_setup(cbmem_console->buffer_body,
-   min(cbmem_console->buffer_cursor, cbmem_console->buffer_size));
-
+   memconsole_setup(memconsole_coreboot_read);
return 0;
 }
 
diff --git a/drivers/firmware/google/memconsole-x86-legacy.c 
b/drivers/firmware/google/memconsole-x86-legacy.c
index 529078c62488..1b556e944599 100644
--- a/drivers/firmware/google/memconsole-x86-legacy.c
+++ b/drivers/firmware/google/memconsole-x86-legacy.c
@@ -49,6 +49,15 @@ struct biosmemcon_ebda {
};
 } __packed;
 
+static char *memconsole_baseaddr;
+static size_t memconsole_length;
+
+static ssize_t memconsole_read(char *buf, loff_t pos, size_t count)
+{
+   return memory_read_from_buffer(buf, count, , memconsole_baseaddr,
+  memconsole_length);
+}
+
 static void found_v1_header(struct biosmemcon_ebda *hdr)
 {
pr_info("memconsole: BIOS console v1 EBDA structure found at %p\n",
@@ -57,7 +66,9 @@ static void found_v1_header(struct biosmemcon_ebda *hdr)
hdr->v1.buffer_addr, hdr->v1.start,
hdr->v1.end, hdr->v1.num_chars);
 
-   memconsole_setup(phys_to_virt(hdr->v1.buffer_addr), hdr->v1.num_chars);
+   memconsole_baseaddr = phys_to_virt(hdr->v1.buffer_addr);
+   memconsole_length = hdr->v1.num_chars;
+   memconsole_setup(memconsole_read);
 }
 
 static void found_v2_header(struct biosmemcon_ebda *hdr)
@@ -68,8 +79,9 @@ static void found_v2_header(struct biosmemcon_ebda *hdr)
hdr->v2.buffer_addr, hdr->v2.start,
hdr->v2.end, hdr->v2.num_bytes);
 
-   memconsole_setup(phys_to_virt(hdr->v2.buffer_addr + hdr->v2.start),
-hdr->v2.end - hdr->v2.start);
+   memconsole_baseaddr = phys_to_virt(hdr->v2.buffer_addr + hdr->v2.start);
+   memconsole_length = hdr->v2.end - hdr->v2.start;
+   memconsole_setup(memconsole_read);
 }
 
 /*
diff --git a/drivers/firmware/google/memconsole.c 
b/drivers/firmware/google/memconsole.c
index 94e200ddb4fa..166f07c68c02 100644
--- a/drivers/firmware/google/memconsole.c
+++ b/drivers/firmware/google/memconsole.c
@@ -22,15 +22,15 @@
 
 #include "memconsole.h"
 
-static char *memconsole_baseaddr;
-static size_t memconsole_length;
+static ssize_t (*memconsole_read_func)(char *, loff_t, size_t);
 
 static ssize_t memconsole_read(struct file *filp, struct kobject *kobp,
   struct bin_attribute *bin_attr, char *buf,
   loff_t pos, size_t count)
 {
-   return memory_read_from_buffer(buf, count, , memconsole_baseaddr,
-  memconsole_length);
+   if (WARN_ON_ONCE(!memconsole_read_func))
+   return -EIO;
+   return memconsole_read_func(buf, pos, count);
 }
 
 static struct bin_attribute memconsole_bin_attr = {
@@ -38,16 +38,14 @@ static struct bin_attribute memconsole_bin_attr = {
.read = memconsole_read,
 };
 
-void memconsole_setup(void *baseaddr, size_t length)
+void memconsole_setup(ssize_t (*read_func)(char *, loff_t, size_t))
 {
-   memconsole_baseaddr = baseaddr;
-   memconsole_leng

[PATCH v3 0/2] Memconsole changes for new coreboot format

[Julius Werner]

This series enhances the memconsole driver to work well with the new
persistent ring buffer console introduced in coreboot with
https://review.coreboot.org/#/c/18301. It needs to be applied on top of
the other memconsole patches currently queued up in char-misc-next
(ending in 049a59db34e).

Julius Werner (2):
  firmware: google: memconsole: Make memconsole interface more flexible
  firmware: google: memconsole: Adapt to new coreboot ring buffer format

 drivers/firmware/google/memconsole-coreboot.c   | 51 +
 drivers/firmware/google/memconsole-x86-legacy.c | 18 +++--
 drivers/firmware/google/memconsole.c| 14 +++
 drivers/firmware/google/memconsole.h|  7 ++--
 4 files changed, 69 insertions(+), 21 deletions(-)

-- 
2.12.2

Re: [PATCH v2 2/3] firmware: google: memconsole: Escape unprintable characters

[Julius Werner]

> Binary sysfs files are supposed to be "pass through" only, the kernel
> should not be touching the data at all, it's up to userspace to do what
> it wants to do with things.  So don't escape anything at all, that's not
> the kernel's job here.

Okay, I'll drop this patch.

Re: [PATCH v2 2/3] firmware: google: memconsole: Escape unprintable characters

[Julius Werner]

> Binary sysfs files are supposed to be "pass through" only, the kernel
> should not be touching the data at all, it's up to userspace to do what
> it wants to do with things.  So don't escape anything at all, that's not
> the kernel's job here.

Okay, I'll drop this patch.

Re: [PATCH v2 2/3] firmware: google: memconsole: Escape unprintable characters

[Julius Werner]

On Fri, Apr 28, 2017 at 10:37 PM, Greg Kroah-Hartman
<gre...@linuxfoundation.org> wrote:
> On Fri, Apr 28, 2017 at 01:42:24PM -0700, Julius Werner wrote:
>> Recent improvements in coreboot's memory console allow it to contain
>> logs from more than one boot as long as the information persists in
>> memory. Since trying to persist a memory buffer across reboots often
>> doesn't quite work perfectly it is now more likely for random bit flips
>> to occur in the console. With the current implementation this can lead
>> to stray control characters that cause weird effects for the most common
>> use cases (such as just dumping the console with 'cat').
>>
>> This patch changes the memconsole driver to replace unprintable
>> characters with '?' by default. It also adds a new /sys/firmware/rawlog
>> node next to the existing /sys/firmware/log for use cases where it's
>> desired to read the raw characters.
>
> Again, you are doing multiple things here.  Break it up.

Sorry, I'm not sure what else you want me to break up here? If I add
the escaping in one patch and then add the raw node in a different
patch, there's a gap between the two patches where we're losing
functionality. Isn't that undesirable?

> And, can userspace handle this change?  You are now changing the format
> of the existing file.

I'm escaping characters that previously hadn't been escaped, but
really, I'm just trying to make sure that people can continue to use
this node as before. The real change has already happened in coreboot,
outside of Linux. This node could previously be relied upon to only
contain printable characters, and with newer versions of coreboot
that's not always the case anymore. I chose to do it this way because
I thought it would be the least disruptive for most users.

It can be handled in userspace, yes. It depends on what you use to
read the file, mostly... people using less will be fine, but people
using cat may get weird behavior. You're right that it doesn't really
*need* to be escaped in the kernel, and honestly I don't care too
much... I can leave it out if you want. But I think this is the most
convenient way to do it for existing users.

>> - return sysfs_create_bin_file(firmware_kobj, _bin_attr);
>> + return sysfs_create_bin_file(firmware_kobj, _log_attr) ||
>> + sysfs_create_bin_file(firmware_kobj, _raw_attr);
>
> While it is really rare, please do this properly and create one file,
> and then the other, and provide proper error handling here (i.e. if the
> second fails, remove the first).

Okay, will send that with the next set once we agree on the other stuff.

Re: [PATCH v2 2/3] firmware: google: memconsole: Escape unprintable characters

[Julius Werner]

On Fri, Apr 28, 2017 at 10:37 PM, Greg Kroah-Hartman
 wrote:
> On Fri, Apr 28, 2017 at 01:42:24PM -0700, Julius Werner wrote:
>> Recent improvements in coreboot's memory console allow it to contain
>> logs from more than one boot as long as the information persists in
>> memory. Since trying to persist a memory buffer across reboots often
>> doesn't quite work perfectly it is now more likely for random bit flips
>> to occur in the console. With the current implementation this can lead
>> to stray control characters that cause weird effects for the most common
>> use cases (such as just dumping the console with 'cat').
>>
>> This patch changes the memconsole driver to replace unprintable
>> characters with '?' by default. It also adds a new /sys/firmware/rawlog
>> node next to the existing /sys/firmware/log for use cases where it's
>> desired to read the raw characters.
>
> Again, you are doing multiple things here.  Break it up.

Sorry, I'm not sure what else you want me to break up here? If I add
the escaping in one patch and then add the raw node in a different
patch, there's a gap between the two patches where we're losing
functionality. Isn't that undesirable?

> And, can userspace handle this change?  You are now changing the format
> of the existing file.

I'm escaping characters that previously hadn't been escaped, but
really, I'm just trying to make sure that people can continue to use
this node as before. The real change has already happened in coreboot,
outside of Linux. This node could previously be relied upon to only
contain printable characters, and with newer versions of coreboot
that's not always the case anymore. I chose to do it this way because
I thought it would be the least disruptive for most users.

It can be handled in userspace, yes. It depends on what you use to
read the file, mostly... people using less will be fine, but people
using cat may get weird behavior. You're right that it doesn't really
*need* to be escaped in the kernel, and honestly I don't care too
much... I can leave it out if you want. But I think this is the most
convenient way to do it for existing users.

>> - return sysfs_create_bin_file(firmware_kobj, _bin_attr);
>> + return sysfs_create_bin_file(firmware_kobj, _log_attr) ||
>> + sysfs_create_bin_file(firmware_kobj, _raw_attr);
>
> While it is really rare, please do this properly and create one file,
> and then the other, and provide proper error handling here (i.e. if the
> second fails, remove the first).

Okay, will send that with the next set once we agree on the other stuff.

[PATCH v2 2/3] firmware: google: memconsole: Escape unprintable characters

[Julius Werner]

Recent improvements in coreboot's memory console allow it to contain
logs from more than one boot as long as the information persists in
memory. Since trying to persist a memory buffer across reboots often
doesn't quite work perfectly it is now more likely for random bit flips
to occur in the console. With the current implementation this can lead
to stray control characters that cause weird effects for the most common
use cases (such as just dumping the console with 'cat').

This patch changes the memconsole driver to replace unprintable
characters with '?' by default. It also adds a new /sys/firmware/rawlog
node next to the existing /sys/firmware/log for use cases where it's
desired to read the raw characters.

Signed-off-by: Julius Werner <jwer...@chromium.org>
---
 drivers/firmware/google/memconsole.c | 32 +++-
 1 file changed, 27 insertions(+), 5 deletions(-)

diff --git a/drivers/firmware/google/memconsole.c 
b/drivers/firmware/google/memconsole.c
index 166f07c68c02..807fcd4f7f85 100644
--- a/drivers/firmware/google/memconsole.c
+++ b/drivers/firmware/google/memconsole.c
@@ -15,6 +15,7 @@
  * GNU General Public License for more details.
  */
 
+#include 
 #include 
 #include 
 #include 
@@ -24,7 +25,7 @@
 
 static ssize_t (*memconsole_read_func)(char *, loff_t, size_t);
 
-static ssize_t memconsole_read(struct file *filp, struct kobject *kobp,
+static ssize_t memconsole_read_raw(struct file *filp, struct kobject *kobp,
   struct bin_attribute *bin_attr, char *buf,
   loff_t pos, size_t count)
 {
@@ -33,9 +34,28 @@ static ssize_t memconsole_read(struct file *filp, struct 
kobject *kobp,
return memconsole_read_func(buf, pos, count);
 }
 
-static struct bin_attribute memconsole_bin_attr = {
+static ssize_t memconsole_read_log(struct file *filp, struct kobject *kobp,
+  struct bin_attribute *bin_attr, char *buf,
+  loff_t pos, size_t count)
+{
+   ssize_t i;
+   ssize_t rv = memconsole_read_raw(filp, kobp, bin_attr, buf, pos, count);
+
+   if (rv > 0)
+   for (i = 0; i < rv; i++)
+   if (!isprint(buf[i]) && !isspace(buf[i]))
+   buf[i] = '?';
+   return rv;
+}
+
+/* Memconsoles may be much longer than 4K, so need to use binary attributes. */
+static struct bin_attribute memconsole_log_attr = {
.attr = {.name = "log", .mode = 0444},
-   .read = memconsole_read,
+   .read = memconsole_read_log,
+};
+static struct bin_attribute memconsole_raw_attr = {
+   .attr = {.name = "rawlog", .mode = 0444},
+   .read = memconsole_read_raw,
 };
 
 void memconsole_setup(ssize_t (*read_func)(char *, loff_t, size_t))
@@ -46,13 +66,15 @@ EXPORT_SYMBOL(memconsole_setup);
 
 int memconsole_sysfs_init(void)
 {
-   return sysfs_create_bin_file(firmware_kobj, _bin_attr);
+   return sysfs_create_bin_file(firmware_kobj, _log_attr) ||
+   sysfs_create_bin_file(firmware_kobj, _raw_attr);
 }
 EXPORT_SYMBOL(memconsole_sysfs_init);
 
 void memconsole_exit(void)
 {
-   sysfs_remove_bin_file(firmware_kobj, _bin_attr);
+   sysfs_remove_bin_file(firmware_kobj, _log_attr);
+   sysfs_remove_bin_file(firmware_kobj, _raw_attr);
 }
 EXPORT_SYMBOL(memconsole_exit);
 
-- 
2.12.2

[PATCH v2 2/3] firmware: google: memconsole: Escape unprintable characters

[Julius Werner]

Recent improvements in coreboot's memory console allow it to contain
logs from more than one boot as long as the information persists in
memory. Since trying to persist a memory buffer across reboots often
doesn't quite work perfectly it is now more likely for random bit flips
to occur in the console. With the current implementation this can lead
to stray control characters that cause weird effects for the most common
use cases (such as just dumping the console with 'cat').

This patch changes the memconsole driver to replace unprintable
characters with '?' by default. It also adds a new /sys/firmware/rawlog
node next to the existing /sys/firmware/log for use cases where it's
desired to read the raw characters.

Signed-off-by: Julius Werner 
---
 drivers/firmware/google/memconsole.c | 32 +++-
 1 file changed, 27 insertions(+), 5 deletions(-)

diff --git a/drivers/firmware/google/memconsole.c 
b/drivers/firmware/google/memconsole.c
index 166f07c68c02..807fcd4f7f85 100644
--- a/drivers/firmware/google/memconsole.c
+++ b/drivers/firmware/google/memconsole.c
@@ -15,6 +15,7 @@
  * GNU General Public License for more details.
  */
 
+#include 
 #include 
 #include 
 #include 
@@ -24,7 +25,7 @@
 
 static ssize_t (*memconsole_read_func)(char *, loff_t, size_t);
 
-static ssize_t memconsole_read(struct file *filp, struct kobject *kobp,
+static ssize_t memconsole_read_raw(struct file *filp, struct kobject *kobp,
   struct bin_attribute *bin_attr, char *buf,
   loff_t pos, size_t count)
 {
@@ -33,9 +34,28 @@ static ssize_t memconsole_read(struct file *filp, struct 
kobject *kobp,
return memconsole_read_func(buf, pos, count);
 }
 
-static struct bin_attribute memconsole_bin_attr = {
+static ssize_t memconsole_read_log(struct file *filp, struct kobject *kobp,
+  struct bin_attribute *bin_attr, char *buf,
+  loff_t pos, size_t count)
+{
+   ssize_t i;
+   ssize_t rv = memconsole_read_raw(filp, kobp, bin_attr, buf, pos, count);
+
+   if (rv > 0)
+   for (i = 0; i < rv; i++)
+   if (!isprint(buf[i]) && !isspace(buf[i]))
+   buf[i] = '?';
+   return rv;
+}
+
+/* Memconsoles may be much longer than 4K, so need to use binary attributes. */
+static struct bin_attribute memconsole_log_attr = {
.attr = {.name = "log", .mode = 0444},
-   .read = memconsole_read,
+   .read = memconsole_read_log,
+};
+static struct bin_attribute memconsole_raw_attr = {
+   .attr = {.name = "rawlog", .mode = 0444},
+   .read = memconsole_read_raw,
 };
 
 void memconsole_setup(ssize_t (*read_func)(char *, loff_t, size_t))
@@ -46,13 +66,15 @@ EXPORT_SYMBOL(memconsole_setup);
 
 int memconsole_sysfs_init(void)
 {
-   return sysfs_create_bin_file(firmware_kobj, _bin_attr);
+   return sysfs_create_bin_file(firmware_kobj, _log_attr) ||
+   sysfs_create_bin_file(firmware_kobj, _raw_attr);
 }
 EXPORT_SYMBOL(memconsole_sysfs_init);
 
 void memconsole_exit(void)
 {
-   sysfs_remove_bin_file(firmware_kobj, _bin_attr);
+   sysfs_remove_bin_file(firmware_kobj, _log_attr);
+   sysfs_remove_bin_file(firmware_kobj, _raw_attr);
 }
 EXPORT_SYMBOL(memconsole_exit);
 
-- 
2.12.2

[PATCH v2 3/3] firmware: google: memconsole: Adapt to new coreboot ring buffer format

[Julius Werner]

The upstream coreboot implementation of memconsole was enhanced from a
single-boot console to a persistent ring buffer
(https://review.coreboot.org/#/c/18301). This patch changes the kernel
memconsole driver to be able to read the new format in all cases.

Signed-off-by: Julius Werner <jwer...@chromium.org>
---
 drivers/firmware/google/memconsole-coreboot.c | 47 ++-
 1 file changed, 39 insertions(+), 8 deletions(-)

diff --git a/drivers/firmware/google/memconsole-coreboot.c 
b/drivers/firmware/google/memconsole-coreboot.c
index b6234e61004d..23dc363b8519 100644
--- a/drivers/firmware/google/memconsole-coreboot.c
+++ b/drivers/firmware/google/memconsole-coreboot.c
@@ -26,19 +26,50 @@
 
 /* CBMEM firmware console log descriptor. */
 struct cbmem_cons {
-   u32 buffer_size;
-   u32 buffer_cursor;
-   u8  buffer_body[0];
+   u32 size;
+   u32 cursor;
+   u8  body[0];
 } __packed;
 
+#define CURSOR_MASK ((1 << 28) - 1)
+#define OVERFLOW (1 << 31)
+
 static struct cbmem_cons __iomem *cbmem_console;
 
+/*
+ * The cbmem_console structure is read again on every access because it may
+ * change at any time if runtime firmware logs new messages. This may rarely
+ * lead to race conditions where the firmware overwrites the beginning of the
+ * ring buffer with more lines after we have already read |cursor|. It should 
be
+ * rare and harmless enough that we don't spend extra effort working around it.
+ */
 static ssize_t memconsole_coreboot_read(char *buf, loff_t pos, size_t count)
 {
-   return memory_read_from_buffer(buf, count, ,
-  cbmem_console->buffer_body,
-  min(cbmem_console->buffer_cursor,
-  cbmem_console->buffer_size));
+   u32 cursor = cbmem_console->cursor & CURSOR_MASK;
+   u32 flags = cbmem_console->cursor & ~CURSOR_MASK;
+   u32 size = cbmem_console->size;
+   struct seg {/* describes ring buffer segments in logical order */
+   u32 phys;   /* physical offset from start of mem buffer */
+   u32 len;/* length of segment */
+   } seg[2] = { {0}, {0} };
+   size_t done = 0;
+   int i;
+
+   if (flags & OVERFLOW) {
+   if (cursor > size)  /* Shouldn't really happen, but... */
+   cursor = 0;
+   seg[0] = (struct seg){.phys = cursor, .len = size - cursor};
+   seg[1] = (struct seg){.phys = 0, .len = cursor};
+   } else {
+   seg[0] = (struct seg){.phys = 0, .len = min(cursor, size)};
+   }
+
+   for (i = 0; i < ARRAY_SIZE(seg) && count > done; i++) {
+   done += memory_read_from_buffer(buf + done, count - done, ,
+   cbmem_console->body + seg[i].phys, seg[i].len);
+   pos -= seg[i].len;
+   }
+   return done;
 }
 
 static int memconsole_coreboot_init(phys_addr_t physaddr)
@@ -51,7 +82,7 @@ static int memconsole_coreboot_init(phys_addr_t physaddr)
return -ENOMEM;
 
cbmem_console = memremap(physaddr,
-tmp_cbmc->buffer_size + sizeof(*cbmem_console),
+tmp_cbmc->size + sizeof(*cbmem_console),
 MEMREMAP_WB);
memunmap(tmp_cbmc);
 
-- 
2.12.2

[PATCH v2 1/3] firmware: google: memconsole: Make memconsole interface more flexible

[Julius Werner]

This patch redesigns the interface between the generic memconsole driver
and its implementations to become more flexible than a flat memory
buffer with unchanging bounds. This allows memconsoles like coreboot's
to include lines that were added by runtime firmware after the driver
was initialized. Since the console log size is thus no longer static,
this means that the /sys/firmware/log file has to become unseekable.

Signed-off-by: Julius Werner <jwer...@chromium.org>
---
 drivers/firmware/google/memconsole-coreboot.c   | 12 +---
 drivers/firmware/google/memconsole-x86-legacy.c | 18 +++---
 drivers/firmware/google/memconsole.c| 14 ++
 drivers/firmware/google/memconsole.h|  7 ---
 4 files changed, 34 insertions(+), 17 deletions(-)

diff --git a/drivers/firmware/google/memconsole-coreboot.c 
b/drivers/firmware/google/memconsole-coreboot.c
index 21210144def7..b6234e61004d 100644
--- a/drivers/firmware/google/memconsole-coreboot.c
+++ b/drivers/firmware/google/memconsole-coreboot.c
@@ -33,6 +33,14 @@ struct cbmem_cons {
 
 static struct cbmem_cons __iomem *cbmem_console;
 
+static ssize_t memconsole_coreboot_read(char *buf, loff_t pos, size_t count)
+{
+   return memory_read_from_buffer(buf, count, ,
+  cbmem_console->buffer_body,
+  min(cbmem_console->buffer_cursor,
+  cbmem_console->buffer_size));
+}
+
 static int memconsole_coreboot_init(phys_addr_t physaddr)
 {
struct cbmem_cons __iomem *tmp_cbmc;
@@ -50,9 +58,7 @@ static int memconsole_coreboot_init(phys_addr_t physaddr)
if (!cbmem_console)
return -ENOMEM;
 
-   memconsole_setup(cbmem_console->buffer_body,
-   min(cbmem_console->buffer_cursor, cbmem_console->buffer_size));
-
+   memconsole_setup(memconsole_coreboot_read);
return 0;
 }
 
diff --git a/drivers/firmware/google/memconsole-x86-legacy.c 
b/drivers/firmware/google/memconsole-x86-legacy.c
index 529078c62488..1b556e944599 100644
--- a/drivers/firmware/google/memconsole-x86-legacy.c
+++ b/drivers/firmware/google/memconsole-x86-legacy.c
@@ -49,6 +49,15 @@ struct biosmemcon_ebda {
};
 } __packed;
 
+static char *memconsole_baseaddr;
+static size_t memconsole_length;
+
+static ssize_t memconsole_read(char *buf, loff_t pos, size_t count)
+{
+   return memory_read_from_buffer(buf, count, , memconsole_baseaddr,
+  memconsole_length);
+}
+
 static void found_v1_header(struct biosmemcon_ebda *hdr)
 {
pr_info("memconsole: BIOS console v1 EBDA structure found at %p\n",
@@ -57,7 +66,9 @@ static void found_v1_header(struct biosmemcon_ebda *hdr)
hdr->v1.buffer_addr, hdr->v1.start,
hdr->v1.end, hdr->v1.num_chars);
 
-   memconsole_setup(phys_to_virt(hdr->v1.buffer_addr), hdr->v1.num_chars);
+   memconsole_baseaddr = phys_to_virt(hdr->v1.buffer_addr);
+   memconsole_length = hdr->v1.num_chars;
+   memconsole_setup(memconsole_read);
 }
 
 static void found_v2_header(struct biosmemcon_ebda *hdr)
@@ -68,8 +79,9 @@ static void found_v2_header(struct biosmemcon_ebda *hdr)
hdr->v2.buffer_addr, hdr->v2.start,
hdr->v2.end, hdr->v2.num_bytes);
 
-   memconsole_setup(phys_to_virt(hdr->v2.buffer_addr + hdr->v2.start),
-hdr->v2.end - hdr->v2.start);
+   memconsole_baseaddr = phys_to_virt(hdr->v2.buffer_addr + hdr->v2.start);
+   memconsole_length = hdr->v2.end - hdr->v2.start;
+   memconsole_setup(memconsole_read);
 }
 
 /*
diff --git a/drivers/firmware/google/memconsole.c 
b/drivers/firmware/google/memconsole.c
index 94e200ddb4fa..166f07c68c02 100644
--- a/drivers/firmware/google/memconsole.c
+++ b/drivers/firmware/google/memconsole.c
@@ -22,15 +22,15 @@
 
 #include "memconsole.h"
 
-static char *memconsole_baseaddr;
-static size_t memconsole_length;
+static ssize_t (*memconsole_read_func)(char *, loff_t, size_t);
 
 static ssize_t memconsole_read(struct file *filp, struct kobject *kobp,
   struct bin_attribute *bin_attr, char *buf,
   loff_t pos, size_t count)
 {
-   return memory_read_from_buffer(buf, count, , memconsole_baseaddr,
-  memconsole_length);
+   if (WARN_ON_ONCE(!memconsole_read_func))
+   return -EIO;
+   return memconsole_read_func(buf, pos, count);
 }
 
 static struct bin_attribute memconsole_bin_attr = {
@@ -38,16 +38,14 @@ static struct bin_attribute memconsole_bin_attr = {
.read = memconsole_read,
 };
 
-void memconsole_setup(void *baseaddr, size_t length)
+void memconsole_setup(ssize_t (*read_func)(char *, loff_t, size_t))
 {
-   memconsole_baseaddr = baseaddr;
-   

[PATCH v2 3/3] firmware: google: memconsole: Adapt to new coreboot ring buffer format

[Julius Werner]

The upstream coreboot implementation of memconsole was enhanced from a
single-boot console to a persistent ring buffer
(https://review.coreboot.org/#/c/18301). This patch changes the kernel
memconsole driver to be able to read the new format in all cases.

Signed-off-by: Julius Werner 
---
 drivers/firmware/google/memconsole-coreboot.c | 47 ++-
 1 file changed, 39 insertions(+), 8 deletions(-)

diff --git a/drivers/firmware/google/memconsole-coreboot.c 
b/drivers/firmware/google/memconsole-coreboot.c
index b6234e61004d..23dc363b8519 100644
--- a/drivers/firmware/google/memconsole-coreboot.c
+++ b/drivers/firmware/google/memconsole-coreboot.c
@@ -26,19 +26,50 @@
 
 /* CBMEM firmware console log descriptor. */
 struct cbmem_cons {
-   u32 buffer_size;
-   u32 buffer_cursor;
-   u8  buffer_body[0];
+   u32 size;
+   u32 cursor;
+   u8  body[0];
 } __packed;
 
+#define CURSOR_MASK ((1 << 28) - 1)
+#define OVERFLOW (1 << 31)
+
 static struct cbmem_cons __iomem *cbmem_console;
 
+/*
+ * The cbmem_console structure is read again on every access because it may
+ * change at any time if runtime firmware logs new messages. This may rarely
+ * lead to race conditions where the firmware overwrites the beginning of the
+ * ring buffer with more lines after we have already read |cursor|. It should 
be
+ * rare and harmless enough that we don't spend extra effort working around it.
+ */
 static ssize_t memconsole_coreboot_read(char *buf, loff_t pos, size_t count)
 {
-   return memory_read_from_buffer(buf, count, ,
-  cbmem_console->buffer_body,
-  min(cbmem_console->buffer_cursor,
-  cbmem_console->buffer_size));
+   u32 cursor = cbmem_console->cursor & CURSOR_MASK;
+   u32 flags = cbmem_console->cursor & ~CURSOR_MASK;
+   u32 size = cbmem_console->size;
+   struct seg {/* describes ring buffer segments in logical order */
+   u32 phys;   /* physical offset from start of mem buffer */
+   u32 len;/* length of segment */
+   } seg[2] = { {0}, {0} };
+   size_t done = 0;
+   int i;
+
+   if (flags & OVERFLOW) {
+   if (cursor > size)  /* Shouldn't really happen, but... */
+   cursor = 0;
+   seg[0] = (struct seg){.phys = cursor, .len = size - cursor};
+   seg[1] = (struct seg){.phys = 0, .len = cursor};
+   } else {
+   seg[0] = (struct seg){.phys = 0, .len = min(cursor, size)};
+   }
+
+   for (i = 0; i < ARRAY_SIZE(seg) && count > done; i++) {
+   done += memory_read_from_buffer(buf + done, count - done, ,
+   cbmem_console->body + seg[i].phys, seg[i].len);
+   pos -= seg[i].len;
+   }
+   return done;
 }
 
 static int memconsole_coreboot_init(phys_addr_t physaddr)
@@ -51,7 +82,7 @@ static int memconsole_coreboot_init(phys_addr_t physaddr)
return -ENOMEM;
 
cbmem_console = memremap(physaddr,
-tmp_cbmc->buffer_size + sizeof(*cbmem_console),
+tmp_cbmc->size + sizeof(*cbmem_console),
 MEMREMAP_WB);
memunmap(tmp_cbmc);
 
-- 
2.12.2

[PATCH v2 1/3] firmware: google: memconsole: Make memconsole interface more flexible

[Julius Werner]

This patch redesigns the interface between the generic memconsole driver
and its implementations to become more flexible than a flat memory
buffer with unchanging bounds. This allows memconsoles like coreboot's
to include lines that were added by runtime firmware after the driver
was initialized. Since the console log size is thus no longer static,
this means that the /sys/firmware/log file has to become unseekable.

Signed-off-by: Julius Werner 
---
 drivers/firmware/google/memconsole-coreboot.c   | 12 +---
 drivers/firmware/google/memconsole-x86-legacy.c | 18 +++---
 drivers/firmware/google/memconsole.c| 14 ++
 drivers/firmware/google/memconsole.h|  7 ---
 4 files changed, 34 insertions(+), 17 deletions(-)

diff --git a/drivers/firmware/google/memconsole-coreboot.c 
b/drivers/firmware/google/memconsole-coreboot.c
index 21210144def7..b6234e61004d 100644
--- a/drivers/firmware/google/memconsole-coreboot.c
+++ b/drivers/firmware/google/memconsole-coreboot.c
@@ -33,6 +33,14 @@ struct cbmem_cons {
 
 static struct cbmem_cons __iomem *cbmem_console;
 
+static ssize_t memconsole_coreboot_read(char *buf, loff_t pos, size_t count)
+{
+   return memory_read_from_buffer(buf, count, ,
+  cbmem_console->buffer_body,
+  min(cbmem_console->buffer_cursor,
+  cbmem_console->buffer_size));
+}
+
 static int memconsole_coreboot_init(phys_addr_t physaddr)
 {
struct cbmem_cons __iomem *tmp_cbmc;
@@ -50,9 +58,7 @@ static int memconsole_coreboot_init(phys_addr_t physaddr)
if (!cbmem_console)
return -ENOMEM;
 
-   memconsole_setup(cbmem_console->buffer_body,
-   min(cbmem_console->buffer_cursor, cbmem_console->buffer_size));
-
+   memconsole_setup(memconsole_coreboot_read);
return 0;
 }
 
diff --git a/drivers/firmware/google/memconsole-x86-legacy.c 
b/drivers/firmware/google/memconsole-x86-legacy.c
index 529078c62488..1b556e944599 100644
--- a/drivers/firmware/google/memconsole-x86-legacy.c
+++ b/drivers/firmware/google/memconsole-x86-legacy.c
@@ -49,6 +49,15 @@ struct biosmemcon_ebda {
};
 } __packed;
 
+static char *memconsole_baseaddr;
+static size_t memconsole_length;
+
+static ssize_t memconsole_read(char *buf, loff_t pos, size_t count)
+{
+   return memory_read_from_buffer(buf, count, , memconsole_baseaddr,
+  memconsole_length);
+}
+
 static void found_v1_header(struct biosmemcon_ebda *hdr)
 {
pr_info("memconsole: BIOS console v1 EBDA structure found at %p\n",
@@ -57,7 +66,9 @@ static void found_v1_header(struct biosmemcon_ebda *hdr)
hdr->v1.buffer_addr, hdr->v1.start,
hdr->v1.end, hdr->v1.num_chars);
 
-   memconsole_setup(phys_to_virt(hdr->v1.buffer_addr), hdr->v1.num_chars);
+   memconsole_baseaddr = phys_to_virt(hdr->v1.buffer_addr);
+   memconsole_length = hdr->v1.num_chars;
+   memconsole_setup(memconsole_read);
 }
 
 static void found_v2_header(struct biosmemcon_ebda *hdr)
@@ -68,8 +79,9 @@ static void found_v2_header(struct biosmemcon_ebda *hdr)
hdr->v2.buffer_addr, hdr->v2.start,
hdr->v2.end, hdr->v2.num_bytes);
 
-   memconsole_setup(phys_to_virt(hdr->v2.buffer_addr + hdr->v2.start),
-hdr->v2.end - hdr->v2.start);
+   memconsole_baseaddr = phys_to_virt(hdr->v2.buffer_addr + hdr->v2.start);
+   memconsole_length = hdr->v2.end - hdr->v2.start;
+   memconsole_setup(memconsole_read);
 }
 
 /*
diff --git a/drivers/firmware/google/memconsole.c 
b/drivers/firmware/google/memconsole.c
index 94e200ddb4fa..166f07c68c02 100644
--- a/drivers/firmware/google/memconsole.c
+++ b/drivers/firmware/google/memconsole.c
@@ -22,15 +22,15 @@
 
 #include "memconsole.h"
 
-static char *memconsole_baseaddr;
-static size_t memconsole_length;
+static ssize_t (*memconsole_read_func)(char *, loff_t, size_t);
 
 static ssize_t memconsole_read(struct file *filp, struct kobject *kobp,
   struct bin_attribute *bin_attr, char *buf,
   loff_t pos, size_t count)
 {
-   return memory_read_from_buffer(buf, count, , memconsole_baseaddr,
-  memconsole_length);
+   if (WARN_ON_ONCE(!memconsole_read_func))
+   return -EIO;
+   return memconsole_read_func(buf, pos, count);
 }
 
 static struct bin_attribute memconsole_bin_attr = {
@@ -38,16 +38,14 @@ static struct bin_attribute memconsole_bin_attr = {
.read = memconsole_read,
 };
 
-void memconsole_setup(void *baseaddr, size_t length)
+void memconsole_setup(ssize_t (*read_func)(char *, loff_t, size_t))
 {
-   memconsole_baseaddr = baseaddr;
-   memconsole_leng

[PATCH v2 0/3] Memconsole changes for new coreboot format

[Julius Werner]

This series enhances the memconsole driver to work well with the new
persistent ring buffer console introduced in coreboot with
https://review.coreboot.org/#/c/18301. It needs to be applied on top of
the other memconsole patches currently queued up in char-misc-next
(ending in 049a59db34e).

Julius Werner (3):
  firmware: google: memconsole: Make memconsole interface more flexible
  firmware: google: memconsole: Escape unprintable characters
  firmware: google: memconsole: Adapt to new coreboot ring buffer format

 drivers/firmware/google/memconsole-coreboot.c   | 51 +
 drivers/firmware/google/memconsole-x86-legacy.c | 18 +++--
 drivers/firmware/google/memconsole.c| 46 +++---
 drivers/firmware/google/memconsole.h|  7 ++--
 4 files changed, 96 insertions(+), 26 deletions(-)

-- 
2.12.2

[PATCH v2 0/3] Memconsole changes for new coreboot format

[Julius Werner]

This series enhances the memconsole driver to work well with the new
persistent ring buffer console introduced in coreboot with
https://review.coreboot.org/#/c/18301. It needs to be applied on top of
the other memconsole patches currently queued up in char-misc-next
(ending in 049a59db34e).

Julius Werner (3):
  firmware: google: memconsole: Make memconsole interface more flexible
  firmware: google: memconsole: Escape unprintable characters
  firmware: google: memconsole: Adapt to new coreboot ring buffer format

 drivers/firmware/google/memconsole-coreboot.c   | 51 +
 drivers/firmware/google/memconsole-x86-legacy.c | 18 +++--
 drivers/firmware/google/memconsole.c| 46 +++---
 drivers/firmware/google/memconsole.h|  7 ++--
 4 files changed, 96 insertions(+), 26 deletions(-)

-- 
2.12.2

[PATCH] firmware: google: memconsole: Adapt to new coreboot ring buffer format

[Julius Werner]

This patch needs to be applied on top of the other memconsole patches
currently queued up in char-misc-next (ending in 049a59db34e).

-8<

The upstream coreboot implementation of memconsole was enhanced from a
single-boot console to a persistent ring buffer
(https://review.coreboot.org/#/c/18301). This patch changes the kernel
memconsole driver to be able to read the new format.

In addition, it is desirable for the driver to always return the current
state of the console, so that it may be used by runtime firmware to log
further messages after the kernel is already initialized. The existing
implementation would already re-read the buffer on every access, but
only measured the total size of the console once during initialization.
Thus, the generic console interface was redesigned to allow the
implementation full control over how to process an access, and the
coreboot implementation will re-read the console size every time.

Finally, add a new /sys/firmware/rawlog node and change the existing
/sys/firmware/log to sanitize non-printable characters from the output.
With the new persistent console it becomes more likely that bytes might
get slightly corrupted (due to DRAM degradation during a reboot), and
it's usually undesirable to get stray control characters in the console
dump because a bit in a letter flipped.

Signed-off-by: Julius Werner <jwer...@chromium.org>
---
 drivers/firmware/google/memconsole-coreboot.c   | 51 +
 drivers/firmware/google/memconsole-x86-legacy.c | 18 +++--
 drivers/firmware/google/memconsole.c| 46 +++---
 drivers/firmware/google/memconsole.h|  7 ++--
 4 files changed, 96 insertions(+), 26 deletions(-)

diff --git a/drivers/firmware/google/memconsole-coreboot.c 
b/drivers/firmware/google/memconsole-coreboot.c
index 21210144def7..6e9b07697f70 100644
--- a/drivers/firmware/google/memconsole-coreboot.c
+++ b/drivers/firmware/google/memconsole-coreboot.c
@@ -26,13 +26,52 @@
 
 /* CBMEM firmware console log descriptor. */
 struct cbmem_cons {
-   u32 buffer_size;
-   u32 buffer_cursor;
-   u8  buffer_body[0];
+   u32 size;
+   u32 cursor;
+   u8  body[0];
 } __packed;
 
+#define CURSOR_MASK ((1 << 28) - 1)
+#define OVERFLOW (1 << 31)
+
 static struct cbmem_cons __iomem *cbmem_console;
 
+/*
+ * The cbmem_console structure is read again on every access because it may
+ * change at any time if runtime firmware logs new messages. This may rarely
+ * lead to race conditions where the firmware overwrites the beginning of the
+ * ring buffer with more lines after we have already read |cursor|. It should 
be
+ * rare and harmless enough that we don't spend extra effort working around it.
+ */
+static ssize_t memconsole_coreboot_read(char *buf, loff_t pos, size_t count)
+{
+   u32 cursor = cbmem_console->cursor & CURSOR_MASK;
+   u32 flags = cbmem_console->cursor & ~CURSOR_MASK;
+   u32 size = cbmem_console->size;
+   struct seg {/* describes ring buffer segments in logical order */
+   u32 phys;   /* physical offset from start of mem buffer */
+   u32 len;/* length of segment */
+   } seg[2] = { {0}, {0} };
+   size_t done = 0;
+   int i;
+
+   if (flags & OVERFLOW) {
+   if (cursor > size)  /* Shouldn't really happen, but... */
+   cursor = 0;
+   seg[0] = (struct seg){.phys = cursor, .len = size - cursor};
+   seg[1] = (struct seg){.phys = 0, .len = cursor};
+   } else {
+   seg[0] = (struct seg){.phys = 0, .len = min(cursor, size)};
+   }
+
+   for (i = 0; i < ARRAY_SIZE(seg) && count > done; i++) {
+   done += memory_read_from_buffer(buf + done, count - done, ,
+   cbmem_console->body + seg[i].phys, seg[i].len);
+   pos -= seg[i].len;
+   }
+   return done;
+}
+
 static int memconsole_coreboot_init(phys_addr_t physaddr)
 {
struct cbmem_cons __iomem *tmp_cbmc;
@@ -43,16 +82,14 @@ static int memconsole_coreboot_init(phys_addr_t physaddr)
return -ENOMEM;
 
cbmem_console = memremap(physaddr,
-tmp_cbmc->buffer_size + sizeof(*cbmem_console),
+tmp_cbmc->size + sizeof(*cbmem_console),
 MEMREMAP_WB);
memunmap(tmp_cbmc);
 
if (!cbmem_console)
return -ENOMEM;
 
-   memconsole_setup(cbmem_console->buffer_body,
-   min(cbmem_console->buffer_cursor, cbmem_console->buffer_size));
-
+   memconsole_setup(memconsole_coreboot_read);
return 0;
 }
 
diff --git a/drivers/firmware/google/memconsole-x86-legacy.c 
b/drivers/firmware/google/memconsole-x86-legacy.c
index 529078c62488..1b556e9445

[PATCH] firmware: google: memconsole: Adapt to new coreboot ring buffer format

[Julius Werner]

This patch needs to be applied on top of the other memconsole patches
currently queued up in char-misc-next (ending in 049a59db34e).

-8<

The upstream coreboot implementation of memconsole was enhanced from a
single-boot console to a persistent ring buffer
(https://review.coreboot.org/#/c/18301). This patch changes the kernel
memconsole driver to be able to read the new format.

In addition, it is desirable for the driver to always return the current
state of the console, so that it may be used by runtime firmware to log
further messages after the kernel is already initialized. The existing
implementation would already re-read the buffer on every access, but
only measured the total size of the console once during initialization.
Thus, the generic console interface was redesigned to allow the
implementation full control over how to process an access, and the
coreboot implementation will re-read the console size every time.

Finally, add a new /sys/firmware/rawlog node and change the existing
/sys/firmware/log to sanitize non-printable characters from the output.
With the new persistent console it becomes more likely that bytes might
get slightly corrupted (due to DRAM degradation during a reboot), and
it's usually undesirable to get stray control characters in the console
dump because a bit in a letter flipped.

Signed-off-by: Julius Werner 
---
 drivers/firmware/google/memconsole-coreboot.c   | 51 +
 drivers/firmware/google/memconsole-x86-legacy.c | 18 +++--
 drivers/firmware/google/memconsole.c| 46 +++---
 drivers/firmware/google/memconsole.h|  7 ++--
 4 files changed, 96 insertions(+), 26 deletions(-)

diff --git a/drivers/firmware/google/memconsole-coreboot.c 
b/drivers/firmware/google/memconsole-coreboot.c
index 21210144def7..6e9b07697f70 100644
--- a/drivers/firmware/google/memconsole-coreboot.c
+++ b/drivers/firmware/google/memconsole-coreboot.c
@@ -26,13 +26,52 @@
 
 /* CBMEM firmware console log descriptor. */
 struct cbmem_cons {
-   u32 buffer_size;
-   u32 buffer_cursor;
-   u8  buffer_body[0];
+   u32 size;
+   u32 cursor;
+   u8  body[0];
 } __packed;
 
+#define CURSOR_MASK ((1 << 28) - 1)
+#define OVERFLOW (1 << 31)
+
 static struct cbmem_cons __iomem *cbmem_console;
 
+/*
+ * The cbmem_console structure is read again on every access because it may
+ * change at any time if runtime firmware logs new messages. This may rarely
+ * lead to race conditions where the firmware overwrites the beginning of the
+ * ring buffer with more lines after we have already read |cursor|. It should 
be
+ * rare and harmless enough that we don't spend extra effort working around it.
+ */
+static ssize_t memconsole_coreboot_read(char *buf, loff_t pos, size_t count)
+{
+   u32 cursor = cbmem_console->cursor & CURSOR_MASK;
+   u32 flags = cbmem_console->cursor & ~CURSOR_MASK;
+   u32 size = cbmem_console->size;
+   struct seg {/* describes ring buffer segments in logical order */
+   u32 phys;   /* physical offset from start of mem buffer */
+   u32 len;/* length of segment */
+   } seg[2] = { {0}, {0} };
+   size_t done = 0;
+   int i;
+
+   if (flags & OVERFLOW) {
+   if (cursor > size)  /* Shouldn't really happen, but... */
+   cursor = 0;
+   seg[0] = (struct seg){.phys = cursor, .len = size - cursor};
+   seg[1] = (struct seg){.phys = 0, .len = cursor};
+   } else {
+   seg[0] = (struct seg){.phys = 0, .len = min(cursor, size)};
+   }
+
+   for (i = 0; i < ARRAY_SIZE(seg) && count > done; i++) {
+   done += memory_read_from_buffer(buf + done, count - done, ,
+   cbmem_console->body + seg[i].phys, seg[i].len);
+   pos -= seg[i].len;
+   }
+   return done;
+}
+
 static int memconsole_coreboot_init(phys_addr_t physaddr)
 {
struct cbmem_cons __iomem *tmp_cbmc;
@@ -43,16 +82,14 @@ static int memconsole_coreboot_init(phys_addr_t physaddr)
return -ENOMEM;
 
cbmem_console = memremap(physaddr,
-tmp_cbmc->buffer_size + sizeof(*cbmem_console),
+tmp_cbmc->size + sizeof(*cbmem_console),
 MEMREMAP_WB);
memunmap(tmp_cbmc);
 
if (!cbmem_console)
return -ENOMEM;
 
-   memconsole_setup(cbmem_console->buffer_body,
-   min(cbmem_console->buffer_cursor, cbmem_console->buffer_size));
-
+   memconsole_setup(memconsole_coreboot_read);
return 0;
 }
 
diff --git a/drivers/firmware/google/memconsole-x86-legacy.c 
b/drivers/firmware/google/memconsole-x86-legacy.c
index 529078c62488..1b556e944599 100644
--- a/driver

Re: [PATCH 5/5] firmware: google memconsole: Add ARM/ARM64 support

[Julius Werner]

> What exactly is the "memory console"? Is it a log that coreboot writes into?

Yes. It contains log messages, like coreboot's equivalent of dmesg.

Re: [PATCH 5/5] firmware: google memconsole: Add ARM/ARM64 support

[Julius Werner]

> What exactly is the "memory console"? Is it a log that coreboot writes into?

Yes. It contains log messages, like coreboot's equivalent of dmesg.

Re: [PATCH 4/5] firmware: Add coreboot device tree binding documentation

[Julius Werner]

...and again in plaintext, sorry about that.

On Fri, Mar 24, 2017 at 12:32 PM, Julius Werner <jwer...@chromium.org> wrote:
>> > Devicetree bindings should be in vendor,prefix format. This doesn't
>> > represent every aspect of coreboot, so it needs a more descriptive
>> > string.
>>
>> Any particular suggestion? I suppose this is Google's interpretation of
>> coreboot tables, so "google,coreboot"?
>
>
> No. This binding is for the coreboot project in general and has nothing to
> do with Google. coreboot is both the vendor and the product, so I think
> "coreboot" would look better than "coreboot,coreboot". And yes, it is
> supposed to represent every aspect of coreboot (right now the "coreboot
> tables" are already sort of a catch-all data structure used by coreboot to
> pass on any sort of info it wants later stages to know... and if we ever
> have any additional things we'd like to pass on, we'd probably want to add
> them to this binding as well).
>
>>
>> > > + - reg: Address and length of the following two memory regions, in
>> > > order:
>> > > +   1.) The coreboot table. This is a list of variable-sized
>> > > descriptors
>> > > +   that contain various compile- and run-time generated firmware
>> > > +   parameters. It is identified by the magic string "LBIO" in its
>> > > first
>> > > +   four bytes.
>> > > +   See coreboot's src/commonlib/include/commonlib/coreboot_tables.h
>> > > for
>> > > +   details.
>> >
>> > Given this is a memory region, it should be described under the
>> > reserved-memory node.
>>
>> We've painted this bikeshed before. I guess the result was either to use
>> /reserved-memory or /memreserve/. I believe we've been using
>> /memreserve/. I suppose we could document a method to use either, but
>> the main "agreement" was to use the /firmware/coreboot path instead of
>> /reserved-memory/coreboot.
>
>
> See the old thread Brian linked for some arguments for and against this. I
> think particularly because we want this node to represent every aspect of
> coreboot (which I think makes more sense than spreading stuff all over the
> place), treating it as reserved memory would not work well if we might later
> add other fields.
>
> Also, since we didn't get any more responses the last time we tried to
> upstream this and had schedules to keep, we had to go ahead with what we
> had. So right now there are already millions of devices shipped with this
> binding in firmware, and the only question we still have left to decide is
> whether Linux wants to support them upstream or not.

Re: [PATCH 4/5] firmware: Add coreboot device tree binding documentation

[Julius Werner]

...and again in plaintext, sorry about that.

On Fri, Mar 24, 2017 at 12:32 PM, Julius Werner  wrote:
>> > Devicetree bindings should be in vendor,prefix format. This doesn't
>> > represent every aspect of coreboot, so it needs a more descriptive
>> > string.
>>
>> Any particular suggestion? I suppose this is Google's interpretation of
>> coreboot tables, so "google,coreboot"?
>
>
> No. This binding is for the coreboot project in general and has nothing to
> do with Google. coreboot is both the vendor and the product, so I think
> "coreboot" would look better than "coreboot,coreboot". And yes, it is
> supposed to represent every aspect of coreboot (right now the "coreboot
> tables" are already sort of a catch-all data structure used by coreboot to
> pass on any sort of info it wants later stages to know... and if we ever
> have any additional things we'd like to pass on, we'd probably want to add
> them to this binding as well).
>
>>
>> > > + - reg: Address and length of the following two memory regions, in
>> > > order:
>> > > +   1.) The coreboot table. This is a list of variable-sized
>> > > descriptors
>> > > +   that contain various compile- and run-time generated firmware
>> > > +   parameters. It is identified by the magic string "LBIO" in its
>> > > first
>> > > +   four bytes.
>> > > +   See coreboot's src/commonlib/include/commonlib/coreboot_tables.h
>> > > for
>> > > +   details.
>> >
>> > Given this is a memory region, it should be described under the
>> > reserved-memory node.
>>
>> We've painted this bikeshed before. I guess the result was either to use
>> /reserved-memory or /memreserve/. I believe we've been using
>> /memreserve/. I suppose we could document a method to use either, but
>> the main "agreement" was to use the /firmware/coreboot path instead of
>> /reserved-memory/coreboot.
>
>
> See the old thread Brian linked for some arguments for and against this. I
> think particularly because we want this node to represent every aspect of
> coreboot (which I think makes more sense than spreading stuff all over the
> place), treating it as reserved memory would not work well if we might later
> add other fields.
>
> Also, since we didn't get any more responses the last time we tried to
> upstream this and had schedules to keep, we had to go ahead with what we
> had. So right now there are already millions of devices shipped with this
> binding in firmware, and the only question we still have left to decide is
> whether Linux wants to support them upstream or not.

[PATCH] clk: rockchip: Ignore frac divisor for PLL equivalence when it's unused

[Julius Werner]

Rockchip RK3399 PLLs can be used in two separate modes: integral and
fractional. We can select between these two modes with the unambiguously
named DSMPD bit.

During boot, we check all PLL settings to confirm that they match our
PLL table for that frequency, and reinitialize the PLLs where they
don't. The settings checked for this include the fractional divider
field that is only used in fractional mode, even if we're in integral
mode (DSMPD = 1) and that field has no effect.

This patch changes the check to only compare the fractional divider if
we're actually in fractional mode. This way, we won't reinitialize the
PLL in cases where there's absolutely no reason for that, which may
avoid glitching child clocks that should better not be glitched (e.g.
PWM regulators).

Signed-off-by: Julius Werner <jwer...@chromium.org>
---
 drivers/clk/rockchip/clk-pll.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/rockchip/clk-pll.c b/drivers/clk/rockchip/clk-pll.c
index 9c1373e..1449c76 100644
--- a/drivers/clk/rockchip/clk-pll.c
+++ b/drivers/clk/rockchip/clk-pll.c
@@ -795,7 +795,8 @@ static void rockchip_rk3399_pll_init(struct clk_hw *hw)
 
if (rate->fbdiv != cur.fbdiv || rate->postdiv1 != cur.postdiv1 ||
rate->refdiv != cur.refdiv || rate->postdiv2 != cur.postdiv2 ||
-   rate->dsmpd != cur.dsmpd || rate->frac != cur.frac) {
+   rate->dsmpd != cur.dsmpd ||
+   (!cur.dsmpd && (rate->frac != cur.frac))) {
struct clk *parent = clk_get_parent(hw->clk);
 
if (!parent) {
-- 
2.6.6

[PATCH] clk: rockchip: Ignore frac divisor for PLL equivalence when it's unused

[Julius Werner]

Rockchip RK3399 PLLs can be used in two separate modes: integral and
fractional. We can select between these two modes with the unambiguously
named DSMPD bit.

During boot, we check all PLL settings to confirm that they match our
PLL table for that frequency, and reinitialize the PLLs where they
don't. The settings checked for this include the fractional divider
field that is only used in fractional mode, even if we're in integral
mode (DSMPD = 1) and that field has no effect.

This patch changes the check to only compare the fractional divider if
we're actually in fractional mode. This way, we won't reinitialize the
PLL in cases where there's absolutely no reason for that, which may
avoid glitching child clocks that should better not be glitched (e.g.
PWM regulators).

Signed-off-by: Julius Werner 
---
 drivers/clk/rockchip/clk-pll.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/rockchip/clk-pll.c b/drivers/clk/rockchip/clk-pll.c
index 9c1373e..1449c76 100644
--- a/drivers/clk/rockchip/clk-pll.c
+++ b/drivers/clk/rockchip/clk-pll.c
@@ -795,7 +795,8 @@ static void rockchip_rk3399_pll_init(struct clk_hw *hw)
 
if (rate->fbdiv != cur.fbdiv || rate->postdiv1 != cur.postdiv1 ||
rate->refdiv != cur.refdiv || rate->postdiv2 != cur.postdiv2 ||
-   rate->dsmpd != cur.dsmpd || rate->frac != cur.frac) {
+   rate->dsmpd != cur.dsmpd ||
+   (!cur.dsmpd && (rate->frac != cur.frac))) {
struct clk *parent = clk_get_parent(hw->clk);
 
if (!parent) {
-- 
2.6.6