On Mon, Jun 17, 2024 at 6:30 AM Dan Carpenter <dan.carpenter@xxxxxxxxxx> wrote:
>
> Hello Badhri Jagan Sridharan,
>
> Commit 599f008c257d ("usb: typec: tcpm: Add callbacks to mitigate
> wakeups due to contaminant") from Jan 14, 2023 (linux-next), leads to
> the following Smatch static checker warning:
>
> drivers/usb/typec/tcpm/tcpm.c:4620 run_state_machine()
> error: we previously assumed 'port->tcpc->check_contaminant' could be null (see line 4607)
>
> drivers/usb/typec/tcpm/tcpm.c
> 4600 static void run_state_machine(struct tcpm_port *port)
> 4601 {
> 4602 int ret;
> 4603 enum typec_pwr_opmode opmode;
> 4604 unsigned int msecs;
> 4605 enum tcpm_state upcoming_state;
> 4606
> 4607 if (port->tcpc->check_contaminant && port->state != CHECK_CONTAMINANT)
>
> Assume that ->check_contaminant is NULL and port->state == CHECK_CONTAMINANT
>
> 4608 port->potential_contaminant = ((port->enter_state == SRC_ATTACH_WAIT &&
> 4609 port->state == SRC_UNATTACHED) ||
> 4610 (port->enter_state == SNK_ATTACH_WAIT &&
> 4611 port->state == SNK_UNATTACHED) ||
> 4612 (port->enter_state == SNK_DEBOUNCED &&
> 4613 port->state == SNK_UNATTACHED));
> 4614
> 4615 port->enter_state = port->state;
> 4616 switch (port->state) {
> 4617 case TOGGLING:
> 4618 break;
> 4619 case CHECK_CONTAMINANT:
> --> 4620 port->tcpc->check_contaminant(port->tcpc);
Hi Dan,
We could add a null pointer check before invoking
port->tcpc->check_contaminant(), however, it would be redundant.
Unless port->potential_contaminant is set, TCPM would never call
tcpm_set_state(port, CHECK_CONTAMINANT..) and
port->potential_contaminant would not be set unless
port->tcpc->check_contaminant () is NOT NULL.
So the assumption of "->check_contaminant is NULL and port->state ==
CHECK_CONTAMINANT" is not possible in the code as it stands. But we
can add an explicit check for port->tcpc->check_contaminant() in
CHECK_CONTAMINANT to make it more obvious if needed.
Thanks,
Badhri.
>
>
> then we're in case CHECK_CONTAMINANT and the function pointer is NULL.
>
> 4621 break;
> 4622 /* SRC states */
> 4623 case SRC_UNATTACHED:
> 4624 if (!port->non_pd_role_swap)
> 4625 tcpm_swap_complete(port, -ENOTCONN);
> 4626 tcpm_src_detach(port);
> 4627 if (port->potential_contaminant) {
> 4628 tcpm_set_state(port, CHECK_CONTAMINANT, 0);
> 4629 break;
> 4630 }
> 4631 if (tcpm_start_toggling(port, tcpm_rp_cc(port))) {
> 4632 tcpm_set_state(port, TOGGLING, 0);
> 4633 break;
>
> regards,
> dan carpenter
