Received-SPF: pass (google.com: domain of linux-kernel+bounces-13141-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Date: Fri, 29 Dec 2023 16:32:16 +0100
From: "Christian A. Ehrhardt" <lk@c--e.de>
To: RD Babiera <rdbabiera@google.com>
Cc: heikki.krogerus@linux.intel.com, gregkh@linuxfoundation.org,
	linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org,
	badhri@google.com, stable@vger.kernel.org,
	Chris Bainbridge <chris.bainbridge@gmail.com>
Subject: Re: [PATCH v1] usb: typec: class: fix typec_altmode_put_partner to
 put plugs
Message-ID: <ZY7mgMkoaZDZGua4@cae.in-ulm.de>
References: <20231121203954.173364-2-rdbabiera@google.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20231121203954.173364-2-rdbabiera@google.com>


Hi,

I found this mail in the archives after looking at a bug report
that was bisected to the change that resulted from the following
analysis:

https://lore.kernel.org/all/CAP-bSRb3SXpgo_BEdqZB-p1K5625fMegRZ17ZkPE1J8ZYgEHDg@mail.gmail.com/

AFAICS the analysis below is partially flawed

On Tue, Nov 21, 2023 at 08:39:55PM +0000, RD Babiera wrote:
> When releasing an Alt Mode, typec_altmode_release called by a plug device
> will not release the plug Alt Mode, meaning that a port will hold a
> reference to a plug Alt Mode even if the port partner is unregistered.
> As a result, typec_altmode_get_plug() can return an old plug altmode.
> 
> Currently, typec_altmode_put_partner does not raise issues
> when unregistering a partner altmode. Looking at the current
> implementation:
> 
> > static void typec_altmode_put_partner(struct altmode *altmode)
> > {
> >	struct altmode *partner = altmode->partner;
> 
> When called by the partner Alt Mode, then partner evaluates to the port's
> Alt Mode. When called by the plug Alt Mode, this also evaluates to the
> port's Alt Mode.
> 
> >	struct typec_altmode *adev;
> >
> >	if (!partner)
> >		return;
> >
> >	adev = &partner->adev;
> 
> This always evaluates to the port's typec_altmode
> 
> >	if (is_typec_plug(adev->dev.parent)) {
> >		struct typec_plug *plug = to_typec_plug(adev->dev.parent);
> >
> >		partner->plug[plug->index] = NULL;
> 
> If the routine is called to put the plug's Alt mode and altmode refers to
> the plug, then adev referring to the port can never be a typec_plug. If
> altmode refers to the port, adev will always refer to the port partner,
> which runs the block below.
> 
> >	} else {
> >		partner->partner = NULL;
> >	}
> >	put_device(&adev->dev);
> > }

So far everything is fine.

> When calling typec_altmode_set_partner, a registration always calls
> get_device() on the port partner or the plug being registered,

This is wrong. It is the altmode of the plug or partner
that holds a reference to the altmode of the port not the other
way around. The port's altmode has (back) pointers to the altmodes
of its partner and the cable plugs but these are weak references that
do not contribute to the refcount.

> therefore
> typec_altmode_put_partner should put_device() the same device. By changing

Thus this conclusion is wrong. The put_device() used to be correct.

> adev to altmode->adev, we make sure to put the correct device and properly
> unregister plugs. The reason port partners are always properly
> unregistered is because even when adev refers to the port, the port
> partner gets nullified in the else block. The port device currently gets
> put().

Please correct me if I missed something.

       regards    Christian