On Tue, Apr 2, 2024 at 3:37 PM Thomas Gleixner <tglx@xxxxxxxxxxxxx> wrote: > > On Tue, Apr 02 2024 at 14:16, Mahesh Bandewar (महेश बंडेवार) wrote: > > On Tue, Apr 2, 2024 at 2:25 AM Thomas Gleixner <tglx@xxxxxxxxxxxxx> wrote: > >> Works as well. I'm not seing the point for CLOCK_MONOTONIC and the > >> change logs are not really telling anything about the problem being > >> solved.... > >> > > https://lore.kernel.org/lkml/20240104212431.3275688-1-maheshb@xxxxxxxxxx/T/#:~:text=*%20[PATCHv3%20net%2Dnext%200/3]%20add%20ptp_gettimex64any()%20API,21:24%20Mahesh%20Bandewar%200%20siblings%2C%200%20replies; > > > > This is the cover letter where I tried to explain the need for this. > > The justification for a patch needs to be in the change log and not in > the cover letter because the cover letter is not part of the git > history. > ack > > Granted, my current use case is for CLOCK_MONOTONIC_RAW but just > > because I don't have a use case doesn't mean someone else may not have > > it and hence added it. > > Then why did you not five other clock IDs? Someone else might have a > use case, no? > > While a syscall/ioctl should be flexible for future use, the kernel does > not add features just because there might be some use case. It's > documented how this works. > I see your point. I don't mind removing the CLOCK_MONOTONIC for now and just have CLOCK_REALTIME and CLOCK_MONOTONIC_RAW support. Also as I mentioned, it will be just a matter of adding new clock-ids and support for the pre/post-ts for respective clock-ids if needed in the future. The modification that you have proposed (in a couple of posts back) would work but it's still not ideal since the pre/post ts are not close enough as they are currently (properly implemented!) gettimex64() would have. The only way to do that would be to have another ioctl as I have proposed which is a superset of current gettimex64 and pre-post collection is the closest possible. Here is my sample mlx4 (since I use that) of the new ioctl method (just for the reference) --- a/drivers/net/ethernet/mellanox/mlx4/main.c +++ b/drivers/net/ethernet/mellanox/mlx4/main.c @@ -43,6 +43,7 @@ #include <linux/io-mapping.h> #include <linux/delay.h> #include <linux/etherdevice.h> +#include <linux/ptp_clock_kernel.h> #include <net/devlink.h> #include <uapi/rdma/mlx4-abi.h> @@ -1929,7 +1930,7 @@ static void unmap_bf_area(struct mlx4_dev *dev) io_mapping_free(mlx4_priv(dev)->bf_mapping); } -u64 mlx4_read_clock(struct mlx4_dev *dev) +u64 mlx4_read_clock(struct mlx4_dev *dev, struct ptp_system_timestamp *sts, int clkid) { u32 clockhi, clocklo, clockhi1; u64 cycles; @@ -1937,7 +1938,13 @@ u64 mlx4_read_clock(struct mlx4_dev *dev) struct mlx4_priv *priv = mlx4_priv(dev); for (i = 0; i < 10; i++) { - clockhi = swab32(readl(priv->clock_mapping)); + if (sts) { + ptp_read_any_prets(sts, clkid); + clockhi = swab32(readl(priv->clock_mapping)); + ptp_read_any_postts(sts, clkid); + } else { + clockhi = swab32(readl(priv->clock_mapping)); + } clocklo = swab32(readl(priv->clock_mapping + 4)); clockhi1 = swab32(readl(priv->clock_mapping)); if (clockhi == clockhi1) Having said that, the 'flag' modification proposal is a good backup for the drivers that don't have good implementation (close enough but not ideal). Also, you don't need a new ioctl-op. So if we really want precision, I believe, we need a new ioctl op (with supporting implementation similar to the mlx4 code above). but we want to save the new ioctl-op and have less precision then proposed modification would work fine. > Thanks, > > tglx >
