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); }
When calling typec_altmode_set_partner, a registration always calls get_device() on the port partner or the plug being registered, therefore typec_altmode_put_partner should put_device() the same device. By changing 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().
Fixes: 8a37d87d72f0 ("usb: typec: Bus type for alternate modes") Cc: stable@vger.kernel.org Signed-off-by: RD Babiera rdbabiera@google.com --- drivers/usb/typec/class.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/usb/typec/class.c b/drivers/usb/typec/class.c index 2e0451bd336e..803be1943445 100644 --- a/drivers/usb/typec/class.c +++ b/drivers/usb/typec/class.c @@ -267,7 +267,7 @@ static void typec_altmode_put_partner(struct altmode *altmode) if (!partner) return;
- adev = &partner->adev; + adev = &altmode->adev;
if (is_typec_plug(adev->dev.parent)) { struct typec_plug *plug = to_typec_plug(adev->dev.parent);
base-commit: b85ea95d086471afb4ad062012a4d73cd328fa86
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); }
When calling typec_altmode_set_partner, a registration always calls get_device() on the port partner or the plug being registered, therefore typec_altmode_put_partner should put_device() the same device. By changing 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().
Fixes: 8a37d87d72f0 ("usb: typec: Bus type for alternate modes") Cc: stable@vger.kernel.org Signed-off-by: RD Babiera rdbabiera@google.com
drivers/usb/typec/class.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/usb/typec/class.c b/drivers/usb/typec/class.c index 2e0451bd336e..803be1943445 100644 --- a/drivers/usb/typec/class.c +++ b/drivers/usb/typec/class.c @@ -267,7 +267,7 @@ static void typec_altmode_put_partner(struct altmode *altmode) if (!partner) return;
- adev = &partner->adev;
- adev = &altmode->adev;
if (is_typec_plug(adev->dev.parent)) { struct typec_plug *plug = to_typec_plug(adev->dev.parent);
Sorry, I may have missed something, but do we need to call this function with ports at all?
static void typec_altmode_release(struct device *dev) { struct altmode *alt = to_altmode(to_typec_altmode(dev));
- typec_altmode_put_partner(alt); + if (!is_typec_port(dev->parent)) + typec_altmode_put_partner(alt);
altmode_id_remove(alt->adev.dev.parent, alt->id); kfree(alt); ...
thanks,
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-p1K5625fMegRZ17ZkPE1J8ZYgE...
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
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-p1K5625fMegRZ17ZkPE1J8ZYgE...
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,
Hi Heikki,
That sounds good to me. Christian had proposed a fix in another email thread, so I can post the patch after testing on my end.
Thanks again Christian for the analysis and fix.
Best, --- RD
linux-stable-mirror@lists.linaro.org
-
Christian A. Ehrhardt -
Heikki Krogerus -
RD Babiera