Hi Quentin,

On Wed, 15 Jan 2025 at 03:58, Quentin Schulz quentin.schulz@cherry.de wrote:

Hi Liya,

On 1/14/25 8:09 AM, 1425075683@qq.com wrote:

...

[You don't often get email from 1425075683@qq.com. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]

From: Liya Huang 1425075683@qq.com

This patch updates the alignment of linker lists to use the CONFIG_LINKER_LIST_ALIGN macro instead of a hardcoded value. This ensures that the alignment is consistent with the configuration. Replace __attribute__((unused)) with __maybe_unused and __always_unused to eliminate the warning of checkpatch.pl.

Reviewed-by: Simon Glass sjg@chromium.org

This... is odd. I don't see an earlier version of that patch and your name/mail only returns two patches on the U-Boot mailing list. I believe Reviewed-by needs to be publicly given on the ML as they are a proof of review by the mentioned person and is usually a trust mark for maintainers to merge code. Here it may have been added without Simon's consent. Now imagine Simon is not answering for a few days/weeks, the maintainer could still believe Simon went through a proper review and merge that patch taking that into account while no review may actually have been conducted. This is making me uncomfortable.

Same issue for https://lore.kernel.org/u-boot/tencent_CCE8303926957C427AAE06F9D91282458109@....

I'm not saying review wasn't done properly, but its acknowledgment should be made public by the mentioned person instead of being put into the v1.

Have I missed an earlier version or discussion maybe?

I was wondering the same thing, actually.

I don't see anything wrong with the code in this patch, though.

The docs are here: https://docs.u-boot.org/en/latest/develop/sending_patches.html

Regards, Simon

Hi,

On 1/16/25 1:24 AM, 黄利亚 wrote:

Sorry for the confusion and disruption.  I haven't submitted patches this way before,  and my English is not very good.  I was conducted according to the steps  in the https://docs.u-boot.org/en/latest/develop/patman.html operation.

The patch is fine, it is properly formatted and sent correctly so that is okay.

In the documentation[1], you can read:

""" Tested-by / Reviewed-by / Acked-by

These indicate that someone has tested/reviewed/acked your patch. When you get this reply on the mailing list, you can add this tag to the relevant commit and the script will include it when you send out the next version. If ‘Tested-by:’ is set to yourself, it will be removed. No one will believe you. """

Specifically: "when you get this reply on the mailing list". Since you didn't send the patch on the mailing list before (as this is the first version), you didn't get a reply on the mailing list yet so you cannot add "Reviewed-by: " from Simon (yet). If Simon gives his Reviewed-by publicly on the mailing list and if someone else requests a change, you will need to send a second version (v2) of your patch, then you need to add "Reviewed-by:" from Simon.

Did you add the Reviewed-by from Simon manually in the commit log? Did patman add it automatically? It would be nice for us to know if it's a bug in the tooling we use so we can fix it.

If you added the tag manually, what was your intent? Did you want to specifically request Simon to review your patch? If this is what you wanted, you can use "Cc: " instead.

For what it's worth, I personally use `b4` now (but I have never used patman before so I don't know how what would be best for your workflow). You can have a look there: https://b4.docs.kernel.org/en/latest/contributor/overview.html#what-s-the-b4....

[1] https://docs.u-boot.org/en/latest/develop/patman.html#how-to-add-tags

Can you tell me what else I need to do?  What I know so far is :

1. I should use another email address; 

The email address is okay, but you are not allowed to put Reviewed-by: <person> in the commit log if <person> didn't actually **publicly** told you in a mail on the mailing list. For example,

https://lore.kernel.org/u-boot/20241220222612.1757884-1-trini@konsulko.com/T... is the first version (v1) of a patch. See there's no Reviewed-by tag in the commit log.

Then, Heinrich answers with Reviewed-by: Heinrich Schuchardt xypron.glpk@gmx.de c.f. https://lore.kernel.org/u-boot/20241220222612.1757884-1-trini@konsulko.com/T...

Then you can see in the second version of the patch, here https://lore.kernel.org/u-boot/20241220222612.1757884-1-trini@konsulko.com/T... that the Reviewed-by tag from Heinrich is added in the commit log.

This is okay, because Heinrich publicly said they reviewed Tom's patch by sending a mail with "Reviewed-by: Heinrich Schuchardt xypron.glpk@gmx.de" in it.

2. Seems like i shouldn't send patches directly to this list?

&nbsp;And it's only submitted to the reviewer for review and then submitted by the reviewer?

You did almost everything correctly! The only change you need to do is remove the "Reviewed-by:" lines from the commit log in your first version. The rest is perfectly ok with the normal process!

Let me know if you want me to rephrase or clarify something.

Cheers, Quentin