| Message ID | 20210628182533.2930715-1-jonathan.lemon@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 8602e40fc8132383298f304ae060d80f210be23c |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | ptp: Set lookup cookie when creating a PTP PPS source. | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
On Mon, Jun 28, 2021 at 11:25:33AM -0700, Jonathan Lemon wrote: > When creating a PTP device, the configuration block allows > creation of an associated PPS device. However, there isn't > any way to associate the two devices after creation. > > Set the PPS cookie, so pps_lookup_dev(ptp) performs correctly. Setting lookup_cookie is harmless, AFAICT, but I wonder about the use case. The doc for pps_lookup_dev() says, * This is a bit of a kludge that is currently used only by the PPS * serial line discipline. and indeed that is the case. The PHC devices are never serial ports, so how does this help? Thanks, Richard
On Mon, Jun 28, 2021 at 04:38:35PM -0700, Richard Cochran wrote: > On Mon, Jun 28, 2021 at 11:25:33AM -0700, Jonathan Lemon wrote: > > When creating a PTP device, the configuration block allows > > creation of an associated PPS device. However, there isn't > > any way to associate the two devices after creation. > > > > Set the PPS cookie, so pps_lookup_dev(ptp) performs correctly. > > Setting lookup_cookie is harmless, AFAICT, but I wonder about the use > case. The doc for pps_lookup_dev() says, > > * This is a bit of a kludge that is currently used only by the PPS > * serial line discipline. > > and indeed that is the case. > > The PHC devices are never serial ports, so how does this help? This is for the ptp_ocp driver, I have an update coming. The systems I'm using have the OpenCompute time card and several nics, all of which publish PTP/PPS devices. When there are several PPS devices, this patch allows selecting the correct one.
Hello: This patch was applied to netdev/net-next.git (refs/heads/master): On Mon, 28 Jun 2021 11:25:33 -0700 you wrote: > When creating a PTP device, the configuration block allows > creation of an associated PPS device. However, there isn't > any way to associate the two devices after creation. > > Set the PPS cookie, so pps_lookup_dev(ptp) performs correctly. > > Signed-off-by: Jonathan Lemon <jonathan.lemon@gmail.com> > > [...] Here is the summary with links: - ptp: Set lookup cookie when creating a PTP PPS source. https://git.kernel.org/netdev/net-next/c/8602e40fc813 You are awesome, thank you! -- Deet-doot-dot, I am a bot. https://korg.docs.kernel.org/patchwork/pwbot.html
On Tue, Jun 29, 2021 at 06:40:04PM +0000, patchwork-bot+netdevbpf@kernel.org wrote: > Hello: > > This patch was applied to netdev/net-next.git (refs/heads/master): Why is this bot applying patches to net-next with issues that are under review? Thanks, Richard
On Mon, Jun 28, 2021 at 04:38:35PM -0700, Richard Cochran wrote: > On Mon, Jun 28, 2021 at 11:25:33AM -0700, Jonathan Lemon wrote: > > When creating a PTP device, the configuration block allows > > creation of an associated PPS device. However, there isn't > > any way to associate the two devices after creation. > > > > Set the PPS cookie, so pps_lookup_dev(ptp) performs correctly. > > Setting lookup_cookie is harmless, AFAICT, but I wonder about the use > case. The doc for pps_lookup_dev() says, Harmless you say? Let's look at the code in a larger context: struct ptp_clock *ptp_clock_register(struct ptp_clock_info *info, struct device *parent) { struct ptp_clock *ptp; ... ptp = kzalloc(...); ... ptp->info = info; ... if (ptp->info->do_aux_work) { ... + ptp->pps_source->lookup_cookie = ptp; } /* Register a new PPS source. */ if (info->pps) { struct pps_source_info pps; ... ptp->pps_source = pps_register_source(&pps, PTP_PPS_DEFAULTS); ... } Notice anything out of the ordinary? Like perhaps the fact that ptp->pps_source is an arbitrary NULL pointer at the time the assignment to ->lookup_cookie is being made, because it is being created later? How on earth is this patch supposed to work?
On Fri, Jul 02, 2021 at 03:39:36AM +0300, Vladimir Oltean wrote: > Like perhaps the fact that ptp->pps_source is an arbitrary NULL pointer > at the time the assignment to ->lookup_cookie is being made, because it > is being created later? Oops. @Jonathan please submit a fix as soon as you can. This patch is already in net-next. Thanks, Richard
On Fri, Jul 02, 2021 at 03:39:36AM +0300, Vladimir Oltean wrote: > On Mon, Jun 28, 2021 at 04:38:35PM -0700, Richard Cochran wrote: > > On Mon, Jun 28, 2021 at 11:25:33AM -0700, Jonathan Lemon wrote: > > > When creating a PTP device, the configuration block allows > > > creation of an associated PPS device. However, there isn't > > > any way to associate the two devices after creation. > > > > > > Set the PPS cookie, so pps_lookup_dev(ptp) performs correctly. > > > > Setting lookup_cookie is harmless, AFAICT, but I wonder about the use > > case. The doc for pps_lookup_dev() says, > > Harmless you say? > > Let's look at the code in a larger context: > > struct ptp_clock *ptp_clock_register(struct ptp_clock_info *info, > struct device *parent) > { > struct ptp_clock *ptp; > > ... > ptp = kzalloc(...); > ... > ptp->info = info; > ... > > if (ptp->info->do_aux_work) { > ... > + ptp->pps_source->lookup_cookie = ptp; > } > > /* Register a new PPS source. */ > if (info->pps) { > struct pps_source_info pps; > ... > ptp->pps_source = pps_register_source(&pps, PTP_PPS_DEFAULTS); > ... > } > > Notice anything out of the ordinary? > Like perhaps the fact that ptp->pps_source is an arbitrary NULL pointer > at the time the assignment to ->lookup_cookie is being made, because it > is being created later? > > How on earth is this patch supposed to work? It was added to the wrong code block. Checking my tree, I seem to have it located twice - probably a bad patch that I didn't notice, and since I don't have an do_aux_work, the first one didn't trigger. Correction follows.
diff --git a/drivers/ptp/ptp_clock.c b/drivers/ptp/ptp_clock.c index c176fa82df85..45429853d60f 100644 --- a/drivers/ptp/ptp_clock.c +++ b/drivers/ptp/ptp_clock.c @@ -240,6 +240,7 @@ struct ptp_clock *ptp_clock_register(struct ptp_clock_info *info, pr_err("failed to create ptp aux_worker %d\n", err); goto kworker_err; } + ptp->pps_source->lookup_cookie = ptp; } err = ptp_populate_pin_groups(ptp);
When creating a PTP device, the configuration block allows creation of an associated PPS device. However, there isn't any way to associate the two devices after creation. Set the PPS cookie, so pps_lookup_dev(ptp) performs correctly. Signed-off-by: Jonathan Lemon <jonathan.lemon@gmail.com> --- drivers/ptp/ptp_clock.c | 1 + 1 file changed, 1 insertion(+)