Re: [PATCH v1] usb: typec: class: fix typec_altmode_put_partner to put plugs

[Heikki Krogerus]

Hi Christian,

On Fri, Dec 29, 2023 at 04:32:16PM +0100, Christian A. Ehrhardt wrote:
> 
> 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@xxxxxxxxxxxxxx/
> 
> 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.

Thanks for checking this. Your analysis sounds correct to me.

RD, I think we need to revert the commmit. If you still see the original
problem, please prepare a new patch.

thanks,

-- 
heikki