* Re: [PATCH v5 03/11] powerpc/pseries/iommu: Add iommu_pseries_alloc_table() helper
From: Leonardo Brás @ 2021-07-19 18:47 UTC (permalink / raw)
To: Frederic Barrat, Michael Ellerman, Benjamin Herrenschmidt,
Paul Mackerras, Alexey Kardashevskiy, David Gibson,
kernel test robot, Nicolin Chen
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <8110756f-a41f-3ba1-d7bc-af15630c4fbc@linux.ibm.com>
On Mon, 2021-07-19 at 16:04 +0200, Frederic Barrat wrote:
>
>
> On 16/07/2021 10:27, Leonardo Bras wrote:
> > Creates a helper to allow allocating a new iommu_table without the
> > need
> > to reallocate the iommu_group.
> >
> > This will be helpful for replacing the iommu_table for the new DMA
> > window,
> > after we remove the old one with iommu_tce_table_put().
> >
> > Signed-off-by: Leonardo Bras <leobras.c@gmail.com>
> > Reviewed-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> > ---
> > arch/powerpc/platforms/pseries/iommu.c | 25 ++++++++++++++-------
> > ----
> > 1 file changed, 14 insertions(+), 11 deletions(-)
> >
> > diff --git a/arch/powerpc/platforms/pseries/iommu.c
> > b/arch/powerpc/platforms/pseries/iommu.c
> > index b1b8d12bab39..33d82865d6e6 100644
> > --- a/arch/powerpc/platforms/pseries/iommu.c
> > +++ b/arch/powerpc/platforms/pseries/iommu.c
> > @@ -53,28 +53,31 @@ enum {
> > DDW_EXT_QUERY_OUT_SIZE = 2
> > };
> >
> > -static struct iommu_table_group *iommu_pseries_alloc_group(int
> > node)
> > +static struct iommu_table *iommu_pseries_alloc_table(int node)
> > {
> > - struct iommu_table_group *table_group;
> > struct iommu_table *tbl;
> >
> > - table_group = kzalloc_node(sizeof(struct
> > iommu_table_group), GFP_KERNEL,
> > - node);
> > - if (!table_group)
> > - return NULL;
> > -
> > tbl = kzalloc_node(sizeof(struct iommu_table), GFP_KERNEL,
> > node);
> > if (!tbl)
> > - goto free_group;
> > + return NULL;
> >
> > INIT_LIST_HEAD_RCU(&tbl->it_group_list);
> > kref_init(&tbl->it_kref);
> > + return tbl;
> > +}
> >
> > - table_group->tables[0] = tbl;
> > +static struct iommu_table_group *iommu_pseries_alloc_group(int
> > node)
> > +{
> > + struct iommu_table_group *table_group;
> > +
> > + table_group = kzalloc_node(sizeof(*table_group),
> > GFP_KERNEL, node);
> > + if (!table_group)
> > + return NULL;
> >
> > - return table_group;
> > + table_group->tables[0] = iommu_pseries_alloc_table(node);
> > + if (table_group->tables[0])
> > + return table_group;
>
>
> Nitpick: for readability, we'd usually expect the error path to be
> detected with the if statement and keep going on the good path, and
> here
> the code does the opposite. No big deal though, so
>
> Reviewed-by: Frederic Barrat <fbarrat@linux.ibm.com>
>
>
Thanks for the tip and review!
Best regards,
Leonardo Bras
> >
> > -free_group:
> > kfree(table_group);
> > return NULL;
> > }
> >
^ permalink raw reply
* Re: [PATCH v5 01/11] powerpc/pseries/iommu: Replace hard-coded page shift
From: Leonardo Brás @ 2021-07-19 18:43 UTC (permalink / raw)
To: Frederic Barrat, Michael Ellerman, Benjamin Herrenschmidt,
Paul Mackerras, Alexey Kardashevskiy, David Gibson,
kernel test robot, Nicolin Chen
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <d96c25e1-41da-881b-da35-87ffb28ce335@linux.ibm.com>
On Mon, 2021-07-19 at 15:48 +0200, Frederic Barrat wrote:
>
>
> On 16/07/2021 10:27, Leonardo Bras wrote:
> > Some functions assume IOMMU page size can only be 4K (pageshift ==
> > 12).
> > Update them to accept any page size passed, so we can use 64K
> > pages.
> >
> > In the process, some defines like TCE_SHIFT were made obsolete, and
> > then
> > removed.
> >
> > IODA3 Revision 3.0_prd1 (OpenPowerFoundation), Figures 3.4 and 3.5
> > show
> > a RPN of 52-bit, and considers a 12-bit pageshift, so there should
> > be
> > no need of using TCE_RPN_MASK, which masks out any bit after 40 in
> > rpn.
> > It's usage removed from tce_build_pSeries(), tce_build_pSeriesLP(),
> > and
> > tce_buildmulti_pSeriesLP().
> >
> > Most places had a tbl struct, so using tbl->it_page_shift was
> > simple.
> > tce_free_pSeriesLP() was a special case, since callers not always
> > have a
> > tbl struct, so adding a tceshift parameter seems the right thing to
> > do.
> >
> > Signed-off-by: Leonardo Bras <leobras.c@gmail.com>
> > Reviewed-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> > ---
>
> FWIW,
> Reviewed-by: Frederic Barrat <fbarrat@linux.ibm.com>
>
Thanks!
^ permalink raw reply
* Re: [PATCH] replace if with min
From: Segher Boessenkool @ 2021-07-19 17:04 UTC (permalink / raw)
To: Christophe Leroy
Cc: herbert, Salah Triki, linux-kernel, paulus, linux-crypto,
linuxppc-dev, davem
In-Reply-To: <20210719181205.Horde.xU8C00MIRgjqhZQ3-RrANw8@messagerie.c-s.fr>
On Mon, Jul 19, 2021 at 06:12:05PM +0200, Christophe Leroy wrote:
> Salah Triki <salah.triki@gmail.com> a écrit :
> >Replace if with min in order to make code more clean.
> >--- a/drivers/crypto/nx/nx-842.c
> >+++ b/drivers/crypto/nx/nx-842.c
> >@@ -134,8 +134,7 @@ EXPORT_SYMBOL_GPL(nx842_crypto_exit);
> > static void check_constraints(struct nx842_constraints *c)
> > {
> > /* limit maximum, to always have enough bounce buffer to decompress
> > */
> >- if (c->maximum > BOUNCE_BUFFER_SIZE)
> >- c->maximum = BOUNCE_BUFFER_SIZE;
> >+ c->maximum = min(c->maximum, BOUNCE_BUFFER_SIZE);
>
> For me the code is less clear with this change, and in addition it
> slightly changes the behaviour. Before, the write was done only when
> the value was changing. Now you rewrite the value always, even when it
> doesn't change.
In both cases the compiler can decide to either write it more often than
strictly needed, depending on what it thinks best (and it usually has
better estimates than the programmer). The behaviour is identical (and
the generated machine code is as well, in my testing).
The field name "maximum" is not the best choice, which makes the code
read a bit funny ("the min of max"), but the comment makes things pretty
clear.
Segher
^ permalink raw reply
* [PATCH 5.12 286/292] perf script python: Fix buffer size to report iregs in perf script
From: Greg Kroah-Hartman @ 2021-07-19 14:55 UTC (permalink / raw)
To: linux-kernel
Cc: Sasha Levin, Ravi Bangoria, Athira Jajeev, Nageswara R Sastry,
Greg Kroah-Hartman, linuxppc-dev, stable,
Arnaldo Carvalho de Melo, Madhavan Srinivasan, Paul Clarke,
Kajol Jain, Jiri Olsa
In-Reply-To: <20210719144942.514164272@linuxfoundation.org>
From: Kajol Jain <kjain@linux.ibm.com>
[ Upstream commit dea8cfcc33695f70f56023b416cf88ae44c8a45a ]
Commit 48a1f565261d2ab1 ("perf script python: Add more PMU fields to
event handler dict") added functionality to report fields like weight,
iregs, uregs etc via perf report. That commit predefined buffer size to
512 bytes to print those fields.
But in PowerPC, since we added extended regs support in:
068aeea3773a6f4c ("perf powerpc: Support exposing Performance Monitor Counter SPRs as part of extended regs")
d735599a069f6936 ("powerpc/perf: Add extended regs support for power10 platform")
Now iregs can carry more bytes of data and this predefined buffer size
can result to data loss in perf script output.
This patch resolves this issue by making the buffer size dynamic, based
on the number of registers needed to print. It also changes the
regs_map() return type from int to void, as it is not being used by the
set_regs_in_dict(), its only caller.
Fixes: 068aeea3773a6f4c ("perf powerpc: Support exposing Performance Monitor Counter SPRs as part of extended regs")
Signed-off-by: Kajol Jain <kjain@linux.ibm.com>
Tested-by: Nageswara R Sastry <rnsastry@linux.ibm.com>
Cc: Athira Jajeev <atrajeev@linux.vnet.ibm.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Madhavan Srinivasan <maddy@linux.vnet.ibm.com>
Cc: Paul Clarke <pc@us.ibm.com>
Cc: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
Cc: linuxppc-dev@lists.ozlabs.org
Link: http://lore.kernel.org/lkml/20210628062341.155839-1-kjain@linux.ibm.com
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
.../util/scripting-engines/trace-event-python.c | 17 ++++++++++++-----
1 file changed, 12 insertions(+), 5 deletions(-)
diff --git a/tools/perf/util/scripting-engines/trace-event-python.c b/tools/perf/util/scripting-engines/trace-event-python.c
index 23dc5014e711..a61be9c07565 100644
--- a/tools/perf/util/scripting-engines/trace-event-python.c
+++ b/tools/perf/util/scripting-engines/trace-event-python.c
@@ -687,7 +687,7 @@ static void set_sample_datasrc_in_dict(PyObject *dict,
_PyUnicode_FromString(decode));
}
-static int regs_map(struct regs_dump *regs, uint64_t mask, char *bf, int size)
+static void regs_map(struct regs_dump *regs, uint64_t mask, char *bf, int size)
{
unsigned int i = 0, r;
int printed = 0;
@@ -695,7 +695,7 @@ static int regs_map(struct regs_dump *regs, uint64_t mask, char *bf, int size)
bf[0] = 0;
if (!regs || !regs->regs)
- return 0;
+ return;
for_each_set_bit(r, (unsigned long *) &mask, sizeof(mask) * 8) {
u64 val = regs->regs[i++];
@@ -704,8 +704,6 @@ static int regs_map(struct regs_dump *regs, uint64_t mask, char *bf, int size)
"%5s:0x%" PRIx64 " ",
perf_reg_name(r), val);
}
-
- return printed;
}
static void set_regs_in_dict(PyObject *dict,
@@ -713,7 +711,16 @@ static void set_regs_in_dict(PyObject *dict,
struct evsel *evsel)
{
struct perf_event_attr *attr = &evsel->core.attr;
- char bf[512];
+
+ /*
+ * Here value 28 is a constant size which can be used to print
+ * one register value and its corresponds to:
+ * 16 chars is to specify 64 bit register in hexadecimal.
+ * 2 chars is for appending "0x" to the hexadecimal value and
+ * 10 chars is for register name.
+ */
+ int size = __sw_hweight64(attr->sample_regs_intr) * 28;
+ char bf[size];
regs_map(&sample->intr_regs, attr->sample_regs_intr, bf, sizeof(bf));
--
2.30.2
^ permalink raw reply related
* [PATCH 5.13 345/351] perf script python: Fix buffer size to report iregs in perf script
From: Greg Kroah-Hartman @ 2021-07-19 14:54 UTC (permalink / raw)
To: linux-kernel
Cc: Sasha Levin, Ravi Bangoria, Athira Jajeev, Nageswara R Sastry,
Greg Kroah-Hartman, linuxppc-dev, stable,
Arnaldo Carvalho de Melo, Madhavan Srinivasan, Paul Clarke,
Kajol Jain, Jiri Olsa
In-Reply-To: <20210719144944.537151528@linuxfoundation.org>
From: Kajol Jain <kjain@linux.ibm.com>
[ Upstream commit dea8cfcc33695f70f56023b416cf88ae44c8a45a ]
Commit 48a1f565261d2ab1 ("perf script python: Add more PMU fields to
event handler dict") added functionality to report fields like weight,
iregs, uregs etc via perf report. That commit predefined buffer size to
512 bytes to print those fields.
But in PowerPC, since we added extended regs support in:
068aeea3773a6f4c ("perf powerpc: Support exposing Performance Monitor Counter SPRs as part of extended regs")
d735599a069f6936 ("powerpc/perf: Add extended regs support for power10 platform")
Now iregs can carry more bytes of data and this predefined buffer size
can result to data loss in perf script output.
This patch resolves this issue by making the buffer size dynamic, based
on the number of registers needed to print. It also changes the
regs_map() return type from int to void, as it is not being used by the
set_regs_in_dict(), its only caller.
Fixes: 068aeea3773a6f4c ("perf powerpc: Support exposing Performance Monitor Counter SPRs as part of extended regs")
Signed-off-by: Kajol Jain <kjain@linux.ibm.com>
Tested-by: Nageswara R Sastry <rnsastry@linux.ibm.com>
Cc: Athira Jajeev <atrajeev@linux.vnet.ibm.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Madhavan Srinivasan <maddy@linux.vnet.ibm.com>
Cc: Paul Clarke <pc@us.ibm.com>
Cc: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
Cc: linuxppc-dev@lists.ozlabs.org
Link: http://lore.kernel.org/lkml/20210628062341.155839-1-kjain@linux.ibm.com
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
.../util/scripting-engines/trace-event-python.c | 17 ++++++++++++-----
1 file changed, 12 insertions(+), 5 deletions(-)
diff --git a/tools/perf/util/scripting-engines/trace-event-python.c b/tools/perf/util/scripting-engines/trace-event-python.c
index 3dfc543327af..18dbd9cddda8 100644
--- a/tools/perf/util/scripting-engines/trace-event-python.c
+++ b/tools/perf/util/scripting-engines/trace-event-python.c
@@ -687,7 +687,7 @@ static void set_sample_datasrc_in_dict(PyObject *dict,
_PyUnicode_FromString(decode));
}
-static int regs_map(struct regs_dump *regs, uint64_t mask, char *bf, int size)
+static void regs_map(struct regs_dump *regs, uint64_t mask, char *bf, int size)
{
unsigned int i = 0, r;
int printed = 0;
@@ -695,7 +695,7 @@ static int regs_map(struct regs_dump *regs, uint64_t mask, char *bf, int size)
bf[0] = 0;
if (!regs || !regs->regs)
- return 0;
+ return;
for_each_set_bit(r, (unsigned long *) &mask, sizeof(mask) * 8) {
u64 val = regs->regs[i++];
@@ -704,8 +704,6 @@ static int regs_map(struct regs_dump *regs, uint64_t mask, char *bf, int size)
"%5s:0x%" PRIx64 " ",
perf_reg_name(r), val);
}
-
- return printed;
}
static void set_regs_in_dict(PyObject *dict,
@@ -713,7 +711,16 @@ static void set_regs_in_dict(PyObject *dict,
struct evsel *evsel)
{
struct perf_event_attr *attr = &evsel->core.attr;
- char bf[512];
+
+ /*
+ * Here value 28 is a constant size which can be used to print
+ * one register value and its corresponds to:
+ * 16 chars is to specify 64 bit register in hexadecimal.
+ * 2 chars is for appending "0x" to the hexadecimal value and
+ * 10 chars is for register name.
+ */
+ int size = __sw_hweight64(attr->sample_regs_intr) * 28;
+ char bf[size];
regs_map(&sample->intr_regs, attr->sample_regs_intr, bf, sizeof(bf));
--
2.30.2
^ permalink raw reply related
* Re: [PATCH] replace if with min
From: Christophe Leroy @ 2021-07-19 16:12 UTC (permalink / raw)
To: Salah Triki
Cc: herbert, linux-kernel, paulus, linux-crypto, linuxppc-dev, davem
In-Reply-To: <20210712204546.GA1492390@pc>
Salah Triki <salah.triki@gmail.com> a écrit :
> Replace if with min in order to make code more clean.
>
> Signed-off-by: Salah Triki <salah.triki@gmail.com>
> ---
> drivers/crypto/nx/nx-842.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/crypto/nx/nx-842.c b/drivers/crypto/nx/nx-842.c
> index 2ab90ec10e61..0d1d5a463899 100644
> --- a/drivers/crypto/nx/nx-842.c
> +++ b/drivers/crypto/nx/nx-842.c
> @@ -134,8 +134,7 @@ EXPORT_SYMBOL_GPL(nx842_crypto_exit);
> static void check_constraints(struct nx842_constraints *c)
> {
> /* limit maximum, to always have enough bounce buffer to decompress */
> - if (c->maximum > BOUNCE_BUFFER_SIZE)
> - c->maximum = BOUNCE_BUFFER_SIZE;
> + c->maximum = min(c->maximum, BOUNCE_BUFFER_SIZE);
For me the code is less clear with this change, and in addition it
slightly changes the behaviour. Before, the write was done only when
the value was changing. Now you rewrite the value always, even when it
doesn't change.
> }
>
> static int nx842_crypto_add_header(struct nx842_crypto_header *hdr, u8 *buf)
> --
> 2.25.1
^ permalink raw reply
* Re: FAILED: patch "[PATCH] drm/radeon/ni_dpm: Fix booting bug" failed to apply to 5.13-stable tree
From: Christian Zigotzky @ 2021-07-19 15:06 UTC (permalink / raw)
To: Stan Johnson
Cc: Alex Deucher, Debian PowerPC, linuxppc-dev, gustavoars, stable
In-Reply-To: <b7e99e02-c2ed-1d11-4f7d-5b0bc9ac8043@xenosoft.de>
On 19 July 2021 04:58 pm, Christian Zigotzky wrote:
> On 19 July 2021 at 04:32 pm, Stan Johnson wrote:
>> On 7/18/21 10:23 PM, Christian Zigotzky wrote:
>>> Hello Stan,
>>>
>>> We had the same issue during the 5.14 merge window. Please look in the
>>> following thread:
>>>
>>> https://forum.hyperion-entertainment.com/viewtopic.php?p=53511#p53511
>>>
>>> There is a patch available. Please try it.
>>>
>>> Thanks,
>>> Christian
>>> ...
>> Hello Christian,
>>
>> Thanks. There were some errors applying the patch, so it wasn't fully
>> applied (see below). Of course, I'm using 5.13.2, not 5.14, so maybe
>> that's expected.
>>
>> The patched 5.13.2 kernel still results in a blank screen while trying
>> to run wdm. On this attempt, wdm has died (oddly the screen remains
>> blank; it should display a text login after X dies). The Xorg.0.log
>> looks reasonable enough.
>>
>> I tried disabling wdm, then rebooted, logged in at the console and ran
>> "startx". The screen goes blank, X is running, startx is running:
>>
>> johnson 1392 0.0 0.2 2572 1452 tty1 S+ 08:06 0:00 /bin/sh
>> /usr/bin/startx
>> johnson 1414 0.0 0.4 4904 2096 tty1 S+ 08:06 0:00 xinit
>> /etc/X11/xinit/xinitrc -- /etc/X11/xinit/xserverrc :0 vt1 -keeptty -auth
>> /tmp/serverauth.dJ7lSnzjjo
>> johnson 1415 1.0 8.2 128436 41924 tty1 Sl 08:06 0:04
>> /usr/lib/xorg/Xorg -nolisten tcp :0 vt1 -keeptty -auth
>> /tmp/serverauth.dJ7lSnzjjo
>>
>> I had to use "kill -KILL" to kill the startx, xinit and Xorg processes.
>> After those were killed, the screen was still blank, and even though
>> nothing was running, the load average was still around 1.00 several
>> minutes later, so something is still taking CPU time:
>>
>> $ uptime
>> 08:25:15 up 20 min, 2 users, load average: 1.00, 1.00, 0.84
>>
>> I can attempt a git bisect, though that will take some time.
>>
>> -Stan
>>
>> ----------
>> $ patch -p1
>> <../v3-drm-radeon-Fix-NULL-dereference-when-updating-memory-stats.patch
>> patching file drivers/gpu/drm/radeon/radeon_object.c
>> Hunk #2 FAILED at 76.
>> Hunk #3 FAILED at 727.
>> 2 out of 3 hunks FAILED -- saving rejects to file
>> drivers/gpu/drm/radeon/radeon_object.c.rej
>> patching file drivers/gpu/drm/radeon/radeon_object.h
>> patching file drivers/gpu/drm/radeon/radeon_ttm.c
>> Hunk #1 FAILED at 199.
>> Hunk #2 succeeded at 227 (offset 11 lines).
>> Hunk #3 succeeded at 275 (offset 11 lines).
>> Hunk #4 succeeded at 697 (offset 12 lines).
>> 1 out of 4 hunks FAILED -- saving rejects to file
>> drivers/gpu/drm/radeon/radeon_ttm.c.rej
>> johnson@mac-server:/data/software/working/linux-5.13.2$ cat
>> drivers/gpu/drm/radeon/radeon_ttm.c.rej
>> --- drivers/gpu/drm/radeon/radeon_ttm.c
>> +++ drivers/gpu/drm/radeon/radeon_ttm.c
>> @@ -199,7 +199,7 @@ static int radeon_bo_move(struct ttm_buffer_object
>> *bo, bool evict,
>> struct ttm_resource *old_mem = bo->resource;
>> struct radeon_device *rdev;
>> struct radeon_bo *rbo;
>> - int r;
>> + int r, old_type;
>>
>> if (new_mem->mem_type == TTM_PL_TT) {
>> r = radeon_ttm_tt_bind(bo->bdev, bo->ttm, new_mem);
>>
>> ---------
> Hello Stan,
>
> Greg has the same issue with patching the kernel 5.13 [1]. We have to
> wait for a solution.
>
> - Christian
Stan,
Forget it. It's another issue below.
Sorry,
Christian
>
> [1]
>
> The patch below does not apply to the 5.13-stable tree.
> If someone wants it applied there, or to any other stable or longterm
> tree, then please email the backport, including the original git commit
> id to <stable@vger.kernel.org>.
>
> thanks,
>
> greg k-h
>
> ------------------ original commit in Linus's tree ------------------
>
> >From 293774413a3f519c826d4eb5313ef02e20515700 Mon Sep 17 00:00:00 2001
> From: "Gustavo A. R. Silva" <gustavoars@kernel.org>
> Date: Sun, 9 May 2021 17:49:26 -0500
> Subject: [PATCH] drm/radeon/ni_dpm: Fix booting bug
>
> Create new structure NISLANDS_SMC_SWSTATE_SINGLE, as initialState.levels
> and ACPIState.levels are never actually used as flexible arrays. Those
> arrays can be used as simple objects of type
> NISLANDS_SMC_HW_PERFORMANCE_LEVEL, instead.
>
> Currently, the code fails because flexible array _levels_ in
> struct NISLANDS_SMC_SWSTATE doesn't allow for code that access
> the first element of initialState.levels and ACPIState.levels
> arrays:
>
> drivers/gpu/drm/radeon/ni_dpm.c:
> 1690 table->initialState.levels[0].mclk.vMPLL_AD_FUNC_CNTL =
> 1691 cpu_to_be32(ni_pi->clock_registers.mpll_ad_func_cntl);
> ...
> 1903: table->ACPIState.levels[0].mclk.vMPLL_AD_FUNC_CNTL =
> cpu_to_be32(mpll_ad_func_cntl);
> 1904: table->ACPIState.levels[0].mclk.vMPLL_AD_FUNC_CNTL_2 =
> cpu_to_be32(mpll_ad_func_cntl_2);
>
> because such element cannot exist without previously allocating
> any dynamic memory for it (which never actually happens).
>
> That's why struct NISLANDS_SMC_SWSTATE should only be used as type
> for object driverState and new struct SISLANDS_SMC_SWSTATE_SINGLE is
> created as type for objects initialState, ACPIState and ULVState.
>
> Also, with the change from one-element array to flexible-array member
> in commit 434fb1e7444a ("drm/radeon/nislands_smc.h: Replace one-element
> array with flexible-array member in struct NISLANDS_SMC_SWSTATE"), the
> size of dpmLevels in struct NISLANDS_SMC_STATETABLE should be fixed to
> be NISLANDS_MAX_SMC_PERFORMANCE_LEVELS_PER_SWSTATE instead of
> NISLANDS_MAX_SMC_PERFORMANCE_LEVELS_PER_SWSTATE - 1.
>
> Bug:
> https://lore.kernel.org/dri-devel/3eedbe78-1fbd-4763-a7f3-ac5665e76a4a@xenosoft.de/
> Fixes: 434fb1e7444a ("drm/radeon/nislands_smc.h: Replace one-element
> array with flexible-array member in struct NISLANDS_SMC_SWSTATE")
> Cc: stable@vger.kernel.org
> Reported-by: Christian Zigotzky <chzigotzky@xenosoft.de>
> Tested-by: Christian Zigotzky <chzigotzky@xenosoft.de>
> Link:
> https://lore.kernel.org/dri-devel/9bb5fcbd-daf5-1669-b3e7-b8624b3c36f9@xenosoft.de/
> Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
> Signed-off-by: Alex Deucher <alexander.deucher@amd.com>
>
> diff --git a/drivers/gpu/drm/radeon/ni_dpm.c
> b/drivers/gpu/drm/radeon/ni_dpm.c
> index dd5ef6493723..769f666335ac 100644
> --- a/drivers/gpu/drm/radeon/ni_dpm.c
> +++ b/drivers/gpu/drm/radeon/ni_dpm.c
> @@ -1687,102 +1687,102 @@ static int
> ni_populate_smc_initial_state(struct radeon_device *rdev,
> u32 reg;
> int ret;
>
> - table->initialState.levels[0].mclk.vMPLL_AD_FUNC_CNTL =
> + table->initialState.level.mclk.vMPLL_AD_FUNC_CNTL =
> cpu_to_be32(ni_pi->clock_registers.mpll_ad_func_cntl);
> - table->initialState.levels[0].mclk.vMPLL_AD_FUNC_CNTL_2 =
> + table->initialState.level.mclk.vMPLL_AD_FUNC_CNTL_2 =
> cpu_to_be32(ni_pi->clock_registers.mpll_ad_func_cntl_2);
> - table->initialState.levels[0].mclk.vMPLL_DQ_FUNC_CNTL =
> + table->initialState.level.mclk.vMPLL_DQ_FUNC_CNTL =
> cpu_to_be32(ni_pi->clock_registers.mpll_dq_func_cntl);
> - table->initialState.levels[0].mclk.vMPLL_DQ_FUNC_CNTL_2 =
> + table->initialState.level.mclk.vMPLL_DQ_FUNC_CNTL_2 =
> cpu_to_be32(ni_pi->clock_registers.mpll_dq_func_cntl_2);
> - table->initialState.levels[0].mclk.vMCLK_PWRMGT_CNTL =
> + table->initialState.level.mclk.vMCLK_PWRMGT_CNTL =
> cpu_to_be32(ni_pi->clock_registers.mclk_pwrmgt_cntl);
> - table->initialState.levels[0].mclk.vDLL_CNTL =
> + table->initialState.level.mclk.vDLL_CNTL =
> cpu_to_be32(ni_pi->clock_registers.dll_cntl);
> - table->initialState.levels[0].mclk.vMPLL_SS =
> + table->initialState.level.mclk.vMPLL_SS =
> cpu_to_be32(ni_pi->clock_registers.mpll_ss1);
> - table->initialState.levels[0].mclk.vMPLL_SS2 =
> + table->initialState.level.mclk.vMPLL_SS2 =
> cpu_to_be32(ni_pi->clock_registers.mpll_ss2);
> - table->initialState.levels[0].mclk.mclk_value =
> + table->initialState.level.mclk.mclk_value =
> cpu_to_be32(initial_state->performance_levels[0].mclk);
>
> - table->initialState.levels[0].sclk.vCG_SPLL_FUNC_CNTL =
> + table->initialState.level.sclk.vCG_SPLL_FUNC_CNTL =
> cpu_to_be32(ni_pi->clock_registers.cg_spll_func_cntl);
> - table->initialState.levels[0].sclk.vCG_SPLL_FUNC_CNTL_2 =
> + table->initialState.level.sclk.vCG_SPLL_FUNC_CNTL_2 =
> cpu_to_be32(ni_pi->clock_registers.cg_spll_func_cntl_2);
> - table->initialState.levels[0].sclk.vCG_SPLL_FUNC_CNTL_3 =
> + table->initialState.level.sclk.vCG_SPLL_FUNC_CNTL_3 =
> cpu_to_be32(ni_pi->clock_registers.cg_spll_func_cntl_3);
> - table->initialState.levels[0].sclk.vCG_SPLL_FUNC_CNTL_4 =
> + table->initialState.level.sclk.vCG_SPLL_FUNC_CNTL_4 =
> cpu_to_be32(ni_pi->clock_registers.cg_spll_func_cntl_4);
> - table->initialState.levels[0].sclk.vCG_SPLL_SPREAD_SPECTRUM =
> + table->initialState.level.sclk.vCG_SPLL_SPREAD_SPECTRUM =
> cpu_to_be32(ni_pi->clock_registers.cg_spll_spread_spectrum);
> - table->initialState.levels[0].sclk.vCG_SPLL_SPREAD_SPECTRUM_2 =
> + table->initialState.level.sclk.vCG_SPLL_SPREAD_SPECTRUM_2 =
> cpu_to_be32(ni_pi->clock_registers.cg_spll_spread_spectrum_2);
> - table->initialState.levels[0].sclk.sclk_value =
> + table->initialState.level.sclk.sclk_value =
> cpu_to_be32(initial_state->performance_levels[0].sclk);
> - table->initialState.levels[0].arbRefreshState =
> + table->initialState.level.arbRefreshState =
> NISLANDS_INITIAL_STATE_ARB_INDEX;
>
> - table->initialState.levels[0].ACIndex = 0;
> + table->initialState.level.ACIndex = 0;
>
> ret = ni_populate_voltage_value(rdev, &eg_pi->vddc_voltage_table,
> initial_state->performance_levels[0].vddc,
> - &table->initialState.levels[0].vddc);
> + &table->initialState.level.vddc);
> if (!ret) {
> u16 std_vddc;
>
> ret = ni_get_std_voltage_value(rdev,
> - &table->initialState.levels[0].vddc,
> + &table->initialState.level.vddc,
> &std_vddc);
> if (!ret)
> ni_populate_std_voltage_value(rdev, std_vddc,
> - table->initialState.levels[0].vddc.index,
> - &table->initialState.levels[0].std_vddc);
> + table->initialState.level.vddc.index,
> + &table->initialState.level.std_vddc);
> }
>
> if (eg_pi->vddci_control)
> ni_populate_voltage_value(rdev,
> &eg_pi->vddci_voltage_table,
> initial_state->performance_levels[0].vddci,
> - &table->initialState.levels[0].vddci);
> + &table->initialState.level.vddci);
>
> - ni_populate_initial_mvdd_value(rdev,
> &table->initialState.levels[0].mvdd);
> + ni_populate_initial_mvdd_value(rdev,
> &table->initialState.level.mvdd);
>
> reg = CG_R(0xffff) | CG_L(0);
> - table->initialState.levels[0].aT = cpu_to_be32(reg);
> + table->initialState.level.aT = cpu_to_be32(reg);
>
> - table->initialState.levels[0].bSP = cpu_to_be32(pi->dsp);
> + table->initialState.level.bSP = cpu_to_be32(pi->dsp);
>
> if (pi->boot_in_gen2)
> - table->initialState.levels[0].gen2PCIE = 1;
> + table->initialState.level.gen2PCIE = 1;
> else
> - table->initialState.levels[0].gen2PCIE = 0;
> + table->initialState.level.gen2PCIE = 0;
>
> if (pi->mem_gddr5) {
> - table->initialState.levels[0].strobeMode =
> + table->initialState.level.strobeMode =
> cypress_get_strobe_mode_settings(rdev,
> initial_state->performance_levels[0].mclk);
>
> if (initial_state->performance_levels[0].mclk >
> pi->mclk_edc_enable_threshold)
> - table->initialState.levels[0].mcFlags =
> NISLANDS_SMC_MC_EDC_RD_FLAG | NISLANDS_SMC_MC_EDC_WR_FLAG;
> + table->initialState.level.mcFlags =
> NISLANDS_SMC_MC_EDC_RD_FLAG | NISLANDS_SMC_MC_EDC_WR_FLAG;
> else
> - table->initialState.levels[0].mcFlags = 0;
> + table->initialState.level.mcFlags = 0;
> }
>
> table->initialState.levelCount = 1;
>
> table->initialState.flags |= PPSMC_SWSTATE_FLAG_DC;
>
> - table->initialState.levels[0].dpm2.MaxPS = 0;
> - table->initialState.levels[0].dpm2.NearTDPDec = 0;
> - table->initialState.levels[0].dpm2.AboveSafeInc = 0;
> - table->initialState.levels[0].dpm2.BelowSafeInc = 0;
> + table->initialState.level.dpm2.MaxPS = 0;
> + table->initialState.level.dpm2.NearTDPDec = 0;
> + table->initialState.level.dpm2.AboveSafeInc = 0;
> + table->initialState.level.dpm2.BelowSafeInc = 0;
>
> reg = MIN_POWER_MASK | MAX_POWER_MASK;
> - table->initialState.levels[0].SQPowerThrottle = cpu_to_be32(reg);
> + table->initialState.level.SQPowerThrottle = cpu_to_be32(reg);
>
> reg = MAX_POWER_DELTA_MASK | STI_SIZE_MASK | LTI_RATIO_MASK;
> - table->initialState.levels[0].SQPowerThrottle_2 = cpu_to_be32(reg);
> + table->initialState.level.SQPowerThrottle_2 = cpu_to_be32(reg);
>
> return 0;
> }
> @@ -1813,43 +1813,43 @@ static int ni_populate_smc_acpi_state(struct
> radeon_device *rdev,
> if (pi->acpi_vddc) {
> ret = ni_populate_voltage_value(rdev,
> &eg_pi->vddc_voltage_table,
> - pi->acpi_vddc,
> &table->ACPIState.levels[0].vddc);
> + pi->acpi_vddc, &table->ACPIState.level.vddc);
> if (!ret) {
> u16 std_vddc;
>
> ret = ni_get_std_voltage_value(rdev,
> - &table->ACPIState.levels[0].vddc, &std_vddc);
> + &table->ACPIState.level.vddc, &std_vddc);
> if (!ret)
> ni_populate_std_voltage_value(rdev, std_vddc,
> - table->ACPIState.levels[0].vddc.index,
> - &table->ACPIState.levels[0].std_vddc);
> + table->ACPIState.level.vddc.index,
> + &table->ACPIState.level.std_vddc);
> }
>
> if (pi->pcie_gen2) {
> if (pi->acpi_pcie_gen2)
> - table->ACPIState.levels[0].gen2PCIE = 1;
> + table->ACPIState.level.gen2PCIE = 1;
> else
> - table->ACPIState.levels[0].gen2PCIE = 0;
> + table->ACPIState.level.gen2PCIE = 0;
> } else {
> - table->ACPIState.levels[0].gen2PCIE = 0;
> + table->ACPIState.level.gen2PCIE = 0;
> }
> } else {
> ret = ni_populate_voltage_value(rdev,
> &eg_pi->vddc_voltage_table,
> pi->min_vddc_in_table,
> - &table->ACPIState.levels[0].vddc);
> + &table->ACPIState.level.vddc);
> if (!ret) {
> u16 std_vddc;
>
> ret = ni_get_std_voltage_value(rdev,
> - &table->ACPIState.levels[0].vddc,
> + &table->ACPIState.level.vddc,
> &std_vddc);
> if (!ret)
> ni_populate_std_voltage_value(rdev, std_vddc,
> - table->ACPIState.levels[0].vddc.index,
> - &table->ACPIState.levels[0].std_vddc);
> + table->ACPIState.level.vddc.index,
> + &table->ACPIState.level.std_vddc);
> }
> - table->ACPIState.levels[0].gen2PCIE = 0;
> + table->ACPIState.level.gen2PCIE = 0;
> }
>
> if (eg_pi->acpi_vddci) {
> @@ -1857,7 +1857,7 @@ static int ni_populate_smc_acpi_state(struct
> radeon_device *rdev,
> ni_populate_voltage_value(rdev,
> &eg_pi->vddci_voltage_table,
> eg_pi->acpi_vddci,
> - &table->ACPIState.levels[0].vddci);
> + &table->ACPIState.level.vddci);
> }
>
>
> @@ -1900,37 +1900,37 @@ static int ni_populate_smc_acpi_state(struct
> radeon_device *rdev,
> spll_func_cntl_2 &= ~SCLK_MUX_SEL_MASK;
> spll_func_cntl_2 |= SCLK_MUX_SEL(4);
>
> - table->ACPIState.levels[0].mclk.vMPLL_AD_FUNC_CNTL =
> cpu_to_be32(mpll_ad_func_cntl);
> - table->ACPIState.levels[0].mclk.vMPLL_AD_FUNC_CNTL_2 =
> cpu_to_be32(mpll_ad_func_cntl_2);
> - table->ACPIState.levels[0].mclk.vMPLL_DQ_FUNC_CNTL =
> cpu_to_be32(mpll_dq_func_cntl);
> - table->ACPIState.levels[0].mclk.vMPLL_DQ_FUNC_CNTL_2 =
> cpu_to_be32(mpll_dq_func_cntl_2);
> - table->ACPIState.levels[0].mclk.vMCLK_PWRMGT_CNTL =
> cpu_to_be32(mclk_pwrmgt_cntl);
> - table->ACPIState.levels[0].mclk.vDLL_CNTL = cpu_to_be32(dll_cntl);
> + table->ACPIState.level.mclk.vMPLL_AD_FUNC_CNTL =
> cpu_to_be32(mpll_ad_func_cntl);
> + table->ACPIState.level.mclk.vMPLL_AD_FUNC_CNTL_2 =
> cpu_to_be32(mpll_ad_func_cntl_2);
> + table->ACPIState.level.mclk.vMPLL_DQ_FUNC_CNTL =
> cpu_to_be32(mpll_dq_func_cntl);
> + table->ACPIState.level.mclk.vMPLL_DQ_FUNC_CNTL_2 =
> cpu_to_be32(mpll_dq_func_cntl_2);
> + table->ACPIState.level.mclk.vMCLK_PWRMGT_CNTL =
> cpu_to_be32(mclk_pwrmgt_cntl);
> + table->ACPIState.level.mclk.vDLL_CNTL = cpu_to_be32(dll_cntl);
>
> - table->ACPIState.levels[0].mclk.mclk_value = 0;
> + table->ACPIState.level.mclk.mclk_value = 0;
>
> - table->ACPIState.levels[0].sclk.vCG_SPLL_FUNC_CNTL =
> cpu_to_be32(spll_func_cntl);
> - table->ACPIState.levels[0].sclk.vCG_SPLL_FUNC_CNTL_2 =
> cpu_to_be32(spll_func_cntl_2);
> - table->ACPIState.levels[0].sclk.vCG_SPLL_FUNC_CNTL_3 =
> cpu_to_be32(spll_func_cntl_3);
> - table->ACPIState.levels[0].sclk.vCG_SPLL_FUNC_CNTL_4 =
> cpu_to_be32(spll_func_cntl_4);
> + table->ACPIState.level.sclk.vCG_SPLL_FUNC_CNTL =
> cpu_to_be32(spll_func_cntl);
> + table->ACPIState.level.sclk.vCG_SPLL_FUNC_CNTL_2 =
> cpu_to_be32(spll_func_cntl_2);
> + table->ACPIState.level.sclk.vCG_SPLL_FUNC_CNTL_3 =
> cpu_to_be32(spll_func_cntl_3);
> + table->ACPIState.level.sclk.vCG_SPLL_FUNC_CNTL_4 =
> cpu_to_be32(spll_func_cntl_4);
>
> - table->ACPIState.levels[0].sclk.sclk_value = 0;
> + table->ACPIState.level.sclk.sclk_value = 0;
>
> - ni_populate_mvdd_value(rdev, 0, &table->ACPIState.levels[0].mvdd);
> + ni_populate_mvdd_value(rdev, 0, &table->ACPIState.level.mvdd);
>
> if (eg_pi->dynamic_ac_timing)
> - table->ACPIState.levels[0].ACIndex = 1;
> + table->ACPIState.level.ACIndex = 1;
>
> - table->ACPIState.levels[0].dpm2.MaxPS = 0;
> - table->ACPIState.levels[0].dpm2.NearTDPDec = 0;
> - table->ACPIState.levels[0].dpm2.AboveSafeInc = 0;
> - table->ACPIState.levels[0].dpm2.BelowSafeInc = 0;
> + table->ACPIState.level.dpm2.MaxPS = 0;
> + table->ACPIState.level.dpm2.NearTDPDec = 0;
> + table->ACPIState.level.dpm2.AboveSafeInc = 0;
> + table->ACPIState.level.dpm2.BelowSafeInc = 0;
>
> reg = MIN_POWER_MASK | MAX_POWER_MASK;
> - table->ACPIState.levels[0].SQPowerThrottle = cpu_to_be32(reg);
> + table->ACPIState.level.SQPowerThrottle = cpu_to_be32(reg);
>
> reg = MAX_POWER_DELTA_MASK | STI_SIZE_MASK | LTI_RATIO_MASK;
> - table->ACPIState.levels[0].SQPowerThrottle_2 = cpu_to_be32(reg);
> + table->ACPIState.level.SQPowerThrottle_2 = cpu_to_be32(reg);
>
> return 0;
> }
> @@ -1980,7 +1980,9 @@ static int ni_init_smc_table(struct
> radeon_device *rdev)
> if (ret)
> return ret;
>
> - table->driverState = table->initialState;
> + table->driverState.flags = table->initialState.flags;
> + table->driverState.levelCount = table->initialState.levelCount;
> + table->driverState.levels[0] = table->initialState.level;
>
> table->ULVState = table->initialState;
>
> diff --git a/drivers/gpu/drm/radeon/nislands_smc.h
> b/drivers/gpu/drm/radeon/nislands_smc.h
> index 7395cb6b3cac..42f3bab0f9ee 100644
> --- a/drivers/gpu/drm/radeon/nislands_smc.h
> +++ b/drivers/gpu/drm/radeon/nislands_smc.h
> @@ -143,6 +143,14 @@ struct NISLANDS_SMC_SWSTATE
>
> typedef struct NISLANDS_SMC_SWSTATE NISLANDS_SMC_SWSTATE;
>
> +struct NISLANDS_SMC_SWSTATE_SINGLE {
> + uint8_t flags;
> + uint8_t levelCount;
> + uint8_t padding2;
> + uint8_t padding3;
> + NISLANDS_SMC_HW_PERFORMANCE_LEVEL level;
> +};
> +
> #define NISLANDS_SMC_VOLTAGEMASK_VDDC 0
> #define NISLANDS_SMC_VOLTAGEMASK_MVDD 1
> #define NISLANDS_SMC_VOLTAGEMASK_VDDCI 2
> @@ -160,19 +168,19 @@ typedef struct NISLANDS_SMC_VOLTAGEMASKTABLE
> NISLANDS_SMC_VOLTAGEMASKTABLE;
>
> struct NISLANDS_SMC_STATETABLE
> {
> - uint8_t thermalProtectType;
> - uint8_t systemFlags;
> - uint8_t maxVDDCIndexInPPTable;
> - uint8_t extraFlags;
> - uint8_t highSMIO[NISLANDS_MAX_NO_VREG_STEPS];
> - uint32_t lowSMIO[NISLANDS_MAX_NO_VREG_STEPS];
> - NISLANDS_SMC_VOLTAGEMASKTABLE voltageMaskTable;
> - PP_NIslands_DPM2Parameters dpm2Params;
> - NISLANDS_SMC_SWSTATE initialState;
> - NISLANDS_SMC_SWSTATE ACPIState;
> - NISLANDS_SMC_SWSTATE ULVState;
> - NISLANDS_SMC_SWSTATE driverState;
> - NISLANDS_SMC_HW_PERFORMANCE_LEVEL
> dpmLevels[NISLANDS_MAX_SMC_PERFORMANCE_LEVELS_PER_SWSTATE - 1];
> + uint8_t thermalProtectType;
> + uint8_t systemFlags;
> + uint8_t maxVDDCIndexInPPTable;
> + uint8_t extraFlags;
> + uint8_t highSMIO[NISLANDS_MAX_NO_VREG_STEPS];
> + uint32_t lowSMIO[NISLANDS_MAX_NO_VREG_STEPS];
> + NISLANDS_SMC_VOLTAGEMASKTABLE voltageMaskTable;
> + PP_NIslands_DPM2Parameters dpm2Params;
> + struct NISLANDS_SMC_SWSTATE_SINGLE initialState;
> + struct NISLANDS_SMC_SWSTATE_SINGLE ACPIState;
> + struct NISLANDS_SMC_SWSTATE_SINGLE ULVState;
> + NISLANDS_SMC_SWSTATE driverState;
> + NISLANDS_SMC_HW_PERFORMANCE_LEVEL
> dpmLevels[NISLANDS_MAX_SMC_PERFORMANCE_LEVELS_PER_SWSTATE];
> };
>
> typedef struct NISLANDS_SMC_STATETABLE NISLANDS_SMC_STATETABLE;
>
^ permalink raw reply
* FAILED: patch "[PATCH] drm/radeon/ni_dpm: Fix booting bug" failed to apply to 5.13-stable tree
From: Christian Zigotzky @ 2021-07-19 14:58 UTC (permalink / raw)
To: Stan Johnson
Cc: Alex Deucher, Debian PowerPC, linuxppc-dev, gustavoars, stable
In-Reply-To: <51f4531c-fc97-4176-616f-fd972834bdae@yahoo.com>
On 19 July 2021 at 04:32 pm, Stan Johnson wrote:
> On 7/18/21 10:23 PM, Christian Zigotzky wrote:
>> Hello Stan,
>>
>> We had the same issue during the 5.14 merge window. Please look in the
>> following thread:
>>
>> https://forum.hyperion-entertainment.com/viewtopic.php?p=53511#p53511
>>
>> There is a patch available. Please try it.
>>
>> Thanks,
>> Christian
>> ...
> Hello Christian,
>
> Thanks. There were some errors applying the patch, so it wasn't fully
> applied (see below). Of course, I'm using 5.13.2, not 5.14, so maybe
> that's expected.
>
> The patched 5.13.2 kernel still results in a blank screen while trying
> to run wdm. On this attempt, wdm has died (oddly the screen remains
> blank; it should display a text login after X dies). The Xorg.0.log
> looks reasonable enough.
>
> I tried disabling wdm, then rebooted, logged in at the console and ran
> "startx". The screen goes blank, X is running, startx is running:
>
> johnson 1392 0.0 0.2 2572 1452 tty1 S+ 08:06 0:00 /bin/sh
> /usr/bin/startx
> johnson 1414 0.0 0.4 4904 2096 tty1 S+ 08:06 0:00 xinit
> /etc/X11/xinit/xinitrc -- /etc/X11/xinit/xserverrc :0 vt1 -keeptty -auth
> /tmp/serverauth.dJ7lSnzjjo
> johnson 1415 1.0 8.2 128436 41924 tty1 Sl 08:06 0:04
> /usr/lib/xorg/Xorg -nolisten tcp :0 vt1 -keeptty -auth
> /tmp/serverauth.dJ7lSnzjjo
>
> I had to use "kill -KILL" to kill the startx, xinit and Xorg processes.
> After those were killed, the screen was still blank, and even though
> nothing was running, the load average was still around 1.00 several
> minutes later, so something is still taking CPU time:
>
> $ uptime
> 08:25:15 up 20 min, 2 users, load average: 1.00, 1.00, 0.84
>
> I can attempt a git bisect, though that will take some time.
>
> -Stan
>
> ----------
> $ patch -p1
> <../v3-drm-radeon-Fix-NULL-dereference-when-updating-memory-stats.patch
> patching file drivers/gpu/drm/radeon/radeon_object.c
> Hunk #2 FAILED at 76.
> Hunk #3 FAILED at 727.
> 2 out of 3 hunks FAILED -- saving rejects to file
> drivers/gpu/drm/radeon/radeon_object.c.rej
> patching file drivers/gpu/drm/radeon/radeon_object.h
> patching file drivers/gpu/drm/radeon/radeon_ttm.c
> Hunk #1 FAILED at 199.
> Hunk #2 succeeded at 227 (offset 11 lines).
> Hunk #3 succeeded at 275 (offset 11 lines).
> Hunk #4 succeeded at 697 (offset 12 lines).
> 1 out of 4 hunks FAILED -- saving rejects to file
> drivers/gpu/drm/radeon/radeon_ttm.c.rej
> johnson@mac-server:/data/software/working/linux-5.13.2$ cat
> drivers/gpu/drm/radeon/radeon_ttm.c.rej
> --- drivers/gpu/drm/radeon/radeon_ttm.c
> +++ drivers/gpu/drm/radeon/radeon_ttm.c
> @@ -199,7 +199,7 @@ static int radeon_bo_move(struct ttm_buffer_object
> *bo, bool evict,
> struct ttm_resource *old_mem = bo->resource;
> struct radeon_device *rdev;
> struct radeon_bo *rbo;
> - int r;
> + int r, old_type;
>
> if (new_mem->mem_type == TTM_PL_TT) {
> r = radeon_ttm_tt_bind(bo->bdev, bo->ttm, new_mem);
>
> ---------
Hello Stan,
Greg has the same issue with patching the kernel 5.13 [1]. We have to
wait for a solution.
- Christian
[1]
The patch below does not apply to the 5.13-stable tree.
If someone wants it applied there, or to any other stable or longterm
tree, then please email the backport, including the original git commit
id to <stable@vger.kernel.org>.
thanks,
greg k-h
------------------ original commit in Linus's tree ------------------
>From 293774413a3f519c826d4eb5313ef02e20515700 Mon Sep 17 00:00:00 2001
From: "Gustavo A. R. Silva" <gustavoars@kernel.org>
Date: Sun, 9 May 2021 17:49:26 -0500
Subject: [PATCH] drm/radeon/ni_dpm: Fix booting bug
Create new structure NISLANDS_SMC_SWSTATE_SINGLE, as initialState.levels
and ACPIState.levels are never actually used as flexible arrays. Those
arrays can be used as simple objects of type
NISLANDS_SMC_HW_PERFORMANCE_LEVEL, instead.
Currently, the code fails because flexible array _levels_ in
struct NISLANDS_SMC_SWSTATE doesn't allow for code that access
the first element of initialState.levels and ACPIState.levels
arrays:
drivers/gpu/drm/radeon/ni_dpm.c:
1690 table->initialState.levels[0].mclk.vMPLL_AD_FUNC_CNTL =
1691 cpu_to_be32(ni_pi->clock_registers.mpll_ad_func_cntl);
...
1903: table->ACPIState.levels[0].mclk.vMPLL_AD_FUNC_CNTL =
cpu_to_be32(mpll_ad_func_cntl);
1904: table->ACPIState.levels[0].mclk.vMPLL_AD_FUNC_CNTL_2 =
cpu_to_be32(mpll_ad_func_cntl_2);
because such element cannot exist without previously allocating
any dynamic memory for it (which never actually happens).
That's why struct NISLANDS_SMC_SWSTATE should only be used as type
for object driverState and new struct SISLANDS_SMC_SWSTATE_SINGLE is
created as type for objects initialState, ACPIState and ULVState.
Also, with the change from one-element array to flexible-array member
in commit 434fb1e7444a ("drm/radeon/nislands_smc.h: Replace one-element
array with flexible-array member in struct NISLANDS_SMC_SWSTATE"), the
size of dpmLevels in struct NISLANDS_SMC_STATETABLE should be fixed to
be NISLANDS_MAX_SMC_PERFORMANCE_LEVELS_PER_SWSTATE instead of
NISLANDS_MAX_SMC_PERFORMANCE_LEVELS_PER_SWSTATE - 1.
Bug:
https://lore.kernel.org/dri-devel/3eedbe78-1fbd-4763-a7f3-ac5665e76a4a@xenosoft.de/
Fixes: 434fb1e7444a ("drm/radeon/nislands_smc.h: Replace one-element
array with flexible-array member in struct NISLANDS_SMC_SWSTATE")
Cc: stable@vger.kernel.org
Reported-by: Christian Zigotzky <chzigotzky@xenosoft.de>
Tested-by: Christian Zigotzky <chzigotzky@xenosoft.de>
Link:
https://lore.kernel.org/dri-devel/9bb5fcbd-daf5-1669-b3e7-b8624b3c36f9@xenosoft.de/
Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
Signed-off-by: Alex Deucher <alexander.deucher@amd.com>
diff --git a/drivers/gpu/drm/radeon/ni_dpm.c
b/drivers/gpu/drm/radeon/ni_dpm.c
index dd5ef6493723..769f666335ac 100644
--- a/drivers/gpu/drm/radeon/ni_dpm.c
+++ b/drivers/gpu/drm/radeon/ni_dpm.c
@@ -1687,102 +1687,102 @@ static int
ni_populate_smc_initial_state(struct radeon_device *rdev,
u32 reg;
int ret;
- table->initialState.levels[0].mclk.vMPLL_AD_FUNC_CNTL =
+ table->initialState.level.mclk.vMPLL_AD_FUNC_CNTL =
cpu_to_be32(ni_pi->clock_registers.mpll_ad_func_cntl);
- table->initialState.levels[0].mclk.vMPLL_AD_FUNC_CNTL_2 =
+ table->initialState.level.mclk.vMPLL_AD_FUNC_CNTL_2 =
cpu_to_be32(ni_pi->clock_registers.mpll_ad_func_cntl_2);
- table->initialState.levels[0].mclk.vMPLL_DQ_FUNC_CNTL =
+ table->initialState.level.mclk.vMPLL_DQ_FUNC_CNTL =
cpu_to_be32(ni_pi->clock_registers.mpll_dq_func_cntl);
- table->initialState.levels[0].mclk.vMPLL_DQ_FUNC_CNTL_2 =
+ table->initialState.level.mclk.vMPLL_DQ_FUNC_CNTL_2 =
cpu_to_be32(ni_pi->clock_registers.mpll_dq_func_cntl_2);
- table->initialState.levels[0].mclk.vMCLK_PWRMGT_CNTL =
+ table->initialState.level.mclk.vMCLK_PWRMGT_CNTL =
cpu_to_be32(ni_pi->clock_registers.mclk_pwrmgt_cntl);
- table->initialState.levels[0].mclk.vDLL_CNTL =
+ table->initialState.level.mclk.vDLL_CNTL =
cpu_to_be32(ni_pi->clock_registers.dll_cntl);
- table->initialState.levels[0].mclk.vMPLL_SS =
+ table->initialState.level.mclk.vMPLL_SS =
cpu_to_be32(ni_pi->clock_registers.mpll_ss1);
- table->initialState.levels[0].mclk.vMPLL_SS2 =
+ table->initialState.level.mclk.vMPLL_SS2 =
cpu_to_be32(ni_pi->clock_registers.mpll_ss2);
- table->initialState.levels[0].mclk.mclk_value =
+ table->initialState.level.mclk.mclk_value =
cpu_to_be32(initial_state->performance_levels[0].mclk);
- table->initialState.levels[0].sclk.vCG_SPLL_FUNC_CNTL =
+ table->initialState.level.sclk.vCG_SPLL_FUNC_CNTL =
cpu_to_be32(ni_pi->clock_registers.cg_spll_func_cntl);
- table->initialState.levels[0].sclk.vCG_SPLL_FUNC_CNTL_2 =
+ table->initialState.level.sclk.vCG_SPLL_FUNC_CNTL_2 =
cpu_to_be32(ni_pi->clock_registers.cg_spll_func_cntl_2);
- table->initialState.levels[0].sclk.vCG_SPLL_FUNC_CNTL_3 =
+ table->initialState.level.sclk.vCG_SPLL_FUNC_CNTL_3 =
cpu_to_be32(ni_pi->clock_registers.cg_spll_func_cntl_3);
- table->initialState.levels[0].sclk.vCG_SPLL_FUNC_CNTL_4 =
+ table->initialState.level.sclk.vCG_SPLL_FUNC_CNTL_4 =
cpu_to_be32(ni_pi->clock_registers.cg_spll_func_cntl_4);
- table->initialState.levels[0].sclk.vCG_SPLL_SPREAD_SPECTRUM =
+ table->initialState.level.sclk.vCG_SPLL_SPREAD_SPECTRUM =
cpu_to_be32(ni_pi->clock_registers.cg_spll_spread_spectrum);
- table->initialState.levels[0].sclk.vCG_SPLL_SPREAD_SPECTRUM_2 =
+ table->initialState.level.sclk.vCG_SPLL_SPREAD_SPECTRUM_2 =
cpu_to_be32(ni_pi->clock_registers.cg_spll_spread_spectrum_2);
- table->initialState.levels[0].sclk.sclk_value =
+ table->initialState.level.sclk.sclk_value =
cpu_to_be32(initial_state->performance_levels[0].sclk);
- table->initialState.levels[0].arbRefreshState =
+ table->initialState.level.arbRefreshState =
NISLANDS_INITIAL_STATE_ARB_INDEX;
- table->initialState.levels[0].ACIndex = 0;
+ table->initialState.level.ACIndex = 0;
ret = ni_populate_voltage_value(rdev, &eg_pi->vddc_voltage_table,
initial_state->performance_levels[0].vddc,
- &table->initialState.levels[0].vddc);
+ &table->initialState.level.vddc);
if (!ret) {
u16 std_vddc;
ret = ni_get_std_voltage_value(rdev,
- &table->initialState.levels[0].vddc,
+ &table->initialState.level.vddc,
&std_vddc);
if (!ret)
ni_populate_std_voltage_value(rdev, std_vddc,
- table->initialState.levels[0].vddc.index,
- &table->initialState.levels[0].std_vddc);
+ table->initialState.level.vddc.index,
+ &table->initialState.level.std_vddc);
}
if (eg_pi->vddci_control)
ni_populate_voltage_value(rdev,
&eg_pi->vddci_voltage_table,
initial_state->performance_levels[0].vddci,
- &table->initialState.levels[0].vddci);
+ &table->initialState.level.vddci);
- ni_populate_initial_mvdd_value(rdev,
&table->initialState.levels[0].mvdd);
+ ni_populate_initial_mvdd_value(rdev, &table->initialState.level.mvdd);
reg = CG_R(0xffff) | CG_L(0);
- table->initialState.levels[0].aT = cpu_to_be32(reg);
+ table->initialState.level.aT = cpu_to_be32(reg);
- table->initialState.levels[0].bSP = cpu_to_be32(pi->dsp);
+ table->initialState.level.bSP = cpu_to_be32(pi->dsp);
if (pi->boot_in_gen2)
- table->initialState.levels[0].gen2PCIE = 1;
+ table->initialState.level.gen2PCIE = 1;
else
- table->initialState.levels[0].gen2PCIE = 0;
+ table->initialState.level.gen2PCIE = 0;
if (pi->mem_gddr5) {
- table->initialState.levels[0].strobeMode =
+ table->initialState.level.strobeMode =
cypress_get_strobe_mode_settings(rdev,
initial_state->performance_levels[0].mclk);
if (initial_state->performance_levels[0].mclk >
pi->mclk_edc_enable_threshold)
- table->initialState.levels[0].mcFlags =
NISLANDS_SMC_MC_EDC_RD_FLAG | NISLANDS_SMC_MC_EDC_WR_FLAG;
+ table->initialState.level.mcFlags =
NISLANDS_SMC_MC_EDC_RD_FLAG | NISLANDS_SMC_MC_EDC_WR_FLAG;
else
- table->initialState.levels[0].mcFlags = 0;
+ table->initialState.level.mcFlags = 0;
}
table->initialState.levelCount = 1;
table->initialState.flags |= PPSMC_SWSTATE_FLAG_DC;
- table->initialState.levels[0].dpm2.MaxPS = 0;
- table->initialState.levels[0].dpm2.NearTDPDec = 0;
- table->initialState.levels[0].dpm2.AboveSafeInc = 0;
- table->initialState.levels[0].dpm2.BelowSafeInc = 0;
+ table->initialState.level.dpm2.MaxPS = 0;
+ table->initialState.level.dpm2.NearTDPDec = 0;
+ table->initialState.level.dpm2.AboveSafeInc = 0;
+ table->initialState.level.dpm2.BelowSafeInc = 0;
reg = MIN_POWER_MASK | MAX_POWER_MASK;
- table->initialState.levels[0].SQPowerThrottle = cpu_to_be32(reg);
+ table->initialState.level.SQPowerThrottle = cpu_to_be32(reg);
reg = MAX_POWER_DELTA_MASK | STI_SIZE_MASK | LTI_RATIO_MASK;
- table->initialState.levels[0].SQPowerThrottle_2 = cpu_to_be32(reg);
+ table->initialState.level.SQPowerThrottle_2 = cpu_to_be32(reg);
return 0;
}
@@ -1813,43 +1813,43 @@ static int ni_populate_smc_acpi_state(struct
radeon_device *rdev,
if (pi->acpi_vddc) {
ret = ni_populate_voltage_value(rdev,
&eg_pi->vddc_voltage_table,
- pi->acpi_vddc, &table->ACPIState.levels[0].vddc);
+ pi->acpi_vddc, &table->ACPIState.level.vddc);
if (!ret) {
u16 std_vddc;
ret = ni_get_std_voltage_value(rdev,
- &table->ACPIState.levels[0].vddc, &std_vddc);
+ &table->ACPIState.level.vddc, &std_vddc);
if (!ret)
ni_populate_std_voltage_value(rdev, std_vddc,
- table->ACPIState.levels[0].vddc.index,
- &table->ACPIState.levels[0].std_vddc);
+ table->ACPIState.level.vddc.index,
+ &table->ACPIState.level.std_vddc);
}
if (pi->pcie_gen2) {
if (pi->acpi_pcie_gen2)
- table->ACPIState.levels[0].gen2PCIE = 1;
+ table->ACPIState.level.gen2PCIE = 1;
else
- table->ACPIState.levels[0].gen2PCIE = 0;
+ table->ACPIState.level.gen2PCIE = 0;
} else {
- table->ACPIState.levels[0].gen2PCIE = 0;
+ table->ACPIState.level.gen2PCIE = 0;
}
} else {
ret = ni_populate_voltage_value(rdev,
&eg_pi->vddc_voltage_table,
pi->min_vddc_in_table,
- &table->ACPIState.levels[0].vddc);
+ &table->ACPIState.level.vddc);
if (!ret) {
u16 std_vddc;
ret = ni_get_std_voltage_value(rdev,
- &table->ACPIState.levels[0].vddc,
+ &table->ACPIState.level.vddc,
&std_vddc);
if (!ret)
ni_populate_std_voltage_value(rdev, std_vddc,
- table->ACPIState.levels[0].vddc.index,
- &table->ACPIState.levels[0].std_vddc);
+ table->ACPIState.level.vddc.index,
+ &table->ACPIState.level.std_vddc);
}
- table->ACPIState.levels[0].gen2PCIE = 0;
+ table->ACPIState.level.gen2PCIE = 0;
}
if (eg_pi->acpi_vddci) {
@@ -1857,7 +1857,7 @@ static int ni_populate_smc_acpi_state(struct
radeon_device *rdev,
ni_populate_voltage_value(rdev,
&eg_pi->vddci_voltage_table,
eg_pi->acpi_vddci,
- &table->ACPIState.levels[0].vddci);
+ &table->ACPIState.level.vddci);
}
@@ -1900,37 +1900,37 @@ static int ni_populate_smc_acpi_state(struct
radeon_device *rdev,
spll_func_cntl_2 &= ~SCLK_MUX_SEL_MASK;
spll_func_cntl_2 |= SCLK_MUX_SEL(4);
- table->ACPIState.levels[0].mclk.vMPLL_AD_FUNC_CNTL =
cpu_to_be32(mpll_ad_func_cntl);
- table->ACPIState.levels[0].mclk.vMPLL_AD_FUNC_CNTL_2 =
cpu_to_be32(mpll_ad_func_cntl_2);
- table->ACPIState.levels[0].mclk.vMPLL_DQ_FUNC_CNTL =
cpu_to_be32(mpll_dq_func_cntl);
- table->ACPIState.levels[0].mclk.vMPLL_DQ_FUNC_CNTL_2 =
cpu_to_be32(mpll_dq_func_cntl_2);
- table->ACPIState.levels[0].mclk.vMCLK_PWRMGT_CNTL =
cpu_to_be32(mclk_pwrmgt_cntl);
- table->ACPIState.levels[0].mclk.vDLL_CNTL = cpu_to_be32(dll_cntl);
+ table->ACPIState.level.mclk.vMPLL_AD_FUNC_CNTL =
cpu_to_be32(mpll_ad_func_cntl);
+ table->ACPIState.level.mclk.vMPLL_AD_FUNC_CNTL_2 =
cpu_to_be32(mpll_ad_func_cntl_2);
+ table->ACPIState.level.mclk.vMPLL_DQ_FUNC_CNTL =
cpu_to_be32(mpll_dq_func_cntl);
+ table->ACPIState.level.mclk.vMPLL_DQ_FUNC_CNTL_2 =
cpu_to_be32(mpll_dq_func_cntl_2);
+ table->ACPIState.level.mclk.vMCLK_PWRMGT_CNTL =
cpu_to_be32(mclk_pwrmgt_cntl);
+ table->ACPIState.level.mclk.vDLL_CNTL = cpu_to_be32(dll_cntl);
- table->ACPIState.levels[0].mclk.mclk_value = 0;
+ table->ACPIState.level.mclk.mclk_value = 0;
- table->ACPIState.levels[0].sclk.vCG_SPLL_FUNC_CNTL =
cpu_to_be32(spll_func_cntl);
- table->ACPIState.levels[0].sclk.vCG_SPLL_FUNC_CNTL_2 =
cpu_to_be32(spll_func_cntl_2);
- table->ACPIState.levels[0].sclk.vCG_SPLL_FUNC_CNTL_3 =
cpu_to_be32(spll_func_cntl_3);
- table->ACPIState.levels[0].sclk.vCG_SPLL_FUNC_CNTL_4 =
cpu_to_be32(spll_func_cntl_4);
+ table->ACPIState.level.sclk.vCG_SPLL_FUNC_CNTL =
cpu_to_be32(spll_func_cntl);
+ table->ACPIState.level.sclk.vCG_SPLL_FUNC_CNTL_2 =
cpu_to_be32(spll_func_cntl_2);
+ table->ACPIState.level.sclk.vCG_SPLL_FUNC_CNTL_3 =
cpu_to_be32(spll_func_cntl_3);
+ table->ACPIState.level.sclk.vCG_SPLL_FUNC_CNTL_4 =
cpu_to_be32(spll_func_cntl_4);
- table->ACPIState.levels[0].sclk.sclk_value = 0;
+ table->ACPIState.level.sclk.sclk_value = 0;
- ni_populate_mvdd_value(rdev, 0, &table->ACPIState.levels[0].mvdd);
+ ni_populate_mvdd_value(rdev, 0, &table->ACPIState.level.mvdd);
if (eg_pi->dynamic_ac_timing)
- table->ACPIState.levels[0].ACIndex = 1;
+ table->ACPIState.level.ACIndex = 1;
- table->ACPIState.levels[0].dpm2.MaxPS = 0;
- table->ACPIState.levels[0].dpm2.NearTDPDec = 0;
- table->ACPIState.levels[0].dpm2.AboveSafeInc = 0;
- table->ACPIState.levels[0].dpm2.BelowSafeInc = 0;
+ table->ACPIState.level.dpm2.MaxPS = 0;
+ table->ACPIState.level.dpm2.NearTDPDec = 0;
+ table->ACPIState.level.dpm2.AboveSafeInc = 0;
+ table->ACPIState.level.dpm2.BelowSafeInc = 0;
reg = MIN_POWER_MASK | MAX_POWER_MASK;
- table->ACPIState.levels[0].SQPowerThrottle = cpu_to_be32(reg);
+ table->ACPIState.level.SQPowerThrottle = cpu_to_be32(reg);
reg = MAX_POWER_DELTA_MASK | STI_SIZE_MASK | LTI_RATIO_MASK;
- table->ACPIState.levels[0].SQPowerThrottle_2 = cpu_to_be32(reg);
+ table->ACPIState.level.SQPowerThrottle_2 = cpu_to_be32(reg);
return 0;
}
@@ -1980,7 +1980,9 @@ static int ni_init_smc_table(struct radeon_device
*rdev)
if (ret)
return ret;
- table->driverState = table->initialState;
+ table->driverState.flags = table->initialState.flags;
+ table->driverState.levelCount = table->initialState.levelCount;
+ table->driverState.levels[0] = table->initialState.level;
table->ULVState = table->initialState;
diff --git a/drivers/gpu/drm/radeon/nislands_smc.h
b/drivers/gpu/drm/radeon/nislands_smc.h
index 7395cb6b3cac..42f3bab0f9ee 100644
--- a/drivers/gpu/drm/radeon/nislands_smc.h
+++ b/drivers/gpu/drm/radeon/nislands_smc.h
@@ -143,6 +143,14 @@ struct NISLANDS_SMC_SWSTATE
typedef struct NISLANDS_SMC_SWSTATE NISLANDS_SMC_SWSTATE;
+struct NISLANDS_SMC_SWSTATE_SINGLE {
+ uint8_t flags;
+ uint8_t levelCount;
+ uint8_t padding2;
+ uint8_t padding3;
+ NISLANDS_SMC_HW_PERFORMANCE_LEVEL level;
+};
+
#define NISLANDS_SMC_VOLTAGEMASK_VDDC 0
#define NISLANDS_SMC_VOLTAGEMASK_MVDD 1
#define NISLANDS_SMC_VOLTAGEMASK_VDDCI 2
@@ -160,19 +168,19 @@ typedef struct NISLANDS_SMC_VOLTAGEMASKTABLE
NISLANDS_SMC_VOLTAGEMASKTABLE;
struct NISLANDS_SMC_STATETABLE
{
- uint8_t thermalProtectType;
- uint8_t systemFlags;
- uint8_t maxVDDCIndexInPPTable;
- uint8_t extraFlags;
- uint8_t highSMIO[NISLANDS_MAX_NO_VREG_STEPS];
- uint32_t lowSMIO[NISLANDS_MAX_NO_VREG_STEPS];
- NISLANDS_SMC_VOLTAGEMASKTABLE voltageMaskTable;
- PP_NIslands_DPM2Parameters dpm2Params;
- NISLANDS_SMC_SWSTATE initialState;
- NISLANDS_SMC_SWSTATE ACPIState;
- NISLANDS_SMC_SWSTATE ULVState;
- NISLANDS_SMC_SWSTATE driverState;
- NISLANDS_SMC_HW_PERFORMANCE_LEVEL
dpmLevels[NISLANDS_MAX_SMC_PERFORMANCE_LEVELS_PER_SWSTATE - 1];
+ uint8_t thermalProtectType;
+ uint8_t systemFlags;
+ uint8_t maxVDDCIndexInPPTable;
+ uint8_t extraFlags;
+ uint8_t highSMIO[NISLANDS_MAX_NO_VREG_STEPS];
+ uint32_t lowSMIO[NISLANDS_MAX_NO_VREG_STEPS];
+ NISLANDS_SMC_VOLTAGEMASKTABLE voltageMaskTable;
+ PP_NIslands_DPM2Parameters dpm2Params;
+ struct NISLANDS_SMC_SWSTATE_SINGLE initialState;
+ struct NISLANDS_SMC_SWSTATE_SINGLE ACPIState;
+ struct NISLANDS_SMC_SWSTATE_SINGLE ULVState;
+ NISLANDS_SMC_SWSTATE driverState;
+ NISLANDS_SMC_HW_PERFORMANCE_LEVEL
dpmLevels[NISLANDS_MAX_SMC_PERFORMANCE_LEVELS_PER_SWSTATE];
};
typedef struct NISLANDS_SMC_STATETABLE NISLANDS_SMC_STATETABLE;
^ permalink raw reply related
* Re: [PATCH v5 04/11] powerpc/pseries/iommu: Add ddw_list_new_entry() helper
From: Frederic Barrat @ 2021-07-19 14:14 UTC (permalink / raw)
To: Leonardo Bras, Michael Ellerman, Benjamin Herrenschmidt,
Paul Mackerras, Alexey Kardashevskiy, David Gibson,
kernel test robot, Nicolin Chen
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <20210716082755.428187-5-leobras.c@gmail.com>
On 16/07/2021 10:27, Leonardo Bras wrote:
> There are two functions creating direct_window_list entries in a
> similar way, so create a ddw_list_new_entry() to avoid duplicity and
> simplify those functions.
>
> Signed-off-by: Leonardo Bras <leobras.c@gmail.com>
> Reviewed-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> ---
LGTM
Reviewed-by: Frederic Barrat <fbarrat@linux.ibm.com>
> arch/powerpc/platforms/pseries/iommu.c | 32 +++++++++++++++++---------
> 1 file changed, 21 insertions(+), 11 deletions(-)
>
> diff --git a/arch/powerpc/platforms/pseries/iommu.c b/arch/powerpc/platforms/pseries/iommu.c
> index 33d82865d6e6..712d1667144a 100644
> --- a/arch/powerpc/platforms/pseries/iommu.c
> +++ b/arch/powerpc/platforms/pseries/iommu.c
> @@ -874,6 +874,21 @@ static u64 find_existing_ddw(struct device_node *pdn, int *window_shift)
> return dma_addr;
> }
>
> +static struct direct_window *ddw_list_new_entry(struct device_node *pdn,
> + const struct dynamic_dma_window_prop *dma64)
> +{
> + struct direct_window *window;
> +
> + window = kzalloc(sizeof(*window), GFP_KERNEL);
> + if (!window)
> + return NULL;
> +
> + window->device = pdn;
> + window->prop = dma64;
> +
> + return window;
> +}
> +
> static int find_existing_ddw_windows(void)
> {
> int len;
> @@ -886,18 +901,15 @@ static int find_existing_ddw_windows(void)
>
> for_each_node_with_property(pdn, DIRECT64_PROPNAME) {
> direct64 = of_get_property(pdn, DIRECT64_PROPNAME, &len);
> - if (!direct64)
> - continue;
> -
> - window = kzalloc(sizeof(*window), GFP_KERNEL);
> - if (!window || len < sizeof(struct dynamic_dma_window_prop)) {
> - kfree(window);
> + if (!direct64 || len < sizeof(*direct64)) {
> remove_ddw(pdn, true);
> continue;
> }
>
> - window->device = pdn;
> - window->prop = direct64;
> + window = ddw_list_new_entry(pdn, direct64);
> + if (!window)
> + break;
> +
> spin_lock(&direct_window_list_lock);
> list_add(&window->list, &direct_window_list);
> spin_unlock(&direct_window_list_lock);
> @@ -1307,7 +1319,7 @@ static u64 enable_ddw(struct pci_dev *dev, struct device_node *pdn)
> dev_dbg(&dev->dev, "created tce table LIOBN 0x%x for %pOF\n",
> create.liobn, dn);
>
> - window = kzalloc(sizeof(*window), GFP_KERNEL);
> + window = ddw_list_new_entry(pdn, ddwprop);
> if (!window)
> goto out_clear_window;
>
> @@ -1326,8 +1338,6 @@ static u64 enable_ddw(struct pci_dev *dev, struct device_node *pdn)
> goto out_free_window;
> }
>
> - window->device = pdn;
> - window->prop = ddwprop;
> spin_lock(&direct_window_list_lock);
> list_add(&window->list, &direct_window_list);
> spin_unlock(&direct_window_list_lock);
>
^ permalink raw reply
* Re: [PATCH v5 03/11] powerpc/pseries/iommu: Add iommu_pseries_alloc_table() helper
From: Frederic Barrat @ 2021-07-19 14:04 UTC (permalink / raw)
To: Leonardo Bras, Michael Ellerman, Benjamin Herrenschmidt,
Paul Mackerras, Alexey Kardashevskiy, David Gibson,
kernel test robot, Nicolin Chen
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <20210716082755.428187-4-leobras.c@gmail.com>
On 16/07/2021 10:27, Leonardo Bras wrote:
> Creates a helper to allow allocating a new iommu_table without the need
> to reallocate the iommu_group.
>
> This will be helpful for replacing the iommu_table for the new DMA window,
> after we remove the old one with iommu_tce_table_put().
>
> Signed-off-by: Leonardo Bras <leobras.c@gmail.com>
> Reviewed-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> ---
> arch/powerpc/platforms/pseries/iommu.c | 25 ++++++++++++++-----------
> 1 file changed, 14 insertions(+), 11 deletions(-)
>
> diff --git a/arch/powerpc/platforms/pseries/iommu.c b/arch/powerpc/platforms/pseries/iommu.c
> index b1b8d12bab39..33d82865d6e6 100644
> --- a/arch/powerpc/platforms/pseries/iommu.c
> +++ b/arch/powerpc/platforms/pseries/iommu.c
> @@ -53,28 +53,31 @@ enum {
> DDW_EXT_QUERY_OUT_SIZE = 2
> };
>
> -static struct iommu_table_group *iommu_pseries_alloc_group(int node)
> +static struct iommu_table *iommu_pseries_alloc_table(int node)
> {
> - struct iommu_table_group *table_group;
> struct iommu_table *tbl;
>
> - table_group = kzalloc_node(sizeof(struct iommu_table_group), GFP_KERNEL,
> - node);
> - if (!table_group)
> - return NULL;
> -
> tbl = kzalloc_node(sizeof(struct iommu_table), GFP_KERNEL, node);
> if (!tbl)
> - goto free_group;
> + return NULL;
>
> INIT_LIST_HEAD_RCU(&tbl->it_group_list);
> kref_init(&tbl->it_kref);
> + return tbl;
> +}
>
> - table_group->tables[0] = tbl;
> +static struct iommu_table_group *iommu_pseries_alloc_group(int node)
> +{
> + struct iommu_table_group *table_group;
> +
> + table_group = kzalloc_node(sizeof(*table_group), GFP_KERNEL, node);
> + if (!table_group)
> + return NULL;
>
> - return table_group;
> + table_group->tables[0] = iommu_pseries_alloc_table(node);
> + if (table_group->tables[0])
> + return table_group;
Nitpick: for readability, we'd usually expect the error path to be
detected with the if statement and keep going on the good path, and here
the code does the opposite. No big deal though, so
Reviewed-by: Frederic Barrat <fbarrat@linux.ibm.com>
>
> -free_group:
> kfree(table_group);
> return NULL;
> }
>
^ permalink raw reply
* Re: [PATCH v5 02/11] powerpc/kernel/iommu: Add new iommu_table_in_use() helper
From: Frederic Barrat @ 2021-07-19 13:53 UTC (permalink / raw)
To: Leonardo Bras, Michael Ellerman, Benjamin Herrenschmidt,
Paul Mackerras, Alexey Kardashevskiy, David Gibson,
kernel test robot, Nicolin Chen
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <20210716082755.428187-3-leobras.c@gmail.com>
On 16/07/2021 10:27, Leonardo Bras wrote:
> @@ -1099,18 +1105,13 @@ int iommu_take_ownership(struct iommu_table *tbl)
> for (i = 0; i < tbl->nr_pools; i++)
> spin_lock_nest_lock(&tbl->pools[i].lock, &tbl->large_pool.lock);
>
> - iommu_table_release_pages(tbl);
> -
> - if (!bitmap_empty(tbl->it_map, tbl->it_size)) {
> + if (iommu_table_in_use(tbl)) {
> pr_err("iommu_tce: it_map is not empty");
> ret = -EBUSY;
> - /* Undo iommu_table_release_pages, i.e. restore bit#0, etc */
> - iommu_table_reserve_pages(tbl, tbl->it_reserved_start,
> - tbl->it_reserved_end);
> - } else {
> - memset(tbl->it_map, 0xff, sz);
> }
>
> + memset(tbl->it_map, 0xff, sz);
> +
So if the table is not empty, we fail (EBUSY) but we now also completely
overwrite the bitmap. It was in an unexpected state, but we're making it
worse. Or am I missing something?
Fred
> for (i = 0; i < tbl->nr_pools; i++)
> spin_unlock(&tbl->pools[i].lock);
> spin_unlock_irqrestore(&tbl->large_pool.lock, flags);
^ permalink raw reply
* Re: [PATCH v5 01/11] powerpc/pseries/iommu: Replace hard-coded page shift
From: Frederic Barrat @ 2021-07-19 13:48 UTC (permalink / raw)
To: Leonardo Bras, Michael Ellerman, Benjamin Herrenschmidt,
Paul Mackerras, Alexey Kardashevskiy, David Gibson,
kernel test robot, Nicolin Chen
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <20210716082755.428187-2-leobras.c@gmail.com>
On 16/07/2021 10:27, Leonardo Bras wrote:
> Some functions assume IOMMU page size can only be 4K (pageshift == 12).
> Update them to accept any page size passed, so we can use 64K pages.
>
> In the process, some defines like TCE_SHIFT were made obsolete, and then
> removed.
>
> IODA3 Revision 3.0_prd1 (OpenPowerFoundation), Figures 3.4 and 3.5 show
> a RPN of 52-bit, and considers a 12-bit pageshift, so there should be
> no need of using TCE_RPN_MASK, which masks out any bit after 40 in rpn.
> It's usage removed from tce_build_pSeries(), tce_build_pSeriesLP(), and
> tce_buildmulti_pSeriesLP().
>
> Most places had a tbl struct, so using tbl->it_page_shift was simple.
> tce_free_pSeriesLP() was a special case, since callers not always have a
> tbl struct, so adding a tceshift parameter seems the right thing to do.
>
> Signed-off-by: Leonardo Bras <leobras.c@gmail.com>
> Reviewed-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> ---
FWIW,
Reviewed-by: Frederic Barrat <fbarrat@linux.ibm.com>
> arch/powerpc/include/asm/tce.h | 8 ------
> arch/powerpc/platforms/pseries/iommu.c | 39 +++++++++++++++-----------
> 2 files changed, 23 insertions(+), 24 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/tce.h b/arch/powerpc/include/asm/tce.h
> index db5fc2f2262d..0c34d2756d92 100644
> --- a/arch/powerpc/include/asm/tce.h
> +++ b/arch/powerpc/include/asm/tce.h
> @@ -19,15 +19,7 @@
> #define TCE_VB 0
> #define TCE_PCI 1
>
> -/* TCE page size is 4096 bytes (1 << 12) */
> -
> -#define TCE_SHIFT 12
> -#define TCE_PAGE_SIZE (1 << TCE_SHIFT)
> -
> #define TCE_ENTRY_SIZE 8 /* each TCE is 64 bits */
> -
> -#define TCE_RPN_MASK 0xfffffffffful /* 40-bit RPN (4K pages) */
> -#define TCE_RPN_SHIFT 12
> #define TCE_VALID 0x800 /* TCE valid */
> #define TCE_ALLIO 0x400 /* TCE valid for all lpars */
> #define TCE_PCI_WRITE 0x2 /* write from PCI allowed */
> diff --git a/arch/powerpc/platforms/pseries/iommu.c b/arch/powerpc/platforms/pseries/iommu.c
> index 0c55b991f665..b1b8d12bab39 100644
> --- a/arch/powerpc/platforms/pseries/iommu.c
> +++ b/arch/powerpc/platforms/pseries/iommu.c
> @@ -107,6 +107,8 @@ static int tce_build_pSeries(struct iommu_table *tbl, long index,
> u64 proto_tce;
> __be64 *tcep;
> u64 rpn;
> + const unsigned long tceshift = tbl->it_page_shift;
> + const unsigned long pagesize = IOMMU_PAGE_SIZE(tbl);
>
> proto_tce = TCE_PCI_READ; // Read allowed
>
> @@ -117,10 +119,10 @@ static int tce_build_pSeries(struct iommu_table *tbl, long index,
>
> while (npages--) {
> /* can't move this out since we might cross MEMBLOCK boundary */
> - rpn = __pa(uaddr) >> TCE_SHIFT;
> - *tcep = cpu_to_be64(proto_tce | (rpn & TCE_RPN_MASK) << TCE_RPN_SHIFT);
> + rpn = __pa(uaddr) >> tceshift;
> + *tcep = cpu_to_be64(proto_tce | rpn << tceshift);
>
> - uaddr += TCE_PAGE_SIZE;
> + uaddr += pagesize;
> tcep++;
> }
> return 0;
> @@ -146,7 +148,7 @@ static unsigned long tce_get_pseries(struct iommu_table *tbl, long index)
> return be64_to_cpu(*tcep);
> }
>
> -static void tce_free_pSeriesLP(unsigned long liobn, long, long);
> +static void tce_free_pSeriesLP(unsigned long liobn, long, long, long);
> static void tce_freemulti_pSeriesLP(struct iommu_table*, long, long);
>
> static int tce_build_pSeriesLP(unsigned long liobn, long tcenum, long tceshift,
> @@ -166,12 +168,12 @@ static int tce_build_pSeriesLP(unsigned long liobn, long tcenum, long tceshift,
> proto_tce |= TCE_PCI_WRITE;
>
> while (npages--) {
> - tce = proto_tce | (rpn & TCE_RPN_MASK) << tceshift;
> + tce = proto_tce | rpn << tceshift;
> rc = plpar_tce_put((u64)liobn, (u64)tcenum << tceshift, tce);
>
> if (unlikely(rc == H_NOT_ENOUGH_RESOURCES)) {
> ret = (int)rc;
> - tce_free_pSeriesLP(liobn, tcenum_start,
> + tce_free_pSeriesLP(liobn, tcenum_start, tceshift,
> (npages_start - (npages + 1)));
> break;
> }
> @@ -205,10 +207,11 @@ static int tce_buildmulti_pSeriesLP(struct iommu_table *tbl, long tcenum,
> long tcenum_start = tcenum, npages_start = npages;
> int ret = 0;
> unsigned long flags;
> + const unsigned long tceshift = tbl->it_page_shift;
>
> if ((npages == 1) || !firmware_has_feature(FW_FEATURE_PUT_TCE_IND)) {
> return tce_build_pSeriesLP(tbl->it_index, tcenum,
> - tbl->it_page_shift, npages, uaddr,
> + tceshift, npages, uaddr,
> direction, attrs);
> }
>
> @@ -225,13 +228,13 @@ static int tce_buildmulti_pSeriesLP(struct iommu_table *tbl, long tcenum,
> if (!tcep) {
> local_irq_restore(flags);
> return tce_build_pSeriesLP(tbl->it_index, tcenum,
> - tbl->it_page_shift,
> + tceshift,
> npages, uaddr, direction, attrs);
> }
> __this_cpu_write(tce_page, tcep);
> }
>
> - rpn = __pa(uaddr) >> TCE_SHIFT;
> + rpn = __pa(uaddr) >> tceshift;
> proto_tce = TCE_PCI_READ;
> if (direction != DMA_TO_DEVICE)
> proto_tce |= TCE_PCI_WRITE;
> @@ -245,12 +248,12 @@ static int tce_buildmulti_pSeriesLP(struct iommu_table *tbl, long tcenum,
> limit = min_t(long, npages, 4096/TCE_ENTRY_SIZE);
>
> for (l = 0; l < limit; l++) {
> - tcep[l] = cpu_to_be64(proto_tce | (rpn & TCE_RPN_MASK) << TCE_RPN_SHIFT);
> + tcep[l] = cpu_to_be64(proto_tce | rpn << tceshift);
> rpn++;
> }
>
> rc = plpar_tce_put_indirect((u64)tbl->it_index,
> - (u64)tcenum << 12,
> + (u64)tcenum << tceshift,
> (u64)__pa(tcep),
> limit);
>
> @@ -277,12 +280,13 @@ static int tce_buildmulti_pSeriesLP(struct iommu_table *tbl, long tcenum,
> return ret;
> }
>
> -static void tce_free_pSeriesLP(unsigned long liobn, long tcenum, long npages)
> +static void tce_free_pSeriesLP(unsigned long liobn, long tcenum, long tceshift,
> + long npages)
> {
> u64 rc;
>
> while (npages--) {
> - rc = plpar_tce_put((u64)liobn, (u64)tcenum << 12, 0);
> + rc = plpar_tce_put((u64)liobn, (u64)tcenum << tceshift, 0);
>
> if (rc && printk_ratelimit()) {
> printk("tce_free_pSeriesLP: plpar_tce_put failed. rc=%lld\n", rc);
> @@ -301,9 +305,11 @@ static void tce_freemulti_pSeriesLP(struct iommu_table *tbl, long tcenum, long n
> u64 rc;
>
> if (!firmware_has_feature(FW_FEATURE_STUFF_TCE))
> - return tce_free_pSeriesLP(tbl->it_index, tcenum, npages);
> + return tce_free_pSeriesLP(tbl->it_index, tcenum,
> + tbl->it_page_shift, npages);
>
> - rc = plpar_tce_stuff((u64)tbl->it_index, (u64)tcenum << 12, 0, npages);
> + rc = plpar_tce_stuff((u64)tbl->it_index,
> + (u64)tcenum << tbl->it_page_shift, 0, npages);
>
> if (rc && printk_ratelimit()) {
> printk("tce_freemulti_pSeriesLP: plpar_tce_stuff failed\n");
> @@ -319,7 +325,8 @@ static unsigned long tce_get_pSeriesLP(struct iommu_table *tbl, long tcenum)
> u64 rc;
> unsigned long tce_ret;
>
> - rc = plpar_tce_get((u64)tbl->it_index, (u64)tcenum << 12, &tce_ret);
> + rc = plpar_tce_get((u64)tbl->it_index,
> + (u64)tcenum << tbl->it_page_shift, &tce_ret);
>
> if (rc && printk_ratelimit()) {
> printk("tce_get_pSeriesLP: plpar_tce_get failed. rc=%lld\n", rc);
>
^ permalink raw reply
* [PATCH] powerpc: use IRQF_NO_DEBUG for IPIs
From: Cédric Le Goater @ 2021-07-19 13:06 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Thomas Gleixner, Cédric Le Goater
There is no need to use the lockup detector ("noirqdebug") for IPIs.
The ipistorm benchmark measures a ~10% improvement on high systems
when this flag is set.
Cc: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
arch/powerpc/sysdev/xics/xics-common.c | 2 +-
arch/powerpc/sysdev/xive/common.c | 3 ++-
2 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/arch/powerpc/sysdev/xics/xics-common.c b/arch/powerpc/sysdev/xics/xics-common.c
index b14c502e56a8..18174ccefbc0 100644
--- a/arch/powerpc/sysdev/xics/xics-common.c
+++ b/arch/powerpc/sysdev/xics/xics-common.c
@@ -133,7 +133,7 @@ static void xics_request_ipi(void)
* IPIs are marked IRQF_PERCPU. The handler was set in map.
*/
BUG_ON(request_irq(ipi, icp_ops->ipi_action,
- IRQF_PERCPU | IRQF_NO_THREAD, "IPI", NULL));
+ IRQF_NO_DEBUG | IRQF_PERCPU | IRQF_NO_THREAD, "IPI", NULL));
}
void __init xics_smp_probe(void)
diff --git a/arch/powerpc/sysdev/xive/common.c b/arch/powerpc/sysdev/xive/common.c
index dbdbbc2f1dc5..9ab44d069704 100644
--- a/arch/powerpc/sysdev/xive/common.c
+++ b/arch/powerpc/sysdev/xive/common.c
@@ -1161,7 +1161,8 @@ static int __init xive_request_ipi(void)
snprintf(xid->name, sizeof(xid->name), "IPI-%d", node);
ret = request_irq(xid->irq, xive_muxed_ipi_action,
- IRQF_PERCPU | IRQF_NO_THREAD, xid->name, NULL);
+ IRQF_NO_DEBUG | IRQF_PERCPU | IRQF_NO_THREAD,
+ xid->name, NULL);
WARN(ret < 0, "Failed to request IPI %d: %d\n", xid->irq, ret);
}
--
2.31.1
^ permalink raw reply related
* [PATCH] PCI: Move pci_dev_is/assign_added() to pci.h
From: Niklas Schnelle @ 2021-07-19 12:11 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: linux-arch, linux-pci, linuxppc-dev, linux-kernel, linux-s390
The helper function pci_dev_is_added() from drivers/pci/pci.h is used in
PCI arch code of both s390 and powerpc leading to awkward relative
includes. Move it to the global include/linux/pci.h and get rid of these
includes just for that one function.
Signed-off-by: Niklas Schnelle <schnelle@linux.ibm.com>
---
arch/powerpc/platforms/powernv/pci-sriov.c | 3 ---
arch/powerpc/platforms/pseries/setup.c | 1 -
arch/s390/pci/pci_sysfs.c | 2 --
drivers/pci/hotplug/acpiphp_glue.c | 1 -
drivers/pci/pci.h | 15 ---------------
include/linux/pci.h | 13 +++++++++++++
6 files changed, 13 insertions(+), 22 deletions(-)
diff --git a/arch/powerpc/platforms/powernv/pci-sriov.c b/arch/powerpc/platforms/powernv/pci-sriov.c
index 28aac933a439..2e0ca5451e85 100644
--- a/arch/powerpc/platforms/powernv/pci-sriov.c
+++ b/arch/powerpc/platforms/powernv/pci-sriov.c
@@ -9,9 +9,6 @@
#include "pci.h"
-/* for pci_dev_is_added() */
-#include "../../../../drivers/pci/pci.h"
-
/*
* The majority of the complexity in supporting SR-IOV on PowerNV comes from
* the need to put the MMIO space for each VF into a separate PE. Internally
diff --git a/arch/powerpc/platforms/pseries/setup.c b/arch/powerpc/platforms/pseries/setup.c
index 631a0d57b6cd..17585ec9f955 100644
--- a/arch/powerpc/platforms/pseries/setup.c
+++ b/arch/powerpc/platforms/pseries/setup.c
@@ -74,7 +74,6 @@
#include <asm/hvconsole.h>
#include "pseries.h"
-#include "../../../../drivers/pci/pci.h"
DEFINE_STATIC_KEY_FALSE(shared_processor);
EXPORT_SYMBOL_GPL(shared_processor);
diff --git a/arch/s390/pci/pci_sysfs.c b/arch/s390/pci/pci_sysfs.c
index 6e2450c2b9c1..8dbe54ef8f8e 100644
--- a/arch/s390/pci/pci_sysfs.c
+++ b/arch/s390/pci/pci_sysfs.c
@@ -13,8 +13,6 @@
#include <linux/stat.h>
#include <linux/pci.h>
-#include "../../../drivers/pci/pci.h"
-
#include <asm/sclp.h>
#define zpci_attr(name, fmt, member) \
diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c
index f031302ad401..4cb963f88183 100644
--- a/drivers/pci/hotplug/acpiphp_glue.c
+++ b/drivers/pci/hotplug/acpiphp_glue.c
@@ -38,7 +38,6 @@
#include <linux/slab.h>
#include <linux/acpi.h>
-#include "../pci.h"
#include "acpiphp.h"
static LIST_HEAD(bridge_list);
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 93dcdd431072..a159cd0f6f05 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -383,21 +383,6 @@ static inline bool pci_dev_is_disconnected(const struct pci_dev *dev)
return dev->error_state == pci_channel_io_perm_failure;
}
-/* pci_dev priv_flags */
-#define PCI_DEV_ADDED 0
-#define PCI_DPC_RECOVERED 1
-#define PCI_DPC_RECOVERING 2
-
-static inline void pci_dev_assign_added(struct pci_dev *dev, bool added)
-{
- assign_bit(PCI_DEV_ADDED, &dev->priv_flags, added);
-}
-
-static inline bool pci_dev_is_added(const struct pci_dev *dev)
-{
- return test_bit(PCI_DEV_ADDED, &dev->priv_flags);
-}
-
#ifdef CONFIG_PCIEAER
#include <linux/aer.h>
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 540b377ca8f6..b3b7bafa17e5 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -507,6 +507,19 @@ struct pci_dev {
unsigned long priv_flags; /* Private flags for the PCI driver */
};
+/* pci_dev priv_flags */
+#define PCI_DEV_ADDED 0
+
+static inline void pci_dev_assign_added(struct pci_dev *dev, bool added)
+{
+ assign_bit(PCI_DEV_ADDED, &dev->priv_flags, added);
+}
+
+static inline bool pci_dev_is_added(const struct pci_dev *dev)
+{
+ return test_bit(PCI_DEV_ADDED, &dev->priv_flags);
+}
+
static inline struct pci_dev *pci_physfn(struct pci_dev *dev)
{
#ifdef CONFIG_PCI_IOV
--
2.25.1
^ permalink raw reply related
* [PATCH v2] soc: fsl: qe: convert QE interrupt controller to platform_device
From: Maxim Kochetkov @ 2021-07-19 10:18 UTC (permalink / raw)
To: linuxppc-dev
Cc: kernel test robot, saravanak, gregkh, linux-kernel, leoyang.li,
Maxim Kochetkov, Dan Carpenter, linux-arm-kernel, qiang.zhao
Since 5.13 QE's ucc nodes can't get interrupts from devicetree:
ucc@2000 {
cell-index = <1>;
reg = <0x2000 0x200>;
interrupts = <32>;
interrupt-parent = <&qeic>;
};
Now fw_devlink expects driver to create and probe a struct device
for interrupt controller.
So lets convert this driver to simple platform_device with probe().
Also use platform_get_ and devm_ family function to get/allocate
resources and drop unused .compatible = "qeic".
[1] - https://lore.kernel.org/lkml/CAGETcx9PiX==mLxB9PO8Myyk6u2vhPVwTMsA5NkD-ywH5xhusw@mail.gmail.com
Fixes: e590474768f1 ("driver core: Set fw_devlink=on by default")
Fixes: ea718c699055 ("Revert "Revert "driver core: Set fw_devlink=on by default""")
Signed-off-by: Maxim Kochetkov <fido_max@inbox.ru>
Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
---
Changes in v2:
- use devm_ family functions to allocate mem/resources
- use platform_get_ family functions to get resources/irqs
- drop unused .compatible = "qeic"
drivers/soc/fsl/qe/qe_ic.c | 74 ++++++++++++++++++++++----------------
1 file changed, 43 insertions(+), 31 deletions(-)
diff --git a/drivers/soc/fsl/qe/qe_ic.c b/drivers/soc/fsl/qe/qe_ic.c
index 3f711c1a0996..07db977df349 100644
--- a/drivers/soc/fsl/qe/qe_ic.c
+++ b/drivers/soc/fsl/qe/qe_ic.c
@@ -23,6 +23,7 @@
#include <linux/signal.h>
#include <linux/device.h>
#include <linux/spinlock.h>
+#include <linux/platform_device.h>
#include <asm/irq.h>
#include <asm/io.h>
#include <soc/fsl/qe/qe.h>
@@ -404,41 +405,40 @@ static void qe_ic_cascade_muxed_mpic(struct irq_desc *desc)
chip->irq_eoi(&desc->irq_data);
}
-static void __init qe_ic_init(struct device_node *node)
+static int qe_ic_init(struct platform_device *pdev)
{
+ struct device *dev = &pdev->dev;
void (*low_handler)(struct irq_desc *desc);
void (*high_handler)(struct irq_desc *desc);
struct qe_ic *qe_ic;
- struct resource res;
- u32 ret;
+ struct resource *res;
+ struct device_node *node = pdev->dev.of_node;
- ret = of_address_to_resource(node, 0, &res);
- if (ret)
- return;
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ if (res == NULL) {
+ dev_err(dev, "no memory resource defined\n");
+ return -ENODEV;
+ }
- qe_ic = kzalloc(sizeof(*qe_ic), GFP_KERNEL);
+ qe_ic = devm_kzalloc(dev, sizeof(*qe_ic), GFP_KERNEL);
if (qe_ic == NULL)
- return;
+ return -ENOMEM;
- qe_ic->irqhost = irq_domain_add_linear(node, NR_QE_IC_INTS,
- &qe_ic_host_ops, qe_ic);
- if (qe_ic->irqhost == NULL) {
- kfree(qe_ic);
- return;
+ qe_ic->regs = devm_ioremap(dev, res->start, resource_size(res));
+ if (qe_ic->regs == NULL) {
+ dev_err(dev, "failed to ioremap() registers\n");
+ return -ENODEV;
}
- qe_ic->regs = ioremap(res.start, resource_size(&res));
-
qe_ic->hc_irq = qe_ic_irq_chip;
- qe_ic->virq_high = irq_of_parse_and_map(node, 0);
- qe_ic->virq_low = irq_of_parse_and_map(node, 1);
+ qe_ic->virq_high = platform_get_irq(pdev, 0);
+ qe_ic->virq_low = platform_get_irq(pdev, 1);
- if (!qe_ic->virq_low) {
- printk(KERN_ERR "Failed to map QE_IC low IRQ\n");
- kfree(qe_ic);
- return;
+ if (qe_ic->virq_low < 0) {
+ return -ENODEV;
}
+
if (qe_ic->virq_high != qe_ic->virq_low) {
low_handler = qe_ic_cascade_low;
high_handler = qe_ic_cascade_high;
@@ -447,6 +447,13 @@ static void __init qe_ic_init(struct device_node *node)
high_handler = NULL;
}
+ qe_ic->irqhost = irq_domain_add_linear(node, NR_QE_IC_INTS,
+ &qe_ic_host_ops, qe_ic);
+ if (qe_ic->irqhost == NULL) {
+ dev_err(dev, "failed to add irq domain\n");
+ return -ENODEV;
+ }
+
qe_ic_write(qe_ic->regs, QEIC_CICR, 0);
irq_set_handler_data(qe_ic->virq_low, qe_ic);
@@ -456,20 +463,25 @@ static void __init qe_ic_init(struct device_node *node)
irq_set_handler_data(qe_ic->virq_high, qe_ic);
irq_set_chained_handler(qe_ic->virq_high, high_handler);
}
+ return 0;
}
+static const struct of_device_id qe_ic_ids[] = {
+ { .compatible = "fsl,qe-ic"},
+ {},
+};
-static int __init qe_ic_of_init(void)
+static struct platform_driver qe_ic_driver =
{
- struct device_node *np;
+ .driver = {
+ .name = "qe-ic",
+ .of_match_table = qe_ic_ids,
+ },
+ .probe = qe_ic_init,
+};
- np = of_find_compatible_node(NULL, NULL, "fsl,qe-ic");
- if (!np) {
- np = of_find_node_by_type(NULL, "qeic");
- if (!np)
- return -ENODEV;
- }
- qe_ic_init(np);
- of_node_put(np);
+static int __init qe_ic_of_init(void)
+{
+ platform_driver_register(&qe_ic_driver);
return 0;
}
subsys_initcall(qe_ic_of_init);
--
2.31.1
^ permalink raw reply related
* [PATCH v5 0/1] Interface to represent PAPR firmware attributes
From: Pratik R. Sampat @ 2021-07-19 9:32 UTC (permalink / raw)
To: mpe, benh, paulus, linuxppc-dev, kvm-ppc, linux-kernel, psampat,
pratik.r.sampat
RFC: https://lkml.org/lkml/2021/6/4/791
PATCH v1: https://lkml.org/lkml/2021/6/16/805
PATCH v2: https://lkml.org/lkml/2021/7/6/138
PATCH v3: https://lkml.org/lkml/2021/7/12/2799
PATCH v4: https://lkml.org/lkml/2021/7/16/532
Changelog v4 --> v5
Based on comments from Fabiano
1. Cleaned up unused/redundant headers
2. Make "add_attr_group" use MAX_ATTRS instead of paramterizing "len"
3. Cleanup previous "pgs[idx].pg.attrs" allocations on failiure
4. Replaced strlen call with strnlen
5. Cleanup of pgs structures for "num_attrs" instead of "MAX_ATTRS"
Also, have implemented a POC using this interface for the powerpc-utils'
ppc64_cpu --frequency command-line tool to utilize this information
in userspace.
The POC for the new interface has been hosted here:
https://github.com/pratiksampat/powerpc-utils/tree/H_GET_ENERGY_SCALE_INFO_v2
Sample output from the powerpc-utils tool is as follows:
# ppc64_cpu --frequency
Power and Performance Mode: XXXX
Idle Power Saver Status : XXXX
Processor Folding Status : XXXX --> Printed if Idle power save status is supported
Platform reported frequencies --> Frequencies reported from the platform's H_CALL i.e PAPR interface
min : NNNN GHz
max : NNNN GHz
static : NNNN GHz
Tool Computed frequencies
min : NNNN GHz (cpu XX)
max : NNNN GHz (cpu XX)
avg : NNNN GHz
Pratik R. Sampat (1):
powerpc/pseries: Interface to represent PAPR firmware attributes
.../sysfs-firmware-papr-energy-scale-info | 26 ++
arch/powerpc/include/asm/hvcall.h | 24 +-
arch/powerpc/kvm/trace_hv.h | 1 +
arch/powerpc/platforms/pseries/Makefile | 3 +-
.../pseries/papr_platform_attributes.c | 317 ++++++++++++++++++
5 files changed, 369 insertions(+), 2 deletions(-)
create mode 100644 Documentation/ABI/testing/sysfs-firmware-papr-energy-scale-info
create mode 100644 arch/powerpc/platforms/pseries/papr_platform_attributes.c
--
2.31.1
^ permalink raw reply
* [PATCH v5 1/1] powerpc/pseries: Interface to represent PAPR firmware attributes
From: Pratik R. Sampat @ 2021-07-19 9:32 UTC (permalink / raw)
To: mpe, benh, paulus, linuxppc-dev, kvm-ppc, linux-kernel, psampat,
pratik.r.sampat
In-Reply-To: <20210719093250.41405-1-psampat@linux.ibm.com>
Adds a generic interface to represent the energy and frequency related
PAPR attributes on the system using the new H_CALL
"H_GET_ENERGY_SCALE_INFO".
H_GET_EM_PARMS H_CALL was previously responsible for exporting this
information in the lparcfg, however the H_GET_EM_PARMS H_CALL
will be deprecated P10 onwards.
The H_GET_ENERGY_SCALE_INFO H_CALL is of the following call format:
hcall(
uint64 H_GET_ENERGY_SCALE_INFO, // Get energy scale info
uint64 flags, // Per the flag request
uint64 firstAttributeId,// The attribute id
uint64 bufferAddress, // Guest physical address of the output buffer
uint64 bufferSize // The size in bytes of the output buffer
);
This H_CALL can query either all the attributes at once with
firstAttributeId = 0, flags = 0 as well as query only one attribute
at a time with firstAttributeId = id, flags = 1.
The output buffer consists of the following
1. number of attributes - 8 bytes
2. array offset to the data location - 8 bytes
3. version info - 1 byte
4. A data array of size num attributes, which contains the following:
a. attribute ID - 8 bytes
b. attribute value in number - 8 bytes
c. attribute name in string - 64 bytes
d. attribute value in string - 64 bytes
The new H_CALL exports information in direct string value format, hence
a new interface has been introduced in
/sys/firmware/papr/energy_scale_info to export this information to
userspace in an extensible pass-through format.
The H_CALL returns the name, numeric value and string value (if exists)
The format of exposing the sysfs information is as follows:
/sys/firmware/papr/energy_scale_info/
|-- <id>/
|-- desc
|-- value
|-- value_desc (if exists)
|-- <id>/
|-- desc
|-- value
|-- value_desc (if exists)
...
The energy information that is exported is useful for userspace tools
such as powerpc-utils. Currently these tools infer the
"power_mode_data" value in the lparcfg, which in turn is obtained from
the to be deprecated H_GET_EM_PARMS H_CALL.
On future platforms, such userspace utilities will have to look at the
data returned from the new H_CALL being populated in this new sysfs
interface and report this information directly without the need of
interpretation.
Signed-off-by: Pratik R. Sampat <psampat@linux.ibm.com>
Reviewed-by: Gautham R. Shenoy <ego@linux.vnet.ibm.com>
---
.../sysfs-firmware-papr-energy-scale-info | 26 ++
arch/powerpc/include/asm/hvcall.h | 24 +-
arch/powerpc/kvm/trace_hv.h | 1 +
arch/powerpc/platforms/pseries/Makefile | 3 +-
.../pseries/papr_platform_attributes.c | 317 ++++++++++++++++++
5 files changed, 369 insertions(+), 2 deletions(-)
create mode 100644 Documentation/ABI/testing/sysfs-firmware-papr-energy-scale-info
create mode 100644 arch/powerpc/platforms/pseries/papr_platform_attributes.c
diff --git a/Documentation/ABI/testing/sysfs-firmware-papr-energy-scale-info b/Documentation/ABI/testing/sysfs-firmware-papr-energy-scale-info
new file mode 100644
index 000000000000..139a576c7c9d
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-firmware-papr-energy-scale-info
@@ -0,0 +1,26 @@
+What: /sys/firmware/papr/energy_scale_info
+Date: June 2021
+Contact: Linux for PowerPC mailing list <linuxppc-dev@ozlabs.org>
+Description: Directory hosting a set of platform attributes like
+ energy/frequency on Linux running as a PAPR guest.
+
+ Each file in a directory contains a platform
+ attribute hierarchy pertaining to performance/
+ energy-savings mode and processor frequency.
+
+What: /sys/firmware/papr/energy_scale_info/<id>
+ /sys/firmware/papr/energy_scale_info/<id>/desc
+ /sys/firmware/papr/energy_scale_info/<id>/value
+ /sys/firmware/papr/energy_scale_info/<id>/value_desc
+Date: June 2021
+Contact: Linux for PowerPC mailing list <linuxppc-dev@ozlabs.org>
+Description: Energy, frequency attributes directory for POWERVM servers
+
+ This directory provides energy, frequency, folding information. It
+ contains below sysfs attributes:
+
+ - desc: String description of the attribute <id>
+
+ - value: Numeric value of attribute <id>
+
+ - value_desc: String value of attribute <id>
diff --git a/arch/powerpc/include/asm/hvcall.h b/arch/powerpc/include/asm/hvcall.h
index e3b29eda8074..c91714ea6719 100644
--- a/arch/powerpc/include/asm/hvcall.h
+++ b/arch/powerpc/include/asm/hvcall.h
@@ -316,7 +316,8 @@
#define H_SCM_PERFORMANCE_STATS 0x418
#define H_RPT_INVALIDATE 0x448
#define H_SCM_FLUSH 0x44C
-#define MAX_HCALL_OPCODE H_SCM_FLUSH
+#define H_GET_ENERGY_SCALE_INFO 0x450
+#define MAX_HCALL_OPCODE H_GET_ENERGY_SCALE_INFO
/* Scope args for H_SCM_UNBIND_ALL */
#define H_UNBIND_SCOPE_ALL (0x1)
@@ -631,6 +632,27 @@ struct hv_gpci_request_buffer {
uint8_t bytes[HGPCI_MAX_DATA_BYTES];
} __packed;
+#define ESI_VERSION 0x1
+#define MAX_ESI_ATTRS 10
+#define MAX_BUF_SZ (sizeof(struct h_energy_scale_info_hdr) + \
+ (sizeof(struct energy_scale_attribute) * MAX_ESI_ATTRS))
+
+struct energy_scale_attribute {
+ __be64 id;
+ __be64 value;
+ unsigned char desc[64];
+ unsigned char value_desc[64];
+} __packed;
+
+struct h_energy_scale_info_hdr {
+ __be64 num_attrs;
+ __be64 array_offset;
+ __u8 data_header_version;
+} __packed;
+
+/* /sys/firmware/papr */
+extern struct kobject *papr_kobj;
+
#endif /* __ASSEMBLY__ */
#endif /* __KERNEL__ */
#endif /* _ASM_POWERPC_HVCALL_H */
diff --git a/arch/powerpc/kvm/trace_hv.h b/arch/powerpc/kvm/trace_hv.h
index 830a126e095d..38cd0ed0a617 100644
--- a/arch/powerpc/kvm/trace_hv.h
+++ b/arch/powerpc/kvm/trace_hv.h
@@ -115,6 +115,7 @@
{H_VASI_STATE, "H_VASI_STATE"}, \
{H_ENABLE_CRQ, "H_ENABLE_CRQ"}, \
{H_GET_EM_PARMS, "H_GET_EM_PARMS"}, \
+ {H_GET_ENERGY_SCALE_INFO, "H_GET_ENERGY_SCALE_INFO"}, \
{H_SET_MPP, "H_SET_MPP"}, \
{H_GET_MPP, "H_GET_MPP"}, \
{H_HOME_NODE_ASSOCIATIVITY, "H_HOME_NODE_ASSOCIATIVITY"}, \
diff --git a/arch/powerpc/platforms/pseries/Makefile b/arch/powerpc/platforms/pseries/Makefile
index c8a2b0b05ac0..d14fca89ac25 100644
--- a/arch/powerpc/platforms/pseries/Makefile
+++ b/arch/powerpc/platforms/pseries/Makefile
@@ -6,7 +6,8 @@ obj-y := lpar.o hvCall.o nvram.o reconfig.o \
of_helpers.o \
setup.o iommu.o event_sources.o ras.o \
firmware.o power.o dlpar.o mobility.o rng.o \
- pci.o pci_dlpar.o eeh_pseries.o msi.o
+ pci.o pci_dlpar.o eeh_pseries.o msi.o \
+ papr_platform_attributes.o
obj-$(CONFIG_SMP) += smp.o
obj-$(CONFIG_SCANLOG) += scanlog.o
obj-$(CONFIG_KEXEC_CORE) += kexec.o
diff --git a/arch/powerpc/platforms/pseries/papr_platform_attributes.c b/arch/powerpc/platforms/pseries/papr_platform_attributes.c
new file mode 100644
index 000000000000..52a14997fafc
--- /dev/null
+++ b/arch/powerpc/platforms/pseries/papr_platform_attributes.c
@@ -0,0 +1,317 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Platform energy and frequency attributes driver
+ *
+ * This driver creates a sys file at /sys/firmware/papr/ which encapsulates a
+ * directory structure containing files in keyword - value pairs that specify
+ * energy and frequency configuration of the system.
+ *
+ * The format of exposing the sysfs information is as follows:
+ * /sys/firmware/papr/energy_scale_info/
+ * |-- <id>/
+ * |-- desc
+ * |-- value
+ * |-- value_desc (if exists)
+ * |-- <id>/
+ * |-- desc
+ * |-- value
+ * |-- value_desc (if exists)
+ *
+ * Copyright 2021 IBM Corp.
+ */
+
+#include <asm/hvcall.h>
+#include <asm/machdep.h>
+
+#include "pseries.h"
+
+/*
+ * Flag attributes to fetch either all or one attribute from the HCALL
+ * flag = BE(0) => fetch all attributes with firstAttributeId = 0
+ * flag = BE(1) => fetch a single attribute with firstAttributeId = id
+ */
+#define ESI_FLAGS_ALL 0
+#define ESI_FLAGS_SINGLE PPC_BIT(0)
+
+#define MAX_ATTRS 3
+
+struct papr_attr {
+ u64 id;
+ struct kobj_attribute kobj_attr;
+};
+struct papr_group {
+ struct attribute_group pg;
+ struct papr_attr pgattrs[MAX_ATTRS];
+} *pgs;
+
+/* /sys/firmware/papr */
+struct kobject *papr_kobj;
+/* /sys/firmware/papr/energy_scale_info */
+struct kobject *esi_kobj;
+
+/*
+ * Extract and export the description of the energy scale attributes
+ */
+static ssize_t papr_show_desc(struct kobject *kobj,
+ struct kobj_attribute *kobj_attr,
+ char *buf)
+{
+ struct papr_attr *pattr = container_of(kobj_attr, struct papr_attr,
+ kobj_attr);
+ struct h_energy_scale_info_hdr *t_hdr;
+ struct energy_scale_attribute *t_esi;
+ char *t_buf;
+ int ret = 0;
+
+ t_buf = kmalloc(MAX_BUF_SZ, GFP_KERNEL);
+ if (t_buf == NULL)
+ return -ENOMEM;
+
+ ret = plpar_hcall_norets(H_GET_ENERGY_SCALE_INFO, ESI_FLAGS_SINGLE,
+ pattr->id, virt_to_phys(t_buf),
+ MAX_BUF_SZ);
+
+ if (ret != H_SUCCESS) {
+ pr_warn("hcall failed: H_GET_ENERGY_SCALE_INFO");
+ goto out;
+ }
+
+ t_hdr = (struct h_energy_scale_info_hdr *) t_buf;
+ t_esi = (struct energy_scale_attribute *)
+ (t_buf + be64_to_cpu(t_hdr->array_offset));
+
+ ret = snprintf(buf, sizeof(t_esi->desc), "%s\n", t_esi->desc);
+ if (ret < 0)
+ ret = -EIO;
+out:
+ kfree(t_buf);
+
+ return ret;
+}
+
+/*
+ * Extract and export the numeric value of the energy scale attributes
+ */
+static ssize_t papr_show_value(struct kobject *kobj,
+ struct kobj_attribute *kobj_attr,
+ char *buf)
+{
+ struct papr_attr *pattr = container_of(kobj_attr, struct papr_attr,
+ kobj_attr);
+ struct h_energy_scale_info_hdr *t_hdr;
+ struct energy_scale_attribute *t_esi;
+ char *t_buf;
+ int ret = 0;
+
+ t_buf = kmalloc(MAX_BUF_SZ, GFP_KERNEL);
+ if (t_buf == NULL)
+ return -ENOMEM;
+
+ ret = plpar_hcall_norets(H_GET_ENERGY_SCALE_INFO, ESI_FLAGS_SINGLE,
+ pattr->id, virt_to_phys(t_buf),
+ MAX_BUF_SZ);
+
+ if (ret != H_SUCCESS) {
+ pr_warn("hcall failed: H_GET_ENERGY_SCALE_INFO");
+ goto out;
+ }
+
+ t_hdr = (struct h_energy_scale_info_hdr *) t_buf;
+ t_esi = (struct energy_scale_attribute *)
+ (t_buf + be64_to_cpu(t_hdr->array_offset));
+
+ ret = snprintf(buf, sizeof(t_esi->value), "%llu\n",
+ be64_to_cpu(t_esi->value));
+ if (ret < 0)
+ ret = -EIO;
+out:
+ kfree(t_buf);
+
+ return ret;
+}
+
+/*
+ * Extract and export the value description in string format of the energy
+ * scale attributes
+ */
+static ssize_t papr_show_value_desc(struct kobject *kobj,
+ struct kobj_attribute *kobj_attr,
+ char *buf)
+{
+ struct papr_attr *pattr = container_of(kobj_attr, struct papr_attr,
+ kobj_attr);
+ struct h_energy_scale_info_hdr *t_hdr;
+ struct energy_scale_attribute *t_esi;
+ char *t_buf;
+ int ret = 0;
+
+ t_buf = kmalloc(MAX_BUF_SZ, GFP_KERNEL);
+ if (t_buf == NULL)
+ return -ENOMEM;
+
+ ret = plpar_hcall_norets(H_GET_ENERGY_SCALE_INFO, ESI_FLAGS_SINGLE,
+ pattr->id, virt_to_phys(t_buf),
+ MAX_BUF_SZ);
+
+ if (ret != H_SUCCESS) {
+ pr_warn("hcall failed: H_GET_ENERGY_SCALE_INFO");
+ goto out;
+ }
+
+ t_hdr = (struct h_energy_scale_info_hdr *) t_buf;
+ t_esi = (struct energy_scale_attribute *)
+ (t_buf + be64_to_cpu(t_hdr->array_offset));
+
+ ret = snprintf(buf, sizeof(t_esi->value_desc), "%s\n",
+ t_esi->value_desc);
+ if (ret < 0)
+ ret = -EIO;
+out:
+ kfree(t_buf);
+
+ return ret;
+}
+
+static struct papr_ops_info {
+ const char *attr_name;
+ ssize_t (*show)(struct kobject *kobj, struct kobj_attribute *kobj_attr,
+ char *buf);
+} ops_info[MAX_ATTRS] = {
+ { "desc", papr_show_desc },
+ { "value", papr_show_value },
+ { "value_desc", papr_show_value_desc },
+};
+
+static void add_attr(u64 id, int index, struct papr_attr *attr)
+{
+ attr->id = id;
+ sysfs_attr_init(&attr->kobj_attr.attr);
+ attr->kobj_attr.attr.name = ops_info[index].attr_name;
+ attr->kobj_attr.attr.mode = 0444;
+ attr->kobj_attr.show = ops_info[index].show;
+}
+
+static int add_attr_group(u64 id, struct papr_group *pg, bool show_val_desc)
+{
+ int i;
+
+ for (i = 0; i < MAX_ATTRS; i++) {
+ if (!strcmp(ops_info[i].attr_name, "value_desc") &&
+ !show_val_desc) {
+ continue;
+ }
+ add_attr(id, i, &pg->pgattrs[i]);
+ pg->pg.attrs[i] = &pg->pgattrs[i].kobj_attr.attr;
+ }
+
+ return sysfs_create_group(esi_kobj, &pg->pg);
+}
+
+static int __init papr_init(void)
+{
+ struct h_energy_scale_info_hdr *esi_hdr;
+ struct energy_scale_attribute *esi_attrs;
+ uint64_t num_attrs;
+ int ret, idx, i;
+ char *esi_buf;
+
+ if (!firmware_has_feature(FW_FEATURE_LPAR))
+ return -ENXIO;
+
+ esi_buf = kmalloc(MAX_BUF_SZ, GFP_KERNEL);
+ if (esi_buf == NULL)
+ return -ENOMEM;
+ /*
+ * hcall(
+ * uint64 H_GET_ENERGY_SCALE_INFO, // Get energy scale info
+ * uint64 flags, // Per the flag request
+ * uint64 firstAttributeId, // The attribute id
+ * uint64 bufferAddress, // Guest physical address of the output buffer
+ * uint64 bufferSize); // The size in bytes of the output buffer
+ */
+ ret = plpar_hcall_norets(H_GET_ENERGY_SCALE_INFO, ESI_FLAGS_ALL, 0,
+ virt_to_phys(esi_buf), MAX_BUF_SZ);
+ if (ret != H_SUCCESS) {
+ pr_warn("hcall failed: H_GET_ENERGY_SCALE_INFO");
+ goto out;
+ }
+
+ esi_hdr = (struct h_energy_scale_info_hdr *) esi_buf;
+ if (esi_hdr->data_header_version != ESI_VERSION) {
+ pr_warn("H_GET_ENERGY_SCALE_INFO VER MISMATCH - EXP: 0x%x, REC: 0x%x",
+ ESI_VERSION, esi_hdr->data_header_version);
+ }
+
+ num_attrs = be64_to_cpu(esi_hdr->num_attrs);
+ esi_attrs = (struct energy_scale_attribute *)
+ (esi_buf + be64_to_cpu(esi_hdr->array_offset));
+
+ pgs = kcalloc(num_attrs, sizeof(*pgs), GFP_KERNEL);
+ if (!pgs)
+ goto out;
+
+ papr_kobj = kobject_create_and_add("papr", firmware_kobj);
+ if (!papr_kobj) {
+ pr_warn("kobject_create_and_add papr failed\n");
+ goto out_pgs;
+ }
+
+ esi_kobj = kobject_create_and_add("energy_scale_info", papr_kobj);
+ if (!esi_kobj) {
+ pr_warn("kobject_create_and_add energy_scale_info failed\n");
+ goto out_kobj;
+ }
+
+ for (idx = 0; idx < num_attrs; idx++) {
+ bool show_val_desc = true;
+
+ pgs[idx].pg.attrs = kcalloc(MAX_ATTRS + 1,
+ sizeof(*pgs[idx].pg.attrs),
+ GFP_KERNEL);
+ if (!pgs[idx].pg.attrs) {
+ for (i = idx - 1; i >= 0; i--)
+ kfree(pgs[i].pg.attrs);
+ goto out_ekobj;
+ }
+
+ pgs[idx].pg.name = kasprintf(GFP_KERNEL, "%lld",
+ be64_to_cpu(esi_attrs[idx].id));
+ if (pgs[idx].pg.name == NULL) {
+ for (i = idx; i >= 0; i--)
+ kfree(pgs[i].pg.attrs);
+ goto out_ekobj;
+ }
+ /* Do not add the value description if it does not exist */
+ if (strnlen(esi_attrs[idx].value_desc,
+ sizeof(esi_attrs[idx].value_desc)) == 0)
+ show_val_desc = false;
+
+ if (add_attr_group(be64_to_cpu(esi_attrs[idx].id), &pgs[idx],
+ show_val_desc)) {
+ pr_warn("Failed to create papr attribute group %s\n",
+ pgs[idx].pg.name);
+ goto out_pgattrs;
+ }
+ }
+
+ kfree(esi_buf);
+ return 0;
+
+out_pgattrs:
+ for (i = 0; i < num_attrs ; i++) {
+ kfree(pgs[i].pg.attrs);
+ kfree(pgs[i].pg.name);
+ }
+out_ekobj:
+ kobject_put(esi_kobj);
+out_kobj:
+ kobject_put(papr_kobj);
+out_pgs:
+ kfree(pgs);
+out:
+ kfree(esi_buf);
+
+ return -ENOMEM;
+}
+
+machine_device_initcall(pseries, papr_init);
--
2.31.1
^ permalink raw reply related
* Re: [PATCH v5] pseries: prevent free CPU ids to be reused on another node
From: Laurent Dufour @ 2021-07-19 9:13 UTC (permalink / raw)
To: mpe, benh, paulus; +Cc: nathanl, linuxppc-dev, linux-kernel
In-Reply-To: <20210429174908.16613-1-ldufour@linux.ibm.com>
Hi Michael,
Is there a way to get that patch in 5.14?
Thanks,
Laurent.
Le 29/04/2021 à 19:49, Laurent Dufour a écrit :
> When a CPU is hot added, the CPU ids are taken from the available mask from
> the lower possible set. If that set of values was previously used for CPU
> attached to a different node, this seems to application like if these CPUs
> have migrated from a node to another one which is not expected in real
> life.
>
> To prevent this, it is needed to record the CPU ids used for each node and
> to not reuse them on another node. However, to prevent CPU hot plug to
> fail, in the case the CPU ids is starved on a node, the capability to reuse
> other nodes’ free CPU ids is kept. A warning is displayed in such a case
> to warn the user.
>
> A new CPU bit mask (node_recorded_ids_map) is introduced for each possible
> node. It is populated with the CPU onlined at boot time, and then when a
> CPU is hot plug to a node. The bits in that mask remain when the CPU is hot
> unplugged, to remind this CPU ids have been used for this node.
>
> If no id set was found, a retry is made without removing the ids used on
> the other nodes to try reusing them. This is the way ids have been
> allocated prior to this patch.
>
> The effect of this patch can be seen by removing and adding CPUs using the
> Qemu monitor. In the following case, the first CPU from the node 2 is
> removed, then the first one from the node 1 is removed too. Later, the
> first CPU of the node 2 is added back. Without that patch, the kernel will
> numbered these CPUs using the first CPU ids available which are the ones
> freed when removing the second CPU of the node 0. This leads to the CPU ids
> 16-23 to move from the node 1 to the node 2. With the patch applied, the
> CPU ids 32-39 are used since they are the lowest free ones which have not
> been used on another node.
>
> At boot time:
> [root@vm40 ~]# numactl -H | grep cpus
> node 0 cpus: 0 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15
> node 1 cpus: 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31
> node 2 cpus: 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47
>
> Vanilla kernel, after the CPU hot unplug/plug operations:
> [root@vm40 ~]# numactl -H | grep cpus
> node 0 cpus: 0 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15
> node 1 cpus: 24 25 26 27 28 29 30 31
> node 2 cpus: 16 17 18 19 20 21 22 23 40 41 42 43 44 45 46 47
>
> Patched kernel, after the CPU hot unplug/plug operations:
> [root@vm40 ~]# numactl -H | grep cpus
> node 0 cpus: 0 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15
> node 1 cpus: 24 25 26 27 28 29 30 31
> node 2 cpus: 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47
>
> Signed-off-by: Laurent Dufour <ldufour@linux.ibm.com>
> ---
> V5:
> - Rework code structure
> - Reintroduce the capability to reuse other node's ids.
> V4: addressing Nathan's comment
> - Rename the local variable named 'nid' into 'assigned_node'
> V3: addressing Nathan's comments
> - Remove the retry feature
> - Reduce the number of local variables (removing 'i')
> - Add comment about the cpu_add_remove_lock protecting the added CPU mask.
> V2: (no functional changes)
> - update the test's output in the commit's description
> - node_recorded_ids_map should be static
> ---
> arch/powerpc/platforms/pseries/hotplug-cpu.c | 171 ++++++++++++++-----
> 1 file changed, 132 insertions(+), 39 deletions(-)
>
> diff --git a/arch/powerpc/platforms/pseries/hotplug-cpu.c b/arch/powerpc/platforms/pseries/hotplug-cpu.c
> index 7e970f81d8ff..e1f224320102 100644
> --- a/arch/powerpc/platforms/pseries/hotplug-cpu.c
> +++ b/arch/powerpc/platforms/pseries/hotplug-cpu.c
> @@ -39,6 +39,12 @@
> /* This version can't take the spinlock, because it never returns */
> static int rtas_stop_self_token = RTAS_UNKNOWN_SERVICE;
>
> +/*
> + * Record the CPU ids used on each nodes.
> + * Protected by cpu_add_remove_lock.
> + */
> +static cpumask_var_t node_recorded_ids_map[MAX_NUMNODES];
> +
> static void rtas_stop_self(void)
> {
> static struct rtas_args args;
> @@ -139,72 +145,148 @@ static void pseries_cpu_die(unsigned int cpu)
> paca_ptrs[cpu]->cpu_start = 0;
> }
>
> +/**
> + * find_cpu_id_range - found a linear ranger of @nthreads free CPU ids.
> + * @nthreads : the number of threads (cpu ids)
> + * @assigned_node : the node it belongs to or NUMA_NO_NODE if free ids from any
> + * node can be peek.
> + * @cpu_mask: the returned CPU mask.
> + *
> + * Returns 0 on success.
> + */
> +static int find_cpu_id_range(unsigned int nthreads, int assigned_node,
> + cpumask_var_t *cpu_mask)
> +{
> + cpumask_var_t candidate_mask;
> + unsigned int cpu, node;
> + int rc = -ENOSPC;
> +
> + if (!zalloc_cpumask_var(&candidate_mask, GFP_KERNEL))
> + return -ENOMEM;
> +
> + cpumask_clear(*cpu_mask);
> + for (cpu = 0; cpu < nthreads; cpu++)
> + cpumask_set_cpu(cpu, *cpu_mask);
> +
> + BUG_ON(!cpumask_subset(cpu_present_mask, cpu_possible_mask));
> +
> + /* Get a bitmap of unoccupied slots. */
> + cpumask_xor(candidate_mask, cpu_possible_mask, cpu_present_mask);
> +
> + if (assigned_node != NUMA_NO_NODE) {
> + /*
> + * Remove free ids previously assigned on the other nodes. We
> + * can walk only online nodes because once a node became online
> + * it is not turned offlined back.
> + */
> + for_each_online_node(node) {
> + if (node == assigned_node)
> + continue;
> + cpumask_andnot(candidate_mask, candidate_mask,
> + node_recorded_ids_map[node]);
> + }
> + }
> +
> + if (cpumask_empty(candidate_mask))
> + goto out;
> +
> + while (!cpumask_empty(*cpu_mask)) {
> + if (cpumask_subset(*cpu_mask, candidate_mask))
> + /* Found a range where we can insert the new cpu(s) */
> + break;
> + cpumask_shift_left(*cpu_mask, *cpu_mask, nthreads);
> + }
> +
> + if (!cpumask_empty(*cpu_mask))
> + rc = 0;
> +
> +out:
> + free_cpumask_var(candidate_mask);
> + return rc;
> +}
> +
> /*
> * Update cpu_present_mask and paca(s) for a new cpu node. The wrinkle
> - * here is that a cpu device node may represent up to two logical cpus
> + * here is that a cpu device node may represent multiple logical cpus
> * in the SMT case. We must honor the assumption in other code that
> * the logical ids for sibling SMT threads x and y are adjacent, such
> * that x^1 == y and y^1 == x.
> */
> static int pseries_add_processor(struct device_node *np)
> {
> - unsigned int cpu;
> - cpumask_var_t candidate_mask, tmp;
> - int err = -ENOSPC, len, nthreads, i;
> + int len, nthreads, node, cpu, assigned_node;
> + int rc = 0;
> + cpumask_var_t cpu_mask;
> const __be32 *intserv;
>
> intserv = of_get_property(np, "ibm,ppc-interrupt-server#s", &len);
> if (!intserv)
> return 0;
>
> - zalloc_cpumask_var(&candidate_mask, GFP_KERNEL);
> - zalloc_cpumask_var(&tmp, GFP_KERNEL);
> -
> nthreads = len / sizeof(u32);
> - for (i = 0; i < nthreads; i++)
> - cpumask_set_cpu(i, tmp);
>
> - cpu_maps_update_begin();
> + if (!alloc_cpumask_var(&cpu_mask, GFP_KERNEL))
> + return -ENOMEM;
>
> - BUG_ON(!cpumask_subset(cpu_present_mask, cpu_possible_mask));
> + /*
> + * Fetch from the DT nodes read by dlpar_configure_connector() the NUMA
> + * node id the added CPU belongs to.
> + */
> + node = of_node_to_nid(np);
> + if (node < 0 || !node_possible(node))
> + node = first_online_node;
>
> - /* Get a bitmap of unoccupied slots. */
> - cpumask_xor(candidate_mask, cpu_possible_mask, cpu_present_mask);
> - if (cpumask_empty(candidate_mask)) {
> - /* If we get here, it most likely means that NR_CPUS is
> - * less than the partition's max processors setting.
> + BUG_ON(node == NUMA_NO_NODE);
> + assigned_node = node;
> +
> + cpu_maps_update_begin();
> +
> + rc = find_cpu_id_range(nthreads, node, &cpu_mask);
> + if (rc && nr_node_ids > 1) {
> + /*
> + * Try again, considering the free CPU ids from the other node.
> */
> - printk(KERN_ERR "Cannot add cpu %pOF; this system configuration"
> - " supports %d logical cpus.\n", np,
> - num_possible_cpus());
> - goto out_unlock;
> + node = NUMA_NO_NODE;
> + rc = find_cpu_id_range(nthreads, NUMA_NO_NODE, &cpu_mask);
> }
>
> - while (!cpumask_empty(tmp))
> - if (cpumask_subset(tmp, candidate_mask))
> - /* Found a range where we can insert the new cpu(s) */
> - break;
> - else
> - cpumask_shift_left(tmp, tmp, nthreads);
> -
> - if (cpumask_empty(tmp)) {
> - printk(KERN_ERR "Unable to find space in cpu_present_mask for"
> - " processor %pOFn with %d thread(s)\n", np,
> - nthreads);
> - goto out_unlock;
> + if (rc) {
> + pr_err("Cannot add cpu %pOF; this system configuration"
> + " supports %d logical cpus.\n", np, num_possible_cpus());
> + goto out;
> }
>
> - for_each_cpu(cpu, tmp) {
> + for_each_cpu(cpu, cpu_mask) {
> BUG_ON(cpu_present(cpu));
> set_cpu_present(cpu, true);
> set_hard_smp_processor_id(cpu, be32_to_cpu(*intserv++));
> }
> - err = 0;
> -out_unlock:
> +
> + /* Record the newly used CPU ids for the associate node. */
> + cpumask_or(node_recorded_ids_map[assigned_node],
> + node_recorded_ids_map[assigned_node], cpu_mask);
> +
> + /*
> + * If node is set to NUMA_NO_NODE, CPU ids have be reused from
> + * another node, remove them from its mask.
> + */
> + if (node == NUMA_NO_NODE) {
> + cpu = cpumask_first(cpu_mask);
> + pr_warn("Reusing free CPU ids %d-%d from another node\n",
> + cpu, cpu + nthreads - 1);
> + for_each_online_node(node) {
> + if (node == assigned_node)
> + continue;
> + cpumask_andnot(node_recorded_ids_map[node],
> + node_recorded_ids_map[node],
> + cpu_mask);
> + }
> + }
> +
> +out:
> cpu_maps_update_done();
> - free_cpumask_var(candidate_mask);
> - free_cpumask_var(tmp);
> - return err;
> + free_cpumask_var(cpu_mask);
> + return rc;
> }
>
> /*
> @@ -908,6 +990,7 @@ static struct notifier_block pseries_smp_nb = {
> static int __init pseries_cpu_hotplug_init(void)
> {
> int qcss_tok;
> + unsigned int node;
>
> #ifdef CONFIG_ARCH_CPU_PROBE_RELEASE
> ppc_md.cpu_probe = dlpar_cpu_probe;
> @@ -929,8 +1012,18 @@ static int __init pseries_cpu_hotplug_init(void)
> smp_ops->cpu_die = pseries_cpu_die;
>
> /* Processors can be added/removed only on LPAR */
> - if (firmware_has_feature(FW_FEATURE_LPAR))
> + if (firmware_has_feature(FW_FEATURE_LPAR)) {
> + for_each_node(node) {
> + alloc_bootmem_cpumask_var(&node_recorded_ids_map[node]);
> +
> + /* Record ids of CPU added at boot time */
> + cpumask_or(node_recorded_ids_map[node],
> + node_recorded_ids_map[node],
> + node_to_cpumask_map[node]);
> + }
> +
> of_reconfig_notifier_register(&pseries_smp_nb);
> + }
>
> return 0;
> }
>
^ permalink raw reply
* Re: [PATCH v2] ppc64/numa: consider the max numa node for migratable LPAR
From: Laurent Dufour @ 2021-07-19 9:12 UTC (permalink / raw)
To: mpe, benh, paulus; +Cc: nathanl, linuxppc-dev, linux-kernel, Srikar Dronamraju
In-Reply-To: <20210511073136.17795-1-ldufour@linux.ibm.com>
Hi Michael,
Is there a way to get that patch in 5.14?
Thanks,
Laurent.
Le 11/05/2021 à 09:31, Laurent Dufour a écrit :
> When a LPAR is migratable, we should consider the maximum possible NUMA
> node instead the number of NUMA node from the actual system.
>
> The DT property 'ibm,current-associativity-domains' is defining the maximum
> number of nodes the LPAR can see when running on that box. But if the LPAR
> is being migrated on another box, it may seen up to the nodes defined by
> 'ibm,max-associativity-domains'. So if a LPAR is migratable, that value
> should be used.
>
> Unfortunately, there is no easy way to know if a LPAR is migratable or
> not. The hypervisor is exporting the property 'ibm,migratable-partition' in
> the case it set to migrate partition, but that would not mean that the
> current partition is migratable.
>
> Without this patch, when a LPAR is started on a 2 nodes box and then
> migrated to a 3 nodes box, the hypervisor may spread the LPAR's CPUs on the
> 3rd node. In that case if a CPU from that 3rd node is added to the LPAR, it
> will be wrongly assigned to the node because the kernel has been set to use
> up to 2 nodes (the configuration of the departure node). With this patch
> applies, the CPU is correctly added to the 3rd node.
>
> Fixes: f9f130ff2ec9 ("powerpc/numa: Detect support for coregroup")
> Reviewed-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
> Signed-off-by: Laurent Dufour <ldufour@linux.ibm.com>
> ---
> V2: Address Srikar's comments
> - Fix the commit message
> - Use pr_info instead printk(KERN_INFO..)
> ---
> arch/powerpc/mm/numa.c | 13 ++++++++++---
> 1 file changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/arch/powerpc/mm/numa.c b/arch/powerpc/mm/numa.c
> index f2bf98bdcea2..094a1076fd1f 100644
> --- a/arch/powerpc/mm/numa.c
> +++ b/arch/powerpc/mm/numa.c
> @@ -893,7 +893,7 @@ static void __init setup_node_data(int nid, u64 start_pfn, u64 end_pfn)
> static void __init find_possible_nodes(void)
> {
> struct device_node *rtas;
> - const __be32 *domains;
> + const __be32 *domains = NULL;
> int prop_length, max_nodes;
> u32 i;
>
> @@ -909,9 +909,14 @@ static void __init find_possible_nodes(void)
> * it doesn't exist, then fallback on ibm,max-associativity-domains.
> * Current denotes what the platform can support compared to max
> * which denotes what the Hypervisor can support.
> + *
> + * If the LPAR is migratable, new nodes might be activated after a LPM,
> + * so we should consider the max number in that case.
> */
> - domains = of_get_property(rtas, "ibm,current-associativity-domains",
> - &prop_length);
> + if (!of_get_property(of_root, "ibm,migratable-partition", NULL))
> + domains = of_get_property(rtas,
> + "ibm,current-associativity-domains",
> + &prop_length);
> if (!domains) {
> domains = of_get_property(rtas, "ibm,max-associativity-domains",
> &prop_length);
> @@ -920,6 +925,8 @@ static void __init find_possible_nodes(void)
> }
>
> max_nodes = of_read_number(&domains[min_common_depth], 1);
> + pr_info("Partition configured for %d NUMA nodes.\n", max_nodes);
> +
> for (i = 0; i < max_nodes; i++) {
> if (!node_possible(i))
> node_set(i, node_possible_map);
>
^ permalink raw reply
* Re: [PATCH v5] pseries/drmem: update LMBs after LPM
From: Laurent Dufour @ 2021-07-19 9:10 UTC (permalink / raw)
To: mpe, benh, paulus
Cc: Nathan Lynch, Aneesh Kumar K . V, linuxppc-dev, linux-kernel,
Tyrel Datwyler
In-Reply-To: <20210517090606.56930-1-ldufour@linux.ibm.com>
Hi Michael,
Is there a way to get that patch in 5.14?
Thanks,
Laurent.
Le 17/05/2021 à 11:06, Laurent Dufour a écrit :
> After a LPM, the device tree node ibm,dynamic-reconfiguration-memory may be
> updated by the hypervisor in the case the NUMA topology of the LPAR's
> memory is updated.
>
> This is handled by the kernel, but the memory's node is not updated because
> there is no way to move a memory block between nodes from the Linux kernel
> point of view.
>
> If later a memory block is added or removed, drmem_update_dt() is called
> and it is overwriting the DT node ibm,dynamic-reconfiguration-memory to
> match the added or removed LMB. But the LMB's associativity node has not
> been updated after the DT node update and thus the node is overwritten by
> the Linux's topology instead of the hypervisor one.
>
> Introduce a hook called when the ibm,dynamic-reconfiguration-memory node is
> updated to force an update of the LMB's associativity. However, ignore the
> call to that hook when the update has been triggered by drmem_update_dt().
> Because, in that case, the LMB tree has been used to set the DT property
> and thus it doesn't need to be updated back. Since drmem_update_dt() is
> called under the protection of the device_hotplug_lock and the hook is
> called in the same context, use a simple boolean variable to detect that
> call.
>
> Cc: Nathan Lynch <nathanl@linux.ibm.com>
> Cc: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
> Cc: Tyrel Datwyler <tyreld@linux.ibm.com>
> Signed-off-by: Laurent Dufour <ldufour@linux.ibm.com>
> ---
>
> V5:
> - Reword the commit's description to address Nathan's comments.
> V4:
> - Prevent the LMB to be updated back in the case the request came from the
> LMB tree's update.
> V3:
> - Check rd->dn->name instead of rd->dn->full_name
> V2:
> - Take Tyrel's idea to rely on OF_RECONFIG_UPDATE_PROPERTY instead of
> introducing a new hook mechanism.
> ---
> arch/powerpc/include/asm/drmem.h | 1 +
> arch/powerpc/mm/drmem.c | 46 +++++++++++++++++++
> .../platforms/pseries/hotplug-memory.c | 4 ++
> 3 files changed, 51 insertions(+)
>
> diff --git a/arch/powerpc/include/asm/drmem.h b/arch/powerpc/include/asm/drmem.h
> index bf2402fed3e0..4265d5e95c2c 100644
> --- a/arch/powerpc/include/asm/drmem.h
> +++ b/arch/powerpc/include/asm/drmem.h
> @@ -111,6 +111,7 @@ int drmem_update_dt(void);
> int __init
> walk_drmem_lmbs_early(unsigned long node, void *data,
> int (*func)(struct drmem_lmb *, const __be32 **, void *));
> +void drmem_update_lmbs(struct property *prop);
> #endif
>
> static inline void invalidate_lmb_associativity_index(struct drmem_lmb *lmb)
> diff --git a/arch/powerpc/mm/drmem.c b/arch/powerpc/mm/drmem.c
> index 9af3832c9d8d..22197b18d85e 100644
> --- a/arch/powerpc/mm/drmem.c
> +++ b/arch/powerpc/mm/drmem.c
> @@ -18,6 +18,7 @@ static int n_root_addr_cells, n_root_size_cells;
>
> static struct drmem_lmb_info __drmem_info;
> struct drmem_lmb_info *drmem_info = &__drmem_info;
> +static bool in_drmem_update;
>
> u64 drmem_lmb_memory_max(void)
> {
> @@ -178,6 +179,11 @@ int drmem_update_dt(void)
> if (!memory)
> return -1;
>
> + /*
> + * Set in_drmem_update to prevent the notifier callback to process the
> + * DT property back since the change is coming from the LMB tree.
> + */
> + in_drmem_update = true;
> prop = of_find_property(memory, "ibm,dynamic-memory", NULL);
> if (prop) {
> rc = drmem_update_dt_v1(memory, prop);
> @@ -186,6 +192,7 @@ int drmem_update_dt(void)
> if (prop)
> rc = drmem_update_dt_v2(memory, prop);
> }
> + in_drmem_update = false;
>
> of_node_put(memory);
> return rc;
> @@ -307,6 +314,45 @@ int __init walk_drmem_lmbs_early(unsigned long node, void *data,
> return ret;
> }
>
> +/*
> + * Update the LMB associativity index.
> + */
> +static int update_lmb(struct drmem_lmb *updated_lmb,
> + __maybe_unused const __be32 **usm,
> + __maybe_unused void *data)
> +{
> + struct drmem_lmb *lmb;
> +
> + for_each_drmem_lmb(lmb) {
> + if (lmb->drc_index != updated_lmb->drc_index)
> + continue;
> +
> + lmb->aa_index = updated_lmb->aa_index;
> + break;
> + }
> + return 0;
> +}
> +
> +/*
> + * Update the LMB associativity index.
> + *
> + * This needs to be called when the hypervisor is updating the
> + * dynamic-reconfiguration-memory node property.
> + */
> +void drmem_update_lmbs(struct property *prop)
> +{
> + /*
> + * Don't update the LMBs if triggered by the update done in
> + * drmem_update_dt(), the LMB values have been used to the update the DT
> + * property in that case.
> + */
> + if (in_drmem_update)
> + return;
> + if (!strcmp(prop->name, "ibm,dynamic-memory"))
> + __walk_drmem_v1_lmbs(prop->value, NULL, NULL, update_lmb);
> + else if (!strcmp(prop->name, "ibm,dynamic-memory-v2"))
> + __walk_drmem_v2_lmbs(prop->value, NULL, NULL, update_lmb);
> +}
> #endif
>
> static int init_drmem_lmb_size(struct device_node *dn)
> diff --git a/arch/powerpc/platforms/pseries/hotplug-memory.c b/arch/powerpc/platforms/pseries/hotplug-memory.c
> index 8377f1f7c78e..672ffbee2e78 100644
> --- a/arch/powerpc/platforms/pseries/hotplug-memory.c
> +++ b/arch/powerpc/platforms/pseries/hotplug-memory.c
> @@ -949,6 +949,10 @@ static int pseries_memory_notifier(struct notifier_block *nb,
> case OF_RECONFIG_DETACH_NODE:
> err = pseries_remove_mem_node(rd->dn);
> break;
> + case OF_RECONFIG_UPDATE_PROPERTY:
> + if (!strcmp(rd->dn->name,
> + "ibm,dynamic-reconfiguration-memory"))
> + drmem_update_lmbs(rd->prop);
> }
> return notifier_from_errno(err);
> }
>
^ permalink raw reply
* Re: [PATCH] powerpc/xive: Do not skip CPU-less nodes when creating the IPIs
From: Laurent Vivier @ 2021-07-19 7:34 UTC (permalink / raw)
To: Cédric Le Goater, linuxppc-dev
Cc: Geetika Moolchandani, Srikar Dronamraju, stable
In-Reply-To: <20210629131542.743888-1-clg@kaod.org>
On 29/06/2021 15:15, Cédric Le Goater wrote:
> On PowerVM, CPU-less nodes can be populated with hot-plugged CPUs at
> runtime. Today, the IPI is not created for such nodes, and hot-plugged
> CPUs use a bogus IPI, which leads to soft lockups.
>
> We could create the node IPI on demand but it is a bit complex because
> this code would be called under bringup_up() and some IRQ locking is
> being done. The simplest solution is to create the IPIs for all nodes
> at startup.
>
> Fixes: 7dcc37b3eff9 ("powerpc/xive: Map one IPI interrupt per node")
> Cc: stable@vger.kernel.org # v5.13
> Reported-by: Geetika Moolchandani <Geetika.Moolchandani1@ibm.com>
> Cc: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
>
> This patch breaks old versions of irqbalance (<= v1.4). Possible nodes
> are collected from /sys/devices/system/node/ but CPU-less nodes are
> not listed there. When interrupts are scanned, the link representing
> the node structure is NULL and segfault occurs.
>
> Version 1.7 seems immune.
>
> ---
> arch/powerpc/sysdev/xive/common.c | 4 ----
> 1 file changed, 4 deletions(-)
>
> diff --git a/arch/powerpc/sysdev/xive/common.c b/arch/powerpc/sysdev/xive/common.c
> index f3b16ed48b05..5d2c58dba57e 100644
> --- a/arch/powerpc/sysdev/xive/common.c
> +++ b/arch/powerpc/sysdev/xive/common.c
> @@ -1143,10 +1143,6 @@ static int __init xive_request_ipi(void)
> struct xive_ipi_desc *xid = &xive_ipis[node];
> struct xive_ipi_alloc_info info = { node };
>
> - /* Skip nodes without CPUs */
> - if (cpumask_empty(cpumask_of_node(node)))
> - continue;
> -
> /*
> * Map one IPI interrupt per node for all cpus of that node.
> * Since the HW interrupt number doesn't have any meaning,
>
What happened to this fix? Will it be merged?
Thanks,
Laurent
^ permalink raw reply
* Re: [PATCH] soc: fsl: qe: convert QE interrupt controller to platform_device
From: Maxim Kochetkov @ 2021-07-19 6:58 UTC (permalink / raw)
To: Li Yang
Cc: saravanak, Greg Kroah-Hartman, lkml, linuxppc-dev,
moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
Zhao Qiang
In-Reply-To: <CADRPPNRYDBFHEppfpYLwsy7MMEdtsOLS764MJboL9ERW0-KK3Q@mail.gmail.com>
15.07.2021 01:29, Li Yang wrote:
> From the original code, this should be type = "qeic". It is not
> defined in current binding but probably needed for backward
> compatibility.
I took these strings from this part:
np = of_find_compatible_node(NULL, NULL, "fsl,qe-ic");
if (!np) {
np = of_find_node_by_type(NULL, "qeic");
if (!np)
return -ENODEV;
}
However I can't find usage of "qeic" in any dts, so I will drop this in V2
^ permalink raw reply
* [PATCH v5 1/2] cpuidle/pseries: Fixup CEDE0 latency only for POWER10 onwards
From: Gautham R. Shenoy @ 2021-07-19 6:33 UTC (permalink / raw)
To: Rafael J. Wysocki, Daniel Lezcano, Michael Ellerman,
Aneesh Kumar K.V, Vaidyanathan Srinivasan, Michal Suchanek
Cc: Gautham R. Shenoy, linuxppc-dev, joedecke, linux-pm
In-Reply-To: <1626676399-15975-1-git-send-email-ego@linux.vnet.ibm.com>
From: "Gautham R. Shenoy" <ego@linux.vnet.ibm.com>
Commit d947fb4c965c ("cpuidle: pseries: Fixup exit latency for
CEDE(0)") sets the exit latency of CEDE(0) based on the latency values
of the Extended CEDE states advertised by the platform
On POWER9 LPARs, the firmwares advertise a very low value of 2us for
CEDE1 exit latency on a Dedicated LPAR. The latency advertized by the
PHYP hypervisor corresponds to the latency required to wakeup from the
underlying hardware idle state. However the wakeup latency from the
LPAR perspective should include
1. The time taken to transition the CPU from the Hypervisor into the
LPAR post wakeup from platform idle state
2. Time taken to send the IPI from the source CPU (waker) to the idle
target CPU (wakee).
1. can be measured via timer idle test, where we queue a timer, say
for 1ms, and enter the CEDE state. When the timer fires, in the timer
handler we compute how much extra timer over the expected 1ms have we
consumed. On a a POWER9 LPAR the numbers are
CEDE latency measured using a timer (numbers in ns)
N Min Median Avg 90%ile 99%ile Max Stddev
400 2601 5677 5668.74 5917 6413 9299 455.01
1. and 2. combined can be determined by an IPI latency test where we
send an IPI to an idle CPU and in the handler compute the time
difference between when the IPI was sent and when the handler ran. We
see the following numbers on POWER9 LPAR.
CEDE latency measured using an IPI (numbers in ns)
N Min Median Avg 90%ile 99%ile Max Stddev
400 711 7564 7369.43 8559 9514 9698 1200.01
Suppose, we consider the 99th percentile latency value measured using
the IPI to be the wakeup latency, the value would be 9.5us This is in
the ballpark of the default value of 10us.
Hence, use the exit latency of CEDE(0) based on the latency values
advertized by platform only from POWER10 onwards. The values
advertized on POWER10 platforms is more realistic and informed by the
latency measurements. For earlier platforms stick to the default value
of 10us. The fix was suggested by Michael Ellerman.
Reported-by: Enrico Joedecke <joedecke@de.ibm.com>
Fixes: commit d947fb4c965c ("cpuidle: pseries: Fixup exit latency for
CEDE(0)")
Cc: Michal Suchanek <msuchanek@suse.de>
Cc: Vaidyanathan Srinivasan <svaidy@linux.ibm.com>
Signed-off-by: Gautham R. Shenoy <ego@linux.vnet.ibm.com>
---
drivers/cpuidle/cpuidle-pseries.c | 16 +++++++++++++++-
1 file changed, 15 insertions(+), 1 deletion(-)
diff --git a/drivers/cpuidle/cpuidle-pseries.c b/drivers/cpuidle/cpuidle-pseries.c
index a2b5c6f..e592280d 100644
--- a/drivers/cpuidle/cpuidle-pseries.c
+++ b/drivers/cpuidle/cpuidle-pseries.c
@@ -419,7 +419,21 @@ static int pseries_idle_probe(void)
cpuidle_state_table = shared_states;
max_idle_state = ARRAY_SIZE(shared_states);
} else {
- fixup_cede0_latency();
+ /*
+ * Use firmware provided latency values
+ * starting with POWER10 platforms. In the
+ * case that we are running on a POWER10
+ * platform but in an earlier compat mode, we
+ * can still use the firmware provided values.
+ *
+ * However, on platforms prior to POWER10, we
+ * cannot rely on the accuracy of the firmware
+ * provided latency values. On such platforms,
+ * go with the conservative default estimate
+ * of 10us.
+ */
+ if (cpu_has_feature(CPU_FTR_ARCH_31) || pvr_version_is(PVR_POWER10))
+ fixup_cede0_latency();
cpuidle_state_table = dedicated_states;
max_idle_state = NR_DEDICATED_STATES;
}
--
1.9.4
^ permalink raw reply related
* [PATCH v5 2/2] cpuidle/pseries: Do not cap the CEDE0 latency in fixup_cede0_latency()
From: Gautham R. Shenoy @ 2021-07-19 6:33 UTC (permalink / raw)
To: Rafael J. Wysocki, Daniel Lezcano, Michael Ellerman,
Aneesh Kumar K.V, Vaidyanathan Srinivasan, Michal Suchanek
Cc: Gautham R. Shenoy, linuxppc-dev, joedecke, linux-pm
In-Reply-To: <1626676399-15975-1-git-send-email-ego@linux.vnet.ibm.com>
From: "Gautham R. Shenoy" <ego@linux.vnet.ibm.com>
Currently in fixup_cede0_latency() code, we perform the fixup the
CEDE(0) exit latency value only if minimum advertized extended CEDE
latency values are less than 10us. This was done so as to not break
the expected behaviour on POWER8 platforms where the advertised
latency was higher than the default 10us, which would delay the SMT
folding on the core.
However, after the earlier patch "cpuidle/pseries: Fixup CEDE0 latency
only for POWER10 onwards", we can be sure that the fixup of CEDE0
latency is going to happen only from POWER10 onwards. Hence
unconditionally use the minimum exit latency provided by the platform.
Signed-off-by: Gautham R. Shenoy <ego@linux.vnet.ibm.com>
---
drivers/cpuidle/cpuidle-pseries.c | 59 ++++++++++++++++++++-------------------
1 file changed, 30 insertions(+), 29 deletions(-)
diff --git a/drivers/cpuidle/cpuidle-pseries.c b/drivers/cpuidle/cpuidle-pseries.c
index e592280d..bba449b 100644
--- a/drivers/cpuidle/cpuidle-pseries.c
+++ b/drivers/cpuidle/cpuidle-pseries.c
@@ -346,11 +346,9 @@ static int pseries_cpuidle_driver_init(void)
static void __init fixup_cede0_latency(void)
{
struct xcede_latency_payload *payload;
- u64 min_latency_us;
+ u64 min_xcede_latency_us = UINT_MAX;
int i;
- min_latency_us = dedicated_states[1].exit_latency; // CEDE latency
-
if (parse_cede_parameters())
return;
@@ -358,42 +356,45 @@ static void __init fixup_cede0_latency(void)
nr_xcede_records);
payload = &xcede_latency_parameter.payload;
+
+ /*
+ * The CEDE idle state maps to CEDE(0). While the hypervisor
+ * does not advertise CEDE(0) exit latency values, it does
+ * advertise the latency values of the extended CEDE states.
+ * We use the lowest advertised exit latency value as a proxy
+ * for the exit latency of CEDE(0).
+ */
for (i = 0; i < nr_xcede_records; i++) {
struct xcede_latency_record *record = &payload->records[i];
+ u8 hint = record->hint;
u64 latency_tb = be64_to_cpu(record->latency_ticks);
u64 latency_us = DIV_ROUND_UP_ULL(tb_to_ns(latency_tb), NSEC_PER_USEC);
- if (latency_us == 0)
- pr_warn("cpuidle: xcede record %d has an unrealistic latency of 0us.\n", i);
-
- if (latency_us < min_latency_us)
- min_latency_us = latency_us;
- }
-
- /*
- * By default, we assume that CEDE(0) has exit latency 10us,
- * since there is no way for us to query from the platform.
- *
- * However, if the wakeup latency of an Extended CEDE state is
- * smaller than 10us, then we can be sure that CEDE(0)
- * requires no more than that.
- *
- * Perform the fix-up.
- */
- if (min_latency_us < dedicated_states[1].exit_latency) {
/*
- * We set a minimum of 1us wakeup latency for cede0 to
- * distinguish it from snooze
+ * We expect the exit latency of an extended CEDE
+ * state to be non-zero, it to since it takes at least
+ * a few nanoseconds to wakeup the idle CPU and
+ * dispatch the virtual processor into the Linux
+ * Guest.
+ *
+ * So we consider only non-zero value for performing
+ * the fixup of CEDE(0) latency.
*/
- u64 cede0_latency = 1;
+ if (latency_us == 0) {
+ pr_warn("cpuidle: Skipping xcede record %d [hint=%d]. Exit latency = 0us\n",
+ i, hint);
+ continue;
+ }
- if (min_latency_us > cede0_latency)
- cede0_latency = min_latency_us - 1;
+ if (latency_us < min_xcede_latency_us)
+ min_xcede_latency_us = latency_us;
+ }
- dedicated_states[1].exit_latency = cede0_latency;
- dedicated_states[1].target_residency = 10 * (cede0_latency);
+ if (min_xcede_latency_us != UINT_MAX) {
+ dedicated_states[1].exit_latency = min_xcede_latency_us;
+ dedicated_states[1].target_residency = 10 * (min_xcede_latency_us);
pr_info("cpuidle: Fixed up CEDE exit latency to %llu us\n",
- cede0_latency);
+ min_xcede_latency_us);
}
}
--
1.9.4
^ permalink raw reply related
* [PATCH v5 0/2] cpuidle/pseries: cleanup of the CEDE0 latency fixup code
From: Gautham R. Shenoy @ 2021-07-19 6:33 UTC (permalink / raw)
To: Rafael J. Wysocki, Daniel Lezcano, Michael Ellerman,
Aneesh Kumar K.V, Vaidyanathan Srinivasan, Michal Suchanek
Cc: Gautham R. Shenoy, linuxppc-dev, joedecke, linux-pm
From: "Gautham R. Shenoy" <ego@linux.vnet.ibm.com>
Hi,
This is the v5 of the patchset to fixup CEDE0 latency only from
POWER10 onwards.
The previous versions of the patches are
v4 : https://lore.kernel.org/linux-pm/1623048014-16451-1-git-send-email-ego@linux.vnet.ibm.com/
v3 : https://lore.kernel.org/linuxppc-dev/1619697982-28461-1-git-send-email-ego@linux.vnet.ibm.com/
v2 : https://lore.kernel.org/linuxppc-dev/1619673517-10853-1-git-send-email-ego@linux.vnet.ibm.com/
v1 : https://lore.kernel.org/linuxppc-dev/1619104049-5118-1-git-send-email-ego@linux.vnet.ibm.com/
v4 --> v5 changes
* Patch 1 : Unchanged. Rebased it against the latest powerpc/merge
tree. With this patch, on processors older than POWER10, the CEDE
latency is set to the hardcoded value of 10us which is closer to the
measured value (details of the measurement in Patch 1).
* Added a Patch 2/2 titled "cpuidle/pseries: Do not cap the CEDE0
latency in fixup_cede0_latency()" which will ensure that on POWER10
onwards we simply take the latency value exposed by the firmware
without keeping an upper cap of 10us. This upper cap was previously
required to prevent regression on POWER8 which had advertized
latency values higher than 10us while the measured values were
lesser. With Patch 1, we don't need the upper cap any longer.
Tested the series on POWER8, POWER9 and POWER10 with the
cpuidle-smt-performance test case
(https://github.com/gautshen/misc/tree/master/cpuidle-smt-performance) .
This test has three classes of threads
1. Workload thread which computes fibonacci numbers (Pinned to the
primary thread CPU 8). We are interested in the throughput of this
workload thread.
2. Three irritator threads which are pinned to the secondary CPUs of
the core on which the workload thread is running (CPUs 10, 12,
14). These irritators block on a pipe until woken up by a
waker. After being woken up, they again go back to sleep by
blocking on a pipe read. We are interested in the wakeup latency of
the irritator threads.
3. A waker thread which, pinned to a different core (CPU 16) from
where the workload and the irritators are running, periodically
wakes up the three irritator threads by writing to their respective
pipes. The purpose of these periodic wakeups is to prime the
cpuidle governor on the irritator CPUs to pick the idle state the
wakeup period.
We measure the wakeup latency of the irritator threads, which tells us
the impact of entering a particular combinations of idle states. Thus
shallower the state, lower should be the wakeup latency.
We also measure the throughput of the fibonacci workload to measure
the single-thread performance in the presence of the waking irritators
on the sibling threads. Entering an idle state which performs SMT
folding should show greater throughput.
There is no observable difference in the behaviour on POWER8 and
POWER10 with and without the patch series, since the CEDE latencies on
both of them with and without the patch are 10us.
However, on POWER9, without the patch, the CEDE latency is 1us based
on the value returned by the firmware (which is not accurate), while
with the patch it is set to the default value of 10us which is closer
to the accurate measure.
The throughput, wakeup latency, throughput and the snooze, CEDE idle
percentage residency results on POWER9 with and without patch are as
follows.
We observe that for a wakeup period between 20us - 100us, the wakeup
latency of the irritator threads with the patch improves by 40-45%.
Though note that with the patch, the throughput of the fibbonacci
workload drops by 5-10% when the wakeup period of the irritator
threads is between 20us-100us. This is an acceptable tradeoff since
there are certain benchmarks on POWER9 which are very sensitive to the
wakeup latency and have a sleeping duration of less than 100us.
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Avg Wakeup Latency of the irritator threads
(lower the better)
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Irritator | |
wakeup | Without | With
period | Patch | Patch
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
1 us | 3.703 us | 3.632 us ( -1.91%)
2 us | 3.843 us | 3.925 us ( +2.13%)
5 us | 8.575 us | 8.656 us ( +0.94%)
10 us | 8.264 us | 8.242 us ( -0.27%)
20 us | 8.672 us | 8.256 us ( -4.80%)
50 us | 15.552 us | 8.257 us (-46.90%)
80 us | 15.603 us | 8.803 us (-43.58%)
100 us | 15.617 us | 8.328 us (-46.67%)
120 us | 15.612 us | 14.505 us ( -7.09%)
500 us | 15.957 us | 15.723 us ( -1.47%)
1000 us | 16.526 us | 16.502 us ( -0.14%)
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Fibonacci workload throughput in Million Operations
per second (higher the better)
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Irritator | |
wakeup | Without | With
period | Patch | Patch
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
1 us | 44.234 Mops/s | 44.305 Mops/s ( +0.16%)
2 us | 44.290 Mops/s | 44.233 Mops/s ( -0.13%)
5 us | 44.757 Mops/s | 44.759 Mops/s ( -0.01%)
10 us | 46.169 Mops/s | 46.049 Mops/s ( -0.25%)
20 us | 48.263 Mops/s | 49.647 Mops/s ( +2.87%)
50 us | 52.817 Mops/s | 52.310 Mops/s ( -0.96%)
80 us | 57.338 Mops/s | 53.216 Mops/s ( -7.19%)
100 us | 58.958 Mops/s | 53.497 Mops/s ( -9.26%)
120 us | 60.060 Mops/s | 58.980 Mops/s ( -1.80%)
500 us | 64.484 Mops/s | 64.460 Mops/s ( -0.04%)
1000 us | 65.200 Mops/s | 65.188 Mops/s ( -0.02%)
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
(snooze, CEDE Residency Percentage)
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Irritator | |
wakeup | Without | With
period | Patch | Patch
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
1 us | ( 0.40%, 0.00%) | (0.28%, 0.00%)
2 us | ( 0.42%, 0.00%) | (0.33%, 0.00%)
5 us | ( 3.94%, 0.00%) | (3.89%, 0.00%)
10 us | (21.85%, 0.00%) | (21.62%, 0.00%)
20 us | (43.44%, 0.00%) | (50.90%, 0.00%)
50 us | ( 0.03%, 76.07%) | (76.85%, 0.00%)
80 us | ( 0.07%, 84.14%) | (84.85%, 0.00%)
100 us | ( 0.03%, 87.18%) | (87.61%, 0.02%)
120 us | ( 0.02%, 89.21%) | (14.71%, 74.40%)
500 us | ( 0.00%, 97.27%) | ( 3.70%, 93.53%
1000 us | ( 0.00%, 98.57%) | ( 0.17%, 98.40%)
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Gautham R. Shenoy (2):
cpuidle/pseries: Fixup CEDE0 latency only for POWER10 onwards
cpuidle/pseries: Do not cap the CEDE0 latency in fixup_cede0_latency()
drivers/cpuidle/cpuidle-pseries.c | 75 +++++++++++++++++++++++----------------
1 file changed, 45 insertions(+), 30 deletions(-)
--
1.9.4
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox