* [PATCH 0/9] mmc: fsl_esdhc: support eMMC HS200/HS400 modes
From: Jaehoon Chung @ 2020-07-17 0:31 UTC (permalink / raw)
To: u-boot
In-Reply-To: <20200716022954.36774-1-yangbo.lu@nxp.com>
Hi Yangbo,
On 7/16/20 11:29 AM, Yangbo Lu wrote:
> This patch-set is to support eMMC HS200 and HS400 speed modes for
> eSDHC, and enable them on LX2160ARDB board.
Is there any result about performance?
Best Regards,
Jaehoon Chung
>
> CI build link
> https://travis-ci.org/github/yangbolu1991/u-boot-test/builds/708215558
>
> Yangbo Lu (9):
> mmc: add a reinit() API
> mmc: fsl_esdhc: add a reinit() callback
> mmc: fsl_esdhc: support tuning for eMMC HS200
> mmc: fsl_esdhc: clean TBCTL[TB_EN] manually during init
> mmc: add a hs400_tuning flag
> mmc: add a mmc_hs400_prepare_ddr() interface
> mmc: fsl_esdhc: support eMMC HS400 mode
> arm: dts: lx2160ardb: support eMMC HS400 mode
> configs: lx2160ardb: enable eMMC HS400 mode support
>
> arch/arm/dts/fsl-lx2160a-rdb.dts | 2 +
> configs/lx2160ardb_tfa_SECURE_BOOT_defconfig | 1 +
> configs/lx2160ardb_tfa_defconfig | 1 +
> configs/lx2160ardb_tfa_stmm_defconfig | 1 +
> drivers/mmc/fsl_esdhc.c | 148 ++++++++++++++++++++++++++-
> drivers/mmc/mmc-uclass.c | 30 ++++++
> drivers/mmc/mmc.c | 12 ++-
> include/fsl_esdhc.h | 29 +++++-
> include/mmc.h | 26 ++++-
> 9 files changed, 240 insertions(+), 10 deletions(-)
>
^ permalink raw reply
* Re: [Intel-gfx] [PATCH 1/1] drm/i915/perf: Map OA buffer to user space
From: Umesh Nerlige Ramappa @ 2020-07-17 0:28 UTC (permalink / raw)
To: Lionel Landwerlin; +Cc: intel-gfx
In-Reply-To: <20200716192847.GB31154@orsosgc001.amr.corp.intel.com>
On Thu, Jul 16, 2020 at 12:28:47PM -0700, Umesh Nerlige Ramappa wrote:
>On Thu, Jul 16, 2020 at 09:44:46PM +0300, Lionel Landwerlin wrote:
>>On 16/07/2020 21:06, Umesh Nerlige Ramappa wrote:
>>>On Thu, Jul 16, 2020 at 06:32:10PM +0300, Lionel Landwerlin wrote:
>>>>On 14/07/2020 10:22, Umesh Nerlige Ramappa wrote:
>>>>>From: Piotr Maciejewski <piotr.maciejewski@intel.com>
>>>>>
>>>>>i915 used to support time based sampling mode which is good for overall
>>>>>system monitoring, but is not enough for query mode used to measure a
>>>>>single draw call or dispatch. Gen9-Gen11 are using current i915 perf
>>>>>implementation for query, but Gen12+ requires a new approach based on
>>>>>triggered reports within oa buffer. In order to enable above feature
>>>>>two changes are required:
>>>>>
>>>>>1. Whitelist update:
>>>>>- enable triggered reports within oa buffer
>>>>>- reading oa buffer head/tail/status information
>>>>>- reading gpu ticks counter.
>>>>
>>>>I would break the patch into feature sets :
>>>>
>>>> - 1 patch to whitelist the trigger registers
>>>>
>>>> - 1 patch to whitelist OA counters & tail/head/... registers
>>>>
>>>> - 1 patch for mmap feature
>>>>
>>>>
>>>>Here are some IGT tests for the trigger feature :
>>>>https://patchwork.freedesktop.org/series/75311/
>>>>
>>>>
>>>>We should verify that when not i915-perf is not active the
>>>>whitelisted OA counters are not incrementing.
>>>>
>>>>Otherwise we're starting to leak information that was not
>>>>available before.
>>>>
>>>>
>>>>>
>>>>>2. Map oa buffer at umd driver level to solve below constraints related
>>>>> to time based sampling interface:
>>>>>- longer time to access reports collected by oa buffer
>>>>>- slow oa reports browsing since oa buffer size is large
>>>>>- missing oa report index, so query cannot browse report directly
>>>>>- with direct access to oa buffer, query can extract other useful
>>>>> reports like context switch information needed to calculate correct
>>>>> performance counters values.
>>>>>
>>>>>Signed-off-by: Piotr Maciejewski <piotr.maciejewski@intel.com>
>>>>>---
>>>>> drivers/gpu/drm/i915/gt/intel_workarounds.c | 54 ++++++++
>>>>> drivers/gpu/drm/i915/i915_perf.c | 130 +++++++++++++++++++-
>>>>> drivers/gpu/drm/i915/i915_perf_types.h | 13 ++
>>>>> drivers/gpu/drm/i915/i915_reg.h | 14 +++
>>>>> include/uapi/drm/i915_drm.h | 19 +++
>>>>> 5 files changed, 227 insertions(+), 3 deletions(-)
>>>>>
>>>>>diff --git a/drivers/gpu/drm/i915/gt/intel_workarounds.c
>>>>>b/drivers/gpu/drm/i915/gt/intel_workarounds.c
>>>>>index 5726cd0a37e0..cf89928fc3a5 100644
>>>>>--- a/drivers/gpu/drm/i915/gt/intel_workarounds.c
>>>>>+++ b/drivers/gpu/drm/i915/gt/intel_workarounds.c
>>>>>@@ -1365,6 +1365,48 @@ whitelist_reg(struct i915_wa_list *wal,
>>>>>i915_reg_t reg)
>>>>> whitelist_reg_ext(wal, reg, RING_FORCE_TO_NONPRIV_ACCESS_RW);
>>>>> }
>>>>>+static void gen9_whitelist_build_performance_counters(struct
>>>>>i915_wa_list *w)
>>>>>+{
>>>>>+ /* OA buffer trigger report 2/6 used by performance query */
>>>>>+ whitelist_reg(w, OAREPORTTRIG2);
>>>>>+ whitelist_reg(w, OAREPORTTRIG6);
>>>>>+
>>>>>+ /* Performance counters A18-20 used by tbs marker query */
>>>>>+ whitelist_reg_ext(w, OA_PERF_COUNTER_A18,
>>>>>+ RING_FORCE_TO_NONPRIV_ACCESS_RW |
>>>>>+ RING_FORCE_TO_NONPRIV_RANGE_16);
>>>>
>>>>I would actually whitelist the entire set of OA_PERF_COUNTER (0-36).
>>>
>>>Not sure about that. I thought the only reason we chose to
>>>whitelist A18-A20 was because these counters did not count
>>>anything. Whitelisting all A counters would just mean that an
>>>unprivileged user can keep sampling them all the time from a
>>>command buffer. Right?
>>
>>
>>A7-20 are flex EU counters, they can be programmed to count
>>particular events.
>>
>>We just happen to not program them all.
>>
>>
>>Sure an unprivileged user could sample them, but it can already
>>sample them through MI_RPC.
>>
>>
>>My reason for whitelisting them is that I can sample them without
>>having to look at the OA buffer since on Gen12+ MI_RPC sources
>>values from OAR which doesn't have the same values as OAG for the
>>counters.
>>
>
>I see. gen12 broke an earlier use case due to OAR/OAG split. I will
>follow up on the range of registers you requested for Vulkan.
>
>Thanks,
>Umesh
>
>>
>>-Lionel
>>
>>
>>>
>>>Thanks,
>>>Umesh
>>>
>>>>
>>>>I would like to make use of them in Mesa for the Vulkan driver.
>>>>
>>>>>+
>>>>>+ /* Read access to gpu ticks */
>>>>>+ whitelist_reg_ext(w, GEN8_GPU_TICKS,
>>>>>+ RING_FORCE_TO_NONPRIV_ACCESS_RD);
>>>>>+
>>>>>+ /* Read access to: oa status, head, tail, buffer settings */
>>>>>+ whitelist_reg_ext(w, GEN8_OASTATUS,
>>>>>+ RING_FORCE_TO_NONPRIV_ACCESS_RD |
>>>>>+ RING_FORCE_TO_NONPRIV_RANGE_4);
>>>>>+}
>>>>>+
>>>>>+static void gen12_whitelist_build_performance_counters(struct
>>>>>i915_wa_list *w)
>>>>>+{
>>>>>+ /* OA buffer trigger report 2/6 used by performance query */
>>>>>+ whitelist_reg(w, GEN12_OAG_OAREPORTTRIG2);
>>>>>+ whitelist_reg(w, GEN12_OAG_OAREPORTTRIG6);
>>>>>+
>>>>>+ /* Performance counters A18-20 used by tbs marker query */
>>>>>+ whitelist_reg_ext(w, GEN12_OAG_PERF_COUNTER_A18,
>>>>>+ RING_FORCE_TO_NONPRIV_ACCESS_RW |
>>>>>+ RING_FORCE_TO_NONPRIV_RANGE_16);
>>>>>+
>>>>>+ /* Read access to gpu ticks */
>>>>>+ whitelist_reg_ext(w, GEN12_OAG_GPU_TICKS,
>>>>>+ RING_FORCE_TO_NONPRIV_ACCESS_RD);
>>>>>+
>>>>>+ /* Read access to: oa status, head, tail, buffer settings */
>>>>>+ whitelist_reg_ext(w, GEN12_OAG_OASTATUS,
>>>>>+ RING_FORCE_TO_NONPRIV_ACCESS_RD |
>>>>>+ RING_FORCE_TO_NONPRIV_RANGE_4);
>>>>>+}
>>>>>+
>>>>
>>>>>...
>>>>
>>>>> struct i915_perf {
>>>>>diff --git a/drivers/gpu/drm/i915/i915_reg.h
>>>>>b/drivers/gpu/drm/i915/i915_reg.h
>>>>>index 86a23ced051b..2e3d264339e0 100644
>>>>>--- a/drivers/gpu/drm/i915/i915_reg.h
>>>>>+++ b/drivers/gpu/drm/i915/i915_reg.h
>>>>>@@ -675,6 +675,7 @@ static inline bool
>>>>>i915_mmio_reg_valid(i915_reg_t reg)
>>>>> #define GEN7_OASTATUS2_HEAD_MASK 0xffffffc0
>>>>> #define GEN7_OASTATUS2_MEM_SELECT_GGTT (1 << 0) /* 0:
>>>>>PPGTT, 1: GGTT */
>>>>>+#define GEN8_GPU_TICKS _MMIO(0x2910)
>>>>> #define GEN8_OASTATUS _MMIO(0x2b08)
>>>>> #define GEN8_OASTATUS_OVERRUN_STATUS (1 << 3)
>>>>> #define GEN8_OASTATUS_COUNTER_OVERFLOW (1 << 2)
>>>>>@@ -696,6 +697,7 @@ static inline bool
>>>>>i915_mmio_reg_valid(i915_reg_t reg)
>>>>> #define OABUFFER_SIZE_16M (7 << 3)
>>>>> #define GEN12_OA_TLB_INV_CR _MMIO(0xceec)
>>>>>+#define GEN12_SQCNT1 _MMIO(0x8718)
>>>>> /* Gen12 OAR unit */
>>>>> #define GEN12_OAR_OACONTROL _MMIO(0x2960)
>>>>>@@ -731,6 +733,7 @@ static inline bool
>>>>>i915_mmio_reg_valid(i915_reg_t reg)
>>>>> #define GEN12_OAG_OA_DEBUG_DISABLE_GO_1_0_REPORTS (1 << 2)
>>>>> #define GEN12_OAG_OA_DEBUG_DISABLE_CTX_SWITCH_REPORTS (1 << 1)
>>>>>+#define GEN12_OAG_GPU_TICKS _MMIO(0xda90)
>>>>> #define GEN12_OAG_OASTATUS _MMIO(0xdafc)
>>>>> #define GEN12_OAG_OASTATUS_COUNTER_OVERFLOW (1 << 2)
>>>>> #define GEN12_OAG_OASTATUS_BUFFER_OVERFLOW (1 << 1)
>>>>>@@ -972,6 +975,17 @@ static inline bool
>>>>>i915_mmio_reg_valid(i915_reg_t reg)
>>>>> #define OAREPORTTRIG8_NOA_SELECT_6_SHIFT 24
>>>>> #define OAREPORTTRIG8_NOA_SELECT_7_SHIFT 28
>>>>>+/* Performance counters registers */
>>>>>+#define OA_PERF_COUNTER_A18 _MMIO(0x2890)
>>>>>+#define OA_PERF_COUNTER_A19 _MMIO(0x2898)
>>>>>+#define OA_PERF_COUNTER_A20 _MMIO(0x28A0)
>>>>Maybe turn this into OA_PERF_COUNTER(idx) _MMIO(0x2800 + idx * 8)
That's a good idea, although not all _UPPER counters exist. Some
counters are just 32 bits A33 etc. I would rather leave it like
this and index into the counters if we end up adding more here.
Thanks,
Umesh
>>>>>+
>>>>>+/* Gen12 Performance counters registers */
>>>>>+#define GEN12_OAG_PERF_COUNTER_A16 _MMIO(0xDA00)
>>>>>+#define GEN12_OAG_PERF_COUNTER_A18 _MMIO(0xDA10)
>>>>>+#define GEN12_OAG_PERF_COUNTER_A19 _MMIO(0xDA18)
>>>>>+#define GEN12_OAG_PERF_COUNTER_A20 _MMIO(0xDA20)
>>>>Same here
>>>>>+
>>>>> /* Same layout as OASTARTTRIGX */
>>>>> #define GEN12_OAG_OASTARTTRIG1 _MMIO(0xd900)
>>>>> #define GEN12_OAG_OASTARTTRIG2 _MMIO(0xd904)
>>>>>diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
>>>>>index 14b67cd6b54b..62b88c0123c8 100644
>>>>>--- a/include/uapi/drm/i915_drm.h
>>>>>+++ b/include/uapi/drm/i915_drm.h
>>>>>@@ -2048,6 +2048,25 @@ struct drm_i915_perf_open_param {z
>>>>> */
>>>>> #define I915_PERF_IOCTL_CONFIG _IO('i', 0x2)
>>>>>+/**
>>>>>+ * Returns OA buffer properties.
>>>>>+ *
>>>>>+ * This ioctl is available in perf revision 6.
>>>>>+ */
>>>>>+#define I915_PERF_IOCTL_GET_OA_BUFFER_INFO _IO('i', 0x3)
>>>>>+
>>>>>+/**
>>>>>+ * OA buffer information structure.
>>>>>+ */
>>>>>+struct drm_i915_perf_oa_buffer_info {
>>>>>+ __u32 size;
>>>>>+ __u32 head;
>>>>>+ __u32 tail;
>>>>>+ __u32 gpu_address;
>>>>>+ __u64 cpu_address;
>>>>>+ __u64 reserved[4];
>>>>>+};
>>>>>+
>>>>> /**
>>>>> * Common to all i915 perf records
>>>>> */
>>>>
>>>>Thanks a lot,
>>>>
>>>>
>>>>-Lionel
>>>>
>>
>_______________________________________________
>Intel-gfx mailing list
>Intel-gfx@lists.freedesktop.org
>https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply
* Re: [PATCH net-next] ibmvnic: Increase driver logging
From: Stephen Hemminger @ 2020-07-17 0:26 UTC (permalink / raw)
To: Jakub Kicinski
Cc: netdev, Thomas Falcon, drt, Michal Suchánek, linuxppc-dev,
David Miller
In-Reply-To: <20200716132200.37934905@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
On Thu, 16 Jul 2020 13:22:00 -0700
Jakub Kicinski <kuba@kernel.org> wrote:
> On Thu, 16 Jul 2020 18:07:37 +0200 Michal Suchánek wrote:
> > On Thu, Jul 16, 2020 at 10:59:58AM -0500, Thomas Falcon wrote:
> > > On 7/15/20 8:29 PM, David Miller wrote:
> > > > From: Jakub Kicinski <kuba@kernel.org>
> > > > Date: Wed, 15 Jul 2020 17:06:32 -0700
> > > >
> > > > > On Wed, 15 Jul 2020 18:51:55 -0500 Thomas Falcon wrote:
> > > > > > free_netdev(netdev);
> > > > > > dev_set_drvdata(&dev->dev, NULL);
> > > > > > + netdev_info(netdev, "VNIC client device has been successfully removed.\n");
> > > > > A step too far, perhaps.
> > > > >
> > > > > In general this patch looks a little questionable IMHO, this amount of
> > > > > logging output is not commonly seen in drivers. All the the info
> > > > > messages are just static text, not even carrying any extra information.
> > > > > In an era of ftrace, and bpftrace, do we really need this?
> > > > Agreed, this is too much. This is debugging, and thus suitable for tracing
> > > > facilities, at best.
> > >
> > > Thanks for your feedback. I see now that I was overly aggressive with this
> > > patch to be sure, but it would help with narrowing down problems at a first
> > > glance, should they arise. The driver in its current state logs very little
> > > of what is it doing without the use of additional debugging or tracing
> > > facilities. Would it be worth it to pursue a less aggressive version or
> > > would that be dead on arrival? What are acceptable driver operations to log
> > > at this level?
>
> Sadly it's much more of an art than hard science. Most networking
> drivers will print identifying information when they probe the device
> and then only about major config changes or when link comes up or goes
> down. And obviously when anything unexpected, like an error happens,
> that's key.
>
> You seem to be adding start / end information for each driver init /
> deinit stage. I'd say try to focus on the actual errors you're trying
> to catch.
>
> > Also would it be advisable to add the messages as pr_dbg to be enabled on demand?
>
> I personally have had a pretty poor experience with pr_debug() because
> CONFIG_DYNAMIC_DEBUG is not always enabled. Since you're just printing
> static text there shouldn't be much difference between pr_debug and
> ftrace and/or bpftrace, honestly.
>
> Again, slightly hard to advise not knowing what you're trying to catch.
Linux drivers in general are far too noisy.
In production it is not uncommon to set kernel to suppress all info messages.
^ permalink raw reply
* Re: [LKP] Re: db57e98d87 ("mm/sparse.c: fix ALIGN() without power of 2 in .."): BUG: kernel reboot-without-warning in early-boot stage, last printk: early console in setup code
From: Philip Li @ 2020-07-17 0:27 UTC (permalink / raw)
To: Andrew Morton
Cc: kernel test robot, Lecopzer Chen, Mark-PK Tsai,
Linux Memory Management List, linux-kernel, LKP
In-Reply-To: <20200716150613.409103a2d1e4c3f5f63652b8@linux-foundation.org>
On Thu, Jul 16, 2020 at 03:06:13PM -0700, Andrew Morton wrote:
> On Thu, 16 Jul 2020 16:32:00 +0800 kernel test robot <lkp@intel.com> wrote:
>
> > Greetings,
> >
> > 0day kernel testing robot got the below dmesg and the first bad commit is
> >
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master
> >
> > commit db57e98d87908b8837352abe08515e42752270c1
> > Author: Lecopzer Chen <lecopzer.chen@mediatek.com>
> > AuthorDate: Mon Sep 23 15:36:24 2019 -0700
> > Commit: Linus Torvalds <torvalds@linux-foundation.org>
> > CommitDate: Tue Sep 24 15:54:09 2019 -0700
> >
> > mm/sparse.c: fix ALIGN() without power of 2 in sparse_buffer_alloc()
>
> Are we sure about this? That patch is a year old - has something
> in the test setup changed to make it visible at this late stage?
sorry, kindly ignore this, this looks like a false positive which
probably due to test environment. We will double check, if any finding
related to kernel, we will provide udpdate.
Thanks
> _______________________________________________
> LKP mailing list -- lkp@lists.01.org
> To unsubscribe send an email to lkp-leave@lists.01.org
^ permalink raw reply
* Re: [PATCH v5 08/13] gpio: add support for the sl28cpld GPIO controller
From: kernel test robot @ 2020-07-17 0:25 UTC (permalink / raw)
To: Michael Walle, linux-gpio, devicetree, linux-kernel, linux-hwmon,
linux-pwm, linux-watchdog, linux-arm-kernel
Cc: kbuild-all, clang-built-linux, Linus Walleij, Bartosz Golaszewski,
Rob Herring
In-Reply-To: <20200706175353.16404-9-michael@walle.cc>
[-- Attachment #1: Type: text/plain, Size: 3895 bytes --]
Hi Michael,
I love your patch! Perhaps something to improve:
[auto build test WARNING on ljones-mfd/for-mfd-next]
[also build test WARNING on shawnguo/for-next v5.8-rc5]
[cannot apply to gpio/for-next hwmon/hwmon-next next-20200716]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Michael-Walle/Add-support-for-Kontron-sl28cpld/20200707-020034
base: https://git.kernel.org/pub/scm/linux/kernel/git/lee/mfd.git for-mfd-next
config: x86_64-allyesconfig (attached as .config)
compiler: clang version 12.0.0 (https://github.com/llvm/llvm-project ed6b578040a85977026c93bf4188f996148f3218)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# install x86_64 cross compiling tool for clang build
# apt-get install binutils-x86-64-linux-gnu
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=x86_64
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
>> drivers/gpio/gpio-sl28cpld.c:121:29: warning: implicit conversion from 'unsigned long' to 'unsigned int' changes value from 18446744073709551615 to 4294967295 [-Wconstant-conversion]
config.reg_dir_out_base = GPIO_REGMAP_ADDR(base + GPIO_REG_DIR);
~ ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/gpio/regmap.h:12:44: note: expanded from macro 'GPIO_REGMAP_ADDR'
#define GPIO_REGMAP_ADDR(addr) ((addr) ? : GPIO_REGMAP_ADDR_ZERO)
^~~~~~~~~~~~~~~~~~~~~
include/linux/gpio/regmap.h:11:32: note: expanded from macro 'GPIO_REGMAP_ADDR_ZERO'
#define GPIO_REGMAP_ADDR_ZERO ((unsigned long)(-1))
^~~~~~~~~~~~~~~~~~~
1 warning generated.
vim +121 drivers/gpio/gpio-sl28cpld.c
88
89 static int sl28cpld_gpio_probe(struct platform_device *pdev)
90 {
91 struct gpio_regmap_config config = {0};
92 enum sl28cpld_gpio_type type;
93 struct regmap *regmap;
94 u32 base;
95 int ret;
96
97 if (!pdev->dev.parent)
98 return -ENODEV;
99
100 type = (uintptr_t)device_get_match_data(&pdev->dev);
101 if (!type)
102 return -ENODEV;
103
104 ret = device_property_read_u32(&pdev->dev, "reg", &base);
105 if (ret)
106 return -EINVAL;
107
108 regmap = dev_get_regmap(pdev->dev.parent, NULL);
109 if (!regmap)
110 return -ENODEV;
111
112 config.regmap = regmap;
113 config.parent = &pdev->dev;
114 config.ngpio = 8;
115
116 switch (type) {
117 case SL28CPLD_GPIO:
118 config.reg_dat_base = base + GPIO_REG_IN;
119 config.reg_set_base = base + GPIO_REG_OUT;
120 /* reg_dir_out_base might be zero */
> 121 config.reg_dir_out_base = GPIO_REGMAP_ADDR(base + GPIO_REG_DIR);
122
123 /* This type supports interrupts */
124 ret = sl28cpld_gpio_irq_init(pdev, base, &config);
125 if (ret)
126 return ret;
127 break;
128 case SL28CPLD_GPO:
129 config.reg_set_base = base + GPO_REG_OUT;
130 break;
131 case SL28CPLD_GPI:
132 config.reg_dat_base = base + GPI_REG_IN;
133 break;
134 default:
135 dev_err(&pdev->dev, "unknown type %d\n", type);
136 return -ENODEV;
137 }
138
139 return PTR_ERR_OR_ZERO(devm_gpio_regmap_register(&pdev->dev, &config));
140 }
141
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 75399 bytes --]
^ permalink raw reply
* Re: [PATCH net-next] ibmvnic: Increase driver logging
From: Stephen Hemminger @ 2020-07-17 0:26 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Michal Suchánek, Thomas Falcon, David Miller, drt, netdev,
linuxppc-dev
In-Reply-To: <20200716132200.37934905@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
On Thu, 16 Jul 2020 13:22:00 -0700
Jakub Kicinski <kuba@kernel.org> wrote:
> On Thu, 16 Jul 2020 18:07:37 +0200 Michal Suchánek wrote:
> > On Thu, Jul 16, 2020 at 10:59:58AM -0500, Thomas Falcon wrote:
> > > On 7/15/20 8:29 PM, David Miller wrote:
> > > > From: Jakub Kicinski <kuba@kernel.org>
> > > > Date: Wed, 15 Jul 2020 17:06:32 -0700
> > > >
> > > > > On Wed, 15 Jul 2020 18:51:55 -0500 Thomas Falcon wrote:
> > > > > > free_netdev(netdev);
> > > > > > dev_set_drvdata(&dev->dev, NULL);
> > > > > > + netdev_info(netdev, "VNIC client device has been successfully removed.\n");
> > > > > A step too far, perhaps.
> > > > >
> > > > > In general this patch looks a little questionable IMHO, this amount of
> > > > > logging output is not commonly seen in drivers. All the the info
> > > > > messages are just static text, not even carrying any extra information.
> > > > > In an era of ftrace, and bpftrace, do we really need this?
> > > > Agreed, this is too much. This is debugging, and thus suitable for tracing
> > > > facilities, at best.
> > >
> > > Thanks for your feedback. I see now that I was overly aggressive with this
> > > patch to be sure, but it would help with narrowing down problems at a first
> > > glance, should they arise. The driver in its current state logs very little
> > > of what is it doing without the use of additional debugging or tracing
> > > facilities. Would it be worth it to pursue a less aggressive version or
> > > would that be dead on arrival? What are acceptable driver operations to log
> > > at this level?
>
> Sadly it's much more of an art than hard science. Most networking
> drivers will print identifying information when they probe the device
> and then only about major config changes or when link comes up or goes
> down. And obviously when anything unexpected, like an error happens,
> that's key.
>
> You seem to be adding start / end information for each driver init /
> deinit stage. I'd say try to focus on the actual errors you're trying
> to catch.
>
> > Also would it be advisable to add the messages as pr_dbg to be enabled on demand?
>
> I personally have had a pretty poor experience with pr_debug() because
> CONFIG_DYNAMIC_DEBUG is not always enabled. Since you're just printing
> static text there shouldn't be much difference between pr_debug and
> ftrace and/or bpftrace, honestly.
>
> Again, slightly hard to advise not knowing what you're trying to catch.
Linux drivers in general are far too noisy.
In production it is not uncommon to set kernel to suppress all info messages.
^ permalink raw reply
* Re: [PATCH v4 0/3] ASoC: merge .digital_mute() into .mute_stream()
From: Kuninori Morimoto @ 2020-07-17 0:25 UTC (permalink / raw)
To: Mark Brown; +Cc: Linux-ALSA
In-Reply-To: <87imen2drw.wl-kuninori.morimoto.gx@renesas.com>
Hi Mark
> These are v4 digital_mute() patch which adjusts
> to atmel which had conflict on v3.
I'm sorry but my posted patch has strange numbers.
The correct order is
[3/3] ASoC: soc-core: snd_soc_dai_digital_mute() for both CPU/Codec
[2/3] ASoC: soc-dai: remove .digital_mute
[1/3] ASoC: atmel: merge .digital_mute() into .mute_stream()
Thank you for your help !!
Best regards
---
Kuninori Morimoto
^ permalink raw reply
* Re: [PATCH v5 08/13] gpio: add support for the sl28cpld GPIO controller
From: kernel test robot @ 2020-07-17 0:25 UTC (permalink / raw)
To: kbuild-all
In-Reply-To: <20200706175353.16404-9-michael@walle.cc>
[-- Attachment #1: Type: text/plain, Size: 3996 bytes --]
Hi Michael,
I love your patch! Perhaps something to improve:
[auto build test WARNING on ljones-mfd/for-mfd-next]
[also build test WARNING on shawnguo/for-next v5.8-rc5]
[cannot apply to gpio/for-next hwmon/hwmon-next next-20200716]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Michael-Walle/Add-support-for-Kontron-sl28cpld/20200707-020034
base: https://git.kernel.org/pub/scm/linux/kernel/git/lee/mfd.git for-mfd-next
config: x86_64-allyesconfig (attached as .config)
compiler: clang version 12.0.0 (https://github.com/llvm/llvm-project ed6b578040a85977026c93bf4188f996148f3218)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# install x86_64 cross compiling tool for clang build
# apt-get install binutils-x86-64-linux-gnu
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=x86_64
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
>> drivers/gpio/gpio-sl28cpld.c:121:29: warning: implicit conversion from 'unsigned long' to 'unsigned int' changes value from 18446744073709551615 to 4294967295 [-Wconstant-conversion]
config.reg_dir_out_base = GPIO_REGMAP_ADDR(base + GPIO_REG_DIR);
~ ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/gpio/regmap.h:12:44: note: expanded from macro 'GPIO_REGMAP_ADDR'
#define GPIO_REGMAP_ADDR(addr) ((addr) ? : GPIO_REGMAP_ADDR_ZERO)
^~~~~~~~~~~~~~~~~~~~~
include/linux/gpio/regmap.h:11:32: note: expanded from macro 'GPIO_REGMAP_ADDR_ZERO'
#define GPIO_REGMAP_ADDR_ZERO ((unsigned long)(-1))
^~~~~~~~~~~~~~~~~~~
1 warning generated.
vim +121 drivers/gpio/gpio-sl28cpld.c
88
89 static int sl28cpld_gpio_probe(struct platform_device *pdev)
90 {
91 struct gpio_regmap_config config = {0};
92 enum sl28cpld_gpio_type type;
93 struct regmap *regmap;
94 u32 base;
95 int ret;
96
97 if (!pdev->dev.parent)
98 return -ENODEV;
99
100 type = (uintptr_t)device_get_match_data(&pdev->dev);
101 if (!type)
102 return -ENODEV;
103
104 ret = device_property_read_u32(&pdev->dev, "reg", &base);
105 if (ret)
106 return -EINVAL;
107
108 regmap = dev_get_regmap(pdev->dev.parent, NULL);
109 if (!regmap)
110 return -ENODEV;
111
112 config.regmap = regmap;
113 config.parent = &pdev->dev;
114 config.ngpio = 8;
115
116 switch (type) {
117 case SL28CPLD_GPIO:
118 config.reg_dat_base = base + GPIO_REG_IN;
119 config.reg_set_base = base + GPIO_REG_OUT;
120 /* reg_dir_out_base might be zero */
> 121 config.reg_dir_out_base = GPIO_REGMAP_ADDR(base + GPIO_REG_DIR);
122
123 /* This type supports interrupts */
124 ret = sl28cpld_gpio_irq_init(pdev, base, &config);
125 if (ret)
126 return ret;
127 break;
128 case SL28CPLD_GPO:
129 config.reg_set_base = base + GPO_REG_OUT;
130 break;
131 case SL28CPLD_GPI:
132 config.reg_dat_base = base + GPI_REG_IN;
133 break;
134 default:
135 dev_err(&pdev->dev, "unknown type %d\n", type);
136 return -ENODEV;
137 }
138
139 return PTR_ERR_OR_ZERO(devm_gpio_regmap_register(&pdev->dev, &config));
140 }
141
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org
[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 75399 bytes --]
^ permalink raw reply
* Re: [PATCH -next v2] usb: usbtest: reduce stack usage in test_queue
From: Bixuan Cui @ 2020-07-17 0:25 UTC (permalink / raw)
To: Greg KH
Cc: linux-next, gustavoars, stern, linux-kernel, linux-usb,
qiang.zhang, Wanghui (John)
In-Reply-To: <20200716142620.GB2176745@kroah.com>
On 2020/7/16 22:26, Greg KH wrote:
>> Reported-by: kbuild test robot <lkp@intel.com>
>> Signed-off-by: Bixuan Cui <cuibixuan@huawei.com>
>> ---
>> drivers/usb/misc/usbtest.c | 10 +++++++++-
>> 1 file changed, 9 insertions(+), 1 deletion(-)
> What changed from v1? Always put that below the --- line.
>
> Please fix up and resend a v2.
Thank you,it's my mistake. I resend a v2.
^ permalink raw reply
* Re: [PATCH v6 2/5] rebase -i: support --committer-date-is-author-date
From: Đoàn Trần Công Danh @ 2020-07-17 0:25 UTC (permalink / raw)
To: Phillip Wood
Cc: phillip.wood, Johannes Schindelin, Junio C Hamano, Elijah Newren,
Rohit Ashiwal, Alban Gruin, Git Mailing List
In-Reply-To: <640a3f1b-4e00-4b8a-9a9f-207ea76c648a@gmail.com>
On 2020-07-16 18:34:51+0100, Phillip Wood <phillip.wood123@gmail.com> wrote:
> >> That translation isn't correct either.
> >> It seems like it's recurring pattern.
> >> I'll take it to the Vietnamese translation team.
> >>
> >> Anyway, I've checked with other translation that I can understand
> >> in part. I think
> >>
> >> invalid ident line: %s
> >>
> >> is better candidate for the message.
> >
> > Yes I realized after sending my email that the string we're printing is
> > NUL terminated so we don't need to specify the length with '*'. I think
> > the best thing would be to change the message in this patch to 'invalid
> > ident line: %s' and then have a follow up after this is merged to change
> > all the "invalid ident line" messages to use "identity" instead. Would
> > you be interested in taking on the follow up patch?
>
> In the end I decided it was better just to change the message in this
> patch to something more descriptive. We can update the other commands
> separately. For format-patch we should probably update the option
> description for `--from` as well as the error message.
Yes, I also think we should leave the other messages there for now.
When this topic get merged, we will come back to see which one should
be updated/changed.
All other parts look sane to me.
I don't mind get my hand wet if noone steps in.
Thanks,
Danh
>
> Best Wishes
>
> Phillip
>
> >
> > Best Wishes
> >
> > Phillip
> >
> >> Since Spanish translation also mis-translates the message:
> >>
> >> es.po:9836:msgid "invalid ident line: %.*s"
> >> es.po-9837-msgstr "sangría no válida: %.*s"
> >>
> >> "sangría" also means "indentation" in this context.
> >>
> >> Thanks,
> >> -Danh
> >>
> >>>
> >>> Best Wishes
> >>>
> >>> Phillip
> >>>
> >>>>
> >>>>> + goto out;
> >>>>> + }
> >>>>> + if (!ident.date_begin) {
> >>>>> + res = error(_("corrupted author without date
> >>>>> information"));
> >>>>> + goto out;
> >>>>> + }
> >>>>> +
> >>>>> + strbuf_addf(&date, "@%.*s %.*s",
> >>>>> + (int)(ident.date_end - ident.date_begin),
> >>>>> + ident.date_begin,
> >>>>> + (int)(ident.tz_end - ident.tz_begin),
> >>>>> + ident.tz_begin);
> >>>>> + res = setenv("GIT_COMMITTER_DATE", date.buf, 1);
> >>>>> + strbuf_release(&date);
> >>>>> +
> >>>>> + if (res)
> >>>>> + goto out;
> >>>>> + }
> >>>>> +
> >>>>> if (write_index_as_tree(&tree, r->index, r->index_file, 0,
> >>>>> NULL)) {
> >>>>> res = error(_("git write-tree failed to write a tree"));
> >>>>> goto out;
> >>>>> @@ -2532,6 +2578,11 @@ static int read_populate_opts(struct
> >>>>> replay_opts *opts)
> >>>>> opts->signoff = 1;
> >>>>> }
> >>>>> + if (file_exists(rebase_path_cdate_is_adate())) {
> >>>>> + opts->allow_ff = 0;
> >>>>> + opts->committer_date_is_author_date = 1;
> >>>>> + }
> >>>>> +
> >>>>> if (file_exists(rebase_path_reschedule_failed_exec()))
> >>>>> opts->reschedule_failed_exec = 1;
> >>>>> @@ -2622,6 +2673,8 @@ int write_basic_state(struct replay_opts
> >>>>> *opts, const char *head_name,
> >>>>> write_file(rebase_path_drop_redundant_commits(), "%s", "");
> >>>>> if (opts->keep_redundant_commits)
> >>>>> write_file(rebase_path_keep_redundant_commits(), "%s", "");
> >>>>> + if (opts->committer_date_is_author_date)
> >>>>> + write_file(rebase_path_cdate_is_adate(), "%s", "");
> >>>>> if (opts->reschedule_failed_exec)
> >>>>> write_file(rebase_path_reschedule_failed_exec(), "%s", "");
> >>>>> @@ -3542,6 +3595,10 @@ static int do_merge(struct repository *r,
> >>>>> goto leave_merge;
> >>>>> }
> >>>>> + if (opts->committer_date_is_author_date)
> >>>>> + argv_array_pushf(&cmd.env_array, "GIT_COMMITTER_DATE=%s",
> >>>>> + author_date_from_env_array(&cmd.env_array));
> >>>>> +
> >>>>> cmd.git_cmd = 1;
> >>>>> argv_array_push(&cmd.args, "merge");
> >>>>> argv_array_push(&cmd.args, "-s");
> >>>>> @@ -3819,7 +3876,8 @@ static int pick_commits(struct repository *r,
> >>>>> setenv(GIT_REFLOG_ACTION, action_name(opts), 0);
> >>>>> if (opts->allow_ff)
> >>>>> assert(!(opts->signoff || opts->no_commit ||
> >>>>> - opts->record_origin || opts->edit));
> >>>>> + opts->record_origin || opts->edit ||
> >>>>> + opts->committer_date_is_author_date));
> >>>>> if (read_and_refresh_cache(r, opts))
> >>>>> return -1;
> >>>>> diff --git a/sequencer.h b/sequencer.h
> >>>>> index 0bee85093e..4ab94119ae 100644
> >>>>> --- a/sequencer.h
> >>>>> +++ b/sequencer.h
> >>>>> @@ -45,6 +45,7 @@ struct replay_opts {
> >>>>> int verbose;
> >>>>> int quiet;
> >>>>> int reschedule_failed_exec;
> >>>>> + int committer_date_is_author_date;
> >>>>> int mainline;
> >>>>> diff --git a/t/t3422-rebase-incompatible-options.sh
> >>>>> b/t/t3422-rebase-incompatible-options.sh
> >>>>> index 55ca46786d..c8234062c6 100755
> >>>>> --- a/t/t3422-rebase-incompatible-options.sh
> >>>>> +++ b/t/t3422-rebase-incompatible-options.sh
> >>>>> @@ -61,7 +61,6 @@ test_rebase_am_only () {
> >>>>> }
> >>>>> test_rebase_am_only --whitespace=fix
> >>>>> -test_rebase_am_only --committer-date-is-author-date
> >>>>> test_rebase_am_only -C4
> >>>>> test_expect_success REBASE_P '--preserve-merges incompatible
> >>>>> with --signoff' '
> >>>>> diff --git a/t/t3436-rebase-more-options.sh
> >>>>> b/t/t3436-rebase-more-options.sh
> >>>>> index 4f8a6e51c9..50a63d8ebe 100755
> >>>>> --- a/t/t3436-rebase-more-options.sh
> >>>>> +++ b/t/t3436-rebase-more-options.sh
> >>>>> @@ -9,6 +9,9 @@ test_description='tests to ensure compatibility
> >>>>> between am and interactive backe
> >>>>> . "$TEST_DIRECTORY"/lib-rebase.sh
> >>>>> +GIT_AUTHOR_DATE="1999-04-02T08:03:20+05:30"
> >>>>> +export GIT_AUTHOR_DATE
> >>>>> +
> >>>>> # This is a special case in which both am and interactive backends
> >>>>> # provide the same output. It was done intentionally because
> >>>>> # both the backends fall short of optimal behaviour.
> >>>>> @@ -21,11 +24,20 @@ test_expect_success 'setup' '
> >>>>> test_write_lines "line 1" "new line 2" "line 3" >file &&
> >>>>> git commit -am "update file" &&
> >>>>> git tag side &&
> >>>>> + test_commit commit1 foo foo1 &&
> >>>>> + test_commit commit2 foo foo2 &&
> >>>>> + test_commit commit3 foo foo3 &&
> >>>>> git checkout --orphan master &&
> >>>>> + rm foo &&
> >>>>> test_write_lines "line 1" " line 2" "line 3" >file &&
> >>>>> git commit -am "add file" &&
> >>>>> - git tag main
> >>>>> + git tag main &&
> >>>>> +
> >>>>> + mkdir test-bin &&
> >>>>> + write_script test-bin/git-merge-test <<-\EOF
> >>>>> + exec git-merge-recursive "$@"
> >>>>> + EOF
> >>>>> '
> >>>>> test_expect_success '--ignore-whitespace works with apply
> >>>>> backend' '
> >>>>> @@ -52,6 +64,50 @@ test_expect_success '--ignore-whitespace is
> >>>>> remembered when continuing' '
> >>>>> git diff --exit-code side
> >>>>> '
> >>>>> +test_ctime_is_atime () {
> >>>>> + git log $1 --format=%ai >authortime &&
> >>>>> + git log $1 --format=%ci >committertime &&
> >>>>> + test_cmp authortime committertime
> >>>>> +}
> >>>>> +
> >>>>> +test_expect_success '--committer-date-is-author-date works with
> >>>>> apply backend' '
> >>>>> + GIT_AUTHOR_DATE="@1234 +0300" git commit --amend
> >>>>> --reset-author &&
> >>>>> + git rebase --apply --committer-date-is-author-date HEAD^ &&
> >>>>> + test_ctime_is_atime -1
> >>>>> +'
> >>>>> +
> >>>>> +test_expect_success '--committer-date-is-author-date works with
> >>>>> merge backend' '
> >>>>> + GIT_AUTHOR_DATE="@1234 +0300" git commit --amend
> >>>>> --reset-author &&
> >>>>> + git rebase -m --committer-date-is-author-date HEAD^ &&
> >>>>> + test_ctime_is_atime -1
> >>>>> +'
> >>>>> +
> >>>>> +test_expect_success '--committer-date-is-author-date works with
> >>>>> rebase -r' '
> >>>>> + git checkout side &&
> >>>>> + GIT_AUTHOR_DATE="@1234 +0300" git merge --no-ff commit3 &&
> >>>>> + git rebase -r --root --committer-date-is-author-date &&
> >>>>> + test_ctime_is_atime
> >>>>> +'
> >>>>> +
> >>>>> +test_expect_success '--committer-date-is-author-date works when
> >>>>> forking merge' '
> >>>>> + git checkout side &&
> >>>>> + GIT_AUTHOR_DATE="@1234 +0300" git merge --no-ff commit3 &&
> >>>>> + PATH="./test-bin:$PATH" git rebase -r --root --strategy=test \
> >>>>> + --committer-date-is-author-date &&
> >>>>> + test_ctime_is_atime
> >>>>> +'
> >>>>> +
> >>>>> +test_expect_success '--committer-date-is-author-date works when
> >>>>> committing conflict resolution' '
> >>>>> + git checkout commit2 &&
> >>>>> + GIT_AUTHOR_DATE="@1980 +0000" git commit --amend --only
> >>>>> --reset-author &&
> >>>>> + test_must_fail git rebase -m --committer-date-is-author-date \
> >>>>> + --onto HEAD^^ HEAD^ &&
> >>>>> + echo resolved > foo &&
> >>>>
> >>>> Nitpick: no space after ">" :D
> >>>>
> >>
>
--
Danh
^ permalink raw reply
* [PATCH v4 1/3] ASoC: atmel: merge .digital_mute() into .mute_stream()
From: Kuninori Morimoto @ 2020-07-17 0:23 UTC (permalink / raw)
To: Mark Brown; +Cc: Linux-ALSA
In-Reply-To: <87imen2drw.wl-kuninori.morimoto.gx@renesas.com>
From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
snd_soc_dai_digital_mute() is internally using both
mute_stream() (1) or digital_mute() (2), but the difference between
these 2 are only handling direction.
We can merge digital_mute() into mute_stream
int snd_soc_dai_digital_mute(xxx, int direction)
{
...
else if (dai->driver->ops->mute_stream)
(1) return dai->driver->ops->mute_stream(xxx, direction);
else if (direction == SNDRV_PCM_STREAM_PLAYBACK &&
dai->driver->ops->digital_mute)
(2) return dai->driver->ops->digital_mute(xxx);
...
}
Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
Reviewed-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
Reviewed-by: Alexandre Belloni <alexandre.belloni@bootlin.com>
-
---
sound/soc/atmel/atmel-classd.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/sound/soc/atmel/atmel-classd.c b/sound/soc/atmel/atmel-classd.c
index 2d35b08f0565..0469f50a0366 100644
--- a/sound/soc/atmel/atmel-classd.c
+++ b/sound/soc/atmel/atmel-classd.c
@@ -288,8 +288,8 @@ static int atmel_classd_component_resume(struct snd_soc_component *component)
return regcache_sync(dd->regmap);
}
-static int atmel_classd_cpu_dai_digital_mute(struct snd_soc_dai *cpu_dai,
- int mute)
+static int atmel_classd_cpu_dai_mute_stream(struct snd_soc_dai *cpu_dai,
+ int mute, int direction)
{
struct snd_soc_component *component = cpu_dai->component;
u32 mask, val;
@@ -432,10 +432,11 @@ static int atmel_classd_cpu_dai_trigger(struct snd_pcm_substream *substream,
static const struct snd_soc_dai_ops atmel_classd_cpu_dai_ops = {
.startup = atmel_classd_cpu_dai_startup,
.shutdown = atmel_classd_cpu_dai_shutdown,
- .digital_mute = atmel_classd_cpu_dai_digital_mute,
+ .mute_stream = atmel_classd_cpu_dai_mute_stream,
.hw_params = atmel_classd_cpu_dai_hw_params,
.prepare = atmel_classd_cpu_dai_prepare,
.trigger = atmel_classd_cpu_dai_trigger,
+ .no_capture_mute = 1,
};
static struct snd_soc_dai_driver atmel_classd_cpu_dai = {
--
2.25.1
^ permalink raw reply related
* Re: [Intel-gfx] sw_sync deadlock avoidance, take 3
From: Daniel Stone @ 2020-07-17 0:24 UTC (permalink / raw)
To: Daniel Vetter
Cc: Rob Herring, intel-gfx, ML dri-devel, Chris Wilson, John Stultz,
Bas Nieuwenhuizen
In-Reply-To: <CAKMK7uFw_cu4kc5poS2et3j0UXogS4z0Gt3CXj-kS9OvpGpkDA@mail.gmail.com>
Hi all,
On Wed, 15 Jul 2020 at 12:57, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Wed, Jul 15, 2020 at 1:47 PM Daniel Stone <daniel@fooishbar.org> wrote:
> > On Wed, 15 Jul 2020 at 12:05, Bas Nieuwenhuizen <bas@basnieuwenhuizen.nl> wrote:
> > > Yes, this is used as part of the Android stack on Chrome OS (need to
> > > see if ChromeOS specific, but
> > > https://source.android.com/devices/graphics/sync#sync_timeline
> > > suggests not)
> >
> > Android used to mandate it for their earlier iteration of release
> > fences, which was an empty/future fence having no guarantee of
> > eventual forward progress until someone committed work later on. For
> > example, when you committed a buffer to SF, it would give you an empty
> > 'release fence' for that buffer which would only be tied to work to
> > signal it when you committed your _next_ buffer, which might never
> > happen. They removed that because a) future fences were a bad idea,
> > and b) it was only ever useful if you assumed strictly
> > FIFO/round-robin return order which wasn't always true.
> >
> > So now it's been watered down to 'use this if you don't have a
> > hardware timeline', but why don't we work with Android people to get
> > that removed entirely?
>
> I think there's some testcases still using these, but most real fence
> testcases use vgem nowadays. So from an upstream pov there's indeed
> not much if anything holding us back from just deleting this all. And
> would probably be a good idea.
It looks like this is just a docs hangover which can be fixed; sw_sync
is no longer part of the unified Android kernel image, so it can no
longer be relied on post-Treble. So let's just continue on the
assumption that sw_sync is not anything anyone can rely on.
Cheers,
Daniel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply
* Re: [Intel-gfx] sw_sync deadlock avoidance, take 3
From: Daniel Stone @ 2020-07-17 0:24 UTC (permalink / raw)
To: Daniel Vetter; +Cc: intel-gfx, ML dri-devel, Chris Wilson
In-Reply-To: <CAKMK7uFw_cu4kc5poS2et3j0UXogS4z0Gt3CXj-kS9OvpGpkDA@mail.gmail.com>
Hi all,
On Wed, 15 Jul 2020 at 12:57, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Wed, Jul 15, 2020 at 1:47 PM Daniel Stone <daniel@fooishbar.org> wrote:
> > On Wed, 15 Jul 2020 at 12:05, Bas Nieuwenhuizen <bas@basnieuwenhuizen.nl> wrote:
> > > Yes, this is used as part of the Android stack on Chrome OS (need to
> > > see if ChromeOS specific, but
> > > https://source.android.com/devices/graphics/sync#sync_timeline
> > > suggests not)
> >
> > Android used to mandate it for their earlier iteration of release
> > fences, which was an empty/future fence having no guarantee of
> > eventual forward progress until someone committed work later on. For
> > example, when you committed a buffer to SF, it would give you an empty
> > 'release fence' for that buffer which would only be tied to work to
> > signal it when you committed your _next_ buffer, which might never
> > happen. They removed that because a) future fences were a bad idea,
> > and b) it was only ever useful if you assumed strictly
> > FIFO/round-robin return order which wasn't always true.
> >
> > So now it's been watered down to 'use this if you don't have a
> > hardware timeline', but why don't we work with Android people to get
> > that removed entirely?
>
> I think there's some testcases still using these, but most real fence
> testcases use vgem nowadays. So from an upstream pov there's indeed
> not much if anything holding us back from just deleting this all. And
> would probably be a good idea.
It looks like this is just a docs hangover which can be fixed; sw_sync
is no longer part of the unified Android kernel image, so it can no
longer be relied on post-Treble. So let's just continue on the
assumption that sw_sync is not anything anyone can rely on.
Cheers,
Daniel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply
* [PATCH v4 2/3] ASoC: soc-core: snd_soc_dai_digital_mute() for both CPU/Codec
From: Kuninori Morimoto @ 2020-07-17 0:22 UTC (permalink / raw)
To: Mark Brown; +Cc: Linux-ALSA
In-Reply-To: <87imen2drw.wl-kuninori.morimoto.gx@renesas.com>
From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
snd_soc_dai_digital_mute() is used for both CPU and Codec.
For example, soc_pcm_prepare() / soc_pcm_hw_free() are caring
both CPU and Codec.
But soc_resume_deferred() / snd_soc_suspend() are not.
This patch cares it.
Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
Reviewed-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
---
sound/soc/soc-core.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c
index 7c58e45c1c3f..defd96b14c28 100644
--- a/sound/soc/soc-core.c
+++ b/sound/soc/soc-core.c
@@ -548,7 +548,7 @@ int snd_soc_suspend(struct device *dev)
if (rtd->dai_link->ignore_suspend)
continue;
- for_each_rtd_codec_dais(rtd, i, dai) {
+ for_each_rtd_dais(rtd, i, dai) {
if (snd_soc_dai_stream_active(dai, playback))
snd_soc_dai_digital_mute(dai, 1, playback);
}
@@ -687,7 +687,7 @@ static void soc_resume_deferred(struct work_struct *work)
if (rtd->dai_link->ignore_suspend)
continue;
- for_each_rtd_codec_dais(rtd, i, dai) {
+ for_each_rtd_dais(rtd, i, dai) {
if (snd_soc_dai_stream_active(dai, playback))
snd_soc_dai_digital_mute(dai, 0, playback);
}
--
2.25.1
^ permalink raw reply related
* [PATCH v4 1/3] ASoC: soc-dai: remove .digital_mute
From: Kuninori Morimoto @ 2020-07-17 0:21 UTC (permalink / raw)
To: Mark Brown; +Cc: Linux-ALSA
In-Reply-To: <87imen2drw.wl-kuninori.morimoto.gx@renesas.com>
From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
All drivers are now using .mute_stream.
Let's remove .digital_mute.
Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
Reviewed-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
---
include/sound/soc-dai.h | 1 -
sound/soc/soc-dai.c | 4 ----
2 files changed, 5 deletions(-)
diff --git a/include/sound/soc-dai.h b/include/sound/soc-dai.h
index e0e061b8e9bd..2c5a49411276 100644
--- a/include/sound/soc-dai.h
+++ b/include/sound/soc-dai.h
@@ -246,7 +246,6 @@ struct snd_soc_dai_ops {
* DAI digital mute - optional.
* Called by soc-core to minimise any pops.
*/
- int (*digital_mute)(struct snd_soc_dai *dai, int mute);
int (*mute_stream)(struct snd_soc_dai *dai, int mute, int stream);
/*
diff --git a/sound/soc/soc-dai.c b/sound/soc/soc-dai.c
index 458d2ea44329..c89a1929d141 100644
--- a/sound/soc/soc-dai.c
+++ b/sound/soc/soc-dai.c
@@ -307,10 +307,6 @@ int snd_soc_dai_digital_mute(struct snd_soc_dai *dai, int mute,
(direction == SNDRV_PCM_STREAM_PLAYBACK ||
!dai->driver->ops->no_capture_mute))
ret = dai->driver->ops->mute_stream(dai, mute, direction);
- else if (direction == SNDRV_PCM_STREAM_PLAYBACK &&
- dai->driver->ops &&
- dai->driver->ops->digital_mute)
- ret = dai->driver->ops->digital_mute(dai, mute);
return soc_dai_ret(dai, ret);
}
--
2.25.1
^ permalink raw reply related
* [PATCH -next v2] usb: usbtest: reduce stack usage in test_queue
From: Bixuan Cui @ 2020-07-17 0:22 UTC (permalink / raw)
To: Alan Stern
Cc: linux-next, gustavoars, gregkh, linux-kernel, linux-usb,
qiang.zhang, Wanghui (John)
In-Reply-To: <20200716154510.GE1112537@rowland.harvard.edu>
Fix the warning: [-Werror=-Wframe-larger-than=]
drivers/usb/misc/usbtest.c: In function 'test_queue':
drivers/usb/misc/usbtest.c:2148:1:
warning: the frame size of 1232 bytes is larger than 1024 bytes
Reported-by: kbuild test robot <lkp@intel.com>
Acked-by: Alan Stern <stern@rowland.harvard.edu>
Signed-off-by: Bixuan Cui <cuibixuan@huawei.com>
---
v2: Change MAX_SGLEN to param->sglen.
drivers/usb/misc/usbtest.c | 10 +++++++++-
1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/misc/usbtest.c b/drivers/usb/misc/usbtest.c
index 8b220d56647b..150090ee4ec1 100644
--- a/drivers/usb/misc/usbtest.c
+++ b/drivers/usb/misc/usbtest.c
@@ -2043,7 +2043,7 @@ test_queue(struct usbtest_dev *dev, struct usbtest_param_32 *param,
unsigned i;
unsigned long packets = 0;
int status = 0;
- struct urb *urbs[MAX_SGLEN];
+ struct urb **urbs;
if (!param->sglen || param->iterations > UINT_MAX / param->sglen)
return -EINVAL;
@@ -2051,6 +2051,10 @@ test_queue(struct usbtest_dev *dev, struct usbtest_param_32 *param,
if (param->sglen > MAX_SGLEN)
return -EINVAL;
+ urbs = kcalloc(param->sglen, sizeof(*urbs), GFP_KERNEL);
+ if (!urbs)
+ return -ENOMEM;
+
memset(&context, 0, sizeof(context));
context.count = param->iterations * param->sglen;
context.dev = dev;
@@ -2137,6 +2141,8 @@ test_queue(struct usbtest_dev *dev, struct usbtest_param_32 *param,
else if (context.errors >
(context.is_iso ? context.packet_count / 10 : 0))
status = -EIO;
+
+ kfree(urbs);
return status;
fail:
@@ -2144,6 +2150,8 @@ test_queue(struct usbtest_dev *dev, struct usbtest_param_32 *param,
if (urbs[i])
simple_free_urb(urbs[i]);
}
+
+ kfree(urbs);
return status;
}
--
2.17.1
.
^ permalink raw reply related
* Re: [PATCH -next v2] usb: usbtest: reduce stack usage in test_queue
From: Bixuan Cui @ 2020-07-17 0:22 UTC (permalink / raw)
To: Alan Stern
Cc: linux-next, gustavoars, gregkh, linux-kernel, linux-usb,
qiang.zhang, Wanghui (John)
In-Reply-To: <20200716154510.GE1112537@rowland.harvard.edu>
Fix the warning: [-Werror=-Wframe-larger-than=]
drivers/usb/misc/usbtest.c: In function 'test_queue':
drivers/usb/misc/usbtest.c:2148:1:
warning: the frame size of 1232 bytes is larger than 1024 bytes
Reported-by: kbuild test robot <lkp@intel.com>
Acked-by: Alan Stern <stern@rowland.harvard.edu>
Signed-off-by: Bixuan Cui <cuibixuan@huawei.com>
---
v2: Change MAX_SGLEN to param->sglen.
drivers/usb/misc/usbtest.c | 10 +++++++++-
1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/misc/usbtest.c b/drivers/usb/misc/usbtest.c
index 8b220d56647b..150090ee4ec1 100644
--- a/drivers/usb/misc/usbtest.c
+++ b/drivers/usb/misc/usbtest.c
@@ -2043,7 +2043,7 @@ test_queue(struct usbtest_dev *dev, struct usbtest_param_32 *param,
unsigned i;
unsigned long packets = 0;
int status = 0;
- struct urb *urbs[MAX_SGLEN];
+ struct urb **urbs;
if (!param->sglen || param->iterations > UINT_MAX / param->sglen)
return -EINVAL;
@@ -2051,6 +2051,10 @@ test_queue(struct usbtest_dev *dev, struct usbtest_param_32 *param,
if (param->sglen > MAX_SGLEN)
return -EINVAL;
+ urbs = kcalloc(param->sglen, sizeof(*urbs), GFP_KERNEL);
+ if (!urbs)
+ return -ENOMEM;
+
memset(&context, 0, sizeof(context));
context.count = param->iterations * param->sglen;
context.dev = dev;
@@ -2137,6 +2141,8 @@ test_queue(struct usbtest_dev *dev, struct usbtest_param_32 *param,
else if (context.errors >
(context.is_iso ? context.packet_count / 10 : 0))
status = -EIO;
+
+ kfree(urbs);
return status;
fail:
@@ -2144,6 +2150,8 @@ test_queue(struct usbtest_dev *dev, struct usbtest_param_32 *param,
if (urbs[i])
simple_free_urb(urbs[i]);
}
+
+ kfree(urbs);
return status;
}
--
2.17.1
.
^ permalink raw reply related
* [PATCH v4 0/3] ASoC: merge .digital_mute() into .mute_stream()
From: Kuninori Morimoto @ 2020-07-17 0:21 UTC (permalink / raw)
To: Mark Brown; +Cc: Linux-ALSA
Hi Mark
These are v4 digital_mute() patch which adjusts
to atmel which had conflict on v3.
v3 -> v4
- tidyup for atmel which had conflict
v2 -> v3
- uses "xxx_mute_stream" for .mute_stream naming
if it was better
- removed verbose Cc email address
v1 -> v2
- return -ENOTSUPP at hdmi-codec
- add new .no_capture_mute flag and emulate .digital_mute()
by .mute_stream()
Link: https://lore.kernel.org/r/87h7uhxxk6.wl-kuninori.morimoto.gx@renesas.com
Link: https://lore.kernel.org/r/874kqy2y5t.wl-kuninori.morimoto.gx@renesas.com
Link: https://lore.kernel.org/r/87ftam37ko.wl-kuninori.morimoto.gx@renesas.com
Kuninori Morimoto (3):
ASoC: soc-dai: remove .digital_mute
ASoC: soc-core: snd_soc_dai_digital_mute() for both CPU/Codec
ASoC: soc-xxx: add asoc_substream_to_rtd()
include/sound/soc-dai.h | 1 -
include/sound/soc.h | 2 +
sound/soc/soc-component.c | 20 ++++-----
sound/soc/soc-core.c | 4 +-
sound/soc/soc-dai.c | 12 ++----
sound/soc/soc-dapm.c | 6 +--
sound/soc/soc-generic-dmaengine-pcm.c | 4 +-
sound/soc/soc-link.c | 12 +++---
sound/soc/soc-pcm.c | 62 +++++++++++++--------------
sound/soc/soc-utils.c | 2 +-
10 files changed, 61 insertions(+), 64 deletions(-)
--
2.25.1
^ permalink raw reply
* Re: [PATCH] cpufreq: intel_pstate: Implement passive mode with HWP enabled
From: Francisco Jerez @ 2020-07-17 0:21 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Rafael J. Wysocki, Rafael J. Wysocki, Linux PM,
Linux Documentation, LKML, Peter Zijlstra, Srinivas Pandruvada,
Giovanni Gherdovich, Doug Smythies
In-Reply-To: <CAJZ5v0hhLWvbNA6w0yHtzKa5ANR9yF++u63dh8wWAgkhbtLXXA@mail.gmail.com>
[-- Attachment #1.1: Type: text/plain, Size: 25393 bytes --]
"Rafael J. Wysocki" <rafael@kernel.org> writes:
> On Wed, Jul 15, 2020 at 11:35 PM Francisco Jerez <currojerez@riseup.net> wrote:
>>
>> "Rafael J. Wysocki" <rafael@kernel.org> writes:
>>
>> > On Wed, Jul 15, 2020 at 2:09 AM Francisco Jerez <currojerez@riseup.net> wrote:
>> >>
>> >> "Rafael J. Wysocki" <rjw@rjwysocki.net> writes:
>> >>
>> >> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>> >> >
>> >> > Allow intel_pstate to work in the passive mode with HWP enabled and
>> >> > make it set the HWP minimum performance limit (HWP floor) to the
>> >> > P-state value given by the target frequency supplied by the cpufreq
>> >> > governor, so as to prevent the HWP algorithm and the CPU scheduler
>> >> > from working against each other, at least when the schedutil governor
>> >> > is in use, and update the intel_pstate documentation accordingly.
>> >> >
>> >> > Among other things, this allows utilization clamps to be taken
>> >> > into account, at least to a certain extent, when intel_pstate is
>> >> > in use and makes it more likely that sufficient capacity for
>> >> > deadline tasks will be provided.
>> >> >
>> >> > After this change, the resulting behavior of an HWP system with
>> >> > intel_pstate in the passive mode should be close to the behavior
>> >> > of the analogous non-HWP system with intel_pstate in the passive
>> >> > mode, except that in the frequency range below the base frequency
>> >> > (ie. the frequency retured by the base_frequency cpufreq attribute
>> >> > in sysfs on HWP systems) the HWP algorithm is allowed to go above
>> >> > the floor P-state set by intel_pstate with or without hardware
>> >> > coordination of P-states among CPUs in the same package.
>> >> >
>> >> > Also note that the setting of the HWP floor may not be taken into
>> >> > account by the processor in the following cases:
>> >> >
>> >> > * For the HWP floor in the range of P-states above the base
>> >> > frequency, referred to as the turbo range, the processor has a
>> >> > license to choose any P-state from that range, either below or
>> >> > above the HWP floor, just like a non-HWP processor in the case
>> >> > when the target P-state falls into the turbo range.
>> >> >
>> >> > * If P-states of the CPUs in the same package are coordinated
>> >> > at the hardware level, the processor may choose a P-state
>> >> > above the HWP floor, just like a non-HWP processor in the
>> >> > analogous case.
>> >> >
>> >> > With this change applied, intel_pstate in the passive mode
>> >> > assumes complete control over the HWP request MSR and concurrent
>> >> > changes of that MSR (eg. via the direct MSR access interface) are
>> >> > overridden by it.
>> >> >
>> >> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>> >> > ---
>> >> >
>> >> > This basically unifies the passive mode behavior of intel_pstate for systems
>> >> > with and without HWP enabled. The only case in which there is a difference
>> >> > between the two (after this patch) is below the turbo range, where the HWP
>> >> > algorithm can go above the floor regardless of whether or not P-state are
>> >> > coordinated package-wide (this means the systems with per-core P-states
>> >> > mostly is where the difference can be somewhat visible).
>> >> >
>> >> > Since the passive mode hasn't worked with HWP at all, and it is not going to
>> >> > the default for HWP systems anyway, I don't see any drawbacks related to making
>> >> > this change, so I would consider this as 5.9 material unless there are any
>> >> > serious objections.
>> >> >
>> >> > Thanks!
>> >> >
>> >> > ---
>> >> > Documentation/admin-guide/pm/intel_pstate.rst | 89 +++++++---------
>> >> > drivers/cpufreq/intel_pstate.c | 141 ++++++++++++++++++++------
>> >> > 2 files changed, 152 insertions(+), 78 deletions(-)
>> >> >
>> >> > Index: linux-pm/drivers/cpufreq/intel_pstate.c
>> >> > ===================================================================
>> >> > --- linux-pm.orig/drivers/cpufreq/intel_pstate.c
>> >> > +++ linux-pm/drivers/cpufreq/intel_pstate.c
>> >> > @@ -36,6 +36,7 @@
>> >> > #define INTEL_PSTATE_SAMPLING_INTERVAL (10 * NSEC_PER_MSEC)
>> >> >
>> >> > #define INTEL_CPUFREQ_TRANSITION_LATENCY 20000
>> >> > +#define INTEL_CPUFREQ_TRANSITION_DELAY_HWP 5000
>> >> > #define INTEL_CPUFREQ_TRANSITION_DELAY 500
>> >> >
>> >> > #ifdef CONFIG_ACPI
>> >> > @@ -222,6 +223,7 @@ struct global_params {
>> >> > * preference/bias
>> >> > * @epp_saved: Saved EPP/EPB during system suspend or CPU offline
>> >> > * operation
>> >> > + * @epp_cached Cached HWP energy-performance preference value
>> >> > * @hwp_req_cached: Cached value of the last HWP Request MSR
>> >> > * @hwp_cap_cached: Cached value of the last HWP Capabilities MSR
>> >> > * @last_io_update: Last time when IO wake flag was set
>> >> > @@ -259,6 +261,7 @@ struct cpudata {
>> >> > s16 epp_policy;
>> >> > s16 epp_default;
>> >> > s16 epp_saved;
>> >> > + s16 epp_cached;
>> >> > u64 hwp_req_cached;
>> >> > u64 hwp_cap_cached;
>> >> > u64 last_io_update;
>> >> > @@ -676,6 +679,8 @@ static int intel_pstate_set_energy_pref_
>> >> >
>> >> > value |= (u64)epp << 24;
>> >> > ret = wrmsrl_on_cpu(cpu_data->cpu, MSR_HWP_REQUEST, value);
>> >> > +
>> >> > + WRITE_ONCE(cpu_data->epp_cached, epp);
>> >>
>> >> Why introduce a new EPP cache variable if there is already
>> >> hwp_req_cached? If intel_pstate_set_energy_pref_index() is failing to
>> >> update hwp_req_cached maybe we should fix that instead. That will save
>> >> you a little bit of work in intel_cpufreq_adjust_hwp().
>> >
>> > Yes, it would, but then we'd need to explicitly synchronize
>> > intel_pstate_set_energy_pref_index() with the scheduler context which
>> > I'd rather avoid.
>> >
>>
>> How does using a differently named variable save you from doing that?
>
> It is a separate variable.
>
> The only updater of epp_cached, except for the initialization, is
> intel_pstate_set_energy_pref_index() and it cannot race with another
> instance of itself, so there are no concurrent writes to epp_cached.
>
> In the passive mode the only updater of hwp_req_cached, except for the
> initialization, is intel_cpufreq_adjust_hwp() (or there is a bug in
> the patch that I have missed) and it cannot race with another instance
> of itself for the same CPU, so there are no concurrent writes to
> hwp_req_cached.
>
> if intel_pstate_set_energy_pref_index() updated hwp_req_cached
> directly, however, it might be updated in two places concurrently and
> so explicit synchronization would be necessary.
>
That's fair, but we may need to add such synchronization anyway due to
the bug I pointed out above, so it might be simpler to avoid introducing
additional state and simply stick to hwp_req_cached with proper
synchronization.
>> And won't the EPP setting programmed by intel_pstate_set_energy_pref_index()
>> be lost if intel_pstate_hwp_boost_up() or some other user of
>> hwp_req_cached is executed afterwards with the current approach?
>
> The value written to the register by it may be overwritten by a
> concurrent intel_cpufreq_adjust_hwp(), but that is not a problem,
> because next time intel_cpufreq_adjust_hwp() runs for the target CPU,
> it will pick up the updated epp_cached value which will be written to
> the register.
However intel_cpufreq_adjust_hwp() may never be executed afterwards if
intel_pstate is in active mode, in which case the overwritten value may
remain there forever potentially.
> So there may be a short time window after the
> intel_pstate_set_energy_pref_index() invocation in which the new EPP
> value may not be in effect, but in general there is no guarantee that
> the new EPP will take effect immediately after updating the MSR
> anyway, so that race doesn't matter.
>
> That said, that race is avoidable, but I was thinking that trying to
> avoid it might not be worth it. Now I see a better way to avoid it,
> though, so I'm going to update the patch to that end.
>
>> Seems like a bug to me.
>
> It is racy, but not every race is a bug.
>
Still seems like there is a bug in intel_pstate_set_energy_pref_index()
AFAICT.
>> >> > } else {
>> >> > if (epp == -EINVAL)
>> >> > epp = (pref_index - 1) << 2;
>> >> > @@ -2047,6 +2052,7 @@ static int intel_pstate_init_cpu(unsigne
>> >> > cpu->epp_default = -EINVAL;
>> >> > cpu->epp_powersave = -EINVAL;
>> >> > cpu->epp_saved = -EINVAL;
>> >> > + WRITE_ONCE(cpu->epp_cached, -EINVAL);
>> >> > }
>> >> >
>> >> > cpu = all_cpu_data[cpunum];
>> >> > @@ -2245,7 +2251,10 @@ static int intel_pstate_verify_policy(st
>> >> >
>> >> > static void intel_cpufreq_stop_cpu(struct cpufreq_policy *policy)
>> >> > {
>> >> > - intel_pstate_set_min_pstate(all_cpu_data[policy->cpu]);
>> >> > + if (hwp_active)
>> >> > + intel_pstate_hwp_force_min_perf(policy->cpu);
>> >> > + else
>> >> > + intel_pstate_set_min_pstate(all_cpu_data[policy->cpu]);
>> >> > }
>> >> >
>> >> > static void intel_pstate_stop_cpu(struct cpufreq_policy *policy)
>> >> > @@ -2253,12 +2262,10 @@ static void intel_pstate_stop_cpu(struct
>> >> > pr_debug("CPU %d exiting\n", policy->cpu);
>> >> >
>> >> > intel_pstate_clear_update_util_hook(policy->cpu);
>> >> > - if (hwp_active) {
>> >> > + if (hwp_active)
>> >> > intel_pstate_hwp_save_state(policy);
>> >> > - intel_pstate_hwp_force_min_perf(policy->cpu);
>> >> > - } else {
>> >> > - intel_cpufreq_stop_cpu(policy);
>> >> > - }
>> >> > +
>> >> > + intel_cpufreq_stop_cpu(policy);
>> >> > }
>> >> >
>> >> > static int intel_pstate_cpu_exit(struct cpufreq_policy *policy)
>> >> > @@ -2388,13 +2395,82 @@ static void intel_cpufreq_trace(struct c
>> >> > fp_toint(cpu->iowait_boost * 100));
>> >> > }
>> >> >
>> >> > +static void intel_cpufreq_adjust_hwp(struct cpudata *cpu, u32 target_pstate,
>> >> > + bool fast_switch)
>> >> > +{
>> >> > + u64 prev = READ_ONCE(cpu->hwp_req_cached), value = prev;
>> >> > + s16 epp;
>> >> > +
>> >> > + value &= ~HWP_MIN_PERF(~0L);
>> >> > + value |= HWP_MIN_PERF(target_pstate);
>> >> > +
>> >> > + /*
>> >> > + * The entire MSR needs to be updated in order to update the HWP min
>> >> > + * field in it, so opportunistically update the max too if needed.
>> >> > + */
>> >> > + value &= ~HWP_MAX_PERF(~0L);
>> >> > + value |= HWP_MAX_PERF(cpu->max_perf_ratio);
>> >> > +
>> >> > + /*
>> >> > + * In case the EPP has been adjusted via sysfs, write the last cached
>> >> > + * value of it to the MSR as well.
>> >> > + */
>> >> > + epp = READ_ONCE(cpu->epp_cached);
>> >> > + if (epp >= 0) {
>> >> > + value &= ~GENMASK_ULL(31, 24);
>> >> > + value |= (u64)epp << 24;
>> >> > + }
>> >> > +
>> >> > + if (value == prev)
>> >> > + return;
>> >> > +
>> >> > + WRITE_ONCE(cpu->hwp_req_cached, value);
>> >> > + if (fast_switch)
>> >> > + wrmsrl(MSR_HWP_REQUEST, value);
>> >> > + else
>> >> > + wrmsrl_on_cpu(cpu->cpu, MSR_HWP_REQUEST, value);
>> >> > +}
>> >>
>> >> I've asked this question already but you may have missed it: Given that
>> >> you are of the opinion that [1] should be implemented in schedutil
>> >> instead with intel_pstate in HWP passive mode, what's your plan for
>> >> exposing the HWP_MAX_PERF knob to the governor in addition to
>> >> HWP_MIN_PERF, since the interface implemented here only allows the
>> >> governor to provide a single frequency?
>> >>
>> >> [1] https://lwn.net/ml/linux-pm/20200428032258.2518-1-currojerez@riseup.net/
>> >
>> > This is not just about the schedutil governor, but about cpufreq
>> > governors in general (someone may still want to use the performance
>> > governor on top of intel_pstate, for example).
>> >
>> > And while governors can only provide one frequency, the policy limits
>> > in the cpufreq framework are based on QoS lists now and so it is
>> > possible to add a max limit request, say from a driver, to the max QoS
>> > list, and update it as needed, causing the max policy limit to be
>> > adjusted.
>> >
>> > That said I'm not exactly sure how useful the max limit generally is
>> > in practice on HWP systems, given that setting it above the base
>> > frequency causes it to be ignored, effectively, and the turbo range
>> > may be wider than the range of P-states below the base frequency.
>> >
>>
>> I don't think that's accurate. I've looked at hundreds of traces while
>> my series [1] was in control of HWP_REQ_MAX and I've never seen an
>> excursion above the maximum HWP_REQ_MAX control specified by it within a
>> given P-state domain, even while that maximum specified was well into
>> the turbo range.
>
> I'm not going to argue with your experience. :-)
>
> What I'm saying is that there is no guarantee that the processor will
> always select P-states below HWP_REQ_MAX in the turbo range. That may
> not happen in practice, but it is not precluded AFAICS.
>
> Also while HWP_REQ_MAX can work in practice most of the time with HWP
> enabled, without HWP there is no easy way to limit the max frequency
> if the current request falls into the turbo range. The HWP case is
> more important nowadays, but there still are systems without it and
> ideally they should be covered as well.
>
In the non-HWP case we have a single P-state control so the question of
how to plumb an extra P-state control from the CPUFREQ governor seems
largely irrelevant. The current interface seems fine as-is for such
systems.
>> So, yeah, I agree that HWP_REQ_MAX is nothing like a
>> hard limit, particularly when multiple threads are running on the same
>> clock domain, but the processor will still make its best effort to limit
>> the clock frequency to the maximum of the requested maximums, even if it
>> happens to be within the turbo range. That doesn't make it useless.
>
> I haven't used the word "useless" anywhere in my previous message.
>
> Using the max frequency to control power has merit, but how much of it
> is there depends on some factors that may change from one system to
> another.
>
> The alternative power control methods may be more reliable in general.
>
That's precisely what I've been calling into question. IIRC the
alternative power control methods we have discussed in the past are:
- EPP: The semantics of this control are largely unspecified beyond
higher values being more energy-efficient than lower values. The set
of energy efficiency optimizations controlled by it and the
thresholds at which they become active are fully platform-specific.
I guess you didn't mean this one as example of a more reliable and
less platform-specific control.
- RAPL: The semantics of this control are indeed well-defined, it's
able to set an absolute average power constraint to the involved
power planes. However, the relationship between the information
available to the kernel about a given workload (e.g. from CPU
performance counters) and the optimal value of the RAPL constraints
is highly platform-specific, requiring multiple iterations of
adjustments and performance monitoring in order to approach the
optimal value (unlike HWP_REQ_MAX since there is a simple,
platform-independent relationship between observed frequency
and... frequency -- More on that below).
- P-code mailbox interface: Available to the graphics driver when GuC
submission is in use, which is not available currently on any
production platform. It won't allow the energy efficiency
optimization I'm proposing to be taken advantage of by discrete
graphics nor IO devices other than the GPU. Like HWP_REQ_MAX it sets
a constraint on the CPU P-states so most caveats of HWP_REQ_MAX would
apply to it too. But unlike HWP_REQ_MAX it has global effect on the
system limiting its usefulness in a multitasking environment.
Requires a governor to run in a GPU microcontroller with more limited
information than CPUFREQ.
So I'm either missing some alternative power control method or I
strongly disagree that there is a more reliable and platform-independent
alternative to HWP_REQ_MAX.
>> The exact same thing can be said about controlling HWP_REQ_MIN as you're
>> doing now in this revision of your patch, BTW.
>
> Which has been clearly stated in the changelog I believe.
>
Right, which is why I found it surprising to hear the same point as a
counterargument against HWP_REQ_MAX.
> The point here is that this is as good as using the perf control
> register to ask for a given P-state without HWP which trying to drive
> the max too is added complexity.
>
>> If you don't believe me here is the turbostat sample with maximum
>> Bzy_MHz I get on the computer I'm sitting on right now while compiling a
>> kernel on CPU0 if I set HWP_REQ_MAX to 0x1c (within the turbo range):
>>
>> | Core CPU Avg_MHz Busy% Bzy_MHz HWP_REQ PkgWatt CorWatt
>> | - - 757 27.03 2800 0x0000000000000000 7.13 4.90
>> | 0 0 2794 99.77 2800 0x0000000080001c04 7.13 4.90
>> | 0 2 83 2.98 2800 0x0000000080001c04
>> | 1 1 73 2.60 2800 0x0000000080001c04
>> | 1 3 78 2.79 2800 0x0000000080001c04
>>
>> With the default HWP_REQUEST:
>>
>> | Core CPU Avg_MHz Busy% Bzy_MHz HWP_REQ PkgWatt CorWatt
>> | - - 814 27.00 3015 0x0000000000000000 8.49 6.18
>> | 0 0 2968 98.24 3021 0x0000000080001f04 8.49 6.18
>> | 0 2 84 2.81 2982 0x0000000080001f04
>> | 1 1 99 3.34 2961 0x0000000080001f04
>> | 1 3 105 3.60 2921 0x0000000080001f04
>>
>> > Generally, I'm not quite convinced that limiting the max frequency is
>> > really the right choice for controlling the processor's power draw on
>> > the systems in question. There are other ways to do that, which in
>> > theory should be more effective. I mentioned RAPL somewhere in this
>> > context and there's the GUC firmware too.
>>
>> I feel like we've had that conversation before and it's somewhat
>> off-topic so I'll keep it short: Yes, in theory RAPL is more effective
>> than HWP_REQ_MAX as a mechanism to limit the absolute power consumption
>> of the processor package, but that's not the purpose of [1], its purpose
>> is setting a lower limit to the energy efficiency of the processor when
>> the maximum usable CPU frequency is known (due to the existence of an IO
>> device bottleneck)
>
> Whether or not that frequency is actually known seems quite
> questionable to me, but that's aside.
>
It's not actually known, but it can be approximated easily under a
widely-applicable assumption -- More on that below.
> More important, it is unclear to me what you mean by "a lower limit to
> the energy efficiency of the processor".
>
If we define the instantaneous energy efficiency of a CPU (eta) to be
the ratio between its instantaneous frequency (f) and power consumption
(P), I want to be able to set a lower limit to that ratio in cases where
I can determine that doing so won't impact the performance of the
application:
| eta_min <= eta = f / P
Setting such a lower limit to the instantaneous energy efficiency of the
processor can only lower the total amount of energy consumed by the
processor in order to perform a given amount of work (If you don't
believe me on that feel free to express it as the integral of P over
time, with P recovered from the expression above), therefore it can only
improve the average energy efficiency of the workload in the long run.
Because of the convex relationship between P and f above a certain
inflection point (AKA maximum efficiency ratio, AKA min_pstate in
intel_pstate.c), eta is monotonically decreasing with respect to
frequency above that point, therefore setting a lower limit to the
energy efficiency of the processor is equivalent to setting an upper
limit to its frequency within that range.
> I guess what you mean is that the processor might decide to go for a
> more energy-efficient configuration by increasing its frequency in a
> "race to idle" fashion (in response to a perceived utilization spike)
> and you want to prevent that from occurring.
>
No, a race to idle response to a utilization spike would only be more
energy efficient than the performance-equivalent constant-frequency
response in cases where the latter constant frequency is in the
concavity region of the system's power curve (below the inflection
point). I certainly don't want to prevent that from occurring when it's
the most energy-efficient thing to do.
> Or, generally speaking, that the CPU performance scaling logic, either
> in the kernel or in the processor itself, might select a higher
> operating frequency of a CPU in response to a perceived utilization
> spike, but that may be a mistake in the presence of another data
> processing device sharing the power budget with the processor, so you
> want to prevent that from taking place.
>
Yes, I do.
> In both cases, I wouldn't call that limiting the energy-efficiency of
> the processor. Effectively, this means putting a limit on the
> processor's power budget, which is exactly what RAPL is for.
>
No, limiting the processor frequency also imposes a limit to its energy
efficiency due to the reason explained above.
>> -- And if the maximum usable CPU frequency is the
>> information we have at hand,
>
> How so?
>
> How can you tell what that frequency is?
>
In the general case it would take a crystal ball to know the amount of
work the CPU is going to have to do in the future, however as soon as
the system has reached a steady state (which amounts to a large fraction
of the time and energy consumption of many workloads, therefore it's an
interesting case to optimize for) its previous behavior can be taken as
proxy for its future behavior (by definition of steady state), therefore
we can measure the performance delivered by the processor in the
immediate past and make sure that the governor's response doesn't
prevent it from achieving the same performance (plus some margin in
order to account for potential fluctuations in the workload).
That's, yes, an essentially heuristic assumption, but one that underlies
every other CPU frequency governor in the Linux kernel tree to a greater
or lower extent.
>> controlling the maximum CPU frequency
>> directly is optimal, rather than trying to find the RAPL constraint that
>> achieves the same average frequency by trial an error. Also, in theory,
>> even if you had an oracle to tell you what the appropriate RAPL
>> constraint is, the result would necessarily be more energy-inefficient
>> than controlling the maximum CPU frequency directly, since you're giving
>> the processor additional freedom to run at frequencies above the one you
>> want to average, which is guaranteed to be more energy-inefficient than
>> running at that fixed frequency, assuming we are in the region of
>> convexity of the processor's power curve.
>
> So the reason why you want to limit the processor's max frequency in
> the first place is because it is sharing the power budget with
> something else.
No, my ultimate goal is to optimize the energy efficiency of the CPU in
cases where the system has a bottleneck elsewhere.
> If there's no sharing of the power budget or thermal constraints,
> there is no need to limit the CPU frequency other than for the sake of
> saving energy.
>
Yes!
> What you can achieve by limiting the max CPU frequency is to make the
> processor draw less power (and cause it to use either less or more
> energy, depending on the energy-efficiency curve).
Yes, in order to make sure that limiting the maximum CPU frequency
doesn't lead to increased energy usage the response of the governor is
clamped to the convexity range of the CPU power curve (which yes, I'm
aware is only an approximation to the convexity range of the
whole-system power curve).
> You don't know how much less power it will draw then, however.
>
I don't see any need to care how much less power is drawn in absolute
terms, as long as the energy efficiency of the system is improved *and*
its performance is at least the same as it was before.
> You seem to be saying "I know exactly what the maximum frequency of
> the CPU can be, so why I don't set it as the upper limit", but I'm
> questioning the source of that knowledge. Does it not come from
> knowing the power budget you want to give to the processor?
>
No, it comes from CPU performance counters -- More on that above.
>> Anyway, if you still have some disagreement on the theoretical details
>> you're more than welcome to bring up the matter on the other thread [1],
>> or accept the invitation for a presentation I sent you months ago... ;)
>
> Why don't we continue the discussion here instead?
>
> I think we are getting to the bottom of things here.
Up to you.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 227 bytes --]
^ permalink raw reply
* Re: [PATCH v4 1/2] kernel: Implement selective syscall userspace redirection
From: Andy Lutomirski @ 2020-07-17 0:20 UTC (permalink / raw)
To: Gabriel Krisman Bertazi
Cc: Thomas Gleixner, LKML, kernel, Matthew Wilcox, Andrew Lutomirski,
Paul Gofman, Kees Cook, open list:KERNEL SELFTEST FRAMEWORK,
Shuah Khan
In-Reply-To: <20200716193141.4068476-2-krisman@collabora.com>
On Thu, Jul 16, 2020 at 12:31 PM Gabriel Krisman Bertazi
<krisman@collabora.com> wrote:
>
This is quite nice. I have a few comments, though:
You mentioned rt_sigreturn(). Should this automatically exempt the
kernel-provided signal restorer on architectures (e.g. x86_32) that
provide one?
The amount of syscall entry wiring that arches need to do is IMO
already a bit out of hand. Should we instead rename TIF_SECCOMP to
TIF_SYSCALL_INTERCEPTION and have one generic callback that handles
seccomp and this new thing?
> +int do_syscall_user_dispatch(struct pt_regs *regs)
> +{
> + struct syscall_user_dispatch *sd = ¤t->syscall_dispatch;
> + unsigned long ip = instruction_pointer(regs);
> + char state;
> +
> + if (likely(ip >= sd->dispatcher_start && ip <= sd->dispatcher_end))
> + return 0;
> +
> + if (likely(sd->selector)) {
> + if (unlikely(__get_user(state, sd->selector)))
> + do_exit(SIGSEGV);
> +
> + if (likely(state == 0))
> + return 0;
> +
> + if (state != 1)
> + do_exit(SIGSEGV);
This seems a bit extreme and hard to debug if it ever happens.
^ permalink raw reply
* Re: [PATCH 2/2] kvm: mmu: page_track: Fix RCU list API usage
From: Dexuan-Linux Cui @ 2020-07-17 0:19 UTC (permalink / raw)
To: Naresh Kamboju
Cc: madhuparnabhowmik10, Paul E. McKenney, Josh Triplett,
Joel Fernandes, Paolo Bonzini, rcu, open list, X86 ML, kvm list,
frextrite, lkft-triage, Dexuan Cui, juhlee
In-Reply-To: <CA+G9fYuVmTcttBpVtegwPbKxufupPOtk_WqEtOdS+HDQi7WS9Q@mail.gmail.com>
On Thu, Jul 16, 2020 at 7:47 AM Naresh Kamboju
<naresh.kamboju@linaro.org> wrote:
>
> On Sun, 12 Jul 2020 at 21:39, Paul E. McKenney <paulmck@kernel.org> wrote:
> >
> > On Sun, Jul 12, 2020 at 06:40:03PM +0530, madhuparnabhowmik10@gmail.com wrote:
> > > From: Madhuparna Bhowmik <madhuparnabhowmik10@gmail.com>
> > >
> > > Use hlist_for_each_entry_srcu() instead of hlist_for_each_entry_rcu()
> > > as it also checkes if the right lock is held.
> > > Using hlist_for_each_entry_rcu() with a condition argument will not
> > > report the cases where a SRCU protected list is traversed using
> > > rcu_read_lock(). Hence, use hlist_for_each_entry_srcu().
> > >
> > > Signed-off-by: Madhuparna Bhowmik <madhuparnabhowmik10@gmail.com>
> >
> > I queued both for testing and review, thank you!
> >
> > In particular, this one needs an ack by the maintainer.
> >
> > Thanx, Paul
> >
> > > arch/x86/kvm/mmu/page_track.c | 6 ++++--
> > > 1 file changed, 4 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/arch/x86/kvm/mmu/page_track.c b/arch/x86/kvm/mmu/page_track.c
> > > index a7bcde34d1f2..a9cd17625950 100644
> > > --- a/arch/x86/kvm/mmu/page_track.c
> > > +++ b/arch/x86/kvm/mmu/page_track.c
> > > @@ -229,7 +229,8 @@ void kvm_page_track_write(struct kvm_vcpu *vcpu, gpa_t gpa, const u8 *new,
> > > return;
> > >
> > > idx = srcu_read_lock(&head->track_srcu);
> > > - hlist_for_each_entry_rcu(n, &head->track_notifier_list, node)
> > > + hlist_for_each_entry_srcu(n, &head->track_notifier_list, node,
> > > + srcu_read_lock_held(&head->track_srcu))
>
> x86 build failed on linux -next 20200716.
>
> arch/x86/kvm/mmu/page_track.c: In function 'kvm_page_track_write':
> include/linux/rculist.h:727:30: error: left-hand operand of comma
> expression has no effect [-Werror=unused-value]
> for (__list_check_srcu(cond), \
> ^
> arch/x86/kvm/mmu/page_track.c:232:2: note: in expansion of macro
> 'hlist_for_each_entry_srcu'
> hlist_for_each_entry_srcu(n, &head->track_notifier_list, node,
> ^~~~~~~~~~~~~~~~~~~~~~~~~
> arch/x86/kvm/mmu/page_track.c: In function 'kvm_page_track_flush_slot':
> include/linux/rculist.h:727:30: error: left-hand operand of comma
> expression has no effect [-Werror=unused-value]
> for (__list_check_srcu(cond), \
> ^
> arch/x86/kvm/mmu/page_track.c:258:2: note: in expansion of macro
> 'hlist_for_each_entry_srcu'
> hlist_for_each_entry_srcu(n, &head->track_notifier_list, node,
> ^~~~~~~~~~~~~~~~~~~~~~~~~
> cc1: all warnings being treated as errors
> make[3]: *** [arch/x86/kvm/mmu/page_track.o] Error 1
>
> build link,
> https://ci.linaro.org/view/lkft/job/openembedded-lkft-linux-next/DISTRO=lkft,MACHINE=intel-corei7-64,label=docker-lkft/815/consoleText
>
> --
> Linaro LKFT
> https://lkft.linaro.org
Hi, we're seeing the same building failure with the latest linux-next tree.
Thanks,
Dexuan
^ permalink raw reply
* Re: Dubios pointer casting with put_user()
From: Richard Sailer @ 2020-07-17 0:19 UTC (permalink / raw)
To: kernelnewbies
In-Reply-To: <d8a96eaf-3cbf-a088-5dd5-ad8b64ec9749@systemli.org>
[-- Attachment #1.1.1: Type: text/plain, Size: 94 bytes --]
Link to full source:
https://elixir.bootlin.com/linux/latest/source/net/dccp/proto.c#L390
[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
[-- Attachment #2: Type: text/plain, Size: 170 bytes --]
_______________________________________________
Kernelnewbies mailing list
Kernelnewbies@kernelnewbies.org
https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies
^ permalink raw reply
* [PATCH v2 2/5] iommu/arm-smmu: Emulate bypass by using context banks
From: Bjorn Andersson @ 2020-07-17 0:16 UTC (permalink / raw)
To: Will Deacon, Robin Murphy, Joerg Roedel, Thierry Reding,
Laurentiu Tudor
Cc: Vinod Koul, Jonathan Marek, linux-arm-msm, linux-kernel, iommu,
John Stultz, linux-arm-kernel
In-Reply-To: <20200717001619.325317-1-bjorn.andersson@linaro.org>
Some firmware found on various Qualcomm platforms traps writes to S2CR
of type BYPASS and writes FAULT into the register. This prevents us from
marking the streams for the display controller as BYPASS to allow
continued scanout of the screen through the initialization of the ARM
SMMU.
This adds a Qualcomm specific cfg_probe function, which probes the
behavior of the S2CR registers and if found faulty enables the related
quirk. Based on this quirk context banks are allocated for IDENTITY
domains as well, but with ARM_SMMU_SCTLR_M omitted.
The result is valid stream mappings, without translation.
Tested-by: John Stultz <john.stultz@linaro.org>
Tested-by: Vinod Koul <vkoul@kernel.org>
Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---
Changes since v1:
- Picked up tested-by
drivers/iommu/arm-smmu-qcom.c | 21 +++++++++++++++++++++
drivers/iommu/arm-smmu.c | 14 ++++++++++++--
drivers/iommu/arm-smmu.h | 3 +++
3 files changed, 36 insertions(+), 2 deletions(-)
diff --git a/drivers/iommu/arm-smmu-qcom.c b/drivers/iommu/arm-smmu-qcom.c
index be4318044f96..d95a5ee8c83c 100644
--- a/drivers/iommu/arm-smmu-qcom.c
+++ b/drivers/iommu/arm-smmu-qcom.c
@@ -23,6 +23,26 @@ static const struct of_device_id qcom_smmu_client_of_match[] __maybe_unused = {
{ }
};
+static int qcom_smmu_cfg_probe(struct arm_smmu_device *smmu)
+{
+ unsigned int last_s2cr = ARM_SMMU_GR0_S2CR(smmu->num_mapping_groups - 1);
+ u32 reg;
+
+ /*
+ * With some firmware writes to S2CR of type FAULT are ignored, and
+ * writing BYPASS will end up as FAULT in the register. Perform a write
+ * to S2CR to detect if this is the case with the current firmware.
+ */
+ arm_smmu_gr0_write(smmu, last_s2cr, FIELD_PREP(ARM_SMMU_S2CR_TYPE, S2CR_TYPE_BYPASS) |
+ FIELD_PREP(ARM_SMMU_S2CR_CBNDX, 0xff) |
+ FIELD_PREP(ARM_SMMU_S2CR_PRIVCFG, S2CR_PRIVCFG_DEFAULT));
+ reg = arm_smmu_gr0_read(smmu, last_s2cr);
+ if (FIELD_GET(ARM_SMMU_S2CR_TYPE, reg) != S2CR_TYPE_BYPASS)
+ smmu->qcom_bypass_quirk = true;
+
+ return 0;
+}
+
static int qcom_smmu_def_domain_type(struct device *dev)
{
const struct of_device_id *match =
@@ -61,6 +81,7 @@ static int qcom_smmu500_reset(struct arm_smmu_device *smmu)
}
static const struct arm_smmu_impl qcom_smmu_impl = {
+ .cfg_probe = qcom_smmu_cfg_probe,
.def_domain_type = qcom_smmu_def_domain_type,
.reset = qcom_smmu500_reset,
};
diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index fb85e716ae9a..5d5fe6741ed4 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -654,7 +654,9 @@ static void arm_smmu_write_context_bank(struct arm_smmu_device *smmu, int idx)
/* SCTLR */
reg = ARM_SMMU_SCTLR_CFIE | ARM_SMMU_SCTLR_CFRE | ARM_SMMU_SCTLR_AFE |
- ARM_SMMU_SCTLR_TRE | ARM_SMMU_SCTLR_M;
+ ARM_SMMU_SCTLR_TRE;
+ if (cfg->m)
+ reg |= ARM_SMMU_SCTLR_M;
if (stage1)
reg |= ARM_SMMU_SCTLR_S1_ASIDPNE;
if (IS_ENABLED(CONFIG_CPU_BIG_ENDIAN))
@@ -678,7 +680,11 @@ static int arm_smmu_init_domain_context(struct iommu_domain *domain,
if (smmu_domain->smmu)
goto out_unlock;
- if (domain->type == IOMMU_DOMAIN_IDENTITY) {
+ /*
+ * Nothing to do for IDENTITY domains,unless disabled context banks are
+ * used to emulate bypass mappings on Qualcomm platforms.
+ */
+ if (domain->type == IOMMU_DOMAIN_IDENTITY && !smmu->qcom_bypass_quirk) {
smmu_domain->stage = ARM_SMMU_DOMAIN_BYPASS;
smmu_domain->smmu = smmu;
goto out_unlock;
@@ -826,6 +832,10 @@ static int arm_smmu_init_domain_context(struct iommu_domain *domain,
domain->geometry.aperture_end = (1UL << ias) - 1;
domain->geometry.force_aperture = true;
+ /* Enable translation for non-identity context banks */
+ if (domain->type != IOMMU_DOMAIN_IDENTITY)
+ cfg->m = true;
+
/* Initialise the context bank with our page table cfg */
arm_smmu_init_context_bank(smmu_domain, &pgtbl_cfg);
arm_smmu_write_context_bank(smmu, cfg->cbndx);
diff --git a/drivers/iommu/arm-smmu.h b/drivers/iommu/arm-smmu.h
index d172c024be61..a71d193073e4 100644
--- a/drivers/iommu/arm-smmu.h
+++ b/drivers/iommu/arm-smmu.h
@@ -305,6 +305,8 @@ struct arm_smmu_device {
/* IOMMU core code handle */
struct iommu_device iommu;
+
+ bool qcom_bypass_quirk;
};
enum arm_smmu_context_fmt {
@@ -323,6 +325,7 @@ struct arm_smmu_cfg {
};
enum arm_smmu_cbar_type cbar;
enum arm_smmu_context_fmt fmt;
+ bool m;
};
#define ARM_SMMU_INVALID_IRPTNDX 0xff
--
2.26.2
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related
* [PATCH v2 4/5] iommu/arm-smmu-qcom: Consistently initialize stream mappings
From: Bjorn Andersson @ 2020-07-17 0:16 UTC (permalink / raw)
To: Will Deacon, Robin Murphy, Joerg Roedel, Thierry Reding,
Laurentiu Tudor
Cc: Vinod Koul, Jonathan Marek, linux-arm-msm, linux-kernel, iommu,
John Stultz, linux-arm-kernel
In-Reply-To: <20200717001619.325317-1-bjorn.andersson@linaro.org>
Firmware that traps writes to S2CR to translate BYPASS into FAULT also
ignores writes of type FAULT. As such booting with "disable_bypass" set
will result in all S2CR registers left as configured by the bootloader.
This has been seen to result in indeterministic results, as these
mappings might linger and reference context banks that Linux is
reconfiguring.
Use the fact that BYPASS writes result in FAULT type to force all stream
mappings to FAULT.
Tested-by: John Stultz <john.stultz@linaro.org>
Tested-by: Vinod Koul <vkoul@kernel.org>
Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---
Changes since v1:
- Fixed subject spelling mistake
- Picked up tested-by
drivers/iommu/arm-smmu-qcom.c | 18 +++++++++++++++++-
1 file changed, 17 insertions(+), 1 deletion(-)
diff --git a/drivers/iommu/arm-smmu-qcom.c b/drivers/iommu/arm-smmu-qcom.c
index d95a5ee8c83c..10eb024981d1 100644
--- a/drivers/iommu/arm-smmu-qcom.c
+++ b/drivers/iommu/arm-smmu-qcom.c
@@ -27,6 +27,7 @@ static int qcom_smmu_cfg_probe(struct arm_smmu_device *smmu)
{
unsigned int last_s2cr = ARM_SMMU_GR0_S2CR(smmu->num_mapping_groups - 1);
u32 reg;
+ int i;
/*
* With some firmware writes to S2CR of type FAULT are ignored, and
@@ -37,9 +38,24 @@ static int qcom_smmu_cfg_probe(struct arm_smmu_device *smmu)
FIELD_PREP(ARM_SMMU_S2CR_CBNDX, 0xff) |
FIELD_PREP(ARM_SMMU_S2CR_PRIVCFG, S2CR_PRIVCFG_DEFAULT));
reg = arm_smmu_gr0_read(smmu, last_s2cr);
- if (FIELD_GET(ARM_SMMU_S2CR_TYPE, reg) != S2CR_TYPE_BYPASS)
+ if (FIELD_GET(ARM_SMMU_S2CR_TYPE, reg) != S2CR_TYPE_BYPASS) {
smmu->qcom_bypass_quirk = true;
+ /*
+ * With firmware ignoring writes of type FAULT, booting the
+ * Linux kernel with disable_bypass disabled (i.e. "enable
+ * bypass") the initialization during probe will leave mappings
+ * in an inconsistent state. Avoid this by configuring all
+ * S2CRs to BYPASS.
+ */
+ for (i = 0; i < smmu->num_mapping_groups; i++) {
+ smmu->s2crs[i].type = S2CR_TYPE_BYPASS;
+ smmu->s2crs[i].privcfg = S2CR_PRIVCFG_DEFAULT;
+ smmu->s2crs[i].cbndx = 0xff;
+ smmu->s2crs[i].count = 0;
+ }
+ }
+
return 0;
}
--
2.26.2
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related
* [PATCH v2 5/5] iommu/arm-smmu: Setup identity domain for boot mappings
From: Bjorn Andersson @ 2020-07-17 0:16 UTC (permalink / raw)
To: Will Deacon, Robin Murphy, Joerg Roedel, Thierry Reding,
Laurentiu Tudor
Cc: Vinod Koul, Jonathan Marek, linux-arm-msm, linux-kernel, iommu,
John Stultz, linux-arm-kernel
In-Reply-To: <20200717001619.325317-1-bjorn.andersson@linaro.org>
With many Qualcomm platforms not having functional S2CR BYPASS a
temporary IOMMU domain, without translation, needs to be allocated in
order to allow these memory transactions.
Unfortunately the boot loader uses the first few context banks, so
rather than overwriting a active bank the last context bank is used and
streams are diverted here during initialization.
This also performs the readback of SMR registers for the Qualcomm
platform, to trigger the mechanism.
This is based on prior work by Thierry Reding and Laurentiu Tudor.
Tested-by: John Stultz <john.stultz@linaro.org>
Tested-by: Vinod Koul <vkoul@kernel.org>
Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---
Changes since v1:
- Rebased to avoid conflict
- Picked up tested-by
drivers/iommu/arm-smmu-qcom.c | 11 +++++
drivers/iommu/arm-smmu.c | 79 +++++++++++++++++++++++++++++++++--
drivers/iommu/arm-smmu.h | 3 ++
3 files changed, 89 insertions(+), 4 deletions(-)
diff --git a/drivers/iommu/arm-smmu-qcom.c b/drivers/iommu/arm-smmu-qcom.c
index 10eb024981d1..147af11049eb 100644
--- a/drivers/iommu/arm-smmu-qcom.c
+++ b/drivers/iommu/arm-smmu-qcom.c
@@ -26,6 +26,7 @@ static const struct of_device_id qcom_smmu_client_of_match[] __maybe_unused = {
static int qcom_smmu_cfg_probe(struct arm_smmu_device *smmu)
{
unsigned int last_s2cr = ARM_SMMU_GR0_S2CR(smmu->num_mapping_groups - 1);
+ u32 smr;
u32 reg;
int i;
@@ -56,6 +57,16 @@ static int qcom_smmu_cfg_probe(struct arm_smmu_device *smmu)
}
}
+ for (i = 0; i < smmu->num_mapping_groups; i++) {
+ smr = arm_smmu_gr0_read(smmu, ARM_SMMU_GR0_SMR(i));
+
+ if (FIELD_GET(ARM_SMMU_SMR_VALID, smr)) {
+ smmu->smrs[i].id = FIELD_GET(ARM_SMMU_SMR_ID, smr);
+ smmu->smrs[i].mask = FIELD_GET(ARM_SMMU_SMR_MASK, smr);
+ smmu->smrs[i].valid = true;
+ }
+ }
+
return 0;
}
diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index 08a650fe02e3..69bd8ee03516 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -652,7 +652,8 @@ static void arm_smmu_write_context_bank(struct arm_smmu_device *smmu, int idx)
}
static int arm_smmu_init_domain_context(struct iommu_domain *domain,
- struct arm_smmu_device *smmu)
+ struct arm_smmu_device *smmu,
+ bool boot_domain)
{
int irq, start, ret = 0;
unsigned long ias, oas;
@@ -770,6 +771,15 @@ static int arm_smmu_init_domain_context(struct iommu_domain *domain,
ret = -EINVAL;
goto out_unlock;
}
+
+ /*
+ * Use the last context bank for identity mappings during boot, to
+ * avoid overwriting in-use bank configuration while we're setting up
+ * the new mappings.
+ */
+ if (boot_domain)
+ start = smmu->num_context_banks - 1;
+
ret = __arm_smmu_alloc_bitmap(smmu->context_map, start,
smmu->num_context_banks);
if (ret < 0)
@@ -1149,7 +1159,10 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
struct arm_smmu_master_cfg *cfg;
struct arm_smmu_device *smmu;
+ bool free_identity_domain = false;
+ int idx;
int ret;
+ int i;
if (!fwspec || fwspec->ops != &arm_smmu_ops) {
dev_err(dev, "cannot attach to SMMU, is it on the same bus?\n");
@@ -1174,7 +1187,7 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
return ret;
/* Ensure that the domain is finalised */
- ret = arm_smmu_init_domain_context(domain, smmu);
+ ret = arm_smmu_init_domain_context(domain, smmu, false);
if (ret < 0)
goto rpm_put;
@@ -1190,9 +1203,33 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
goto rpm_put;
}
+ /* Decrement use counter for any references to the identity domain */
+ mutex_lock(&smmu->stream_map_mutex);
+ if (smmu->identity) {
+ struct arm_smmu_domain *identity = to_smmu_domain(smmu->identity);
+
+ for_each_cfg_sme(cfg, fwspec, i, idx) {
+ if (smmu->s2crs[idx].cbndx == identity->cfg.cbndx) {
+ smmu->num_identity_masters--;
+ if (smmu->num_identity_masters == 0)
+ free_identity_domain = true;
+ }
+ }
+ }
+ mutex_unlock(&smmu->stream_map_mutex);
+
/* Looks ok, so add the device to the domain */
ret = arm_smmu_domain_add_master(smmu_domain, cfg, fwspec);
+ /*
+ * The last stream map to reference the identity domain has been
+ * overwritten, so it's now okay to free it.
+ */
+ if (free_identity_domain) {
+ arm_smmu_domain_free(smmu->identity);
+ smmu->identity = NULL;
+ }
+
/*
* Setup an autosuspend delay to avoid bouncing runpm state.
* Otherwise, if a driver for a suspended consumer device
@@ -1922,17 +1959,51 @@ static int arm_smmu_device_cfg_probe(struct arm_smmu_device *smmu)
static int arm_smmu_setup_identity(struct arm_smmu_device *smmu)
{
+ struct device *dev = smmu->dev;
+ int cbndx = 0xff;
+ int type = S2CR_TYPE_BYPASS;
+ int ret;
int i;
+ if (smmu->qcom_bypass_quirk) {
+ /* Create a IDENTITY domain to use for all inherited streams */
+ smmu->identity = arm_smmu_domain_alloc(IOMMU_DOMAIN_IDENTITY);
+ if (!smmu->identity) {
+ dev_err(dev, "failed to create identity domain\n");
+ return -ENOMEM;
+ }
+
+ smmu->identity->pgsize_bitmap = smmu->pgsize_bitmap;
+ smmu->identity->type = IOMMU_DOMAIN_IDENTITY;
+ smmu->identity->ops = &arm_smmu_ops;
+
+ ret = arm_smmu_init_domain_context(smmu->identity, smmu, true);
+ if (ret < 0) {
+ dev_err(dev, "failed to initialize identity domain: %d\n", ret);
+ return ret;
+ }
+
+ type = S2CR_TYPE_TRANS;
+ cbndx = to_smmu_domain(smmu->identity)->cfg.cbndx;
+ }
+
for (i = 0; i < smmu->num_mapping_groups; i++) {
if (smmu->smrs[i].valid) {
- smmu->s2crs[i].type = S2CR_TYPE_BYPASS;
+ smmu->s2crs[i].type = type;
smmu->s2crs[i].privcfg = S2CR_PRIVCFG_DEFAULT;
- smmu->s2crs[i].cbndx = 0xff;
+ smmu->s2crs[i].cbndx = cbndx;
smmu->s2crs[i].count++;
+
+ smmu->num_identity_masters++;
}
}
+ /* If no mappings where found, free the identiy domain again */
+ if (smmu->identity && !smmu->num_identity_masters) {
+ arm_smmu_domain_free(smmu->identity);
+ smmu->identity = NULL;
+ }
+
return 0;
}
diff --git a/drivers/iommu/arm-smmu.h b/drivers/iommu/arm-smmu.h
index bcd160d01c53..37257ede86fa 100644
--- a/drivers/iommu/arm-smmu.h
+++ b/drivers/iommu/arm-smmu.h
@@ -321,6 +321,9 @@ struct arm_smmu_device {
/* IOMMU core code handle */
struct iommu_device iommu;
+ struct iommu_domain *identity;
+ unsigned int num_identity_masters;
+
bool qcom_bypass_quirk;
};
--
2.26.2
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.