Attention is currently required from: Nico Huber, Thomas Heijligen.
Peter Marheine has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/61300 )
Change subject: Makeile: reenable internal programmer for mipsel
......................................................................
Patch Set 3:
(1 comment)
File Makefile:
https://review.coreboot.org/c/flashrom/+/61300/comment/d1c82fc5_d5da20ff
PS3, Line 332: # Disable the internal programmer on unsupported architectures (everything but x86 and mipsel)
> The internal programmer should fail at runtime at internal. […]
processor_flash_enable() is only called after try_mtd() on line 255, which will skip the rest of the function if MTD works. It looks like MTD support landed sometime after the change that was meant to disable the internal programmer on other platforms, but this mark_unsupported only works as expected if CONFIG_INTERNAL is explicitly set.
CONFIG_INTERNAL is defaulted to yes further down the file, so it looks like the internal programmer can still be built as long as it's not explicitly enabled even with this change. I'm guessing nobody noticed that this conflicted with the MTD support because this disablement didn't work, with the end result that this mark_unsupported is now incorrect because internal is meant to work on other platforms when MTD is available.
--
To view, visit https://review.coreboot.org/c/flashrom/+/61300
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Ia607ea60c3d7d15fe231fa412595992dadc535ad
Gerrit-Change-Number: 61300
Gerrit-PatchSet: 3
Gerrit-Owner: Thomas Heijligen <src(a)posteo.de>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Felix Singer <felixsinger(a)posteo.net>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-CC: Peter Marheine <pmarheine(a)chromium.org>
Gerrit-Attention: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Thomas Heijligen <src(a)posteo.de>
Gerrit-Comment-Date: Mon, 24 Jan 2022 01:02:32 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de>
Comment-In-Reply-To: Thomas Heijligen <src(a)posteo.de>
Comment-In-Reply-To: Peter Marheine <pmarheine(a)chromium.org>
Gerrit-MessageType: comment
Attention is currently required from: Nico Huber, Edward O'Callaghan, Angel Pons.
Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/59742 )
Change subject: tests: Upgrade linux_spi test to run probe lifecycle
......................................................................
Patch Set 5:
(3 comments)
File tests/lifecycle.c:
https://review.coreboot.org/c/flashrom/+/59742/comment/a60c9ae7_9680a4b1
PS4, Line 18: #include <linux/spi/spidev.h>
Ok thanks! I remember a conversation about libusb earlier. I will leave it.
> put the code into a separate .c file, e.g. one specific to linux_spi tests
I might do it later for a different reason. I want to try and write extended version of chip tests, with real programmer not dummy. I can imagine, that would need more mocks and more code per programmer.
However I want to finish probing lifecycle implementation first :)
https://review.coreboot.org/c/flashrom/+/59742/comment/5bd86458_16aaad1e
PS4, Line 321: 0x40406b00
> Isn't this covered by the include, something like `SPI_IOC_MESSAGE(2)`?
Oh yes! I probably wrote this line before I added the include.
https://review.coreboot.org/c/flashrom/+/59742/comment/964cbd2e_b7dbb1ee
PS4, Line 327: readcnt == 3
> Checking the actual command would be more accurate and not much harder, […]
Yes true, done.
--
To view, visit https://review.coreboot.org/c/flashrom/+/59742
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I32d8e972d99b52c2b18f688aa6aeae75dd170f72
Gerrit-Change-Number: 59742
Gerrit-PatchSet: 5
Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Comment-Date: Mon, 24 Jan 2022 01:00:10 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de>
Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-MessageType: comment
Attention is currently required from: Nico Huber, Thomas Heijligen, Paul Menzel, Edward O'Callaghan.
Peter Marheine has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/61194 )
Change subject: hwaccess: fix build on non-x86 targets
......................................................................
Patch Set 5:
(1 comment)
Patchset:
PS1:
> It doesn't change a thing about the problem at hand, though, that code compiled […]
I've moved the inclusions of the x86 headers around to fit in existing guards where possible, though for internal.c I had to add a new one since that file has x86 and generic code interleaved in multiple places.
--
To view, visit https://review.coreboot.org/c/flashrom/+/61194
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I20f122679c30340b2c73afd7419e79644ddc3c4e
Gerrit-Change-Number: 61194
Gerrit-PatchSet: 5
Gerrit-Owner: Peter Marheine <pmarheine(a)chromium.org>
Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Thomas Heijligen <src(a)posteo.de>
Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Comment-Date: Mon, 24 Jan 2022 00:37:52 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de>
Comment-In-Reply-To: Thomas Heijligen <src(a)posteo.de>
Comment-In-Reply-To: Peter Marheine <pmarheine(a)chromium.org>
Gerrit-MessageType: comment
Attention is currently required from: Thomas Heijligen, Paul Menzel, Edward O'Callaghan, Peter Marheine.
Hello build bot (Jenkins), Nico Huber, Thomas Heijligen, Edward O'Callaghan,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/61194
to look at the new patch set (#6).
Change subject: hwaccess: fix build on non-x86 targets
......................................................................
hwaccess: fix build on non-x86 targets
The changes to hwaccess in commit 49d758698a0dd166679c48b1a2785e50e9b0cc83
cause build failure on non-x86 systems because the hwaccess_x86_*
headers are included in some files that are built for all platforms
(particularly those in the internal programmer) and those headers in
turn include <sys/io.h> which only exists on x86.
This change makes the hwaccess_x86 headers a nop on non-x86 platforms so
the internal programmer can be built without errors.
The comment on the stub implementation of rget_io_perms() is also
modified to remove references to non-x86 platforms, since that file is
only built on x86 now.
BUG=None
TEST=meson build succeeds for both x86 and ARM targets
Signed-off-by: Peter Marheine <pmarheine(a)chromium.org>
Change-Id: I20f122679c30340b2c73afd7419e79644ddc3c4e
---
M board_enable.c
M chipset_enable.c
M hwaccess_x86_io.c
M hwaccess_x86_msr.h
M internal.c
5 files changed, 17 insertions(+), 12 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/94/61194/6
--
To view, visit https://review.coreboot.org/c/flashrom/+/61194
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I20f122679c30340b2c73afd7419e79644ddc3c4e
Gerrit-Change-Number: 61194
Gerrit-PatchSet: 6
Gerrit-Owner: Peter Marheine <pmarheine(a)chromium.org>
Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Thomas Heijligen <src(a)posteo.de>
Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Attention: Peter Marheine <pmarheine(a)chromium.org>
Gerrit-MessageType: newpatchset
Attention is currently required from: Edward O'Callaghan, Angel Pons, Anastasia Klimchuk.
Hello build bot (Jenkins), Nico Huber, Edward O'Callaghan, Angel Pons,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/59742
to look at the new patch set (#5).
Change subject: tests: Upgrade linux_spi test to run probe lifecycle
......................................................................
tests: Upgrade linux_spi test to run probe lifecycle
This test adds a mock for linux_spi ioctl and mocks it for read
request. Read buffer is populated with chip manufacture id and
chip model id to emulate successful probing.
BUG=b:181803212
TEST=ninja test
Change-Id: I32d8e972d99b52c2b18f688aa6aeae75dd170f72
Signed-off-by: Anastasia Klimchuk <aklm(a)chromium.org>
---
M tests/lifecycle.c
M tests/tests.c
M tests/tests.h
3 files changed, 30 insertions(+), 4 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/42/59742/5
--
To view, visit https://review.coreboot.org/c/flashrom/+/59742
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I32d8e972d99b52c2b18f688aa6aeae75dd170f72
Gerrit-Change-Number: 59742
Gerrit-PatchSet: 5
Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-MessageType: newpatchset
Attention is currently required from: Nico Huber, Thomas Heijligen, Peter Marheine.
Felix Singer has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/61300 )
Change subject: Makeile: reenable internal programmer for mipsel
......................................................................
Patch Set 3:
(2 comments)
Commit Message:
https://review.coreboot.org/c/flashrom/+/61300/comment/92c8255b_322c13e5
PS3, Line 7: Makeile
Make_f_ile
https://review.coreboot.org/c/flashrom/+/61300/comment/25ab0ddc_fe2d21a9
PS3, Line 10: for
Remove this
--
To view, visit https://review.coreboot.org/c/flashrom/+/61300
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Ia607ea60c3d7d15fe231fa412595992dadc535ad
Gerrit-Change-Number: 61300
Gerrit-PatchSet: 3
Gerrit-Owner: Thomas Heijligen <src(a)posteo.de>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Felix Singer <felixsinger(a)posteo.net>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-CC: Peter Marheine <pmarheine(a)chromium.org>
Gerrit-Attention: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Thomas Heijligen <src(a)posteo.de>
Gerrit-Attention: Peter Marheine <pmarheine(a)chromium.org>
Gerrit-Comment-Date: Mon, 24 Jan 2022 00:29:17 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Attention is currently required from: Nico Huber, Peter Marheine.
Thomas Heijligen has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/61300 )
Change subject: Makeile: reenable internal programmer for mipsel
......................................................................
Patch Set 3:
(2 comments)
File Makefile:
https://review.coreboot.org/c/flashrom/+/61300/comment/547cfbd6_e5babc55
PS3, Line 104: CONFIG_INTERNAL_MIPSLE \
> Is this actually true?
I haven't looked it up yet
https://review.coreboot.org/c/flashrom/+/61300/comment/64c8059b_ca168f11
PS3, Line 332: # Disable the internal programmer on unsupported architectures (everything but x86 and mipsel)
> Sounds like it compiled with the meson.build by coincidence all the time? […]
The internal programmer should fail at runtime at internal.c:266 processor_flash_enable() which return 1 on non x86 or specific mips and let the init function return also 1, which indicates an error.
The whole point of having CONFIG_INTERNAL_<arch> and not only CONFIG_INTERNAL and CONFIG_INTERNAL_X86 is to be able to have multiple implementations of the internal programmer. The CONFIG_INTERNAL is only an alias for the user.
--
To view, visit https://review.coreboot.org/c/flashrom/+/61300
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Ia607ea60c3d7d15fe231fa412595992dadc535ad
Gerrit-Change-Number: 61300
Gerrit-PatchSet: 3
Gerrit-Owner: Thomas Heijligen <src(a)posteo.de>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-CC: Peter Marheine <pmarheine(a)chromium.org>
Gerrit-Attention: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Peter Marheine <pmarheine(a)chromium.org>
Gerrit-Comment-Date: Mon, 24 Jan 2022 00:06:28 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de>
Comment-In-Reply-To: Peter Marheine <pmarheine(a)chromium.org>
Gerrit-MessageType: comment
Attention is currently required from: Thomas Heijligen, Paul Menzel, Edward O'Callaghan, Peter Marheine.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/61194 )
Change subject: hwaccess: fix build on non-x86 targets
......................................................................
Patch Set 5: Code-Review-1
(2 comments)
Patchset:
PS1:
> 61276 in particular looks like a nice cleanup of these headers. […]
It doesn't change a thing about the problem at hand, though, that code compiled
for non-x86 includes an x86 file.
Patchset:
PS5:
This is a step backwards, having an `if x86` in an x86 file can't be good.
--
To view, visit https://review.coreboot.org/c/flashrom/+/61194
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I20f122679c30340b2c73afd7419e79644ddc3c4e
Gerrit-Change-Number: 61194
Gerrit-PatchSet: 5
Gerrit-Owner: Peter Marheine <pmarheine(a)chromium.org>
Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Thomas Heijligen <src(a)posteo.de>
Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Attention: Peter Marheine <pmarheine(a)chromium.org>
Gerrit-Comment-Date: Sun, 23 Jan 2022 23:27:28 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de>
Comment-In-Reply-To: Thomas Heijligen <src(a)posteo.de>
Comment-In-Reply-To: Peter Marheine <pmarheine(a)chromium.org>
Gerrit-MessageType: comment
Attention is currently required from: Thomas Heijligen, Peter Marheine.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/61300 )
Change subject: Makeile: reenable internal programmer for mipsel
......................................................................
Patch Set 3:
(2 comments)
File Makefile:
https://review.coreboot.org/c/flashrom/+/61300/comment/a5ba1dbd_d97cadbd
PS3, Line 104: CONFIG_INTERNAL_MIPSLE \
Is this actually true?
https://review.coreboot.org/c/flashrom/+/61300/comment/d284d3d3_5073a47c
PS3, Line 332: # Disable the internal programmer on unsupported architectures (everything but x86 and mipsel)
> I think this is overly strict, because the internal programmer will delegate to MTD if enabled, whic […]
Sounds like it compiled with the meson.build by coincidence all the time?
Looking at the code, it seems possible, but there's a shitload of dead,
code not making any sense.
I guess we could have a CONFIG_INTERNAL (with raw mem and libpci (spurious?) dependencies) and CONFIG_INTERNAL_X86 like suggested by this patch. Thomas,
WDYT? For the dependencies, we'd have to disable CONFIG_INTERNAL if
CONFIG_INTERNAL_X86 = no.
--
To view, visit https://review.coreboot.org/c/flashrom/+/61300
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Ia607ea60c3d7d15fe231fa412595992dadc535ad
Gerrit-Change-Number: 61300
Gerrit-PatchSet: 3
Gerrit-Owner: Thomas Heijligen <src(a)posteo.de>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-CC: Peter Marheine <pmarheine(a)chromium.org>
Gerrit-Attention: Thomas Heijligen <src(a)posteo.de>
Gerrit-Attention: Peter Marheine <pmarheine(a)chromium.org>
Gerrit-Comment-Date: Sun, 23 Jan 2022 23:22:44 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Peter Marheine <pmarheine(a)chromium.org>
Gerrit-MessageType: comment
Attention is currently required from: Nico Huber, Thomas Heijligen, Paul Menzel.
Peter Marheine has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/61194 )
Change subject: hwaccess: fix build on non-x86 targets
......................................................................
Patch Set 5:
(3 comments)
Patchset:
PS1:
> I've started working on an overhaul of the hwaccess code in CB:61275 and CB:61276. […]
61276 in particular looks like a nice cleanup of these headers. Fixing the build regression seems like the important aspect of this change, and calling it a temporary solution until the headers are cleaned up more comprehensively sounds fine to me.
File hwaccess_x86_io.h:
https://review.coreboot.org/c/flashrom/+/61194/comment/3796a02d_fc2baee8
PS4, Line 41: #define HAVE_RGET_IO_PERMS 1
> not needed
Done
File internal.c:
https://review.coreboot.org/c/flashrom/+/61194/comment/4343a592_6843769f
PS4, Line 245: if (rget_io_perms()) {
> You can move this under the x86 guard at line 275. […]
Done
--
To view, visit https://review.coreboot.org/c/flashrom/+/61194
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I20f122679c30340b2c73afd7419e79644ddc3c4e
Gerrit-Change-Number: 61194
Gerrit-PatchSet: 5
Gerrit-Owner: Peter Marheine <pmarheine(a)chromium.org>
Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-CC: Nico Huber <nico.h(a)gmx.de>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Thomas Heijligen <src(a)posteo.de>
Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Comment-Date: Sun, 23 Jan 2022 22:22:46 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de>
Comment-In-Reply-To: Thomas Heijligen <src(a)posteo.de>
Comment-In-Reply-To: Peter Marheine <pmarheine(a)chromium.org>
Gerrit-MessageType: comment