On 17.12.21 16:14, Heikki Krogerus wrote:

Hi,

On Fri, Dec 17, 2021 at 03:32:59PM +0100, Thorsten Leemhuis wrote:

quoted

Lo! Thx for working out a fix this quickly!

I'm just the regression tracker, but I think there are a few minor
details to improve here.

On 17.12.21 15:03, Heikki Krogerus wrote:

quoted

The driver must make sure there is an actual connection
before checking details about the USB Power Delivery
contract. Those details are not valid unless there is a
connection.

This fixes NULL pointer dereference that is caused by an
attempt to register bogus partner alternate mode that the
firmware on some platform may report before the actual
connection.

Fixes: 6cbe4b2d5a3f ("usb: typec: ucsi: Check the partner alt modes always if there is PD contract")
BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=215117

BugLink? Is that a tag we really use? Then I'm unaware of it. Greg is
the expert and can likely clarify, but that line afaik needs to replaced
by this:

Although not yet documented, it is the appropriate tag for the link to
the bug.

For you maybe. But it kind of becomes a mess if various people create
different tags, as they do now (you are just one of them).

It makes it clear that the link is to the bug and not to
the discussion on the list.

I agree that some clarification is needed, that's why I recently
proposed something:
https://lore.kernel.org/lkml/cover.1639042966.git.linux@leemhuis.info/ (local)

Maybe chime in there.

quoted

Link: https://bugzilla.kernel.org/show_bug.cgi?id=215117
Link:
https://lore.kernel.org/linux-usb/bug-215117-208809@https.bugzilla.kernel.org%2F/ (local)

Normally the last line would need a 's!linux-usb!r!', but seems the
kernel.org redirector doesn't work well in this particular case, so I
guess it's better this way than not at all :-/

The second line will also make the regression tracking bot automatically
close the issue (but I fear it might also fail due to the slash at the
end of the message-id :-/)

Greg will add the "Link" tag to the commit when, and if, he actually
takes the patch.

No, that is another Link tag. Let me quote
Documentation/process/submitting-patches.rst:

If related discussions or any other background information behind the
change can be found on the web, add 'Link:' tags pointing to it. In case
your patch fixes a bug, for example, add a tag with a URL referencing
the report in the mailing list archives or a bug tracker;

This concept is old, but the text was reworked recently to make this use
case for the Link: tag clearer.
For details see: https://git.kernel.org/linus/1f57bd42b77c

As the issue was discussed in a bug tracker and on the list, please add
Link tags to both places.

I do not add it because I do not want any bots to
react to the patch before it has actually been accepted.

The bug shouldn't be closed before the fix has really been accepted.

quoted

I think this line should be there as well:

Reported-by: Chris Hixon <redacted>

+Chris

This is true. I'll add the Reported-by tag if it's OK to you Chris?

thx!

Ciao, Thorsten