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
