linux-kernel - Re: [PATCH v5 2/4] fpga pr ip: Core driver support for Altera Partial Reconfiguration IP.

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, 20 Mar 2017 16:49:32 -0700 (PDT)
From:   matthew.gerlach@...ux.intel.com
To:     Anatolij Gustschin <agust@...x.de>
cc:     atull@...nel.org, moritz.fischer@...us.com,
        linux-fpga@...r.kernel.org, linux-kernel@...r.kernel.org,
        devicetree@...r.kernel.org, robh+dt@...nel.org,
        mark.rutland@....com
Subject: Re: [PATCH v5 2/4] fpga pr ip: Core driver support for Altera Partial
 Reconfiguration IP.



On Sat, 18 Mar 2017, Anatolij Gustschin wrote:

> Hi Matthew,
>

Hi Anatolij,

More good feedback.  See below.


> thanks for the patches. Please see some comments below.
>
> On Fri, 10 Mar 2017 11:40:25 -0800
> matthew.gerlach@...ux.intel.com matthew.gerlach@...ux.intel.com wrote:
>
> ...
>> +	if (!(info->flags & FPGA_MGR_PARTIAL_RECONFIG)) {
>> +		pr_err("%s Partial Reconfiguration flag not set\n", __func__);
>
> please use dev_err() here.
>
> ...
>> +	if (val & ALT_PR_CSR_PR_START) {
>> +		pr_err("%s Partial Reconfiguration already started\n",
>
> dev_err(), too.

Good catch on both of these pr_err().  dev_err() would be better.
>
> ...
>> +static int alt_pr_fpga_write_complete(struct fpga_manager *mgr,
>> +				      struct fpga_image_info *info)
>> +{
>> +	u32 i;
>> +
>> +	for (i = 0; i < info->config_complete_timeout_us; i++) {
>> +		switch (alt_pr_fpga_state(mgr)) {
>> +		case FPGA_MGR_STATE_WRITE_ERR:
>> +			return -EIO;
>> +
>> +		case FPGA_MGR_STATE_OPERATING:
>> +			dev_info(&mgr->dev,
>> +				 "successful partial reconfiguration\n");
>> +			return 0;
>> +
>> +		default:
>> +			break;
>> +		}
>> +		udelay(1);
>> +	}
>> +	dev_err(&mgr->dev, "timed out waiting for write to complete\n");
>> +	return -ETIMEDOUT;
>> +}
>
> we will always get timed out error if info->config_complete_timeout_us
> is zero. Can we change to
> 	u32 i = 0;
> 	...
> 	do {
> 		...
> 	} while (info->config_complete_timeout_us > i++);
> ?

This seems more than reasonable to me.

>
> ...
>> diff --git a/drivers/fpga/altera-pr-ip-core.h b/drivers/fpga/altera-pr-ip-core.h
>> new file mode 100644
>> index 0000000..3810a90
>> --- /dev/null
>> +++ b/drivers/fpga/altera-pr-ip-core.h
>
> Should we move this header to include/linux/? We can use register/
> unregister functions in other drivers (PCIe) outside drivers/fpga
> then.
>
> Thanks,
>
> Anatolij
>