Re: [RFC PATCH v2] fpga: Introduce new fpga subsystem

[Michal Simek]

On 10/02/2013 07:46 PM, Jason Gunthorpe wrote:
> On Wed, Oct 02, 2013 at 05:35:58PM +0200, Michal Simek wrote:
> 
>> +What:		/sys/class/fpga_manager/fpga<dev-id>/fpga_config_state
>> +Date:		October 2013
>> +KernelVersion:	3.12
>> +Contact:	Michal Simek <michal.simek@xxxxxxxxxx>
>> +Description:
>> +		By reading this file you will get current fpga manager state.
>> +		Flag bits are present in include/linux/fpga.h (FPGA_MGR_XX).
>> +		By writing to this file you can change fpga manager state.
>> +		Valid options are: write_init, write_complete, read_init,
>> +		read_complete.
> 
> This shouldn't be asymmetric - read/write should be in the same
> format.
> 
> I strongly encourage you to use text strings to indicate the state of
> the configuration FSM, and I *really* think you should rework things
> to have an explicit configuration FSM rather than trying to bodge one
> together with a bunch of bit flags.

Any favourite names for states?
Or ready, write_init, write_complete is enough for now?

Alan: This should be unified way for user to get proper states from the
driver. It means from my point of view will be better to have set of states
which this function can return instead of calling origin status function
where we can't control states for all these drivers.

> Plus error handling is missing, failures need to be reported back.

Will fix this.

> Noticed several typos:

Ah yeah c&p. :-(

> 
>> +
>> +	if (mgr->mops->read_init) {
>> +		ret = mgr->mops->read_init(mgr);
>> +		if (ret) {
>> +			dev_dbg(mgr->dev, "Failed to write_init\n");
>                                                     ^^^^^^^^
> read_init
> 
>> +	if (mgr->mops->write) {
>                    ^^^^^^^^^^
> read
> 
>> +		ret = mgr->mops->read(mgr, buf, count);
>> +		if (ret) {
>> +			dev_dbg(mgr->dev, "Failed to write\n");
>                                              ^^^^^^^^^^^^^^^^^
> read
> 
>> +
>> +	if (mgr->mops->write_complete) {
>                     ^^^^^^^^^^
> read
>> +		ret = mgr->mops->read_complete(mgr);
>> +		if (ret) {
>> +			dev_dbg(mgr->dev, "Failed to write_complete\n");
>   					  	  ^^^^^^^^^^^^
> read
> 
>> +static inline int fpga_mgr_write(struct fpga_manager *mgr, const u8 *buf,
>> +				 ssize_t count)
>> +{
>> +	int bit, ret;
>> +
>> +	dev_dbg(mgr->dev, "%s %lx\n", __func__, mgr->flags);
>> +
>> +	/* FPGE init has to be done to be able to continue */
>            ^^^^^^
> FPGA
> 
>> +static struct device_attribute fpga_mgr_attrs[] = {
>> +	__ATTR(name, S_IRUGO, fpga_mgr_name_show, NULL),
>> +	__ATTR(firmware, S_IWUSR, NULL, fpga_mgr_attr_write),
>> +	__ATTR(fpga_config_state, S_IRUSR | S_IWUSR,
>> +	       fpga_mgr_status_read, fpga_mgr_status_write),
>> +	__ATTR_NULL
>> +};
> 
> AFAIK it is preferred to use DEVICE_ATTR_RO(), ATTRIBUTE_GROUPS()
> and struct class.dev_groups
> 
> eg see this note in linux/device.h
> 
> struct class {
>         struct device_attribute         *dev_attrs;     /* use dev_groups instead */
>         const struct attribute_group    **dev_groups;
> }
> 

This will be fixed in v3. I have already changed this.

>> +	struct fpga_manager *mgr;
>> +	int ret;
>> +
>> +	if (!mops) {
>> +		dev_dbg(&pdev->dev, "Register failed: NO fpga_manager_ops\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	if (!name || !strlen(name)) {
>> +		dev_dbg(&pdev->dev, "Register failed: NO name specific\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	mgr = devm_kzalloc(&pdev->dev, sizeof(*mgr), GFP_KERNEL);
>> +	if (!mgr)
>> +		return -ENOMEM;
> 
> I wonder if this is right, it seems like a strange way to make a class
> subsystem, usually the struct fpga_manager would contain the 'struct
> device' (not a pointer, so you can use container_of) and drvdata would
> be reserved for something else.

I am not following you here. mrg structure is connected with the driver
it means when driver is removed then this structure is freed.


> 
> This seems to create lifetime issues since the devm above will be
> free'd when the platform driver is released, but the class device will
> live on with the stray pointer. Better to allocate everything against
> the class device below.

device in unregistered before this structure is freed.
fpga_mgr_unregister() is called in the platform driver in remove function.

> 
> Plus you need to ensure the device is fully functionally before
> device_register is called, otherwise you race with notifications to
> userspace.

fpga_mgr_register() should be called as the latest function in the probe
in platform_driver. At least it is what I have for zynq.

>> +/**
>> + * fpga_mgr_unregister: Remove fpga manager
>> + * @pdev: Pointer to the platform device of fpga manager
>> + *
>> + * Function unregister fpga manager and release all temporary structures
>> + *
>> + * Returns 0 for all cases
>> + */
>> +int fpga_mgr_unregister(struct platform_device *pdev)
>> +{
>> +	struct fpga_manager *mgr = platform_get_drvdata(pdev);
>> +
>> +	if (mgr && mgr->mops && mgr->mops->fpga_remove)
>> +		mgr->mops->fpga_remove(mgr);
>> +
>> +	device_unregister(mgr->dev);
>> +
>> +	spin_lock(&fpga_mgr_idr_lock);
>> +	idr_remove(&fpga_mgr_idr, mgr->nr);
>> +	spin_unlock(&fpga_mgr_idr_lock);
> 
> What happens when userspace is holding one of the sysfs files open and
> you unload the module? Looks like bad things?

I didn't test this but feel free to check it.

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

Attachment: signature.asc
Description: OpenPGP digital signature