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: <a978c45e-db51-a67c-9240-10eeeaa2db8d@novek.ru>
Date:   Fri, 24 Jun 2022 00:11:43 +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>
>>
>> Implement DPLL operations in ptp_ocp driver.
> 
> Please CC: me as well.
> 

Sure, sorry for not doing it last.

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

Not sure that string comparison is a good idea. Maybe it's better to extend 
struct ocp_selector with netlink type id and fill it according to hardware?

> 
>> +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?
No, didn't implement it yet

> Also, what happens if there is more than
> one GNSS receiver, how is this differentiated?

Good question. Haven't thought about such case, open for suggestions!

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

Thank you Jonathan for pointing it out. I've just realised that unregister path 
is messy in the subsystem, will polish it in the next version and add it to 
driver unload.