Re: [PATCH 0/1] net: ethtool: add SmartNIC reset support

[Scott Branden]

Hi Andrew,


On 17-10-18 09:28 AM, Andrew Lunn wrote:

On Wed, Oct 18, 2017 at 09:01:35AM -0700, Scott Branden wrote:

Ethtool provides support for resetting other internal portions of the
NIC already.  Seems appropriate to use one of the bits for resetting
the application processor (AP) for SmartNICs.

Hi Scott

Do you also have a management processor on the NIC?

Or is the Application Processor just the Marketing Departments name
for the management processor?

Yes, there is also a management processor.
In our next gen SmartNIC the "Application processor" may actually
have 8 very powerful cores.
But, if they need to be reset, we reset them all.


   Andrew

Re: [PATCH 0/1] net: ethtool: add SmartNIC reset support

[Scott Branden]

Hi Andrew,


On 17-10-18 09:28 AM, Andrew Lunn wrote:

On Wed, Oct 18, 2017 at 09:01:35AM -0700, Scott Branden wrote:

Ethtool provides support for resetting other internal portions of the
NIC already.  Seems appropriate to use one of the bits for resetting
the application processor (AP) for SmartNICs.

Hi Scott

Do you also have a management processor on the NIC?

Or is the Application Processor just the Marketing Departments name
for the management processor?

Yes, there is also a management processor.
In our next gen SmartNIC the "Application processor" may actually
have 8 very powerful cores.
But, if they need to be reset, we reset them all.


   Andrew

Re: [PATCH v12 08/11] arm64/kasan: add and use kasan_map_populate()

[Andrey Ryabinin]

On 10/13/2017 08:32 PM, Pavel Tatashin wrote:
> During early boot, kasan uses vmemmap_populate() to establish its shadow
> memory. But, that interface is intended for struct pages use.
> 
> Because of the current project, vmemmap won't be zeroed during allocation,
> but kasan expects that memory to be zeroed. We are adding a new
> kasan_map_populate() function to resolve this difference.
> 
> Therefore, we must use a new interface to allocate and map kasan shadow
> memory, that also zeroes memory for us.
> 

What's the point of this patch? We have "arm64: kasan: Avoid using 
vmemmap_populate to initialise shadow"
which does the right thing and basically reverts this patch.
This patch as intermediate step looks absolutely useless. We can just avoid 
vemmap_populate() right away.

> Signed-off-by: Pavel Tatashin 
> ---
>  arch/arm64/mm/kasan_init.c | 72 
> ++
>  1 file changed, 66 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/arm64/mm/kasan_init.c b/arch/arm64/mm/kasan_init.c
> index 81f03959a4ab..cb4af2951c90 100644
> --- a/arch/arm64/mm/kasan_init.c
> +++ b/arch/arm64/mm/kasan_init.c
> @@ -28,6 +28,66 @@
>  

Re: [PATCH v12 08/11] arm64/kasan: add and use kasan_map_populate()

[Andrey Ryabinin]

On 10/13/2017 08:32 PM, Pavel Tatashin wrote:
> During early boot, kasan uses vmemmap_populate() to establish its shadow
> memory. But, that interface is intended for struct pages use.
> 
> Because of the current project, vmemmap won't be zeroed during allocation,
> but kasan expects that memory to be zeroed. We are adding a new
> kasan_map_populate() function to resolve this difference.
> 
> Therefore, we must use a new interface to allocate and map kasan shadow
> memory, that also zeroes memory for us.
> 

What's the point of this patch? We have "arm64: kasan: Avoid using 
vmemmap_populate to initialise shadow"
which does the right thing and basically reverts this patch.
This patch as intermediate step looks absolutely useless. We can just avoid 
vemmap_populate() right away.

> Signed-off-by: Pavel Tatashin 
> ---
>  arch/arm64/mm/kasan_init.c | 72 
> ++
>  1 file changed, 66 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/arm64/mm/kasan_init.c b/arch/arm64/mm/kasan_init.c
> index 81f03959a4ab..cb4af2951c90 100644
> --- a/arch/arm64/mm/kasan_init.c
> +++ b/arch/arm64/mm/kasan_init.c
> @@ -28,6 +28,66 @@
>  

Re: libbattery was Re: [RFC PATCH 5/5] power: generic-adc-battery: Add capacity handling

[H. Nikolaus Schaller]

> Am 18.10.2017 um 18:13 schrieb Pavel Machek :
> 
> On Wed 2017-10-18 17:52:22, H. Nikolaus Schaller wrote:
>> 
>>> Am 18.10.2017 um 15:56 schrieb Pavel Machek :
>>> 
>>> On Wed 2017-10-18 06:22:04, Tony Lindgren wrote:
 * H. Nikolaus Schaller  [171018 05:49]:
>> Am 18.10.2017 um 14:28 schrieb Pavel Machek :
>> 
>> So I started something, it is at.
>> 
>> https://github.com/pavelmachek/libbattery
>> 
>> My battery on n900 is currently uncalibrated (and charging), still it
>> gets some kind of estimation:
>> 
>> Battery -1 %
>> Seconds -1
>> State 1
>> Voltage 3.88 V
>> Battery 63 %
>> 
>> Of course, there's a lot more work to be done.
> 
> Nice start but not a solution to our problem.
> 
> Our problem is that people simply expect that for example 
> https://packages.debian.org/wheezy/xfce/xfce4-battery-plugin
> displays the battery percentage.
 
 I think we could make things compatible with various battery apps by
 having libbattery write back the capacity percentage and time remaining
 to the kernel driver via sysfs or a dev entry. Then the kernel interface
 can just display the data to whatever apps.
>>> 
>>> Hmm. This could be as simple as providing symlink from
>>> /sys/class/power/userland-battery to some place writable by
>>> userspace...
>> 
>> Well, please implement arbitrary symlinks in /sysfs...
> 
> Any reason why I'd like to do that?

I don't know if you like it or not.

But it appears to be a strict necessity to make your proposal of
"as simple as providing symlink from /sys/class/power/userland-battery
to some place writable by userspace" work.

So if a different solution to provide /sys/class/power/*-battery
is possible, it should probably be preferred and nobody will ask
you to do something you don't like.

BR,
Nikolaus



signature.asc
Description: Message signed with OpenPGP using GPGMail

Re: libbattery was Re: [RFC PATCH 5/5] power: generic-adc-battery: Add capacity handling

[H. Nikolaus Schaller]

> Am 18.10.2017 um 18:13 schrieb Pavel Machek :
> 
> On Wed 2017-10-18 17:52:22, H. Nikolaus Schaller wrote:
>> 
>>> Am 18.10.2017 um 15:56 schrieb Pavel Machek :
>>> 
>>> On Wed 2017-10-18 06:22:04, Tony Lindgren wrote:
 * H. Nikolaus Schaller  [171018 05:49]:
>> Am 18.10.2017 um 14:28 schrieb Pavel Machek :
>> 
>> So I started something, it is at.
>> 
>> https://github.com/pavelmachek/libbattery
>> 
>> My battery on n900 is currently uncalibrated (and charging), still it
>> gets some kind of estimation:
>> 
>> Battery -1 %
>> Seconds -1
>> State 1
>> Voltage 3.88 V
>> Battery 63 %
>> 
>> Of course, there's a lot more work to be done.
> 
> Nice start but not a solution to our problem.
> 
> Our problem is that people simply expect that for example 
> https://packages.debian.org/wheezy/xfce/xfce4-battery-plugin
> displays the battery percentage.
 
 I think we could make things compatible with various battery apps by
 having libbattery write back the capacity percentage and time remaining
 to the kernel driver via sysfs or a dev entry. Then the kernel interface
 can just display the data to whatever apps.
>>> 
>>> Hmm. This could be as simple as providing symlink from
>>> /sys/class/power/userland-battery to some place writable by
>>> userspace...
>> 
>> Well, please implement arbitrary symlinks in /sysfs...
> 
> Any reason why I'd like to do that?

I don't know if you like it or not.

But it appears to be a strict necessity to make your proposal of
"as simple as providing symlink from /sys/class/power/userland-battery
to some place writable by userspace" work.

So if a different solution to provide /sys/class/power/*-battery
is possible, it should probably be preferred and nobody will ask
you to do something you don't like.

BR,
Nikolaus



signature.asc
Description: Message signed with OpenPGP using GPGMail

[PATCH] SoC: intel: byt: Introduce new custom IN2 map

[Carlo Caione]

From: Carlo Caione 

Introduce a new custom dapm routes map to quirk platforms with the
internal mic connected to IN2P.

Signed-off-by: Carlo Caione 
---
 sound/soc/intel/boards/bytcr_rt5651.c | 16 +++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/sound/soc/intel/boards/bytcr_rt5651.c 
b/sound/soc/intel/boards/bytcr_rt5651.c
index 3076bfc0db5e..1dad5c98c9ef 100644
--- a/sound/soc/intel/boards/bytcr_rt5651.c
+++ b/sound/soc/intel/boards/bytcr_rt5651.c
@@ -37,6 +37,7 @@
 enum {
BYT_RT5651_DMIC_MAP,
BYT_RT5651_IN1_MAP,
+   BYT_RT5651_IN2_MAP,
 };
 
 #define BYT_RT5651_MAP(quirk)  ((quirk) & GENMASK(7, 0))
@@ -58,6 +59,8 @@ static void log_quirks(struct device *dev)
dev_info(dev, "quirk DMIC_MAP enabled");
if (BYT_RT5651_MAP(byt_rt5651_quirk) == BYT_RT5651_IN1_MAP)
dev_info(dev, "quirk IN1_MAP enabled");
+   if (BYT_RT5651_MAP(byt_rt5651_quirk) == BYT_RT5651_IN2_MAP)
+   dev_info(dev, "quirk IN2_MAP enabled");
if (byt_rt5651_quirk & BYT_RT5651_DMIC_EN)
dev_info(dev, "quirk DMIC enabled");
if (byt_rt5651_quirk & BYT_RT5651_MCLK_EN)
@@ -143,7 +146,6 @@ static const struct snd_soc_dapm_route 
byt_rt5651_audio_map[] = {
{"ssp2 Rx", NULL, "AIF1 Capture"},
 
{"Headset Mic", NULL, "micbias1"}, /* lowercase for rt5651 */
-   {"IN2P", NULL, "Headset Mic"},
{"Headphone", NULL, "HPOL"},
{"Headphone", NULL, "HPOR"},
{"Speaker", NULL, "LOUTL"},
@@ -151,15 +153,23 @@ static const struct snd_soc_dapm_route 
byt_rt5651_audio_map[] = {
 };
 
 static const struct snd_soc_dapm_route byt_rt5651_intmic_dmic_map[] = {
+   {"IN2P", NULL, "Headset Mic"},
{"DMIC L1", NULL, "Internal Mic"},
{"DMIC R1", NULL, "Internal Mic"},
 };
 
 static const struct snd_soc_dapm_route byt_rt5651_intmic_in1_map[] = {
{"Internal Mic", NULL, "micbias1"},
+   {"IN2P", NULL, "Headset Mic"},
{"IN1P", NULL, "Internal Mic"},
 };
 
+static const struct snd_soc_dapm_route byt_rt5651_intmic_in2_map[] = {
+   {"Internal Mic", NULL, "micbias1"},
+   {"IN1P", NULL, "Headset Mic"},
+   {"IN2P", NULL, "Internal Mic"},
+};
+
 static const struct snd_kcontrol_new byt_rt5651_controls[] = {
SOC_DAPM_PIN_SWITCH("Headphone"),
SOC_DAPM_PIN_SWITCH("Headset Mic"),
@@ -246,6 +256,10 @@ static int byt_rt5651_init(struct snd_soc_pcm_runtime 
*runtime)
custom_map = byt_rt5651_intmic_in1_map;
num_routes = ARRAY_SIZE(byt_rt5651_intmic_in1_map);
break;
+   case BYT_RT5651_IN2_MAP:
+   custom_map = byt_rt5651_intmic_in2_map;
+   num_routes = ARRAY_SIZE(byt_rt5651_intmic_in2_map);
+   break;
default:
custom_map = byt_rt5651_intmic_dmic_map;
num_routes = ARRAY_SIZE(byt_rt5651_intmic_dmic_map);
-- 
2.14.2

[PATCH] SoC: intel: byt: Introduce new custom IN2 map

[Carlo Caione]

From: Carlo Caione 

Introduce a new custom dapm routes map to quirk platforms with the
internal mic connected to IN2P.

Signed-off-by: Carlo Caione 
---
 sound/soc/intel/boards/bytcr_rt5651.c | 16 +++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/sound/soc/intel/boards/bytcr_rt5651.c 
b/sound/soc/intel/boards/bytcr_rt5651.c
index 3076bfc0db5e..1dad5c98c9ef 100644
--- a/sound/soc/intel/boards/bytcr_rt5651.c
+++ b/sound/soc/intel/boards/bytcr_rt5651.c
@@ -37,6 +37,7 @@
 enum {
BYT_RT5651_DMIC_MAP,
BYT_RT5651_IN1_MAP,
+   BYT_RT5651_IN2_MAP,
 };
 
 #define BYT_RT5651_MAP(quirk)  ((quirk) & GENMASK(7, 0))
@@ -58,6 +59,8 @@ static void log_quirks(struct device *dev)
dev_info(dev, "quirk DMIC_MAP enabled");
if (BYT_RT5651_MAP(byt_rt5651_quirk) == BYT_RT5651_IN1_MAP)
dev_info(dev, "quirk IN1_MAP enabled");
+   if (BYT_RT5651_MAP(byt_rt5651_quirk) == BYT_RT5651_IN2_MAP)
+   dev_info(dev, "quirk IN2_MAP enabled");
if (byt_rt5651_quirk & BYT_RT5651_DMIC_EN)
dev_info(dev, "quirk DMIC enabled");
if (byt_rt5651_quirk & BYT_RT5651_MCLK_EN)
@@ -143,7 +146,6 @@ static const struct snd_soc_dapm_route 
byt_rt5651_audio_map[] = {
{"ssp2 Rx", NULL, "AIF1 Capture"},
 
{"Headset Mic", NULL, "micbias1"}, /* lowercase for rt5651 */
-   {"IN2P", NULL, "Headset Mic"},
{"Headphone", NULL, "HPOL"},
{"Headphone", NULL, "HPOR"},
{"Speaker", NULL, "LOUTL"},
@@ -151,15 +153,23 @@ static const struct snd_soc_dapm_route 
byt_rt5651_audio_map[] = {
 };
 
 static const struct snd_soc_dapm_route byt_rt5651_intmic_dmic_map[] = {
+   {"IN2P", NULL, "Headset Mic"},
{"DMIC L1", NULL, "Internal Mic"},
{"DMIC R1", NULL, "Internal Mic"},
 };
 
 static const struct snd_soc_dapm_route byt_rt5651_intmic_in1_map[] = {
{"Internal Mic", NULL, "micbias1"},
+   {"IN2P", NULL, "Headset Mic"},
{"IN1P", NULL, "Internal Mic"},
 };
 
+static const struct snd_soc_dapm_route byt_rt5651_intmic_in2_map[] = {
+   {"Internal Mic", NULL, "micbias1"},
+   {"IN1P", NULL, "Headset Mic"},
+   {"IN2P", NULL, "Internal Mic"},
+};
+
 static const struct snd_kcontrol_new byt_rt5651_controls[] = {
SOC_DAPM_PIN_SWITCH("Headphone"),
SOC_DAPM_PIN_SWITCH("Headset Mic"),
@@ -246,6 +256,10 @@ static int byt_rt5651_init(struct snd_soc_pcm_runtime 
*runtime)
custom_map = byt_rt5651_intmic_in1_map;
num_routes = ARRAY_SIZE(byt_rt5651_intmic_in1_map);
break;
+   case BYT_RT5651_IN2_MAP:
+   custom_map = byt_rt5651_intmic_in2_map;
+   num_routes = ARRAY_SIZE(byt_rt5651_intmic_in2_map);
+   break;
default:
custom_map = byt_rt5651_intmic_dmic_map;
num_routes = ARRAY_SIZE(byt_rt5651_intmic_dmic_map);
-- 
2.14.2

Re: [PATCH v4 0/2] kbuild: Cache exploratory calls to the compiler

[Masahiro Yamada]

2017-10-17 2:12 GMT+09:00 Douglas Anderson :
> This two-patch series attempts to speed incremental builds of the
> kernel up by a bit.  How much of a speedup you get depends a lot on
> your environment, specifically the speed of your workstation and how
> fast it takes to invoke the compiler.
>
> In the Chrome OS build environment you get a really big win.  For an
> incremental build (via emerge) I measured a speedup from ~1 minute to
> ~35 seconds.  ...but Chrome OS calls the compiler through a number of
> wrapper scripts and also calls the kernel make at least twice for an
> emerge (during compile stage and install stage), so it's a bit of a
> worst case.
>
> Perhaps a more realistic measure of the speedup others might see is
> running "time make help > /dev/null" outside of the Chrome OS build
> environment on my system.  When I do this I see that it took more than
> 1.0 seconds before and less than 0.2 seconds after.  So presumably
> this has the ability to shave ~0.8 seconds off an incremental build
> for most folks out there.  While 0.8 seconds savings isn't huge, it
> does make incremental builds feel a lot snappier.
>
> Ingo Molnar also did some testing of this in his environment and found
> that an incremental build of his subsystem sped up from ~.44 seconds
> before to ~.15 seconds after.  Clean builds also sped up by a marginal
> amount.  :)
>
> Changes in v4:
> - Add a mkdir
> - Point to forward declaration commit by git hash
>
> Changes in v3:
> - Rule to prevent make from trying to generate the cache
> - Rule to clean .cache.mk
> - No more doc changes
> - Moved cache stuff below cc-cross-prefix
> - Removed duplicate documentation of try-run (oops)
> - Add Tested-by for Ingo and Guenter since v2 and v3 are very similar
>
> Changes in v2:
> - Abstract at a different level (like shell-cached) per Masahiro Yamada
> - Include ld-version, which I missed the first time
>
> Douglas Anderson (2):
>   kbuild: Add a cache for generated variables
>   kbuild: Cache a few more calls to the compiler
>
>  Makefile   |  5 +--
>  scripts/Kbuild.include | 90 
> ++
>  2 files changed, 79 insertions(+), 16 deletions(-)
>


Applied to linux-kbuild/kbuild.  Thanks.

-- 
Best Regards
Masahiro Yamada

Re: [PATCH v4 0/2] kbuild: Cache exploratory calls to the compiler

[Masahiro Yamada]

2017-10-17 2:12 GMT+09:00 Douglas Anderson :
> This two-patch series attempts to speed incremental builds of the
> kernel up by a bit.  How much of a speedup you get depends a lot on
> your environment, specifically the speed of your workstation and how
> fast it takes to invoke the compiler.
>
> In the Chrome OS build environment you get a really big win.  For an
> incremental build (via emerge) I measured a speedup from ~1 minute to
> ~35 seconds.  ...but Chrome OS calls the compiler through a number of
> wrapper scripts and also calls the kernel make at least twice for an
> emerge (during compile stage and install stage), so it's a bit of a
> worst case.
>
> Perhaps a more realistic measure of the speedup others might see is
> running "time make help > /dev/null" outside of the Chrome OS build
> environment on my system.  When I do this I see that it took more than
> 1.0 seconds before and less than 0.2 seconds after.  So presumably
> this has the ability to shave ~0.8 seconds off an incremental build
> for most folks out there.  While 0.8 seconds savings isn't huge, it
> does make incremental builds feel a lot snappier.
>
> Ingo Molnar also did some testing of this in his environment and found
> that an incremental build of his subsystem sped up from ~.44 seconds
> before to ~.15 seconds after.  Clean builds also sped up by a marginal
> amount.  :)
>
> Changes in v4:
> - Add a mkdir
> - Point to forward declaration commit by git hash
>
> Changes in v3:
> - Rule to prevent make from trying to generate the cache
> - Rule to clean .cache.mk
> - No more doc changes
> - Moved cache stuff below cc-cross-prefix
> - Removed duplicate documentation of try-run (oops)
> - Add Tested-by for Ingo and Guenter since v2 and v3 are very similar
>
> Changes in v2:
> - Abstract at a different level (like shell-cached) per Masahiro Yamada
> - Include ld-version, which I missed the first time
>
> Douglas Anderson (2):
>   kbuild: Add a cache for generated variables
>   kbuild: Cache a few more calls to the compiler
>
>  Makefile   |  5 +--
>  scripts/Kbuild.include | 90 
> ++
>  2 files changed, 79 insertions(+), 16 deletions(-)
>


Applied to linux-kbuild/kbuild.  Thanks.

-- 
Best Regards
Masahiro Yamada

Re: [PATCH v4 0/2] irqchip: meson: add support for the gpio interrupt controller

[Marc Zyngier]

On Mon, Sep 18 2017 at  3:46:07 pm BST, Jerome Brunet  
wrote:
> This patch series adds support for the GPIO interrupt controller found on
> Amlogic's meson SoC families.
>
> Unlike what the name suggests, this controller is not part of the SoC
> GPIO subsystem. It is a separate controller which can watch almost all
> gpio pads of the SoC and generate and interrupt from it. "Almost" because
> there are always exceptions and some specific gpios (TEST_N) lack this
> capability.
>
> Hardware wise, the controller is a 256 to 8 router with a filtering block
> to select edge or level input and the polarity of the signal. We can't
> setup the filtring to generate a signal on both the high and low polarity
> so, ATM, IRQ_TYPE_EDGE_BOTH is not supported.
>
> The number of interrupt line routed to the controller depends on the SoC,
> and essentially the total number of GPIO available on the different gpio
> controllers of the SoC.
>
> This series has been tested on Amlogic S905-P200 board with the front
> panel power button. Also tested on the Nanopi-k2 with the ethernet PHY
> interrupt pin.
>
> This work is derived from the previous work of Carlo Caione [1].

Queued for 4.15.

M.
-- 
Jazz is not dead, it just smell funny.

Re: [PATCH v4 0/2] irqchip: meson: add support for the gpio interrupt controller

[Marc Zyngier]

On Mon, Sep 18 2017 at  3:46:07 pm BST, Jerome Brunet  
wrote:
> This patch series adds support for the GPIO interrupt controller found on
> Amlogic's meson SoC families.
>
> Unlike what the name suggests, this controller is not part of the SoC
> GPIO subsystem. It is a separate controller which can watch almost all
> gpio pads of the SoC and generate and interrupt from it. "Almost" because
> there are always exceptions and some specific gpios (TEST_N) lack this
> capability.
>
> Hardware wise, the controller is a 256 to 8 router with a filtering block
> to select edge or level input and the polarity of the signal. We can't
> setup the filtring to generate a signal on both the high and low polarity
> so, ATM, IRQ_TYPE_EDGE_BOTH is not supported.
>
> The number of interrupt line routed to the controller depends on the SoC,
> and essentially the total number of GPIO available on the different gpio
> controllers of the SoC.
>
> This series has been tested on Amlogic S905-P200 board with the front
> panel power button. Also tested on the Nanopi-k2 with the ethernet PHY
> interrupt pin.
>
> This work is derived from the previous work of Carlo Caione [1].

Queued for 4.15.

M.
-- 
Jazz is not dead, it just smell funny.

Re: [PATCH 1/9] SOC: brcmstb: add memory API

[Florian Fainelli]

On 10/17/2017 11:46 PM, Christoph Hellwig wrote:
> On Tue, Oct 17, 2017 at 09:12:22AM -0700, Florian Fainelli wrote:
>>> Please move this into the arm arch code.
>>
>> No, this needs to work on both ARM and ARM64, hence the reason why this
>> is in a reasonably architecture neutral place.
> 
> So there is no other shared code between the ARM and ARM64 ports for
> this SOC?

The biuctrl.c file is also shared, and there are pending changes for
v4.15-rc1 that also bring more shared files to this directory.

> 
 +EXPORT_SYMBOL(brcmstb_memory_phys_addr_to_memc);
>>>
 +EXPORT_SYMBOL(brcmstb_memory_memc_size);
>>>
>>> Why is this exported?
>>
>> Because the PCIE RC driver can be built as a module.
> 
> Hmm, supporting PCIE RC as module sounds odd, but it seems like there
> are a few others like that.  At least make it EXPORT_SYMBOL_GPL() then
> to document the internal nature.

Fair enough. Thanks
-- 
-- 
Florian

Re: [PATCH 1/9] SOC: brcmstb: add memory API

[Florian Fainelli]

On 10/17/2017 11:46 PM, Christoph Hellwig wrote:
> On Tue, Oct 17, 2017 at 09:12:22AM -0700, Florian Fainelli wrote:
>>> Please move this into the arm arch code.
>>
>> No, this needs to work on both ARM and ARM64, hence the reason why this
>> is in a reasonably architecture neutral place.
> 
> So there is no other shared code between the ARM and ARM64 ports for
> this SOC?

The biuctrl.c file is also shared, and there are pending changes for
v4.15-rc1 that also bring more shared files to this directory.

> 
 +EXPORT_SYMBOL(brcmstb_memory_phys_addr_to_memc);
>>>
 +EXPORT_SYMBOL(brcmstb_memory_memc_size);
>>>
>>> Why is this exported?
>>
>> Because the PCIE RC driver can be built as a module.
> 
> Hmm, supporting PCIE RC as module sounds odd, but it seems like there
> are a few others like that.  At least make it EXPORT_SYMBOL_GPL() then
> to document the internal nature.

Fair enough. Thanks
-- 
-- 
Florian

Re: [PATCH v3 2/7] ACPI: Enable PPTT support on ARM64

[Lorenzo Pieralisi]

On Thu, Oct 12, 2017 at 02:48:51PM -0500, Jeremy Linton wrote:
> Now that we have a PPTT parser, in preparation for its use
> on arm64, lets build it.
> 
> Signed-off-by: Jeremy Linton 
> ---
>  arch/arm64/Kconfig | 1 +
>  drivers/acpi/Makefile  | 1 +
>  drivers/acpi/arm64/Kconfig | 3 +++
>  3 files changed, 5 insertions(+)
> 
> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> index 0df64a6a56d4..68c9d1289735 100644
> --- a/arch/arm64/Kconfig
> +++ b/arch/arm64/Kconfig
> @@ -7,6 +7,7 @@ config ARM64
>   select ACPI_REDUCED_HARDWARE_ONLY if ACPI
>   select ACPI_MCFG if ACPI
>   select ACPI_SPCR_TABLE if ACPI
> + select ACPI_PPTT if ACPI
>   select ARCH_CLOCKSOURCE_DATA
>   select ARCH_HAS_DEBUG_VIRTUAL
>   select ARCH_HAS_DEVMEM_IS_ALLOWED
> diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
> index 90265ab4437a..c92a0c937551 100644
> --- a/drivers/acpi/Makefile
> +++ b/drivers/acpi/Makefile
> @@ -85,6 +85,7 @@ obj-$(CONFIG_ACPI_BGRT) += bgrt.o
>  obj-$(CONFIG_ACPI_CPPC_LIB)  += cppc_acpi.o
>  obj-$(CONFIG_ACPI_SPCR_TABLE)+= spcr.o
>  obj-$(CONFIG_ACPI_DEBUGGER_USER) += acpi_dbg.o
> +obj-$(CONFIG_ACPI_PPTT)  += pptt.o
>  
>  # processor has its own "processor." module_param namespace
>  processor-y  := processor_driver.o
> diff --git a/drivers/acpi/arm64/Kconfig b/drivers/acpi/arm64/Kconfig
> index 5a6f80fce0d6..74b855a669ea 100644
> --- a/drivers/acpi/arm64/Kconfig
> +++ b/drivers/acpi/arm64/Kconfig
> @@ -7,3 +7,6 @@ config ACPI_IORT
>  
>  config ACPI_GTDT
>   bool
> +
> +config ACPI_PPTT
> + bool
> \ No newline at end of file

I do not understand the logic. Why should we have a Kconfig option
in drivers/acpi/arm64 for code in drivers/acpi ?

AFAIK PPTT is not an ACPI ARM64 specific binding.

Lorenzo

Re: [PATCH v3 2/7] ACPI: Enable PPTT support on ARM64

[Lorenzo Pieralisi]

On Thu, Oct 12, 2017 at 02:48:51PM -0500, Jeremy Linton wrote:
> Now that we have a PPTT parser, in preparation for its use
> on arm64, lets build it.
> 
> Signed-off-by: Jeremy Linton 
> ---
>  arch/arm64/Kconfig | 1 +
>  drivers/acpi/Makefile  | 1 +
>  drivers/acpi/arm64/Kconfig | 3 +++
>  3 files changed, 5 insertions(+)
> 
> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> index 0df64a6a56d4..68c9d1289735 100644
> --- a/arch/arm64/Kconfig
> +++ b/arch/arm64/Kconfig
> @@ -7,6 +7,7 @@ config ARM64
>   select ACPI_REDUCED_HARDWARE_ONLY if ACPI
>   select ACPI_MCFG if ACPI
>   select ACPI_SPCR_TABLE if ACPI
> + select ACPI_PPTT if ACPI
>   select ARCH_CLOCKSOURCE_DATA
>   select ARCH_HAS_DEBUG_VIRTUAL
>   select ARCH_HAS_DEVMEM_IS_ALLOWED
> diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
> index 90265ab4437a..c92a0c937551 100644
> --- a/drivers/acpi/Makefile
> +++ b/drivers/acpi/Makefile
> @@ -85,6 +85,7 @@ obj-$(CONFIG_ACPI_BGRT) += bgrt.o
>  obj-$(CONFIG_ACPI_CPPC_LIB)  += cppc_acpi.o
>  obj-$(CONFIG_ACPI_SPCR_TABLE)+= spcr.o
>  obj-$(CONFIG_ACPI_DEBUGGER_USER) += acpi_dbg.o
> +obj-$(CONFIG_ACPI_PPTT)  += pptt.o
>  
>  # processor has its own "processor." module_param namespace
>  processor-y  := processor_driver.o
> diff --git a/drivers/acpi/arm64/Kconfig b/drivers/acpi/arm64/Kconfig
> index 5a6f80fce0d6..74b855a669ea 100644
> --- a/drivers/acpi/arm64/Kconfig
> +++ b/drivers/acpi/arm64/Kconfig
> @@ -7,3 +7,6 @@ config ACPI_IORT
>  
>  config ACPI_GTDT
>   bool
> +
> +config ACPI_PPTT
> + bool
> \ No newline at end of file

I do not understand the logic. Why should we have a Kconfig option
in drivers/acpi/arm64 for code in drivers/acpi ?

AFAIK PPTT is not an ACPI ARM64 specific binding.

Lorenzo

Re: [PATCH] ext4: Convert timers to use timer_setup()

[Theodore Ts'o]

On Mon, Oct 16, 2017 at 05:00:19PM -0700, Kees Cook wrote:
> In preparation for unconditionally passing the struct timer_list pointer to
> all timer callbacks, switch to using the new timer_setup() and from_timer()
> to pass the timer pointer explicitly.
> 
> Cc: "Theodore Ts'o" 
> Cc: Andreas Dilger 
> Cc: linux-e...@vger.kernel.org
> Signed-off-by: Kees Cook 
> Reviewed-by: Reviewed-by: Jan Kara 

Applied, thanks.

- Ted

Re: [PATCH] ext4: Convert timers to use timer_setup()

[Theodore Ts'o]

On Mon, Oct 16, 2017 at 05:00:19PM -0700, Kees Cook wrote:
> In preparation for unconditionally passing the struct timer_list pointer to
> all timer callbacks, switch to using the new timer_setup() and from_timer()
> to pass the timer pointer explicitly.
> 
> Cc: "Theodore Ts'o" 
> Cc: Andreas Dilger 
> Cc: linux-e...@vger.kernel.org
> Signed-off-by: Kees Cook 
> Reviewed-by: Reviewed-by: Jan Kara 

Applied, thanks.

- Ted

Re: [RFC PATCH] kbuild: Allow specifying some base host CFLAGS

[Masahiro Yamada]

2017-10-14 3:02 GMT+09:00 Douglas Anderson :
> Right now there is a way to add some CFLAGS that affect target builds,
> but no way to add CFLAGS that affect host builds.  Let's add a way.
> We'll document two environment variables: CFLAGS_HOST and
> CXXFLAGS_HOST.
>
> We'll document that these variables get appended to by the kernel to
> make the final CFLAGS.  That means that, though the environment can
> specify some flags, if there is a conflict the kernel can override and
> win.  This works differently than KCFLAGS which is appended (and thus
> can override) the kernel specified CFLAGS.
>
> Why would I make KCFLAGS and CFLAGS_HOST work differently in this way?
> My argument is that it's about expected usage.  Typically the build
> system invoking the kernel has some idea about some basic CFLAGS that
> it wants to use to build things for the host and things for the
> target.  In general the build system would expect that its flags can
> be overridden if necessary (perhaps we need to turn off a warning when
> compiling a certain file, for instance).  So, all other things being
> equal, the way I'm making CFLAGS_HOST is the way I'd expect things to
> work.
>
> So, if it's expected that the build system can pass in a base set of
> flags, why didn't we make KCFLAGS work that way?  The short answer is:
> when building for the target the kernel is just "special".  The build
> system's "target" CFLAGS are likely intended for userspace programs
> and likely make very little sense to use as a basis.  This was talked
> about in the seminal commit 69ee0b352242 ("kbuild: do not pick up
> CFLAGS from the environment").  Basically: if the build system REALLY
> knows what it's doing then it can pass in flags that the kernel will
> use, but otherwise it should butt out.  Presumably this build system
> that really knows what it's doing knows better than the kernel so
> KCFLAGS comes after the kernel's normal flags.
>
> One last note: I chose to add new variables rather than just having
> the build system try to pass HOSTCFLAGS in somehow (either through the
> environment or the command line) to avoid weird interactions with
> recursive invocations of make.
>
> Signed-off-by: Douglas Anderson 
> ---

I'd like to know for-instance cases where this is useful.







-- 
Best Regards
Masahiro Yamada

Re: [RFC PATCH] kbuild: Allow specifying some base host CFLAGS

[Masahiro Yamada]

2017-10-14 3:02 GMT+09:00 Douglas Anderson :
> Right now there is a way to add some CFLAGS that affect target builds,
> but no way to add CFLAGS that affect host builds.  Let's add a way.
> We'll document two environment variables: CFLAGS_HOST and
> CXXFLAGS_HOST.
>
> We'll document that these variables get appended to by the kernel to
> make the final CFLAGS.  That means that, though the environment can
> specify some flags, if there is a conflict the kernel can override and
> win.  This works differently than KCFLAGS which is appended (and thus
> can override) the kernel specified CFLAGS.
>
> Why would I make KCFLAGS and CFLAGS_HOST work differently in this way?
> My argument is that it's about expected usage.  Typically the build
> system invoking the kernel has some idea about some basic CFLAGS that
> it wants to use to build things for the host and things for the
> target.  In general the build system would expect that its flags can
> be overridden if necessary (perhaps we need to turn off a warning when
> compiling a certain file, for instance).  So, all other things being
> equal, the way I'm making CFLAGS_HOST is the way I'd expect things to
> work.
>
> So, if it's expected that the build system can pass in a base set of
> flags, why didn't we make KCFLAGS work that way?  The short answer is:
> when building for the target the kernel is just "special".  The build
> system's "target" CFLAGS are likely intended for userspace programs
> and likely make very little sense to use as a basis.  This was talked
> about in the seminal commit 69ee0b352242 ("kbuild: do not pick up
> CFLAGS from the environment").  Basically: if the build system REALLY
> knows what it's doing then it can pass in flags that the kernel will
> use, but otherwise it should butt out.  Presumably this build system
> that really knows what it's doing knows better than the kernel so
> KCFLAGS comes after the kernel's normal flags.
>
> One last note: I chose to add new variables rather than just having
> the build system try to pass HOSTCFLAGS in somehow (either through the
> environment or the command line) to avoid weird interactions with
> recursive invocations of make.
>
> Signed-off-by: Douglas Anderson 
> ---

I'd like to know for-instance cases where this is useful.







-- 
Best Regards
Masahiro Yamada

Re: [PATCH v7 02/10] arm: dts: sunxi: Restore EMAC changes

[Andrew Lunn]

On Wed, Oct 18, 2017 at 01:44:50PM +0200, Corentin Labbe wrote:
> The original dwmac-sun8i DT bindings have some issue on how to handle
> integrated PHY and was reverted in last RC of 4.13.
> But now we have a solution so we need to get back that was reverted.
> 
> This patch restore arm DT about dwmac-sun8i
> This reverts commit fe45174b72ae ("arm: dts: sunxi: Revert EMAC changes")
> 
> Signed-off-by: Corentin Labbe 
> ---
>  arch/arm/boot/dts/sun8i-h2-plus-orangepi-zero.dts |  9 
>  arch/arm/boot/dts/sun8i-h3-bananapi-m2-plus.dts   | 19 +
>  arch/arm/boot/dts/sun8i-h3-nanopi-m1-plus.dts | 19 +
>  arch/arm/boot/dts/sun8i-h3-nanopi-neo.dts |  7 ++
>  arch/arm/boot/dts/sun8i-h3-orangepi-2.dts |  8 +++
>  arch/arm/boot/dts/sun8i-h3-orangepi-one.dts   |  8 +++
>  arch/arm/boot/dts/sun8i-h3-orangepi-pc-plus.dts   |  5 +
>  arch/arm/boot/dts/sun8i-h3-orangepi-pc.dts|  8 +++
>  arch/arm/boot/dts/sun8i-h3-orangepi-plus.dts  | 22 +++
>  arch/arm/boot/dts/sun8i-h3-orangepi-plus2e.dts| 16 ++
>  arch/arm/boot/dts/sunxi-h3-h5.dtsi| 26 
> +++
>  11 files changed, 147 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/sun8i-h2-plus-orangepi-zero.dts 
> b/arch/arm/boot/dts/sun8i-h2-plus-orangepi-zero.dts
> index b1502df7b509..6713d0f2b3f4 100644
> --- a/arch/arm/boot/dts/sun8i-h2-plus-orangepi-zero.dts
> +++ b/arch/arm/boot/dts/sun8i-h2-plus-orangepi-zero.dts
> @@ -56,6 +56,8 @@
>  
>   aliases {
>   serial0 = 
> + /* ethernet0 is the H3 emac, defined in sun8i-h3.dtsi */
> + ethernet0 = 
>   ethernet1 = 
>   };
>  
> @@ -102,6 +104,13 @@
>   status = "okay";
>  };
>  
> + {
> + phy-handle = <_mii_phy>;
> + phy-mode = "mii";
> + allwinner,leds-active-low;
> + status = "okay";
> +};
> +
>   {
>   pinctrl-names = "default";
>   pinctrl-0 = <_pins_a>;
> diff --git a/arch/arm/boot/dts/sun8i-h3-bananapi-m2-plus.dts 
> b/arch/arm/boot/dts/sun8i-h3-bananapi-m2-plus.dts
> index e1dba9ffa94b..f2292deaa590 100644
> --- a/arch/arm/boot/dts/sun8i-h3-bananapi-m2-plus.dts
> +++ b/arch/arm/boot/dts/sun8i-h3-bananapi-m2-plus.dts
> @@ -52,6 +52,7 @@
>   compatible = "sinovoip,bpi-m2-plus", "allwinner,sun8i-h3";
>  
>   aliases {
> + ethernet0 = 
>   serial0 = 
>   serial1 = 
>   };
> @@ -111,6 +112,24 @@
>   status = "okay";
>  };
>  
> + {
> + pinctrl-names = "default";
> + pinctrl-0 = <_rgmii_pins>;
> + phy-supply = <_gmac_3v3>;
> + phy-handle = <_rgmii_phy>;
> + phy-mode = "rgmii";
> +
> + allwinner,leds-active-low;
> + status = "okay";
> +};
> +


> +_mdio {
> + ext_rgmii_phy: ethernet-phy@1 {
> + compatible = "ethernet-phy-ieee802.3-c22";
> + reg = <0>;
> + };
> +};
> +

Hi Corentin

I'm wondering about the order of the patches. Does the external_mdio
node actually exist at this point? Or only later when other patches
are applied?

Andrew

Re: [PATCH v7 02/10] arm: dts: sunxi: Restore EMAC changes

[Andrew Lunn]

On Wed, Oct 18, 2017 at 01:44:50PM +0200, Corentin Labbe wrote:
> The original dwmac-sun8i DT bindings have some issue on how to handle
> integrated PHY and was reverted in last RC of 4.13.
> But now we have a solution so we need to get back that was reverted.
> 
> This patch restore arm DT about dwmac-sun8i
> This reverts commit fe45174b72ae ("arm: dts: sunxi: Revert EMAC changes")
> 
> Signed-off-by: Corentin Labbe 
> ---
>  arch/arm/boot/dts/sun8i-h2-plus-orangepi-zero.dts |  9 
>  arch/arm/boot/dts/sun8i-h3-bananapi-m2-plus.dts   | 19 +
>  arch/arm/boot/dts/sun8i-h3-nanopi-m1-plus.dts | 19 +
>  arch/arm/boot/dts/sun8i-h3-nanopi-neo.dts |  7 ++
>  arch/arm/boot/dts/sun8i-h3-orangepi-2.dts |  8 +++
>  arch/arm/boot/dts/sun8i-h3-orangepi-one.dts   |  8 +++
>  arch/arm/boot/dts/sun8i-h3-orangepi-pc-plus.dts   |  5 +
>  arch/arm/boot/dts/sun8i-h3-orangepi-pc.dts|  8 +++
>  arch/arm/boot/dts/sun8i-h3-orangepi-plus.dts  | 22 +++
>  arch/arm/boot/dts/sun8i-h3-orangepi-plus2e.dts| 16 ++
>  arch/arm/boot/dts/sunxi-h3-h5.dtsi| 26 
> +++
>  11 files changed, 147 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/sun8i-h2-plus-orangepi-zero.dts 
> b/arch/arm/boot/dts/sun8i-h2-plus-orangepi-zero.dts
> index b1502df7b509..6713d0f2b3f4 100644
> --- a/arch/arm/boot/dts/sun8i-h2-plus-orangepi-zero.dts
> +++ b/arch/arm/boot/dts/sun8i-h2-plus-orangepi-zero.dts
> @@ -56,6 +56,8 @@
>  
>   aliases {
>   serial0 = 
> + /* ethernet0 is the H3 emac, defined in sun8i-h3.dtsi */
> + ethernet0 = 
>   ethernet1 = 
>   };
>  
> @@ -102,6 +104,13 @@
>   status = "okay";
>  };
>  
> + {
> + phy-handle = <_mii_phy>;
> + phy-mode = "mii";
> + allwinner,leds-active-low;
> + status = "okay";
> +};
> +
>   {
>   pinctrl-names = "default";
>   pinctrl-0 = <_pins_a>;
> diff --git a/arch/arm/boot/dts/sun8i-h3-bananapi-m2-plus.dts 
> b/arch/arm/boot/dts/sun8i-h3-bananapi-m2-plus.dts
> index e1dba9ffa94b..f2292deaa590 100644
> --- a/arch/arm/boot/dts/sun8i-h3-bananapi-m2-plus.dts
> +++ b/arch/arm/boot/dts/sun8i-h3-bananapi-m2-plus.dts
> @@ -52,6 +52,7 @@
>   compatible = "sinovoip,bpi-m2-plus", "allwinner,sun8i-h3";
>  
>   aliases {
> + ethernet0 = 
>   serial0 = 
>   serial1 = 
>   };
> @@ -111,6 +112,24 @@
>   status = "okay";
>  };
>  
> + {
> + pinctrl-names = "default";
> + pinctrl-0 = <_rgmii_pins>;
> + phy-supply = <_gmac_3v3>;
> + phy-handle = <_rgmii_phy>;
> + phy-mode = "rgmii";
> +
> + allwinner,leds-active-low;
> + status = "okay";
> +};
> +


> +_mdio {
> + ext_rgmii_phy: ethernet-phy@1 {
> + compatible = "ethernet-phy-ieee802.3-c22";
> + reg = <0>;
> + };
> +};
> +

Hi Corentin

I'm wondering about the order of the patches. Does the external_mdio
node actually exist at this point? Or only later when other patches
are applied?

Andrew

Re: [PATCH] jbd2: Convert timers to use timer_setup()

[Theodore Ts'o]

On Tue, Oct 10, 2017 at 01:20:50PM +0200, Jan Kara wrote:
> On Wed 04-10-17 17:48:46, Kees Cook wrote:
> > In preparation for unconditionally passing the struct timer_list pointer to
> > all timer callbacks, switch to using the new timer_setup() and from_timer()
> > to pass the timer pointer explicitly.
> > 
> > Cc: "Theodore Ts'o" 
> > Cc: Jan Kara 
> > Cc: linux-e...@vger.kernel.org
> > Cc: Thomas Gleixner 
> > Signed-off-by: Kees Cook 
> 
> The patch looks good to me. You can add:
> 
> Reviewed-by: Jan Kara 

Applied, thanks.

- Ted

Re: [PATCH] jbd2: Convert timers to use timer_setup()

[Theodore Ts'o]

On Tue, Oct 10, 2017 at 01:20:50PM +0200, Jan Kara wrote:
> On Wed 04-10-17 17:48:46, Kees Cook wrote:
> > In preparation for unconditionally passing the struct timer_list pointer to
> > all timer callbacks, switch to using the new timer_setup() and from_timer()
> > to pass the timer pointer explicitly.
> > 
> > Cc: "Theodore Ts'o" 
> > Cc: Jan Kara 
> > Cc: linux-e...@vger.kernel.org
> > Cc: Thomas Gleixner 
> > Signed-off-by: Kees Cook 
> 
> The patch looks good to me. You can add:
> 
> Reviewed-by: Jan Kara 

Applied, thanks.

- Ted

Re: [RFC 00/19] KVM: s390/crypto/vfio: guest dedicated crypto adapters

[Christian Borntraeger]

As a remark. I think it makes more sense to concentrate on the whole concept 
before doing an depth review (of course, if you have the cycles - feel free
to go ahead :-) ).
Let me just summarize the HW idea in a very simplified fashion, so that we can
try to let everybody understand what is going on here.

Implementation-wise (in LPAR and as supported by the HW-virtualization for KVM) 
the 
implementation itself is really really simple. As a satellite block of the
state descriptor (sie control block) there is the crypto control block.
(usually one per guest). This contains 3 256bit bitfields which allow/disallow
1. adapters by number (0-255)
2. usage domains by number (0-255)
3. control domains by number (0-255)

For simplicity lets ignore the control domains, the mechanism is similar to
the usage domain.

The guest will then have access to ALL elements of the cross product.
So adapter 1,2,3 and domain 10,11,12 will result in 9 tuples accessible by 
the guest (1,10) ; (1,11) ; (1,12) ; (2,10) ; (2,11) ; (2,12) ; (3,10) ;
(3,11) ; (3,12)
the admin (or the management instance) must ensure that each tuple
is only in use by 1 guest.
Setting the bits is basically all what is needed (plus some minimal glue
and handling), everything else will be handled by hardware/firmware.

Now if we do not need to care about security, we could just use some kvm
ioctl and be done with it. As we learned for PCI, this is not going to be
secure as qemu is untrusted in svirt/appamor contexts.

So this is basically using vfio and mdev as a means to orchestrate things.
And this is where things get complicated and multiple options are possible.

Christian

Re: [RFC 00/19] KVM: s390/crypto/vfio: guest dedicated crypto adapters

[Christian Borntraeger]

As a remark. I think it makes more sense to concentrate on the whole concept 
before doing an depth review (of course, if you have the cycles - feel free
to go ahead :-) ).
Let me just summarize the HW idea in a very simplified fashion, so that we can
try to let everybody understand what is going on here.

Implementation-wise (in LPAR and as supported by the HW-virtualization for KVM) 
the 
implementation itself is really really simple. As a satellite block of the
state descriptor (sie control block) there is the crypto control block.
(usually one per guest). This contains 3 256bit bitfields which allow/disallow
1. adapters by number (0-255)
2. usage domains by number (0-255)
3. control domains by number (0-255)

For simplicity lets ignore the control domains, the mechanism is similar to
the usage domain.

The guest will then have access to ALL elements of the cross product.
So adapter 1,2,3 and domain 10,11,12 will result in 9 tuples accessible by 
the guest (1,10) ; (1,11) ; (1,12) ; (2,10) ; (2,11) ; (2,12) ; (3,10) ;
(3,11) ; (3,12)
the admin (or the management instance) must ensure that each tuple
is only in use by 1 guest.
Setting the bits is basically all what is needed (plus some minimal glue
and handling), everything else will be handled by hardware/firmware.

Now if we do not need to care about security, we could just use some kvm
ioctl and be done with it. As we learned for PCI, this is not going to be
secure as qemu is untrusted in svirt/appamor contexts.

So this is basically using vfio and mdev as a means to orchestrate things.
And this is where things get complicated and multiple options are possible.

Christian

Re: char/tpm: Improve a size determination in nine functions

[SF Markus Elfring]

> Commit message should just describe in plain text what you are doing

Did other contributors find the wording “Replace …”


> and why.

and “… a bit safer according to the Linux coding style convention.”
sufficient often enough already?

Which description would you find more appropriate for this change pattern?

Regards,
Markus

Re: [PATCH] perf vendor events arm64: Add hip08 implementation defined PMU core events

[John Garry]

On 18/10/2017 11:49, Will Deacon wrote:

On Wed, Oct 18, 2017 at 10:25:39AM +0100, John Garry wrote:

On 17/10/2017 13:59, Will Deacon wrote:

Hi Shaokun,

Thanks for the patch. One comment below.

On Tue, Oct 17, 2017 at 03:01:39PM +0800, Shaokun Zhang wrote:

This is a short list of useful implementation defined PMU events of
hip08, other supported events are not listed in this JSON file.

This patch is dependent on Cavium's patch-v9 (Add support for
ThunderX2 pmu events using json files), Link:
https://www.spinics.net/lists/arm-kernel/msg611895.html

Signed-off-by: Shaokun Zhang 
Cc: Peter Zijlstra 
Cc: Ingo Molnar 
Cc: Arnaldo Carvalho de Melo 
Cc: Alexander Shishkin 
Cc: Will Deacon 
Cc: Ganapatrao Kulkarni 
Cc: John Garry 
---
.../arch/arm64/hisilicon/hip08-imp-def.json| 176 +
tools/perf/pmu-events/arch/arm64/mapfile.csv   |   1 +
2 files changed, 177 insertions(+)
create mode 100644 tools/perf/pmu-events/arch/arm64/hisilicon/hip08-imp-def.json

diff --git a/tools/perf/pmu-events/arch/arm64/hisilicon/hip08-imp-def.json 
b/tools/perf/pmu-events/arch/arm64/hisilicon/hip08-imp-def.json
new file mode 100644
index 000..6bb31da
--- /dev/null
+++ b/tools/perf/pmu-events/arch/arm64/hisilicon/hip08-imp-def.json
@@ -0,0 +1,176 @@
+[
+{
+"PublicDescription": "Attributable Level 1 data cache access, read",
+"EventCode": "0x40",
+"EventName": "L1D_CACHE_RD",
+"BriefDescription": "L1D cache access, read",
+},
+{
+"PublicDescription": "Attributable Level 1 data cache access, write",
+"EventCode": "0x41",
+"EventName": "L1D_CACHE_WR",
+"BriefDescription": "L1D cache access, write",
+},


So these are the same as the events in cavium/thunderx2-imp-def.json and
should be factored out. In fact, ARM recommends event numbers for 0x40-0xBF,
so the best thing would be to have those defined in their own file, then
have a way for the various CPU-specific .json files to pick and chose the
events they need from there.


Right, this seems reasonable. Just need to check on feasible.

In terms of coordinating this work, shall we do it? Will arm64+ThunderX
support be accepted as is?


Yes, that would be my preference if you don't mind.



Cool.

I am looking at this topic now. But I am doubting the folder structure 
again.


Firstly, we still have this comment in the README:
All the topic JSON files for a CPU model/family should be in a separate
sub directory.

Now, when thunderx3 or hip09 comes along, I assume that their jsons will 
similarly go into cavium and hisilcon folders, respectively. So, for 
example, we add thunderx3 json, like this:

arm64/cavium/thunderx3-imp-def.json
[
{
"PublicDescription": "foo",
"EventCode": "0x40",
"EventName": "bar",
"BriefDescription": "sieve",
},
]

#Family-model,Version,Filename,EventType
0x420f5160,v1,cavium,core
0x420f5161,v1,cavium,core

Then we have generated pmu_events.c, like this:
#include "../../pmu-events/pmu-events.h"
struct pmu_event pme_cavium[] = {
{
.name = "bar",
.event = "event=0x40",
.desc = "sieve",
.topic = "thunderx3 imp def",
.long_desc = "foo",
},
{
.name = "l1d_cache_rd",
.event = "event=0x40",
.desc = "L1D cache read",
.topic = "thunderx2 imp def",
.long_desc = "Attributable Level 1 data cache access, read",
},
{
.name = "l1d_cache_wr",
.event = "event=0x41",
.desc = "L1D cache write",
.topic = "thunderx2 imp def",
.long_desc = "Attributable Level 1 data cache access, write ",
},

[ ... ]

{
.name = 0,
.event = 0,
.desc = 0,
},
};
struct pmu_events_map pmu_events_map[] = {
{
.cpuid = "0x420f5160",
.version = "v1",
.type = "core",
.table = pme_cavium
},
{
.cpuid = "0x420f5161",
.version = "v1",
.type = "core",
.table = pme_cavium
},
{
.cpuid = 0,
.version = 0,
.type = 0,
.table = 0,
},
};

It doesn't look right, espcially since we have conflicting definitions 
for event 0x40. We really should have table per cpu.


John




Will

.

Re: char/tpm: Improve a size determination in nine functions

[SF Markus Elfring]

> Commit message should just describe in plain text what you are doing

Did other contributors find the wording “Replace …”


> and why.

and “… a bit safer according to the Linux coding style convention.”
sufficient often enough already?

Which description would you find more appropriate for this change pattern?

Regards,
Markus

Re: [PATCH] perf vendor events arm64: Add hip08 implementation defined PMU core events

[John Garry]

On 18/10/2017 11:49, Will Deacon wrote:

On Wed, Oct 18, 2017 at 10:25:39AM +0100, John Garry wrote:

On 17/10/2017 13:59, Will Deacon wrote:

Hi Shaokun,

Thanks for the patch. One comment below.

On Tue, Oct 17, 2017 at 03:01:39PM +0800, Shaokun Zhang wrote:

This is a short list of useful implementation defined PMU events of
hip08, other supported events are not listed in this JSON file.

This patch is dependent on Cavium's patch-v9 (Add support for
ThunderX2 pmu events using json files), Link:
https://www.spinics.net/lists/arm-kernel/msg611895.html

Signed-off-by: Shaokun Zhang 
Cc: Peter Zijlstra 
Cc: Ingo Molnar 
Cc: Arnaldo Carvalho de Melo 
Cc: Alexander Shishkin 
Cc: Will Deacon 
Cc: Ganapatrao Kulkarni 
Cc: John Garry 
---
.../arch/arm64/hisilicon/hip08-imp-def.json| 176 +
tools/perf/pmu-events/arch/arm64/mapfile.csv   |   1 +
2 files changed, 177 insertions(+)
create mode 100644 tools/perf/pmu-events/arch/arm64/hisilicon/hip08-imp-def.json

diff --git a/tools/perf/pmu-events/arch/arm64/hisilicon/hip08-imp-def.json 
b/tools/perf/pmu-events/arch/arm64/hisilicon/hip08-imp-def.json
new file mode 100644
index 000..6bb31da
--- /dev/null
+++ b/tools/perf/pmu-events/arch/arm64/hisilicon/hip08-imp-def.json
@@ -0,0 +1,176 @@
+[
+{
+"PublicDescription": "Attributable Level 1 data cache access, read",
+"EventCode": "0x40",
+"EventName": "L1D_CACHE_RD",
+"BriefDescription": "L1D cache access, read",
+},
+{
+"PublicDescription": "Attributable Level 1 data cache access, write",
+"EventCode": "0x41",
+"EventName": "L1D_CACHE_WR",
+"BriefDescription": "L1D cache access, write",
+},


So these are the same as the events in cavium/thunderx2-imp-def.json and
should be factored out. In fact, ARM recommends event numbers for 0x40-0xBF,
so the best thing would be to have those defined in their own file, then
have a way for the various CPU-specific .json files to pick and chose the
events they need from there.


Right, this seems reasonable. Just need to check on feasible.

In terms of coordinating this work, shall we do it? Will arm64+ThunderX
support be accepted as is?


Yes, that would be my preference if you don't mind.



Cool.

I am looking at this topic now. But I am doubting the folder structure 
again.


Firstly, we still have this comment in the README:
All the topic JSON files for a CPU model/family should be in a separate
sub directory.

Now, when thunderx3 or hip09 comes along, I assume that their jsons will 
similarly go into cavium and hisilcon folders, respectively. So, for 
example, we add thunderx3 json, like this:

arm64/cavium/thunderx3-imp-def.json
[
{
"PublicDescription": "foo",
"EventCode": "0x40",
"EventName": "bar",
"BriefDescription": "sieve",
},
]

#Family-model,Version,Filename,EventType
0x420f5160,v1,cavium,core
0x420f5161,v1,cavium,core

Then we have generated pmu_events.c, like this:
#include "../../pmu-events/pmu-events.h"
struct pmu_event pme_cavium[] = {
{
.name = "bar",
.event = "event=0x40",
.desc = "sieve",
.topic = "thunderx3 imp def",
.long_desc = "foo",
},
{
.name = "l1d_cache_rd",
.event = "event=0x40",
.desc = "L1D cache read",
.topic = "thunderx2 imp def",
.long_desc = "Attributable Level 1 data cache access, read",
},
{
.name = "l1d_cache_wr",
.event = "event=0x41",
.desc = "L1D cache write",
.topic = "thunderx2 imp def",
.long_desc = "Attributable Level 1 data cache access, write ",
},

[ ... ]

{
.name = 0,
.event = 0,
.desc = 0,
},
};
struct pmu_events_map pmu_events_map[] = {
{
.cpuid = "0x420f5160",
.version = "v1",
.type = "core",
.table = pme_cavium
},
{
.cpuid = "0x420f5161",
.version = "v1",
.type = "core",
.table = pme_cavium
},
{
.cpuid = 0,
.version = 0,
.type = 0,
.table = 0,
},
};

It doesn't look right, espcially since we have conflicting definitions 
for event 0x40. We really should have table per cpu.


John




Will

.

Re: [RFC PATCH v9 for 4.15 01/14] Restartable sequences system call

[Ben Maurer]

> The layout of struct rseq_cs is as follows:

> start_ip
> Instruction pointer address of the first instruction of the
> sequence of consecutive assembly instructions.

> post_commit_ip
> Instruction pointer address after the last  instruction  of
>  the sequence of consecutive assembly instructions.

>  abort_ip
> Instruction  pointer  address  where  to move the execution
>  flow in case of abort of the sequence of consecutive assem‐
>  bly instructions.

Really minor performance performance thought here.

1) In the kernel at context switch time you'd need code like:

if (ip >= start_ip && ip <= post_commit_ip)

This branch would be hard to predict because most instruction pointers would be 
either before or after. If post_commit_ip were relative to start_ip you could 
do this:

if (ip - start_ip <= post_commit_offset)

which is a single branch that would be more predictable.

2) In a shared library a rseq_cs structure would have to be relocated at 
runtime because at compilation time the final address of the library wouldn't 
be known. I'm not sure if this is important enough to address, but it could be 
solved by making the pointers relative to the address of rseq_cs. But this 
would make for an uglier API.

Re: [RFC PATCH v9 for 4.15 01/14] Restartable sequences system call

[Ben Maurer]

> The layout of struct rseq_cs is as follows:

> start_ip
> Instruction pointer address of the first instruction of the
> sequence of consecutive assembly instructions.

> post_commit_ip
> Instruction pointer address after the last  instruction  of
>  the sequence of consecutive assembly instructions.

>  abort_ip
> Instruction  pointer  address  where  to move the execution
>  flow in case of abort of the sequence of consecutive assem‐
>  bly instructions.

Really minor performance performance thought here.

1) In the kernel at context switch time you'd need code like:

if (ip >= start_ip && ip <= post_commit_ip)

This branch would be hard to predict because most instruction pointers would be 
either before or after. If post_commit_ip were relative to start_ip you could 
do this:

if (ip - start_ip <= post_commit_offset)

which is a single branch that would be more predictable.

2) In a shared library a rseq_cs structure would have to be relocated at 
runtime because at compilation time the final address of the library wouldn't 
be known. I'm not sure if this is important enough to address, but it could be 
solved by making the pointers relative to the address of rseq_cs. But this 
would make for an uglier API.

orc unwinder: stack-out-of-bounds accesses

[Dmitry Vyukov]

Hello,

I am seeing lots of KASAN-detected stack-out-of-bounds accesses in the
new ORC unwinder. Examples of reports below. linux-next on
a7dd40274d75326ca868479c62090b1198a357ad.
You can reproduce this by enabling CONFIG_KASAN with gcc7+ (which
supports stack instrumentation).


==
BUG: KASAN: stack-out-of-bounds in __read_once_size
include/linux/compiler.h:276 [inline]
BUG: KASAN: stack-out-of-bounds in deref_stack_reg+0x1b8/0x1d0
arch/x86/kernel/unwind_orc.c:282
Read of size 8 at addr 8801c6c3f210 by task syz-executor0/18988

CPU: 0 PID: 18988 Comm: syz-executor0 Not tainted 4.14.0-rc5-next-20171017+ #34
Hardware name: Google Google Compute Engine/Google Compute Engine,
BIOS Google 01/01/2011
Call Trace:
 
 __dump_stack lib/dump_stack.c:16 [inline]
 dump_stack+0x173/0x237 lib/dump_stack.c:52
 print_address_description+0x6e/0x250 mm/kasan/report.c:252
 kasan_report_error mm/kasan/report.c:351 [inline]
 kasan_report+0x251/0x340 mm/kasan/report.c:409
 __read_once_size include/linux/compiler.h:276 [inline]
 deref_stack_reg+0x1b8/0x1d0 arch/x86/kernel/unwind_orc.c:282
 unwind_next_frame+0xebc/0x1df0 arch/x86/kernel/unwind_orc.c:426
 __save_stack_trace+0x6e/0xd0 arch/x86/kernel/stacktrace.c:44
 save_stack+0x32/0xb0 mm/kasan/kasan.c:447
 set_track mm/kasan/kasan.c:459 [inline]
 kasan_slab_free+0x71/0xc0 mm/kasan/kasan.c:524
 __cache_free mm/slab.c:3492 [inline]
 kfree+0xc8/0x250 mm/slab.c:3807
 security_cred_free+0x42/0x80 security/security.c:994
 put_cred_rcu+0xee/0x3c0 kernel/cred.c:117
 __rcu_reclaim kernel/rcu/rcu.h:172 [inline]
 rcu_do_batch kernel/rcu/tree.c:2676 [inline]
 invoke_rcu_callbacks kernel/rcu/tree.c:2930 [inline]
 __rcu_process_callbacks kernel/rcu/tree.c:2897 [inline]
 rcu_process_callbacks+0xcd4/0x1600 kernel/rcu/tree.c:2914
 __do_softirq+0x2ba/0xafb kernel/softirq.c:284
 invoke_softirq kernel/softirq.c:364 [inline]
 irq_exit+0x1c7/0x200 kernel/softirq.c:405
 exiting_irq arch/x86/include/asm/apic.h:540 [inline]
 smp_apic_timer_interrupt+0x154/0x6b0 arch/x86/kernel/apic/apic.c:1052
 apic_timer_interrupt+0x96/0xa0 arch/x86/entry/entry_64.S:770
 
RIP: 0010:arch_local_save_flags arch/x86/include/asm/paravirt.h:772 [inline]
RIP: 0010:arch_local_irq_save arch/x86/include/asm/paravirt.h:794 [inline]
RIP: 0010:lock_is_held_type+0x84/0x200 kernel/locking/lockdep.c:4025
RSP: 0018:8801c6c3f098 EFLAGS: 0282 ORIG_RAX: ff11
RAX: dc00 RBX: 8801d588e480 RCX: 
RDX: 10ad8f50 RSI:  RDI: 856c7a80
RBP: 8801dac78680 R08: 0002 R09: 
R10:  R11:  R12: 85737c60
R13: 81941b12 R14: 8801c9d83b40 R15: 8801c9d83678
 anon_vma_chain_free mm/rmap.c:133 [inline]
 unlink_anon_vmas+0x1e2/0x900 mm/rmap.c:400

The buggy address belongs to the page:
page:ea00071b0fc0 count:0 mapcount:0 mapping:  (null) index:0x0
flags: 0x200()
raw: 0200   
raw:  00010001  
page dumped because: kasan: bad access detected

Memory state around the buggy address:
 8801c6c3f100: 00 00 00 00 00 00 00 00 f1 f1 f1 f1 f8 f2 f2 f2
 8801c6c3f180: f2 f2 f2 f2 f8 f2 f2 f2 f2 f2 f2 f2 00 f2 f2 f2
>8801c6c3f200: f3 f3 f3 f3 00 00 00 00 00 00 00 00 00 00 00 00
 ^
 8801c6c3f280: 00 00 00 00 00 00 f1 f1 f1 f1 00 00 00 00 00 00
 8801c6c3f300: 00 00 00 00 00 00 00 00 00 00 00 f2 f2 f2 f3 f3
==


==
BUG: KASAN: stack-out-of-bounds in __read_once_size
include/linux/compiler.h:276 [inline]
BUG: KASAN: stack-out-of-bounds in deref_stack_reg+0x1b8/0x1d0
arch/x86/kernel/unwind_orc.c:282
Read of size 8 at addr 8801d4cdf340 by task syz-executor6/7070

CPU: 0 PID: 7070 Comm: syz-executor6 Not tainted 4.14.0-rc5-mm1+ #17
Hardware name: Google Google Compute Engine/Google Compute Engine,
BIOS Google 01/01/2011
Call Trace:
 
 __dump_stack lib/dump_stack.c:16 [inline]
 dump_stack+0x173/0x237 lib/dump_stack.c:52
 print_address_description+0x6e/0x250 mm/kasan/report.c:252
 kasan_report_error mm/kasan/report.c:351 [inline]
 kasan_report+0x251/0x340 mm/kasan/report.c:409
 __read_once_size include/linux/compiler.h:276 [inline]
 deref_stack_reg+0x1b8/0x1d0 arch/x86/kernel/unwind_orc.c:282
 unwind_next_frame+0xebc/0x1df0 arch/x86/kernel/unwind_orc.c:426
 __save_stack_trace+0x6e/0xd0 arch/x86/kernel/stacktrace.c:44
 save_stack+0x32/0xb0 mm/kasan/kasan.c:447
 set_track mm/kasan/kasan.c:459 [inline]
 kasan_slab_free+0x71/0xc0 mm/kasan/kasan.c:524
 __cache_free mm/slab.c:3492 [inline]
 kmem_cache_free+0x72/0x280 mm/slab.c:3750
 __rcu_reclaim kernel/rcu/rcu.h:172 [inline]
 rcu_do_batch kernel/rcu/tree.c:2676 [inline]
 invoke_rcu_callbacks 

orc unwinder: stack-out-of-bounds accesses

[Dmitry Vyukov]

Hello,

I am seeing lots of KASAN-detected stack-out-of-bounds accesses in the
new ORC unwinder. Examples of reports below. linux-next on
a7dd40274d75326ca868479c62090b1198a357ad.
You can reproduce this by enabling CONFIG_KASAN with gcc7+ (which
supports stack instrumentation).


==
BUG: KASAN: stack-out-of-bounds in __read_once_size
include/linux/compiler.h:276 [inline]
BUG: KASAN: stack-out-of-bounds in deref_stack_reg+0x1b8/0x1d0
arch/x86/kernel/unwind_orc.c:282
Read of size 8 at addr 8801c6c3f210 by task syz-executor0/18988

CPU: 0 PID: 18988 Comm: syz-executor0 Not tainted 4.14.0-rc5-next-20171017+ #34
Hardware name: Google Google Compute Engine/Google Compute Engine,
BIOS Google 01/01/2011
Call Trace:
 
 __dump_stack lib/dump_stack.c:16 [inline]
 dump_stack+0x173/0x237 lib/dump_stack.c:52
 print_address_description+0x6e/0x250 mm/kasan/report.c:252
 kasan_report_error mm/kasan/report.c:351 [inline]
 kasan_report+0x251/0x340 mm/kasan/report.c:409
 __read_once_size include/linux/compiler.h:276 [inline]
 deref_stack_reg+0x1b8/0x1d0 arch/x86/kernel/unwind_orc.c:282
 unwind_next_frame+0xebc/0x1df0 arch/x86/kernel/unwind_orc.c:426
 __save_stack_trace+0x6e/0xd0 arch/x86/kernel/stacktrace.c:44
 save_stack+0x32/0xb0 mm/kasan/kasan.c:447
 set_track mm/kasan/kasan.c:459 [inline]
 kasan_slab_free+0x71/0xc0 mm/kasan/kasan.c:524
 __cache_free mm/slab.c:3492 [inline]
 kfree+0xc8/0x250 mm/slab.c:3807
 security_cred_free+0x42/0x80 security/security.c:994
 put_cred_rcu+0xee/0x3c0 kernel/cred.c:117
 __rcu_reclaim kernel/rcu/rcu.h:172 [inline]
 rcu_do_batch kernel/rcu/tree.c:2676 [inline]
 invoke_rcu_callbacks kernel/rcu/tree.c:2930 [inline]
 __rcu_process_callbacks kernel/rcu/tree.c:2897 [inline]
 rcu_process_callbacks+0xcd4/0x1600 kernel/rcu/tree.c:2914
 __do_softirq+0x2ba/0xafb kernel/softirq.c:284
 invoke_softirq kernel/softirq.c:364 [inline]
 irq_exit+0x1c7/0x200 kernel/softirq.c:405
 exiting_irq arch/x86/include/asm/apic.h:540 [inline]
 smp_apic_timer_interrupt+0x154/0x6b0 arch/x86/kernel/apic/apic.c:1052
 apic_timer_interrupt+0x96/0xa0 arch/x86/entry/entry_64.S:770
 
RIP: 0010:arch_local_save_flags arch/x86/include/asm/paravirt.h:772 [inline]
RIP: 0010:arch_local_irq_save arch/x86/include/asm/paravirt.h:794 [inline]
RIP: 0010:lock_is_held_type+0x84/0x200 kernel/locking/lockdep.c:4025
RSP: 0018:8801c6c3f098 EFLAGS: 0282 ORIG_RAX: ff11
RAX: dc00 RBX: 8801d588e480 RCX: 
RDX: 10ad8f50 RSI:  RDI: 856c7a80
RBP: 8801dac78680 R08: 0002 R09: 
R10:  R11:  R12: 85737c60
R13: 81941b12 R14: 8801c9d83b40 R15: 8801c9d83678
 anon_vma_chain_free mm/rmap.c:133 [inline]
 unlink_anon_vmas+0x1e2/0x900 mm/rmap.c:400

The buggy address belongs to the page:
page:ea00071b0fc0 count:0 mapcount:0 mapping:  (null) index:0x0
flags: 0x200()
raw: 0200   
raw:  00010001  
page dumped because: kasan: bad access detected

Memory state around the buggy address:
 8801c6c3f100: 00 00 00 00 00 00 00 00 f1 f1 f1 f1 f8 f2 f2 f2
 8801c6c3f180: f2 f2 f2 f2 f8 f2 f2 f2 f2 f2 f2 f2 00 f2 f2 f2
>8801c6c3f200: f3 f3 f3 f3 00 00 00 00 00 00 00 00 00 00 00 00
 ^
 8801c6c3f280: 00 00 00 00 00 00 f1 f1 f1 f1 00 00 00 00 00 00
 8801c6c3f300: 00 00 00 00 00 00 00 00 00 00 00 f2 f2 f2 f3 f3
==


==
BUG: KASAN: stack-out-of-bounds in __read_once_size
include/linux/compiler.h:276 [inline]
BUG: KASAN: stack-out-of-bounds in deref_stack_reg+0x1b8/0x1d0
arch/x86/kernel/unwind_orc.c:282
Read of size 8 at addr 8801d4cdf340 by task syz-executor6/7070

CPU: 0 PID: 7070 Comm: syz-executor6 Not tainted 4.14.0-rc5-mm1+ #17
Hardware name: Google Google Compute Engine/Google Compute Engine,
BIOS Google 01/01/2011
Call Trace:
 
 __dump_stack lib/dump_stack.c:16 [inline]
 dump_stack+0x173/0x237 lib/dump_stack.c:52
 print_address_description+0x6e/0x250 mm/kasan/report.c:252
 kasan_report_error mm/kasan/report.c:351 [inline]
 kasan_report+0x251/0x340 mm/kasan/report.c:409
 __read_once_size include/linux/compiler.h:276 [inline]
 deref_stack_reg+0x1b8/0x1d0 arch/x86/kernel/unwind_orc.c:282
 unwind_next_frame+0xebc/0x1df0 arch/x86/kernel/unwind_orc.c:426
 __save_stack_trace+0x6e/0xd0 arch/x86/kernel/stacktrace.c:44
 save_stack+0x32/0xb0 mm/kasan/kasan.c:447
 set_track mm/kasan/kasan.c:459 [inline]
 kasan_slab_free+0x71/0xc0 mm/kasan/kasan.c:524
 __cache_free mm/slab.c:3492 [inline]
 kmem_cache_free+0x72/0x280 mm/slab.c:3750
 __rcu_reclaim kernel/rcu/rcu.h:172 [inline]
 rcu_do_batch kernel/rcu/tree.c:2676 [inline]
 invoke_rcu_callbacks 

Re: [Patch v6 3/7] slimbus: qcom: Add Qualcomm Slimbus controller driver

[Srinivas Kandagatla]

Thanks for Review Comments.

On 18/10/17 08:27, Bjorn Andersson wrote:

On Fri 06 Oct 08:51 PDT 2017, srinivas.kandaga...@linaro.org wrote:


From: Sagar Dharia 

This controller driver programs manager, interface, and framer
devices for Qualcomm's slimbus HW block.
Manager component currently implements logical address setting,
and messaging interface.
Interface device reports bus synchronization information, and framer
device clocks the bus from the time it's woken up, until clock-pause
is executed by the manager device.

Signed-off-by: Sagar Dharia 
Signed-off-by: Srinivas Kandagatla 
---
  .../devicetree/bindings/slimbus/slim-qcom-ctrl.txt |  43 ++
  drivers/slimbus/Kconfig|   9 +
  drivers/slimbus/Makefile   |   3 +
  drivers/slimbus/slim-qcom-ctrl.c   | 594 +
  drivers/slimbus/slim-qcom.h|  63 +++
  5 files changed, 712 insertions(+)
  create mode 100644 
Documentation/devicetree/bindings/slimbus/slim-qcom-ctrl.txt
  create mode 100644 drivers/slimbus/slim-qcom-ctrl.c
  create mode 100644 drivers/slimbus/slim-qcom.h

diff --git a/Documentation/devicetree/bindings/slimbus/slim-qcom-ctrl.txt 
b/Documentation/devicetree/bindings/slimbus/slim-qcom-ctrl.txt
new file mode 100644
index 000..081110d
--- /dev/null
+++ b/Documentation/devicetree/bindings/slimbus/slim-qcom-ctrl.txt
@@ -0,0 +1,43 @@
+Qualcomm SLIMBUS controller
+This controller is used if applications processor driver controls slimbus
+master component.
+
+Required properties:
+
+ - #address-cells - refer to Documentation/devicetree/bindings/slimbus/bus.txt
+ - #size-cells - refer to Documentation/devicetree/bindings/slimbus/bus.txt
+
+ - reg : Offset and length of the register region(s) for the device
+ - reg-names : Register region name(s) referenced in reg above
+Required register resource entries are:
+"ctrl": Physical adderess of controller register blocks
+ - compatible : should be "qcom,-slim" for SOC specific compatible or


As you implementation is only compatible with "qcom,slim" this should be
"and" not "or".

yep,




+   "qcom,slim" if using generic qcom SLIM IP.
+ - interrupts : Interrupt number used by this controller
+ - clocks : Interface and core clocks used by this slimbus controller
+ - clock-names : Required clock-name entries are:
+   "iface_clk" : Interface clock for this controller
+   "core_clk" : Interrupt for controller core's BAM
+
+
+Optional property:
+ - reg entry for slew rate : If slew rate control register is provided, this
+   entry should be used.
+ - reg-name for slew rate: "slew"
+
+Example:
+   slim@2808 {
+   compatible = "qcom,slim";
+   #address-cells = <4>;
+   #size-cells = <0>;
+   reg = <0x2808 0x2000>, <0x80207C 4>;
+   reg-names = "ctrl", "slew";
+   interrupts = <0 33 0>;
+   clocks = < SLIMBUS_SRC>, < AUDIO_SLIMBUS_CLK>;
+   clock-names = "iface_clk", "core_clk";
+
+   codec: wcd9310@1{
+   compatible = "slim217,60";
+   reg = <1 0>;
+   };
+   };
diff --git a/drivers/slimbus/Kconfig b/drivers/slimbus/Kconfig
index f0b118a..438773e 100644
--- a/drivers/slimbus/Kconfig
+++ b/drivers/slimbus/Kconfig
@@ -9,3 +9,12 @@ menuconfig SLIMBUS
  
  	  If unsure, choose N.

yep.

  
+if SLIMBUS

+config SLIM_QCOM_CTRL
+   tristate "Qualcomm Slimbus Manager Component"
+   depends on SLIMBUS
+   help
+ Select driver if Qualcomm's Slimbus Manager Component is
+ programmed using Linux kernel.
+
+endif
diff --git a/drivers/slimbus/Makefile b/drivers/slimbus/Makefile
index bd1d35e..ed8521a 100644
--- a/drivers/slimbus/Makefile
+++ b/drivers/slimbus/Makefile
@@ -3,3 +3,6 @@
  #
  obj-$(CONFIG_SLIMBUS) += slimbus.o
  slimbus-y := slim-core.o slim-messaging.o
+
+#Controllers
+obj-$(CONFIG_SLIM_QCOM_CTRL)   += slim-qcom-ctrl.o
diff --git a/drivers/slimbus/slim-qcom-ctrl.c b/drivers/slimbus/slim-qcom-ctrl.c
new file mode 100644
index 000..d0574e1
--- /dev/null
+++ b/drivers/slimbus/slim-qcom-ctrl.c

..

+#include "slim-qcom.h"
+
+#define MSM_SLIM_NAME  "msm_slim_ctrl"


Just put this string in the driver struct.


yes..




+
+/* Manager registers */
+#defineMGR_CFG 0x200

...



+
+static int msm_slim_queue_tx(struct msm_slim_ctrl *dev, u32 *buf, u8 len,
+u32 tx_reg)


Use void * for buf.


okay.


+{
+   int i;
+
+   for (i = 0; i < (len + 3) >> 2; i++) {


If len is in bytes then this looks like you will read outside the
buffer. If buf is guaranteed to be 4-byte aligned make "len" number of
32-bit entries in the array.


+   dev_dbg(dev->dev, "AHB TX data:0x%x\n", 

Re: [Patch v6 3/7] slimbus: qcom: Add Qualcomm Slimbus controller driver

[Srinivas Kandagatla]

Thanks for Review Comments.

On 18/10/17 08:27, Bjorn Andersson wrote:

On Fri 06 Oct 08:51 PDT 2017, srinivas.kandaga...@linaro.org wrote:


From: Sagar Dharia 

This controller driver programs manager, interface, and framer
devices for Qualcomm's slimbus HW block.
Manager component currently implements logical address setting,
and messaging interface.
Interface device reports bus synchronization information, and framer
device clocks the bus from the time it's woken up, until clock-pause
is executed by the manager device.

Signed-off-by: Sagar Dharia 
Signed-off-by: Srinivas Kandagatla 
---
  .../devicetree/bindings/slimbus/slim-qcom-ctrl.txt |  43 ++
  drivers/slimbus/Kconfig|   9 +
  drivers/slimbus/Makefile   |   3 +
  drivers/slimbus/slim-qcom-ctrl.c   | 594 +
  drivers/slimbus/slim-qcom.h|  63 +++
  5 files changed, 712 insertions(+)
  create mode 100644 
Documentation/devicetree/bindings/slimbus/slim-qcom-ctrl.txt
  create mode 100644 drivers/slimbus/slim-qcom-ctrl.c
  create mode 100644 drivers/slimbus/slim-qcom.h

diff --git a/Documentation/devicetree/bindings/slimbus/slim-qcom-ctrl.txt 
b/Documentation/devicetree/bindings/slimbus/slim-qcom-ctrl.txt
new file mode 100644
index 000..081110d
--- /dev/null
+++ b/Documentation/devicetree/bindings/slimbus/slim-qcom-ctrl.txt
@@ -0,0 +1,43 @@
+Qualcomm SLIMBUS controller
+This controller is used if applications processor driver controls slimbus
+master component.
+
+Required properties:
+
+ - #address-cells - refer to Documentation/devicetree/bindings/slimbus/bus.txt
+ - #size-cells - refer to Documentation/devicetree/bindings/slimbus/bus.txt
+
+ - reg : Offset and length of the register region(s) for the device
+ - reg-names : Register region name(s) referenced in reg above
+Required register resource entries are:
+"ctrl": Physical adderess of controller register blocks
+ - compatible : should be "qcom,-slim" for SOC specific compatible or


As you implementation is only compatible with "qcom,slim" this should be
"and" not "or".

yep,




+   "qcom,slim" if using generic qcom SLIM IP.
+ - interrupts : Interrupt number used by this controller
+ - clocks : Interface and core clocks used by this slimbus controller
+ - clock-names : Required clock-name entries are:
+   "iface_clk" : Interface clock for this controller
+   "core_clk" : Interrupt for controller core's BAM
+
+
+Optional property:
+ - reg entry for slew rate : If slew rate control register is provided, this
+   entry should be used.
+ - reg-name for slew rate: "slew"
+
+Example:
+   slim@2808 {
+   compatible = "qcom,slim";
+   #address-cells = <4>;
+   #size-cells = <0>;
+   reg = <0x2808 0x2000>, <0x80207C 4>;
+   reg-names = "ctrl", "slew";
+   interrupts = <0 33 0>;
+   clocks = < SLIMBUS_SRC>, < AUDIO_SLIMBUS_CLK>;
+   clock-names = "iface_clk", "core_clk";
+
+   codec: wcd9310@1{
+   compatible = "slim217,60";
+   reg = <1 0>;
+   };
+   };
diff --git a/drivers/slimbus/Kconfig b/drivers/slimbus/Kconfig
index f0b118a..438773e 100644
--- a/drivers/slimbus/Kconfig
+++ b/drivers/slimbus/Kconfig
@@ -9,3 +9,12 @@ menuconfig SLIMBUS
  
  	  If unsure, choose N.

yep.

  
+if SLIMBUS

+config SLIM_QCOM_CTRL
+   tristate "Qualcomm Slimbus Manager Component"
+   depends on SLIMBUS
+   help
+ Select driver if Qualcomm's Slimbus Manager Component is
+ programmed using Linux kernel.
+
+endif
diff --git a/drivers/slimbus/Makefile b/drivers/slimbus/Makefile
index bd1d35e..ed8521a 100644
--- a/drivers/slimbus/Makefile
+++ b/drivers/slimbus/Makefile
@@ -3,3 +3,6 @@
  #
  obj-$(CONFIG_SLIMBUS) += slimbus.o
  slimbus-y := slim-core.o slim-messaging.o
+
+#Controllers
+obj-$(CONFIG_SLIM_QCOM_CTRL)   += slim-qcom-ctrl.o
diff --git a/drivers/slimbus/slim-qcom-ctrl.c b/drivers/slimbus/slim-qcom-ctrl.c
new file mode 100644
index 000..d0574e1
--- /dev/null
+++ b/drivers/slimbus/slim-qcom-ctrl.c

..

+#include "slim-qcom.h"
+
+#define MSM_SLIM_NAME  "msm_slim_ctrl"


Just put this string in the driver struct.


yes..




+
+/* Manager registers */
+#defineMGR_CFG 0x200

...



+
+static int msm_slim_queue_tx(struct msm_slim_ctrl *dev, u32 *buf, u8 len,
+u32 tx_reg)


Use void * for buf.


okay.


+{
+   int i;
+
+   for (i = 0; i < (len + 3) >> 2; i++) {


If len is in bytes then this looks like you will read outside the
buffer. If buf is guaranteed to be 4-byte aligned make "len" number of
32-bit entries in the array.


+   dev_dbg(dev->dev, "AHB TX data:0x%x\n", buf[i]);


Drop the debug print, if anything use print_hex_dump to get the whole

Re: [PATCH] Makefile: add targets for config-help and pkg-help

[Shuah Khan]

On 10/18/2017 10:23 AM, Masahiro Yamada wrote:
> 2017-10-18 0:18 GMT+09:00 Shuah Khan :
>> Change to enable config help and package help from the main make level
>> to make it easier to use. It has become difficult to find config help
>> and pkg help specific output from the "help" information.
>>
>> Signed-off-by: Shuah Khan 
>> ---
>>  Makefile | 7 +++
>>  1 file changed, 7 insertions(+)
>>
>> diff --git a/Makefile b/Makefile
>> index 46bfb0ed2257..1d6f86df1b6c 100644
>> --- a/Makefile
>> +++ b/Makefile
>> @@ -1441,6 +1441,13 @@ help:
>> @echo  'Execute "make" or "make all" to build all targets marked 
>> with [*] '
>> @echo  'For further info see the ./README file'
>>
>> +PHONY += config-help
>> +config-help:
>> +   @$(MAKE) -f $(srctree)/scripts/kconfig/Makefile help
>> +
>> +PHONY += pkg-help
>> +pkg-help:
>> +   @$(MAKE) $(build)=$(package-dir) help
>>
>>  help-board-dirs := $(addprefix help-,$(board-dirs))
>>
>> --
> 
> 
> What happened to "doc-help" ?
> (I want to see consistent hyphenation)

I added doc-help to DOC_TARGETS directly. I should have done this
patch on top of the last one.

> 
> Please follow Randy's suggestion.

I will re-do this patch as per Randy's suggestions.
> 
> Also you need to add %-help pattern to no-dot-config-targets.
> 

Right. I will send v2 this week if possible. I have a couple of
trips coming up including the Kernel Summit.

thanks,
-- Shuah

Re: [PATCH] Makefile: add targets for config-help and pkg-help

[Shuah Khan]

On 10/18/2017 10:23 AM, Masahiro Yamada wrote:
> 2017-10-18 0:18 GMT+09:00 Shuah Khan :
>> Change to enable config help and package help from the main make level
>> to make it easier to use. It has become difficult to find config help
>> and pkg help specific output from the "help" information.
>>
>> Signed-off-by: Shuah Khan 
>> ---
>>  Makefile | 7 +++
>>  1 file changed, 7 insertions(+)
>>
>> diff --git a/Makefile b/Makefile
>> index 46bfb0ed2257..1d6f86df1b6c 100644
>> --- a/Makefile
>> +++ b/Makefile
>> @@ -1441,6 +1441,13 @@ help:
>> @echo  'Execute "make" or "make all" to build all targets marked 
>> with [*] '
>> @echo  'For further info see the ./README file'
>>
>> +PHONY += config-help
>> +config-help:
>> +   @$(MAKE) -f $(srctree)/scripts/kconfig/Makefile help
>> +
>> +PHONY += pkg-help
>> +pkg-help:
>> +   @$(MAKE) $(build)=$(package-dir) help
>>
>>  help-board-dirs := $(addprefix help-,$(board-dirs))
>>
>> --
> 
> 
> What happened to "doc-help" ?
> (I want to see consistent hyphenation)

I added doc-help to DOC_TARGETS directly. I should have done this
patch on top of the last one.

> 
> Please follow Randy's suggestion.

I will re-do this patch as per Randy's suggestions.
> 
> Also you need to add %-help pattern to no-dot-config-targets.
> 

Right. I will send v2 this week if possible. I have a couple of
trips coming up including the Kernel Summit.

thanks,
-- Shuah

Re: [Patch v6 2/7] slimbus: Add messaging APIs to slimbus framework

[Srinivas Kandagatla]

Thanks for Review Comments,


On 18/10/17 07:15, Bjorn Andersson wrote:

On Fri 06 Oct 08:51 PDT 2017, srinivas.kandaga...@linaro.org wrote:


From: Sagar Dharia 

Slimbus devices use value-element, and information elements to
control device parameters (e.g. value element is used to represent
gain for codec, information element is used to represent interrupt
status for codec when codec interrupt fires).
Messaging APIs are used to set/get these value and information
elements. Slimbus specification uses 8-bit "transaction IDs" for
messages where a read-value is anticipated. Framework uses a table
of pointers to store those TIDs and responds back to the caller in
O(1).


I think we can implement this "optimization" with less complex code,
regardless I don't think we need to mention this in the commit
message...

[..]

diff --git a/drivers/slimbus/slim-messaging.c b/drivers/slimbus/slim-messaging.c

[..]

+/**
+ * slim_msg_response: Deliver Message response received from a device to the
+ * framework.
+ * @ctrl: Controller handle
+ * @reply: Reply received from the device
+ * @len: Length of the reply
+ * @tid: Transaction ID received with which framework can associate reply.
+ * Called by controller to inform framework about the response received.
+ * This helps in making the API asynchronous, and controller-driver doesn't 
need
+ * to manage 1 more table other than the one managed by framework mapping TID
+ * with buffers
+ */
+void slim_msg_response(struct slim_controller *ctrl, u8 *reply, u8 tid, u8 len)


Even if tid and len comes from the spec I recommend you making them int
and size_t.

okay, will give that a go.



+{
+   struct slim_val_inf *msg;
+   unsigned long flags;
+
+   spin_lock_irqsave(>txn_lock, flags);
+   msg = ctrl->tid_tbl[tid];
+   if (msg == NULL || msg->rbuf == NULL) {


if (!msg || !msg->rbuf)


When is it valid to add a transaction to tid_tbl with msg->rbuf = NULL?
Should we reject it earlier?


We do sanity checks before posting the request, however there are cases 
where this checks are not in place like calling slim_processtxn() directly.


May be we should add this check all the case



+   spin_unlock_irqrestore(>txn_lock, flags);
+   dev_err(>dev, "Got response to invalid TID:%d, len:%d\n",
+   tid, len);
+   return;
+   }
+   ctrl->tid_tbl[tid] = NULL;
+   spin_unlock_irqrestore(>txn_lock, flags);
+
+   memcpy(msg->rbuf, reply, len);
+   if (msg->comp_cb)
+   msg->comp_cb(msg->ctx, 0);
+}
+EXPORT_SYMBOL_GPL(slim_msg_response);

[..]

+int slim_processtxn(struct slim_controller *ctrl,
+   struct slim_msg_txn *txn)
+{
+   int ret, i = 0;
+   unsigned long flags;
+   u8 *buf;
+   bool async = false;
+   struct slim_cb_data cbd;
+   DECLARE_COMPLETION_ONSTACK(done);
+   bool need_tid = slim_tid_txn(txn->mt, txn->mc);
+
+   if (!txn->msg->comp_cb) {
+   txn->msg->comp_cb = slim_sync_default_cb;
+   cbd.comp = 
+   txn->msg->ctx = 
+   } else {
+   async = true;
+   }
+
+   buf = slim_get_tx(ctrl, txn, need_tid);
+   if (!buf)
+   return -ENOMEM;
+
+   if (need_tid) {
+   spin_lock_irqsave(>txn_lock, flags);
+   for (i = 0; i < ctrl->last_tid; i++) {
+   if (ctrl->tid_tbl[i] == NULL)
+   break;
+   }
+   if (i >= ctrl->last_tid) {
+   if (ctrl->last_tid == (SLIM_MAX_TIDS - 1)) {
+   spin_unlock_irqrestore(>txn_lock, flags);
+   slim_return_tx(ctrl, -ENOMEM);
+   return -ENOMEM;
+   }
+   ctrl->last_tid++;
+   }
+   ctrl->tid_tbl[i] = txn->msg;
+   txn->tid = i;
+   spin_unlock_irqrestore(>txn_lock, flags);
+   }
+
+   ret = ctrl->xfer_msg(ctrl, txn, buf);
+
+   if (!ret && !async) { /* sync transaction */
+   /* Fine-tune calculation after bandwidth management */
+   unsigned long ms = txn->rl + 100;
+
+   ret = wait_for_completion_timeout(,
+ msecs_to_jiffies(ms));
+   if (!ret)
+   slim_return_tx(ctrl, -ETIMEDOUT);
+
+   ret = cbd.ret;
+   }
+
+   if (ret && need_tid) {
+   spin_lock_irqsave(>txn_lock, flags);
+   /* Invalidate the transaction */
+   ctrl->tid_tbl[txn->tid] = NULL;
+   spin_unlock_irqrestore(>txn_lock, flags);
+   }
+   if (ret)
+   dev_err(>dev, "Tx:MT:0x%x, MC:0x%x, LA:0x%x failed:%d\n",
+   txn->mt, txn->mc, txn->la, ret);


if (ret) {
if (need_tid)
 

Re: [Patch v6 2/7] slimbus: Add messaging APIs to slimbus framework

[Srinivas Kandagatla]

Thanks for Review Comments,


On 18/10/17 07:15, Bjorn Andersson wrote:

On Fri 06 Oct 08:51 PDT 2017, srinivas.kandaga...@linaro.org wrote:


From: Sagar Dharia 

Slimbus devices use value-element, and information elements to
control device parameters (e.g. value element is used to represent
gain for codec, information element is used to represent interrupt
status for codec when codec interrupt fires).
Messaging APIs are used to set/get these value and information
elements. Slimbus specification uses 8-bit "transaction IDs" for
messages where a read-value is anticipated. Framework uses a table
of pointers to store those TIDs and responds back to the caller in
O(1).


I think we can implement this "optimization" with less complex code,
regardless I don't think we need to mention this in the commit
message...

[..]

diff --git a/drivers/slimbus/slim-messaging.c b/drivers/slimbus/slim-messaging.c

[..]

+/**
+ * slim_msg_response: Deliver Message response received from a device to the
+ * framework.
+ * @ctrl: Controller handle
+ * @reply: Reply received from the device
+ * @len: Length of the reply
+ * @tid: Transaction ID received with which framework can associate reply.
+ * Called by controller to inform framework about the response received.
+ * This helps in making the API asynchronous, and controller-driver doesn't 
need
+ * to manage 1 more table other than the one managed by framework mapping TID
+ * with buffers
+ */
+void slim_msg_response(struct slim_controller *ctrl, u8 *reply, u8 tid, u8 len)


Even if tid and len comes from the spec I recommend you making them int
and size_t.

okay, will give that a go.



+{
+   struct slim_val_inf *msg;
+   unsigned long flags;
+
+   spin_lock_irqsave(>txn_lock, flags);
+   msg = ctrl->tid_tbl[tid];
+   if (msg == NULL || msg->rbuf == NULL) {


if (!msg || !msg->rbuf)


When is it valid to add a transaction to tid_tbl with msg->rbuf = NULL?
Should we reject it earlier?


We do sanity checks before posting the request, however there are cases 
where this checks are not in place like calling slim_processtxn() directly.


May be we should add this check all the case



+   spin_unlock_irqrestore(>txn_lock, flags);
+   dev_err(>dev, "Got response to invalid TID:%d, len:%d\n",
+   tid, len);
+   return;
+   }
+   ctrl->tid_tbl[tid] = NULL;
+   spin_unlock_irqrestore(>txn_lock, flags);
+
+   memcpy(msg->rbuf, reply, len);
+   if (msg->comp_cb)
+   msg->comp_cb(msg->ctx, 0);
+}
+EXPORT_SYMBOL_GPL(slim_msg_response);

[..]

+int slim_processtxn(struct slim_controller *ctrl,
+   struct slim_msg_txn *txn)
+{
+   int ret, i = 0;
+   unsigned long flags;
+   u8 *buf;
+   bool async = false;
+   struct slim_cb_data cbd;
+   DECLARE_COMPLETION_ONSTACK(done);
+   bool need_tid = slim_tid_txn(txn->mt, txn->mc);
+
+   if (!txn->msg->comp_cb) {
+   txn->msg->comp_cb = slim_sync_default_cb;
+   cbd.comp = 
+   txn->msg->ctx = 
+   } else {
+   async = true;
+   }
+
+   buf = slim_get_tx(ctrl, txn, need_tid);
+   if (!buf)
+   return -ENOMEM;
+
+   if (need_tid) {
+   spin_lock_irqsave(>txn_lock, flags);
+   for (i = 0; i < ctrl->last_tid; i++) {
+   if (ctrl->tid_tbl[i] == NULL)
+   break;
+   }
+   if (i >= ctrl->last_tid) {
+   if (ctrl->last_tid == (SLIM_MAX_TIDS - 1)) {
+   spin_unlock_irqrestore(>txn_lock, flags);
+   slim_return_tx(ctrl, -ENOMEM);
+   return -ENOMEM;
+   }
+   ctrl->last_tid++;
+   }
+   ctrl->tid_tbl[i] = txn->msg;
+   txn->tid = i;
+   spin_unlock_irqrestore(>txn_lock, flags);
+   }
+
+   ret = ctrl->xfer_msg(ctrl, txn, buf);
+
+   if (!ret && !async) { /* sync transaction */
+   /* Fine-tune calculation after bandwidth management */
+   unsigned long ms = txn->rl + 100;
+
+   ret = wait_for_completion_timeout(,
+ msecs_to_jiffies(ms));
+   if (!ret)
+   slim_return_tx(ctrl, -ETIMEDOUT);
+
+   ret = cbd.ret;
+   }
+
+   if (ret && need_tid) {
+   spin_lock_irqsave(>txn_lock, flags);
+   /* Invalidate the transaction */
+   ctrl->tid_tbl[txn->tid] = NULL;
+   spin_unlock_irqrestore(>txn_lock, flags);
+   }
+   if (ret)
+   dev_err(>dev, "Tx:MT:0x%x, MC:0x%x, LA:0x%x failed:%d\n",
+   txn->mt, txn->mc, txn->la, ret);


if (ret) {
if (need_tid)
drop();

Re: [Patch v6 1/7] slimbus: Device management on SLIMbus

[Srinivas Kandagatla]

Thanks for the Review Bjorn,

On 17/10/17 07:23, Bjorn Andersson wrote:

On Fri 06 Oct 08:51 PDT 2017, srinivas.kandaga...@linaro.org wrote:
[..]

diff --git a/drivers/slimbus/slim-core.c b/drivers/slimbus/slim-core.c

[..]

+/**
+ * Report callbacks(device_up, device_down) are implemented by slimbus-devices.
+ * The calls are scheduled into a workqueue to avoid holding up controller
+ * initialization/tear-down.
+ */
+static void schedule_slim_report(struct slim_controller *ctrl,
+struct slim_device *sb, bool report)
+{
+   struct sb_report_wd *sbw;
+
+   dev_dbg(>dev, "report:%d for slave:%s\n", report, sb->name);


This is the only place where sb->name is used in this driver. If you
instead invoke dev_*() on >dev you should get prettier output and
can drop the double storage of the device name.


Makes sense, we could get rid of sb->name storage too.




+
+   sbw = kmalloc(sizeof(*sbw), GFP_KERNEL);
+   if (!sbw)
+   return;
+
+   INIT_WORK(>wd, slim_report);
+   sbw->sbdev = sb;
+   sbw->report = report;
+   if (!queue_work(ctrl->wq, >wd)) {


When a controller is torn down destroy_workqueue() is called after all
child devices has been unregistered, so this work might be scheduled
after "sb" is gone, if I get this properly.


I agree, That is possible!
We should probably flush the workqueue before we start removing the clients.




+   dev_err(>dev, "failed to queue report:%d slave:%s\n",
+   report, sb->name);
+   kfree(sbw);
+   }
+}
+
+static int slim_device_probe(struct device *dev)
+{
+   struct slim_device  *sbdev;
+   struct slim_driver  *sbdrv;
+   int status = 0;
+
+   sbdev = to_slim_device(dev);
+   sbdrv = to_slim_driver(dev->driver);
+
+   sbdev->driver = sbdrv;
+
+   if (sbdrv->probe)
+   status = sbdrv->probe(sbdev);


So a driver can have a probe() and device_up() or just any one of them?

And probe() is called when the controller enumerates all devices
mentioned in DT and then device_up() is called at that point in time and
when it's advertised on the bus?

Is there a reason for this split model?

yes, Some of the devices need to be powered up before they become 
usable, so probe is used to do the initial power up of the device.




+
+   if (status)
+   sbdev->driver = NULL;
+   else if (sbdrv->device_up)
+   schedule_slim_report(sbdev->ctrl, sbdev, true);
+
+   return status;
+}
+
+static int slim_device_remove(struct device *dev)
+{
+   struct slim_device *sbdev;
+   struct slim_driver *sbdrv;
+   int status = 0;
+
+   sbdev = to_slim_device(dev);
+   if (!dev->driver)
+   return 0;
+
+   sbdrv = to_slim_driver(dev->driver);
+   if (sbdrv->remove)
+   status = sbdrv->remove(sbdev);
+
+   mutex_lock(>report_lock);
+   sbdev->notified = false;
+   if (status == 0)
+   sbdev->driver = NULL;
+   mutex_unlock(>report_lock);
+   return status;


device_unregister() will call device_del() which will end up in
__device_release_driver() which will call this function. Upon returning
from this function the core expect the bus to have cleaned up after the
dev (normally by calling drv->remove(dev)).

It will completely ignore the return value and continue tearing down the
rest of the core resources, e.g. three lines down it will
devres_release_all().


So you have the option of sleeping, while waiting for stuff to be
aborted/finished and then you need to clean things up.

The slim_device object itself will stick around until all references are
dropped though.


So you are suggesting that we make slim_driver remove not return anything?




+}
+
+struct bus_type slimbus_type = {
+   .name   = "slimbus",
+   .match  = slim_device_match,
+   .probe  = slim_device_probe,
+   .remove = slim_device_remove,
+};
+EXPORT_SYMBOL_GPL(slimbus_type);
+
+/**
+ * slim_driver_register: Client driver registration with slimbus
+ * @drv:Client driver to be associated with client-device.
+ * @owner: owning module/driver
+ * This API will register the client driver with the slimbus
+ * It is called from the driver's module-init function.
+ */
+int __slim_driver_register(struct slim_driver *drv, struct module *owner)
+{
+   drv->driver.bus = _type;
+   drv->driver.owner = owner;
+   return driver_register(>driver);
+}
+EXPORT_SYMBOL_GPL(__slim_driver_register);
+
+/**
+ * slim_driver_unregister: Undo effect of slim_driver_register
+ * @drv: Client driver to be unregistered
+ */
+void slim_driver_unregister(struct slim_driver *drv)
+{
+   if (drv)


A driver invoking slim_driver_unregister(NULL) is broken, drop this
check and let it oops on the dereference instead.


Yep.




+   driver_unregister(>driver);
+}

Re: [Patch v6 1/7] slimbus: Device management on SLIMbus

[Srinivas Kandagatla]

Thanks for the Review Bjorn,

On 17/10/17 07:23, Bjorn Andersson wrote:

On Fri 06 Oct 08:51 PDT 2017, srinivas.kandaga...@linaro.org wrote:
[..]

diff --git a/drivers/slimbus/slim-core.c b/drivers/slimbus/slim-core.c

[..]

+/**
+ * Report callbacks(device_up, device_down) are implemented by slimbus-devices.
+ * The calls are scheduled into a workqueue to avoid holding up controller
+ * initialization/tear-down.
+ */
+static void schedule_slim_report(struct slim_controller *ctrl,
+struct slim_device *sb, bool report)
+{
+   struct sb_report_wd *sbw;
+
+   dev_dbg(>dev, "report:%d for slave:%s\n", report, sb->name);


This is the only place where sb->name is used in this driver. If you
instead invoke dev_*() on >dev you should get prettier output and
can drop the double storage of the device name.


Makes sense, we could get rid of sb->name storage too.




+
+   sbw = kmalloc(sizeof(*sbw), GFP_KERNEL);
+   if (!sbw)
+   return;
+
+   INIT_WORK(>wd, slim_report);
+   sbw->sbdev = sb;
+   sbw->report = report;
+   if (!queue_work(ctrl->wq, >wd)) {


When a controller is torn down destroy_workqueue() is called after all
child devices has been unregistered, so this work might be scheduled
after "sb" is gone, if I get this properly.


I agree, That is possible!
We should probably flush the workqueue before we start removing the clients.




+   dev_err(>dev, "failed to queue report:%d slave:%s\n",
+   report, sb->name);
+   kfree(sbw);
+   }
+}
+
+static int slim_device_probe(struct device *dev)
+{
+   struct slim_device  *sbdev;
+   struct slim_driver  *sbdrv;
+   int status = 0;
+
+   sbdev = to_slim_device(dev);
+   sbdrv = to_slim_driver(dev->driver);
+
+   sbdev->driver = sbdrv;
+
+   if (sbdrv->probe)
+   status = sbdrv->probe(sbdev);


So a driver can have a probe() and device_up() or just any one of them?

And probe() is called when the controller enumerates all devices
mentioned in DT and then device_up() is called at that point in time and
when it's advertised on the bus?

Is there a reason for this split model?

yes, Some of the devices need to be powered up before they become 
usable, so probe is used to do the initial power up of the device.




+
+   if (status)
+   sbdev->driver = NULL;
+   else if (sbdrv->device_up)
+   schedule_slim_report(sbdev->ctrl, sbdev, true);
+
+   return status;
+}
+
+static int slim_device_remove(struct device *dev)
+{
+   struct slim_device *sbdev;
+   struct slim_driver *sbdrv;
+   int status = 0;
+
+   sbdev = to_slim_device(dev);
+   if (!dev->driver)
+   return 0;
+
+   sbdrv = to_slim_driver(dev->driver);
+   if (sbdrv->remove)
+   status = sbdrv->remove(sbdev);
+
+   mutex_lock(>report_lock);
+   sbdev->notified = false;
+   if (status == 0)
+   sbdev->driver = NULL;
+   mutex_unlock(>report_lock);
+   return status;


device_unregister() will call device_del() which will end up in
__device_release_driver() which will call this function. Upon returning
from this function the core expect the bus to have cleaned up after the
dev (normally by calling drv->remove(dev)).

It will completely ignore the return value and continue tearing down the
rest of the core resources, e.g. three lines down it will
devres_release_all().


So you have the option of sleeping, while waiting for stuff to be
aborted/finished and then you need to clean things up.

The slim_device object itself will stick around until all references are
dropped though.


So you are suggesting that we make slim_driver remove not return anything?




+}
+
+struct bus_type slimbus_type = {
+   .name   = "slimbus",
+   .match  = slim_device_match,
+   .probe  = slim_device_probe,
+   .remove = slim_device_remove,
+};
+EXPORT_SYMBOL_GPL(slimbus_type);
+
+/**
+ * slim_driver_register: Client driver registration with slimbus
+ * @drv:Client driver to be associated with client-device.
+ * @owner: owning module/driver
+ * This API will register the client driver with the slimbus
+ * It is called from the driver's module-init function.
+ */
+int __slim_driver_register(struct slim_driver *drv, struct module *owner)
+{
+   drv->driver.bus = _type;
+   drv->driver.owner = owner;
+   return driver_register(>driver);
+}
+EXPORT_SYMBOL_GPL(__slim_driver_register);
+
+/**
+ * slim_driver_unregister: Undo effect of slim_driver_register
+ * @drv: Client driver to be unregistered
+ */
+void slim_driver_unregister(struct slim_driver *drv)
+{
+   if (drv)


A driver invoking slim_driver_unregister(NULL) is broken, drop this
check and let it oops on the dereference instead.


Yep.




+   driver_unregister(>driver);
+}

Re: [PATCH v7 02/10] arm: dts: sunxi: Restore EMAC changes

[Andrew Lunn]

> index e1dba9ffa94b..f2292deaa590 100644
> --- a/arch/arm/boot/dts/sun8i-h3-bananapi-m2-plus.dts
> +++ b/arch/arm/boot/dts/sun8i-h3-bananapi-m2-plus.dts
> @@ -52,6 +52,7 @@
>   compatible = "sinovoip,bpi-m2-plus", "allwinner,sun8i-h3";
>  
>   aliases {
> + ethernet0 = 
>   serial0 = 
>   serial1 = 
>   };
> @@ -111,6 +112,24 @@
>   status = "okay";
>  };
>  
> + {
> + pinctrl-names = "default";
> + pinctrl-0 = <_rgmii_pins>;
> + phy-supply = <_gmac_3v3>;
> + phy-handle = <_rgmii_phy>;
> + phy-mode = "rgmii";
> +
> + allwinner,leds-active-low;
> + status = "okay";
> +};
> +
> +_mdio {
> + ext_rgmii_phy: ethernet-phy@1 {
> + compatible = "ethernet-phy-ieee802.3-c22";
> + reg = <0>;
> + };

The reg value should match the @ value.

Andrew

Re: [PATCH v7 02/10] arm: dts: sunxi: Restore EMAC changes

[Andrew Lunn]

> index e1dba9ffa94b..f2292deaa590 100644
> --- a/arch/arm/boot/dts/sun8i-h3-bananapi-m2-plus.dts
> +++ b/arch/arm/boot/dts/sun8i-h3-bananapi-m2-plus.dts
> @@ -52,6 +52,7 @@
>   compatible = "sinovoip,bpi-m2-plus", "allwinner,sun8i-h3";
>  
>   aliases {
> + ethernet0 = 
>   serial0 = 
>   serial1 = 
>   };
> @@ -111,6 +112,24 @@
>   status = "okay";
>  };
>  
> + {
> + pinctrl-names = "default";
> + pinctrl-0 = <_rgmii_pins>;
> + phy-supply = <_gmac_3v3>;
> + phy-handle = <_rgmii_phy>;
> + phy-mode = "rgmii";
> +
> + allwinner,leds-active-low;
> + status = "okay";
> +};
> +
> +_mdio {
> + ext_rgmii_phy: ethernet-phy@1 {
> + compatible = "ethernet-phy-ieee802.3-c22";
> + reg = <0>;
> + };

The reg value should match the @ value.

Andrew

Re: rcu kernel-doc issues (4.14-rc1)

[Jonathan Corbet]

On Wed, 18 Oct 2017 09:27:01 -0700
"Paul E. McKenney"  wrote:

> On a related topic...  Is there anything that test-builds docbook prior
> to patches hitting mainline?  My experience indicates that the answer is
> "no".

The zero-day robot is said to be testing for new doc-build errors, but I
haven't actually seen much of that.

jon

Re: rcu kernel-doc issues (4.14-rc1)

[Jonathan Corbet]

On Wed, 18 Oct 2017 09:27:01 -0700
"Paul E. McKenney"  wrote:

> On a related topic...  Is there anything that test-builds docbook prior
> to patches hitting mainline?  My experience indicates that the answer is
> "no".

The zero-day robot is said to be testing for new doc-build errors, but I
haven't actually seen much of that.

jon

Re: [PATCH 0/2] rt5651: Enable platforms with int mic on IN2

[Carlo Caione]

On Wed, Oct 18, 2017 at 5:19 PM, Pierre-Louis Bossart
 wrote:
> On 10/18/17 11:07 AM, Carlo Caione wrote:
>>
>> From: Carlo Caione 
>>
>> While working on enabling a cherry-trail laptop shipping the rt5651
>> codec I realized that the machine driver needed some fixup.
>>
>> In particular the laptop I'm working on (KIANO SlimNote 14.2) has the
>> internal mic connected to the IN2 port.
>>
>> All the laptop-specific work with the related quirks will follow.
>
>
> Nice, thanks for the work. You'll have to rebase and resubmit to fix
> conflicts. I provided changes to simplify DMIC quirks (there is a single
> interface, not 2 on this chip) that aren't reflected in your patches and the
> addition of MCLK support. Mark's for-next branch already took the changes
> this morning.

Ups, missed that for a few hours. I'll prepare a new PR.

> Also let me know if you want the UCM files to be updated.

I have a UCM file already that seems to work fine on this laptop. I'll
prepare a PR also for that once all the kernel pieces are in place.

Thank you,

-- 
Carlo Caione  |  +39.340.80.30.096  |  Endless

Re: [PATCH 0/2] rt5651: Enable platforms with int mic on IN2

[Carlo Caione]

On Wed, Oct 18, 2017 at 5:19 PM, Pierre-Louis Bossart
 wrote:
> On 10/18/17 11:07 AM, Carlo Caione wrote:
>>
>> From: Carlo Caione 
>>
>> While working on enabling a cherry-trail laptop shipping the rt5651
>> codec I realized that the machine driver needed some fixup.
>>
>> In particular the laptop I'm working on (KIANO SlimNote 14.2) has the
>> internal mic connected to the IN2 port.
>>
>> All the laptop-specific work with the related quirks will follow.
>
>
> Nice, thanks for the work. You'll have to rebase and resubmit to fix
> conflicts. I provided changes to simplify DMIC quirks (there is a single
> interface, not 2 on this chip) that aren't reflected in your patches and the
> addition of MCLK support. Mark's for-next branch already took the changes
> this morning.

Ups, missed that for a few hours. I'll prepare a new PR.

> Also let me know if you want the UCM files to be updated.

I have a UCM file already that seems to work fine on this laptop. I'll
prepare a PR also for that once all the kernel pieces are in place.

Thank you,

-- 
Carlo Caione  |  +39.340.80.30.096  |  Endless

Re: usb/core: slab-out-of-bounds in usb_get_bos_descriptor

[Andrey Konovalov]

On Wed, Oct 18, 2017 at 5:25 PM, Alan Stern  wrote:
> On Wed, 18 Oct 2017, Andrey Konovalov wrote:
>
>> Hi!
>>
>> I've got the following report while fuzzing the kernel with syzkaller.
>>
>> On commit 3e0cc09a3a2c40ec1ffb6b4e12da86e98feccb11 (4.14-rc5+).
>>
>> Looks like usb_get_bos_descriptor() doesn't check that buffer has
>> enough space for usb_dev_cap_header, which causes out-of-bounds
>> accesses.
>
> Please try the patch below.
>
> Alan Stern

Hi Alan,

This patch fixes the issue.

Thanks!

Tested-by: Andrey Konovalov 

>
>
>
> Index: usb-4.x/drivers/usb/core/config.c
> ===
> --- usb-4.x.orig/drivers/usb/core/config.c
> +++ usb-4.x/drivers/usb/core/config.c
> @@ -952,10 +952,12 @@ int usb_get_bos_descriptor(struct usb_de
> for (i = 0; i < num; i++) {
> buffer += length;
> cap = (struct usb_dev_cap_header *)buffer;
> -   length = cap->bLength;
>
> -   if (total_len < length)
> +   if (total_len < sizeof(*cap) || total_len < cap->bLength) {
> +   dev->bos->desc->bNumDeviceCaps = i;
> break;
> +   }
> +   length = cap->bLength;
> total_len -= length;
>
> if (cap->bDescriptorType != USB_DT_DEVICE_CAPABILITY) {
>

Re: usb/core: slab-out-of-bounds in usb_get_bos_descriptor

[Andrey Konovalov]

On Wed, Oct 18, 2017 at 5:25 PM, Alan Stern  wrote:
> On Wed, 18 Oct 2017, Andrey Konovalov wrote:
>
>> Hi!
>>
>> I've got the following report while fuzzing the kernel with syzkaller.
>>
>> On commit 3e0cc09a3a2c40ec1ffb6b4e12da86e98feccb11 (4.14-rc5+).
>>
>> Looks like usb_get_bos_descriptor() doesn't check that buffer has
>> enough space for usb_dev_cap_header, which causes out-of-bounds
>> accesses.
>
> Please try the patch below.
>
> Alan Stern

Hi Alan,

This patch fixes the issue.

Thanks!

Tested-by: Andrey Konovalov 

>
>
>
> Index: usb-4.x/drivers/usb/core/config.c
> ===
> --- usb-4.x.orig/drivers/usb/core/config.c
> +++ usb-4.x/drivers/usb/core/config.c
> @@ -952,10 +952,12 @@ int usb_get_bos_descriptor(struct usb_de
> for (i = 0; i < num; i++) {
> buffer += length;
> cap = (struct usb_dev_cap_header *)buffer;
> -   length = cap->bLength;
>
> -   if (total_len < length)
> +   if (total_len < sizeof(*cap) || total_len < cap->bLength) {
> +   dev->bos->desc->bNumDeviceCaps = i;
> break;
> +   }
> +   length = cap->bLength;
> total_len -= length;
>
> if (cap->bDescriptorType != USB_DT_DEVICE_CAPABILITY) {
>

[PATCH] gpiolib: clear irq handler and data in one go

[Martin Kaiser]

Replace the two separate calls for clearing the irqchip's chained handler
and its data with a single irq_set_chained_handler_and_data() call.

Signed-off-by: Martin Kaiser 
---
 drivers/gpio/gpiolib.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index eb80dac..809654a 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -1718,8 +1718,8 @@ static void gpiochip_irqchip_remove(struct gpio_chip 
*gpiochip)
acpi_gpiochip_free_interrupts(gpiochip);
 
if (gpiochip->irq_chained_parent) {
-   irq_set_chained_handler(gpiochip->irq_chained_parent, NULL);
-   irq_set_handler_data(gpiochip->irq_chained_parent, NULL);
+   irq_set_chained_handler_and_data(
+   gpiochip->irq_chained_parent, NULL, NULL);
}
 
/* Remove all IRQ mappings and delete the domain */
-- 
2.1.4

[PATCH] gpiolib: clear irq handler and data in one go

[Martin Kaiser]

Replace the two separate calls for clearing the irqchip's chained handler
and its data with a single irq_set_chained_handler_and_data() call.

Signed-off-by: Martin Kaiser 
---
 drivers/gpio/gpiolib.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index eb80dac..809654a 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -1718,8 +1718,8 @@ static void gpiochip_irqchip_remove(struct gpio_chip 
*gpiochip)
acpi_gpiochip_free_interrupts(gpiochip);
 
if (gpiochip->irq_chained_parent) {
-   irq_set_chained_handler(gpiochip->irq_chained_parent, NULL);
-   irq_set_handler_data(gpiochip->irq_chained_parent, NULL);
+   irq_set_chained_handler_and_data(
+   gpiochip->irq_chained_parent, NULL, NULL);
}
 
/* Remove all IRQ mappings and delete the domain */
-- 
2.1.4

Re: [PATCH] checkpatch: Add TP_printk to list of logging functions

[Song Liu]

> On Oct 17, 2017, at 4:29 PM, Joe Perches  wrote:
> 
> So the line length check can be bypassed by its callers.
> 
> Reported-by: Song Liu 
> Signed-off-by: Joe Perches 
> ---
> scripts/checkpatch.pl | 1 +
> 1 file changed, 1 insertion(+)
> 
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 2a8c6c3c1bdb..359c02b0954e 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -454,6 +454,7 @@ our $zero_initializer = 
> qr{(?:(?:0[xX])?0+$Int_type?|NULL|false)\b};
> our $logFunctions = qr{(?x:
>   printk(?:_ratelimited|_once|_deferred_once|_deferred|)|
>   
> (?:[a-z0-9]+_){1,2}(?:printk|emerg|alert|crit|err|warning|warn|notice|info|debug|dbg|vdbg|devel|cont|WARN)(?:_ratelimited|_once|)|
> + TP_printk|
>   WARN(?:_RATELIMIT|_ONCE|)|
>   panic|
>   MODULE_[A-Z_]+|
> -- 
> 2.10.0.rc2.1.g053435c
> 

Tested-by: Song Liu 

Thanks for the fix!

Re: [PATCH] checkpatch: Add TP_printk to list of logging functions

[Song Liu]

> On Oct 17, 2017, at 4:29 PM, Joe Perches  wrote:
> 
> So the line length check can be bypassed by its callers.
> 
> Reported-by: Song Liu 
> Signed-off-by: Joe Perches 
> ---
> scripts/checkpatch.pl | 1 +
> 1 file changed, 1 insertion(+)
> 
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 2a8c6c3c1bdb..359c02b0954e 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -454,6 +454,7 @@ our $zero_initializer = 
> qr{(?:(?:0[xX])?0+$Int_type?|NULL|false)\b};
> our $logFunctions = qr{(?x:
>   printk(?:_ratelimited|_once|_deferred_once|_deferred|)|
>   
> (?:[a-z0-9]+_){1,2}(?:printk|emerg|alert|crit|err|warning|warn|notice|info|debug|dbg|vdbg|devel|cont|WARN)(?:_ratelimited|_once|)|
> + TP_printk|
>   WARN(?:_RATELIMIT|_ONCE|)|
>   panic|
>   MODULE_[A-Z_]+|
> -- 
> 2.10.0.rc2.1.g053435c
> 

Tested-by: Song Liu 

Thanks for the fix!

Re: [PATCH] lib/dynamic_queue_limits.c: relax BUG_ON to WARN_ON in dql_complete()

[Eric Dumazet]

On Wed, 2017-10-18 at 16:45 +0100, Ard Biesheuvel wrote:
> Even though calling dql_completed() with a count that exceeds the
> queued count is a serious error, it still does not justify bringing
> down the entire kernel with a BUG_ON(). So relax it to a WARN_ON()
> instead.
> 
> Signed-off-by: Ard Biesheuvel 
> ---
>  lib/dynamic_queue_limits.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/lib/dynamic_queue_limits.c b/lib/dynamic_queue_limits.c
> index f346715e2255..24ce495d78f3 100644
> --- a/lib/dynamic_queue_limits.c
> +++ b/lib/dynamic_queue_limits.c
> @@ -23,7 +23,7 @@ void dql_completed(struct dql *dql, unsigned int count)
>   num_queued = ACCESS_ONCE(dql->num_queued);
>  
>   /* Can't complete more than what's in queue */
> - BUG_ON(count > num_queued - dql->num_completed);
> + WARN_ON(count > num_queued - dql->num_completed);
>  
>   completed = dql->num_completed + count;
>   limit = dql->limit;

So instead fixing the faulty driver, you'll have strange lockups, and
force your users to reboot anyway, after annoying periods where
"Internet does not work"

These kinds of errors should be found when testing a new device driver
or new kernel.

Have you found the root cause ?

Re: [PATCH] lib/dynamic_queue_limits.c: relax BUG_ON to WARN_ON in dql_complete()

[Eric Dumazet]

On Wed, 2017-10-18 at 16:45 +0100, Ard Biesheuvel wrote:
> Even though calling dql_completed() with a count that exceeds the
> queued count is a serious error, it still does not justify bringing
> down the entire kernel with a BUG_ON(). So relax it to a WARN_ON()
> instead.
> 
> Signed-off-by: Ard Biesheuvel 
> ---
>  lib/dynamic_queue_limits.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/lib/dynamic_queue_limits.c b/lib/dynamic_queue_limits.c
> index f346715e2255..24ce495d78f3 100644
> --- a/lib/dynamic_queue_limits.c
> +++ b/lib/dynamic_queue_limits.c
> @@ -23,7 +23,7 @@ void dql_completed(struct dql *dql, unsigned int count)
>   num_queued = ACCESS_ONCE(dql->num_queued);
>  
>   /* Can't complete more than what's in queue */
> - BUG_ON(count > num_queued - dql->num_completed);
> + WARN_ON(count > num_queued - dql->num_completed);
>  
>   completed = dql->num_completed + count;
>   limit = dql->limit;

So instead fixing the faulty driver, you'll have strange lockups, and
force your users to reboot anyway, after annoying periods where
"Internet does not work"

These kinds of errors should be found when testing a new device driver
or new kernel.

Have you found the root cause ?

[tip:x86/urgent] x86/mm: Remove debug/x86/tlb_defer_switch_to_init_mm

[tip-bot for Andy Lutomirski]

Commit-ID:  7ac7f2c315ef76437f5119df354d334448534fb5
Gitweb: https://git.kernel.org/tip/7ac7f2c315ef76437f5119df354d334448534fb5
Author: Andy Lutomirski 
AuthorDate: Sat, 14 Oct 2017 09:59:51 -0700
Committer:  Ingo Molnar 
CommitDate: Wed, 18 Oct 2017 15:25:02 +0200

x86/mm: Remove debug/x86/tlb_defer_switch_to_init_mm

Borislav thinks that we don't need this knob in a released kernel.
Get rid of it.

Requested-by: Borislav Petkov 
Signed-off-by: Andy Lutomirski 
Cc: Linus Torvalds 
Cc: Peter Zijlstra 
Cc: Thomas Gleixner 
Fixes: b956575bed91 ("x86/mm: Flush more aggressively in lazy TLB mode")
Link: 
http://lkml.kernel.org/r/1fa72431924e81e86c164ff7881bf9240d1f1a6c.1508000261.git.l...@kernel.org
Signed-off-by: Ingo Molnar 
---
 arch/x86/include/asm/tlbflush.h | 20 --
 arch/x86/mm/tlb.c   | 58 -
 2 files changed, 12 insertions(+), 66 deletions(-)

diff --git a/arch/x86/include/asm/tlbflush.h b/arch/x86/include/asm/tlbflush.h
index 0d4a1bb..c4aed0d 100644
--- a/arch/x86/include/asm/tlbflush.h
+++ b/arch/x86/include/asm/tlbflush.h
@@ -82,16 +82,20 @@ static inline u64 inc_mm_tlb_gen(struct mm_struct *mm)
 #define __flush_tlb_single(addr) __native_flush_tlb_single(addr)
 #endif
 
-/*
- * If tlb_use_lazy_mode is true, then we try to avoid switching CR3 to point
- * to init_mm when we switch to a kernel thread (e.g. the idle thread).  If
- * it's false, then we immediately switch CR3 when entering a kernel thread.
- */
-DECLARE_STATIC_KEY_TRUE(__tlb_defer_switch_to_init_mm);
-
 static inline bool tlb_defer_switch_to_init_mm(void)
 {
-   return static_branch_unlikely(&__tlb_defer_switch_to_init_mm);
+   /*
+* If we have PCID, then switching to init_mm is reasonably
+* fast.  If we don't have PCID, then switching to init_mm is
+* quite slow, so we try to defer it in the hopes that we can
+* avoid it entirely.  The latter approach runs the risk of
+* receiving otherwise unnecessary IPIs.
+*
+* This choice is just a heuristic.  The tlb code can handle this
+* function returning true or false regardless of whether we have
+* PCID.
+*/
+   return !static_cpu_has(X86_FEATURE_PCID);
 }
 
 /*
diff --git a/arch/x86/mm/tlb.c b/arch/x86/mm/tlb.c
index 5ee3b59..0f3d0ce 100644
--- a/arch/x86/mm/tlb.c
+++ b/arch/x86/mm/tlb.c
@@ -30,7 +30,6 @@
 
 atomic64_t last_mm_ctx_id = ATOMIC64_INIT(1);
 
-DEFINE_STATIC_KEY_TRUE(__tlb_defer_switch_to_init_mm);
 
 static void choose_new_asid(struct mm_struct *next, u64 next_tlb_gen,
u16 *new_asid, bool *need_flush)
@@ -629,60 +628,3 @@ static int __init 
create_tlb_single_page_flush_ceiling(void)
return 0;
 }
 late_initcall(create_tlb_single_page_flush_ceiling);
-
-static ssize_t tlblazy_read_file(struct file *file, char __user *user_buf,
-size_t count, loff_t *ppos)
-{
-   char buf[2];
-
-   buf[0] = static_branch_likely(&__tlb_defer_switch_to_init_mm)
-   ? '1' : '0';
-   buf[1] = '\n';
-
-   return simple_read_from_buffer(user_buf, count, ppos, buf, 2);
-}
-
-static ssize_t tlblazy_write_file(struct file *file,
-const char __user *user_buf, size_t count, loff_t *ppos)
-{
-   bool val;
-
-   if (kstrtobool_from_user(user_buf, count, ))
-   return -EINVAL;
-
-   if (val)
-   static_branch_enable(&__tlb_defer_switch_to_init_mm);
-   else
-   static_branch_disable(&__tlb_defer_switch_to_init_mm);
-
-   return count;
-}
-
-static const struct file_operations fops_tlblazy = {
-   .read = tlblazy_read_file,
-   .write = tlblazy_write_file,
-   .llseek = default_llseek,
-};
-
-static int __init init_tlblazy(void)
-{
-   if (boot_cpu_has(X86_FEATURE_PCID)) {
-   /*
-* If we have PCID, then switching to init_mm is reasonably
-* fast.  If we don't have PCID, then switching to init_mm is
-* quite slow, so we default to trying to defer it in the
-* hopes that we can avoid it entirely.  The latter approach
-* runs the risk of receiving otherwise unnecessary IPIs.
-*
-* We can't do this in setup_pcid() because static keys
-* haven't been initialized yet, and it would blow up
-* badly.
-*/
-   static_branch_disable(&__tlb_defer_switch_to_init_mm);
-   }
-
-   debugfs_create_file("tlb_defer_switch_to_init_mm", S_IRUSR | S_IWUSR,
-   arch_debugfs_dir, NULL, _tlblazy);
-   return 0;
-}
-late_initcall(init_tlblazy);

[tip:x86/urgent] x86/mm: Remove debug/x86/tlb_defer_switch_to_init_mm

[tip-bot for Andy Lutomirski]

Commit-ID:  7ac7f2c315ef76437f5119df354d334448534fb5
Gitweb: https://git.kernel.org/tip/7ac7f2c315ef76437f5119df354d334448534fb5
Author: Andy Lutomirski 
AuthorDate: Sat, 14 Oct 2017 09:59:51 -0700
Committer:  Ingo Molnar 
CommitDate: Wed, 18 Oct 2017 15:25:02 +0200

x86/mm: Remove debug/x86/tlb_defer_switch_to_init_mm

Borislav thinks that we don't need this knob in a released kernel.
Get rid of it.

Requested-by: Borislav Petkov 
Signed-off-by: Andy Lutomirski 
Cc: Linus Torvalds 
Cc: Peter Zijlstra 
Cc: Thomas Gleixner 
Fixes: b956575bed91 ("x86/mm: Flush more aggressively in lazy TLB mode")
Link: 
http://lkml.kernel.org/r/1fa72431924e81e86c164ff7881bf9240d1f1a6c.1508000261.git.l...@kernel.org
Signed-off-by: Ingo Molnar 
---
 arch/x86/include/asm/tlbflush.h | 20 --
 arch/x86/mm/tlb.c   | 58 -
 2 files changed, 12 insertions(+), 66 deletions(-)

diff --git a/arch/x86/include/asm/tlbflush.h b/arch/x86/include/asm/tlbflush.h
index 0d4a1bb..c4aed0d 100644
--- a/arch/x86/include/asm/tlbflush.h
+++ b/arch/x86/include/asm/tlbflush.h
@@ -82,16 +82,20 @@ static inline u64 inc_mm_tlb_gen(struct mm_struct *mm)
 #define __flush_tlb_single(addr) __native_flush_tlb_single(addr)
 #endif
 
-/*
- * If tlb_use_lazy_mode is true, then we try to avoid switching CR3 to point
- * to init_mm when we switch to a kernel thread (e.g. the idle thread).  If
- * it's false, then we immediately switch CR3 when entering a kernel thread.
- */
-DECLARE_STATIC_KEY_TRUE(__tlb_defer_switch_to_init_mm);
-
 static inline bool tlb_defer_switch_to_init_mm(void)
 {
-   return static_branch_unlikely(&__tlb_defer_switch_to_init_mm);
+   /*
+* If we have PCID, then switching to init_mm is reasonably
+* fast.  If we don't have PCID, then switching to init_mm is
+* quite slow, so we try to defer it in the hopes that we can
+* avoid it entirely.  The latter approach runs the risk of
+* receiving otherwise unnecessary IPIs.
+*
+* This choice is just a heuristic.  The tlb code can handle this
+* function returning true or false regardless of whether we have
+* PCID.
+*/
+   return !static_cpu_has(X86_FEATURE_PCID);
 }
 
 /*
diff --git a/arch/x86/mm/tlb.c b/arch/x86/mm/tlb.c
index 5ee3b59..0f3d0ce 100644
--- a/arch/x86/mm/tlb.c
+++ b/arch/x86/mm/tlb.c
@@ -30,7 +30,6 @@
 
 atomic64_t last_mm_ctx_id = ATOMIC64_INIT(1);
 
-DEFINE_STATIC_KEY_TRUE(__tlb_defer_switch_to_init_mm);
 
 static void choose_new_asid(struct mm_struct *next, u64 next_tlb_gen,
u16 *new_asid, bool *need_flush)
@@ -629,60 +628,3 @@ static int __init 
create_tlb_single_page_flush_ceiling(void)
return 0;
 }
 late_initcall(create_tlb_single_page_flush_ceiling);
-
-static ssize_t tlblazy_read_file(struct file *file, char __user *user_buf,
-size_t count, loff_t *ppos)
-{
-   char buf[2];
-
-   buf[0] = static_branch_likely(&__tlb_defer_switch_to_init_mm)
-   ? '1' : '0';
-   buf[1] = '\n';
-
-   return simple_read_from_buffer(user_buf, count, ppos, buf, 2);
-}
-
-static ssize_t tlblazy_write_file(struct file *file,
-const char __user *user_buf, size_t count, loff_t *ppos)
-{
-   bool val;
-
-   if (kstrtobool_from_user(user_buf, count, ))
-   return -EINVAL;
-
-   if (val)
-   static_branch_enable(&__tlb_defer_switch_to_init_mm);
-   else
-   static_branch_disable(&__tlb_defer_switch_to_init_mm);
-
-   return count;
-}
-
-static const struct file_operations fops_tlblazy = {
-   .read = tlblazy_read_file,
-   .write = tlblazy_write_file,
-   .llseek = default_llseek,
-};
-
-static int __init init_tlblazy(void)
-{
-   if (boot_cpu_has(X86_FEATURE_PCID)) {
-   /*
-* If we have PCID, then switching to init_mm is reasonably
-* fast.  If we don't have PCID, then switching to init_mm is
-* quite slow, so we default to trying to defer it in the
-* hopes that we can avoid it entirely.  The latter approach
-* runs the risk of receiving otherwise unnecessary IPIs.
-*
-* We can't do this in setup_pcid() because static keys
-* haven't been initialized yet, and it would blow up
-* badly.
-*/
-   static_branch_disable(&__tlb_defer_switch_to_init_mm);
-   }
-
-   debugfs_create_file("tlb_defer_switch_to_init_mm", S_IRUSR | S_IWUSR,
-   arch_debugfs_dir, NULL, _tlblazy);
-   return 0;
-}
-late_initcall(init_tlblazy);

Re: [PATCH 0/1] net: ethtool: add SmartNIC reset support

[Andrew Lunn]

On Wed, Oct 18, 2017 at 09:01:35AM -0700, Scott Branden wrote:
> Ethtool provides support for resetting other internal portions of the
> NIC already.  Seems appropriate to use one of the bits for resetting
> the application processor (AP) for SmartNICs.

Hi Scott

Do you also have a management processor on the NIC?

Or is the Application Processor just the Marketing Departments name
for the management processor?

   Andrew

Re: [PATCH 0/1] net: ethtool: add SmartNIC reset support

[Andrew Lunn]

On Wed, Oct 18, 2017 at 09:01:35AM -0700, Scott Branden wrote:
> Ethtool provides support for resetting other internal portions of the
> NIC already.  Seems appropriate to use one of the bits for resetting
> the application processor (AP) for SmartNICs.

Hi Scott

Do you also have a management processor on the NIC?

Or is the Application Processor just the Marketing Departments name
for the management processor?

   Andrew

[tip:x86/urgent] x86/mm: Tidy up "x86/mm: Flush more aggressively in lazy TLB mode"

[tip-bot for Andy Lutomirski]

Commit-ID:  4e57b94664fef55aa71cac33b4632fdfdd52b695
Gitweb: https://git.kernel.org/tip/4e57b94664fef55aa71cac33b4632fdfdd52b695
Author: Andy Lutomirski 
AuthorDate: Sat, 14 Oct 2017 09:59:50 -0700
Committer:  Ingo Molnar 
CommitDate: Wed, 18 Oct 2017 15:25:02 +0200

x86/mm: Tidy up "x86/mm: Flush more aggressively in lazy TLB mode"

Due to timezones, commit:

  b956575bed91 ("x86/mm: Flush more aggressively in lazy TLB mode")

was an outdated patch that well tested and fixed the bug but didn't
address Borislav's review comments.

Tidy it up:

 - The name "tlb_use_lazy_mode()" was highly confusing.  Change it to
   "tlb_defer_switch_to_init_mm()", which describes what it actually
   means.

 - Move the static_branch crap into a helper.

 - Improve comments.

Actually removing the debugfs option is in the next patch.

Reported-by: Borislav Petkov 
Signed-off-by: Andy Lutomirski 
Cc: Linus Torvalds 
Cc: Peter Zijlstra 
Cc: Thomas Gleixner 
Fixes: b956575bed91 ("x86/mm: Flush more aggressively in lazy TLB mode")
Link: 
http://lkml.kernel.org/r/154ef95428d4592596b6e98b0af1d2747d6cfbf8.1508000261.git.l...@kernel.org
Signed-off-by: Ingo Molnar 
---
 arch/x86/include/asm/tlbflush.h |  7 ++-
 arch/x86/mm/tlb.c   | 30 ++
 2 files changed, 24 insertions(+), 13 deletions(-)

diff --git a/arch/x86/include/asm/tlbflush.h b/arch/x86/include/asm/tlbflush.h
index d362161..0d4a1bb 100644
--- a/arch/x86/include/asm/tlbflush.h
+++ b/arch/x86/include/asm/tlbflush.h
@@ -87,7 +87,12 @@ static inline u64 inc_mm_tlb_gen(struct mm_struct *mm)
  * to init_mm when we switch to a kernel thread (e.g. the idle thread).  If
  * it's false, then we immediately switch CR3 when entering a kernel thread.
  */
-DECLARE_STATIC_KEY_TRUE(tlb_use_lazy_mode);
+DECLARE_STATIC_KEY_TRUE(__tlb_defer_switch_to_init_mm);
+
+static inline bool tlb_defer_switch_to_init_mm(void)
+{
+   return static_branch_unlikely(&__tlb_defer_switch_to_init_mm);
+}
 
 /*
  * 6 because 6 should be plenty and struct tlb_state will fit in
diff --git a/arch/x86/mm/tlb.c b/arch/x86/mm/tlb.c
index 7db23f9..5ee3b59 100644
--- a/arch/x86/mm/tlb.c
+++ b/arch/x86/mm/tlb.c
@@ -30,7 +30,7 @@
 
 atomic64_t last_mm_ctx_id = ATOMIC64_INIT(1);
 
-DEFINE_STATIC_KEY_TRUE(tlb_use_lazy_mode);
+DEFINE_STATIC_KEY_TRUE(__tlb_defer_switch_to_init_mm);
 
 static void choose_new_asid(struct mm_struct *next, u64 next_tlb_gen,
u16 *new_asid, bool *need_flush)
@@ -213,6 +213,9 @@ void switch_mm_irqs_off(struct mm_struct *prev, struct 
mm_struct *next,
 }
 
 /*
+ * Please ignore the name of this function.  It should be called
+ * switch_to_kernel_thread().
+ *
  * enter_lazy_tlb() is a hint from the scheduler that we are entering a
  * kernel thread or other context without an mm.  Acceptable implementations
  * include doing nothing whatsoever, switching to init_mm, or various clever
@@ -227,7 +230,7 @@ void enter_lazy_tlb(struct mm_struct *mm, struct 
task_struct *tsk)
if (this_cpu_read(cpu_tlbstate.loaded_mm) == _mm)
return;
 
-   if (static_branch_unlikely(_use_lazy_mode)) {
+   if (tlb_defer_switch_to_init_mm()) {
/*
 * There's a significant optimization that may be possible
 * here.  We have accurate enough TLB flush tracking that we
@@ -632,7 +635,8 @@ static ssize_t tlblazy_read_file(struct file *file, char 
__user *user_buf,
 {
char buf[2];
 
-   buf[0] = static_branch_likely(_use_lazy_mode) ? '1' : '0';
+   buf[0] = static_branch_likely(&__tlb_defer_switch_to_init_mm)
+   ? '1' : '0';
buf[1] = '\n';
 
return simple_read_from_buffer(user_buf, count, ppos, buf, 2);
@@ -647,9 +651,9 @@ static ssize_t tlblazy_write_file(struct file *file,
return -EINVAL;
 
if (val)
-   static_branch_enable(_use_lazy_mode);
+   static_branch_enable(&__tlb_defer_switch_to_init_mm);
else
-   static_branch_disable(_use_lazy_mode);
+   static_branch_disable(&__tlb_defer_switch_to_init_mm);
 
return count;
 }
@@ -660,23 +664,25 @@ static const struct file_operations fops_tlblazy = {
.llseek = default_llseek,
 };
 
-static int __init init_tlb_use_lazy_mode(void)
+static int __init init_tlblazy(void)
 {
if (boot_cpu_has(X86_FEATURE_PCID)) {
/*
-* Heuristic: with PCID on, switching to and from
-* init_mm is reasonably fast, but remote flush IPIs
-* as expensive as ever, so turn off lazy TLB mode.
+* If we have PCID, then switching to init_mm is reasonably
+* fast.  If we don't have PCID, then switching to init_mm is
+* quite slow, so we default 

[tip:x86/urgent] x86/mm: Tidy up "x86/mm: Flush more aggressively in lazy TLB mode"

[tip-bot for Andy Lutomirski]

Commit-ID:  4e57b94664fef55aa71cac33b4632fdfdd52b695
Gitweb: https://git.kernel.org/tip/4e57b94664fef55aa71cac33b4632fdfdd52b695
Author: Andy Lutomirski 
AuthorDate: Sat, 14 Oct 2017 09:59:50 -0700
Committer:  Ingo Molnar 
CommitDate: Wed, 18 Oct 2017 15:25:02 +0200

x86/mm: Tidy up "x86/mm: Flush more aggressively in lazy TLB mode"

Due to timezones, commit:

  b956575bed91 ("x86/mm: Flush more aggressively in lazy TLB mode")

was an outdated patch that well tested and fixed the bug but didn't
address Borislav's review comments.

Tidy it up:

 - The name "tlb_use_lazy_mode()" was highly confusing.  Change it to
   "tlb_defer_switch_to_init_mm()", which describes what it actually
   means.

 - Move the static_branch crap into a helper.

 - Improve comments.

Actually removing the debugfs option is in the next patch.

Reported-by: Borislav Petkov 
Signed-off-by: Andy Lutomirski 
Cc: Linus Torvalds 
Cc: Peter Zijlstra 
Cc: Thomas Gleixner 
Fixes: b956575bed91 ("x86/mm: Flush more aggressively in lazy TLB mode")
Link: 
http://lkml.kernel.org/r/154ef95428d4592596b6e98b0af1d2747d6cfbf8.1508000261.git.l...@kernel.org
Signed-off-by: Ingo Molnar 
---
 arch/x86/include/asm/tlbflush.h |  7 ++-
 arch/x86/mm/tlb.c   | 30 ++
 2 files changed, 24 insertions(+), 13 deletions(-)

diff --git a/arch/x86/include/asm/tlbflush.h b/arch/x86/include/asm/tlbflush.h
index d362161..0d4a1bb 100644
--- a/arch/x86/include/asm/tlbflush.h
+++ b/arch/x86/include/asm/tlbflush.h
@@ -87,7 +87,12 @@ static inline u64 inc_mm_tlb_gen(struct mm_struct *mm)
  * to init_mm when we switch to a kernel thread (e.g. the idle thread).  If
  * it's false, then we immediately switch CR3 when entering a kernel thread.
  */
-DECLARE_STATIC_KEY_TRUE(tlb_use_lazy_mode);
+DECLARE_STATIC_KEY_TRUE(__tlb_defer_switch_to_init_mm);
+
+static inline bool tlb_defer_switch_to_init_mm(void)
+{
+   return static_branch_unlikely(&__tlb_defer_switch_to_init_mm);
+}
 
 /*
  * 6 because 6 should be plenty and struct tlb_state will fit in
diff --git a/arch/x86/mm/tlb.c b/arch/x86/mm/tlb.c
index 7db23f9..5ee3b59 100644
--- a/arch/x86/mm/tlb.c
+++ b/arch/x86/mm/tlb.c
@@ -30,7 +30,7 @@
 
 atomic64_t last_mm_ctx_id = ATOMIC64_INIT(1);
 
-DEFINE_STATIC_KEY_TRUE(tlb_use_lazy_mode);
+DEFINE_STATIC_KEY_TRUE(__tlb_defer_switch_to_init_mm);
 
 static void choose_new_asid(struct mm_struct *next, u64 next_tlb_gen,
u16 *new_asid, bool *need_flush)
@@ -213,6 +213,9 @@ void switch_mm_irqs_off(struct mm_struct *prev, struct 
mm_struct *next,
 }
 
 /*
+ * Please ignore the name of this function.  It should be called
+ * switch_to_kernel_thread().
+ *
  * enter_lazy_tlb() is a hint from the scheduler that we are entering a
  * kernel thread or other context without an mm.  Acceptable implementations
  * include doing nothing whatsoever, switching to init_mm, or various clever
@@ -227,7 +230,7 @@ void enter_lazy_tlb(struct mm_struct *mm, struct 
task_struct *tsk)
if (this_cpu_read(cpu_tlbstate.loaded_mm) == _mm)
return;
 
-   if (static_branch_unlikely(_use_lazy_mode)) {
+   if (tlb_defer_switch_to_init_mm()) {
/*
 * There's a significant optimization that may be possible
 * here.  We have accurate enough TLB flush tracking that we
@@ -632,7 +635,8 @@ static ssize_t tlblazy_read_file(struct file *file, char 
__user *user_buf,
 {
char buf[2];
 
-   buf[0] = static_branch_likely(_use_lazy_mode) ? '1' : '0';
+   buf[0] = static_branch_likely(&__tlb_defer_switch_to_init_mm)
+   ? '1' : '0';
buf[1] = '\n';
 
return simple_read_from_buffer(user_buf, count, ppos, buf, 2);
@@ -647,9 +651,9 @@ static ssize_t tlblazy_write_file(struct file *file,
return -EINVAL;
 
if (val)
-   static_branch_enable(_use_lazy_mode);
+   static_branch_enable(&__tlb_defer_switch_to_init_mm);
else
-   static_branch_disable(_use_lazy_mode);
+   static_branch_disable(&__tlb_defer_switch_to_init_mm);
 
return count;
 }
@@ -660,23 +664,25 @@ static const struct file_operations fops_tlblazy = {
.llseek = default_llseek,
 };
 
-static int __init init_tlb_use_lazy_mode(void)
+static int __init init_tlblazy(void)
 {
if (boot_cpu_has(X86_FEATURE_PCID)) {
/*
-* Heuristic: with PCID on, switching to and from
-* init_mm is reasonably fast, but remote flush IPIs
-* as expensive as ever, so turn off lazy TLB mode.
+* If we have PCID, then switching to init_mm is reasonably
+* fast.  If we don't have PCID, then switching to init_mm is
+* quite slow, so we default to trying to defer it in the
+* hopes that we can avoid it entirely.  The latter approach
+* runs the risk of receiving 

Re: [PATCH] KVM: X86: #GP when guest attempts to write MCi banks w/o 0

[Jim Mattson]

The AMD APM says, "For each error-reporting register bank, software
should set the enable bits to 1 in the MCi_CTL register for the error
types it wants the processor to report. Software can write each
MCi_CTL with all 1s to enable all error-reporting mechanisms.' It does
not say that only all 1's or all 0's are allowed, and it implies that
any value is allowed.

Since this is a vendor-agnostic function, the Intel-specific
constraints should only be applied when virtualizing Intel CPUs (in
particular, Intel P6 family CPUs). The same comment applies to the
existing constraints from commit 890ca9aefa78 ("KVM: Add MCE
support"), which were only partially relaxed by commit 114be429c8cd4
("KVM: allow bit 10 to be cleared in MSR_IA32_MC4_CTL").

On Wed, Oct 18, 2017 at 2:49 AM, Wanpeng Li  wrote:
> From: Wanpeng Li 
>
> SDM section 15.3.2.2~15.3.2.4 mentioned that MCi_STATUS/ADDR/MISC, when the
> registers are implemented, these registers can be cleared by explicitly 
> writing
> 0s to these registers. Writing 1s to these registers will cause a
> general-protection exception.
>
> The mce is emulated in qemu, so just the guest attempts to write 1 to these
> registers should cause a #GP, this patch does it.
>
> Cc: Radim Krčmář 
> Cc: Jim Mattson 
> Signed-off-by: Wanpeng Li 
> ---
>  arch/x86/kvm/x86.c | 9 +++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 5669af0..a8680ea 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -2006,10 +2006,12 @@ static void kvmclock_sync_fn(struct work_struct *work)
> KVMCLOCK_SYNC_PERIOD);
>  }
>
> -static int set_msr_mce(struct kvm_vcpu *vcpu, u32 msr, u64 data)
> +static int set_msr_mce(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
>  {
> u64 mcg_cap = vcpu->arch.mcg_cap;
> unsigned bank_num = mcg_cap & 0xff;
> +   u32 msr = msr_info->index;
> +   u64 data = msr_info->data;
>
> switch (msr) {
> case MSR_IA32_MCG_STATUS:
> @@ -2034,6 +2036,9 @@ static int set_msr_mce(struct kvm_vcpu *vcpu, u32 msr, 
> u64 data)
> if ((offset & 0x3) == 0 &&
> data != 0 && (data | (1 << 10)) != ~(u64)0)
> return -1;
> +   if (!msr_info->host_initiated &&
> +   (offset & 0x3) != 0 && data != 0)
> +   return -1;
> vcpu->arch.mce_banks[offset] = data;
> break;
> }
> @@ -2283,7 +2288,7 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct 
> msr_data *msr_info)
> case MSR_IA32_MCG_CTL:
> case MSR_IA32_MCG_STATUS:
> case MSR_IA32_MC0_CTL ... MSR_IA32_MCx_CTL(KVM_MAX_MCE_BANKS) - 1:
> -   return set_msr_mce(vcpu, msr, data);
> +   return set_msr_mce(vcpu, msr_info);
>
> case MSR_K7_PERFCTR0 ... MSR_K7_PERFCTR3:
> case MSR_P6_PERFCTR0 ... MSR_P6_PERFCTR1:
> --
> 2.7.4
>

Re: [PATCH] KVM: X86: #GP when guest attempts to write MCi banks w/o 0

[Jim Mattson]

The AMD APM says, "For each error-reporting register bank, software
should set the enable bits to 1 in the MCi_CTL register for the error
types it wants the processor to report. Software can write each
MCi_CTL with all 1s to enable all error-reporting mechanisms.' It does
not say that only all 1's or all 0's are allowed, and it implies that
any value is allowed.

Since this is a vendor-agnostic function, the Intel-specific
constraints should only be applied when virtualizing Intel CPUs (in
particular, Intel P6 family CPUs). The same comment applies to the
existing constraints from commit 890ca9aefa78 ("KVM: Add MCE
support"), which were only partially relaxed by commit 114be429c8cd4
("KVM: allow bit 10 to be cleared in MSR_IA32_MC4_CTL").

On Wed, Oct 18, 2017 at 2:49 AM, Wanpeng Li  wrote:
> From: Wanpeng Li 
>
> SDM section 15.3.2.2~15.3.2.4 mentioned that MCi_STATUS/ADDR/MISC, when the
> registers are implemented, these registers can be cleared by explicitly 
> writing
> 0s to these registers. Writing 1s to these registers will cause a
> general-protection exception.
>
> The mce is emulated in qemu, so just the guest attempts to write 1 to these
> registers should cause a #GP, this patch does it.
>
> Cc: Radim Krčmář 
> Cc: Jim Mattson 
> Signed-off-by: Wanpeng Li 
> ---
>  arch/x86/kvm/x86.c | 9 +++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 5669af0..a8680ea 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -2006,10 +2006,12 @@ static void kvmclock_sync_fn(struct work_struct *work)
> KVMCLOCK_SYNC_PERIOD);
>  }
>
> -static int set_msr_mce(struct kvm_vcpu *vcpu, u32 msr, u64 data)
> +static int set_msr_mce(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
>  {
> u64 mcg_cap = vcpu->arch.mcg_cap;
> unsigned bank_num = mcg_cap & 0xff;
> +   u32 msr = msr_info->index;
> +   u64 data = msr_info->data;
>
> switch (msr) {
> case MSR_IA32_MCG_STATUS:
> @@ -2034,6 +2036,9 @@ static int set_msr_mce(struct kvm_vcpu *vcpu, u32 msr, 
> u64 data)
> if ((offset & 0x3) == 0 &&
> data != 0 && (data | (1 << 10)) != ~(u64)0)
> return -1;
> +   if (!msr_info->host_initiated &&
> +   (offset & 0x3) != 0 && data != 0)
> +   return -1;
> vcpu->arch.mce_banks[offset] = data;
> break;
> }
> @@ -2283,7 +2288,7 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct 
> msr_data *msr_info)
> case MSR_IA32_MCG_CTL:
> case MSR_IA32_MCG_STATUS:
> case MSR_IA32_MC0_CTL ... MSR_IA32_MCx_CTL(KVM_MAX_MCE_BANKS) - 1:
> -   return set_msr_mce(vcpu, msr, data);
> +   return set_msr_mce(vcpu, msr_info);
>
> case MSR_K7_PERFCTR0 ... MSR_K7_PERFCTR3:
> case MSR_P6_PERFCTR0 ... MSR_P6_PERFCTR1:
> --
> 2.7.4
>

[tip:core/objtool] objtool: Print top level commands on incorrect usage

[tip-bot for Kamalesh Babulal]

Commit-ID:  6a93bb7e4a7d6670677d5b0eb980936eb9cc5d2e
Gitweb: https://git.kernel.org/tip/6a93bb7e4a7d6670677d5b0eb980936eb9cc5d2e
Author: Kamalesh Babulal 
AuthorDate: Sat, 14 Oct 2017 20:17:54 +0530
Committer:  Ingo Molnar 
CommitDate: Wed, 18 Oct 2017 15:22:26 +0200

objtool: Print top level commands on incorrect usage

Print top-level objtool commands, along with the error on incorrect
command line usage. Objtool command line parser exit's with code 129,
for incorrect usage. Convert the cmd_usage() exit code also, to maintain
consistency across objtool.

After the patch:

  $ ./objtool -j

  Unknown option: -j

  usage: objtool COMMAND [ARGS]

  Commands:
 check   Perform stack metadata validation on an object file
 orc Generate in-place ORC unwind tables for an object file

  $ echo $?
  129

Signed-off-by: Kamalesh Babulal 
Acked-by: Josh Poimboeuf 
Cc: Linus Torvalds 
Cc: Peter Zijlstra 
Cc: Thomas Gleixner 
Link: 
http://lkml.kernel.org/r/1507992474-16142-1-git-send-email-kamal...@linux.vnet.ibm.com
Signed-off-by: Ingo Molnar 
---
 tools/objtool/objtool.c | 6 ++
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/tools/objtool/objtool.c b/tools/objtool/objtool.c
index 31e0f91..07f3299 100644
--- a/tools/objtool/objtool.c
+++ b/tools/objtool/objtool.c
@@ -70,7 +70,7 @@ static void cmd_usage(void)
 
printf("\n");
 
-   exit(1);
+   exit(129);
 }
 
 static void handle_options(int *argc, const char ***argv)
@@ -86,9 +86,7 @@ static void handle_options(int *argc, const char ***argv)
break;
} else {
fprintf(stderr, "Unknown option: %s\n", cmd);
-   fprintf(stderr, "\n Usage: %s\n",
-   objtool_usage_string);
-   exit(1);
+   cmd_usage();
}
 
(*argv)++;

[tip:core/objtool] objtool: Print top level commands on incorrect usage

[tip-bot for Kamalesh Babulal]

Commit-ID:  6a93bb7e4a7d6670677d5b0eb980936eb9cc5d2e
Gitweb: https://git.kernel.org/tip/6a93bb7e4a7d6670677d5b0eb980936eb9cc5d2e
Author: Kamalesh Babulal 
AuthorDate: Sat, 14 Oct 2017 20:17:54 +0530
Committer:  Ingo Molnar 
CommitDate: Wed, 18 Oct 2017 15:22:26 +0200

objtool: Print top level commands on incorrect usage

Print top-level objtool commands, along with the error on incorrect
command line usage. Objtool command line parser exit's with code 129,
for incorrect usage. Convert the cmd_usage() exit code also, to maintain
consistency across objtool.

After the patch:

  $ ./objtool -j

  Unknown option: -j

  usage: objtool COMMAND [ARGS]

  Commands:
 check   Perform stack metadata validation on an object file
 orc Generate in-place ORC unwind tables for an object file

  $ echo $?
  129

Signed-off-by: Kamalesh Babulal 
Acked-by: Josh Poimboeuf 
Cc: Linus Torvalds 
Cc: Peter Zijlstra 
Cc: Thomas Gleixner 
Link: 
http://lkml.kernel.org/r/1507992474-16142-1-git-send-email-kamal...@linux.vnet.ibm.com
Signed-off-by: Ingo Molnar 
---
 tools/objtool/objtool.c | 6 ++
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/tools/objtool/objtool.c b/tools/objtool/objtool.c
index 31e0f91..07f3299 100644
--- a/tools/objtool/objtool.c
+++ b/tools/objtool/objtool.c
@@ -70,7 +70,7 @@ static void cmd_usage(void)
 
printf("\n");
 
-   exit(1);
+   exit(129);
 }
 
 static void handle_options(int *argc, const char ***argv)
@@ -86,9 +86,7 @@ static void handle_options(int *argc, const char ***argv)
break;
} else {
fprintf(stderr, "Unknown option: %s\n", cmd);
-   fprintf(stderr, "\n Usage: %s\n",
-   objtool_usage_string);
-   exit(1);
+   cmd_usage();
}
 
(*argv)++;

[tip:x86/urgent] x86/mm/64: Remove the last VM_BUG_ON() from the TLB code

[tip-bot for Andy Lutomirski]

Commit-ID:  e8b9b0cc8269c85d8167aaee024bfcbb4976c031
Gitweb: https://git.kernel.org/tip/e8b9b0cc8269c85d8167aaee024bfcbb4976c031
Author: Andy Lutomirski 
AuthorDate: Sat, 14 Oct 2017 09:59:49 -0700
Committer:  Ingo Molnar 
CommitDate: Wed, 18 Oct 2017 15:25:02 +0200

x86/mm/64: Remove the last VM_BUG_ON() from the TLB code

Let's avoid hard-to-diagnose crashes in the future.

Signed-off-by: Andy Lutomirski 
Cc: Borislav Petkov 
Cc: Linus Torvalds 
Cc: Peter Zijlstra 
Cc: Thomas Gleixner 
Link: 
http://lkml.kernel.org/r/f423bbc97864089fbdeb813f1ea126c6eaed844a.1508000261.git.l...@kernel.org
Signed-off-by: Ingo Molnar 
---
 arch/x86/mm/tlb.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/x86/mm/tlb.c b/arch/x86/mm/tlb.c
index 658bf00..7db23f9 100644
--- a/arch/x86/mm/tlb.c
+++ b/arch/x86/mm/tlb.c
@@ -147,8 +147,8 @@ void switch_mm_irqs_off(struct mm_struct *prev, struct 
mm_struct *next,
this_cpu_write(cpu_tlbstate.is_lazy, false);
 
if (real_prev == next) {
-   VM_BUG_ON(this_cpu_read(cpu_tlbstate.ctxs[prev_asid].ctx_id) !=
- next->context.ctx_id);
+   VM_WARN_ON(this_cpu_read(cpu_tlbstate.ctxs[prev_asid].ctx_id) !=
+  next->context.ctx_id);
 
/*
 * We don't currently support having a real mm loaded without

[tip:x86/urgent] x86/mm/64: Remove the last VM_BUG_ON() from the TLB code

[tip-bot for Andy Lutomirski]

Commit-ID:  e8b9b0cc8269c85d8167aaee024bfcbb4976c031
Gitweb: https://git.kernel.org/tip/e8b9b0cc8269c85d8167aaee024bfcbb4976c031
Author: Andy Lutomirski 
AuthorDate: Sat, 14 Oct 2017 09:59:49 -0700
Committer:  Ingo Molnar 
CommitDate: Wed, 18 Oct 2017 15:25:02 +0200

x86/mm/64: Remove the last VM_BUG_ON() from the TLB code

Let's avoid hard-to-diagnose crashes in the future.

Signed-off-by: Andy Lutomirski 
Cc: Borislav Petkov 
Cc: Linus Torvalds 
Cc: Peter Zijlstra 
Cc: Thomas Gleixner 
Link: 
http://lkml.kernel.org/r/f423bbc97864089fbdeb813f1ea126c6eaed844a.1508000261.git.l...@kernel.org
Signed-off-by: Ingo Molnar 
---
 arch/x86/mm/tlb.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/x86/mm/tlb.c b/arch/x86/mm/tlb.c
index 658bf00..7db23f9 100644
--- a/arch/x86/mm/tlb.c
+++ b/arch/x86/mm/tlb.c
@@ -147,8 +147,8 @@ void switch_mm_irqs_off(struct mm_struct *prev, struct 
mm_struct *next,
this_cpu_write(cpu_tlbstate.is_lazy, false);
 
if (real_prev == next) {
-   VM_BUG_ON(this_cpu_read(cpu_tlbstate.ctxs[prev_asid].ctx_id) !=
- next->context.ctx_id);
+   VM_WARN_ON(this_cpu_read(cpu_tlbstate.ctxs[prev_asid].ctx_id) !=
+  next->context.ctx_id);
 
/*
 * We don't currently support having a real mm loaded without

[tip:perf/core] Revert "kprobes: Warn if optprobe handler tries to change execution path"

[tip-bot for Naveen N. Rao]

Commit-ID:  4f3a871443669c6b4d458a60ac8d8ca5eedc3f97
Gitweb: https://git.kernel.org/tip/4f3a871443669c6b4d458a60ac8d8ca5eedc3f97
Author: Naveen N. Rao 
AuthorDate: Tue, 17 Oct 2017 13:48:34 +0530
Committer:  Ingo Molnar 
CommitDate: Wed, 18 Oct 2017 15:21:35 +0200

Revert "kprobes: Warn if optprobe handler tries to change execution path"

This reverts commit:

  e863d539614641 ("kprobes: Warn if optprobe handler tries to change execution 
path")

On PowerPC, we place a probe at kretprobe_trampoline to catch function
returns and with CONFIG_OPTPROBES=y, this probe gets optimized. This
works for us due to the way we handle the optprobe as described in
commit:

  762df10bad6954 ("powerpc/kprobes: Optimize kprobe in kretprobe_trampoline()")

With the above commit, we end up with a warning. As such, revert this change.

Reported-by: Michael Ellerman 
Signed-off-by: Naveen N. Rao 
Cc: Ananth N Mavinakayanahalli 
Cc: Linus Torvalds 
Cc: Masami Hiramatsu 
Cc: Peter Zijlstra 
Cc: Thomas Gleixner 
Link: 
http://lkml.kernel.org/r/20171017081834.3629-1-naveen.n@linux.vnet.ibm.com
Signed-off-by: Ingo Molnar 
---
 kernel/kprobes.c | 5 +
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/kernel/kprobes.c b/kernel/kprobes.c
index 2d28377..15fba7f 100644
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -387,10 +387,7 @@ void opt_pre_handler(struct kprobe *p, struct pt_regs 
*regs)
list_for_each_entry_rcu(kp, >list, list) {
if (kp->pre_handler && likely(!kprobe_disabled(kp))) {
set_kprobe_instance(kp);
-   if (kp->pre_handler(kp, regs)) {
-   if (WARN_ON_ONCE(1))
-   pr_err("Optprobe ignores instruction 
pointer changing.(%pF)\n", p->addr);
-   }
+   kp->pre_handler(kp, regs);
}
reset_kprobe_instance();
}

[tip:perf/core] Revert "kprobes: Warn if optprobe handler tries to change execution path"

[tip-bot for Naveen N. Rao]

Commit-ID:  4f3a871443669c6b4d458a60ac8d8ca5eedc3f97
Gitweb: https://git.kernel.org/tip/4f3a871443669c6b4d458a60ac8d8ca5eedc3f97
Author: Naveen N. Rao 
AuthorDate: Tue, 17 Oct 2017 13:48:34 +0530
Committer:  Ingo Molnar 
CommitDate: Wed, 18 Oct 2017 15:21:35 +0200

Revert "kprobes: Warn if optprobe handler tries to change execution path"

This reverts commit:

  e863d539614641 ("kprobes: Warn if optprobe handler tries to change execution 
path")

On PowerPC, we place a probe at kretprobe_trampoline to catch function
returns and with CONFIG_OPTPROBES=y, this probe gets optimized. This
works for us due to the way we handle the optprobe as described in
commit:

  762df10bad6954 ("powerpc/kprobes: Optimize kprobe in kretprobe_trampoline()")

With the above commit, we end up with a warning. As such, revert this change.

Reported-by: Michael Ellerman 
Signed-off-by: Naveen N. Rao 
Cc: Ananth N Mavinakayanahalli 
Cc: Linus Torvalds 
Cc: Masami Hiramatsu 
Cc: Peter Zijlstra 
Cc: Thomas Gleixner 
Link: 
http://lkml.kernel.org/r/20171017081834.3629-1-naveen.n@linux.vnet.ibm.com
Signed-off-by: Ingo Molnar 
---
 kernel/kprobes.c | 5 +
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/kernel/kprobes.c b/kernel/kprobes.c
index 2d28377..15fba7f 100644
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -387,10 +387,7 @@ void opt_pre_handler(struct kprobe *p, struct pt_regs 
*regs)
list_for_each_entry_rcu(kp, >list, list) {
if (kp->pre_handler && likely(!kprobe_disabled(kp))) {
set_kprobe_instance(kp);
-   if (kp->pre_handler(kp, regs)) {
-   if (WARN_ON_ONCE(1))
-   pr_err("Optprobe ignores instruction 
pointer changing.(%pF)\n", p->addr);
-   }
+   kp->pre_handler(kp, regs);
}
reset_kprobe_instance();
}

Re: rcu kernel-doc issues (4.14-rc1)

[Paul E. McKenney]

On Wed, Oct 18, 2017 at 10:03:40AM -0600, Jonathan Corbet wrote:
> On Mon, 16 Oct 2017 13:26:19 -0700
> "Paul E. McKenney"  wrote:
> 
> > OK, how about if I submit them to the 4.15 merge window, but add the
> > appropriate -stable tags to get them backported?  Yes, these are bugs,
> > but I cannot in good conscience claim that they are v4.14 regressions.
> > 
> > But if Jon agrees with you, I will of course create a patch series,
> > pull request, or whatever and send it along to him.
> 
> [Sorry for being slow ... $EXCUSES ... ]
> 
> One could argue that they are indeed a regression; they introduced a bunch
> of build errors into 4.14.  I think it would be better to see it fixed if
> possible.  I don't care that much about how it gets there; Paul, I can
> send it Linusward if you don't want to.

I never have pushed directly to Linus, so I might as well get started
doing so.

On a related topic...  Is there anything that test-builds docbook prior
to patches hitting mainline?  My experience indicates that the answer is
"no".

Thanx, Paul

Re: rcu kernel-doc issues (4.14-rc1)

[Paul E. McKenney]

On Wed, Oct 18, 2017 at 10:03:40AM -0600, Jonathan Corbet wrote:
> On Mon, 16 Oct 2017 13:26:19 -0700
> "Paul E. McKenney"  wrote:
> 
> > OK, how about if I submit them to the 4.15 merge window, but add the
> > appropriate -stable tags to get them backported?  Yes, these are bugs,
> > but I cannot in good conscience claim that they are v4.14 regressions.
> > 
> > But if Jon agrees with you, I will of course create a patch series,
> > pull request, or whatever and send it along to him.
> 
> [Sorry for being slow ... $EXCUSES ... ]
> 
> One could argue that they are indeed a regression; they introduced a bunch
> of build errors into 4.14.  I think it would be better to see it fixed if
> possible.  I don't care that much about how it gets there; Paul, I can
> send it Linusward if you don't want to.

I never have pushed directly to Linus, so I might as well get started
doing so.

On a related topic...  Is there anything that test-builds docbook prior
to patches hitting mainline?  My experience indicates that the answer is
"no".

Thanx, Paul

[tip:x86/urgent] x86/microcode/intel: Disable late loading on model 79

[tip-bot for Borislav Petkov]

Commit-ID:  723f2828a98c8ca19842042f418fb30dd8cfc0f7
Gitweb: https://git.kernel.org/tip/723f2828a98c8ca19842042f418fb30dd8cfc0f7
Author: Borislav Petkov 
AuthorDate: Wed, 18 Oct 2017 13:12:25 +0200
Committer:  Ingo Molnar 
CommitDate: Wed, 18 Oct 2017 15:20:20 +0200

x86/microcode/intel: Disable late loading on model 79

Blacklist Broadwell X model 79 for late loading due to an erratum.

Signed-off-by: Borislav Petkov 
Acked-by: Tony Luck 
Cc: 
Cc: Linus Torvalds 
Cc: Peter Zijlstra 
Cc: Thomas Gleixner 
Link: http://lkml.kernel.org/r/20171018111225.25635-1...@alien8.de
Signed-off-by: Ingo Molnar 
---
 arch/x86/kernel/cpu/microcode/intel.c | 19 +++
 1 file changed, 19 insertions(+)

diff --git a/arch/x86/kernel/cpu/microcode/intel.c 
b/arch/x86/kernel/cpu/microcode/intel.c
index 8f7a9bb..7dbcb7a 100644
--- a/arch/x86/kernel/cpu/microcode/intel.c
+++ b/arch/x86/kernel/cpu/microcode/intel.c
@@ -34,6 +34,7 @@
 #include 
 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -918,6 +919,18 @@ static int get_ucode_fw(void *to, const void *from, size_t 
n)
return 0;
 }
 
+static bool is_blacklisted(unsigned int cpu)
+{
+   struct cpuinfo_x86 *c = _data(cpu);
+
+   if (c->x86 == 6 && c->x86_model == INTEL_FAM6_BROADWELL_X) {
+   pr_err_once("late loading on model 79 is disabled.\n");
+   return true;
+   }
+
+   return false;
+}
+
 static enum ucode_state request_microcode_fw(int cpu, struct device *device,
 bool refresh_fw)
 {
@@ -926,6 +939,9 @@ static enum ucode_state request_microcode_fw(int cpu, 
struct device *device,
const struct firmware *firmware;
enum ucode_state ret;
 
+   if (is_blacklisted(cpu))
+   return UCODE_NFOUND;
+
sprintf(name, "intel-ucode/%02x-%02x-%02x",
c->x86, c->x86_model, c->x86_mask);
 
@@ -950,6 +966,9 @@ static int get_ucode_user(void *to, const void *from, 
size_t n)
 static enum ucode_state
 request_microcode_user(int cpu, const void __user *buf, size_t size)
 {
+   if (is_blacklisted(cpu))
+   return UCODE_NFOUND;
+
return generic_load_microcode(cpu, (void *)buf, size, _ucode_user);
 }
 

Re: linux-next: manual merge of the sound-asoc tree with the drm-misc tree

[Mark Brown]

On Wed, Oct 18, 2017 at 02:33:37PM +, Deucher, Alexander wrote:

Please fix your mail client to word wrap within paragraphs at something
substantially less than 80 columns.  Doing this makes your messages much
easier to read and reply to.

> Your conflict change affectively reverted 1e4448648333a which is what
> caused the problem.  It looks like Dave did not yet pull the request I
> made.  I can send another pull request, but you may run into the same
> issue if you resolve the conflict the same way again.  

OK...  I really wasn't expecting to see any conflicts at all TBH.  I'll
have a look tomorrow.


signature.asc
Description: PGP signature

[tip:x86/urgent] x86/microcode/intel: Disable late loading on model 79

[tip-bot for Borislav Petkov]

Commit-ID:  723f2828a98c8ca19842042f418fb30dd8cfc0f7
Gitweb: https://git.kernel.org/tip/723f2828a98c8ca19842042f418fb30dd8cfc0f7
Author: Borislav Petkov 
AuthorDate: Wed, 18 Oct 2017 13:12:25 +0200
Committer:  Ingo Molnar 
CommitDate: Wed, 18 Oct 2017 15:20:20 +0200

x86/microcode/intel: Disable late loading on model 79

Blacklist Broadwell X model 79 for late loading due to an erratum.

Signed-off-by: Borislav Petkov 
Acked-by: Tony Luck 
Cc: 
Cc: Linus Torvalds 
Cc: Peter Zijlstra 
Cc: Thomas Gleixner 
Link: http://lkml.kernel.org/r/20171018111225.25635-1...@alien8.de
Signed-off-by: Ingo Molnar 
---
 arch/x86/kernel/cpu/microcode/intel.c | 19 +++
 1 file changed, 19 insertions(+)

diff --git a/arch/x86/kernel/cpu/microcode/intel.c 
b/arch/x86/kernel/cpu/microcode/intel.c
index 8f7a9bb..7dbcb7a 100644
--- a/arch/x86/kernel/cpu/microcode/intel.c
+++ b/arch/x86/kernel/cpu/microcode/intel.c
@@ -34,6 +34,7 @@
 #include 
 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -918,6 +919,18 @@ static int get_ucode_fw(void *to, const void *from, size_t 
n)
return 0;
 }
 
+static bool is_blacklisted(unsigned int cpu)
+{
+   struct cpuinfo_x86 *c = _data(cpu);
+
+   if (c->x86 == 6 && c->x86_model == INTEL_FAM6_BROADWELL_X) {
+   pr_err_once("late loading on model 79 is disabled.\n");
+   return true;
+   }
+
+   return false;
+}
+
 static enum ucode_state request_microcode_fw(int cpu, struct device *device,
 bool refresh_fw)
 {
@@ -926,6 +939,9 @@ static enum ucode_state request_microcode_fw(int cpu, 
struct device *device,
const struct firmware *firmware;
enum ucode_state ret;
 
+   if (is_blacklisted(cpu))
+   return UCODE_NFOUND;
+
sprintf(name, "intel-ucode/%02x-%02x-%02x",
c->x86, c->x86_model, c->x86_mask);
 
@@ -950,6 +966,9 @@ static int get_ucode_user(void *to, const void *from, 
size_t n)
 static enum ucode_state
 request_microcode_user(int cpu, const void __user *buf, size_t size)
 {
+   if (is_blacklisted(cpu))
+   return UCODE_NFOUND;
+
return generic_load_microcode(cpu, (void *)buf, size, _ucode_user);
 }
 

Re: linux-next: manual merge of the sound-asoc tree with the drm-misc tree

[Mark Brown]

On Wed, Oct 18, 2017 at 02:33:37PM +, Deucher, Alexander wrote:

Please fix your mail client to word wrap within paragraphs at something
substantially less than 80 columns.  Doing this makes your messages much
easier to read and reply to.

> Your conflict change affectively reverted 1e4448648333a which is what
> caused the problem.  It looks like Dave did not yet pull the request I
> made.  I can send another pull request, but you may run into the same
> issue if you resolve the conflict the same way again.  

OK...  I really wasn't expecting to see any conflicts at all TBH.  I'll
have a look tomorrow.


signature.asc
Description: PGP signature

Re: [RFC PATCH v9 for 4.15 01/14] Restartable sequences system call

[Mathieu Desnoyers]

- On Oct 18, 2017, at 2:22 AM, Greg Kroah-Hartman 
gre...@linuxfoundation.org wrote:

> On Tue, Oct 17, 2017 at 04:19:41PM +, Ben Maurer wrote:
>> Hey,
>> 
>> > So far the restrictions I see for libraries using this symbol are:
>> > - They should never be unloaded,
>> > - They should never be loaded with dlopen RTLD_LOCAL flag.
>> 
>> We talked a bit about this off-list but I wanted to state publicly
>> that I think this model works well for our use case. Specifically,
>> 
>> (1) It reduces complexity by focusing on the common case -- long term
>> we expect glibc to manage the process of using this feature and
>> registering/deregistering threads for rseq. Unloading isn't a
>> challenge in these situations, so why add the complexity for it?
> 
> You never install a new version of glibc on a running system, and expect
> everything to keep running successfully?  Breaking that would not be
> good...

If we share the __rseq_abi TLS weak symbol between glibc, applications,
and early-adopter libraries, we just need those early adopters to
check whether the TLS is already registered (cpu_id field >= 0), and
don't bother doing their lazy registration if it's already been done
for them (either by glibc or by the application).

If either the application or a lazy-registering library gets a EBUSY
errno from rseq registration, it can consider that another library
already performed the registration for them (probably glibc).

As long as early adopter libraries don't expect to be sole users
of __rseq_abi, upgrading to newer glibc should work fine.

But this means we need to get all early adopters to get it right from
the get-go, or things could break when you compose them.

Thoughts ?

Thanks,

Mathieu

> 
> thanks,
> 
> greg k-h

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com

Re: [RFC PATCH v9 for 4.15 01/14] Restartable sequences system call

[Mathieu Desnoyers]

- On Oct 18, 2017, at 2:22 AM, Greg Kroah-Hartman 
gre...@linuxfoundation.org wrote:

> On Tue, Oct 17, 2017 at 04:19:41PM +, Ben Maurer wrote:
>> Hey,
>> 
>> > So far the restrictions I see for libraries using this symbol are:
>> > - They should never be unloaded,
>> > - They should never be loaded with dlopen RTLD_LOCAL flag.
>> 
>> We talked a bit about this off-list but I wanted to state publicly
>> that I think this model works well for our use case. Specifically,
>> 
>> (1) It reduces complexity by focusing on the common case -- long term
>> we expect glibc to manage the process of using this feature and
>> registering/deregistering threads for rseq. Unloading isn't a
>> challenge in these situations, so why add the complexity for it?
> 
> You never install a new version of glibc on a running system, and expect
> everything to keep running successfully?  Breaking that would not be
> good...

If we share the __rseq_abi TLS weak symbol between glibc, applications,
and early-adopter libraries, we just need those early adopters to
check whether the TLS is already registered (cpu_id field >= 0), and
don't bother doing their lazy registration if it's already been done
for them (either by glibc or by the application).

If either the application or a lazy-registering library gets a EBUSY
errno from rseq registration, it can consider that another library
already performed the registration for them (probably glibc).

As long as early adopter libraries don't expect to be sole users
of __rseq_abi, upgrading to newer glibc should work fine.

But this means we need to get all early adopters to get it right from
the get-go, or things could break when you compose them.

Thoughts ?

Thanks,

Mathieu

> 
> thanks,
> 
> greg k-h

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com

Re: [PATCH] ftrace/docs: Add documentation on how to use ftrace from within the kernel

[Jonathan Corbet]

On Mon, 9 Oct 2017 15:32:30 -0400
Steven Rostedt  wrote:

> With the coming removal of jprobes, using ftrace callbacks is one of the
> utilities that replace the jprobes functionality. Having a document that
> explains how to use ftrace as such will help in the transition from jprobes
> to ftrace. This document is for kernel developers that require attaching a
> callback to a function within the kernel.

So I'm sorry, this kind of fell through the cracks.  There seem to be a lot
of cracks recently...

It generally looks good, and could be merged in this form, but I'm gonna
poke at the RST side of it for just a bit.

> Link: 
> http://lkml.kernel.org/r/150724519527.5014.10207042218696587159.stgit@devbox
> 
> Signed-off-by: Steven Rostedt (VMware) 
> ---
>  Documentation/trace/ftrace-uses.rst | 297 
> 
>  1 file changed, 297 insertions(+)
>  create mode 100644 Documentation/trace/ftrace-uses.rst

You've created an RST file here, but haven't hooked it into the overall
document build.  It could go in the development tools book, or we could
consider whether a separate tracing manual will eventually make sense?

> diff --git a/Documentation/trace/ftrace-uses.rst 
> b/Documentation/trace/ftrace-uses.rst
> new file mode 100644
> index 000..5914d71
> --- /dev/null
> +++ b/Documentation/trace/ftrace-uses.rst
> @@ -0,0 +1,297 @@
> + Using ftrace to hook to functions
> + =
> +
> +Copyright 2017 VMware Inc.
> +   Author:   Steven Rostedt 
> +  License:   The GNU Free Documentation License, Version 1.2
> +   (dual licensed under the GPL v2)

This isn't proper RST and may not format up the way you expect it to.

> +Written for: 4.14
> +
> +Introduction
> +
> +
> +The ftrace infrastructure was originially created to attach callbacks to the
> +beginning of functions in order to record and trace the flow of the kernel.
> +But callbacks to the start of a function can have other use cases. Either
> +for live kernel patching, or for security monitoring. This document describes
> +how to use ftrace to implement your own function callbacks.
> +
> +
> +The ftrace context
> +==

In Documentation/doc-guide/sphinx.rst we have some recommended conventions
for subsection headers; it would be good to follow them.  This, again, will
not format the way you expect.

> +WARNING: The ability to add a callback to almost any function within the
> +kernel comes with risks. A callback can be called from any context
> +(normal, softirq, irq, and NMI). Callbacks can also be called just before
> +going to idle, during CPU bring up and takedown, or going to user space.
> +This requires extra care to what can be done inside a callback. A callback
> +can be called outside the protective scope of RCU.
> +
> +The ftrace infrastructure has some protections agains recursions and RCU
> +but one must still be very careful how they use the callbacks.
> +
> +
> +The ftrace_ops structure
> +
> +
> +To register a function callback, a ftrace_ops is required. This structure
> +is used to tell ftrace what function should be called as the callback
> +as well as what protections the callback will perform and not require
> +ftrace to handle.
> +
> +There is only one field that is needed to be set when registering
> +an ftrace_ops with ftrace::
> +
> + struct ftrace_ops ops = {
> +   .func = my_callback_func,
> +   .flags= MY_FTRACE_FLAGS
> +   .private  = any_private_data_structure,
> + };
> +
> +Both .flags and .private are optional. Only .func is required.
> +
> +To enable tracing call::
> +
> +  register_ftrace_function();
> +
> +To disable tracing call::
> +
> +  unregister_ftrace_function();
> +
> +The above is defined by including the header::
> +
> + #include 
> +
> +The registered callback will start being called some time after the
> +register_ftrace_function() is called and before it returns. The exact time
> +that callbacks start being called is dependent upon architecture and 
> scheduling
> +of services. The callback itself will have to handle any synchronization if 
> it
> +must begin at an exact moment.
> +
> +The unregister_ftrace_function() will guarantee that the callback is
> +no longer being called by functions after the unregister_ftrace_function()
> +returns. Note that to perform this guarantee, the 
> unregister_ftrace_function()
> +may take some time to finish.
> +
> +
> +The callback function
> +=
> +
> +The prototype of the callback function is as follows (as of v4.14)::

This would be nice to pull directly from the kerneldoc comments if
possible. 

> + void callback_func(unsigned long ip, unsigned long parent_ip,
> + struct ftrace_ops *op, struct pt_regs *regs);
> +
> +@ip
> +  This is the instruction pointer 

Re: [PATCH] ftrace/docs: Add documentation on how to use ftrace from within the kernel

[Jonathan Corbet]

On Mon, 9 Oct 2017 15:32:30 -0400
Steven Rostedt  wrote:

> With the coming removal of jprobes, using ftrace callbacks is one of the
> utilities that replace the jprobes functionality. Having a document that
> explains how to use ftrace as such will help in the transition from jprobes
> to ftrace. This document is for kernel developers that require attaching a
> callback to a function within the kernel.

So I'm sorry, this kind of fell through the cracks.  There seem to be a lot
of cracks recently...

It generally looks good, and could be merged in this form, but I'm gonna
poke at the RST side of it for just a bit.

> Link: 
> http://lkml.kernel.org/r/150724519527.5014.10207042218696587159.stgit@devbox
> 
> Signed-off-by: Steven Rostedt (VMware) 
> ---
>  Documentation/trace/ftrace-uses.rst | 297 
> 
>  1 file changed, 297 insertions(+)
>  create mode 100644 Documentation/trace/ftrace-uses.rst

You've created an RST file here, but haven't hooked it into the overall
document build.  It could go in the development tools book, or we could
consider whether a separate tracing manual will eventually make sense?

> diff --git a/Documentation/trace/ftrace-uses.rst 
> b/Documentation/trace/ftrace-uses.rst
> new file mode 100644
> index 000..5914d71
> --- /dev/null
> +++ b/Documentation/trace/ftrace-uses.rst
> @@ -0,0 +1,297 @@
> + Using ftrace to hook to functions
> + =
> +
> +Copyright 2017 VMware Inc.
> +   Author:   Steven Rostedt 
> +  License:   The GNU Free Documentation License, Version 1.2
> +   (dual licensed under the GPL v2)

This isn't proper RST and may not format up the way you expect it to.

> +Written for: 4.14
> +
> +Introduction
> +
> +
> +The ftrace infrastructure was originially created to attach callbacks to the
> +beginning of functions in order to record and trace the flow of the kernel.
> +But callbacks to the start of a function can have other use cases. Either
> +for live kernel patching, or for security monitoring. This document describes
> +how to use ftrace to implement your own function callbacks.
> +
> +
> +The ftrace context
> +==

In Documentation/doc-guide/sphinx.rst we have some recommended conventions
for subsection headers; it would be good to follow them.  This, again, will
not format the way you expect.

> +WARNING: The ability to add a callback to almost any function within the
> +kernel comes with risks. A callback can be called from any context
> +(normal, softirq, irq, and NMI). Callbacks can also be called just before
> +going to idle, during CPU bring up and takedown, or going to user space.
> +This requires extra care to what can be done inside a callback. A callback
> +can be called outside the protective scope of RCU.
> +
> +The ftrace infrastructure has some protections agains recursions and RCU
> +but one must still be very careful how they use the callbacks.
> +
> +
> +The ftrace_ops structure
> +
> +
> +To register a function callback, a ftrace_ops is required. This structure
> +is used to tell ftrace what function should be called as the callback
> +as well as what protections the callback will perform and not require
> +ftrace to handle.
> +
> +There is only one field that is needed to be set when registering
> +an ftrace_ops with ftrace::
> +
> + struct ftrace_ops ops = {
> +   .func = my_callback_func,
> +   .flags= MY_FTRACE_FLAGS
> +   .private  = any_private_data_structure,
> + };
> +
> +Both .flags and .private are optional. Only .func is required.
> +
> +To enable tracing call::
> +
> +  register_ftrace_function();
> +
> +To disable tracing call::
> +
> +  unregister_ftrace_function();
> +
> +The above is defined by including the header::
> +
> + #include 
> +
> +The registered callback will start being called some time after the
> +register_ftrace_function() is called and before it returns. The exact time
> +that callbacks start being called is dependent upon architecture and 
> scheduling
> +of services. The callback itself will have to handle any synchronization if 
> it
> +must begin at an exact moment.
> +
> +The unregister_ftrace_function() will guarantee that the callback is
> +no longer being called by functions after the unregister_ftrace_function()
> +returns. Note that to perform this guarantee, the 
> unregister_ftrace_function()
> +may take some time to finish.
> +
> +
> +The callback function
> +=
> +
> +The prototype of the callback function is as follows (as of v4.14)::

This would be nice to pull directly from the kerneldoc comments if
possible. 

> + void callback_func(unsigned long ip, unsigned long parent_ip,
> + struct ftrace_ops *op, struct pt_regs *regs);
> +
> +@ip
> +  This is the instruction pointer of the function that is being traced.
> +  (where 

Re: [PATCH] Makefile: add targets for config-help and pkg-help

[Masahiro Yamada]

2017-10-18 0:18 GMT+09:00 Shuah Khan :
> Change to enable config help and package help from the main make level
> to make it easier to use. It has become difficult to find config help
> and pkg help specific output from the "help" information.
>
> Signed-off-by: Shuah Khan 
> ---
>  Makefile | 7 +++
>  1 file changed, 7 insertions(+)
>
> diff --git a/Makefile b/Makefile
> index 46bfb0ed2257..1d6f86df1b6c 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -1441,6 +1441,13 @@ help:
> @echo  'Execute "make" or "make all" to build all targets marked with 
> [*] '
> @echo  'For further info see the ./README file'
>
> +PHONY += config-help
> +config-help:
> +   @$(MAKE) -f $(srctree)/scripts/kconfig/Makefile help
> +
> +PHONY += pkg-help
> +pkg-help:
> +   @$(MAKE) $(build)=$(package-dir) help
>
>  help-board-dirs := $(addprefix help-,$(board-dirs))
>
> --


What happened to "doc-help" ?
(I want to see consistent hyphenation)

Please follow Randy's suggestion.

Also you need to add %-help pattern to no-dot-config-targets.



-- 
Best Regards
Masahiro Yamada

Re: [PATCH] Makefile: add targets for config-help and pkg-help

[Masahiro Yamada]

2017-10-18 0:18 GMT+09:00 Shuah Khan :
> Change to enable config help and package help from the main make level
> to make it easier to use. It has become difficult to find config help
> and pkg help specific output from the "help" information.
>
> Signed-off-by: Shuah Khan 
> ---
>  Makefile | 7 +++
>  1 file changed, 7 insertions(+)
>
> diff --git a/Makefile b/Makefile
> index 46bfb0ed2257..1d6f86df1b6c 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -1441,6 +1441,13 @@ help:
> @echo  'Execute "make" or "make all" to build all targets marked with 
> [*] '
> @echo  'For further info see the ./README file'
>
> +PHONY += config-help
> +config-help:
> +   @$(MAKE) -f $(srctree)/scripts/kconfig/Makefile help
> +
> +PHONY += pkg-help
> +pkg-help:
> +   @$(MAKE) $(build)=$(package-dir) help
>
>  help-board-dirs := $(addprefix help-,$(board-dirs))
>
> --


What happened to "doc-help" ?
(I want to see consistent hyphenation)

Please follow Randy's suggestion.

Also you need to add %-help pattern to no-dot-config-targets.



-- 
Best Regards
Masahiro Yamada

Re: [PATCH 02/25] thermal/drivers/hisi: Remove the multiple sensors support

[Daniel Lezcano]

On 18/10/2017 17:51, Eduardo Valentin wrote:
> On Wed, Oct 18, 2017 at 09:48:21AM +0800, Leo Yan wrote:
>> On Tue, Oct 17, 2017 at 02:07:08PM -0700, Eduardo Valentin wrote:
>>> On Tue, Oct 17, 2017 at 09:03:40PM +0200, Daniel Lezcano wrote:
 On 17/10/2017 20:25, Eduardo Valentin wrote:
> Hello,
>
> On Tue, Oct 17, 2017 at 02:28:27PM +0200, Daniel Lezcano wrote:
>> On 17/10/2017 05:54, Eduardo Valentin wrote:
>>> On Tue, Oct 10, 2017 at 08:02:27PM +0200, Daniel Lezcano wrote:
 By essence, the tsensor does not really support multiple sensor at the 
 same
 time. It allows to set a sensor and use it to get the temperature, 
 another
 sensor could be switched but with a delay of 3-5ms. It is difficult to 
 read
 simultaneously several sensors without a big delay.

>>>
>>> Is 3-5 ms enough to loose an event? Is this really a problem?
>>
>> There are several aspects:
>>
>>  - the multiple sensors is not needed here
>
> Well, that is debatable, I cannot really agree or disagree with the
> above statement without understanding the use cases and most important,
> the location of each sensor. What is the location of each sensor?
>
>>
>>  - the temperature controller is not designed to read several sensors at
>> the same time, we switch the sensor and that clears some internal
>> buffers and re-init the controller
>
> Which is still very helpful in case you have multiple hotspots that you
> want to track and they are exposed on different workloads. Sacrificing
> the availability of sensors is something needs a better justification
> other than "current code uses only one".
>
>
>>
>>  - some boards can take 40°C in 1 sec, the temperature increase is
>> insanely fast and reading several sensors add an extra 15ms.
>>
>
>
> Ok... What is the difference in update rate with and without the switch
> of sensors? With the above worst case, you have about 4/6 mC/ms. Can
> your tsensor support that resolution for a single sensor? What is the
> maximum resolution a tsensor can support? What is the penalty added with
> switch?
>
> Based on this data, and the above 3-5ms, that  means you would miss about
> ~ 3 - 4 mC while switching ( assuming tsensor can really achieve the
> above rate of change: 5ms * 4/6 mC /ms). Are you sure that is
> enough justification to drop three extra sensors?

 Ok if I refer to the documentation the rate is 0.768 ms with the current
 configuration.

 The driver is currently bogus: register overwritten, bouncing interrupt,
 unneeded lock, ... So the proposition was to remove the multiple sensors
 support, clean the driver, and re-introduce it if there is a need.

 If I remember correctly Leo, author of the driver, agreed on this. Leo ?

 Note, I'm not strongly against multiple sensors support in the driver if
 you think it is convenient but it is much simpler to remove the current
 code as it is not used and put it back on top of a sane foundation
 instead of circumventing that on the existing code.


>>>
>>> I am also fine with the above strategy, as long as you are sure you are
>>> not breaking anyone (specially userspace). Also, it would be good to get
>>> a reviewed-by from hisilicon just to confirm (Leo?).
>>
>> Sorry I missed to reply this patch. And yes, I have tested and
>> reviewed it at my side:
>>
>> Reviewed-by: Leo Yan 
>>
>> P.s. I am working for Linaro; I am continously co-working with
>> Hisilicon to maintain this driver due it's important for Hikey/Hikey960
>> two boards stability; this driver also is important for our daily
>> profiling for power and performance. Eduardo, so please let us know if
>> you still need ack from Hisilicon engineer.
> 
> 
> Yeah, I think adding your Reviewed-by and Kevin's is enough for this
> series to go through. As I asked Daniel already, only few minor stuff
> needs to be fixed along with the addition of the reviewed-by's.

The different warnings you reported are fixed and the reviewed-by's /
acked-by's added. I think the patches 19-25 may need an extra look, so I
will resend all the other patches meanwhile.

Does it sound good?


-- 
  Linaro.org │ Open source software for ARM SoCs

Follow Linaro:   Facebook |
 Twitter |
 Blog

Re: [PATCH 02/25] thermal/drivers/hisi: Remove the multiple sensors support

[Daniel Lezcano]

On 18/10/2017 17:51, Eduardo Valentin wrote:
> On Wed, Oct 18, 2017 at 09:48:21AM +0800, Leo Yan wrote:
>> On Tue, Oct 17, 2017 at 02:07:08PM -0700, Eduardo Valentin wrote:
>>> On Tue, Oct 17, 2017 at 09:03:40PM +0200, Daniel Lezcano wrote:
 On 17/10/2017 20:25, Eduardo Valentin wrote:
> Hello,
>
> On Tue, Oct 17, 2017 at 02:28:27PM +0200, Daniel Lezcano wrote:
>> On 17/10/2017 05:54, Eduardo Valentin wrote:
>>> On Tue, Oct 10, 2017 at 08:02:27PM +0200, Daniel Lezcano wrote:
 By essence, the tsensor does not really support multiple sensor at the 
 same
 time. It allows to set a sensor and use it to get the temperature, 
 another
 sensor could be switched but with a delay of 3-5ms. It is difficult to 
 read
 simultaneously several sensors without a big delay.

>>>
>>> Is 3-5 ms enough to loose an event? Is this really a problem?
>>
>> There are several aspects:
>>
>>  - the multiple sensors is not needed here
>
> Well, that is debatable, I cannot really agree or disagree with the
> above statement without understanding the use cases and most important,
> the location of each sensor. What is the location of each sensor?
>
>>
>>  - the temperature controller is not designed to read several sensors at
>> the same time, we switch the sensor and that clears some internal
>> buffers and re-init the controller
>
> Which is still very helpful in case you have multiple hotspots that you
> want to track and they are exposed on different workloads. Sacrificing
> the availability of sensors is something needs a better justification
> other than "current code uses only one".
>
>
>>
>>  - some boards can take 40°C in 1 sec, the temperature increase is
>> insanely fast and reading several sensors add an extra 15ms.
>>
>
>
> Ok... What is the difference in update rate with and without the switch
> of sensors? With the above worst case, you have about 4/6 mC/ms. Can
> your tsensor support that resolution for a single sensor? What is the
> maximum resolution a tsensor can support? What is the penalty added with
> switch?
>
> Based on this data, and the above 3-5ms, that  means you would miss about
> ~ 3 - 4 mC while switching ( assuming tsensor can really achieve the
> above rate of change: 5ms * 4/6 mC /ms). Are you sure that is
> enough justification to drop three extra sensors?

 Ok if I refer to the documentation the rate is 0.768 ms with the current
 configuration.

 The driver is currently bogus: register overwritten, bouncing interrupt,
 unneeded lock, ... So the proposition was to remove the multiple sensors
 support, clean the driver, and re-introduce it if there is a need.

 If I remember correctly Leo, author of the driver, agreed on this. Leo ?

 Note, I'm not strongly against multiple sensors support in the driver if
 you think it is convenient but it is much simpler to remove the current
 code as it is not used and put it back on top of a sane foundation
 instead of circumventing that on the existing code.


>>>
>>> I am also fine with the above strategy, as long as you are sure you are
>>> not breaking anyone (specially userspace). Also, it would be good to get
>>> a reviewed-by from hisilicon just to confirm (Leo?).
>>
>> Sorry I missed to reply this patch. And yes, I have tested and
>> reviewed it at my side:
>>
>> Reviewed-by: Leo Yan 
>>
>> P.s. I am working for Linaro; I am continously co-working with
>> Hisilicon to maintain this driver due it's important for Hikey/Hikey960
>> two boards stability; this driver also is important for our daily
>> profiling for power and performance. Eduardo, so please let us know if
>> you still need ack from Hisilicon engineer.
> 
> 
> Yeah, I think adding your Reviewed-by and Kevin's is enough for this
> series to go through. As I asked Daniel already, only few minor stuff
> needs to be fixed along with the addition of the reviewed-by's.

The different warnings you reported are fixed and the reviewed-by's /
acked-by's added. I think the patches 19-25 may need an extra look, so I
will resend all the other patches meanwhile.

Does it sound good?


-- 
  Linaro.org │ Open source software for ARM SoCs

Follow Linaro:   Facebook |
 Twitter |
 Blog

Re: [PATCH] RDMA/cxgb4: Convert timers to use timer_setup()

[Doug Ledford]

On Tue, 2017-10-17 at 09:25 -0500, Steve Wise wrote:
> > 
> > In preparation for unconditionally passing the struct timer_list
> > pointer to
> > all timer callbacks, switch to using the new timer_setup() and
> > from_timer()
> > to pass the timer pointer explicitly. Also removes an unused timer
> > and
> > drops a redundant initialization.
> > 
> > Cc: Steve Wise 
> > Cc: Doug Ledford 
> > Cc: Sean Hefty 
> > Cc: Hal Rosenstock 
> > Cc: linux-r...@vger.kernel.org
> > Signed-off-by: Kees Cook 
> 
> Acked-by: Steve Wise 

Thanks, applied.

-- 
Doug Ledford 
GPG KeyID: B826A3330E572FDD
Key fingerprint = AE6B 1BDA 122B 23B4 265B  1274 B826 A333 0E57 2FDD

Re: [PATCH] RDMA/cxgb4: Convert timers to use timer_setup()

[Doug Ledford]

On Tue, 2017-10-17 at 09:25 -0500, Steve Wise wrote:
> > 
> > In preparation for unconditionally passing the struct timer_list
> > pointer to
> > all timer callbacks, switch to using the new timer_setup() and
> > from_timer()
> > to pass the timer pointer explicitly. Also removes an unused timer
> > and
> > drops a redundant initialization.
> > 
> > Cc: Steve Wise 
> > Cc: Doug Ledford 
> > Cc: Sean Hefty 
> > Cc: Hal Rosenstock 
> > Cc: linux-r...@vger.kernel.org
> > Signed-off-by: Kees Cook 
> 
> Acked-by: Steve Wise 

Thanks, applied.

-- 
Doug Ledford 
GPG KeyID: B826A3330E572FDD
Key fingerprint = AE6B 1BDA 122B 23B4 265B  1274 B826 A333 0E57 2FDD

Re: [RFC 04/19] s390/zcrypt: create an AP matrix device on the AP matrix bus

[Cornelia Huck]

On Fri, 13 Oct 2017 13:38:49 -0400
Tony Krowiak  wrote:

[Please take with a grain of salt as I did not yet have time to take
more than a very superficial glance at the whole structure.]

> Creates a single AP matrix device on the AP matrix bus.
> The matrix device will be created as part of the AP matrix bus
> initialization. The matrix device will hold the AP queues that
> have been reserved for use by KVM guests. Mediated matrix devices
> can then be created for each guest. The mediated matrix devices can
> then be configured with a matrix of AP adapters, usage and
> control domains that will be made accessible to the guest.
> 
> The sysfs location of the matrix device is:
> 
> /sys/bus/ap_matrix
> ... [devices]
> .. [matrix]

Also /sys/devices/ap_matrix/matrix, isn't it?

> 
> Signed-off-by: Tony Krowiak 
> ---
>  drivers/s390/crypto/ap_matrix_bus.c |   54 
> +++
>  drivers/s390/crypto/ap_matrix_bus.h |6 
>  2 files changed, 60 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/s390/crypto/ap_matrix_bus.c 
> b/drivers/s390/crypto/ap_matrix_bus.c
> index fbae175..4eb1e3c 100644
> --- a/drivers/s390/crypto/ap_matrix_bus.c
> +++ b/drivers/s390/crypto/ap_matrix_bus.c
> @@ -12,6 +12,7 @@
>  
>  #include 
>  #include 
> +#include 
>  #include 
>  
>  #include "ap_matrix_bus.h"
> @@ -21,13 +22,59 @@
>  MODULE_LICENSE("GPL v2");
>  
>  #define AP_MATRIX_BUS_NAME "ap_matrix"
> +#define AP_MATRIX_DEV_TYPE_NAME "ap_matrix"
> +#define AP_MATRIX_DEV_NAME "matrix"
>  
>  static struct device *ap_matrix_root_device;
> +static struct ap_matrix *matrix;
>  
>  static struct bus_type ap_matrix_bus_type = {
>   .name = AP_MATRIX_BUS_NAME,
>  };
>  
> +static struct device_type ap_matrix_type = {
> + .name = AP_MATRIX_DEV_TYPE_NAME,
> +};
> +
> +static void ap_matrix_dev_release(struct device *dev)
> +{
> + struct ap_matrix *ap_matrix;
> +
> + ap_matrix = container_of(dev, struct ap_matrix, device);
> +
> + if (matrix == ap_matrix)
> + kfree(matrix);
> +
> + matrix = NULL;

This looks very, very odd. Refcounting should take care that the
release function is only invoked if the device is really gone.

Also, I don't think you need to keep a pointer to the device as it is a
singleton: It's simply the only device on the list and you should be
able to easily pick it that way. If your code does not register further
devices on the bus, there won't be ambiguities.

> +}
> +
> +static int ap_matrix_dev_create(void)
> +{
> + int ret;
> +
> + matrix = kzalloc(sizeof(*matrix), GFP_KERNEL);
> + if (!matrix)
> + return -ENOMEM;
> +
> + matrix->device.type = _matrix_type;
> + dev_set_name(>device, "%s", AP_MATRIX_DEV_NAME);
> + matrix->device.bus = _matrix_bus_type;
> + matrix->device.parent = ap_matrix_root_device;
> + matrix->device.release = ap_matrix_dev_release;
> +
> + ret = device_register(>device);
> + if (ret) {
> + put_device(>device);
> + kfree(matrix);
> + matrix = NULL;

That's wrong. The release function needs to clean up the embedding
structure, so that kfree is at best useless and at worst wrong, if
something else grabbed a reference.

> + return ret;
> + }
> +
> + get_device(>device);

Why should you need an extra reference here? I'd expect the code
hanging devices off this one to properly grab a reference, so you
should be all good?

> +
> + return 0;
> +}
> +
>  int __init ap_matrix_init(void)
>  {
>   int ret;
> @@ -41,8 +88,15 @@ int __init ap_matrix_init(void)
>   if (ret)
>   goto bus_reg_err;
>  
> + ret = ap_matrix_dev_create();
> + if (ret)
> + goto matrix_create_err;
> +
>   return 0;
>  
> +matrix_create_err:
> + bus_unregister(_matrix_bus_type);
> +
>  bus_reg_err:
>   root_device_unregister(ap_matrix_root_device);

Re: [RFC 04/19] s390/zcrypt: create an AP matrix device on the AP matrix bus

[Cornelia Huck]

On Fri, 13 Oct 2017 13:38:49 -0400
Tony Krowiak  wrote:

[Please take with a grain of salt as I did not yet have time to take
more than a very superficial glance at the whole structure.]

> Creates a single AP matrix device on the AP matrix bus.
> The matrix device will be created as part of the AP matrix bus
> initialization. The matrix device will hold the AP queues that
> have been reserved for use by KVM guests. Mediated matrix devices
> can then be created for each guest. The mediated matrix devices can
> then be configured with a matrix of AP adapters, usage and
> control domains that will be made accessible to the guest.
> 
> The sysfs location of the matrix device is:
> 
> /sys/bus/ap_matrix
> ... [devices]
> .. [matrix]

Also /sys/devices/ap_matrix/matrix, isn't it?

> 
> Signed-off-by: Tony Krowiak 
> ---
>  drivers/s390/crypto/ap_matrix_bus.c |   54 
> +++
>  drivers/s390/crypto/ap_matrix_bus.h |6 
>  2 files changed, 60 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/s390/crypto/ap_matrix_bus.c 
> b/drivers/s390/crypto/ap_matrix_bus.c
> index fbae175..4eb1e3c 100644
> --- a/drivers/s390/crypto/ap_matrix_bus.c
> +++ b/drivers/s390/crypto/ap_matrix_bus.c
> @@ -12,6 +12,7 @@
>  
>  #include 
>  #include 
> +#include 
>  #include 
>  
>  #include "ap_matrix_bus.h"
> @@ -21,13 +22,59 @@
>  MODULE_LICENSE("GPL v2");
>  
>  #define AP_MATRIX_BUS_NAME "ap_matrix"
> +#define AP_MATRIX_DEV_TYPE_NAME "ap_matrix"
> +#define AP_MATRIX_DEV_NAME "matrix"
>  
>  static struct device *ap_matrix_root_device;
> +static struct ap_matrix *matrix;
>  
>  static struct bus_type ap_matrix_bus_type = {
>   .name = AP_MATRIX_BUS_NAME,
>  };
>  
> +static struct device_type ap_matrix_type = {
> + .name = AP_MATRIX_DEV_TYPE_NAME,
> +};
> +
> +static void ap_matrix_dev_release(struct device *dev)
> +{
> + struct ap_matrix *ap_matrix;
> +
> + ap_matrix = container_of(dev, struct ap_matrix, device);
> +
> + if (matrix == ap_matrix)
> + kfree(matrix);
> +
> + matrix = NULL;

This looks very, very odd. Refcounting should take care that the
release function is only invoked if the device is really gone.

Also, I don't think you need to keep a pointer to the device as it is a
singleton: It's simply the only device on the list and you should be
able to easily pick it that way. If your code does not register further
devices on the bus, there won't be ambiguities.

> +}
> +
> +static int ap_matrix_dev_create(void)
> +{
> + int ret;
> +
> + matrix = kzalloc(sizeof(*matrix), GFP_KERNEL);
> + if (!matrix)
> + return -ENOMEM;
> +
> + matrix->device.type = _matrix_type;
> + dev_set_name(>device, "%s", AP_MATRIX_DEV_NAME);
> + matrix->device.bus = _matrix_bus_type;
> + matrix->device.parent = ap_matrix_root_device;
> + matrix->device.release = ap_matrix_dev_release;
> +
> + ret = device_register(>device);
> + if (ret) {
> + put_device(>device);
> + kfree(matrix);
> + matrix = NULL;

That's wrong. The release function needs to clean up the embedding
structure, so that kfree is at best useless and at worst wrong, if
something else grabbed a reference.

> + return ret;
> + }
> +
> + get_device(>device);

Why should you need an extra reference here? I'd expect the code
hanging devices off this one to properly grab a reference, so you
should be all good?

> +
> + return 0;
> +}
> +
>  int __init ap_matrix_init(void)
>  {
>   int ret;
> @@ -41,8 +88,15 @@ int __init ap_matrix_init(void)
>   if (ret)
>   goto bus_reg_err;
>  
> + ret = ap_matrix_dev_create();
> + if (ret)
> + goto matrix_create_err;
> +
>   return 0;
>  
> +matrix_create_err:
> + bus_unregister(_matrix_bus_type);
> +
>  bus_reg_err:
>   root_device_unregister(ap_matrix_root_device);

Re: [PATCH 0/2] rt5651: Enable platforms with int mic on IN2

[Pierre-Louis Bossart]

On 10/18/17 11:07 AM, Carlo Caione wrote:

From: Carlo Caione 

While working on enabling a cherry-trail laptop shipping the rt5651
codec I realized that the machine driver needed some fixup.

In particular the laptop I'm working on (KIANO SlimNote 14.2) has the
internal mic connected to the IN2 port.

All the laptop-specific work with the related quirks will follow.


Nice, thanks for the work. You'll have to rebase and resubmit to fix 
conflicts. I provided changes to simplify DMIC quirks (there is a single 
interface, not 2 on this chip) that aren't reflected in your patches and 
the addition of MCLK support. Mark's for-next branch already took the 
changes this morning.

Also let me know if you want the UCM files to be updated.



Carlo Caione (2):
   ASoC: intel: byt: Enable quirk logic for custom maps
   ASoC: intel: byt: Introduce new custom IN2 map

  sound/soc/intel/boards/bytcr_rt5651.c | 21 -
  1 file changed, 20 insertions(+), 1 deletion(-)

Re: [PATCH 0/2] rt5651: Enable platforms with int mic on IN2

[Pierre-Louis Bossart]

On 10/18/17 11:07 AM, Carlo Caione wrote:

From: Carlo Caione 

While working on enabling a cherry-trail laptop shipping the rt5651
codec I realized that the machine driver needed some fixup.

In particular the laptop I'm working on (KIANO SlimNote 14.2) has the
internal mic connected to the IN2 port.

All the laptop-specific work with the related quirks will follow.


Nice, thanks for the work. You'll have to rebase and resubmit to fix 
conflicts. I provided changes to simplify DMIC quirks (there is a single 
interface, not 2 on this chip) that aren't reflected in your patches and 
the addition of MCLK support. Mark's for-next branch already took the 
changes this morning.

Also let me know if you want the UCM files to be updated.



Carlo Caione (2):
   ASoC: intel: byt: Enable quirk logic for custom maps
   ASoC: intel: byt: Introduce new custom IN2 map

  sound/soc/intel/boards/bytcr_rt5651.c | 21 -
  1 file changed, 20 insertions(+), 1 deletion(-)

Re: [PATCH] x86, syscalls: use SYSCALL_DEFINE() macros for sys_modify_ldt()

[Ingo Molnar]

* Dave Hansen  wrote:

> 
> We do not have tracepoints for sys_modify_ldt() because we define
> it directly instead of using the normal SYSCALL_DEFINEx() macros.
> 
> However, there is a reason sys_modify_ldt() does not use the macros:
> it has an 'int' return type instead of 'unsigned long'.  This is
> a bug, but it's a bug cemented in the ABI.
> 
> What does this mean?  If we return -EINVAL from a function that
> returns 'int', we have 0xffea in %rax.  But, if we
> return -EINVAL from a function returning 'unsigned long', we end
> up with 0xffea in %rax, which is wrong.
> 
> To work around this and maintain the 'int' behavior while using
> the SYSCALL_DEFINEx() macros, so we add a cast to 'unsigned int'
> in both implementations of sys_modify_ldt().
> 
> Cc: x...@kernel.org
> Cc: Andy Lutomirski 
> Cc: Brian Gerst 
> 
> ---
> 
>  b/arch/x86/include/asm/syscalls.h |2 +-
>  b/arch/x86/kernel/ldt.c   |   16 +---
>  b/arch/x86/um/ldt.c   |6 --
>  3 files changed, 18 insertions(+), 6 deletions(-)

Fails to build on UML:

/home/mingo/tip/arch/x86/um/ldt.c:372:29: error: expected ‘)’ before ‘int’
 SYSCALL_DEFINE3(modify_ldt, int , func , void __user * , ptr ,
 ^
/home/mingo/tip/arch/x86/um/ldt.c:206:13: warning: ‘do_modify_ldt_skas’ defined 
but not used [-Wunused-function]
 static long do_modify_ldt_skas(int func, void __user *ptr,

etc.

Thanks,

Ingo

Re: [PATCH] x86, syscalls: use SYSCALL_DEFINE() macros for sys_modify_ldt()

[Ingo Molnar]

* Dave Hansen  wrote:

> 
> We do not have tracepoints for sys_modify_ldt() because we define
> it directly instead of using the normal SYSCALL_DEFINEx() macros.
> 
> However, there is a reason sys_modify_ldt() does not use the macros:
> it has an 'int' return type instead of 'unsigned long'.  This is
> a bug, but it's a bug cemented in the ABI.
> 
> What does this mean?  If we return -EINVAL from a function that
> returns 'int', we have 0xffea in %rax.  But, if we
> return -EINVAL from a function returning 'unsigned long', we end
> up with 0xffea in %rax, which is wrong.
> 
> To work around this and maintain the 'int' behavior while using
> the SYSCALL_DEFINEx() macros, so we add a cast to 'unsigned int'
> in both implementations of sys_modify_ldt().
> 
> Cc: x...@kernel.org
> Cc: Andy Lutomirski 
> Cc: Brian Gerst 
> 
> ---
> 
>  b/arch/x86/include/asm/syscalls.h |2 +-
>  b/arch/x86/kernel/ldt.c   |   16 +---
>  b/arch/x86/um/ldt.c   |6 --
>  3 files changed, 18 insertions(+), 6 deletions(-)

Fails to build on UML:

/home/mingo/tip/arch/x86/um/ldt.c:372:29: error: expected ‘)’ before ‘int’
 SYSCALL_DEFINE3(modify_ldt, int , func , void __user * , ptr ,
 ^
/home/mingo/tip/arch/x86/um/ldt.c:206:13: warning: ‘do_modify_ldt_skas’ defined 
but not used [-Wunused-function]
 static long do_modify_ldt_skas(int func, void __user *ptr,

etc.

Thanks,

Ingo

Re: [PATCH 1/3] Documentation/devicetree: Add pcie-reset-suspend property

[Bjorn Helgaas]

On Tue, Oct 17, 2017 at 04:39:05PM -0700, Brian Norris wrote:
> On Fri, Oct 13, 2017 at 11:51:52AM -0500, Bjorn Helgaas wrote:
> > On Thu, Oct 12, 2017 at 01:52:18PM -0700, Brian Norris wrote:
> > > The patch is self-descriptive. I've found that we may need
> > > platform-specific behavior for the PERST# signal in system suspend,
> > > depending on the type of PCIe endpoints are attached.
> > > 
> > > Signed-off-by: Brian Norris 
> > > ---
> > >  Documentation/devicetree/bindings/pci/pci.txt | 11 +++
> > >  1 file changed, 11 insertions(+)
> > > 
> > > diff --git a/Documentation/devicetree/bindings/pci/pci.txt 
> > > b/Documentation/devicetree/bindings/pci/pci.txt
> > > index c77981c5dd18..91339b6d0652 100644
> > > --- a/Documentation/devicetree/bindings/pci/pci.txt
> > > +++ b/Documentation/devicetree/bindings/pci/pci.txt
> > > @@ -24,3 +24,14 @@ driver implementation may support the following 
> > > properties:
> > > unsupported link speed, for instance, trying to do training for
> > > unsupported link speed, etc.  Must be '4' for gen4, '3' for gen3, '2'
> > > for gen2, and '1' for gen1. Any other values are invalid.
> > > +- pcie-reset-suspend:
> > > +   If present this property defines whether the PCIe Reset signal 
> > > (referred to
> > > +   as PERST#) should be asserted when the system enters low-power 
> > > suspend modes
> > > +   (e.g., S3). Depending on the form factor, the associated PCIe
> > > +   electromechanical specification may specify a particular behavior 
> > > (e.g.,
> > > +   "PERST# is asserted in advance of the power being switched off in a
> > > +   power-managed state like S3") or it may be less clear. The net result 
> > > is
> > > +   that some endpoints perform better (e.g., lower power consumption) 
> > > with
> > > +   PERST# asserted, and others prefer PERST# deasserted. The value must 
> > > be '0'
> > > +   or '1', where '0' means do not assert PERST# and '1' means assert 
> > > PERST#.
> > > +   When absent, behavior may be platform-specific.
> > 
> > I don't understand this at all.  Are you suggesting that we need
> > different "pcie-reset-suspend" values based on what sort of endpoint
> > the user plugs in?
> 
> Partly. I guess I also failed to mention in this particular text (but I
> did, in patch 3) that it can be a board-specific problem, related to the
> fact that the only endpoint used on said board--soldered on, not
> replaceable--never seemed to require PERST# to be asserted in suspend.
> On such boards, I believe [1] it is physically impossible to drive this
> pin low in S3 suspend. So even if we tried to assert PERST#, it would
> pull back up when we suspend the system. I'm not sure if that's better
> or worse than not asserting it at all.
> 
> So, that's why I settled on a device tree property. It describes the
> physical ability of the board too, in some cases. (I could document this
> better, I see.)

It would make sense to me to have a DT property in the PCIe host
controller object that describes how that controller works, including
its capabilities with respect to PERST#, assuming there's a reasonable
way to use that information.

If there's a DT way to describe a hard-wired PCIe endpoint, it might
make sense to have a second property in the endpoint object that
describes its preferences.  Obviously this couldn't apply to slots
where we don't know what might be plugged in.

> > If you want a quirk to tune this based on specific devices, that might
> > make sense.  It would still sound like an interoperability issue and
> > an ongoing maintenance problem, but at least we would have specific
> > details about which devices are involved, and we'd have a chance to
> > make them work on more controllers than just Rockchip.
> 
> (continued) ...I suppose we could do this too. Like, a new entry in enum
> pci_dev_flags, and code in drivers/pci/quirks.c? And then some helper so
> that a PCIe root port driver like Rockchip's can walk its children and
> check for the existence of this quirk? I'll see if I can write that up
> reasonably.

I'm not sure how this would work out.  I can see the quirk side (e.g.,
the quirk could set a bit in the root port), but who would consume it?
Would every host bridge driver have to look at the bit?  This doesn't
feel like a generic model that works well across vendors.

If devices rely on things not specified by the spec, they will work
better on some systems than on others.  That feels like an issue for
the vendors, not for us.

Obviously you want to tweak something for your particular
configuration, and you can do that either via out-of-tree code or via
some more generic way that I haven't seen yet.

Bjorn

Re: [PATCH 1/3] Documentation/devicetree: Add pcie-reset-suspend property

[Bjorn Helgaas]

On Tue, Oct 17, 2017 at 04:39:05PM -0700, Brian Norris wrote:
> On Fri, Oct 13, 2017 at 11:51:52AM -0500, Bjorn Helgaas wrote:
> > On Thu, Oct 12, 2017 at 01:52:18PM -0700, Brian Norris wrote:
> > > The patch is self-descriptive. I've found that we may need
> > > platform-specific behavior for the PERST# signal in system suspend,
> > > depending on the type of PCIe endpoints are attached.
> > > 
> > > Signed-off-by: Brian Norris 
> > > ---
> > >  Documentation/devicetree/bindings/pci/pci.txt | 11 +++
> > >  1 file changed, 11 insertions(+)
> > > 
> > > diff --git a/Documentation/devicetree/bindings/pci/pci.txt 
> > > b/Documentation/devicetree/bindings/pci/pci.txt
> > > index c77981c5dd18..91339b6d0652 100644
> > > --- a/Documentation/devicetree/bindings/pci/pci.txt
> > > +++ b/Documentation/devicetree/bindings/pci/pci.txt
> > > @@ -24,3 +24,14 @@ driver implementation may support the following 
> > > properties:
> > > unsupported link speed, for instance, trying to do training for
> > > unsupported link speed, etc.  Must be '4' for gen4, '3' for gen3, '2'
> > > for gen2, and '1' for gen1. Any other values are invalid.
> > > +- pcie-reset-suspend:
> > > +   If present this property defines whether the PCIe Reset signal 
> > > (referred to
> > > +   as PERST#) should be asserted when the system enters low-power 
> > > suspend modes
> > > +   (e.g., S3). Depending on the form factor, the associated PCIe
> > > +   electromechanical specification may specify a particular behavior 
> > > (e.g.,
> > > +   "PERST# is asserted in advance of the power being switched off in a
> > > +   power-managed state like S3") or it may be less clear. The net result 
> > > is
> > > +   that some endpoints perform better (e.g., lower power consumption) 
> > > with
> > > +   PERST# asserted, and others prefer PERST# deasserted. The value must 
> > > be '0'
> > > +   or '1', where '0' means do not assert PERST# and '1' means assert 
> > > PERST#.
> > > +   When absent, behavior may be platform-specific.
> > 
> > I don't understand this at all.  Are you suggesting that we need
> > different "pcie-reset-suspend" values based on what sort of endpoint
> > the user plugs in?
> 
> Partly. I guess I also failed to mention in this particular text (but I
> did, in patch 3) that it can be a board-specific problem, related to the
> fact that the only endpoint used on said board--soldered on, not
> replaceable--never seemed to require PERST# to be asserted in suspend.
> On such boards, I believe [1] it is physically impossible to drive this
> pin low in S3 suspend. So even if we tried to assert PERST#, it would
> pull back up when we suspend the system. I'm not sure if that's better
> or worse than not asserting it at all.
> 
> So, that's why I settled on a device tree property. It describes the
> physical ability of the board too, in some cases. (I could document this
> better, I see.)

It would make sense to me to have a DT property in the PCIe host
controller object that describes how that controller works, including
its capabilities with respect to PERST#, assuming there's a reasonable
way to use that information.

If there's a DT way to describe a hard-wired PCIe endpoint, it might
make sense to have a second property in the endpoint object that
describes its preferences.  Obviously this couldn't apply to slots
where we don't know what might be plugged in.

> > If you want a quirk to tune this based on specific devices, that might
> > make sense.  It would still sound like an interoperability issue and
> > an ongoing maintenance problem, but at least we would have specific
> > details about which devices are involved, and we'd have a chance to
> > make them work on more controllers than just Rockchip.
> 
> (continued) ...I suppose we could do this too. Like, a new entry in enum
> pci_dev_flags, and code in drivers/pci/quirks.c? And then some helper so
> that a PCIe root port driver like Rockchip's can walk its children and
> check for the existence of this quirk? I'll see if I can write that up
> reasonably.

I'm not sure how this would work out.  I can see the quirk side (e.g.,
the quirk could set a bit in the root port), but who would consume it?
Would every host bridge driver have to look at the bit?  This doesn't
feel like a generic model that works well across vendors.

If devices rely on things not specified by the spec, they will work
better on some systems than on others.  That feels like an issue for
the vendors, not for us.

Obviously you want to tweak something for your particular
configuration, and you can do that either via out-of-tree code or via
some more generic way that I haven't seen yet.

Bjorn

Re: [PATCH] rcu: do not include rtmutex_common.h unconditionally

[Sebastian Andrzej Siewior]

On 2017-10-18 08:39:46 [-0700], Paul E. McKenney wrote:
> Thank you very much, hand-applied as a preparatory patch for
> "Suppress lockdep false-positive ->boost_mtx complaints", please see
> below.
okay.

> What I don't understand is why 0day test robot didn't complain about
> my copy of the exact same patch.  Or maybe it did and I fat-fingered it?
> Except that I have gotten "BUILD SUCCESS" reports for commits including
> that one.

I don't know. It is a "defconfig" for m32r. Unless it skipped that one,
dunno.

> commit a06f537e75ea0a9e81245ede1b97bb3a5762b81b
> Author: Sebastian Andrzej Siewior 
> Date:   Wed Oct 18 08:33:44 2017 -0700
> 
> rcu: do not include rtmutex_common.h unconditionally
> 
> This commit adjusts include files and provides definitions in preparation
> for suppressing lockdep false-positive ->boost_mtx complaints.  Without
> this preparation, architectures not supporting rt_mutex will get build
> failures.
> 
> Reported-by: kbuild test robot 
> Signed-off-by: Sebastian Andrzej Siewior 
> Signed-off-by: Paul E. McKenney 
> 
> diff --git a/kernel/rcu/tree_plugin.h b/kernel/rcu/tree_plugin.h
> index fed95fa941e6..969eae45f05d 100644
> --- a/kernel/rcu/tree_plugin.h
> +++ b/kernel/rcu/tree_plugin.h
> @@ -54,6 +54,7 @@ DEFINE_PER_CPU(char, rcu_cpu_has_work);
>   * This probably needs to be excluded from -rt builds.
>   */
>  #define rt_mutex_owner(a) ({ WARN_ON_ONCE(1); NULL; })
> +#define rt_mutex_futex_unlock(x) WARN_ON_ONCE(1)
>  
>  #endif /* #else #ifdef CONFIG_RCU_BOOST */
>  
> @@ -911,8 +912,6 @@ void exit_rcu(void)
>  
>  #ifdef CONFIG_RCU_BOOST
>  
> -#include "../locking/rtmutex_common.h"
> -
>  static void rcu_wake_cond(struct task_struct *t, int status)
>  {
>   /*

So this probably works. This is
  
https://git.kernel.org/pub/scm/linux/kernel/git/paulmck/linux-rcu.git/commit/?h=rcu%2Fdev=a06f537e75ea0a9e81245ede1b97bb3a5762b81b=40=0=0

and the rtmutex_common is still in the ifdef which confused me at first.
But then you wrote "preparatory" and I saw the following patch
  
https://git.kernel.org/pub/scm/linux/kernel/git/paulmck/linux-rcu.git/commit/?h=rcu/next=33d7471ce21202ce954993552c2e0298d9e0f031

where you move that include rtmutex_common.h. You shouldn't do that
because "rt_mutex_futex_unlock()" has been added added here for the
!BOOST + TREE case. So I thing this should break your build if you
disable CONFIG_FUTEX (which in turn unselects CONFIg_RT_MUTEX).

Sebastian

Re: [PATCH] rcu: do not include rtmutex_common.h unconditionally

[Sebastian Andrzej Siewior]

On 2017-10-18 08:39:46 [-0700], Paul E. McKenney wrote:
> Thank you very much, hand-applied as a preparatory patch for
> "Suppress lockdep false-positive ->boost_mtx complaints", please see
> below.
okay.

> What I don't understand is why 0day test robot didn't complain about
> my copy of the exact same patch.  Or maybe it did and I fat-fingered it?
> Except that I have gotten "BUILD SUCCESS" reports for commits including
> that one.

I don't know. It is a "defconfig" for m32r. Unless it skipped that one,
dunno.

> commit a06f537e75ea0a9e81245ede1b97bb3a5762b81b
> Author: Sebastian Andrzej Siewior 
> Date:   Wed Oct 18 08:33:44 2017 -0700
> 
> rcu: do not include rtmutex_common.h unconditionally
> 
> This commit adjusts include files and provides definitions in preparation
> for suppressing lockdep false-positive ->boost_mtx complaints.  Without
> this preparation, architectures not supporting rt_mutex will get build
> failures.
> 
> Reported-by: kbuild test robot 
> Signed-off-by: Sebastian Andrzej Siewior 
> Signed-off-by: Paul E. McKenney 
> 
> diff --git a/kernel/rcu/tree_plugin.h b/kernel/rcu/tree_plugin.h
> index fed95fa941e6..969eae45f05d 100644
> --- a/kernel/rcu/tree_plugin.h
> +++ b/kernel/rcu/tree_plugin.h
> @@ -54,6 +54,7 @@ DEFINE_PER_CPU(char, rcu_cpu_has_work);
>   * This probably needs to be excluded from -rt builds.
>   */
>  #define rt_mutex_owner(a) ({ WARN_ON_ONCE(1); NULL; })
> +#define rt_mutex_futex_unlock(x) WARN_ON_ONCE(1)
>  
>  #endif /* #else #ifdef CONFIG_RCU_BOOST */
>  
> @@ -911,8 +912,6 @@ void exit_rcu(void)
>  
>  #ifdef CONFIG_RCU_BOOST
>  
> -#include "../locking/rtmutex_common.h"
> -
>  static void rcu_wake_cond(struct task_struct *t, int status)
>  {
>   /*

So this probably works. This is
  
https://git.kernel.org/pub/scm/linux/kernel/git/paulmck/linux-rcu.git/commit/?h=rcu%2Fdev=a06f537e75ea0a9e81245ede1b97bb3a5762b81b=40=0=0

and the rtmutex_common is still in the ifdef which confused me at first.
But then you wrote "preparatory" and I saw the following patch
  
https://git.kernel.org/pub/scm/linux/kernel/git/paulmck/linux-rcu.git/commit/?h=rcu/next=33d7471ce21202ce954993552c2e0298d9e0f031

where you move that include rtmutex_common.h. You shouldn't do that
because "rt_mutex_futex_unlock()" has been added added here for the
!BOOST + TREE case. So I thing this should break your build if you
disable CONFIG_FUTEX (which in turn unselects CONFIg_RT_MUTEX).

Sebastian

Re: [PATCH v3 2/2] livepatch: add atomic replace

[Josh Poimboeuf]

On Wed, Oct 18, 2017 at 03:36:42PM +0200, Jiri Kosina wrote:
> On Wed, 18 Oct 2017, Miroslav Benes wrote:
> 
> > 3. Drop immediate. It causes problems only and its advantages on x86_64 
> > are theoretical. You would still need to solve the interaction with atomic 
> > replace on other architecture with immediate preserved, but that may be 
> > easier. Or we can be aggressive and drop immediate completely. The force 
> > transition I proposed earlier could achieve the same.
> 
> After brief off-thread discussion, I've been thinking about this a bit 
> more and I also think that we should claim immediate "an experiment that 
> failed", especially as the force functionality (which provides equal 
> functionality from the userspace POV) will likely be there sonnish.

Agreed.

To clarify, we'll need the force patch before removing
klp_patch.immediate, so we don't break non-x86 arches in the meantime.

On the other hand I think we could remove klp_func.immediate
immediately.


While we're on the subject of removing code... :-)


I've mentioned this several times before, but the more features we add,
the more obvious this point becomes: if we could figure out how to get
rid of the "patching unloaded modules" feature, the code would be so
much better, and I'd actually be able to fit the code in my brain.  Then
we could get rid of all these sneaky bugs that Miroslav and Petr keep
finding, and I wouldn't get an uneasy feeling everytime somebody posts a
new feature.

Here's one vague idea for how to achieve that.  More ideas welcome.

1) Make the consistency model synchronous with respect to modules: don't
   allow any modules to load or unload until the patch transition is
   complete.

2) Instead of one big uber patch module which patches vmlinux and
   modules at the same time, make each patch module specific to a single
   object.  Then bundle the patch modules together somehow into a "patch
   bundle" so they're treated as a single atomic unit.

3) The patch bundle, when loaded, would load some of its patch modules
   immediately (for those objects which are already loaded).  For
   unloaded objects, the corresponding patch modules will be copied into
   memory but not loaded.

4) Then, when a to-be-patched module is loaded, the module loader loads
   it into memory and relocates it, but doesn't make it live.  Then it
   loads the patch module from the memory blob, makes the patch module
   live, and then makes the to-be-patched module live.

(A variation would be to create a way for user space to load a module in
the paused state.  Then user space can handle the dependencies and do
the patch juggling.  I think that would mean depmod/modprobe would need
to be involved.)

It could be a terrible idea, though it might be worth looking into.

-- 
Josh

Re: [PATCH v3 2/2] livepatch: add atomic replace

[Josh Poimboeuf]

On Wed, Oct 18, 2017 at 03:36:42PM +0200, Jiri Kosina wrote:
> On Wed, 18 Oct 2017, Miroslav Benes wrote:
> 
> > 3. Drop immediate. It causes problems only and its advantages on x86_64 
> > are theoretical. You would still need to solve the interaction with atomic 
> > replace on other architecture with immediate preserved, but that may be 
> > easier. Or we can be aggressive and drop immediate completely. The force 
> > transition I proposed earlier could achieve the same.
> 
> After brief off-thread discussion, I've been thinking about this a bit 
> more and I also think that we should claim immediate "an experiment that 
> failed", especially as the force functionality (which provides equal 
> functionality from the userspace POV) will likely be there sonnish.

Agreed.

To clarify, we'll need the force patch before removing
klp_patch.immediate, so we don't break non-x86 arches in the meantime.

On the other hand I think we could remove klp_func.immediate
immediately.


While we're on the subject of removing code... :-)


I've mentioned this several times before, but the more features we add,
the more obvious this point becomes: if we could figure out how to get
rid of the "patching unloaded modules" feature, the code would be so
much better, and I'd actually be able to fit the code in my brain.  Then
we could get rid of all these sneaky bugs that Miroslav and Petr keep
finding, and I wouldn't get an uneasy feeling everytime somebody posts a
new feature.

Here's one vague idea for how to achieve that.  More ideas welcome.

1) Make the consistency model synchronous with respect to modules: don't
   allow any modules to load or unload until the patch transition is
   complete.

2) Instead of one big uber patch module which patches vmlinux and
   modules at the same time, make each patch module specific to a single
   object.  Then bundle the patch modules together somehow into a "patch
   bundle" so they're treated as a single atomic unit.

3) The patch bundle, when loaded, would load some of its patch modules
   immediately (for those objects which are already loaded).  For
   unloaded objects, the corresponding patch modules will be copied into
   memory but not loaded.

4) Then, when a to-be-patched module is loaded, the module loader loads
   it into memory and relocates it, but doesn't make it live.  Then it
   loads the patch module from the memory blob, makes the patch module
   live, and then makes the to-be-patched module live.

(A variation would be to create a way for user space to load a module in
the paused state.  Then user space can handle the dependencies and do
the patch juggling.  I think that would mean depmod/modprobe would need
to be involved.)

It could be a terrible idea, though it might be worth looking into.

-- 
Josh

Re: libbattery was Re: [RFC PATCH 5/5] power: generic-adc-battery: Add capacity handling

[Pavel Machek]

On Wed 2017-10-18 17:52:22, H. Nikolaus Schaller wrote:
> 
> > Am 18.10.2017 um 15:56 schrieb Pavel Machek :
> > 
> > On Wed 2017-10-18 06:22:04, Tony Lindgren wrote:
> >> * H. Nikolaus Schaller  [171018 05:49]:
>  Am 18.10.2017 um 14:28 schrieb Pavel Machek :
>  
>  So I started something, it is at.
>  
>  https://github.com/pavelmachek/libbattery
>  
>  My battery on n900 is currently uncalibrated (and charging), still it
>  gets some kind of estimation:
>  
>  Battery -1 %
>  Seconds -1
>  State 1
>  Voltage 3.88 V
>  Battery 63 %
>  
>  Of course, there's a lot more work to be done.
> >>> 
> >>> Nice start but not a solution to our problem.
> >>> 
> >>> Our problem is that people simply expect that for example 
> >>> https://packages.debian.org/wheezy/xfce/xfce4-battery-plugin
> >>> displays the battery percentage.
> >> 
> >> I think we could make things compatible with various battery apps by
> >> having libbattery write back the capacity percentage and time remaining
> >> to the kernel driver via sysfs or a dev entry. Then the kernel interface
> >> can just display the data to whatever apps.
> > 
> > Hmm. This could be as simple as providing symlink from
> > /sys/class/power/userland-battery to some place writable by
> > userspace...
> 
> Well, please implement arbitrary symlinks in /sysfs...

Any reason why I'd like to do that?
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: libbattery was Re: [RFC PATCH 5/5] power: generic-adc-battery: Add capacity handling

[Pavel Machek]

On Wed 2017-10-18 17:52:22, H. Nikolaus Schaller wrote:
> 
> > Am 18.10.2017 um 15:56 schrieb Pavel Machek :
> > 
> > On Wed 2017-10-18 06:22:04, Tony Lindgren wrote:
> >> * H. Nikolaus Schaller  [171018 05:49]:
>  Am 18.10.2017 um 14:28 schrieb Pavel Machek :
>  
>  So I started something, it is at.
>  
>  https://github.com/pavelmachek/libbattery
>  
>  My battery on n900 is currently uncalibrated (and charging), still it
>  gets some kind of estimation:
>  
>  Battery -1 %
>  Seconds -1
>  State 1
>  Voltage 3.88 V
>  Battery 63 %
>  
>  Of course, there's a lot more work to be done.
> >>> 
> >>> Nice start but not a solution to our problem.
> >>> 
> >>> Our problem is that people simply expect that for example 
> >>> https://packages.debian.org/wheezy/xfce/xfce4-battery-plugin
> >>> displays the battery percentage.
> >> 
> >> I think we could make things compatible with various battery apps by
> >> having libbattery write back the capacity percentage and time remaining
> >> to the kernel driver via sysfs or a dev entry. Then the kernel interface
> >> can just display the data to whatever apps.
> > 
> > Hmm. This could be as simple as providing symlink from
> > /sys/class/power/userland-battery to some place writable by
> > userspace...
> 
> Well, please implement arbitrary symlinks in /sysfs...

Any reason why I'd like to do that?
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