Re: [PATCH v2 02/17] compat_ioctl: move drivers to generic_compat_ioctl_ptrarg
On Wed, Sep 12, 2018 at 05:01:03PM +0200, Arnd Bergmann wrote:
> Each of these drivers has a copy of the same trivial helper function to
> convert the pointer argument and then call the native ioctl handler.
>
> We now have a generic implementation of that, so use it.
>
> Signed-off-by: Arnd Bergmann
> drivers/char/ppdev.c | 12 +-
> drivers/char/tpm/tpm_vtpm_proxy.c | 12 +-
> drivers/firewire/core-cdev.c | 12 +-
> drivers/hid/usbhid/hiddev.c | 11 +
> drivers/hwtracing/stm/core.c | 12 +-
> drivers/misc/mei/main.c | 22 +
> drivers/mtd/ubi/cdev.c| 36 +++-
> drivers/net/tap.c | 12 +-
> drivers/staging/pi433/pi433_if.c | 12 +-
> drivers/usb/core/devio.c | 16 +
> drivers/vfio/vfio.c | 39 +++
> drivers/vhost/net.c | 12 +-
> drivers/vhost/scsi.c | 12 +-
> drivers/vhost/test.c | 12 +-
> drivers/vhost/vsock.c | 12 +-
> fs/fat/file.c | 13 +--
> 16 files changed, 20 insertions(+), 237 deletions(-)
>
> diff --git a/drivers/char/tpm/tpm_vtpm_proxy.c
> b/drivers/char/tpm/tpm_vtpm_proxy.c
> index 87a0ce47f201..a170f5ca7416 100644
> +++ b/drivers/char/tpm/tpm_vtpm_proxy.c
> @@ -678,20 +678,10 @@ static long vtpmx_fops_ioctl(struct file *f, unsigned
> int ioctl,
> }
> }
>
> -#ifdef CONFIG_COMPAT
> -static long vtpmx_fops_compat_ioctl(struct file *f, unsigned int ioctl,
> - unsigned long arg)
> -{
> - return vtpmx_fops_ioctl(f, ioctl, (unsigned long)compat_ptr(arg));
> -}
> -#endif
> -
> static const struct file_operations vtpmx_fops = {
> .owner = THIS_MODULE,
> .unlocked_ioctl = vtpmx_fops_ioctl,
> -#ifdef CONFIG_COMPAT
> - .compat_ioctl = vtpmx_fops_compat_ioctl,
> -#endif
> + .compat_ioctl = generic_compat_ioctl_ptrarg,
> .llseek = noop_llseek,
> };
For vtpm:
Reviewed-by: Jason Gunthorpe
Arnd, would you consider including a patch as part of/after this
series to make compat_ioctl in drivers/infiniband/core/uverbs_main.c
use this as well? Looks like a bug too?
Thanks,
Jason
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 05/17] compat_ioctl: move more drivers to generic_compat_ioctl_ptrarg
On Wed, Sep 12, 2018 at 05:08:52PM +0200, Arnd Bergmann wrote:
> The .ioctl and .compat_ioctl file operations have the same prototype so
> they can both point to the same function, which works great almost all
> the time when all the commands are compatible.
>
> One exception is the s390 architecture, where a compat pointer is only
> 31 bit wide, and converting it into a 64-bit pointer requires calling
> compat_ptr(). Most drivers here will ever run in s390, but since we now
> have a generic helper for it, it's easy enough to use it consistently.
>
> I double-checked all these drivers to ensure that all ioctl arguments
> are used as pointers or are ignored, but are not interpreted as integer
> values.
>
> Signed-off-by: Arnd Bergmann
> drivers/android/binder.c| 2 +-
> drivers/crypto/qat/qat_common/adf_ctl_drv.c | 2 +-
> drivers/dma-buf/dma-buf.c | 4 +---
> drivers/dma-buf/sw_sync.c | 2 +-
> drivers/dma-buf/sync_file.c | 2 +-
> drivers/gpu/drm/amd/amdkfd/kfd_chardev.c| 2 +-
> drivers/hid/hidraw.c| 4 +---
> drivers/iio/industrialio-core.c | 2 +-
> drivers/infiniband/core/uverbs_main.c | 4 ++--
> drivers/media/rc/lirc_dev.c | 4 +---
> drivers/mfd/cros_ec_dev.c | 4 +---
> drivers/misc/vmw_vmci/vmci_host.c | 2 +-
> drivers/nvdimm/bus.c| 4 ++--
> drivers/nvme/host/core.c| 2 +-
> drivers/pci/switch/switchtec.c | 2 +-
> drivers/platform/x86/wmi.c | 2 +-
> drivers/rpmsg/rpmsg_char.c | 4 ++--
> drivers/sbus/char/display7seg.c | 2 +-
> drivers/sbus/char/envctrl.c | 4 +---
> drivers/scsi/3w-.c | 4 +---
> drivers/scsi/cxlflash/main.c| 2 +-
> drivers/scsi/esas2r/esas2r_main.c | 2 +-
> drivers/scsi/pmcraid.c | 4 +---
> drivers/staging/android/ion/ion.c | 4 +---
> drivers/staging/vme/devices/vme_user.c | 2 +-
> drivers/tee/tee_core.c | 2 +-
> drivers/usb/class/cdc-wdm.c | 2 +-
> drivers/usb/class/usbtmc.c | 4 +---
> drivers/video/fbdev/ps3fb.c | 2 +-
> drivers/virt/fsl_hypervisor.c | 2 +-
> fs/btrfs/super.c| 2 +-
> fs/ceph/dir.c | 2 +-
> fs/ceph/file.c | 2 +-
> fs/fuse/dev.c | 2 +-
> fs/notify/fanotify/fanotify_user.c | 2 +-
> fs/userfaultfd.c| 2 +-
> net/rfkill/core.c | 2 +-
> 37 files changed, 40 insertions(+), 58 deletions(-)
> diff --git a/drivers/infiniband/core/uverbs_main.c
> b/drivers/infiniband/core/uverbs_main.c
> index 823beca448e1..f4755c1c9cfa 100644
> +++ b/drivers/infiniband/core/uverbs_main.c
> @@ -930,7 +930,7 @@ static const struct file_operations uverbs_fops = {
> .release = ib_uverbs_close,
> .llseek = no_llseek,
> .unlocked_ioctl = ib_uverbs_ioctl,
> - .compat_ioctl = ib_uverbs_ioctl,
> + .compat_ioctl = generic_compat_ioctl_ptrarg,
> };
>
> static const struct file_operations uverbs_mmap_fops = {
> @@ -941,7 +941,7 @@ static const struct file_operations uverbs_mmap_fops = {
> .release = ib_uverbs_close,
> .llseek = no_llseek,
> .unlocked_ioctl = ib_uverbs_ioctl,
> - .compat_ioctl = ib_uverbs_ioctl,
> + .compat_ioctl = generic_compat_ioctl_ptrarg,
> };
>
> static struct ib_client uverbs_client = {
For uverbs:
Acked-by: Jason Gunthorpe
It is very strange, this patch did not appear in the RDMA patchworks,
I almost missed it :|
Jason
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 05/17] compat_ioctl: move more drivers to generic_compat_ioctl_ptrarg
On Tue, Sep 18, 2018 at 10:51:08AM -0700, Darren Hart wrote: > On Fri, Sep 14, 2018 at 09:57:48PM +0100, Al Viro wrote: > > On Fri, Sep 14, 2018 at 01:35:06PM -0700, Darren Hart wrote: > > > > > Acked-by: Darren Hart (VMware) > > > > > > As for a longer term solution, would it be possible to init fops in such > > > a way that the compat_ioctl call defaults to generic_compat_ioctl_ptrarg > > > so we don't have to duplicate this boilerplate for every ioctl fops > > > structure? > > > > Bad idea, that... Because several years down the road somebody will add > > an ioctl that takes an unsigned int for argument. Without so much as > > looking > > at your magical mystery macro being used to initialize file_operations. > > Fair, being explicit in the declaration as it is currently may be > preferable then. It would be much cleaner and safer if you could arrange things to add something like this to struct file_operations: long (*ptr_ioctl) (struct file *, unsigned int, void __user *); Where the core code automatically converts the unsigned long to the void __user * as appropriate. Then it just works right always and the compiler will help address Al's concern down the road. Cheers, Jason ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 05/17] compat_ioctl: move more drivers to generic_compat_ioctl_ptrarg
On Mon, Sep 24, 2018 at 10:18:52PM +0200, Arnd Bergmann wrote: > On Tue, Sep 18, 2018 at 7:59 PM Jason Gunthorpe wrote: > > > > On Tue, Sep 18, 2018 at 10:51:08AM -0700, Darren Hart wrote: > > > On Fri, Sep 14, 2018 at 09:57:48PM +0100, Al Viro wrote: > > > > On Fri, Sep 14, 2018 at 01:35:06PM -0700, Darren Hart wrote: > > > > > > > > > Acked-by: Darren Hart (VMware) > > > > > > > > > > As for a longer term solution, would it be possible to init fops in > > > > > such > > > > > a way that the compat_ioctl call defaults to > > > > > generic_compat_ioctl_ptrarg > > > > > so we don't have to duplicate this boilerplate for every ioctl fops > > > > > structure? > > > > > > > > Bad idea, that... Because several years down the road somebody > > > > will add > > > > an ioctl that takes an unsigned int for argument. Without so much as > > > > looking > > > > at your magical mystery macro being used to initialize file_operations. > > > > > > Fair, being explicit in the declaration as it is currently may be > > > preferable then. > > > > It would be much cleaner and safer if you could arrange things to add > > something like this to struct file_operations: > > > > long (*ptr_ioctl) (struct file *, unsigned int, void __user *); > > > > Where the core code automatically converts the unsigned long to the > > void __user * as appropriate. > > > > Then it just works right always and the compiler will help address > > Al's concern down the road. > > I think if we wanted to do this with a new file operation, the best > way would be to do the copy_from_user()/copy_to_user() in the caller > as well. > > We already do this inside of some subsystems, notably drivers/media/, > and it simplifies the implementation of the ioctl handler function > significantly. We obviously cannot do this in general, both because of > traditional drivers that have 16-bit command codes (drivers/tty and others) > and also because of drivers that by accident defined the commands > incorrectly and use the wrong type or the wrong direction in the > definition. That could work well, but the first idea could be done globally and mechanically, while this would require very careful per-driver investigation. Particularly if the core code has worse performance.. ie due to kmalloc calls or something. I think it would make more sense to start by having the core do the case to __user and then add another entry point to have the core do the copy_from_user, and so on. Jason ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [RFC] treewide: cleanup unreachable breaks
On Mon, Oct 19, 2020 at 12:42:15PM -0700, Nick Desaulniers wrote: > On Sat, Oct 17, 2020 at 10:43 PM Greg KH wrote: > > > > On Sat, Oct 17, 2020 at 09:09:28AM -0700, t...@redhat.com wrote: > > > From: Tom Rix > > > > > > This is a upcoming change to clean up a new warning treewide. > > > I am wondering if the change could be one mega patch (see below) or > > > normal patch per file about 100 patches or somewhere half way by > > > collecting > > > early acks. > > > > Please break it up into one-patch-per-subsystem, like normal, and get it > > merged that way. > > > > Sending us a patch, without even a diffstat to review, isn't going to > > get you very far... > > Tom, > If you're able to automate this cleanup, I suggest checking in a > script that can be run on a directory. Then for each subsystem you > can say in your commit "I ran scripts/fix_whatever.py on this subdir." > Then others can help you drive the tree wide cleanup. Then we can > enable -Wunreachable-code-break either by default, or W=2 right now > might be a good idea. I remember using clang-modernize in the past to fix issues very similar to this, if clang machinery can generate the warning, can't something like clang-tidy directly generate the patch? You can send me a patch for drivers/infiniband/* as well Thanks, Jason ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 000/141] Fix fall-through warnings for Clang
On Fri, Nov 20, 2020 at 12:21:39PM -0600, Gustavo A. R. Silva wrote: > IB/hfi1: Fix fall-through warnings for Clang > IB/mlx4: Fix fall-through warnings for Clang > IB/qedr: Fix fall-through warnings for Clang > RDMA/mlx5: Fix fall-through warnings for Clang I picked these four to the rdma tree, thanks Jason ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH V2 3/7] mm/gup: Change GUP fast to use flags rather than a write 'bool'
On Wed, Feb 13, 2019 at 03:04:51PM -0800, ira.we...@intel.com wrote: > From: Ira Weiny > > To facilitate additional options to get_user_pages_fast() change the > singular write parameter to be gup_flags. So now we have: long get_user_pages_unlocked(unsigned long start, unsigned long nr_pages, struct page **pages, unsigned int gup_flags); and int get_user_pages_fast(unsigned long start, int nr_pages, unsigned int gup_flags, struct page **pages) Does this make any sense? At least the arguments should be in the same order, I think. Also this comment: /* * get_user_pages_unlocked() is suitable to replace the form: * * down_read(&mm->mmap_sem); * get_user_pages(tsk, mm, ..., pages, NULL); * up_read(&mm->mmap_sem); * * with: * * get_user_pages_unlocked(tsk, mm, ..., pages); * * It is functionally equivalent to get_user_pages_fast so * get_user_pages_fast should be used instead if specific gup_flags * (e.g. FOLL_FORCE) are not required. */ Needs some attention as the recommendation is now nonsense. Honestly a proper explanation of why two functions exist would be great at this point :) Jason ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v8 2/4] fpga manager: add sysfs interface document
On Sun, Jan 11, 2015 at 10:29:00AM -0600, atull wrote: > the FPGA image. If someone wants there to be only one FPGA image on > the FGPA forever, they will probably not be using this framework; their > FPGA will probably be loaded before Linux boots up. Nonsense, loading the FPGA through Linux is much better, it avoids having to deal with this complexity in the boot loader and means that Linux can be used to locate the huge image in some kind of sensible filesystem, log failures, do any FPGA startup sequence/etc. With hotplug and DT I find this this works very well. The FPGA devices simply are not registered with the kernel until userspace gives the OK (in future a DT overload can handle this) If you keep with the notion that the DT overload specifies the matching FPGA firmware then it makes alot more sense to me to use DT overlays as the API to change the file name - not sysfs. To reload a FPGA, unload the DT overlay (this would have to disconnect all the drivers), de-program the FPGA, the load a new DT overlay, which reprograms and re-binds the new drivers. All those steps have to be done anyhow, you can't just swap the HW while drivers are attached. .. and if there are no drivers then Alan is right, this is the wrong interface for the 'FPGA as a user space co-processor' model. I would also re-iterate my earlier comments: requiring the whole FPGA image to be held in ram makes this useless for any of my applications. Jason ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v8 2/4] fpga manager: add sysfs interface document
On Mon, Jan 12, 2015 at 09:01:34PM +, One Thousand Gnomes wrote: > There are plenty of people today who treat the FPGA as an entirely > dynamic resource. It's not like flashing a controller, its near > immediate. But this is a completely different use case. Remember, there are *megabytes* of internal state in a FPGA, and it isn't really feasible to dump/restore that state. It is one thing to context switch a maths algorithm that is built to be stateless, it is quite another to context switch between, say an ethernet core with an operating Linux Net driver doing DMA and a maths algorithm. A DT overlay approach where the overlay has to be unloaded to 'free' the FPGA makes alot of sense to me for the stateful kernel driver environment, and open/close/etc makes alot of sense for the stateless switchable userspace environment - other than sharing configuration code, is there any overlap between these use cases > Its completely dynamic and it will get more so as we switch from the > painful world of VHDL and friends to high level parallel aware language > compilers for FPGAs and everyone will be knocking up quick FPGA hacks. Only for some users. In my world FPGAs are filled with bus interface logic, ethernet controllers, memory controllers, packet processing engines, etc. This is all incredibly stateful - both in the FPGA itself, and in the Linux side w/ drviers. It certainly will not ever work in the model you are talking about. Even if the digital state could somehow be frozen, dumped and restored, all the FPGA external interface logic has *ANALOG* state that cannot ever be dump/restored. It just isn't feasible for that class of application. Jason ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v8 2/4] fpga manager: add sysfs interface document
On Tue, Jan 13, 2015 at 04:28:47PM +, One Thousand Gnomes wrote:
> There is a lot of code overlap in things like loading the bitstreams,
> there is also some overlap because you want to be able to assign FPGA
> resources. For example if you have four FPGAs and you want to use one for
> OS stuff (say video) you want the other three to be open/close
> accessible, but if you've not got a video driver loaded and are running
> the same board headless you'd like all four to be handed out as normal
> resources.
>
> So IMHO it's no different to say kmalloc. We don't pre-empt kernel memory
> and give it to users but we don't reserve memory for kernel and not use
> it.
I do agree with this, and I think this is where this patch set goes so
wrong.
Just exposing all sorts of controls to userspace and having a way for
the kernel to load/unload a bitstream compleletely misses the point that
the two main ways to use FPGAs *require* some kind of kernel side
driver suppport on the FPGA.
Even if it is just a maths FPGA kernel side has to be involved in some
way to restrict DMAs, MMIO, by the user process. Obviously a FPGA that
uses kernel drivers needs kernel side help to bind those drivers.
Plonking /sys/class/fpga_manager//reset for either case is more
likely than not to completely crash the system.
I would look at this problem from a completely different angle - the
90% case is a single FPGA that is loaded to provide HW that Linux
drivers bind to, these days the majority use DT.
So..
soc
{
zynq_cfg: ps7-dev-cfg@f8007000 { compatible = "xlnx,zynq-devcfg-1.0"; }
fpga@pl {
compatible = "..";
fpga,api = "gpio-api1";
fpga,controller = &zynq_cfg;
gp0_axi: axi@4000 {
gpio3: klina_gpio@80010 {
#gpio-cells = <2>;
compatible = "linux,basic-mmio-gpio";
gpio-controller;
reg-names = "dat", "set", "dirin";
reg = <0x80010 4>,
<0x80014 4>,
<0x80018 4>;
};
}
}
}
Make it so linux can boot, automatically fetch the gpio-api1 bit file
and load it into the chip and then bind the GPIO driver to the FPGA
resource. All without userspace involvment, all potentially done
before calling INIT.
Make it so via sysfs we can reverse this process and reboot the FPGA.
Make it so userspace can't do something to the FPGA without first
unloading the drivers.
Then worry about how to change the FPGA, or providing more user space
control over the process (DT overlays are a natural answer).
Providing a roll-your-own approach via a bunch of sysfs controls
satisfies the 'make the HW work' need without actually providing the
overall architecture someone needs to *make use* of this mechanism.
And no, this isn't just a 'theory', I have 4 shipping products today
that work pretty much exactly like this, including a Zynq
design. Binding kernel drivers to a FPGA once it is loaded is a big
functionality gap, and an annoying problem. Programming the actual
FPGA? That is the *EASIEST* part of this whole thing!
Honestly, I'd much rather see the above use case solved properly
without sysfs support and then go from there to build a replacable
FPGA scheme on the same concepts.
Providing a 'build your own' solution with sysfs controls just seems
to completely miss the mark to me.
> So IMHO it's no different to say kmalloc. We don't pre-empt kernel memory
> and give it to users but we don't reserve memory for kernel and not use
> it.
I think we see a kernel side with more structure, that knows when the
FPGA is in use or not, there is a pretty clear path to later add a
/dev/ scheme for user space use that can properly interlock.
A /dev/ scheme doesn't make much sense to bind kernel drivers...
Jason
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v8 2/4] fpga manager: add sysfs interface document
On Tue, Jan 13, 2015 at 03:37:14PM -0600, atull wrote: > > I do agree with this, and I think this is where this patch set goes so > > wrong. > > > > Just exposing all sorts of controls to userspace and having a way for > > the kernel to load/unload a bitstream compleletely misses the point that > > the two main ways to use FPGAs *require* some kind of kernel side > > driver suppport on the FPGA. > > Hi Jason, > > I may be misunderstanding. I thought you wanted lots of user control > a couple years ago. > > https://lkml.org/lkml/2013/9/18/490 Yes, that is absolutely true, that is certainly where my projects have been historically, and in the context of requiring user space to manage the whole process I think that is completely right. But - I like Alan's point - this should be a higher level thing, the kernel should be able to load the FPGA, and handle binding the drivers so user space doesn't need to be involved, and it makes sense that should be the place to start the API design. > Your feedback carried a lot of weight as I've been developing this patchset > although I didn't do absolutely everything you asked for. Back then you > were asking for lots of user control including being able to reset the FPGA > and being able to know from userspace what configuration steps failed (if > any). Yes, this is all true. Since my FPGAs all seem to require a software mediated startup sequence the dream of having a DT scheme like I described is farther away for me, but I think that is not the 90% case. Almost all vendor IP designs do not require a SW startup and auto-start after programming. 90% of designs can program and then immediately connect drivers. > I'm glad you like DT support for FPGA images + drivers loading since I've > been pushing that since April and have submitted several earlier versions > of this patchset which included it. Yes, I do, thank you for working on this! > I think your arguements against the current patchset are a bit over > the top here. Perhaps, and I apologize. But as you know I've never liked the sysfs interface, and I think Pavel is right. catting a file name just so you can call request_firmware on that file in the kernel is a bad design. The request_firmware interface should be for the DT overlay path, and other schemes shouldn't use it. The name should come from the DT and no place else. > > Make it so linux can boot, automatically fetch the gpio-api1 bit file > > and load it into the chip and then bind the GPIO driver to the FPGA > > resource. All without userspace involvment, all potentially done > > before calling INIT. > > Where would the fpga image be fetch from? What is the mechanism for > doing that? Do we need to reqire everybody to do that or can > firmware be one of the available mechanisms? I see there are three options: 1) The bootloader programs the FPGA and passes a DT to the kernel that completely describes the hard and soft logic, the kernel doesn't need FPGA bitfile management code or other special support. Obviously things like reprogram on resume have to be done by the bootloader, there is no handoff here where the kernel takes over control of the bitfile programming. I think that is fine, #2/#3 are better choices for most people anyhow. 2) The bootloader starts the kernel and passes a DT that describes the hard logic and soft logic using a scheme like I outlined. The kernel is responsible to request_firmware the bitfile and start the FPGA and connect the drivers. DT purists will rightly point out that this isn't great, but it is a simple and pragmatic solution. This probably falls out automatically if #3 is implemented.. 3) The bootloader starts the kernel with a DT that only describes the hard logic. Userspace must load a DT overlay early on. Otherwise proceeds like #2. In both 2 and 3 the FPGA can be reprogrammed on resume by re-doing request_firmware. And for folks that need more control, expose all the knobs explicitly to user space: 4) There is a /dev/ node for the fpga that allows - ioctl to program via mmap'd file (no request_firmware) The ioctl has an option for the kernel to hang on to the mmap for the repogram on resume case. - ioctl to get status/error reporting/etc - ioctl to load/bind a DT overlay to the FPGA instance This provides a gap where userspace can boot and configure the FPGA internals before binding the kernel drivers. (end result is the same as #3 case, but no request_firmware) - (future) ioctl to load a swappable FPGA (what Alan is talking about) The key thing is that we have a FPGA object that can be associated with some DT element, and the kernel can then keep track if the FPGA is is in use or not. Exactly like you said in your reply. Otherwise, for the /dev/ case the FPGA becomes unuse once the FD is closed if it wasn't attached to an overlay. This is at least the start of
Re: [PATCH v8 2/4] fpga manager: add sysfs interface document
On Wed, Jan 14, 2015 at 04:06:17PM +, One Thousand Gnomes wrote: > and I think you effectively have the user usage covered here for such > things. It much like GPIO pins - we can describe them but we can also > declare they are not visible to the user. A missing element in mainline is a kind of VFIO scheme to let user space access the FPGA registers designated for user space use. We have been using these patches since 2.6.16 to allow user space to access the FPGA register resources via a PCI like /sys/../resource0 mmapable: https://github.com/jgunthorpe/linux/commit/59d5d13ddeffa8980ccc6248ebb5f1678ccb23f4 https://github.com/jgunthorpe/linux/commit/7c29c4344627be8a3906d64d32db533bc131df86 https://github.com/jgunthorpe/linux/commit/e41bb4a197368a9d505d66b627aee82f2d2b8895 We deliberately split up the FPGA registers and the assign user space permissions to the resource0 files in a way that makes sense for our app. Typically there are 10-20 FPGA register regions. User space does not access register regions that control DMA. > The swappable case mostly comes out of the /dev node. Once you have the > dev node it becomes a detail of the OS not the FPGA driver as to who may > open it, and how it is handed about. It might be an FPU manager daemon it > might not. Right, but the thing that scares me about the swappable case is the kernel side support.. The FPGA has to connect to the CPU in some way, it must have some address assigned, etc. Swapping needs to take care of all those details in some way. A fixed bus interface block and dynamic reconfiguration for the remainder is probably the way to manage that. But, that implies that even a family of swappable FPGAs will have a DT overlay associated with it. Ideally, I could see wanting to have a file format that combines the overlay and FPGA bitfile. A loader tool would use the /dev/ interface to setup both elements. That would be much more robust than the current scheme I see (eg Xilinx) using where the bitfile and DT bit live in completely different places and have to be perfectly matched. Jason ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v8 2/4] fpga manager: add sysfs interface document
On Thu, Jan 15, 2015 at 10:34:39AM -0600, atull wrote: > This is great! The way I had it working was using Pantelis' devicetree > configfs interface. I figured you were very close to this already in your overlay work.. > The DT fragment described the FPGA logic and included a filename > for firmware to load. In another branch of this thread, I see discussion > starting on what the overlay should look like and whether it could somehow > contain the DT itself. It is a novel idea, my concern would be that embedding the FPGA in the DT makes it permanent unswappable kernel memory. Not having the kernel hold the FPGA is best for many uses. Having the kernel hold the FPGA as a swppable file handle/mmap of some sort is next best (obviously the fs must be operating during resume) Unswappable kernel memory is the worst choice > Long ago this driver started out with a /dev interface. It didn't have > an ioctl yet at that point, but programming the fpga was by opening > the devnode and writing to it. Greg KH preferred sysfs or configfs > over adding another ioctl: I think to justify the ioctls you need a reason to have the context of a FD. sysfs is stateless, so if my process dies the kernel doesn't know. But now that we are talking about adding locking and ownership concepts a FD is the natural anchor for that in user space. Ie, if I open the dev node, program a FPGA and then crash the kernel doesn't attach drivers, and immediately de-programs the chip. Userspace has to make it all the way through to the DT bind before the FPGA lifetime would exceed the FD. > https://lkml.org/lkml/2013/10/8/677 I think Greg's reply makes sense in the context of the question being asked. Thinking of the FPGA as lockable ref counted kind of resource changes the question somewhat. Identifying the ioctls needed would probably clarify things. My rough start would be - Get status: programed, not programmed, error? Bitfile type? (eg Xilinx has nearly every permutation of bit/byte ordering) - Start Program with with some kind of context (ie this a new bit file, partial reconfiguration basis X, partial reconfiguration overlay on X) - for (;;) write() to do programming - Get Error to return detailed failure information (CRC error, auth error, etc) - Hand over to a DT overlay (how does this work?) Lock transfers from FD to kernel - .. something something VFIO .. ? Where start program is refused if the FPGA is already locked, and locks it Where start program -> close() returns the FPGA back to reset and unlocks Where start program -> hand over -> close() keeps the FPGA loaded with kernel drivers attached and fpga locked (remove the overlay to de-program and unlock) Not sure exactly how to tie together DT overlays with the FPGA state, but that seems the natural combination.. Not sure about partial reconfiguration - clearly the kernel needs to know and check that the bitfiles are of the correct family, I wonder if the approach should be to program a basis on the FPGA which then creates a new FPGA device in the system that can accept the partial reconfiguration - this way the locking makes sense, the basis is locked by the kernel and devices and the overlay remains lockable/swappable/whatever by a dedicated /dev/ node ?? Just thinking aloud, I've never had a use case for partial reconfiguration. Jason ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v8 2/4] fpga manager: add sysfs interface document
On Thu, Jan 15, 2015 at 08:45:02PM +, One Thousand Gnomes wrote:
> > - Hand over to a DT overlay (how does this work?) Lock transfers
> > from FD to kernel
>
> That bit isn't stateful so I would actually have expected something in
> the kernel ABI along the lines of
>
>request_fpga(blah)
>
> which does
>
> if in use by user
> EBUSY
> lock it (all user opens will fail)
>
> and
>
> release_fpga(blah)
I am imagining two ways to start a kernel FPGA, the in-kernel method
triggered by a DT node:
fpga = request_and_lock_fpga(of_get_fpga_controller(dev->of_node))
fw = request_firmat(of_get_fpga_firmware(dev->of_node));
fpga_program_fw(fpga, fw);
for_each_child_of_node(dev->of_node, child)
.. of_platform_bus_probe(child ... ) ..
.. somehow fpga and its lock transfer to dev->of_node ..
The problem with this is it assumes the FPGA is ready to go
immediately after fpga_program_fw. There are a few platforms that can
manage this, but many others require at least some kind of startup
sequence - eg wait for clocking PLLs to lock, do low level setup, etc.
For very common cases (like Zynq can have a pretty common setup scheme
for the PL side) the DT binding can guide the kernel to run the right
code, and the code can live in the kernel because it is simple and
broadly useful.
For wild cases I'd like to just punt the setup code to user space. It
is safer and simpler to run that complexity in a user process than in
the kernel.
Maybe there is a better way to handle this, but I have been under the
impression that these days it is frowned on for the kernel to wait for
userspace?
So my idea is to use the user space method to also load a 'kernel'
fpga, the process follows the kernel flow:
fd = open("/dev/fpga0"); // request_and_lock_fpga
ioctl(fd,START_PROGRAMMING); // fpga_program_fw
write(fd,fw,fw_size);
// Arbitary complexity here
userspace_setup_fpga();
icotl(fd,BIND_DT_OVERLAY, .. ?? ..) // for_each_child_of_node
This is what the state is about, if the setup fails I expect the
kernel to go and unlock and deprogram the FPGA if fd is closed. Only a
success on BIND_DT_OVERLAY will make the FPGA permanent beyond closing
fd.
Pantelis: I think this explains why a fd and ioctls. configfs, sysfs,
etc lack the FD context to do the cleanup.
Essentially, I think a running FPGA should always be attached to some
context that is the user - either the user is in-kernel (a DT of_node)
or the user is in userspace (the fd).
The invarient is - if there is no user then the kernel automatically
makes the FPGA deprogrammed.
Having a device that is potentially attached to the CPU bus running
'rouge' is dangerous. We can't realistically deprogram it safely if we
don't know what it is doing. Eg deprogramming in the middle of a DMA
operation between the CPU and FPGA will often crash the entire system.
The only way to provide reasonable safe guards is to have a clear
chain of ownership. When the kernel takes ownership of the FPGA it
also takes ownership of any cleanup required prior to deprogram.
Jason
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: use exact allocation for dma coherent memory
On Mon, Jun 17, 2019 at 10:33:42AM +0200, Christoph Hellwig wrote:
> > drivers/infiniband/hw/cxgb4/qp.c
> >129 static int alloc_host_sq(struct c4iw_rdev *rdev, struct t4_sq *sq)
> >130 {
> >131 sq->queue = dma_alloc_coherent(&(rdev->lldi.pdev->dev),
> > sq->memsize,
> >132 &(sq->dma_addr), GFP_KERNEL);
> >133 if (!sq->queue)
> >134 return -ENOMEM;
> >135 sq->phys_addr = virt_to_phys(sq->queue);
> >136 dma_unmap_addr_set(sq, mapping, sq->dma_addr);
> >137 return 0;
> >138 }
> >
> > Is this a bug?
>
> Yes. This will blow up badly on many platforms, as sq->queue
> might be vmapped, ioremapped, come from a pool without page backing.
Gah, this addr gets fed into io_remap_pfn_range/remap_pfn_range too..
Potnuri, you should fix this..
You probably need to use dma_mmap_from_dev_coherent() in the mmap ?
Jason
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v4 2/3][v4.9.y] coredump: fix race condition between mmget_not_zero()/get_task_mm() and core dumping
On Tue, Jun 25, 2019 at 02:33:04AM +0530, Ajay Kaher wrote:
> This patch is the extension of following upstream commit to fix
> the race condition between get_task_mm() and core dumping
> for IB->mlx4 and IB->mlx5 drivers:
>
> commit 04f5866e41fb ("coredump: fix race condition between
> mmget_not_zero()/get_task_mm() and core dumping")'
>
> Thanks to Jason for pointing this.
>
> Signed-off-by: Ajay Kaher
> ---
> drivers/infiniband/hw/mlx4/main.c | 4 +++-
> drivers/infiniband/hw/mlx5/main.c | 3 +++
> 2 files changed, 6 insertions(+), 1 deletion(-)
Looks OK
Reviewed-by: Jason Gunthorpe
Thanks
Jason
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] IB: Revert "remove redundant INFINIBAND kconfig dependencies"
On Fri, May 25, 2018 at 05:32:52PM -0700, Greg Thelen wrote:
> On Fri, May 25, 2018 at 2:32 PM Arnd Bergmann wrote:
>
> > Several subsystems depend on INFINIBAND_ADDR_TRANS, which in turn depends
> > on INFINIBAND. However, when with CONFIG_INIFIBAND=m, this leads to a
> > link error when another driver using it is built-in. The
> > INFINIBAND_ADDR_TRANS dependency is insufficient here as this is
> > a 'bool' symbol that does not force anything to be a module in turn.
>
> > fs/cifs/smbdirect.o: In function `smbd_disconnect_rdma_work':
> > smbdirect.c:(.text+0x1e4): undefined reference to `rdma_disconnect'
> > net/9p/trans_rdma.o: In function `rdma_request':
> > trans_rdma.c:(.text+0x7bc): undefined reference to `rdma_disconnect'
> > net/9p/trans_rdma.o: In function `rdma_destroy_trans':
> > trans_rdma.c:(.text+0x830): undefined reference to `ib_destroy_qp'
> > trans_rdma.c:(.text+0x858): undefined reference to `ib_dealloc_pd'
>
> > Fixes: 9533b292a7ac ("IB: remove redundant INFINIBAND kconfig
> dependencies")
> > Signed-off-by: Arnd Bergmann
>
> Acked-by: Greg Thelen
>
> Sorry for the 9533b292a7ac problem.
> At this point the in release cycle, I think Arnd's revert is best.
>
> If there is interest, I've put a little thought into an alternative fix:
> making INFINIBAND_ADDR_TRANS tristate. But it's nontrivial.
> So I prefer this simple revert for now.
Is that a normal thing to do?
> Doug: do you need anything from me on this?
I can take the revert..
Jason
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] IB: Revert "remove redundant INFINIBAND kconfig dependencies"
On Fri, May 25, 2018 at 11:29:59PM +0200, Arnd Bergmann wrote:
> Several subsystems depend on INFINIBAND_ADDR_TRANS, which in turn depends
> on INFINIBAND. However, when with CONFIG_INIFIBAND=m, this leads to a
> link error when another driver using it is built-in. The
> INFINIBAND_ADDR_TRANS dependency is insufficient here as this is
> a 'bool' symbol that does not force anything to be a module in turn.
>
> fs/cifs/smbdirect.o: In function `smbd_disconnect_rdma_work':
> smbdirect.c:(.text+0x1e4): undefined reference to `rdma_disconnect'
> net/9p/trans_rdma.o: In function `rdma_request':
> trans_rdma.c:(.text+0x7bc): undefined reference to `rdma_disconnect'
> net/9p/trans_rdma.o: In function `rdma_destroy_trans':
> trans_rdma.c:(.text+0x830): undefined reference to `ib_destroy_qp'
> trans_rdma.c:(.text+0x858): undefined reference to `ib_dealloc_pd'
>
> Fixes: 9533b292a7ac ("IB: remove redundant INFINIBAND kconfig dependencies")
> Signed-off-by: Arnd Bergmann
> Acked-by: Greg Thelen
> ---
> The patch that introduced the problem has been queued in the
> rdma-fixes/for-rc tree. Please revert the patch before sending
> the branch to Linus.
> ---
> drivers/infiniband/ulp/srpt/Kconfig | 2 +-
> drivers/nvme/host/Kconfig | 2 +-
> drivers/nvme/target/Kconfig | 2 +-
> drivers/staging/lustre/lnet/Kconfig | 2 +-
> fs/cifs/Kconfig | 2 +-
> net/9p/Kconfig | 2 +-
> net/rds/Kconfig | 2 +-
> net/sunrpc/Kconfig | 2 +-
> 8 files changed, 8 insertions(+), 8 deletions(-)
Applied to for-rc, thanks
Jason
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 01/21] scatterlist: Introduce sg_map helper functions
On Thu, Apr 27, 2017 at 08:53:38AM +0200, Christoph Hellwig wrote: > > The main difficulty we > > have now is that neither of those functions are expected to fail and we > > need them to be able to in cases where the page doesn't map to system > > RAM. This patch series is trying to address it for users of scatterlist. > > I'm certainly open to other suggestions. > > I think you'll need to follow the existing kmap semantics and never > fail the iomem version either. Otherwise you'll have a special case > that's almost never used that has a different error path. How about first switching as many call sites as possible to use sg_copy_X_buffer instead of kmap? A random audit of Logan's series suggests this is actually a fairly common thing. eg drivers/mmc/host/sdhci.c is only doing this: buffer = sdhci_kmap_atomic(sg, &flags); memcpy(buffer, align, size); sdhci_kunmap_atomic(buffer, &flags); drivers/scsi/mvsas/mv_sas.c is this: + to = sg_map(sg_resp, 0, SG_KMAP_ATOMIC); + memcpy(to, + slot->response + sizeof(struct mvs_err_info), + sg_dma_len(sg_resp)); + sg_unmap(sg_resp, to, 0, SG_KMAP_ATOMIC); etc. Lots of other places seem similar, if not sometimes a little bit more convoluted.. Switching all the trivial cases to use copy might bring more clarity as to what is actually required for the remaining few users? If there are only a few then it may no longer matter if the API is not idyllic. Jason ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 15/21] xen-blkfront: Make use of the new sg_map helper function
On Thu, Apr 27, 2017 at 02:19:24PM -0600, Logan Gunthorpe wrote:
>
>
> On 26/04/17 01:37 AM, Roger Pau Monné wrote:
> > On Tue, Apr 25, 2017 at 12:21:02PM -0600, Logan Gunthorpe wrote:
> >> Straightforward conversion to the new helper, except due to the lack
> >> of error path, we have to use SG_MAP_MUST_NOT_FAIL which may BUG_ON in
> >> certain cases in the future.
> >>
> >> Signed-off-by: Logan Gunthorpe
> >> Cc: Boris Ostrovsky
> >> Cc: Juergen Gross
> >> Cc: Konrad Rzeszutek Wilk
> >> Cc: "Roger Pau Monné"
> >> drivers/block/xen-blkfront.c | 20 +++-
> >> 1 file changed, 11 insertions(+), 9 deletions(-)
> >>
> >> diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c
> >> index 3945963..ed62175 100644
> >> +++ b/drivers/block/xen-blkfront.c
> >> @@ -816,8 +816,9 @@ static int blkif_queue_rw_req(struct request *req,
> >> struct blkfront_ring_info *ri
> >>BUG_ON(sg->offset + sg->length > PAGE_SIZE);
> >>
> >>if (setup.need_copy) {
> >> - setup.bvec_off = sg->offset;
> >> - setup.bvec_data = kmap_atomic(sg_page(sg));
> >> + setup.bvec_off = 0;
> >> + setup.bvec_data = sg_map(sg, 0, SG_KMAP_ATOMIC |
> >> + SG_MAP_MUST_NOT_FAIL);
> >
> > I assume that sg_map already adds sg->offset to the address?
>
> Correct.
>
> > Also wondering whether we can get rid of bvec_off and just increment
> > bvec_data,
> > adding Julien who IIRC added this code.
>
> bvec_off is used to keep track of the offset within the current mapping
> so it's not a great idea given that you'd want to kunmap_atomic the
> original address and not something with an offset. It would be nice if
> this could be converted to use the sg_miter interface but that's a much
> more invasive change that would require someone who knows this code and
> can properly test it. I'd be very grateful if someone actually took that on.
blkfront is one of the drivers I looked at, and it appears to only be
memcpying with the bvec_data pointer, so I wonder why it does not use
sg_copy_X_buffer instead..
Jason
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 15/21] xen-blkfront: Make use of the new sg_map helper function
On Thu, Apr 27, 2017 at 03:53:37PM -0600, Logan Gunthorpe wrote: > On 27/04/17 02:53 PM, Jason Gunthorpe wrote: > > blkfront is one of the drivers I looked at, and it appears to only be > > memcpying with the bvec_data pointer, so I wonder why it does not use > > sg_copy_X_buffer instead.. > > But you'd potentially end up calling sg_copy_to_buffer multiple times > per page within the sg (given that gnttab_foreach_grant_in_range might > call blkif_copy_from_grant/blkif_setup_rw_req_grant multiple times). > Even calling sg_copy_to_buffer once per page seems rather inefficient as > it uses sg_miter internally. Well, that is in the current form, with more users it would make sense to optimize for the single page case, eg by providing the existing call, providing a faster single-page-only variant of the copy, perhaps even one that is inlined. > Switching the for_each_sg to sg_miter is probably the nicer solution as > it takes care of the mapping and the offset/length accounting for you > and will have similar performance. sg_miter will still fail when the sg contains __iomem, however I would expect that the sg_copy will work with iomem, by using the __iomem memcpy variant. So, sg_copy should always be preferred in this new world with mixed __iomem since it is the only primitive that can transparently handle it. Jason ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 15/21] xen-blkfront: Make use of the new sg_map helper function
On Thu, Apr 27, 2017 at 05:03:45PM -0600, Logan Gunthorpe wrote: > > > On 27/04/17 04:11 PM, Jason Gunthorpe wrote: > > On Thu, Apr 27, 2017 at 03:53:37PM -0600, Logan Gunthorpe wrote: > > Well, that is in the current form, with more users it would make sense > > to optimize for the single page case, eg by providing the existing > > call, providing a faster single-page-only variant of the copy, perhaps > > even one that is inlined. > > Ok, does it make sense then to have an sg_copy_page_to_buffer (or some > such... I'm having trouble thinking of a sane name that isn't too long). > That just does k(un)map_atomic and memcpy? I could try that if it makes > sense to people. It seems the most robust: test for iomem, and jump to a slow path copy, otherwise inline the kmap and memcpy Every place doing memcpy from sgl will need that pattern to be correct. > > sg_miter will still fail when the sg contains __iomem, however I would > > expect that the sg_copy will work with iomem, by using the __iomem > > memcpy variant. > > Yes, that's true. Any sg_miters that ever see iomem will need to be > converted to support it. This isn't much different than the other > kmap(sg_page()) users I was converting that will also fail if they see > iomem. Though, I suspect an sg_miter user would be easier to convert to > iomem than a random kmap user. How? sg_miter seems like the next nightmare down this path, what is sg_miter_next supposed to do when something hits an iomem sgl? miter.addr is supposed to be a kernel pointer that must not be __iomem.. Jason ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 04/17] staging/rdma/hfi1: Fix qp.h comments
On Tue, Dec 01, 2015 at 03:38:13PM -0500, Jubin John wrote: > From: Kaike Wan > > This patch fixes a few incorrect header file comments in qp.h FWIW, within drivers/infiniband we've been moving these comments to the implementation, not the function prototype. Jason ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v9 0/7] FPGA Manager Framework and Simple FPGA Bus
On Fri, Jul 17, 2015 at 10:51:10AM -0500, at...@opensource.altera.com wrote: > From: Alan Tull > > This patchset adds two chunks plus documentation: > * fpga manager core: exports ABI functions that write an image to a FPGA > * DT Overlay support: simple-fpga-bus to handle FPGA from a DT overlay I didn't read super closely, but overall it makes sense to me.. Providing an in-kernel API will let someone else figure out how to expose that to user space. The DT based scheme seems pretty nice. Can you use this without DT overlay? Ie if I provide the FGPA description as part of my boot time DT will it just work? Jason ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v9 3/7] staging: add bindings document for simple fpga bus
On Fri, Jul 17, 2015 at 09:49:13PM +0200, Steffen Trumtrar wrote:
> What you are describing here is a virtual bus, that is not existing on
> at least the Socfpga, right?
I wouldn't get too hung up on bus or not bus, the HW these days
doesn't even use busses.
For AXI systems, which is all the ARM designs, the the bridge between
the CPU and FPGA is always a physical AXI link hanging off one of the
CPU block's internal AXI switches.
I choose to model these ports in DT explicitly, because they represent
swappable attachment points, and often the switch ports have
programmable registers related to that port.
FPGA logic is always hanging off one of these physical ports.
Usually there are multiple independent links between the CPU and the
FPGA (ie Xilinx Virtex 4 has at least 4 CPU to FPGA bus links, Zynq
has something like 7)
Ie on Zynq, I do things like this:
/ {
/* This is a simplification of the 5 AXI interconnect switches
hardwired inside the CPU block */
ps7_axi_interconnect_0: amba@0 {
// This is the register block that programs the FPGA
ps7-dev-cfg@f8007000 {
clock-names = "ref_clk", "fclk0", "fclk1", "fclk2",
"fclk3";
clocks = <&clkc 12>, <&clkc 15>, <&clkc 16>, <&clkc
17>, <&clkc 18>;
compatible = "xlnx,zynq-devcfg-1.0";
interrupt-parent = <&ps7_scugic_0>;
interrupts = <0 8 4>;
reg = <0xf8007000 0x100>;
};
// This node bundles the entire FPGA side
programmable: fpga@pl {
// This is a physical port on one of the CPU core's AXI swithces
gp0_axi: axi@4000 {
k_gpio@8 {
k_sysmon@81000 {
gpio3: klina_gpio@80010 {
i2c_qsfp1 {
}
// This is another physical port on one of the CPU core's AXI
swithces
gp1_axi: axi@8000 {
}
// The FPGA bitstream also hooks up to a 3rd AXI port for
initiating DMA
// hp0_axi ...
}
}
Zynq has 5 internal AXI switches, but the typical Linux DT models them
all as single DT 'bus'
I've brought the FPGA exposed AXI ports out under the programmable
node, purely for convenience of coding the dynamic load/unload of all
the FPGA elements. We do full hot swap of the FPGA during system
operation.
The physical ports could be located someplace within the amba@0, but
since amba@0 is basically already a lie, I don't really mind this
slight divergence as it makes logical sense and life easier.
Usually what my FPGA designs do is take the AXI port(s) and then fan
out to something else, either more AXI ports through yet another AXI
switch, or convert to a low speed multi-drop bus and fan that out to a
number of devices. I don't usually model this, because it provide no
value to Linux to know these details.
Ultimately the gp0_axi/gp1_axi have a number of peripheral childern,
each with their own drivers, interrupts, etc.
In this particular design gp1_axi bridges to a FPGA soft-core which
provides a physical bus to another FPGA, which is represented as more
nested nodes, and another FPGA programmable node.
DMA for the brdige side flows through the hp0_axi port. (it consumes 3
AXI ports IIRC)
I think the simplified DT modeling is a reasonable compromise.
> b) shouldn't the h2f/lwh2f/f2h bridges be the "simple-bus devices" instead
> of
> just reset-controllers ? What about e.g. the bus width of the
> bridges?
On Zynq there are a variety of resets and clocks going from the CPU
block to the FPGA, they all need to be configured properly to run
correctly, and they need a home. The control registers for these are
located someplace under amba@0, but I'd be happy to see the actual
values to program contained under the programming node. That fits much
better with the overlay scheme.
There is also some AXI port-specific registers that may need tuning as
well, but they can naturally fit under the axi port nodes..
> It can change depending on the bitstream. When I have an IP core that
> does
> DMA I might want my driver to be able to configure the bus width
> accordingly.
> There are other settings in the bridges that I can not set when they are
> just
> reset controllers.
bridge@0xff20 should be the bus port and it can have configuration...
AXI negotiates things like link width dynamically, and for AXI, DMA
doesn't flow through the same AXI port as the control registers
anyhow.
DT is a very poor fit for a modeling a modern AXI interconnect system,
so there is often some irrelevant lossage..
Jason
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v9 0/7] FPGA Manager Framework and Simple FPGA Bus
On Wed, Jul 22, 2015 at 03:32:32PM -0500, atull wrote: > I looked some more; I don't see a simple way of deferring probing until > after the filesystem is loaded (so that the image file would be > available), late_initcall is still not late enough. That seems weird, are you saying you can't use request firmware at all from a compiled in driver? I don't know much about that flow, sorry. Having that work would mean the system can have a reasonable in-tree use case without requiring the out of tree dt overlay stuff. Jason ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v9 6/7] staging: add simple-fpga-bus
On Thu, Jul 23, 2015 at 02:55:52PM -0700, Moritz Fischer wrote: > Hi Alan, > > I saw that your socfpga driver doesn't support the partial reconfig > use case (not a big deal). > What I currently do for Zynq is if I'm doing a non-partial reconfig is > that I disable input > level shifters and assert *all* resets while reprogramming in my FPGA > manager .write_init() and .write_complete(). I do this as well, but it is a bit more complex.. FPGA specific code has to run around and ensure all DMA is shut off, then we need to make sure no CPU issued AXI transactions can happen, then we can tear down the FPGA side. If the FPGA is torn down while an AXI op is inprogress things go sideways, we have to work to prevent that :) This happens almost for free, I use DT and the device model to disconnect the drivers. The drivers are careful to synchronously fence off in-progress DMA. Then drop the DT nodes associated with the FPGA, finally the actual FPGA cells can be reset. > In a partial reconfiguration situation, would I have separate > simple-fpga buses for each of the parts that I swap out, each with > it's own reset and bitfile attached? I'd think of partial reconfiguration as another nested FPGA. The resets and so forth could be attached to soft controllers in the unswappable part of the FPGA. DT nodes have to surround it in some way... Jason ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v8 2/4] fpga manager: add sysfs interface document
On Wed, Jan 21, 2015 at 06:33:12PM +0200, Pantelis Antoniou wrote: > Hi Alan, > > > On Jan 21, 2015, at 18:01 , One Thousand Gnomes > > wrote: > > > > On Thu, 15 Jan 2015 22:54:46 +0200 > > Pantelis Antoniou wrote: > > > >> Hi Alan, > >> > >>> On Jan 15, 2015, at 22:45 , One Thousand Gnomes > >>> wrote: > >>> > >>> On Thu, 15 Jan 2015 11:47:26 -0700 > >>> Jason Gunthorpe wrote: > >>>> It is a novel idea, my concern would be that embedding the FPGA in the > >>>> DT makes it permanent unswappable kernel memory. > >>>> Not having the kernel hold the FPGA is best for many uses. > >>> > >>> If you have a filesysytem before the FPGA is set up then it belongs in > >>> the file system. As you presumably loaded the kernel from somewhere there > >>> ought to be a file system (even an initrd). > >>> > >> > >> Request firmware does not imply keeping it around. You can always > >> re-request > >> when reloading (although there’s a nasty big of caching that needs to be > >> resolved with the firmware loader). > > > > Which comes down to the same thing. Unless you can prove that there is a > > path to recover the firmware file that does not have any dependancies > > upon the firmware executing (and those can be subtle and horrid at times) > > you need to keep it around for suspend/resume at least and potentially > > any unexpected error/reset. > > > > In that case the only safe place to put it is in the kernel image itself, > which > is something the firmware loader already supports. My point is that the current firmware layer is overly cautious and FPGAs are very big. My current project on small Xilinx device has a 10MB programming file. The biggest Xilinx device today has a max bitfile size around 122MB. So keeping that much memory pinned in the kernel when I can prove it is uncessary for my system (either because there is no suspend/resume possibility, or because I know the CPU can always access the filesytem) is very undesirable. Other systems might have to take the ram hit. Since it seems like the kernel has no way to know, we probably have have a way to tell it not to cache the bitfile. Jason ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v8 2/4] fpga manager: add sysfs interface document
On Sun, Feb 15, 2015 at 11:40:06PM +0100, Pavel Machek wrote: > > So keeping that much memory pinned in the kernel when I can prove it > > is uncessary for my system (either because there is no suspend/resume > > possibility, or because I know the CPU can always access the > > filesytem) is very undesirable. > > Well, your current device aalso has 1GB RAM, no? No, certainly not. 256MB - the board only has space for two x16 DDR3 chips, and at design time 1GBit was about the biggest that could be reasonably obtained. It is also using a Zynq chip for management and the next firmware spin is likely to throw away 50% of that space to enable Xilinx's narrow ECC scheme on the ram. The Linux environment requires only about 40M of ram for runtime, as it was designed for systems with only 64M of ram, so even the 128M is overkill. Jason ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
