Re: [PATCH v2] fs, elf: don't complain MAP_FIXED_NOREPLACE unless -EEXIST error.

[Michal Hocko]

On Wed 18-04-18 23:07:12, Tetsuo Handa wrote:
> >From 3f396857d23d4bf1fac4d4332316b5ba0af6d2f9 Mon Sep 17 00:00:00 2001
> From: Tetsuo Handa 
> Date: Wed, 18 Apr 2018 23:00:53 +0900
> Subject: [PATCH v2] fs, elf: don't complain MAP_FIXED_NOREPLACE unless 
> -EEXIST error.
> 
> Commit 4ed28639519c7bad ("fs, elf: drop MAP_FIXED usage from elf_map") is
> printing spurious messages under memory pressure due to map_addr == -ENOMEM.
> 
>  9794 (a.out): Uhuuh, elf segment at 7f2e34738000(fff4) 
> requested but the memory is mapped already
>  14104 (a.out): Uhuuh, elf segment at 7f34fd76c000(fff4) 
> requested but the memory is mapped already
>  16843 (a.out): Uhuuh, elf segment at 7f930ecc7000(fff4) 
> requested but the memory is mapped already
> 
> Complain only if -EEXIST, and use %px for printing the address.
> 
> Signed-off-by: Tetsuo Handa 
> Acked-by: Michal Hocko 
> Cc: Andrei Vagin 
> Cc: Khalid Aziz 
> Cc: Michael Ellerman 
> Cc: Kees Cook 
> Cc: Abdul Haleem 
> Cc: Joel Stanley 
> Cc: Anshuman Khandual 

Thanks!

> ---
>  fs/binfmt_elf.c | 8 
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
> index 41e0418..4ad6f66 100644
> --- a/fs/binfmt_elf.c
> +++ b/fs/binfmt_elf.c
> @@ -377,10 +377,10 @@ static unsigned long elf_map(struct file *filep, 
> unsigned long addr,
>   } else
>   map_addr = vm_mmap(filep, addr, size, prot, type, off);
>  
> - if ((type & MAP_FIXED_NOREPLACE) && BAD_ADDR(map_addr))
> - pr_info("%d (%s): Uhuuh, elf segment at %p requested but the 
> memory is mapped already\n",
> - task_pid_nr(current), current->comm,
> - (void *)addr);
> + if ((type & MAP_FIXED_NOREPLACE) &&
> + PTR_ERR((void *)map_addr) == -EEXIST)
> + pr_info("%d (%s): Uhuuh, elf segment at %px requested but the 
> memory is mapped already\n",
> + task_pid_nr(current), current->comm, (void *)addr);
>  
>   return(map_addr);
>  }
> -- 
> 1.8.3.1

-- 
Michal Hocko
SUSE Labs

Re: [PATCH v2] fs, elf: don't complain MAP_FIXED_NOREPLACE unless -EEXIST error.

[Michal Hocko]

On Wed 18-04-18 23:07:12, Tetsuo Handa wrote:
> >From 3f396857d23d4bf1fac4d4332316b5ba0af6d2f9 Mon Sep 17 00:00:00 2001
> From: Tetsuo Handa 
> Date: Wed, 18 Apr 2018 23:00:53 +0900
> Subject: [PATCH v2] fs, elf: don't complain MAP_FIXED_NOREPLACE unless 
> -EEXIST error.
> 
> Commit 4ed28639519c7bad ("fs, elf: drop MAP_FIXED usage from elf_map") is
> printing spurious messages under memory pressure due to map_addr == -ENOMEM.
> 
>  9794 (a.out): Uhuuh, elf segment at 7f2e34738000(fff4) 
> requested but the memory is mapped already
>  14104 (a.out): Uhuuh, elf segment at 7f34fd76c000(fff4) 
> requested but the memory is mapped already
>  16843 (a.out): Uhuuh, elf segment at 7f930ecc7000(fff4) 
> requested but the memory is mapped already
> 
> Complain only if -EEXIST, and use %px for printing the address.
> 
> Signed-off-by: Tetsuo Handa 
> Acked-by: Michal Hocko 
> Cc: Andrei Vagin 
> Cc: Khalid Aziz 
> Cc: Michael Ellerman 
> Cc: Kees Cook 
> Cc: Abdul Haleem 
> Cc: Joel Stanley 
> Cc: Anshuman Khandual 

Thanks!

> ---
>  fs/binfmt_elf.c | 8 
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
> index 41e0418..4ad6f66 100644
> --- a/fs/binfmt_elf.c
> +++ b/fs/binfmt_elf.c
> @@ -377,10 +377,10 @@ static unsigned long elf_map(struct file *filep, 
> unsigned long addr,
>   } else
>   map_addr = vm_mmap(filep, addr, size, prot, type, off);
>  
> - if ((type & MAP_FIXED_NOREPLACE) && BAD_ADDR(map_addr))
> - pr_info("%d (%s): Uhuuh, elf segment at %p requested but the 
> memory is mapped already\n",
> - task_pid_nr(current), current->comm,
> - (void *)addr);
> + if ((type & MAP_FIXED_NOREPLACE) &&
> + PTR_ERR((void *)map_addr) == -EEXIST)
> + pr_info("%d (%s): Uhuuh, elf segment at %px requested but the 
> memory is mapped already\n",
> + task_pid_nr(current), current->comm, (void *)addr);
>  
>   return(map_addr);
>  }
> -- 
> 1.8.3.1

-- 
Michal Hocko
SUSE Labs

Re: WARNING: stack going in the wrong direction? ip=__schedule+0x489/0x830

[Fengguang Wu]

On Thu, Apr 19, 2018 at 01:49:41PM +0800, Fengguang Wu wrote:

Hello,

FYI this warning dates back to v4.16-rc5 .



It's rather rare and often happen together with other errors.


Sorry, that should be 0day didn't catch this particular WARNING.
So it just occasionally show up in the context of other errors.

I jut added that WARNING pattern to 0day and hope we can get more
information about it.

Thanks,
Fengguang

Re: WARNING: stack going in the wrong direction? ip=__schedule+0x489/0x830

[Fengguang Wu]

On Thu, Apr 19, 2018 at 01:49:41PM +0800, Fengguang Wu wrote:

Hello,

FYI this warning dates back to v4.16-rc5 .



It's rather rare and often happen together with other errors.


Sorry, that should be 0day didn't catch this particular WARNING.
So it just occasionally show up in the context of other errors.

I jut added that WARNING pattern to 0day and hope we can get more
information about it.

Thanks,
Fengguang

Re: [PATCH 1/2] regulator: add QCOM RPMh regulator driver

[Stephen Boyd]

Quoting David Collins (2018-03-22 18:30:06)
> On 03/21/2018 12:07 PM, Stephen Boyd wrote:
> > Quoting David Collins (2018-03-16 18:09:10)
> >> diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
> >> index 097f617..e0ecd0a 100644
> >> --- a/drivers/regulator/Kconfig
> >> +++ b/drivers/regulator/Kconfig
> >> @@ -671,6 +671,15 @@ config REGULATOR_QCOM_RPM
> >>   Qualcomm RPM as a module. The module will be named
> >>   "qcom_rpm-regulator".
> >>  
> >> +config REGULATOR_QCOM_RPMH
> >> +   tristate "Qualcomm Technologies, Inc. RPMh regulator driver"
> >> +   depends on (QCOM_RPMH && QCOM_COMMAND_DB && OF) || COMPILE_TEST
> > 
> > What's the build dependency on OF?
> 
> The qcom_rpmh-regulator driver cannot function without device tree support
> enabled.  I suppose that it might be able to compile, but it wouldn't be
> useful.

If it isn't a build dependency then we usually leave it out because it's
not always useful to express runtime constraints in Kconfig. Probably
all we need is depends on QCOM_RPMH || COMPILE_TEST and then stubs take
care of it from there.

> 
> >> + * struct rpmh_vreg_init_data - initialization data for an RPMh regulator
> >> + * @name:  Name for the regulator which also 
> >> corresponds
> >> + * to the device tree subnode name of the 
> >> regulator
> >> + * @resource_name_base:RPMh regulator resource name 
> >> prefix.  E.g.
> >> + * "ldo" for RPMh resource "ldoa1".
> > 
> > Maybe it should be "ldo%c1"? Then we could kasprintf the name with the
> > pmic_id and drop the 'id' member entirely.
> 
> I can make this modification (though with %s instead of %c for simplicity
> with DT string parsing).  Hopefully having a variable format string
> doesn't trigger any static analysis tools.

Oh it's variable how many digits in the 'id' number there are? I'll look
at v2.

> 
> 
> >> +   int vreg_count;
> >> +   const char  *pmic_id;
> >> +   const struct rpmh_pmic_init_data*init_data;
> > 
> > Hopefully we don't really need this entire struct and we can just use
> > local variables instead.
> 
> Outside of probe-time, this is used by struct rpmh_vreg in order to access
> rpmh_client (for RPMh transactions) and pmic->init_data->name (for debug
> and error messages).  I suppose that rpmh_client could be specified in
> struct rpmh_vreg directly.
> 

Hopefully rpm_client goes away. Maybe it would just be dev pointer to
hold onto then and possibly rip the name of each regulator out of the
framework name for it?

> 
> 
> >> +/**
> >> + * rpmh_regulator_parse_vrm_modes() - parse the supported mode 
> >> configurations
> >> + * for a VRM RPMh resource from device tree
> >> + * vreg:   Pointer to the rpmh regulator resource
> >> + *
> >> + * This function initializes the mode[] array of vreg based upon the 
> >> values
> >> + * of optional device tree properties.
> >> + *
> >> + * Return: 0 on success, errno on failure
> >> + */
> >> +static int rpmh_regulator_parse_vrm_modes(struct rpmh_vreg *vreg)
> >> +{
> > 
> > I have a feeling this should all come from the driver data, not DT.
> > Doubtful this really changes for each board.
> 
> This needs to be determined at a board level instead of hard-coded per
> regulator type.  For LDOs switching between LPM and HPM typically happens
> at 10 mA or 30 mA per hardware documentation.  Unfortunately, sharing
> control of regulators with other processors adds some subtlety.
> 
> Consider the case of a regulator with 10 mA LPM/HPM threshold physically.
> Say that modem and application processors each have a load on the
> regulator that draws 9 mA.  If they each respect the 10 mA threshold, then
> they'd each vote for LPM.  VRM will aggregate these requests together
> which will result in the regulator being set to LPM even though the total
> load is 18 mA (which would require HPM).  To get around this corner case,
> a threshold of 1 uA can be used for all supplies that have non-application
> processor consumers.  Thus, any non-zero current request will result in
> setting the regulator to HPM (which is always safe).

Sad. Sounds like the rpmh interface is broken and should be expressing
the load instead of the mode so that aggregation on the rpm side can
pick the right mode. So now we have a workaround by specifying some
default minimum value that we add in?

> 
> Another example is SMPS regulators which should theoretically always be
> able to operate in AUTO mode.  However, there may be board/system issues
> that require switching to PWM mode (HPM) for certain use cases as it has
> better load regulation (even though it can source the same amount of
> current as AUTO mode).  If there is more than one consumer that requires
> this ability for a given regulator, then regulator_set_load() is the only
> option since it provides 

Re: [PATCH 1/2] regulator: add QCOM RPMh regulator driver

[Stephen Boyd]

Quoting David Collins (2018-03-22 18:30:06)
> On 03/21/2018 12:07 PM, Stephen Boyd wrote:
> > Quoting David Collins (2018-03-16 18:09:10)
> >> diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
> >> index 097f617..e0ecd0a 100644
> >> --- a/drivers/regulator/Kconfig
> >> +++ b/drivers/regulator/Kconfig
> >> @@ -671,6 +671,15 @@ config REGULATOR_QCOM_RPM
> >>   Qualcomm RPM as a module. The module will be named
> >>   "qcom_rpm-regulator".
> >>  
> >> +config REGULATOR_QCOM_RPMH
> >> +   tristate "Qualcomm Technologies, Inc. RPMh regulator driver"
> >> +   depends on (QCOM_RPMH && QCOM_COMMAND_DB && OF) || COMPILE_TEST
> > 
> > What's the build dependency on OF?
> 
> The qcom_rpmh-regulator driver cannot function without device tree support
> enabled.  I suppose that it might be able to compile, but it wouldn't be
> useful.

If it isn't a build dependency then we usually leave it out because it's
not always useful to express runtime constraints in Kconfig. Probably
all we need is depends on QCOM_RPMH || COMPILE_TEST and then stubs take
care of it from there.

> 
> >> + * struct rpmh_vreg_init_data - initialization data for an RPMh regulator
> >> + * @name:  Name for the regulator which also 
> >> corresponds
> >> + * to the device tree subnode name of the 
> >> regulator
> >> + * @resource_name_base:RPMh regulator resource name 
> >> prefix.  E.g.
> >> + * "ldo" for RPMh resource "ldoa1".
> > 
> > Maybe it should be "ldo%c1"? Then we could kasprintf the name with the
> > pmic_id and drop the 'id' member entirely.
> 
> I can make this modification (though with %s instead of %c for simplicity
> with DT string parsing).  Hopefully having a variable format string
> doesn't trigger any static analysis tools.

Oh it's variable how many digits in the 'id' number there are? I'll look
at v2.

> 
> 
> >> +   int vreg_count;
> >> +   const char  *pmic_id;
> >> +   const struct rpmh_pmic_init_data*init_data;
> > 
> > Hopefully we don't really need this entire struct and we can just use
> > local variables instead.
> 
> Outside of probe-time, this is used by struct rpmh_vreg in order to access
> rpmh_client (for RPMh transactions) and pmic->init_data->name (for debug
> and error messages).  I suppose that rpmh_client could be specified in
> struct rpmh_vreg directly.
> 

Hopefully rpm_client goes away. Maybe it would just be dev pointer to
hold onto then and possibly rip the name of each regulator out of the
framework name for it?

> 
> 
> >> +/**
> >> + * rpmh_regulator_parse_vrm_modes() - parse the supported mode 
> >> configurations
> >> + * for a VRM RPMh resource from device tree
> >> + * vreg:   Pointer to the rpmh regulator resource
> >> + *
> >> + * This function initializes the mode[] array of vreg based upon the 
> >> values
> >> + * of optional device tree properties.
> >> + *
> >> + * Return: 0 on success, errno on failure
> >> + */
> >> +static int rpmh_regulator_parse_vrm_modes(struct rpmh_vreg *vreg)
> >> +{
> > 
> > I have a feeling this should all come from the driver data, not DT.
> > Doubtful this really changes for each board.
> 
> This needs to be determined at a board level instead of hard-coded per
> regulator type.  For LDOs switching between LPM and HPM typically happens
> at 10 mA or 30 mA per hardware documentation.  Unfortunately, sharing
> control of regulators with other processors adds some subtlety.
> 
> Consider the case of a regulator with 10 mA LPM/HPM threshold physically.
> Say that modem and application processors each have a load on the
> regulator that draws 9 mA.  If they each respect the 10 mA threshold, then
> they'd each vote for LPM.  VRM will aggregate these requests together
> which will result in the regulator being set to LPM even though the total
> load is 18 mA (which would require HPM).  To get around this corner case,
> a threshold of 1 uA can be used for all supplies that have non-application
> processor consumers.  Thus, any non-zero current request will result in
> setting the regulator to HPM (which is always safe).

Sad. Sounds like the rpmh interface is broken and should be expressing
the load instead of the mode so that aggregation on the rpm side can
pick the right mode. So now we have a workaround by specifying some
default minimum value that we add in?

> 
> Another example is SMPS regulators which should theoretically always be
> able to operate in AUTO mode.  However, there may be board/system issues
> that require switching to PWM mode (HPM) for certain use cases as it has
> better load regulation (even though it can source the same amount of
> current as AUTO mode).  If there is more than one consumer that requires
> this ability for a given regulator, then regulator_set_load() is the only
> option since it provides 

Re: [RFC v4 3/4] irqflags: Avoid unnecessary calls to trace_ if you can

[Namhyung Kim]

On Wed, Apr 18, 2018 at 06:02:50PM +0900, Masami Hiramatsu wrote:
> On Mon, 16 Apr 2018 21:07:47 -0700
> Joel Fernandes  wrote:
> 
> > With TRACE_IRQFLAGS, we call trace_ API too many times. We don't need
> > to if local_irq_restore or local_irq_save didn't actually do anything.
> > 
> > This gives around a 4% improvement in performance when doing the
> > following command: "time find / > /dev/null"
> > 
> > Also its best to avoid these calls where possible, since in this series,
> > the RCU code in tracepoint.h seems to be call these quite a bit and I'd
> > like to keep this overhead low.
> 
> Can we assume that the "flags" has only 1 bit irq-disable flag?
> Since it skips calling raw_local_irq_restore(flags); too,

I don't know how many it impacts on performance but maybe we can have
an arch-specific config option something like below?


> if there is any state in the flags on any arch, it may change the
> result. In that case, we can do it as below (just skipping trace_hardirqs_*)
> 
> int disabled = irqs_disabled();

  if (disabled == raw_irqs_disabled_flags(flags)) {
#ifndef CONFIG_ARCH_CAN_SKIP_NESTED_IRQ_RESTORE
raw_local_irq_restore(flags);
#endif
return;
  }

> 
> if (!raw_irqs_disabled_flags(flags) && disabled)
>   trace_hardirqs_on();
> 
> raw_local_irq_restore(flags);
> 
> if (raw_irqs_disabled_flags(flags) && !disabled)
>   trace_hardirqs_off();

Thanks,
Namhyung

Re: [RFC v4 3/4] irqflags: Avoid unnecessary calls to trace_ if you can

[Namhyung Kim]

On Wed, Apr 18, 2018 at 06:02:50PM +0900, Masami Hiramatsu wrote:
> On Mon, 16 Apr 2018 21:07:47 -0700
> Joel Fernandes  wrote:
> 
> > With TRACE_IRQFLAGS, we call trace_ API too many times. We don't need
> > to if local_irq_restore or local_irq_save didn't actually do anything.
> > 
> > This gives around a 4% improvement in performance when doing the
> > following command: "time find / > /dev/null"
> > 
> > Also its best to avoid these calls where possible, since in this series,
> > the RCU code in tracepoint.h seems to be call these quite a bit and I'd
> > like to keep this overhead low.
> 
> Can we assume that the "flags" has only 1 bit irq-disable flag?
> Since it skips calling raw_local_irq_restore(flags); too,

I don't know how many it impacts on performance but maybe we can have
an arch-specific config option something like below?


> if there is any state in the flags on any arch, it may change the
> result. In that case, we can do it as below (just skipping trace_hardirqs_*)
> 
> int disabled = irqs_disabled();

  if (disabled == raw_irqs_disabled_flags(flags)) {
#ifndef CONFIG_ARCH_CAN_SKIP_NESTED_IRQ_RESTORE
raw_local_irq_restore(flags);
#endif
return;
  }

> 
> if (!raw_irqs_disabled_flags(flags) && disabled)
>   trace_hardirqs_on();
> 
> raw_local_irq_restore(flags);
> 
> if (raw_irqs_disabled_flags(flags) && !disabled)
>   trace_hardirqs_off();

Thanks,
Namhyung

Re: cpu stopper threads and load balancing leads to deadlock

[Mike Galbraith]

On Wed, 2018-04-18 at 07:47 +0200, Mike Galbraith wrote:
> On Tue, 2018-04-17 at 15:21 +0100, Matt Fleming wrote:
> > Hi guys,
> > 
> > We've seen a bug in one of our SLE kernels where the cpu stopper
> > thread ("migration/15") is entering idle balance. This then triggers
> > active load balance.
> > 
> > At the same time, a task on another CPU triggers a page fault and NUMA
> > balancing kicks in to try and migrate the task closer to the NUMA node
> > for that page (we're inside stop_two_cpus()). This faulting task is
> > spinning in try_to_wake_up() (inside smp_cond_load_acquire(>on_cpu,
> > !VAL)), waiting for "migration/15" to context switch.
> > 
> > Unfortunately, because "migration/15" is doing active load balance
> > it's spinning waiting for the NUMA-page-faulting CPU's stopper lock,
> > which is already held (since it's inside stop_two_cpus()).
> > 
> > Deadlock ensues.
> > 
> > This seems like a situation that should be prohibited, but I cannot
> > find any code to prevent it. Is it OK for stopper threads to load
> > balance? Is there something that should prevent this situation from
> > happening?
> 
> I don't see anything to stop the deadlock either, would exclude stop
> class from playing idle balancer entirely...

Bah, insufficient: __do_softirq() -> rebalance_domains() still bites.

Re: cpu stopper threads and load balancing leads to deadlock

[Mike Galbraith]

On Wed, 2018-04-18 at 07:47 +0200, Mike Galbraith wrote:
> On Tue, 2018-04-17 at 15:21 +0100, Matt Fleming wrote:
> > Hi guys,
> > 
> > We've seen a bug in one of our SLE kernels where the cpu stopper
> > thread ("migration/15") is entering idle balance. This then triggers
> > active load balance.
> > 
> > At the same time, a task on another CPU triggers a page fault and NUMA
> > balancing kicks in to try and migrate the task closer to the NUMA node
> > for that page (we're inside stop_two_cpus()). This faulting task is
> > spinning in try_to_wake_up() (inside smp_cond_load_acquire(>on_cpu,
> > !VAL)), waiting for "migration/15" to context switch.
> > 
> > Unfortunately, because "migration/15" is doing active load balance
> > it's spinning waiting for the NUMA-page-faulting CPU's stopper lock,
> > which is already held (since it's inside stop_two_cpus()).
> > 
> > Deadlock ensues.
> > 
> > This seems like a situation that should be prohibited, but I cannot
> > find any code to prevent it. Is it OK for stopper threads to load
> > balance? Is there something that should prevent this situation from
> > happening?
> 
> I don't see anything to stop the deadlock either, would exclude stop
> class from playing idle balancer entirely...

Bah, insufficient: __do_softirq() -> rebalance_domains() still bites.

Re: 4.15.17 regression: bisected: timeout during microcode update

[Vitezslav Samel]

On Wed, Apr 18, 2018 at 06:53:30AM -0700, Raj, Ashok wrote:
> On Wed, Apr 18, 2018 at 02:22:12PM +0200, Borislav Petkov wrote:
> > On Wed, Apr 18, 2018 at 02:08:40PM +0200, Vitezslav Samel wrote:
> > > I switched to firmware-in-kernel early loading and that works OK.
> 
> firmware-in-kernel means you compile your microcode image in linux?

  Yes:

CONFIG_FW_LOADER=y
CONFIG_EXTRA_FIRMWARE="intel-ucode/06-3c-03"
CONFIG_EXTRA_FIRMWARE_DIR="/lib/firmware"

> Can you tell me which distro you are using? The ones we used 
> doesn't do late-loading (i.e echo 1 > 
> /sys/devices/system/cpu/microcode/reload)
> 
> I suspect that might be the problem. 

  Yes, this is it. I followed instructions included in current microcode
package downloaded from intel website and added "echo 1 > /sys/dev..."
to my Slackware's /etc/rc.d/rc.local.

> - Can you remove your builtin microcode, 
> - rename the /lib/firmware/intel-ucode so we don't find it during late 
> loading. 
> - let the system boot completely
> - then rename the intel-ucode back for this test.
> - write 1 to reload and see if that update succeeds or fails?

  Just tested, it fails.

Cheers,

Vita

> > Ok, and keep using that from now on.
> > 
> > People should all move away from that late loading dance. I'm saying
> > that in case someone else reads this on lkml.
> > 
> > > But still, the reported issue is regression in 4.15.17 and 4.16+.
> > 
> > Oh, I know it is a regression.
> > 
> > @Ashok: anything particular about his microcode revision not being able
> > to stomach late loading?
> 
> nothing about the microcode itself comes to mind. I'm wondering if this 
> similar to the Arch linux that used late-load during booting might be an 
> issue.

Re: 4.15.17 regression: bisected: timeout during microcode update

[Vitezslav Samel]

On Wed, Apr 18, 2018 at 06:53:30AM -0700, Raj, Ashok wrote:
> On Wed, Apr 18, 2018 at 02:22:12PM +0200, Borislav Petkov wrote:
> > On Wed, Apr 18, 2018 at 02:08:40PM +0200, Vitezslav Samel wrote:
> > > I switched to firmware-in-kernel early loading and that works OK.
> 
> firmware-in-kernel means you compile your microcode image in linux?

  Yes:

CONFIG_FW_LOADER=y
CONFIG_EXTRA_FIRMWARE="intel-ucode/06-3c-03"
CONFIG_EXTRA_FIRMWARE_DIR="/lib/firmware"

> Can you tell me which distro you are using? The ones we used 
> doesn't do late-loading (i.e echo 1 > 
> /sys/devices/system/cpu/microcode/reload)
> 
> I suspect that might be the problem. 

  Yes, this is it. I followed instructions included in current microcode
package downloaded from intel website and added "echo 1 > /sys/dev..."
to my Slackware's /etc/rc.d/rc.local.

> - Can you remove your builtin microcode, 
> - rename the /lib/firmware/intel-ucode so we don't find it during late 
> loading. 
> - let the system boot completely
> - then rename the intel-ucode back for this test.
> - write 1 to reload and see if that update succeeds or fails?

  Just tested, it fails.

Cheers,

Vita

> > Ok, and keep using that from now on.
> > 
> > People should all move away from that late loading dance. I'm saying
> > that in case someone else reads this on lkml.
> > 
> > > But still, the reported issue is regression in 4.15.17 and 4.16+.
> > 
> > Oh, I know it is a regression.
> > 
> > @Ashok: anything particular about his microcode revision not being able
> > to stomach late loading?
> 
> nothing about the microcode itself comes to mind. I'm wondering if this 
> similar to the Arch linux that used late-load during booting might be an 
> issue.

Re: [PATCH bpf-next 4/5] samples/bpf: Refine printing symbol for sampleip

[Leo Yan]

On Wed, Apr 18, 2018 at 10:21:25PM -0700, Alexei Starovoitov wrote:
> On Thu, Apr 19, 2018 at 01:12:49PM +0800, Leo Yan wrote:
> > On Wed, Apr 18, 2018 at 09:47:45PM -0700, Alexei Starovoitov wrote:
> > > On Thu, Apr 19, 2018 at 09:34:05AM +0800, Leo Yan wrote:
> > > > The code defines macro 'PAGE_OFFSET' and uses it to decide if the
> > > > address is in kernel space or not.  But different architecture has
> > > > different 'PAGE_OFFSET' so this program cannot be used for all
> > > > platforms.
> > > > 
> > > > This commit changes to check returned pointer from ksym_search() to
> > > > judge if the address falls into kernel space or not, and removes
> > > > macro 'PAGE_OFFSET' as it isn't used anymore.  As result, this program
> > > > has no architecture dependency.
> > > > 
> > > > Signed-off-by: Leo Yan 
> > > > ---
> > > >  samples/bpf/sampleip_user.c | 8 +++-
> > > >  1 file changed, 3 insertions(+), 5 deletions(-)
> > > > 
> > > > diff --git a/samples/bpf/sampleip_user.c b/samples/bpf/sampleip_user.c
> > > > index 4ed690b..0eea1b3 100644
> > > > --- a/samples/bpf/sampleip_user.c
> > > > +++ b/samples/bpf/sampleip_user.c
> > > > @@ -26,7 +26,6 @@
> > > >  #define DEFAULT_FREQ   99
> > > >  #define DEFAULT_SECS   5
> > > >  #define MAX_IPS8192
> > > > -#define PAGE_OFFSET0x8800
> > > >  
> > > >  static int nr_cpus;
> > > >  
> > > > @@ -107,14 +106,13 @@ static void print_ip_map(int fd)
> > > > /* sort and print */
> > > > qsort(counts, max, sizeof(struct ipcount), count_cmp);
> > > > for (i = 0; i < max; i++) {
> > > > -   if (counts[i].ip > PAGE_OFFSET) {
> > > > -   sym = ksym_search(counts[i].ip);
> > > 
> > > yes. it is x64 specific, since it's a sample code,
> > > but simply removing it is not a fix.
> > > It makes this sampleip code behaving incorrectly.
> > > To do such 'cleanup of ksym' please refactor it in the true generic way,
> > > so these ksym* helpers can work on all archs and put this new
> > > functionality into selftests.
> > 
> > Just want to check, are you suggesting to create a standalone
> > testing for kallsyms (like a folder tools/testing/selftests/ksym) or
> > do you mean to place the generic code into folder
> > tools/testing/selftests/bpf/?
> 
> I think the minimal first step is to cleanup ksym bits into something
> that bpf selftests can use and keep it as new .c in
> tools/testing/selftests/bpf/
> Later if it becomes useful for other tests in selftests it can be moved.

Thanks for explanation, now it's clear for me :)

> Thanks!
> 

Re: [PATCH bpf-next 4/5] samples/bpf: Refine printing symbol for sampleip

[Leo Yan]

On Wed, Apr 18, 2018 at 10:21:25PM -0700, Alexei Starovoitov wrote:
> On Thu, Apr 19, 2018 at 01:12:49PM +0800, Leo Yan wrote:
> > On Wed, Apr 18, 2018 at 09:47:45PM -0700, Alexei Starovoitov wrote:
> > > On Thu, Apr 19, 2018 at 09:34:05AM +0800, Leo Yan wrote:
> > > > The code defines macro 'PAGE_OFFSET' and uses it to decide if the
> > > > address is in kernel space or not.  But different architecture has
> > > > different 'PAGE_OFFSET' so this program cannot be used for all
> > > > platforms.
> > > > 
> > > > This commit changes to check returned pointer from ksym_search() to
> > > > judge if the address falls into kernel space or not, and removes
> > > > macro 'PAGE_OFFSET' as it isn't used anymore.  As result, this program
> > > > has no architecture dependency.
> > > > 
> > > > Signed-off-by: Leo Yan 
> > > > ---
> > > >  samples/bpf/sampleip_user.c | 8 +++-
> > > >  1 file changed, 3 insertions(+), 5 deletions(-)
> > > > 
> > > > diff --git a/samples/bpf/sampleip_user.c b/samples/bpf/sampleip_user.c
> > > > index 4ed690b..0eea1b3 100644
> > > > --- a/samples/bpf/sampleip_user.c
> > > > +++ b/samples/bpf/sampleip_user.c
> > > > @@ -26,7 +26,6 @@
> > > >  #define DEFAULT_FREQ   99
> > > >  #define DEFAULT_SECS   5
> > > >  #define MAX_IPS8192
> > > > -#define PAGE_OFFSET0x8800
> > > >  
> > > >  static int nr_cpus;
> > > >  
> > > > @@ -107,14 +106,13 @@ static void print_ip_map(int fd)
> > > > /* sort and print */
> > > > qsort(counts, max, sizeof(struct ipcount), count_cmp);
> > > > for (i = 0; i < max; i++) {
> > > > -   if (counts[i].ip > PAGE_OFFSET) {
> > > > -   sym = ksym_search(counts[i].ip);
> > > 
> > > yes. it is x64 specific, since it's a sample code,
> > > but simply removing it is not a fix.
> > > It makes this sampleip code behaving incorrectly.
> > > To do such 'cleanup of ksym' please refactor it in the true generic way,
> > > so these ksym* helpers can work on all archs and put this new
> > > functionality into selftests.
> > 
> > Just want to check, are you suggesting to create a standalone
> > testing for kallsyms (like a folder tools/testing/selftests/ksym) or
> > do you mean to place the generic code into folder
> > tools/testing/selftests/bpf/?
> 
> I think the minimal first step is to cleanup ksym bits into something
> that bpf selftests can use and keep it as new .c in
> tools/testing/selftests/bpf/
> Later if it becomes useful for other tests in selftests it can be moved.

Thanks for explanation, now it's clear for me :)

> Thanks!
> 

[PATCH v2 net 1/3] virtio_net: split out ctrl buffer

[Michael S. Tsirkin]

When sending control commands, virtio net sets up several buffers for
DMA. The buffers are all part of the net device which means it's
actually allocated by kvmalloc so it's in theory (on extreme memory
pressure) possible to get a vmalloc'ed buffer which on some platforms
means we can't DMA there.

Fix up by moving the DMA buffers into a separate structure.

Reported-by: Mikulas Patocka 
Suggested-by: Eric Dumazet 
Signed-off-by: Michael S. Tsirkin 
---

Changes from v1:
build fix

 drivers/net/virtio_net.c | 68 +++-
 1 file changed, 39 insertions(+), 29 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 7b187ec..3d0eff53 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -147,6 +147,17 @@ struct receive_queue {
struct xdp_rxq_info xdp_rxq;
 };
 
+/* Control VQ buffers: protected by the rtnl lock */
+struct control_buf {
+   struct virtio_net_ctrl_hdr hdr;
+   virtio_net_ctrl_ack status;
+   struct virtio_net_ctrl_mq mq;
+   u8 promisc;
+   u8 allmulti;
+   u16 vid;
+   u64 offloads;
+};
+
 struct virtnet_info {
struct virtio_device *vdev;
struct virtqueue *cvq;
@@ -192,14 +203,7 @@ struct virtnet_info {
struct hlist_node node;
struct hlist_node node_dead;
 
-   /* Control VQ buffers: protected by the rtnl lock */
-   struct virtio_net_ctrl_hdr ctrl_hdr;
-   virtio_net_ctrl_ack ctrl_status;
-   struct virtio_net_ctrl_mq ctrl_mq;
-   u8 ctrl_promisc;
-   u8 ctrl_allmulti;
-   u16 ctrl_vid;
-   u64 ctrl_offloads;
+   struct control_buf *ctrl;
 
/* Ethtool settings */
u8 duplex;
@@ -1454,25 +1458,25 @@ static bool virtnet_send_command(struct virtnet_info 
*vi, u8 class, u8 cmd,
/* Caller should know better */
BUG_ON(!virtio_has_feature(vi->vdev, VIRTIO_NET_F_CTRL_VQ));
 
-   vi->ctrl_status = ~0;
-   vi->ctrl_hdr.class = class;
-   vi->ctrl_hdr.cmd = cmd;
+   vi->ctrl->status = ~0;
+   vi->ctrl->hdr.class = class;
+   vi->ctrl->hdr.cmd = cmd;
/* Add header */
-   sg_init_one(, >ctrl_hdr, sizeof(vi->ctrl_hdr));
+   sg_init_one(, >ctrl->hdr, sizeof(vi->ctrl->hdr));
sgs[out_num++] = 
 
if (out)
sgs[out_num++] = out;
 
/* Add return status. */
-   sg_init_one(, >ctrl_status, sizeof(vi->ctrl_status));
+   sg_init_one(, >ctrl->status, sizeof(vi->ctrl->status));
sgs[out_num] = 
 
BUG_ON(out_num + 1 > ARRAY_SIZE(sgs));
virtqueue_add_sgs(vi->cvq, sgs, out_num, 1, vi, GFP_ATOMIC);
 
if (unlikely(!virtqueue_kick(vi->cvq)))
-   return vi->ctrl_status == VIRTIO_NET_OK;
+   return vi->ctrl->status == VIRTIO_NET_OK;
 
/* Spin for a response, the kick causes an ioport write, trapping
 * into the hypervisor, so the request should be handled immediately.
@@ -1481,7 +1485,7 @@ static bool virtnet_send_command(struct virtnet_info *vi, 
u8 class, u8 cmd,
   !virtqueue_is_broken(vi->cvq))
cpu_relax();
 
-   return vi->ctrl_status == VIRTIO_NET_OK;
+   return vi->ctrl->status == VIRTIO_NET_OK;
 }
 
 static int virtnet_set_mac_address(struct net_device *dev, void *p)
@@ -1593,8 +1597,8 @@ static int _virtnet_set_queues(struct virtnet_info *vi, 
u16 queue_pairs)
if (!vi->has_cvq || !virtio_has_feature(vi->vdev, VIRTIO_NET_F_MQ))
return 0;
 
-   vi->ctrl_mq.virtqueue_pairs = cpu_to_virtio16(vi->vdev, queue_pairs);
-   sg_init_one(, >ctrl_mq, sizeof(vi->ctrl_mq));
+   vi->ctrl->mq.virtqueue_pairs = cpu_to_virtio16(vi->vdev, queue_pairs);
+   sg_init_one(, >ctrl->mq, sizeof(vi->ctrl->mq));
 
if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_MQ,
  VIRTIO_NET_CTRL_MQ_VQ_PAIRS_SET, )) {
@@ -1653,22 +1657,22 @@ static void virtnet_set_rx_mode(struct net_device *dev)
if (!virtio_has_feature(vi->vdev, VIRTIO_NET_F_CTRL_RX))
return;
 
-   vi->ctrl_promisc = ((dev->flags & IFF_PROMISC) != 0);
-   vi->ctrl_allmulti = ((dev->flags & IFF_ALLMULTI) != 0);
+   vi->ctrl->promisc = ((dev->flags & IFF_PROMISC) != 0);
+   vi->ctrl->allmulti = ((dev->flags & IFF_ALLMULTI) != 0);
 
-   sg_init_one(sg, >ctrl_promisc, sizeof(vi->ctrl_promisc));
+   sg_init_one(sg, >ctrl->promisc, sizeof(vi->ctrl->promisc));
 
if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_RX,
  VIRTIO_NET_CTRL_RX_PROMISC, sg))
dev_warn(>dev, "Failed to %sable promisc mode.\n",
-vi->ctrl_promisc ? "en" : "dis");
+vi->ctrl->promisc ? "en" : "dis");
 
-   sg_init_one(sg, >ctrl_allmulti, sizeof(vi->ctrl_allmulti));
+   sg_init_one(sg, >ctrl->allmulti, 

[PATCH v2 net 2/3] virtio_net: fix adding vids on big-endian

[Michael S. Tsirkin]

Programming vids (adding or removing them) still passes
guest-endian values in the DMA buffer. That's wrong
if guest is big-endian and when virtio 1 is enabled.

Note: this is on top of a previous patch:
virtio_net: split out ctrl buffer

Fixes: 9465a7a6f ("virtio_net: enable v1.0 support")
Signed-off-by: Michael S. Tsirkin 
---
 drivers/net/virtio_net.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 3d0eff53..f84fe04 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -154,7 +154,7 @@ struct control_buf {
struct virtio_net_ctrl_mq mq;
u8 promisc;
u8 allmulti;
-   u16 vid;
+   __virtio16 vid;
u64 offloads;
 };
 
@@ -1718,7 +1718,7 @@ static int virtnet_vlan_rx_add_vid(struct net_device *dev,
struct virtnet_info *vi = netdev_priv(dev);
struct scatterlist sg;
 
-   vi->ctrl->vid = vid;
+   vi->ctrl->vid = cpu_to_virtio16(vi->vdev, vid);
sg_init_one(, >ctrl->vid, sizeof(vi->ctrl->vid));
 
if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_VLAN,
@@ -1733,7 +1733,7 @@ static int virtnet_vlan_rx_kill_vid(struct net_device 
*dev,
struct virtnet_info *vi = netdev_priv(dev);
struct scatterlist sg;
 
-   vi->ctrl->vid = vid;
+   vi->ctrl->vid = cpu_to_virtio16(vi->vdev, vid);
sg_init_one(, >ctrl->vid, sizeof(vi->ctrl->vid));
 
if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_VLAN,
-- 
MST

[PATCH v2 net 1/3] virtio_net: split out ctrl buffer

[Michael S. Tsirkin]

When sending control commands, virtio net sets up several buffers for
DMA. The buffers are all part of the net device which means it's
actually allocated by kvmalloc so it's in theory (on extreme memory
pressure) possible to get a vmalloc'ed buffer which on some platforms
means we can't DMA there.

Fix up by moving the DMA buffers into a separate structure.

Reported-by: Mikulas Patocka 
Suggested-by: Eric Dumazet 
Signed-off-by: Michael S. Tsirkin 
---

Changes from v1:
build fix

 drivers/net/virtio_net.c | 68 +++-
 1 file changed, 39 insertions(+), 29 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 7b187ec..3d0eff53 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -147,6 +147,17 @@ struct receive_queue {
struct xdp_rxq_info xdp_rxq;
 };
 
+/* Control VQ buffers: protected by the rtnl lock */
+struct control_buf {
+   struct virtio_net_ctrl_hdr hdr;
+   virtio_net_ctrl_ack status;
+   struct virtio_net_ctrl_mq mq;
+   u8 promisc;
+   u8 allmulti;
+   u16 vid;
+   u64 offloads;
+};
+
 struct virtnet_info {
struct virtio_device *vdev;
struct virtqueue *cvq;
@@ -192,14 +203,7 @@ struct virtnet_info {
struct hlist_node node;
struct hlist_node node_dead;
 
-   /* Control VQ buffers: protected by the rtnl lock */
-   struct virtio_net_ctrl_hdr ctrl_hdr;
-   virtio_net_ctrl_ack ctrl_status;
-   struct virtio_net_ctrl_mq ctrl_mq;
-   u8 ctrl_promisc;
-   u8 ctrl_allmulti;
-   u16 ctrl_vid;
-   u64 ctrl_offloads;
+   struct control_buf *ctrl;
 
/* Ethtool settings */
u8 duplex;
@@ -1454,25 +1458,25 @@ static bool virtnet_send_command(struct virtnet_info 
*vi, u8 class, u8 cmd,
/* Caller should know better */
BUG_ON(!virtio_has_feature(vi->vdev, VIRTIO_NET_F_CTRL_VQ));
 
-   vi->ctrl_status = ~0;
-   vi->ctrl_hdr.class = class;
-   vi->ctrl_hdr.cmd = cmd;
+   vi->ctrl->status = ~0;
+   vi->ctrl->hdr.class = class;
+   vi->ctrl->hdr.cmd = cmd;
/* Add header */
-   sg_init_one(, >ctrl_hdr, sizeof(vi->ctrl_hdr));
+   sg_init_one(, >ctrl->hdr, sizeof(vi->ctrl->hdr));
sgs[out_num++] = 
 
if (out)
sgs[out_num++] = out;
 
/* Add return status. */
-   sg_init_one(, >ctrl_status, sizeof(vi->ctrl_status));
+   sg_init_one(, >ctrl->status, sizeof(vi->ctrl->status));
sgs[out_num] = 
 
BUG_ON(out_num + 1 > ARRAY_SIZE(sgs));
virtqueue_add_sgs(vi->cvq, sgs, out_num, 1, vi, GFP_ATOMIC);
 
if (unlikely(!virtqueue_kick(vi->cvq)))
-   return vi->ctrl_status == VIRTIO_NET_OK;
+   return vi->ctrl->status == VIRTIO_NET_OK;
 
/* Spin for a response, the kick causes an ioport write, trapping
 * into the hypervisor, so the request should be handled immediately.
@@ -1481,7 +1485,7 @@ static bool virtnet_send_command(struct virtnet_info *vi, 
u8 class, u8 cmd,
   !virtqueue_is_broken(vi->cvq))
cpu_relax();
 
-   return vi->ctrl_status == VIRTIO_NET_OK;
+   return vi->ctrl->status == VIRTIO_NET_OK;
 }
 
 static int virtnet_set_mac_address(struct net_device *dev, void *p)
@@ -1593,8 +1597,8 @@ static int _virtnet_set_queues(struct virtnet_info *vi, 
u16 queue_pairs)
if (!vi->has_cvq || !virtio_has_feature(vi->vdev, VIRTIO_NET_F_MQ))
return 0;
 
-   vi->ctrl_mq.virtqueue_pairs = cpu_to_virtio16(vi->vdev, queue_pairs);
-   sg_init_one(, >ctrl_mq, sizeof(vi->ctrl_mq));
+   vi->ctrl->mq.virtqueue_pairs = cpu_to_virtio16(vi->vdev, queue_pairs);
+   sg_init_one(, >ctrl->mq, sizeof(vi->ctrl->mq));
 
if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_MQ,
  VIRTIO_NET_CTRL_MQ_VQ_PAIRS_SET, )) {
@@ -1653,22 +1657,22 @@ static void virtnet_set_rx_mode(struct net_device *dev)
if (!virtio_has_feature(vi->vdev, VIRTIO_NET_F_CTRL_RX))
return;
 
-   vi->ctrl_promisc = ((dev->flags & IFF_PROMISC) != 0);
-   vi->ctrl_allmulti = ((dev->flags & IFF_ALLMULTI) != 0);
+   vi->ctrl->promisc = ((dev->flags & IFF_PROMISC) != 0);
+   vi->ctrl->allmulti = ((dev->flags & IFF_ALLMULTI) != 0);
 
-   sg_init_one(sg, >ctrl_promisc, sizeof(vi->ctrl_promisc));
+   sg_init_one(sg, >ctrl->promisc, sizeof(vi->ctrl->promisc));
 
if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_RX,
  VIRTIO_NET_CTRL_RX_PROMISC, sg))
dev_warn(>dev, "Failed to %sable promisc mode.\n",
-vi->ctrl_promisc ? "en" : "dis");
+vi->ctrl->promisc ? "en" : "dis");
 
-   sg_init_one(sg, >ctrl_allmulti, sizeof(vi->ctrl_allmulti));
+   sg_init_one(sg, >ctrl->allmulti, sizeof(vi->ctrl->allmulti));
 
if (!virtnet_send_command(vi, 

[PATCH v2 net 2/3] virtio_net: fix adding vids on big-endian

[Michael S. Tsirkin]

Programming vids (adding or removing them) still passes
guest-endian values in the DMA buffer. That's wrong
if guest is big-endian and when virtio 1 is enabled.

Note: this is on top of a previous patch:
virtio_net: split out ctrl buffer

Fixes: 9465a7a6f ("virtio_net: enable v1.0 support")
Signed-off-by: Michael S. Tsirkin 
---
 drivers/net/virtio_net.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 3d0eff53..f84fe04 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -154,7 +154,7 @@ struct control_buf {
struct virtio_net_ctrl_mq mq;
u8 promisc;
u8 allmulti;
-   u16 vid;
+   __virtio16 vid;
u64 offloads;
 };
 
@@ -1718,7 +1718,7 @@ static int virtnet_vlan_rx_add_vid(struct net_device *dev,
struct virtnet_info *vi = netdev_priv(dev);
struct scatterlist sg;
 
-   vi->ctrl->vid = vid;
+   vi->ctrl->vid = cpu_to_virtio16(vi->vdev, vid);
sg_init_one(, >ctrl->vid, sizeof(vi->ctrl->vid));
 
if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_VLAN,
@@ -1733,7 +1733,7 @@ static int virtnet_vlan_rx_kill_vid(struct net_device 
*dev,
struct virtnet_info *vi = netdev_priv(dev);
struct scatterlist sg;
 
-   vi->ctrl->vid = vid;
+   vi->ctrl->vid = cpu_to_virtio16(vi->vdev, vid);
sg_init_one(, >ctrl->vid, sizeof(vi->ctrl->vid));
 
if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_VLAN,
-- 
MST

[PATCH v2 net 0/3] virtio: ctrl buffer fixes

[Michael S. Tsirkin]

Here are a couple of fixes related to the virtio control buffer.
Lightly tested on x86 only.

Michael S. Tsirkin (3):
  virtio_net: split out ctrl buffer
  virtio_net: fix adding vids on big-endian
  virtio_net: sparse annotation fix

 drivers/net/virtio_net.c | 68 +++-
 1 file changed, 39 insertions(+), 29 deletions(-)

-- 
MST

[PATCH v2 net 0/3] virtio: ctrl buffer fixes

[Michael S. Tsirkin]

Here are a couple of fixes related to the virtio control buffer.
Lightly tested on x86 only.

Michael S. Tsirkin (3):
  virtio_net: split out ctrl buffer
  virtio_net: fix adding vids on big-endian
  virtio_net: sparse annotation fix

 drivers/net/virtio_net.c | 68 +++-
 1 file changed, 39 insertions(+), 29 deletions(-)

-- 
MST

[PATCH v2 net 3/3] virtio_net: sparse annotation fix

[Michael S. Tsirkin]

offloads is a buffer in virtio format, should use
the __virtio64 tag.

Signed-off-by: Michael S. Tsirkin 
---
 drivers/net/virtio_net.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index f84fe04..c5b11f2 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -155,7 +155,7 @@ struct control_buf {
u8 promisc;
u8 allmulti;
__virtio16 vid;
-   u64 offloads;
+   __virtio64 offloads;
 };
 
 struct virtnet_info {
-- 
MST

[PATCH v2 net 3/3] virtio_net: sparse annotation fix

[Michael S. Tsirkin]

offloads is a buffer in virtio format, should use
the __virtio64 tag.

Signed-off-by: Michael S. Tsirkin 
---
 drivers/net/virtio_net.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index f84fe04..c5b11f2 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -155,7 +155,7 @@ struct control_buf {
u8 promisc;
u8 allmulti;
__virtio16 vid;
-   u64 offloads;
+   __virtio64 offloads;
 };
 
 struct virtnet_info {
-- 
MST

Re: [PATCH bpf-next 4/5] samples/bpf: Refine printing symbol for sampleip

[Alexei Starovoitov]

On Thu, Apr 19, 2018 at 01:12:49PM +0800, Leo Yan wrote:
> On Wed, Apr 18, 2018 at 09:47:45PM -0700, Alexei Starovoitov wrote:
> > On Thu, Apr 19, 2018 at 09:34:05AM +0800, Leo Yan wrote:
> > > The code defines macro 'PAGE_OFFSET' and uses it to decide if the
> > > address is in kernel space or not.  But different architecture has
> > > different 'PAGE_OFFSET' so this program cannot be used for all
> > > platforms.
> > > 
> > > This commit changes to check returned pointer from ksym_search() to
> > > judge if the address falls into kernel space or not, and removes
> > > macro 'PAGE_OFFSET' as it isn't used anymore.  As result, this program
> > > has no architecture dependency.
> > > 
> > > Signed-off-by: Leo Yan 
> > > ---
> > >  samples/bpf/sampleip_user.c | 8 +++-
> > >  1 file changed, 3 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/samples/bpf/sampleip_user.c b/samples/bpf/sampleip_user.c
> > > index 4ed690b..0eea1b3 100644
> > > --- a/samples/bpf/sampleip_user.c
> > > +++ b/samples/bpf/sampleip_user.c
> > > @@ -26,7 +26,6 @@
> > >  #define DEFAULT_FREQ 99
> > >  #define DEFAULT_SECS 5
> > >  #define MAX_IPS  8192
> > > -#define PAGE_OFFSET  0x8800
> > >  
> > >  static int nr_cpus;
> > >  
> > > @@ -107,14 +106,13 @@ static void print_ip_map(int fd)
> > >   /* sort and print */
> > >   qsort(counts, max, sizeof(struct ipcount), count_cmp);
> > >   for (i = 0; i < max; i++) {
> > > - if (counts[i].ip > PAGE_OFFSET) {
> > > - sym = ksym_search(counts[i].ip);
> > 
> > yes. it is x64 specific, since it's a sample code,
> > but simply removing it is not a fix.
> > It makes this sampleip code behaving incorrectly.
> > To do such 'cleanup of ksym' please refactor it in the true generic way,
> > so these ksym* helpers can work on all archs and put this new
> > functionality into selftests.
> 
> Just want to check, are you suggesting to create a standalone
> testing for kallsyms (like a folder tools/testing/selftests/ksym) or
> do you mean to place the generic code into folder
> tools/testing/selftests/bpf/?

I think the minimal first step is to cleanup ksym bits into something
that bpf selftests can use and keep it as new .c in
tools/testing/selftests/bpf/
Later if it becomes useful for other tests in selftests it can be moved.

Thanks!

Re: [PATCH bpf-next 4/5] samples/bpf: Refine printing symbol for sampleip

[Alexei Starovoitov]

On Thu, Apr 19, 2018 at 01:12:49PM +0800, Leo Yan wrote:
> On Wed, Apr 18, 2018 at 09:47:45PM -0700, Alexei Starovoitov wrote:
> > On Thu, Apr 19, 2018 at 09:34:05AM +0800, Leo Yan wrote:
> > > The code defines macro 'PAGE_OFFSET' and uses it to decide if the
> > > address is in kernel space or not.  But different architecture has
> > > different 'PAGE_OFFSET' so this program cannot be used for all
> > > platforms.
> > > 
> > > This commit changes to check returned pointer from ksym_search() to
> > > judge if the address falls into kernel space or not, and removes
> > > macro 'PAGE_OFFSET' as it isn't used anymore.  As result, this program
> > > has no architecture dependency.
> > > 
> > > Signed-off-by: Leo Yan 
> > > ---
> > >  samples/bpf/sampleip_user.c | 8 +++-
> > >  1 file changed, 3 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/samples/bpf/sampleip_user.c b/samples/bpf/sampleip_user.c
> > > index 4ed690b..0eea1b3 100644
> > > --- a/samples/bpf/sampleip_user.c
> > > +++ b/samples/bpf/sampleip_user.c
> > > @@ -26,7 +26,6 @@
> > >  #define DEFAULT_FREQ 99
> > >  #define DEFAULT_SECS 5
> > >  #define MAX_IPS  8192
> > > -#define PAGE_OFFSET  0x8800
> > >  
> > >  static int nr_cpus;
> > >  
> > > @@ -107,14 +106,13 @@ static void print_ip_map(int fd)
> > >   /* sort and print */
> > >   qsort(counts, max, sizeof(struct ipcount), count_cmp);
> > >   for (i = 0; i < max; i++) {
> > > - if (counts[i].ip > PAGE_OFFSET) {
> > > - sym = ksym_search(counts[i].ip);
> > 
> > yes. it is x64 specific, since it's a sample code,
> > but simply removing it is not a fix.
> > It makes this sampleip code behaving incorrectly.
> > To do such 'cleanup of ksym' please refactor it in the true generic way,
> > so these ksym* helpers can work on all archs and put this new
> > functionality into selftests.
> 
> Just want to check, are you suggesting to create a standalone
> testing for kallsyms (like a folder tools/testing/selftests/ksym) or
> do you mean to place the generic code into folder
> tools/testing/selftests/bpf/?

I think the minimal first step is to cleanup ksym bits into something
that bpf selftests can use and keep it as new .c in
tools/testing/selftests/bpf/
Later if it becomes useful for other tests in selftests it can be moved.

Thanks!

Re: [PATCH net] virtio_net: split out ctrl buffer

[Michael S. Tsirkin]

On Thu, Apr 19, 2018 at 08:01:49AM +0300, Michael S. Tsirkin wrote:
> When sending control commands, virtio net sets up several buffers for
> DMA. The buffers are all part of the net device which means it's
> actually allocated by kvmalloc so in theory (on extreme memory pressure)
> it's possible to get a vmalloc'ed buffer which on some platforms means
> we can't DMA there.
> 
> Fix up by moving the DMA buffers out into a separate structure.
> 
> Reported-by: Mikulas Patocka 
> Suggested-by: Eric Dumazet 
> Signed-off-by: Michael S. Tsirkin 

Ouch forgot to commit a fix. Pls ignore will send v2 now.

> ---
> 
> Lightly tested.
> Mikulas, does this address your problem?
> 
>  drivers/net/virtio_net.c | 68 
> +++-
>  1 file changed, 39 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 7b187ec..82f50e5 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -147,6 +147,17 @@ struct receive_queue {
>   struct xdp_rxq_info xdp_rxq;
>  };
>  
> +/* Control VQ buffers: protected by the rtnl lock */
> +struct control_buf {
> + struct virtio_net_ctrl_hdr hdr;
> + virtio_net_ctrl_ack status;
> + struct virtio_net_ctrl_mq mq;
> + u8 promisc;
> + u8 allmulti;
> + u16 vid;
> + u64 offloads;
> +};
> +
>  struct virtnet_info {
>   struct virtio_device *vdev;
>   struct virtqueue *cvq;
> @@ -192,14 +203,7 @@ struct virtnet_info {
>   struct hlist_node node;
>   struct hlist_node node_dead;
>  
> - /* Control VQ buffers: protected by the rtnl lock */
> - struct virtio_net_ctrl_hdr ctrl_hdr;
> - virtio_net_ctrl_ack ctrl_status;
> - struct virtio_net_ctrl_mq ctrl_mq;
> - u8 ctrl_promisc;
> - u8 ctrl_allmulti;
> - u16 ctrl_vid;
> - u64 ctrl_offloads;
> + struct control_buf *ctrl;
>  
>   /* Ethtool settings */
>   u8 duplex;
> @@ -1454,25 +1458,25 @@ static bool virtnet_send_command(struct virtnet_info 
> *vi, u8 class, u8 cmd,
>   /* Caller should know better */
>   BUG_ON(!virtio_has_feature(vi->vdev, VIRTIO_NET_F_CTRL_VQ));
>  
> - vi->ctrl_status = ~0;
> - vi->ctrl_hdr.class = class;
> - vi->ctrl_hdr.cmd = cmd;
> + vi->ctrl->status = ~0;
> + vi->ctrl->hdr.class = class;
> + vi->ctrl->hdr.cmd = cmd;
>   /* Add header */
> - sg_init_one(, >ctrl_hdr, sizeof(vi->ctrl_hdr));
> + sg_init_one(, >ctrl->hdr, sizeof(vi->ctrl->hdr));
>   sgs[out_num++] = 
>  
>   if (out)
>   sgs[out_num++] = out;
>  
>   /* Add return status. */
> - sg_init_one(, >ctrl_status, sizeof(vi->ctrl_status));
> + sg_init_one(, >ctrl->status, sizeof(vi->ctrl->status));
>   sgs[out_num] = 
>  
>   BUG_ON(out_num + 1 > ARRAY_SIZE(sgs));
>   virtqueue_add_sgs(vi->cvq, sgs, out_num, 1, vi, GFP_ATOMIC);
>  
>   if (unlikely(!virtqueue_kick(vi->cvq)))
> - return vi->ctrl_status == VIRTIO_NET_OK;
> + return vi->ctrl->status == VIRTIO_NET_OK;
>  
>   /* Spin for a response, the kick causes an ioport write, trapping
>* into the hypervisor, so the request should be handled immediately.
> @@ -1481,7 +1485,7 @@ static bool virtnet_send_command(struct virtnet_info 
> *vi, u8 class, u8 cmd,
>  !virtqueue_is_broken(vi->cvq))
>   cpu_relax();
>  
> - return vi->ctrl_status == VIRTIO_NET_OK;
> + return vi->ctrl->status == VIRTIO_NET_OK;
>  }
>  
>  static int virtnet_set_mac_address(struct net_device *dev, void *p)
> @@ -1593,8 +1597,8 @@ static int _virtnet_set_queues(struct virtnet_info *vi, 
> u16 queue_pairs)
>   if (!vi->has_cvq || !virtio_has_feature(vi->vdev, VIRTIO_NET_F_MQ))
>   return 0;
>  
> - vi->ctrl_mq.virtqueue_pairs = cpu_to_virtio16(vi->vdev, queue_pairs);
> - sg_init_one(, >ctrl_mq, sizeof(vi->ctrl_mq));
> + vi->ctrl->mq.virtqueue_pairs = cpu_to_virtio16(vi->vdev, queue_pairs);
> + sg_init_one(, >ctrl->mq, sizeof(vi->ctrl->mq));
>  
>   if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_MQ,
> VIRTIO_NET_CTRL_MQ_VQ_PAIRS_SET, )) {
> @@ -1653,22 +1657,22 @@ static void virtnet_set_rx_mode(struct net_device 
> *dev)
>   if (!virtio_has_feature(vi->vdev, VIRTIO_NET_F_CTRL_RX))
>   return;
>  
> - vi->ctrl_promisc = ((dev->flags & IFF_PROMISC) != 0);
> - vi->ctrl_allmulti = ((dev->flags & IFF_ALLMULTI) != 0);
> + vi->ctrl->promisc = ((dev->flags & IFF_PROMISC) != 0);
> + vi->ctrl->allmulti = ((dev->flags & IFF_ALLMULTI) != 0);
>  
> - sg_init_one(sg, >ctrl_promisc, sizeof(vi->ctrl_promisc));
> + sg_init_one(sg, >ctrl->promisc, sizeof(vi->ctrl->promisc));
>  
>   if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_RX,
> VIRTIO_NET_CTRL_RX_PROMISC, sg))
>   dev_warn(>dev, "Failed to 

Re: [PATCH net] virtio_net: split out ctrl buffer

[Michael S. Tsirkin]

On Thu, Apr 19, 2018 at 08:01:49AM +0300, Michael S. Tsirkin wrote:
> When sending control commands, virtio net sets up several buffers for
> DMA. The buffers are all part of the net device which means it's
> actually allocated by kvmalloc so in theory (on extreme memory pressure)
> it's possible to get a vmalloc'ed buffer which on some platforms means
> we can't DMA there.
> 
> Fix up by moving the DMA buffers out into a separate structure.
> 
> Reported-by: Mikulas Patocka 
> Suggested-by: Eric Dumazet 
> Signed-off-by: Michael S. Tsirkin 

Ouch forgot to commit a fix. Pls ignore will send v2 now.

> ---
> 
> Lightly tested.
> Mikulas, does this address your problem?
> 
>  drivers/net/virtio_net.c | 68 
> +++-
>  1 file changed, 39 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 7b187ec..82f50e5 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -147,6 +147,17 @@ struct receive_queue {
>   struct xdp_rxq_info xdp_rxq;
>  };
>  
> +/* Control VQ buffers: protected by the rtnl lock */
> +struct control_buf {
> + struct virtio_net_ctrl_hdr hdr;
> + virtio_net_ctrl_ack status;
> + struct virtio_net_ctrl_mq mq;
> + u8 promisc;
> + u8 allmulti;
> + u16 vid;
> + u64 offloads;
> +};
> +
>  struct virtnet_info {
>   struct virtio_device *vdev;
>   struct virtqueue *cvq;
> @@ -192,14 +203,7 @@ struct virtnet_info {
>   struct hlist_node node;
>   struct hlist_node node_dead;
>  
> - /* Control VQ buffers: protected by the rtnl lock */
> - struct virtio_net_ctrl_hdr ctrl_hdr;
> - virtio_net_ctrl_ack ctrl_status;
> - struct virtio_net_ctrl_mq ctrl_mq;
> - u8 ctrl_promisc;
> - u8 ctrl_allmulti;
> - u16 ctrl_vid;
> - u64 ctrl_offloads;
> + struct control_buf *ctrl;
>  
>   /* Ethtool settings */
>   u8 duplex;
> @@ -1454,25 +1458,25 @@ static bool virtnet_send_command(struct virtnet_info 
> *vi, u8 class, u8 cmd,
>   /* Caller should know better */
>   BUG_ON(!virtio_has_feature(vi->vdev, VIRTIO_NET_F_CTRL_VQ));
>  
> - vi->ctrl_status = ~0;
> - vi->ctrl_hdr.class = class;
> - vi->ctrl_hdr.cmd = cmd;
> + vi->ctrl->status = ~0;
> + vi->ctrl->hdr.class = class;
> + vi->ctrl->hdr.cmd = cmd;
>   /* Add header */
> - sg_init_one(, >ctrl_hdr, sizeof(vi->ctrl_hdr));
> + sg_init_one(, >ctrl->hdr, sizeof(vi->ctrl->hdr));
>   sgs[out_num++] = 
>  
>   if (out)
>   sgs[out_num++] = out;
>  
>   /* Add return status. */
> - sg_init_one(, >ctrl_status, sizeof(vi->ctrl_status));
> + sg_init_one(, >ctrl->status, sizeof(vi->ctrl->status));
>   sgs[out_num] = 
>  
>   BUG_ON(out_num + 1 > ARRAY_SIZE(sgs));
>   virtqueue_add_sgs(vi->cvq, sgs, out_num, 1, vi, GFP_ATOMIC);
>  
>   if (unlikely(!virtqueue_kick(vi->cvq)))
> - return vi->ctrl_status == VIRTIO_NET_OK;
> + return vi->ctrl->status == VIRTIO_NET_OK;
>  
>   /* Spin for a response, the kick causes an ioport write, trapping
>* into the hypervisor, so the request should be handled immediately.
> @@ -1481,7 +1485,7 @@ static bool virtnet_send_command(struct virtnet_info 
> *vi, u8 class, u8 cmd,
>  !virtqueue_is_broken(vi->cvq))
>   cpu_relax();
>  
> - return vi->ctrl_status == VIRTIO_NET_OK;
> + return vi->ctrl->status == VIRTIO_NET_OK;
>  }
>  
>  static int virtnet_set_mac_address(struct net_device *dev, void *p)
> @@ -1593,8 +1597,8 @@ static int _virtnet_set_queues(struct virtnet_info *vi, 
> u16 queue_pairs)
>   if (!vi->has_cvq || !virtio_has_feature(vi->vdev, VIRTIO_NET_F_MQ))
>   return 0;
>  
> - vi->ctrl_mq.virtqueue_pairs = cpu_to_virtio16(vi->vdev, queue_pairs);
> - sg_init_one(, >ctrl_mq, sizeof(vi->ctrl_mq));
> + vi->ctrl->mq.virtqueue_pairs = cpu_to_virtio16(vi->vdev, queue_pairs);
> + sg_init_one(, >ctrl->mq, sizeof(vi->ctrl->mq));
>  
>   if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_MQ,
> VIRTIO_NET_CTRL_MQ_VQ_PAIRS_SET, )) {
> @@ -1653,22 +1657,22 @@ static void virtnet_set_rx_mode(struct net_device 
> *dev)
>   if (!virtio_has_feature(vi->vdev, VIRTIO_NET_F_CTRL_RX))
>   return;
>  
> - vi->ctrl_promisc = ((dev->flags & IFF_PROMISC) != 0);
> - vi->ctrl_allmulti = ((dev->flags & IFF_ALLMULTI) != 0);
> + vi->ctrl->promisc = ((dev->flags & IFF_PROMISC) != 0);
> + vi->ctrl->allmulti = ((dev->flags & IFF_ALLMULTI) != 0);
>  
> - sg_init_one(sg, >ctrl_promisc, sizeof(vi->ctrl_promisc));
> + sg_init_one(sg, >ctrl->promisc, sizeof(vi->ctrl->promisc));
>  
>   if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_RX,
> VIRTIO_NET_CTRL_RX_PROMISC, sg))
>   dev_warn(>dev, "Failed to %sable promisc mode.\n",
> -  vi->ctrl_promisc 

Re: [PATCH] checkpatch: Add a --strict test for structs with bool member definitions

[Julia Lawall]

On Wed, 18 Apr 2018, Joe Perches wrote:

> On Thu, 2018-04-19 at 06:40 +0200, Julia Lawall wrote:
> >
> > On Wed, 18 Apr 2018, Joe Perches wrote:
> >
> > > On Tue, 2018-04-17 at 17:07 +0800, yuank...@codeaurora.org wrote:
> > > > Hi julia,
> > > >
> > > > On 2018-04-15 05:19 AM, Julia Lawall wrote:
> > > > > On Wed, 11 Apr 2018, Joe Perches wrote:
> > > > >
> > > > > > On Thu, 2018-04-12 at 08:22 +0200, Julia Lawall wrote:
> > > > > > > On Wed, 11 Apr 2018, Joe Perches wrote:
> > > > > > > > On Wed, 2018-04-11 at 09:29 -0700, Andrew Morton wrote:
> > > > > > > > > We already have some 500 bools-in-structs
> > > > > > > >
> > > > > > > > I got at least triple that only in include/
> > > > > > > > so I expect there are at probably an order
> > > > > > > > of magnitude more than 500 in the kernel.
> > > > > > > >
> > > > > > > > I suppose some cocci script could count the
> > > > > > > > actual number of instances.  A regex can not.
> > > > > > >
> > > > > > > I got 12667.
> > > > > >
> > > > > > Could you please post the cocci script?
> > > > > >
> > > > > > > I'm not sure to understand the issue.  Will using a bitfield help 
> > > > > > > if there
> > > > > > > are no other bitfields in the structure?
> > > > > >
> > > > > > IMO, not really.
> > > > > >
> > > > > > The primary issue is described by Linus here:
> > > > > > https://lkml.org/lkml/2017/11/21/384
> > > > > >
> > > > > > I personally do not find a significant issue with
> > > > > > uncontrolled sizes of bool in kernel structs as
> > > > > > all of the kernel structs are transitory and not
> > > > > > written out to storage.
> > > > > >
> > > > > > I suppose bool bitfields are also OK, but for the
> > > > > > RMW required.
> > > > > >
> > > > > > Using unsigned int :1 bitfield instead of bool :1
> > > > > > has the negative of truncation so that the uint
> > > > > > has to be set with !! instead of a simple assign.
> > > > >
> > > > > At least with gcc 5.4.0, a number of structures become larger with
> > > > > unsigned int :1. bool:1 seems to mostly solve this problem.  The
> > > > > structure
> > > > > ichx_desc, defined in drivers/gpio/gpio-ich.c seems to become larger
> > > > > with
> > > > > both approaches.
> > > >
> > > > [ZJ] Hopefully, this could make it better in your environment.
> > > >   IMHO, this is just for double check.
> > >
> > > I doubt this is actually better or smaller code.
> > >
> > > Check the actual object code using objdump and the
> > > struct alignment using pahole.
> >
> > I didn't have a chance to try it, but it looks quite likely to result in a
> > smaller data structure based on the other examples that I looked at.
>
> I _really_ doubt there is any difference in size between the
> below in any architecture
>
> struct foo {
>   int bar;
>   bool baz:1;
>   int qux;
> };
>
> and
>
> struct foo {
>   int bar;
>   bool baz;
>   int qux;
> };
>
> Where there would be a difference in size is
>
> struct foo {
>   int bar;
>   bool baz1:1;
>   bool baz2:1;
>   int qux;
> };
>
> and
>
> struct foo {
>   int bar;
>   bool baz1;
>   bool baz2;
>
> int qux;
> };

In the situation of the example there are two bools together in the middle
of the structure and one at the end.  Somehow, even converting to bool:1
increases the size.  But it seems plausible that putting all three bools
together and converting them all to :1 would reduce the size.  I don't
know.  The size increase (more than 8 bytes) seems out of proportion for 3
bools.

I was able to check around 3000 structures that were not declared with any
attributes, that don't declare named types internally, and that are
compiled for x86.  Around 10% become smaller whn using bool:1, typically
by at most 8 bytes.

julia

>
>

Re: [PATCH] checkpatch: Add a --strict test for structs with bool member definitions

[Julia Lawall]

On Wed, 18 Apr 2018, Joe Perches wrote:

> On Thu, 2018-04-19 at 06:40 +0200, Julia Lawall wrote:
> >
> > On Wed, 18 Apr 2018, Joe Perches wrote:
> >
> > > On Tue, 2018-04-17 at 17:07 +0800, yuank...@codeaurora.org wrote:
> > > > Hi julia,
> > > >
> > > > On 2018-04-15 05:19 AM, Julia Lawall wrote:
> > > > > On Wed, 11 Apr 2018, Joe Perches wrote:
> > > > >
> > > > > > On Thu, 2018-04-12 at 08:22 +0200, Julia Lawall wrote:
> > > > > > > On Wed, 11 Apr 2018, Joe Perches wrote:
> > > > > > > > On Wed, 2018-04-11 at 09:29 -0700, Andrew Morton wrote:
> > > > > > > > > We already have some 500 bools-in-structs
> > > > > > > >
> > > > > > > > I got at least triple that only in include/
> > > > > > > > so I expect there are at probably an order
> > > > > > > > of magnitude more than 500 in the kernel.
> > > > > > > >
> > > > > > > > I suppose some cocci script could count the
> > > > > > > > actual number of instances.  A regex can not.
> > > > > > >
> > > > > > > I got 12667.
> > > > > >
> > > > > > Could you please post the cocci script?
> > > > > >
> > > > > > > I'm not sure to understand the issue.  Will using a bitfield help 
> > > > > > > if there
> > > > > > > are no other bitfields in the structure?
> > > > > >
> > > > > > IMO, not really.
> > > > > >
> > > > > > The primary issue is described by Linus here:
> > > > > > https://lkml.org/lkml/2017/11/21/384
> > > > > >
> > > > > > I personally do not find a significant issue with
> > > > > > uncontrolled sizes of bool in kernel structs as
> > > > > > all of the kernel structs are transitory and not
> > > > > > written out to storage.
> > > > > >
> > > > > > I suppose bool bitfields are also OK, but for the
> > > > > > RMW required.
> > > > > >
> > > > > > Using unsigned int :1 bitfield instead of bool :1
> > > > > > has the negative of truncation so that the uint
> > > > > > has to be set with !! instead of a simple assign.
> > > > >
> > > > > At least with gcc 5.4.0, a number of structures become larger with
> > > > > unsigned int :1. bool:1 seems to mostly solve this problem.  The
> > > > > structure
> > > > > ichx_desc, defined in drivers/gpio/gpio-ich.c seems to become larger
> > > > > with
> > > > > both approaches.
> > > >
> > > > [ZJ] Hopefully, this could make it better in your environment.
> > > >   IMHO, this is just for double check.
> > >
> > > I doubt this is actually better or smaller code.
> > >
> > > Check the actual object code using objdump and the
> > > struct alignment using pahole.
> >
> > I didn't have a chance to try it, but it looks quite likely to result in a
> > smaller data structure based on the other examples that I looked at.
>
> I _really_ doubt there is any difference in size between the
> below in any architecture
>
> struct foo {
>   int bar;
>   bool baz:1;
>   int qux;
> };
>
> and
>
> struct foo {
>   int bar;
>   bool baz;
>   int qux;
> };
>
> Where there would be a difference in size is
>
> struct foo {
>   int bar;
>   bool baz1:1;
>   bool baz2:1;
>   int qux;
> };
>
> and
>
> struct foo {
>   int bar;
>   bool baz1;
>   bool baz2;
>
> int qux;
> };

In the situation of the example there are two bools together in the middle
of the structure and one at the end.  Somehow, even converting to bool:1
increases the size.  But it seems plausible that putting all three bools
together and converting them all to :1 would reduce the size.  I don't
know.  The size increase (more than 8 bytes) seems out of proportion for 3
bools.

I was able to check around 3000 structures that were not declared with any
attributes, that don't declare named types internally, and that are
compiled for x86.  Around 10% become smaller whn using bool:1, typically
by at most 8 bytes.

julia

>
>

Smatch check for Spectre stuff

[Dan Carpenter]

Several people have asked me to write this and I think one person was
maybe working on writing it themselves...

The point of this check is to find place which might be vulnerable to
the Spectre vulnerability.  In the kernel we have the array_index_nospec()
macro which turns off speculation.  There are fewer than 10 places where
it's used.  Meanwhile this check complains about 800 places where maybe
it could be used.  Probably the 100x difference means there is something
that I haven't understood...

What the test does is it looks at array accesses where the user controls
the offset.  It asks "is this a read?" and have we used the
array_index_nospec() macro?  If the answers are yes, and no respectively
then print a warning.

http://repo.or.cz/smatch.git/blob/HEAD:/check_spectre.c

The other thing is that speculation probably goes to 200 instructions
ahead at most.  But the Smatch doesn't have any concept of CPU
instructions.  I've marked the offsets which were recently compared to
something as "local cap" because they were probably compared to the
array limit.  Those are maybe more likely to be under the 200 CPU
instruction count.

This obviously a first draft.

What would help me, is maybe people could tell me why there are so many
false positives.  Saying "the lower level checks for that" is not
helpful but if you could tell me the exact function name where the check
is that helps a lot...

I have included the warnings from yesterday's linux-next.

regards,
dan carpenter

arch/x86/events/core.c:319 set_ext_hw_attr() warn: potential spectre issue 
'hw_cache_event_ids[cache_type]' (local cap)
arch/x86/events/core.c:319 set_ext_hw_attr() warn: potential spectre issue 
'hw_cache_event_ids' (local cap)
arch/x86/events/core.c:328 set_ext_hw_attr() warn: potential spectre issue 
'hw_cache_extra_regs[cache_type]' (local cap)
arch/x86/events/core.c:328 set_ext_hw_attr() warn: potential spectre issue 
'hw_cache_extra_regs' (local cap)
arch/x86/events/intel/cstate.c:307 cstate_pmu_event_init() warn: potential 
spectre issue 'pkg_msr' (local cap)
arch/x86/events/msr.c:178 msr_event_init() warn: potential spectre issue 'msr' 
(local cap)
./arch/x86/include/asm/xen/page.h:120 __pfn_to_mfn() warn: potential spectre 
issue 'xen_p2m_addr' (local cap)
arch/x86/kvm/lapic.c:136 kvm_apic_map_get_logical_dest() warn: potential 
spectre issue 'map->phys_map' (local cap)
arch/x86/kvm/lapic.c:841 kvm_apic_map_get_dest_lapic() warn: potential spectre 
issue 'map->phys_map' (local cap)
crypto/anubis.c:557 anubis_setkey() warn: potential spectre issue 'ctx->E' 
(local cap)
drivers/acpi/acpica/dbmethod.c:181 acpi_db_set_method_data() warn: potential 
spectre issue 'walk_state->arguments' (local cap)
drivers/acpi/acpica/dsmthdat.c:249 acpi_ds_method_data_get_node() warn: 
potential spectre issue 'walk_state->arguments' (local cap)
drivers/acpi/acpica/hwxface.c:360 acpi_get_sleep_type_data() warn: potential 
spectre issue 'acpi_gbl_sleep_state_names' (local cap)
drivers/acpi/acpica/utdecode.c:467 acpi_ut_get_notify_name() warn: potential 
spectre issue 'acpi_gbl_generic_notify' (local cap)
drivers/acpi/fan.c:203 fan_set_state_acpi4() warn: potential spectre issue 
'fan->fps' (local cap)
drivers/ata/libata-scsi.c:3040 ata_find_dev() warn: potential spectre issue 
'ap->pmp_link' (local cap)
drivers/atm/lanai.c:2503 lanai_proc_read() warn: potential spectre issue 
'lanai->vccs' (local cap)
drivers/block/DAC960.c:6638 DAC960_gam_get_controller_info() warn: potential 
spectre issue 'DAC960_Controllers' (local cap)
drivers/block/DAC960.c:6689 DAC960_gam_v1_execute_command() warn: potential 
spectre issue 'DAC960_Controllers' (local cap)
drivers/block/DAC960.c:6856 DAC960_gam_v2_execute_command() warn: potential 
spectre issue 'DAC960_Controllers' (local cap)
drivers/block/DAC960.c:7014 DAC960_gam_v2_get_health_status() warn: potential 
spectre issue 'DAC960_Controllers' (local cap)
drivers/block/loop.c:1110 loop_set_status() warn: potential spectre issue 
'xfer_funcs' (local cap)
drivers/bluetooth/hci_ldisc.c:90 hci_uart_get_proto() warn: potential spectre 
issue 'hup' (local cap)
drivers/cdrom/cdrom.c:2383 cdrom_ioctl_media_changed() warn: potential spectre 
issue 'info->slots' (local cap)
drivers/char/applicom.c:837 ac_ioctl() warn: potential spectre issue 'apbs' 
(local cap)
drivers/char/raw.c:139 bind_set() warn: potential spectre issue 'raw_devices' 
(local cap)
drivers/char/raw.c:195 bind_get() warn: potential spectre issue 'raw_devices' 
(local cap)
drivers/crypto/qat/qat_common/adf_transport.c:271 adf_create_ring() warn: 
potential spectre issue 'transport_data->banks' (local cap)
drivers/gpio/gpiolib.c:1029 gpio_ioctl() warn: potential spectre issue 
'gdev->descs' (local cap)
drivers/gpio/gpiolib.c:902 lineevent_create() warn: potential spectre issue 
'gdev->descs' (local cap)
drivers/gpu/drm/amd/amdkfd/kfd_chardev.c:1573 kfd_ioctl() warn: potential 
spectre issue 'amdkfd_ioctls' (local cap)
drivers/gpu/drm/drm_bufs.c:1420 

Smatch check for Spectre stuff

[Dan Carpenter]

Several people have asked me to write this and I think one person was
maybe working on writing it themselves...

The point of this check is to find place which might be vulnerable to
the Spectre vulnerability.  In the kernel we have the array_index_nospec()
macro which turns off speculation.  There are fewer than 10 places where
it's used.  Meanwhile this check complains about 800 places where maybe
it could be used.  Probably the 100x difference means there is something
that I haven't understood...

What the test does is it looks at array accesses where the user controls
the offset.  It asks "is this a read?" and have we used the
array_index_nospec() macro?  If the answers are yes, and no respectively
then print a warning.

http://repo.or.cz/smatch.git/blob/HEAD:/check_spectre.c

The other thing is that speculation probably goes to 200 instructions
ahead at most.  But the Smatch doesn't have any concept of CPU
instructions.  I've marked the offsets which were recently compared to
something as "local cap" because they were probably compared to the
array limit.  Those are maybe more likely to be under the 200 CPU
instruction count.

This obviously a first draft.

What would help me, is maybe people could tell me why there are so many
false positives.  Saying "the lower level checks for that" is not
helpful but if you could tell me the exact function name where the check
is that helps a lot...

I have included the warnings from yesterday's linux-next.

regards,
dan carpenter

arch/x86/events/core.c:319 set_ext_hw_attr() warn: potential spectre issue 
'hw_cache_event_ids[cache_type]' (local cap)
arch/x86/events/core.c:319 set_ext_hw_attr() warn: potential spectre issue 
'hw_cache_event_ids' (local cap)
arch/x86/events/core.c:328 set_ext_hw_attr() warn: potential spectre issue 
'hw_cache_extra_regs[cache_type]' (local cap)
arch/x86/events/core.c:328 set_ext_hw_attr() warn: potential spectre issue 
'hw_cache_extra_regs' (local cap)
arch/x86/events/intel/cstate.c:307 cstate_pmu_event_init() warn: potential 
spectre issue 'pkg_msr' (local cap)
arch/x86/events/msr.c:178 msr_event_init() warn: potential spectre issue 'msr' 
(local cap)
./arch/x86/include/asm/xen/page.h:120 __pfn_to_mfn() warn: potential spectre 
issue 'xen_p2m_addr' (local cap)
arch/x86/kvm/lapic.c:136 kvm_apic_map_get_logical_dest() warn: potential 
spectre issue 'map->phys_map' (local cap)
arch/x86/kvm/lapic.c:841 kvm_apic_map_get_dest_lapic() warn: potential spectre 
issue 'map->phys_map' (local cap)
crypto/anubis.c:557 anubis_setkey() warn: potential spectre issue 'ctx->E' 
(local cap)
drivers/acpi/acpica/dbmethod.c:181 acpi_db_set_method_data() warn: potential 
spectre issue 'walk_state->arguments' (local cap)
drivers/acpi/acpica/dsmthdat.c:249 acpi_ds_method_data_get_node() warn: 
potential spectre issue 'walk_state->arguments' (local cap)
drivers/acpi/acpica/hwxface.c:360 acpi_get_sleep_type_data() warn: potential 
spectre issue 'acpi_gbl_sleep_state_names' (local cap)
drivers/acpi/acpica/utdecode.c:467 acpi_ut_get_notify_name() warn: potential 
spectre issue 'acpi_gbl_generic_notify' (local cap)
drivers/acpi/fan.c:203 fan_set_state_acpi4() warn: potential spectre issue 
'fan->fps' (local cap)
drivers/ata/libata-scsi.c:3040 ata_find_dev() warn: potential spectre issue 
'ap->pmp_link' (local cap)
drivers/atm/lanai.c:2503 lanai_proc_read() warn: potential spectre issue 
'lanai->vccs' (local cap)
drivers/block/DAC960.c:6638 DAC960_gam_get_controller_info() warn: potential 
spectre issue 'DAC960_Controllers' (local cap)
drivers/block/DAC960.c:6689 DAC960_gam_v1_execute_command() warn: potential 
spectre issue 'DAC960_Controllers' (local cap)
drivers/block/DAC960.c:6856 DAC960_gam_v2_execute_command() warn: potential 
spectre issue 'DAC960_Controllers' (local cap)
drivers/block/DAC960.c:7014 DAC960_gam_v2_get_health_status() warn: potential 
spectre issue 'DAC960_Controllers' (local cap)
drivers/block/loop.c:1110 loop_set_status() warn: potential spectre issue 
'xfer_funcs' (local cap)
drivers/bluetooth/hci_ldisc.c:90 hci_uart_get_proto() warn: potential spectre 
issue 'hup' (local cap)
drivers/cdrom/cdrom.c:2383 cdrom_ioctl_media_changed() warn: potential spectre 
issue 'info->slots' (local cap)
drivers/char/applicom.c:837 ac_ioctl() warn: potential spectre issue 'apbs' 
(local cap)
drivers/char/raw.c:139 bind_set() warn: potential spectre issue 'raw_devices' 
(local cap)
drivers/char/raw.c:195 bind_get() warn: potential spectre issue 'raw_devices' 
(local cap)
drivers/crypto/qat/qat_common/adf_transport.c:271 adf_create_ring() warn: 
potential spectre issue 'transport_data->banks' (local cap)
drivers/gpio/gpiolib.c:1029 gpio_ioctl() warn: potential spectre issue 
'gdev->descs' (local cap)
drivers/gpio/gpiolib.c:902 lineevent_create() warn: potential spectre issue 
'gdev->descs' (local cap)
drivers/gpu/drm/amd/amdkfd/kfd_chardev.c:1573 kfd_ioctl() warn: potential 
spectre issue 'amdkfd_ioctls' (local cap)
drivers/gpu/drm/drm_bufs.c:1420 

Re: [PATCH bpf-next 4/5] samples/bpf: Refine printing symbol for sampleip

[Leo Yan]

On Wed, Apr 18, 2018 at 09:47:45PM -0700, Alexei Starovoitov wrote:
> On Thu, Apr 19, 2018 at 09:34:05AM +0800, Leo Yan wrote:
> > The code defines macro 'PAGE_OFFSET' and uses it to decide if the
> > address is in kernel space or not.  But different architecture has
> > different 'PAGE_OFFSET' so this program cannot be used for all
> > platforms.
> > 
> > This commit changes to check returned pointer from ksym_search() to
> > judge if the address falls into kernel space or not, and removes
> > macro 'PAGE_OFFSET' as it isn't used anymore.  As result, this program
> > has no architecture dependency.
> > 
> > Signed-off-by: Leo Yan 
> > ---
> >  samples/bpf/sampleip_user.c | 8 +++-
> >  1 file changed, 3 insertions(+), 5 deletions(-)
> > 
> > diff --git a/samples/bpf/sampleip_user.c b/samples/bpf/sampleip_user.c
> > index 4ed690b..0eea1b3 100644
> > --- a/samples/bpf/sampleip_user.c
> > +++ b/samples/bpf/sampleip_user.c
> > @@ -26,7 +26,6 @@
> >  #define DEFAULT_FREQ   99
> >  #define DEFAULT_SECS   5
> >  #define MAX_IPS8192
> > -#define PAGE_OFFSET0x8800
> >  
> >  static int nr_cpus;
> >  
> > @@ -107,14 +106,13 @@ static void print_ip_map(int fd)
> > /* sort and print */
> > qsort(counts, max, sizeof(struct ipcount), count_cmp);
> > for (i = 0; i < max; i++) {
> > -   if (counts[i].ip > PAGE_OFFSET) {
> > -   sym = ksym_search(counts[i].ip);
> 
> yes. it is x64 specific, since it's a sample code,
> but simply removing it is not a fix.
> It makes this sampleip code behaving incorrectly.
> To do such 'cleanup of ksym' please refactor it in the true generic way,
> so these ksym* helpers can work on all archs and put this new
> functionality into selftests.

Just want to check, are you suggesting to create a standalone
testing for kallsyms (like a folder tools/testing/selftests/ksym) or
do you mean to place the generic code into folder
tools/testing/selftests/bpf/?

> If you can convert these examples into proper self-checking tests
> that run out of selftests that would be awesome.

Thank you for suggestions, Alexei.

> Thanks!
> 

Re: [PATCH bpf-next 4/5] samples/bpf: Refine printing symbol for sampleip

[Leo Yan]

On Wed, Apr 18, 2018 at 09:47:45PM -0700, Alexei Starovoitov wrote:
> On Thu, Apr 19, 2018 at 09:34:05AM +0800, Leo Yan wrote:
> > The code defines macro 'PAGE_OFFSET' and uses it to decide if the
> > address is in kernel space or not.  But different architecture has
> > different 'PAGE_OFFSET' so this program cannot be used for all
> > platforms.
> > 
> > This commit changes to check returned pointer from ksym_search() to
> > judge if the address falls into kernel space or not, and removes
> > macro 'PAGE_OFFSET' as it isn't used anymore.  As result, this program
> > has no architecture dependency.
> > 
> > Signed-off-by: Leo Yan 
> > ---
> >  samples/bpf/sampleip_user.c | 8 +++-
> >  1 file changed, 3 insertions(+), 5 deletions(-)
> > 
> > diff --git a/samples/bpf/sampleip_user.c b/samples/bpf/sampleip_user.c
> > index 4ed690b..0eea1b3 100644
> > --- a/samples/bpf/sampleip_user.c
> > +++ b/samples/bpf/sampleip_user.c
> > @@ -26,7 +26,6 @@
> >  #define DEFAULT_FREQ   99
> >  #define DEFAULT_SECS   5
> >  #define MAX_IPS8192
> > -#define PAGE_OFFSET0x8800
> >  
> >  static int nr_cpus;
> >  
> > @@ -107,14 +106,13 @@ static void print_ip_map(int fd)
> > /* sort and print */
> > qsort(counts, max, sizeof(struct ipcount), count_cmp);
> > for (i = 0; i < max; i++) {
> > -   if (counts[i].ip > PAGE_OFFSET) {
> > -   sym = ksym_search(counts[i].ip);
> 
> yes. it is x64 specific, since it's a sample code,
> but simply removing it is not a fix.
> It makes this sampleip code behaving incorrectly.
> To do such 'cleanup of ksym' please refactor it in the true generic way,
> so these ksym* helpers can work on all archs and put this new
> functionality into selftests.

Just want to check, are you suggesting to create a standalone
testing for kallsyms (like a folder tools/testing/selftests/ksym) or
do you mean to place the generic code into folder
tools/testing/selftests/bpf/?

> If you can convert these examples into proper self-checking tests
> that run out of selftests that would be awesome.

Thank you for suggestions, Alexei.

> Thanks!
> 

Re: [PATCH] powerpc: Allow selection of CONFIG_LD_DEAD_CODE_DATA_ELIMINATION

[Nicholas Piggin]

On Wed, 18 Apr 2018 15:11:24 +0200
Christophe LEROY  wrote:

> Le 18/04/2018 à 10:36, Mathieu Malaterre a écrit :
> > Christophe,
> > 
> > On Wed, Apr 18, 2018 at 8:34 AM, Christophe LEROY
> >  wrote:  
> >>
> >>
> >> Le 17/04/2018 à 19:10, Mathieu Malaterre a écrit :  
> >>>
> >>> On Tue, Apr 17, 2018 at 6:49 PM, Christophe LEROY
> >>>  wrote:  
> 
> 
> 
>  Le 17/04/2018 à 18:45, Mathieu Malaterre a écrit :  
> >
> >
> > On Tue, Apr 17, 2018 at 12:49 PM, Christophe Leroy
> >  wrote:  
> >>
> >>
> >> This option does dead code and data elimination with the linker by
> >> compiling with -ffunction-sections -fdata-sections and linking with
> >> --gc-sections.
> >>
> >> By selecting this option on mpc885_ads_defconfig,
> >> vmlinux LOAD segment size gets reduced by 10%
> >>
> >> Program Header before the patch:
> >>LOAD off0x0001 vaddr 0xc000 paddr 0x align
> >> 2**16
> >> filesz 0x0036eda4 memsz 0x0038de04 flags rwx
> >>
> >> Program Header after the patch:
> >>LOAD off0x0001 vaddr 0xc000 paddr 0x align
> >> 2**16
> >> filesz 0x00316da4 memsz 0x00334268 flags rwx
> >>
> >> Signed-off-by: Christophe Leroy 
> >> ---
> >> arch/powerpc/Kconfig | 8 
> >> 1 file changed, 8 insertions(+)
> >>
> >> diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
> >> index 8fe4353be5e3..e1fac49cf465 100644
> >> --- a/arch/powerpc/Kconfig
> >> +++ b/arch/powerpc/Kconfig
> >> @@ -888,6 +888,14 @@ config PPC_MEM_KEYS
> >>
> >>  If unsure, say y.
> >>
> >> +config PPC_UNUSED_ELIMINATION
> >> +   bool "Eliminate unused functions and data from vmlinux"
> >> +   default n
> >> +   select LD_DEAD_CODE_DATA_ELIMINATION
> >> +   help
> >> + Select this to do dead code and data elimination with the
> >> linker
> >> + by compiling with -ffunction-sections -fdata-sections and
> >> linking
> >> + with --gc-sections.
> >> endmenu
> >>  
> >
> > Just for reference, I cannot boot my Mac Mini G4 anymore (yaboot). The
> > messages I can see (prom_init) are:  
> 
> 
> 
>  Which version of GCC do you use ?  
> >>>
> >>>
> >>> $ powerpc-linux-gnu-gcc --version
> >>> powerpc-linux-gnu-gcc (Debian 6.3.0-18) 6.3.0 20170516
> >>> Copyright (C) 2016 Free Software Foundation, Inc.
> >>> This is free software; see the source for copying conditions.  There is NO
> >>> warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR
> >>> PURPOSE.
> >>>
> >>> this is simply coming from:
> >>>
> >>> $ apt-cache policy crossbuild-essential-powerpc
> >>> crossbuild-essential-powerpc:
> >>> Installed: 12.3
> >>> Candidate: 12.3
> >>> Version table:
> >>>*** 12.3 500
> >>>   500 http://ftp.fr.debian.org/debian stretch/main amd64 Packages
> >>>   500 http://ftp.fr.debian.org/debian stretch/main i386 Packages
> >>>   100 /var/lib/dpkg/status
> >>>
> >>>  
>  Can you provide the generated System.map with and without that option
>  active
>  ?  
> >>>
> >>>
> >>> $ du -sh g4/System.map.*
> >>> 1.7M g4/System.map.with
> >>> 1.8M g4/System.map.without  
> >>
> >>
> >> Here below is the list of objects removed with the option selected. I can't
> >> see anything suspect at first.  
> > 
> > Does this help:
> > 
> > $ grep orphan /tmp/g4.log|grep prom_init
> > powerpc-linux-gnu-ld: warning: orphan section `.sbss.of_workarounds'
> > from `arch/powerpc/kernel/prom_init.o' being placed in section
> > `.sbss.of_workarounds'.
> > powerpc-linux-gnu-ld: warning: orphan section `.sbss.of_workarounds'
> > from `arch/powerpc/kernel/prom_init.o' being placed in section
> > `.sbss.of_workarounds'.
> > powerpc-linux-gnu-ld: warning: orphan section `.sbss.of_workarounds'
> > from `arch/powerpc/kernel/prom_init.o' being placed in section
> > `.sbss.of_workarounds'.  
> 
> Well, in a way yes. I initially thought that all those warnings where
> normal, but indeed not. We were missing some things in powerpc linker 
> script, and most likely some sections ended up in the wrong place.
> 
> Last week I tested on an 8xx and it was booting without any issue.
> I tested today on an 83xx and it was not booting.
> 
> I will soon send new patches with the fixes in the linker scripts.

Yeah there needs to be a bit more work for powerpc before we can enable
this. I have some old patches I will dust off and re-send. I never got
modules working properly, I'll see if I can figure it out.

Thanks,
Nick

Re: [PATCH] powerpc: Allow selection of CONFIG_LD_DEAD_CODE_DATA_ELIMINATION

[Nicholas Piggin]

On Wed, 18 Apr 2018 15:11:24 +0200
Christophe LEROY  wrote:

> Le 18/04/2018 à 10:36, Mathieu Malaterre a écrit :
> > Christophe,
> > 
> > On Wed, Apr 18, 2018 at 8:34 AM, Christophe LEROY
> >  wrote:  
> >>
> >>
> >> Le 17/04/2018 à 19:10, Mathieu Malaterre a écrit :  
> >>>
> >>> On Tue, Apr 17, 2018 at 6:49 PM, Christophe LEROY
> >>>  wrote:  
> 
> 
> 
>  Le 17/04/2018 à 18:45, Mathieu Malaterre a écrit :  
> >
> >
> > On Tue, Apr 17, 2018 at 12:49 PM, Christophe Leroy
> >  wrote:  
> >>
> >>
> >> This option does dead code and data elimination with the linker by
> >> compiling with -ffunction-sections -fdata-sections and linking with
> >> --gc-sections.
> >>
> >> By selecting this option on mpc885_ads_defconfig,
> >> vmlinux LOAD segment size gets reduced by 10%
> >>
> >> Program Header before the patch:
> >>LOAD off0x0001 vaddr 0xc000 paddr 0x align
> >> 2**16
> >> filesz 0x0036eda4 memsz 0x0038de04 flags rwx
> >>
> >> Program Header after the patch:
> >>LOAD off0x0001 vaddr 0xc000 paddr 0x align
> >> 2**16
> >> filesz 0x00316da4 memsz 0x00334268 flags rwx
> >>
> >> Signed-off-by: Christophe Leroy 
> >> ---
> >> arch/powerpc/Kconfig | 8 
> >> 1 file changed, 8 insertions(+)
> >>
> >> diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
> >> index 8fe4353be5e3..e1fac49cf465 100644
> >> --- a/arch/powerpc/Kconfig
> >> +++ b/arch/powerpc/Kconfig
> >> @@ -888,6 +888,14 @@ config PPC_MEM_KEYS
> >>
> >>  If unsure, say y.
> >>
> >> +config PPC_UNUSED_ELIMINATION
> >> +   bool "Eliminate unused functions and data from vmlinux"
> >> +   default n
> >> +   select LD_DEAD_CODE_DATA_ELIMINATION
> >> +   help
> >> + Select this to do dead code and data elimination with the
> >> linker
> >> + by compiling with -ffunction-sections -fdata-sections and
> >> linking
> >> + with --gc-sections.
> >> endmenu
> >>  
> >
> > Just for reference, I cannot boot my Mac Mini G4 anymore (yaboot). The
> > messages I can see (prom_init) are:  
> 
> 
> 
>  Which version of GCC do you use ?  
> >>>
> >>>
> >>> $ powerpc-linux-gnu-gcc --version
> >>> powerpc-linux-gnu-gcc (Debian 6.3.0-18) 6.3.0 20170516
> >>> Copyright (C) 2016 Free Software Foundation, Inc.
> >>> This is free software; see the source for copying conditions.  There is NO
> >>> warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR
> >>> PURPOSE.
> >>>
> >>> this is simply coming from:
> >>>
> >>> $ apt-cache policy crossbuild-essential-powerpc
> >>> crossbuild-essential-powerpc:
> >>> Installed: 12.3
> >>> Candidate: 12.3
> >>> Version table:
> >>>*** 12.3 500
> >>>   500 http://ftp.fr.debian.org/debian stretch/main amd64 Packages
> >>>   500 http://ftp.fr.debian.org/debian stretch/main i386 Packages
> >>>   100 /var/lib/dpkg/status
> >>>
> >>>  
>  Can you provide the generated System.map with and without that option
>  active
>  ?  
> >>>
> >>>
> >>> $ du -sh g4/System.map.*
> >>> 1.7M g4/System.map.with
> >>> 1.8M g4/System.map.without  
> >>
> >>
> >> Here below is the list of objects removed with the option selected. I can't
> >> see anything suspect at first.  
> > 
> > Does this help:
> > 
> > $ grep orphan /tmp/g4.log|grep prom_init
> > powerpc-linux-gnu-ld: warning: orphan section `.sbss.of_workarounds'
> > from `arch/powerpc/kernel/prom_init.o' being placed in section
> > `.sbss.of_workarounds'.
> > powerpc-linux-gnu-ld: warning: orphan section `.sbss.of_workarounds'
> > from `arch/powerpc/kernel/prom_init.o' being placed in section
> > `.sbss.of_workarounds'.
> > powerpc-linux-gnu-ld: warning: orphan section `.sbss.of_workarounds'
> > from `arch/powerpc/kernel/prom_init.o' being placed in section
> > `.sbss.of_workarounds'.  
> 
> Well, in a way yes. I initially thought that all those warnings where
> normal, but indeed not. We were missing some things in powerpc linker 
> script, and most likely some sections ended up in the wrong place.
> 
> Last week I tested on an 8xx and it was booting without any issue.
> I tested today on an 83xx and it was not booting.
> 
> I will soon send new patches with the fixes in the linker scripts.

Yeah there needs to be a bit more work for powerpc before we can enable
this. I have some old patches I will dust off and re-send. I never got
modules working properly, I'll see if I can figure it out.

Thanks,
Nick

[PATCH] net: qrtr: Expose tunneling endpoint to user space

[Bjorn Andersson]

This implements a misc character device named "qrtr-tun" for the purpose
of allowing user space applications to implement endpoints in the qrtr
network.

This allows more advanced (and dynamic) testing of the qrtr code as well
as opens up the ability of tunneling qrtr over a network or USB link.

Signed-off-by: Bjorn Andersson 
---
 net/qrtr/Kconfig  |   7 +++
 net/qrtr/Makefile |   2 +
 net/qrtr/tun.c| 162 ++
 3 files changed, 171 insertions(+)
 create mode 100644 net/qrtr/tun.c

diff --git a/net/qrtr/Kconfig b/net/qrtr/Kconfig
index 326fd97444f5..1944834d225c 100644
--- a/net/qrtr/Kconfig
+++ b/net/qrtr/Kconfig
@@ -21,4 +21,11 @@ config QRTR_SMD
  Say Y here to support SMD based ipcrouter channels.  SMD is the
  most common transport for IPC Router.
 
+config QRTR_TUN
+   tristate "TUN device for Qualcomm IPC Router"
+   ---help---
+ Say Y here to expose a character device that allows user space to
+ implement endpoints of QRTR, for purpose of tunneling data to other
+ hosts or testing purposes.
+
 endif # QRTR
diff --git a/net/qrtr/Makefile b/net/qrtr/Makefile
index ab09e40f7c74..be012bfd3e52 100644
--- a/net/qrtr/Makefile
+++ b/net/qrtr/Makefile
@@ -2,3 +2,5 @@ obj-$(CONFIG_QRTR) := qrtr.o
 
 obj-$(CONFIG_QRTR_SMD) += qrtr-smd.o
 qrtr-smd-y := smd.o
+obj-$(CONFIG_QRTR_TUN) += qrtr-tun.o
+qrtr-tun-y := tun.o
diff --git a/net/qrtr/tun.c b/net/qrtr/tun.c
new file mode 100644
index ..ab87a12cbb17
--- /dev/null
+++ b/net/qrtr/tun.c
@@ -0,0 +1,162 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2018, Linaro Ltd */
+
+#include 
+#include 
+#include 
+#include 
+
+#include "qrtr.h"
+
+struct qrtr_tun {
+   struct qrtr_endpoint ep;
+
+   struct mutex queue_lock;
+   struct sk_buff_head queue;
+   wait_queue_head_t readq;
+};
+
+static int qrtr_tun_send(struct qrtr_endpoint *ep, struct sk_buff *skb)
+{
+   struct qrtr_tun *tun = container_of(ep, struct qrtr_tun, ep);
+
+   mutex_lock(>queue_lock);
+   skb_queue_tail(>queue, skb);
+   mutex_unlock(>queue_lock);
+
+   /* wake up any blocking processes, waiting for new data */
+   wake_up_interruptible(>readq);
+
+   return 0;
+}
+
+static int qrtr_tun_open(struct inode *inode, struct file *filp)
+{
+   struct qrtr_tun *tun;
+
+   tun = kzalloc(sizeof(*tun), GFP_KERNEL);
+   if (!tun)
+   return -ENOMEM;
+
+   mutex_init(>queue_lock);
+   skb_queue_head_init(>queue);
+   init_waitqueue_head(>readq);
+
+   tun->ep.xmit = qrtr_tun_send;
+
+   filp->private_data = tun;
+
+   return qrtr_endpoint_register(>ep, QRTR_EP_NID_AUTO);
+}
+
+static ssize_t qrtr_tun_read_iter(struct kiocb *iocb, struct iov_iter *to)
+{
+   struct file *filp = iocb->ki_filp;
+   struct qrtr_tun *tun = filp->private_data;
+   struct sk_buff *skb;
+   int count;
+
+   mutex_lock(>queue_lock);
+
+   /* Wait for data in the queue */
+   if (skb_queue_empty(>queue)) {
+   mutex_unlock(>queue_lock);
+
+   if (filp->f_flags & O_NONBLOCK)
+   return -EAGAIN;
+
+   /* Wait until we get data or the endpoint goes away */
+   if (wait_event_interruptible(tun->readq,
+!skb_queue_empty(>queue)))
+   return -ERESTARTSYS;
+
+   mutex_lock(>queue_lock);
+   }
+
+   skb = skb_dequeue(>queue);
+   mutex_unlock(>queue_lock);
+   if (!skb)
+   return -EFAULT;
+
+   count = min_t(size_t, iov_iter_count(to), skb->len);
+   if (copy_to_iter(skb->data, count, to) != count)
+   count = -EFAULT;
+
+   kfree_skb(skb);
+
+   return count;
+}
+
+static ssize_t qrtr_tun_write_iter(struct kiocb *iocb, struct iov_iter *from)
+{
+   struct file *filp = iocb->ki_filp;
+   struct qrtr_tun *tun = filp->private_data;
+   size_t len = iov_iter_count(from);
+   ssize_t ret;
+   void *kbuf;
+
+   kbuf = kzalloc(len, GFP_KERNEL);
+   if (!kbuf)
+   return -ENOMEM;
+
+   if (!copy_from_iter_full(kbuf, len, from))
+   return -EFAULT;
+
+   ret = qrtr_endpoint_post(>ep, kbuf, len);
+
+   return ret < 0 ? ret : len;
+}
+
+static int qrtr_tun_release(struct inode *inode, struct file *filp)
+{
+   struct qrtr_tun *tun = filp->private_data;
+   struct sk_buff *skb;
+
+   qrtr_endpoint_unregister(>ep);
+
+   /* Discard all SKBs */
+   while (!skb_queue_empty(>queue)) {
+   skb = skb_dequeue(>queue);
+   kfree_skb(skb);
+   }
+
+   kfree(tun);
+
+   return 0;
+}
+
+static const struct file_operations qrtr_tun_ops = {
+   .owner = THIS_MODULE,
+   .open = qrtr_tun_open,
+   .read_iter = qrtr_tun_read_iter,
+   .write_iter = 

[PATCH] net: qrtr: Expose tunneling endpoint to user space

[Bjorn Andersson]

This implements a misc character device named "qrtr-tun" for the purpose
of allowing user space applications to implement endpoints in the qrtr
network.

This allows more advanced (and dynamic) testing of the qrtr code as well
as opens up the ability of tunneling qrtr over a network or USB link.

Signed-off-by: Bjorn Andersson 
---
 net/qrtr/Kconfig  |   7 +++
 net/qrtr/Makefile |   2 +
 net/qrtr/tun.c| 162 ++
 3 files changed, 171 insertions(+)
 create mode 100644 net/qrtr/tun.c

diff --git a/net/qrtr/Kconfig b/net/qrtr/Kconfig
index 326fd97444f5..1944834d225c 100644
--- a/net/qrtr/Kconfig
+++ b/net/qrtr/Kconfig
@@ -21,4 +21,11 @@ config QRTR_SMD
  Say Y here to support SMD based ipcrouter channels.  SMD is the
  most common transport for IPC Router.
 
+config QRTR_TUN
+   tristate "TUN device for Qualcomm IPC Router"
+   ---help---
+ Say Y here to expose a character device that allows user space to
+ implement endpoints of QRTR, for purpose of tunneling data to other
+ hosts or testing purposes.
+
 endif # QRTR
diff --git a/net/qrtr/Makefile b/net/qrtr/Makefile
index ab09e40f7c74..be012bfd3e52 100644
--- a/net/qrtr/Makefile
+++ b/net/qrtr/Makefile
@@ -2,3 +2,5 @@ obj-$(CONFIG_QRTR) := qrtr.o
 
 obj-$(CONFIG_QRTR_SMD) += qrtr-smd.o
 qrtr-smd-y := smd.o
+obj-$(CONFIG_QRTR_TUN) += qrtr-tun.o
+qrtr-tun-y := tun.o
diff --git a/net/qrtr/tun.c b/net/qrtr/tun.c
new file mode 100644
index ..ab87a12cbb17
--- /dev/null
+++ b/net/qrtr/tun.c
@@ -0,0 +1,162 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2018, Linaro Ltd */
+
+#include 
+#include 
+#include 
+#include 
+
+#include "qrtr.h"
+
+struct qrtr_tun {
+   struct qrtr_endpoint ep;
+
+   struct mutex queue_lock;
+   struct sk_buff_head queue;
+   wait_queue_head_t readq;
+};
+
+static int qrtr_tun_send(struct qrtr_endpoint *ep, struct sk_buff *skb)
+{
+   struct qrtr_tun *tun = container_of(ep, struct qrtr_tun, ep);
+
+   mutex_lock(>queue_lock);
+   skb_queue_tail(>queue, skb);
+   mutex_unlock(>queue_lock);
+
+   /* wake up any blocking processes, waiting for new data */
+   wake_up_interruptible(>readq);
+
+   return 0;
+}
+
+static int qrtr_tun_open(struct inode *inode, struct file *filp)
+{
+   struct qrtr_tun *tun;
+
+   tun = kzalloc(sizeof(*tun), GFP_KERNEL);
+   if (!tun)
+   return -ENOMEM;
+
+   mutex_init(>queue_lock);
+   skb_queue_head_init(>queue);
+   init_waitqueue_head(>readq);
+
+   tun->ep.xmit = qrtr_tun_send;
+
+   filp->private_data = tun;
+
+   return qrtr_endpoint_register(>ep, QRTR_EP_NID_AUTO);
+}
+
+static ssize_t qrtr_tun_read_iter(struct kiocb *iocb, struct iov_iter *to)
+{
+   struct file *filp = iocb->ki_filp;
+   struct qrtr_tun *tun = filp->private_data;
+   struct sk_buff *skb;
+   int count;
+
+   mutex_lock(>queue_lock);
+
+   /* Wait for data in the queue */
+   if (skb_queue_empty(>queue)) {
+   mutex_unlock(>queue_lock);
+
+   if (filp->f_flags & O_NONBLOCK)
+   return -EAGAIN;
+
+   /* Wait until we get data or the endpoint goes away */
+   if (wait_event_interruptible(tun->readq,
+!skb_queue_empty(>queue)))
+   return -ERESTARTSYS;
+
+   mutex_lock(>queue_lock);
+   }
+
+   skb = skb_dequeue(>queue);
+   mutex_unlock(>queue_lock);
+   if (!skb)
+   return -EFAULT;
+
+   count = min_t(size_t, iov_iter_count(to), skb->len);
+   if (copy_to_iter(skb->data, count, to) != count)
+   count = -EFAULT;
+
+   kfree_skb(skb);
+
+   return count;
+}
+
+static ssize_t qrtr_tun_write_iter(struct kiocb *iocb, struct iov_iter *from)
+{
+   struct file *filp = iocb->ki_filp;
+   struct qrtr_tun *tun = filp->private_data;
+   size_t len = iov_iter_count(from);
+   ssize_t ret;
+   void *kbuf;
+
+   kbuf = kzalloc(len, GFP_KERNEL);
+   if (!kbuf)
+   return -ENOMEM;
+
+   if (!copy_from_iter_full(kbuf, len, from))
+   return -EFAULT;
+
+   ret = qrtr_endpoint_post(>ep, kbuf, len);
+
+   return ret < 0 ? ret : len;
+}
+
+static int qrtr_tun_release(struct inode *inode, struct file *filp)
+{
+   struct qrtr_tun *tun = filp->private_data;
+   struct sk_buff *skb;
+
+   qrtr_endpoint_unregister(>ep);
+
+   /* Discard all SKBs */
+   while (!skb_queue_empty(>queue)) {
+   skb = skb_dequeue(>queue);
+   kfree_skb(skb);
+   }
+
+   kfree(tun);
+
+   return 0;
+}
+
+static const struct file_operations qrtr_tun_ops = {
+   .owner = THIS_MODULE,
+   .open = qrtr_tun_open,
+   .read_iter = qrtr_tun_read_iter,
+   .write_iter = qrtr_tun_write_iter,
+   .release = 

[PATCH net] virtio_net: split out ctrl buffer

[Michael S. Tsirkin]

When sending control commands, virtio net sets up several buffers for
DMA. The buffers are all part of the net device which means it's
actually allocated by kvmalloc so in theory (on extreme memory pressure)
it's possible to get a vmalloc'ed buffer which on some platforms means
we can't DMA there.

Fix up by moving the DMA buffers out into a separate structure.

Reported-by: Mikulas Patocka 
Suggested-by: Eric Dumazet 
Signed-off-by: Michael S. Tsirkin 
---

Lightly tested.
Mikulas, does this address your problem?

 drivers/net/virtio_net.c | 68 +++-
 1 file changed, 39 insertions(+), 29 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 7b187ec..82f50e5 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -147,6 +147,17 @@ struct receive_queue {
struct xdp_rxq_info xdp_rxq;
 };
 
+/* Control VQ buffers: protected by the rtnl lock */
+struct control_buf {
+   struct virtio_net_ctrl_hdr hdr;
+   virtio_net_ctrl_ack status;
+   struct virtio_net_ctrl_mq mq;
+   u8 promisc;
+   u8 allmulti;
+   u16 vid;
+   u64 offloads;
+};
+
 struct virtnet_info {
struct virtio_device *vdev;
struct virtqueue *cvq;
@@ -192,14 +203,7 @@ struct virtnet_info {
struct hlist_node node;
struct hlist_node node_dead;
 
-   /* Control VQ buffers: protected by the rtnl lock */
-   struct virtio_net_ctrl_hdr ctrl_hdr;
-   virtio_net_ctrl_ack ctrl_status;
-   struct virtio_net_ctrl_mq ctrl_mq;
-   u8 ctrl_promisc;
-   u8 ctrl_allmulti;
-   u16 ctrl_vid;
-   u64 ctrl_offloads;
+   struct control_buf *ctrl;
 
/* Ethtool settings */
u8 duplex;
@@ -1454,25 +1458,25 @@ static bool virtnet_send_command(struct virtnet_info 
*vi, u8 class, u8 cmd,
/* Caller should know better */
BUG_ON(!virtio_has_feature(vi->vdev, VIRTIO_NET_F_CTRL_VQ));
 
-   vi->ctrl_status = ~0;
-   vi->ctrl_hdr.class = class;
-   vi->ctrl_hdr.cmd = cmd;
+   vi->ctrl->status = ~0;
+   vi->ctrl->hdr.class = class;
+   vi->ctrl->hdr.cmd = cmd;
/* Add header */
-   sg_init_one(, >ctrl_hdr, sizeof(vi->ctrl_hdr));
+   sg_init_one(, >ctrl->hdr, sizeof(vi->ctrl->hdr));
sgs[out_num++] = 
 
if (out)
sgs[out_num++] = out;
 
/* Add return status. */
-   sg_init_one(, >ctrl_status, sizeof(vi->ctrl_status));
+   sg_init_one(, >ctrl->status, sizeof(vi->ctrl->status));
sgs[out_num] = 
 
BUG_ON(out_num + 1 > ARRAY_SIZE(sgs));
virtqueue_add_sgs(vi->cvq, sgs, out_num, 1, vi, GFP_ATOMIC);
 
if (unlikely(!virtqueue_kick(vi->cvq)))
-   return vi->ctrl_status == VIRTIO_NET_OK;
+   return vi->ctrl->status == VIRTIO_NET_OK;
 
/* Spin for a response, the kick causes an ioport write, trapping
 * into the hypervisor, so the request should be handled immediately.
@@ -1481,7 +1485,7 @@ static bool virtnet_send_command(struct virtnet_info *vi, 
u8 class, u8 cmd,
   !virtqueue_is_broken(vi->cvq))
cpu_relax();
 
-   return vi->ctrl_status == VIRTIO_NET_OK;
+   return vi->ctrl->status == VIRTIO_NET_OK;
 }
 
 static int virtnet_set_mac_address(struct net_device *dev, void *p)
@@ -1593,8 +1597,8 @@ static int _virtnet_set_queues(struct virtnet_info *vi, 
u16 queue_pairs)
if (!vi->has_cvq || !virtio_has_feature(vi->vdev, VIRTIO_NET_F_MQ))
return 0;
 
-   vi->ctrl_mq.virtqueue_pairs = cpu_to_virtio16(vi->vdev, queue_pairs);
-   sg_init_one(, >ctrl_mq, sizeof(vi->ctrl_mq));
+   vi->ctrl->mq.virtqueue_pairs = cpu_to_virtio16(vi->vdev, queue_pairs);
+   sg_init_one(, >ctrl->mq, sizeof(vi->ctrl->mq));
 
if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_MQ,
  VIRTIO_NET_CTRL_MQ_VQ_PAIRS_SET, )) {
@@ -1653,22 +1657,22 @@ static void virtnet_set_rx_mode(struct net_device *dev)
if (!virtio_has_feature(vi->vdev, VIRTIO_NET_F_CTRL_RX))
return;
 
-   vi->ctrl_promisc = ((dev->flags & IFF_PROMISC) != 0);
-   vi->ctrl_allmulti = ((dev->flags & IFF_ALLMULTI) != 0);
+   vi->ctrl->promisc = ((dev->flags & IFF_PROMISC) != 0);
+   vi->ctrl->allmulti = ((dev->flags & IFF_ALLMULTI) != 0);
 
-   sg_init_one(sg, >ctrl_promisc, sizeof(vi->ctrl_promisc));
+   sg_init_one(sg, >ctrl->promisc, sizeof(vi->ctrl->promisc));
 
if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_RX,
  VIRTIO_NET_CTRL_RX_PROMISC, sg))
dev_warn(>dev, "Failed to %sable promisc mode.\n",
-vi->ctrl_promisc ? "en" : "dis");
+vi->ctrl->promisc ? "en" : "dis");
 
-   sg_init_one(sg, >ctrl_allmulti, sizeof(vi->ctrl_allmulti));
+   sg_init_one(sg, 

[PATCH net] virtio_net: split out ctrl buffer

[Michael S. Tsirkin]

When sending control commands, virtio net sets up several buffers for
DMA. The buffers are all part of the net device which means it's
actually allocated by kvmalloc so in theory (on extreme memory pressure)
it's possible to get a vmalloc'ed buffer which on some platforms means
we can't DMA there.

Fix up by moving the DMA buffers out into a separate structure.

Reported-by: Mikulas Patocka 
Suggested-by: Eric Dumazet 
Signed-off-by: Michael S. Tsirkin 
---

Lightly tested.
Mikulas, does this address your problem?

 drivers/net/virtio_net.c | 68 +++-
 1 file changed, 39 insertions(+), 29 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 7b187ec..82f50e5 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -147,6 +147,17 @@ struct receive_queue {
struct xdp_rxq_info xdp_rxq;
 };
 
+/* Control VQ buffers: protected by the rtnl lock */
+struct control_buf {
+   struct virtio_net_ctrl_hdr hdr;
+   virtio_net_ctrl_ack status;
+   struct virtio_net_ctrl_mq mq;
+   u8 promisc;
+   u8 allmulti;
+   u16 vid;
+   u64 offloads;
+};
+
 struct virtnet_info {
struct virtio_device *vdev;
struct virtqueue *cvq;
@@ -192,14 +203,7 @@ struct virtnet_info {
struct hlist_node node;
struct hlist_node node_dead;
 
-   /* Control VQ buffers: protected by the rtnl lock */
-   struct virtio_net_ctrl_hdr ctrl_hdr;
-   virtio_net_ctrl_ack ctrl_status;
-   struct virtio_net_ctrl_mq ctrl_mq;
-   u8 ctrl_promisc;
-   u8 ctrl_allmulti;
-   u16 ctrl_vid;
-   u64 ctrl_offloads;
+   struct control_buf *ctrl;
 
/* Ethtool settings */
u8 duplex;
@@ -1454,25 +1458,25 @@ static bool virtnet_send_command(struct virtnet_info 
*vi, u8 class, u8 cmd,
/* Caller should know better */
BUG_ON(!virtio_has_feature(vi->vdev, VIRTIO_NET_F_CTRL_VQ));
 
-   vi->ctrl_status = ~0;
-   vi->ctrl_hdr.class = class;
-   vi->ctrl_hdr.cmd = cmd;
+   vi->ctrl->status = ~0;
+   vi->ctrl->hdr.class = class;
+   vi->ctrl->hdr.cmd = cmd;
/* Add header */
-   sg_init_one(, >ctrl_hdr, sizeof(vi->ctrl_hdr));
+   sg_init_one(, >ctrl->hdr, sizeof(vi->ctrl->hdr));
sgs[out_num++] = 
 
if (out)
sgs[out_num++] = out;
 
/* Add return status. */
-   sg_init_one(, >ctrl_status, sizeof(vi->ctrl_status));
+   sg_init_one(, >ctrl->status, sizeof(vi->ctrl->status));
sgs[out_num] = 
 
BUG_ON(out_num + 1 > ARRAY_SIZE(sgs));
virtqueue_add_sgs(vi->cvq, sgs, out_num, 1, vi, GFP_ATOMIC);
 
if (unlikely(!virtqueue_kick(vi->cvq)))
-   return vi->ctrl_status == VIRTIO_NET_OK;
+   return vi->ctrl->status == VIRTIO_NET_OK;
 
/* Spin for a response, the kick causes an ioport write, trapping
 * into the hypervisor, so the request should be handled immediately.
@@ -1481,7 +1485,7 @@ static bool virtnet_send_command(struct virtnet_info *vi, 
u8 class, u8 cmd,
   !virtqueue_is_broken(vi->cvq))
cpu_relax();
 
-   return vi->ctrl_status == VIRTIO_NET_OK;
+   return vi->ctrl->status == VIRTIO_NET_OK;
 }
 
 static int virtnet_set_mac_address(struct net_device *dev, void *p)
@@ -1593,8 +1597,8 @@ static int _virtnet_set_queues(struct virtnet_info *vi, 
u16 queue_pairs)
if (!vi->has_cvq || !virtio_has_feature(vi->vdev, VIRTIO_NET_F_MQ))
return 0;
 
-   vi->ctrl_mq.virtqueue_pairs = cpu_to_virtio16(vi->vdev, queue_pairs);
-   sg_init_one(, >ctrl_mq, sizeof(vi->ctrl_mq));
+   vi->ctrl->mq.virtqueue_pairs = cpu_to_virtio16(vi->vdev, queue_pairs);
+   sg_init_one(, >ctrl->mq, sizeof(vi->ctrl->mq));
 
if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_MQ,
  VIRTIO_NET_CTRL_MQ_VQ_PAIRS_SET, )) {
@@ -1653,22 +1657,22 @@ static void virtnet_set_rx_mode(struct net_device *dev)
if (!virtio_has_feature(vi->vdev, VIRTIO_NET_F_CTRL_RX))
return;
 
-   vi->ctrl_promisc = ((dev->flags & IFF_PROMISC) != 0);
-   vi->ctrl_allmulti = ((dev->flags & IFF_ALLMULTI) != 0);
+   vi->ctrl->promisc = ((dev->flags & IFF_PROMISC) != 0);
+   vi->ctrl->allmulti = ((dev->flags & IFF_ALLMULTI) != 0);
 
-   sg_init_one(sg, >ctrl_promisc, sizeof(vi->ctrl_promisc));
+   sg_init_one(sg, >ctrl->promisc, sizeof(vi->ctrl->promisc));
 
if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_RX,
  VIRTIO_NET_CTRL_RX_PROMISC, sg))
dev_warn(>dev, "Failed to %sable promisc mode.\n",
-vi->ctrl_promisc ? "en" : "dis");
+vi->ctrl->promisc ? "en" : "dis");
 
-   sg_init_one(sg, >ctrl_allmulti, sizeof(vi->ctrl_allmulti));
+   sg_init_one(sg, >ctrl->allmulti, sizeof(vi->ctrl->allmulti));
 
if 

Re: [PATCH] soc: Unconditionally include qcom Makefile

[Bjorn Andersson]

On Wed 18 Apr 16:28 PDT 2018, Evan Green wrote:

> From: Guenter Roeck 
> 
> Incoming Qualcomm changes for GENI, i2c [1], and cmd-db [2] are enabled
> with COMPILE_TEST in drivers/soc/qcom. For this to work, the Makefile
> in that directory has to be included unconditionally, rather than only
> if ARCH_QCOM is enabled.
> 
> Example of the errors seen on allmodconfig with the GENI, i2c, and
> cmd-db patches applied:
> 
> Kernel: arch/x86/boot/bzImage is ready  (#1)
> ERROR: "geni_se_select_mode" [drivers/tty/serial/qcom_geni_serial.ko] 
> undefined!
> ERROR: "geni_se_init" [drivers/tty/serial/qcom_geni_serial.ko] undefined!
> ERROR: "geni_se_config_packing" [drivers/tty/serial/qcom_geni_serial.ko] 
> undefined!
> ERROR: "geni_se_resources_on" [drivers/tty/serial/qcom_geni_serial.ko] 
> undefined!
> ERROR: "geni_se_resources_off" [drivers/tty/serial/qcom_geni_serial.ko] 
> undefined!
> ERROR: "geni_se_tx_dma_unprep" [drivers/i2c/busses/i2c-qcom-geni.ko] 
> undefined!
> ERROR: "geni_se_tx_dma_prep" [drivers/i2c/busses/i2c-qcom-geni.ko] undefined!
> ERROR: "geni_se_rx_dma_unprep" [drivers/i2c/busses/i2c-qcom-geni.ko] 
> undefined!
> ERROR: "geni_se_rx_dma_prep" [drivers/i2c/busses/i2c-qcom-geni.ko] undefined!
> ERROR: "geni_se_select_mode" [drivers/i2c/busses/i2c-qcom-geni.ko] undefined!
> ERROR: "geni_se_config_packing" [drivers/i2c/busses/i2c-qcom-geni.ko] 
> undefined!
> ERROR: "geni_se_init" [drivers/i2c/busses/i2c-qcom-geni.ko] undefined!
> ERROR: "geni_se_resources_off" [drivers/i2c/busses/i2c-qcom-geni.ko] 
> undefined!
> ERROR: "geni_se_resources_on" [drivers/i2c/busses/i2c-qcom-geni.ko] undefined!
> make[1]: *** [scripts/Makefile.modpost:92: __modpost] Error 1
> make: *** [Makefile:1237: modules] Error 2
> 
> [1] https://patchwork.ozlabs.org/cover/893437/
> [2] https://lkml.org/lkml/2018/4/10/714
> 

Thanks, we have been working around this for a while now, let's just fix
it!

Reviewed-by: Bjorn Andersson 

Regards,
Bjorn

> Signed-off-by: Guenter Roeck 
> Signed-off-by: Evan Green 
> ---
>  drivers/soc/Makefile | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/soc/Makefile b/drivers/soc/Makefile
> index 40523577bdaa..113e884697fd 100644
> --- a/drivers/soc/Makefile
> +++ b/drivers/soc/Makefile
> @@ -14,7 +14,7 @@ obj-$(CONFIG_ARCH_MXC)  += imx/
>  obj-$(CONFIG_SOC_XWAY)   += lantiq/
>  obj-y+= mediatek/
>  obj-$(CONFIG_ARCH_MESON) += amlogic/
> -obj-$(CONFIG_ARCH_QCOM)  += qcom/
> +obj-y+= qcom/
>  obj-y+= renesas/
>  obj-$(CONFIG_ARCH_ROCKCHIP)  += rockchip/
>  obj-$(CONFIG_SOC_SAMSUNG)+= samsung/
> -- 
> 2.17.0.484.g0c8726318c-goog
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] soc: Unconditionally include qcom Makefile

[Bjorn Andersson]

On Wed 18 Apr 16:28 PDT 2018, Evan Green wrote:

> From: Guenter Roeck 
> 
> Incoming Qualcomm changes for GENI, i2c [1], and cmd-db [2] are enabled
> with COMPILE_TEST in drivers/soc/qcom. For this to work, the Makefile
> in that directory has to be included unconditionally, rather than only
> if ARCH_QCOM is enabled.
> 
> Example of the errors seen on allmodconfig with the GENI, i2c, and
> cmd-db patches applied:
> 
> Kernel: arch/x86/boot/bzImage is ready  (#1)
> ERROR: "geni_se_select_mode" [drivers/tty/serial/qcom_geni_serial.ko] 
> undefined!
> ERROR: "geni_se_init" [drivers/tty/serial/qcom_geni_serial.ko] undefined!
> ERROR: "geni_se_config_packing" [drivers/tty/serial/qcom_geni_serial.ko] 
> undefined!
> ERROR: "geni_se_resources_on" [drivers/tty/serial/qcom_geni_serial.ko] 
> undefined!
> ERROR: "geni_se_resources_off" [drivers/tty/serial/qcom_geni_serial.ko] 
> undefined!
> ERROR: "geni_se_tx_dma_unprep" [drivers/i2c/busses/i2c-qcom-geni.ko] 
> undefined!
> ERROR: "geni_se_tx_dma_prep" [drivers/i2c/busses/i2c-qcom-geni.ko] undefined!
> ERROR: "geni_se_rx_dma_unprep" [drivers/i2c/busses/i2c-qcom-geni.ko] 
> undefined!
> ERROR: "geni_se_rx_dma_prep" [drivers/i2c/busses/i2c-qcom-geni.ko] undefined!
> ERROR: "geni_se_select_mode" [drivers/i2c/busses/i2c-qcom-geni.ko] undefined!
> ERROR: "geni_se_config_packing" [drivers/i2c/busses/i2c-qcom-geni.ko] 
> undefined!
> ERROR: "geni_se_init" [drivers/i2c/busses/i2c-qcom-geni.ko] undefined!
> ERROR: "geni_se_resources_off" [drivers/i2c/busses/i2c-qcom-geni.ko] 
> undefined!
> ERROR: "geni_se_resources_on" [drivers/i2c/busses/i2c-qcom-geni.ko] undefined!
> make[1]: *** [scripts/Makefile.modpost:92: __modpost] Error 1
> make: *** [Makefile:1237: modules] Error 2
> 
> [1] https://patchwork.ozlabs.org/cover/893437/
> [2] https://lkml.org/lkml/2018/4/10/714
> 

Thanks, we have been working around this for a while now, let's just fix
it!

Reviewed-by: Bjorn Andersson 

Regards,
Bjorn

> Signed-off-by: Guenter Roeck 
> Signed-off-by: Evan Green 
> ---
>  drivers/soc/Makefile | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/soc/Makefile b/drivers/soc/Makefile
> index 40523577bdaa..113e884697fd 100644
> --- a/drivers/soc/Makefile
> +++ b/drivers/soc/Makefile
> @@ -14,7 +14,7 @@ obj-$(CONFIG_ARCH_MXC)  += imx/
>  obj-$(CONFIG_SOC_XWAY)   += lantiq/
>  obj-y+= mediatek/
>  obj-$(CONFIG_ARCH_MESON) += amlogic/
> -obj-$(CONFIG_ARCH_QCOM)  += qcom/
> +obj-y+= qcom/
>  obj-y+= renesas/
>  obj-$(CONFIG_ARCH_ROCKCHIP)  += rockchip/
>  obj-$(CONFIG_SOC_SAMSUNG)+= samsung/
> -- 
> 2.17.0.484.g0c8726318c-goog
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH v2] storvsc: Set up correct queue depth values for IDE devices

[Long Li]

> Subject: Re: [PATCH v2] storvsc: Set up correct queue depth values for IDE
> devices
> 
> 
> Long,
> 
> > Can you take a look at the following patch?
> 
> >> > + max_sub_channels =
> >> > +(num_cpus - 1) / storvsc_vcpus_per_sub_channel;
> 
> What happens if num_cpus = 1?

If num_cpus=1, we don't have any sub channels.

The host offers one sub channel for VM with 5 CPUs, after that it offers an 
additional sub channel every 4 CPUs.

The primary channel is always offered.

> 
> --
> Martin K. PetersenOracle Linux Engineering

RE: [PATCH v2] storvsc: Set up correct queue depth values for IDE devices

[Long Li]

> Subject: Re: [PATCH v2] storvsc: Set up correct queue depth values for IDE
> devices
> 
> 
> Long,
> 
> > Can you take a look at the following patch?
> 
> >> > + max_sub_channels =
> >> > +(num_cpus - 1) / storvsc_vcpus_per_sub_channel;
> 
> What happens if num_cpus = 1?

If num_cpus=1, we don't have any sub channels.

The host offers one sub channel for VM with 5 CPUs, after that it offers an 
additional sub channel every 4 CPUs.

The primary channel is always offered.

> 
> --
> Martin K. PetersenOracle Linux Engineering

Re: [PATCH] checkpatch: Add a --strict test for structs with bool member definitions

[Joe Perches]

On Thu, 2018-04-19 at 06:40 +0200, Julia Lawall wrote:
> 
> On Wed, 18 Apr 2018, Joe Perches wrote:
> 
> > On Tue, 2018-04-17 at 17:07 +0800, yuank...@codeaurora.org wrote:
> > > Hi julia,
> > > 
> > > On 2018-04-15 05:19 AM, Julia Lawall wrote:
> > > > On Wed, 11 Apr 2018, Joe Perches wrote:
> > > > 
> > > > > On Thu, 2018-04-12 at 08:22 +0200, Julia Lawall wrote:
> > > > > > On Wed, 11 Apr 2018, Joe Perches wrote:
> > > > > > > On Wed, 2018-04-11 at 09:29 -0700, Andrew Morton wrote:
> > > > > > > > We already have some 500 bools-in-structs
> > > > > > > 
> > > > > > > I got at least triple that only in include/
> > > > > > > so I expect there are at probably an order
> > > > > > > of magnitude more than 500 in the kernel.
> > > > > > > 
> > > > > > > I suppose some cocci script could count the
> > > > > > > actual number of instances.  A regex can not.
> > > > > > 
> > > > > > I got 12667.
> > > > > 
> > > > > Could you please post the cocci script?
> > > > > 
> > > > > > I'm not sure to understand the issue.  Will using a bitfield help 
> > > > > > if there
> > > > > > are no other bitfields in the structure?
> > > > > 
> > > > > IMO, not really.
> > > > > 
> > > > > The primary issue is described by Linus here:
> > > > > https://lkml.org/lkml/2017/11/21/384
> > > > > 
> > > > > I personally do not find a significant issue with
> > > > > uncontrolled sizes of bool in kernel structs as
> > > > > all of the kernel structs are transitory and not
> > > > > written out to storage.
> > > > > 
> > > > > I suppose bool bitfields are also OK, but for the
> > > > > RMW required.
> > > > > 
> > > > > Using unsigned int :1 bitfield instead of bool :1
> > > > > has the negative of truncation so that the uint
> > > > > has to be set with !! instead of a simple assign.
> > > > 
> > > > At least with gcc 5.4.0, a number of structures become larger with
> > > > unsigned int :1. bool:1 seems to mostly solve this problem.  The
> > > > structure
> > > > ichx_desc, defined in drivers/gpio/gpio-ich.c seems to become larger
> > > > with
> > > > both approaches.
> > > 
> > > [ZJ] Hopefully, this could make it better in your environment.
> > >   IMHO, this is just for double check.
> > 
> > I doubt this is actually better or smaller code.
> > 
> > Check the actual object code using objdump and the
> > struct alignment using pahole.
> 
> I didn't have a chance to try it, but it looks quite likely to result in a
> smaller data structure based on the other examples that I looked at.

I _really_ doubt there is any difference in size between the
below in any architecture

struct foo {
int bar;
bool baz:1;
int qux;
};

and

struct foo {
int bar;
bool baz;
int qux;
};

Where there would be a difference in size is

struct foo {
int bar;
bool baz1:1;
bool baz2:1;
int qux;
};

and

struct foo {
int bar;
bool baz1;
bool baz2;

int qux;
};

Re: [PATCH] checkpatch: Add a --strict test for structs with bool member definitions

[Joe Perches]

On Thu, 2018-04-19 at 06:40 +0200, Julia Lawall wrote:
> 
> On Wed, 18 Apr 2018, Joe Perches wrote:
> 
> > On Tue, 2018-04-17 at 17:07 +0800, yuank...@codeaurora.org wrote:
> > > Hi julia,
> > > 
> > > On 2018-04-15 05:19 AM, Julia Lawall wrote:
> > > > On Wed, 11 Apr 2018, Joe Perches wrote:
> > > > 
> > > > > On Thu, 2018-04-12 at 08:22 +0200, Julia Lawall wrote:
> > > > > > On Wed, 11 Apr 2018, Joe Perches wrote:
> > > > > > > On Wed, 2018-04-11 at 09:29 -0700, Andrew Morton wrote:
> > > > > > > > We already have some 500 bools-in-structs
> > > > > > > 
> > > > > > > I got at least triple that only in include/
> > > > > > > so I expect there are at probably an order
> > > > > > > of magnitude more than 500 in the kernel.
> > > > > > > 
> > > > > > > I suppose some cocci script could count the
> > > > > > > actual number of instances.  A regex can not.
> > > > > > 
> > > > > > I got 12667.
> > > > > 
> > > > > Could you please post the cocci script?
> > > > > 
> > > > > > I'm not sure to understand the issue.  Will using a bitfield help 
> > > > > > if there
> > > > > > are no other bitfields in the structure?
> > > > > 
> > > > > IMO, not really.
> > > > > 
> > > > > The primary issue is described by Linus here:
> > > > > https://lkml.org/lkml/2017/11/21/384
> > > > > 
> > > > > I personally do not find a significant issue with
> > > > > uncontrolled sizes of bool in kernel structs as
> > > > > all of the kernel structs are transitory and not
> > > > > written out to storage.
> > > > > 
> > > > > I suppose bool bitfields are also OK, but for the
> > > > > RMW required.
> > > > > 
> > > > > Using unsigned int :1 bitfield instead of bool :1
> > > > > has the negative of truncation so that the uint
> > > > > has to be set with !! instead of a simple assign.
> > > > 
> > > > At least with gcc 5.4.0, a number of structures become larger with
> > > > unsigned int :1. bool:1 seems to mostly solve this problem.  The
> > > > structure
> > > > ichx_desc, defined in drivers/gpio/gpio-ich.c seems to become larger
> > > > with
> > > > both approaches.
> > > 
> > > [ZJ] Hopefully, this could make it better in your environment.
> > >   IMHO, this is just for double check.
> > 
> > I doubt this is actually better or smaller code.
> > 
> > Check the actual object code using objdump and the
> > struct alignment using pahole.
> 
> I didn't have a chance to try it, but it looks quite likely to result in a
> smaller data structure based on the other examples that I looked at.

I _really_ doubt there is any difference in size between the
below in any architecture

struct foo {
int bar;
bool baz:1;
int qux;
};

and

struct foo {
int bar;
bool baz;
int qux;
};

Where there would be a difference in size is

struct foo {
int bar;
bool baz1:1;
bool baz2:1;
int qux;
};

and

struct foo {
int bar;
bool baz1;
bool baz2;

int qux;
};

Re: [PATCH bpf-next 4/5] samples/bpf: Refine printing symbol for sampleip

[Alexei Starovoitov]

On Thu, Apr 19, 2018 at 09:34:05AM +0800, Leo Yan wrote:
> The code defines macro 'PAGE_OFFSET' and uses it to decide if the
> address is in kernel space or not.  But different architecture has
> different 'PAGE_OFFSET' so this program cannot be used for all
> platforms.
> 
> This commit changes to check returned pointer from ksym_search() to
> judge if the address falls into kernel space or not, and removes
> macro 'PAGE_OFFSET' as it isn't used anymore.  As result, this program
> has no architecture dependency.
> 
> Signed-off-by: Leo Yan 
> ---
>  samples/bpf/sampleip_user.c | 8 +++-
>  1 file changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/samples/bpf/sampleip_user.c b/samples/bpf/sampleip_user.c
> index 4ed690b..0eea1b3 100644
> --- a/samples/bpf/sampleip_user.c
> +++ b/samples/bpf/sampleip_user.c
> @@ -26,7 +26,6 @@
>  #define DEFAULT_FREQ 99
>  #define DEFAULT_SECS 5
>  #define MAX_IPS  8192
> -#define PAGE_OFFSET  0x8800
>  
>  static int nr_cpus;
>  
> @@ -107,14 +106,13 @@ static void print_ip_map(int fd)
>   /* sort and print */
>   qsort(counts, max, sizeof(struct ipcount), count_cmp);
>   for (i = 0; i < max; i++) {
> - if (counts[i].ip > PAGE_OFFSET) {
> - sym = ksym_search(counts[i].ip);

yes. it is x64 specific, since it's a sample code,
but simply removing it is not a fix.
It makes this sampleip code behaving incorrectly.
To do such 'cleanup of ksym' please refactor it in the true generic way,
so these ksym* helpers can work on all archs and put this new
functionality into selftests.
If you can convert these examples into proper self-checking tests
that run out of selftests that would be awesome.
Thanks!

Re: [PATCH bpf-next 4/5] samples/bpf: Refine printing symbol for sampleip

[Alexei Starovoitov]

On Thu, Apr 19, 2018 at 09:34:05AM +0800, Leo Yan wrote:
> The code defines macro 'PAGE_OFFSET' and uses it to decide if the
> address is in kernel space or not.  But different architecture has
> different 'PAGE_OFFSET' so this program cannot be used for all
> platforms.
> 
> This commit changes to check returned pointer from ksym_search() to
> judge if the address falls into kernel space or not, and removes
> macro 'PAGE_OFFSET' as it isn't used anymore.  As result, this program
> has no architecture dependency.
> 
> Signed-off-by: Leo Yan 
> ---
>  samples/bpf/sampleip_user.c | 8 +++-
>  1 file changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/samples/bpf/sampleip_user.c b/samples/bpf/sampleip_user.c
> index 4ed690b..0eea1b3 100644
> --- a/samples/bpf/sampleip_user.c
> +++ b/samples/bpf/sampleip_user.c
> @@ -26,7 +26,6 @@
>  #define DEFAULT_FREQ 99
>  #define DEFAULT_SECS 5
>  #define MAX_IPS  8192
> -#define PAGE_OFFSET  0x8800
>  
>  static int nr_cpus;
>  
> @@ -107,14 +106,13 @@ static void print_ip_map(int fd)
>   /* sort and print */
>   qsort(counts, max, sizeof(struct ipcount), count_cmp);
>   for (i = 0; i < max; i++) {
> - if (counts[i].ip > PAGE_OFFSET) {
> - sym = ksym_search(counts[i].ip);

yes. it is x64 specific, since it's a sample code,
but simply removing it is not a fix.
It makes this sampleip code behaving incorrectly.
To do such 'cleanup of ksym' please refactor it in the true generic way,
so these ksym* helpers can work on all archs and put this new
functionality into selftests.
If you can convert these examples into proper self-checking tests
that run out of selftests that would be awesome.
Thanks!

Re: [PATCH] checkpatch: Add a --strict test for structs with bool member definitions

[Julia Lawall]

On Wed, 18 Apr 2018, Joe Perches wrote:

> On Tue, 2018-04-17 at 17:07 +0800, yuank...@codeaurora.org wrote:
> > Hi julia,
> >
> > On 2018-04-15 05:19 AM, Julia Lawall wrote:
> > > On Wed, 11 Apr 2018, Joe Perches wrote:
> > >
> > > > On Thu, 2018-04-12 at 08:22 +0200, Julia Lawall wrote:
> > > > > On Wed, 11 Apr 2018, Joe Perches wrote:
> > > > > > On Wed, 2018-04-11 at 09:29 -0700, Andrew Morton wrote:
> > > > > > > We already have some 500 bools-in-structs
> > > > > >
> > > > > > I got at least triple that only in include/
> > > > > > so I expect there are at probably an order
> > > > > > of magnitude more than 500 in the kernel.
> > > > > >
> > > > > > I suppose some cocci script could count the
> > > > > > actual number of instances.  A regex can not.
> > > > >
> > > > > I got 12667.
> > > >
> > > > Could you please post the cocci script?
> > > >
> > > > > I'm not sure to understand the issue.  Will using a bitfield help if 
> > > > > there
> > > > > are no other bitfields in the structure?
> > > >
> > > > IMO, not really.
> > > >
> > > > The primary issue is described by Linus here:
> > > > https://lkml.org/lkml/2017/11/21/384
> > > >
> > > > I personally do not find a significant issue with
> > > > uncontrolled sizes of bool in kernel structs as
> > > > all of the kernel structs are transitory and not
> > > > written out to storage.
> > > >
> > > > I suppose bool bitfields are also OK, but for the
> > > > RMW required.
> > > >
> > > > Using unsigned int :1 bitfield instead of bool :1
> > > > has the negative of truncation so that the uint
> > > > has to be set with !! instead of a simple assign.
> > >
> > > At least with gcc 5.4.0, a number of structures become larger with
> > > unsigned int :1. bool:1 seems to mostly solve this problem.  The
> > > structure
> > > ichx_desc, defined in drivers/gpio/gpio-ich.c seems to become larger
> > > with
> > > both approaches.
> >
> > [ZJ] Hopefully, this could make it better in your environment.
> >   IMHO, this is just for double check.
>
> I doubt this is actually better or smaller code.
>
> Check the actual object code using objdump and the
> struct alignment using pahole.

I didn't have a chance to try it, but it looks quite likely to result in a
smaller data structure based on the other examples that I looked at.

julia

>
> > diff --git a/drivers/gpio/gpio-ich.c b/drivers/gpio/gpio-ich.c
> > index 4f6d643..b46e170 100644
> > --- a/drivers/gpio/gpio-ich.c
> > +++ b/drivers/gpio/gpio-ich.c
> > @@ -70,6 +70,18 @@ static const u8 avoton_reglen[3] = {
> >   #define ICHX_READ(reg, base_res)   inl((reg) + (base_res)->start)
> >
> >   struct ichx_desc {
> > +   /* GPO_BLINK is available on this chipset */
> > +   bool uses_gpe0:1;
> > +
> > +   /* Whether the chipset has GPIO in GPE0_STS in the PM IO region
> > */
> > +bool uses_gpe0:1;
> > +
> > +/*
> > + * Some chipsets don't let reading output values on GPIO_LVL
> > register
> > + * this option allows driver caching written output values
> > + */
> > +bool use_outlvl_cache:1;
> > +
> >  /* Max GPIO pins the chipset can have */
> >  uint ngpio;
> >
> > @@ -77,24 +89,12 @@ struct ichx_desc {
> >  const u8 (*regs)[3];
> >  const u8 *reglen;
> >
> > -   /* GPO_BLINK is available on this chipset */
> > -   bool have_blink;
> > -
> > -   /* Whether the chipset has GPIO in GPE0_STS in the PM IO region
> > */
> > -   bool uses_gpe0;
> > -
> >  /* USE_SEL is bogus on some chipsets, eg 3100 */
> >  u32 use_sel_ignore[3];
> >
> >  /* Some chipsets have quirks, let these use their own
> > request/get */
> >  int (*request)(struct gpio_chip *chip, unsigned offset);
> >  int (*get)(struct gpio_chip *chip, unsigned offset);
> > -
> > -   /*
> > -* Some chipsets don't let reading output values on GPIO_LVL
> > register
> > -* this option allows driver caching written output values
> > -*/
> > -   bool use_outlvl_cache;
> >   };
> >
> >
> > ZJ
>

Re: [PATCH] checkpatch: Add a --strict test for structs with bool member definitions

[Julia Lawall]

On Wed, 18 Apr 2018, Joe Perches wrote:

> On Tue, 2018-04-17 at 17:07 +0800, yuank...@codeaurora.org wrote:
> > Hi julia,
> >
> > On 2018-04-15 05:19 AM, Julia Lawall wrote:
> > > On Wed, 11 Apr 2018, Joe Perches wrote:
> > >
> > > > On Thu, 2018-04-12 at 08:22 +0200, Julia Lawall wrote:
> > > > > On Wed, 11 Apr 2018, Joe Perches wrote:
> > > > > > On Wed, 2018-04-11 at 09:29 -0700, Andrew Morton wrote:
> > > > > > > We already have some 500 bools-in-structs
> > > > > >
> > > > > > I got at least triple that only in include/
> > > > > > so I expect there are at probably an order
> > > > > > of magnitude more than 500 in the kernel.
> > > > > >
> > > > > > I suppose some cocci script could count the
> > > > > > actual number of instances.  A regex can not.
> > > > >
> > > > > I got 12667.
> > > >
> > > > Could you please post the cocci script?
> > > >
> > > > > I'm not sure to understand the issue.  Will using a bitfield help if 
> > > > > there
> > > > > are no other bitfields in the structure?
> > > >
> > > > IMO, not really.
> > > >
> > > > The primary issue is described by Linus here:
> > > > https://lkml.org/lkml/2017/11/21/384
> > > >
> > > > I personally do not find a significant issue with
> > > > uncontrolled sizes of bool in kernel structs as
> > > > all of the kernel structs are transitory and not
> > > > written out to storage.
> > > >
> > > > I suppose bool bitfields are also OK, but for the
> > > > RMW required.
> > > >
> > > > Using unsigned int :1 bitfield instead of bool :1
> > > > has the negative of truncation so that the uint
> > > > has to be set with !! instead of a simple assign.
> > >
> > > At least with gcc 5.4.0, a number of structures become larger with
> > > unsigned int :1. bool:1 seems to mostly solve this problem.  The
> > > structure
> > > ichx_desc, defined in drivers/gpio/gpio-ich.c seems to become larger
> > > with
> > > both approaches.
> >
> > [ZJ] Hopefully, this could make it better in your environment.
> >   IMHO, this is just for double check.
>
> I doubt this is actually better or smaller code.
>
> Check the actual object code using objdump and the
> struct alignment using pahole.

I didn't have a chance to try it, but it looks quite likely to result in a
smaller data structure based on the other examples that I looked at.

julia

>
> > diff --git a/drivers/gpio/gpio-ich.c b/drivers/gpio/gpio-ich.c
> > index 4f6d643..b46e170 100644
> > --- a/drivers/gpio/gpio-ich.c
> > +++ b/drivers/gpio/gpio-ich.c
> > @@ -70,6 +70,18 @@ static const u8 avoton_reglen[3] = {
> >   #define ICHX_READ(reg, base_res)   inl((reg) + (base_res)->start)
> >
> >   struct ichx_desc {
> > +   /* GPO_BLINK is available on this chipset */
> > +   bool uses_gpe0:1;
> > +
> > +   /* Whether the chipset has GPIO in GPE0_STS in the PM IO region
> > */
> > +bool uses_gpe0:1;
> > +
> > +/*
> > + * Some chipsets don't let reading output values on GPIO_LVL
> > register
> > + * this option allows driver caching written output values
> > + */
> > +bool use_outlvl_cache:1;
> > +
> >  /* Max GPIO pins the chipset can have */
> >  uint ngpio;
> >
> > @@ -77,24 +89,12 @@ struct ichx_desc {
> >  const u8 (*regs)[3];
> >  const u8 *reglen;
> >
> > -   /* GPO_BLINK is available on this chipset */
> > -   bool have_blink;
> > -
> > -   /* Whether the chipset has GPIO in GPE0_STS in the PM IO region
> > */
> > -   bool uses_gpe0;
> > -
> >  /* USE_SEL is bogus on some chipsets, eg 3100 */
> >  u32 use_sel_ignore[3];
> >
> >  /* Some chipsets have quirks, let these use their own
> > request/get */
> >  int (*request)(struct gpio_chip *chip, unsigned offset);
> >  int (*get)(struct gpio_chip *chip, unsigned offset);
> > -
> > -   /*
> > -* Some chipsets don't let reading output values on GPIO_LVL
> > register
> > -* this option allows driver caching written output values
> > -*/
> > -   bool use_outlvl_cache;
> >   };
> >
> >
> > ZJ
>

Re: issues with suspend on Dell XPS 13 2-in-1

[Pandruvada, Srinivas]

On Wed, 2018-04-18 at 21:01 +, mario.limoncie...@dell.com wrote:

> > 
> > > [...]

> > > Srinivas,
> > > 
> > > Do you know why Runtime PM is defaulting to disabled for all of
> > > these
> > > devices?  Is that a default kernel policy problem or a distro
> > > policy
> > > problem?
> > 
> > This is default kernel policy.
> 
> Why is policy set this way by default in kernel?  Could we discuss to
> change
> Default kernel policy so that users can get better power consumption
> by default
Good question. This was discussed before, the problem is that it can
cause some issues with legacy devices. It should be better done in user
 space by some policy in gnome power manager, which user can
enable/disable.

> 
> I think it will be especially important as more machines continue to
> adopt
> suspend-to-idle.
Those should impact opportunistic idle since for S2I regular suspend
callbacks are called. In my 9365, I can get PC10 during S2I path
without adjusting them. Only problematic is systems with SATA, where we
need to enable devsleep.

> 
> > I suggest run
> > #turbostat
> > then suspend and wake
> > when wake up let the turbostat collect data for next sampling
> > interval.
> > We have to see what are PkgC% residencies are?
> 
> Assuming you mean for Dan to run this after he adjusts all the PM
> settings
> for those options power top called out, right?
It is better.
May be some device is not behaving correctly. So he can try with
powertop --auto-tune and check turbostat and see status of PkgC states.

Thanks,
Srinivas

> 
> > 
> > Thanks,
> > Srinivas
> > 
> > > 
> > > 
> > > >    Good  Bluetooth device interface status
> > > >    Good  Enable Audio codec power management
> > > >    Good  Runtime PM for I2C Adapter i2c-8 (Synopsys
> > > > DesignWare
> > > > I2C adapter)
> > > >    Good  Autosuspend for USB device
> > > > Integrated_Webcam_HD
> > > > [CNFGE16N092020028362]
> > > >    Good  Autosuspend for USB device xHCI Host
> > > > Controller
> > > > [usb1]
> > > >    Good  Autosuspend for USB device xHCI Host
> > > > Controller
> > > > [usb2]
> > > >    Good  Runtime PM for I2C Adapter i2c-7 (SMBus I801
> > > > adapter
> > > > at efa0)
> > > >    Good  Autosuspend for unknown USB device 1-2
> > > > (8087:0a2b)
> > > >    Good  Runtime PM for I2C Adapter i2c-6 (Synopsys
> > > > DesignWare
> > > > I2C adapter)
> > > >    Good  I2C Device i2c-DLL077A:01 has no runtime power
> > > > management
> > > >    Good  I2C Device i2c-WCOM482F:00 has no runtime
> > > > power
> > > > management
> > > >    Good  Runtime PM for PCI Device Intel Corporation
> > > > Sunrise
> > > > Point-LP PCI Express Root Port #10
> > > >    Good  Runtime PM for PCI Device Intel Corporation
> > > > Sunrise
> > > > Point-LP CSME HECI #1
> > > >    Good  Runtime PM for PCI Device Intel Corporation
> > > > Sunrise
> > > > Point-LP Serial IO I2C Controller #1
> > > >    Good  Runtime PM for PCI Device Intel Corporation
> > > > Sunrise
> > > > Point-LP SMBus
> > > >    Good  Runtime PM for PCI Device Intel Corporation
> > > > Sunrise
> > > > Point-LP PCI Express Root Port #5
> > > >    Good  Runtime PM for PCI Device Intel Corporation
> > > > Sunrise
> > > > Point-LP Serial IO I2C Controller #0
> > > >    Good  Wake-on-lan status for device virbr0-nic
> > > >    Good  Wake-on-lan status for device virbr0
> > > >    Good  Wake-on-lan status for device wlp60s0
> > > > 
> > > > Regards
> > > > 
> > > > Dennis

smime.p7s
Description: S/MIME cryptographic signature

Re: issues with suspend on Dell XPS 13 2-in-1

[Pandruvada, Srinivas]

On Wed, 2018-04-18 at 21:01 +, mario.limoncie...@dell.com wrote:

> > 
> > > [...]

> > > Srinivas,
> > > 
> > > Do you know why Runtime PM is defaulting to disabled for all of
> > > these
> > > devices?  Is that a default kernel policy problem or a distro
> > > policy
> > > problem?
> > 
> > This is default kernel policy.
> 
> Why is policy set this way by default in kernel?  Could we discuss to
> change
> Default kernel policy so that users can get better power consumption
> by default
Good question. This was discussed before, the problem is that it can
cause some issues with legacy devices. It should be better done in user
 space by some policy in gnome power manager, which user can
enable/disable.

> 
> I think it will be especially important as more machines continue to
> adopt
> suspend-to-idle.
Those should impact opportunistic idle since for S2I regular suspend
callbacks are called. In my 9365, I can get PC10 during S2I path
without adjusting them. Only problematic is systems with SATA, where we
need to enable devsleep.

> 
> > I suggest run
> > #turbostat
> > then suspend and wake
> > when wake up let the turbostat collect data for next sampling
> > interval.
> > We have to see what are PkgC% residencies are?
> 
> Assuming you mean for Dan to run this after he adjusts all the PM
> settings
> for those options power top called out, right?
It is better.
May be some device is not behaving correctly. So he can try with
powertop --auto-tune and check turbostat and see status of PkgC states.

Thanks,
Srinivas

> 
> > 
> > Thanks,
> > Srinivas
> > 
> > > 
> > > 
> > > >    Good  Bluetooth device interface status
> > > >    Good  Enable Audio codec power management
> > > >    Good  Runtime PM for I2C Adapter i2c-8 (Synopsys
> > > > DesignWare
> > > > I2C adapter)
> > > >    Good  Autosuspend for USB device
> > > > Integrated_Webcam_HD
> > > > [CNFGE16N092020028362]
> > > >    Good  Autosuspend for USB device xHCI Host
> > > > Controller
> > > > [usb1]
> > > >    Good  Autosuspend for USB device xHCI Host
> > > > Controller
> > > > [usb2]
> > > >    Good  Runtime PM for I2C Adapter i2c-7 (SMBus I801
> > > > adapter
> > > > at efa0)
> > > >    Good  Autosuspend for unknown USB device 1-2
> > > > (8087:0a2b)
> > > >    Good  Runtime PM for I2C Adapter i2c-6 (Synopsys
> > > > DesignWare
> > > > I2C adapter)
> > > >    Good  I2C Device i2c-DLL077A:01 has no runtime power
> > > > management
> > > >    Good  I2C Device i2c-WCOM482F:00 has no runtime
> > > > power
> > > > management
> > > >    Good  Runtime PM for PCI Device Intel Corporation
> > > > Sunrise
> > > > Point-LP PCI Express Root Port #10
> > > >    Good  Runtime PM for PCI Device Intel Corporation
> > > > Sunrise
> > > > Point-LP CSME HECI #1
> > > >    Good  Runtime PM for PCI Device Intel Corporation
> > > > Sunrise
> > > > Point-LP Serial IO I2C Controller #1
> > > >    Good  Runtime PM for PCI Device Intel Corporation
> > > > Sunrise
> > > > Point-LP SMBus
> > > >    Good  Runtime PM for PCI Device Intel Corporation
> > > > Sunrise
> > > > Point-LP PCI Express Root Port #5
> > > >    Good  Runtime PM for PCI Device Intel Corporation
> > > > Sunrise
> > > > Point-LP Serial IO I2C Controller #0
> > > >    Good  Wake-on-lan status for device virbr0-nic
> > > >    Good  Wake-on-lan status for device virbr0
> > > >    Good  Wake-on-lan status for device wlp60s0
> > > > 
> > > > Regards
> > > > 
> > > > Dennis

smime.p7s
Description: S/MIME cryptographic signature

linux-next: Tree for Apr 19

[Stephen Rothwell]

Hi all,

Changes since 20180418:

New tree: modules-fixes

I have added a patch to the arm-current tree to fix build problems
discovered overnight.

The sound-asoc tree lost its build failure.

Non-merge commits (relative to Linus' tree): 1055
 1121 files changed, 36028 insertions(+), 19043 deletions(-)



I have created today's linux-next tree at
git://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git
(patches at http://www.kernel.org/pub/linux/kernel/next/ ).  If you
are tracking the linux-next tree using git, you should not use "git pull"
to do so as that will try to merge the new linux-next release with the
old one.  You should use "git fetch" and checkout or reset to the new
master.

You can see which trees have been included by looking in the Next/Trees
file in the source.  There are also quilt-import.log and merge.log
files in the Next directory.  Between each merge, the tree was built
with a ppc64_defconfig for powerpc, an allmodconfig for x86_64, a
multi_v7_defconfig for arm and a native build of tools/perf. After
the final fixups (if any), I do an x86_64 modules_install followed by
builds for x86_64 allnoconfig, powerpc allnoconfig (32 and 64 bit),
ppc44x_defconfig, allyesconfig and pseries_le_defconfig and i386, sparc
and sparc64 defconfig. And finally, a simple boot test of the powerpc
pseries_le_defconfig kernel in qemu (with and without kvm enabled).

Below is a summary of the state of the merge.

I am currently merging 258 trees (counting Linus' and 44 trees of bug
fix patches pending for the current merge release).

Stats about the size of the tree over time can be seen at
http://neuling.org/linux-next-size.html .

Status of my local build tests will be at
http://kisskb.ellerman.id.au/linux-next .  If maintainers want to give
advice about cross compilers/configs that work, we are always open to add
more builds.

Thanks to Randy Dunlap for doing many randconfig builds.  And to Paul
Gortmaker for triage and bug fixes.

-- 
Cheers,
Stephen Rothwell

$ git checkout master
$ git reset --hard stable
Merging origin/master (a27fc14219f2 Merge branch 'parisc-4.17-3' of 
git://git.kernel.org/pub/scm/linux/kernel/git/deller/parisc-linux)
Merging fixes/master (147a89bc71e7 Merge tag 'kconfig-v4.17' of 
git://git.kernel.org/pub/scm/linux/kernel/git/masahiroy/linux-kbuild)
Merging kbuild-current/fixes (28913ee8191a netfilter: nf_nat_snmp_basic: add 
correct dependency to Makefile)
Merging arc-current/for-curr (661e50bc8532 Linux 4.16-rc4)
Merging arm-current/fixes (fe680ca02c1e ARM: replace unnecessary perl with sed 
and the shell $(( )) operator)
Applying: arm: check for A as well as B type sybols when calculating BSS size
Merging arm64-fixes/for-next/fixes (b2d71b3cda19 arm64: signal: don't force 
known signals to SIGKILL)
Merging m68k-current/for-linus (ecd685580c8f m68k/mac: Remove bogus "FIXME" 
comment)
Merging powerpc-fixes/fixes (9dfbf78e4114 powerpc/64s: Default l1d_size to 64K 
in RFI fallback flush)
Merging sparc/master (17dec0a94915 Merge branch 'userns-linus' of 
git://git.kernel.org/pub/scm/linux/kernel/git/ebiederm/user-namespace)
Merging fscrypt-current/for-stable (ae64f9bd1d36 Linux 4.15-rc2)
Merging net/master (81c895072d29 tun: fix vlan packet truncation)
Merging bpf/master (0a0a7e00a250 tools/bpf: fix test_sock and test_sock_addr.sh 
failure)
Merging ipsec/master (b48c05ab5d32 xfrm: Fix warning in xfrm6_tunnel_net_exit.)
Merging netfilter/master (765cca91b895 netfilter: conntrack: include kmemleak.h 
for kmemleak_not_leak())
Merging ipvs/master (f7fb77fc1235 netfilter: nft_compat: check extension hook 
mask only if set)
Merging wireless-drivers/master (77e30e10ee28 iwlwifi: mvm: query regdb for wmm 
rule if needed)
Merging mac80211/master (b5dbc28762fd Merge tag 'kbuild-fixes-v4.16-3' of 
git://git.kernel.org/pub/scm/linux/kernel/git/masahiroy/linux-kbuild)
Merging rdma-fixes/for-rc (60cc43fc8884 Linux 4.17-rc1)
Merging sound-current/for-linus (af52f9982e41 ALSA: hda - New VIA controller 
suppor no-snoop path)
Merging pci-current/for-linus (60cc43fc8884 Linux 4.17-rc1)
Merging driver-core.current/driver-core-linus (60cc43fc8884 Linux 4.17-rc1)
Merging tty.current/tty-linus (60cc43fc8884 Linux 4.17-rc1)
Merging usb.current/usb-linus (60cc43fc8884 Linux 4.17-rc1)
Merging usb-gadget-fixes/fixes (c6ba5084ce0d usb: gadget: udc: renesas_usb3: 
add binging for r8a77965)
Merging usb-serial-fixes/usb-linus (470b5d6f0cf4 USB: serial: ftdi_sio: use 
jtag quirk for Arrow USB Blaster)
Merging usb-chipidea-fixes/ci-for-usb-stable (964728f9f407 USB: chipidea: msm: 
fix ulpi-node lookup)
Merging phy/fixes (60cc43fc8884 Linux 4.17-rc1)
Merging staging.current/staging-linus (edf5c17d866e staging: irda: remove 
remaining remants of irda code removal)
Merging char-misc.current/char-misc-linus (60cc43fc8884 Linux 4.17-rc1)
Merging input-current/for-linus (664b0bae0b87 Merge branch 'next' i

linux-next: Tree for Apr 19

[Stephen Rothwell]

Hi all,

Changes since 20180418:

New tree: modules-fixes

I have added a patch to the arm-current tree to fix build problems
discovered overnight.

The sound-asoc tree lost its build failure.

Non-merge commits (relative to Linus' tree): 1055
 1121 files changed, 36028 insertions(+), 19043 deletions(-)



I have created today's linux-next tree at
git://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git
(patches at http://www.kernel.org/pub/linux/kernel/next/ ).  If you
are tracking the linux-next tree using git, you should not use "git pull"
to do so as that will try to merge the new linux-next release with the
old one.  You should use "git fetch" and checkout or reset to the new
master.

You can see which trees have been included by looking in the Next/Trees
file in the source.  There are also quilt-import.log and merge.log
files in the Next directory.  Between each merge, the tree was built
with a ppc64_defconfig for powerpc, an allmodconfig for x86_64, a
multi_v7_defconfig for arm and a native build of tools/perf. After
the final fixups (if any), I do an x86_64 modules_install followed by
builds for x86_64 allnoconfig, powerpc allnoconfig (32 and 64 bit),
ppc44x_defconfig, allyesconfig and pseries_le_defconfig and i386, sparc
and sparc64 defconfig. And finally, a simple boot test of the powerpc
pseries_le_defconfig kernel in qemu (with and without kvm enabled).

Below is a summary of the state of the merge.

I am currently merging 258 trees (counting Linus' and 44 trees of bug
fix patches pending for the current merge release).

Stats about the size of the tree over time can be seen at
http://neuling.org/linux-next-size.html .

Status of my local build tests will be at
http://kisskb.ellerman.id.au/linux-next .  If maintainers want to give
advice about cross compilers/configs that work, we are always open to add
more builds.

Thanks to Randy Dunlap for doing many randconfig builds.  And to Paul
Gortmaker for triage and bug fixes.

-- 
Cheers,
Stephen Rothwell

$ git checkout master
$ git reset --hard stable
Merging origin/master (a27fc14219f2 Merge branch 'parisc-4.17-3' of 
git://git.kernel.org/pub/scm/linux/kernel/git/deller/parisc-linux)
Merging fixes/master (147a89bc71e7 Merge tag 'kconfig-v4.17' of 
git://git.kernel.org/pub/scm/linux/kernel/git/masahiroy/linux-kbuild)
Merging kbuild-current/fixes (28913ee8191a netfilter: nf_nat_snmp_basic: add 
correct dependency to Makefile)
Merging arc-current/for-curr (661e50bc8532 Linux 4.16-rc4)
Merging arm-current/fixes (fe680ca02c1e ARM: replace unnecessary perl with sed 
and the shell $(( )) operator)
Applying: arm: check for A as well as B type sybols when calculating BSS size
Merging arm64-fixes/for-next/fixes (b2d71b3cda19 arm64: signal: don't force 
known signals to SIGKILL)
Merging m68k-current/for-linus (ecd685580c8f m68k/mac: Remove bogus "FIXME" 
comment)
Merging powerpc-fixes/fixes (9dfbf78e4114 powerpc/64s: Default l1d_size to 64K 
in RFI fallback flush)
Merging sparc/master (17dec0a94915 Merge branch 'userns-linus' of 
git://git.kernel.org/pub/scm/linux/kernel/git/ebiederm/user-namespace)
Merging fscrypt-current/for-stable (ae64f9bd1d36 Linux 4.15-rc2)
Merging net/master (81c895072d29 tun: fix vlan packet truncation)
Merging bpf/master (0a0a7e00a250 tools/bpf: fix test_sock and test_sock_addr.sh 
failure)
Merging ipsec/master (b48c05ab5d32 xfrm: Fix warning in xfrm6_tunnel_net_exit.)
Merging netfilter/master (765cca91b895 netfilter: conntrack: include kmemleak.h 
for kmemleak_not_leak())
Merging ipvs/master (f7fb77fc1235 netfilter: nft_compat: check extension hook 
mask only if set)
Merging wireless-drivers/master (77e30e10ee28 iwlwifi: mvm: query regdb for wmm 
rule if needed)
Merging mac80211/master (b5dbc28762fd Merge tag 'kbuild-fixes-v4.16-3' of 
git://git.kernel.org/pub/scm/linux/kernel/git/masahiroy/linux-kbuild)
Merging rdma-fixes/for-rc (60cc43fc8884 Linux 4.17-rc1)
Merging sound-current/for-linus (af52f9982e41 ALSA: hda - New VIA controller 
suppor no-snoop path)
Merging pci-current/for-linus (60cc43fc8884 Linux 4.17-rc1)
Merging driver-core.current/driver-core-linus (60cc43fc8884 Linux 4.17-rc1)
Merging tty.current/tty-linus (60cc43fc8884 Linux 4.17-rc1)
Merging usb.current/usb-linus (60cc43fc8884 Linux 4.17-rc1)
Merging usb-gadget-fixes/fixes (c6ba5084ce0d usb: gadget: udc: renesas_usb3: 
add binging for r8a77965)
Merging usb-serial-fixes/usb-linus (470b5d6f0cf4 USB: serial: ftdi_sio: use 
jtag quirk for Arrow USB Blaster)
Merging usb-chipidea-fixes/ci-for-usb-stable (964728f9f407 USB: chipidea: msm: 
fix ulpi-node lookup)
Merging phy/fixes (60cc43fc8884 Linux 4.17-rc1)
Merging staging.current/staging-linus (edf5c17d866e staging: irda: remove 
remaining remants of irda code removal)
Merging char-misc.current/char-misc-linus (60cc43fc8884 Linux 4.17-rc1)
Merging input-current/for-linus (664b0bae0b87 Merge branch 'next' i

Re: [PATCH v2 2/3] ASoC: amd: dma driver changes for BT I2S instance

[Mukunda,Vijendar]

On Thursday 19 April 2018 05:57 AM, Daniel Kurtz wrote:

Hi Vijendar,

On Wed, Apr 18, 2018 at 5:02 AM Vijendar Mukunda 
wrote:


With in ACP, There are three I2S controllers can be
configured/enabled ( I2S SP, I2S MICSP, I2S BT).
Default enabled I2S controller instance is I2S SP.
This patch provides required changes to support I2S BT
controller Instance.


I like the direction this patch is taking, but I think it would be easier
to review if you could split it into 2 parts:
   (1) the cleanup of the existing driver to use a simplified flow for
playback vs capture paths.
   (2) adding the BT I2S channel.


Thanks,
-Dan



Hi Dan,

I am fine with splitting the patch into two parts.

Thanks,
Vijendar

Re: [PATCH v2 2/3] ASoC: amd: dma driver changes for BT I2S instance

[Mukunda,Vijendar]

On Thursday 19 April 2018 05:57 AM, Daniel Kurtz wrote:

Hi Vijendar,

On Wed, Apr 18, 2018 at 5:02 AM Vijendar Mukunda 
wrote:


With in ACP, There are three I2S controllers can be
configured/enabled ( I2S SP, I2S MICSP, I2S BT).
Default enabled I2S controller instance is I2S SP.
This patch provides required changes to support I2S BT
controller Instance.


I like the direction this patch is taking, but I think it would be easier
to review if you could split it into 2 parts:
   (1) the cleanup of the existing driver to use a simplified flow for
playback vs capture paths.
   (2) adding the BT I2S channel.


Thanks,
-Dan



Hi Dan,

I am fine with splitting the patch into two parts.

Thanks,
Vijendar

Re: [PATCH 01/12] iscsi_tcp: don't set a bounce limit

[Martin K. Petersen]

Christoph,

> The default already is to never bounce, so the call is a no-op.

Applied to 4.18/scsi-queue. Thanks!

-- 
Martin K. Petersen  Oracle Linux Engineering

Re: [PATCH 01/12] iscsi_tcp: don't set a bounce limit

[Martin K. Petersen]

Christoph,

> The default already is to never bounce, so the call is a no-op.

Applied to 4.18/scsi-queue. Thanks!

-- 
Martin K. Petersen  Oracle Linux Engineering

Re: [PATCH 02/12] storsvc: don't set a bounce limit

[Martin K. Petersen]

Christoph,

> The default already is to never bounce, so the call is a no-op.

Applied to 4.18/scsi-queue. Thanks!

-- 
Martin K. Petersen  Oracle Linux Engineering

Re: [PATCH 02/12] storsvc: don't set a bounce limit

[Martin K. Petersen]

Christoph,

> The default already is to never bounce, so the call is a no-op.

Applied to 4.18/scsi-queue. Thanks!

-- 
Martin K. Petersen  Oracle Linux Engineering

Re: tg3 crashes under high load, when using 100Mbits

[Siva Reddy Kallam]

On Sat, Apr 14, 2018 at 9:17 PM, Kai-Heng Feng
 wrote:
> Hi Satish,
>
>> On 2018Mar21, at 00:57, Kai-Heng Feng  wrote:
>>
>> Satish Baddipadige  wrote:
>>
>>> On Thu, Feb 15, 2018 at 7:37 PM, Siva Reddy Kallam
>>>  wrote:
 On Mon, Feb 12, 2018 at 10:59 AM, Siva Reddy Kallam
  wrote:
> On Fri, Feb 9, 2018 at 10:41 AM, Kai Heng Feng
>  wrote:
>> Hi Broadcom folks,
>>
>> We are now enabling a new platform with tg3 nic, unfortunately we 
>> observed
>> the bug [1] that dated back to 2015.
>> I tried commit 4419bb1cedcd ("tg3: Add workaround to restrict 5762 MRRS 
>> to
>> 2048”) but it does’t work.
>>
>> Do you have any idea how to solve the issue?
>>
>> [1] https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1447664
>>
>> Kai-Heng
> Thank you for reporting. We will check and update you.
 With link aware mode, the clock speed could be slow and boot code does not
 complete within the expected time with lower link speeds. Need to override
 and the clock in driver. We are checking the feasibility of adding
 this in driver or firmware.
>>>
>>> Hi Kai-Heng,
>>>
>>> Can you please test the attached patch?
>>
>> I built a kernel and asked affected users to try.
>
> Users reported that the crash still happens with the patch.
>
> Kai-Heng
>
Thanks for the feedback. We will re-work on the patch and soon provide
you the update.
>>
>> Thanks for your work.
>>
>> Kai-Heng
>>
>>>
>>> Thanks,
>>> Satish
>>> 
>

Re: tg3 crashes under high load, when using 100Mbits

[Siva Reddy Kallam]

On Sat, Apr 14, 2018 at 9:17 PM, Kai-Heng Feng
 wrote:
> Hi Satish,
>
>> On 2018Mar21, at 00:57, Kai-Heng Feng  wrote:
>>
>> Satish Baddipadige  wrote:
>>
>>> On Thu, Feb 15, 2018 at 7:37 PM, Siva Reddy Kallam
>>>  wrote:
 On Mon, Feb 12, 2018 at 10:59 AM, Siva Reddy Kallam
  wrote:
> On Fri, Feb 9, 2018 at 10:41 AM, Kai Heng Feng
>  wrote:
>> Hi Broadcom folks,
>>
>> We are now enabling a new platform with tg3 nic, unfortunately we 
>> observed
>> the bug [1] that dated back to 2015.
>> I tried commit 4419bb1cedcd ("tg3: Add workaround to restrict 5762 MRRS 
>> to
>> 2048”) but it does’t work.
>>
>> Do you have any idea how to solve the issue?
>>
>> [1] https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1447664
>>
>> Kai-Heng
> Thank you for reporting. We will check and update you.
 With link aware mode, the clock speed could be slow and boot code does not
 complete within the expected time with lower link speeds. Need to override
 and the clock in driver. We are checking the feasibility of adding
 this in driver or firmware.
>>>
>>> Hi Kai-Heng,
>>>
>>> Can you please test the attached patch?
>>
>> I built a kernel and asked affected users to try.
>
> Users reported that the crash still happens with the patch.
>
> Kai-Heng
>
Thanks for the feedback. We will re-work on the patch and soon provide
you the update.
>>
>> Thanks for your work.
>>
>> Kai-Heng
>>
>>>
>>> Thanks,
>>> Satish
>>> 
>

Re: [PATCH 2/2] cpufreq: brcmstb-avs-cpufreq: prefer SCMI cpufreq if supported

[Viresh Kumar]

On 18-04-18, 08:56, Markus Mayer wrote:
> From: Jim Quinlan 
> 
> If the SCMI cpufreq driver is supported, we bail, so that the new
> approach can be used.
> 
> Signed-off-by: Jim Quinlan 
> Signed-off-by: Markus Mayer 
> ---
>  drivers/cpufreq/brcmstb-avs-cpufreq.c | 16 
>  1 file changed, 16 insertions(+)
> 
> diff --git a/drivers/cpufreq/brcmstb-avs-cpufreq.c 
> b/drivers/cpufreq/brcmstb-avs-cpufreq.c
> index b07559b9ed99..b4861a730162 100644
> --- a/drivers/cpufreq/brcmstb-avs-cpufreq.c
> +++ b/drivers/cpufreq/brcmstb-avs-cpufreq.c
> @@ -164,6 +164,8 @@
>  #define BRCM_AVS_CPU_INTR"brcm,avs-cpu-l2-intr"
>  #define BRCM_AVS_HOST_INTR   "sw_intr"
>  
> +#define ARM_SCMI_COMPAT  "arm,scmi"
> +
>  struct pmap {
>   unsigned int mode;
>   unsigned int p1;
> @@ -511,6 +513,20 @@ static int brcm_avs_prepare_init(struct platform_device 
> *pdev)
>   struct device *dev;
>   int host_irq, ret;
>  
> + /*
> +  * If the SCMI cpufreq driver is supported, we bail, so that the more
> +  * modern approach can be used.
> +  */
> + if (IS_ENABLED(CONFIG_ARM_SCMI_PROTOCOL)) {
> + struct device_node *np;
> +
> + np = of_find_compatible_node(NULL, NULL, ARM_SCMI_COMPAT);
> + if (np) {
> + of_node_put(np);
> + return -ENXIO;
> + }
> + }
> +

What about adding !CONFIG_ARM_SCMI_PROTOCOL in Kconfig dependency and don't
compile the driver at all ?

-- 
viresh

Re: [PATCH 2/2] cpufreq: brcmstb-avs-cpufreq: prefer SCMI cpufreq if supported

[Viresh Kumar]

On 18-04-18, 08:56, Markus Mayer wrote:
> From: Jim Quinlan 
> 
> If the SCMI cpufreq driver is supported, we bail, so that the new
> approach can be used.
> 
> Signed-off-by: Jim Quinlan 
> Signed-off-by: Markus Mayer 
> ---
>  drivers/cpufreq/brcmstb-avs-cpufreq.c | 16 
>  1 file changed, 16 insertions(+)
> 
> diff --git a/drivers/cpufreq/brcmstb-avs-cpufreq.c 
> b/drivers/cpufreq/brcmstb-avs-cpufreq.c
> index b07559b9ed99..b4861a730162 100644
> --- a/drivers/cpufreq/brcmstb-avs-cpufreq.c
> +++ b/drivers/cpufreq/brcmstb-avs-cpufreq.c
> @@ -164,6 +164,8 @@
>  #define BRCM_AVS_CPU_INTR"brcm,avs-cpu-l2-intr"
>  #define BRCM_AVS_HOST_INTR   "sw_intr"
>  
> +#define ARM_SCMI_COMPAT  "arm,scmi"
> +
>  struct pmap {
>   unsigned int mode;
>   unsigned int p1;
> @@ -511,6 +513,20 @@ static int brcm_avs_prepare_init(struct platform_device 
> *pdev)
>   struct device *dev;
>   int host_irq, ret;
>  
> + /*
> +  * If the SCMI cpufreq driver is supported, we bail, so that the more
> +  * modern approach can be used.
> +  */
> + if (IS_ENABLED(CONFIG_ARM_SCMI_PROTOCOL)) {
> + struct device_node *np;
> +
> + np = of_find_compatible_node(NULL, NULL, ARM_SCMI_COMPAT);
> + if (np) {
> + of_node_put(np);
> + return -ENXIO;
> + }
> + }
> +

What about adding !CONFIG_ARM_SCMI_PROTOCOL in Kconfig dependency and don't
compile the driver at all ?

-- 
viresh

Re: [PATCH 1/2] cpufreq: brcmstb-avs-cpufreq: remove development debug support

[Viresh Kumar]

On 18-04-18, 08:56, Markus Mayer wrote:
> From: Markus Mayer 
> 
> This debug code was helpful while developing the driver, but it isn't
> being used for anything anymore.
> 
> Signed-off-by: Markus Mayer 
> ---
>  drivers/cpufreq/Kconfig.arm   |  10 --
>  drivers/cpufreq/brcmstb-avs-cpufreq.c | 323 
> +-
>  2 files changed, 1 insertion(+), 332 deletions(-)

Acked-by: Viresh Kumar 

-- 
viresh

Re: [PATCH 1/2] cpufreq: brcmstb-avs-cpufreq: remove development debug support

[Viresh Kumar]

On 18-04-18, 08:56, Markus Mayer wrote:
> From: Markus Mayer 
> 
> This debug code was helpful while developing the driver, but it isn't
> being used for anything anymore.
> 
> Signed-off-by: Markus Mayer 
> ---
>  drivers/cpufreq/Kconfig.arm   |  10 --
>  drivers/cpufreq/brcmstb-avs-cpufreq.c | 323 
> +-
>  2 files changed, 1 insertion(+), 332 deletions(-)

Acked-by: Viresh Kumar 

-- 
viresh

Re: [PATCH 01/14] memory: ti-emif-sram: Add resume function to recopy sram code

[Keerthy]

On Thursday 12 April 2018 10:14 PM, santosh.shilim...@oracle.com wrote:
> On 4/11/18 9:53 PM, Keerthy wrote:
>> From: Dave Gerlach 
>>
>> After an RTC+DDR cycle we lose sram context so emif pm functions present
>> in sram are lost. We can check if the first byte of the original
>> code in DDR contains the same first byte as the code in sram, and if
>> they do not match we know we have lost context and must recopy the
>> functions to the previous address to maintain PM functionality.
>>
>> Signed-off-by: Dave Gerlach 
>> Signed-off-by: Keerthy 
>> ---
>>   drivers/memory/ti-emif-pm.c | 24 
>>   1 file changed, 24 insertions(+)
>>
>> diff --git a/drivers/memory/ti-emif-pm.c b/drivers/memory/ti-emif-pm.c
>> index 632651f..ec4a62c 100644
>> --- a/drivers/memory/ti-emif-pm.c
>> +++ b/drivers/memory/ti-emif-pm.c
>> @@ -249,6 +249,25 @@ int ti_emif_get_mem_type(void)
>>   };
>>   MODULE_DEVICE_TABLE(of, ti_emif_of_match);
>>   +#ifdef CONFIG_PM_SLEEP
>> +static int ti_emif_resume(struct device *dev)
>> +{
>> +    unsigned long tmp =
>> +    __raw_readl((void *)emif_instance->ti_emif_sram_virt);
>> +
>> +    /*
>> + * Check to see if what we are copying is already present in the
>> + * first byte at the destination, only copy if it is not which
>> + * indicates we have lost context and sram no longer contains
>> + * the PM code
>> + */
> 
>> +    if (tmp != ti_emif_sram)
>> +    ti_emif_push_sram(dev, emif_instance);
>> +
>> +    return 0;
>> +}
>> +#endif /* CONFIG_PM_SLEEP */
> Instead of this indirect method , why can't just check the previous
> deep sleep mode and based on that do copy or not. EMIF power status
> register should have something like that ?

I will check if we have a register that tells previous state of sram,
not sure of it.
> 
> Another minor point is even though there is nothing to do in suspend,
> might be good to have a callback with comment that nothing to do with
> some explanation why not. Don't have strong preference but may for
> better readability.
> 
> Regards,
> Santosh
> 
> 

Re: [PATCH 01/14] memory: ti-emif-sram: Add resume function to recopy sram code

[Keerthy]

On Thursday 12 April 2018 10:14 PM, santosh.shilim...@oracle.com wrote:
> On 4/11/18 9:53 PM, Keerthy wrote:
>> From: Dave Gerlach 
>>
>> After an RTC+DDR cycle we lose sram context so emif pm functions present
>> in sram are lost. We can check if the first byte of the original
>> code in DDR contains the same first byte as the code in sram, and if
>> they do not match we know we have lost context and must recopy the
>> functions to the previous address to maintain PM functionality.
>>
>> Signed-off-by: Dave Gerlach 
>> Signed-off-by: Keerthy 
>> ---
>>   drivers/memory/ti-emif-pm.c | 24 
>>   1 file changed, 24 insertions(+)
>>
>> diff --git a/drivers/memory/ti-emif-pm.c b/drivers/memory/ti-emif-pm.c
>> index 632651f..ec4a62c 100644
>> --- a/drivers/memory/ti-emif-pm.c
>> +++ b/drivers/memory/ti-emif-pm.c
>> @@ -249,6 +249,25 @@ int ti_emif_get_mem_type(void)
>>   };
>>   MODULE_DEVICE_TABLE(of, ti_emif_of_match);
>>   +#ifdef CONFIG_PM_SLEEP
>> +static int ti_emif_resume(struct device *dev)
>> +{
>> +    unsigned long tmp =
>> +    __raw_readl((void *)emif_instance->ti_emif_sram_virt);
>> +
>> +    /*
>> + * Check to see if what we are copying is already present in the
>> + * first byte at the destination, only copy if it is not which
>> + * indicates we have lost context and sram no longer contains
>> + * the PM code
>> + */
> 
>> +    if (tmp != ti_emif_sram)
>> +    ti_emif_push_sram(dev, emif_instance);
>> +
>> +    return 0;
>> +}
>> +#endif /* CONFIG_PM_SLEEP */
> Instead of this indirect method , why can't just check the previous
> deep sleep mode and based on that do copy or not. EMIF power status
> register should have something like that ?

I will check if we have a register that tells previous state of sram,
not sure of it.
> 
> Another minor point is even though there is nothing to do in suspend,
> might be good to have a callback with comment that nothing to do with
> some explanation why not. Don't have strong preference but may for
> better readability.
> 
> Regards,
> Santosh
> 
> 

Re: [PATCH v11 0/4] set VSESR_EL2 by user space and support NOTIFY_SEI notification

[gengdongjiu]

James,

> 
>> I do not know when it is merge-window. About the apply version, it does not 
>> have limited.
> 
> 'git fetch' Linus' tree and look at the tags. 'v4.16' lost its '-rc' suffixes,
> and there isn't a 'v4.17-rc1' yet, so we are still in the merge window.
> 
> Linus sends a message to LKML. eg:
> https://lkml.org/lkml/2018/4/1/175
> 
> net-next closes shortly before the merge window, and re-opens afterwards. 
> There
> is a handy web page:
> http://vger.kernel.org/~davem/net-next.html

 Thanks for this information, got it.

Re: [PATCH v11 0/4] set VSESR_EL2 by user space and support NOTIFY_SEI notification

[gengdongjiu]

James,

> 
>> I do not know when it is merge-window. About the apply version, it does not 
>> have limited.
> 
> 'git fetch' Linus' tree and look at the tags. 'v4.16' lost its '-rc' suffixes,
> and there isn't a 'v4.17-rc1' yet, so we are still in the merge window.
> 
> Linus sends a message to LKML. eg:
> https://lkml.org/lkml/2018/4/1/175
> 
> net-next closes shortly before the merge window, and re-opens afterwards. 
> There
> is a handy web page:
> http://vger.kernel.org/~davem/net-next.html

 Thanks for this information, got it.

Re: [PATCH] scsi: mptfc: fix spelling mistake in macro names

[Martin K. Petersen]

Colin,

> Rename macros MPI_FCPORTPAGE0_SUPPORT_SPEED_UKNOWN and
> MPI_FCPORTPAGE0_CURRENT_SPEED_UKNOWN to add in missing N in UNKNOWN

Applied to 4.18/scsi-queue. Thanks.

-- 
Martin K. Petersen  Oracle Linux Engineering

Re: [PATCH] scsi: mptfc: fix spelling mistake in macro names

[Martin K. Petersen]

Colin,

> Rename macros MPI_FCPORTPAGE0_SUPPORT_SPEED_UKNOWN and
> MPI_FCPORTPAGE0_CURRENT_SPEED_UKNOWN to add in missing N in UNKNOWN

Applied to 4.18/scsi-queue. Thanks.

-- 
Martin K. Petersen  Oracle Linux Engineering

Re: [PATCH] clk: aspeed: Support second reset register

[Andrew Jeffery]

On Thu, 19 Apr 2018, at 12:22, Joel Stanley wrote:
> The ast2500 has an additional reset register that contains resets not
> present in the ast2400. This enables support for this register, and adds
> the one reset line that is controlled by it.
> 
> Signed-off-by: Joel Stanley 

The duplication is a bit unfortunate, but all the same:

Reviewed-by: Andrew Jeffery 

> ---
>  drivers/clk/clk-aspeed.c | 44 +++-
>  include/dt-bindings/clock/aspeed-clock.h |  1 +
>  2 files changed, 37 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/clk/clk-aspeed.c b/drivers/clk/clk-aspeed.c
> index 5eb50c31e455..93d1dae96624 100644
> --- a/drivers/clk/clk-aspeed.c
> +++ b/drivers/clk/clk-aspeed.c
> @@ -16,6 +16,8 @@
>  
>  #define ASPEED_NUM_CLKS  35
>  
> +#define ASPEED_RESET2_OFFSET 32
> +
>  #define ASPEED_RESET_CTRL0x04
>  #define ASPEED_CLK_SELECTION 0x08
>  #define ASPEED_CLK_STOP_CTRL 0x0c
> @@ -30,6 +32,7 @@
>  #define  CLKIN_25MHZ_EN  BIT(23)
>  #define  AST2400_CLK_SOURCE_SEL  BIT(18)
>  #define ASPEED_CLK_SELECTION_2   0xd8
> +#define ASPEED_RESET_CTRL2   0xd4
>  
>  /* Globally visible clocks */
>  static DEFINE_SPINLOCK(aspeed_clk_lock);
> @@ -291,6 +294,7 @@ struct aspeed_reset {
>  #define to_aspeed_reset(p) container_of((p), struct aspeed_reset, rcdev)
>  
>  static const u8 aspeed_resets[] = {
> + /* SCU04 resets */
>   [ASPEED_RESET_XDMA] = 25,
>   [ASPEED_RESET_MCTP] = 24,
>   [ASPEED_RESET_ADC]  = 23,
> @@ -300,38 +304,62 @@ static const u8 aspeed_resets[] = {
>   [ASPEED_RESET_PCIVGA]   =  8,
>   [ASPEED_RESET_I2C]  =  2,
>   [ASPEED_RESET_AHB]  =  1,
> +
> + /*
> +  * SCUD4 resets start at an offset to separate them from
> +  * the SCU04 resets.
> +  */
> + [ASPEED_RESET_CRT1] = ASPEED_RESET2_OFFSET + 5,
>  };
>  
>  static int aspeed_reset_deassert(struct reset_controller_dev *rcdev,
>unsigned long id)
>  {
>   struct aspeed_reset *ar = to_aspeed_reset(rcdev);
> - u32 rst = BIT(aspeed_resets[id]);
> + u32 reg = ASPEED_RESET_CTRL;
> + u32 bit = aspeed_resets[id];
> +
> + if (bit >= ASPEED_RESET2_OFFSET) {
> + bit -= ASPEED_RESET2_OFFSET;
> + reg = ASPEED_RESET_CTRL2;
> + }
>  
> - return regmap_update_bits(ar->map, ASPEED_RESET_CTRL, rst, 0);
> + return regmap_update_bits(ar->map, reg, BIT(bit), 0);
>  }
>  
>  static int aspeed_reset_assert(struct reset_controller_dev *rcdev,
>  unsigned long id)
>  {
>   struct aspeed_reset *ar = to_aspeed_reset(rcdev);
> - u32 rst = BIT(aspeed_resets[id]);
> + u32 reg = ASPEED_RESET_CTRL;
> + u32 bit = aspeed_resets[id];
>  
> - return regmap_update_bits(ar->map, ASPEED_RESET_CTRL, rst, rst);
> + if (bit >= ASPEED_RESET_CTRL2) {
> + bit -= ASPEED_RESET2_OFFSET;
> + reg = ASPEED_RESET_CTRL2;
> + }
> +
> + return regmap_update_bits(ar->map, reg, BIT(bit), BIT(bit));
>  }
>  
>  static int aspeed_reset_status(struct reset_controller_dev *rcdev,
>  unsigned long id)
>  {
>   struct aspeed_reset *ar = to_aspeed_reset(rcdev);
> - u32 val, rst = BIT(aspeed_resets[id]);
> - int ret;
> + u32 reg = ASPEED_RESET_CTRL;
> + u32 bit = aspeed_resets[id];
> + int ret, val;
> +
> + if (bit >= ASPEED_RESET2_OFFSET) {
> + bit -= ASPEED_RESET2_OFFSET;
> + reg = ASPEED_RESET_CTRL2;
> + }
>  
> - ret = regmap_read(ar->map, ASPEED_RESET_CTRL, );
> + ret = regmap_read(ar->map, reg, );
>   if (ret)
>   return ret;
>  
> - return !!(val & rst);
> + return !!(val & BIT(bit));
>  }
>  
>  static const struct reset_control_ops aspeed_reset_ops = {
> diff --git a/include/dt-bindings/clock/aspeed-clock.h b/include/dt-
> bindings/clock/aspeed-clock.h
> index d3558d897a4d..513c1b4af7a8 100644
> --- a/include/dt-bindings/clock/aspeed-clock.h
> +++ b/include/dt-bindings/clock/aspeed-clock.h
> @@ -48,5 +48,6 @@
>  #define ASPEED_RESET_PCIVGA  6
>  #define ASPEED_RESET_I2C 7
>  #define ASPEED_RESET_AHB 8
> +#define ASPEED_RESET_CRT19
>  
>  #endif
> -- 
> 2.17.0
> 

Re: [PATCH] clk: aspeed: Support second reset register

[Andrew Jeffery]

On Thu, 19 Apr 2018, at 12:22, Joel Stanley wrote:
> The ast2500 has an additional reset register that contains resets not
> present in the ast2400. This enables support for this register, and adds
> the one reset line that is controlled by it.
> 
> Signed-off-by: Joel Stanley 

The duplication is a bit unfortunate, but all the same:

Reviewed-by: Andrew Jeffery 

> ---
>  drivers/clk/clk-aspeed.c | 44 +++-
>  include/dt-bindings/clock/aspeed-clock.h |  1 +
>  2 files changed, 37 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/clk/clk-aspeed.c b/drivers/clk/clk-aspeed.c
> index 5eb50c31e455..93d1dae96624 100644
> --- a/drivers/clk/clk-aspeed.c
> +++ b/drivers/clk/clk-aspeed.c
> @@ -16,6 +16,8 @@
>  
>  #define ASPEED_NUM_CLKS  35
>  
> +#define ASPEED_RESET2_OFFSET 32
> +
>  #define ASPEED_RESET_CTRL0x04
>  #define ASPEED_CLK_SELECTION 0x08
>  #define ASPEED_CLK_STOP_CTRL 0x0c
> @@ -30,6 +32,7 @@
>  #define  CLKIN_25MHZ_EN  BIT(23)
>  #define  AST2400_CLK_SOURCE_SEL  BIT(18)
>  #define ASPEED_CLK_SELECTION_2   0xd8
> +#define ASPEED_RESET_CTRL2   0xd4
>  
>  /* Globally visible clocks */
>  static DEFINE_SPINLOCK(aspeed_clk_lock);
> @@ -291,6 +294,7 @@ struct aspeed_reset {
>  #define to_aspeed_reset(p) container_of((p), struct aspeed_reset, rcdev)
>  
>  static const u8 aspeed_resets[] = {
> + /* SCU04 resets */
>   [ASPEED_RESET_XDMA] = 25,
>   [ASPEED_RESET_MCTP] = 24,
>   [ASPEED_RESET_ADC]  = 23,
> @@ -300,38 +304,62 @@ static const u8 aspeed_resets[] = {
>   [ASPEED_RESET_PCIVGA]   =  8,
>   [ASPEED_RESET_I2C]  =  2,
>   [ASPEED_RESET_AHB]  =  1,
> +
> + /*
> +  * SCUD4 resets start at an offset to separate them from
> +  * the SCU04 resets.
> +  */
> + [ASPEED_RESET_CRT1] = ASPEED_RESET2_OFFSET + 5,
>  };
>  
>  static int aspeed_reset_deassert(struct reset_controller_dev *rcdev,
>unsigned long id)
>  {
>   struct aspeed_reset *ar = to_aspeed_reset(rcdev);
> - u32 rst = BIT(aspeed_resets[id]);
> + u32 reg = ASPEED_RESET_CTRL;
> + u32 bit = aspeed_resets[id];
> +
> + if (bit >= ASPEED_RESET2_OFFSET) {
> + bit -= ASPEED_RESET2_OFFSET;
> + reg = ASPEED_RESET_CTRL2;
> + }
>  
> - return regmap_update_bits(ar->map, ASPEED_RESET_CTRL, rst, 0);
> + return regmap_update_bits(ar->map, reg, BIT(bit), 0);
>  }
>  
>  static int aspeed_reset_assert(struct reset_controller_dev *rcdev,
>  unsigned long id)
>  {
>   struct aspeed_reset *ar = to_aspeed_reset(rcdev);
> - u32 rst = BIT(aspeed_resets[id]);
> + u32 reg = ASPEED_RESET_CTRL;
> + u32 bit = aspeed_resets[id];
>  
> - return regmap_update_bits(ar->map, ASPEED_RESET_CTRL, rst, rst);
> + if (bit >= ASPEED_RESET_CTRL2) {
> + bit -= ASPEED_RESET2_OFFSET;
> + reg = ASPEED_RESET_CTRL2;
> + }
> +
> + return regmap_update_bits(ar->map, reg, BIT(bit), BIT(bit));
>  }
>  
>  static int aspeed_reset_status(struct reset_controller_dev *rcdev,
>  unsigned long id)
>  {
>   struct aspeed_reset *ar = to_aspeed_reset(rcdev);
> - u32 val, rst = BIT(aspeed_resets[id]);
> - int ret;
> + u32 reg = ASPEED_RESET_CTRL;
> + u32 bit = aspeed_resets[id];
> + int ret, val;
> +
> + if (bit >= ASPEED_RESET2_OFFSET) {
> + bit -= ASPEED_RESET2_OFFSET;
> + reg = ASPEED_RESET_CTRL2;
> + }
>  
> - ret = regmap_read(ar->map, ASPEED_RESET_CTRL, );
> + ret = regmap_read(ar->map, reg, );
>   if (ret)
>   return ret;
>  
> - return !!(val & rst);
> + return !!(val & BIT(bit));
>  }
>  
>  static const struct reset_control_ops aspeed_reset_ops = {
> diff --git a/include/dt-bindings/clock/aspeed-clock.h b/include/dt-
> bindings/clock/aspeed-clock.h
> index d3558d897a4d..513c1b4af7a8 100644
> --- a/include/dt-bindings/clock/aspeed-clock.h
> +++ b/include/dt-bindings/clock/aspeed-clock.h
> @@ -48,5 +48,6 @@
>  #define ASPEED_RESET_PCIVGA  6
>  #define ASPEED_RESET_I2C 7
>  #define ASPEED_RESET_AHB 8
> +#define ASPEED_RESET_CRT19
>  
>  #endif
> -- 
> 2.17.0
> 

[PATCH 2/2] blkcg: init root blkcg_gq under lock

[Jiang Biao]

The initializing of q->root_blkg is currently outside of queue lock
and rcu, so the blkg may be destroied before the initializing, which
may cause dangling/null references. On the other side, the destroys
of blkg are protected by queue lock or rcu. Put the initializing
inside the queue lock and rcu to make it safer.

Signed-off-by: Jiang Biao 
Signed-off-by: Wen Yang 
CC: Tejun Heo 
CC: Jens Axboe 
---
 block/blk-cgroup.c | 17 +++--
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c
index 07e3359..ec86837 100644
--- a/block/blk-cgroup.c
+++ b/block/blk-cgroup.c
@@ -1138,18 +1138,16 @@ int blkcg_init_queue(struct request_queue *q)
rcu_read_lock();
spin_lock_irq(q->queue_lock);
blkg = blkg_create(_root, q, new_blkg);
+   if (IS_ERR(blkg))
+   goto err_unlock;
+   q->root_blkg = blkg;
+   q->root_rl.blkg = blkg;
spin_unlock_irq(q->queue_lock);
rcu_read_unlock();
 
if (preloaded)
radix_tree_preload_end();
 
-   if (IS_ERR(blkg))
-   return PTR_ERR(blkg);
-
-   q->root_blkg = blkg;
-   q->root_rl.blkg = blkg;
-
ret = blk_throtl_init(q);
if (ret) {
spin_lock_irq(q->queue_lock);
@@ -1157,6 +1155,13 @@ int blkcg_init_queue(struct request_queue *q)
spin_unlock_irq(q->queue_lock);
}
return ret;
+
+err_unlock:
+   spin_unlock_irq(q->queue_lock);
+   rcu_read_unlock();
+   if (preloaded)
+   radix_tree_preload_end();
+   return PTR_ERR(blkg);
 }
 
 /**
-- 
2.7.4

[PATCH 2/2] blkcg: init root blkcg_gq under lock

[Jiang Biao]

The initializing of q->root_blkg is currently outside of queue lock
and rcu, so the blkg may be destroied before the initializing, which
may cause dangling/null references. On the other side, the destroys
of blkg are protected by queue lock or rcu. Put the initializing
inside the queue lock and rcu to make it safer.

Signed-off-by: Jiang Biao 
Signed-off-by: Wen Yang 
CC: Tejun Heo 
CC: Jens Axboe 
---
 block/blk-cgroup.c | 17 +++--
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c
index 07e3359..ec86837 100644
--- a/block/blk-cgroup.c
+++ b/block/blk-cgroup.c
@@ -1138,18 +1138,16 @@ int blkcg_init_queue(struct request_queue *q)
rcu_read_lock();
spin_lock_irq(q->queue_lock);
blkg = blkg_create(_root, q, new_blkg);
+   if (IS_ERR(blkg))
+   goto err_unlock;
+   q->root_blkg = blkg;
+   q->root_rl.blkg = blkg;
spin_unlock_irq(q->queue_lock);
rcu_read_unlock();
 
if (preloaded)
radix_tree_preload_end();
 
-   if (IS_ERR(blkg))
-   return PTR_ERR(blkg);
-
-   q->root_blkg = blkg;
-   q->root_rl.blkg = blkg;
-
ret = blk_throtl_init(q);
if (ret) {
spin_lock_irq(q->queue_lock);
@@ -1157,6 +1155,13 @@ int blkcg_init_queue(struct request_queue *q)
spin_unlock_irq(q->queue_lock);
}
return ret;
+
+err_unlock:
+   spin_unlock_irq(q->queue_lock);
+   rcu_read_unlock();
+   if (preloaded)
+   radix_tree_preload_end();
+   return PTR_ERR(blkg);
 }
 
 /**
-- 
2.7.4

[PATCH 1/2] blkcg: small fix on comment in blkcg_init_queue

[Jiang Biao]

The comment before blkg_create() in blkcg_init_queue() was moved
from blkcg_activate_policy() by commit ec13b1d6f0a0457312e615, but
it does not suit for the new context.

Signed-off-by: Jiang Biao 
Signed-off-by: Wen Yang 
CC: Tejun Heo 
CC: Jens Axboe 
---
 block/blk-cgroup.c | 6 +-
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c
index 2b7f8d0..07e3359 100644
--- a/block/blk-cgroup.c
+++ b/block/blk-cgroup.c
@@ -1134,11 +1134,7 @@ int blkcg_init_queue(struct request_queue *q)
 
preloaded = !radix_tree_preload(GFP_KERNEL);
 
-   /*
-* Make sure the root blkg exists and count the existing blkgs.  As
-* @q is bypassing at this point, blkg_lookup_create() can't be
-* used.  Open code insertion.
-*/
+   /* Make sure the root blkg exists. */
rcu_read_lock();
spin_lock_irq(q->queue_lock);
blkg = blkg_create(_root, q, new_blkg);
-- 
2.7.4

[PATCH 1/2] blkcg: small fix on comment in blkcg_init_queue

[Jiang Biao]

The comment before blkg_create() in blkcg_init_queue() was moved
from blkcg_activate_policy() by commit ec13b1d6f0a0457312e615, but
it does not suit for the new context.

Signed-off-by: Jiang Biao 
Signed-off-by: Wen Yang 
CC: Tejun Heo 
CC: Jens Axboe 
---
 block/blk-cgroup.c | 6 +-
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c
index 2b7f8d0..07e3359 100644
--- a/block/blk-cgroup.c
+++ b/block/blk-cgroup.c
@@ -1134,11 +1134,7 @@ int blkcg_init_queue(struct request_queue *q)
 
preloaded = !radix_tree_preload(GFP_KERNEL);
 
-   /*
-* Make sure the root blkg exists and count the existing blkgs.  As
-* @q is bypassing at this point, blkg_lookup_create() can't be
-* used.  Open code insertion.
-*/
+   /* Make sure the root blkg exists. */
rcu_read_lock();
spin_lock_irq(q->queue_lock);
blkg = blkg_create(_root, q, new_blkg);
-- 
2.7.4

[PATCH] net: hns: Avoid action name truncation

[dann frazier]

When longer interface names are used, the action names exposed in
/proc/interrupts and /proc/irq/* maybe truncated. For example, when
using the predictable name algorithm in systemd on a HiSilicon D05,
I see:

  ubuntu@d05-3:~$  grep enahisic2i0-tx /proc/interrupts | sed 's/.* //'
  enahisic2i0-tx0
  enahisic2i0-tx1
  [...]
  enahisic2i0-tx8
  enahisic2i0-tx9
  enahisic2i0-tx1
  enahisic2i0-tx1
  enahisic2i0-tx1
  enahisic2i0-tx1
  enahisic2i0-tx1
  enahisic2i0-tx1

Increase the max ring name length to allow for an interface name
of IFNAMSIZE. After this change, I now see:

  $ grep enahisic2i0-tx /proc/interrupts | sed 's/.* //'
  enahisic2i0-tx0
  enahisic2i0-tx1
  enahisic2i0-tx2
  [...]
  enahisic2i0-tx8
  enahisic2i0-tx9
  enahisic2i0-tx10
  enahisic2i0-tx11
  enahisic2i0-tx12
  enahisic2i0-tx13
  enahisic2i0-tx14
  enahisic2i0-tx15

Signed-off-by: dann frazier 
---
 drivers/net/ethernet/hisilicon/hns/hnae.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/hisilicon/hns/hnae.h 
b/drivers/net/ethernet/hisilicon/hns/hnae.h
index 3e62692af011..fa5b30f547f6 100644
--- a/drivers/net/ethernet/hisilicon/hns/hnae.h
+++ b/drivers/net/ethernet/hisilicon/hns/hnae.h
@@ -87,7 +87,7 @@ do { \
 
 #define HNAE_AE_REGISTER 0x1
 
-#define RCB_RING_NAME_LEN 16
+#define RCB_RING_NAME_LEN (IFNAMSIZ + 4)
 
 #define HNAE_LOWEST_LATENCY_COAL_PARAM 30
 #define HNAE_LOW_LATENCY_COAL_PARAM80
-- 
2.17.0

[PATCH] net: hns: Avoid action name truncation

[dann frazier]

When longer interface names are used, the action names exposed in
/proc/interrupts and /proc/irq/* maybe truncated. For example, when
using the predictable name algorithm in systemd on a HiSilicon D05,
I see:

  ubuntu@d05-3:~$  grep enahisic2i0-tx /proc/interrupts | sed 's/.* //'
  enahisic2i0-tx0
  enahisic2i0-tx1
  [...]
  enahisic2i0-tx8
  enahisic2i0-tx9
  enahisic2i0-tx1
  enahisic2i0-tx1
  enahisic2i0-tx1
  enahisic2i0-tx1
  enahisic2i0-tx1
  enahisic2i0-tx1

Increase the max ring name length to allow for an interface name
of IFNAMSIZE. After this change, I now see:

  $ grep enahisic2i0-tx /proc/interrupts | sed 's/.* //'
  enahisic2i0-tx0
  enahisic2i0-tx1
  enahisic2i0-tx2
  [...]
  enahisic2i0-tx8
  enahisic2i0-tx9
  enahisic2i0-tx10
  enahisic2i0-tx11
  enahisic2i0-tx12
  enahisic2i0-tx13
  enahisic2i0-tx14
  enahisic2i0-tx15

Signed-off-by: dann frazier 
---
 drivers/net/ethernet/hisilicon/hns/hnae.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/hisilicon/hns/hnae.h 
b/drivers/net/ethernet/hisilicon/hns/hnae.h
index 3e62692af011..fa5b30f547f6 100644
--- a/drivers/net/ethernet/hisilicon/hns/hnae.h
+++ b/drivers/net/ethernet/hisilicon/hns/hnae.h
@@ -87,7 +87,7 @@ do { \
 
 #define HNAE_AE_REGISTER 0x1
 
-#define RCB_RING_NAME_LEN 16
+#define RCB_RING_NAME_LEN (IFNAMSIZ + 4)
 
 #define HNAE_LOWEST_LATENCY_COAL_PARAM 30
 #define HNAE_LOW_LATENCY_COAL_PARAM80
-- 
2.17.0

Re: [PATCH] net: don't use kvzalloc for DMA memory

[Michael S. Tsirkin]

On Wed, Apr 18, 2018 at 01:38:43PM -0700, Eric Dumazet wrote:
> 
> 
> On 04/18/2018 10:55 AM, Michael S. Tsirkin wrote:
> 
> > Imagine you want to pass some data to card.
> > Natural thing is to just put it in a variable and start DMA.
> > However DMA API disallows stack access nowdays,
> > so it's natural to put this within struct device.
> > 
> > See e.g.
> > 
> > commit a725ee3e44e39dab1ec82cc745899a785d2a555e
> > Author: Andy Lutomirski 
> > Date:   Mon Jul 18 15:34:49 2016 -0700
> > 
> > virtio-net: Remove more stack DMA
> >
> 
> Andy just moved the problem to another one, since at that time we already
> had vmalloc() fallback for at least 2 years.
> 
> Note that my original patch had :
> 
> p = kzalloc(alloc_size, GFP_KERNEL | __GFP_NOWARN | __GFP_REPEAT);
> if (!p)
>   p = vzalloc(alloc_size);
> 
> So really, normal (less than PAGE_SIZE) allocations would have 
> almost-zero-chance to end up to vmalloc(one_page)

Thanks Eric, I'll fix virtio.


-- 
MST

Re: [PATCH] net: don't use kvzalloc for DMA memory

[Michael S. Tsirkin]

On Wed, Apr 18, 2018 at 01:38:43PM -0700, Eric Dumazet wrote:
> 
> 
> On 04/18/2018 10:55 AM, Michael S. Tsirkin wrote:
> 
> > Imagine you want to pass some data to card.
> > Natural thing is to just put it in a variable and start DMA.
> > However DMA API disallows stack access nowdays,
> > so it's natural to put this within struct device.
> > 
> > See e.g.
> > 
> > commit a725ee3e44e39dab1ec82cc745899a785d2a555e
> > Author: Andy Lutomirski 
> > Date:   Mon Jul 18 15:34:49 2016 -0700
> > 
> > virtio-net: Remove more stack DMA
> >
> 
> Andy just moved the problem to another one, since at that time we already
> had vmalloc() fallback for at least 2 years.
> 
> Note that my original patch had :
> 
> p = kzalloc(alloc_size, GFP_KERNEL | __GFP_NOWARN | __GFP_REPEAT);
> if (!p)
>   p = vzalloc(alloc_size);
> 
> So really, normal (less than PAGE_SIZE) allocations would have 
> almost-zero-chance to end up to vmalloc(one_page)

Thanks Eric, I'll fix virtio.


-- 
MST

Re: [PATCH v1 2/4] dt-bindings: clock: mediatek: add g3dsys bindings

[Sean Wang]

On Wed, 2018-04-18 at 20:18 -0700, Stephen Boyd wrote:
> Quoting sean.w...@mediatek.com (2018-04-18 03:24:54)
> > From: Sean Wang 
> > 
> > Add bindings to g3dsys providing necessary clock and reset control to
> > Mali-450.
> > 
> > Signed-off-by: Sean Wang 
> > ---
> >  .../bindings/arm/mediatek/mediatek,g3dsys.txt  | 30 
> > ++
> 
> Why isn't this under bindings/clock/ ?
> 

Tons of bindings for clock controller have been present at
binding/arm/mediatek. g3dsys just have a follow-up to them.

It's worth another patch moving them all from bindings/arm/mediatek/ to
bindings/clock/mediatek.

what's your opinion for either doing it prior to g3dsys binding being
added or doing it in the future ?

> 
> ___
> Linux-mediatek mailing list
> linux-media...@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-mediatek

Re: [PATCH v1 2/4] dt-bindings: clock: mediatek: add g3dsys bindings

[Sean Wang]

On Wed, 2018-04-18 at 20:18 -0700, Stephen Boyd wrote:
> Quoting sean.w...@mediatek.com (2018-04-18 03:24:54)
> > From: Sean Wang 
> > 
> > Add bindings to g3dsys providing necessary clock and reset control to
> > Mali-450.
> > 
> > Signed-off-by: Sean Wang 
> > ---
> >  .../bindings/arm/mediatek/mediatek,g3dsys.txt  | 30 
> > ++
> 
> Why isn't this under bindings/clock/ ?
> 

Tons of bindings for clock controller have been present at
binding/arm/mediatek. g3dsys just have a follow-up to them.

It's worth another patch moving them all from bindings/arm/mediatek/ to
bindings/clock/mediatek.

what's your opinion for either doing it prior to g3dsys binding being
added or doing it in the future ?

> 
> ___
> Linux-mediatek mailing list
> linux-media...@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-mediatek

Re: [PATCH -next] user_namespace: Replace gotos with return statements

[Eric W. Biederman]

Marcos Paulo de Souza  writes:

> Found while inspecting the code that handles the setgroups procfs
> file.

What perchance might be the advantage of introducing multiple exits
into proc_setgroups_write?

I strongly suspect that if you look at the generated code it will
be worse after your patch.


Eric

> Signed-off-by: Marcos Paulo de Souza 
> ---
> Tested locally setting up a new userns, and setting setgroups as deny and 
> allow,
> worked as before.
>
>  kernel/user_namespace.c | 20 +++-
>  1 file changed, 7 insertions(+), 13 deletions(-)
>
> diff --git a/kernel/user_namespace.c b/kernel/user_namespace.c
> index 246d4d4ce5c7..64a01254ac6b 100644
> --- a/kernel/user_namespace.c
> +++ b/kernel/user_namespace.c
> @@ -1142,22 +1142,18 @@ ssize_t proc_setgroups_write(struct file *file, const 
> char __user *buf,
>   struct user_namespace *ns = seq->private;
>   char kbuf[8], *pos;
>   bool setgroups_allowed;
> - ssize_t ret;
>  
>   /* Only allow a very narrow range of strings to be written */
> - ret = -EINVAL;
>   if ((*ppos != 0) || (count >= sizeof(kbuf)))
> - goto out;
> + return -EINVAL;
>  
>   /* What was written? */
> - ret = -EFAULT;
>   if (copy_from_user(kbuf, buf, count))
> - goto out;
> + return -EFAULT;
>   kbuf[count] = '\0';
>   pos = kbuf;
>  
>   /* What is being requested? */
> - ret = -EINVAL;
>   if (strncmp(pos, "allow", 5) == 0) {
>   pos += 5;
>   setgroups_allowed = true;
> @@ -1167,14 +1163,13 @@ ssize_t proc_setgroups_write(struct file *file, const 
> char __user *buf,
>   setgroups_allowed = false;
>   }
>   else
> - goto out;
> + return -EINVAL;
>  
>   /* Verify there is not trailing junk on the line */
>   pos = skip_spaces(pos);
>   if (*pos != '\0')
> - goto out;
> + return -EINVAL;
>  
> - ret = -EPERM;
>   mutex_lock(_state_mutex);
>   if (setgroups_allowed) {
>   /* Enabling setgroups after setgroups has been disabled
> @@ -1194,12 +1189,11 @@ ssize_t proc_setgroups_write(struct file *file, const 
> char __user *buf,
>  
>   /* Report a successful write */
>   *ppos = count;
> - ret = count;
> -out:
> - return ret;
> + return count;
> +
>  out_unlock:
>   mutex_unlock(_state_mutex);
> - goto out;
> + return -EPERM;
>  }
>  
>  bool userns_may_setgroups(const struct user_namespace *ns)

Re: [PATCH -next] user_namespace: Replace gotos with return statements

[Eric W. Biederman]

Marcos Paulo de Souza  writes:

> Found while inspecting the code that handles the setgroups procfs
> file.

What perchance might be the advantage of introducing multiple exits
into proc_setgroups_write?

I strongly suspect that if you look at the generated code it will
be worse after your patch.


Eric

> Signed-off-by: Marcos Paulo de Souza 
> ---
> Tested locally setting up a new userns, and setting setgroups as deny and 
> allow,
> worked as before.
>
>  kernel/user_namespace.c | 20 +++-
>  1 file changed, 7 insertions(+), 13 deletions(-)
>
> diff --git a/kernel/user_namespace.c b/kernel/user_namespace.c
> index 246d4d4ce5c7..64a01254ac6b 100644
> --- a/kernel/user_namespace.c
> +++ b/kernel/user_namespace.c
> @@ -1142,22 +1142,18 @@ ssize_t proc_setgroups_write(struct file *file, const 
> char __user *buf,
>   struct user_namespace *ns = seq->private;
>   char kbuf[8], *pos;
>   bool setgroups_allowed;
> - ssize_t ret;
>  
>   /* Only allow a very narrow range of strings to be written */
> - ret = -EINVAL;
>   if ((*ppos != 0) || (count >= sizeof(kbuf)))
> - goto out;
> + return -EINVAL;
>  
>   /* What was written? */
> - ret = -EFAULT;
>   if (copy_from_user(kbuf, buf, count))
> - goto out;
> + return -EFAULT;
>   kbuf[count] = '\0';
>   pos = kbuf;
>  
>   /* What is being requested? */
> - ret = -EINVAL;
>   if (strncmp(pos, "allow", 5) == 0) {
>   pos += 5;
>   setgroups_allowed = true;
> @@ -1167,14 +1163,13 @@ ssize_t proc_setgroups_write(struct file *file, const 
> char __user *buf,
>   setgroups_allowed = false;
>   }
>   else
> - goto out;
> + return -EINVAL;
>  
>   /* Verify there is not trailing junk on the line */
>   pos = skip_spaces(pos);
>   if (*pos != '\0')
> - goto out;
> + return -EINVAL;
>  
> - ret = -EPERM;
>   mutex_lock(_state_mutex);
>   if (setgroups_allowed) {
>   /* Enabling setgroups after setgroups has been disabled
> @@ -1194,12 +1189,11 @@ ssize_t proc_setgroups_write(struct file *file, const 
> char __user *buf,
>  
>   /* Report a successful write */
>   *ppos = count;
> - ret = count;
> -out:
> - return ret;
> + return count;
> +
>  out_unlock:
>   mutex_unlock(_state_mutex);
> - goto out;
> + return -EPERM;
>  }
>  
>  bool userns_may_setgroups(const struct user_namespace *ns)

Re: [PATCH] crypto: testmgr: Allow different compression results

[Herbert Xu]

On Wed, Apr 11, 2018 at 08:28:32PM +0200, Jan Glauber wrote:
>
> @@ -1362,7 +1373,17 @@ static int test_comp(struct crypto_comp *tfm,
>   goto out;
>   }
>  
> - if (dlen != ctemplate[i].outlen) {
> + ilen = dlen;
> + dlen = COMP_BUF_SIZE;
> + ret = crypto_comp_decompress(tfm, output,
> +  ilen, decomp_output, );
> + if (ret) {
> + pr_err("alg: comp: compression failed: decompress: on 
> test %d for %s failed: ret=%d\n",
> +i + 1, algo, -ret);
> + goto out;
> + }
> +
> + if (dlen != ctemplate[i].inlen) {
>   printk(KERN_ERR "alg: comp: Compression test %d "
>  "failed for %s: output len = %d\n", i + 1, algo,
>  dlen);

Your patch is fine as it is.

But I just thought I'd mention that if anyone wants to we should
really change this to use a different tfm, e.g., always use the
generic algorithm to perform the decompression.  This way if there
were multiple implementations we can at least test them against
the generic one.

Otherwise you could end up with a buggy implementation that works
against itself but still generates incorrect output.

Cheers,
-- 
Email: Herbert Xu 
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH] crypto: testmgr: Allow different compression results

[Herbert Xu]

On Wed, Apr 11, 2018 at 08:28:32PM +0200, Jan Glauber wrote:
>
> @@ -1362,7 +1373,17 @@ static int test_comp(struct crypto_comp *tfm,
>   goto out;
>   }
>  
> - if (dlen != ctemplate[i].outlen) {
> + ilen = dlen;
> + dlen = COMP_BUF_SIZE;
> + ret = crypto_comp_decompress(tfm, output,
> +  ilen, decomp_output, );
> + if (ret) {
> + pr_err("alg: comp: compression failed: decompress: on 
> test %d for %s failed: ret=%d\n",
> +i + 1, algo, -ret);
> + goto out;
> + }
> +
> + if (dlen != ctemplate[i].inlen) {
>   printk(KERN_ERR "alg: comp: Compression test %d "
>  "failed for %s: output len = %d\n", i + 1, algo,
>  dlen);

Your patch is fine as it is.

But I just thought I'd mention that if anyone wants to we should
really change this to use a different tfm, e.g., always use the
generic algorithm to perform the decompression.  This way if there
were multiple implementations we can at least test them against
the generic one.

Otherwise you could end up with a buggy implementation that works
against itself but still generates incorrect output.

Cheers,
-- 
Email: Herbert Xu 
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH v2] storvsc: Set up correct queue depth values for IDE devices

[Martin K. Petersen]

Long,

> Can you take a look at the following patch?

>> > + max_sub_channels =
>> > +  (num_cpus - 1) / storvsc_vcpus_per_sub_channel;

What happens if num_cpus = 1?

-- 
Martin K. Petersen  Oracle Linux Engineering

Re: [PATCH v2] storvsc: Set up correct queue depth values for IDE devices

[Martin K. Petersen]

Long,

> Can you take a look at the following patch?

>> > + max_sub_channels =
>> > +  (num_cpus - 1) / storvsc_vcpus_per_sub_channel;

What happens if num_cpus = 1?

-- 
Martin K. Petersen  Oracle Linux Engineering

Re: [PATCH 2/2] crypto: ccree: enable support for hardware keys

[Herbert Xu]

On Mon, Apr 09, 2018 at 11:42:31AM +0300, Gilad Ben-Yossef wrote:
>
> Please look again. The stub version of cc_is_hw_key() doing that is being
> replaced in this patch.

The point is that the existing mechanism was unused before and this
is new code.  So you can't really point to the stubbed-out function
as a precedent.
 
> The s390 key and the cryptocell keys are not the same:
> 
> Their is, I believe, is an AES key encrypted by some internal key/algorithm.
> 
> The cryptocell "key" is a token, which is internally comprised of one
> or two indexes, referencing slots in the internal memory in the
> hardware, and a key size, that describe the size of the key.
> 
> I thought it would be confusing to use "paes" to describe both, since
> they are not interchangeable.
> You would not be able to feed an paes key that works with the s390
> version to cryptocell and vice verse and get it work.

Thanks for the info.

> Having said, if you prefer to have "paes" simply designate
> "implementation specific token for an AES key" I'm perfectly fine with
> that.

Well by definition none of these hardware keys will be compatible
with each other so I don't really see the point of using individual
algorithm names such as paes or haes.  This would make sense only if
they were somehow compatible with each other.

So instead of using algorithm names, you really want refer to the
specific driver name, which means that they can all use the same
algorithm name.

> > As to your patch specifically, there is one issue where you're
> > directly dereferencing the key as a struct.  This is a no-no because
> > the key may have come from user-space.  You must treat it as a
> > binary blob.  The s390 code seems to do this correctly.
> 
> As noted above, the haes "key" is really a token encoding 3 different
> pieces of information:

My point is that you should not just cast it but instead do a
copy to properly aligned kernel memory.

Cheers,
-- 
Email: Herbert Xu 
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH 2/2] crypto: ccree: enable support for hardware keys

[Herbert Xu]

On Mon, Apr 09, 2018 at 11:42:31AM +0300, Gilad Ben-Yossef wrote:
>
> Please look again. The stub version of cc_is_hw_key() doing that is being
> replaced in this patch.

The point is that the existing mechanism was unused before and this
is new code.  So you can't really point to the stubbed-out function
as a precedent.
 
> The s390 key and the cryptocell keys are not the same:
> 
> Their is, I believe, is an AES key encrypted by some internal key/algorithm.
> 
> The cryptocell "key" is a token, which is internally comprised of one
> or two indexes, referencing slots in the internal memory in the
> hardware, and a key size, that describe the size of the key.
> 
> I thought it would be confusing to use "paes" to describe both, since
> they are not interchangeable.
> You would not be able to feed an paes key that works with the s390
> version to cryptocell and vice verse and get it work.

Thanks for the info.

> Having said, if you prefer to have "paes" simply designate
> "implementation specific token for an AES key" I'm perfectly fine with
> that.

Well by definition none of these hardware keys will be compatible
with each other so I don't really see the point of using individual
algorithm names such as paes or haes.  This would make sense only if
they were somehow compatible with each other.

So instead of using algorithm names, you really want refer to the
specific driver name, which means that they can all use the same
algorithm name.

> > As to your patch specifically, there is one issue where you're
> > directly dereferencing the key as a struct.  This is a no-no because
> > the key may have come from user-space.  You must treat it as a
> > binary blob.  The s390 code seems to do this correctly.
> 
> As noted above, the haes "key" is really a token encoding 3 different
> pieces of information:

My point is that you should not just cast it but instead do a
copy to properly aligned kernel memory.

Cheers,
-- 
Email: Herbert Xu 
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: KASAN: slab-out-of-bounds Write in tcp_v6_syn_recv_sock

[Eric Biggers]

On Tue, Jan 02, 2018 at 03:58:01PM -0800, syzbot wrote:
> Hello,
> 
> syzkaller hit the following crash on
> 61233580f1f33c50e159c50e24d80ffd2ba2e06b
> git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/master
> compiler: gcc (GCC) 7.1.1 20170620
> .config is attached
> Raw console output is attached.
> C reproducer is attached
> syzkaller reproducer is attached. See https://goo.gl/kgGztJ
> for information about syzkaller reproducers
> 
> 
> IMPORTANT: if you fix the bug, please add the following tag to the commit:
> Reported-by: syzbot+6dc95bddc6976b800...@syzkaller.appspotmail.com
> It will help syzbot understand when the bug is fixed. See footer for
> details.
> If you forward the report, please keep this part and the footer.
> 
> TCP: request_sock_TCPv6: Possible SYN flooding on port 20002. Sending
> cookies.  Check SNMP counters.
> ==
> BUG: KASAN: slab-out-of-bounds in memcpy include/linux/string.h:344 [inline]
> BUG: KASAN: slab-out-of-bounds in tcp_v6_syn_recv_sock+0x628/0x23a0
> net/ipv6/tcp_ipv6.c:1144
> Write of size 160 at addr 8801cbdd7460 by task syzkaller545407/3196
> 
> CPU: 1 PID: 3196 Comm: syzkaller545407 Not tainted 4.15.0-rc5+ #241
> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS
> Google 01/01/2011
> Call Trace:
>  
>  __dump_stack lib/dump_stack.c:17 [inline]
>  dump_stack+0x194/0x257 lib/dump_stack.c:53
>  print_address_description+0x73/0x250 mm/kasan/report.c:252
>  kasan_report_error mm/kasan/report.c:351 [inline]
>  kasan_report+0x25b/0x340 mm/kasan/report.c:409
>  check_memory_region_inline mm/kasan/kasan.c:260 [inline]
>  check_memory_region+0x137/0x190 mm/kasan/kasan.c:267
>  memcpy+0x37/0x50 mm/kasan/kasan.c:303
>  memcpy include/linux/string.h:344 [inline]
>  tcp_v6_syn_recv_sock+0x628/0x23a0 net/ipv6/tcp_ipv6.c:1144
>  tcp_get_cookie_sock+0x102/0x540 net/ipv4/syncookies.c:213
>  cookie_v6_check+0x177d/0x2160 net/ipv6/syncookies.c:255
>  tcp_v6_cookie_check net/ipv6/tcp_ipv6.c:1008 [inline]
>  tcp_v6_do_rcv+0xe4d/0x11c0 net/ipv6/tcp_ipv6.c:1316
>  tcp_v6_rcv+0x22ee/0x2b40 net/ipv6/tcp_ipv6.c:1510
>  ip6_input_finish+0x36f/0x1700 net/ipv6/ip6_input.c:284
>  NF_HOOK include/linux/netfilter.h:250 [inline]
>  ip6_input+0xe9/0x560 net/ipv6/ip6_input.c:327
>  dst_input include/net/dst.h:466 [inline]
>  ip6_rcv_finish+0x1a9/0x7a0 net/ipv6/ip6_input.c:71
>  NF_HOOK include/linux/netfilter.h:250 [inline]
>  ipv6_rcv+0xf1f/0x1f80 net/ipv6/ip6_input.c:208
>  __netif_receive_skb_core+0x1a3e/0x3450 net/core/dev.c:4461
>  __netif_receive_skb+0x2c/0x1b0 net/core/dev.c:4526
>  process_backlog+0x203/0x740 net/core/dev.c:5205
>  napi_poll net/core/dev.c:5603 [inline]
>  net_rx_action+0x792/0x1910 net/core/dev.c:5669
>  __do_softirq+0x2d7/0xb85 kernel/softirq.c:285
>  do_softirq_own_stack+0x2a/0x40 arch/x86/entry/entry_64.S:1115
>  
>  do_softirq.part.21+0x14d/0x190 kernel/softirq.c:329
>  do_softirq kernel/softirq.c:177 [inline]
>  __local_bh_enable_ip+0x1ee/0x230 kernel/softirq.c:182
>  local_bh_enable include/linux/bottom_half.h:32 [inline]
>  rcu_read_unlock_bh include/linux/rcupdate.h:727 [inline]
>  ip6_finish_output2+0xba6/0x2390 net/ipv6/ip6_output.c:121
>  ip6_finish_output+0x2f9/0x920 net/ipv6/ip6_output.c:146
>  NF_HOOK_COND include/linux/netfilter.h:239 [inline]
>  ip6_output+0x1eb/0x840 net/ipv6/ip6_output.c:163
>  dst_output include/net/dst.h:460 [inline]
>  NF_HOOK include/linux/netfilter.h:250 [inline]
>  ip6_xmit+0xd75/0x2080 net/ipv6/ip6_output.c:269
>  inet6_csk_xmit+0x2fc/0x580 net/ipv6/inet6_connection_sock.c:139
>  tcp_transmit_skb+0x1b12/0x38b0 net/ipv4/tcp_output.c:1176
>  tcp_write_xmit+0x680/0x5190 net/ipv4/tcp_output.c:2367
>  __tcp_push_pending_frames+0xa0/0x250 net/ipv4/tcp_output.c:2543
>  tcp_send_fin+0x1b0/0xd20 net/ipv4/tcp_output.c:3087
>  tcp_close+0xbe0/0xfc0 net/ipv4/tcp.c:2234
>  inet_release+0xed/0x1c0 net/ipv4/af_inet.c:426
>  inet6_release+0x50/0x70 net/ipv6/af_inet6.c:432
>  sock_release+0x8d/0x1e0 net/socket.c:600
>  sock_close+0x16/0x20 net/socket.c:1129
>  __fput+0x327/0x7e0 fs/file_table.c:210
>  fput+0x15/0x20 fs/file_table.c:244
>  task_work_run+0x199/0x270 kernel/task_work.c:113
>  exit_task_work include/linux/task_work.h:22 [inline]
>  do_exit+0x9bb/0x1ad0 kernel/exit.c:865
>  do_group_exit+0x149/0x400 kernel/exit.c:968
>  get_signal+0x73f/0x16c0 kernel/signal.c:2335
>  do_signal+0x94/0x1ee0 arch/x86/kernel/signal.c:809
>  exit_to_usermode_loop+0x214/0x310 arch/x86/entry/common.c:158
>  prepare_exit_to_usermode arch/x86/entry/common.c:195 [inline]
>  syscall_return_slowpath+0x490/0x550 arch/x86/entry/common.c:264
>  entry_SYSCALL_64_fastpath+0x94/0x96
> RIP: 0033:0x4456e9
> RSP: 002b:7fb4de631da8 EFLAGS: 0246 ORIG_RAX: 00ca
> RAX: fe00 RBX: 006dac3c RCX: 004456e9
> RDX:  RSI:  RDI: 006dac3c
> RBP:  R08:  R09: 

Re: KASAN: slab-out-of-bounds Write in tcp_v6_syn_recv_sock

[Eric Biggers]

On Tue, Jan 02, 2018 at 03:58:01PM -0800, syzbot wrote:
> Hello,
> 
> syzkaller hit the following crash on
> 61233580f1f33c50e159c50e24d80ffd2ba2e06b
> git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/master
> compiler: gcc (GCC) 7.1.1 20170620
> .config is attached
> Raw console output is attached.
> C reproducer is attached
> syzkaller reproducer is attached. See https://goo.gl/kgGztJ
> for information about syzkaller reproducers
> 
> 
> IMPORTANT: if you fix the bug, please add the following tag to the commit:
> Reported-by: syzbot+6dc95bddc6976b800...@syzkaller.appspotmail.com
> It will help syzbot understand when the bug is fixed. See footer for
> details.
> If you forward the report, please keep this part and the footer.
> 
> TCP: request_sock_TCPv6: Possible SYN flooding on port 20002. Sending
> cookies.  Check SNMP counters.
> ==
> BUG: KASAN: slab-out-of-bounds in memcpy include/linux/string.h:344 [inline]
> BUG: KASAN: slab-out-of-bounds in tcp_v6_syn_recv_sock+0x628/0x23a0
> net/ipv6/tcp_ipv6.c:1144
> Write of size 160 at addr 8801cbdd7460 by task syzkaller545407/3196
> 
> CPU: 1 PID: 3196 Comm: syzkaller545407 Not tainted 4.15.0-rc5+ #241
> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS
> Google 01/01/2011
> Call Trace:
>  
>  __dump_stack lib/dump_stack.c:17 [inline]
>  dump_stack+0x194/0x257 lib/dump_stack.c:53
>  print_address_description+0x73/0x250 mm/kasan/report.c:252
>  kasan_report_error mm/kasan/report.c:351 [inline]
>  kasan_report+0x25b/0x340 mm/kasan/report.c:409
>  check_memory_region_inline mm/kasan/kasan.c:260 [inline]
>  check_memory_region+0x137/0x190 mm/kasan/kasan.c:267
>  memcpy+0x37/0x50 mm/kasan/kasan.c:303
>  memcpy include/linux/string.h:344 [inline]
>  tcp_v6_syn_recv_sock+0x628/0x23a0 net/ipv6/tcp_ipv6.c:1144
>  tcp_get_cookie_sock+0x102/0x540 net/ipv4/syncookies.c:213
>  cookie_v6_check+0x177d/0x2160 net/ipv6/syncookies.c:255
>  tcp_v6_cookie_check net/ipv6/tcp_ipv6.c:1008 [inline]
>  tcp_v6_do_rcv+0xe4d/0x11c0 net/ipv6/tcp_ipv6.c:1316
>  tcp_v6_rcv+0x22ee/0x2b40 net/ipv6/tcp_ipv6.c:1510
>  ip6_input_finish+0x36f/0x1700 net/ipv6/ip6_input.c:284
>  NF_HOOK include/linux/netfilter.h:250 [inline]
>  ip6_input+0xe9/0x560 net/ipv6/ip6_input.c:327
>  dst_input include/net/dst.h:466 [inline]
>  ip6_rcv_finish+0x1a9/0x7a0 net/ipv6/ip6_input.c:71
>  NF_HOOK include/linux/netfilter.h:250 [inline]
>  ipv6_rcv+0xf1f/0x1f80 net/ipv6/ip6_input.c:208
>  __netif_receive_skb_core+0x1a3e/0x3450 net/core/dev.c:4461
>  __netif_receive_skb+0x2c/0x1b0 net/core/dev.c:4526
>  process_backlog+0x203/0x740 net/core/dev.c:5205
>  napi_poll net/core/dev.c:5603 [inline]
>  net_rx_action+0x792/0x1910 net/core/dev.c:5669
>  __do_softirq+0x2d7/0xb85 kernel/softirq.c:285
>  do_softirq_own_stack+0x2a/0x40 arch/x86/entry/entry_64.S:1115
>  
>  do_softirq.part.21+0x14d/0x190 kernel/softirq.c:329
>  do_softirq kernel/softirq.c:177 [inline]
>  __local_bh_enable_ip+0x1ee/0x230 kernel/softirq.c:182
>  local_bh_enable include/linux/bottom_half.h:32 [inline]
>  rcu_read_unlock_bh include/linux/rcupdate.h:727 [inline]
>  ip6_finish_output2+0xba6/0x2390 net/ipv6/ip6_output.c:121
>  ip6_finish_output+0x2f9/0x920 net/ipv6/ip6_output.c:146
>  NF_HOOK_COND include/linux/netfilter.h:239 [inline]
>  ip6_output+0x1eb/0x840 net/ipv6/ip6_output.c:163
>  dst_output include/net/dst.h:460 [inline]
>  NF_HOOK include/linux/netfilter.h:250 [inline]
>  ip6_xmit+0xd75/0x2080 net/ipv6/ip6_output.c:269
>  inet6_csk_xmit+0x2fc/0x580 net/ipv6/inet6_connection_sock.c:139
>  tcp_transmit_skb+0x1b12/0x38b0 net/ipv4/tcp_output.c:1176
>  tcp_write_xmit+0x680/0x5190 net/ipv4/tcp_output.c:2367
>  __tcp_push_pending_frames+0xa0/0x250 net/ipv4/tcp_output.c:2543
>  tcp_send_fin+0x1b0/0xd20 net/ipv4/tcp_output.c:3087
>  tcp_close+0xbe0/0xfc0 net/ipv4/tcp.c:2234
>  inet_release+0xed/0x1c0 net/ipv4/af_inet.c:426
>  inet6_release+0x50/0x70 net/ipv6/af_inet6.c:432
>  sock_release+0x8d/0x1e0 net/socket.c:600
>  sock_close+0x16/0x20 net/socket.c:1129
>  __fput+0x327/0x7e0 fs/file_table.c:210
>  fput+0x15/0x20 fs/file_table.c:244
>  task_work_run+0x199/0x270 kernel/task_work.c:113
>  exit_task_work include/linux/task_work.h:22 [inline]
>  do_exit+0x9bb/0x1ad0 kernel/exit.c:865
>  do_group_exit+0x149/0x400 kernel/exit.c:968
>  get_signal+0x73f/0x16c0 kernel/signal.c:2335
>  do_signal+0x94/0x1ee0 arch/x86/kernel/signal.c:809
>  exit_to_usermode_loop+0x214/0x310 arch/x86/entry/common.c:158
>  prepare_exit_to_usermode arch/x86/entry/common.c:195 [inline]
>  syscall_return_slowpath+0x490/0x550 arch/x86/entry/common.c:264
>  entry_SYSCALL_64_fastpath+0x94/0x96
> RIP: 0033:0x4456e9
> RSP: 002b:7fb4de631da8 EFLAGS: 0246 ORIG_RAX: 00ca
> RAX: fe00 RBX: 006dac3c RCX: 004456e9
> RDX:  RSI:  RDI: 006dac3c
> RBP:  R08:  R09: 

Re: [PATCH V8 1/5] crypto: Multi-buffer encryption infrastructure support

[Herbert Xu]

On Thu, Apr 19, 2018 at 12:54:16AM +, Dey, Megha wrote:
>
> Yeah I think I misunderstood. I think what you mean is to remove mcryptd.c 
> completely and avoid the extra layer of indirection to call the underlying 
> algorithm, instead call it directly, correct?
> 
> So currently we have 3 algorithms registered for every multibuffer algorithm:
> name : __sha1-mb
> driver   : mcryptd(__intel_sha1-mb)
> 
> name : sha1
> driver   : sha1_mb
> 
> name : __sha1-mb
> driver   : __intel_sha1-mb
> 
> If we remove mcryptd, then we will have just the 2?

It should be down to just one, i.e., the current inner algorithm.
It's doing all the scheduling work already so I don't really see
why it needs the wrappers around it.

Cheers,
-- 
Email: Herbert Xu 
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH V8 1/5] crypto: Multi-buffer encryption infrastructure support

[Herbert Xu]

On Thu, Apr 19, 2018 at 12:54:16AM +, Dey, Megha wrote:
>
> Yeah I think I misunderstood. I think what you mean is to remove mcryptd.c 
> completely and avoid the extra layer of indirection to call the underlying 
> algorithm, instead call it directly, correct?
> 
> So currently we have 3 algorithms registered for every multibuffer algorithm:
> name : __sha1-mb
> driver   : mcryptd(__intel_sha1-mb)
> 
> name : sha1
> driver   : sha1_mb
> 
> name : __sha1-mb
> driver   : __intel_sha1-mb
> 
> If we remove mcryptd, then we will have just the 2?

It should be down to just one, i.e., the current inner algorithm.
It's doing all the scheduling work already so I don't really see
why it needs the wrappers around it.

Cheers,
-- 
Email: Herbert Xu 
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

RE: [PATCH V2 2/2] ARM: dts: imx6sx-sabreauto: add external 24MHz clock source

[Anson Huang]

Anson Huang
Best Regards!


> -Original Message-
> From: Stephen Boyd [mailto:sb...@kernel.org]
> Sent: Thursday, April 19, 2018 11:18 AM
> To: Anson Huang ; Shawn Guo
> 
> Cc: mark.rutl...@arm.com; devicet...@vger.kernel.org;
> mturque...@baylibre.com; linux-...@vger.kernel.org; li...@armlinux.org.uk;
> linux-kernel@vger.kernel.org; robh...@kernel.org; dl-linux-imx
> ; ker...@pengutronix.de; Fabio Estevam
> ; S.j. Wang ;
> linux-arm-ker...@lists.infradead.org
> Subject: Re: [PATCH V2 2/2] ARM: dts: imx6sx-sabreauto: add external 24MHz
> clock source
> 
> Quoting Shawn Guo (2018-04-17 07:22:05)
> > On Mon, Mar 19, 2018 at 10:30:45AM +0800, Anson Huang wrote:
> > > On i.MX6SX SabreAuto board, there is external 24MHz clock source for
> > > analog clock2, add this clock source to clock tree.
> > >
> > > Signed-off-by: Anson Huang 
> > > ---
> > > changes since V1:
> > >   remove unnecessary clocks container.
> >
> > I understand this is suggested by Fabio, but I'm afraid that it's not
> > going to work with imx_obtain_fixed_clock() call, which is coded to
> > look for clocks under /clocks node.
> >
> 
> Should patch #1 be dropped from clk tree?

If so, I think we should use V1 patch to keep clocks container?
 
Anson.

RE: [PATCH V2 2/2] ARM: dts: imx6sx-sabreauto: add external 24MHz clock source

[Anson Huang]

Anson Huang
Best Regards!


> -Original Message-
> From: Stephen Boyd [mailto:sb...@kernel.org]
> Sent: Thursday, April 19, 2018 11:18 AM
> To: Anson Huang ; Shawn Guo
> 
> Cc: mark.rutl...@arm.com; devicet...@vger.kernel.org;
> mturque...@baylibre.com; linux-...@vger.kernel.org; li...@armlinux.org.uk;
> linux-kernel@vger.kernel.org; robh...@kernel.org; dl-linux-imx
> ; ker...@pengutronix.de; Fabio Estevam
> ; S.j. Wang ;
> linux-arm-ker...@lists.infradead.org
> Subject: Re: [PATCH V2 2/2] ARM: dts: imx6sx-sabreauto: add external 24MHz
> clock source
> 
> Quoting Shawn Guo (2018-04-17 07:22:05)
> > On Mon, Mar 19, 2018 at 10:30:45AM +0800, Anson Huang wrote:
> > > On i.MX6SX SabreAuto board, there is external 24MHz clock source for
> > > analog clock2, add this clock source to clock tree.
> > >
> > > Signed-off-by: Anson Huang 
> > > ---
> > > changes since V1:
> > >   remove unnecessary clocks container.
> >
> > I understand this is suggested by Fabio, but I'm afraid that it's not
> > going to work with imx_obtain_fixed_clock() call, which is coded to
> > look for clocks under /clocks node.
> >
> 
> Should patch #1 be dropped from clk tree?

If so, I think we should use V1 patch to keep clocks container?
 
Anson.

Re: [PATCH 2/6] rhashtable: remove incorrect comment on r{hl, hash}table_walk_enter()

[Herbert Xu]

On Thu, Apr 19, 2018 at 08:56:28AM +1000, NeilBrown wrote:
>
> I don't want to do that - I just want the documentation to be correct
> (or at least, not be blatantly incorrect).  The function does not sleep,
> and is safe to call with spin locks held.
> Do we need to spell out when it can be called?  If so, maybe:
> 
>This function may be called from any process context, including
>non-preemptable context, but cannot be called from interrupts.

Just to make it perfectly clear, how about "cannot be called from
softirq or hardirq context"? Previously the not able to sleep part
completely ruled out any ambiguity but the new wording could confuse
people into thinking that this can be called from softirq context
where it would be unsafe if mixed with process context usage.

Thanks,
-- 
Email: Herbert Xu 
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH 2/6] rhashtable: remove incorrect comment on r{hl, hash}table_walk_enter()

[Herbert Xu]

On Thu, Apr 19, 2018 at 08:56:28AM +1000, NeilBrown wrote:
>
> I don't want to do that - I just want the documentation to be correct
> (or at least, not be blatantly incorrect).  The function does not sleep,
> and is safe to call with spin locks held.
> Do we need to spell out when it can be called?  If so, maybe:
> 
>This function may be called from any process context, including
>non-preemptable context, but cannot be called from interrupts.

Just to make it perfectly clear, how about "cannot be called from
softirq or hardirq context"? Previously the not able to sleep part
completely ruled out any ambiguity but the new wording could confuse
people into thinking that this can be called from softirq context
where it would be unsafe if mixed with process context usage.

Thanks,
-- 
Email: Herbert Xu 
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH v1 2/4] dt-bindings: clock: mediatek: add g3dsys bindings

[Stephen Boyd]

Quoting sean.w...@mediatek.com (2018-04-18 03:24:54)
> From: Sean Wang 
> 
> Add bindings to g3dsys providing necessary clock and reset control to
> Mali-450.
> 
> Signed-off-by: Sean Wang 
> ---
>  .../bindings/arm/mediatek/mediatek,g3dsys.txt  | 30 
> ++

Why isn't this under bindings/clock/ ?

Re: [PATCH v1 2/4] dt-bindings: clock: mediatek: add g3dsys bindings

[Stephen Boyd]

Quoting sean.w...@mediatek.com (2018-04-18 03:24:54)
> From: Sean Wang 
> 
> Add bindings to g3dsys providing necessary clock and reset control to
> Mali-450.
> 
> Signed-off-by: Sean Wang 
> ---
>  .../bindings/arm/mediatek/mediatek,g3dsys.txt  | 30 
> ++

Why isn't this under bindings/clock/ ?