Message-ID: <a95906c3-1960-e753-e306-74a90045269e@redhat.com>
Date:   Mon, 21 Sep 2020 12:32:16 -0700
From:   Tom Rix <trix@...hat.com>
To:     Xu Yilun <yilun.xu@...el.com>
Cc:     alex.williamson@...hat.com, hao.wu@...el.com, mdf@...nel.org,
        linux-kernel@...r.kernel.org, linux-fpga@...r.kernel.org
Subject: Re: [RFC] fpga: dfl: a prototype uio driver


On 9/21/20 1:55 AM, Xu Yilun wrote:
> On Sat, Sep 19, 2020 at 09:51:13AM -0700, trix@...hat.com wrote:
>> From: Tom Rix <trix@...hat.com>
>>
>> I following up on Moritz asking for early RFC's by showing how this
>> could be done with the concrete example of around
>>
>> [PATCH 0/3] add VFIO mdev support for DFL devices
>>
>> I hacked this together, it barely works. Do not expect that this
>> patch to apply anywhere.  It has enough to show where I want
>> to go and people can comment without me having invested a lot of
>> effort.  I am not expecting to carry this forward, it only
>> addresses the issues I had with the origin patch.
>>
>> This change adds a uio driver for any unclaimed dfl feature
>>
>> During the normal matching of feature id's to drivers, some
>> of the features are not claimed because there neither a
>> builtin driver nor a module driver that matches the feature
>> id.  For these unclaimed features, provide a uio driver to a
>> possible user space driver.
> I think we don't have to setup UIO interfaces for all unclaimed 
> features. It could be the decision of the user whether the UIO
> device is needed for a feature. My opinion is that, the
> driver_override sysfs is still generic enough for user's to switch
> between kernel device drivers and dfl uio driver.

Instead of a string, could there just be a 'use_uio' flag ?

How does the 'driver_override' work when there is no starting driver ?

>
> There may be cases the dfl device driver modules are not inserted
> during dfl-fme/port initialization, but they are to be inserted
> manually. If the features are all registered to uio, then there will
> be problems when dfl device drivers module are inserted.

How does this manual loading work ? The driver list for the features

seems to only be used during the card probe time.

To change the order the dfl-pci needs to be unloaded and that will

destroy all the uio devices as well as usual feature drivers attached to

the pci driver.


>
>
>> I have doubts that a uio for an afu feature is approptiate as these
>> can be any device.
> I think generally afu could also be as uio device if we don't concern
> about dma isolation.

I am thinking about afu with its own pci id.

So there could be a conflict with some other driver that responds to the pci id.

>
> But now seems not possible to match afu to uio driver, cause now in DFL
> framework AFU is managed by dfl-afu.ko
>
>> Another possible problem is if the number of interrupts for the
>> feature is greater than 1.  Uio seems to only support 1. My guess
>> is this would need some hackery in the open() to add to the
>> interrupt handler.
> I think it may not possible for UIO to support multiple interrupts if
> user cannot access the interrupt enable/pending registers. The uio
> device have just one /dev/uioX node for interrupt. So I tend to
> accept the limitation. And for now we don't have board specific
> features that needs multiple interrupts. For PAC N3000, no interrupt is
> needed.
Maybe uio needs to change to support multiple interrupts ?
>
>> It is for this type of reason I think a dfl-uio driver is needed
>> rather than reusing an existing generic uio driver.
> So if we don't try to change interrupt, the implementation of dfl-uio is
> just a subset of generic uio platform driver, so why not we just use it?

Its a toss up.

Maybe there some dfl only constraints like write/read is a multiple 4 bytes

Or just why have another layer in the setup.

Tom

>
> Thanks,
> Yilun
>
>> Signed-off-by: Tom Rix <trix@...hat.com>
>> ---
>>  drivers/fpga/dfl-fme-main.c |   9 ++-
>>  drivers/fpga/dfl-uio.c      | 107 ++++++++++++++++++++++++++++++++++++
>>  drivers/fpga/dfl.c          |  47 +++++++++++++++-
>>  drivers/fpga/dfl.h          |   8 +++
>>  4 files changed, 169 insertions(+), 2 deletions(-)
>>  create mode 100644 drivers/fpga/dfl-uio.c
>>
>> diff --git a/drivers/fpga/dfl-fme-main.c b/drivers/fpga/dfl-fme-main.c
>> index 037dc4f..3323e90 100644
>> --- a/drivers/fpga/dfl-fme-main.c
>> +++ b/drivers/fpga/dfl-fme-main.c
>> @@ -709,12 +709,18 @@ static int fme_probe(struct platform_device *pdev)
>>  	if (ret)
>>  		goto dev_destroy;
>>  
>> -	ret = dfl_fpga_dev_ops_register(pdev, &fme_fops, THIS_MODULE);
>> +	ret = dfl_fpga_dev_feature_init_uio(pdev, DFH_TYPE_FIU);
>>  	if (ret)
>>  		goto feature_uinit;
>>  
>> +	ret = dfl_fpga_dev_ops_register(pdev, &fme_fops, THIS_MODULE);
>> +	if (ret)
>> +		goto feature_uinit_uio;
>> +
>>  	return 0;
>>  
>> +feature_uinit_uio:
>> +	dfl_fpga_dev_feature_uinit_uio(pdev, DFH_TYPE_FIU);
>>  feature_uinit:
>>  	dfl_fpga_dev_feature_uinit(pdev);
>>  dev_destroy:
>> @@ -726,6 +732,7 @@ exit:
>>  static int fme_remove(struct platform_device *pdev)
>>  {
>>  	dfl_fpga_dev_ops_unregister(pdev);
>> +	dfl_fpga_dev_feature_uinit_uio(pdev, DFH_TYPE_FIU);
>>  	dfl_fpga_dev_feature_uinit(pdev);
>>  	fme_dev_destroy(pdev);
>>  
>> diff --git a/drivers/fpga/dfl-uio.c b/drivers/fpga/dfl-uio.c
>> new file mode 100644
>> index 0000000..185fbab
>> --- /dev/null
>> +++ b/drivers/fpga/dfl-uio.c
>> @@ -0,0 +1,107 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * prototype dfl uio driver
>> + *
>> + * Copyright Tom Rix 2020
>> + */
>> +#include <linux/module.h>
>> +#include "dfl.h"
>> +
>> +static irqreturn_t dfl_uio_handler(int irq, struct uio_info *dev_info)
>> +{
>> +	return IRQ_HANDLED;
>> +}
>> +
>> +static int dfl_uio_mmap(struct uio_info *info, struct vm_area_struct *vma)
>> +{
>> +	return 0;
>> +}
>> +
>> +static int dfl_uio_open(struct uio_info *info, struct inode *inode)
>> +{
>> +	return 0;
>> +}
>> +
>> +static int dfl_uio_release(struct uio_info *info, struct inode *inode)
>> +{
>> +	return 0;
>> +}
>> +
>> +static int dfl_uio_irqcontrol(struct uio_info *info, s32 irq_on)
>> +{
>> +	return 0;
>> +}
>> +
>> +int dfl_uio_add(struct dfl_feature *feature)
>> +{
>> +	struct uio_info *uio;
>> +	struct resource *res =
>> +		&feature->dev->resource[feature->resource_index];
>> +	int ret = 0;
>> +
>> +	uio = kzalloc(sizeof(struct uio_info), GFP_KERNEL);
>> +	if (!uio) {
>> +		ret = -ENOMEM;
>> +		goto exit;
>> +	}
>> +
>> +	uio->name = kasprintf(GFP_KERNEL, "dfl-uio-%llx", feature->id);
>> +	if (!uio->name) {
>> +		ret = -ENOMEM;
>> +		goto err_name;
>> +	}
>> +
>> +	uio->version = "0.1";
>> +	uio->mem[0].memtype = UIO_MEM_PHYS;
>> +	uio->mem[0].addr = res->start & PAGE_MASK;
>> +	uio->mem[0].offs = res->start & ~PAGE_MASK;
>> +	uio->mem[0].size = (uio->mem[0].offs + resource_size(res)
>> +			    + PAGE_SIZE - 1) & PAGE_MASK;
>> +	/* How are nr_irqs > 1 handled ??? */
>> +	if (feature->nr_irqs == 1)
>> +		uio->irq = feature->irq_ctx[0].irq;
>> +	uio->handler = dfl_uio_handler;
>> +	uio->mmap = dfl_uio_mmap;
>> +	uio->open = dfl_uio_open;
>> +	uio->release = dfl_uio_release;
>> +	uio->irqcontrol = dfl_uio_irqcontrol;
>> +
>> +	ret = uio_register_device(&feature->dev->dev, uio);
>> +	if (ret)
>> +		goto err_register;
>> +
>> +	feature->uio = uio;
>> +exit:
>> +	return ret;
>> +err_register:
>> +	kfree(uio->name);
>> +err_name:
>> +	kfree(uio);
>> +	goto exit;
>> +}
>> +EXPORT_SYMBOL_GPL(dfl_uio_add);
>> +
>> +int dfl_uio_remove(struct dfl_feature *feature)
>> +{
>> +	uio_unregister_device(feature->uio);
>> +	kfree(feature->uio->name);
>> +	kfree(feature->uio);
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(dfl_uio_remove);
>> +
>> +static int __init dfl_uio_init(void)
>> +{
>> +	return 0;
>> +}
>> +
>> +static void __exit dfl_uio_exit(void)
>> +{
>> +}
>> +
>> +module_init(dfl_uio_init);
>> +module_exit(dfl_uio_exit);
>> +
>> +MODULE_DESCRIPTION("DFL UIO prototype driver");
>> +MODULE_AUTHOR("Tom");
>> +MODULE_LICENSE("GPL v2");
>> diff --git a/drivers/fpga/dfl.c b/drivers/fpga/dfl.c
>> index 1305be4..26de8e1 100644
>> --- a/drivers/fpga/dfl.c
>> +++ b/drivers/fpga/dfl.c
>> @@ -663,10 +664,57 @@ exit:
>>  }
>>  EXPORT_SYMBOL_GPL(dfl_fpga_dev_feature_init);
>>  
>> +int dfl_fpga_dev_feature_init_uio(struct platform_device *pdev, int dfh_type)
>> +{
>> +	struct dfl_feature_platform_data *pdata = dev_get_platdata(&pdev->dev);
>> +	struct dfl_feature *feature;
>> +	int ret;
>> +
>> +	dfl_fpga_dev_for_each_feature(pdata, feature) {
>> +		if (dfh_type == DFH_TYPE_FIU) {
>> +			if (feature->id == FEATURE_ID_FIU_HEADER ||
>> +			    feature->id == FEATURE_ID_AFU)
>> +			continue;
>> +
>> +			/* give the unclamined feature to uio */
>> +			if (!feature->ioaddr) {
>> +				ret = dfl_uio_add(feature);
>> +				if (ret)
>> +					goto exit;
>> +			}
>> +		}
>> +	}
>> +
>> +	return 0;
>> +exit:
>> +	dfl_fpga_dev_feature_uinit_uio(pdev, dfh_type);
>> +	return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(dfl_fpga_dev_feature_init_uio);
>> +
>> +int dfl_fpga_dev_feature_uinit_uio(struct platform_device *pdev, int dfh_type)
>> +{
>> +	struct dfl_feature_platform_data *pdata = dev_get_platdata(&pdev->dev);
>> +	struct dfl_feature *feature;
>> +	int ret = 0;
>> +
>> +	dfl_fpga_dev_for_each_feature(pdata, feature) {
>> +		if (dfh_type == DFH_TYPE_FIU) {
>> +			if (feature->id == FEATURE_ID_FIU_HEADER ||
>> +			    feature->id == FEATURE_ID_AFU)
>> +				continue;
>> +
>> +			if (feature->uio)
>> +				ret |= dfl_uio_remove(feature);
>> +		}
>> +	}
>> +	return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(dfl_fpga_dev_feature_uinit_uio);
>> +
>>  static void dfl_chardev_uinit(void)
>>  {
>>  	int i;
>> -
>>  	for (i = 0; i < DFL_FPGA_DEVT_MAX; i++)
>>  		if (MAJOR(dfl_chrdevs[i].devt)) {
>>  			unregister_chrdev_region(dfl_chrdevs[i].devt,
>> diff --git a/drivers/fpga/dfl.h b/drivers/fpga/dfl.h
>> index a85d1cd..6e37aef 100644
>> --- a/drivers/fpga/dfl.h
>> +++ b/drivers/fpga/dfl.h
>> @@ -26,6 +26,7 @@
>>  #include <linux/platform_device.h>
>>  #include <linux/slab.h>
>>  #include <linux/uuid.h>
>> +#include <linux/uio_driver.h>
>>  #include <linux/fpga/fpga-region.h>
>>  
>>  /* maximum supported number of ports */
>> @@ -232,6 +233,7 @@ struct dfl_feature_irq_ctx {
>>   * struct dfl_feature - sub feature of the feature devices
>>   *
>>   * @dev: ptr to pdev of the feature device which has the sub feature.
>> + * @uio: for fallback uio driver.
>>   * @id: sub feature id.
>>   * @index: unique identifier for an sub feature within the feature device.
>>   *	   It is possible that multiply sub features with same feature id are
>> @@ -248,6 +250,7 @@ struct dfl_feature_irq_ctx {
>>   */
>>  struct dfl_feature {
>>  	struct platform_device *dev;
>> +	struct uio_info *uio;
>>  	u64 id;
>>  	int index;
>>  	int resource_index;
>> @@ -360,6 +363,11 @@ void dfl_fpga_dev_feature_uinit(struct platform_device *pdev);
>>  int dfl_fpga_dev_feature_init(struct platform_device *pdev,
>>  			      struct dfl_feature_driver *feature_drvs);
>>  
>> +int dfl_fpga_dev_feature_init_uio(struct platform_device *pdev, int dfh_type);
>> +int dfl_fpga_dev_feature_uinit_uio(struct platform_device *pdev, int dfh_type);
>> +int dfl_uio_add(struct dfl_feature *feature);
>> +int dfl_uio_remove(struct dfl_feature *feature);
>> +
>>  int dfl_fpga_dev_ops_register(struct platform_device *pdev,
>>  			      const struct file_operations *fops,
>>  			      struct module *owner);
>> -- 
>> 2.18.1

Powered by blists - more mailing lists