Messages in this thread Patch in this message | | | Date | Wed, 4 Mar 2020 15:11:44 -0500 (EST) | From | Vladis Dronov <> | Subject | Re: [PATCH] ptp: free ptp clock properly |
| |
Hello, Andrea, all,
----- Original Message ----- > From: "Andrea Righi" <andrea.righi@canonical.com> > Subject: [PATCH] ptp: free ptp clock properly > > There is a bug in ptp_clock_unregister() where ptp_clock_release() can > free up resources needed by posix_clock_unregister() to properly destroy > a related sysfs device. > > Fix this by calling posix_clock_unregister() in ptp_clock_release().
Honestly, this does not seem right. The calls at PTP clock release are:
ptp_clock_unregister() -> posix_clock_unregister() -> cdev_device_del() -> -> ... bla ... -> ptp_clock_release()
So, it looks like with this patch both posix_clock_unregister() and ptp_clock_release() are not called at all. And it looks like the "fix" is not removing PTP clock's cdev, i.e. leaking it and related sysfs resources.
I would guess that a kernel in question (5.3.0-40-generic) has the commit a33121e5487b but does not have the commit 75718584cb3c, which should be exactly fixing a docking station disconnect crash. Could you please, check this?
Why? We have 2 crash call traces. 1) the launchpad bug 2) the email which led to the commit 75718584cb3c creation (see Link:).
Aaaaand they are identical starting from device_release_driver_internal() and almost to the top.
> See also: > commit 75718584cb3c ("ptp: free ptp device pin descriptors properly"). > > BugLink: https://bugs.launchpad.net/bugs/1864754 > Fixes: a33121e5487b ("ptp: fix the race between the release of ptp_clock and > cdev") > Signed-off-by: Andrea Righi <andrea.righi@canonical.com> > --- > drivers/ptp/ptp_clock.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/drivers/ptp/ptp_clock.c b/drivers/ptp/ptp_clock.c > index ac1f2bf9e888..12951023d0c6 100644 > --- a/drivers/ptp/ptp_clock.c > +++ b/drivers/ptp/ptp_clock.c > @@ -171,6 +171,7 @@ static void ptp_clock_release(struct device *dev) > struct ptp_clock *ptp = container_of(dev, struct ptp_clock, dev); > > ptp_cleanup_pin_groups(ptp); > + posix_clock_unregister(&ptp->clock); > mutex_destroy(&ptp->tsevq_mux); > mutex_destroy(&ptp->pincfg_mux); > ida_simple_remove(&ptp_clocks_map, ptp->index); > @@ -303,8 +304,6 @@ int ptp_clock_unregister(struct ptp_clock *ptp) > if (ptp->pps_source) > pps_unregister_source(ptp->pps_source); > > - posix_clock_unregister(&ptp->clock); > - > return 0; > } > EXPORT_SYMBOL(ptp_clock_unregister); > -- > 2.25.0
Best regards, Vladis Dronov | Red Hat, Inc. | The Core Kernel | Senior Software Engineer
| |