All of lore.kernel.org
 help / color / mirror / Atom feed
From: "Alex Bennée" <alex.bennee@linaro.org>
To: Nicolas Eder <nicolas.eder@lauterbach.com>
Cc: qemu-devel@nongnu.org,
	"Philippe Mathieu-Daudé" <philmd@linaro.org>,
	Christian.Boenig@lauterbach.com
Subject: Re: [PATCH v2 00/29] first version of mcdstub
Date: Fri, 13 Oct 2023 17:47:25 +0100	[thread overview]
Message-ID: <87mswmlaz9.fsf@linaro.org> (raw)
In-Reply-To: <20231006090610.26171-1-nicolas.eder@lauterbach.com>


Nicolas Eder <nicolas.eder@lauterbach.com> writes:

<snip>
> Signed-off-by: Nicolas Eder <nicolas.eder@lauterbach.com>
>
> neder (29):

I think you need to fix your author attribution here.

>   mcdstub initial commit, mcdstub file structure added
>   TCP chardev added, handshake with TRACE32 working
>   TCP packet handling added
>   queries for resets and triggers added
>   queries for memory spaces and register groups added
>   query for registers added
>   query data preparation improved
>   shared header file added, used for TCP packet data
>   memory and register query data now stored per core
>   handler for resets added
>   query for the VM state added
>   handler for reading registers added
>   handler for reading memory added
>   handler for single step added
>   adapting to the qemu coding style
>   deleting the mcdd startup option
>   handler for breakpoints and watchpoints added
>   making step and go handlers core-specific
>   adding trigger ID handling for TRACE32
>   cp register read/write added
>   switching between secure and non-secure memory added
>   transitioning to unsinged integers in TCP packets and removing
>     MCD-API-specific terms
>   moved all ARM code to the ARM mcdstub and added now commom header file
>   step and go handlers now propperly perform global operations
>   Doxygen documentation added
>   moved all mcd related header files into include/mcdstub
>   MCD stub entry added to maintainers file
>   added description to out-commented gdb function
>   introducing the DebugClass. It is used to abstract the gdb/mcd
>     set_stop_cpu function.

As you need to re-base anyway for this to apply cleanly I'm going to
wait until v3 for another pass. However I have noticed these patches are
quite noisy with a number of issues:

  - commented out code
  - code introduced then deleted
  - code motion after introduction
  - random white space changes

All of which makes it hard to review. A lot of this stems from the c&p
scaffolding from gdbstub which I understand as an approach to write the
initial version. However this should be squashed and merged away in the
final patches presented for review. Also please make sure:

  - commit messages match changes
  - each patch compiles cleanly on its own
  - you run through checkpatch.pl

Thanks,

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro


      parent reply	other threads:[~2023-10-13 16:54 UTC|newest]

Thread overview: 39+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-10-06  9:05 [PATCH v2 00/29] first version of mcdstub Nicolas Eder
2023-10-06  9:05 ` [PATCH v2 01/29] mcdstub initial commit, mcdstub file structure added Nicolas Eder
2023-10-13 15:51   ` Alex Bennée
2023-10-06  9:05 ` [PATCH v2 02/29] TCP chardev added, handshake with TRACE32 working Nicolas Eder
2023-10-13 16:15   ` Alex Bennée
2023-10-06  9:05 ` [PATCH v2 03/29] TCP packet handling added Nicolas Eder
2023-10-06  9:05 ` [PATCH v2 04/29] queries for resets and triggers added Nicolas Eder
2023-10-06  9:05 ` [PATCH v2 05/29] queries for memory spaces and register groups added Nicolas Eder
2023-10-06  9:05 ` [PATCH v2 06/29] query for registers added Nicolas Eder
2023-10-13 16:38   ` Alex Bennée
2023-10-06  9:05 ` [PATCH v2 07/29] query data preparation improved Nicolas Eder
2023-10-06  9:05 ` [PATCH v2 08/29] shared header file added, used for TCP packet data Nicolas Eder
2023-10-06  9:05 ` [PATCH v2 09/29] memory and register query data now stored per core Nicolas Eder
2023-10-06  9:05 ` [PATCH v2 10/29] handler for resets added Nicolas Eder
2023-10-06  9:05 ` [PATCH v2 11/29] query for the VM state added Nicolas Eder
2023-10-06  9:05 ` [PATCH v2 12/29] handler for reading registers added Nicolas Eder
2023-10-13 16:40   ` Alex Bennée
2023-10-06  9:05 ` [PATCH v2 13/29] handler for reading memory added Nicolas Eder
2023-10-06  9:05 ` [PATCH v2 14/29] handler for single step added Nicolas Eder
2023-10-06  9:05 ` [PATCH v2 15/29] adapting to the qemu coding style Nicolas Eder
2023-10-06  9:05 ` [PATCH v2 16/29] deleting the mcdd startup option Nicolas Eder
2023-10-06  9:05 ` [PATCH v2 17/29] handler for breakpoints and watchpoints added Nicolas Eder
2023-10-06  9:05 ` [PATCH v2 18/29] making step and go handlers core-specific Nicolas Eder
2023-10-06  9:06 ` [PATCH v2 19/29] adding trigger ID handling for TRACE32 Nicolas Eder
2023-10-06  9:06 ` [PATCH v2 20/29] cp register read/write added Nicolas Eder
2023-10-06  9:06 ` [PATCH v2 21/29] switching between secure and non-secure memory added Nicolas Eder
2023-10-06  9:06 ` [PATCH v2 22/29] transitioning to unsinged integers in TCP packets and removing MCD-API-specific terms Nicolas Eder
2023-10-06  9:06 ` [PATCH v2 23/29] moved all ARM code to the ARM mcdstub and added now commom header file Nicolas Eder
2023-10-06  9:06 ` [PATCH v2 24/29] step and go handlers now propperly perform global operations Nicolas Eder
2023-10-06  9:06 ` [PATCH v2 25/29] Doxygen documentation added Nicolas Eder
2023-10-13 16:34   ` Alex Bennée
2023-10-06  9:06 ` [PATCH v2 26/29] moved all mcd related header files into include/mcdstub Nicolas Eder
2023-10-13 16:45   ` Alex Bennée
2023-10-06  9:06 ` [PATCH v2 27/29] MCD stub entry added to maintainers file Nicolas Eder
2023-10-13 16:46   ` Alex Bennée
2023-10-06  9:06 ` [PATCH v2 28/29] added description to out-commented gdb function Nicolas Eder
2023-10-06  9:06 ` [PATCH v2 29/29] introducing the DebugClass. It is used to abstract the gdb/mcd set_stop_cpu function Nicolas Eder
2023-10-06  9:50 ` [PATCH v2 00/29] first version of mcdstub Philippe Mathieu-Daudé
2023-10-13 16:47 ` Alex Bennée [this message]

Reply instructions:

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

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

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

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

  git send-email \
    --in-reply-to=87mswmlaz9.fsf@linaro.org \
    --to=alex.bennee@linaro.org \
    --cc=Christian.Boenig@lauterbach.com \
    --cc=nicolas.eder@lauterbach.com \
    --cc=philmd@linaro.org \
    --cc=qemu-devel@nongnu.org \
    /path/to/YOUR_REPLY

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

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.