Re: Sleeping BUG in khugepaged for i586

[David Rientjes]

On Thu, 8 Jun 2017, Michal Hocko wrote:

> I would just pull the cond_resched out of __collapse_huge_page_copy
> right after pte_unmap. But I am not really sure why this cond_resched is
> really needed because the changelog of the patch which adds is is quite
> terse on details.

I'm not sure what could possibly be added to the changelog.  We have 
encountered need_resched warnings during the iteration.  We fix these 
because need_resched warnings suppress future warnings of the same type 
for issues that are more important.

I can fix the i386 issue but removing the cond_resched() entirely isn't 
really suitable.

Re: Sleeping BUG in khugepaged for i586

[David Rientjes]

On Thu, 8 Jun 2017, Michal Hocko wrote:

> I would just pull the cond_resched out of __collapse_huge_page_copy
> right after pte_unmap. But I am not really sure why this cond_resched is
> really needed because the changelog of the patch which adds is is quite
> terse on details.

I'm not sure what could possibly be added to the changelog.  We have 
encountered need_resched warnings during the iteration.  We fix these 
because need_resched warnings suppress future warnings of the same type 
for issues that are more important.

I can fix the i386 issue but removing the cond_resched() entirely isn't 
really suitable.

[git pull] IOMMU Fixes for Linux v4.12-rc4

[Joerg Roedel]

Hi Linus,

The following changes since commit 5ed02dbb497422bf225783f46e6eadd237d23d6b:

  Linux 4.12-rc3 (2017-05-28 17:20:53 -0700)

are available in the git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/joro/iommu.git 
tags/iommu-fixes-v4.12-rc4

for you to fetch changes up to d3e01c51598b2639a4830549058500e5f2ace86f:

  arm: dma-mapping: Reset the device's dma_ops (2017-05-30 11:31:34 +0200)


IOMMU Fixes for Linux v4.12-rc4

Including:

* Another compile-fix for my header cleanup

* A couple of fixes for the recently merged IOMMU probe
  deferal code

* Includes fixes for ACPI/IORT code necessary with
  IOMMU probe deferal


Arnd Bergmann (1):
  iommu/dma: Fix function declaration

Laurent Pinchart (1):
  ARM: dma-mapping: Don't tear down third-party mappings

Lorenzo Pieralisi (1):
  ACPI/IORT: Move the check to get iommu_ops from translated fwspec

Sricharan R (4):
  iommu/of: Fix check for returning EPROBE_DEFER
  iommu/of: Ignore all errors except EPROBE_DEFER
  ACPI/IORT: Ignore all errors except EPROBE_DEFER
  arm: dma-mapping: Reset the device's dma_ops

 arch/arm/include/asm/device.h |  3 ++-
 arch/arm/mm/dma-mapping.c | 29 ++---
 drivers/acpi/arm64/iort.c | 22 ++
 drivers/acpi/scan.c   |  4 ++--
 drivers/iommu/of_iommu.c  |  7 +++
 drivers/of/device.c   |  4 ++--
 include/linux/dma-iommu.h |  1 +
 7 files changed, 42 insertions(+), 28 deletions(-)

Please pull.

Thanks,

Joerg


signature.asc
Description: Digital signature

[git pull] IOMMU Fixes for Linux v4.12-rc4

[Joerg Roedel]

Hi Linus,

The following changes since commit 5ed02dbb497422bf225783f46e6eadd237d23d6b:

  Linux 4.12-rc3 (2017-05-28 17:20:53 -0700)

are available in the git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/joro/iommu.git 
tags/iommu-fixes-v4.12-rc4

for you to fetch changes up to d3e01c51598b2639a4830549058500e5f2ace86f:

  arm: dma-mapping: Reset the device's dma_ops (2017-05-30 11:31:34 +0200)


IOMMU Fixes for Linux v4.12-rc4

Including:

* Another compile-fix for my header cleanup

* A couple of fixes for the recently merged IOMMU probe
  deferal code

* Includes fixes for ACPI/IORT code necessary with
  IOMMU probe deferal


Arnd Bergmann (1):
  iommu/dma: Fix function declaration

Laurent Pinchart (1):
  ARM: dma-mapping: Don't tear down third-party mappings

Lorenzo Pieralisi (1):
  ACPI/IORT: Move the check to get iommu_ops from translated fwspec

Sricharan R (4):
  iommu/of: Fix check for returning EPROBE_DEFER
  iommu/of: Ignore all errors except EPROBE_DEFER
  ACPI/IORT: Ignore all errors except EPROBE_DEFER
  arm: dma-mapping: Reset the device's dma_ops

 arch/arm/include/asm/device.h |  3 ++-
 arch/arm/mm/dma-mapping.c | 29 ++---
 drivers/acpi/arm64/iort.c | 22 ++
 drivers/acpi/scan.c   |  4 ++--
 drivers/iommu/of_iommu.c  |  7 +++
 drivers/of/device.c   |  4 ++--
 include/linux/dma-iommu.h |  1 +
 7 files changed, 42 insertions(+), 28 deletions(-)

Please pull.

Thanks,

Joerg


signature.asc
Description: Digital signature

Re: [PATCH v4 6/7] ARM: dts: meson8: switch to new bindings for UART nodes

[Martin Blumenstingl]

Hi Neil,

On Fri, Jun 9, 2017 at 11:49 AM, Neil Armstrong  wrote:
> Switch to the stable UART bindings by adding a XTAL node and using the
> proper compatible strings.
unfortunately this won't apply now that Kevin has merged my "ARM: dts:
meson8: add and use the real clock controller"
on the other hand this will make the patch easier as you can now do
the same changes as in meson8b.dtsi

> Signed-off-by: Neil Armstrong 
> ---
>  arch/arm/boot/dts/meson8.dtsi | 23 +++
>  1 file changed, 19 insertions(+), 4 deletions(-)
>
> diff --git a/arch/arm/boot/dts/meson8.dtsi b/arch/arm/boot/dts/meson8.dtsi
> index 6993077..a2ea112 100644
> --- a/arch/arm/boot/dts/meson8.dtsi
> +++ b/arch/arm/boot/dts/meson8.dtsi
> @@ -83,6 +83,13 @@
> };
> };
>
> +   xtal: xtal-clk {
> +   compatible = "fixed-clock";
> +   clock-frequency = <2400>;
> +   clock-output-names = "xtal";
> +   #clock-cells = <0>;
> +   };
> +
> clk81: clk@0 {
> #clock-cells = <0>;
> compatible = "fixed-clock";
> @@ -199,17 +206,25 @@
>  };
>
>  _AO {
> -   clocks = <>;
> +   compatible = "amlogic,meson8-uart", "amlogic,meson-ao-uart";
> +   clocks = <>, <>, <>;
> +   clock-names = "xtal", "pclk", "baud";
>  };
>
>  _A {
> -   clocks = <>;
> +   compatible = "amlogic,meson8-uart";
> +   clocks = <>, <>, <>;
> +   clock-names = "xtal", "pclk", "baud";
>  };
>
>  _B {
> -   clocks = <>;
> +   compatible = "amlogic,meson8-uart";
> +   clocks = <>, <>, <>;
> +   clock-names = "xtal", "pclk", "baud";
>  };
>
>  _C {
> -   clocks = <>;
> +   compatible = "amlogic,meson8-uart";
> +   clocks = <>, <>, <>;
> +   clock-names = "xtal", "pclk", "baud";
>  };
> --
> 1.9.1
>
>
> ___
> linux-amlogic mailing list
> linux-amlo...@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-amlogic

Re: [PATCH v4 6/7] ARM: dts: meson8: switch to new bindings for UART nodes

[Martin Blumenstingl]

Hi Neil,

On Fri, Jun 9, 2017 at 11:49 AM, Neil Armstrong  wrote:
> Switch to the stable UART bindings by adding a XTAL node and using the
> proper compatible strings.
unfortunately this won't apply now that Kevin has merged my "ARM: dts:
meson8: add and use the real clock controller"
on the other hand this will make the patch easier as you can now do
the same changes as in meson8b.dtsi

> Signed-off-by: Neil Armstrong 
> ---
>  arch/arm/boot/dts/meson8.dtsi | 23 +++
>  1 file changed, 19 insertions(+), 4 deletions(-)
>
> diff --git a/arch/arm/boot/dts/meson8.dtsi b/arch/arm/boot/dts/meson8.dtsi
> index 6993077..a2ea112 100644
> --- a/arch/arm/boot/dts/meson8.dtsi
> +++ b/arch/arm/boot/dts/meson8.dtsi
> @@ -83,6 +83,13 @@
> };
> };
>
> +   xtal: xtal-clk {
> +   compatible = "fixed-clock";
> +   clock-frequency = <2400>;
> +   clock-output-names = "xtal";
> +   #clock-cells = <0>;
> +   };
> +
> clk81: clk@0 {
> #clock-cells = <0>;
> compatible = "fixed-clock";
> @@ -199,17 +206,25 @@
>  };
>
>  _AO {
> -   clocks = <>;
> +   compatible = "amlogic,meson8-uart", "amlogic,meson-ao-uart";
> +   clocks = <>, <>, <>;
> +   clock-names = "xtal", "pclk", "baud";
>  };
>
>  _A {
> -   clocks = <>;
> +   compatible = "amlogic,meson8-uart";
> +   clocks = <>, <>, <>;
> +   clock-names = "xtal", "pclk", "baud";
>  };
>
>  _B {
> -   clocks = <>;
> +   compatible = "amlogic,meson8-uart";
> +   clocks = <>, <>, <>;
> +   clock-names = "xtal", "pclk", "baud";
>  };
>
>  _C {
> -   clocks = <>;
> +   compatible = "amlogic,meson8-uart";
> +   clocks = <>, <>, <>;
> +   clock-names = "xtal", "pclk", "baud";
>  };
> --
> 1.9.1
>
>
> ___
> linux-amlogic mailing list
> linux-amlo...@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-amlogic

Re: [PATCH] thermal: int340x: check for sensor when PTYP is missing

[Andy Shevchenko]

On Sat, Jun 10, 2017 at 1:23 AM, Srinivas Pandruvada
 wrote:
> On Wed, 2017-06-07 at 13:55 +0300, Andy Shevchenko wrote:
>> On Wed, Jun 7, 2017 at 2:00 AM, Srinivas Pandruvada
>>  wrote:

>> > This change checks for the presence of _TMP method and if present,
>> > then
>> > treats this device as a sensor.
>> >
>> > status = acpi_evaluate_integer(priv->adev->handle, "PTYP",
>> >NULL, >type);
>> > if (ACPI_FAILURE(status)) {
>> > -   result = -EINVAL;
>> > -   goto err;
>> >
>> > +   unsigned long long tmp;
>> You may use >type as temporary variable, though I would go
>> other
>> way around:
>> declare tmp for function, then
>>
>> unsigned long long tmp;
>> ...
>> status = acpi_evaluate_integer(priv->adev->handle, "PTYP",  NULL,
>> );
>> if (ACPI_FAILURE(status)) {
>> status = acpi_evaluate_integer(priv->adev->handle, "_TMP",
>> NULL, );
>> if (ACPI_FAILURE(status)) {
>> result = -EINVAL;
>> goto err;
>> }
>> tmp = INT3403_TYPE_SENSOR;
>> }
>> priv->type = tmp;
>>
> So what are we saving by doing this way?

We make priv->type assignment in one place.

That's the difference.

So, you may choose your way of course. It's not a big deal.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] thermal: int340x: check for sensor when PTYP is missing

[Andy Shevchenko]

On Sat, Jun 10, 2017 at 1:23 AM, Srinivas Pandruvada
 wrote:
> On Wed, 2017-06-07 at 13:55 +0300, Andy Shevchenko wrote:
>> On Wed, Jun 7, 2017 at 2:00 AM, Srinivas Pandruvada
>>  wrote:

>> > This change checks for the presence of _TMP method and if present,
>> > then
>> > treats this device as a sensor.
>> >
>> > status = acpi_evaluate_integer(priv->adev->handle, "PTYP",
>> >NULL, >type);
>> > if (ACPI_FAILURE(status)) {
>> > -   result = -EINVAL;
>> > -   goto err;
>> >
>> > +   unsigned long long tmp;
>> You may use >type as temporary variable, though I would go
>> other
>> way around:
>> declare tmp for function, then
>>
>> unsigned long long tmp;
>> ...
>> status = acpi_evaluate_integer(priv->adev->handle, "PTYP",  NULL,
>> );
>> if (ACPI_FAILURE(status)) {
>> status = acpi_evaluate_integer(priv->adev->handle, "_TMP",
>> NULL, );
>> if (ACPI_FAILURE(status)) {
>> result = -EINVAL;
>> goto err;
>> }
>> tmp = INT3403_TYPE_SENSOR;
>> }
>> priv->type = tmp;
>>
> So what are we saving by doing this way?

We make priv->type assignment in one place.

That's the difference.

So, you may choose your way of course. It's not a big deal.

-- 
With Best Regards,
Andy Shevchenko

[patch v2 -mm] mm, hugetlb: schedule when potentially allocating many hugepages

[David Rientjes]

A few hugetlb allocators loop while calling the page allocator and can
potentially prevent rescheduling if the page allocator slowpath is not
utilized.

Conditionally schedule when large numbers of hugepages can be allocated.

Signed-off-by: David Rientjes 
---
 Based on -mm only to prevent merge conflicts with
 "mm/hugetlb.c: warn the user when issues arise on boot due to hugepages"

 v2: removed redundant cond_resched() per Mike

 mm/hugetlb.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/mm/hugetlb.c b/mm/hugetlb.c
--- a/mm/hugetlb.c
+++ b/mm/hugetlb.c
@@ -1754,6 +1754,7 @@ static int gather_surplus_pages(struct hstate *h, int 
delta)
break;
}
list_add(>lru, _list);
+   cond_resched();
}
allocated += i;
 
@@ -,6 +2223,7 @@ static void __init hugetlb_hstate_alloc_pages(struct 
hstate *h)
} else if (!alloc_fresh_huge_page(h,
 _states[N_MEMORY]))
break;
+   cond_resched();
}
if (i < h->max_huge_pages) {
char buf[32];

[patch v2 -mm] mm, hugetlb: schedule when potentially allocating many hugepages

[David Rientjes]

A few hugetlb allocators loop while calling the page allocator and can
potentially prevent rescheduling if the page allocator slowpath is not
utilized.

Conditionally schedule when large numbers of hugepages can be allocated.

Signed-off-by: David Rientjes 
---
 Based on -mm only to prevent merge conflicts with
 "mm/hugetlb.c: warn the user when issues arise on boot due to hugepages"

 v2: removed redundant cond_resched() per Mike

 mm/hugetlb.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/mm/hugetlb.c b/mm/hugetlb.c
--- a/mm/hugetlb.c
+++ b/mm/hugetlb.c
@@ -1754,6 +1754,7 @@ static int gather_surplus_pages(struct hstate *h, int 
delta)
break;
}
list_add(>lru, _list);
+   cond_resched();
}
allocated += i;
 
@@ -,6 +2223,7 @@ static void __init hugetlb_hstate_alloc_pages(struct 
hstate *h)
} else if (!alloc_fresh_huge_page(h,
 _states[N_MEMORY]))
break;
+   cond_resched();
}
if (i < h->max_huge_pages) {
char buf[32];

Re: [PATCH v8 14/34] ARM: dts: imx6-sabreauto: add the ADV7180 video decoder

[Steve Longerbeam]

On 06/09/2017 02:38 PM, Pavel Machek wrote:

On Thu 2017-06-08 13:36:12, Steve Longerbeam wrote:



On 06/08/2017 01:25 PM, Tim Harvey wrote:



Steve,

You need to remove the fim node now that you've moved this to V4L2 controls.



Yep, I caught this just after sending the v8 patchset. I'll send
a v9 of this patch.


This needs ack from devicetree people, then it can be merged. Can you
be a bit more forceful getting the ack?


OK, I need an Ack please, he said, in a forceful way. :)

In fact Ack's are needed for all the changes to dts sources,
patches 4-14.




I don't think it makes sense to resubmit v9 before that. This is not a
rocket science.


I guess that makes sense, I'll wait for Ack's from all these patches
before submitting the entire patchset as v9.

Steve

Re: [PATCH v8 14/34] ARM: dts: imx6-sabreauto: add the ADV7180 video decoder

[Steve Longerbeam]

On 06/09/2017 02:38 PM, Pavel Machek wrote:

On Thu 2017-06-08 13:36:12, Steve Longerbeam wrote:



On 06/08/2017 01:25 PM, Tim Harvey wrote:



Steve,

You need to remove the fim node now that you've moved this to V4L2 controls.



Yep, I caught this just after sending the v8 patchset. I'll send
a v9 of this patch.


This needs ack from devicetree people, then it can be merged. Can you
be a bit more forceful getting the ack?


OK, I need an Ack please, he said, in a forceful way. :)

In fact Ack's are needed for all the changes to dts sources,
patches 4-14.




I don't think it makes sense to resubmit v9 before that. This is not a
rocket science.


I guess that makes sense, I'll wait for Ack's from all these patches
before submitting the entire patchset as v9.

Steve

Re: [patch -mm] mm, hugetlb: schedule when potentially allocating many hugepages

[David Rientjes]

On Wed, 7 Jun 2017, Mike Kravetz wrote:

> > @@ -2364,6 +2366,7 @@ static unsigned long set_max_huge_pages(struct hstate 
> > *h, unsigned long count,
> > ret = alloc_fresh_gigantic_page(h, nodes_allowed);
> > else
> > ret = alloc_fresh_huge_page(h, nodes_allowed);
> > +   cond_resched();
> 
> Are not the following lines immediately before the above huge page allocation
> in set_max_huge_pages, or am I looking at an incorrect version of the file?
> 
>   /* yield cpu to avoid soft lockup */
>   cond_resched();

Ahh, we don't have this in our tree, thanks for catching it.  The other 
two cond_resched()'s are needed because we have reproduced them, so I'll 
send a v2.

Re: [patch -mm] mm, hugetlb: schedule when potentially allocating many hugepages

[David Rientjes]

On Wed, 7 Jun 2017, Mike Kravetz wrote:

> > @@ -2364,6 +2366,7 @@ static unsigned long set_max_huge_pages(struct hstate 
> > *h, unsigned long count,
> > ret = alloc_fresh_gigantic_page(h, nodes_allowed);
> > else
> > ret = alloc_fresh_huge_page(h, nodes_allowed);
> > +   cond_resched();
> 
> Are not the following lines immediately before the above huge page allocation
> in set_max_huge_pages, or am I looking at an incorrect version of the file?
> 
>   /* yield cpu to avoid soft lockup */
>   cond_resched();

Ahh, we don't have this in our tree, thanks for catching it.  The other 
two cond_resched()'s are needed because we have reproduced them, so I'll 
send a v2.

Re: [PATCH] mm: Drop useless local parameters of register_one_node()

[David Rientjes]

On Thu, 8 Jun 2017, Dou Liyang wrote:

> ... initializes local parameters "p_node" & "parent" for
> register_node().
> 
> But, register_node() does not use them.
> 
> Remove the related code of "parent" node, cleanup register_one_node()
> and register_node().
> 
> [Test in Qemu by 4 hotpluggable nodes in x86-64 system]
> 
> Signed-off-by: Dou Liyang 

Acked-by: David Rientjes 

Re: [PATCH] mm: Drop useless local parameters of register_one_node()

[David Rientjes]

On Thu, 8 Jun 2017, Dou Liyang wrote:

> ... initializes local parameters "p_node" & "parent" for
> register_node().
> 
> But, register_node() does not use them.
> 
> Remove the related code of "parent" node, cleanup register_one_node()
> and register_node().
> 
> [Test in Qemu by 4 hotpluggable nodes in x86-64 system]
> 
> Signed-off-by: Dou Liyang 

Acked-by: David Rientjes 

[PATCH] ARM: OMAP: PM: stop early on systems without twl

[Sebastian Reichel]

Motorola Droid 4 has an OMAP4, but no TWL6030. It currently
complains verbosely about this during boot:

twl: not initialized
twl6030_uv_to_vsel:OUT OF RANGE! non mapped vsel for 1375000 Vs max 1316660
twl6030_uv_to_vsel:OUT OF RANGE! non mapped vsel for 1375000 Vs max 1316660
twl6030_uv_to_vsel:OUT OF RANGE! non mapped vsel for 1375000 Vs max 1316660
twl6030_uv_to_vsel:OUT OF RANGE! non mapped vsel for 1375000 Vs max 1316660
twl6030_uv_to_vsel:OUT OF RANGE! non mapped vsel for 1375000 Vs max 1316660
twl6030_uv_to_vsel:OUT OF RANGE! non mapped vsel for 1375000 Vs max 1316660
twl6030_uv_to_vsel:OUT OF RANGE! non mapped vsel for 141 Vs max 1316660
omap2_set_init_voltage: unable to find boot up OPP for vdd_core
omap2_set_init_voltage: unable to set vdd_core
omap2_set_init_voltage: unable to find boot up OPP for vdd_iva
omap2_set_init_voltage: unable to set vdd_iva

While proper support for CPCAP should be added at some point,
let's exit early in omap2_common_pm_late_init() until that
has been implemented to avoid the above errors. There is still
a reminder about missing PM in dmesg:

Missing OMAP4 PM for this platform!

Signed-off-by: Sebastian Reichel 
---
 arch/arm/mach-omap2/pm.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/arm/mach-omap2/pm.c b/arch/arm/mach-omap2/pm.c
index 63027e60cc20..4e4313cecc26 100644
--- a/arch/arm/mach-omap2/pm.c
+++ b/arch/arm/mach-omap2/pm.c
@@ -240,6 +240,9 @@ omap_postcore_initcall(omap2_common_pm_init);
 
 int __init omap2_common_pm_late_init(void)
 {
+   if (!twl_rev())
+   return -ENODEV;
+
/* Init the voltage layer */
omap3_twl_init();
omap4_twl_init();
-- 
2.11.0

[PATCH] ARM: OMAP: PM: stop early on systems without twl

[Sebastian Reichel]

Motorola Droid 4 has an OMAP4, but no TWL6030. It currently
complains verbosely about this during boot:

twl: not initialized
twl6030_uv_to_vsel:OUT OF RANGE! non mapped vsel for 1375000 Vs max 1316660
twl6030_uv_to_vsel:OUT OF RANGE! non mapped vsel for 1375000 Vs max 1316660
twl6030_uv_to_vsel:OUT OF RANGE! non mapped vsel for 1375000 Vs max 1316660
twl6030_uv_to_vsel:OUT OF RANGE! non mapped vsel for 1375000 Vs max 1316660
twl6030_uv_to_vsel:OUT OF RANGE! non mapped vsel for 1375000 Vs max 1316660
twl6030_uv_to_vsel:OUT OF RANGE! non mapped vsel for 1375000 Vs max 1316660
twl6030_uv_to_vsel:OUT OF RANGE! non mapped vsel for 141 Vs max 1316660
omap2_set_init_voltage: unable to find boot up OPP for vdd_core
omap2_set_init_voltage: unable to set vdd_core
omap2_set_init_voltage: unable to find boot up OPP for vdd_iva
omap2_set_init_voltage: unable to set vdd_iva

While proper support for CPCAP should be added at some point,
let's exit early in omap2_common_pm_late_init() until that
has been implemented to avoid the above errors. There is still
a reminder about missing PM in dmesg:

Missing OMAP4 PM for this platform!

Signed-off-by: Sebastian Reichel 
---
 arch/arm/mach-omap2/pm.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/arm/mach-omap2/pm.c b/arch/arm/mach-omap2/pm.c
index 63027e60cc20..4e4313cecc26 100644
--- a/arch/arm/mach-omap2/pm.c
+++ b/arch/arm/mach-omap2/pm.c
@@ -240,6 +240,9 @@ omap_postcore_initcall(omap2_common_pm_init);
 
 int __init omap2_common_pm_late_init(void)
 {
+   if (!twl_rev())
+   return -ENODEV;
+
/* Init the voltage layer */
omap3_twl_init();
omap4_twl_init();
-- 
2.11.0

Re: [PATCH 1/4] dt-bindings: tps65217: Update binding documentation.

[Enric Balletbo Serra]

Hello Rob,

2017-06-09 16:03 GMT+02:00 Rob Herring :
> On Wed, Jun 07, 2017 at 12:32:39PM +0200, Enric Balletbo i Serra wrote:
>> This patch adds a new binding documentation for the TPS65217 MFD and
>> updates the documentation for all the sub-devices in accordance to get
>> each individual sub-driver functioning correctly.
>
> Explain why breaking compatibility is okay.
>

We had some discussion in patch 4 that make me rethink a bit all this,
please let me send a new version and continue the discussion there
(now I'm not sure if I'll break the compatibility or not)

But let me ask a question. The TPS65217 MFD has different sub-nodes
(charger, backlight, pwrbutton, regulators) in DT, I suspect the
answer is no, but is it ok that some of them were not described in the
DT because there is nothing to configure?

i.e: Have this because the resources of charger and pwrbutton are
static so can be hard-coded in the driver

 {
compatible = "ti,tps65217";
interrupt-controller;
#interrupt-cells = <1>;

regulators {
#address-cells = <1>;
#size-cells = <0>;

dcdc1_reg: regulator@0 {
reg = <0>;
...
   };
};

instead of :

 {
compatible = "ti,tps65217";
interrupt-controller;
#interrupt-cells = <1>;

charger {
compatible = "ti,tps65217-charger";
interrupts = <0>, <1>;
interrupt-names = "USB", "AC";
};

pwrbutton {
compatible = "ti,tps65217-pwrbutton";
interrupts = <2>;
};

regulators {
#address-cells = <1>;
#size-cells = <0>;

dcdc1_reg: regulator@0 {
reg = <0>;
...
   };
};

Best regards,
 Enric

>> Signed-off-by: Enric Balletbo i Serra 
>> ---
>>  .../bindings/input/tps65218-pwrbutton.txt  |   2 +-
>>  .../bindings/leds/backlight/tps65217-backlight.txt |  24 ++---
>>  Documentation/devicetree/bindings/mfd/tps65217.txt | 100 
>> +
>>  .../devicetree/bindings/regulator/tps65217.txt |   8 +-
>>  4 files changed, 119 insertions(+), 15 deletions(-)
>>  create mode 100644 Documentation/devicetree/bindings/mfd/tps65217.txt
>>
>> diff --git a/Documentation/devicetree/bindings/input/tps65218-pwrbutton.txt 
>> b/Documentation/devicetree/bindings/input/tps65218-pwrbutton.txt
>> index 8682ab6..603a3f0 100644
>> --- a/Documentation/devicetree/bindings/input/tps65218-pwrbutton.txt
>> +++ b/Documentation/devicetree/bindings/input/tps65218-pwrbutton.txt
>> @@ -1,7 +1,7 @@
>>  Texas Instruments TPS65217 and TPS65218 power button
>>
>>  This module is part of the TPS65217/TPS65218. For more details about the 
>> whole
>> -TPS65217 chip see Documentation/devicetree/bindings/regulator/tps65217.txt.
>> +TPS65217 chip see Documentation/devicetree/bindings/mfd/tps65217.txt.
>>
>>  This driver provides a simple power button event via an Interrupt.
>>
>> diff --git 
>> a/Documentation/devicetree/bindings/leds/backlight/tps65217-backlight.txt 
>> b/Documentation/devicetree/bindings/leds/backlight/tps65217-backlight.txt
>> index 5fb9279..a1bc465 100644
>> --- a/Documentation/devicetree/bindings/leds/backlight/tps65217-backlight.txt
>> +++ b/Documentation/devicetree/bindings/leds/backlight/tps65217-backlight.txt
>> @@ -1,11 +1,13 @@
>> -TPS65217 family of regulators
>> +Texas Instruments TPS65217 backlight regulator
>> +
>> +This module is part of the TPS65217. For more details about the whole
>> +TPS65217 chip see Documentation/devicetree/bindings/mfd/tps65217.txt.
>>
>>  The TPS65217 chip contains a boost converter and current sinks which can be
>>  used to drive LEDs for use as backlights.
>>
>>  Required properties:
>> -- compatible: "ti,tps65217"
>> -- reg: I2C slave address
>> +- compatible: "ti,tps65217-bl"
>>  - backlight: node for specifying WLED1 and WLED2 lines in TPS65217
>>  - isel: selection bit, valid values: 1 for ISEL1 (low-level) and 2 for 
>> ISEL2 (high-level)
>>  - fdim: PWM dimming frequency, valid values: 100, 200, 500, 1000
>> @@ -15,13 +17,13 @@ Each regulator is defined using the standard binding for 
>> regulators.
>>
>>  Example:
>>
>> - tps: tps@24 {
>> - reg = <0x24>;
>> - compatible = "ti,tps65217";
>> - backlight {
>> - isel = <1>;  /* 1 - ISET1, 2 ISET2 */
>> - fdim = <100>; /* TPS65217_BL_FDIM_100HZ */
>> - default-brightness = <50>;
>> - };
>> + {
>> + backlight {
>> + compatible = "ti,tps65217-bl";
>> + status = "okay";
>> + isel = <1>;  /* 1 - ISET1, 2 ISET2 */
>> + fdim = <100>; /* TPS65217_BL_FDIM_100HZ */
>> + default-brightness = <50>;
>>   };
>> +};
>>
>> diff --git 

Re: [PATCH 1/4] dt-bindings: tps65217: Update binding documentation.

[Enric Balletbo Serra]

Hello Rob,

2017-06-09 16:03 GMT+02:00 Rob Herring :
> On Wed, Jun 07, 2017 at 12:32:39PM +0200, Enric Balletbo i Serra wrote:
>> This patch adds a new binding documentation for the TPS65217 MFD and
>> updates the documentation for all the sub-devices in accordance to get
>> each individual sub-driver functioning correctly.
>
> Explain why breaking compatibility is okay.
>

We had some discussion in patch 4 that make me rethink a bit all this,
please let me send a new version and continue the discussion there
(now I'm not sure if I'll break the compatibility or not)

But let me ask a question. The TPS65217 MFD has different sub-nodes
(charger, backlight, pwrbutton, regulators) in DT, I suspect the
answer is no, but is it ok that some of them were not described in the
DT because there is nothing to configure?

i.e: Have this because the resources of charger and pwrbutton are
static so can be hard-coded in the driver

 {
compatible = "ti,tps65217";
interrupt-controller;
#interrupt-cells = <1>;

regulators {
#address-cells = <1>;
#size-cells = <0>;

dcdc1_reg: regulator@0 {
reg = <0>;
...
   };
};

instead of :

 {
compatible = "ti,tps65217";
interrupt-controller;
#interrupt-cells = <1>;

charger {
compatible = "ti,tps65217-charger";
interrupts = <0>, <1>;
interrupt-names = "USB", "AC";
};

pwrbutton {
compatible = "ti,tps65217-pwrbutton";
interrupts = <2>;
};

regulators {
#address-cells = <1>;
#size-cells = <0>;

dcdc1_reg: regulator@0 {
reg = <0>;
...
   };
};

Best regards,
 Enric

>> Signed-off-by: Enric Balletbo i Serra 
>> ---
>>  .../bindings/input/tps65218-pwrbutton.txt  |   2 +-
>>  .../bindings/leds/backlight/tps65217-backlight.txt |  24 ++---
>>  Documentation/devicetree/bindings/mfd/tps65217.txt | 100 
>> +
>>  .../devicetree/bindings/regulator/tps65217.txt |   8 +-
>>  4 files changed, 119 insertions(+), 15 deletions(-)
>>  create mode 100644 Documentation/devicetree/bindings/mfd/tps65217.txt
>>
>> diff --git a/Documentation/devicetree/bindings/input/tps65218-pwrbutton.txt 
>> b/Documentation/devicetree/bindings/input/tps65218-pwrbutton.txt
>> index 8682ab6..603a3f0 100644
>> --- a/Documentation/devicetree/bindings/input/tps65218-pwrbutton.txt
>> +++ b/Documentation/devicetree/bindings/input/tps65218-pwrbutton.txt
>> @@ -1,7 +1,7 @@
>>  Texas Instruments TPS65217 and TPS65218 power button
>>
>>  This module is part of the TPS65217/TPS65218. For more details about the 
>> whole
>> -TPS65217 chip see Documentation/devicetree/bindings/regulator/tps65217.txt.
>> +TPS65217 chip see Documentation/devicetree/bindings/mfd/tps65217.txt.
>>
>>  This driver provides a simple power button event via an Interrupt.
>>
>> diff --git 
>> a/Documentation/devicetree/bindings/leds/backlight/tps65217-backlight.txt 
>> b/Documentation/devicetree/bindings/leds/backlight/tps65217-backlight.txt
>> index 5fb9279..a1bc465 100644
>> --- a/Documentation/devicetree/bindings/leds/backlight/tps65217-backlight.txt
>> +++ b/Documentation/devicetree/bindings/leds/backlight/tps65217-backlight.txt
>> @@ -1,11 +1,13 @@
>> -TPS65217 family of regulators
>> +Texas Instruments TPS65217 backlight regulator
>> +
>> +This module is part of the TPS65217. For more details about the whole
>> +TPS65217 chip see Documentation/devicetree/bindings/mfd/tps65217.txt.
>>
>>  The TPS65217 chip contains a boost converter and current sinks which can be
>>  used to drive LEDs for use as backlights.
>>
>>  Required properties:
>> -- compatible: "ti,tps65217"
>> -- reg: I2C slave address
>> +- compatible: "ti,tps65217-bl"
>>  - backlight: node for specifying WLED1 and WLED2 lines in TPS65217
>>  - isel: selection bit, valid values: 1 for ISEL1 (low-level) and 2 for 
>> ISEL2 (high-level)
>>  - fdim: PWM dimming frequency, valid values: 100, 200, 500, 1000
>> @@ -15,13 +17,13 @@ Each regulator is defined using the standard binding for 
>> regulators.
>>
>>  Example:
>>
>> - tps: tps@24 {
>> - reg = <0x24>;
>> - compatible = "ti,tps65217";
>> - backlight {
>> - isel = <1>;  /* 1 - ISET1, 2 ISET2 */
>> - fdim = <100>; /* TPS65217_BL_FDIM_100HZ */
>> - default-brightness = <50>;
>> - };
>> + {
>> + backlight {
>> + compatible = "ti,tps65217-bl";
>> + status = "okay";
>> + isel = <1>;  /* 1 - ISET1, 2 ISET2 */
>> + fdim = <100>; /* TPS65217_BL_FDIM_100HZ */
>> + default-brightness = <50>;
>>   };
>> +};
>>
>> diff --git a/Documentation/devicetree/bindings/mfd/tps65217.txt 
>> 

Re: [PATCH] thermal: int340x: check for sensor when PTYP is missing

[Srinivas Pandruvada]

On Wed, 2017-06-07 at 13:55 +0300, Andy Shevchenko wrote:
> On Wed, Jun 7, 2017 at 2:00 AM, Srinivas Pandruvada
>  wrote:
> > 
> > For INT3403 sensor PTYP field is mandatory. But some platforms
> > didn't
> > have this field for sensors. This cause load failure for int3403
> > driver.
> > 
> > This change checks for the presence of _TMP method and if present,
> > then
> > treats this device as a sensor.
> > 
> > status = acpi_evaluate_integer(priv->adev->handle, "PTYP",
> >    NULL, >type);
> > if (ACPI_FAILURE(status)) {
> > -   result = -EINVAL;
> > -   goto err;
> > 
> > +   unsigned long long tmp;
> You may use >type as temporary variable, though I would go
> other
> way around:
> declare tmp for function, then
> 
> unsigned long long tmp;
> ...
> status = acpi_evaluate_integer(priv->adev->handle, "PTYP",  NULL,
> );
> if (ACPI_FAILURE(status)) {
> status = acpi_evaluate_integer(priv->adev->handle, "_TMP",
> NULL, );
> if (ACPI_FAILURE(status)) {
> result = -EINVAL;
> goto err;
> }
> tmp = INT3403_TYPE_SENSOR;
> }
> priv->type = tmp;
> 
So what are we saving by doing this way?

Thanks,
Srinivas

> > 
> > +   } else {
> This is redundant.
> 
> > 
> > +   priv->type = INT3403_TYPE_SENSOR;
> > +   }
> > }

Re: [PATCH] thermal: int340x: check for sensor when PTYP is missing

[Srinivas Pandruvada]

On Wed, 2017-06-07 at 13:55 +0300, Andy Shevchenko wrote:
> On Wed, Jun 7, 2017 at 2:00 AM, Srinivas Pandruvada
>  wrote:
> > 
> > For INT3403 sensor PTYP field is mandatory. But some platforms
> > didn't
> > have this field for sensors. This cause load failure for int3403
> > driver.
> > 
> > This change checks for the presence of _TMP method and if present,
> > then
> > treats this device as a sensor.
> > 
> > status = acpi_evaluate_integer(priv->adev->handle, "PTYP",
> >    NULL, >type);
> > if (ACPI_FAILURE(status)) {
> > -   result = -EINVAL;
> > -   goto err;
> > 
> > +   unsigned long long tmp;
> You may use >type as temporary variable, though I would go
> other
> way around:
> declare tmp for function, then
> 
> unsigned long long tmp;
> ...
> status = acpi_evaluate_integer(priv->adev->handle, "PTYP",  NULL,
> );
> if (ACPI_FAILURE(status)) {
> status = acpi_evaluate_integer(priv->adev->handle, "_TMP",
> NULL, );
> if (ACPI_FAILURE(status)) {
> result = -EINVAL;
> goto err;
> }
> tmp = INT3403_TYPE_SENSOR;
> }
> priv->type = tmp;
> 
So what are we saving by doing this way?

Thanks,
Srinivas

> > 
> > +   } else {
> This is redundant.
> 
> > 
> > +   priv->type = INT3403_TYPE_SENSOR;
> > +   }
> > }

Re: [PATCH 4/7] iommu: Add driver-not-bound notification

[Joerg Roedel]

On Fri, Jun 09, 2017 at 03:59:59PM -0600, Alex Williamson wrote:
> The driver core supports a BUS_NOTIFY_DRIVER_NOT_BOUND notification
> sent if a driver fails to bind to a device.  Extend IOMMU group
> notifications to include a version of this.
> 
> Signed-off-by: Alex Williamson 
> Cc: Joerg Roedel 

Since this is part of a larger patch-set I assume it won't go through
iommu-tree, so:

Acked-by: Joerg Roedel 

Re: [PATCH 4/7] iommu: Add driver-not-bound notification

[Joerg Roedel]

On Fri, Jun 09, 2017 at 03:59:59PM -0600, Alex Williamson wrote:
> The driver core supports a BUS_NOTIFY_DRIVER_NOT_BOUND notification
> sent if a driver fails to bind to a device.  Extend IOMMU group
> notifications to include a version of this.
> 
> Signed-off-by: Alex Williamson 
> Cc: Joerg Roedel 

Since this is part of a larger patch-set I assume it won't go through
iommu-tree, so:

Acked-by: Joerg Roedel 

RE: [PATCH v1 3/3] ACPI / PMIC: Add TI PMIC TPS68470 operation region driver

[Mani, Rajmohan]

Hi Hans,

> -Original Message-
> From: Hans de Goede [mailto:hdego...@redhat.com]
> Sent: Tuesday, June 06, 2017 8:22 AM
> To: Andy Shevchenko ; Mani, Rajmohan
> 
> Cc: linux-kernel@vger.kernel.org; linux-g...@vger.kernel.org; linux-
> a...@vger.kernel.org; Lee Jones ; Linus Walleij
> ; Alexandre Courbot ; Rafael J.
> Wysocki ; Len Brown 
> Subject: Re: [PATCH v1 3/3] ACPI / PMIC: Add TI PMIC TPS68470 operation
> region driver
> 
> Hi,
> 
> On 06/06/2017 04:23 PM, Andy Shevchenko wrote:
> > +Cc Hans (that's why didn't delete anything from original mail, just
> > adding my comments).
> >
> > Hans, if you have few minutes it would be appreciated to glance on the
> > below for some issues if any since you did pass quite a good quest
> > with other PMIC drivers.
> 
> I've gone over this driver, nothing stands out in a bad way to me, IOW this
> seems like a normal PMIC OpRegion handler to me.
> 

Thanks for the reviews and time.

RE: [PATCH v1 3/3] ACPI / PMIC: Add TI PMIC TPS68470 operation region driver

[Mani, Rajmohan]

Hi Hans,

> -Original Message-
> From: Hans de Goede [mailto:hdego...@redhat.com]
> Sent: Tuesday, June 06, 2017 8:22 AM
> To: Andy Shevchenko ; Mani, Rajmohan
> 
> Cc: linux-kernel@vger.kernel.org; linux-g...@vger.kernel.org; linux-
> a...@vger.kernel.org; Lee Jones ; Linus Walleij
> ; Alexandre Courbot ; Rafael J.
> Wysocki ; Len Brown 
> Subject: Re: [PATCH v1 3/3] ACPI / PMIC: Add TI PMIC TPS68470 operation
> region driver
> 
> Hi,
> 
> On 06/06/2017 04:23 PM, Andy Shevchenko wrote:
> > +Cc Hans (that's why didn't delete anything from original mail, just
> > adding my comments).
> >
> > Hans, if you have few minutes it would be appreciated to glance on the
> > below for some issues if any since you did pass quite a good quest
> > with other PMIC drivers.
> 
> I've gone over this driver, nothing stands out in a bad way to me, IOW this
> seems like a normal PMIC OpRegion handler to me.
> 

Thanks for the reviews and time.

RE: [PATCH v1 3/3] ACPI / PMIC: Add TI PMIC TPS68470 operation region driver

[Mani, Rajmohan]

Hi Andy,

Thanks for the reviews and patience.

> -Original Message-
> From: Andy Shevchenko [mailto:andy.shevche...@gmail.com]
> Sent: Tuesday, June 06, 2017 7:24 AM
> To: Mani, Rajmohan ; Hans de Goede
> 
> Cc: linux-kernel@vger.kernel.org; linux-g...@vger.kernel.org; linux-
> a...@vger.kernel.org; Lee Jones ; Linus Walleij
> ; Alexandre Courbot ; Rafael J.
> Wysocki ; Len Brown 
> Subject: Re: [PATCH v1 3/3] ACPI / PMIC: Add TI PMIC TPS68470 operation
> region driver
> 
> +Cc Hans (that's why didn't delete anything from original mail, just
> adding my comments).
> 
> Hans, if you have few minutes it would be appreciated to glance on the below
> for some issues if any since you did pass quite a good quest with other PMIC
> drivers.
> 
> On Tue, Jun 6, 2017 at 2:55 PM, Rajmohan Mani 
> wrote:
> > The Kabylake platform coreboot (Chrome OS equivalent of
> > BIOS) has defined 4 operation regions for the TI TPS68470 PMIC.
> > These operation regions are to enable/disable voltage regulators,
> > configure voltage regulators, enable/disable clocks and to configure
> > clocks.
> >
> > This config adds ACPI operation region support for TI TPS68470 PMIC.
> > TPS68470 device is an advanced power management unit that powers a
> > Compact Camera Module (CCM), generates clocks for image sensors,
> > drives a dual LED for flash and incorporates two LED drivers for
> > general purpose indicators.
> > This driver enables ACPI operation region support to control voltage
> > regulators and clocks for the TPS68470 PMIC.
> >
> > Signed-off-by: Rajmohan Mani 
> > ---
> >  drivers/acpi/Kconfig  |  12 +
> >  drivers/acpi/Makefile |   2 +
> 
> >  drivers/acpi/pmic/pmic_tps68470.c | 454
> > ++
> 
> Follow the pattern, please, I suppose
> ti_pmic_tps68470.c
> 

I will reply to this, on the later threads on the same subject

> >  3 files changed, 468 insertions(+)
> >  create mode 100644 drivers/acpi/pmic/pmic_tps68470.c
> >
> > diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig index
> > 1ce52f8..218d22d 100644
> > --- a/drivers/acpi/Kconfig
> > +++ b/drivers/acpi/Kconfig
> > @@ -535,4 +535,16 @@ if ARM64
> >  source "drivers/acpi/arm64/Kconfig"
> >  endif
> >
> > +config TPS68470_PMIC_OPREGION
> > +   bool "ACPI operation region support for TPS68470 PMIC"
> > +   help
> > + This config adds ACPI operation region support for TI TPS68470 
> > PMIC.
> > + TPS68470 device is an advanced power management unit that powers
> > + a Compact Camera Module (CCM), generates clocks for image sensors,
> > + drives a dual LED for flash and incorporates two LED drivers for
> > + general purpose indicators.
> > + This driver enables ACPI operation region support control voltage
> > + regulators and clocks.
> > +
> 
> > +
> 
> Extra line, remove.
> 

Ack

> >  endif  # ACPI
> > diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile index
> > b1aacfc..7113d05 100644
> > --- a/drivers/acpi/Makefile
> > +++ b/drivers/acpi/Makefile
> > @@ -106,6 +106,8 @@ obj-$(CONFIG_CHT_WC_PMIC_OPREGION) +=
> > pmic/intel_pmic_chtwc.o
> >
> >  obj-$(CONFIG_ACPI_CONFIGFS)+= acpi_configfs.o
> >
> > +obj-$(CONFIG_TPS68470_PMIC_OPREGION)   += pmic/pmic_tps68470.o
> > +
> >  video-objs += acpi_video.o video_detect.o
> >  obj-y  += dptf/
> >
> > diff --git a/drivers/acpi/pmic/pmic_tps68470.c
> > b/drivers/acpi/pmic/pmic_tps68470.c
> > new file mode 100644
> > index 000..b2d608b
> > --- /dev/null
> > +++ b/drivers/acpi/pmic/pmic_tps68470.c
> > @@ -0,0 +1,454 @@
> > +/*
> > + * TI TPS68470 PMIC operation region driver
> > + *
> > + * Copyright (C) 2017 Intel Corporation. All rights reserved.
> > + * Author: Rajmohan Mani 
> > + *
> > + * This program is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU General Public License
> > +version
> > + * 2 as published by the Free Software Foundation.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > + * GNU General Public License for more details.
> > + *
> > + * Based on drivers/acpi/pmic/intel_pmic* drivers
> > + *
> > + */
> > +
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +
> > +struct ti_pmic_table {
> > +   u32 address;/* operation region address */
> > +   u32 reg;/* corresponding register */
> > +   u32 bitmask;/* bit mask for power, clock */
> > +};
> > +
> > +#define TI_PMIC_POWER_OPREGION_ID  0xB0
> > 

RE: [PATCH v1 3/3] ACPI / PMIC: Add TI PMIC TPS68470 operation region driver

[Mani, Rajmohan]

Hi Andy,

Thanks for the reviews and patience.

> -Original Message-
> From: Andy Shevchenko [mailto:andy.shevche...@gmail.com]
> Sent: Tuesday, June 06, 2017 7:24 AM
> To: Mani, Rajmohan ; Hans de Goede
> 
> Cc: linux-kernel@vger.kernel.org; linux-g...@vger.kernel.org; linux-
> a...@vger.kernel.org; Lee Jones ; Linus Walleij
> ; Alexandre Courbot ; Rafael J.
> Wysocki ; Len Brown 
> Subject: Re: [PATCH v1 3/3] ACPI / PMIC: Add TI PMIC TPS68470 operation
> region driver
> 
> +Cc Hans (that's why didn't delete anything from original mail, just
> adding my comments).
> 
> Hans, if you have few minutes it would be appreciated to glance on the below
> for some issues if any since you did pass quite a good quest with other PMIC
> drivers.
> 
> On Tue, Jun 6, 2017 at 2:55 PM, Rajmohan Mani 
> wrote:
> > The Kabylake platform coreboot (Chrome OS equivalent of
> > BIOS) has defined 4 operation regions for the TI TPS68470 PMIC.
> > These operation regions are to enable/disable voltage regulators,
> > configure voltage regulators, enable/disable clocks and to configure
> > clocks.
> >
> > This config adds ACPI operation region support for TI TPS68470 PMIC.
> > TPS68470 device is an advanced power management unit that powers a
> > Compact Camera Module (CCM), generates clocks for image sensors,
> > drives a dual LED for flash and incorporates two LED drivers for
> > general purpose indicators.
> > This driver enables ACPI operation region support to control voltage
> > regulators and clocks for the TPS68470 PMIC.
> >
> > Signed-off-by: Rajmohan Mani 
> > ---
> >  drivers/acpi/Kconfig  |  12 +
> >  drivers/acpi/Makefile |   2 +
> 
> >  drivers/acpi/pmic/pmic_tps68470.c | 454
> > ++
> 
> Follow the pattern, please, I suppose
> ti_pmic_tps68470.c
> 

I will reply to this, on the later threads on the same subject

> >  3 files changed, 468 insertions(+)
> >  create mode 100644 drivers/acpi/pmic/pmic_tps68470.c
> >
> > diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig index
> > 1ce52f8..218d22d 100644
> > --- a/drivers/acpi/Kconfig
> > +++ b/drivers/acpi/Kconfig
> > @@ -535,4 +535,16 @@ if ARM64
> >  source "drivers/acpi/arm64/Kconfig"
> >  endif
> >
> > +config TPS68470_PMIC_OPREGION
> > +   bool "ACPI operation region support for TPS68470 PMIC"
> > +   help
> > + This config adds ACPI operation region support for TI TPS68470 
> > PMIC.
> > + TPS68470 device is an advanced power management unit that powers
> > + a Compact Camera Module (CCM), generates clocks for image sensors,
> > + drives a dual LED for flash and incorporates two LED drivers for
> > + general purpose indicators.
> > + This driver enables ACPI operation region support control voltage
> > + regulators and clocks.
> > +
> 
> > +
> 
> Extra line, remove.
> 

Ack

> >  endif  # ACPI
> > diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile index
> > b1aacfc..7113d05 100644
> > --- a/drivers/acpi/Makefile
> > +++ b/drivers/acpi/Makefile
> > @@ -106,6 +106,8 @@ obj-$(CONFIG_CHT_WC_PMIC_OPREGION) +=
> > pmic/intel_pmic_chtwc.o
> >
> >  obj-$(CONFIG_ACPI_CONFIGFS)+= acpi_configfs.o
> >
> > +obj-$(CONFIG_TPS68470_PMIC_OPREGION)   += pmic/pmic_tps68470.o
> > +
> >  video-objs += acpi_video.o video_detect.o
> >  obj-y  += dptf/
> >
> > diff --git a/drivers/acpi/pmic/pmic_tps68470.c
> > b/drivers/acpi/pmic/pmic_tps68470.c
> > new file mode 100644
> > index 000..b2d608b
> > --- /dev/null
> > +++ b/drivers/acpi/pmic/pmic_tps68470.c
> > @@ -0,0 +1,454 @@
> > +/*
> > + * TI TPS68470 PMIC operation region driver
> > + *
> > + * Copyright (C) 2017 Intel Corporation. All rights reserved.
> > + * Author: Rajmohan Mani 
> > + *
> > + * This program is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU General Public License
> > +version
> > + * 2 as published by the Free Software Foundation.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > + * GNU General Public License for more details.
> > + *
> > + * Based on drivers/acpi/pmic/intel_pmic* drivers
> > + *
> > + */
> > +
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +
> > +struct ti_pmic_table {
> > +   u32 address;/* operation region address */
> > +   u32 reg;/* corresponding register */
> > +   u32 bitmask;/* bit mask for power, clock */
> > +};
> > +
> > +#define TI_PMIC_POWER_OPREGION_ID  0xB0
> > +#define TI_PMIC_VR_VAL_OPREGION_ID 0xB1
> > +#define TI_PMIC_CLOCK_OPREGION_ID  0xB2
> > +#define TI_PMIC_CLKFREQ_OPREGION_ID0xB3
> > +
> > +struct ti_pmic_opregion {
> > +   struct 

Re: [PATCH] l2tp: cast l2tp traffic counter to unsigned

[Stephen Hemminger]

On Fri,  9 Jun 2017 16:29:47 +0200
Dominik Heidler  wrote:

> This fixes a counter problem on 32bit systems:
> When the rx_bytes counter reached 2 GiB, it jumpd to (2^64 Bytes - 2GiB) 
> Bytes.
> 
> rtnl_link_stats64 has __u64 type and atomic_long_read returns
> atomic_long_t which is signed. Due to the conversation
> we get an incorrect value on 32bit systems if the MSB of
> the atomic_long_t value is set.
> 
> CC: Tom Parkin 
> Fixes: 7b7c0719cd7a ("l2tp: avoid deadlock in l2tp stats update")
> Signed-off-by: Dominik Heidler 
> ---
>  net/l2tp/l2tp_eth.c | 13 +++--
>  1 file changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/net/l2tp/l2tp_eth.c b/net/l2tp/l2tp_eth.c
> index 8b21af7321b9..668a75e002e9 100644
> --- a/net/l2tp/l2tp_eth.c
> +++ b/net/l2tp/l2tp_eth.c
> @@ -114,12 +114,13 @@ static void l2tp_eth_get_stats64(struct net_device *dev,
>  {
>   struct l2tp_eth *priv = netdev_priv(dev);
>  
> - stats->tx_bytes   = atomic_long_read(>tx_bytes);
> - stats->tx_packets = atomic_long_read(>tx_packets);
> - stats->tx_dropped = atomic_long_read(>tx_dropped);
> - stats->rx_bytes   = atomic_long_read(>rx_bytes);
> - stats->rx_packets = atomic_long_read(>rx_packets);
> - stats->rx_errors  = atomic_long_read(>rx_errors);
> + stats->tx_bytes   = (unsigned long) atomic_long_read(>tx_bytes);
> + stats->tx_packets = (unsigned long) atomic_long_read(>tx_packets);
> + stats->tx_dropped = (unsigned long) atomic_long_read(>tx_dropped);
> + stats->rx_bytes   = (unsigned long) atomic_long_read(>rx_bytes);
> + stats->rx_packets = (unsigned long) atomic_long_read(>rx_packets);
> + stats->rx_errors  = (unsigned long) atomic_long_read(>rx_errors);
> +
>  }
>  
>  static const struct net_device_ops l2tp_eth_netdev_ops = {

This is not the right way to fix this.

1. shouldn't be using atomic's for network counters, look at other network 
devices.

2. should be using u64_stats_fetch  api to handle 64 bit counters.

Re: [PATCH] l2tp: cast l2tp traffic counter to unsigned

[Stephen Hemminger]

On Fri,  9 Jun 2017 16:29:47 +0200
Dominik Heidler  wrote:

> This fixes a counter problem on 32bit systems:
> When the rx_bytes counter reached 2 GiB, it jumpd to (2^64 Bytes - 2GiB) 
> Bytes.
> 
> rtnl_link_stats64 has __u64 type and atomic_long_read returns
> atomic_long_t which is signed. Due to the conversation
> we get an incorrect value on 32bit systems if the MSB of
> the atomic_long_t value is set.
> 
> CC: Tom Parkin 
> Fixes: 7b7c0719cd7a ("l2tp: avoid deadlock in l2tp stats update")
> Signed-off-by: Dominik Heidler 
> ---
>  net/l2tp/l2tp_eth.c | 13 +++--
>  1 file changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/net/l2tp/l2tp_eth.c b/net/l2tp/l2tp_eth.c
> index 8b21af7321b9..668a75e002e9 100644
> --- a/net/l2tp/l2tp_eth.c
> +++ b/net/l2tp/l2tp_eth.c
> @@ -114,12 +114,13 @@ static void l2tp_eth_get_stats64(struct net_device *dev,
>  {
>   struct l2tp_eth *priv = netdev_priv(dev);
>  
> - stats->tx_bytes   = atomic_long_read(>tx_bytes);
> - stats->tx_packets = atomic_long_read(>tx_packets);
> - stats->tx_dropped = atomic_long_read(>tx_dropped);
> - stats->rx_bytes   = atomic_long_read(>rx_bytes);
> - stats->rx_packets = atomic_long_read(>rx_packets);
> - stats->rx_errors  = atomic_long_read(>rx_errors);
> + stats->tx_bytes   = (unsigned long) atomic_long_read(>tx_bytes);
> + stats->tx_packets = (unsigned long) atomic_long_read(>tx_packets);
> + stats->tx_dropped = (unsigned long) atomic_long_read(>tx_dropped);
> + stats->rx_bytes   = (unsigned long) atomic_long_read(>rx_bytes);
> + stats->rx_packets = (unsigned long) atomic_long_read(>rx_packets);
> + stats->rx_errors  = (unsigned long) atomic_long_read(>rx_errors);
> +
>  }
>  
>  static const struct net_device_ops l2tp_eth_netdev_ops = {

This is not the right way to fix this.

1. shouldn't be using atomic's for network counters, look at other network 
devices.

2. should be using u64_stats_fetch  api to handle 64 bit counters.

RE: [PATCH v1 1/3] mfd: Add new mfd device TPS68470

[Mani, Rajmohan]

Hi Andy,

> On Tue, Jun 06, 2017 at 03:59:49PM +0300, Andy Shevchenko wrote:
> > On Tue, Jun 6, 2017 at 2:55 PM, Rajmohan Mani
>  wrote:
> > > The TPS68470 device is an advanced power management unit that powers
> > > a Compact Camera Module (CCM), generates clocks for image sensors,
> > > drives a dual LED for Flash and incorporates two LED drivers for
> > > general purpose indicators.
> > >
> > > This patch adds support for TPS68470 mfd device.
> >
> > I dunno why you decide to send this out now, see my comments below.
> >
> > > +static int tps68470_chip_init(struct tps68470 *tps) {
> > > +   unsigned int version;
> > > +   int ret;
> >
> > > +   /* FIXME: configure these dynamically */
> >
> > So, what prevents you to fix this?
> 
> Nothing I suppose. They're however not needed right now and can be
> implemented later on if they're ever needed.
> 

Ack

RE: [PATCH v1 1/3] mfd: Add new mfd device TPS68470

[Mani, Rajmohan]

Hi Andy,

> On Tue, Jun 06, 2017 at 03:59:49PM +0300, Andy Shevchenko wrote:
> > On Tue, Jun 6, 2017 at 2:55 PM, Rajmohan Mani
>  wrote:
> > > The TPS68470 device is an advanced power management unit that powers
> > > a Compact Camera Module (CCM), generates clocks for image sensors,
> > > drives a dual LED for Flash and incorporates two LED drivers for
> > > general purpose indicators.
> > >
> > > This patch adds support for TPS68470 mfd device.
> >
> > I dunno why you decide to send this out now, see my comments below.
> >
> > > +static int tps68470_chip_init(struct tps68470 *tps) {
> > > +   unsigned int version;
> > > +   int ret;
> >
> > > +   /* FIXME: configure these dynamically */
> >
> > So, what prevents you to fix this?
> 
> Nothing I suppose. They're however not needed right now and can be
> implemented later on if they're ever needed.
> 

Ack

RE: [PATCH v1 1/3] mfd: Add new mfd device TPS68470

[Mani, Rajmohan]

Hi Andy,

Thanks for the reviews and patience.

> -Original Message-
> From: Andy Shevchenko [mailto:andy.shevche...@gmail.com]
> Sent: Tuesday, June 06, 2017 6:00 AM
> To: Mani, Rajmohan 
> Cc: linux-kernel@vger.kernel.org; linux-g...@vger.kernel.org; linux-
> a...@vger.kernel.org; Lee Jones ; Linus Walleij
> ; Alexandre Courbot ; Rafael J.
> Wysocki ; Len Brown 
> Subject: Re: [PATCH v1 1/3] mfd: Add new mfd device TPS68470
> 
> On Tue, Jun 6, 2017 at 2:55 PM, Rajmohan Mani 
> wrote:
> > The TPS68470 device is an advanced power management unit that powers a
> > Compact Camera Module (CCM), generates clocks for image sensors,
> > drives a dual LED for Flash and incorporates two LED drivers for
> > general purpose indicators.
> >
> > This patch adds support for TPS68470 mfd device.
> 
> I dunno why you decide to send this out now, see my comments below.
> 

We decided to go with the submission of these drivers for upstream review 
sooner rather than later.

> > +static int tps68470_chip_init(struct tps68470 *tps) {
> > +   unsigned int version;
> > +   int ret;
> 
> > +   /* FIXME: configure these dynamically */
> 
> So, what prevents you to fix this?
> 

I will respond on top of Sakari's response.

> > +   /* Enable Daisy Chain LDO and configure relevant GPIOs as
> > + output */
> 
> > +}
> 
> > +static int tps68470_probe(struct i2c_client *client) {
> > +   struct tps68470 *tps;
> > +   int ret;
> > +
> > +   tps = devm_kzalloc(>dev, sizeof(*tps), GFP_KERNEL);
> > +   if (!tps)
> > +   return -ENOMEM;
> > +
> > +   mutex_init(>lock);
> > +   i2c_set_clientdata(client, tps);
> > +   tps->dev = >dev;
> > +
> > +   tps->regmap = devm_regmap_init_i2c(client,
> _regmap_config);
> > +   if (IS_ERR(tps->regmap)) {
> > +   dev_err(tps->dev, "devm_regmap_init_i2c Error %d\n", ret);
> > +   return PTR_ERR(tps->regmap);
> > +   }
> > +
> 
> > +   ret = mfd_add_devices(tps->dev, -1, tps68470s,
> > + ARRAY_SIZE(tps68470s), NULL, 0, NULL);
> 
> devm_?
> 

Ack

> > +   if (ret < 0) {
> > +   dev_err(tps->dev, "mfd_add_devices failed: %d\n", ret);
> > +   return ret;
> > +   }
> > +
> > +   ret = tps68470_chip_init(tps);
> > +   if (ret < 0) {
> > +   dev_err(tps->dev, "TPS68470 Init Error %d\n", ret);
> > +   goto fail;
> > +   }
> > +
> > +   return 0;
> 
> > +fail:
> > +   mutex_lock(>lock);
> 
> I'm not sure you need this mutex to be held here.
> Otherwise your code has a bug with locking.
> 

Repeating the response to Heikki here

I had this following question from Alan Cox on the original code without these 
wrappers.

"What is the model for insuring that no interrupt or thread of a driver is not 
in parallel issuing a tps68470_ operation when the device goes away (eg if I 
down the i2c controller) ?"

To address the above concerns, I got extra cautious and implemented locks 
around the regmap_* calls.
Now, I have been asked from more than one reviewer about the necessity of the 
same.
With the use of devm_* calls, tps68470_remove() goes away and leaves the driver 
just with regmap_* calls.
Unless I hear from Alan or other reviewers otherwise, I will drop these 
wrappers around regmap_* calls.

> > +   mfd_remove_devices(tps->dev);
> > +   mutex_unlock(>lock);
> > +
> > +   return ret;
> 
> Taking above into consideration I suggest to clarify your locking scheme.
> 

Same as above.

> > +}
> > +
> > +static int tps68470_remove(struct i2c_client *client) {
> > +   struct tps68470 *tps = i2c_get_clientdata(client);
> > +
> 
> > +   mutex_lock(>lock);
> > +   mfd_remove_devices(tps->dev);
> > +   mutex_unlock(>lock);
> 
> Ditto.
> 

Same as above

> > +   return 0;
> > +}
> 
> > +/**
> > + * struct tps68470 - tps68470 sub-driver chip access routines
> > + *
> 
> kbuild bot will be unhappy. You need to file a description per field.
> 

Ack
It looks like this structure will go away, once I implement the feedback from 
Heikki.

> > + * Device data may be used to access the TPS68470 chip */
> > +
> > +struct tps68470 {
> > +   struct device *dev;
> > +   struct regmap *regmap;
> 
> > +   /*
> > +* Used to synchronize access to tps68470_ operations
> > +* and addition and removal of mfd devices
> > +*/
> 
> Move it to kernel-doc above.
> 

Same as above

> > +   struct mutex lock;
> > +};
> 
> --
> With Best Regards,
> Andy Shevchenko

RE: [PATCH v1 1/3] mfd: Add new mfd device TPS68470

[Mani, Rajmohan]

Hi Andy,

Thanks for the reviews and patience.

> -Original Message-
> From: Andy Shevchenko [mailto:andy.shevche...@gmail.com]
> Sent: Tuesday, June 06, 2017 6:00 AM
> To: Mani, Rajmohan 
> Cc: linux-kernel@vger.kernel.org; linux-g...@vger.kernel.org; linux-
> a...@vger.kernel.org; Lee Jones ; Linus Walleij
> ; Alexandre Courbot ; Rafael J.
> Wysocki ; Len Brown 
> Subject: Re: [PATCH v1 1/3] mfd: Add new mfd device TPS68470
> 
> On Tue, Jun 6, 2017 at 2:55 PM, Rajmohan Mani 
> wrote:
> > The TPS68470 device is an advanced power management unit that powers a
> > Compact Camera Module (CCM), generates clocks for image sensors,
> > drives a dual LED for Flash and incorporates two LED drivers for
> > general purpose indicators.
> >
> > This patch adds support for TPS68470 mfd device.
> 
> I dunno why you decide to send this out now, see my comments below.
> 

We decided to go with the submission of these drivers for upstream review 
sooner rather than later.

> > +static int tps68470_chip_init(struct tps68470 *tps) {
> > +   unsigned int version;
> > +   int ret;
> 
> > +   /* FIXME: configure these dynamically */
> 
> So, what prevents you to fix this?
> 

I will respond on top of Sakari's response.

> > +   /* Enable Daisy Chain LDO and configure relevant GPIOs as
> > + output */
> 
> > +}
> 
> > +static int tps68470_probe(struct i2c_client *client) {
> > +   struct tps68470 *tps;
> > +   int ret;
> > +
> > +   tps = devm_kzalloc(>dev, sizeof(*tps), GFP_KERNEL);
> > +   if (!tps)
> > +   return -ENOMEM;
> > +
> > +   mutex_init(>lock);
> > +   i2c_set_clientdata(client, tps);
> > +   tps->dev = >dev;
> > +
> > +   tps->regmap = devm_regmap_init_i2c(client,
> _regmap_config);
> > +   if (IS_ERR(tps->regmap)) {
> > +   dev_err(tps->dev, "devm_regmap_init_i2c Error %d\n", ret);
> > +   return PTR_ERR(tps->regmap);
> > +   }
> > +
> 
> > +   ret = mfd_add_devices(tps->dev, -1, tps68470s,
> > + ARRAY_SIZE(tps68470s), NULL, 0, NULL);
> 
> devm_?
> 

Ack

> > +   if (ret < 0) {
> > +   dev_err(tps->dev, "mfd_add_devices failed: %d\n", ret);
> > +   return ret;
> > +   }
> > +
> > +   ret = tps68470_chip_init(tps);
> > +   if (ret < 0) {
> > +   dev_err(tps->dev, "TPS68470 Init Error %d\n", ret);
> > +   goto fail;
> > +   }
> > +
> > +   return 0;
> 
> > +fail:
> > +   mutex_lock(>lock);
> 
> I'm not sure you need this mutex to be held here.
> Otherwise your code has a bug with locking.
> 

Repeating the response to Heikki here

I had this following question from Alan Cox on the original code without these 
wrappers.

"What is the model for insuring that no interrupt or thread of a driver is not 
in parallel issuing a tps68470_ operation when the device goes away (eg if I 
down the i2c controller) ?"

To address the above concerns, I got extra cautious and implemented locks 
around the regmap_* calls.
Now, I have been asked from more than one reviewer about the necessity of the 
same.
With the use of devm_* calls, tps68470_remove() goes away and leaves the driver 
just with regmap_* calls.
Unless I hear from Alan or other reviewers otherwise, I will drop these 
wrappers around regmap_* calls.

> > +   mfd_remove_devices(tps->dev);
> > +   mutex_unlock(>lock);
> > +
> > +   return ret;
> 
> Taking above into consideration I suggest to clarify your locking scheme.
> 

Same as above.

> > +}
> > +
> > +static int tps68470_remove(struct i2c_client *client) {
> > +   struct tps68470 *tps = i2c_get_clientdata(client);
> > +
> 
> > +   mutex_lock(>lock);
> > +   mfd_remove_devices(tps->dev);
> > +   mutex_unlock(>lock);
> 
> Ditto.
> 

Same as above

> > +   return 0;
> > +}
> 
> > +/**
> > + * struct tps68470 - tps68470 sub-driver chip access routines
> > + *
> 
> kbuild bot will be unhappy. You need to file a description per field.
> 

Ack
It looks like this structure will go away, once I implement the feedback from 
Heikki.

> > + * Device data may be used to access the TPS68470 chip */
> > +
> > +struct tps68470 {
> > +   struct device *dev;
> > +   struct regmap *regmap;
> 
> > +   /*
> > +* Used to synchronize access to tps68470_ operations
> > +* and addition and removal of mfd devices
> > +*/
> 
> Move it to kernel-doc above.
> 

Same as above

> > +   struct mutex lock;
> > +};
> 
> --
> With Best Regards,
> Andy Shevchenko

Re: [PATCH 06/10] fpga manager: Add altera-ps-spi driver for Altera FPGAs

[stillcompiling]

On Friday, June 9, 2017 3:18:40 PM PDT Anatolij Gustschin wrote:
> On Fri, 9 Jun 2017 11:51:12 +0200
> Greg Kroah-Hartman gre...@linuxfoundation.org wrote:
> ...
> 
> >I get the following build error with this patch:
> >
> >ERROR: "__spi_register_driver" [drivers/fpga/altera-ps-spi.ko] undefined!
> 
> it is due to enabled COMPILE_TEST and disabled CONFIG_SPI.
> 
> >So I'll just ignore this whole series, can you fix these issues up and
> >resend?
> 
> would "depends on SPI || (COMPILE_TEST && SPI)" in Kconfig be a proper
> fix for this? Anyone an idea?
> 

> Thanks,
> Anatolij
I don't think that ends up being any different than "depends on SPI"
But, it is an SPI slave driver driver, after all.
COMPILE_TEST is intended to build stuff that can't work due to a lack of 
hardware support, or lack of crosscompiler.
"selects SPI" would work, but is discouraged.

I just built successfully on native x86_64 with no problem, by enabling SPI.
"depends on SPI" is the right thing here.
I'm not sure when || COMPILE_TEST sneaked in there.

I'll submit a v13 with that small change.
-- 
~Joshua A Clayton

Re: [PATCH 06/10] fpga manager: Add altera-ps-spi driver for Altera FPGAs

[stillcompiling]

On Friday, June 9, 2017 3:18:40 PM PDT Anatolij Gustschin wrote:
> On Fri, 9 Jun 2017 11:51:12 +0200
> Greg Kroah-Hartman gre...@linuxfoundation.org wrote:
> ...
> 
> >I get the following build error with this patch:
> >
> >ERROR: "__spi_register_driver" [drivers/fpga/altera-ps-spi.ko] undefined!
> 
> it is due to enabled COMPILE_TEST and disabled CONFIG_SPI.
> 
> >So I'll just ignore this whole series, can you fix these issues up and
> >resend?
> 
> would "depends on SPI || (COMPILE_TEST && SPI)" in Kconfig be a proper
> fix for this? Anyone an idea?
> 

> Thanks,
> Anatolij
I don't think that ends up being any different than "depends on SPI"
But, it is an SPI slave driver driver, after all.
COMPILE_TEST is intended to build stuff that can't work due to a lack of 
hardware support, or lack of crosscompiler.
"selects SPI" would work, but is discouraged.

I just built successfully on native x86_64 with no problem, by enabling SPI.
"depends on SPI" is the right thing here.
I'm not sure when || COMPILE_TEST sneaked in there.

I'll submit a v13 with that small change.
-- 
~Joshua A Clayton

RE: [PATCH v1 1/3] mfd: Add new mfd device TPS68470

[Mani, Rajmohan]

Hi Heikki,

Thanks for the reviews and patience.

> -Original Message-
> From: Heikki Krogerus [mailto:heikki.kroge...@linux.intel.com]
> Sent: Tuesday, June 06, 2017 5:49 AM
> To: Mani, Rajmohan 
> Cc: linux-kernel@vger.kernel.org; linux-g...@vger.kernel.org; linux-
> a...@vger.kernel.org; Lee Jones ; Linus Walleij
> ; Alexandre Courbot ; Rafael J.
> Wysocki ; Len Brown 
> Subject: Re: [PATCH v1 1/3] mfd: Add new mfd device TPS68470
> 
> Hi Rajmohan,
> 
> On Tue, Jun 06, 2017 at 04:55:16AM -0700, Rajmohan Mani wrote:
> > +/*
> > + * tps68470_reg_read: Read a single tps68470 register.
> > + *
> > + * @tps: Device to read from.
> > + * @reg: Register to read.
> > + * @val: Contains the value
> > + */
> > +int tps68470_reg_read(struct tps68470 *tps, unsigned int reg,
> > +   unsigned int *val)
> > +{
> > +   int ret;
> > +
> > +   mutex_lock(>lock);
> > +   ret = regmap_read(tps->regmap, reg, val);
> > +   mutex_unlock(>lock);
> > +   return ret;
> > +}
> > +EXPORT_SYMBOL_GPL(tps68470_reg_read);
> > +
> > +/*
> > + * tps68470_reg_write: Write a single tps68470 register.
> > + *
> > + * @tps68470: Device to write to.
> > + * @reg: Register to write to.
> > + * @val: Value to write.
> > + */
> > +int tps68470_reg_write(struct tps68470 *tps, unsigned int reg,
> > +   unsigned int val)
> > +{
> > +   int ret;
> > +
> > +   mutex_lock(>lock);
> > +   ret = regmap_write(tps->regmap, reg, val);
> > +   mutex_unlock(>lock);
> > +   return ret;
> > +}
> > +EXPORT_SYMBOL_GPL(tps68470_reg_write);
> > +
> > +/*
> > + * tps68470_update_bits: Modify bits w.r.t mask and val.
> > + *
> > + * @tps68470: Device to write to.
> > + * @reg: Register to read-write to.
> > + * @mask: Mask.
> > + * @val: Value to write.
> > + */
> > +int tps68470_update_bits(struct tps68470 *tps, unsigned int reg,
> > +   unsigned int mask, unsigned int val) {
> > +   int ret;
> > +
> > +   mutex_lock(>lock);
> > +   ret = regmap_update_bits(tps->regmap, reg, mask, val);
> > +   mutex_unlock(>lock);
> > +   return ret;
> > +}
> > +EXPORT_SYMBOL_GPL(tps68470_update_bits);
> 
> I'm not sure you need those above wrappers at all, regmap is handling locking 
> in
> any case.
> 

I had this following question from Alan Cox on the original code without these 
wrappers.

"What is the model for insuring that no interrupt or thread of a driver is not 
in parallel issuing a tps68470_ operation when the device goes away (eg if I 
down the i2c controller) ?"

To address the above concerns, I got extra cautious and implemented locks 
around the regmap_* calls.
Now, I have been asked from more than one reviewer about the necessity of the 
same.
With the use of devm_* calls, tps68470_remove() goes away and leaves the driver 
just with regmap_* calls.
Unless I hear from Alan or other reviewers otherwise, I will drop these 
wrappers around regmap_* calls.

> > +static const struct regmap_config tps68470_regmap_config = {
> > +   .reg_bits = 8,
> > +   .val_bits = 8,
> > +   .max_register = TPS68470_REG_MAX,
> > +};
> > +
> > +static int tps68470_chip_init(struct tps68470 *tps) {
> > +   unsigned int version;
> > +   int ret;
> > +
> > +   ret = tps68470_reg_read(tps, TPS68470_REG_REVID, );
> > +   if (ret < 0) {
> > +   dev_err(tps->dev,
> > +   "Failed to read revision register: %d\n", ret);
> > +   return ret;
> > +   }
> > +
> > +   dev_info(tps->dev, "TPS68470 REVID: 0x%x\n", version);
> > +
> > +   ret = tps68470_reg_write(tps, TPS68470_REG_RESET, 0xff);
> > +   if (ret < 0)
> > +   return ret;
> > +
> > +   /* FIXME: configure these dynamically */
> > +   /* Enable Daisy Chain LDO and configure relevant GPIOs as output */
> > +   ret = tps68470_reg_write(tps, TPS68470_REG_S_I2C_CTL, 2);
> > +   if (ret < 0)
> > +   return ret;
> > +
> > +   ret = tps68470_reg_write(tps, TPS68470_REG_GPCTL4A, 2);
> > +   if (ret < 0)
> > +   return ret;
> > +
> > +   ret = tps68470_reg_write(tps, TPS68470_REG_GPCTL5A, 2);
> > +   if (ret < 0)
> > +   return ret;
> > +
> > +   ret = tps68470_reg_write(tps, TPS68470_REG_GPCTL6A, 2);
> > +   if (ret < 0)
> > +   return ret;
> > +
> > +   /*
> > +* When SDA and SCL are routed to GPIO1 and GPIO2, the mode
> > +* for these GPIOs must be configured using their respective
> > +* GPCTLxA registers as inputs with no pull-ups.
> > +*/
> > +   ret = tps68470_reg_write(tps, TPS68470_REG_GPCTL1A, 0);
> > +   if (ret < 0)
> > +   return ret;
> > +
> > +   ret = tps68470_reg_write(tps, TPS68470_REG_GPCTL2A, 0);
> > +   if (ret < 0)
> > +   return ret;
> > +
> > +   /* Enable daisy chain */
> > +   ret = tps68470_update_bits(tps, TPS68470_REG_S_I2C_CTL, 1, 1);
> > +   if (ret < 0)
> > +   return ret;
> > +
> > +   

RE: [PATCH v1 1/3] mfd: Add new mfd device TPS68470

[Mani, Rajmohan]

Hi Heikki,

Thanks for the reviews and patience.

> -Original Message-
> From: Heikki Krogerus [mailto:heikki.kroge...@linux.intel.com]
> Sent: Tuesday, June 06, 2017 5:49 AM
> To: Mani, Rajmohan 
> Cc: linux-kernel@vger.kernel.org; linux-g...@vger.kernel.org; linux-
> a...@vger.kernel.org; Lee Jones ; Linus Walleij
> ; Alexandre Courbot ; Rafael J.
> Wysocki ; Len Brown 
> Subject: Re: [PATCH v1 1/3] mfd: Add new mfd device TPS68470
> 
> Hi Rajmohan,
> 
> On Tue, Jun 06, 2017 at 04:55:16AM -0700, Rajmohan Mani wrote:
> > +/*
> > + * tps68470_reg_read: Read a single tps68470 register.
> > + *
> > + * @tps: Device to read from.
> > + * @reg: Register to read.
> > + * @val: Contains the value
> > + */
> > +int tps68470_reg_read(struct tps68470 *tps, unsigned int reg,
> > +   unsigned int *val)
> > +{
> > +   int ret;
> > +
> > +   mutex_lock(>lock);
> > +   ret = regmap_read(tps->regmap, reg, val);
> > +   mutex_unlock(>lock);
> > +   return ret;
> > +}
> > +EXPORT_SYMBOL_GPL(tps68470_reg_read);
> > +
> > +/*
> > + * tps68470_reg_write: Write a single tps68470 register.
> > + *
> > + * @tps68470: Device to write to.
> > + * @reg: Register to write to.
> > + * @val: Value to write.
> > + */
> > +int tps68470_reg_write(struct tps68470 *tps, unsigned int reg,
> > +   unsigned int val)
> > +{
> > +   int ret;
> > +
> > +   mutex_lock(>lock);
> > +   ret = regmap_write(tps->regmap, reg, val);
> > +   mutex_unlock(>lock);
> > +   return ret;
> > +}
> > +EXPORT_SYMBOL_GPL(tps68470_reg_write);
> > +
> > +/*
> > + * tps68470_update_bits: Modify bits w.r.t mask and val.
> > + *
> > + * @tps68470: Device to write to.
> > + * @reg: Register to read-write to.
> > + * @mask: Mask.
> > + * @val: Value to write.
> > + */
> > +int tps68470_update_bits(struct tps68470 *tps, unsigned int reg,
> > +   unsigned int mask, unsigned int val) {
> > +   int ret;
> > +
> > +   mutex_lock(>lock);
> > +   ret = regmap_update_bits(tps->regmap, reg, mask, val);
> > +   mutex_unlock(>lock);
> > +   return ret;
> > +}
> > +EXPORT_SYMBOL_GPL(tps68470_update_bits);
> 
> I'm not sure you need those above wrappers at all, regmap is handling locking 
> in
> any case.
> 

I had this following question from Alan Cox on the original code without these 
wrappers.

"What is the model for insuring that no interrupt or thread of a driver is not 
in parallel issuing a tps68470_ operation when the device goes away (eg if I 
down the i2c controller) ?"

To address the above concerns, I got extra cautious and implemented locks 
around the regmap_* calls.
Now, I have been asked from more than one reviewer about the necessity of the 
same.
With the use of devm_* calls, tps68470_remove() goes away and leaves the driver 
just with regmap_* calls.
Unless I hear from Alan or other reviewers otherwise, I will drop these 
wrappers around regmap_* calls.

> > +static const struct regmap_config tps68470_regmap_config = {
> > +   .reg_bits = 8,
> > +   .val_bits = 8,
> > +   .max_register = TPS68470_REG_MAX,
> > +};
> > +
> > +static int tps68470_chip_init(struct tps68470 *tps) {
> > +   unsigned int version;
> > +   int ret;
> > +
> > +   ret = tps68470_reg_read(tps, TPS68470_REG_REVID, );
> > +   if (ret < 0) {
> > +   dev_err(tps->dev,
> > +   "Failed to read revision register: %d\n", ret);
> > +   return ret;
> > +   }
> > +
> > +   dev_info(tps->dev, "TPS68470 REVID: 0x%x\n", version);
> > +
> > +   ret = tps68470_reg_write(tps, TPS68470_REG_RESET, 0xff);
> > +   if (ret < 0)
> > +   return ret;
> > +
> > +   /* FIXME: configure these dynamically */
> > +   /* Enable Daisy Chain LDO and configure relevant GPIOs as output */
> > +   ret = tps68470_reg_write(tps, TPS68470_REG_S_I2C_CTL, 2);
> > +   if (ret < 0)
> > +   return ret;
> > +
> > +   ret = tps68470_reg_write(tps, TPS68470_REG_GPCTL4A, 2);
> > +   if (ret < 0)
> > +   return ret;
> > +
> > +   ret = tps68470_reg_write(tps, TPS68470_REG_GPCTL5A, 2);
> > +   if (ret < 0)
> > +   return ret;
> > +
> > +   ret = tps68470_reg_write(tps, TPS68470_REG_GPCTL6A, 2);
> > +   if (ret < 0)
> > +   return ret;
> > +
> > +   /*
> > +* When SDA and SCL are routed to GPIO1 and GPIO2, the mode
> > +* for these GPIOs must be configured using their respective
> > +* GPCTLxA registers as inputs with no pull-ups.
> > +*/
> > +   ret = tps68470_reg_write(tps, TPS68470_REG_GPCTL1A, 0);
> > +   if (ret < 0)
> > +   return ret;
> > +
> > +   ret = tps68470_reg_write(tps, TPS68470_REG_GPCTL2A, 0);
> > +   if (ret < 0)
> > +   return ret;
> > +
> > +   /* Enable daisy chain */
> > +   ret = tps68470_update_bits(tps, TPS68470_REG_S_I2C_CTL, 1, 1);
> > +   if (ret < 0)
> > +   return ret;
> > +
> > +   usleep_range(TPS68470_DAISY_CHAIN_DELAY_US,
> > +   TPS68470_DAISY_CHAIN_DELAY_US + 10);
> > +   return 0;
> > +}
> > +
> > +static int 

[PATCH 7/7] vfio: Use driver_override to avert binding to compromising drivers

[Alex Williamson]

If a device is bound to a non-vfio, non-whitelisted driver while a
group is in use, then the integrity of the group is compromised and
will result in hitting a BUG_ON.  This code tries to avoid this case
by mangling driver_override to force a no-match for the driver.  The
driver-core will either follow-up with a DRIVER_NOT_BOUND (preferred)
or BOUND_DRIVER, at which point we can remove the driver_override
mangling.

A complication here is that even though the window between these
notifications is expected to be extremely small, the vfio group could
be removed, which would prevent us from finding the group again to
remove the driver_override.  We therefore take a group reference when
adding to driver_override and release it when removed.  A second
complication is that driver_override can be modified by the system
admin through sysfs.  To avoid trivial interference, we add a non-
user-visible UUID to the group and use this as part of the mangle
string.

The above blocks binding to a driver that would compromise the host,
but we'd also like to avoid reaching that step if possible.  For this
we add a wait_event_timeout() with a short, 1 second timeout, which is
highly effective in allowing empty groups to finish cleanup.

Signed-off-by: Alex Williamson 
---
 drivers/vfio/vfio.c |  151 +--
 1 file changed, 144 insertions(+), 7 deletions(-)

diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
index a25ee4930200..ea786d512faa 100644
--- a/drivers/vfio/vfio.c
+++ b/drivers/vfio/vfio.c
@@ -25,6 +25,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -32,8 +33,10 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
+#include 
 
 #define DRIVER_VERSION "0.3"
 #define DRIVER_AUTHOR  "Alex Williamson "
@@ -95,6 +98,7 @@ struct vfio_group {
boolnoiommu;
struct kvm  *kvm;
struct blocking_notifier_head   notifier;
+   unsigned char   uuid[16];
 };
 
 struct vfio_device {
@@ -352,6 +356,8 @@ static struct vfio_group *vfio_create_group(struct 
iommu_group *iommu_group)
 
group->nb.notifier_call = vfio_iommu_group_notifier;
 
+   generate_random_uuid(group->uuid);
+
/*
 * blocking notifiers acquire a rwsem around registering and hold
 * it around callback.  Therefore, need to register outside of
@@ -728,6 +734,116 @@ static int vfio_group_nb_verify(struct vfio_group *group, 
struct device *dev)
return vfio_dev_viable(dev, group);
 }
 
+#define VFIO_TAG_PREFIX "#vfio_group:"
+
+static char **vfio_find_driver_override(struct device *dev)
+{
+   if (dev_is_pci(dev)) {
+   struct pci_dev *pdev = to_pci_dev(dev);
+   return >driver_override;
+   } else if (dev->bus == _bus_type) {
+   struct platform_device *pdev = to_platform_device(dev);
+   return >driver_override;
+#ifdef CONFIG_ARM_AMBA
+   } else if (dev->bus == _bustype) {
+   struct amba_device *adev = to_amba_device(dev);
+   return >driver_override;
+#endif
+   }
+
+   return NULL;
+}
+
+/*
+ * If we're about to bind to something other than a known whitelisted driver
+ * or known vfio bus driver, try to avert it with driver_override.
+ */
+static void vfio_group_nb_pre_bind(struct vfio_group *group, struct device 
*dev)
+{
+   struct vfio_bus_driver *driver;
+   struct device_driver *drv = ACCESS_ONCE(dev->driver);
+   char **driver_override;
+
+   if (vfio_dev_whitelisted(dev, drv))
+   return; /* Binding to known "innocuous" device/driver */
+
+   mutex_lock(_drivers_lock);
+   list_for_each_entry(driver, _drivers_list, vfio_next) {
+   if (driver->drv == drv) {
+   mutex_unlock(_drivers_lock);
+   return; /* Binding to known vfio bus driver, ok */
+   }
+   }
+   mutex_unlock(_drivers_lock);
+
+   /* Can we stall slightly to let users fall off? */
+   if (list_empty(>device_list)) {
+   if (wait_event_timeout(vfio.release_q,
+   !atomic_read(>container_users), HZ))
+   return;
+   }
+
+   driver_override = vfio_find_driver_override(dev);
+   if (driver_override) {
+   char tag[50], *new = NULL, *old = *driver_override;
+
+   snprintf(tag, sizeof(tag), "%s%pU",
+VFIO_TAG_PREFIX, group->uuid);
+
+   if (old && strstr(old, tag))
+   return; /* block already in place */
+
+   new = kasprintf(GFP_KERNEL, "%s%s", old ? old : "", tag);
+   if (new) {
+   *driver_override = new;
+   kfree(old);
+   vfio_group_get(group);
+   

[PATCH 7/7] vfio: Use driver_override to avert binding to compromising drivers

[Alex Williamson]

If a device is bound to a non-vfio, non-whitelisted driver while a
group is in use, then the integrity of the group is compromised and
will result in hitting a BUG_ON.  This code tries to avoid this case
by mangling driver_override to force a no-match for the driver.  The
driver-core will either follow-up with a DRIVER_NOT_BOUND (preferred)
or BOUND_DRIVER, at which point we can remove the driver_override
mangling.

A complication here is that even though the window between these
notifications is expected to be extremely small, the vfio group could
be removed, which would prevent us from finding the group again to
remove the driver_override.  We therefore take a group reference when
adding to driver_override and release it when removed.  A second
complication is that driver_override can be modified by the system
admin through sysfs.  To avoid trivial interference, we add a non-
user-visible UUID to the group and use this as part of the mangle
string.

The above blocks binding to a driver that would compromise the host,
but we'd also like to avoid reaching that step if possible.  For this
we add a wait_event_timeout() with a short, 1 second timeout, which is
highly effective in allowing empty groups to finish cleanup.

Signed-off-by: Alex Williamson 
---
 drivers/vfio/vfio.c |  151 +--
 1 file changed, 144 insertions(+), 7 deletions(-)

diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
index a25ee4930200..ea786d512faa 100644
--- a/drivers/vfio/vfio.c
+++ b/drivers/vfio/vfio.c
@@ -25,6 +25,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -32,8 +33,10 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
+#include 
 
 #define DRIVER_VERSION "0.3"
 #define DRIVER_AUTHOR  "Alex Williamson "
@@ -95,6 +98,7 @@ struct vfio_group {
boolnoiommu;
struct kvm  *kvm;
struct blocking_notifier_head   notifier;
+   unsigned char   uuid[16];
 };
 
 struct vfio_device {
@@ -352,6 +356,8 @@ static struct vfio_group *vfio_create_group(struct 
iommu_group *iommu_group)
 
group->nb.notifier_call = vfio_iommu_group_notifier;
 
+   generate_random_uuid(group->uuid);
+
/*
 * blocking notifiers acquire a rwsem around registering and hold
 * it around callback.  Therefore, need to register outside of
@@ -728,6 +734,116 @@ static int vfio_group_nb_verify(struct vfio_group *group, 
struct device *dev)
return vfio_dev_viable(dev, group);
 }
 
+#define VFIO_TAG_PREFIX "#vfio_group:"
+
+static char **vfio_find_driver_override(struct device *dev)
+{
+   if (dev_is_pci(dev)) {
+   struct pci_dev *pdev = to_pci_dev(dev);
+   return >driver_override;
+   } else if (dev->bus == _bus_type) {
+   struct platform_device *pdev = to_platform_device(dev);
+   return >driver_override;
+#ifdef CONFIG_ARM_AMBA
+   } else if (dev->bus == _bustype) {
+   struct amba_device *adev = to_amba_device(dev);
+   return >driver_override;
+#endif
+   }
+
+   return NULL;
+}
+
+/*
+ * If we're about to bind to something other than a known whitelisted driver
+ * or known vfio bus driver, try to avert it with driver_override.
+ */
+static void vfio_group_nb_pre_bind(struct vfio_group *group, struct device 
*dev)
+{
+   struct vfio_bus_driver *driver;
+   struct device_driver *drv = ACCESS_ONCE(dev->driver);
+   char **driver_override;
+
+   if (vfio_dev_whitelisted(dev, drv))
+   return; /* Binding to known "innocuous" device/driver */
+
+   mutex_lock(_drivers_lock);
+   list_for_each_entry(driver, _drivers_list, vfio_next) {
+   if (driver->drv == drv) {
+   mutex_unlock(_drivers_lock);
+   return; /* Binding to known vfio bus driver, ok */
+   }
+   }
+   mutex_unlock(_drivers_lock);
+
+   /* Can we stall slightly to let users fall off? */
+   if (list_empty(>device_list)) {
+   if (wait_event_timeout(vfio.release_q,
+   !atomic_read(>container_users), HZ))
+   return;
+   }
+
+   driver_override = vfio_find_driver_override(dev);
+   if (driver_override) {
+   char tag[50], *new = NULL, *old = *driver_override;
+
+   snprintf(tag, sizeof(tag), "%s%pU",
+VFIO_TAG_PREFIX, group->uuid);
+
+   if (old && strstr(old, tag))
+   return; /* block already in place */
+
+   new = kasprintf(GFP_KERNEL, "%s%s", old ? old : "", tag);
+   if (new) {
+   *driver_override = new;
+   kfree(old);
+   vfio_group_get(group);
+   dev_warn(dev, "vfio: Blocking unsafe driver bind\n");
+ 

[PATCH 3/7] vfio: New external user group/file match

[Alex Williamson]

At the point where the kvm-vfio pseudo device wants to release its
vfio group reference, we can't always acquire a new reference to make
that happen.  The group can be in a state where we wouldn't allow a
new reference to be added.  This new helper function allows a caller
to match a file to a group to facilitate this.  Given a file and
group, report if they match.  Thus the caller needs to already have a
group reference to match to the file.  This allows the deletion of a
group without acquiring a new reference.

Signed-off-by: Alex Williamson 
Cc: Paolo Bonzini 
---
 drivers/vfio/vfio.c  |9 +
 include/linux/vfio.h |2 ++
 virt/kvm/vfio.c  |   27 +++
 3 files changed, 30 insertions(+), 8 deletions(-)

diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
index f2e24d5699f2..e6117de60f87 100644
--- a/drivers/vfio/vfio.c
+++ b/drivers/vfio/vfio.c
@@ -1776,6 +1776,15 @@ void vfio_group_put_external_user(struct vfio_group 
*group)
 }
 EXPORT_SYMBOL_GPL(vfio_group_put_external_user);
 
+bool vfio_external_group_match_file(struct vfio_group *test_group,
+   struct file *filep)
+{
+   struct vfio_group *group = filep->private_data;
+
+   return (filep->f_op == _group_fops) && (group == test_group);
+}
+EXPORT_SYMBOL_GPL(vfio_external_group_match_file);
+
 int vfio_external_user_iommu_id(struct vfio_group *group)
 {
return iommu_group_id(group->iommu_group);
diff --git a/include/linux/vfio.h b/include/linux/vfio.h
index edf9b2cad277..9b34d0af5d27 100644
--- a/include/linux/vfio.h
+++ b/include/linux/vfio.h
@@ -97,6 +97,8 @@ extern void vfio_unregister_iommu_driver(
  */
 extern struct vfio_group *vfio_group_get_external_user(struct file *filep);
 extern void vfio_group_put_external_user(struct vfio_group *group);
+extern bool vfio_external_group_match_file(struct vfio_group *group,
+  struct file *filep);
 extern int vfio_external_user_iommu_id(struct vfio_group *group);
 extern long vfio_external_check_extension(struct vfio_group *group,
  unsigned long arg);
diff --git a/virt/kvm/vfio.c b/virt/kvm/vfio.c
index db9036ef8c73..7c14729b1f2c 100644
--- a/virt/kvm/vfio.c
+++ b/virt/kvm/vfio.c
@@ -47,6 +47,22 @@ static struct vfio_group 
*kvm_vfio_group_get_external_user(struct file *filep)
return vfio_group;
 }
 
+static bool kvm_vfio_external_group_match_file(struct vfio_group *group,
+  struct file *filep)
+{
+   bool ret, (*fn)(struct vfio_group *, struct file *);
+
+   fn = symbol_get(vfio_external_group_match_file);
+   if (!fn)
+   return false;
+
+   ret = fn(group, filep);
+
+   symbol_put(vfio_external_group_match_file);
+
+   return ret;
+}
+
 static void kvm_vfio_group_put_external_user(struct vfio_group *vfio_group)
 {
void (*fn)(struct vfio_group *);
@@ -186,18 +202,13 @@ static int kvm_vfio_set_group(struct kvm_device *dev, 
long attr, u64 arg)
if (!f.file)
return -EBADF;
 
-   vfio_group = kvm_vfio_group_get_external_user(f.file);
-   fdput(f);
-
-   if (IS_ERR(vfio_group))
-   return PTR_ERR(vfio_group);
-
ret = -ENOENT;
 
mutex_lock(>lock);
 
list_for_each_entry(kvg, >group_list, node) {
-   if (kvg->vfio_group != vfio_group)
+   if (!kvm_vfio_external_group_match_file(kvg->vfio_group,
+   f.file))
continue;
 
list_del(>node);
@@ -211,7 +222,7 @@ static int kvm_vfio_set_group(struct kvm_device *dev, long 
attr, u64 arg)
 
mutex_unlock(>lock);
 
-   kvm_vfio_group_put_external_user(vfio_group);
+   fdput(f);
 
kvm_vfio_update_coherency(dev);
 

[PATCH 3/7] vfio: New external user group/file match

[Alex Williamson]

At the point where the kvm-vfio pseudo device wants to release its
vfio group reference, we can't always acquire a new reference to make
that happen.  The group can be in a state where we wouldn't allow a
new reference to be added.  This new helper function allows a caller
to match a file to a group to facilitate this.  Given a file and
group, report if they match.  Thus the caller needs to already have a
group reference to match to the file.  This allows the deletion of a
group without acquiring a new reference.

Signed-off-by: Alex Williamson 
Cc: Paolo Bonzini 
---
 drivers/vfio/vfio.c  |9 +
 include/linux/vfio.h |2 ++
 virt/kvm/vfio.c  |   27 +++
 3 files changed, 30 insertions(+), 8 deletions(-)

diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
index f2e24d5699f2..e6117de60f87 100644
--- a/drivers/vfio/vfio.c
+++ b/drivers/vfio/vfio.c
@@ -1776,6 +1776,15 @@ void vfio_group_put_external_user(struct vfio_group 
*group)
 }
 EXPORT_SYMBOL_GPL(vfio_group_put_external_user);
 
+bool vfio_external_group_match_file(struct vfio_group *test_group,
+   struct file *filep)
+{
+   struct vfio_group *group = filep->private_data;
+
+   return (filep->f_op == _group_fops) && (group == test_group);
+}
+EXPORT_SYMBOL_GPL(vfio_external_group_match_file);
+
 int vfio_external_user_iommu_id(struct vfio_group *group)
 {
return iommu_group_id(group->iommu_group);
diff --git a/include/linux/vfio.h b/include/linux/vfio.h
index edf9b2cad277..9b34d0af5d27 100644
--- a/include/linux/vfio.h
+++ b/include/linux/vfio.h
@@ -97,6 +97,8 @@ extern void vfio_unregister_iommu_driver(
  */
 extern struct vfio_group *vfio_group_get_external_user(struct file *filep);
 extern void vfio_group_put_external_user(struct vfio_group *group);
+extern bool vfio_external_group_match_file(struct vfio_group *group,
+  struct file *filep);
 extern int vfio_external_user_iommu_id(struct vfio_group *group);
 extern long vfio_external_check_extension(struct vfio_group *group,
  unsigned long arg);
diff --git a/virt/kvm/vfio.c b/virt/kvm/vfio.c
index db9036ef8c73..7c14729b1f2c 100644
--- a/virt/kvm/vfio.c
+++ b/virt/kvm/vfio.c
@@ -47,6 +47,22 @@ static struct vfio_group 
*kvm_vfio_group_get_external_user(struct file *filep)
return vfio_group;
 }
 
+static bool kvm_vfio_external_group_match_file(struct vfio_group *group,
+  struct file *filep)
+{
+   bool ret, (*fn)(struct vfio_group *, struct file *);
+
+   fn = symbol_get(vfio_external_group_match_file);
+   if (!fn)
+   return false;
+
+   ret = fn(group, filep);
+
+   symbol_put(vfio_external_group_match_file);
+
+   return ret;
+}
+
 static void kvm_vfio_group_put_external_user(struct vfio_group *vfio_group)
 {
void (*fn)(struct vfio_group *);
@@ -186,18 +202,13 @@ static int kvm_vfio_set_group(struct kvm_device *dev, 
long attr, u64 arg)
if (!f.file)
return -EBADF;
 
-   vfio_group = kvm_vfio_group_get_external_user(f.file);
-   fdput(f);
-
-   if (IS_ERR(vfio_group))
-   return PTR_ERR(vfio_group);
-
ret = -ENOENT;
 
mutex_lock(>lock);
 
list_for_each_entry(kvg, >group_list, node) {
-   if (kvg->vfio_group != vfio_group)
+   if (!kvm_vfio_external_group_match_file(kvg->vfio_group,
+   f.file))
continue;
 
list_del(>node);
@@ -211,7 +222,7 @@ static int kvm_vfio_set_group(struct kvm_device *dev, long 
attr, u64 arg)
 
mutex_unlock(>lock);
 
-   kvm_vfio_group_put_external_user(vfio_group);
+   fdput(f);
 
kvm_vfio_update_coherency(dev);
 

[PATCH 6/7] vfio: Register pci, platform, amba, and mdev bus drivers

[Alex Williamson]

Hook into vfio bus driver register/unregister support.

Signed-off-by: Alex Williamson 
Cc: Baptiste Reynal 
Cc: Kirti Wankhede 
Cc: Eric Auger 
---
 drivers/vfio/mdev/vfio_mdev.c |   13 -
 drivers/vfio/pci/vfio_pci.c   |7 +++
 drivers/vfio/platform/vfio_amba.c |   24 +++-
 drivers/vfio/platform/vfio_platform.c |   24 +++-
 4 files changed, 65 insertions(+), 3 deletions(-)

diff --git a/drivers/vfio/mdev/vfio_mdev.c b/drivers/vfio/mdev/vfio_mdev.c
index fa848a701b8b..b73d6f3e8ad5 100644
--- a/drivers/vfio/mdev/vfio_mdev.c
+++ b/drivers/vfio/mdev/vfio_mdev.c
@@ -131,11 +131,22 @@ struct mdev_driver vfio_mdev_driver = {
 
 static int __init vfio_mdev_init(void)
 {
-   return mdev_register_driver(_mdev_driver, THIS_MODULE);
+   int ret;
+
+   ret = mdev_register_driver(_mdev_driver, THIS_MODULE);
+   if (ret)
+   return ret;
+
+   ret = vfio_register_bus_driver(_mdev_driver.driver);
+   if (ret)
+   mdev_unregister_driver(_mdev_driver);
+
+   return ret;
 }
 
 static void __exit vfio_mdev_exit(void)
 {
+   vfio_unregister_bus_driver(_mdev_driver.driver);
mdev_unregister_driver(_mdev_driver);
 }
 
diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
index 324c52e3a1a4..15f4b190ad88 100644
--- a/drivers/vfio/pci/vfio_pci.c
+++ b/drivers/vfio/pci/vfio_pci.c
@@ -1397,6 +1397,7 @@ static void vfio_pci_try_bus_reset(struct vfio_pci_device 
*vdev)
 
 static void __exit vfio_pci_cleanup(void)
 {
+   vfio_unregister_bus_driver(_pci_driver.driver);
pci_unregister_driver(_pci_driver);
vfio_pci_uninit_perm_bits();
 }
@@ -1456,6 +1457,12 @@ static int __init vfio_pci_init(void)
if (ret)
goto out_driver;
 
+   ret = vfio_register_bus_driver(_pci_driver.driver);
+   if (ret) {
+   pci_unregister_driver(_pci_driver);
+   goto out_driver;
+   }
+
vfio_pci_fill_ids();
 
return 0;
diff --git a/drivers/vfio/platform/vfio_amba.c 
b/drivers/vfio/platform/vfio_amba.c
index 31372fbf6c5b..7fd9cb4a6756 100644
--- a/drivers/vfio/platform/vfio_amba.c
+++ b/drivers/vfio/platform/vfio_amba.c
@@ -109,7 +109,29 @@ static int vfio_amba_remove(struct amba_device *adev)
},
 };
 
-module_amba_driver(vfio_amba_driver);
+static void __exit vfio_amba_exit(void)
+{
+   vfio_unregister_bus_driver(_amba_driver.drv);
+   amba_driver_unregister(_amba_driver);
+}
+
+static int __init vfio_amba_init(void)
+{
+   int ret;
+
+   ret = amba_driver_register(_amba_driver);
+   if (ret)
+   return ret;
+
+   ret = vfio_register_bus_driver(_amba_driver.drv);
+   if (ret)
+   amba_driver_unregister(_amba_driver);
+
+   return ret;
+}
+
+module_init(vfio_amba_init);
+module_exit(vfio_amba_exit);
 
 MODULE_VERSION(DRIVER_VERSION);
 MODULE_LICENSE("GPL v2");
diff --git a/drivers/vfio/platform/vfio_platform.c 
b/drivers/vfio/platform/vfio_platform.c
index 6561751a1063..3974dc65e6dc 100644
--- a/drivers/vfio/platform/vfio_platform.c
+++ b/drivers/vfio/platform/vfio_platform.c
@@ -100,7 +100,29 @@ static int vfio_platform_remove(struct platform_device 
*pdev)
},
 };
 
-module_platform_driver(vfio_platform_driver);
+static void __exit vfio_platform_exit(void)
+{
+   vfio_unregister_bus_driver(_platform_driver.driver);
+   platform_driver_unregister(_platform_driver);
+}
+
+static int __init vfio_platform_init(void)
+{
+   int ret;
+
+   ret = platform_driver_register(_platform_driver);
+   if (ret)
+   return ret;
+
+   ret = vfio_register_bus_driver(_platform_driver.driver);
+   if (ret)
+   platform_driver_unregister(_platform_driver);
+
+   return ret;
+}
+
+module_init(vfio_platform_init);
+module_exit(vfio_platform_exit);
 
 MODULE_VERSION(DRIVER_VERSION);
 MODULE_LICENSE("GPL v2");

[PATCH 5/7] vfio: Create interface for vfio bus drivers to register

[Alex Williamson]

Generally we don't know about vfio bus drivers until a device is
added to the vfio-core with vfio_add_group_dev(), this optional
registration with vfio_register_bus_driver() allows vfio-core to
track known drivers.  Our current use for this information is to
know whether a driver is vfio compatible during a bind operation.
For devices on buses with driver_override support, we can use this
linkage to block non-vfio drivers from binding to devices where
the iommu group state would trigger a BUG to avoid host/user
integrity issues.

Signed-off-by: Alex Williamson 
---
 drivers/vfio/vfio.c  |   55 ++
 include/linux/vfio.h |3 +++
 2 files changed, 58 insertions(+)

diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
index e6117de60f87..a25ee4930200 100644
--- a/drivers/vfio/vfio.c
+++ b/drivers/vfio/vfio.c
@@ -43,6 +43,8 @@
struct class*class;
struct list_headiommu_drivers_list;
struct mutexiommu_drivers_lock;
+   struct list_headbus_drivers_list;
+   struct mutexbus_drivers_lock;
struct list_headgroup_list;
struct idr  group_idr;
struct mutexgroup_lock;
@@ -51,6 +53,11 @@
wait_queue_head_t   release_q;
 } vfio;
 
+struct vfio_bus_driver {
+   struct device_driver*drv;
+   struct list_headvfio_next;
+};
+
 struct vfio_iommu_driver {
const struct vfio_iommu_driver_ops  *ops;
struct list_headvfio_next;
@@ -2243,6 +2250,52 @@ int vfio_unregister_notifier(struct device *dev, enum 
vfio_notify_type type,
 }
 EXPORT_SYMBOL(vfio_unregister_notifier);
 
+int vfio_register_bus_driver(struct device_driver *drv)
+{
+   struct vfio_bus_driver *driver, *tmp;
+
+   driver = kzalloc(sizeof(*driver), GFP_KERNEL);
+   if (!driver)
+   return -ENOMEM;
+
+   driver->drv = drv;
+
+   mutex_lock(_drivers_lock);
+
+   /* Check for duplicates */
+   list_for_each_entry(tmp, _drivers_list, vfio_next) {
+   if (tmp->drv == drv) {
+   mutex_unlock(_drivers_lock);
+   kfree(driver);
+   return -EINVAL;
+   }
+   }
+
+   list_add(>vfio_next, _drivers_list);
+
+   mutex_unlock(_drivers_lock);
+
+   return 0;
+}
+EXPORT_SYMBOL_GPL(vfio_register_bus_driver);
+
+void vfio_unregister_bus_driver(struct device_driver *drv)
+{
+   struct vfio_bus_driver *driver;
+
+   mutex_lock(_drivers_lock);
+   list_for_each_entry(driver, _drivers_list, vfio_next) {
+   if (driver->drv == drv) {
+   list_del(>vfio_next);
+   mutex_unlock(_drivers_lock);
+   kfree(driver);
+   return;
+   }
+   }
+   mutex_unlock(_drivers_lock);
+}
+EXPORT_SYMBOL_GPL(vfio_unregister_bus_driver);
+
 /**
  * Module/class support
  */
@@ -2266,8 +2319,10 @@ static int __init vfio_init(void)
idr_init(_idr);
mutex_init(_lock);
mutex_init(_drivers_lock);
+   mutex_init(_drivers_lock);
INIT_LIST_HEAD(_list);
INIT_LIST_HEAD(_drivers_list);
+   INIT_LIST_HEAD(_drivers_list);
init_waitqueue_head(_q);
 
ret = misc_register(_dev);
diff --git a/include/linux/vfio.h b/include/linux/vfio.h
index 9b34d0af5d27..dab0f8105e4a 100644
--- a/include/linux/vfio.h
+++ b/include/linux/vfio.h
@@ -92,6 +92,9 @@ struct vfio_iommu_driver_ops {
 extern void vfio_unregister_iommu_driver(
const struct vfio_iommu_driver_ops *ops);
 
+extern int vfio_register_bus_driver(struct device_driver *drv);
+extern void vfio_unregister_bus_driver(struct device_driver *drv);
+
 /*
  * External user API
  */

[PATCH 6/7] vfio: Register pci, platform, amba, and mdev bus drivers

[Alex Williamson]

Hook into vfio bus driver register/unregister support.

Signed-off-by: Alex Williamson 
Cc: Baptiste Reynal 
Cc: Kirti Wankhede 
Cc: Eric Auger 
---
 drivers/vfio/mdev/vfio_mdev.c |   13 -
 drivers/vfio/pci/vfio_pci.c   |7 +++
 drivers/vfio/platform/vfio_amba.c |   24 +++-
 drivers/vfio/platform/vfio_platform.c |   24 +++-
 4 files changed, 65 insertions(+), 3 deletions(-)

diff --git a/drivers/vfio/mdev/vfio_mdev.c b/drivers/vfio/mdev/vfio_mdev.c
index fa848a701b8b..b73d6f3e8ad5 100644
--- a/drivers/vfio/mdev/vfio_mdev.c
+++ b/drivers/vfio/mdev/vfio_mdev.c
@@ -131,11 +131,22 @@ struct mdev_driver vfio_mdev_driver = {
 
 static int __init vfio_mdev_init(void)
 {
-   return mdev_register_driver(_mdev_driver, THIS_MODULE);
+   int ret;
+
+   ret = mdev_register_driver(_mdev_driver, THIS_MODULE);
+   if (ret)
+   return ret;
+
+   ret = vfio_register_bus_driver(_mdev_driver.driver);
+   if (ret)
+   mdev_unregister_driver(_mdev_driver);
+
+   return ret;
 }
 
 static void __exit vfio_mdev_exit(void)
 {
+   vfio_unregister_bus_driver(_mdev_driver.driver);
mdev_unregister_driver(_mdev_driver);
 }
 
diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
index 324c52e3a1a4..15f4b190ad88 100644
--- a/drivers/vfio/pci/vfio_pci.c
+++ b/drivers/vfio/pci/vfio_pci.c
@@ -1397,6 +1397,7 @@ static void vfio_pci_try_bus_reset(struct vfio_pci_device 
*vdev)
 
 static void __exit vfio_pci_cleanup(void)
 {
+   vfio_unregister_bus_driver(_pci_driver.driver);
pci_unregister_driver(_pci_driver);
vfio_pci_uninit_perm_bits();
 }
@@ -1456,6 +1457,12 @@ static int __init vfio_pci_init(void)
if (ret)
goto out_driver;
 
+   ret = vfio_register_bus_driver(_pci_driver.driver);
+   if (ret) {
+   pci_unregister_driver(_pci_driver);
+   goto out_driver;
+   }
+
vfio_pci_fill_ids();
 
return 0;
diff --git a/drivers/vfio/platform/vfio_amba.c 
b/drivers/vfio/platform/vfio_amba.c
index 31372fbf6c5b..7fd9cb4a6756 100644
--- a/drivers/vfio/platform/vfio_amba.c
+++ b/drivers/vfio/platform/vfio_amba.c
@@ -109,7 +109,29 @@ static int vfio_amba_remove(struct amba_device *adev)
},
 };
 
-module_amba_driver(vfio_amba_driver);
+static void __exit vfio_amba_exit(void)
+{
+   vfio_unregister_bus_driver(_amba_driver.drv);
+   amba_driver_unregister(_amba_driver);
+}
+
+static int __init vfio_amba_init(void)
+{
+   int ret;
+
+   ret = amba_driver_register(_amba_driver);
+   if (ret)
+   return ret;
+
+   ret = vfio_register_bus_driver(_amba_driver.drv);
+   if (ret)
+   amba_driver_unregister(_amba_driver);
+
+   return ret;
+}
+
+module_init(vfio_amba_init);
+module_exit(vfio_amba_exit);
 
 MODULE_VERSION(DRIVER_VERSION);
 MODULE_LICENSE("GPL v2");
diff --git a/drivers/vfio/platform/vfio_platform.c 
b/drivers/vfio/platform/vfio_platform.c
index 6561751a1063..3974dc65e6dc 100644
--- a/drivers/vfio/platform/vfio_platform.c
+++ b/drivers/vfio/platform/vfio_platform.c
@@ -100,7 +100,29 @@ static int vfio_platform_remove(struct platform_device 
*pdev)
},
 };
 
-module_platform_driver(vfio_platform_driver);
+static void __exit vfio_platform_exit(void)
+{
+   vfio_unregister_bus_driver(_platform_driver.driver);
+   platform_driver_unregister(_platform_driver);
+}
+
+static int __init vfio_platform_init(void)
+{
+   int ret;
+
+   ret = platform_driver_register(_platform_driver);
+   if (ret)
+   return ret;
+
+   ret = vfio_register_bus_driver(_platform_driver.driver);
+   if (ret)
+   platform_driver_unregister(_platform_driver);
+
+   return ret;
+}
+
+module_init(vfio_platform_init);
+module_exit(vfio_platform_exit);
 
 MODULE_VERSION(DRIVER_VERSION);
 MODULE_LICENSE("GPL v2");

[PATCH 5/7] vfio: Create interface for vfio bus drivers to register

[Alex Williamson]

Generally we don't know about vfio bus drivers until a device is
added to the vfio-core with vfio_add_group_dev(), this optional
registration with vfio_register_bus_driver() allows vfio-core to
track known drivers.  Our current use for this information is to
know whether a driver is vfio compatible during a bind operation.
For devices on buses with driver_override support, we can use this
linkage to block non-vfio drivers from binding to devices where
the iommu group state would trigger a BUG to avoid host/user
integrity issues.

Signed-off-by: Alex Williamson 
---
 drivers/vfio/vfio.c  |   55 ++
 include/linux/vfio.h |3 +++
 2 files changed, 58 insertions(+)

diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
index e6117de60f87..a25ee4930200 100644
--- a/drivers/vfio/vfio.c
+++ b/drivers/vfio/vfio.c
@@ -43,6 +43,8 @@
struct class*class;
struct list_headiommu_drivers_list;
struct mutexiommu_drivers_lock;
+   struct list_headbus_drivers_list;
+   struct mutexbus_drivers_lock;
struct list_headgroup_list;
struct idr  group_idr;
struct mutexgroup_lock;
@@ -51,6 +53,11 @@
wait_queue_head_t   release_q;
 } vfio;
 
+struct vfio_bus_driver {
+   struct device_driver*drv;
+   struct list_headvfio_next;
+};
+
 struct vfio_iommu_driver {
const struct vfio_iommu_driver_ops  *ops;
struct list_headvfio_next;
@@ -2243,6 +2250,52 @@ int vfio_unregister_notifier(struct device *dev, enum 
vfio_notify_type type,
 }
 EXPORT_SYMBOL(vfio_unregister_notifier);
 
+int vfio_register_bus_driver(struct device_driver *drv)
+{
+   struct vfio_bus_driver *driver, *tmp;
+
+   driver = kzalloc(sizeof(*driver), GFP_KERNEL);
+   if (!driver)
+   return -ENOMEM;
+
+   driver->drv = drv;
+
+   mutex_lock(_drivers_lock);
+
+   /* Check for duplicates */
+   list_for_each_entry(tmp, _drivers_list, vfio_next) {
+   if (tmp->drv == drv) {
+   mutex_unlock(_drivers_lock);
+   kfree(driver);
+   return -EINVAL;
+   }
+   }
+
+   list_add(>vfio_next, _drivers_list);
+
+   mutex_unlock(_drivers_lock);
+
+   return 0;
+}
+EXPORT_SYMBOL_GPL(vfio_register_bus_driver);
+
+void vfio_unregister_bus_driver(struct device_driver *drv)
+{
+   struct vfio_bus_driver *driver;
+
+   mutex_lock(_drivers_lock);
+   list_for_each_entry(driver, _drivers_list, vfio_next) {
+   if (driver->drv == drv) {
+   list_del(>vfio_next);
+   mutex_unlock(_drivers_lock);
+   kfree(driver);
+   return;
+   }
+   }
+   mutex_unlock(_drivers_lock);
+}
+EXPORT_SYMBOL_GPL(vfio_unregister_bus_driver);
+
 /**
  * Module/class support
  */
@@ -2266,8 +2319,10 @@ static int __init vfio_init(void)
idr_init(_idr);
mutex_init(_lock);
mutex_init(_drivers_lock);
+   mutex_init(_drivers_lock);
INIT_LIST_HEAD(_list);
INIT_LIST_HEAD(_drivers_list);
+   INIT_LIST_HEAD(_drivers_list);
init_waitqueue_head(_q);
 
ret = misc_register(_dev);
diff --git a/include/linux/vfio.h b/include/linux/vfio.h
index 9b34d0af5d27..dab0f8105e4a 100644
--- a/include/linux/vfio.h
+++ b/include/linux/vfio.h
@@ -92,6 +92,9 @@ struct vfio_iommu_driver_ops {
 extern void vfio_unregister_iommu_driver(
const struct vfio_iommu_driver_ops *ops);
 
+extern int vfio_register_bus_driver(struct device_driver *drv);
+extern void vfio_unregister_bus_driver(struct device_driver *drv);
+
 /*
  * External user API
  */

[PATCH 4/7] iommu: Add driver-not-bound notification

[Alex Williamson]

The driver core supports a BUS_NOTIFY_DRIVER_NOT_BOUND notification
sent if a driver fails to bind to a device.  Extend IOMMU group
notifications to include a version of this.

Signed-off-by: Alex Williamson 
Cc: Joerg Roedel 
---
 drivers/iommu/iommu.c |2 ++
 include/linux/iommu.h |1 +
 2 files changed, 3 insertions(+)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 3b67144dead2..cf6051db4208 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -1113,6 +1113,8 @@ static int iommu_bus_notifier(struct notifier_block *nb,
case BUS_NOTIFY_UNBOUND_DRIVER:
group_action = IOMMU_GROUP_NOTIFY_UNBOUND_DRIVER;
break;
+   case BUS_NOTIFY_DRIVER_NOT_BOUND:
+   group_action = IOMMU_GROUP_NOTIFY_DRIVER_NOT_BOUND;
}
 
if (group_action)
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 2e4de0deee53..54a0eb96da25 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -268,6 +268,7 @@ static inline void iommu_device_set_fwnode(struct 
iommu_device *iommu,
 #define IOMMU_GROUP_NOTIFY_BOUND_DRIVER4 /* Post Driver bind */
 #define IOMMU_GROUP_NOTIFY_UNBIND_DRIVER   5 /* Pre Driver unbind */
 #define IOMMU_GROUP_NOTIFY_UNBOUND_DRIVER  6 /* Post Driver unbind */
+#define IOMMU_GROUP_NOTIFY_DRIVER_NOT_BOUND7 /* Driver bind failed */
 
 extern int bus_set_iommu(struct bus_type *bus, const struct iommu_ops *ops);
 extern bool iommu_present(struct bus_type *bus);

[PATCH 4/7] iommu: Add driver-not-bound notification

[Alex Williamson]

The driver core supports a BUS_NOTIFY_DRIVER_NOT_BOUND notification
sent if a driver fails to bind to a device.  Extend IOMMU group
notifications to include a version of this.

Signed-off-by: Alex Williamson 
Cc: Joerg Roedel 
---
 drivers/iommu/iommu.c |2 ++
 include/linux/iommu.h |1 +
 2 files changed, 3 insertions(+)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 3b67144dead2..cf6051db4208 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -1113,6 +1113,8 @@ static int iommu_bus_notifier(struct notifier_block *nb,
case BUS_NOTIFY_UNBOUND_DRIVER:
group_action = IOMMU_GROUP_NOTIFY_UNBOUND_DRIVER;
break;
+   case BUS_NOTIFY_DRIVER_NOT_BOUND:
+   group_action = IOMMU_GROUP_NOTIFY_DRIVER_NOT_BOUND;
}
 
if (group_action)
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 2e4de0deee53..54a0eb96da25 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -268,6 +268,7 @@ static inline void iommu_device_set_fwnode(struct 
iommu_device *iommu,
 #define IOMMU_GROUP_NOTIFY_BOUND_DRIVER4 /* Post Driver bind */
 #define IOMMU_GROUP_NOTIFY_UNBIND_DRIVER   5 /* Pre Driver unbind */
 #define IOMMU_GROUP_NOTIFY_UNBOUND_DRIVER  6 /* Post Driver unbind */
+#define IOMMU_GROUP_NOTIFY_DRIVER_NOT_BOUND7 /* Driver bind failed */
 
 extern int bus_set_iommu(struct bus_type *bus, const struct iommu_ops *ops);
 extern bool iommu_present(struct bus_type *bus);

[PATCH 2/7] kvm-vfio: Decouple only when we match a group

[Alex Williamson]

Unset-KVM and decrement-assignment only when we find the group in our
list.  Otherwise we can get out of sync if the user triggers this for
groups that aren't currently on our list.

Signed-off-by: Alex Williamson 
Cc: Paolo Bonzini 
---
 virt/kvm/vfio.c |6 ++
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/virt/kvm/vfio.c b/virt/kvm/vfio.c
index d32f239eb471..db9036ef8c73 100644
--- a/virt/kvm/vfio.c
+++ b/virt/kvm/vfio.c
@@ -201,18 +201,16 @@ static int kvm_vfio_set_group(struct kvm_device *dev, 
long attr, u64 arg)
continue;
 
list_del(>node);
+   kvm_arch_end_assignment(dev->kvm);
+   kvm_vfio_group_set_kvm(kvg->vfio_group, NULL);
kvm_vfio_group_put_external_user(kvg->vfio_group);
kfree(kvg);
ret = 0;
break;
}
 
-   kvm_arch_end_assignment(dev->kvm);
-
mutex_unlock(>lock);
 
-   kvm_vfio_group_set_kvm(vfio_group, NULL);
-
kvm_vfio_group_put_external_user(vfio_group);
 
kvm_vfio_update_coherency(dev);

[PATCH 1/7] vfio: Fix group release deadlock

[Alex Williamson]

If vfio_iommu_group_notifier() acquires a group reference and that
reference becomes the last reference to the group, then vfio_group_put
introduces a deadlock code path where we're trying to unregister from
the iommu notifier chain from within a callout of that chain.  Use a
work_struct to release this reference asynchronously.

Signed-off-by: Alex Williamson 
---
 drivers/vfio/vfio.c |   37 -
 1 file changed, 36 insertions(+), 1 deletion(-)

diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
index 561084ab387f..f2e24d5699f2 100644
--- a/drivers/vfio/vfio.c
+++ b/drivers/vfio/vfio.c
@@ -423,6 +423,34 @@ static void vfio_group_put(struct vfio_group *group)
kref_put_mutex(>kref, vfio_group_release, _lock);
 }
 
+struct vfio_group_put_work {
+   struct work_struct work;
+   struct vfio_group *group;
+};
+
+static void vfio_group_put_bg(struct work_struct *work)
+{
+   struct vfio_group_put_work *do_work;
+
+   do_work = container_of(work, struct vfio_group_put_work, work);
+
+   vfio_group_put(do_work->group);
+   kfree(do_work);
+}
+
+static void vfio_group_schedule_put(struct vfio_group *group)
+{
+   struct vfio_group_put_work *do_work;
+
+   do_work = kmalloc(sizeof(*do_work), GFP_KERNEL);
+   if (WARN_ON(!do_work))
+   return;
+
+   INIT_WORK(_work->work, vfio_group_put_bg);
+   do_work->group = group;
+   schedule_work(_work->work);
+}
+
 /* Assume group_lock or group reference is held */
 static void vfio_group_get(struct vfio_group *group)
 {
@@ -762,7 +790,14 @@ static int vfio_iommu_group_notifier(struct notifier_block 
*nb,
break;
}
 
-   vfio_group_put(group);
+   /*
+* If we're the last reference to the group, the group will be
+* released, which includes unregistering the iommu group notifier.
+* We hold a read-lock on that notifier list, unregistering needs
+* a write-lock... deadlock.  Release our reference asyncronously
+* to avoid that situation.
+*/
+   vfio_group_schedule_put(group);
return NOTIFY_OK;
 }
 

[PATCH 2/7] kvm-vfio: Decouple only when we match a group

[Alex Williamson]

Unset-KVM and decrement-assignment only when we find the group in our
list.  Otherwise we can get out of sync if the user triggers this for
groups that aren't currently on our list.

Signed-off-by: Alex Williamson 
Cc: Paolo Bonzini 
---
 virt/kvm/vfio.c |6 ++
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/virt/kvm/vfio.c b/virt/kvm/vfio.c
index d32f239eb471..db9036ef8c73 100644
--- a/virt/kvm/vfio.c
+++ b/virt/kvm/vfio.c
@@ -201,18 +201,16 @@ static int kvm_vfio_set_group(struct kvm_device *dev, 
long attr, u64 arg)
continue;
 
list_del(>node);
+   kvm_arch_end_assignment(dev->kvm);
+   kvm_vfio_group_set_kvm(kvg->vfio_group, NULL);
kvm_vfio_group_put_external_user(kvg->vfio_group);
kfree(kvg);
ret = 0;
break;
}
 
-   kvm_arch_end_assignment(dev->kvm);
-
mutex_unlock(>lock);
 
-   kvm_vfio_group_set_kvm(vfio_group, NULL);
-
kvm_vfio_group_put_external_user(vfio_group);
 
kvm_vfio_update_coherency(dev);

[PATCH 1/7] vfio: Fix group release deadlock

[Alex Williamson]

If vfio_iommu_group_notifier() acquires a group reference and that
reference becomes the last reference to the group, then vfio_group_put
introduces a deadlock code path where we're trying to unregister from
the iommu notifier chain from within a callout of that chain.  Use a
work_struct to release this reference asynchronously.

Signed-off-by: Alex Williamson 
---
 drivers/vfio/vfio.c |   37 -
 1 file changed, 36 insertions(+), 1 deletion(-)

diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
index 561084ab387f..f2e24d5699f2 100644
--- a/drivers/vfio/vfio.c
+++ b/drivers/vfio/vfio.c
@@ -423,6 +423,34 @@ static void vfio_group_put(struct vfio_group *group)
kref_put_mutex(>kref, vfio_group_release, _lock);
 }
 
+struct vfio_group_put_work {
+   struct work_struct work;
+   struct vfio_group *group;
+};
+
+static void vfio_group_put_bg(struct work_struct *work)
+{
+   struct vfio_group_put_work *do_work;
+
+   do_work = container_of(work, struct vfio_group_put_work, work);
+
+   vfio_group_put(do_work->group);
+   kfree(do_work);
+}
+
+static void vfio_group_schedule_put(struct vfio_group *group)
+{
+   struct vfio_group_put_work *do_work;
+
+   do_work = kmalloc(sizeof(*do_work), GFP_KERNEL);
+   if (WARN_ON(!do_work))
+   return;
+
+   INIT_WORK(_work->work, vfio_group_put_bg);
+   do_work->group = group;
+   schedule_work(_work->work);
+}
+
 /* Assume group_lock or group reference is held */
 static void vfio_group_get(struct vfio_group *group)
 {
@@ -762,7 +790,14 @@ static int vfio_iommu_group_notifier(struct notifier_block 
*nb,
break;
}
 
-   vfio_group_put(group);
+   /*
+* If we're the last reference to the group, the group will be
+* released, which includes unregistering the iommu group notifier.
+* We hold a read-lock on that notifier list, unregistering needs
+* a write-lock... deadlock.  Release our reference asyncronously
+* to avoid that situation.
+*/
+   vfio_group_schedule_put(group);
return NOTIFY_OK;
 }
 

[PATCH 0/7] vfio: Fix release ordering races and use driver_override

[Alex Williamson]

VM hotplug testing reveals a number of races in the vfio device,
group, container shutdown path, some attributed to libvirt's ask/take
unplug behavior and some long standing with groups potentially
composed of multiple devices, where each device can be independently
bound to drivers.  Libvirt's ask/take behavior is a result of the
asynchronous nature of PCI hotplug, libvirt registers a hot-unplug
request (ask), which is acknowledged almost immediately and then
proceeds to try to unbind the device from the vfio bus driver (take).
This sets us off on racing paths where we allow the device to be
released from the group much like would happen in groups with multiple
devices, while the group and container are torn down separately.
These races are addressed in the first 3 patches of this series.

The long standing issue with removing devices from in-use groups is
that we feel that the system is compromised if we allow user and host
devices within the same non-isolated group.  This triggers a BUG_ON
when we detect this condition after the rogue driver binding.  Since
that code was put in place we've added driver_override support for
all of the physical buses supported by vfio, giving us a way to block
binding to such compromising drivers.  We finally enable that in the
latter 4 patches of this series, minding that we need to allow
re-binding to non-compromising drivers, and also noting that a small
synchronization stall is effective in eliminating the need for this
blocking in the more common singleton device group case.

Reviews, comments, and acks appreciated.  Thanks,

Alex
---

Alex Williamson (7):
  vfio: Fix group release deadlock
  kvm-vfio: Decouple only when we match a group
  vfio: New external user group/file match
  iommu: Add driver-not-bound notification
  vfio: Create interface for vfio bus drivers to register
  vfio: Register pci, platform, amba, and mdev bus drivers
  vfio: Use driver_override to avert binding to compromising drivers


 drivers/iommu/iommu.c |2 
 drivers/vfio/mdev/vfio_mdev.c |   13 ++
 drivers/vfio/pci/vfio_pci.c   |7 +
 drivers/vfio/platform/vfio_amba.c |   24 +++
 drivers/vfio/platform/vfio_platform.c |   24 +++
 drivers/vfio/vfio.c   |  252 -
 include/linux/iommu.h |1 
 include/linux/vfio.h  |5 +
 virt/kvm/vfio.c   |   33 +++-
 9 files changed, 338 insertions(+), 23 deletions(-)

[PATCH 0/7] vfio: Fix release ordering races and use driver_override

[Alex Williamson]

VM hotplug testing reveals a number of races in the vfio device,
group, container shutdown path, some attributed to libvirt's ask/take
unplug behavior and some long standing with groups potentially
composed of multiple devices, where each device can be independently
bound to drivers.  Libvirt's ask/take behavior is a result of the
asynchronous nature of PCI hotplug, libvirt registers a hot-unplug
request (ask), which is acknowledged almost immediately and then
proceeds to try to unbind the device from the vfio bus driver (take).
This sets us off on racing paths where we allow the device to be
released from the group much like would happen in groups with multiple
devices, while the group and container are torn down separately.
These races are addressed in the first 3 patches of this series.

The long standing issue with removing devices from in-use groups is
that we feel that the system is compromised if we allow user and host
devices within the same non-isolated group.  This triggers a BUG_ON
when we detect this condition after the rogue driver binding.  Since
that code was put in place we've added driver_override support for
all of the physical buses supported by vfio, giving us a way to block
binding to such compromising drivers.  We finally enable that in the
latter 4 patches of this series, minding that we need to allow
re-binding to non-compromising drivers, and also noting that a small
synchronization stall is effective in eliminating the need for this
blocking in the more common singleton device group case.

Reviews, comments, and acks appreciated.  Thanks,

Alex
---

Alex Williamson (7):
  vfio: Fix group release deadlock
  kvm-vfio: Decouple only when we match a group
  vfio: New external user group/file match
  iommu: Add driver-not-bound notification
  vfio: Create interface for vfio bus drivers to register
  vfio: Register pci, platform, amba, and mdev bus drivers
  vfio: Use driver_override to avert binding to compromising drivers


 drivers/iommu/iommu.c |2 
 drivers/vfio/mdev/vfio_mdev.c |   13 ++
 drivers/vfio/pci/vfio_pci.c   |7 +
 drivers/vfio/platform/vfio_amba.c |   24 +++
 drivers/vfio/platform/vfio_platform.c |   24 +++
 drivers/vfio/vfio.c   |  252 -
 include/linux/iommu.h |1 
 include/linux/vfio.h  |5 +
 virt/kvm/vfio.c   |   33 +++-
 9 files changed, 338 insertions(+), 23 deletions(-)

Re: [PATCH 08/17] dts: include documentation for the RISC-V interrupt controllers

[Wesley Terpstra]

Ugh. Clicked reply without being done writing the reply!

On Thu, Jun 8, 2017 at 3:52 AM, Mark Rutland  wrote:
> Edge vs level, active high vs active low. Typically some of these are
> programmable, and are described as flags in the interrupt-specifier.
>
> See the examples in:
>
> Documentation/devicetree/bindings/interrupt-controller/interrupts.txt

Ok. Those are not really relevant to the PLIC. It has a flat interrupt
namespace and the way you handle all four kinds of interrupts in the
driver is uniform. I don't think we want to architecturally expose
this to the operating system.

> So do any CSRs affect the state of the PLIC? If it's just MMIO, the
> mention of CSRs above is just a little confusing.

HLIC = CSR only. PLIC = MMIO only.

> It might be best to just say "The PLIC is connect to the HLIC embedded
> in each RISC-V core".

Fair enough.

> It sounds like we'd need these to distinguish edge/level interrupts,
> unless that's fixed at integration time and you can determine it from
> the PLIC itself?

They are fixed at integration time and the PLIC driver does not need to care.

> It's not too much of a problem, but if we end up having to change
> anything else from the proposed bindings, those trees are going to
> require updates anyway.

I'll talk to the other stakeholders.

> Please update the binding to explicitly define the ordering requirement.

The ordering requirement is that the first interrupts-extended entry
corresponds to the first context, second to second, etc. If a context
is unused for some reason, that's when you need a -1. The contexts are
linear and contiguous in the MMIO address map.

> Does this mean that you expect Linux logical CPU IDs to be equal to
> physical CPU IDs in all cases?

No. There is no 'physical CPU ID' anyway, except the mhartid CSR which
is unavailable to linux. The SBI conveys your CPU ID by passing it as
the first argument to the linux kernel.

The interrupts-extended in the PLIC uses a phandle to reference the
matching CPU in DTS. The num in cpu@ only need correspond to the
first register argument to the kernel.

If for some reason there end up too many -1 holes in the PLIC (b/c you
virtualized a 128-core machine down to say 2), you can always
virtualize the PLIC device and provide a matching simplified DTB.

Re: [PATCH 08/17] dts: include documentation for the RISC-V interrupt controllers

[Wesley Terpstra]

Ugh. Clicked reply without being done writing the reply!

On Thu, Jun 8, 2017 at 3:52 AM, Mark Rutland  wrote:
> Edge vs level, active high vs active low. Typically some of these are
> programmable, and are described as flags in the interrupt-specifier.
>
> See the examples in:
>
> Documentation/devicetree/bindings/interrupt-controller/interrupts.txt

Ok. Those are not really relevant to the PLIC. It has a flat interrupt
namespace and the way you handle all four kinds of interrupts in the
driver is uniform. I don't think we want to architecturally expose
this to the operating system.

> So do any CSRs affect the state of the PLIC? If it's just MMIO, the
> mention of CSRs above is just a little confusing.

HLIC = CSR only. PLIC = MMIO only.

> It might be best to just say "The PLIC is connect to the HLIC embedded
> in each RISC-V core".

Fair enough.

> It sounds like we'd need these to distinguish edge/level interrupts,
> unless that's fixed at integration time and you can determine it from
> the PLIC itself?

They are fixed at integration time and the PLIC driver does not need to care.

> It's not too much of a problem, but if we end up having to change
> anything else from the proposed bindings, those trees are going to
> require updates anyway.

I'll talk to the other stakeholders.

> Please update the binding to explicitly define the ordering requirement.

The ordering requirement is that the first interrupts-extended entry
corresponds to the first context, second to second, etc. If a context
is unused for some reason, that's when you need a -1. The contexts are
linear and contiguous in the MMIO address map.

> Does this mean that you expect Linux logical CPU IDs to be equal to
> physical CPU IDs in all cases?

No. There is no 'physical CPU ID' anyway, except the mhartid CSR which
is unavailable to linux. The SBI conveys your CPU ID by passing it as
the first argument to the linux kernel.

The interrupts-extended in the PLIC uses a phandle to reference the
matching CPU in DTS. The num in cpu@ only need correspond to the
first register argument to the kernel.

If for some reason there end up too many -1 holes in the PLIC (b/c you
virtualized a 128-core machine down to say 2), you can always
virtualize the PLIC device and provide a matching simplified DTB.

Re: [PATCH 15/16] platform/x86: wmi-mof: New driver to expose embedded WMI MOF metadata

[Pali Rohár]

On Friday 09 June 2017 17:46:12 mario.limoncie...@dell.com wrote:
> > -Original Message-
> > From: Pali Rohár [mailto:pali.ro...@gmail.com]
> > Sent: Wednesday, June 7, 2017 3:50 PM
> > To: Limonciello, Mario 
> > Cc: l...@amacapital.net; dvh...@infradead.org; platform-driver-
> > x...@vger.kernel.org; andriy.shevche...@linux.intel.com;
> > l...@kernel.org; r...@rjwysocki.net; linux-kernel@vger.kernel.org;
> > linux-a...@vger.kernel.org Subject: Re: [PATCH 15/16]
> > platform/x86: wmi-mof: New driver to expose embedded WMI MOF
> > metadata
> > 
> > On Wednesday 07 June 2017 22:23:08 mario.limoncie...@dell.com wrote:
> > > > -Original Message-
> > > > From: Pali Rohár [mailto:pali.ro...@gmail.com]
> > > > Sent: Wednesday, June 7, 2017 12:39 PM
> > > > To: Limonciello, Mario 
> > > > Cc: l...@amacapital.net; dvh...@infradead.org; platform-driver-
> > > > x...@vger.kernel.org; andriy.shevche...@linux.intel.com;
> > > > l...@kernel.org; r...@rjwysocki.net;
> > > > linux-kernel@vger.kernel.org; linux-a...@vger.kernel.org
> > > > Subject: Re: [PATCH 15/16]
> > > > platform/x86: wmi-mof: New driver to expose embedded WMI MOF
> > > > metadata
> > > > 
> > > > On Tuesday 06 June 2017 15:56:21 Pali Rohár wrote:
> > > > > On Tuesday 06 June 2017 13:46:16 mario.limoncie...@dell.com
> > > > > wrote:
> > > > > > 2) On my system when you expand the arguments for "void
> > > > > > DoBFn" the source doesn't describe individual arguments
> > > > > > like you do. Again this might not matter to MOF parsing
> > > > > > tools but wanted to let you know in case it does.
> > > > > 
> > > > > I know, this part is missing. Order of arguments are only in
> > > > > ID qualifier and not sorted + in/out de-duplicated.
> > > > 
> > > > Implemented! Now arguments are correctly placed based on ID
> > > > qualifier.
> > > 
> > > I think it's still off a little though.
> > > 
> > > What I'm getting back now from bmf2mof is:
> > >   void DoBFn([in, Description("Fn buf"), out] BDat Data);
> > > 
> > > Whereas source puts Description as the last argument:
> > >   void DoBFn([in, out, Description("Fn buf")] BDat Data);
> > 
> > In BMOF from my Latitude E6440 there are specified two parameters
> > with index 0. One with qualifiers ("in", Description("Fn buf"))
> > and one with ("out", Description("Fn buf")). I think you have
> > similar/same data in BMOF.
> > 
> > In my bmf2mof I just combined those two parameters into one (when
> > name, type and index matches) and concatenate also qualifiers with
> > removing duplicates.
> > 
> > Do not know what is correct way, but I think qualifiers are just
> > unordered set. MS decompiler probably put "in" and "out" qualifiers
> > before any other for better readability.
> 
> Have you tried to run it through mofcomp.exe and then decompile again
> with bmf2mof?  As long as it's coming out the same you're probably
> right.

Yes, bmf2mof+mofcomp.exe+bmf2mof gives same output as just bmf2mof.

> > > > > > source:
> > > > > > void DoBFn([in, out, Description("Fn buf")] BDat Data);
> > > > > > 
> > > > > > bmf2mof:
> > > > > > void doBFn([in, Description("Fn buf"), ID(0)] BDat Data,
> > > > > > [out, Description("Fn buf"), ID(0)] BDat Data);
> > > > 
> > > > --
> > > > Pali Rohár
> > > > pali.ro...@gmail.com
> > 
> > --
> > Pali Rohár
> > pali.ro...@gmail.com

-- 
Pali Rohár
pali.ro...@gmail.com


signature.asc
Description: This is a digitally signed message part.

Re: [PATCH 15/16] platform/x86: wmi-mof: New driver to expose embedded WMI MOF metadata

[Pali Rohár]

On Friday 09 June 2017 17:46:12 mario.limoncie...@dell.com wrote:
> > -Original Message-
> > From: Pali Rohár [mailto:pali.ro...@gmail.com]
> > Sent: Wednesday, June 7, 2017 3:50 PM
> > To: Limonciello, Mario 
> > Cc: l...@amacapital.net; dvh...@infradead.org; platform-driver-
> > x...@vger.kernel.org; andriy.shevche...@linux.intel.com;
> > l...@kernel.org; r...@rjwysocki.net; linux-kernel@vger.kernel.org;
> > linux-a...@vger.kernel.org Subject: Re: [PATCH 15/16]
> > platform/x86: wmi-mof: New driver to expose embedded WMI MOF
> > metadata
> > 
> > On Wednesday 07 June 2017 22:23:08 mario.limoncie...@dell.com wrote:
> > > > -Original Message-
> > > > From: Pali Rohár [mailto:pali.ro...@gmail.com]
> > > > Sent: Wednesday, June 7, 2017 12:39 PM
> > > > To: Limonciello, Mario 
> > > > Cc: l...@amacapital.net; dvh...@infradead.org; platform-driver-
> > > > x...@vger.kernel.org; andriy.shevche...@linux.intel.com;
> > > > l...@kernel.org; r...@rjwysocki.net;
> > > > linux-kernel@vger.kernel.org; linux-a...@vger.kernel.org
> > > > Subject: Re: [PATCH 15/16]
> > > > platform/x86: wmi-mof: New driver to expose embedded WMI MOF
> > > > metadata
> > > > 
> > > > On Tuesday 06 June 2017 15:56:21 Pali Rohár wrote:
> > > > > On Tuesday 06 June 2017 13:46:16 mario.limoncie...@dell.com
> > > > > wrote:
> > > > > > 2) On my system when you expand the arguments for "void
> > > > > > DoBFn" the source doesn't describe individual arguments
> > > > > > like you do. Again this might not matter to MOF parsing
> > > > > > tools but wanted to let you know in case it does.
> > > > > 
> > > > > I know, this part is missing. Order of arguments are only in
> > > > > ID qualifier and not sorted + in/out de-duplicated.
> > > > 
> > > > Implemented! Now arguments are correctly placed based on ID
> > > > qualifier.
> > > 
> > > I think it's still off a little though.
> > > 
> > > What I'm getting back now from bmf2mof is:
> > >   void DoBFn([in, Description("Fn buf"), out] BDat Data);
> > > 
> > > Whereas source puts Description as the last argument:
> > >   void DoBFn([in, out, Description("Fn buf")] BDat Data);
> > 
> > In BMOF from my Latitude E6440 there are specified two parameters
> > with index 0. One with qualifiers ("in", Description("Fn buf"))
> > and one with ("out", Description("Fn buf")). I think you have
> > similar/same data in BMOF.
> > 
> > In my bmf2mof I just combined those two parameters into one (when
> > name, type and index matches) and concatenate also qualifiers with
> > removing duplicates.
> > 
> > Do not know what is correct way, but I think qualifiers are just
> > unordered set. MS decompiler probably put "in" and "out" qualifiers
> > before any other for better readability.
> 
> Have you tried to run it through mofcomp.exe and then decompile again
> with bmf2mof?  As long as it's coming out the same you're probably
> right.

Yes, bmf2mof+mofcomp.exe+bmf2mof gives same output as just bmf2mof.

> > > > > > source:
> > > > > > void DoBFn([in, out, Description("Fn buf")] BDat Data);
> > > > > > 
> > > > > > bmf2mof:
> > > > > > void doBFn([in, Description("Fn buf"), ID(0)] BDat Data,
> > > > > > [out, Description("Fn buf"), ID(0)] BDat Data);
> > > > 
> > > > --
> > > > Pali Rohár
> > > > pali.ro...@gmail.com
> > 
> > --
> > Pali Rohár
> > pali.ro...@gmail.com

-- 
Pali Rohár
pali.ro...@gmail.com


signature.asc
Description: This is a digitally signed message part.

[PATCH] net: aquantia: atlantic: remove declaration of hw_atl_utils_hw_set_power

[Philippe Reynes]

This function is not defined, so no need to declare it.

As I don't have the hardware, I'd be very pleased if
someone may test this patch.

Signed-off-by: Philippe Reynes 
---
 .../aquantia/atlantic/hw_atl/hw_atl_utils.h|3 ---
 1 files changed, 0 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_utils.h 
b/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_utils.h
index b8e3d88..a66aee5 100644
--- a/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_utils.h
+++ b/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_utils.h
@@ -193,9 +193,6 @@ int hw_atl_utils_hw_get_regs(struct aq_hw_s *self,
 struct aq_hw_caps_s *aq_hw_caps,
 u32 *regs_buff);
 
-int hw_atl_utils_hw_get_settings(struct aq_hw_s *self,
-struct ethtool_cmd *cmd);
-
 int hw_atl_utils_hw_set_power(struct aq_hw_s *self,
  unsigned int power_state);
 
-- 
1.7.4.4

[PATCH] net: aquantia: atlantic: remove declaration of hw_atl_utils_hw_set_power

[Philippe Reynes]

This function is not defined, so no need to declare it.

As I don't have the hardware, I'd be very pleased if
someone may test this patch.

Signed-off-by: Philippe Reynes 
---
 .../aquantia/atlantic/hw_atl/hw_atl_utils.h|3 ---
 1 files changed, 0 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_utils.h 
b/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_utils.h
index b8e3d88..a66aee5 100644
--- a/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_utils.h
+++ b/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_utils.h
@@ -193,9 +193,6 @@ int hw_atl_utils_hw_get_regs(struct aq_hw_s *self,
 struct aq_hw_caps_s *aq_hw_caps,
 u32 *regs_buff);
 
-int hw_atl_utils_hw_get_settings(struct aq_hw_s *self,
-struct ethtool_cmd *cmd);
-
 int hw_atl_utils_hw_set_power(struct aq_hw_s *self,
  unsigned int power_state);
 
-- 
1.7.4.4

Re: [PATCH] mtd: nand: fsl_ifc: remove unused variable

[Pavel Machek]

On Fri 2017-06-09 12:47:43, Arnd Bergmann wrote:
> This one was accidentally introduced without any references,
> and it causes a harmless warning:
> 
> drivers/mtd/nand/fsl_ifc_nand.c: In function 'fsl_ifc_read_page':
> drivers/mtd/nand/fsl_ifc_nand.c:696:7: error: unused variable 'res' 
> [-Werror=unused-variable]
> 
> Fixes: 79f40cc12fd3 ("mtd: nand: fsl_ifc: fix handing of bit flips in erased 
> pages")
> Signed-off-by: Arnd Bergmann 

Acked-by: Pavel Machek 

Thanks!
Pavel

> ---
>  drivers/mtd/nand/fsl_ifc_nand.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/mtd/nand/fsl_ifc_nand.c b/drivers/mtd/nand/fsl_ifc_nand.c
> index 14ef2f4524ac..d1c4538f870f 100644
> --- a/drivers/mtd/nand/fsl_ifc_nand.c
> +++ b/drivers/mtd/nand/fsl_ifc_nand.c
> @@ -693,8 +693,6 @@ static int fsl_ifc_read_page(struct mtd_info *mtd, struct 
> nand_chip *chip,
>   fsl_ifc_read_buf(mtd, chip->oob_poi, mtd->oobsize);
>  
>   if (ctrl->nand_stat & IFC_NAND_EVTER_STAT_ECCER) {
> - int res;
> -
>   if (!oob_required)
>   fsl_ifc_read_buf(mtd, chip->oob_poi, mtd->oobsize);
>  

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) 
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html


signature.asc
Description: Digital signature

Re: [PATCH] mtd: nand: fsl_ifc: remove unused variable

[Pavel Machek]

On Fri 2017-06-09 12:47:43, Arnd Bergmann wrote:
> This one was accidentally introduced without any references,
> and it causes a harmless warning:
> 
> drivers/mtd/nand/fsl_ifc_nand.c: In function 'fsl_ifc_read_page':
> drivers/mtd/nand/fsl_ifc_nand.c:696:7: error: unused variable 'res' 
> [-Werror=unused-variable]
> 
> Fixes: 79f40cc12fd3 ("mtd: nand: fsl_ifc: fix handing of bit flips in erased 
> pages")
> Signed-off-by: Arnd Bergmann 

Acked-by: Pavel Machek 

Thanks!
Pavel

> ---
>  drivers/mtd/nand/fsl_ifc_nand.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/mtd/nand/fsl_ifc_nand.c b/drivers/mtd/nand/fsl_ifc_nand.c
> index 14ef2f4524ac..d1c4538f870f 100644
> --- a/drivers/mtd/nand/fsl_ifc_nand.c
> +++ b/drivers/mtd/nand/fsl_ifc_nand.c
> @@ -693,8 +693,6 @@ static int fsl_ifc_read_page(struct mtd_info *mtd, struct 
> nand_chip *chip,
>   fsl_ifc_read_buf(mtd, chip->oob_poi, mtd->oobsize);
>  
>   if (ctrl->nand_stat & IFC_NAND_EVTER_STAT_ECCER) {
> - int res;
> -
>   if (!oob_required)
>   fsl_ifc_read_buf(mtd, chip->oob_poi, mtd->oobsize);
>  

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) 
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html


signature.asc
Description: Digital signature

Re: [PATCH 08/17] dts: include documentation for the RISC-V interrupt controllers

[Wesley Terpstra]

On Thu, Jun 8, 2017 at 3:52 AM, Mark Rutland  wrote:
>> What flags?
>
> Edge vs level, active high vs active low. Typically some of these are
> programmable, and are described as flags in the interrupt-specifier.
>
> See the examples in:
>
> Documentation/devicetree/bindings/interrupt-controller/interrupts.txt

Ok. Those are not applicable to the PLIC.

>
>> > Are all HLIC interrupts edge triggered (or level triggered)?
>>
>> HLIC = level. PLIC = both.
>
> Ok. Given that, flags aren't necessary for the HLIC, and the
> interrupt-specifier is fine as-is.
>
>> >> > +RISC-V cores typically include a PLIC, which route interrupts from 
>> >> > multiple
>> >> > +devices to multiple hart contexts.  The PLIC is connected to the 
>> >> > interrupt
>> >> > +controller embedded in a RISC-V core via the interrupt-related CSRs.
>> >
>> > Do you mean that the PLIC is connected to the HLIC, or that the HLIC is
>> > also managed in part through CSRs?
>>
>> Both. The HLIC is entirely manager through CSRs. The PLIC is managed
>> through a memory mapped interface. The PLIC is attached to the HLIC.
>
> So do any CSRs affect the state of the PLIC? If it's just MMIO, the
> mention of CSRs above is just a little confusing.
>
> It might be best to just say "The PLIC is connect to the HLIC embedded
> in each RISC-V core".
>
>> >> > +Required properties:
>> >> > +- compatible : "riscv,plic0"
>> >> > +- #address-cells : should be <0>
>> >> > +- #interrupt-cells : should be <1>
>> >
>> > As with the HLIC, what about the flags?
>>
>> Still unsure what flags we're talking about.
>
> As covered above for the HLIC.
>
> It sounds like we'd need these to distinguish edge/level interrupts,
> unless that's fixed at integration time and you can determine it from
> the PLIC itself?
>
>> >> > +- riscv,ndev : Specifies the number of interrupts attached to the PLIC
>> >
>> > Why do we need to know this?
>> >
>> > I suspect this ia actually the number of interrupts implemented in the
>> > PLIC, rather than the number of interrupts attached. i.e. the PLIC can
>> > be implemented with a subset of the potential registers/bits. Is that
>> > correct?
>>
>> You're in principle correct, although these are probably always the same.
>
> For now, perhaps. Let's not embed an assumption we cannot guarantee.
>
>> > If so, something like "riscv,num-interrupts" would be better, along with
>> > a clearer description.
>>
>> Uhm. I suppose we can change this. However, it would requires changes
>> to quite a number of riscv repositories. I believe this is also
>> included in the riscv platform specification. A better description is
>> easy, do we really need to change the key?
>
> It's not too much of a problem, but if we end up having to change
> anything else from the proposed bindings, those trees are going to
> require updates anyway.
>
> If we can, I would like to change this to keep things as clear as
> possible from the outset.
>
> [...]
>
>> > You will need to be explicit about the order of interrupts in this
>> > property. i.e. which interrupt is routed to which context?
>>
>> Yes. Order and position matter.
>
> Ok.
>
> Please update the binding to explicitly define the ordering requirement.
>
> [...]
>
>> > Also, please consider how you will handle the case when the Linux
>> > logical CPU ID is not the same as the physical ID, and how you will
>> > handle physical IDs being sparse.
>>
>> We already deal with this. If the interrupt is '-1', we skip it.
>> That's done in plic.c:
>> if (parent.args[0] == -1) continue; // skip context holes
>
> If this is what we expect people to do, it needs to be documented in the
> binding.
>
> Does this mean that you expect Linux logical CPU IDs to be equal to
> physical CPU IDs in all cases?
>
> That's going to be painful for very sparse ID ranges, and won't work for
> cases like kexec/kdump where you cannot guarantee which physical CPU
> will end up being CPU0 in the new kernel.
>
> I would strongly advise that you explicitly describe the relationship
> using phandles to CPU nodes.
>
> I would also advise that you try to decouple the physical CPU IDs and
> Linux logical IDs. While assuming they're the same might simplify things
> today, it will create longer term maintenance problems and get in the
> way of a number of features.
>
> Thanks,
> Mark.

Re: [PATCH 08/17] dts: include documentation for the RISC-V interrupt controllers

[Wesley Terpstra]

On Thu, Jun 8, 2017 at 3:52 AM, Mark Rutland  wrote:
>> What flags?
>
> Edge vs level, active high vs active low. Typically some of these are
> programmable, and are described as flags in the interrupt-specifier.
>
> See the examples in:
>
> Documentation/devicetree/bindings/interrupt-controller/interrupts.txt

Ok. Those are not applicable to the PLIC.

>
>> > Are all HLIC interrupts edge triggered (or level triggered)?
>>
>> HLIC = level. PLIC = both.
>
> Ok. Given that, flags aren't necessary for the HLIC, and the
> interrupt-specifier is fine as-is.
>
>> >> > +RISC-V cores typically include a PLIC, which route interrupts from 
>> >> > multiple
>> >> > +devices to multiple hart contexts.  The PLIC is connected to the 
>> >> > interrupt
>> >> > +controller embedded in a RISC-V core via the interrupt-related CSRs.
>> >
>> > Do you mean that the PLIC is connected to the HLIC, or that the HLIC is
>> > also managed in part through CSRs?
>>
>> Both. The HLIC is entirely manager through CSRs. The PLIC is managed
>> through a memory mapped interface. The PLIC is attached to the HLIC.
>
> So do any CSRs affect the state of the PLIC? If it's just MMIO, the
> mention of CSRs above is just a little confusing.
>
> It might be best to just say "The PLIC is connect to the HLIC embedded
> in each RISC-V core".
>
>> >> > +Required properties:
>> >> > +- compatible : "riscv,plic0"
>> >> > +- #address-cells : should be <0>
>> >> > +- #interrupt-cells : should be <1>
>> >
>> > As with the HLIC, what about the flags?
>>
>> Still unsure what flags we're talking about.
>
> As covered above for the HLIC.
>
> It sounds like we'd need these to distinguish edge/level interrupts,
> unless that's fixed at integration time and you can determine it from
> the PLIC itself?
>
>> >> > +- riscv,ndev : Specifies the number of interrupts attached to the PLIC
>> >
>> > Why do we need to know this?
>> >
>> > I suspect this ia actually the number of interrupts implemented in the
>> > PLIC, rather than the number of interrupts attached. i.e. the PLIC can
>> > be implemented with a subset of the potential registers/bits. Is that
>> > correct?
>>
>> You're in principle correct, although these are probably always the same.
>
> For now, perhaps. Let's not embed an assumption we cannot guarantee.
>
>> > If so, something like "riscv,num-interrupts" would be better, along with
>> > a clearer description.
>>
>> Uhm. I suppose we can change this. However, it would requires changes
>> to quite a number of riscv repositories. I believe this is also
>> included in the riscv platform specification. A better description is
>> easy, do we really need to change the key?
>
> It's not too much of a problem, but if we end up having to change
> anything else from the proposed bindings, those trees are going to
> require updates anyway.
>
> If we can, I would like to change this to keep things as clear as
> possible from the outset.
>
> [...]
>
>> > You will need to be explicit about the order of interrupts in this
>> > property. i.e. which interrupt is routed to which context?
>>
>> Yes. Order and position matter.
>
> Ok.
>
> Please update the binding to explicitly define the ordering requirement.
>
> [...]
>
>> > Also, please consider how you will handle the case when the Linux
>> > logical CPU ID is not the same as the physical ID, and how you will
>> > handle physical IDs being sparse.
>>
>> We already deal with this. If the interrupt is '-1', we skip it.
>> That's done in plic.c:
>> if (parent.args[0] == -1) continue; // skip context holes
>
> If this is what we expect people to do, it needs to be documented in the
> binding.
>
> Does this mean that you expect Linux logical CPU IDs to be equal to
> physical CPU IDs in all cases?
>
> That's going to be painful for very sparse ID ranges, and won't work for
> cases like kexec/kdump where you cannot guarantee which physical CPU
> will end up being CPU0 in the new kernel.
>
> I would strongly advise that you explicitly describe the relationship
> using phandles to CPU nodes.
>
> I would also advise that you try to decouple the physical CPU IDs and
> Linux logical IDs. While assuming they're the same might simplify things
> today, it will create longer term maintenance problems and get in the
> way of a number of features.
>
> Thanks,
> Mark.

Re: [PATCH v8 14/34] ARM: dts: imx6-sabreauto: add the ADV7180 video decoder

[Pavel Machek]

On Thu 2017-06-08 13:36:12, Steve Longerbeam wrote:
> 
> 
> On 06/08/2017 01:25 PM, Tim Harvey wrote:
> >
> >
> >Steve,
> >
> >You need to remove the fim node now that you've moved this to V4L2 controls.
> >
> 
> Yep, I caught this just after sending the v8 patchset. I'll send
> a v9 of this patch.

This needs ack from devicetree people, then it can be merged. Can you
be a bit more forceful getting the ack?

I don't think it makes sense to resubmit v9 before that. This is not a
rocket science.

Thanks,
Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) 
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html


signature.asc
Description: Digital signature

Re: [PATCH v8 14/34] ARM: dts: imx6-sabreauto: add the ADV7180 video decoder

[Pavel Machek]

On Thu 2017-06-08 13:36:12, Steve Longerbeam wrote:
> 
> 
> On 06/08/2017 01:25 PM, Tim Harvey wrote:
> >
> >
> >Steve,
> >
> >You need to remove the fim node now that you've moved this to V4L2 controls.
> >
> 
> Yep, I caught this just after sending the v8 patchset. I'll send
> a v9 of this patch.

This needs ack from devicetree people, then it can be merged. Can you
be a bit more forceful getting the ack?

I don't think it makes sense to resubmit v9 before that. This is not a
rocket science.

Thanks,
Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) 
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html


signature.asc
Description: Digital signature

[git pull] first batch of ufs fixes

[Al Viro]

That's just the obvious backport fodder; I'm pretty sure that there
will be more - definitely so wrt performance and quite possibly correctness
as well.

The following changes since commit a8c39544a6eb2093c04afd5005b6192bd0e880c6:

  osf_wait4(): fix infoleak (2017-05-21 13:10:07 -0400)

are available in the git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs.git for-linus

for you to fetch changes up to babef37dccbaa49249a22bae9150686815d7be71:

  excessive checks in ufs_write_failed() and ufs_evict_inode() (2017-06-09 
16:28:01 -0400)


Al Viro (7):
  ufs: restore proper tail allocation
  fix ufs_isblockset()
  ufs: restore maintaining ->i_blocks
  ufs: set correct ->s_maxsize
  ufs_extend_tail(): fix the braino in calling conventions of 
ufs_new_fragments()
  ufs_getfrag_block(): we only grab ->truncate_mutex on block creation path
  excessive checks in ufs_write_failed() and ufs_evict_inode()

 fs/stat.c   |  1 +
 fs/ufs/balloc.c | 26 +-
 fs/ufs/inode.c  | 27 +++
 fs/ufs/super.c  | 18 ++
 fs/ufs/util.h   | 10 +++---
 5 files changed, 62 insertions(+), 20 deletions(-)

[git pull] first batch of ufs fixes

[Al Viro]

That's just the obvious backport fodder; I'm pretty sure that there
will be more - definitely so wrt performance and quite possibly correctness
as well.

The following changes since commit a8c39544a6eb2093c04afd5005b6192bd0e880c6:

  osf_wait4(): fix infoleak (2017-05-21 13:10:07 -0400)

are available in the git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs.git for-linus

for you to fetch changes up to babef37dccbaa49249a22bae9150686815d7be71:

  excessive checks in ufs_write_failed() and ufs_evict_inode() (2017-06-09 
16:28:01 -0400)


Al Viro (7):
  ufs: restore proper tail allocation
  fix ufs_isblockset()
  ufs: restore maintaining ->i_blocks
  ufs: set correct ->s_maxsize
  ufs_extend_tail(): fix the braino in calling conventions of 
ufs_new_fragments()
  ufs_getfrag_block(): we only grab ->truncate_mutex on block creation path
  excessive checks in ufs_write_failed() and ufs_evict_inode()

 fs/stat.c   |  1 +
 fs/ufs/balloc.c | 26 +-
 fs/ufs/inode.c  | 27 +++
 fs/ufs/super.c  | 18 ++
 fs/ufs/util.h   | 10 +++---
 5 files changed, 62 insertions(+), 20 deletions(-)

[PATCH] [media] davinci/dm644x: work around ccdc_update_raw_params trainwreck

[Arnd Bergmann]

Now that the davinci drivers can be enabled in compile tests on other
architectures, I ran into this warning on a 64-bit build:

drivers/media/platform/davinci/dm644x_ccdc.c: In function 
'ccdc_update_raw_params':
drivers/media/platform/davinci/dm644x_ccdc.c:279:7: error: cast to pointer from 
integer of different size [-Werror=int-to-pointer-cast]

While that looks fairly harmless (it would be fine on 32-bit), it was
just the tip of the iceberg:

- The function constantly mixes up pointers and phys_addr_t numbers
- This is part of a 'VPFE_CMD_S_CCDC_RAW_PARAMS' ioctl command that is
  described as an 'experimental ioctl that will change in future kernels',
  but if we have users that probably won't happen.
- The code to allocate the table never gets called after we copy_from_user
  the user input over the kernel settings, and then compare them
  for inequality.
- We then go on to use an address provided by user space as both the
  __user pointer for input and pass it through phys_to_virt to come up
  with a kernel pointer to copy the data to. This looks like a trivially
  exploitable root hole.

This patch disables all the obviously broken code, by zeroing out the
sensitive data provided by user space. I also fix the type confusion
here. If we think the ioctl has no stable users, we could consider
just removing it instead.

Fixes: 5f15fbb68fd7 ("V4L/DVB (12251): v4l: dm644x ccdc module for vpfe capture 
driver")
Signed-off-by: Arnd Bergmann 
---
 drivers/media/platform/davinci/dm644x_ccdc.c | 40 +---
 1 file changed, 25 insertions(+), 15 deletions(-)

diff --git a/drivers/media/platform/davinci/dm644x_ccdc.c 
b/drivers/media/platform/davinci/dm644x_ccdc.c
index 740fbc7a8c14..1b42f50cad38 100644
--- a/drivers/media/platform/davinci/dm644x_ccdc.c
+++ b/drivers/media/platform/davinci/dm644x_ccdc.c
@@ -236,10 +236,22 @@ static int ccdc_update_raw_params(struct 
ccdc_config_params_raw *raw_params)
 {
struct ccdc_config_params_raw *config_params =
_cfg.bayer.config_params;
-   unsigned int *fpc_virtaddr = NULL;
-   unsigned int *fpc_physaddr = NULL;
+   unsigned int *fpc_virtaddr;
+   phys_addr_t fpc_physaddr;
 
memcpy(config_params, raw_params, sizeof(*raw_params));
+
+   /*
+* FIXME: the code to copy the fault_pxl settings was present
+*in the original version but clearly could never
+*work and will interpret user-provided data in
+*dangerous ways. Let's disable it completely to be
+*on the safe side.
+*/
+   config_params->fault_pxl.enable = 0;
+   config_params->fault_pxl.fp_num = 0;
+   config_params->fault_pxl.fpc_table_addr = 0;
+
/*
 * allocate memory for fault pixel table and copy the user
 * values to the table
@@ -247,16 +259,15 @@ static int ccdc_update_raw_params(struct 
ccdc_config_params_raw *raw_params)
if (!config_params->fault_pxl.enable)
return 0;
 
-   fpc_physaddr = (unsigned int *)config_params->fault_pxl.fpc_table_addr;
-   fpc_virtaddr = (unsigned int *)phys_to_virt(
-   (unsigned long)fpc_physaddr);
+   fpc_physaddr = config_params->fault_pxl.fpc_table_addr;
+   fpc_virtaddr = (unsigned int *)phys_to_virt(fpc_physaddr);
/*
 * Allocate memory for FPC table if current
 * FPC table buffer is not big enough to
 * accommodate FPC Number requested
 */
if (raw_params->fault_pxl.fp_num != config_params->fault_pxl.fp_num) {
-   if (fpc_physaddr != NULL) {
+   if (fpc_physaddr) {
free_pages((unsigned long)fpc_virtaddr,
   get_order
   (config_params->fault_pxl.fp_num *
@@ -270,13 +281,12 @@ static int ccdc_update_raw_params(struct 
ccdc_config_params_raw *raw_params)
 fault_pxl.fp_num *
 FP_NUM_BYTES));
 
-   if (fpc_virtaddr == NULL) {
+   if (fpc_virtaddr) {
dev_dbg(ccdc_cfg.dev,
"\nUnable to allocate memory for FPC");
return -EFAULT;
}
-   fpc_physaddr =
-   (unsigned int *)virt_to_phys((void *)fpc_virtaddr);
+   fpc_physaddr = virt_to_phys(fpc_virtaddr);
}
 
/* Copy number of fault pixels and FPC table */
@@ -287,7 +297,7 @@ static int ccdc_update_raw_params(struct 
ccdc_config_params_raw *raw_params)
dev_dbg(ccdc_cfg.dev, "\n copy_from_user failed");
return -EFAULT;
}
-   config_params->fault_pxl.fpc_table_addr = (unsigned long)fpc_physaddr;
+   config_params->fault_pxl.fpc_table_addr = fpc_physaddr;
return 0;
 

[PATCH] [media] davinci/dm644x: work around ccdc_update_raw_params trainwreck

[Arnd Bergmann]

Now that the davinci drivers can be enabled in compile tests on other
architectures, I ran into this warning on a 64-bit build:

drivers/media/platform/davinci/dm644x_ccdc.c: In function 
'ccdc_update_raw_params':
drivers/media/platform/davinci/dm644x_ccdc.c:279:7: error: cast to pointer from 
integer of different size [-Werror=int-to-pointer-cast]

While that looks fairly harmless (it would be fine on 32-bit), it was
just the tip of the iceberg:

- The function constantly mixes up pointers and phys_addr_t numbers
- This is part of a 'VPFE_CMD_S_CCDC_RAW_PARAMS' ioctl command that is
  described as an 'experimental ioctl that will change in future kernels',
  but if we have users that probably won't happen.
- The code to allocate the table never gets called after we copy_from_user
  the user input over the kernel settings, and then compare them
  for inequality.
- We then go on to use an address provided by user space as both the
  __user pointer for input and pass it through phys_to_virt to come up
  with a kernel pointer to copy the data to. This looks like a trivially
  exploitable root hole.

This patch disables all the obviously broken code, by zeroing out the
sensitive data provided by user space. I also fix the type confusion
here. If we think the ioctl has no stable users, we could consider
just removing it instead.

Fixes: 5f15fbb68fd7 ("V4L/DVB (12251): v4l: dm644x ccdc module for vpfe capture 
driver")
Signed-off-by: Arnd Bergmann 
---
 drivers/media/platform/davinci/dm644x_ccdc.c | 40 +---
 1 file changed, 25 insertions(+), 15 deletions(-)

diff --git a/drivers/media/platform/davinci/dm644x_ccdc.c 
b/drivers/media/platform/davinci/dm644x_ccdc.c
index 740fbc7a8c14..1b42f50cad38 100644
--- a/drivers/media/platform/davinci/dm644x_ccdc.c
+++ b/drivers/media/platform/davinci/dm644x_ccdc.c
@@ -236,10 +236,22 @@ static int ccdc_update_raw_params(struct 
ccdc_config_params_raw *raw_params)
 {
struct ccdc_config_params_raw *config_params =
_cfg.bayer.config_params;
-   unsigned int *fpc_virtaddr = NULL;
-   unsigned int *fpc_physaddr = NULL;
+   unsigned int *fpc_virtaddr;
+   phys_addr_t fpc_physaddr;
 
memcpy(config_params, raw_params, sizeof(*raw_params));
+
+   /*
+* FIXME: the code to copy the fault_pxl settings was present
+*in the original version but clearly could never
+*work and will interpret user-provided data in
+*dangerous ways. Let's disable it completely to be
+*on the safe side.
+*/
+   config_params->fault_pxl.enable = 0;
+   config_params->fault_pxl.fp_num = 0;
+   config_params->fault_pxl.fpc_table_addr = 0;
+
/*
 * allocate memory for fault pixel table and copy the user
 * values to the table
@@ -247,16 +259,15 @@ static int ccdc_update_raw_params(struct 
ccdc_config_params_raw *raw_params)
if (!config_params->fault_pxl.enable)
return 0;
 
-   fpc_physaddr = (unsigned int *)config_params->fault_pxl.fpc_table_addr;
-   fpc_virtaddr = (unsigned int *)phys_to_virt(
-   (unsigned long)fpc_physaddr);
+   fpc_physaddr = config_params->fault_pxl.fpc_table_addr;
+   fpc_virtaddr = (unsigned int *)phys_to_virt(fpc_physaddr);
/*
 * Allocate memory for FPC table if current
 * FPC table buffer is not big enough to
 * accommodate FPC Number requested
 */
if (raw_params->fault_pxl.fp_num != config_params->fault_pxl.fp_num) {
-   if (fpc_physaddr != NULL) {
+   if (fpc_physaddr) {
free_pages((unsigned long)fpc_virtaddr,
   get_order
   (config_params->fault_pxl.fp_num *
@@ -270,13 +281,12 @@ static int ccdc_update_raw_params(struct 
ccdc_config_params_raw *raw_params)
 fault_pxl.fp_num *
 FP_NUM_BYTES));
 
-   if (fpc_virtaddr == NULL) {
+   if (fpc_virtaddr) {
dev_dbg(ccdc_cfg.dev,
"\nUnable to allocate memory for FPC");
return -EFAULT;
}
-   fpc_physaddr =
-   (unsigned int *)virt_to_phys((void *)fpc_virtaddr);
+   fpc_physaddr = virt_to_phys(fpc_virtaddr);
}
 
/* Copy number of fault pixels and FPC table */
@@ -287,7 +297,7 @@ static int ccdc_update_raw_params(struct 
ccdc_config_params_raw *raw_params)
dev_dbg(ccdc_cfg.dev, "\n copy_from_user failed");
return -EFAULT;
}
-   config_params->fault_pxl.fpc_table_addr = (unsigned long)fpc_physaddr;
+   config_params->fault_pxl.fpc_table_addr = fpc_physaddr;
return 0;
 }
 
@@ -295,13 

[PATCH] drm/core: Fail atomic IOCTL with no CRTC state but with signaling.

[Andrey Grodzovsky]

Problem:
While running IGT kms_atomic_transition test suite i encountered
a hang in drmHandleEvent immidietly follwoing an atomic_commit.
After dumping the atomic state I relized that in this case there was
not even one CRTC attached to the state and only disabled
planes. This probably due to a commit which hadn't changed any property
which would require attaching crtc state. This means drmHandleEvent
will never wake up from read since without CRTC in atomic state
the event fd will not be singnaled.
This point to a bug in IGT but also DRM should gracefully
fail  such scenario so no hang on user side will happen.

Fix:
Explicitly fail by failing atomic_commit early in
drm_mode_atomic_commit where such problem can be identified.

Signed-off-by: Andrey Grodzovsky 
---
 drivers/gpu/drm/drm_atomic.c | 13 -
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index a567310..32eae1c 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -1933,7 +1933,7 @@ static int prepare_crtc_signaling(struct drm_device *dev,
 {
struct drm_crtc *crtc;
struct drm_crtc_state *crtc_state;
-   int i, ret;
+   int i, c = 0, ret;
 
if (arg->flags & DRM_MODE_ATOMIC_TEST_ONLY)
return 0;
@@ -1994,8 +1994,17 @@ static int prepare_crtc_signaling(struct drm_device *dev,
 
crtc_state->event->base.fence = fence;
}
+
+   c++;
}
 
+   /*
+* Having this flag means user mode pends on event which will never
+* reach due to lack of at least one CRTC for signaling
+*/
+   if (c == 0 && (arg->flags & DRM_MODE_PAGE_FLIP_EVENT))
+   return -EINVAL;
+
return 0;
 }
 
@@ -2179,6 +2188,8 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
drm_mode_object_unreference(obj);
}
 
+
+
ret = prepare_crtc_signaling(dev, state, arg, file_priv, _state,
 _fences);
if (ret)
-- 
2.7.4

[PATCH] drm/core: Fail atomic IOCTL with no CRTC state but with signaling.

[Andrey Grodzovsky]

Problem:
While running IGT kms_atomic_transition test suite i encountered
a hang in drmHandleEvent immidietly follwoing an atomic_commit.
After dumping the atomic state I relized that in this case there was
not even one CRTC attached to the state and only disabled
planes. This probably due to a commit which hadn't changed any property
which would require attaching crtc state. This means drmHandleEvent
will never wake up from read since without CRTC in atomic state
the event fd will not be singnaled.
This point to a bug in IGT but also DRM should gracefully
fail  such scenario so no hang on user side will happen.

Fix:
Explicitly fail by failing atomic_commit early in
drm_mode_atomic_commit where such problem can be identified.

Signed-off-by: Andrey Grodzovsky 
---
 drivers/gpu/drm/drm_atomic.c | 13 -
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index a567310..32eae1c 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -1933,7 +1933,7 @@ static int prepare_crtc_signaling(struct drm_device *dev,
 {
struct drm_crtc *crtc;
struct drm_crtc_state *crtc_state;
-   int i, ret;
+   int i, c = 0, ret;
 
if (arg->flags & DRM_MODE_ATOMIC_TEST_ONLY)
return 0;
@@ -1994,8 +1994,17 @@ static int prepare_crtc_signaling(struct drm_device *dev,
 
crtc_state->event->base.fence = fence;
}
+
+   c++;
}
 
+   /*
+* Having this flag means user mode pends on event which will never
+* reach due to lack of at least one CRTC for signaling
+*/
+   if (c == 0 && (arg->flags & DRM_MODE_PAGE_FLIP_EVENT))
+   return -EINVAL;
+
return 0;
 }
 
@@ -2179,6 +2188,8 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
drm_mode_object_unreference(obj);
}
 
+
+
ret = prepare_crtc_signaling(dev, state, arg, file_priv, _state,
 _fences);
if (ret)
-- 
2.7.4

Re: [PATCH v2] firmware: fix sending -ERESTARTSYS due to signal on fallback

[Luis R. Rodriguez]

On Thu, Jun 8, 2017 at 6:33 PM, Luis R. Rodriguez  wrote:
> On Thu, Jun 8, 2017 at 6:14 PM, Andy Lutomirski  wrote:
>> That's what I meant, but I said it unclearly.  I meant that, if we're
>> going to start allowing interruption, we would need to audit all the
>> callers.  Ugh.
>
> There are actually two audits worth evaluating if what we've concluded
> is fair game:
>
>   a) firmware sync calls on interruptible paths
>   b) use of swait / old interruptible waits on sysfs paths

And as I noted in the other thread -- another possible issue could be
any swait / interruptable wait on init or probe. Provided any child
completes and the kernel code for wait handler does abort, that
request would be terminated. This could for instance happen at bootup
as modules load and any child from the loader terminates.

We already have Coccinelle grammar to hunt for "though shall not
request firmware on init or probe", such SmPL grammar could be in turn
be repruposed to hunt for these types of conditions.

  Luis

Re: [PATCH v2] firmware: fix sending -ERESTARTSYS due to signal on fallback

[Luis R. Rodriguez]

On Thu, Jun 8, 2017 at 6:33 PM, Luis R. Rodriguez  wrote:
> On Thu, Jun 8, 2017 at 6:14 PM, Andy Lutomirski  wrote:
>> That's what I meant, but I said it unclearly.  I meant that, if we're
>> going to start allowing interruption, we would need to audit all the
>> callers.  Ugh.
>
> There are actually two audits worth evaluating if what we've concluded
> is fair game:
>
>   a) firmware sync calls on interruptible paths
>   b) use of swait / old interruptible waits on sysfs paths

And as I noted in the other thread -- another possible issue could be
any swait / interruptable wait on init or probe. Provided any child
completes and the kernel code for wait handler does abort, that
request would be terminated. This could for instance happen at bootup
as modules load and any child from the loader terminates.

We already have Coccinelle grammar to hunt for "though shall not
request firmware on init or probe", such SmPL grammar could be in turn
be repruposed to hunt for these types of conditions.

  Luis

Re: [linux-sunxi] Re: [PATCH v2 01/11] dt-bindings: update the binding for Allwinner H3 TVE support

[Jernej Škrabec]

Hi!

Dne petek, 09. junij 2017 ob 18:51:02 CEST je Icenowy Zheng napisal(a):
> 于 2017年6月10日 GMT+08:00 上午12:49:15, Maxime Ripard  写到:
> >On Wed, Jun 07, 2017 at 04:48:50PM +0800, Icenowy Zheng wrote:
> >> >> @@ -189,6 +211,8 @@ supported.
> >> >> 
> >> >>  Required properties:
> >> >>- compatible: value must be one of:
> >> >>  * allwinner,sun8i-v3s-de2-mixer
> >> >> 
> >> >> +* allwinner,sun8i-h3-de2-mixer0
> >> >> +* allwinner,sun8i-h3-de2-mixer1
> >> >
> >> >Again, please explain why we need to have different compatibles
> >> >here. If it's only about the number of planes, this should be dealt
> >> >with a property, not a compatible.
> >> 
> >> Only mixer0 has "VEP" and write-back support, at least on H3.
> >
> >What is that VEP? writeback support can also be expressed by a
> >property.
> 
> I don't know what VEP is...

VEP is probably Video Enhancement Processor (?). Although I'm not sure if that 
is really mixer specific. Icenowy, can you explain where did you get this info? 

Best regards,
Jernej

> 
> But we are really facing different cores, like sun50i-a64-mmc
> and sun50i-a64-emmc.
> 
> >Maxime
> 
> --
> You received this message because you are subscribed to the Google Groups
> "linux-sunxi" group. To unsubscribe from this group and stop receiving
> emails from it, send an email to linux-sunxi+unsubscr...@googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.

Re: [linux-sunxi] Re: [PATCH v2 01/11] dt-bindings: update the binding for Allwinner H3 TVE support

[Jernej Škrabec]

Hi!

Dne petek, 09. junij 2017 ob 18:51:02 CEST je Icenowy Zheng napisal(a):
> 于 2017年6月10日 GMT+08:00 上午12:49:15, Maxime Ripard  写到:
> >On Wed, Jun 07, 2017 at 04:48:50PM +0800, Icenowy Zheng wrote:
> >> >> @@ -189,6 +211,8 @@ supported.
> >> >> 
> >> >>  Required properties:
> >> >>- compatible: value must be one of:
> >> >>  * allwinner,sun8i-v3s-de2-mixer
> >> >> 
> >> >> +* allwinner,sun8i-h3-de2-mixer0
> >> >> +* allwinner,sun8i-h3-de2-mixer1
> >> >
> >> >Again, please explain why we need to have different compatibles
> >> >here. If it's only about the number of planes, this should be dealt
> >> >with a property, not a compatible.
> >> 
> >> Only mixer0 has "VEP" and write-back support, at least on H3.
> >
> >What is that VEP? writeback support can also be expressed by a
> >property.
> 
> I don't know what VEP is...

VEP is probably Video Enhancement Processor (?). Although I'm not sure if that 
is really mixer specific. Icenowy, can you explain where did you get this info? 

Best regards,
Jernej

> 
> But we are really facing different cores, like sun50i-a64-mmc
> and sun50i-a64-emmc.
> 
> >Maxime
> 
> --
> You received this message because you are subscribed to the Google Groups
> "linux-sunxi" group. To unsubscribe from this group and stop receiving
> emails from it, send an email to linux-sunxi+unsubscr...@googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.

Re: [PATCH 1/3] mm: add vm_insert_mixed_mkwrite()

[Dan Williams]

On Wed, Jun 7, 2017 at 1:48 PM, Ross Zwisler
 wrote:
> To be able to use the common 4k zero page in DAX we need to have our PTE
> fault path look more like our PMD fault path where a PTE entry can be
> marked as dirty and writeable as it is first inserted, rather than waiting
> for a follow-up dax_pfn_mkwrite() => finish_mkwrite_fault() call.
>
> Right now we can rely on having a dax_pfn_mkwrite() call because we can
> distinguish between these two cases in do_wp_page():
>
> case 1: 4k zero page => writable DAX storage
> case 2: read-only DAX storage => writeable DAX storage
>
> This distinction is made by via vm_normal_page().  vm_normal_page() returns
> false for the common 4k zero page, though, just as it does for DAX ptes.
> Instead of special casing the DAX + 4k zero page case, we will simplify our
> DAX PTE page fault sequence so that it matches our DAX PMD sequence, and
> get rid of dax_pfn_mkwrite() completely.
>
> This means that insert_pfn() needs to follow the lead of insert_pfn_pmd()
> and allow us to pass in a 'mkwrite' flag.  If 'mkwrite' is set insert_pfn()
> will do the work that was previously done by wp_page_reuse() as part of the
> dax_pfn_mkwrite() call path.
>
> Signed-off-by: Ross Zwisler 
> ---
>  include/linux/mm.h |  9 +++--
>  mm/memory.c| 21 ++---
>  2 files changed, 21 insertions(+), 9 deletions(-)
>
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index b892e95..11e323a 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -2294,10 +2294,15 @@ int vm_insert_pfn(struct vm_area_struct *vma, 
> unsigned long addr,
> unsigned long pfn);
>  int vm_insert_pfn_prot(struct vm_area_struct *vma, unsigned long addr,
> unsigned long pfn, pgprot_t pgprot);
> -int vm_insert_mixed(struct vm_area_struct *vma, unsigned long addr,
> -   pfn_t pfn);
> +int vm_insert_mixed_mkwrite(struct vm_area_struct *vma, unsigned long addr,
> +   pfn_t pfn, bool mkwrite);

Are there any other planned public users of vm_insert_mixed_mkwrite()
that would pass false? I think not.

>  int vm_iomap_memory(struct vm_area_struct *vma, phys_addr_t start, unsigned 
> long len);
>
> +static inline int vm_insert_mixed(struct vm_area_struct *vma,
> +   unsigned long addr, pfn_t pfn)
> +{
> +   return vm_insert_mixed_mkwrite(vma, addr, pfn, false);
> +}

...in other words instead of making the distinction of
vm_insert_mixed_mkwrite() and vm_insert_mixed() with extra flag
argument just move the distinction into mm/memory.c directly.

So, the prototype remains the same as vm_insert_mixed()

int vm_insert_mixed_mkwrite(struct vm_area_struct *vma, unsigned long
addr, pfn_t pfn);

...and only static insert_pfn(...) needs to change.

Re: [PATCH 1/3] mm: add vm_insert_mixed_mkwrite()

[Dan Williams]

On Wed, Jun 7, 2017 at 1:48 PM, Ross Zwisler
 wrote:
> To be able to use the common 4k zero page in DAX we need to have our PTE
> fault path look more like our PMD fault path where a PTE entry can be
> marked as dirty and writeable as it is first inserted, rather than waiting
> for a follow-up dax_pfn_mkwrite() => finish_mkwrite_fault() call.
>
> Right now we can rely on having a dax_pfn_mkwrite() call because we can
> distinguish between these two cases in do_wp_page():
>
> case 1: 4k zero page => writable DAX storage
> case 2: read-only DAX storage => writeable DAX storage
>
> This distinction is made by via vm_normal_page().  vm_normal_page() returns
> false for the common 4k zero page, though, just as it does for DAX ptes.
> Instead of special casing the DAX + 4k zero page case, we will simplify our
> DAX PTE page fault sequence so that it matches our DAX PMD sequence, and
> get rid of dax_pfn_mkwrite() completely.
>
> This means that insert_pfn() needs to follow the lead of insert_pfn_pmd()
> and allow us to pass in a 'mkwrite' flag.  If 'mkwrite' is set insert_pfn()
> will do the work that was previously done by wp_page_reuse() as part of the
> dax_pfn_mkwrite() call path.
>
> Signed-off-by: Ross Zwisler 
> ---
>  include/linux/mm.h |  9 +++--
>  mm/memory.c| 21 ++---
>  2 files changed, 21 insertions(+), 9 deletions(-)
>
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index b892e95..11e323a 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -2294,10 +2294,15 @@ int vm_insert_pfn(struct vm_area_struct *vma, 
> unsigned long addr,
> unsigned long pfn);
>  int vm_insert_pfn_prot(struct vm_area_struct *vma, unsigned long addr,
> unsigned long pfn, pgprot_t pgprot);
> -int vm_insert_mixed(struct vm_area_struct *vma, unsigned long addr,
> -   pfn_t pfn);
> +int vm_insert_mixed_mkwrite(struct vm_area_struct *vma, unsigned long addr,
> +   pfn_t pfn, bool mkwrite);

Are there any other planned public users of vm_insert_mixed_mkwrite()
that would pass false? I think not.

>  int vm_iomap_memory(struct vm_area_struct *vma, phys_addr_t start, unsigned 
> long len);
>
> +static inline int vm_insert_mixed(struct vm_area_struct *vma,
> +   unsigned long addr, pfn_t pfn)
> +{
> +   return vm_insert_mixed_mkwrite(vma, addr, pfn, false);
> +}

...in other words instead of making the distinction of
vm_insert_mixed_mkwrite() and vm_insert_mixed() with extra flag
argument just move the distinction into mm/memory.c directly.

So, the prototype remains the same as vm_insert_mixed()

int vm_insert_mixed_mkwrite(struct vm_area_struct *vma, unsigned long
addr, pfn_t pfn);

...and only static insert_pfn(...) needs to change.

Re: [PATCH v6 10/34] x86, x86/mm, x86/xen, olpc: Use __va() against just the physical address in cr3

[Tom Lendacky]

On 6/9/2017 1:46 PM, Andy Lutomirski wrote:

On Thu, Jun 8, 2017 at 3:38 PM, Tom Lendacky  wrote:

On 6/8/2017 1:05 AM, Andy Lutomirski wrote:


On Wed, Jun 7, 2017 at 12:14 PM, Tom Lendacky 
wrote:


The cr3 register entry can contain the SME encryption bit that indicates
the PGD is encrypted.  The encryption bit should not be used when
creating
a virtual address for the PGD table.

Create a new function, read_cr3_pa(), that will extract the physical
address from the cr3 register. This function is then used where a virtual
address of the PGD needs to be created/used from the cr3 register.



This is going to conflict with:


https://git.kernel.org/pub/scm/linux/kernel/git/luto/linux.git/commit/?h=x86/pcid=555c81e5d01a62b629ec426a2f50d27e2127c1df

We're both encountering the fact that CR3 munges the page table PA
with some other stuff, and some readers want to see the actual CR3
value and other readers just want the PA.  The thing I prefer about my
patch is that I get rid of read_cr3() entirely, forcing the patch to
update every single reader, making review and conflict resolution much
safer.

I'd be willing to send a patch tomorrow that just does the split into
__read_cr3() and read_cr3_pa() (I like your name better) and then we
can both base on top of it.  Would that make sense?



That makes sense to me.


Draft patch:

https://git.kernel.org/pub/scm/linux/kernel/git/luto/linux.git/commit/?h=x86/read_cr3=9adebbc1071f066421a27b4f6e040190f1049624


Looks good to me. I'll look at how to best mask off the encryption bit
in CR3_ADDR_MASK for SME support.  I should be able to just do an
__sme_clr() against it.







Also:


+static inline unsigned long read_cr3_pa(void)
+{
+   return (read_cr3() & PHYSICAL_PAGE_MASK);
+}



Is there any guarantee that the magic encryption bit is masked out in
PHYSICAL_PAGE_MASK?  The docs make it sound like it could be any bit.
(But if it's one of the low 12 bits, that would be quite confusing.)



Right now it's bit 47 and we're steering away from any of the currently
reserved bits so we should be safe.


Should the SME init code check that it's a usable bit (i.e. outside
our physical address mask and not one of the bottom twelve bits)?  If
some future CPU daftly picks, say, bit 12, we'll regret it if we
enable SME.


I think I can safely say that it will never be any of the lower 12 bits,
but let me talk to some of the hardware folks and see about the other
end of the range.

Thanks,
Tom



--Andy

Re: [PATCH v6 10/34] x86, x86/mm, x86/xen, olpc: Use __va() against just the physical address in cr3

[Tom Lendacky]

On 6/9/2017 1:46 PM, Andy Lutomirski wrote:

On Thu, Jun 8, 2017 at 3:38 PM, Tom Lendacky  wrote:

On 6/8/2017 1:05 AM, Andy Lutomirski wrote:


On Wed, Jun 7, 2017 at 12:14 PM, Tom Lendacky 
wrote:


The cr3 register entry can contain the SME encryption bit that indicates
the PGD is encrypted.  The encryption bit should not be used when
creating
a virtual address for the PGD table.

Create a new function, read_cr3_pa(), that will extract the physical
address from the cr3 register. This function is then used where a virtual
address of the PGD needs to be created/used from the cr3 register.



This is going to conflict with:


https://git.kernel.org/pub/scm/linux/kernel/git/luto/linux.git/commit/?h=x86/pcid=555c81e5d01a62b629ec426a2f50d27e2127c1df

We're both encountering the fact that CR3 munges the page table PA
with some other stuff, and some readers want to see the actual CR3
value and other readers just want the PA.  The thing I prefer about my
patch is that I get rid of read_cr3() entirely, forcing the patch to
update every single reader, making review and conflict resolution much
safer.

I'd be willing to send a patch tomorrow that just does the split into
__read_cr3() and read_cr3_pa() (I like your name better) and then we
can both base on top of it.  Would that make sense?



That makes sense to me.


Draft patch:

https://git.kernel.org/pub/scm/linux/kernel/git/luto/linux.git/commit/?h=x86/read_cr3=9adebbc1071f066421a27b4f6e040190f1049624


Looks good to me. I'll look at how to best mask off the encryption bit
in CR3_ADDR_MASK for SME support.  I should be able to just do an
__sme_clr() against it.







Also:


+static inline unsigned long read_cr3_pa(void)
+{
+   return (read_cr3() & PHYSICAL_PAGE_MASK);
+}



Is there any guarantee that the magic encryption bit is masked out in
PHYSICAL_PAGE_MASK?  The docs make it sound like it could be any bit.
(But if it's one of the low 12 bits, that would be quite confusing.)



Right now it's bit 47 and we're steering away from any of the currently
reserved bits so we should be safe.


Should the SME init code check that it's a usable bit (i.e. outside
our physical address mask and not one of the bottom twelve bits)?  If
some future CPU daftly picks, say, bit 12, we'll regret it if we
enable SME.


I think I can safely say that it will never be any of the lower 12 bits,
but let me talk to some of the hardware folks and see about the other
end of the range.

Thanks,
Tom



--Andy

Re: [PATCH v2 0/6] Appended signatures support for IMA appraisal

[Thiago Jung Bauermann]

Michael Ellerman  writes:

> Thiago Jung Bauermann  writes:
>
>> On the OpenPOWER platform, secure boot and trusted boot are being
>> implemented using IMA for taking measurements and verifying signatures.
>
> I still want you to implement arch_kexec_kernel_verify_sig() as well :)

Yes, I will implement it! We are still working on loading the public
keys for kernel signing from the firmware into a kernel keyring, so
there's not much point in implementing arch_kexec_kernel_verify_sig
without having that first.

The same problem also affects IMA: even with these patches, new code
still neededs to be added to make IMA use the platform keys for kernel
signature verification.

-- 
Thiago Jung Bauermann
IBM Linux Technology Center

Re: [PATCH v2 0/6] Appended signatures support for IMA appraisal

[Thiago Jung Bauermann]

Michael Ellerman  writes:

> Thiago Jung Bauermann  writes:
>
>> On the OpenPOWER platform, secure boot and trusted boot are being
>> implemented using IMA for taking measurements and verifying signatures.
>
> I still want you to implement arch_kexec_kernel_verify_sig() as well :)

Yes, I will implement it! We are still working on loading the public
keys for kernel signing from the firmware into a kernel keyring, so
there's not much point in implementing arch_kexec_kernel_verify_sig
without having that first.

The same problem also affects IMA: even with these patches, new code
still neededs to be added to make IMA use the platform keys for kernel
signature verification.

-- 
Thiago Jung Bauermann
IBM Linux Technology Center

[PATCH] hil_kbd: Use more common logging style

[Joe Perches]

Remove use of #define PREFIX and use #define pr_fmt

Miscellanea:

o Convert printk(KERN_ to pr_
o Realign arguments
o Remove duplicate "HIL" prefixes from a few messages

Signed-off-by: Joe Perches 
---
 drivers/input/keyboard/hil_kbd.c | 23 +--
 1 file changed, 9 insertions(+), 14 deletions(-)

diff --git a/drivers/input/keyboard/hil_kbd.c b/drivers/input/keyboard/hil_kbd.c
index 5b152f25a8e1..ee5f005adb60 100644
--- a/drivers/input/keyboard/hil_kbd.c
+++ b/drivers/input/keyboard/hil_kbd.c
@@ -31,6 +31,8 @@
  *
  */
 
+#define pr_fmt(fmt) "HIL: " fmt
+
 #include 
 #include 
 #include 
@@ -40,8 +42,6 @@
 #include 
 #include 
 
-#define PREFIX "HIL: "
-
 MODULE_AUTHOR("Brian S. Julin ");
 MODULE_DESCRIPTION("HIL keyboard/mouse driver");
 MODULE_LICENSE("Dual BSD/GPL");
@@ -130,7 +130,7 @@ static void hil_dev_handle_command_response(struct hil_dev 
*dev)
/* These occur when device isn't present */
if (p != (HIL_ERR_INT | HIL_PKT_CMD)) {
/* Anything else we'd like to know about. */
-   printk(KERN_WARNING PREFIX "Device sent unknown record 
%x\n", p);
+   pr_warn("Device sent unknown record %x\n", p);
}
goto out;
}
@@ -211,8 +211,7 @@ static void hil_dev_handle_ptr_events(struct hil_dev *ptr)
bool absdev, ax16;
 
if ((p & HIL_CMDCT_POL) != idx - 1) {
-   printk(KERN_WARNING PREFIX
-   "Malformed poll packet %x (idx = %i)\n", p, idx);
+   pr_warn("Malformed poll packet %x (idx = %i)\n", p, idx);
return;
}
 
@@ -266,7 +265,7 @@ static void hil_dev_handle_ptr_events(struct hil_dev *ptr)
 
 static void hil_dev_process_err(struct hil_dev *dev)
 {
-   printk(KERN_WARNING PREFIX "errored HIL packet\n");
+   pr_warn("errored HIL packet\n");
dev->idx4 = 0;
complete(>cmd_done); /* just in case somebody is waiting */
 }
@@ -346,7 +345,7 @@ static void hil_dev_keyboard_setup(struct hil_dev *kbd)
input_dev->name = strlen(kbd->rnm) ? kbd->rnm : "HIL keyboard";
input_dev->phys = "hpkbd/input0";
 
-   printk(KERN_INFO PREFIX "HIL keyboard found (did = 0x%02x, lang = 
%s)\n",
+   pr_info("keyboard found (did = 0x%02x, lang = %s)\n",
did, hil_language[did & HIL_IDD_DID_TYPE_KB_LANG_MASK]);
 }
 
@@ -432,11 +431,8 @@ static void hil_dev_pointer_setup(struct hil_dev *ptr)
 
input_dev->name = strlen(ptr->rnm) ? ptr->rnm : "HIL pointer device";
 
-   printk(KERN_INFO PREFIX
-   "HIL pointer device found (did: 0x%02x, axis: %s)\n",
-   did, txt);
-   printk(KERN_INFO PREFIX
-   "HIL pointer has %i buttons and %i sets of %i axes\n",
+   pr_info("pointer device found (did: 0x%02x, axis: %s)\n", did, txt);
+   pr_info("pointer has %i buttons and %i sets of %i axes\n",
ptr->nbtn, naxsets, ptr->naxes);
 }
 
@@ -510,8 +506,7 @@ static int hil_dev_connect(struct serio *serio, struct 
serio_driver *drv)
case HIL_IDD_DID_TYPE_CHAR:
if (HIL_IDD_NUM_BUTTONS(idd) ||
HIL_IDD_NUM_AXES_PER_SET(*idd)) {
-   printk(KERN_INFO PREFIX
-   "combo devices are not supported.\n");
+   pr_info("combo devices are not supported\n");
goto bail1;
}
 
-- 
2.10.0.rc2.1.g053435c

[PATCH] hil_kbd: Use more common logging style

[Joe Perches]

Remove use of #define PREFIX and use #define pr_fmt

Miscellanea:

o Convert printk(KERN_ to pr_
o Realign arguments
o Remove duplicate "HIL" prefixes from a few messages

Signed-off-by: Joe Perches 
---
 drivers/input/keyboard/hil_kbd.c | 23 +--
 1 file changed, 9 insertions(+), 14 deletions(-)

diff --git a/drivers/input/keyboard/hil_kbd.c b/drivers/input/keyboard/hil_kbd.c
index 5b152f25a8e1..ee5f005adb60 100644
--- a/drivers/input/keyboard/hil_kbd.c
+++ b/drivers/input/keyboard/hil_kbd.c
@@ -31,6 +31,8 @@
  *
  */
 
+#define pr_fmt(fmt) "HIL: " fmt
+
 #include 
 #include 
 #include 
@@ -40,8 +42,6 @@
 #include 
 #include 
 
-#define PREFIX "HIL: "
-
 MODULE_AUTHOR("Brian S. Julin ");
 MODULE_DESCRIPTION("HIL keyboard/mouse driver");
 MODULE_LICENSE("Dual BSD/GPL");
@@ -130,7 +130,7 @@ static void hil_dev_handle_command_response(struct hil_dev 
*dev)
/* These occur when device isn't present */
if (p != (HIL_ERR_INT | HIL_PKT_CMD)) {
/* Anything else we'd like to know about. */
-   printk(KERN_WARNING PREFIX "Device sent unknown record 
%x\n", p);
+   pr_warn("Device sent unknown record %x\n", p);
}
goto out;
}
@@ -211,8 +211,7 @@ static void hil_dev_handle_ptr_events(struct hil_dev *ptr)
bool absdev, ax16;
 
if ((p & HIL_CMDCT_POL) != idx - 1) {
-   printk(KERN_WARNING PREFIX
-   "Malformed poll packet %x (idx = %i)\n", p, idx);
+   pr_warn("Malformed poll packet %x (idx = %i)\n", p, idx);
return;
}
 
@@ -266,7 +265,7 @@ static void hil_dev_handle_ptr_events(struct hil_dev *ptr)
 
 static void hil_dev_process_err(struct hil_dev *dev)
 {
-   printk(KERN_WARNING PREFIX "errored HIL packet\n");
+   pr_warn("errored HIL packet\n");
dev->idx4 = 0;
complete(>cmd_done); /* just in case somebody is waiting */
 }
@@ -346,7 +345,7 @@ static void hil_dev_keyboard_setup(struct hil_dev *kbd)
input_dev->name = strlen(kbd->rnm) ? kbd->rnm : "HIL keyboard";
input_dev->phys = "hpkbd/input0";
 
-   printk(KERN_INFO PREFIX "HIL keyboard found (did = 0x%02x, lang = 
%s)\n",
+   pr_info("keyboard found (did = 0x%02x, lang = %s)\n",
did, hil_language[did & HIL_IDD_DID_TYPE_KB_LANG_MASK]);
 }
 
@@ -432,11 +431,8 @@ static void hil_dev_pointer_setup(struct hil_dev *ptr)
 
input_dev->name = strlen(ptr->rnm) ? ptr->rnm : "HIL pointer device";
 
-   printk(KERN_INFO PREFIX
-   "HIL pointer device found (did: 0x%02x, axis: %s)\n",
-   did, txt);
-   printk(KERN_INFO PREFIX
-   "HIL pointer has %i buttons and %i sets of %i axes\n",
+   pr_info("pointer device found (did: 0x%02x, axis: %s)\n", did, txt);
+   pr_info("pointer has %i buttons and %i sets of %i axes\n",
ptr->nbtn, naxsets, ptr->naxes);
 }
 
@@ -510,8 +506,7 @@ static int hil_dev_connect(struct serio *serio, struct 
serio_driver *drv)
case HIL_IDD_DID_TYPE_CHAR:
if (HIL_IDD_NUM_BUTTONS(idd) ||
HIL_IDD_NUM_AXES_PER_SET(*idd)) {
-   printk(KERN_INFO PREFIX
-   "combo devices are not supported.\n");
+   pr_info("combo devices are not supported\n");
goto bail1;
}
 
-- 
2.10.0.rc2.1.g053435c

Re: [PATCH v2] firmware: fix sending -ERESTARTSYS due to signal on fallback

[Luis R. Rodriguez]

On Fri, Jun 09, 2017 at 09:40:47AM +0200, Martin Fuzzey wrote:
> On 09/06/17 03:57, Luis R. Rodriguez wrote:
> > On Thu, Jun 8, 2017 at 6:10 PM, Luis R. Rodriguez  wrote:
> > > > Android didn't send the signal, the kernel did (SIGCHLD).
> > > > 
> > > > Like this:
> > > > 
> > > > 1) Android init (pid=1) fork()s (say pid=42) [this child process is 
> > > > totally
> > > > unrelated to firmware loading]
> > > > 2) Android init (pid=1) does a write() on a (driver custom) sysfs file 
> > > > which
> > > > ends up calling request_firmware() kernel side
> > > > 3) The firmware loading fallback mechanism is used, the request is sent 
> > > > to
> > > > userspace and pid 1 waits in the kernel on wait_*
> > > > 4) before firmware loading completes pid 42 dies (for any reason - in my
> > > > case normal termination)
> > Martin just to be clear, by "normal case termination" do you mean
> > completing successfully ?? Ie the firmware actually did make it onto
> > the device ?
> 
> The firmware did *not* make it onto the device since the request_firmware()
> call returned an error
> (the code that would have transfered it to the device is only executed
> following a successful request_firmware)
> 
> The process that terminates normally is unrelated to firmware loading as I
> said above.
> 
> The only things that matter are:
> - It is a child process of the process that calls request_firmware()
> - It terminates *while* the the wait_ is still in progress
> 
> 
> Here is a way of reproducing the problem using the test_firmware module
> (which I only just saw) on normal linux with no Android or custom driver
> 
> 
> #!/bin/sh
> set -e
> 
> # Make sure the system firmware loader doesn't get in the way
> /etc/init.d/udev stop
> 
> modprobe test_firmware
> 
> DIR=/sys/devices/virtual/misc/test_firmware
> 
> echo 10 >/sys/class/firmware/timeout;
> sleep 2 &
> echo -n "/some/non/existing/file.bin" > "$DIR"/trigger_request;
> 
> 
> 
> If run with the "sleep 2 &" it terminates after 2 seconds
> If the sleep is commented it runs for the expected 10 seconds (the firmware
> loading timeout)
> 
> Since the sleep process is a child of the script process requesting a
> firmware load its death causes a SIGCHLD causing request_firmware() to abort
> prematurely.

Thanks this could mean we also *should* trigger a failure if init is issuing
modprobe on a series of drivers and one completes before another while
request_firmware() is called on init or probe on a subsequent driver. If true
I'm surprised this never was reported back when the fallback mechanism was
popular, I suppose it was not an issue given most firmware *was* present on
/lib/firmware/ and the direct filesystem lookup first step always found the
firmware first, so this would only be an issue for folks relying on the
fallback mechanism exclusively.

Will include a test case based on your above script. Thanks!

  Luis

Re: [PATCH v2] firmware: fix sending -ERESTARTSYS due to signal on fallback

[Luis R. Rodriguez]

On Fri, Jun 09, 2017 at 09:40:47AM +0200, Martin Fuzzey wrote:
> On 09/06/17 03:57, Luis R. Rodriguez wrote:
> > On Thu, Jun 8, 2017 at 6:10 PM, Luis R. Rodriguez  wrote:
> > > > Android didn't send the signal, the kernel did (SIGCHLD).
> > > > 
> > > > Like this:
> > > > 
> > > > 1) Android init (pid=1) fork()s (say pid=42) [this child process is 
> > > > totally
> > > > unrelated to firmware loading]
> > > > 2) Android init (pid=1) does a write() on a (driver custom) sysfs file 
> > > > which
> > > > ends up calling request_firmware() kernel side
> > > > 3) The firmware loading fallback mechanism is used, the request is sent 
> > > > to
> > > > userspace and pid 1 waits in the kernel on wait_*
> > > > 4) before firmware loading completes pid 42 dies (for any reason - in my
> > > > case normal termination)
> > Martin just to be clear, by "normal case termination" do you mean
> > completing successfully ?? Ie the firmware actually did make it onto
> > the device ?
> 
> The firmware did *not* make it onto the device since the request_firmware()
> call returned an error
> (the code that would have transfered it to the device is only executed
> following a successful request_firmware)
> 
> The process that terminates normally is unrelated to firmware loading as I
> said above.
> 
> The only things that matter are:
> - It is a child process of the process that calls request_firmware()
> - It terminates *while* the the wait_ is still in progress
> 
> 
> Here is a way of reproducing the problem using the test_firmware module
> (which I only just saw) on normal linux with no Android or custom driver
> 
> 
> #!/bin/sh
> set -e
> 
> # Make sure the system firmware loader doesn't get in the way
> /etc/init.d/udev stop
> 
> modprobe test_firmware
> 
> DIR=/sys/devices/virtual/misc/test_firmware
> 
> echo 10 >/sys/class/firmware/timeout;
> sleep 2 &
> echo -n "/some/non/existing/file.bin" > "$DIR"/trigger_request;
> 
> 
> 
> If run with the "sleep 2 &" it terminates after 2 seconds
> If the sleep is commented it runs for the expected 10 seconds (the firmware
> loading timeout)
> 
> Since the sleep process is a child of the script process requesting a
> firmware load its death causes a SIGCHLD causing request_firmware() to abort
> prematurely.

Thanks this could mean we also *should* trigger a failure if init is issuing
modprobe on a series of drivers and one completes before another while
request_firmware() is called on init or probe on a subsequent driver. If true
I'm surprised this never was reported back when the fallback mechanism was
popular, I suppose it was not an issue given most firmware *was* present on
/lib/firmware/ and the direct filesystem lookup first step always found the
firmware first, so this would only be an issue for folks relying on the
fallback mechanism exclusively.

Will include a test case based on your above script. Thanks!

  Luis

[PATCH] Staging: vc04_services: bcm2835-audio: bcm2835-ctl.c: Fixed alignment to match open parenthesis.

[srishti sharma]

Fixed alignment so that it matched open parenthesis.

Signed-off-by: srishti sharma 
---
 drivers/staging/vc04_services/bcm2835-audio/bcm2835-ctl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-ctl.c 
b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-ctl.c
index f484bb0..2148ed0 100644
--- a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-ctl.c
+++ b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-ctl.c
@@ -105,7 +105,7 @@ static int snd_bcm2835_ctl_get(struct snd_kcontrol 
*kcontrol,
 }

 static int snd_bcm2835_ctl_put(struct snd_kcontrol *kcontrol,
-   struct snd_ctl_elem_value *ucontrol)
+  struct snd_ctl_elem_value *ucontrol)
 {
struct bcm2835_chip *chip = snd_kcontrol_chip(kcontrol);
int changed = 0;
--
2.7.4

[PATCH] Staging: vc04_services: bcm2835-audio: bcm2835-ctl.c: Fixed alignment to match open parenthesis.

[srishti sharma]

Fixed alignment so that it matched open parenthesis.

Signed-off-by: srishti sharma 
---
 drivers/staging/vc04_services/bcm2835-audio/bcm2835-ctl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-ctl.c 
b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-ctl.c
index f484bb0..2148ed0 100644
--- a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-ctl.c
+++ b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-ctl.c
@@ -105,7 +105,7 @@ static int snd_bcm2835_ctl_get(struct snd_kcontrol 
*kcontrol,
 }

 static int snd_bcm2835_ctl_put(struct snd_kcontrol *kcontrol,
-   struct snd_ctl_elem_value *ucontrol)
+  struct snd_ctl_elem_value *ucontrol)
 {
struct bcm2835_chip *chip = snd_kcontrol_chip(kcontrol);
int changed = 0;
--
2.7.4

Re: [PATCH 1/3] mtd: sharpsl: add sharpslpart MTD partition parser

[Brian Norris]

Hi Boris,

On Fri, Jun 09, 2017 at 09:16:43AM +0200, Boris Brezillon wrote:
> On Thu, 8 Jun 2017 19:32:51 -0700
> Brian Norris  wrote:
> > On Tue, Apr 18, 2017 at 11:35:56AM +0200, Boris Brezillon wrote:
> > > On Tue, 18 Apr 2017 10:58:02 +0200
> > > Andrea Adami  wrote:  
> > > > On Mon, Apr 17, 2017 at 4:44 PM, Boris Brezillon <
> > > > boris.brezil...@free-electrons.com> wrote:  
> > > > > I'll try to document myself on the on-flash format of the FTL and
> > > > > partition table before giving a definitive opinion, but I have the
> > > > > feeling this ancient FTL is not 100% safe, and by accepting an old
> > > > > (maybe unreliable?) FTL we are setting a precedent, and refusing other
> > > > > (broken) proprietary/vendor FTLs will be almost impossible after 
> > > > > that.  
> > 
> > IIUC, drivers/mtd/*ftl*.c shows several FTLs in the kernel... And I'll
> > admit I've basically never reviewed them. I don't think they've really
> > created much precedent either...except that I'm bringing them up right
> > now ;) and possibly that no good alternatives have been developed,
> > except for UBI (and IMO, UBI doesn't necessarily apply in all the same
> > situations that some "boot partitions" might; you have to bootstrap
> > *somewhere*).
> 
> Okay, so you're fine merging FTLs as long as the code is pretty and
> it's already used on real devices, even if the FTL is badly designed?

No, that's not what I'm saying. What I'm saying is that the slippery
slope argument has not held up here, and that in certain cases, 100%
perfection is not necessary. Particularly (and this is what I tried to
understand below), if this is mostly a read-only mapping, then I don't
see a lot of problem. It's just trying to determine a few
essentially-static parameters from the flash.

...

> Does that mean we should support all vendor FTLs just
> because they are shipped on devices? I'm not against FTLs in general,
> but shouldn't we decide to merge them only after reviewing their design
> and making sure they can be safely used?

I more or less agree. The one part I didn't quite get on board with is
why we have to review the entire FTL to accept a (very limited in scope)
parser for a few relatively static pieces of info.

> > 
> > > > Read only is safe.  
> > > 
> > > This is a lie.  
> > 
> > I don't see where you've disproved the claim of safety. The following
> > all seems to be a non sequitur.
> 
> Just because it's only read-only from the kernel point of view, not in
> general.

OK, I get where you're coming from.

> > But my understanding (about the "safety" question) is that this whole
> > FTL construct is:
> > (a) required by the bootloader and
> > (b) not touched outside the bootloader, except (mostly, barring the
> > advanced tooling that people use infrequently, for installation?) read
> > only in a parser like this proposed one
> 
> Andrea, maybe I'm wrong, but I had the impression you had tools to
> update partitions embedded in the FTL (I'm not talking about partitions
> defined in the partition table, but the FTL embeds several sections,
> including one which is storing a kernel image).

I would also appreciate Andrea's analysis on the volatility here. What
type of data gets written, and by whom? When does the FTL mapping ever
get modified?

I did go read through the FTL R/W code from the github link briefly, and
I think at least this one useful property is true:

Eraseblocks that aren't modified cannot get screwed up by R/W behavior
to other blocks, unless a new block manages to duplicate the logical
block number of the one we care about (this shouldn't happen). So
essentially, this partition parsing is fine with all the other blocks
being garbage.

For *writing*, it looks like it's a pretty stupid sequence: copy old
block to memory; make modifications; find unmapped block; erase block;
write new data; erase old block. When rebuilding the mapping from
scratch, it discards duplicate logical blocks, and accepts only the
first one it finds. There doesn't seem to be much provision for
interruption in between blocks of a higher-level operation. So
presumably, interrupting rewriting the kernel region, for example, is
not power-cut safe.

> > > AFAIU, you have all the necessary tools to update the
> > > partition table from user-space, so even if you only have read-only
> > > support in the kernel, one can corrupt it from userspace, and the
> > > kernel may not be able to recover from this corruption.  
> > 
> > How is this any different from, e.g., GPT on block devices? Just because
> > user space can clobber the partition table doesn't mean we don't allow
> > GPT.
> 
> If you can update other partitions embedded in the FTL (like the kernel
> partition) independently, and those updates can corrupt your partition
> table, then it's a bit different from the example you're giving, because
> the user did not asked for a partition table update and may end up with
> a device 

Re: [PATCH 1/3] mtd: sharpsl: add sharpslpart MTD partition parser

[Brian Norris]

Hi Boris,

On Fri, Jun 09, 2017 at 09:16:43AM +0200, Boris Brezillon wrote:
> On Thu, 8 Jun 2017 19:32:51 -0700
> Brian Norris  wrote:
> > On Tue, Apr 18, 2017 at 11:35:56AM +0200, Boris Brezillon wrote:
> > > On Tue, 18 Apr 2017 10:58:02 +0200
> > > Andrea Adami  wrote:  
> > > > On Mon, Apr 17, 2017 at 4:44 PM, Boris Brezillon <
> > > > boris.brezil...@free-electrons.com> wrote:  
> > > > > I'll try to document myself on the on-flash format of the FTL and
> > > > > partition table before giving a definitive opinion, but I have the
> > > > > feeling this ancient FTL is not 100% safe, and by accepting an old
> > > > > (maybe unreliable?) FTL we are setting a precedent, and refusing other
> > > > > (broken) proprietary/vendor FTLs will be almost impossible after 
> > > > > that.  
> > 
> > IIUC, drivers/mtd/*ftl*.c shows several FTLs in the kernel... And I'll
> > admit I've basically never reviewed them. I don't think they've really
> > created much precedent either...except that I'm bringing them up right
> > now ;) and possibly that no good alternatives have been developed,
> > except for UBI (and IMO, UBI doesn't necessarily apply in all the same
> > situations that some "boot partitions" might; you have to bootstrap
> > *somewhere*).
> 
> Okay, so you're fine merging FTLs as long as the code is pretty and
> it's already used on real devices, even if the FTL is badly designed?

No, that's not what I'm saying. What I'm saying is that the slippery
slope argument has not held up here, and that in certain cases, 100%
perfection is not necessary. Particularly (and this is what I tried to
understand below), if this is mostly a read-only mapping, then I don't
see a lot of problem. It's just trying to determine a few
essentially-static parameters from the flash.

...

> Does that mean we should support all vendor FTLs just
> because they are shipped on devices? I'm not against FTLs in general,
> but shouldn't we decide to merge them only after reviewing their design
> and making sure they can be safely used?

I more or less agree. The one part I didn't quite get on board with is
why we have to review the entire FTL to accept a (very limited in scope)
parser for a few relatively static pieces of info.

> > 
> > > > Read only is safe.  
> > > 
> > > This is a lie.  
> > 
> > I don't see where you've disproved the claim of safety. The following
> > all seems to be a non sequitur.
> 
> Just because it's only read-only from the kernel point of view, not in
> general.

OK, I get where you're coming from.

> > But my understanding (about the "safety" question) is that this whole
> > FTL construct is:
> > (a) required by the bootloader and
> > (b) not touched outside the bootloader, except (mostly, barring the
> > advanced tooling that people use infrequently, for installation?) read
> > only in a parser like this proposed one
> 
> Andrea, maybe I'm wrong, but I had the impression you had tools to
> update partitions embedded in the FTL (I'm not talking about partitions
> defined in the partition table, but the FTL embeds several sections,
> including one which is storing a kernel image).

I would also appreciate Andrea's analysis on the volatility here. What
type of data gets written, and by whom? When does the FTL mapping ever
get modified?

I did go read through the FTL R/W code from the github link briefly, and
I think at least this one useful property is true:

Eraseblocks that aren't modified cannot get screwed up by R/W behavior
to other blocks, unless a new block manages to duplicate the logical
block number of the one we care about (this shouldn't happen). So
essentially, this partition parsing is fine with all the other blocks
being garbage.

For *writing*, it looks like it's a pretty stupid sequence: copy old
block to memory; make modifications; find unmapped block; erase block;
write new data; erase old block. When rebuilding the mapping from
scratch, it discards duplicate logical blocks, and accepts only the
first one it finds. There doesn't seem to be much provision for
interruption in between blocks of a higher-level operation. So
presumably, interrupting rewriting the kernel region, for example, is
not power-cut safe.

> > > AFAIU, you have all the necessary tools to update the
> > > partition table from user-space, so even if you only have read-only
> > > support in the kernel, one can corrupt it from userspace, and the
> > > kernel may not be able to recover from this corruption.  
> > 
> > How is this any different from, e.g., GPT on block devices? Just because
> > user space can clobber the partition table doesn't mean we don't allow
> > GPT.
> 
> If you can update other partitions embedded in the FTL (like the kernel
> partition) independently, and those updates can corrupt your partition
> table, then it's a bit different from the example you're giving, because
> the user did not asked for a partition table update and may end up with
> a device that do not boot.

Understood. IIUC, the partition 

Re: [PATCH v2 1/2] dt-bindings: Add bindings for nvidia,tegra186-ccplex-cluster

[Rob Herring]

On Thu, Jun 8, 2017 at 2:32 AM, Mikko Perttunen  wrote:
> On 08.06.2017 01:11, Rob Herring wrote:
>>
>> On Thu, Jun 01, 2017 at 11:04:04AM +0300, Mikko Perttunen wrote:
>>>
>>> The Tegra186 CCPLEX_CLUSTER area contains memory-mapped
>>> registers that initiate CPU frequency/voltage transitions.
>>
>>
>> What the block is should also go in the binding doc. With that,
>
>
> I don't know how to explain it in more detail; this thing is literally just
> a few magic registers that route into some CPU control logic to trigger
> frequency/voltage transitions :)

Copy the commit msg text to the binding doc. That's all I'm asking for.

Rob

Re: [PATCH v2 1/2] dt-bindings: Add bindings for nvidia,tegra186-ccplex-cluster

[Rob Herring]

On Thu, Jun 8, 2017 at 2:32 AM, Mikko Perttunen  wrote:
> On 08.06.2017 01:11, Rob Herring wrote:
>>
>> On Thu, Jun 01, 2017 at 11:04:04AM +0300, Mikko Perttunen wrote:
>>>
>>> The Tegra186 CCPLEX_CLUSTER area contains memory-mapped
>>> registers that initiate CPU frequency/voltage transitions.
>>
>>
>> What the block is should also go in the binding doc. With that,
>
>
> I don't know how to explain it in more detail; this thing is literally just
> a few magic registers that route into some CPU control logic to trigger
> frequency/voltage transitions :)

Copy the commit msg text to the binding doc. That's all I'm asking for.

Rob

[PATCH v12 04/14] sun4i-codec: Add Mic1 Boost Volume, Mic2 Boost Volume.

[Danny Milosavljevic]

---
 sound/soc/sunxi/sun4i-codec.c | 33 +
 1 file changed, 33 insertions(+)

diff --git a/sound/soc/sunxi/sun4i-codec.c b/sound/soc/sunxi/sun4i-codec.c
index 42952af..c69b55c 100644
--- a/sound/soc/sunxi/sun4i-codec.c
+++ b/sound/soc/sunxi/sun4i-codec.c
@@ -95,6 +95,8 @@
 #define SUN4I_CODEC_ADC_ACTL_PREG1EN   (29)
 #define SUN4I_CODEC_ADC_ACTL_PREG2EN   (28)
 #define SUN4I_CODEC_ADC_ACTL_VMICEN(27)
+#define SUN4I_CODEC_ADC_ACTL_PREG1 (25)
+#define SUN4I_CODEC_ADC_ACTL_PREG2 (23)
 #define SUN4I_CODEC_ADC_ACTL_VADCG (20)
 #define SUN4I_CODEC_ADC_ACTL_ADCIS (17)
 #define SUN4I_CODEC_ADC_ACTL_PA_EN (4)
@@ -111,6 +113,9 @@
 /* Microphone controls (sun7i only) */
 #define SUN7I_CODEC_AC_MIC_PHONE_CAL   (0x3c)
 
+#define SUN7I_CODEC_AC_MIC_PHONE_CAL_PREG1 (29)
+#define SUN7I_CODEC_AC_MIC_PHONE_CAL_PREG2 (26)
+
 /*
  * sun6i specific registers
  *
@@ -646,6 +651,12 @@ static const struct snd_kcontrol_new sun4i_codec_pa_mute =
 static DECLARE_TLV_DB_SCALE(sun4i_codec_pa_volume_scale, -6300, 100, 1);
 static DECLARE_TLV_DB_SCALE(sun4i_codec_micin_loopback_gain_scale, -450, 150,
0);
+static DECLARE_TLV_DB_RANGE(sun4i_codec_micin_preamp_gain_scale,
+   0, 0, TLV_DB_SCALE_ITEM(0, 0, 0),
+   1, 7, TLV_DB_SCALE_ITEM(3500, 300, 0));
+static DECLARE_TLV_DB_RANGE(sun7i_codec_micin_preamp_gain_scale,
+   0, 0, TLV_DB_SCALE_ITEM(0, 0, 0),
+   1, 7, TLV_DB_SCALE_ITEM(2400, 300, 0));
 
 static const struct snd_kcontrol_new sun4i_codec_controls[] = {
SOC_SINGLE_TLV("Power Amplifier Volume", SUN4I_CODEC_DAC_ACTL,
@@ -656,6 +667,24 @@ static const struct snd_kcontrol_new 
sun4i_codec_controls[] = {
   sun4i_codec_micin_loopback_gain_scale),
 };
 
+static const struct snd_kcontrol_new sun4i_codec_extra_controls[] = {
+   SOC_SINGLE_TLV("Mic1 Boost Volume", SUN4I_CODEC_ADC_ACTL,
+  SUN4I_CODEC_ADC_ACTL_PREG1, 3, 0,
+  sun4i_codec_micin_preamp_gain_scale),
+   SOC_SINGLE_TLV("Mic2 Boost Volume", SUN4I_CODEC_ADC_ACTL,
+  SUN4I_CODEC_ADC_ACTL_PREG2, 3, 0,
+  sun4i_codec_micin_preamp_gain_scale),
+};
+
+static const struct snd_kcontrol_new sun7i_codec_extra_controls[] = {
+   SOC_SINGLE_TLV("Mic1 Boost Volume", SUN7I_CODEC_AC_MIC_PHONE_CAL,
+  SUN7I_CODEC_AC_MIC_PHONE_CAL_PREG1, 7, 0,
+  sun7i_codec_micin_preamp_gain_scale),
+   SOC_SINGLE_TLV("Mic2 Boost Volume", SUN7I_CODEC_AC_MIC_PHONE_CAL,
+  SUN7I_CODEC_AC_MIC_PHONE_CAL_PREG2, 7, 0,
+  sun7i_codec_micin_preamp_gain_scale),
+};
+
 static const struct snd_kcontrol_new sun4i_codec_left_mixer_controls[] = {
SOC_DAPM_SINGLE("Left DAC Playback Switch", SUN4I_CODEC_DAC_ACTL,
SUN4I_CODEC_DAC_ACTL_LDACLMIXS, 1, 0),
@@ -1464,6 +1493,8 @@ static const struct sun4i_codec_quirks sun4i_codec_quirks 
= {
.reg_adc_fifoc  = REG_FIELD(SUN4I_CODEC_ADC_FIFOC, 0, 31),
.reg_dac_txdata = SUN4I_CODEC_DAC_TXDATA,
.reg_adc_rxdata = SUN4I_CODEC_ADC_RXDATA,
+   .controls = sun4i_codec_extra_controls,
+   .num_controls = ARRAY_SIZE(sun4i_codec_extra_controls),
 };
 
 static const struct sun4i_codec_quirks sun6i_a31_codec_quirks = {
@@ -1483,6 +1514,8 @@ static const struct sun4i_codec_quirks sun7i_codec_quirks 
= {
.reg_adc_fifoc  = REG_FIELD(SUN4I_CODEC_ADC_FIFOC, 0, 31),
.reg_dac_txdata = SUN4I_CODEC_DAC_TXDATA,
.reg_adc_rxdata = SUN4I_CODEC_ADC_RXDATA,
+   .controls = sun7i_codec_extra_controls,
+   .num_controls = ARRAY_SIZE(sun7i_codec_extra_controls),
 };
 
 static const struct sun4i_codec_quirks sun8i_a23_codec_quirks = {
-- 
2.1.4

[PATCH v12 05/14] sun4i-codec: Merge sun4i_codec_left_mixer_controls and sun4i_codec_right_mixer_controls into sun4i_codec_mixer_controls.

[Danny Milosavljevic]

Since it's now possible to have a DAPM mixer control with multiple channels,
use it to cut down the total number of controls.
---
 sound/soc/sunxi/sun4i-codec.c | 27 ---
 1 file changed, 12 insertions(+), 15 deletions(-)

diff --git a/sound/soc/sunxi/sun4i-codec.c b/sound/soc/sunxi/sun4i-codec.c
index c69b55c..3718137 100644
--- a/sound/soc/sunxi/sun4i-codec.c
+++ b/sound/soc/sunxi/sun4i-codec.c
@@ -685,15 +685,12 @@ static const struct snd_kcontrol_new 
sun7i_codec_extra_controls[] = {
   sun7i_codec_micin_preamp_gain_scale),
 };
 
-static const struct snd_kcontrol_new sun4i_codec_left_mixer_controls[] = {
-   SOC_DAPM_SINGLE("Left DAC Playback Switch", SUN4I_CODEC_DAC_ACTL,
-   SUN4I_CODEC_DAC_ACTL_LDACLMIXS, 1, 0),
-};
-
-static const struct snd_kcontrol_new sun4i_codec_right_mixer_controls[] = {
-   SOC_DAPM_SINGLE("Right DAC Playback Switch", SUN4I_CODEC_DAC_ACTL,
+static const struct snd_kcontrol_new sun4i_codec_mixer_controls[] = {
+   SOC_DAPM_DOUBLE("DAC Playback Switch", SUN4I_CODEC_DAC_ACTL,
+   SUN4I_CODEC_DAC_ACTL_LDACLMIXS,
SUN4I_CODEC_DAC_ACTL_RDACRMIXS, 1, 0),
-   SOC_DAPM_SINGLE("Left DAC Playback Switch", SUN4I_CODEC_DAC_ACTL,
+   SOC_DAPM_SINGLE("Right Mixer Left DAC Playback Switch",
+   SUN4I_CODEC_DAC_ACTL,
SUN4I_CODEC_DAC_ACTL_LDACRMIXS, 1, 0),
 };
 
@@ -729,11 +726,11 @@ static const struct snd_soc_dapm_widget 
sun4i_codec_codec_dapm_widgets[] = {
 
/* Mixers */
SND_SOC_DAPM_MIXER("Left Mixer", SND_SOC_NOPM, 0, 0,
-  sun4i_codec_left_mixer_controls,
-  ARRAY_SIZE(sun4i_codec_left_mixer_controls)),
+  sun4i_codec_mixer_controls,
+  ARRAY_SIZE(sun4i_codec_mixer_controls)),
SND_SOC_DAPM_MIXER("Right Mixer", SND_SOC_NOPM, 0, 0,
-  sun4i_codec_right_mixer_controls,
-  ARRAY_SIZE(sun4i_codec_right_mixer_controls)),
+  sun4i_codec_mixer_controls,
+  ARRAY_SIZE(sun4i_codec_mixer_controls)),
 
/* Global Mixer Enable */
SND_SOC_DAPM_SUPPLY("Mixer Enable", SUN4I_CODEC_DAC_ACTL,
@@ -775,12 +772,12 @@ static const struct snd_soc_dapm_route 
sun4i_codec_codec_dapm_routes[] = {
 
/* Right Mixer Routes */
{ "Right Mixer", NULL, "Mixer Enable" },
-   { "Right Mixer", "Left DAC Playback Switch", "Left DAC" },
-   { "Right Mixer", "Right DAC Playback Switch", "Right DAC" },
+   { "Right Mixer", "DAC Playback Switch", "Left DAC" },
+   { "Right Mixer", "Right Mixer Left DAC Playback Switch", "Left DAC" },
 
/* Left Mixer Routes */
{ "Left Mixer", NULL, "Mixer Enable" },
-   { "Left Mixer", "Left DAC Playback Switch", "Left DAC" },
+   { "Left Mixer", "DAC Playback Switch", "Left DAC" },
 
/* Power Amplifier Routes */
{ "Power Amplifier", "Mixer Playback Switch", "Left Mixer" },
-- 
2.1.4

[PATCH v12 04/14] sun4i-codec: Add Mic1 Boost Volume, Mic2 Boost Volume.

[Danny Milosavljevic]

---
 sound/soc/sunxi/sun4i-codec.c | 33 +
 1 file changed, 33 insertions(+)

diff --git a/sound/soc/sunxi/sun4i-codec.c b/sound/soc/sunxi/sun4i-codec.c
index 42952af..c69b55c 100644
--- a/sound/soc/sunxi/sun4i-codec.c
+++ b/sound/soc/sunxi/sun4i-codec.c
@@ -95,6 +95,8 @@
 #define SUN4I_CODEC_ADC_ACTL_PREG1EN   (29)
 #define SUN4I_CODEC_ADC_ACTL_PREG2EN   (28)
 #define SUN4I_CODEC_ADC_ACTL_VMICEN(27)
+#define SUN4I_CODEC_ADC_ACTL_PREG1 (25)
+#define SUN4I_CODEC_ADC_ACTL_PREG2 (23)
 #define SUN4I_CODEC_ADC_ACTL_VADCG (20)
 #define SUN4I_CODEC_ADC_ACTL_ADCIS (17)
 #define SUN4I_CODEC_ADC_ACTL_PA_EN (4)
@@ -111,6 +113,9 @@
 /* Microphone controls (sun7i only) */
 #define SUN7I_CODEC_AC_MIC_PHONE_CAL   (0x3c)
 
+#define SUN7I_CODEC_AC_MIC_PHONE_CAL_PREG1 (29)
+#define SUN7I_CODEC_AC_MIC_PHONE_CAL_PREG2 (26)
+
 /*
  * sun6i specific registers
  *
@@ -646,6 +651,12 @@ static const struct snd_kcontrol_new sun4i_codec_pa_mute =
 static DECLARE_TLV_DB_SCALE(sun4i_codec_pa_volume_scale, -6300, 100, 1);
 static DECLARE_TLV_DB_SCALE(sun4i_codec_micin_loopback_gain_scale, -450, 150,
0);
+static DECLARE_TLV_DB_RANGE(sun4i_codec_micin_preamp_gain_scale,
+   0, 0, TLV_DB_SCALE_ITEM(0, 0, 0),
+   1, 7, TLV_DB_SCALE_ITEM(3500, 300, 0));
+static DECLARE_TLV_DB_RANGE(sun7i_codec_micin_preamp_gain_scale,
+   0, 0, TLV_DB_SCALE_ITEM(0, 0, 0),
+   1, 7, TLV_DB_SCALE_ITEM(2400, 300, 0));
 
 static const struct snd_kcontrol_new sun4i_codec_controls[] = {
SOC_SINGLE_TLV("Power Amplifier Volume", SUN4I_CODEC_DAC_ACTL,
@@ -656,6 +667,24 @@ static const struct snd_kcontrol_new 
sun4i_codec_controls[] = {
   sun4i_codec_micin_loopback_gain_scale),
 };
 
+static const struct snd_kcontrol_new sun4i_codec_extra_controls[] = {
+   SOC_SINGLE_TLV("Mic1 Boost Volume", SUN4I_CODEC_ADC_ACTL,
+  SUN4I_CODEC_ADC_ACTL_PREG1, 3, 0,
+  sun4i_codec_micin_preamp_gain_scale),
+   SOC_SINGLE_TLV("Mic2 Boost Volume", SUN4I_CODEC_ADC_ACTL,
+  SUN4I_CODEC_ADC_ACTL_PREG2, 3, 0,
+  sun4i_codec_micin_preamp_gain_scale),
+};
+
+static const struct snd_kcontrol_new sun7i_codec_extra_controls[] = {
+   SOC_SINGLE_TLV("Mic1 Boost Volume", SUN7I_CODEC_AC_MIC_PHONE_CAL,
+  SUN7I_CODEC_AC_MIC_PHONE_CAL_PREG1, 7, 0,
+  sun7i_codec_micin_preamp_gain_scale),
+   SOC_SINGLE_TLV("Mic2 Boost Volume", SUN7I_CODEC_AC_MIC_PHONE_CAL,
+  SUN7I_CODEC_AC_MIC_PHONE_CAL_PREG2, 7, 0,
+  sun7i_codec_micin_preamp_gain_scale),
+};
+
 static const struct snd_kcontrol_new sun4i_codec_left_mixer_controls[] = {
SOC_DAPM_SINGLE("Left DAC Playback Switch", SUN4I_CODEC_DAC_ACTL,
SUN4I_CODEC_DAC_ACTL_LDACLMIXS, 1, 0),
@@ -1464,6 +1493,8 @@ static const struct sun4i_codec_quirks sun4i_codec_quirks 
= {
.reg_adc_fifoc  = REG_FIELD(SUN4I_CODEC_ADC_FIFOC, 0, 31),
.reg_dac_txdata = SUN4I_CODEC_DAC_TXDATA,
.reg_adc_rxdata = SUN4I_CODEC_ADC_RXDATA,
+   .controls = sun4i_codec_extra_controls,
+   .num_controls = ARRAY_SIZE(sun4i_codec_extra_controls),
 };
 
 static const struct sun4i_codec_quirks sun6i_a31_codec_quirks = {
@@ -1483,6 +1514,8 @@ static const struct sun4i_codec_quirks sun7i_codec_quirks 
= {
.reg_adc_fifoc  = REG_FIELD(SUN4I_CODEC_ADC_FIFOC, 0, 31),
.reg_dac_txdata = SUN4I_CODEC_DAC_TXDATA,
.reg_adc_rxdata = SUN4I_CODEC_ADC_RXDATA,
+   .controls = sun7i_codec_extra_controls,
+   .num_controls = ARRAY_SIZE(sun7i_codec_extra_controls),
 };
 
 static const struct sun4i_codec_quirks sun8i_a23_codec_quirks = {
-- 
2.1.4

[PATCH v12 05/14] sun4i-codec: Merge sun4i_codec_left_mixer_controls and sun4i_codec_right_mixer_controls into sun4i_codec_mixer_controls.

[Danny Milosavljevic]

Since it's now possible to have a DAPM mixer control with multiple channels,
use it to cut down the total number of controls.
---
 sound/soc/sunxi/sun4i-codec.c | 27 ---
 1 file changed, 12 insertions(+), 15 deletions(-)

diff --git a/sound/soc/sunxi/sun4i-codec.c b/sound/soc/sunxi/sun4i-codec.c
index c69b55c..3718137 100644
--- a/sound/soc/sunxi/sun4i-codec.c
+++ b/sound/soc/sunxi/sun4i-codec.c
@@ -685,15 +685,12 @@ static const struct snd_kcontrol_new 
sun7i_codec_extra_controls[] = {
   sun7i_codec_micin_preamp_gain_scale),
 };
 
-static const struct snd_kcontrol_new sun4i_codec_left_mixer_controls[] = {
-   SOC_DAPM_SINGLE("Left DAC Playback Switch", SUN4I_CODEC_DAC_ACTL,
-   SUN4I_CODEC_DAC_ACTL_LDACLMIXS, 1, 0),
-};
-
-static const struct snd_kcontrol_new sun4i_codec_right_mixer_controls[] = {
-   SOC_DAPM_SINGLE("Right DAC Playback Switch", SUN4I_CODEC_DAC_ACTL,
+static const struct snd_kcontrol_new sun4i_codec_mixer_controls[] = {
+   SOC_DAPM_DOUBLE("DAC Playback Switch", SUN4I_CODEC_DAC_ACTL,
+   SUN4I_CODEC_DAC_ACTL_LDACLMIXS,
SUN4I_CODEC_DAC_ACTL_RDACRMIXS, 1, 0),
-   SOC_DAPM_SINGLE("Left DAC Playback Switch", SUN4I_CODEC_DAC_ACTL,
+   SOC_DAPM_SINGLE("Right Mixer Left DAC Playback Switch",
+   SUN4I_CODEC_DAC_ACTL,
SUN4I_CODEC_DAC_ACTL_LDACRMIXS, 1, 0),
 };
 
@@ -729,11 +726,11 @@ static const struct snd_soc_dapm_widget 
sun4i_codec_codec_dapm_widgets[] = {
 
/* Mixers */
SND_SOC_DAPM_MIXER("Left Mixer", SND_SOC_NOPM, 0, 0,
-  sun4i_codec_left_mixer_controls,
-  ARRAY_SIZE(sun4i_codec_left_mixer_controls)),
+  sun4i_codec_mixer_controls,
+  ARRAY_SIZE(sun4i_codec_mixer_controls)),
SND_SOC_DAPM_MIXER("Right Mixer", SND_SOC_NOPM, 0, 0,
-  sun4i_codec_right_mixer_controls,
-  ARRAY_SIZE(sun4i_codec_right_mixer_controls)),
+  sun4i_codec_mixer_controls,
+  ARRAY_SIZE(sun4i_codec_mixer_controls)),
 
/* Global Mixer Enable */
SND_SOC_DAPM_SUPPLY("Mixer Enable", SUN4I_CODEC_DAC_ACTL,
@@ -775,12 +772,12 @@ static const struct snd_soc_dapm_route 
sun4i_codec_codec_dapm_routes[] = {
 
/* Right Mixer Routes */
{ "Right Mixer", NULL, "Mixer Enable" },
-   { "Right Mixer", "Left DAC Playback Switch", "Left DAC" },
-   { "Right Mixer", "Right DAC Playback Switch", "Right DAC" },
+   { "Right Mixer", "DAC Playback Switch", "Left DAC" },
+   { "Right Mixer", "Right Mixer Left DAC Playback Switch", "Left DAC" },
 
/* Left Mixer Routes */
{ "Left Mixer", NULL, "Mixer Enable" },
-   { "Left Mixer", "Left DAC Playback Switch", "Left DAC" },
+   { "Left Mixer", "DAC Playback Switch", "Left DAC" },
 
/* Power Amplifier Routes */
{ "Power Amplifier", "Mixer Playback Switch", "Left Mixer" },
-- 
2.1.4

[PATCH v12 03/14] sun4i-codec: Add support for extra controls to struct sun4i_codec_quirks and use them.

[Danny Milosavljevic]

Some controls use different registers depending on which Allwinner chip it is.
Provide a means of specifying and adding those controls.
---
 sound/soc/sunxi/sun4i-codec.c | 33 +++--
 1 file changed, 23 insertions(+), 10 deletions(-)

diff --git a/sound/soc/sunxi/sun4i-codec.c b/sound/soc/sunxi/sun4i-codec.c
index 49b9cd1..42952af 100644
--- a/sound/soc/sunxi/sun4i-codec.c
+++ b/sound/soc/sunxi/sun4i-codec.c
@@ -777,7 +777,30 @@ static const struct snd_soc_dapm_route 
sun4i_codec_codec_dapm_routes[] = {
{ "Mic2", NULL, "VMIC" },
 };
 
+struct sun4i_codec_quirks {
+   const struct regmap_config *regmap_config;
+   const struct snd_soc_codec_driver *codec;
+   struct snd_soc_card * (*create_card)(struct device *dev);
+   struct reg_field reg_adc_fifoc; /* used for regmap_field */
+   unsigned int reg_dac_txdata;/* TX FIFO offset for DMA config */
+   unsigned int reg_adc_rxdata;/* RX FIFO offset for DMA config */
+   bool has_reset;
+   const struct snd_kcontrol_new *controls;
+   unsigned int num_controls;
+};
+
+static int sun4i_codec_codec_probe(struct snd_soc_codec *scodec)
+{
+   const struct sun4i_codec_quirks *quirks;
+
+   quirks = of_device_get_match_data(scodec->dev);
+   return snd_soc_add_codec_controls(scodec,
+ quirks->controls,
+ quirks->num_controls);
+}
+
 static struct snd_soc_codec_driver sun4i_codec_codec = {
+   .probe = sun4i_codec_codec_probe,
.component_driver = {
.controls   = sun4i_codec_controls,
.num_controls   = ARRAY_SIZE(sun4i_codec_controls),
@@ -1434,16 +1457,6 @@ static const struct regmap_config 
sun8i_v3s_codec_regmap_config = {
.max_register   = SUN8I_H3_CODEC_ADC_DBG,
 };
 
-struct sun4i_codec_quirks {
-   const struct regmap_config *regmap_config;
-   const struct snd_soc_codec_driver *codec;
-   struct snd_soc_card * (*create_card)(struct device *dev);
-   struct reg_field reg_adc_fifoc; /* used for regmap_field */
-   unsigned int reg_dac_txdata;/* TX FIFO offset for DMA config */
-   unsigned int reg_adc_rxdata;/* RX FIFO offset for DMA config */
-   bool has_reset;
-};
-
 static const struct sun4i_codec_quirks sun4i_codec_quirks = {
.regmap_config  = _codec_regmap_config,
.codec  = _codec_codec,
-- 
2.1.4

[PATCH v12 03/14] sun4i-codec: Add support for extra controls to struct sun4i_codec_quirks and use them.

[Danny Milosavljevic]

Some controls use different registers depending on which Allwinner chip it is.
Provide a means of specifying and adding those controls.
---
 sound/soc/sunxi/sun4i-codec.c | 33 +++--
 1 file changed, 23 insertions(+), 10 deletions(-)

diff --git a/sound/soc/sunxi/sun4i-codec.c b/sound/soc/sunxi/sun4i-codec.c
index 49b9cd1..42952af 100644
--- a/sound/soc/sunxi/sun4i-codec.c
+++ b/sound/soc/sunxi/sun4i-codec.c
@@ -777,7 +777,30 @@ static const struct snd_soc_dapm_route 
sun4i_codec_codec_dapm_routes[] = {
{ "Mic2", NULL, "VMIC" },
 };
 
+struct sun4i_codec_quirks {
+   const struct regmap_config *regmap_config;
+   const struct snd_soc_codec_driver *codec;
+   struct snd_soc_card * (*create_card)(struct device *dev);
+   struct reg_field reg_adc_fifoc; /* used for regmap_field */
+   unsigned int reg_dac_txdata;/* TX FIFO offset for DMA config */
+   unsigned int reg_adc_rxdata;/* RX FIFO offset for DMA config */
+   bool has_reset;
+   const struct snd_kcontrol_new *controls;
+   unsigned int num_controls;
+};
+
+static int sun4i_codec_codec_probe(struct snd_soc_codec *scodec)
+{
+   const struct sun4i_codec_quirks *quirks;
+
+   quirks = of_device_get_match_data(scodec->dev);
+   return snd_soc_add_codec_controls(scodec,
+ quirks->controls,
+ quirks->num_controls);
+}
+
 static struct snd_soc_codec_driver sun4i_codec_codec = {
+   .probe = sun4i_codec_codec_probe,
.component_driver = {
.controls   = sun4i_codec_controls,
.num_controls   = ARRAY_SIZE(sun4i_codec_controls),
@@ -1434,16 +1457,6 @@ static const struct regmap_config 
sun8i_v3s_codec_regmap_config = {
.max_register   = SUN8I_H3_CODEC_ADC_DBG,
 };
 
-struct sun4i_codec_quirks {
-   const struct regmap_config *regmap_config;
-   const struct snd_soc_codec_driver *codec;
-   struct snd_soc_card * (*create_card)(struct device *dev);
-   struct reg_field reg_adc_fifoc; /* used for regmap_field */
-   unsigned int reg_dac_txdata;/* TX FIFO offset for DMA config */
-   unsigned int reg_adc_rxdata;/* RX FIFO offset for DMA config */
-   bool has_reset;
-};
-
 static const struct sun4i_codec_quirks sun4i_codec_quirks = {
.regmap_config  = _codec_regmap_config,
.codec  = _codec_codec,
-- 
2.1.4

[PATCH v12 09/14] sun4i-codec: Add Line Playback Volume.

[Danny Milosavljevic]

---
 sound/soc/sunxi/sun4i-codec.c | 8 
 1 file changed, 8 insertions(+)

diff --git a/sound/soc/sunxi/sun4i-codec.c b/sound/soc/sunxi/sun4i-codec.c
index 234ded2..c47ffd5 100644
--- a/sound/soc/sunxi/sun4i-codec.c
+++ b/sound/soc/sunxi/sun4i-codec.c
@@ -64,8 +64,11 @@
 #define SUN4I_CODEC_DAC_ACTL_DACAENR   (31)
 #define SUN4I_CODEC_DAC_ACTL_DACAENL   (30)
 #define SUN4I_CODEC_DAC_ACTL_MIXEN (29)
+#define SUN4I_CODEC_DAC_ACTL_LNG   (26)
 #define SUN4I_CODEC_DAC_ACTL_FMG   (23)
 #define SUN4I_CODEC_DAC_ACTL_MICG  (20)
+#define SUN4I_CODEC_DAC_ACTL_LFMS  (17)
+#define SUN4I_CODEC_DAC_ACTL_RFMS  (16)
 #define SUN4I_CODEC_DAC_ACTL_LDACLMIXS (15)
 #define SUN4I_CODEC_DAC_ACTL_RDACRMIXS (14)
 #define SUN4I_CODEC_DAC_ACTL_LDACRMIXS (13)
@@ -654,6 +657,8 @@ static const struct snd_kcontrol_new sun4i_codec_pa_mute =
SUN4I_CODEC_DAC_ACTL_PA_MUTE, 1, 0);
 
 static DECLARE_TLV_DB_SCALE(sun4i_codec_pa_volume_scale, -6300, 100, 1);
+static DECLARE_TLV_DB_SCALE(sun4i_codec_linein_loopback_gain_scale, -150, 150,
+   0);
 static DECLARE_TLV_DB_SCALE(sun4i_codec_fmin_loopback_gain_scale, -450, 150,
0);
 static DECLARE_TLV_DB_SCALE(sun4i_codec_micin_loopback_gain_scale, -450, 150,
@@ -669,6 +674,9 @@ static const struct snd_kcontrol_new sun4i_codec_controls[] 
= {
SOC_SINGLE_TLV("Power Amplifier Volume", SUN4I_CODEC_DAC_ACTL,
   SUN4I_CODEC_DAC_ACTL_PA_VOL, 0x3F, 0,
   sun4i_codec_pa_volume_scale),
+   SOC_SINGLE_TLV("Line Playback Volume", SUN4I_CODEC_DAC_ACTL,
+  SUN4I_CODEC_DAC_ACTL_LNG, 1, 0,
+  sun4i_codec_linein_loopback_gain_scale),
SOC_SINGLE_TLV("FM Playback Volume", SUN4I_CODEC_DAC_ACTL,
   SUN4I_CODEC_DAC_ACTL_FMG, 3, 0,
   sun4i_codec_fmin_loopback_gain_scale),
-- 
2.1.4

[PATCH v12 09/14] sun4i-codec: Add Line Playback Volume.

[Danny Milosavljevic]

---
 sound/soc/sunxi/sun4i-codec.c | 8 
 1 file changed, 8 insertions(+)

diff --git a/sound/soc/sunxi/sun4i-codec.c b/sound/soc/sunxi/sun4i-codec.c
index 234ded2..c47ffd5 100644
--- a/sound/soc/sunxi/sun4i-codec.c
+++ b/sound/soc/sunxi/sun4i-codec.c
@@ -64,8 +64,11 @@
 #define SUN4I_CODEC_DAC_ACTL_DACAENR   (31)
 #define SUN4I_CODEC_DAC_ACTL_DACAENL   (30)
 #define SUN4I_CODEC_DAC_ACTL_MIXEN (29)
+#define SUN4I_CODEC_DAC_ACTL_LNG   (26)
 #define SUN4I_CODEC_DAC_ACTL_FMG   (23)
 #define SUN4I_CODEC_DAC_ACTL_MICG  (20)
+#define SUN4I_CODEC_DAC_ACTL_LFMS  (17)
+#define SUN4I_CODEC_DAC_ACTL_RFMS  (16)
 #define SUN4I_CODEC_DAC_ACTL_LDACLMIXS (15)
 #define SUN4I_CODEC_DAC_ACTL_RDACRMIXS (14)
 #define SUN4I_CODEC_DAC_ACTL_LDACRMIXS (13)
@@ -654,6 +657,8 @@ static const struct snd_kcontrol_new sun4i_codec_pa_mute =
SUN4I_CODEC_DAC_ACTL_PA_MUTE, 1, 0);
 
 static DECLARE_TLV_DB_SCALE(sun4i_codec_pa_volume_scale, -6300, 100, 1);
+static DECLARE_TLV_DB_SCALE(sun4i_codec_linein_loopback_gain_scale, -150, 150,
+   0);
 static DECLARE_TLV_DB_SCALE(sun4i_codec_fmin_loopback_gain_scale, -450, 150,
0);
 static DECLARE_TLV_DB_SCALE(sun4i_codec_micin_loopback_gain_scale, -450, 150,
@@ -669,6 +674,9 @@ static const struct snd_kcontrol_new sun4i_codec_controls[] 
= {
SOC_SINGLE_TLV("Power Amplifier Volume", SUN4I_CODEC_DAC_ACTL,
   SUN4I_CODEC_DAC_ACTL_PA_VOL, 0x3F, 0,
   sun4i_codec_pa_volume_scale),
+   SOC_SINGLE_TLV("Line Playback Volume", SUN4I_CODEC_DAC_ACTL,
+  SUN4I_CODEC_DAC_ACTL_LNG, 1, 0,
+  sun4i_codec_linein_loopback_gain_scale),
SOC_SINGLE_TLV("FM Playback Volume", SUN4I_CODEC_DAC_ACTL,
   SUN4I_CODEC_DAC_ACTL_FMG, 3, 0,
   sun4i_codec_fmin_loopback_gain_scale),
-- 
2.1.4