netdev - Re: [PATCH net-next v2] ptp: add Alibaba CIPU PTP clock driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <0b0d3dad-3fe2-4b3a-a018-35a3603f8c10@lunn.ch>
Date: Fri, 27 Jun 2025 09:57:12 +0200
From: Andrew Lunn <andrew@...n.ch>
To: Wen Gu <guwen@...ux.alibaba.com>
Cc: richardcochran@...il.com, andrew+netdev@...n.ch, davem@...emloft.net,
	edumazet@...gle.com, kuba@...nel.org, pabeni@...hat.com,
	xuanzhuo@...ux.alibaba.com, dust.li@...ux.alibaba.com,
	netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH net-next v2] ptp: add Alibaba CIPU PTP clock driver

> +#define PTP_CIPU_LOG_SUB(dev, level, type, event, fmt, ...) \
> +({ \
> +	static DEFINE_RATELIMIT_STATE(_rs, \
> +				      DEFAULT_RATELIMIT_INTERVAL, \
> +				      DEFAULT_RATELIMIT_BURST); \
> +	if (__ratelimit(&_rs)) \
> +		dev_printk(level, dev, "[%02x:%02x]: " fmt, \
> +			   type, event, ##__VA_ARGS__); \
> +})

Please don't use such wrappers. Just use dev_dbg_ratelimited() etc.

> +static int cipu_iowrite8_and_check(void __iomem *addr,
> +				   u8 value, u8 *res)
> +{
> +	iowrite8(value, addr);
> +	if (value != ioread8(addr))
> +		return -EIO;
> +	*res = value;
> +	return 0;
> +}

This probably needs a comment. I assume the hardware is broken and
sometimes writes don't work? You should state that.

> +static void ptp_cipu_print_dev_events(struct ptp_cipu_ctx *ptp_ctx,
> +				      int event)
> +{
> +	struct device *dev = &ptp_ctx->pdev->dev;
> +	int type = PTP_CIPU_EVT_TYPE_DEV;
> +
> +	switch (event) {
> +	case PTP_CIPU_EVT_H_CLK_ABN:
> +		PTP_CIPU_LOG_SUB(dev, KERN_ERR, type, event,
> +				 "Atomic Clock Error Detected\n");
> +		break;
> +	case PTP_CIPU_EVT_H_CLK_ABN_REC:
> +		PTP_CIPU_LOG_SUB(dev, KERN_INFO, type, event,
> +				 "Atomic Clock Error Recovered\n");
> +		break;
> +	case PTP_CIPU_EVT_H_DEV_MT:
> +		PTP_CIPU_LOG_SUB(dev, KERN_ERR, type, event,
> +				 "Maintenance Exception Detected\n");
> +		break;
> +	case PTP_CIPU_EVT_H_DEV_MT_REC:
> +		PTP_CIPU_LOG_SUB(dev, KERN_INFO, type, event,
> +				 "Maintenance Exception Recovered\n");
> +		break;
> +	case PTP_CIPU_EVT_H_DEV_MT_TOUT:
> +		PTP_CIPU_LOG_SUB(dev, KERN_INFO, type, event,
> +				 "Maintenance Exception Failed to Recover "
> +				 "within %d us\n", ptp_ctx->regs.mt_tout_us);
> +		break;
> +	case PTP_CIPU_EVT_H_DEV_BUSY:
> +		PTP_CIPU_LOG_SUB(dev, KERN_ERR, type, event,
> +				 "PHC Busy Detected\n");
> +		break;
> +	case PTP_CIPU_EVT_H_DEV_BUSY_REC:
> +		PTP_CIPU_LOG_SUB(dev, KERN_INFO, type, event,
> +				 "PHC Busy Recovered\n");
> +		break;
> +	case PTP_CIPU_EVT_H_DEV_ERR:
> +		PTP_CIPU_LOG_SUB(dev, KERN_ERR, type, event,
> +				 "PHC Error Detected\n");
> +		break;
> +	case PTP_CIPU_EVT_H_DEV_ERR_REC:
> +		PTP_CIPU_LOG_SUB(dev, KERN_INFO, type, event,
> +				 "PHC Error Recovered\n");

Are these fatal? Or can the device still be used after these errors
occur?

> +static int ptp_cipu_enable(struct ptp_clock_info *info,
> +			   struct ptp_clock_request *request, int on)
> +{
> +	return -EOPNOTSUPP;
> +}
> +
> +static int ptp_cipu_settime(struct ptp_clock_info *p,
> +			    const struct timespec64 *ts)
> +{
> +	return -EOPNOTSUPP;
> +}
> +
> +static int ptp_cipu_adjfine(struct ptp_clock_info *ptp, long scaled_ppm)
> +{
> +	return -EOPNOTSUPP;
> +}
> +
> +static int ptp_cipu_adjtime(struct ptp_clock_info *ptp, s64 delta)
> +{
> +	return -EOPNOTSUPP;
> +}

I've not looked at the core. Are these actually required? Or if they
are missing, does the core default to -EOPNOTSUPP?

> +static ssize_t register_snapshot_show(struct device *dev,
> +				      struct device_attribute *attr, char *buf)
> +{
> +	struct ptp_cipu_ctx *ctx = pci_get_drvdata(to_pci_dev(dev));
> +	struct ptp_cipu_regs *regs = &ctx->regs;
> +
> +	return sysfs_emit(buf, "%s 0x%x %s 0x%x %s 0x%x %s 0x%x "
> +			  "%s 0x%x %s 0x%x %s 0x%x %s 0x%x %s 0x%x "
> +			  "%s 0x%x %s 0x%x %s 0x%x\n",
> +			  "device_features", regs->dev_feat,
> +			  "guest_features", regs->gst_feat,
> +			  "driver_version", regs->drv_ver,
> +			  "environment_version", regs->env_ver,
> +			  "device_status", regs->dev_stat,
> +			  "sync_status", regs->sync_stat,
> +			  "time_precision(ns)", regs->tm_prec_ns,
> +			  "epoch_base(years)", regs->epo_base_yr,
> +			  "leap_second(s)", regs->leap_sec,
> +			  "max_latency(ns)", regs->max_lat_ns,
> +			  "maintenance_timeout(us)", regs->mt_tout_us,
> +			  "offset_threshold(us)", regs->thresh_us);
> +}

Is this debug? Maybe it should be placed in debugfs, rather than
sysfs.

	Andrew