linux-kernel - Re: [RFC PATCH v2] fpga: Introduce new fpga subsystem

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: <524EE9A0.2020404@monstr.eu>
Date:	Fri, 04 Oct 2013 18:15:28 +0200
From:	Michal Simek <monstr@...str.eu>
To:	Joe Perches <joe@...ches.com>
CC:	Michal Simek <michal.simek@...inx.com>,
	linux-kernel@...r.kernel.org, Alan Tull <atull@...era.com>,
	Pavel Machek <pavel@....cz>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Dinh Nguyen <dinguyen@...era.com>,
	Philip Balister <philip@...ister.org>,
	Alessandro Rubini <rubini@...dd.com>,
	Steffen Trumtrar <s.trumtrar@...gutronix.de>,
	"H. Peter Anvin" <hpa@...or.com>,
	Jason Gunthorpe <jgunthorpe@...idianresearch.com>,
	Jason Cooper <jason@...edaemon.net>,
	Yves Vandervennet <rocket.yvanderv@...il.com>,
	Kyle Teske <kyle.teske@...com>,
	Josh Cartwright <joshc@....teric.us>,
	Rob Landley <rob@...dley.net>,
	Mauro Carvalho Chehab <m.chehab@...sung.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Cesar Eduardo Barros <cesarb@...arb.net>,
	"David S. Miller" <davem@...emloft.net>,
	David Brown <davidb@...eaurora.org>,
	Samuel Ortiz <sameo@...ux.intel.com>,
	Nicolas Pitre <nico@...aro.org>,
	Mark Langsdorf <mark.langsdorf@...xeda.com>,
	Felipe Balbi <balbi@...com>, linux-doc@...r.kernel.org
Subject: Re: [RFC PATCH v2] fpga: Introduce new fpga subsystem

Hi Joe,

On 10/02/2013 06:06 PM, Joe Perches wrote:
> On Wed, 2013-10-02 at 17:35 +0200, Michal Simek wrote:
>> This new fpga subsystem core should unify all fpga drivers/managers which
>> do the same things. Load configuration data to fpga or another programmable
>> logic through common interface. It doesn't matter if it is MMIO device,
>> gpio bitbanging, etc. connection. The point is to have the same
>> interface for these drivers.
> 
> Does this interface support partial reprogramming/configuration
> for FPGAs that can do that?

Currently we have two interfaces and third one is around (char driver)
that's why I didn't look at this support.
But for Xilinx devices init and complete phase is different.

We can use one more flag parameter for init and complete functions.

It means we can simple extend config states with write_init_partial, etc
Or the second option is to create new sysfs file to indicate
that we will work with partial bitstreams.

> trivial notes:
> 
> There are a _lot_ of dev_dbg statements.
> 
> I hope some of these would be removed one day,
> especially the function tracing style ones, because
> there's already a generic kernel mechanism for that.
> 
> Maybe perf/trace support could be added eventually.

Are you talking about trace_printk?

>> diff --git a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c
> []
>> +/**
>> + * fpga_mgr_status_write - Write fpga manager status
>> + * @dev: Pointer to the device structure
>> + * @attr: Pointer to the device attribute structure
>> + * @buf: Pointer to the buffer location
>> + * @count: Number of characters in @buf
>> + *
>> + * Returns the number of bytes copied to @buf, a negative error number otherwise
>> + */
>> +static ssize_t fpga_mgr_status_write(struct device *dev,
>> +				     struct device_attribute *attr,
>> +				     const char *buf, size_t count)
>> +{
>> +	struct fpga_manager *mgr = dev_get_drvdata(dev);
>> +	int ret;
>> +
>> +	if (test_and_set_bit_lock(FPGA_MGR_DEV_BUSY, &mgr->flags))
>> +		return -EBUSY;
>> +
>> +	ret = strcmp(buf, "write_init");
>> +	if (!ret) {
>> +		ret = fpga_mgr_write_init(mgr);
>> +		goto out;
>> +	}
>> +
>> +	ret = strcmp(buf, "write_complete");
>> +	if (!ret) {
>> +		ret = fpga_mgr_write_complete(mgr);
>> +		goto out;
>> +	}
>> +
>> +	ret = strcmp(buf, "read_init");
>> +	if (!ret) {
>> +		ret = fpga_mgr_read_init(mgr);
>> +		goto out;
>> +	}
>> +
>> +	ret = strcmp(buf, "read_complete");
>> +	if (!ret) {
>> +		ret = fpga_mgr_read_complete(mgr);
>> +		goto out;
>> +	}
>> +
>> +	ret = -EINVAL;
>> +out:
>> +	clear_bit_unlock(FPGA_MGR_DEV_BUSY, &mgr->flags);
>> +
>> +	return ret ? : count;
>> +}
> 
> I think this style is awkward and this would be
> better written as:
> 
> 	if (!strcmp(buf, "write_init"))
> 		ret = fpga_mgr_write_init(mgr);
> 	else if (!strcmp(buf, "write_complete"))
> 		ret = fpga_mgr_write_complete(mgr);
> 	else if (!strcmp(buf, "read_init"))
> 		ret = fpga_mgr_read_init(mgr);
> 	else if (!strcmp(buf, "read_complete"))
> 		ret = fpga_mgr_read_complete(mgr);
> 	else
> 		ret = -EINVAL;
> 
> 	clear_bit_unlock(FPGA_MGR_DEV_BUSY, &mgr->flags);
> 
> 	if (ret)
> 		return ret;
> 
> 	return count;
> }
> 
> Maybe use (strcmp(...) == 0) if you prefer that.
> Both styles are commonly used in linux.


sure.


> Probably all of the "return ret ?: count;" uses
> would be more easily understood on 3 lines.

This structure is also quite common in the kernel
git grep "? :"  | wc -l
415

git grep "?:"  | wc -l
541

Thanks,
Michal

-- 
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/
Maintainer of Linux kernel - Xilinx Zynq ARM architecture
Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform



Download attachment "signature.asc" of type "application/pgp-signature" (264 bytes)