netdev - Re: [RFC PATCH v1 3/3] ptp_ocp: implement DPLL ops

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: <20220623182813.safjhwvu67i4vu3b@bsd-mbp.dhcp.thefacebook.com>
Date:   Thu, 23 Jun 2022 11:28:13 -0700
From:   Jonathan Lemon <jonathan.lemon@...il.com>
To:     Vadim Fedorenko <vfedorenko@...ek.ru>
Cc:     Jakub Kicinski <kuba@...nel.org>, Vadim Fedorenko <vadfed@...com>,
        Aya Levin <ayal@...dia.com>,
        Arkadiusz Kubalewski <arkadiusz.kubalewski@...el.com>,
        netdev@...r.kernel.org, linux-arm-kernel@...ts.infradead.org
Subject: Re: [RFC PATCH v1 3/3] ptp_ocp: implement DPLL ops

On Thu, Jun 23, 2022 at 03:57:17AM +0300, Vadim Fedorenko wrote:
> From: Vadim Fedorenko <vadfed@...com>
> 
> Implement DPLL operations in ptp_ocp driver.

Please CC: me as well.


> +static int ptp_ocp_dpll_get_status(struct dpll_device *dpll)
> +{
> +	struct ptp_ocp *bp = (struct ptp_ocp *)dpll_priv(dpll);
> +	int sync;
> +
> +	sync = ioread32(&bp->reg->status) & OCP_STATUS_IN_SYNC;
> +	return sync;
> +}

Please match existing code style.


> +static int ptp_ocp_dpll_get_source_type(struct dpll_device *dpll, int sma)
> +{
> +	struct ptp_ocp *bp = (struct ptp_ocp *)dpll_priv(dpll);
> +	int ret;
> +
> +	if (bp->sma[sma].mode != SMA_MODE_IN)
> +		return -1;
> +
> +	switch (ptp_ocp_sma_get(bp, sma)) {
> +	case 0:
> +		ret = DPLL_TYPE_EXT_10MHZ;
> +		break;
> +	case 1:
> +	case 2:
> +		ret = DPLL_TYPE_EXT_1PPS;
> +		break;
> +	default:
> +		ret = DPLL_TYPE_INT_OSCILLATOR;
> +	}
> +
> +	return ret;
> +}

These case statements switch on private bits.  This needs to match
on the selector name instead.


> +static int ptp_ocp_dpll_get_output_type(struct dpll_device *dpll, int sma)
> +{
> +	struct ptp_ocp *bp = (struct ptp_ocp *)dpll_priv(dpll);
> +	int ret;
> +
> +	if (bp->sma[sma].mode != SMA_MODE_OUT)
> +		return -1;
> +
> +	switch (ptp_ocp_sma_get(bp, sma)) {
> +	case 0:
> +		ret = DPLL_TYPE_EXT_10MHZ;
> +		break;
> +	case 1:
> +	case 2:
> +		ret = DPLL_TYPE_INT_OSCILLATOR;
> +		break;
> +	case 4:
> +	case 8:
> +		ret = DPLL_TYPE_GNSS;
> +		break;
> +	default:
> +		ret = DPLL_TYPE_INT_OSCILLATOR;

Missing break;


> +	}
> +
> +	return ret;
> +}
> +
> +static struct dpll_device_ops dpll_ops = {
> +	.get_status		= ptp_ocp_dpll_get_status,
> +	.get_lock_status	= ptp_ocp_dpll_get_lock_status,
> +	.get_source_type	= ptp_ocp_dpll_get_source_type,
> +	.get_output_type	= ptp_ocp_dpll_get_output_type,
> +};

No 'set' statements here?  Also, what happens if there is more than
one GNSS receiver, how is this differentiated?
>  static int
>  ptp_ocp_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>  {
> @@ -3768,6 +3846,14 @@ ptp_ocp_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>  
>  	ptp_ocp_info(bp);
>  	devlink_register(devlink);
> +
> +	bp->dpll = dpll_device_alloc(&dpll_ops, ARRAY_SIZE(bp->sma), ARRAY_SIZE(bp->sma), bp);
> +	if (!bp->dpll) {
> +		dev_err(&pdev->dev, "dpll_device_alloc failed\n");
> +		return 0;
> +	}
> +	dpll_device_register(bp->dpll);
> +

How is the release/unregister path called when the module is unloaded?
-- 
Jonathan