[PATCH] ptp: Set lookup cookie when creating a PTP PPS source.

[PATCH] ptp: Set lookup cookie when creating a PTP PPS source.

[Jonathan Lemon]

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(+)

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);
-- 
2.30.2

Re: [PATCH] ptp: Set lookup cookie when creating a PTP PPS source.

[Richard Cochran]

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

Re: [PATCH] ptp: Set lookup cookie when creating a PTP PPS source.

[Jonathan Lemon]

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.
-- 
Joanthan

Re: [PATCH] ptp: Set lookup cookie when creating a PTP PPS source.

[Vladimir Oltean]

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?

Re: [PATCH] ptp: Set lookup cookie when creating a PTP PPS source.

[Richard Cochran]

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

Re: [PATCH] ptp: Set lookup cookie when creating a PTP PPS source.

[Jonathan Lemon]

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.
-- 
Jonathan

Re: [PATCH] ptp: Set lookup cookie when creating a PTP PPS source.

[patchwork-bot+netdevbpf]

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

Re: [PATCH] ptp: Set lookup cookie when creating a PTP PPS source.

[Richard Cochran]

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