Re: [bug report] usb: typec: Add driver for Thunderbolt 3 Alternate Mode

[Benson Leung <bleung@google.com>]

[-- Attachment #1: Type: text/plain, Size: 4597 bytes --]

Hi Abhishek,


On Wed, Jan 22, 2025 at 04:00:47PM -0800, Abhishek Pandit-Subedi wrote:
> On Tue, Jan 7, 2025 at 6:24 AM Heikki Krogerus
> <heikki.krogerus@linux.intel.com> wrote:
> >
> > +Abhishek
> >
> > On Tue, Jan 07, 2025 at 05:16:43PM +0300, Dan Carpenter wrote:
> > > Hello Heikki Krogerus,
> > >
> > > Commit 100e25738659 ("usb: typec: Add driver for Thunderbolt 3
> > > Alternate Mode") from Dec 13, 2024 (linux-next), leads to the
> > > following (unpublished) Smatch static checker warnings:
> > >
> > > drivers/usb/typec/altmodes/thunderbolt.c:116 tbt_altmode_work() warn: why is zero skipped 'i'
> > > drivers/usb/typec/altmodes/thunderbolt.c:147 tbt_enter_modes_ordered() warn: why is zero skipped 'i'
> > > drivers/usb/typec/altmodes/thunderbolt.c:153 tbt_enter_modes_ordered() info: return a literal instead of 'ret'
> > > drivers/usb/typec/altmodes/thunderbolt.c:328 tbt_altmode_remove() warn: why is zero skipped 'i'
> > > drivers/usb/typec/altmodes/thunderbolt.c:354 tbt_ready() warn: 'plug' is not an error pointer
> > >
> > > drivers/usb/typec/altmodes/thunderbolt.c
> > >     66 static void tbt_altmode_work(struct work_struct *work)
> > >     67 {
> > >     68         struct tbt_altmode *tbt = container_of(work, struct tbt_altmode, work);
> > >     69         int ret;
> > >     70
> > >     71         mutex_lock(&tbt->lock);
> > >     72
> > >     73         switch (tbt->state) {
> > >     74         case TBT_STATE_SOP_P_ENTER:
> > >     75                 ret = typec_cable_altmode_enter(tbt->alt, TYPEC_PLUG_SOP_P, NULL);
> > >     76                 if (ret) {
> > >     77                         dev_dbg(&tbt->plug[TYPEC_PLUG_SOP_P]->dev,
> > >     78                                 "failed to enter mode (%d)\n", ret);
> > >     79                         goto disable_plugs;
> > >     80                 }
> > >     81                 break;
> > >     82         case TBT_STATE_SOP_PP_ENTER:
> > >     83                 ret = typec_cable_altmode_enter(tbt->alt, TYPEC_PLUG_SOP_PP,  NULL);
> > >     84                 if (ret) {
> > >     85                         dev_dbg(&tbt->plug[TYPEC_PLUG_SOP_PP]->dev,
> > >     86                                 "failed to enter mode (%d)\n", ret);
> > >     87                         goto disable_plugs;
> > >     88                 }
> > >     89                 break;
> > >     90         case TBT_STATE_ENTER:
> > >     91                 ret = tbt_enter_mode(tbt);
> > >     92                 if (ret)
> > >     93                         dev_dbg(&tbt->alt->dev, "failed to enter mode (%d)\n",
> > >     94                                 ret);
> > >     95                 break;
> > >     96         case TBT_STATE_EXIT:
> > >     97                 typec_altmode_exit(tbt->alt);
> > >     98                 break;
> > >     99         case TBT_STATE_SOP_PP_EXIT:
> > >     100                 typec_cable_altmode_exit(tbt->alt, TYPEC_PLUG_SOP_PP);
> > >     101                 break;
> > >     102         case TBT_STATE_SOP_P_EXIT:
> > >     103                 typec_cable_altmode_exit(tbt->alt, TYPEC_PLUG_SOP_P);
> > >     104                 break;
> > >     105         default:
> > >     106                 break;
> > >     107         }
> > >     108
> > >     109         tbt->state = TBT_STATE_IDLE;
> > >     110
> > >     111         mutex_unlock(&tbt->lock);
> > >     112         return;
> > >     113
> > >     114 disable_plugs:
> > >     115         for (int i = TYPEC_PLUG_SOP_PP; i > 0; --i) {
> > >                                                 ^^^^^
> > > These should be >= 0.  Humans are bad at counting backwards.
> > >
> > > --> 116                 if (tbt->plug[i])
> > >     117                         typec_altmode_put_plug(tbt->plug[i]);
> > >     118
> > >     119                 tbt->plug[i] = NULL;
> > >     120         }
> > >     121
> > >     122         tbt->state = TBT_STATE_ENTER;
> > >     123         schedule_work(&tbt->work);
> > >     124         mutex_unlock(&tbt->lock);
> > >     125 }
> >
> > Abhishek, this looks straighforward, but just in case, can you take
> > look?
> >
> > thanks,
> >
> > --
> > heikki
> 
> +Benson Leung +Łukasz Bartosik can help with a patch to address this
> while I'm out on baby bonding leave. As you noted, they look pretty
> straightforward.

Thanks Dan for the report on these. I agree, seems simple to fix this one.
I'll take a look at the others in this file too.

+Andrei Kuchynski as well.

Thanks,
Benson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]