[PATCH net-next 0/6] ptp: ocp: assorted fixes.

[PATCH net-next 0/6] ptp: ocp: assorted fixes.

[Jonathan Lemon]

Assorted fixes for the ocp timecard.

Jonathan Lemon (6):
  ptp: ocp: Fix the error handling path for the class device.
  ptp: ocp: Add the mapping for the external PPS registers.
  ptp: ocp: Remove devlink health and unused parameters.
  ptp: ocp: Use 'gnss' naming instead of 'gps'
  ptp: ocp: Rename version string shown by devlink.
  ptp: ocp: Remove pending_image indicator from devlink

 drivers/ptp/ptp_ocp.c | 153 ++++++++++--------------------------------
 1 file changed, 35 insertions(+), 118 deletions(-)

-- 
2.31.1

[PATCH net-next 1/6] ptp: ocp: Fix the error handling path for the class device.

[Jonathan Lemon]

Move the put_device() call to the error handling path, so the
device is released after the .release callback, avoiding a
use-after-free.

Signed-off-by: Jonathan Lemon <jonathan.lemon@gmail.com>
---
 drivers/ptp/ptp_ocp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/ptp/ptp_ocp.c b/drivers/ptp/ptp_ocp.c
index 039d3a5c2a6f..261713c6e9a7 100644
--- a/drivers/ptp/ptp_ocp.c
+++ b/drivers/ptp/ptp_ocp.c
@@ -1381,7 +1381,6 @@ ptp_ocp_device_init(struct ptp_ocp *bp, struct pci_dev *pdev)
 	err = device_add(&bp->dev);
 	if (err) {
 		dev_err(&bp->dev, "device add failed: %d\n", err);
-		put_device(&bp->dev);
 		goto out;
 	}
 
@@ -1391,6 +1390,7 @@ ptp_ocp_device_init(struct ptp_ocp *bp, struct pci_dev *pdev)
 
 out:
 	ptp_ocp_dev_release(&bp->dev);
+	put_device(&bp->dev);
 	return err;
 }
 
-- 
2.31.1

[PATCH net-next 2/6] ptp: ocp: Add the mapping for the external PPS registers.

[Jonathan Lemon]

There are two PPS blocks: one handles the external PPS signal output,
with the other handling the PPS signal input to the internal clock.
Add controls for the external PPS block.

Rename the fields so they match their function.

Add cable_delay to the register definitions.

Signed-off-by: Jonathan Lemon <jonathan.lemon@gmail.com>
---
 drivers/ptp/ptp_ocp.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/drivers/ptp/ptp_ocp.c b/drivers/ptp/ptp_ocp.c
index 261713c6e9a7..8804e79477cd 100644
--- a/drivers/ptp/ptp_ocp.c
+++ b/drivers/ptp/ptp_ocp.c
@@ -113,6 +113,8 @@ struct ts_reg {
 struct pps_reg {
 	u32	ctrl;
 	u32	status;
+	u32	__pad0[6];
+	u32	cable_delay;
 };
 
 #define PPS_STATUS_FILTER_ERR	BIT(0)
@@ -149,7 +151,8 @@ struct ptp_ocp {
 	spinlock_t		lock;
 	struct ocp_reg __iomem	*reg;
 	struct tod_reg __iomem	*tod;
-	struct pps_reg __iomem	*pps_monitor;
+	struct pps_reg __iomem	*pps_to_ext;
+	struct pps_reg __iomem	*pps_to_clk;
 	struct ptp_ocp_ext_src	*pps;
 	struct ptp_ocp_ext_src	*ts0;
 	struct ptp_ocp_ext_src	*ts1;
@@ -251,7 +254,11 @@ static struct ocp_resource ocp_fb_resource[] = {
 		},
 	},
 	{
-		OCP_MEM_RESOURCE(pps_monitor),
+		OCP_MEM_RESOURCE(pps_to_ext),
+		.offset = 0x01030000, .size = 0x10000,
+	},
+	{
+		OCP_MEM_RESOURCE(pps_to_clk),
 		.offset = 0x01040000, .size = 0x10000,
 	},
 	{
@@ -537,10 +544,10 @@ ptp_ocp_watchdog(struct timer_list *t)
 	unsigned long flags;
 	u32 status;
 
-	status = ioread32(&bp->pps_monitor->status);
+	status = ioread32(&bp->pps_to_clk->status);
 
 	if (status & PPS_STATUS_SUPERV_ERR) {
-		iowrite32(status, &bp->pps_monitor->status);
+		iowrite32(status, &bp->pps_to_clk->status);
 		if (!bp->gps_lost) {
 			spin_lock_irqsave(&bp->lock, flags);
 			__ptp_ocp_clear_drift_locked(bp);
-- 
2.31.1

[PATCH net-next 3/6] ptp: ocp: Remove devlink health and unused parameters.

[Jonathan Lemon]

"devlink health" was used as a way to monitor the GNSS signal
status.  This isn't really the intended use, and the same
functionality can be achived by monitoring the status file.

Remove the devlink heath support entirely, and also remove the
currently unused devlink parameters.

Signed-off-by: Jonathan Lemon <jonathan.lemon@gmail.com>
---
 drivers/ptp/ptp_ocp.c | 80 -------------------------------------------
 1 file changed, 80 deletions(-)

diff --git a/drivers/ptp/ptp_ocp.c b/drivers/ptp/ptp_ocp.c
index 8804e79477cd..33cbd3135a00 100644
--- a/drivers/ptp/ptp_ocp.c
+++ b/drivers/ptp/ptp_ocp.c
@@ -162,7 +162,6 @@ struct ptp_ocp {
 	struct platform_device	*i2c_ctrl;
 	struct platform_device	*spi_flash;
 	struct clk_hw		*i2c_clk;
-	struct devlink_health_reporter *health;
 	struct timer_list	watchdog;
 	time64_t		gps_lost;
 	int			id;
@@ -184,7 +183,6 @@ struct ocp_resource {
 	unsigned long bp_offset;
 };
 
-static void ptp_ocp_health_update(struct ptp_ocp *bp);
 static int ptp_ocp_register_mem(struct ptp_ocp *bp, struct ocp_resource *r);
 static int ptp_ocp_register_i2c(struct ptp_ocp *bp, struct ocp_resource *r);
 static int ptp_ocp_register_spi(struct ptp_ocp *bp, struct ocp_resource *r);
@@ -553,12 +551,10 @@ ptp_ocp_watchdog(struct timer_list *t)
 			__ptp_ocp_clear_drift_locked(bp);
 			spin_unlock_irqrestore(&bp->lock, flags);
 			bp->gps_lost = ktime_get_real_seconds();
-			ptp_ocp_health_update(bp);
 		}
 
 	} else if (bp->gps_lost) {
 		bp->gps_lost = 0;
-		ptp_ocp_health_update(bp);
 	}
 
 	mod_timer(&bp->watchdog, jiffies + HZ);
@@ -740,14 +736,6 @@ ptp_ocp_info(struct ptp_ocp *bp)
 	ptp_ocp_tod_info(bp);
 }
 
-static const struct devlink_param ptp_ocp_devlink_params[] = {
-};
-
-static void
-ptp_ocp_devlink_set_params_init_values(struct devlink *devlink)
-{
-}
-
 static int
 ptp_ocp_devlink_register(struct devlink *devlink, struct device *dev)
 {
@@ -757,25 +745,12 @@ ptp_ocp_devlink_register(struct devlink *devlink, struct device *dev)
 	if (err)
 		return err;
 
-	err = devlink_params_register(devlink, ptp_ocp_devlink_params,
-				      ARRAY_SIZE(ptp_ocp_devlink_params));
-	ptp_ocp_devlink_set_params_init_values(devlink);
-	if (err)
-		goto out;
-	devlink_params_publish(devlink);
-
 	return 0;
-
-out:
-	devlink_unregister(devlink);
-	return err;
 }
 
 static void
 ptp_ocp_devlink_unregister(struct devlink *devlink)
 {
-	devlink_params_unregister(devlink, ptp_ocp_devlink_params,
-				  ARRAY_SIZE(ptp_ocp_devlink_params));
 	devlink_unregister(devlink);
 }
 
@@ -922,58 +897,6 @@ static const struct devlink_ops ptp_ocp_devlink_ops = {
 	.info_get = ptp_ocp_devlink_info_get,
 };
 
-static int
-ptp_ocp_health_diagnose(struct devlink_health_reporter *reporter,
-			struct devlink_fmsg *fmsg,
-			struct netlink_ext_ack *extack)
-{
-	struct ptp_ocp *bp = devlink_health_reporter_priv(reporter);
-	char buf[32];
-	int err;
-
-	if (!bp->gps_lost)
-		return 0;
-
-	sprintf(buf, "%ptT", &bp->gps_lost);
-	err = devlink_fmsg_string_pair_put(fmsg, "Lost sync at", buf);
-	if (err)
-		return err;
-
-	return 0;
-}
-
-static void
-ptp_ocp_health_update(struct ptp_ocp *bp)
-{
-	int state;
-
-	state = bp->gps_lost ? DEVLINK_HEALTH_REPORTER_STATE_ERROR
-			     : DEVLINK_HEALTH_REPORTER_STATE_HEALTHY;
-
-	if (bp->gps_lost)
-		devlink_health_report(bp->health, "No GPS signal", NULL);
-
-	devlink_health_reporter_state_update(bp->health, state);
-}
-
-static const struct devlink_health_reporter_ops ptp_ocp_health_ops = {
-	.name = "gps_sync",
-	.diagnose = ptp_ocp_health_diagnose,
-};
-
-static void
-ptp_ocp_devlink_health_register(struct devlink *devlink)
-{
-	struct ptp_ocp *bp = devlink_priv(devlink);
-	struct devlink_health_reporter *r;
-
-	r = devlink_health_reporter_create(devlink, &ptp_ocp_health_ops, 0, bp);
-	if (IS_ERR(r))
-		dev_err(&bp->pdev->dev, "Failed to create reporter, err %ld\n",
-			PTR_ERR(r));
-	bp->health = r;
-}
-
 static void __iomem *
 __ptp_ocp_get_mem(struct ptp_ocp *bp, unsigned long start, int size)
 {
@@ -1514,8 +1437,6 @@ ptp_ocp_detach(struct ptp_ocp *bp)
 		pci_free_irq_vectors(bp->pdev);
 	if (bp->ptp)
 		ptp_clock_unregister(bp->ptp);
-	if (bp->health)
-		devlink_health_reporter_destroy(bp->health);
 	device_unregister(&bp->dev);
 }
 
@@ -1578,7 +1499,6 @@ ptp_ocp_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 
 	ptp_ocp_info(bp);
 	ptp_ocp_resource_summary(bp);
-	ptp_ocp_devlink_health_register(devlink);
 
 	return 0;
 
-- 
2.31.1

[PATCH net-next 4/6] ptp: ocp: Use 'gnss' naming instead of 'gps'

[Jonathan Lemon]

GPS is not the only available positioning system.  Use the generic
naming of "GNSS" instead.

Signed-off-by: Jonathan Lemon <jonathan.lemon@gmail.com>
---
 drivers/ptp/ptp_ocp.c | 42 +++++++++++++++++++++---------------------
 1 file changed, 21 insertions(+), 21 deletions(-)

diff --git a/drivers/ptp/ptp_ocp.c b/drivers/ptp/ptp_ocp.c
index 33cbd3135a00..f744bb42f48f 100644
--- a/drivers/ptp/ptp_ocp.c
+++ b/drivers/ptp/ptp_ocp.c
@@ -163,10 +163,10 @@ struct ptp_ocp {
 	struct platform_device	*spi_flash;
 	struct clk_hw		*i2c_clk;
 	struct timer_list	watchdog;
-	time64_t		gps_lost;
+	time64_t		gnss_lost;
 	int			id;
 	int			n_irqs;
-	int			gps_port;
+	int			gnss_port;
 	int			mac_port;	/* miniature atomic clock */
 	u8			serial[6];
 	int			flash_start;
@@ -272,7 +272,7 @@ static struct ocp_resource ocp_fb_resource[] = {
 		.offset = 0x00150000, .size = 0x10000, .irq_vec = 7,
 	},
 	{
-		OCP_SERIAL_RESOURCE(gps_port),
+		OCP_SERIAL_RESOURCE(gnss_port),
 		.offset = 0x00160000 + 0x1000, .irq_vec = 3,
 	},
 	{
@@ -546,15 +546,15 @@ ptp_ocp_watchdog(struct timer_list *t)
 
 	if (status & PPS_STATUS_SUPERV_ERR) {
 		iowrite32(status, &bp->pps_to_clk->status);
-		if (!bp->gps_lost) {
+		if (!bp->gnss_lost) {
 			spin_lock_irqsave(&bp->lock, flags);
 			__ptp_ocp_clear_drift_locked(bp);
 			spin_unlock_irqrestore(&bp->lock, flags);
-			bp->gps_lost = ktime_get_real_seconds();
+			bp->gnss_lost = ktime_get_real_seconds();
 		}
 
-	} else if (bp->gps_lost) {
-		bp->gps_lost = 0;
+	} else if (bp->gnss_lost) {
+		bp->gnss_lost = 0;
 	}
 
 	mod_timer(&bp->watchdog, jiffies + HZ);
@@ -1195,19 +1195,19 @@ serialnum_show(struct device *dev, struct device_attribute *attr, char *buf)
 static DEVICE_ATTR_RO(serialnum);
 
 static ssize_t
-gps_sync_show(struct device *dev, struct device_attribute *attr, char *buf)
+gnss_sync_show(struct device *dev, struct device_attribute *attr, char *buf)
 {
 	struct ptp_ocp *bp = dev_get_drvdata(dev);
 	ssize_t ret;
 
-	if (bp->gps_lost)
-		ret = sysfs_emit(buf, "LOST @ %ptT\n", &bp->gps_lost);
+	if (bp->gnss_lost)
+		ret = sysfs_emit(buf, "LOST @ %ptT\n", &bp->gnss_lost);
 	else
 		ret = sysfs_emit(buf, "SYNC\n");
 
 	return ret;
 }
-static DEVICE_ATTR_RO(gps_sync);
+static DEVICE_ATTR_RO(gnss_sync);
 
 static ssize_t
 clock_source_show(struct device *dev, struct device_attribute *attr, char *buf)
@@ -1264,7 +1264,7 @@ static DEVICE_ATTR_RO(available_clock_sources);
 
 static struct attribute *timecard_attrs[] = {
 	&dev_attr_serialnum.attr,
-	&dev_attr_gps_sync.attr,
+	&dev_attr_gnss_sync.attr,
 	&dev_attr_clock_source.attr,
 	&dev_attr_available_clock_sources.attr,
 	NULL,
@@ -1297,7 +1297,7 @@ ptp_ocp_device_init(struct ptp_ocp *bp, struct pci_dev *pdev)
 
 	bp->ptp_info = ptp_ocp_clock_info;
 	spin_lock_init(&bp->lock);
-	bp->gps_port = -1;
+	bp->gnss_port = -1;
 	bp->mac_port = -1;
 	bp->pdev = pdev;
 
@@ -1356,9 +1356,9 @@ ptp_ocp_complete(struct ptp_ocp *bp)
 	struct pps_device *pps;
 	char buf[32];
 
-	if (bp->gps_port != -1) {
-		sprintf(buf, "ttyS%d", bp->gps_port);
-		ptp_ocp_link_child(bp, buf, "ttyGPS");
+	if (bp->gnss_port != -1) {
+		sprintf(buf, "ttyS%d", bp->gnss_port);
+		ptp_ocp_link_child(bp, buf, "ttyGNSS");
 	}
 	if (bp->mac_port != -1) {
 		sprintf(buf, "ttyS%d", bp->mac_port);
@@ -1393,8 +1393,8 @@ ptp_ocp_resource_summary(struct ptp_ocp *bp)
 			dev_info(dev, "golden image, version %d\n",
 				 ver >> 16);
 	}
-	if (bp->gps_port != -1)
-		dev_info(dev, "GPS @ /dev/ttyS%d  115200\n", bp->gps_port);
+	if (bp->gnss_port != -1)
+		dev_info(dev, "GNSS @ /dev/ttyS%d 115200\n", bp->gnss_port);
 	if (bp->mac_port != -1)
 		dev_info(dev, "MAC @ /dev/ttyS%d   57600\n", bp->mac_port);
 }
@@ -1404,7 +1404,7 @@ ptp_ocp_detach_sysfs(struct ptp_ocp *bp)
 {
 	struct device *dev = &bp->dev;
 
-	sysfs_remove_link(&dev->kobj, "ttyGPS");
+	sysfs_remove_link(&dev->kobj, "ttyGNSS");
 	sysfs_remove_link(&dev->kobj, "ttyMAC");
 	sysfs_remove_link(&dev->kobj, "ptp");
 	sysfs_remove_link(&dev->kobj, "pps");
@@ -1423,8 +1423,8 @@ ptp_ocp_detach(struct ptp_ocp *bp)
 		ptp_ocp_unregister_ext(bp->ts1);
 	if (bp->pps)
 		ptp_ocp_unregister_ext(bp->pps);
-	if (bp->gps_port != -1)
-		serial8250_unregister_port(bp->gps_port);
+	if (bp->gnss_port != -1)
+		serial8250_unregister_port(bp->gnss_port);
 	if (bp->mac_port != -1)
 		serial8250_unregister_port(bp->mac_port);
 	if (bp->spi_flash)
-- 
2.31.1

[PATCH net-next 5/6] ptp: ocp: Rename version string shown by devlink.

[Jonathan Lemon]

The TimeCard has two FPGA images in the flash: the actual firmware,
and a manufacturing fallback version which is intended to act as a
loader in case the flash update failed.

Name these "fw" and "loader", which are reflected in devlink:

    [root@timecard drv]# devlink dev info
    pci/0000:04:00.0:
      driver ptp_ocp
      serial_number fc:c2:3d:2e:d7:c0
      versions:
          running:
            fw 5

Signed-off-by: Jonathan Lemon <jonathan.lemon@gmail.com>
---
 drivers/ptp/ptp_ocp.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/ptp/ptp_ocp.c b/drivers/ptp/ptp_ocp.c
index f744bb42f48f..1412015fd261 100644
--- a/drivers/ptp/ptp_ocp.c
+++ b/drivers/ptp/ptp_ocp.c
@@ -867,12 +867,12 @@ ptp_ocp_devlink_info_get(struct devlink *devlink, struct devlink_info_req *req,
 		if (ver & 0xffff) {
 			sprintf(buf, "%d", ver);
 			err = devlink_info_version_running_put(req,
-							       "timecard",
+							       "fw",
 							       buf);
 		} else {
 			sprintf(buf, "%d", ver >> 16);
 			err = devlink_info_version_running_put(req,
-							       "golden flash",
+							       "loader",
 							       buf);
 		}
 		if (err)
-- 
2.31.1

[PATCH net-next 6/6] ptp: ocp: Remove pending_image indicator from devlink

[Jonathan Lemon]

After writing an image blob to the flash memory, a reboot is required
to reload the FPGA.  There is no versioning prsent in the FPGA image
file, so only a running version is available.  The 'stored version'
was set to 'pending' in order to indicate a reboot was needed.

This isn't reliable, as the module could be unloaded/loaded, losing
the "reboot needed" indicator.  Also, the devlink 'stored version'
information is designed to refer to the actual image version.

Unfortunately, there is no method to determine the flash image version
other than booting it, so remove the devlink stored version setting.

Signed-off-by: Jonathan Lemon <jonathan.lemon@gmail.com>
---
 drivers/ptp/ptp_ocp.c | 10 ----------
 1 file changed, 10 deletions(-)

diff --git a/drivers/ptp/ptp_ocp.c b/drivers/ptp/ptp_ocp.c
index 1412015fd261..6b9c14586987 100644
--- a/drivers/ptp/ptp_ocp.c
+++ b/drivers/ptp/ptp_ocp.c
@@ -171,7 +171,6 @@ struct ptp_ocp {
 	u8			serial[6];
 	int			flash_start;
 	bool			has_serial;
-	bool			pending_image;
 };
 
 struct ocp_resource {
@@ -836,8 +835,6 @@ ptp_ocp_devlink_flash_update(struct devlink *devlink,
 	msg = err ? "Flash error" : "Flash complete";
 	devlink_flash_update_status_notify(devlink, msg, NULL, 0, 0);
 
-	bp->pending_image = true;
-
 	put_device(dev);
 	return err;
 }
@@ -854,13 +851,6 @@ ptp_ocp_devlink_info_get(struct devlink *devlink, struct devlink_info_req *req,
 	if (err)
 		return err;
 
-	if (bp->pending_image) {
-		err = devlink_info_version_stored_put(req,
-						      "timecard", "pending");
-		if (err)
-			return err;
-	}
-
 	if (bp->image) {
 		u32 ver = ioread32(&bp->image->version);
 
-- 
2.31.1

Re: [PATCH net-next 0/6] ptp: ocp: assorted fixes.

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net-next.git (refs/heads/master):

On Thu,  5 Aug 2021 12:52:42 -0700 you wrote:
> Assorted fixes for the ocp timecard.
> 
> Jonathan Lemon (6):
>   ptp: ocp: Fix the error handling path for the class device.
>   ptp: ocp: Add the mapping for the external PPS registers.
>   ptp: ocp: Remove devlink health and unused parameters.
>   ptp: ocp: Use 'gnss' naming instead of 'gps'
>   ptp: ocp: Rename version string shown by devlink.
>   ptp: ocp: Remove pending_image indicator from devlink
> 
> [...]

Here is the summary with links:
  - [net-next,1/6] ptp: ocp: Fix the error handling path for the class device.
    https://git.kernel.org/netdev/net-next/c/d12f23fa5142
  - [net-next,2/6] ptp: ocp: Add the mapping for the external PPS registers.
    https://git.kernel.org/netdev/net-next/c/0d43d4f26cb2
  - [net-next,3/6] ptp: ocp: Remove devlink health and unused parameters.
    https://git.kernel.org/netdev/net-next/c/37a156ba4cbb
  - [net-next,4/6] ptp: ocp: Use 'gnss' naming instead of 'gps'
    https://git.kernel.org/netdev/net-next/c/ef0cfb3460a4
  - [net-next,5/6] ptp: ocp: Rename version string shown by devlink.
    https://git.kernel.org/netdev/net-next/c/1a052da92924
  - [net-next,6/6] ptp: ocp: Remove pending_image indicator from devlink
    https://git.kernel.org/netdev/net-next/c/8ef8ccbc6967

You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html