Re: [PATCH v2] usb: typec: tcpm: IS_ERR_OR_NULL check for port->partner

[Guenter Roeck]

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@xxxxxxxxxx>
> > ---
> >   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