Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230726020946.1409565-1-rdbabiera@google.com> <13b9a16f-0f26-23a4-1e2e-5b1cf9cb4070@roeck-us.net>
In-Reply-To: <13b9a16f-0f26-23a4-1e2e-5b1cf9cb4070@roeck-us.net>
From:   RD Babiera <rdbabiera@google.com>
Date:   Thu, 27 Jul 2023 11:16:05 -0700
Message-ID: <CALzBnUGYZXgocCmycqgf6FyCA9qmVYYj9vuVkeXFoQy=E1GiwA@mail.gmail.com>
Subject: Re: [PATCH v1] usb: typec: bus: verify partner exists in typec_altmode_attention
To:     Guenter Roeck <linux@roeck-us.net>
Cc:     heikki.krogerus@linux.intel.com, gregkh@linuxfoundation.org,
        badhri@google.com, linux-usb@vger.kernel.org,
        linux-kernel@vger.kernel.org, stable@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Tue, Jul 25, 2023 at 7:27=E2=80=AFPM Guenter Roeck <linux@roeck-us.net> =
wrote:
> Is this theory or actually observed ?

This is actually observed.

> This dereferences partner
>
> > +
> > +     if (!partner || !pdev)
>
> ... and then checks if partner is NULL.
>
> On top of that, pdev is not NULL even if partner is NULL because
> adev is not the first element of struct altmode.
>
> In summary, this code and the check as implemented does not make
> sense. Maybe partner can be NULL, but pdev will never be NULL.

After looking at it more carefully, I agree on the pdev assignment and chec=
k
being odd. I'll follow how typec_altmode_vdm handles the NULL partner case
and remove the NULL check on pdev.

> > +             return -ENODEV;
> >
> >       if (pdev->ops && pdev->ops->attention)
> >               pdev->ops->attention(pdev, vdo);
> > +     else
> > +             return -EOPNOTSUPP;
> > +
>
> So far this was explicitly permitted. Now it will log an error each time =
it is
> observed. I do not see the point of this log message; obviously it was
> not intended to be considered an error, and I do not understand why it sh=
ould
> suddenly be one that is worth clogging the log.

My rationale for logging anything is that it will make it easier to
identify port
partners that send Attention messages regardless of whether or not the
port registers the partner Alt Mode. You're right that this is not an error=
, so
I will treat this case as a successful return moving forward. Then the only
log generated will be for a port partner that sends Attention messages to a
port that hasn't registered an altmode for it.

Thanks for the feedback,
RD