On 9/21/20 1:55 AM, Xu Yilun wrote:
> On Sat, Sep 19, 2020 at 09:51:13AM -0700, trix@xxxxxxxxxx wrote:
>> From: Tom Rix <trix@xxxxxxxxxx>
>>
>> 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@xxxxxxxxxx>
>> ---
>> 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
