Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH v6 1/2] fpga: dfl: add the userspace I/O device support
 for DFL devices
To:     Moritz Fischer <mdf@kernel.org>,
        Greg KH <gregkh@linuxfoundation.org>
Cc:     Xu Yilun <yilun.xu@intel.com>, linux-fpga@vger.kernel.org,
        linux-kernel@vger.kernel.org, lgoncalv@redhat.com, hao.wu@intel.com
References: <1610502848-30345-1-git-send-email-yilun.xu@intel.com>
 <1610502848-30345-2-git-send-email-yilun.xu@intel.com>
 <YARbgGU6lr3zZaKP@kroah.com> <YARkTFMrotPo45ic@epycbox.lan>
From:   Tom Rix <trix@redhat.com>
Message-ID: <1d205328-6ffa-0f77-0bdf-0f4b822edc3a@redhat.com>
Date:   Thu, 21 Jan 2021 06:30:20 -0800
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.6.0
MIME-Version: 1.0
In-Reply-To: <YARkTFMrotPo45ic@epycbox.lan>
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 7bit
Content-Language: en-US
Precedence: bulk


On 1/17/21 8:22 AM, Moritz Fischer wrote:
> Greg,
>
> On Sun, Jan 17, 2021 at 04:45:04PM +0100, Greg KH wrote:
>> On Wed, Jan 13, 2021 at 09:54:07AM +0800, Xu Yilun wrote:
>>> This patch supports the DFL drivers be written in userspace. This is
>>> realized by exposing the userspace I/O device interfaces.
>>>
>>> The driver leverages the uio_pdrv_genirq, it adds the uio_pdrv_genirq
>>> platform device with the DFL device's resources, and let the generic UIO
>>> platform device driver provide support to userspace access to kernel
>>> interrupts and memory locations.
>> Why doesn't the existing uio driver work for this, why do you need a new
>> one?
>>
>>> ---
>>>  drivers/fpga/Kconfig        | 10 +++++
>>>  drivers/fpga/Makefile       |  1 +
>>>  drivers/fpga/dfl-uio-pdev.c | 93 +++++++++++++++++++++++++++++++++++++++++++++
>> uio drivers traditionally go in drivers/uio/ and start with "uio", so
>> shouldn't this be drivers/uio/uio_dfl_pdev.c to match the same naming
>> scheme?
> I had considered suggesting that, but ultimately this driver only
> creates a 'uio_pdrv_genirq' platform device, so it didn't seem like a
> good fit.
>> But again, you need to explain in detail, why the existing uio driver
>> doesn't work properly, or why you can't just add a few lines to an
>> existing one.
> Ultimately there are three options I see:
> 1) Do what Xu does, which is re-use the 'uio_pdrv_genirq' uio driver by
>   creating a platform device for it as sub-device of the dfl device that
>   we bind to uio_pdrv_genirq
> 2) Add a module_dfl_driver part to drivers/uio/uio_pdrv_genirq.c and
>   corresponding id table
> 3) Create a new uio_dfl_genirq kind of driver that uses the dfl bus and
>   that would make sense to then put into drivers/uio. (This would
>   duplicate code in uio_pdrv_genirq to some extend)
>
> Overall I think in terms of code re-use I think Xu's choice might be
> less new code as it simply wraps the uio platform device driver, and
> allows for defining the resources passed to the UIO driver to be defined
> by hardware through a DFL.
>
> I've seen the pattern that Xu proposed used in other places like the
> macb network driver where you'd have macb_main (the platform driver) and
> macb_pci that wraps it for a pci usage.
>
> - Moritz

Thinking of this problem more generally.

Every fpga will have a handful of sub devices.

Do we want to carry them in the fpga subsystem or carry them in the other subsystems ?

Consider the short term reviewing and long term maintenance of the sub devices by the subsystem maintainers.

It easier for them if the sub devices are in the other subsystems.


Applying this to specifically for dfl_uio.

No one from the uio subsystem reviewing this change is a problem.

I think this change needs to go to the uio subsystem.

And a new entry in the MAINTAINERS file to keep the fpga subsystem in the loop for reviews and acks

Tom