netdev - Re: [PATCH net-next v2 8/9] ptp

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: <2b7e0024-2a92-58c8-fbeb-d42beebedb03@linux.dev>
Date: Mon, 7 Aug 2023 22:42:41 +0100
From: Vadim Fedorenko <vadim.fedorenko@...ux.dev>
To: Simon Horman <horms@...nel.org>
Cc: Jakub Kicinski <kuba@...nel.org>, Jiri Pirko <jiri@...nulli.us>,
 Arkadiusz Kubalewski <arkadiusz.kubalewski@...el.com>,
 Jonathan Lemon <jonathan.lemon@...il.com>, Paolo Abeni <pabeni@...hat.com>,
 Milena Olech <milena.olech@...el.com>,
 Michal Michalik <michal.michalik@...el.com>,
 linux-arm-kernel@...ts.infradead.org, poros@...hat.com, mschmidt@...hat.com,
 netdev@...r.kernel.org, linux-clk@...r.kernel.org,
 Bart Van Assche <bvanassche@....org>, intel-wired-lan@...ts.osuosl.org,
 Jiri Pirko <jiri@...dia.com>
Subject: Re: [PATCH net-next v2 8/9] ptp_ocp: implement DPLL ops

On 06.08.2023 18:13, Simon Horman wrote:
> On Fri, Aug 04, 2023 at 08:04:53PM +0100, Vadim Fedorenko wrote:
>> Implement basic DPLL operations in ptp_ocp driver as the
>> simplest example of using new subsystem.
>>
>> Signed-off-by: Vadim Fedorenko <vadim.fedorenko@...ux.dev>
>> Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@...el.com>
>> Signed-off-by: Jiri Pirko <jiri@...dia.com>
> 
> Hi Vadim,
> 
> ...
> 
>> diff --git a/drivers/ptp/Kconfig b/drivers/ptp/Kconfig
>> index 32dff1b4f891..e4da62ac9a9f 100644
>> --- a/drivers/ptp/Kconfig
>> +++ b/drivers/ptp/Kconfig
>> @@ -177,6 +177,7 @@ config PTP_1588_CLOCK_OCP
>>   	depends on COMMON_CLK
>>   	select NET_DEVLINK
>>   	select CRC16
>> +	select DPLL
>>   	help
>>   	  This driver adds support for an OpenCompute time card.
>>   
>> diff --git a/drivers/ptp/ptp_ocp.c b/drivers/ptp/ptp_ocp.c
> 
> ...
> 
>> +static int ptp_ocp_dpll_frequency_set(const struct dpll_pin *pin,
>> +				      void *pin_priv,
>> +				      const struct dpll_device *dpll,
>> +				      void *dpll_priv, u64 frequency,
>> +				      struct netlink_ext_ack *extack)
>> +{
>> +	struct ptp_ocp_sma_connector *sma = pin_priv;
>> +	struct ptp_ocp *bp = dpll_priv;
>> +	const struct ocp_selector *tbl;
>> +	int sma_nr = (sma - bp->sma);
>> +	int val, i;
>> +
>> +	if (sma->fixed_fcn)
>> +		return -EOPNOTSUPP;
>> +
>> +	tbl = bp->sma_op->tbl[sma->mode];
>> +	for (i = 0; tbl[i].name; i++)
>> +		if (tbl[i].frequency == frequency)
>> +			return ptp_ocp_sma_store_val(bp, val, sma->mode, sma_nr);
> 
> val appears to be used uninitialised here.
> 
> As flagged by clang-16 W=1, and Smatch.
> 

Ahh, looks like it's not needed at all, thanks!

>> +	return -EINVAL;
>> +}
> 
> ...
> 
>> @@ -4233,8 +4437,40 @@ ptp_ocp_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>>   
>>   	ptp_ocp_info(bp);
>>   	devlink_register(devlink);
>> -	return 0;
>>   
>> +	clkid = pci_get_dsn(pdev);
>> +	bp->dpll = dpll_device_get(clkid, 0, THIS_MODULE);
>> +	if (IS_ERR(bp->dpll)) {
>> +		dev_err(&pdev->dev, "dpll_device_alloc failed\n");
>> +		goto out;
>> +	}
>> +
>> +	err = dpll_device_register(bp->dpll, DPLL_TYPE_PPS, &dpll_ops, bp);
>> +	if (err)
>> +		goto out;
>> +
>> +	for (i = 0; i < OCP_SMA_NUM; i++) {
>> +		bp->sma[i].dpll_pin = dpll_pin_get(clkid, i, THIS_MODULE, &bp->sma[i].dpll_prop);
>> +		if (IS_ERR(bp->sma[i].dpll_pin))
> 
> The function will return err.
> Should it be sett to an error value here?

Yeah, you are right. And I have spotted one more place where err is not properly
set, just above this one. Thanks for the review.


> As flagged by Smatch.
> 
>> +			goto out_dpll;
>> +
>> +		err = dpll_pin_register(bp->dpll, bp->sma[i].dpll_pin, &dpll_pins_ops,
>> +					&bp->sma[i]);
>> +		if (err) {
>> +			dpll_pin_put(bp->sma[i].dpll_pin);
>> +			goto out_dpll;
>> +		}
>> +	}
>> +	queue_delayed_work(system_power_efficient_wq, &bp->sync_work, HZ);
>> +
>> +	return 0;
>> +out_dpll:
>> +	while (i) {
>> +		--i;
>> +		dpll_pin_unregister(bp->dpll, bp->sma[i].dpll_pin, &dpll_pins_ops, &bp->sma[i]);
>> +		dpll_pin_put(bp->sma[i].dpll_pin);
>> +	}
>> +	dpll_device_put(bp->dpll);
>>   out:
>>   	ptp_ocp_detach(bp);
>>   out_disable:
> 
> ...