Re: [PATCH] staging: ion: remove from the tree

[John Stultz]

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

[John Stultz]

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

[John Stultz]

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

[John Stultz]

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

[John Stultz]

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

[John Stultz]

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

[John Stultz]

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

[John Stultz]

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

[John Stultz]

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

[John Stultz]

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

[John Stultz]

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 = <_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 = >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

[John Stultz]

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

[John Stultz]

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

[John Stultz]

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

[John Stultz]

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

[John Stultz]

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, );
> > >   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

[John Stultz]

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, );
> >   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

[John Stultz]

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

[John Stultz]

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

[John Stultz]

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

[John Stultz]

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

[John Stultz]

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

[John Stultz]

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

[John Stultz]

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 = >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();
>> +   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()

[John Stultz]

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'?

[John Stultz]

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

[John Stultz]

On Tue, Aug 22, 2017 at 8:01 PM, Jisheng Zhang <jszh...@marvell.com> wrote:
> On Tue, 22 Aug 2017 19:57:04 -0700 John Stultz wrote:
>
>> On Tue, Aug 22, 2017 at 7:56 PM, John Stultz <john.stu...@linaro.org> wrote:
>> > On Tue, Aug 22, 2017 at 7:34 PM, Jisheng Zhang <jszh...@marvell.com> 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 <jszh...@marvell.com>
>> >>> > ---
>> >>> >  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

[John Stultz]

On Tue, Aug 22, 2017 at 7:56 PM, John Stultz <john.stu...@linaro.org> wrote:
> On Tue, Aug 22, 2017 at 7:34 PM, Jisheng Zhang <jszh...@marvell.com> 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 <jszh...@marvell.com>
>>> > ---
>>> >  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

[John Stultz]

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.

[John Stultz]

On Tue, Aug 8, 2017 at 11:08 AM, Greg KH <gre...@linuxfoundation.org> 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 <m...@android.com> 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 <m...@android.com>
>> > ---
>> >  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 <john.stu...@linaro.org>
>>
>> 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.

[John Stultz]

On Fri, Jul 28, 2017 at 4:56 AM, Martijn Coenen <m...@android.com> 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 <m...@android.com>
> ---
>  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 <john.stu...@linaro.org>

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

[John Stultz]

On Mon, Jul 24, 2017 at 2:23 PM, Greg Kroah-Hartman
<gre...@linuxfoundation.org> 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 <tk...@android.com> 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 <tk...@google.com>
>> > ---
>> >  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(>todo);
>> > init_waitqueue_head(>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

[John Stultz]

On Mon, Jul 24, 2017 at 2:00 PM, John Stultz <john.stu...@linaro.org> wrote:
> On Thu, Jun 29, 2017 at 12:01 PM, Todd Kjos <tk...@android.com> 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 <tk...@google.com>
>> ---
>>  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(>todo);
>> init_waitqueue_head(>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

[John Stultz]

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(>todo);
> init_waitqueue_head(>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()

[John Stultz]

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(_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(>r_safe_completion);
 INIT_LIST_HEAD(>r_unsafe_item);

 -   req->r_stamp = current_fs_time(mdsc->fsc->sb);
 +   ktime_get_real_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()

[John Stultz]

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(>todo) && thread->return_error == BR_OK;
> +   if (thread)
> +   wait_for_proc_work = thread->transaction_stack == NULL &&
> +list_empty(>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 v2 3/4] hv_util: use do_adjtimex() to update system time

[John Stultz]

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

[John Stultz]

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(_ts);
> return;
> }
> @@ -232,6 +236,8 @@ static void hv_set_host_time(struct work_struct *work)
> ret = do_adjtimex();
> 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

[John Stultz]

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

[John Stultz]

On Wed, Jan 4, 2017 at 9:24 AM, Vitaly Kuznetsov <vkuzn...@redhat.com> wrote:
> do_settimeofday() is deprecated, use do_settimeofday64() instead.
>
> Signed-off-by: Vitaly Kuznetsov <vkuzn...@redhat.com>


Looks sane.

Acked-by: John Stultz <john.stu...@linaro.org>

thanks
-john
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] android: binder: fix binder mmap failures

[John Stultz]

On Fri, Feb 27, 2015 at 8:30 AM, Andrey Ryabinin a.ryabi...@samsung.com 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
 [c001246d] (unwind_backtrace) from [c000f7f9] (show_stack+0x11/0x14)
 [c000f7f9] (show_stack) from [c049a221] (dump_stack+0x59/0x7c)
 [c049a221] (dump_stack) from [c001cf21] (warn_slowpath_common+0x55/0x84)
 [c001cf21] (warn_slowpath_common) from [c001cfe3]
 (warn_slowpath_null+0x17/0x1c)
 [c001cfe3] (warn_slowpath_null) from [c00c66c5]
 (vmap_page_range_noflush+0x119/0x144)
 [c00c66c5] (vmap_page_range_noflush) from [c00c716b] 
 (map_vm_area+0x27/0x48)
 [c00c716b] (map_vm_area) from [c038ddaf]
 (binder_update_page_range+0x12f/0x27c)
 [c038ddaf] (binder_update_page_range) from [c038e857]
 (binder_mmap+0xbf/0x1ac)
 [c038e857] (binder_mmap) from [c00c2dc7] (mmap_region+0x2eb/0x4d4)
 [c00c2dc7] (mmap_region) from [c00c3197] (do_mmap_pgoff+0x1e7/0x250)
 [c00c3197] (do_mmap_pgoff) from [c00b35b5] (vm_mmap_pgoff+0x45/0x60)
 [c00b35b5] (vm_mmap_pgoff) from [c00c1f39] (SyS_mmap_pgoff+0x5d/0x80)
 [c00c1f39] (SyS_mmap_pgoff) from [c000ce81] (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 a.ryabi...@samsung.com
 Reported-by: Amit Pundir amit.pun...@linaro.org
 ---
  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 v2] android: binder: fix binder mmap failures

[John Stultz]

On Fri, Feb 27, 2015 at 9:44 AM, Andrey Ryabinin a.ryabi...@samsung.com 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
 [c001246d] (unwind_backtrace) from [c000f7f9] (show_stack+0x11/0x14)
 [c000f7f9] (show_stack) from [c049a221] (dump_stack+0x59/0x7c)
 [c049a221] (dump_stack) from [c001cf21] (warn_slowpath_common+0x55/0x84)
 [c001cf21] (warn_slowpath_common) from [c001cfe3]
 (warn_slowpath_null+0x17/0x1c)
 [c001cfe3] (warn_slowpath_null) from [c00c66c5]
 (vmap_page_range_noflush+0x119/0x144)
 [c00c66c5] (vmap_page_range_noflush) from [c00c716b] 
 (map_vm_area+0x27/0x48)
 [c00c716b] (map_vm_area) from [c038ddaf]
 (binder_update_page_range+0x12f/0x27c)
 [c038ddaf] (binder_update_page_range) from [c038e857]
 (binder_mmap+0xbf/0x1ac)
 [c038e857] (binder_mmap) from [c00c2dc7] (mmap_region+0x2eb/0x4d4)
 [c00c2dc7] (mmap_region) from [c00c3197] (do_mmap_pgoff+0x1e7/0x250)
 [c00c3197] (do_mmap_pgoff) from [c00b35b5] (vm_mmap_pgoff+0x45/0x60)
 [c00b35b5] (vm_mmap_pgoff) from [c00c1f39] (SyS_mmap_pgoff+0x5d/0x80)
 [c00c1f39] (SyS_mmap_pgoff) from [c000ce81] (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 a.ryabi...@samsung.com
 Reported-by: Amit Pundir amit.pun...@linaro.org
 ---
  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.stu...@linaro.org

-john
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] lowmemorykiller: Avoid excessive/redundant calling of LMK

[John Stultz]

On Thu, Jan 15, 2015 at 9:03 AM, Michal Hocko mho...@suse.cz 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()

[John Stultz]

On Mon, Oct 20, 2014 at 8:18 PM, Thomas Shao huis...@microsoft.com 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

[John Stultz]

On Thu, Oct 16, 2014 at 5:47 AM, Greg Kroah-Hartman
gre...@linuxfoundation.org wrote:
 From: Greg Kroah-Hartman gre...@linuxfoundation.org

 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 gre...@linuxfoundation.org
 ---

 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] staging: android: binder: move to the real part of the kernel

[John Stultz]

On Thu, Oct 16, 2014 at 4:12 PM, Greg Kroah-Hartman
gre...@linuxfoundation.org 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
 gre...@linuxfoundation.org wrote:
  From: Greg Kroah-Hartman gre...@linuxfoundation.org
 
  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 v2 0/9] staging: ion: system heap and page pool fixes

[John Stultz]

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 john.stu...@linaro.org

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

[John Stultz]

On Tue, May 27, 2014 at 11:52 AM, Greg Kroah-Hartman
gre...@linuxfoundation.org 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 heesub.s...@samsung.com
 ---
  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

[John Stultz]

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

[John Stultz]

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 tomas.wink...@intel.com
 ---
 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;
   struct page *page = sg_page(table-sgl);
 - ion_phys_addr_t paddr = PFN_PHYS(page_to_pfn(page));
 +