Re: [PATCH] staging: ion: remove from the tree
On Fri, Oct 16, 2020 at 1:29 AM Greg Kroah-Hartman wrote: > > On Thu, Aug 27, 2020 at 09:31:27AM -0400, Laura Abbott wrote: > > On 8/27/20 8:36 AM, Greg Kroah-Hartman wrote: > > > The ION android code has long been marked to be removed, now that we > > > dma-buf support merged into the real part of the kernel. > > > > > > It was thought that we could wait to remove the ion kernel at a later > > > time, but as the out-of-tree Android fork of the ion code has diverged > > > quite a bit, and any Android device using the ion interface uses that > > > forked version and not this in-tree version, the in-tree copy of the > > > code is abandonded and not used by anyone. > > > > > > Combine this abandoned codebase with the need to make changes to it in > > > order to keep the kernel building properly, which then causes merge > > > issues when merging those changes into the out-of-tree Android code, and > > > you end up with two different groups of people (the in-kernel-tree > > > developers, and the Android kernel developers) who are both annoyed at > > > the current situation. Because of this problem, just drop the in-kernel > > > copy of the ion code now, as it's not used, and is only causing problems > > > for everyone involved. > > > > > > Cc: "Arve Hjønnevåg" > > > Cc: "Christian König" > > > Cc: Christian Brauner > > > Cc: Christoph Hellwig > > > Cc: Hridya Valsaraju > > > Cc: Joel Fernandes > > > Cc: John Stultz > > > Cc: Laura Abbott > > > Cc: Martijn Coenen > > > Cc: Shuah Khan > > > Cc: Sumit Semwal > > > Cc: Suren Baghdasaryan > > > Cc: Todd Kjos > > > Cc: de...@driverdev.osuosl.org > > > Cc: dri-de...@lists.freedesktop.org > > > Cc: linaro-mm-...@lists.linaro.org > > > Signed-off-by: Greg Kroah-Hartman > > > > We discussed this at the Android MC on Monday and the plan was to > > remove it after the next LTS release. > > As 5.10 will be the next LTS release, I have now merged it to my > "testing" branch to go into 5.11-rc1. Sounds great! Thanks so much for waiting a bit on this, I really appreciate it! -john ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: ion: remove from the tree
On Fri, Aug 28, 2020 at 1:05 AM Greg Kroah-Hartman wrote: > > On Thu, Aug 27, 2020 at 11:54:12AM -0700, John Stultz wrote: > > On Thu, Aug 27, 2020 at 10:17 AM Greg Kroah-Hartman > > wrote: > > > On Thu, Aug 27, 2020 at 10:31:41PM +0530, Amit Pundir wrote: > > > > I don't know what is the right thing to do here. I just want to > > > > highlight that AOSP's audio (codec2) HAL depends on the ION system > > > > heap and it will break AOSP for people who boot mainline on their > > > > devices, even for just testing purpose like we do in Linaro. Right now > > > > we need only 1 (Android specific out-of-tree) patch to boot AOSP with > > > > mainline and Sumit is already trying to upstream that vma naming > > > > patch. Removal of in-kernel ION, will just add more to that delta. > > > > > > As AOSP will continue to rely on ION after December of this year, all > > > you are doing is postponing the inevitable a few more months. > > > > > > Push back on the Android team to fix up the code to not use ION, they > > > know this needs to happen. > > > > The point though, is your main premise that no one is using this isn't true. > > They are using the version of ion in the Android kernel tree, yes, as it > has new features that many people are relying on. > > The version that is currently in the kernel tree is crippled, and maybe > works for some use cases, but not the majority, right? So my understanding is the Android Common Kernel tree version was mostly reworked to allow heaps as modules, and allowed heaps to have their own exporter logic (not unlike how dmabuf heaps do it). The main allocation interface is maybe slightly tweaked for out-of-tree vendor heaps, but doesn't affect the in-staging heaps. There's also a few optimizations we have skipped taking upstream. So yes, there are differences, but I don't feel your characterization is quite accurate. > > I'm actively working with Hridya and folks on the codec2 HAL side to > > transition this on the userland side: > > > > https://android-review.googlesource.com/c/platform/frameworks/av/+/1368918/3 > > > > I'd like AOSP to not use ION after September (though being external I > > can't promise anything), much less continuing after December. > > The android team has said they will be dropping ION use for the "next" > Android release, which is sometime next year from what I recall. > December is probably not going to happen :) AOSP is what the next Android release forks off of, so it needs to be fixed first. > > I want this migration to happen as much as anyone. But I'd prefer to > > keep ION in staging until after the LTS is announced. Having both > > around helps development for the transition, which helps us have a > > reliable solution, which helps vendors to migrate and be able to do > > comparative performance testing. > > I don't understand what having this in the "next" kernel helps us with > here. And I would really really prefer to NOT have an outdated version > of this code in a kernel tree that I am going to have to support for the > next X number of years, when no one is using that version of the driver. > > What is this LTS fixation to keep this code around for? Who does it > help? Vendors usually target LTS releases for their hardware bringups. Having a LTS release with both ION and DMA BUF Heaps helps them validate their old ION solution as performant, and then migrate to DMA BUF Heaps and be able to do performance comparisons holding all other things equal. > > I do appreciate that keeping it isn't free, but I also don't feel the > > chaos-monkey approach here is really motivational in the way you > > intend. > > I don't see it helping anyone to leave this around, except to cause > merge issues for me, and development issues for other developers. > > Anyone who really wants this code, can easily revert the deletion and > move on and grab the AOSP copy of the code. That's what they did when > we deleted other Android features that are still relied on. > > Given that the "isn't free" is causing _me_ real pain, and not the > actual users of this code, I am leaning toward wanting to move that > pain/cost to those users, for obvious reasons. Sure. Again, I do understand the desire to remove it, and it's your right to do so. Keeping the code for an extra year in LTS (over 5.4) is a cost, so I understand if you drop it. But I'll ask that you make that judgement clear as the main motivator/rationale of the commit message, rather than flippantly pretending it's not being used, and that everyone agrees it has no usefulness to keep around (especially after we've had this conversation a few times already this year). thanks -john ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: ion: remove from the tree
On Thu, Aug 27, 2020 at 10:17 AM Greg Kroah-Hartman wrote: > On Thu, Aug 27, 2020 at 10:31:41PM +0530, Amit Pundir wrote: > > I don't know what is the right thing to do here. I just want to > > highlight that AOSP's audio (codec2) HAL depends on the ION system > > heap and it will break AOSP for people who boot mainline on their > > devices, even for just testing purpose like we do in Linaro. Right now > > we need only 1 (Android specific out-of-tree) patch to boot AOSP with > > mainline and Sumit is already trying to upstream that vma naming > > patch. Removal of in-kernel ION, will just add more to that delta. > > As AOSP will continue to rely on ION after December of this year, all > you are doing is postponing the inevitable a few more months. > > Push back on the Android team to fix up the code to not use ION, they > know this needs to happen. The point though, is your main premise that no one is using this isn't true. I'm actively working with Hridya and folks on the codec2 HAL side to transition this on the userland side: https://android-review.googlesource.com/c/platform/frameworks/av/+/1368918/3 I'd like AOSP to not use ION after September (though being external I can't promise anything), much less continuing after December. I want this migration to happen as much as anyone. But I'd prefer to keep ION in staging until after the LTS is announced. Having both around helps development for the transition, which helps us have a reliable solution, which helps vendors to migrate and be able to do comparative performance testing. I do appreciate that keeping it isn't free, but I also don't feel the chaos-monkey approach here is really motivational in the way you intend. thanks -john ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 25/49] staging: hikey9xx/gpu: do some code cleanups
On Thu, Aug 20, 2020 at 1:23 AM Mauro Carvalho Chehab wrote: > > (added c/c Rob Herring) > > Em Wed, 19 Aug 2020 18:53:06 -0700 > John Stultz escreveu: > > > On Wed, Aug 19, 2020 at 4:46 AM Mauro Carvalho Chehab > > wrote: > > > @@ -376,7 +355,7 @@ static int kirin_drm_platform_resume(struct > > > platform_device *pdev) > > > } > > > > > > static const struct of_device_id kirin_drm_dt_ids[] = { > > > - { .compatible = "hisilicon,hi3660-dpe", > > > + { .compatible = "hisilicon,kirin960-dpe", > > > > > > One issue, elsewhere in your patch stack you still refer to the > > hisilicon,hi3660-dpe compatible string. This should probably be > > consistent one way or the other. > > Agreed with regards to consistency. > > It sounds to me that calling those as Kirin 9xx (and the previous one > as Kirin 620) is better than using the part number. > > Here, googling for "Kirin 970" gave about 6.9 million hits, while "Hi3670" > gave only 75,5K hits. > > Kirin 620 has similar results: 6.85 million hits, against 61,9 hits > for "Hi3620". Hi6620 is kirin 620 I believe. > With "Kirin 960", the numbers are a lot higher: had 21,4 million hits, > against 423K hits for "Hi3660". > > So, my preference is to use "Kirin 620, 960 and 970" for future changes. I think traditionally the DTS is developed with the hardware documentation sometimes before the SoC is announced, so they tend to stick with whatever those documents call it, rather than (later more google-able) marketing names. That said, I don't have a preference, as long as it's consistent, and we don't change compatible strings that are already upstream. > I would love to make this consistent among the Kernel. However, > I'm not sure if changing "compatible" would be acceptable > by DT maintainers. > Existing bindings are already ABI. So we can't change those. New bindings can be set to what makes the most sense. thanks -john ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 00/49] DRM driver for Hikey 970
On Wed, Aug 19, 2020 at 7:01 PM John Stultz wrote: > > On Wed, Aug 19, 2020 at 2:36 PM John Stultz wrote: > > > > On Wed, Aug 19, 2020 at 4:46 AM Mauro Carvalho Chehab > > wrote: > > > So, IMO, the best is to keep it on staging for a while, until those > > > remaining bugs gets solved. > > > > > > I added this series, together with the regulator driver and > > > a few other patches (including a hack to fix a Kernel 5.8 > > > regression at WiFi ) at: > > > > > > > > > https://gitlab.freedesktop.org/mchehab_kernel/hikey-970/-/commits/master > > > > Sorry, one more small request: Could you create a branch that only has > > the DRM driver changes in it? > > > > The reason I ask, is that since the HiKey960 isn't affected by the > > majority of the problems you listed as motivation for going through > > staging. So if we can validate that your tree works fine on HiKey960, > > the series can be cleaned up and submitted properly upstream to enable > > that SoC, and the outstanding 970 issues can be worked out afterwards > > against mainline. > > Just as a heads up, I tried testing your tree with my HiKey960, and > after fixing the compat string inconsistency, the drivers seem to load > properly. However the drm_hwcomposer seems to have some trouble with > the driver: > 01-01 00:12:41.456 345 345 E hwc-drm-display-compositor: Commit > test failed for display 0, FIXME > 01-01 00:12:41.456 345 345 E hwc-drm-two: Failed to apply the > frame composition ret=-22 > 01-01 00:12:41.456 351 351 E HWComposer: > presentAndGetReleaseFences: present failed for display 0: BadParameter > (4) > > I'll dig in a bit further as to why, but wanted to give you a heads up. Ok, I've mostly gotten it sorted out: - You're missing a few color formats. - And I re-discovered a crash that was already fixed in my tree. I'll send those patches in a few here. That said even with the patches I've got on top of your series, I still see a few issues: 1) I'm seeing red-blue swap with your driver. I need to dig a bit to see what the difference is, I know gralloc has a config option for this, and maybe the version of the driver I'm carrying has it wrong? 2) Performance is noticeably worse. Whereas with my tree, I see close to 60fps (that clk issue we mentioned earlier is why it's not exactly 60) in most tests, but with yours it mostly hovers around 30some fps, occasionally speeding up to 40 and then back down. Obviously with some work I suspect we'll be able to sort these out, but I also do feel that the set you're starting with for upstreaming is pretty old. The driver I'm carrying was heavily refactored around 5.0 to share code with the existing kirin driver, in the hopes of making usptreaming easier, and it seems a shame to throw that out and focus your efforts on the older tree. But to be fair, I've not had time to upstream the driver myself, and it's obviously your choice on how you spend your time. I am really excited to see your efforts here, regardless of which driver you end up pushing. thanks -john ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 00/49] DRM driver for Hikey 970
On Wed, Aug 19, 2020 at 2:36 PM John Stultz wrote: > > On Wed, Aug 19, 2020 at 4:46 AM Mauro Carvalho Chehab > wrote: > > So, IMO, the best is to keep it on staging for a while, until those > > remaining bugs gets solved. > > > > I added this series, together with the regulator driver and > > a few other patches (including a hack to fix a Kernel 5.8 > > regression at WiFi ) at: > > > > > > https://gitlab.freedesktop.org/mchehab_kernel/hikey-970/-/commits/master > > Sorry, one more small request: Could you create a branch that only has > the DRM driver changes in it? > > The reason I ask, is that since the HiKey960 isn't affected by the > majority of the problems you listed as motivation for going through > staging. So if we can validate that your tree works fine on HiKey960, > the series can be cleaned up and submitted properly upstream to enable > that SoC, and the outstanding 970 issues can be worked out afterwards > against mainline. Just as a heads up, I tried testing your tree with my HiKey960, and after fixing the compat string inconsistency, the drivers seem to load properly. However the drm_hwcomposer seems to have some trouble with the driver: 01-01 00:12:41.456 345 345 E hwc-drm-display-compositor: Commit test failed for display 0, FIXME 01-01 00:12:41.456 345 345 E hwc-drm-two: Failed to apply the frame composition ret=-22 01-01 00:12:41.456 351 351 E HWComposer: presentAndGetReleaseFences: present failed for display 0: BadParameter (4) I'll dig in a bit further as to why, but wanted to give you a heads up. thanks -john ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 25/49] staging: hikey9xx/gpu: do some code cleanups
On Wed, Aug 19, 2020 at 4:46 AM Mauro Carvalho Chehab wrote: > @@ -376,7 +355,7 @@ static int kirin_drm_platform_resume(struct > platform_device *pdev) > } > > static const struct of_device_id kirin_drm_dt_ids[] = { > - { .compatible = "hisilicon,hi3660-dpe", > + { .compatible = "hisilicon,kirin960-dpe", One issue, elsewhere in your patch stack you still refer to the hisilicon,hi3660-dpe compatible string. This should probably be consistent one way or the other. thanks -john ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 00/49] DRM driver for Hikey 970
On Wed, Aug 19, 2020 at 4:46 AM Mauro Carvalho Chehab wrote: > So, IMO, the best is to keep it on staging for a while, until those > remaining bugs gets solved. > > I added this series, together with the regulator driver and > a few other patches (including a hack to fix a Kernel 5.8 > regression at WiFi ) at: > > > https://gitlab.freedesktop.org/mchehab_kernel/hikey-970/-/commits/master Sorry, one more small request: Could you create a branch that only has the DRM driver changes in it? The reason I ask, is that since the HiKey960 isn't affected by the majority of the problems you listed as motivation for going through staging. So if we can validate that your tree works fine on HiKey960, the series can be cleaned up and submitted properly upstream to enable that SoC, and the outstanding 970 issues can be worked out afterwards against mainline. thanks -john ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 00/49] DRM driver for Hikey 970
On Wed, Aug 19, 2020 at 4:46 AM Mauro Carvalho Chehab wrote: > Yet, I'm submitting it via staging due to the following reasons: > > - It depends on the LDO3 power supply, which is provided by > a regulator driver that it is currently on staging; > - Due to legal reasons, I need to preserve the authorship of > each one responsbile for each patch. So, I need to start from > the original patch from Kernel 4.4; > - There are still some problems I need to figure out how to solve: >- The adv7535 can't get EDID data. Maybe it is a timing issue, > but it requires more research to be sure about how to solve it; I've seen this on the HiKey960 as well. There is a patch to the adv7533 driver I have to add a mdelay that seems to consistently resolve the timing problem. At some point I mentioned it to one of the maintainers who seems open to having it added, but it seemed silly to submit it until there was a upstream driver that needed such a change. So I think that patch can be submitted as a follow on to this (hopefully cleaned up) series. >- The driver only accept resolutions on a defined list, as there's > a known bug that this driver may have troubles with random > resolutions. Probably due to a bug at the pixel clock settings; So, yes, the SoC clks can't generate proper signals for HDMI frequencies (apparently it's not an issue for panels). There is a fixed set that we can get "close enough" that most monitors will work, but its always a bit iffy (some monitors are strict in what they take). On the kirin driver, we were able to do a calculation to figure out if the generated frequency would be close enough: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/hisilicon/kirin/dw_drm_dsi.c#n615 I suspect we could do something similar for the hikey960/70, but I've not really had time to dig in deeply there. Personally, I don't see the allow-list as a problematic short term solution, and again, not sure its worth pushing to staging for. >- Sometimes (at least with 1080p), it generates LDI underflow > errors, which in turn causes the DRM to stop working. That > happens for example when using gdm on Wayland and > gnome on X11; Interestingly, I've not seen this on HiKey960 (at least with Android/Surfaceflinger). The original HiKey board does have the trouble where at 1080p the screen sometimes comes up horizontally offset due to the LDI underflow, but the patches to address it have been worse then the problem, so we reverted those. >- Probably related to the previous issue, when the monitor > suspends due to DPMS, it doesn't return back to life. > I don't believe I see this on HiKey960. But if it's the LDI issue on the 970 that may explain it. > So, IMO, the best is to keep it on staging for a while, until those > remaining bugs gets solved. I'm not sure I see all of these as compelling for pushing it in via staging. And I suspect in the process of submitting the patches for review folks may find the cause of some of the problems you list here. > I added this series, together with the regulator driver and > a few other patches (including a hack to fix a Kernel 5.8 > regression at WiFi ) at: > > > https://gitlab.freedesktop.org/mchehab_kernel/hikey-970/-/commits/master > > > Chen Feng (1): > staging: hikey9xx: Add hisilicon DRM driver for hikey960/970 > > John Stultz (1): > staging: hikey9xx/gpu: port it to work with Kernel v4.9 Nit: This is a display driver and has little to do with the GPU (other then it will eventually live in drivers/gpu/drm/...), so I might suggest using more conventional subject prefix, "drm: hisilicon:" thanks -john ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 00/49] DRM driver for Hikey 970
On Wed, Aug 19, 2020 at 8:31 AM Laurent Pinchart wrote: > On Wed, Aug 19, 2020 at 05:21:20PM +0200, Sam Ravnborg wrote: > > On Wed, Aug 19, 2020 at 01:45:28PM +0200, Mauro Carvalho Chehab wrote: > > > This patch series port the out-of-tree driver for Hikey 970 (which > > > should also support Hikey 960) from the official 96boards tree: > > > > > >https://github.com/96boards-hikey/linux/tree/hikey970-v4.9 > > > > > > Based on his history, this driver seems to be originally written > > > for Kernel 4.4, and was later ported to Kernel 4.9. The original > > > driver used to depend on ION (from Kernel 4.4) and had its own > > > implementation for FB dev API. > > > > > > As I need to preserve the original history (with has patches from > > > both HiSilicon and from Linaro), I'm starting from the original > > > patch applied there. The remaining patches are incremental, > > > and port this driver to work with upstream Kernel. > > > ... > > > - Due to legal reasons, I need to preserve the authorship of > > > each one responsbile for each patch. So, I need to start from > > > the original patch from Kernel 4.4; ... > > I do acknowledge you need to preserve history and all - > > but this patchset is not easy to review. > > Why do we need to preserve history ? Adding relevant Signed-off-by and > Co-developed-by should be enough, shouldn't it ? Having a public branch > that contains the history is useful if anyone is interested, but I don't > think it's required in mainline. Yea. I concur with Laurent here. I'm not sure what legal reasoning you have on this but preserving the "absolute" history here is actively detrimental for review and understanding of the patch set. Preserving Authorship, Signed-off-by lines and adding Co-developed-by lines should be sufficient to provide both atribution credit and DCO history. thanks -john ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 00/16] IOMMU driver for Kirin 960/970
On Tue, Aug 18, 2020 at 9:26 AM Robin Murphy wrote: > On 2020-08-18 16:29, Mauro Carvalho Chehab wrote: > > Em Tue, 18 Aug 2020 15:47:55 +0100 > > Basically, the DT binding has this, for IOMMU: > > > > > > smmu_lpae { > > compatible = "hisilicon,smmu-lpae"; > > }; > > > > ... > > dpe: dpe@e860 { > > compatible = "hisilicon,kirin970-dpe"; > > memory-region = <&drm_dma_reserved>; > > ... > > iommu_info { > > start-addr = <0x8000>; > > size = <0xbfff8000>; > > }; > > } > > > > This is used by kirin9xx_drm_dss.c in order to enable and use > > the iommu: > > > > > > static int dss_enable_iommu(struct platform_device *pdev, struct > > dss_hw_ctx *ctx) > > { > > struct device *dev = NULL; > > > > dev = &pdev->dev; > > > > /* create iommu domain */ > > ctx->mmu_domain = iommu_domain_alloc(dev->bus); > > if (!ctx->mmu_domain) { > > pr_err("iommu_domain_alloc failed!\n"); > > return -EINVAL; > > } > > > > iommu_attach_device(ctx->mmu_domain, dev); > > > > return 0; > > } > > > > The only place where the IOMMU domain is used is on this part of the > > code(error part simplified here) [1]: > > > > void hisi_dss_smmu_on(struct dss_hw_ctx *ctx) > > { > > uint64_t fama_phy_pgd_base; > > uint32_t phy_pgd_base; > > ... > > fama_phy_pgd_base = iommu_iova_to_phys(ctx->mmu_domain, 0); > > phy_pgd_base = (uint32_t)fama_phy_pgd_base; > > if (WARN_ON(!phy_pgd_base)) > > return; > > > > set_reg(smmu_base + SMMU_CB_TTBR0, phy_pgd_base, 32, 0); > > } > > > > [1] > > https://github.com/mchehab/linux/commit/36da105e719b47bbe9d6cb7e5619b30c7f3eb1bd > > > > In other words, the driver needs to get the physical address of the frame > > buffer (mapped via iommu) in order to set some DRM-specific register. > > > > Yeah, the above code is somewhat hackish. I would love to replace > > this part by a more standard approach. > > OK, so from a quick look at that, my impression is that your display > controller has its own MMU and you don't need to pretend to use the > IOMMU API at all. Just have the DRM driver use io-pgtable directly to > run its own set of ARM_32_LPAE_S1 pagetables - see Panfrost for an > example (but try to ignore the wacky "Mali LPAE" format). Yea. For the HiKey960, there was originally a similar patch series but it was refactored out and the (still out of tree) DRM driver I'm carrying doesn't seem to need it (though looking we still have the iommu_info subnode in the dts that maybe needs to be cleaned up). thanks -john ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: android: ion: Skip sync if not mapped
On Fri, Jul 3, 2020 at 12:03 AM Greg Kroah-Hartman wrote: > On Tue, Apr 21, 2020 at 10:05:44AM +0200, Greg Kroah-Hartman wrote: > > On Mon, Apr 20, 2020 at 01:03:39PM -0700, John Stultz wrote: > > > The dmabuf heaps have been in an official kernel now for all of three > > > weeks. So yea, we can "delete [ION] and see who even notices", but I > > > worry that may seem a bit like contempt for the folks doing the work > > > on transitioning over, which doesn't help getting them to participate > > > within the community. > > > > But they aren't participating in the community today as no one is > > touching the ion code. So I fail to see how keeping a dead-end-version > > of ion in the kernel tree really affects anyone these days. > > So, any thoughts here? What's the timeline for ion being able to be > removed that you are comfortable with? Sorry for the slow reply. So my earlier plan was to drop it after the next LTS? thanks -john ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: android: ion: Skip sync if not mapped
On Mon, Apr 20, 2020 at 1:22 AM Christian Brauner wrote: > On Thu, Apr 16, 2020 at 12:25:08PM +0200, Greg Kroah-Hartman wrote: > > On Tue, Apr 14, 2020 at 09:41:31PM -0700, John Stultz wrote: > > > But I do think we can mark it as deprecated and let folks know that > > > around the end of the year it will be deleted. > > > > No one ever notices "depreciated" things, they only notice if the code > > is no longer there :) > > > > So I'm all for just deleting it and seeing who even notices... > > Agreed. I mean, I get there's not much love for ION in staging, and I too am eager to see it go, but I also feel like in the discussions around submitting the dmabuf heaps at talks, etc, that there was clear value in removing ION after a short time so that folks could transition being able to test both implementations against the same kernel so performance regressions, etc could be worked out. I am actively getting many requests for help for vendors who are looking at dmabuf heaps and are starting the transition process, and I'm trying my best to motivate them to directly work within the community so their needed heap functionality can go upstream. But it's going to be a process, and their first attempts aren't going to magically land upstream. I think being able to really compare their implementations as they iterate and push things upstream will help in order to be able to have upstream solutions that are also properly functional for production usage. The dmabuf heaps have been in an official kernel now for all of three weeks. So yea, we can "delete [ION] and see who even notices", but I worry that may seem a bit like contempt for the folks doing the work on transitioning over, which doesn't help getting them to participate within the community. thanks -john ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: android: ion: Skip sync if not mapped
On Tue, Apr 14, 2020 at 9:11 AM Ørjan Eide wrote: > > On Tue, Apr 14, 2020 at 04:28:10PM +0200, Greg Kroah-Hartman wrote: > > On Tue, Apr 14, 2020 at 04:18:47PM +0200, �rjan Eide wrote: > > > Only sync the sg-list of an Ion dma-buf attachment when the attachment > > > is actually mapped on the device. > > > > > > dma-bufs may be synced at any time. It can be reached from user space > > > via DMA_BUF_IOCTL_SYNC, so there are no guarantees from callers on when > > > syncs may be attempted, and dma_buf_end_cpu_access() and > > > dma_buf_begin_cpu_access() may not be paired. > > > > > > Since the sg_list's dma_address isn't set up until the buffer is used > > > on the device, and dma_map_sg() is called on it, the dma_address will be > > > NULL if sync is attempted on the dma-buf before it's mapped on a device. > > > > > > Before v5.0 (commit 55897af63091 ("dma-direct: merge swiotlb_dma_ops > > > into the dma_direct code")) this was a problem as the dma-api (at least > > > the swiotlb_dma_ops on arm64) would use the potentially invalid > > > dma_address. How that failed depended on how the device handled physical > > > address 0. If 0 was a valid address to physical ram, that page would get > > > flushed a lot, while the actual pages in the buffer would not get synced > > > correctly. While if 0 is an invalid physical address it may cause a > > > fault and trigger a crash. > > > > > > In v5.0 this was incidentally fixed by commit 55897af63091 ("dma-direct: > > > merge swiotlb_dma_ops into the dma_direct code"), as this moved the > > > dma-api to use the page pointer in the sg_list, and (for Ion buffers at > > > least) this will always be valid if the sg_list exists at all. > > > > > > But, this issue is re-introduced in v5.3 with > > > commit 449fa54d6815 ("dma-direct: correct the physical addr in > > > dma_direct_sync_sg_for_cpu/device") moves the dma-api back to the old > > > behaviour and picks the dma_address that may be invalid. > > > > > > dma-buf core doesn't ensure that the buffer is mapped on the device, and > > > thus have a valid sg_list, before calling the exporter's > > > begin_cpu_access. > > > > > > Signed-off-by: �rjan Eide > > > --- > > > drivers/staging/android/ion/ion.c | 12 > > > 1 file changed, 12 insertions(+) > > > > > > Resubmit without disclaimer, sorry about that. > > > > > > This seems to be part of a bigger issue where dma-buf exporters assume > > > that their dma-buf begin_cpu_access and end_cpu_access callbacks have a > > > certain guaranteed behavior, which isn't ensured by dma-buf core. > > > > > > This patch fixes this in ion only, but it also needs to be fixed for > > > other exporters, either handled like this in each exporter, or in > > > dma-buf core before calling into the exporters. > > > > > > diff --git a/drivers/staging/android/ion/ion.c > > > b/drivers/staging/android/ion/ion.c > > > index 38b51eace4f9..7b752ba0cb6d 100644 > > > --- a/drivers/staging/android/ion/ion.c > > > +++ b/drivers/staging/android/ion/ion.c > > > > Now that we have the dma-buff stuff in the tree, do we even need the > > ion code in the kernel anymore? Can't we delete it now? > > It looks like the new dma-heaps have the same issue as ion. The > heap-helpers also do dma_sync_sg_for_device() unconditionally on > end_cpu_access which may happen before dma_map_sg(), leading to use of > the 0 dma_address in the sg list of a, yet unmapped, attachment. Yea, the dma-buf heaps code came from the ION logic, so it likely has the same faults. > It could be fixed in dma-heaps just like this patch does for ion. Is > patch a valid way to fix this problem? Or, should this rather be handled > in dma-buf core by tracking the mapped state of attachments there? In the short-term, I'd definitely prefer to have a fix to dmabuf heaps rather then ION, but I also agree that long term it probably shouldn't just be up to the dma-buf exporter (as there are other dmabuf exporters that may have it wrong too), and that we need to address some DMA API expectations/limitations to better handle multiple device pipelines. (I actually gave a talk last fall on some of the issues I see around it: https://youtu.be/UsEVoWD_o0c ) thanks -john ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: android: ion: Skip sync if not mapped
On Tue, Apr 14, 2020 at 7:28 AM Greg Kroah-Hartman wrote: > > On Tue, Apr 14, 2020 at 04:18:47PM +0200, Ørjan Eide wrote: > > Only sync the sg-list of an Ion dma-buf attachment when the attachment > > is actually mapped on the device. > > > > dma-bufs may be synced at any time. It can be reached from user space > > via DMA_BUF_IOCTL_SYNC, so there are no guarantees from callers on when > > syncs may be attempted, and dma_buf_end_cpu_access() and > > dma_buf_begin_cpu_access() may not be paired. > > > > Since the sg_list's dma_address isn't set up until the buffer is used > > on the device, and dma_map_sg() is called on it, the dma_address will be > > NULL if sync is attempted on the dma-buf before it's mapped on a device. > > > > Before v5.0 (commit 55897af63091 ("dma-direct: merge swiotlb_dma_ops > > into the dma_direct code")) this was a problem as the dma-api (at least > > the swiotlb_dma_ops on arm64) would use the potentially invalid > > dma_address. How that failed depended on how the device handled physical > > address 0. If 0 was a valid address to physical ram, that page would get > > flushed a lot, while the actual pages in the buffer would not get synced > > correctly. While if 0 is an invalid physical address it may cause a > > fault and trigger a crash. > > > > In v5.0 this was incidentally fixed by commit 55897af63091 ("dma-direct: > > merge swiotlb_dma_ops into the dma_direct code"), as this moved the > > dma-api to use the page pointer in the sg_list, and (for Ion buffers at > > least) this will always be valid if the sg_list exists at all. > > > > But, this issue is re-introduced in v5.3 with > > commit 449fa54d6815 ("dma-direct: correct the physical addr in > > dma_direct_sync_sg_for_cpu/device") moves the dma-api back to the old > > behaviour and picks the dma_address that may be invalid. > > > > dma-buf core doesn't ensure that the buffer is mapped on the device, and > > thus have a valid sg_list, before calling the exporter's > > begin_cpu_access. > > > > Signed-off-by: Ørjan Eide > > --- > > drivers/staging/android/ion/ion.c | 12 > > 1 file changed, 12 insertions(+) > > > > Resubmit without disclaimer, sorry about that. > > > > This seems to be part of a bigger issue where dma-buf exporters assume > > that their dma-buf begin_cpu_access and end_cpu_access callbacks have a > > certain guaranteed behavior, which isn't ensured by dma-buf core. > > > > This patch fixes this in ion only, but it also needs to be fixed for > > other exporters, either handled like this in each exporter, or in > > dma-buf core before calling into the exporters. > > > > diff --git a/drivers/staging/android/ion/ion.c > > b/drivers/staging/android/ion/ion.c > > index 38b51eace4f9..7b752ba0cb6d 100644 > > --- a/drivers/staging/android/ion/ion.c > > +++ b/drivers/staging/android/ion/ion.c > > Now that we have the dma-buff stuff in the tree, do we even need the > ion code in the kernel anymore? Can't we delete it now? > I agree that we shouldn't be taking further (non-security/cleanup) patches to the ION code. I'd like to give developers a little bit of a transition period (I was thinking a year, but really just one LTS release that has both would do) where they can move their ION heaps over to dmabuf heaps and test both against the same tree. But I do think we can mark it as deprecated and let folks know that around the end of the year it will be deleted. That sound ok? thanks -john ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: Limits for ION Memory Allocator
On Wed, Jul 24, 2019 at 1:18 PM John Stultz wrote: > > On Wed, Jul 24, 2019 at 12:36 PM Laura Abbott wrote: > > > > On 7/17/19 12:31 PM, Alexander Popov wrote: > > > Hello! > > > > > > The syzkaller [1] has a trouble with fuzzing the Linux kernel with ION > > > Memory > > > Allocator. > > > > > > Syzkaller uses several methods [2] to limit memory consumption of the > > > userspace > > > processes calling the syscalls for testing the kernel: > > > - setrlimit(), > > > - cgroups, > > > - various sysctl. > > > But these methods don't work for ION Memory Allocator, so any userspace > > > process > > > that has access to /dev/ion can bring the system to the out-of-memory > > > state. > > > > > > An example of a program doing that: > > > > > > > > > #include > > > #include > > > #include > > > #include > > > #include > > > #include > > > > > > #define ION_IOC_MAGIC 'I' > > > #define ION_IOC_ALLOC _IOWR(ION_IOC_MAGIC, 0, \ > > > struct ion_allocation_data) > > > > > > struct ion_allocation_data { > > > __u64 len; > > > __u32 heap_id_mask; > > > __u32 flags; > > > __u32 fd; > > > __u32 unused; > > > }; > > > > > > int main(void) > > > { > > > unsigned long i = 0; > > > int fd = -1; > > > struct ion_allocation_data data = { > > > .len = 0x13f65d8c, > > > .heap_id_mask = 1, > > > .flags = 0, > > > .fd = -1, > > > .unused = 0 > > > }; > > > > > > fd = open("/dev/ion", 0); > > > if (fd == -1) { > > > perror("[-] open /dev/ion"); > > > return 1; > > > } > > > > > > while (1) { > > > printf("iter %lu\n", i); > > > ioctl(fd, ION_IOC_ALLOC, &data); > > > i++; > > > } > > > > > > return 0; > > > } > > > > > > > > > I looked through the code of ion_alloc() and didn't find any limit checks. > > > Is it currently possible to limit ION kernel allocations for some process? > > > > > > If not, is it a right idea to do that? > > > Thanks! > > > > > > > Yes, I do think that's the right approach. We're working on moving Ion > > out of staging and this is something I mentioned to John Stultz. I don't > > think we've thought too hard about how to do the actual limiting so > > suggestions are welcome. > > In part the dmabuf heaps allow for separate heap devices, so we can > have finer grained permissions to the specific heaps. But that > doesn't provide any controls on how much memory one process could > allocate using the device if it has permission. > > I suspect the same issue is present with any of the dmabuf exporters > (gpu/display drivers, etc), so this is less of an ION/dmabuf heap > issue and more of a dmabuf core accounting issue. > Also, do unmapped memfd buffers have similar accounting issues? thanks -john ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: Limits for ION Memory Allocator
On Wed, Jul 24, 2019 at 12:36 PM Laura Abbott wrote: > > On 7/17/19 12:31 PM, Alexander Popov wrote: > > Hello! > > > > The syzkaller [1] has a trouble with fuzzing the Linux kernel with ION > > Memory > > Allocator. > > > > Syzkaller uses several methods [2] to limit memory consumption of the > > userspace > > processes calling the syscalls for testing the kernel: > > - setrlimit(), > > - cgroups, > > - various sysctl. > > But these methods don't work for ION Memory Allocator, so any userspace > > process > > that has access to /dev/ion can bring the system to the out-of-memory state. > > > > An example of a program doing that: > > > > > > #include > > #include > > #include > > #include > > #include > > #include > > > > #define ION_IOC_MAGIC 'I' > > #define ION_IOC_ALLOC _IOWR(ION_IOC_MAGIC, 0, \ > > struct ion_allocation_data) > > > > struct ion_allocation_data { > > __u64 len; > > __u32 heap_id_mask; > > __u32 flags; > > __u32 fd; > > __u32 unused; > > }; > > > > int main(void) > > { > > unsigned long i = 0; > > int fd = -1; > > struct ion_allocation_data data = { > > .len = 0x13f65d8c, > > .heap_id_mask = 1, > > .flags = 0, > > .fd = -1, > > .unused = 0 > > }; > > > > fd = open("/dev/ion", 0); > > if (fd == -1) { > > perror("[-] open /dev/ion"); > > return 1; > > } > > > > while (1) { > > printf("iter %lu\n", i); > > ioctl(fd, ION_IOC_ALLOC, &data); > > i++; > > } > > > > return 0; > > } > > > > > > I looked through the code of ion_alloc() and didn't find any limit checks. > > Is it currently possible to limit ION kernel allocations for some process? > > > > If not, is it a right idea to do that? > > Thanks! > > > > Yes, I do think that's the right approach. We're working on moving Ion > out of staging and this is something I mentioned to John Stultz. I don't > think we've thought too hard about how to do the actual limiting so > suggestions are welcome. In part the dmabuf heaps allow for separate heap devices, so we can have finer grained permissions to the specific heaps. But that doesn't provide any controls on how much memory one process could allocate using the device if it has permission. I suspect the same issue is present with any of the dmabuf exporters (gpu/display drivers, etc), so this is less of an ION/dmabuf heap issue and more of a dmabuf core accounting issue. Another practical complication is that with Android these days, I believe the gralloc code lives in the HIDL-ized android.hardware.graphics.allocator@2.0-service HAL, which does the buffer allocations on behalf of requests sent over the binder IPC interface. So with all dma-buf allocations effectively going through that single process, I'm not sure we would want to put per-process limits on the allocator. Instead, I suspect we'd want the memory covered by the dmabuf to be accounted against processes that have the dmabuf fd still open? I know Android has some logic with their memtrack HAL to I believe try to do accounting of gpu memory against various processes, but I've not looked at that in detail recently. Todd/Joel: Any input here? thanks -john ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 1/2] staging: android: ion: Remove file ion_carveout_heap.c
On Wed, Jul 3, 2019 at 4:32 AM Laura Abbott wrote: > > On 7/3/19 5:50 AM, Daniel Vetter wrote: > > On Wed, Jul 3, 2019 at 10:37 AM Greg KH wrote: > >> > >> On Wed, Jul 03, 2019 at 01:48:41PM +0530, Nishka Dasgupta wrote: > >>> Remove file ion_carveout_heap.c as its functions and definitions are not > >>> used anywhere. > >>> Issue found with Coccinelle. > >>> > >>> Signed-off-by: Nishka Dasgupta > >>> --- > >>> drivers/staging/android/ion/Kconfig | 9 -- > >>> drivers/staging/android/ion/Makefile | 1 - > >>> .../staging/android/ion/ion_carveout_heap.c | 133 -- > >> > >> I keep trying to do this, but others point out that the ion code is > >> "going to be fixed up soon" and that people rely on this interface now. > >> Well, "code outside of the kernel tree" relies on this, which is not ok, > >> but the "soon" people keep insisting on it... > >> > >> Odds are I should just delete all of ION, as there hasn't been any > >> forward progress on it in a long time. > >> > >> Hopefully that wakes some people up... > > > > John Stultz has done a steady stream on ion destaging patch series > > past few months, und the heading of "DMA-BUF Heaps", targeting > > drivers/dma-buf. I'm not following the details, and it seems a bit a > > crawl, but there's definitely work going on ... Just probably not > > in-place in staging I think. > > -Daniel > > > > > https://lists.freedesktop.org/archives/dri-devel/2019-June/223705.html > > It is making slow and steady progress. Part of this is trying to > make sure we actually get this right before moving anything > out of staging. Hopefully not too much longer. The review feedback has gotten quiet recently so hopefully everyone is nodding. Note, I'd also find it useful to *not* eject ION immediately after dmabuf heaps land, since being able to do A/B validation on the same kernel is useful if folks run into any new perf regressions. But hopefully that transition time is fairly small. > That said, I think we're at the point where nobody wants the > carveout and chunk heaps so I'd actually be okay with removing > those files. Just to be explicit: > > Acked-by: Laura Abbott Agreed. I think there are some out of tree uses by ARM and others for the carveout heaps, but I don't know if anyone is using those unmodified anyway. So no objection from me, as there is no way to use them upstream. thanks -john ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2] staging: android: ion: Allocate from heap ID directly without mask
On Fri, Feb 15, 2019 at 2:51 AM Brian Starkey wrote: > > Hi John, > > On Thu, Feb 14, 2019 at 09:38:29AM -0800, John Stultz wrote: > > > [snip] > > > Some thoughts, as this ABI break has the potential to be pretty painful. > > > > 1) Unfortunately, this ABI is exposed *through* libion via > > ion_alloc/ion_alloc_fd out to gralloc implementations. Which means it > > will have a wider impact to vendor userland code. > > I figured libion could fairly easily loop through all the set bits in > heap_mask and call the ioctl for each until it succeeds. That > preserves the old behaviour from the libion clients' perspective. Potentially, though that implicitly still caps the heaps to 32. So I'm not sure what the net benefit would be. > > 2) For patches that cause ABI breaks, it might be good to make it > > clear in the commit what the userland impact looks like in userspace, > > possibly with an example, so the poor folks who bisect down the change > > as breaking their system in a year or so have a clear example as to > > what they need to change in their code. > > > > 3) Also, its not clear how a given userland should distinguish between > > the different ABIs. We already have logic in libion to distinguish > > between pre-4.12 legacy and post-4.12 implementations (using implicit > > ion_free() behavior). I don't see any such check we can make with this > > code. Adding another ABI version may require we provide an actual > > interface version ioctl. > > > > A slightly fragile/ugly approach might be to attempt a small > allocation with a heap_mask of 0x. On an "old" implementation, > you'd expect that to succeed, whereas it would/could be made to fail > in the "new" one. Yea I think having a proper ION_IOC_VERSION is going to be necessary. I'm hoping to send out an ugly first stab at the kernel side for switching to per-heap devices (with a config for keeping /dev/ion for legacy compat), which I hope will address the core issue this patch does (moving away from heap masks to specifically requested heaps). thanks -john ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2] staging: android: ion: Allocate from heap ID directly without mask
On Fri, Feb 15, 2019 at 12:52 PM Andrew F. Davis wrote: > On 2/15/19 1:58 PM, John Stultz wrote: > > So yea, I don't think we should tie our hands in reworking the > > interfaces, but it would be nice to avoid having subtle ABI changes > > that don't have clear ways for userland to detect which interface > > version its using. > > > > Let me preference this by pointing out I've dealt with the same pain > internally with our Android and soon to also in AOSP for the Beagle x15 > boards[0].. But my stance matches Christoph's in the other ION thread: > > https://lkml.org/lkml/2019/1/19/53 > > The more freely we can make ABI changes here in staging the quicker we > can get this out of staging where the ABI can be locked down. > ION_IOC_VERSION should solve this anyway. If there is real momentum to destaging and locking the ABI down, that's great! I just want to avoid lots of little incompatible changes that don't get us fully out of staging and cause tons of pain for folks trying to make it work (because at that point, its easier for folks to stick to their own out of tree solutions). > > So my initial thought is we simply use a /dev/ion_heaps/ dir which has > > a list of heap devicenodes. /dev/ion goes away. > > > > Currently ION_IOC_HEAP_QUERY returns: > > char name[MAX_HEAP_NAME]; > > __u32 type; > > __u32 heap_id; > > > > The names are discoverable via "ls /dev/ion_heaps/" > > > > The heap_id is really only useful as a handle, and after opening the > > heap device, we'll have the fd to use. > > > > So why have heap_id at all then? > Good question! I'm hoping we can yank them, though internally I didn't quite get there with my first patchset. :) > > But to match the use case you describe: > > 1) ls /dev/ion_heaps/ for a list of heaps > > Yuk, dirent.h and friends :( Yea, its not super fun, but its a standard interface, and the ugly bits can be done once in the helper library. > > Does that sound reasonable? And I don't really mean to dismiss the > > dynamic picking of the best heap part, and having something like a > > opaque constraints bitfield that each device and each heap export so > > userland can match things up would be great. But since we don't have > > any real solutions there yet(still!), it seems like most gralloc > > implementations are likely to be fully knowing which heap they want at > > allocation time. > > > > I think you already touched on my main issue with this, the dynamic > picking not supported. Well, like you said it doesn't really exist now > either. And this doesn't look to stop one from adding it as some ioctl > extensions.. > > Okay, looks like you posted an RFC, lets move the discussion over to > that thread. Sounds good. I look forward to your input! thanks -john ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2] staging: android: ion: Allocate from heap ID directly without mask
On Fri, Feb 15, 2019 at 11:22 AM Andrew F. Davis wrote: > > On 2/15/19 1:01 PM, John Stultz wrote: > > On Fri, Feb 15, 2019 at 2:51 AM Brian Starkey wrote: > >> On Thu, Feb 14, 2019 at 09:38:29AM -0800, John Stultz wrote: > >>> 2) For patches that cause ABI breaks, it might be good to make it > >>> clear in the commit what the userland impact looks like in userspace, > >>> possibly with an example, so the poor folks who bisect down the change > >>> as breaking their system in a year or so have a clear example as to > >>> what they need to change in their code. > >>> > >>> 3) Also, its not clear how a given userland should distinguish between > >>> the different ABIs. We already have logic in libion to distinguish > >>> between pre-4.12 legacy and post-4.12 implementations (using implicit > >>> ion_free() behavior). I don't see any such check we can make with this > >>> code. Adding another ABI version may require we provide an actual > >>> interface version ioctl. > >>> > >> > >> A slightly fragile/ugly approach might be to attempt a small > >> allocation with a heap_mask of 0x. On an "old" implementation, > >> you'd expect that to succeed, whereas it would/could be made to fail > >> in the "new" one. > > > > Yea I think having a proper ION_IOC_VERSION is going to be necessary. > > > > I think that will be helpful to have ready the future just looking at > the way libdrm does things, but not right now as backwards compatibility > with staging code is not a reasonable thing to do. I'm not sure I'm following what you mean here? While we don't have any commitment to userland for interfaces in staging, the reality is that there are a fair number of users affected, and we probably should avoid causing any needless pain if possible. Further, as part of my work, I try to keep the hikey boards with an array of kernels (4.4, 4.9, 4.14, 4.19 and mainline) running with AOSP master. Having hard build breaks so AOSP has to have build time dependencies on newer or older kernels is a big pain, and the 4.12 ABI break was not easy. So yea, I don't think we should tie our hands in reworking the interfaces, but it would be nice to avoid having subtle ABI changes that don't have clear ways for userland to detect which interface version its using. > > I'm hoping to send out an ugly first stab at the kernel side for > > switching to per-heap devices (with a config for keeping /dev/ion for > > legacy compat), which I hope will address the core issue this patch > > does (moving away from heap masks to specifically requested heaps). > > > > Yes, that would remove the need for what this patch does. > Question though, what does the user side look like for this? With the > old /dev/ion we would: > > ion_fd = open("/dev/ion") > ask for a list of heaps (ioctl on ion_fd) > iterate over the details of each heap > pick the best heap for the job > request allocation from that heap (ioctl on ion_fd) > > with per-heap devs we need some way to iterate all over heap devices in > a system, and extract details from each heap device. Maybe we leave > /dev/ion but it's only job is to service ION_IOC_HEAP_QUERY requests but > instead of heap numbers it returns heap names, then device files just > match those names. Then we go allocate() from those. > So my initial thought is we simply use a /dev/ion_heaps/ dir which has a list of heap devicenodes. /dev/ion goes away. Currently ION_IOC_HEAP_QUERY returns: char name[MAX_HEAP_NAME]; __u32 type; __u32 heap_id; The names are discoverable via "ls /dev/ion_heaps/" The heap_id is really only useful as a handle, and after opening the heap device, we'll have the fd to use. The only detail we're missing is the type. I'm a little skeptical how useful type is, but worse case we provide a QUERY ioctl on the heap device to provide type info. Most likely, I expect users to: 1) Open "/dev/ion_heaps/" for the heap they want (since they probably already know) 2) make a HEAP_ALLOCATE ioctl on the fd to allocate But to match the use case you describe: 1) ls /dev/ion_heaps/ for a list of heaps 2) For each heap name, open the heap and make a QUERY ioctl to get type info (for what its worth) 3) Pick best heap for the job (*handwaving magic!*) 4) Do an ALLOC ioctl on the heap's fd to allocate Does that sound reasonable? And I don't really mean to dismiss the dynamic picking of the best heap part, and having something like a opaque constraints bitfield that each device and each heap export so userland can match things up would be great. But since we don't have any real solutions there yet(still!), it seems like most gralloc implementations are likely to be fully knowing which heap they want at allocation time. thanks -john ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2] staging: android: ion: Allocate from heap ID directly without mask
On Mon, Jan 28, 2019 at 1:44 PM Andrew F. Davis wrote: > > Previously the heap to allocate from was selected by a mask of allowed > heap types. This may have been done as a primitive form of constraint > solving, the first heap type that matched any set bit of the heap mask > was allocated from, unless that heap was excluded by having its heap > ID bit not set in the separate passed in heap ID mask. > > The heap type does not really represent constraints that should be > matched against to begin with. So the patch [0] removed the the heap type > mask matching but unfortunately left the heap ID mask check (possibly by > mistake or to preserve API). Therefor we now only have a mask of heap > IDs, but heap IDs are unique identifiers and have nothing to do with the > underlying heap, so mask matching is not useful here. This also limits us > to 32 heaps total in a system. > > With the heap query API users can find the right heap based on type or > name themselves then just supply the ID for that heap. Remove heap ID > mask and allow users to specify heap ID directly by its number. Sorry for the very late reply. I just dug this out of my spam box. While this seems like a reasonable cleanup (ABI pain aside), I'm curious as to other then the 32-heap limitation, what other benefits this brings? My hesitancy is that we still have fixed number to heap mapping. Curious how this fits in (or if it would still be necessary) if we finally moved to previously discussed per-heap device-nodes idea, which is interesting to me as it avoids a fixed enumeration of heaps in the kernel (and also allows for per heap permissions). > I know this is an ABI break, but we are in staging so lets get this over > with now rather than limit ourselves later. > > [0] commit 2bb9f5034ec7 ("gpu: ion: Remove heapmask from client") > > Signed-off-by: Andrew F. Davis > --- > > This also means we don't need to store the available heaps in a plist, > we only operation we care about is lookup, so a better data structure > should be chosen at some point, regular list or xarray maybe? > > This is base on -next as to be on top of the other taken Ion patches. > > Changes from v1: > - Fix spelling in commit message > - Cleanup logic per Brian's suggestion > Some thoughts, as this ABI break has the potential to be pretty painful. 1) Unfortunately, this ABI is exposed *through* libion via ion_alloc/ion_alloc_fd out to gralloc implementations. Which means it will have a wider impact to vendor userland code. 2) For patches that cause ABI breaks, it might be good to make it clear in the commit what the userland impact looks like in userspace, possibly with an example, so the poor folks who bisect down the change as breaking their system in a year or so have a clear example as to what they need to change in their code. 3) Also, its not clear how a given userland should distinguish between the different ABIs. We already have logic in libion to distinguish between pre-4.12 legacy and post-4.12 implementations (using implicit ion_free() behavior). I don't see any such check we can make with this code. Adding another ABI version may require we provide an actual interface version ioctl. thanks -john ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [RFC PATCH v2] android: ion: How to properly clean caches for uncached allocations
On Thu, Nov 1, 2018 at 3:15 PM, Liam Mark wrote: > Based on the suggestions from Laura I created a first draft for a change > which will attempt to ensure that uncached mappings are only applied to > ION memory who's cache lines have been cleaned. > It does this by providing cached mappings (for uncached ION allocations) > until the ION buffer is dma mapped and successfully cleaned, then it drops > the userspace mappings and when pages are accessed they are faulted back > in and uncached mappings are created. > > This change has the following potential disadvantages: > - It assumes that userpace clients won't attempt to access the buffer > while it is being mapped as we are removing the userpspace mappings at > this point (though it is okay for them to have it mapped) > - It assumes that kernel clients won't hold a kernel mapping to the buffer > (ie dma_buf_kmap) while it is being dma-mapped. What should we do if there > is a kernel mapping at the time of dma mapping, fail the mapping, warn? > - There may be a performance penalty as a result of having to fault in the > pages after removing the userspace mappings. > > It passes basic testing involving reading writing and reading from > uncached system heap allocations before and after dma mapping. > > Please let me know if this is heading in the right direction and if there > are any concerns. > > Signed-off-by: Liam Mark Thanks for sending this out! I gave this a whirl on my HiKey960. Seems to work ok, but I'm not sure if the board's usage benefits much from your changes. First, ignore how crazy overall these frame values are right off, we have some cpuidle/cpufreq issues w/ 4.14 that we're still sorting out. Without your patch: default-jankview_list_view,jankbench,1,mean,0,iter_10,List View Fling,48.1333678017, default-jankview_list_view,jankbench,2,mean,0,iter_10,List View Fling,55.8407417387, default-jankview_list_view,jankbench,3,mean,0,iter_10,List View Fling,43.88160374, default-jankview_list_view,jankbench,4,mean,0,iter_10,List View Fling,42.2606222784, default-jankview_list_view,jankbench,5,mean,0,iter_10,List View Fling,44.1791721797, default-jankview_list_view,jankbench,6,mean,0,iter_10,List View Fling,39.7692731775, default-jankview_list_view,jankbench,7,mean,0,iter_10,List View Fling,48.5462154074, default-jankview_list_view,jankbench,8,mean,0,iter_10,List View Fling,40.1321166548, default-jankview_list_view,jankbench,9,mean,0,iter_10,List View Fling,48.0163174397, default-jankview_list_view,jankbench,10,mean,0,iter_10,List View Fling,51.1971686844, With your patch: default-jankview_list_view,jankbench,1,mean,0,iter_10,List View Fling,43.3983274772, default-jankview_list_view,jankbench,2,mean,0,iter_10,List View Fling,45.8456678409, default-jankview_list_view,jankbench,3,mean,0,iter_10,List View Fling,42.9609507211, default-jankview_list_view,jankbench,4,mean,0,iter_10,List View Fling,48.602186248, default-jankview_list_view,jankbench,5,mean,0,iter_10,List View Fling,47.9257658765, default-jankview_list_view,jankbench,6,mean,0,iter_10,List View Fling,47.7405384035, default-jankview_list_view,jankbench,7,mean,0,iter_10,List View Fling,52.0017667611, default-jankview_list_view,jankbench,8,mean,0,iter_10,List View Fling,43.7480812349, default-jankview_list_view,jankbench,9,mean,0,iter_10,List View Fling,44.8138758796, default-jankview_list_view,jankbench,10,mean,0,iter_10,List View Fling,46.4941804068, Just for reference, compared to my earlier patch: default-jankview_list_view,jankbench,1,mean,0,iter_10,List View Fling,33.8638094852, default-jankview_list_view,jankbench,2,mean,0,iter_10,List View Fling,34.0859500474, default-jankview_list_view,jankbench,3,mean,0,iter_10,List View Fling,35.6278973379, default-jankview_list_view,jankbench,4,mean,0,iter_10,List View Fling,31.4999822195, default-jankview_list_view,jankbench,5,mean,0,iter_10,List View Fling,40.0634874771, default-jankview_list_view,jankbench,6,mean,0,iter_10,List View Fling,28.0633472181, default-jankview_list_view,jankbench,7,mean,0,iter_10,List View Fling,36.0400585616, default-jankview_list_view,jankbench,8,mean,0,iter_10,List View Fling,38.1871234374, default-jankview_list_view,jankbench,9,mean,0,iter_10,List View Fling,37.4103602014, default-jankview_list_view,jankbench,10,mean,0,iter_10,List View Fling,40.7147881231, Though I'll spend some more time looking at it closer. thanks -john ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [patch 09/11] x86/vdso: Simplify the invalid vclock case
On Mon, Sep 17, 2018 at 12:25 PM, Andy Lutomirski wrote: > On Fri, Sep 14, 2018 at 5:50 AM, Thomas Gleixner wrote: >> The code flow for the vclocks is convoluted as it requires the vclocks >> which can be invalidated separately from the vsyscall_gtod_data sequence to >> store the fact in a separate variable. That's inefficient. >> > >> notrace static int do_hres(clockid_t clk, struct timespec *ts) >> { >> struct vgtod_ts *base = >od->basetime[clk]; >> unsigned int seq; >> - int mode; >> - u64 ns; >> + u64 cycles, ns; >> >> do { >> seq = gtod_read_begin(gtod); >> - mode = gtod->vclock_mode; >> ts->tv_sec = base->sec; >> ns = base->nsec; >> - ns += vgetsns(&mode); >> + cycles = vgetcyc(gtod->vclock_mode); >> + if (unlikely((s64)cycles < 0)) >> + return vdso_fallback_gettime(clk, ts); > > i was contemplating this, and I would suggest one of two optimizations: > > 1. have all the helpers return a struct containing a u64 and a bool, > and use that bool. The compiler should do okay. > > 2. Be sneaky. Later in the series, you do: > > if (unlikely((s64)cycles < 0)) > return vdso_fallback_gettime(clk, ts); > - ns += (cycles - gtod->cycle_last) * gtod->mult; > + if (cycles > last) > + ns += (cycles - last) * gtod->mult; > > How about: > > if (unlikely((s64)cycles <= last)) { > if (cycles < 0) [or cycles == -1 or whatever] > return vdso_fallback_gettime; > } else { > ns += (cycles - last) * gtod->mult; > } > > which should actually make this whole mess be essentially free. > > Also, I'm not entirely convinced that this "last" thing is needed at > all. John, what's the scenario under which we need it? So my memory is probably a bit foggy, but I recall that as we accelerated gettimeofday, we found that even on systems that claimed to have synced TSCs, they were actually just slightly out of sync. Enough that right after cycles_last had been updated, a read on another cpu could come in just behind cycles_last, resulting in a negative interval causing lots of havoc. So the sanity check is needed to avoid that case. thanks -john ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [patch V2 01/11] clocksource: Provide clocksource_arch_init()
On Mon, Sep 17, 2018 at 5:45 AM, Thomas Gleixner wrote: > Architectures have extra archdata in the clocksource, e.g. for VDSO > support. There are no sanity checks or general initializations for this > available. Add support for that. > > Signed-off-by: Thomas Gleixner Sorry, Let me try reply-all this time. :) Acked-by: John Stultz thanks -john ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [staging:staging-linus 3/3] drivers/staging//android/ion/ion_cma_heap.c:47:14: error: 'CONFIG_CMA_ALIGNMENT' undeclared; did you mean 'CONFIG_CMA_AREAS'?
On Tue, Dec 12, 2017 at 10:25 PM, kbuild test robot wrote: > tree: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git > staging-linus > head: d98e6dbf42f73101128885a1e0ae672cd92b2e1a > commit: d98e6dbf42f73101128885a1e0ae672cd92b2e1a [3/3] staging: ion: Fix > ion_cma_heap allocations > config: i386-randconfig-x019-12132053 (attached as .config) > compiler: gcc-7 (Debian 7.2.0-12) 7.2.1 20171025 > reproduce: > git checkout d98e6dbf42f73101128885a1e0ae672cd92b2e1a > # save the attached .config to linux build tree > make ARCH=i386 > > All errors (new ones prefixed by >>): > >drivers/staging//android/ion/ion_cma_heap.c: In function > 'ion_cma_allocate': >>> drivers/staging//android/ion/ion_cma_heap.c:47:14: error: >>> 'CONFIG_CMA_ALIGNMENT' undeclared (first use in this function); did you >>> mean 'CONFIG_CMA_AREAS'? > if (align > CONFIG_CMA_ALIGNMENT) > ^~~~ > CONFIG_CMA_AREAS >drivers/staging//android/ion/ion_cma_heap.c:47:14: note: each undeclared > identifier is reported only once for each function it appears in > > vim +47 drivers/staging//android/ion/ion_cma_heap.c Sorry about this, I guess CONFIG_CMA_ALIGNMENT isn't generically available. I'll rework the patch to figure out how to properly conditionalize this and resubmit. thanks -john ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] binder: let ANDROID_BINDER_IPC_32BIT be selectable on 32bit ARM
On Tue, Aug 22, 2017 at 8:01 PM, Jisheng Zhang wrote: > On Tue, 22 Aug 2017 19:57:04 -0700 John Stultz wrote: > >> On Tue, Aug 22, 2017 at 7:56 PM, John Stultz wrote: >> > On Tue, Aug 22, 2017 at 7:34 PM, Jisheng Zhang wrote: >> >> On Tue, 22 Aug 2017 18:51:08 -0700 Greg KH wrote: >> >> >> >>> On Tue, Aug 08, 2017 at 07:03:05PM +0800, Jisheng Zhang wrote: >> >>> > As noted in commit d0bdff0db809 ("staging: Fix build issues with new >> >>> > binder API"), we can add back the choice for 32bit ARM "once a 64bit >> >>> > __get_user_asm_* implementation is merged." Commit e38361d032f1 ("ARM: >> >>> > 8091/2: add get_user() support for 8 byte types") has added the >> >>> > support, so it's time to let ANDROID_BINDER_IPC_32BIT be selectable on >> >>> > 32bit ARM >> >>> >> >>> Ok, but: >> >>> >> >>> > >> >>> > Signed-off-by: Jisheng Zhang >> >>> > --- >> >>> > drivers/android/Kconfig | 2 +- >> >>> > 1 file changed, 1 insertion(+), 1 deletion(-) >> >>> > >> >>> > diff --git a/drivers/android/Kconfig b/drivers/android/Kconfig >> >>> > index 832e885349b1..aca5dc30b97b 100644 >> >>> > --- a/drivers/android/Kconfig >> >>> > +++ b/drivers/android/Kconfig >> >>> > @@ -32,7 +32,7 @@ config ANDROID_BINDER_DEVICES >> >>> > therefore logically separated from the other devices. >> >>> > >> >>> > config ANDROID_BINDER_IPC_32BIT >> >>> > - bool >> >>> > + bool "Use old (Android 4.4 and earlier) 32-bit binder API" >> >>> > depends on !64BIT && ANDROID_BINDER_IPC >> >>> >> >>> You don't actually change the depends line :( >> >>> >> >>> Please fix up, and test it, and then resend. >> >> >> >> IHOM, the dependency is correct: 64bit platforms don't support >> >> ANDROID_BINDER_IPC_32BIT. What do you think? >> > >> > I think this indicates the commit message is unclear. >> > >> > Part of it is that the config is inverted from the description. The >> > patch doesn't enable the 32bit legacy binder ABI on 32bit systems, it >> > just allows the option to be unselected, so that the 64bit ABI will be >> > used on 32bit systems. >> > >> > Conceptually I don't have an objection to the change (though maybe try >> > to rework the commit message), but I don't have anything to actually >> > test it on right now, so I'm hesitant to ack it. >> >> It might also be good to add some detail as to the motivation for this >> change? What benefit does it bring to 32bit platforms to use the newer >> 64bit ABI? >> > > To be honest, the motivation is just to add one more choice for 32bit > platform and let the code be tested under 32bit platform. Maybe we > could then remove ANDROID_BINDER_IPC_32BIT and the related code after > some time? I'm mixed. It would be nice to deprecate the old 32bit ABI, but binder is a real Linux kernel interface now, so we don't break compatibility (at least if it affects anyone - which may be questionable here - not sure there's many upstream 32bit platforms concerned with running legacy Android builds). But just adding the extra option just means there's yet another configuration to test and to keep working. So you may want to articulate the benefits better to make this worth the effort of doing a full transition. thanks -john ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] binder: let ANDROID_BINDER_IPC_32BIT be selectable on 32bit ARM
On Tue, Aug 22, 2017 at 7:56 PM, John Stultz wrote: > On Tue, Aug 22, 2017 at 7:34 PM, Jisheng Zhang wrote: >> On Tue, 22 Aug 2017 18:51:08 -0700 Greg KH wrote: >> >>> On Tue, Aug 08, 2017 at 07:03:05PM +0800, Jisheng Zhang wrote: >>> > As noted in commit d0bdff0db809 ("staging: Fix build issues with new >>> > binder API"), we can add back the choice for 32bit ARM "once a 64bit >>> > __get_user_asm_* implementation is merged." Commit e38361d032f1 ("ARM: >>> > 8091/2: add get_user() support for 8 byte types") has added the >>> > support, so it's time to let ANDROID_BINDER_IPC_32BIT be selectable on >>> > 32bit ARM >>> >>> Ok, but: >>> >>> > >>> > Signed-off-by: Jisheng Zhang >>> > --- >>> > drivers/android/Kconfig | 2 +- >>> > 1 file changed, 1 insertion(+), 1 deletion(-) >>> > >>> > diff --git a/drivers/android/Kconfig b/drivers/android/Kconfig >>> > index 832e885349b1..aca5dc30b97b 100644 >>> > --- a/drivers/android/Kconfig >>> > +++ b/drivers/android/Kconfig >>> > @@ -32,7 +32,7 @@ config ANDROID_BINDER_DEVICES >>> > therefore logically separated from the other devices. >>> > >>> > config ANDROID_BINDER_IPC_32BIT >>> > - bool >>> > + bool "Use old (Android 4.4 and earlier) 32-bit binder API" >>> > depends on !64BIT && ANDROID_BINDER_IPC >>> >>> You don't actually change the depends line :( >>> >>> Please fix up, and test it, and then resend. >> >> IHOM, the dependency is correct: 64bit platforms don't support >> ANDROID_BINDER_IPC_32BIT. What do you think? > > I think this indicates the commit message is unclear. > > Part of it is that the config is inverted from the description. The > patch doesn't enable the 32bit legacy binder ABI on 32bit systems, it > just allows the option to be unselected, so that the 64bit ABI will be > used on 32bit systems. > > Conceptually I don't have an objection to the change (though maybe try > to rework the commit message), but I don't have anything to actually > test it on right now, so I'm hesitant to ack it. It might also be good to add some detail as to the motivation for this change? What benefit does it bring to 32bit platforms to use the newer 64bit ABI? thanks -john ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] binder: let ANDROID_BINDER_IPC_32BIT be selectable on 32bit ARM
On Tue, Aug 22, 2017 at 7:34 PM, Jisheng Zhang wrote: > On Tue, 22 Aug 2017 18:51:08 -0700 Greg KH wrote: > >> On Tue, Aug 08, 2017 at 07:03:05PM +0800, Jisheng Zhang wrote: >> > As noted in commit d0bdff0db809 ("staging: Fix build issues with new >> > binder API"), we can add back the choice for 32bit ARM "once a 64bit >> > __get_user_asm_* implementation is merged." Commit e38361d032f1 ("ARM: >> > 8091/2: add get_user() support for 8 byte types") has added the >> > support, so it's time to let ANDROID_BINDER_IPC_32BIT be selectable on >> > 32bit ARM >> >> Ok, but: >> >> > >> > Signed-off-by: Jisheng Zhang >> > --- >> > drivers/android/Kconfig | 2 +- >> > 1 file changed, 1 insertion(+), 1 deletion(-) >> > >> > diff --git a/drivers/android/Kconfig b/drivers/android/Kconfig >> > index 832e885349b1..aca5dc30b97b 100644 >> > --- a/drivers/android/Kconfig >> > +++ b/drivers/android/Kconfig >> > @@ -32,7 +32,7 @@ config ANDROID_BINDER_DEVICES >> > therefore logically separated from the other devices. >> > >> > config ANDROID_BINDER_IPC_32BIT >> > - bool >> > + bool "Use old (Android 4.4 and earlier) 32-bit binder API" >> > depends on !64BIT && ANDROID_BINDER_IPC >> >> You don't actually change the depends line :( >> >> Please fix up, and test it, and then resend. > > IHOM, the dependency is correct: 64bit platforms don't support > ANDROID_BINDER_IPC_32BIT. What do you think? I think this indicates the commit message is unclear. Part of it is that the config is inverted from the description. The patch doesn't enable the 32bit legacy binder ABI on 32bit systems, it just allows the option to be unselected, so that the 64bit ABI will be used on 32bit systems. Conceptually I don't have an objection to the change (though maybe try to rework the commit message), but I don't have anything to actually test it on right now, so I'm hesitant to ack it. thanks -john ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 3/3] ANDROID: binder: fix proc->tsk check.
On Tue, Aug 8, 2017 at 11:08 AM, Greg KH wrote: > On Tue, Aug 08, 2017 at 10:34:47AM -0700, John Stultz wrote: >> On Fri, Jul 28, 2017 at 4:56 AM, Martijn Coenen wrote: >> > Commit c4ea41ba195d ("binder: use group leader instead of open thread")' >> > was incomplete and didn't update a check in binder_mmap(), causing all >> > mmap() calls into the binder driver to fail. >> > >> > Signed-off-by: Martijn Coenen >> > --- >> > drivers/android/binder.c | 2 +- >> > 1 file changed, 1 insertion(+), 1 deletion(-) >> > >> > diff --git a/drivers/android/binder.c b/drivers/android/binder.c >> > index f7665c31feca..831cdd7d197d 100644 >> > --- a/drivers/android/binder.c >> > +++ b/drivers/android/binder.c >> > @@ -3362,7 +3362,7 @@ static int binder_mmap(struct file *filp, struct >> > vm_area_struct *vma) >> > const char *failure_string; >> > struct binder_buffer *buffer; >> > >> > - if (proc->tsk != current) >> > + if (proc->tsk != current->group_leader) >> > return -EINVAL; >> > >> > if ((vma->vm_end - vma->vm_start) > SZ_4M) >> >> Tested-by: John Stultz >> >> As Amit already confirmed, this resolves the wifi and bluetooth >> regression I was seeing with Android using 4.13-rc2. >> >> Though I've not seen it show up in Linus' tree yet, so I wanted to >> pester folks so it gets into 4.13-rc (its given me some slight grief >> trying to bisect down a separate issue). > > I will queue this up in the next few days, I need to resolve the patches > that have been sent to me for this, sorry for the delay. Hey Greg, Sorry to pester you again on this, but this seems to still be missing in -rc6. Don't want it to slip by. thanks -john ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 3/3] ANDROID: binder: fix proc->tsk check.
On Tue, Aug 8, 2017 at 11:08 AM, Greg KH wrote: > On Tue, Aug 08, 2017 at 10:34:47AM -0700, John Stultz wrote: >> On Fri, Jul 28, 2017 at 4:56 AM, Martijn Coenen wrote: >> > Commit c4ea41ba195d ("binder: use group leader instead of open thread")' >> > was incomplete and didn't update a check in binder_mmap(), causing all >> > mmap() calls into the binder driver to fail. >> > >> > Signed-off-by: Martijn Coenen >> > --- >> > drivers/android/binder.c | 2 +- >> > 1 file changed, 1 insertion(+), 1 deletion(-) >> > >> > diff --git a/drivers/android/binder.c b/drivers/android/binder.c >> > index f7665c31feca..831cdd7d197d 100644 >> > --- a/drivers/android/binder.c >> > +++ b/drivers/android/binder.c >> > @@ -3362,7 +3362,7 @@ static int binder_mmap(struct file *filp, struct >> > vm_area_struct *vma) >> > const char *failure_string; >> > struct binder_buffer *buffer; >> > >> > - if (proc->tsk != current) >> > + if (proc->tsk != current->group_leader) >> > return -EINVAL; >> > >> > if ((vma->vm_end - vma->vm_start) > SZ_4M) >> >> Tested-by: John Stultz >> >> As Amit already confirmed, this resolves the wifi and bluetooth >> regression I was seeing with Android using 4.13-rc2. >> >> Though I've not seen it show up in Linus' tree yet, so I wanted to >> pester folks so it gets into 4.13-rc (its given me some slight grief >> trying to bisect down a separate issue). > > I will queue this up in the next few days, I need to resolve the patches > that have been sent to me for this, sorry for the delay. No worries. Just wanted to make sure it hadn't gotten lost. thanks -john ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 3/3] ANDROID: binder: fix proc->tsk check.
On Fri, Jul 28, 2017 at 4:56 AM, Martijn Coenen wrote: > Commit c4ea41ba195d ("binder: use group leader instead of open thread")' > was incomplete and didn't update a check in binder_mmap(), causing all > mmap() calls into the binder driver to fail. > > Signed-off-by: Martijn Coenen > --- > drivers/android/binder.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/android/binder.c b/drivers/android/binder.c > index f7665c31feca..831cdd7d197d 100644 > --- a/drivers/android/binder.c > +++ b/drivers/android/binder.c > @@ -3362,7 +3362,7 @@ static int binder_mmap(struct file *filp, struct > vm_area_struct *vma) > const char *failure_string; > struct binder_buffer *buffer; > > - if (proc->tsk != current) > + if (proc->tsk != current->group_leader) > return -EINVAL; > > if ((vma->vm_end - vma->vm_start) > SZ_4M) Tested-by: John Stultz As Amit already confirmed, this resolves the wifi and bluetooth regression I was seeing with Android using 4.13-rc2. Though I've not seen it show up in Linus' tree yet, so I wanted to pester folks so it gets into 4.13-rc (its given me some slight grief trying to bisect down a separate issue). thanks -john ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 02/37] binder: use group leader instead of open thread
On Mon, Jul 24, 2017 at 2:23 PM, Greg Kroah-Hartman wrote: > On Mon, Jul 24, 2017 at 02:00:45PM -0700, John Stultz wrote: >> On Thu, Jun 29, 2017 at 12:01 PM, Todd Kjos wrote: >> > The binder allocator assumes that the thread that >> > called binder_open will never die for the lifetime of >> > that proc. That thread is normally the group_leader, >> > however it may not be. Use the group_leader instead >> > of current. >> > >> > Signed-off-by: Todd Kjos >> > --- >> > drivers/android/binder.c | 4 ++-- >> > 1 file changed, 2 insertions(+), 2 deletions(-) >> > >> > diff --git a/drivers/android/binder.c b/drivers/android/binder.c >> > index 157bd3e49ff4..9393924ae8e8 100644 >> > --- a/drivers/android/binder.c >> > +++ b/drivers/android/binder.c >> > @@ -3460,8 +3460,8 @@ static int binder_open(struct inode *nodp, struct >> > file *filp) >> > proc = kzalloc(sizeof(*proc), GFP_KERNEL); >> > if (proc == NULL) >> > return -ENOMEM; >> > - get_task_struct(current); >> > - proc->tsk = current; >> > + get_task_struct(current->group_leader); >> > + proc->tsk = current->group_leader; >> > INIT_LIST_HEAD(&proc->todo); >> > init_waitqueue_head(&proc->wait); >> > proc->default_priority = task_nice(current); >> > -- >> >> So this patch landed in 4.13-rc2 (c4ea41ba195d), and seems to be >> causing a regression for me w/ HiKey. With it, I'm getting crashes >> with the bluetooth and wifi HALs. Reverting this patch seems to >> resolve the issue >> >> I suspect some other dependency from the original patchset is missing? > > If you use linux-next (or all of the patches in this series), does the > problem go away? I had tested awhile back the entire set from Todd, and didn't see this issue. I'll try to find some time to give -next a spin, but it might not be today. thanks -john ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 02/37] binder: use group leader instead of open thread
On Mon, Jul 24, 2017 at 2:00 PM, John Stultz wrote: > On Thu, Jun 29, 2017 at 12:01 PM, Todd Kjos wrote: >> The binder allocator assumes that the thread that >> called binder_open will never die for the lifetime of >> that proc. That thread is normally the group_leader, >> however it may not be. Use the group_leader instead >> of current. >> >> Signed-off-by: Todd Kjos >> --- >> drivers/android/binder.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/android/binder.c b/drivers/android/binder.c >> index 157bd3e49ff4..9393924ae8e8 100644 >> --- a/drivers/android/binder.c >> +++ b/drivers/android/binder.c >> @@ -3460,8 +3460,8 @@ static int binder_open(struct inode *nodp, struct file >> *filp) >> proc = kzalloc(sizeof(*proc), GFP_KERNEL); >> if (proc == NULL) >> return -ENOMEM; >> - get_task_struct(current); >> - proc->tsk = current; >> + get_task_struct(current->group_leader); >> + proc->tsk = current->group_leader; >> INIT_LIST_HEAD(&proc->todo); >> init_waitqueue_head(&proc->wait); >> proc->default_priority = task_nice(current); >> -- > > So this patch landed in 4.13-rc2 (c4ea41ba195d), and seems to be > causing a regression for me w/ HiKey. With it, I'm getting crashes > with the bluetooth and wifi HALs. Reverting this patch seems to > resolve the issue > > I suspect some other dependency from the original patchset is missing? As for the crash, the logcat details show: 12-31 16:00:36.632 2518 2584 E hw-ProcessState: Using /dev/hwbinder failed: unable to mmap transaction memory. 12-31 16:00:36.632 2518 2566 D bt_hci : hci_module_start_up starting async portion 12-31 16:00:36.632 2518 2584 E android.hardware.bluetooth@1.0::BluetoothHci: getService: defaultServiceManager()->getTransport returns Status(EX_TRANSACTION_FAILED): '-9 Bad file descriptor: ' 12-31 16:00:36.633 2518 2584 F : [1231/160036:FATAL:hci_layer_android.cc(109)] Check failed: btHci != nullptr. 12-31 16:00:36.634 2518 2584 F libc: Fatal signal 6 (SIGABRT), code -6 in tid 2584 (hci_thread) ... 12-31 16:00:38.027 2009 2061 E SupplicantStaIfaceHal: Exception while trying to register a listener for ISupplicant service: android.os.RemoteException: HwBinder Error: (-2147483648) 12-31 16:00:38.027 2009 2061 E WifiMonitor: startMonitoring(wlan0) failed! 12-31 16:00:38.028 2009 2061 E SupplicantStaIfaceHal: Can't call setDebugParams, ISupplicant is null 12-31 16:00:38.030 2009 2061 D WifiConfigStore: Reading from stores completed in 2 ms. 12-31 16:00:38.034 2009 2061 D WIFI: Registering NetworkFactory 12-31 16:00:38.035 2009 2061 D WIFI_UT : Registering NetworkFactory 12-31 16:00:38.035 2009 2065 D ConnectivityService: Got NetworkFactory Messenger for WIFI 12-31 16:00:38.035 2009 2065 D ConnectivityService: Got NetworkFactory Messenger for WIFI_UT 12-31 16:00:38.037 2009 2061 D WifiConfigStore: Reading from user store completed in 2 ms. 12-31 16:00:38.055 2009 2061 D WifiConfigStore: Writing to stores completed in 17 ms. 12-31 16:00:38.055 2009 2061 E WifiStateMachine: Failed to setup control channel, restart supplicant ... etc. thanks -john ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 02/37] binder: use group leader instead of open thread
On Thu, Jun 29, 2017 at 12:01 PM, Todd Kjos wrote: > The binder allocator assumes that the thread that > called binder_open will never die for the lifetime of > that proc. That thread is normally the group_leader, > however it may not be. Use the group_leader instead > of current. > > Signed-off-by: Todd Kjos > --- > drivers/android/binder.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/android/binder.c b/drivers/android/binder.c > index 157bd3e49ff4..9393924ae8e8 100644 > --- a/drivers/android/binder.c > +++ b/drivers/android/binder.c > @@ -3460,8 +3460,8 @@ static int binder_open(struct inode *nodp, struct file > *filp) > proc = kzalloc(sizeof(*proc), GFP_KERNEL); > if (proc == NULL) > return -ENOMEM; > - get_task_struct(current); > - proc->tsk = current; > + get_task_struct(current->group_leader); > + proc->tsk = current->group_leader; > INIT_LIST_HEAD(&proc->todo); > init_waitqueue_head(&proc->wait); > proc->default_priority = task_nice(current); > -- So this patch landed in 4.13-rc2 (c4ea41ba195d), and seems to be causing a regression for me w/ HiKey. With it, I'm getting crashes with the bluetooth and wifi HALs. Reverting this patch seems to resolve the issue I suspect some other dependency from the original patchset is missing? thanks -john ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 04/12] fs: ceph: CURRENT_TIME with ktime_get_real_ts()
On Thu, Jun 1, 2017 at 5:26 PM, Yan, Zheng wrote: > On Thu, Jun 1, 2017 at 6:22 PM, Arnd Bergmann wrote: >> On Thu, Jun 1, 2017 at 11:56 AM, Yan, Zheng wrote: >>> On Sat, Apr 8, 2017 at 8:57 AM, Deepa Dinamani >>> wrote: >> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c index 517838b..77204da 100644 --- a/drivers/block/rbd.c +++ b/drivers/block/rbd.c @@ -1922,7 +1922,7 @@ static void rbd_osd_req_format_write(struct rbd_obj_request *obj_request) { struct ceph_osd_request *osd_req = obj_request->osd_req; - osd_req->r_mtime = CURRENT_TIME; + ktime_get_real_ts(&osd_req->r_mtime); osd_req->r_data_offset = obj_request->offset; } diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c index c681762..1d3fa90 100644 --- a/fs/ceph/mds_client.c +++ b/fs/ceph/mds_client.c @@ -1666,6 +1666,7 @@ struct ceph_mds_request * ceph_mdsc_create_request(struct ceph_mds_client *mdsc, int op, int mode) { struct ceph_mds_request *req = kzalloc(sizeof(*req), GFP_NOFS); + struct timespec ts; if (!req) return ERR_PTR(-ENOMEM); @@ -1684,7 +1685,8 @@ ceph_mdsc_create_request(struct ceph_mds_client *mdsc, int op, int mode) init_completion(&req->r_safe_completion); INIT_LIST_HEAD(&req->r_unsafe_item); - req->r_stamp = current_fs_time(mdsc->fsc->sb); + ktime_get_real_ts(&ts); + req->r_stamp = timespec_trunc(ts, mdsc->fsc->sb->s_time_gran); >>> >>> This change causes our kernel_untar_tar test case to fail (inode's >>> ctime goes back). The reason is that there is time drift between the >>> time stamps got by ktime_get_real_ts() and current_time(). We need to >>> revert this change until current_time() uses ktime_get_real_ts() >>> internally. >> >> Hmm, the change was not supposed to have a user-visible effect, so >> something has gone wrong, but I don't immediately see how it >> relates to what you observe. >> >> ktime_get_real_ts() and current_time() use the same time base, there >> is no drift, but there is a difference in resolution, as the latter uses >> the time stamp of the last jiffies update, which may be up to one jiffy >> (10ms) behind the exact time we put in the request stamps here. >> >> Do you still see problems if you use current_kernel_time() instead of >> ktime_get_real_ts()? > > The problem disappears after using current_kernel_time(). > > https://github.com/ceph/ceph-client/commit/2e0f648da23167034a3cf1500bc90ec60aef2417 >From the commit above: "It seems there is time drift between ktime_get_real_ts() and current_kernel_time()" Its more of a granularity difference. current_kernel_time() returns the cached time at the last tick, where as ktime_get_real_ts() reads the clocksource hardware and returns the immediate time. Filesystems usually use the cached time (similar to CLOCK_REALTIME_COARSE), for performance reasons, as touching the clocksource takes time. thanks -john ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] android: binder: check result of binder_get_thread() in binder_poll()
On Mon, May 8, 2017 at 1:43 PM, Dmitry Torokhov wrote: > If binder_get_thread() fails to give us a thread data, we should avoid > dereferencing a NULL pointer and return POLLERR instead. > > Signed-off-by: Dmitry Torokhov Pulling Todd Kjos in on this too. -john > --- > drivers/android/binder.c | 12 > 1 file changed, 8 insertions(+), 4 deletions(-) > > diff --git a/drivers/android/binder.c b/drivers/android/binder.c > index aae4d8d4be36..66ed714fedd5 100644 > --- a/drivers/android/binder.c > +++ b/drivers/android/binder.c > @@ -3103,18 +3103,22 @@ static unsigned int binder_poll(struct file *filp, > struct poll_table_struct *wait) > { > struct binder_proc *proc = filp->private_data; > - struct binder_thread *thread = NULL; > + struct binder_thread *thread; > int wait_for_proc_work; > > binder_lock(__func__); > > thread = binder_get_thread(proc); > - > - wait_for_proc_work = thread->transaction_stack == NULL && > - list_empty(&thread->todo) && thread->return_error == BR_OK; > + if (thread) > + wait_for_proc_work = thread->transaction_stack == NULL && > +list_empty(&thread->todo) && > +thread->return_error == BR_OK; > > binder_unlock(__func__); > > + if (!thread) > + return POLLERR; > + > if (wait_for_proc_work) { > if (binder_has_proc_work(proc, thread)) > return POLLIN; > -- > 2.13.0.rc1.294.g07d810a77f-goog > > > -- > Dmitry ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging, android: remove lowmemory killer from the tree
On Wed, Feb 22, 2017 at 4:01 AM, Michal Hocko wrote: > From: Michal Hocko > > Lowmemory killer is sitting in the staging tree since 2008 without any > serious interest for fixing issues brought up by the MM folks. The main > objection is that the implementation is basically broken by design: > - it hooks into slab shrinker API which is not suitable for this > purpose. lowmem_count implementation just shows this nicely. > There is no scaling based on the memory pressure and no > feedback to the generic shrinker infrastructure. > Moreover lowmem_scan is called way too often for the heavy > work it performs. > - it is not reclaim context aware - no NUMA and/or memcg > awareness. > > As the code stands right now it just adds a maintenance overhead when > core MM changes have to update lowmemorykiller.c as well. It also seems > that the alternative LMK implementation will be solely in the userspace > so this code has no perspective it seems. The staging tree is supposed > to be for a code which needs to be put in shape before it can be merged > which is not the case here obviously. So, just for context, Android does have a userland LMK daemon (using the mempressure notifiers) as you mentioned, but unfortunately I'm unaware of any devices that ship with that implementation. This is reportedly because while the mempressure notifiers provide a the signal to userspace, the work the deamon then has to do to look up per process memory usage, in order to figure out who is best to kill at that point was too costly and resulted in poor device performance. So for shipping Android devices, the LMK is still needed. However, its not critical for basic android development, as the system will function without it. Additionally I believe most vendors heavily customize the LMK in their vendor tree, so the value of having it in staging might be relatively low. It would be great however to get a discussion going here on what the ulmkd needs from the kernel in order to efficiently determine who best to kill, and how we might best implement that. thanks -john ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 3/4] hv_util: use do_adjtimex() to update system time
On Wed, Jan 4, 2017 at 9:24 AM, Vitaly Kuznetsov wrote: > With TimeSync version 4 protocol support we started updating system time > continuously through the whole lifetime of Hyper-V guests. Every 5 seconds > there is a time sample from the host which triggers do_settimeofday[64](). > While the time from the host is very accurate such adjustments may cause > issues: > - Time is jumping forward and backward, some applications may misbehave. > - In case an NTP client is run in parallel things may go south, e.g. when > an NTP client tries to adjust tick/frequency with ADJ_TICK/ADJ_FREQUENCY > the Hyper-V module will not see this changes and time will oscillate and > never converge. > - Systemd starts annoying you by printing "Time has been changed" every 5 > seconds to the system log. > > Instead of calling do_settimeofday64() we can pretend being an NTP client > and use do_adjtimex(). Do do_settimeofday64() in case the difference is too > big or ICTIMESYNCFLAG_SYNC flag was set in the request. So how does having the guest kernel (on behalf of the host) calling adjtimex internally interact with NTP clients running on the guest? The kernel sort of assumes a single user of adjtimex (having multiple clients adjusting the clock doesn't work out so well). thanks -john ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 4/4] hv_util: improve time adjustment accuracy by disabling interrupts
On Wed, Jan 4, 2017 at 9:24 AM, Vitaly Kuznetsov wrote: > If we happen to receive interrupts during hv_set_host_time() execution > our adjustments may get inaccurate. Make the whole function atomic. > Unfortunately, we can's call do_settimeofday64() with interrupts > disabled as some cross-CPU work is being done but this call happens > very rarely. > > Signed-off-by: Vitaly Kuznetsov > --- > drivers/hv/hv_util.c | 6 ++ > 1 file changed, 6 insertions(+) > > diff --git a/drivers/hv/hv_util.c b/drivers/hv/hv_util.c > index 7e97231..4e50a42 100644 > --- a/drivers/hv/hv_util.c > +++ b/drivers/hv/hv_util.c > @@ -187,6 +187,9 @@ static void hv_set_host_time(struct work_struct *work) > struct timespec64 host_ts, our_ts; > struct timex txc = {0}; > int ret; > + unsigned long flags; > + > + local_irq_save(flags); > > wrk = container_of(work, struct adj_time_work, work); > > @@ -218,6 +221,7 @@ static void hv_set_host_time(struct work_struct *work) > * ordered to sync our time by the host. > */ > if (abs(delta) > MAXPHASE || wrk->flags & ICTIMESYNCFLAG_SYNC) { > + local_irq_restore(flags); > do_settimeofday64(&host_ts); > return; > } > @@ -232,6 +236,8 @@ static void hv_set_host_time(struct work_struct *work) > ret = do_adjtimex(&txc); > if (ret) > pr_debug("Failed to adjust system time: %d\n", ret); > + > + local_irq_restore(flags); This seems like a long time to disable irqs for what your trying to do. I'd guess you really only want to disable irqs while you aquire the host and guest timestamps (so they are as close together as possible). Since the delta is then calculated, I'm not sure what disabling irqs for calling adjtimex gets you. thanks -john ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 1/4] timekeeping: export do_adjtimex() to modules
On Wed, Jan 4, 2017 at 9:24 AM, Vitaly Kuznetsov wrote: > While do_adjtimex() is available to userspace via adjtimex syscall it is > not available to modules which may want to implement in-kernel 'NTP > clients'. Hyper-V hv_utils is going to be the first one. > > Signed-off-by: Vitaly Kuznetsov > --- > kernel/time/timekeeping.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c > index da233cd..ae4f24f 100644 > --- a/kernel/time/timekeeping.c > +++ b/kernel/time/timekeeping.c > @@ -2312,6 +2312,7 @@ int do_adjtimex(struct timex *txc) > > return ret; > } > +EXPORT_SYMBOL_GPL(do_adjtimex); No real objections to this, although I do want to better understand the benefits (and drawbacks) of doing the adjtimex in the kernel driver rather then via userspace, to make sure the need is sane. thanks -john ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 2/4] hv_util: switch to using timespec64
On Wed, Jan 4, 2017 at 9:24 AM, Vitaly Kuznetsov wrote: > do_settimeofday() is deprecated, use do_settimeofday64() instead. > > Signed-off-by: Vitaly Kuznetsov Looks sane. Acked-by: John Stultz thanks -john ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2] android: binder: fix binder mmap failures
On Fri, Feb 27, 2015 at 9:44 AM, Andrey Ryabinin wrote: > binder_update_page_range() initializes only addr and size > fields in 'struct vm_struct tmp_area;' and passes it to > map_vm_area(). > > Before 71394fe50146 ("mm: vmalloc: add flag preventing guard hole allocation") > this was because map_vm_area() didn't use any other fields > in vm_struct except addr and size. > > Now get_vm_area_size() (used in map_vm_area()) reads vm_struct's > flags to determine whether vm area has guard hole or not. > > binder_update_page_range() don't initialize flags field, so > this causes following binder mmap failures: > ---[ cut here ] > WARNING: CPU: 0 PID: 1971 at mm/vmalloc.c:130 > vmap_page_range_noflush+0x119/0x144() > CPU: 0 PID: 1971 Comm: healthd Not tainted 4.0.0-rc1-00399-g7da3fdc-dirty #157 > Hardware name: ARM-Versatile Express > [] (unwind_backtrace) from [] (show_stack+0x11/0x14) > [] (show_stack) from [] (dump_stack+0x59/0x7c) > [] (dump_stack) from [] (warn_slowpath_common+0x55/0x84) > [] (warn_slowpath_common) from [] > (warn_slowpath_null+0x17/0x1c) > [] (warn_slowpath_null) from [] > (vmap_page_range_noflush+0x119/0x144) > [] (vmap_page_range_noflush) from [] > (map_vm_area+0x27/0x48) > [] (map_vm_area) from [] > (binder_update_page_range+0x12f/0x27c) > [] (binder_update_page_range) from [] > (binder_mmap+0xbf/0x1ac) > [] (binder_mmap) from [] (mmap_region+0x2eb/0x4d4) > [] (mmap_region) from [] (do_mmap_pgoff+0x1e7/0x250) > [] (do_mmap_pgoff) from [] (vm_mmap_pgoff+0x45/0x60) > [] (vm_mmap_pgoff) from [] (SyS_mmap_pgoff+0x5d/0x80) > [] (SyS_mmap_pgoff) from [] (ret_fast_syscall+0x1/0x5c) > ---[ end trace 48c2c4b9a1349e54 ]--- > binder: 1982: binder_alloc_buf failed to map page at f0e0 in kernel > binder: binder_mmap: 1982 b6bde000-b6cdc000 alloc small buf failed -12 > > Use map_kernel_range_noflush() instead of map_vm_area() as this is better > API for binder's purposes and it allows to get rid of 'vm_struct tmp_area' at > all. > > Fixes: 71394fe50146 ("mm: vmalloc: add flag preventing guard hole allocation") > Signed-off-by: Andrey Ryabinin > Reported-by: Amit Pundir > --- > Changes since v1: >- fixed ret check after map_kernel_ranges_noflush(). > > drivers/android/binder.c | 10 +- > 1 file changed, 5 insertions(+), 5 deletions(-) > > diff --git a/drivers/android/binder.c b/drivers/android/binder.c > index 33b09b6..6607f3c 100644 > --- a/drivers/android/binder.c > +++ b/drivers/android/binder.c > @@ -551,7 +551,6 @@ static int binder_update_page_range(struct binder_proc > *proc, int allocate, > { > void *page_addr; > unsigned long user_page_addr; > - struct vm_struct tmp_area; > struct page **page; > struct mm_struct *mm; > > @@ -600,10 +599,11 @@ static int binder_update_page_range(struct binder_proc > *proc, int allocate, > proc->pid, page_addr); > goto err_alloc_page_failed; > } > - tmp_area.addr = page_addr; > - tmp_area.size = PAGE_SIZE + PAGE_SIZE /* guard page? */; > - ret = map_vm_area(&tmp_area, PAGE_KERNEL, page); > - if (ret) { > + ret = map_kernel_range_noflush((unsigned long)page_addr, > + PAGE_SIZE, PAGE_KERNEL, page); > + flush_cache_vmap((unsigned long)page_addr, > + (unsigned long)page_addr + PAGE_SIZE); > + if (ret != 1) { > pr_err("%d: binder_alloc_buf failed to map page at %p > in kernel\n", >proc->pid, page_addr); > goto err_map_kernel_failed; This seems to work better. Thanks! Tested-by: John Stultz -john ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] android: binder: fix binder mmap failures
On Fri, Feb 27, 2015 at 8:30 AM, Andrey Ryabinin wrote: > binder_update_page_range() initializes only addr and size > fields in 'struct vm_struct tmp_area;' and passes it to > map_vm_area(). > > Before 71394fe50146 ("mm: vmalloc: add flag preventing guard hole allocation") > this was because map_vm_area() didn't use any other fields > in vm_struct except addr and size. > > Now get_vm_area_size() (used in map_vm_area()) reads vm_struct's > flags to determine whether vm area has guard hole or not. > > binder_update_page_range() don't initialize flags field, so > this causes following binder mmap failures: > ---[ cut here ] > WARNING: CPU: 0 PID: 1971 at mm/vmalloc.c:130 > vmap_page_range_noflush+0x119/0x144() > CPU: 0 PID: 1971 Comm: healthd Not tainted 4.0.0-rc1-00399-g7da3fdc-dirty #157 > Hardware name: ARM-Versatile Express > [] (unwind_backtrace) from [] (show_stack+0x11/0x14) > [] (show_stack) from [] (dump_stack+0x59/0x7c) > [] (dump_stack) from [] (warn_slowpath_common+0x55/0x84) > [] (warn_slowpath_common) from [] > (warn_slowpath_null+0x17/0x1c) > [] (warn_slowpath_null) from [] > (vmap_page_range_noflush+0x119/0x144) > [] (vmap_page_range_noflush) from [] > (map_vm_area+0x27/0x48) > [] (map_vm_area) from [] > (binder_update_page_range+0x12f/0x27c) > [] (binder_update_page_range) from [] > (binder_mmap+0xbf/0x1ac) > [] (binder_mmap) from [] (mmap_region+0x2eb/0x4d4) > [] (mmap_region) from [] (do_mmap_pgoff+0x1e7/0x250) > [] (do_mmap_pgoff) from [] (vm_mmap_pgoff+0x45/0x60) > [] (vm_mmap_pgoff) from [] (SyS_mmap_pgoff+0x5d/0x80) > [] (SyS_mmap_pgoff) from [] (ret_fast_syscall+0x1/0x5c) > ---[ end trace 48c2c4b9a1349e54 ]--- > binder: 1982: binder_alloc_buf failed to map page at f0e0 in kernel > binder: binder_mmap: 1982 b6bde000-b6cdc000 alloc small buf failed -12 > > Use map_kernel_range_noflush() instead of map_vm_area() as this is better > API for binder's purposes and it allows to get rid of 'vm_struct tmp_area' at > all. > > Fixes: 71394fe50146 ("mm: vmalloc: add flag preventing guard hole allocation") > Signed-off-by: Andrey Ryabinin > Reported-by: Amit Pundir > --- > drivers/android/binder.c | 8 > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/drivers/android/binder.c b/drivers/android/binder.c > index 33b09b6..a984fbb 100644 > --- a/drivers/android/binder.c > +++ b/drivers/android/binder.c > @@ -551,7 +551,6 @@ static int binder_update_page_range(struct binder_proc > *proc, int allocate, > { > void *page_addr; > unsigned long user_page_addr; > - struct vm_struct tmp_area; > struct page **page; > struct mm_struct *mm; > > @@ -600,9 +599,10 @@ static int binder_update_page_range(struct binder_proc > *proc, int allocate, > proc->pid, page_addr); > goto err_alloc_page_failed; > } > - tmp_area.addr = page_addr; > - tmp_area.size = PAGE_SIZE + PAGE_SIZE /* guard page? */; > - ret = map_vm_area(&tmp_area, PAGE_KERNEL, page); > + ret = map_kernel_range_noflush((unsigned long)page_addr, > + PAGE_SIZE, PAGE_KERNEL, page); > + flush_cache_vmap((unsigned long)page_addr, > + (unsigned long)page_addr + PAGE_SIZE); > if (ret) { > pr_err("%d: binder_alloc_buf failed to map page at %p > in kernel\n", >proc->pid, page_addr); So with this patch I don't see the warnings, but I'm still seeing: [ 11.154283] binder: 1956: binder_alloc_buf failed to map page at f034 in kernel [ 11.154916] binder: binder_mmap: 1956 b6ce-b6d0 alloc small buf failed -12 over and over. So I don't think this patch is quite right. thanks -john ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] lowmemorykiller: Avoid excessive/redundant calling of LMK
On Thu, Jan 15, 2015 at 9:03 AM, Michal Hocko wrote: > On Mon 12-01-15 21:49:14, Chintan Pandya wrote: >> The global shrinker will invoke lowmem_shrink in a loop. >> The loop will be run (total_scan_pages/batch_size) times. >> The default batch_size will be 128 which will make >> shrinker invoking 100s of times. LMK does meaningful >> work only during first 2-3 times and then rest of the >> invocations are just CPU cycle waste. Fix that by returning >> to the shrinker with SHRINK_STOP when LMK doesn't find any >> more work to do. The deciding factor here is, no process >> found in the selected LMK bucket or memory conditions are >> sane. > > lowmemory killer is broken by design and this one of the examples which > shows why. It simply doesn't fit into shrinkers concept. > > The count_object callback simply lies and tells the core that all > the reclaimable LRU pages are scanable and gives it this as a number > which the core uses for total_scan. scan_objects callback then happily > ignore nr_to_reclaim and does its one time job where it iterates over > _all_ tasks and picks up the victim and returns its rss as a return > value. This is just a subset of LRU pages of course so it continues > looping until total_scan goes down to 0 finally. > > If this really has to be a shrinker then, shouldn't it evaluate the OOM > situation in the count callback and return non zero only if OOM and then > the scan callback would kill and return nr_to_reclaim. > > Or even better wouldn't it be much better to use vmpressure to wake > up a kernel module which would simply check the situation and kill > something? > > Please do not put only cosmetic changes on top of broken concept and try > to think about a proper solution that is what staging is for AFAIU. > > The code is in this state for quite some time and I would really hate if > it got merged just because it is in staging for too long and it is used > out there. So the in-kernel low-memory-killer is hopefully on its way out. With Lollipop on some devices, Android is using the mempressure notifiers to kill processes from userland. However, not all devices have moved to this new model (and possibly some resulting performance issues are being worked out? Its not clear). So hopefully we can drop it soon, but I'd like to make sure we don't get only a half-working solution upstream before we do remove it. thanks -john ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 1/2] timekeeping: add EXPORT_SYMBOL_GPL for do_adjtimex()
On Mon, Oct 20, 2014 at 8:18 PM, Thomas Shao wrote: > >> -Original Message- >> From: linux-kernel-ow...@vger.kernel.org [mailto:linux-kernel- >> ow...@vger.kernel.org] On Behalf Of Thomas Gleixner >> Sent: Tuesday, October 21, 2014 2:41 AM >> To: Thomas Shao >> Cc: gre...@linuxfoundation.org; LKML; de...@linuxdriverproject.org; >> o...@aepfle.de; a...@canonical.com; jasow...@redhat.com; KY Srinivasan; >> John Stultz; Richard Cochran >> Subject: Re: [PATCH v2 1/2] timekeeping: add EXPORT_SYMBOL_GPL for >> do_adjtimex() >> >> On Mon, 20 Oct 2014, Thomas Gleixner wrote: >> >> > On Wed, 15 Oct 2014, Thomas Shao wrote: >> > >> > And again you forgot to cc John Stultz on this >> > >> > > Export do_adjtimex function for hyper-v Time Synchronization >> > > component >> >> Aside of that, we really want to see the use case for this and how you >> addressed the problems which were pointed out by various folks. >> > > In some situation, the user is not able to enable guest VM to sync with > external > time source, like NTP. But the host is still synced with a trusted time > source. > In this case, host-guest time synchronization is useful. Hyper-v host will > send time > sample to guest VM every 5 seconds. We will use these time samples to adjust > guest > VM time. > > I've got some feedbacks from Richard and Mike, including reference NTP > implementation > and do the adjustment in the host side. I've already referenced some NTP > design in > my patch. I would consider my patch as a simplified implementation. I've also > considered > the host side implementation. But in host, we can only set time but not > gradually slew/adjust > time, which is not acceptable for the time sync solution.We still recommend > user to configure > NTP on the guest, which provides better accuracy. But if NTP is not > applicable, this could be > another option. > >> I still do not have a consistent argument from you WHY you need to abuse >> do_adjtimex() to do that host - guest synchronization in the first place. >> > > I need a function to gradually slew guest time. do_adjtimex() provides all the > functionality. Also I could not find any other exposed func to do this. I'd > like to > hear any feedback from you for this. Do you have any protections from both your kernel module trying to slew time if the guest is also running NTPd? That seems like it could cause some strange behavior. thanks -john ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: android: binder: move to the "real" part of the kernel
On Thu, Oct 16, 2014 at 4:12 PM, Greg Kroah-Hartman wrote: > On Thu, Oct 16, 2014 at 10:09:04AM -0700, John Stultz wrote: >> On Thu, Oct 16, 2014 at 5:47 AM, Greg Kroah-Hartman >> wrote: >> > From: Greg Kroah-Hartman >> > >> > The Android binder code has been "stable" for many years now. No matter >> >> Well, ignoring the ABI break that landed in the last year. :) > > As no one noticed, it wasn't a "break" :) > >> > This was discussed in the Android miniconf at the Plumbers conference. >> > If anyone has any objections to this, please let me know, otherwise I'm >> > queueing this up for 3.19-rc1 >> >> So my main concerns/thoughts here are: >> >> Who is going to maintain this? Has Arve agreed? > > Do we really need someone to do more work that has been done on it in > the past as an official "maintainer"? I'll be glad to do it, as I doubt > it will require any time at all. Ok. The only caution I have if Arve isn't involved is that we have in the past merged cleanup changes that introduced bugs, and it wasn't until Arve took at look at the new kernel that these were sorted out (see e194fd8a5d8e0a7eeed239a8534460724b62fe2d). So I think if at all possible getting his ack on things would be good. >> Are the Android guys comfortable with the ABI stability rules they'll >> now face? > > Just because something is in staging, doesn't mean you don't have to > follow the same ABI stability rules as the rest of the kernel. If a > change had happened to this code that broke userspace in the past, I > would have reverted it. So this should not be anything different from > what has been happening inthe past. > > And the Android developers said they were happy to see this code move > into the "real" part of the kernel. Alright then. >> Currently in the android space no one but libbinder should use the >> kernel interface. > > That is correct. If you do that, you deserve all of the pain and > suffering and rooted machines you will get. You reference this issue quite a bit, and I have a handwavy sense of the problem, but do you have a more detailed link to the specific issue here? >> Can we enforce that no one use this interface out-side of android (To >> ensure its one of those "if the abi breaks and no one notices..." edge >> cases)? > > This is the kernel, we can not dictate "use", that's the good part of > the GPLv2 :) > > Again, as no one has done this before, I strongly doubt it will happen > in the future. Well, the longer something is in the kernel, the more likely someone will depend on it. >> I'm still hopeful that a libbinder over kdbus will eventually pan out. >> I still see having two core IPC mechanisms (even if the use cases are >> different in style) as a negative, and I worry this might reduce >> motivation to unify things at the lower level. Granted, such work can >> still continue, but the incentives do change. > > Yes, things are going to change in the future, there is work happening > here, and there was a presentation at Plumbers about what is going to be > coming. > > But all of the changes will be in new code. Be it kdbus, or something > else if that doesn't work out. This existing binder.c file will not be > changing at all. This existing ABI, and codebase, is something that we > have to maintain forever for those millions of devices out there in the > real world today. So as there really is nothing left to do on it, it > deserves to be in the main part of the kernel source tree. Well, realistically, those millions of devices out there will almost *never* get a kernel version bump But yes, I'm fine with it going in. I'm just a bit surprised at how quickly thoughts change here. This does raise the question if the same standard could be held to ashmem then? That's a *much* simpler and easier to maintain chunk of code, and is just as isolated, logic wise. And while I think it would be ideal if the unpinning feature could be done more generically, if volatile ranges really doesn't have a future, then its maybe silly to hold ashmem out as well? thanks -john ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: android: binder: move to the "real" part of the kernel
On Thu, Oct 16, 2014 at 5:47 AM, Greg Kroah-Hartman wrote: > From: Greg Kroah-Hartman > > The Android binder code has been "stable" for many years now. No matter Well, ignoring the ABI break that landed in the last year. :) > what comes in the future, we are going to have to support this API, so > might as well move it to the "real" part of the kernel as there's no > real work that needs to be done to the existing code. > > Signed-off-by: Greg Kroah-Hartman > --- > > This was discussed in the Android miniconf at the Plumbers conference. > If anyone has any objections to this, please let me know, otherwise I'm > queueing this up for 3.19-rc1 So my main concerns/thoughts here are: Who is going to maintain this? Has Arve agreed? Are the Android guys comfortable with the ABI stability rules they'll now face? Currently in the android space no one but libbinder should use the kernel interface. Can we enforce that no one use this interface out-side of android (To ensure its one of those "if the abi breaks and no one notices..." edge cases)? I'm still hopeful that a libbinder over kdbus will eventually pan out. I still see having two core IPC mechanisms (even if the use cases are different in style) as a negative, and I worry this might reduce motivation to unify things at the lower level. Granted, such work can still continue, but the incentives do change. thanks -john ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 0/9] staging: ion: system heap and page pool fixes
On 05/27/2014 11:52 PM, Heesub Shin wrote: > Hi, > > Here is my patchset with some modification, hoping reviews or comments > from you guys. > > v2: > o No changes in the code, just reworded changelog > o Reorder patch Ran this through Colin's ion-unit-tests and saw no changes in the results on x86_64, i386(compat), and arm. So looks ok there. Would still be good to have Colin's ack, but here's my qualified tested-by line... (Basic Unit)Tested-by: John Stultz thanks -john ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 1/9] staging: ion: tidy up a bit
On Tue, May 27, 2014 at 11:52 AM, Greg Kroah-Hartman wrote: > On Mon, May 26, 2014 at 07:04:53PM +0900, Heesub Shin wrote: >> For aesthetics and readability, rename goto labels, remove >> useless code lines, and clarify function return type. >> >> Signed-off-by: Heesub Shin >> --- >> drivers/staging/android/ion/ion_page_pool.c | 2 +- >> drivers/staging/android/ion/ion_priv.h| 2 +- >> drivers/staging/android/ion/ion_system_heap.c | 57 >> --- >> 3 files changed, 28 insertions(+), 33 deletions(-) > > For this whole series, I need someone from the Android team, or John, to > ack them, as I have no way of testing. Having Colin's ack would be best, but I can do some basic testing when v2 is sent out. thanks -john ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging : android : uapi : fix coding style
On 04/16/2014 07:39 AM, Joe Perches wrote: > On Wed, 2014-04-16 at 23:27 +0900, Seunghun Lee wrote: >> This patch fix checkpatch.pl warnings and errors. > [] >> diff --git a/drivers/staging/android/uapi/binder.h >> b/drivers/staging/android/uapi/binder.h > [] >> @@ -169,7 +169,7 @@ struct binder_ptr_cookie { >> struct binder_handle_cookie { >> __u32 handle; >> binder_uintptr_t cookie; >> -} __attribute__((packed)); >> +} __packed; > If this .h file is meant to be a user-space #include, > then it should not use the kernel specific __packed > but keep the __attribute__((packed)) Agreed. > It does use __u32 though and that's generally > kernel specific. Hmm. Theres a ton of __u32 usage in include/uapi/* as well as typedefs for it too. include/uapi/asm-generic/int-l64.h:typedef unsigned int __u32; include/uapi/asm-generic/int-ll64.h:typedef unsigned int __u32; > John? Does any of these binder uapi files need a > bit more sorting out? I suspect this is ok, but Cc'ing Colin to give him a heads up, as it would probably cause trouble w/ their libc headers first. thanks -john ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH V2] staging: ion: replace ion_phys_addr_t with phys_addr_t
On 02/16/2014 04:00 PM, Tomas Winkler wrote: > Looks like phys_addr_t's are fully plumbed in the kernel. This needs a better commit description. Also you should include Colin and the rest of the Android kernel folks (cc'ed in this mail) so they can review and hopefully provide acks. thanks -john > > Signed-off-by: Tomas Winkler > --- > V2: remove one more comment regarding ion_phys_addr_t > > drivers/staging/android/ion/ion.c | 2 +- > drivers/staging/android/ion/ion.h | 12 +++- > drivers/staging/android/ion/ion_carveout_heap.c | 14 +++--- > drivers/staging/android/ion/ion_chunk_heap.c| 2 +- > drivers/staging/android/ion/ion_cma_heap.c | 2 +- > drivers/staging/android/ion/ion_priv.h | 10 +- > drivers/staging/android/ion/ion_system_heap.c | 2 +- > 7 files changed, 19 insertions(+), 25 deletions(-) > > diff --git a/drivers/staging/android/ion/ion.c > b/drivers/staging/android/ion/ion.c > index 14cb6c6..fb0d105 100644 > --- a/drivers/staging/android/ion/ion.c > +++ b/drivers/staging/android/ion/ion.c > @@ -547,7 +547,7 @@ void ion_free(struct ion_client *client, struct > ion_handle *handle) > EXPORT_SYMBOL(ion_free); > > int ion_phys(struct ion_client *client, struct ion_handle *handle, > - ion_phys_addr_t *addr, size_t *len) > + phys_addr_t *addr, size_t *len) > { > struct ion_buffer *buffer; > int ret; > diff --git a/drivers/staging/android/ion/ion.h > b/drivers/staging/android/ion/ion.h > index dcd2a0c..43b76f3 100644 > --- a/drivers/staging/android/ion/ion.h > +++ b/drivers/staging/android/ion/ion.h > @@ -28,12 +28,6 @@ struct ion_mapper; > struct ion_client; > struct ion_buffer; > > -/* This should be removed some day when phys_addr_t's are fully > - plumbed in the kernel, and all instances of ion_phys_addr_t should > - be converted to phys_addr_t. For the time being many kernel interfaces > - do not accept phys_addr_t's that would have to */ > -#define ion_phys_addr_t unsigned long > - > /** > * struct ion_platform_heap - defines a heap in the given platform > * @type:type of the heap from ion_heap_type enum > @@ -52,9 +46,9 @@ struct ion_platform_heap { > enum ion_heap_type type; > unsigned int id; > const char *name; > - ion_phys_addr_t base; > + phys_addr_t base; > size_t size; > - ion_phys_addr_t align; > + phys_addr_t align; > void *priv; > }; > > @@ -145,7 +139,7 @@ void ion_free(struct ion_client *client, struct > ion_handle *handle); > * holding a reference. > */ > int ion_phys(struct ion_client *client, struct ion_handle *handle, > - ion_phys_addr_t *addr, size_t *len); > + phys_addr_t *addr, size_t *len); > > /** > * ion_map_dma - return an sg_table describing a handle > diff --git a/drivers/staging/android/ion/ion_carveout_heap.c > b/drivers/staging/android/ion/ion_carveout_heap.c > index 3cb05b9..295340e 100644 > --- a/drivers/staging/android/ion/ion_carveout_heap.c > +++ b/drivers/staging/android/ion/ion_carveout_heap.c > @@ -28,10 +28,10 @@ > struct ion_carveout_heap { > struct ion_heap heap; > struct gen_pool *pool; > - ion_phys_addr_t base; > + phys_addr_t base; > }; > > -ion_phys_addr_t ion_carveout_allocate(struct ion_heap *heap, > +phys_addr_t ion_carveout_allocate(struct ion_heap *heap, > unsigned long size, > unsigned long align) > { > @@ -45,7 +45,7 @@ ion_phys_addr_t ion_carveout_allocate(struct ion_heap *heap, > return offset; > } > > -void ion_carveout_free(struct ion_heap *heap, ion_phys_addr_t addr, > +void ion_carveout_free(struct ion_heap *heap, phys_addr_t addr, > unsigned long size) > { > struct ion_carveout_heap *carveout_heap = > @@ -58,11 +58,11 @@ void ion_carveout_free(struct ion_heap *heap, > ion_phys_addr_t addr, > > static int ion_carveout_heap_phys(struct ion_heap *heap, > struct ion_buffer *buffer, > - ion_phys_addr_t *addr, size_t *len) > + phys_addr_t *addr, size_t *len) > { > struct sg_table *table = buffer->priv_virt; > struct page *page = sg_page(table->sgl); > - ion_phys_addr_t paddr = PFN_PHYS(page_to_pfn(page)); > + phys_addr_t paddr = PFN_PHYS(page_to_pfn(page)); > > *addr = paddr; > *len = buffer->size; > @@ -75,7 +75,7 @@ static int ion_carveout_heap_allocate(struct ion_heap *heap, > unsigned long flags) > { > struct sg_table *table; > - ion_phys_addr_t paddr; > + phys_addr_t paddr; > int ret; > > if (align > PAGE_SIZE) > @@ -111,7 +111,7 @@ static void ion_carveout_heap_free(struct ion_buffer > *buffer) > struct ion_heap *heap = buffer->heap; > struct sg_table *table = buffer->priv_virt; >