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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <80568c10-2d73-2a68-aed6-a553ae2410f8@novek.ru>
Date:   Sun, 26 Jun 2022 20:27:17 +0100
From:   Vadim Fedorenko <vfedorenko@...ek.ru>
To:     Jonathan Lemon <jonathan.lemon@...il.com>
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 23.06.2022 19:28, Jonathan Lemon wrote:
> On Thu, Jun 23, 2022 at 03:57:17AM +0300, Vadim Fedorenko wrote:
>> From: Vadim Fedorenko <vadfed@...com>
>>
>> +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.
> 

Didn't get this point. The same code is used through out the driver.
Could you please explain?

> 
>> +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.
> 

Addressed this in v2

> 
>> +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?

I re-implemented unregister/free in v2. Hope it fixes questions.