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: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Mon, 29 Jan 2024 10:59:21 +0000
From: Simon Horman <horms@...nel.org>
To: Sai Krishna <saikrishnag@...vell.com>
Cc: richardcochran@...il.com, davem@...emloft.net, kuba@...nel.org,
	netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
	sgoutham@...vell.com, gakula@...vell.com, lcherian@...vell.com,
	hkelam@...vell.com, sbhatta@...vell.com,
	Naveen Mamindlapalli <naveenm@...vell.com>
Subject: Re: [net-next PATCH] octeontx2: Add PTP clock driver for Octeon PTM
 clock.

On Wed, Jan 24, 2024 at 12:11:56PM +0530, Sai Krishna wrote:
> The PCIe PTM(Precision time measurement) protocol provides precise
> coordination of events across multiple components like PCIe host
> clock, PCIe EP PHC local clocks of PCIe devices. This patch adds
> support for ptp clock based PTM clock. We can use this PTP device
> to sync the PTM time with CLOCK_REALTIME or other PTP PHC
> devices using phc2sys.
> 
> Signed-off-by: Sai Krishna <saikrishnag@...vell.com>
> Signed-off-by: Naveen Mamindlapalli <naveenm@...vell.com>
> Signed-off-by: Sunil Kovvuri Goutham <sgoutham@...vell.com>

Hi Sai,

some minor review items from my side.

> diff --git a/drivers/ptp/ptp_octeon_ptm.c b/drivers/ptp/ptp_octeon_ptm.c

..

> +static u32 read_pcie_config32(int ep_pem, int cfg_addr)
> +{
> +	void __iomem *addr;
> +	u64 val;
> +
> +	if (oct_ptp_clock.cn10k_variant) {
> +		addr  = ioremap(PEMX_PFX_CSX_PFCFGX(ep_pem, 0, cfg_addr), 8);
> +		if (!addr) {
> +			pr_err("PTM_EP: Failed to ioremap Octeon CSR space\n");
> +			return -1U;
> +		}
> +		val = readl(addr);
> +		iounmap(addr);
> +	} else {
> +		addr  = ioremap(PEMX_CFG_RD(ep_pem), 8);
> +		if (!addr) {
> +			pr_err("PTM_EP: Failed to ioremap Octeon CSR space\n");
> +			return -1U;
> +		}
> +		val = ((1 << 15) | (cfg_addr & 0xfff));
> +		writeq(val, addr);

This causes a build failure on x86_32 because writeq() is undefined.

> +		val = readq(addr) >> 32;
> +		iounmap(addr);
> +	}
> +	return (val & 0xffffffff);
> +}
> +
> +static uint64_t octeon_csr_read(u64 csr_addr)
> +{
> +	u64 val;
> +	void __iomem *addr;

nit: In Networking code, please consider arranging local variables
     in reverse xmas tree order - longest line to shortest.

> +
> +	addr = ioremap(csr_addr, 8);
> +	if (!addr) {
> +		pr_err("PTM_EP: Failed to ioremap CSR space\n");
> +		return -1UL;
> +	}
> +	val = (u64)READ_ONCE(*(u64 __iomem *)addr);

Sparse seems unhappy about this cast.
So if this is really what you want to do then probably a
__force is needed in the cast.

But I do wonder if there is an endian consideration
that needs to be taken care of here. And, moreover,
if a standard routine, such as ioread64(), could be
used instead of this function.

N.B. as per the note on writeq, possibly this only works on 64bit systems.

Likewise elsewhere in this patch.

> +	iounmap(addr);
> +	return val;
> +}

..

> +static int __init ptp_oct_ptm_init(void)
> +{
> +	struct pci_dev *pdev = NULL;
> +
> +	pdev = pci_get_device(PCI_VENDOR_ID_CAVIUM,
> +			      PCI_DEVID_OCTEONTX2_PTP, pdev);
> +	if (!pdev)
> +		return 0;
> +
> +	if (octeon_csr_read(PEMX_CFG) & 0x1ULL) {
> +		pr_err("PEM0 is configured as RC\n");
> +		return 0;
> +	}
> +
> +	if (is_otx2_support_ptm(pdev)) {
> +		oct_ptp_clock.cn10k_variant = 0;
> +	} else if (is_cn10k_support_ptm(pdev)) {
> +		oct_ptp_clock.cn10k_variant = 1;
> +	} else {
> +		/* PTM_EP: unsupported processor */
> +		return 0;
> +	}
> +
> +	ptm_ctl_addr = ioremap(PEMX_PTM_CTL, 8);
> +	if (!ptm_ctl_addr) {
> +		pr_err("PTM_EP: Failed to ioremap CSR space\n");
> +		return 0;
> +	}
> +
> +	ptm_lcl_addr = ioremap(PEMX_PTM_LCL_TIME, 8);
> +	if (!ptm_lcl_addr) {
> +		pr_err("PTM_EP: Failed to ioremap CSR space\n");
> +		return 0;
> +	}
> +
> +	oct_ptp_clock.caps = ptp_oct_caps;
> +
> +	oct_ptp_clock.ptp_clock = ptp_clock_register(&oct_ptp_clock.caps, NULL);
> +
> +	pr_info("PTP device index for PTM clock:%d\n", oct_ptp_clock.ptp_clock->index);

It seems that the pr_info() call above assumes that oct_ptp_clock.ptp_clock
is not an error, but it may be.

Perhaps something like this is more appropriate:

	oct_ptp_clock.ptp_clock = ...
	if (IS_ERR(oct_ptp_clock.ptp_clock))
		ERR_PTR(oct_ptp_clock.ptp_clock);

	pr_info(...)
	...

	return 0;

Flagged by Smatch.

> +	pr_info("cn10k_variant %d\n", oct_ptp_clock.cn10k_variant);
> +
> +	return PTR_ERR_OR_ZERO(oct_ptp_clock.ptp_clock);
> +}
> +
> +module_init(ptp_oct_ptm_init);
> +module_exit(ptp_oct_ptm_exit);
> +
> +MODULE_AUTHOR("Marvell Inc.");
> +MODULE_DESCRIPTION("PTP PHC clock using PTM");
> +MODULE_LICENSE("GPL");
> -- 
> 2.25.1

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ