lkml.org 
[lkml]   [2023]   [Aug]   [4]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH v2] usb: typec: tcpm: IS_ERR_OR_NULL check for port->partner
From
On 8/4/23 02:24, Ladislav Michl wrote:
> Hi Jimmy,
>
> On Fri, Aug 04, 2023 at 08:49:28AM +0000, Jimmy Hu wrote:
>> port->partner may be an error or NULL, so we must check it with
>> IS_ERR_OR_NULL() before dereferencing it.
>>
>> Move the check to the beginning of the tcpm_handle_vdm_request function.
>>
>> Fixes: 5e1d4c49fbc8 ("usb: typec: tcpm: Determine common SVDM Version")
>> Signed-off-by: Jimmy Hu <hhhuuu@google.com>
>> ---
>> drivers/usb/typec/tcpm/tcpm.c | 10 ++++++----
>> 1 file changed, 6 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
>> index 829d75ebab42..2c6a0af155ab 100644
>> --- a/drivers/usb/typec/tcpm/tcpm.c
>> +++ b/drivers/usb/typec/tcpm/tcpm.c
>> @@ -1683,10 +1683,6 @@ static int tcpm_pd_svdm(struct tcpm_port *port, struct typec_altmode *adev,
>> (VDO_SVDM_VERS(typec_get_negotiated_svdm_version(typec)));
>> break;
>> case CMDT_RSP_ACK:
>> - /* silently drop message if we are not connected */
>> - if (IS_ERR_OR_NULL(port->partner))
>> - break;
>> -
>> tcpm_ams_finish(port);
>>
>> switch (cmd) {
>> @@ -1792,6 +1788,12 @@ static void tcpm_handle_vdm_request(struct tcpm_port *port,
>> u32 response[8] = { };
>> int i, rlen = 0;
>>
>> + /* silently drop message if we are not connected */
>
> ...comment moved.
>
>> + if (IS_ERR_OR_NULL(port->partner)) {
>> + dev_warn(port->dev, "port partner is an error or NULL\n");
>
> But code is actually not silent. Also, does the verbsity make sense? And
> if it does, is knowing what error port->partner is containing usefull?
>

Not only that, it also changes behavior radically from ignoring certain messages
while not connected or after partner registration failed to rejecting all VDM
messages, even those not requiring any partner data. It now also ignores all VDM
messages if the state machine is not in READY state. This is a significant functional
change which needs _much_ more explanation than provided in the patch description.

Guenter

\
 
 \ /
  Last update: 2023-08-04 16:37    [W:0.052 / U:0.140 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site