| Message ID | 161645534083.2002542.11583610276394664799.stgit@djiang5-desk3.ch.intel.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | idxd 'struct device' lifetime handling fixes | expand |
Hi Dave, On 22-03-21, 16:31, Dave Jiang wrote: > v7: > - Fix up the 'struct device' setup in char device code (Jason) > - Split out the char dev fixes (Jason) > - Split out the DMA dev fixes (Dan) > - Split out the each of the conf_dev fixes > - Split out removal of the pcim_* calls > - Split out removal of the devm_* calls > - Split out the fixes for interrupt config calls > - Reviewed by Dan. > > v6: > - Fix char dev initialization issues (Jason) > - Fix other 'struct device' initialization issues. > > v5: > - Rebased against 5.12-rc dmaengine/fixes > v4: > - fix up the life time of cdev creation/destruction (Jason) > - Tested with KASAN and other memory allocation leak detections. (Jason) > > v3: > - Remove devm_* for irq request and cleanup related bits (Jason) > v2: > - Remove all devm_* alloc for idxd_device (Jason) > - Add kref dep for dma_dev (Jason) > > Vinod, > The series fixes the various 'struct device' lifetime handling in the > idxd driver. The devm managed lifetime is incompatible with 'struct device' > objects that resides in the idxd context. Tested with > CONFIG_DEBUG_KOBJECT_RELEASE and address all issues from that. Sorry for not looking into this earlier.. But I would like to check on the direction idxd is taking. Somehow I get the feel the dmaengine is not the right place for this. Considering that now we have auxdev merged in, should the idxd be spilt into smaller function and no dmaengine parts moved away from dmaengine... I think we do lack a subsystem for all things accelerator in kernel atm... Dan what do you think about splitting idxd? Thanks
On Tue, Mar 23, 2021 at 05:15:30PM +0530, Vinod Koul wrote: > Hi Dave, > > On 22-03-21, 16:31, Dave Jiang wrote: > > v7: > > - Fix up the 'struct device' setup in char device code (Jason) > > - Split out the char dev fixes (Jason) > > - Split out the DMA dev fixes (Dan) > > - Split out the each of the conf_dev fixes > > - Split out removal of the pcim_* calls > > - Split out removal of the devm_* calls > > - Split out the fixes for interrupt config calls > > - Reviewed by Dan. > > > > v6: > > - Fix char dev initialization issues (Jason) > > - Fix other 'struct device' initialization issues. > > > > v5: > > - Rebased against 5.12-rc dmaengine/fixes > > v4: > > - fix up the life time of cdev creation/destruction (Jason) > > - Tested with KASAN and other memory allocation leak detections. (Jason) > > > > v3: > > - Remove devm_* for irq request and cleanup related bits (Jason) > > v2: > > - Remove all devm_* alloc for idxd_device (Jason) > > - Add kref dep for dma_dev (Jason) > > > > Vinod, > > The series fixes the various 'struct device' lifetime handling in the > > idxd driver. The devm managed lifetime is incompatible with 'struct device' > > objects that resides in the idxd context. Tested with > > CONFIG_DEBUG_KOBJECT_RELEASE and address all issues from that. > > Sorry for not looking into this earlier.. But I would like to check on > the direction idxd is taking. Somehow I get the feel the dmaengine is > not the right place for this. Considering that now we have auxdev merged > in, should the idxd be spilt into smaller function and no dmaengine > parts moved away from dmaengine... I think we do lack a subsystem for > all things accelerator in kernel atm... auxdev shouldn't be over-used IMHO. If the main purpose of the driver is dma engine then it is OK if the "core" part lives there too. Jason
On 3/23/2021 4:56 AM, Jason Gunthorpe wrote: > On Tue, Mar 23, 2021 at 05:15:30PM +0530, Vinod Koul wrote: >> Hi Dave, >> >> On 22-03-21, 16:31, Dave Jiang wrote: >>> v7: >>> - Fix up the 'struct device' setup in char device code (Jason) >>> - Split out the char dev fixes (Jason) >>> - Split out the DMA dev fixes (Dan) >>> - Split out the each of the conf_dev fixes >>> - Split out removal of the pcim_* calls >>> - Split out removal of the devm_* calls >>> - Split out the fixes for interrupt config calls >>> - Reviewed by Dan. >>> >>> v6: >>> - Fix char dev initialization issues (Jason) >>> - Fix other 'struct device' initialization issues. >>> >>> v5: >>> - Rebased against 5.12-rc dmaengine/fixes >>> v4: >>> - fix up the life time of cdev creation/destruction (Jason) >>> - Tested with KASAN and other memory allocation leak detections. (Jason) >>> >>> v3: >>> - Remove devm_* for irq request and cleanup related bits (Jason) >>> v2: >>> - Remove all devm_* alloc for idxd_device (Jason) >>> - Add kref dep for dma_dev (Jason) >>> >>> Vinod, >>> The series fixes the various 'struct device' lifetime handling in the >>> idxd driver. The devm managed lifetime is incompatible with 'struct device' >>> objects that resides in the idxd context. Tested with >>> CONFIG_DEBUG_KOBJECT_RELEASE and address all issues from that. >> Sorry for not looking into this earlier.. But I would like to check on >> the direction idxd is taking. Somehow I get the feel the dmaengine is >> not the right place for this. Considering that now we have auxdev merged >> in, should the idxd be spilt into smaller function and no dmaengine >> parts moved away from dmaengine... I think we do lack a subsystem for >> all things accelerator in kernel atm... > auxdev shouldn't be over-used IMHO. > > If the main purpose of the driver is dma engine then it is OK if the > "core" part lives there too. Hi Vinod, So this patch series serves as the basis of getting the idxd dsa_bus_type related bits fixed up so that auxdev is not necessary. When Jason reviewed previous iterations of the mdev series, he noted that the mdev driver needs to go under VFIO. After the auxdev conversion of the mdev series, Jason and Dan after further review suggested that given there's an internal bus in idxd driver already (dsa_bus_type), that can be used to load drivers rather than needing to rely on auxiliary bus. But the implementation of the dsa_bus_type needs some fixes. After this series, I have another series that's going through internal review right now that will fixup the driver setup and initialization of dsa_bus_type and allow us to load external drivers for the wq. The in kernel use cases for DSA is still valid under dmaengine so the core parts remains valid for dmaengine. The plan going forward is after getting all the fixups in we are planning to: 1. Introduce UACCE framework support for idxd and have a wq driver resides under drivers/misc/uacce/idxd to support the char device operations and deprecate the current custom char dev in idxd. This should remove the burden on you to deal with the char device. 2. Resubmit the mdev driver under drivers/vfio/mdev/idxd after Jason's latest VFIO refactoring is done. 3. Introduce new kernel use cases with dmanegine API support for SVA operations. Let me know if this sounds ok to you. Thanks!
On Tue, Mar 23, 2021 at 08:44:29AM -0700, Dave Jiang wrote: > 1. Introduce UACCE framework support for idxd and have a wq driver resides > under drivers/misc/uacce/idxd to support the char device operations and > deprecate the current custom char dev in idxd. This should remove the burden > on you to deal with the char device. Gah, I feel I already complained at Intel for cramming their own private char devices into subsystems! *subsystems* define the user API, not random drivers in them. uacce is a reasonable place to put something like this if there isn't a multi-driver standard If this is the plan we should block of the char dev under CONFIG_EXPERIMENTAL or something to discourage people using the uAPI we are planning to delete Jason
On 3/24/2021 6:56 AM, Jason Gunthorpe wrote: > On Tue, Mar 23, 2021 at 08:44:29AM -0700, Dave Jiang wrote: > >> 1. Introduce UACCE framework support for idxd and have a wq driver resides >> under drivers/misc/uacce/idxd to support the char device operations and >> deprecate the current custom char dev in idxd. This should remove the burden >> on you to deal with the char device. > Gah, I feel I already complained at Intel for cramming their own > private char devices into subsystems! *subsystems* define the user > API, not random drivers in them. > > uacce is a reasonable place to put something like this if there isn't > a multi-driver standard > > If this is the plan we should block of the char dev under > CONFIG_EXPERIMENTAL or something to discourage people using the uAPI > we are planning to delete The whole reason to move to UACCE is to relieve Vinod the burden of having to review that code under dmaengine. It was unfortunate that UACCE showed up a kernel version later after the idxd driver was accepted. Do you have a better suggestion?
On Wed, Mar 24, 2021 at 07:57:57AM -0700, Dave Jiang wrote: > > On 3/24/2021 6:56 AM, Jason Gunthorpe wrote: > > On Tue, Mar 23, 2021 at 08:44:29AM -0700, Dave Jiang wrote: > > > > > 1. Introduce UACCE framework support for idxd and have a wq driver resides > > > under drivers/misc/uacce/idxd to support the char device operations and > > > deprecate the current custom char dev in idxd. This should remove the burden > > > on you to deal with the char device. > > Gah, I feel I already complained at Intel for cramming their own > > private char devices into subsystems! *subsystems* define the user > > API, not random drivers in them. > > > > uacce is a reasonable place to put something like this if there isn't > > a multi-driver standard > > > > If this is the plan we should block of the char dev under > > CONFIG_EXPERIMENTAL or something to discourage people using the uAPI > > we are planning to delete > The whole reason to move to UACCE is to relieve Vinod the burden of having > to review that code under dmaengine. It was unfortunate that UACCE showed up > a kernel version later after the idxd driver was accepted. Do you have a > better suggestion? No, I said it is a reasonable thing to do Jason
On 23-03-21, 08:44, Dave Jiang wrote: > On 3/23/2021 4:56 AM, Jason Gunthorpe wrote: > > On Tue, Mar 23, 2021 at 05:15:30PM +0530, Vinod Koul wrote: > > > > Vinod, > > > > The series fixes the various 'struct device' lifetime handling in the > > > > idxd driver. The devm managed lifetime is incompatible with 'struct device' > > > > objects that resides in the idxd context. Tested with > > > > CONFIG_DEBUG_KOBJECT_RELEASE and address all issues from that. > > > Sorry for not looking into this earlier.. But I would like to check on > > > the direction idxd is taking. Somehow I get the feel the dmaengine is > > > not the right place for this. Considering that now we have auxdev merged > > > in, should the idxd be spilt into smaller function and no dmaengine > > > parts moved away from dmaengine... I think we do lack a subsystem for > > > all things accelerator in kernel atm... > > auxdev shouldn't be over-used IMHO. > > > > If the main purpose of the driver is dma engine then it is OK if the > > "core" part lives there too. > > Hi Vinod, > > So this patch series serves as the basis of getting the idxd dsa_bus_type > related bits fixed up so that auxdev is not necessary. When Jason reviewed > previous iterations of the mdev series, he noted that the mdev driver needs > to go under VFIO. After the auxdev conversion of the mdev series, Jason and > Dan after further review suggested that given there's an internal bus in > idxd driver already (dsa_bus_type), that can be used to load drivers rather > than needing to rely on auxiliary bus. But the implementation of the > dsa_bus_type needs some fixes. After this series, I have another series > that's going through internal review right now that will fixup the driver > setup and initialization of dsa_bus_type and allow us to load external > drivers for the wq. The in kernel use cases for DSA is still valid under > dmaengine so the core parts remains valid for dmaengine. The plan going > forward is after getting all the fixups in we are planning to: > > 1. Introduce UACCE framework support for idxd and have a wq driver resides > under drivers/misc/uacce/idxd to support the char device operations and > deprecate the current custom char dev in idxd. This should remove the burden > on you to deal with the char device. > > 2. Resubmit the mdev driver under drivers/vfio/mdev/idxd after Jason's > latest VFIO refactoring is done. > > 3. Introduce new kernel use cases with dmanegine API support for SVA > operations. > > Let me know if this sounds ok to you. Thanks! Yes that does sound reasonable to me, when should I expect this move to show up on list?
On 3/24/2021 11:28 PM, Vinod Koul wrote: > On 23-03-21, 08:44, Dave Jiang wrote: >> On 3/23/2021 4:56 AM, Jason Gunthorpe wrote: >>> On Tue, Mar 23, 2021 at 05:15:30PM +0530, Vinod Koul wrote: >>>>> Vinod, >>>>> The series fixes the various 'struct device' lifetime handling in the >>>>> idxd driver. The devm managed lifetime is incompatible with 'struct device' >>>>> objects that resides in the idxd context. Tested with >>>>> CONFIG_DEBUG_KOBJECT_RELEASE and address all issues from that. >>>> Sorry for not looking into this earlier.. But I would like to check on >>>> the direction idxd is taking. Somehow I get the feel the dmaengine is >>>> not the right place for this. Considering that now we have auxdev merged >>>> in, should the idxd be spilt into smaller function and no dmaengine >>>> parts moved away from dmaengine... I think we do lack a subsystem for >>>> all things accelerator in kernel atm... >>> auxdev shouldn't be over-used IMHO. >>> >>> If the main purpose of the driver is dma engine then it is OK if the >>> "core" part lives there too. >> Hi Vinod, >> >> So this patch series serves as the basis of getting the idxd dsa_bus_type >> related bits fixed up so that auxdev is not necessary. When Jason reviewed >> previous iterations of the mdev series, he noted that the mdev driver needs >> to go under VFIO. After the auxdev conversion of the mdev series, Jason and >> Dan after further review suggested that given there's an internal bus in >> idxd driver already (dsa_bus_type), that can be used to load drivers rather >> than needing to rely on auxiliary bus. But the implementation of the >> dsa_bus_type needs some fixes. After this series, I have another series >> that's going through internal review right now that will fixup the driver >> setup and initialization of dsa_bus_type and allow us to load external >> drivers for the wq. The in kernel use cases for DSA is still valid under >> dmaengine so the core parts remains valid for dmaengine. The plan going >> forward is after getting all the fixups in we are planning to: >> >> 1. Introduce UACCE framework support for idxd and have a wq driver resides >> under drivers/misc/uacce/idxd to support the char device operations and >> deprecate the current custom char dev in idxd. This should remove the burden >> on you to deal with the char device. >> >> 2. Resubmit the mdev driver under drivers/vfio/mdev/idxd after Jason's >> latest VFIO refactoring is done. >> >> 3. Introduce new kernel use cases with dmanegine API support for SVA >> operations. >> >> Let me know if this sounds ok to you. Thanks! > Yes that does sound reasonable to me, when should I expect this move to > show up on list? We will try to do this in stages. So first we need to get this 'struct device' lifetime fixes series into the kernel. Next I have the 'struct device_driver' setup/shutdown series fix that Dan is reviewing internally right now that I will post as soon as he okays it. I also have the uacce conversion series ready and pending Dan's review. The mdev series I need to rebase it to Jason's new VFIO code refactor. Some of that refactor code is not yet posted public (I think?). So that will take a little longer. For the kernel SVA support, I have internal code but need to discuss with Dan on the implementation. Also we need to measure performance on hardware to make our case for the new kernel usage enablings. So that will come later.
v7: - Fix up the 'struct device' setup in char device code (Jason) - Split out the char dev fixes (Jason) - Split out the DMA dev fixes (Dan) - Split out the each of the conf_dev fixes - Split out removal of the pcim_* calls - Split out removal of the devm_* calls - Split out the fixes for interrupt config calls - Reviewed by Dan. v6: - Fix char dev initialization issues (Jason) - Fix other 'struct device' initialization issues. v5: - Rebased against 5.12-rc dmaengine/fixes v4: - fix up the life time of cdev creation/destruction (Jason) - Tested with KASAN and other memory allocation leak detections. (Jason) v3: - Remove devm_* for irq request and cleanup related bits (Jason) v2: - Remove all devm_* alloc for idxd_device (Jason) - Add kref dep for dma_dev (Jason) Vinod, The series fixes the various 'struct device' lifetime handling in the idxd driver. The devm managed lifetime is incompatible with 'struct device' objects that resides in the idxd context. Tested with CONFIG_DEBUG_KOBJECT_RELEASE and address all issues from that. Please consider for damengine/fixes for the 5.12-rc. --- Dave Jiang (8): dmaengine: idxd: fix dma device lifetime dmaengine: idxd: cleanup pci interrupt vector allocation management dmaengine: idxd: removal of pcim managed mmio mapping dmaengine: idxd: fix idxd conf_dev 'struct device' lifetime dmaengine: idxd: fix wq conf_dev 'struct device' lifetime dmaengine: idxd: fix engine conf_dev lifetime dmaengine: idxd: fix group conf_dev lifetime dmaengine: idxd: fix cdev setup and free device lifetime issues drivers/dma/idxd/cdev.c | 131 +++++++----------- drivers/dma/idxd/device.c | 20 +-- drivers/dma/idxd/idxd.h | 54 ++++++-- drivers/dma/idxd/init.c | 284 +++++++++++++++++++++++++++----------- drivers/dma/idxd/irq.c | 10 +- drivers/dma/idxd/sysfs.c | 239 ++++++++++++++++---------------- 6 files changed, 437 insertions(+), 301 deletions(-) --