[PATCH v2] usb: typec: ucsi: Only check the contract if there is a connection

[PATCH v2] usb: typec: ucsi: Only check the contract if there is a connection

[Heikki Krogerus]

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.

Reported-by: Chris Hixon <linux-kernel-bugs@hixontech.com>
Fixes: 6cbe4b2d5a3f ("usb: typec: ucsi: Check the partner alt modes always if there is PD contract")
Link: https://bugzilla.kernel.org/show_bug.cgi?id=215117
Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---

Hi,

Instead of using the "BugLink" tag, I'm now using "Link" tag with the
link to the bug as requested.

There was a request to have also another Link tag pointing to some
other discussion on the mailing list, but I failed to understand what
was the point with that - I also didn't find any commits where
something like that had been used before.

I may be mistaken here, but I got the impression that you create the
mailing list discussion just so you can have the extra Link tag
pointing to it, and that Link tag you want only because you have made
your scripts rely on it.

The extra email thread in any case does not seem to contain any real
additional information that the bug report does not have, so the extra
Link tag pointing to it does not provide any real value on top of the
link to bug itself.

thanks,

--
 drivers/usb/typec/ucsi/ucsi.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
index 9d6b7e02d6efb..f0c2fa19f3e0f 100644
--- a/drivers/usb/typec/ucsi/ucsi.c
+++ b/drivers/usb/typec/ucsi/ucsi.c
@@ -1164,7 +1164,9 @@ static int ucsi_register_port(struct ucsi *ucsi, int index)
 		ret = 0;
 	}
 
-	if (UCSI_CONSTAT_PWR_OPMODE(con->status.flags) == UCSI_CONSTAT_PWR_OPMODE_PD) {
+	if (con->partner &&
+	    UCSI_CONSTAT_PWR_OPMODE(con->status.flags) ==
+	    UCSI_CONSTAT_PWR_OPMODE_PD) {
 		ucsi_get_src_pdos(con);
 		ucsi_check_altmodes(con);
 	}
-- 
2.34.1

Re: [PATCH v2] usb: typec: ucsi: Only check the contract if there is a connection

[Thorsten Leemhuis]

On 21.12.21 15:03, Heikki Krogerus wrote:
> 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.
> 
> Reported-by: Chris Hixon <linux-kernel-bugs@hixontech.com>
> Fixes: 6cbe4b2d5a3f ("usb: typec: ucsi: Check the partner alt modes always if there is PD contract")
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=215117
> Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> ---
> 
> Hi,
> 
> Instead of using the "BugLink" tag, I'm now using "Link" tag with the
> link to the bug as requested.

Thx!

> There was a request to have also another Link tag pointing to some
> other discussion on the mailing list, but I failed to understand what
> was the point with that - I also didn't find any commits where
> something like that had been used before.

There are quite a few commits that use more that two "Link" tags, as
they should point to "related discussions or any other background
information behind the change" that "can be found on the web" (Quotes
from Documentation/process/submitting-patches.rst) -- hence there is no
limit. That being said:

> I may be mistaken here, but I got the impression that you create the
> mailing list discussion

You are mistaken: I created it because the bugzilla ticket was ignored
in bugzilla for weeks and might have forgotten otherwise -- no wonder,
was bugzilla.kernel.org is not the official place to report USB bugs
according to the MAINTAINERS file.

> just so you can have the extra Link tag
> pointing to it, and that Link tag you want only because you have made
> your scripts rely on it.

They rely on it as almost all subsystems expect bug to be reported by
mail, as they are instructed by
Documentation/admin-guide/reporting-issues.rst

> The extra email thread in any case does not seem to contain any real
> additional information that the bug report does not have, so the extra
> Link tag pointing to it does not provide any real value on top of the
> link to bug itself.

In this case that's true, as I have to tell regzbot about the fix then
manually (the plan it to make regzbot also detect links to
bugzilla.kernel.org, but I fear I have no time to work on that in the
next few weeks :-/ ). But in similar cases it's different, as there the
developers continued discussing the issue by mail -- then it's
definitely worth linking there as well.

Thx.

Ciao, Thorsten

> --
>  drivers/usb/typec/ucsi/ucsi.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
> index 9d6b7e02d6efb..f0c2fa19f3e0f 100644
> --- a/drivers/usb/typec/ucsi/ucsi.c
> +++ b/drivers/usb/typec/ucsi/ucsi.c
> @@ -1164,7 +1164,9 @@ static int ucsi_register_port(struct ucsi *ucsi, int index)
>  		ret = 0;
>  	}
>  
> -	if (UCSI_CONSTAT_PWR_OPMODE(con->status.flags) == UCSI_CONSTAT_PWR_OPMODE_PD) {
> +	if (con->partner &&
> +	    UCSI_CONSTAT_PWR_OPMODE(con->status.flags) ==
> +	    UCSI_CONSTAT_PWR_OPMODE_PD) {
>  		ucsi_get_src_pdos(con);
>  		ucsi_check_altmodes(con);
>  	}

Re: [PATCH v2] usb: typec: ucsi: Only check the contract if there is a connection

[Greg Kroah-Hartman]

On Tue, Dec 21, 2021 at 03:39:04PM +0100, Thorsten Leemhuis wrote:
> On 21.12.21 15:03, Heikki Krogerus wrote:
> > 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.
> > 
> > Reported-by: Chris Hixon <linux-kernel-bugs@hixontech.com>
> > Fixes: 6cbe4b2d5a3f ("usb: typec: ucsi: Check the partner alt modes always if there is PD contract")
> > Link: https://bugzilla.kernel.org/show_bug.cgi?id=215117
> > Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > ---
> > 
> > Hi,
> > 
> > Instead of using the "BugLink" tag, I'm now using "Link" tag with the
> > link to the bug as requested.
> 
> Thx!
> 
> > There was a request to have also another Link tag pointing to some
> > other discussion on the mailing list, but I failed to understand what
> > was the point with that - I also didn't find any commits where
> > something like that had been used before.
> 
> There are quite a few commits that use more that two "Link" tags, as
> they should point to "related discussions or any other background
> information behind the change" that "can be found on the web" (Quotes
> from Documentation/process/submitting-patches.rst) -- hence there is no
> limit. That being said:
> 
> > I may be mistaken here, but I got the impression that you create the
> > mailing list discussion
> 
> You are mistaken: I created it because the bugzilla ticket was ignored
> in bugzilla for weeks and might have forgotten otherwise -- no wonder,
> was bugzilla.kernel.org is not the official place to report USB bugs
> according to the MAINTAINERS file.
> 
> > just so you can have the extra Link tag
> > pointing to it, and that Link tag you want only because you have made
> > your scripts rely on it.
> 
> They rely on it as almost all subsystems expect bug to be reported by
> mail, as they are instructed by
> Documentation/admin-guide/reporting-issues.rst
> 
> > The extra email thread in any case does not seem to contain any real
> > additional information that the bug report does not have, so the extra
> > Link tag pointing to it does not provide any real value on top of the
> > link to bug itself.
> 
> In this case that's true, as I have to tell regzbot about the fix then
> manually (the plan it to make regzbot also detect links to
> bugzilla.kernel.org, but I fear I have no time to work on that in the
> next few weeks :-/ ). But in similar cases it's different, as there the
> developers continued discussing the issue by mail -- then it's
> definitely worth linking there as well.

So for this case, what type of tag should I add here to get rezbot to
manually pick this up?

confused,

greg k-h

Re: [PATCH v2] usb: typec: ucsi: Only check the contract if there is a connection

[Thorsten Leemhuis]

On 21.12.21 16:03, Greg Kroah-Hartman wrote:
> On Tue, Dec 21, 2021 at 03:39:04PM +0100, Thorsten Leemhuis wrote:
>> On 21.12.21 15:03, Heikki Krogerus wrote:
>>> 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.
>>>
>>> Reported-by: Chris Hixon <linux-kernel-bugs@hixontech.com>
>>> Fixes: 6cbe4b2d5a3f ("usb: typec: ucsi: Check the partner alt modes always if there is PD contract")
>>> Link: https://bugzilla.kernel.org/show_bug.cgi?id=215117
>>> Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
>>> ---
>>>
>>> Hi,
>>>
>>> Instead of using the "BugLink" tag, I'm now using "Link" tag with the
>>> link to the bug as requested.
>>
>> Thx!
>>
>>> There was a request to have also another Link tag pointing to some
>>> other discussion on the mailing list, but I failed to understand what
>>> was the point with that - I also didn't find any commits where
>>> something like that had been used before.
>>
>> There are quite a few commits that use more that two "Link" tags, as
>> they should point to "related discussions or any other background
>> information behind the change" that "can be found on the web" (Quotes
>> from Documentation/process/submitting-patches.rst) -- hence there is no
>> limit. That being said:
>>
>>> I may be mistaken here, but I got the impression that you create the
>>> mailing list discussion
>>
>> You are mistaken: I created it because the bugzilla ticket was ignored
>> in bugzilla for weeks and might have forgotten otherwise -- no wonder,
>> was bugzilla.kernel.org is not the official place to report USB bugs
>> according to the MAINTAINERS file.
>>
>>> just so you can have the extra Link tag
>>> pointing to it, and that Link tag you want only because you have made
>>> your scripts rely on it.
>>
>> They rely on it as almost all subsystems expect bug to be reported by
>> mail, as they are instructed by
>> Documentation/admin-guide/reporting-issues.rst
>>
>>> The extra email thread in any case does not seem to contain any real
>>> additional information that the bug report does not have, so the extra
>>> Link tag pointing to it does not provide any real value on top of the
>>> link to bug itself.
>>
>> In this case that's true, as I have to tell regzbot about the fix then
>> manually (the plan it to make regzbot also detect links to
>> bugzilla.kernel.org, but I fear I have no time to work on that in the
>> next few weeks :-/ ). But in similar cases it's different, as there the
>> developers continued discussing the issue by mail -- then it's
>> definitely worth linking there as well.
> 
> So for this case, what type of tag should I add here to get rezbot to
> manually pick this up?

Just commit it. Afterwards someone needs to send '#regzbot fixed-by:
<commit-id>' to the thread with the report, for example as a reply to
this mail:
https://lore.kernel.org/linux-usb/eb34f98f-00ef-3238-2daa-80481116035d@leemhuis.info/

But you can leave that to me, regzbot will alert me anyway once it sees
a fix with "Fixes: 6cbe4b2d5a3f". That's the commit that introduced the
regression. But making regzbot mark the entry as fixed solely on this
afaics would be a bad idea, as one commit can cause different
regressions :-/

Making regbzot detect "Link:" tags to bugzilla tickets it knowns about
is pretty high on my todo list for regzbot. I would have worked on this
already, but I have to focus on other things for now to fulfil the
milestones I promised to deliver with regzbot. :-/

Ciao, Thorsten

Re: [PATCH v2] usb: typec: ucsi: Only check the contract if there is a connection

[Greg Kroah-Hartman]

On Tue, Dec 21, 2021 at 04:24:30PM +0100, Thorsten Leemhuis wrote:
> 
> 
> On 21.12.21 16:03, Greg Kroah-Hartman wrote:
> > On Tue, Dec 21, 2021 at 03:39:04PM +0100, Thorsten Leemhuis wrote:
> >> On 21.12.21 15:03, Heikki Krogerus wrote:
> >>> 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.
> >>>
> >>> Reported-by: Chris Hixon <linux-kernel-bugs@hixontech.com>
> >>> Fixes: 6cbe4b2d5a3f ("usb: typec: ucsi: Check the partner alt modes always if there is PD contract")
> >>> Link: https://bugzilla.kernel.org/show_bug.cgi?id=215117
> >>> Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> >>> ---
> >>>
> >>> Hi,
> >>>
> >>> Instead of using the "BugLink" tag, I'm now using "Link" tag with the
> >>> link to the bug as requested.
> >>
> >> Thx!
> >>
> >>> There was a request to have also another Link tag pointing to some
> >>> other discussion on the mailing list, but I failed to understand what
> >>> was the point with that - I also didn't find any commits where
> >>> something like that had been used before.
> >>
> >> There are quite a few commits that use more that two "Link" tags, as
> >> they should point to "related discussions or any other background
> >> information behind the change" that "can be found on the web" (Quotes
> >> from Documentation/process/submitting-patches.rst) -- hence there is no
> >> limit. That being said:
> >>
> >>> I may be mistaken here, but I got the impression that you create the
> >>> mailing list discussion
> >>
> >> You are mistaken: I created it because the bugzilla ticket was ignored
> >> in bugzilla for weeks and might have forgotten otherwise -- no wonder,
> >> was bugzilla.kernel.org is not the official place to report USB bugs
> >> according to the MAINTAINERS file.
> >>
> >>> just so you can have the extra Link tag
> >>> pointing to it, and that Link tag you want only because you have made
> >>> your scripts rely on it.
> >>
> >> They rely on it as almost all subsystems expect bug to be reported by
> >> mail, as they are instructed by
> >> Documentation/admin-guide/reporting-issues.rst
> >>
> >>> The extra email thread in any case does not seem to contain any real
> >>> additional information that the bug report does not have, so the extra
> >>> Link tag pointing to it does not provide any real value on top of the
> >>> link to bug itself.
> >>
> >> In this case that's true, as I have to tell regzbot about the fix then
> >> manually (the plan it to make regzbot also detect links to
> >> bugzilla.kernel.org, but I fear I have no time to work on that in the
> >> next few weeks :-/ ). But in similar cases it's different, as there the
> >> developers continued discussing the issue by mail -- then it's
> >> definitely worth linking there as well.
> > 
> > So for this case, what type of tag should I add here to get rezbot to
> > manually pick this up?
> 
> Just commit it. Afterwards someone needs to send '#regzbot fixed-by:
> <commit-id>' to the thread with the report, for example as a reply to
> this mail:
> https://lore.kernel.org/linux-usb/eb34f98f-00ef-3238-2daa-80481116035d@leemhuis.info/
> 
> But you can leave that to me, regzbot will alert me anyway once it sees
> a fix with "Fixes: 6cbe4b2d5a3f". That's the commit that introduced the
> regression. But making regzbot mark the entry as fixed solely on this
> afaics would be a bad idea, as one commit can cause different
> regressions :-/

Ok, thanks, now committed to my tree, hopefully gets picked up
automagically...

greg k-h

Re: [PATCH v2] usb: typec: ucsi: Only check the contract if there is a connection

[Heikki Krogerus]

Hi Thorsten,

Tue, Dec 21, 2021 at 03:39:04PM +0100, Thorsten Leemhuis kirjoitti:
> On 21.12.21 15:03, Heikki Krogerus wrote:
> > 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.
> > 
> > Reported-by: Chris Hixon <linux-kernel-bugs@hixontech.com>
> > Fixes: 6cbe4b2d5a3f ("usb: typec: ucsi: Check the partner alt modes always if there is PD contract")
> > Link: https://bugzilla.kernel.org/show_bug.cgi?id=215117
> > Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > ---
> > 
> > Hi,
> > 
> > Instead of using the "BugLink" tag, I'm now using "Link" tag with the
> > link to the bug as requested.
> 
> Thx!
> 
> > There was a request to have also another Link tag pointing to some
> > other discussion on the mailing list, but I failed to understand what
> > was the point with that - I also didn't find any commits where
> > something like that had been used before.
> 
> There are quite a few commits that use more that two "Link" tags, as
> they should point to "related discussions or any other background
> information behind the change" that "can be found on the web" (Quotes
> from Documentation/process/submitting-patches.rst) -- hence there is no
> limit. That being said:
> 
> > I may be mistaken here, but I got the impression that you create the
> > mailing list discussion
> 
> You are mistaken: I created it because the bugzilla ticket was ignored
> in bugzilla for weeks and might have forgotten otherwise -- no wonder,
> was bugzilla.kernel.org is not the official place to report USB bugs
> according to the MAINTAINERS file.
> 
> > just so you can have the extra Link tag
> > pointing to it, and that Link tag you want only because you have made
> > your scripts rely on it.
> 
> They rely on it as almost all subsystems expect bug to be reported by
> mail, as they are instructed by
> Documentation/admin-guide/reporting-issues.rst
> 
> > The extra email thread in any case does not seem to contain any real
> > additional information that the bug report does not have, so the extra
> > Link tag pointing to it does not provide any real value on top of the
> > link to bug itself.
> 
> In this case that's true, as I have to tell regzbot about the fix then
> manually (the plan it to make regzbot also detect links to
> bugzilla.kernel.org, but I fear I have no time to work on that in the
> next few weeks :-/ ). But in similar cases it's different, as there the
> developers continued discussing the issue by mail -- then it's
> definitely worth linking there as well.

Thanks for the explanation. My concern was that we are expected to
supply duplicated information just so the bot is satisfied, but
that's clearly not the case. I misunderstood.

thanks,

-- 
heikki