* [PATCH 2/3] ASoC: fsl_easrc: Fix -Wunused-but-set-variable
From: Shengjiu Wang @ 2020-06-03 3:39 UTC (permalink / raw)
To: timur, nicoleotsuka, Xiubo.Lee, festevam, broonie, alsa-devel,
lgirdwood, perex, tiwai
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <cover.1591155860.git.shengjiu.wang@nxp.com>
Obtained with:
$ make W=1
sound/soc/fsl/fsl_easrc.c: In function 'fsl_easrc_set_rs_ratio':
sound/soc/fsl/fsl_easrc.c:182:15: warning: variable 'int_bits' set but not used [-Wunused-but-set-variable]
unsigned int int_bits;
^
sound/soc/fsl/fsl_easrc.c: In function 'fsl_easrc_set_ctx_organziation':
sound/soc/fsl/fsl_easrc.c:1204:17: warning: variable 'dev' set but not used [-Wunused-but-set-variable]
struct device *dev;
^
sound/soc/fsl/fsl_easrc.c: In function 'fsl_easrc_release_context':
sound/soc/fsl/fsl_easrc.c:1294:17: warning: variable 'dev' set but not used [-Wunused-but-set-variable]
struct device *dev;
^
Fixes: 955ac624058f ("ASoC: fsl_easrc: Add EASRC ASoC CPU DAI drivers")
Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
Reported-by: kbuild test robot <lkp@intel.com>
---
sound/soc/fsl/fsl_easrc.c | 11 +++--------
1 file changed, 3 insertions(+), 8 deletions(-)
diff --git a/sound/soc/fsl/fsl_easrc.c b/sound/soc/fsl/fsl_easrc.c
index f227308a50e2..7d8bf9d47842 100644
--- a/sound/soc/fsl/fsl_easrc.c
+++ b/sound/soc/fsl/fsl_easrc.c
@@ -179,22 +179,21 @@ static int fsl_easrc_set_rs_ratio(struct fsl_asrc_pair *ctx)
struct fsl_easrc_ctx_priv *ctx_priv = ctx->private;
unsigned int in_rate = ctx_priv->in_params.norm_rate;
unsigned int out_rate = ctx_priv->out_params.norm_rate;
- unsigned int int_bits;
unsigned int frac_bits;
u64 val;
u32 *r;
switch (easrc_priv->rs_num_taps) {
case EASRC_RS_32_TAPS:
- int_bits = 5;
+ /* integer bits = 5; */
frac_bits = 39;
break;
case EASRC_RS_64_TAPS:
- int_bits = 6;
+ /* integer bits = 6; */
frac_bits = 38;
break;
case EASRC_RS_128_TAPS:
- int_bits = 7;
+ /* integer bits = 7; */
frac_bits = 37;
break;
default:
@@ -1201,7 +1200,6 @@ static int fsl_easrc_set_ctx_format(struct fsl_asrc_pair *ctx,
static int fsl_easrc_set_ctx_organziation(struct fsl_asrc_pair *ctx)
{
struct fsl_easrc_ctx_priv *ctx_priv;
- struct device *dev;
struct fsl_asrc *easrc;
if (!ctx)
@@ -1209,7 +1207,6 @@ static int fsl_easrc_set_ctx_organziation(struct fsl_asrc_pair *ctx)
easrc = ctx->asrc;
ctx_priv = ctx->private;
- dev = &easrc->pdev->dev;
/* input interleaving parameters */
regmap_update_bits(easrc->regmap, REG_EASRC_CIA(ctx->index),
@@ -1291,13 +1288,11 @@ static void fsl_easrc_release_context(struct fsl_asrc_pair *ctx)
{
unsigned long lock_flags;
struct fsl_asrc *easrc;
- struct device *dev;
if (!ctx)
return;
easrc = ctx->asrc;
- dev = &easrc->pdev->dev;
spin_lock_irqsave(&easrc->lock, lock_flags);
--
2.21.0
^ permalink raw reply related
* [PATCH 0/3] ASoC: fsl_easrc: Fix several warnings
From: Shengjiu Wang @ 2020-06-03 3:39 UTC (permalink / raw)
To: timur, nicoleotsuka, Xiubo.Lee, festevam, broonie, alsa-devel,
lgirdwood, perex, tiwai
Cc: linuxppc-dev, linux-kernel
Fix several warnings with "make W=1"
Shengjiu Wang (3):
ASoC: fsl_easrc: Fix -Wmissing-prototypes warning
ASoC: fsl_easrc: Fix -Wunused-but-set-variable
ASoC: fsl_easrc: Fix "Function parameter not described" warnings
sound/soc/fsl/fsl_easrc.c | 42 +++++++++++++++++----------------------
1 file changed, 18 insertions(+), 24 deletions(-)
--
2.21.0
^ permalink raw reply
* [PATCH 1/3] ASoC: fsl_easrc: Fix -Wmissing-prototypes warning
From: Shengjiu Wang @ 2020-06-03 3:39 UTC (permalink / raw)
To: timur, nicoleotsuka, Xiubo.Lee, festevam, broonie, alsa-devel,
lgirdwood, perex, tiwai
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <cover.1591155860.git.shengjiu.wang@nxp.com>
Obtained with:
$ make W=1
sound/soc/fsl/fsl_easrc.c:967:5: warning: no previous prototype for function 'fsl_easrc_config_context' [-Wmissing-prototypes]
int fsl_easrc_config_context(struct fsl_asrc *easrc, unsigned int ctx_id)
^
sound/soc/fsl/fsl_easrc.c:967:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
int fsl_easrc_config_context(struct fsl_asrc *easrc, unsigned int ctx_id)
^
static
sound/soc/fsl/fsl_easrc.c:1128:5: warning: no previous prototype for function 'fsl_easrc_set_ctx_format' [-Wmissing-prototypes]
int fsl_easrc_set_ctx_format(struct fsl_asrc_pair *ctx,
^
sound/soc/fsl/fsl_easrc.c:1128:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
int fsl_easrc_set_ctx_format(struct fsl_asrc_pair *ctx,
^
static
sound/soc/fsl/fsl_easrc.c:1201:5: warning: no previous prototype for function 'fsl_easrc_set_ctx_organziation' [-Wmissing-prototypes]
int fsl_easrc_set_ctx_organziation(struct fsl_asrc_pair *ctx)
^
sound/soc/fsl/fsl_easrc.c:1201:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
int fsl_easrc_set_ctx_organziation(struct fsl_asrc_pair *ctx)
^
static
sound/soc/fsl/fsl_easrc.c:1245:5: warning: no previous prototype for function 'fsl_easrc_request_context' [-Wmissing-prototypes]
int fsl_easrc_request_context(int channels, struct fsl_asrc_pair *ctx)
^
sound/soc/fsl/fsl_easrc.c:1245:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
int fsl_easrc_request_context(int channels, struct fsl_asrc_pair *ctx)
^
static
sound/soc/fsl/fsl_easrc.c:1290:6: warning: no previous prototype for function 'fsl_easrc_release_context' [-Wmissing-prototypes]
void fsl_easrc_release_context(struct fsl_asrc_pair *ctx)
^
sound/soc/fsl/fsl_easrc.c:1290:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
void fsl_easrc_release_context(struct fsl_asrc_pair *ctx)
^
static
sound/soc/fsl/fsl_easrc.c:1317:5: warning: no previous prototype for function 'fsl_easrc_start_context' [-Wmissing-prototypes]
int fsl_easrc_start_context(struct fsl_asrc_pair *ctx)
^
sound/soc/fsl/fsl_easrc.c:1317:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
int fsl_easrc_start_context(struct fsl_asrc_pair *ctx)
^
static
sound/soc/fsl/fsl_easrc.c:1335:5: warning: no previous prototype for function 'fsl_easrc_stop_context' [-Wmissing-prototypes]
int fsl_easrc_stop_context(struct fsl_asrc_pair *ctx)
^
sound/soc/fsl/fsl_easrc.c:1335:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
int fsl_easrc_stop_context(struct fsl_asrc_pair *ctx)
^
static
sound/soc/fsl/fsl_easrc.c:1382:18: warning: no previous prototype for function 'fsl_easrc_get_dma_channel' [-Wmissing-prototypes]
struct dma_chan *fsl_easrc_get_dma_channel(struct fsl_asrc_pair *ctx,
^
sound/soc/fsl/fsl_easrc.c:1382:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
struct dma_chan *fsl_easrc_get_dma_channel(struct fsl_asrc_pair *ctx,
^
static
Fixes: 955ac624058f ("ASoC: fsl_easrc: Add EASRC ASoC CPU DAI drivers")
Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
Reported-by: kbuild test robot <lkp@intel.com>
---
sound/soc/fsl/fsl_easrc.c | 23 +++++++++++------------
1 file changed, 11 insertions(+), 12 deletions(-)
diff --git a/sound/soc/fsl/fsl_easrc.c b/sound/soc/fsl/fsl_easrc.c
index c6b5eb2d2af7..f227308a50e2 100644
--- a/sound/soc/fsl/fsl_easrc.c
+++ b/sound/soc/fsl/fsl_easrc.c
@@ -964,7 +964,7 @@ static int fsl_easrc_release_slot(struct fsl_asrc *easrc, unsigned int ctx_id)
*
* Configure the register relate with context.
*/
-int fsl_easrc_config_context(struct fsl_asrc *easrc, unsigned int ctx_id)
+static int fsl_easrc_config_context(struct fsl_asrc *easrc, unsigned int ctx_id)
{
struct fsl_easrc_ctx_priv *ctx_priv;
struct fsl_asrc_pair *ctx;
@@ -1125,9 +1125,9 @@ static int fsl_easrc_process_format(struct fsl_asrc_pair *ctx,
return 0;
}
-int fsl_easrc_set_ctx_format(struct fsl_asrc_pair *ctx,
- snd_pcm_format_t *in_raw_format,
- snd_pcm_format_t *out_raw_format)
+static int fsl_easrc_set_ctx_format(struct fsl_asrc_pair *ctx,
+ snd_pcm_format_t *in_raw_format,
+ snd_pcm_format_t *out_raw_format)
{
struct fsl_asrc *easrc = ctx->asrc;
struct fsl_easrc_ctx_priv *ctx_priv = ctx->private;
@@ -1198,7 +1198,7 @@ int fsl_easrc_set_ctx_format(struct fsl_asrc_pair *ctx,
* to conform with this format. Interleaving parameters are accessed
* through the ASRC_CTRL_IN_ACCESSa and ASRC_CTRL_OUT_ACCESSa registers
*/
-int fsl_easrc_set_ctx_organziation(struct fsl_asrc_pair *ctx)
+static int fsl_easrc_set_ctx_organziation(struct fsl_asrc_pair *ctx)
{
struct fsl_easrc_ctx_priv *ctx_priv;
struct device *dev;
@@ -1242,7 +1242,7 @@ int fsl_easrc_set_ctx_organziation(struct fsl_asrc_pair *ctx)
* Returns a negative number on error and >=0 as context id
* on success
*/
-int fsl_easrc_request_context(int channels, struct fsl_asrc_pair *ctx)
+static int fsl_easrc_request_context(int channels, struct fsl_asrc_pair *ctx)
{
enum asrc_pair_index index = ASRC_INVALID_PAIR;
struct fsl_asrc *easrc = ctx->asrc;
@@ -1287,7 +1287,7 @@ int fsl_easrc_request_context(int channels, struct fsl_asrc_pair *ctx)
*
* This funciton is mainly doing the revert thing in request context
*/
-void fsl_easrc_release_context(struct fsl_asrc_pair *ctx)
+static void fsl_easrc_release_context(struct fsl_asrc_pair *ctx)
{
unsigned long lock_flags;
struct fsl_asrc *easrc;
@@ -1314,7 +1314,7 @@ void fsl_easrc_release_context(struct fsl_asrc_pair *ctx)
*
* Enable the DMA request and context
*/
-int fsl_easrc_start_context(struct fsl_asrc_pair *ctx)
+static int fsl_easrc_start_context(struct fsl_asrc_pair *ctx)
{
struct fsl_asrc *easrc = ctx->asrc;
@@ -1332,7 +1332,7 @@ int fsl_easrc_start_context(struct fsl_asrc_pair *ctx)
*
* Disable the DMA request and context
*/
-int fsl_easrc_stop_context(struct fsl_asrc_pair *ctx)
+static int fsl_easrc_stop_context(struct fsl_asrc_pair *ctx)
{
struct fsl_asrc *easrc = ctx->asrc;
int val, i;
@@ -1379,8 +1379,8 @@ int fsl_easrc_stop_context(struct fsl_asrc_pair *ctx)
return 0;
}
-struct dma_chan *fsl_easrc_get_dma_channel(struct fsl_asrc_pair *ctx,
- bool dir)
+static struct dma_chan *fsl_easrc_get_dma_channel(struct fsl_asrc_pair *ctx,
+ bool dir)
{
struct fsl_asrc *easrc = ctx->asrc;
enum asrc_pair_index index = ctx->index;
@@ -1391,7 +1391,6 @@ struct dma_chan *fsl_easrc_get_dma_channel(struct fsl_asrc_pair *ctx,
return dma_request_slave_channel(&easrc->pdev->dev, name);
};
-EXPORT_SYMBOL_GPL(fsl_easrc_get_dma_channel);
static const unsigned int easrc_rates[] = {
8000, 11025, 12000, 16000,
--
2.21.0
^ permalink raw reply related
* Re: [PATCH v2] selftests: powerpc: Add test for execute-disabled pkeys
From: Michael Ellerman @ 2020-06-03 3:17 UTC (permalink / raw)
To: Sandipan Das
Cc: fweimer, aneesh.kumar, linuxram, linux-mm, linux-kselftest,
linuxppc-dev, bauerman
In-Reply-To: <1eb388dc-0fde-64f3-9c05-7f9f2a398543@linux.ibm.com>
Sandipan Das <sandipan@linux.ibm.com> writes:
> Hi Michael,
>
> Thanks for your suggestions. I had a few questions regarding some
> of them.
>
> On 29/05/20 7:18 am, Michael Ellerman wrote:
>>> [...]
>>> +
>>> +static void pkeyreg_set(unsigned long uamr)
>>> +{
>>> + asm volatile("isync; mtspr 0xd, %0; isync;" : : "r"(uamr));
>>> +}
>>
>> You can use mtspr() there, but you'll need to add the isync's yourself.
>>
>
> Would it make sense to add a new macro that adds the CSI instructions?
> Something like this.
I guess. I'm not sure there's that many places that need it, it's just
the pkey tests I think.
I'd be more inclined to have a set_amr() helper that includes the
isyncs, rather than a generic mtspr() version.
> diff --git a/tools/testing/selftests/powerpc/include/reg.h b/tools/testing/selftests/powerpc/include/reg.h
> index 022c5076b2c5..d60f66380cad 100644
> --- a/tools/testing/selftests/powerpc/include/reg.h
> +++ b/tools/testing/selftests/powerpc/include/reg.h
> @@ -15,6 +15,10 @@
> #define mtspr(rn, v) asm volatile("mtspr " _str(rn) ",%0" : \
> : "r" ((unsigned long)(v)) \
> : "memory")
> +#define mtspr_csi(rn, v) ({ \
> + asm volatile("isync; mtspr " _str(rn) ",%0; isync;" : \
> + : "r" ((unsigned long)(v)) \
> + : "memory"); })
>
> #define mb() asm volatile("sync" : : : "memory");
> #define barrier() asm volatile("" : : : "memory");
>
>
> I also noticed that two of the ptrace-related pkey tests were also not
> using CSIs. I will fix those too.
>
>>> [...]
>>> + /* The following two cases will avoid SEGV_PKUERR */
>>> + ftype = -1;
>>> + fpkey = -1;
>>> +
>>> + /*
>>> + * Read an instruction word from the address when AMR bits
>>> + * are not set.
>>
>> You should explain for people who aren't familiar with the ISA that "AMR
>> bits not set" means "read/write access allowed".
>>
>>> + *
>>> + * This should not generate a fault as having PROT_EXEC
>>> + * implicitly allows reads. The pkey currently restricts
>>
>> Whether PROT_EXEC implies read is not well defined (see the man page).
>> If you want to test this case I think you'd be better off specifying
>> PROT_EXEC | PROT_READ explicitly.
>>
>
> But I guess specifying PROT_EXEC | PROT_READ defeats the purpose? I can
> tweak the passing condition though based on whether READ_IMPLIES_EXEC is
> set in the personality.
>
>> [...]
>>> + FAIL_IF(faults != 0 || fcode != SEGV_ACCERR);
>>> +
>>> + /* The following three cases will generate SEGV_PKUERR */
>>> + ftype = PKEY_DISABLE_ACCESS;
>>> + fpkey = pkey;
>>> +
>>> + /*
>>> + * Read an instruction word from the address when AMR bits
>>> + * are set.
>>> + *
>>> + * This should generate a pkey fault based on AMR bits only
>>> + * as having PROT_EXEC implicitly allows reads.
>>
>> Again would be better to specify PROT_READ IMHO.
>>
>
> I can use a personality check here too.
>
>>> + */
>>> + faults = 1;
>>> + FAIL_IF(sys_pkey_mprotect(insns, pgsize, PROT_EXEC, pkey) != 0);
>>> + printf("read from %p, pkey is execute-disabled, access-disabled\n",
>>> + (void *) faddr);
>>> + pkey_set_rights(pkey, PKEY_DISABLE_ACCESS);
>>> + i = *faddr;
>>> + FAIL_IF(faults != 0 || fcode != SEGV_PKUERR);
>>> +
>>> + /*
>>> + * Write an instruction word to the address when AMR bits
>>> + * are set.
>>> + *
>>> + * This should generate two faults. First, a pkey fault based
>>> + * on AMR bits and then an access fault based on PROT_EXEC.
>>> + */
>>> + faults = 2;
>>
>> Setting faults to the expected value and decrementing it in the fault
>> handler is kind of weird.
>>
>> I think it would be clearer if faults was just a zero-based counter of
>> how many faults we've taken, and then you test that it's == 2 below.
>>
>>> + FAIL_IF(sys_pkey_mprotect(insns, pgsize, PROT_EXEC, pkey) != 0);
>>> + printf("write to %p, pkey is execute-disabled, access-disabled\n",
>>> + (void *) faddr);
>>> + pkey_set_rights(pkey, PKEY_DISABLE_ACCESS);
>>> + *faddr = 0x60000000; /* nop */
>>> + FAIL_IF(faults != 0 || fcode != SEGV_ACCERR);
>>
>> ie. FAIL_IF(faults != 2 || ... )
>>
>
> Agreed, it is weird. IIRC, I did this to make sure that if the test
> kept getting repeated faults at the same address and exceeded the
> maximum number of expected faults i.e. it gets another fault when
> 'faults' is already zero, then the signal handler will attempt to
> let the program continue by giving all permissions to the page and
> also the pkey. Would it make sense to just rename 'faults' to
> something like 'remaining_faults'?
It seems like you've tried to make the code cope with a situation that
should not happen, and would indicate a bug if it did happen, in which
case I think it would be fine if the test just timed out.
But if you want to handle it that's up to you, renaming the variable
might help a bit.
cheers
^ permalink raw reply
* Re: [PATCH] cxl: Fix kobject memory leak in cxl_sysfs_afu_new_cr()
From: Michael Ellerman @ 2020-06-03 3:02 UTC (permalink / raw)
To: wanghai (M), Markus Elfring, linuxppc-dev
Cc: Andrew Donnellan, Arnd Bergmann, Greg Kroah-Hartman,
kernel-janitors, linux-kernel, Ian Munsie, Frederic Barrat
In-Reply-To: <25ad528b-beaf-820f-9738-ea304dcbc0d7@huawei.com>
"wanghai (M)" <wanghai38@huawei.com> writes:
> 在 2020/6/3 1:20, Markus Elfring 写道:
>>> Fix it by adding a call to kobject_put() in the error path of
>>> kobject_init_and_add().
>> Thanks for another completion of the exception handling.
>>
>> Would an other patch subject be a bit nicer?
> Thanks for the guidance, I will perfect this description and send a v2
>>
>> …
>>> +++ b/drivers/misc/cxl/sysfs.c
>>> @@ -624,7 +624,7 @@ static struct afu_config_record *cxl_sysfs_afu_new_cr(struct cxl_afu *afu, int c
>>> rc = kobject_init_and_add(&cr->kobj, &afu_config_record_type,
>>> &afu->dev.kobj, "cr%i", cr->cr);
>>> if (rc)
>>> - goto err;
>>> + goto err1;
>> …
>>
>> Can an other label be more reasonable here?
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/coding-style.rst?id=f359287765c04711ff54fbd11645271d8e5ff763#n465
> I just used the original author's label, should I replace all his labels
> like'err','err1' with reasonable one.
No.
cheers
^ permalink raw reply
* Re: linux-next: manual merge of the rcu tree with the powerpc tree
From: Stephen Rothwell @ 2020-06-03 2:02 UTC (permalink / raw)
To: Michael Ellerman, PowerPC
Cc: Paul E. McKenney, Peter Zijlstra, Linux Kernel Mailing List,
Nicholas Piggin, Linux Next Mailing List, Thomas Gleixner
In-Reply-To: <20200519172316.3b37cbae@canb.auug.org.au>
[-- Attachment #1: Type: text/plain, Size: 638 bytes --]
Hi all,
On Tue, 19 May 2020 17:23:16 +1000 Stephen Rothwell <sfr@canb.auug.org.au> wrote:
>
> Hi all,
>
> Today's linux-next merge of the rcu tree got a conflict in:
>
> arch/powerpc/kernel/traps.c
>
> between commit:
>
> 116ac378bb3f ("powerpc/64s: machine check interrupt update NMI accounting")
>
> from the powerpc tree and commit:
>
> 187416eeb388 ("hardirq/nmi: Allow nested nmi_enter()")
>
> from the rcu tree.
This is now a conflict between commit
69ea03b56ed2 ("hardirq/nmi: Allow nested nmi_enter()")
From Linus tree and the above powerpc tree commit.
--
Cheers,
Stephen Rothwell
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* Re: [PATCH] cxl: Fix kobject memory leak in cxl_sysfs_afu_new_cr()
From: wanghai (M) @ 2020-06-03 1:42 UTC (permalink / raw)
To: Markus Elfring, linuxppc-dev
Cc: Andrew Donnellan, Arnd Bergmann, Greg Kroah-Hartman,
kernel-janitors, linux-kernel, Ian Munsie, Frederic Barrat
In-Reply-To: <b9791ff3-8397-f6e9-ca88-59c9bbe8c78f@web.de>
在 2020/6/3 1:20, Markus Elfring 写道:
>> Fix it by adding a call to kobject_put() in the error path of
>> kobject_init_and_add().
> Thanks for another completion of the exception handling.
>
> Would an other patch subject be a bit nicer?
Thanks for the guidance, I will perfect this description and send a v2
>
> …
>> +++ b/drivers/misc/cxl/sysfs.c
>> @@ -624,7 +624,7 @@ static struct afu_config_record *cxl_sysfs_afu_new_cr(struct cxl_afu *afu, int c
>> rc = kobject_init_and_add(&cr->kobj, &afu_config_record_type,
>> &afu->dev.kobj, "cr%i", cr->cr);
>> if (rc)
>> - goto err;
>> + goto err1;
> …
>
> Can an other label be more reasonable here?
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/coding-style.rst?id=f359287765c04711ff54fbd11645271d8e5ff763#n465
I just used the original author's label, should I replace all his labels
like'err','err1' with reasonable one.
^ permalink raw reply
* Re: [RESEND PATCH v9 1/5] powerpc: Document details on H_SCM_HEALTH hcall
From: Ira Weiny @ 2020-06-02 21:21 UTC (permalink / raw)
To: Vaibhav Jain
Cc: Santosh Sivaraj, linux-nvdimm, linux-kernel, Steven Rostedt,
Oliver O'Halloran, Aneesh Kumar K . V, Dan Williams,
linuxppc-dev
In-Reply-To: <20200602101438.73929-2-vaibhav@linux.ibm.com>
On Tue, Jun 02, 2020 at 03:44:34PM +0530, Vaibhav Jain wrote:
> Add documentation to 'papr_hcalls.rst' describing the bitmap flags
> that are returned from H_SCM_HEALTH hcall as per the PAPR-SCM
> specification.
>
> Cc: "Aneesh Kumar K . V" <aneesh.kumar@linux.ibm.com>
> Cc: Dan Williams <dan.j.williams@intel.com>
> Cc: Michael Ellerman <mpe@ellerman.id.au>
> Cc: Ira Weiny <ira.weiny@intel.com>
Acked-by: Ira Weiny <ira.weiny@intel.com>
> Signed-off-by: Vaibhav Jain <vaibhav@linux.ibm.com>
> ---
> Changelog:
>
> Resend:
> * None
>
> v8..v9:
> * s/SCM/PMEM device. [ Dan Williams, Aneesh ]
>
> v7..v8:
> * Added a clarification on bit-ordering of Health Bitmap
>
> Resend:
> * None
>
> v6..v7:
> * None
>
> v5..v6:
> * New patch in the series
> ---
> Documentation/powerpc/papr_hcalls.rst | 46 ++++++++++++++++++++++++---
> 1 file changed, 42 insertions(+), 4 deletions(-)
>
> diff --git a/Documentation/powerpc/papr_hcalls.rst b/Documentation/powerpc/papr_hcalls.rst
> index 3493631a60f8..48fcf1255a33 100644
> --- a/Documentation/powerpc/papr_hcalls.rst
> +++ b/Documentation/powerpc/papr_hcalls.rst
> @@ -220,13 +220,51 @@ from the LPAR memory.
> **H_SCM_HEALTH**
>
> | Input: drcIndex
> -| Out: *health-bitmap, health-bit-valid-bitmap*
> +| Out: *health-bitmap (r4), health-bit-valid-bitmap (r5)*
> | Return Value: *H_Success, H_Parameter, H_Hardware*
>
> Given a DRC Index return the info on predictive failure and overall health of
> -the NVDIMM. The asserted bits in the health-bitmap indicate a single predictive
> -failure and health-bit-valid-bitmap indicate which bits in health-bitmap are
> -valid.
> +the PMEM device. The asserted bits in the health-bitmap indicate one or more states
> +(described in table below) of the PMEM device and health-bit-valid-bitmap indicate
> +which bits in health-bitmap are valid. The bits are reported in
> +reverse bit ordering for example a value of 0xC400000000000000
> +indicates bits 0, 1, and 5 are valid.
> +
> +Health Bitmap Flags:
> +
> ++------+-----------------------------------------------------------------------+
> +| Bit | Definition |
> ++======+=======================================================================+
> +| 00 | PMEM device is unable to persist memory contents. |
> +| | If the system is powered down, nothing will be saved. |
> ++------+-----------------------------------------------------------------------+
> +| 01 | PMEM device failed to persist memory contents. Either contents were |
> +| | not saved successfully on power down or were not restored properly on |
> +| | power up. |
> ++------+-----------------------------------------------------------------------+
> +| 02 | PMEM device contents are persisted from previous IPL. The data from |
> +| | the last boot were successfully restored. |
> ++------+-----------------------------------------------------------------------+
> +| 03 | PMEM device contents are not persisted from previous IPL. There was no|
> +| | data to restore from the last boot. |
> ++------+-----------------------------------------------------------------------+
> +| 04 | PMEM device memory life remaining is critically low |
> ++------+-----------------------------------------------------------------------+
> +| 05 | PMEM device will be garded off next IPL due to failure |
> ++------+-----------------------------------------------------------------------+
> +| 06 | PMEM device contents cannot persist due to current platform health |
> +| | status. A hardware failure may prevent data from being saved or |
> +| | restored. |
> ++------+-----------------------------------------------------------------------+
> +| 07 | PMEM device is unable to persist memory contents in certain conditions|
> ++------+-----------------------------------------------------------------------+
> +| 08 | PMEM device is encrypted |
> ++------+-----------------------------------------------------------------------+
> +| 09 | PMEM device has successfully completed a requested erase or secure |
> +| | erase procedure. |
> ++------+-----------------------------------------------------------------------+
> +|10:63 | Reserved / Unused |
> ++------+-----------------------------------------------------------------------+
>
> **H_SCM_PERFORMANCE_STATS**
>
> --
> 2.26.2
>
^ permalink raw reply
* Re: [RESEND PATCH v9 5/5] powerpc/papr_scm: Implement support for PAPR_PDSM_HEALTH
From: Ira Weiny @ 2020-06-02 21:19 UTC (permalink / raw)
To: Vaibhav Jain
Cc: Santosh Sivaraj, linux-nvdimm, linux-kernel, Steven Rostedt,
Oliver O'Halloran, Aneesh Kumar K . V, Dan Williams,
linuxppc-dev
In-Reply-To: <20200602101438.73929-6-vaibhav@linux.ibm.com>
On Tue, Jun 02, 2020 at 03:44:38PM +0530, Vaibhav Jain wrote:
> This patch implements support for PDSM request 'PAPR_PDSM_HEALTH'
> that returns a newly introduced 'struct nd_papr_pdsm_health' instance
> containing dimm health information back to user space in response to
> ND_CMD_CALL. This functionality is implemented in newly introduced
> papr_pdsm_health() that queries the nvdimm health information and
> then copies this information to the package payload whose layout is
> defined by 'struct nd_papr_pdsm_health'.
>
> The patch also introduces a new member 'struct papr_scm_priv.health'
> thats an instance of 'struct nd_papr_pdsm_health' to cache the health
> information of a nvdimm. As a result functions drc_pmem_query_health()
> and flags_show() are updated to populate and use this new struct
> instead of a u64 integer that was earlier used.
>
> Cc: "Aneesh Kumar K . V" <aneesh.kumar@linux.ibm.com>
> Cc: Dan Williams <dan.j.williams@intel.com>
> Cc: Michael Ellerman <mpe@ellerman.id.au>
> Cc: Ira Weiny <ira.weiny@intel.com>
> Reviewed-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
> Signed-off-by: Vaibhav Jain <vaibhav@linux.ibm.com>
> ---
> Changelog:
>
> Resend:
> * Added ack from Aneesh.
>
> v8..v9:
> * s/PAPR_SCM_PDSM_HEALTH/PAPR_PDSM_HEALTH/g [ Dan , Aneesh ]
> * s/PAPR_SCM_PSDM_DIMM_*/PAPR_PDSM_DIMM_*/g
> * Renamed papr_scm_get_health() to papr_psdm_health()
> * Updated patch description to replace papr-scm dimm with nvdimm.
>
> v7..v8:
> * None
>
> Resend:
> * None
>
> v6..v7:
> * Updated flags_show() to use seq_buf_printf(). [Mpe]
> * Updated papr_scm_get_health() to use newly introduced
> __drc_pmem_query_health() bypassing the cache [Mpe].
>
> v5..v6:
> * Added attribute '__packed' to 'struct nd_papr_pdsm_health_v1' to
> gaurd against possibility of different compilers adding different
> paddings to the struct [ Dan Williams ]
>
> * Updated 'struct nd_papr_pdsm_health_v1' to use __u8 instead of
> 'bool' and also updated drc_pmem_query_health() to take this into
> account. [ Dan Williams ]
>
> v4..v5:
> * None
>
> v3..v4:
> * Call the DSM_PAPR_SCM_HEALTH service function from
> papr_scm_service_dsm() instead of papr_scm_ndctl(). [Aneesh]
>
> v2..v3:
> * Updated struct nd_papr_scm_dimm_health_stat_v1 to use '__xx' types
> as its exported to the userspace [Aneesh]
> * Changed the constants DSM_PAPR_SCM_DIMM_XX indicating dimm health
> from enum to #defines [Aneesh]
>
> v1..v2:
> * New patch in the series
> ---
> arch/powerpc/include/uapi/asm/papr_pdsm.h | 39 +++++++
> arch/powerpc/platforms/pseries/papr_scm.c | 125 +++++++++++++++++++---
> 2 files changed, 147 insertions(+), 17 deletions(-)
>
> diff --git a/arch/powerpc/include/uapi/asm/papr_pdsm.h b/arch/powerpc/include/uapi/asm/papr_pdsm.h
> index 6407fefcc007..411725a91591 100644
> --- a/arch/powerpc/include/uapi/asm/papr_pdsm.h
> +++ b/arch/powerpc/include/uapi/asm/papr_pdsm.h
> @@ -115,6 +115,7 @@ struct nd_pdsm_cmd_pkg {
> */
> enum papr_pdsm {
> PAPR_PDSM_MIN = 0x0,
> + PAPR_PDSM_HEALTH,
> PAPR_PDSM_MAX,
> };
>
> @@ -133,4 +134,42 @@ static inline void *pdsm_cmd_to_payload(struct nd_pdsm_cmd_pkg *pcmd)
> return (void *)(pcmd->payload);
> }
>
> +/* Various nvdimm health indicators */
> +#define PAPR_PDSM_DIMM_HEALTHY 0
> +#define PAPR_PDSM_DIMM_UNHEALTHY 1
> +#define PAPR_PDSM_DIMM_CRITICAL 2
> +#define PAPR_PDSM_DIMM_FATAL 3
> +
> +/*
> + * Struct exchanged between kernel & ndctl in for PAPR_PDSM_HEALTH
> + * Various flags indicate the health status of the dimm.
> + *
> + * dimm_unarmed : Dimm not armed. So contents wont persist.
> + * dimm_bad_shutdown : Previous shutdown did not persist contents.
> + * dimm_bad_restore : Contents from previous shutdown werent restored.
> + * dimm_scrubbed : Contents of the dimm have been scrubbed.
> + * dimm_locked : Contents of the dimm cant be modified until CEC reboot
> + * dimm_encrypted : Contents of dimm are encrypted.
> + * dimm_health : Dimm health indicator. One of PAPR_PDSM_DIMM_XXXX
> + */
> +struct nd_papr_pdsm_health_v1 {
> + __u8 dimm_unarmed;
> + __u8 dimm_bad_shutdown;
> + __u8 dimm_bad_restore;
> + __u8 dimm_scrubbed;
> + __u8 dimm_locked;
> + __u8 dimm_encrypted;
> + __u16 dimm_health;
> +} __packed;
> +
> +/*
> + * Typedef the current struct for dimm_health so that any application
> + * or kernel recompiled after introducing a new version automatically
> + * supports the new version.
> + */
> +#define nd_papr_pdsm_health nd_papr_pdsm_health_v1
> +
> +/* Current version number for the dimm health struct */
This can't be the 'current' version. You will need a list of versions you
support. Because if the user passes in an old version you need to be able to
respond with that old version. Also if you plan to support 'return X for a Y
query' then the user will need both X and Y defined to interpret X.
> +#define ND_PAPR_PDSM_HEALTH_VERSION 1
> +
> #endif /* _UAPI_ASM_POWERPC_PAPR_PDSM_H_ */
> diff --git a/arch/powerpc/platforms/pseries/papr_scm.c b/arch/powerpc/platforms/pseries/papr_scm.c
> index 5e2237e7ec08..c0606c0c659c 100644
> --- a/arch/powerpc/platforms/pseries/papr_scm.c
> +++ b/arch/powerpc/platforms/pseries/papr_scm.c
> @@ -88,7 +88,7 @@ struct papr_scm_priv {
> unsigned long lasthealth_jiffies;
>
> /* Health information for the dimm */
> - u64 health_bitmap;
> + struct nd_papr_pdsm_health health;
ok so we are throwing away all the #defs from patch 1? Are they still valid?
I'm confused that patch 3 added this and we are throwing it away here...
> };
>
> static int drc_pmem_bind(struct papr_scm_priv *p)
> @@ -201,6 +201,7 @@ static int drc_pmem_query_n_bind(struct papr_scm_priv *p)
> static int __drc_pmem_query_health(struct papr_scm_priv *p)
> {
> unsigned long ret[PLPAR_HCALL_BUFSIZE];
> + u64 health;
> long rc;
>
> /* issue the hcall */
> @@ -208,18 +209,46 @@ static int __drc_pmem_query_health(struct papr_scm_priv *p)
> if (rc != H_SUCCESS) {
> dev_err(&p->pdev->dev,
> "Failed to query health information, Err:%ld\n", rc);
> - rc = -ENXIO;
> - goto out;
> + return -ENXIO;
I missed this... probably did not need the goto in the first patch?
> }
>
> p->lasthealth_jiffies = jiffies;
> - p->health_bitmap = ret[0] & ret[1];
> + health = ret[0] & ret[1];
>
> dev_dbg(&p->pdev->dev,
> "Queried dimm health info. Bitmap:0x%016lx Mask:0x%016lx\n",
> ret[0], ret[1]);
> -out:
> - return rc;
> +
> + memset(&p->health, 0, sizeof(p->health));
> +
> + /* Check for various masks in bitmap and set the buffer */
> + if (health & PAPR_PMEM_UNARMED_MASK)
Oh ok... odd. (don't add code then just take it away in a series)
You could have lead with the user structure and put this code in patch 3.
Why does the user need u8 to represent a single bit? Does this help protect
against endian issues?
> + p->health.dimm_unarmed = 1;
> +
> + if (health & PAPR_PMEM_BAD_SHUTDOWN_MASK)
> + p->health.dimm_bad_shutdown = 1;
> +
> + if (health & PAPR_PMEM_BAD_RESTORE_MASK)
> + p->health.dimm_bad_restore = 1;
> +
> + if (health & PAPR_PMEM_ENCRYPTED)
> + p->health.dimm_encrypted = 1;
> +
> + if (health & PAPR_PMEM_SCRUBBED_AND_LOCKED) {
> + p->health.dimm_locked = 1;
> + p->health.dimm_scrubbed = 1;
> + }
> +
> + if (health & PAPR_PMEM_HEALTH_UNHEALTHY)
> + p->health.dimm_health = PAPR_PDSM_DIMM_UNHEALTHY;
> +
> + if (health & PAPR_PMEM_HEALTH_CRITICAL)
> + p->health.dimm_health = PAPR_PDSM_DIMM_CRITICAL;
> +
> + if (health & PAPR_PMEM_HEALTH_FATAL)
> + p->health.dimm_health = PAPR_PDSM_DIMM_FATAL;
> +
> + return 0;
> }
>
> /* Min interval in seconds for assuming stable dimm health */
> @@ -403,6 +432,58 @@ static int is_cmd_valid(struct nvdimm *nvdimm, unsigned int cmd, void *buf,
> return 0;
> }
>
> +/* Fetch the DIMM health info and populate it in provided package. */
> +static int papr_pdsm_health(struct papr_scm_priv *p,
> + struct nd_pdsm_cmd_pkg *pkg)
> +{
> + int rc;
> + size_t copysize = sizeof(p->health);
> +
> + /* Ensure dimm health mutex is taken preventing concurrent access */
> + rc = mutex_lock_interruptible(&p->health_mutex);
> + if (rc)
> + goto out;
> +
> + /* Always fetch upto date dimm health data ignoring cached values */
> + rc = __drc_pmem_query_health(p);
> + if (rc)
> + goto out_unlock;
> + /*
> + * If the requested payload version is greater than one we know
> + * about, return the payload version we know about and let
> + * caller/userspace handle.
> + */
> + if (pkg->payload_version > ND_PAPR_PDSM_HEALTH_VERSION)
> + pkg->payload_version = ND_PAPR_PDSM_HEALTH_VERSION;
I know this seems easy now but I do think you will run into trouble later.
Ira
> +
> + if (pkg->hdr.nd_size_out < copysize) {
> + dev_dbg(&p->pdev->dev, "Truncated payload (%u). Expected (%lu)",
> + pkg->hdr.nd_size_out, copysize);
> + rc = -ENOSPC;
> + goto out_unlock;
> + }
> +
> + dev_dbg(&p->pdev->dev, "Copying payload size=%lu version=0x%x\n",
> + copysize, pkg->payload_version);
> +
> + /* Copy the health struct to the payload */
> + memcpy(pdsm_cmd_to_payload(pkg), &p->health, copysize);
> + pkg->hdr.nd_fw_size = copysize;
> +
> +out_unlock:
> + mutex_unlock(&p->health_mutex);
> +
> +out:
> + /*
> + * Put the error in out package and return success from function
> + * so that errors if any are propogated back to userspace.
> + */
> + pkg->cmd_status = rc;
> + dev_dbg(&p->pdev->dev, "completion code = %d\n", rc);
> +
> + return 0;
> +}
> +
> static int papr_scm_service_pdsm(struct papr_scm_priv *p,
> struct nd_pdsm_cmd_pkg *call_pkg)
> {
> @@ -417,6 +498,9 @@ static int papr_scm_service_pdsm(struct papr_scm_priv *p,
>
> /* Depending on the DSM command call appropriate service routine */
> switch (call_pkg->hdr.nd_command) {
> + case PAPR_PDSM_HEALTH:
> + return papr_pdsm_health(p, call_pkg);
> +
> default:
> dev_dbg(&p->pdev->dev, "Unsupported PDSM request 0x%llx\n",
> call_pkg->hdr.nd_command);
> @@ -485,34 +569,41 @@ static ssize_t flags_show(struct device *dev,
> struct nvdimm *dimm = to_nvdimm(dev);
> struct papr_scm_priv *p = nvdimm_provider_data(dimm);
> struct seq_buf s;
> - u64 health;
> int rc;
>
> rc = drc_pmem_query_health(p);
> if (rc)
> return rc;
>
> - /* Copy health_bitmap locally, check masks & update out buffer */
> - health = READ_ONCE(p->health_bitmap);
> -
> seq_buf_init(&s, buf, PAGE_SIZE);
> - if (health & PAPR_PMEM_UNARMED_MASK)
> +
> + /* Protect concurrent modifications to papr_scm_priv */
> + rc = mutex_lock_interruptible(&p->health_mutex);
> + if (rc)
> + return rc;
> +
> + if (p->health.dimm_unarmed)
> seq_buf_printf(&s, "not_armed ");
>
> - if (health & PAPR_PMEM_BAD_SHUTDOWN_MASK)
> + if (p->health.dimm_bad_shutdown)
> seq_buf_printf(&s, "flush_fail ");
>
> - if (health & PAPR_PMEM_BAD_RESTORE_MASK)
> + if (p->health.dimm_bad_restore)
> seq_buf_printf(&s, "restore_fail ");
>
> - if (health & PAPR_PMEM_ENCRYPTED)
> + if (p->health.dimm_encrypted)
> seq_buf_printf(&s, "encrypted ");
>
> - if (health & PAPR_PMEM_SMART_EVENT_MASK)
> + if (p->health.dimm_health)
> seq_buf_printf(&s, "smart_notify ");
>
> - if (health & PAPR_PMEM_SCRUBBED_AND_LOCKED)
> - seq_buf_printf(&s, "scrubbed locked ");
> + if (p->health.dimm_scrubbed)
> + seq_buf_printf(&s, "scrubbed ");
> +
> + if (p->health.dimm_locked)
> + seq_buf_printf(&s, "locked ");
> +
> + mutex_unlock(&p->health_mutex);
>
> if (seq_buf_used(&s))
> seq_buf_printf(&s, "\n");
> --
> 2.26.2
>
^ permalink raw reply
* Re: [RESEND PATCH v9 4/5] ndctl/papr_scm,uapi: Add support for PAPR nvdimm specific methods
From: Ira Weiny @ 2020-06-02 21:05 UTC (permalink / raw)
To: Vaibhav Jain
Cc: linux-nvdimm, linux-kernel, Steven Rostedt, Aneesh Kumar K . V,
linuxppc-dev
In-Reply-To: <20200602205148.GF1505637@iweiny-DESK2.sc.intel.com>
On Tue, Jun 02, 2020 at 01:51:49PM -0700, 'Ira Weiny' wrote:
> On Tue, Jun 02, 2020 at 03:44:37PM +0530, Vaibhav Jain wrote:
...
> > +
> > +/*
> > + * PDSM Envelope:
> > + *
> > + * The ioctl ND_CMD_CALL transfers data between user-space and kernel via
> > + * envelope which consists of a header and user-defined payload sections.
> > + * The header is described by 'struct nd_pdsm_cmd_pkg' which expects a
> > + * payload following it and accessible via 'nd_pdsm_cmd_pkg.payload' field.
> > + * There is reserved field that can used to introduce new fields to the
> > + * structure in future. It also tries to ensure that 'nd_pdsm_cmd_pkg.payload'
> > + * lies at a 8-byte boundary.
> > + *
> > + * +-------------+---------------------+---------------------------+
> > + * | 64-Bytes | 16-Bytes | Max 176-Bytes |
> > + * +-------------+---------------------+---------------------------+
> > + * | nd_pdsm_cmd_pkg | |
> > + * |-------------+ | |
> > + * | nd_cmd_pkg | | |
> > + * +-------------+---------------------+---------------------------+
> > + * | nd_family | | |
> > + * | nd_size_out | cmd_status | |
> > + * | nd_size_in | payload_version | payload |
> > + * | nd_command | reserved | |
> > + * | nd_fw_size | | |
> > + * +-------------+---------------------+---------------------------+
One more comment WRT nd_size_[in|out]. I know that it is defined as the size
of the FW payload but normally when you nest headers 'size' in Header A
represents everything after Header A, including Header B. In this case that
would be including nd_pdsm_cmd_pkg...
It looks like that is not what you have done? Or perhaps I missed it?
Ira
> > + *
> > + * PDSM Header:
> > + *
> > + * The header is defined as 'struct nd_pdsm_cmd_pkg' which embeds a
> > + * 'struct nd_cmd_pkg' instance. The PDSM command is assigned to member
> > + * 'nd_cmd_pkg.nd_command'. Apart from size information of the envelope which is
> > + * contained in 'struct nd_cmd_pkg', the header also has members following
> ^^^^^
> ... the ...
>
> > + * members:
> > + *
> > + * 'cmd_status' : (Out) Errors if any encountered while servicing PDSM.
> > + * 'payload_version' : (In/Out) Version number associated with the payload.
> > + * 'reserved' : Not used and reserved for future.
> > + *
> > + * PDSM Payload:
> > + *
> > + * The layout of the PDSM Payload is defined by various structs shared between
> > + * papr_scm and libndctl so that contents of payload can be interpreted. During
> > + * servicing of a PDSM the papr_scm module will read input args from the payload
> > + * field by casting its contents to an appropriate struct pointer based on the
> > + * PDSM command. Similarly the output of servicing the PDSM command will be
> > + * copied to the payload field using the same struct.
> > + *
> > + * 'libnvdimm' enforces a hard limit of 256 bytes on the envelope size, which
> > + * leaves around 176 bytes for the envelope payload (ignoring any padding that
> > + * the compiler may silently introduce).
> > + *
> > + * Payload Version:
> > + *
> > + * A 'payload_version' field is present in PDSM header that indicates a specific
> > + * version of the structure present in PDSM Payload for a given PDSM command.
> > + * This provides backward compatibility in case the PDSM Payload structure
> > + * evolves and different structures are supported by 'papr_scm' and 'libndctl'.
> > + *
> > + * When sending a PDSM Payload to 'papr_scm', 'libndctl' should send the version
> > + * of the payload struct it supports via 'payload_version' field. The 'papr_scm'
> > + * module when servicing the PDSM envelope checks the 'payload_version' and then
> > + * uses 'payload struct version' == MIN('payload_version field',
> > + * 'max payload-struct-version supported by papr_scm') to service the PDSM.
> > + * After servicing the PDSM, 'papr_scm' put the negotiated version of payload
> > + * struct in returned 'payload_version' field.
> > + *
> > + * Libndctl on receiving the envelope back from papr_scm again checks the
> > + * 'payload_version' field and based on it use the appropriate version dsm
> > + * struct to parse the results.
> > + *
> > + * Backward Compatibility:
> > + *
> > + * Above scheme of exchanging different versioned PDSM struct between libndctl
> > + * and papr_scm should provide backward compatibility until following two
> > + * assumptions/conditions when defining new PDSM structs hold:
> > + *
> > + * Let T(X) = { set of attributes in PDSM struct 'T' versioned X }
> > + *
> > + * 1. T(X) is a proper subset of T(Y) if Y > X.
> > + * i.e Each new version of PDSM struct should retain existing struct
> > + * attributes from previous version
> > + *
> > + * 2. If an entity (libndctl or papr_scm) supports a PDSM struct T(X) then
> > + * it should also support T(1), T(2)...T(X - 1).
> > + * i.e When adding support for new version of a PDSM struct, libndctl
> > + * and papr_scm should retain support of the existing PDSM struct
> > + * version they support.
>
> Please see this thread for an example why versions are a bad idea in UAPIs:
>
> https://lkml.org/lkml/2020/3/26/213
>
> While the use of version is different in that thread the fundamental issues are
> the same. You end up with some weird matrix of supported features and
> structure definitions. For example, you are opening up the possibility of
> changing structures with a different version for no good reason.
>
> Also having the user query with version Z and get back version X (older) is
> odd. Generally if the kernel does not know about a feature (ie version Z of
> the structure) it should return -EINVAL and let the user figure out what to do.
> The user may just give up or they could try a different query.
>
> > + */
> > +
> > +/* PDSM-header + payload expected with ND_CMD_CALL ioctl from libnvdimm */
> > +struct nd_pdsm_cmd_pkg {
> > + struct nd_cmd_pkg hdr; /* Package header containing sub-cmd */
> > + __s32 cmd_status; /* Out: Sub-cmd status returned back */
> > + __u16 reserved[5]; /* Ignored and to be used in future */
>
> How do you know when reserved is used for something else in the future? Is
> reserved guaranteed (and checked by the code) to be 0?
>
> > + __u16 payload_version; /* In/Out: version of the payload */
>
> Why is payload_version after reserved?
>
> > + __u8 payload[]; /* In/Out: Sub-cmd data buffer */
> > +} __packed;
> > +
> > +/*
> > + * Methods to be embedded in ND_CMD_CALL request. These are sent to the kernel
> > + * via 'nd_pdsm_cmd_pkg.hdr.nd_command' member of the ioctl struct
> > + */
> > +enum papr_pdsm {
> > + PAPR_PDSM_MIN = 0x0,
> > + PAPR_PDSM_MAX,
> > +};
> > +
> > +/* Convert a libnvdimm nd_cmd_pkg to pdsm specific pkg */
> > +static inline struct nd_pdsm_cmd_pkg *nd_to_pdsm_cmd_pkg(struct nd_cmd_pkg *cmd)
> > +{
> > + return (struct nd_pdsm_cmd_pkg *) cmd;
> > +}
> > +
> > +/* Return the payload pointer for a given pcmd */
> > +static inline void *pdsm_cmd_to_payload(struct nd_pdsm_cmd_pkg *pcmd)
> > +{
> > + if (pcmd->hdr.nd_size_in == 0 && pcmd->hdr.nd_size_out == 0)
> > + return NULL;
> > + else
> > + return (void *)(pcmd->payload);
> > +}
> > +
> > +#endif /* _UAPI_ASM_POWERPC_PAPR_PDSM_H_ */
> > diff --git a/arch/powerpc/platforms/pseries/papr_scm.c b/arch/powerpc/platforms/pseries/papr_scm.c
> > index 149431594839..5e2237e7ec08 100644
> > --- a/arch/powerpc/platforms/pseries/papr_scm.c
> > +++ b/arch/powerpc/platforms/pseries/papr_scm.c
> > @@ -15,13 +15,15 @@
> > #include <linux/seq_buf.h>
> >
> > #include <asm/plpar_wrappers.h>
> > +#include <asm/papr_pdsm.h>
> >
> > #define BIND_ANY_ADDR (~0ul)
> >
> > #define PAPR_SCM_DIMM_CMD_MASK \
> > ((1ul << ND_CMD_GET_CONFIG_SIZE) | \
> > (1ul << ND_CMD_GET_CONFIG_DATA) | \
> > - (1ul << ND_CMD_SET_CONFIG_DATA))
> > + (1ul << ND_CMD_SET_CONFIG_DATA) | \
> > + (1ul << ND_CMD_CALL))
> >
> > /* DIMM health bitmap bitmap indicators */
> > /* SCM device is unable to persist memory contents */
> > @@ -350,16 +352,97 @@ static int papr_scm_meta_set(struct papr_scm_priv *p,
> > return 0;
> > }
> >
> > +/*
> > + * Validate the inputs args to dimm-control function and return '0' if valid.
> > + * This also does initial sanity validation to ND_CMD_CALL sub-command packages.
> > + */
> > +static int is_cmd_valid(struct nvdimm *nvdimm, unsigned int cmd, void *buf,
> > + unsigned int buf_len)
> > +{
> > + unsigned long cmd_mask = PAPR_SCM_DIMM_CMD_MASK;
> > + struct nd_pdsm_cmd_pkg *pkg = nd_to_pdsm_cmd_pkg(buf);
> > + struct papr_scm_priv *p;
> > +
> > + /* Only dimm-specific calls are supported atm */
> > + if (!nvdimm)
> > + return -EINVAL;
> > +
> > + /* get the provider date from struct nvdimm */
>
> s/date/data
>
> > + p = nvdimm_provider_data(nvdimm);
> > +
> > + if (!test_bit(cmd, &cmd_mask)) {
> > + dev_dbg(&p->pdev->dev, "Unsupported cmd=%u\n", cmd);
> > + return -EINVAL;
> > + } else if (cmd == ND_CMD_CALL) {
> > +
> > + /* Verify the envelope package */
> > + if (!buf || buf_len < sizeof(struct nd_pdsm_cmd_pkg)) {
> > + dev_dbg(&p->pdev->dev, "Invalid pkg size=%u\n",
> > + buf_len);
> > + return -EINVAL;
> > + }
> > +
> > + /* Verify that the PDSM family is valid */
> > + if (pkg->hdr.nd_family != NVDIMM_FAMILY_PAPR) {
> > + dev_dbg(&p->pdev->dev, "Invalid pkg family=0x%llx\n",
> > + pkg->hdr.nd_family);
> > + return -EINVAL;
> > +
> > + }
> > +
> > + /* We except a payload with all PDSM commands */
> > + if (pdsm_cmd_to_payload(pkg) == NULL) {
> > + dev_dbg(&p->pdev->dev,
> > + "Empty payload for sub-command=0x%llx\n",
> > + pkg->hdr.nd_command);
> > + return -EINVAL;
> > + }
> > + }
> > +
> > + /* Command looks valid */
>
> I assume the first command to be implemented also checks the { nd_command,
> payload_version, payload length } for correctness?
>
> > + return 0;
> > +}
> > +
> > +static int papr_scm_service_pdsm(struct papr_scm_priv *p,
> > + struct nd_pdsm_cmd_pkg *call_pkg)
> > +{
> > + /* unknown subcommands return error in packages */
> > + if (call_pkg->hdr.nd_command <= PAPR_PDSM_MIN ||
> > + call_pkg->hdr.nd_command >= PAPR_PDSM_MAX) {
> > + dev_dbg(&p->pdev->dev, "Invalid PDSM request 0x%llx\n",
> > + call_pkg->hdr.nd_command);
> > + call_pkg->cmd_status = -EINVAL;
> > + return 0;
> > + }
> > +
> > + /* Depending on the DSM command call appropriate service routine */
> > + switch (call_pkg->hdr.nd_command) {
> > + default:
> > + dev_dbg(&p->pdev->dev, "Unsupported PDSM request 0x%llx\n",
> > + call_pkg->hdr.nd_command);
> > + call_pkg->cmd_status = -ENOENT;
> > + return 0;
> > + }
> > +}
> > +
> > static int papr_scm_ndctl(struct nvdimm_bus_descriptor *nd_desc,
> > struct nvdimm *nvdimm, unsigned int cmd, void *buf,
> > unsigned int buf_len, int *cmd_rc)
> > {
> > struct nd_cmd_get_config_size *get_size_hdr;
> > struct papr_scm_priv *p;
> > + struct nd_pdsm_cmd_pkg *call_pkg = NULL;
> > + int rc;
> >
> > - /* Only dimm-specific calls are supported atm */
> > - if (!nvdimm)
> > - return -EINVAL;
> > + /* Use a local variable in case cmd_rc pointer is NULL */
> > + if (cmd_rc == NULL)
> > + cmd_rc = &rc;
>
> Why is this needed? AFAICT The caller of papr_scm_ndctl does not specify null
> and you did not change it.
>
> > +
> > + *cmd_rc = is_cmd_valid(nvdimm, cmd, buf, buf_len);
> > + if (*cmd_rc) {
> > + pr_debug("Invalid cmd=0x%x. Err=%d\n", cmd, *cmd_rc);
> > + return *cmd_rc;
> > + }
> >
> > p = nvdimm_provider_data(nvdimm);
> >
> > @@ -381,13 +464,19 @@ static int papr_scm_ndctl(struct nvdimm_bus_descriptor *nd_desc,
> > *cmd_rc = papr_scm_meta_set(p, buf);
> > break;
> >
> > + case ND_CMD_CALL:
> > + call_pkg = nd_to_pdsm_cmd_pkg(buf);
> > + *cmd_rc = papr_scm_service_pdsm(p, call_pkg);
> > + break;
> > +
> > default:
> > - return -EINVAL;
> > + dev_dbg(&p->pdev->dev, "Unknown command = %d\n", cmd);
> > + *cmd_rc = -EINVAL;
>
> Is this change related? If there is a bug where there is a caller of
> papr_scm_ndctl() with cmd_rc == NULL this should be a separate patch to fix
> that issue.
>
> Ira
>
> > }
> >
> > dev_dbg(&p->pdev->dev, "returned with cmd_rc = %d\n", *cmd_rc);
> >
> > - return 0;
> > + return *cmd_rc;
> > }
> >
> > static ssize_t flags_show(struct device *dev,
> > diff --git a/include/uapi/linux/ndctl.h b/include/uapi/linux/ndctl.h
> > index de5d90212409..0e09dc5cec19 100644
> > --- a/include/uapi/linux/ndctl.h
> > +++ b/include/uapi/linux/ndctl.h
> > @@ -244,6 +244,7 @@ struct nd_cmd_pkg {
> > #define NVDIMM_FAMILY_HPE2 2
> > #define NVDIMM_FAMILY_MSFT 3
> > #define NVDIMM_FAMILY_HYPERV 4
> > +#define NVDIMM_FAMILY_PAPR 5
> >
> > #define ND_IOCTL_CALL _IOWR(ND_IOCTL, ND_CMD_CALL,\
> > struct nd_cmd_pkg)
> > --
> > 2.26.2
> >
> _______________________________________________
> Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
> To unsubscribe send an email to linux-nvdimm-leave@lists.01.org
^ permalink raw reply
* Re: [RESEND PATCH v9 4/5] ndctl/papr_scm,uapi: Add support for PAPR nvdimm specific methods
From: Ira Weiny @ 2020-06-02 20:51 UTC (permalink / raw)
To: Vaibhav Jain
Cc: Santosh Sivaraj, linux-nvdimm, linux-kernel, Steven Rostedt,
Oliver O'Halloran, Aneesh Kumar K . V, Dan Williams,
linuxppc-dev
In-Reply-To: <20200602101438.73929-5-vaibhav@linux.ibm.com>
On Tue, Jun 02, 2020 at 03:44:37PM +0530, Vaibhav Jain wrote:
> Introduce support for PAPR NVDIMM Specific Methods (PDSM) in papr_scm
> module and add the command family NVDIMM_FAMILY_PAPR to the white list
> of NVDIMM command sets. Also advertise support for ND_CMD_CALL for the
> nvdimm command mask and implement necessary scaffolding in the module
> to handle ND_CMD_CALL ioctl and PDSM requests that we receive.
>
> The layout of the PDSM request as we expect from libnvdimm/libndctl is
> described in newly introduced uapi header 'papr_pdsm.h' which
> defines a new 'struct nd_pdsm_cmd_pkg' header. This header is used
> to communicate the PDSM request via member
> 'nd_cmd_pkg.nd_command' and size of payload that need to be
> sent/received for servicing the PDSM.
>
> A new function is_cmd_valid() is implemented that reads the args to
> papr_scm_ndctl() and performs sanity tests on them. A new function
> papr_scm_service_pdsm() is introduced and is called from
> papr_scm_ndctl() in case of a PDSM request is received via ND_CMD_CALL
> command from libnvdimm.
>
> Cc: "Aneesh Kumar K . V" <aneesh.kumar@linux.ibm.com>
> Cc: Dan Williams <dan.j.williams@intel.com>
> Cc: Michael Ellerman <mpe@ellerman.id.au>
> Cc: Ira Weiny <ira.weiny@intel.com>
> Reviewed-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
> Signed-off-by: Vaibhav Jain <vaibhav@linux.ibm.com>
> ---
> Changelog:
>
> Resend:
> * Added ack from Aneesh.
>
> v8..v9:
> * Reduced the usage of term SCM replacing it with appropriate
> replacement [ Dan Williams, Aneesh ]
> * Renamed 'papr_scm_pdsm.h' to 'papr_pdsm.h'
> * s/PAPR_SCM_PDSM_*/PAPR_PDSM_*/g
> * s/NVDIMM_FAMILY_PAPR_SCM/NVDIMM_FAMILY_PAPR/g
> * Minor updates to 'papr_psdm.h' to replace usage of term 'SCM'.
> * Minor update to patch description.
>
> v7..v8:
> * Removed the 'payload_offset' field from 'struct
> nd_pdsm_cmd_pkg'. Instead command payload is always assumed to start
> at 'nd_pdsm_cmd_pkg.payload'. [ Aneesh ]
> * To enable introducing new fields to 'struct nd_pdsm_cmd_pkg',
> 'reserved' field of 10-bytes is introduced. [ Aneesh ]
> * Fixed a typo in "Backward Compatibility" section of papr_scm_pdsm.h
> [ Ira ]
>
> Resend:
> * None
>
> v6..v7 :
> * Removed the re-definitions of __packed macro from papr_scm_pdsm.h
> [Mpe].
> * Removed the usage of __KERNEL__ macros in papr_scm_pdsm.h [Mpe].
> * Removed macros that were unused in papr_scm.c from papr_scm_pdsm.h
> [Mpe].
> * Made functions defined in papr_scm_pdsm.h as static inline. [Mpe]
>
> v5..v6 :
> * Changed the usage of the term DSM to PDSM to distinguish it from the
> ACPI term [ Dan Williams ]
> * Renamed papr_scm_dsm.h to papr_scm_pdsm.h and updated various struct
> to reflect the new terminology.
> * Updated the patch description and title to reflect the new terminology.
> * Squashed patch to introduce new command family in 'ndctl.h' with
> this patch [ Dan Williams ]
> * Updated the papr_scm_pdsm method starting index from 0x10000 to 0x0
> [ Dan Williams ]
> * Removed redundant license text from the papr_scm_psdm.h file.
> [ Dan Williams ]
> * s/envelop/envelope/ at various places [ Dan Williams ]
> * Added '__packed' attribute to command package header to gaurd
> against different compiler adding paddings between the fields.
> [ Dan Williams]
> * Converted various pr_debug to dev_debug [ Dan Williams ]
>
> v4..v5 :
> * None
>
> v3..v4 :
> * None
>
> v2..v3 :
> * Updated the patch prefix to 'ndctl/uapi' [Aneesh]
>
> v1..v2 :
> * None
> ---
> arch/powerpc/include/uapi/asm/papr_pdsm.h | 136 ++++++++++++++++++++++
> arch/powerpc/platforms/pseries/papr_scm.c | 101 +++++++++++++++-
> include/uapi/linux/ndctl.h | 1 +
> 3 files changed, 232 insertions(+), 6 deletions(-)
> create mode 100644 arch/powerpc/include/uapi/asm/papr_pdsm.h
>
> diff --git a/arch/powerpc/include/uapi/asm/papr_pdsm.h b/arch/powerpc/include/uapi/asm/papr_pdsm.h
> new file mode 100644
> index 000000000000..6407fefcc007
> --- /dev/null
> +++ b/arch/powerpc/include/uapi/asm/papr_pdsm.h
> @@ -0,0 +1,136 @@
> +/* SPDX-License-Identifier: GPL-2.0+ WITH Linux-syscall-note */
> +/*
> + * PAPR nvDimm Specific Methods (PDSM) and structs for libndctl
> + *
> + * (C) Copyright IBM 2020
> + *
> + * Author: Vaibhav Jain <vaibhav at linux.ibm.com>
> + */
> +
> +#ifndef _UAPI_ASM_POWERPC_PAPR_PDSM_H_
> +#define _UAPI_ASM_POWERPC_PAPR_PDSM_H_
> +
> +#include <linux/types.h>
> +
> +/*
> + * PDSM Envelope:
> + *
> + * The ioctl ND_CMD_CALL transfers data between user-space and kernel via
> + * envelope which consists of a header and user-defined payload sections.
> + * The header is described by 'struct nd_pdsm_cmd_pkg' which expects a
> + * payload following it and accessible via 'nd_pdsm_cmd_pkg.payload' field.
> + * There is reserved field that can used to introduce new fields to the
> + * structure in future. It also tries to ensure that 'nd_pdsm_cmd_pkg.payload'
> + * lies at a 8-byte boundary.
> + *
> + * +-------------+---------------------+---------------------------+
> + * | 64-Bytes | 16-Bytes | Max 176-Bytes |
> + * +-------------+---------------------+---------------------------+
> + * | nd_pdsm_cmd_pkg | |
> + * |-------------+ | |
> + * | nd_cmd_pkg | | |
> + * +-------------+---------------------+---------------------------+
> + * | nd_family | | |
> + * | nd_size_out | cmd_status | |
> + * | nd_size_in | payload_version | payload |
> + * | nd_command | reserved | |
> + * | nd_fw_size | | |
> + * +-------------+---------------------+---------------------------+
> + *
> + * PDSM Header:
> + *
> + * The header is defined as 'struct nd_pdsm_cmd_pkg' which embeds a
> + * 'struct nd_cmd_pkg' instance. The PDSM command is assigned to member
> + * 'nd_cmd_pkg.nd_command'. Apart from size information of the envelope which is
> + * contained in 'struct nd_cmd_pkg', the header also has members following
^^^^^
... the ...
> + * members:
> + *
> + * 'cmd_status' : (Out) Errors if any encountered while servicing PDSM.
> + * 'payload_version' : (In/Out) Version number associated with the payload.
> + * 'reserved' : Not used and reserved for future.
> + *
> + * PDSM Payload:
> + *
> + * The layout of the PDSM Payload is defined by various structs shared between
> + * papr_scm and libndctl so that contents of payload can be interpreted. During
> + * servicing of a PDSM the papr_scm module will read input args from the payload
> + * field by casting its contents to an appropriate struct pointer based on the
> + * PDSM command. Similarly the output of servicing the PDSM command will be
> + * copied to the payload field using the same struct.
> + *
> + * 'libnvdimm' enforces a hard limit of 256 bytes on the envelope size, which
> + * leaves around 176 bytes for the envelope payload (ignoring any padding that
> + * the compiler may silently introduce).
> + *
> + * Payload Version:
> + *
> + * A 'payload_version' field is present in PDSM header that indicates a specific
> + * version of the structure present in PDSM Payload for a given PDSM command.
> + * This provides backward compatibility in case the PDSM Payload structure
> + * evolves and different structures are supported by 'papr_scm' and 'libndctl'.
> + *
> + * When sending a PDSM Payload to 'papr_scm', 'libndctl' should send the version
> + * of the payload struct it supports via 'payload_version' field. The 'papr_scm'
> + * module when servicing the PDSM envelope checks the 'payload_version' and then
> + * uses 'payload struct version' == MIN('payload_version field',
> + * 'max payload-struct-version supported by papr_scm') to service the PDSM.
> + * After servicing the PDSM, 'papr_scm' put the negotiated version of payload
> + * struct in returned 'payload_version' field.
> + *
> + * Libndctl on receiving the envelope back from papr_scm again checks the
> + * 'payload_version' field and based on it use the appropriate version dsm
> + * struct to parse the results.
> + *
> + * Backward Compatibility:
> + *
> + * Above scheme of exchanging different versioned PDSM struct between libndctl
> + * and papr_scm should provide backward compatibility until following two
> + * assumptions/conditions when defining new PDSM structs hold:
> + *
> + * Let T(X) = { set of attributes in PDSM struct 'T' versioned X }
> + *
> + * 1. T(X) is a proper subset of T(Y) if Y > X.
> + * i.e Each new version of PDSM struct should retain existing struct
> + * attributes from previous version
> + *
> + * 2. If an entity (libndctl or papr_scm) supports a PDSM struct T(X) then
> + * it should also support T(1), T(2)...T(X - 1).
> + * i.e When adding support for new version of a PDSM struct, libndctl
> + * and papr_scm should retain support of the existing PDSM struct
> + * version they support.
Please see this thread for an example why versions are a bad idea in UAPIs:
https://lkml.org/lkml/2020/3/26/213
While the use of version is different in that thread the fundamental issues are
the same. You end up with some weird matrix of supported features and
structure definitions. For example, you are opening up the possibility of
changing structures with a different version for no good reason.
Also having the user query with version Z and get back version X (older) is
odd. Generally if the kernel does not know about a feature (ie version Z of
the structure) it should return -EINVAL and let the user figure out what to do.
The user may just give up or they could try a different query.
> + */
> +
> +/* PDSM-header + payload expected with ND_CMD_CALL ioctl from libnvdimm */
> +struct nd_pdsm_cmd_pkg {
> + struct nd_cmd_pkg hdr; /* Package header containing sub-cmd */
> + __s32 cmd_status; /* Out: Sub-cmd status returned back */
> + __u16 reserved[5]; /* Ignored and to be used in future */
How do you know when reserved is used for something else in the future? Is
reserved guaranteed (and checked by the code) to be 0?
> + __u16 payload_version; /* In/Out: version of the payload */
Why is payload_version after reserved?
> + __u8 payload[]; /* In/Out: Sub-cmd data buffer */
> +} __packed;
> +
> +/*
> + * Methods to be embedded in ND_CMD_CALL request. These are sent to the kernel
> + * via 'nd_pdsm_cmd_pkg.hdr.nd_command' member of the ioctl struct
> + */
> +enum papr_pdsm {
> + PAPR_PDSM_MIN = 0x0,
> + PAPR_PDSM_MAX,
> +};
> +
> +/* Convert a libnvdimm nd_cmd_pkg to pdsm specific pkg */
> +static inline struct nd_pdsm_cmd_pkg *nd_to_pdsm_cmd_pkg(struct nd_cmd_pkg *cmd)
> +{
> + return (struct nd_pdsm_cmd_pkg *) cmd;
> +}
> +
> +/* Return the payload pointer for a given pcmd */
> +static inline void *pdsm_cmd_to_payload(struct nd_pdsm_cmd_pkg *pcmd)
> +{
> + if (pcmd->hdr.nd_size_in == 0 && pcmd->hdr.nd_size_out == 0)
> + return NULL;
> + else
> + return (void *)(pcmd->payload);
> +}
> +
> +#endif /* _UAPI_ASM_POWERPC_PAPR_PDSM_H_ */
> diff --git a/arch/powerpc/platforms/pseries/papr_scm.c b/arch/powerpc/platforms/pseries/papr_scm.c
> index 149431594839..5e2237e7ec08 100644
> --- a/arch/powerpc/platforms/pseries/papr_scm.c
> +++ b/arch/powerpc/platforms/pseries/papr_scm.c
> @@ -15,13 +15,15 @@
> #include <linux/seq_buf.h>
>
> #include <asm/plpar_wrappers.h>
> +#include <asm/papr_pdsm.h>
>
> #define BIND_ANY_ADDR (~0ul)
>
> #define PAPR_SCM_DIMM_CMD_MASK \
> ((1ul << ND_CMD_GET_CONFIG_SIZE) | \
> (1ul << ND_CMD_GET_CONFIG_DATA) | \
> - (1ul << ND_CMD_SET_CONFIG_DATA))
> + (1ul << ND_CMD_SET_CONFIG_DATA) | \
> + (1ul << ND_CMD_CALL))
>
> /* DIMM health bitmap bitmap indicators */
> /* SCM device is unable to persist memory contents */
> @@ -350,16 +352,97 @@ static int papr_scm_meta_set(struct papr_scm_priv *p,
> return 0;
> }
>
> +/*
> + * Validate the inputs args to dimm-control function and return '0' if valid.
> + * This also does initial sanity validation to ND_CMD_CALL sub-command packages.
> + */
> +static int is_cmd_valid(struct nvdimm *nvdimm, unsigned int cmd, void *buf,
> + unsigned int buf_len)
> +{
> + unsigned long cmd_mask = PAPR_SCM_DIMM_CMD_MASK;
> + struct nd_pdsm_cmd_pkg *pkg = nd_to_pdsm_cmd_pkg(buf);
> + struct papr_scm_priv *p;
> +
> + /* Only dimm-specific calls are supported atm */
> + if (!nvdimm)
> + return -EINVAL;
> +
> + /* get the provider date from struct nvdimm */
s/date/data
> + p = nvdimm_provider_data(nvdimm);
> +
> + if (!test_bit(cmd, &cmd_mask)) {
> + dev_dbg(&p->pdev->dev, "Unsupported cmd=%u\n", cmd);
> + return -EINVAL;
> + } else if (cmd == ND_CMD_CALL) {
> +
> + /* Verify the envelope package */
> + if (!buf || buf_len < sizeof(struct nd_pdsm_cmd_pkg)) {
> + dev_dbg(&p->pdev->dev, "Invalid pkg size=%u\n",
> + buf_len);
> + return -EINVAL;
> + }
> +
> + /* Verify that the PDSM family is valid */
> + if (pkg->hdr.nd_family != NVDIMM_FAMILY_PAPR) {
> + dev_dbg(&p->pdev->dev, "Invalid pkg family=0x%llx\n",
> + pkg->hdr.nd_family);
> + return -EINVAL;
> +
> + }
> +
> + /* We except a payload with all PDSM commands */
> + if (pdsm_cmd_to_payload(pkg) == NULL) {
> + dev_dbg(&p->pdev->dev,
> + "Empty payload for sub-command=0x%llx\n",
> + pkg->hdr.nd_command);
> + return -EINVAL;
> + }
> + }
> +
> + /* Command looks valid */
I assume the first command to be implemented also checks the { nd_command,
payload_version, payload length } for correctness?
> + return 0;
> +}
> +
> +static int papr_scm_service_pdsm(struct papr_scm_priv *p,
> + struct nd_pdsm_cmd_pkg *call_pkg)
> +{
> + /* unknown subcommands return error in packages */
> + if (call_pkg->hdr.nd_command <= PAPR_PDSM_MIN ||
> + call_pkg->hdr.nd_command >= PAPR_PDSM_MAX) {
> + dev_dbg(&p->pdev->dev, "Invalid PDSM request 0x%llx\n",
> + call_pkg->hdr.nd_command);
> + call_pkg->cmd_status = -EINVAL;
> + return 0;
> + }
> +
> + /* Depending on the DSM command call appropriate service routine */
> + switch (call_pkg->hdr.nd_command) {
> + default:
> + dev_dbg(&p->pdev->dev, "Unsupported PDSM request 0x%llx\n",
> + call_pkg->hdr.nd_command);
> + call_pkg->cmd_status = -ENOENT;
> + return 0;
> + }
> +}
> +
> static int papr_scm_ndctl(struct nvdimm_bus_descriptor *nd_desc,
> struct nvdimm *nvdimm, unsigned int cmd, void *buf,
> unsigned int buf_len, int *cmd_rc)
> {
> struct nd_cmd_get_config_size *get_size_hdr;
> struct papr_scm_priv *p;
> + struct nd_pdsm_cmd_pkg *call_pkg = NULL;
> + int rc;
>
> - /* Only dimm-specific calls are supported atm */
> - if (!nvdimm)
> - return -EINVAL;
> + /* Use a local variable in case cmd_rc pointer is NULL */
> + if (cmd_rc == NULL)
> + cmd_rc = &rc;
Why is this needed? AFAICT The caller of papr_scm_ndctl does not specify null
and you did not change it.
> +
> + *cmd_rc = is_cmd_valid(nvdimm, cmd, buf, buf_len);
> + if (*cmd_rc) {
> + pr_debug("Invalid cmd=0x%x. Err=%d\n", cmd, *cmd_rc);
> + return *cmd_rc;
> + }
>
> p = nvdimm_provider_data(nvdimm);
>
> @@ -381,13 +464,19 @@ static int papr_scm_ndctl(struct nvdimm_bus_descriptor *nd_desc,
> *cmd_rc = papr_scm_meta_set(p, buf);
> break;
>
> + case ND_CMD_CALL:
> + call_pkg = nd_to_pdsm_cmd_pkg(buf);
> + *cmd_rc = papr_scm_service_pdsm(p, call_pkg);
> + break;
> +
> default:
> - return -EINVAL;
> + dev_dbg(&p->pdev->dev, "Unknown command = %d\n", cmd);
> + *cmd_rc = -EINVAL;
Is this change related? If there is a bug where there is a caller of
papr_scm_ndctl() with cmd_rc == NULL this should be a separate patch to fix
that issue.
Ira
> }
>
> dev_dbg(&p->pdev->dev, "returned with cmd_rc = %d\n", *cmd_rc);
>
> - return 0;
> + return *cmd_rc;
> }
>
> static ssize_t flags_show(struct device *dev,
> diff --git a/include/uapi/linux/ndctl.h b/include/uapi/linux/ndctl.h
> index de5d90212409..0e09dc5cec19 100644
> --- a/include/uapi/linux/ndctl.h
> +++ b/include/uapi/linux/ndctl.h
> @@ -244,6 +244,7 @@ struct nd_cmd_pkg {
> #define NVDIMM_FAMILY_HPE2 2
> #define NVDIMM_FAMILY_MSFT 3
> #define NVDIMM_FAMILY_HYPERV 4
> +#define NVDIMM_FAMILY_PAPR 5
>
> #define ND_IOCTL_CALL _IOWR(ND_IOCTL, ND_CMD_CALL,\
> struct nd_cmd_pkg)
> --
> 2.26.2
>
^ permalink raw reply
* [PATCH v5 2/3] arch: wire-up close_range()
From: Christian Brauner @ 2020-06-02 20:42 UTC (permalink / raw)
To: torvalds, linux-kernel, Kyle Evans, Victor Stinner
Cc: linux-ia64, linux-sh, ldv, dhowells, sparclinux,
Christian Brauner, shuah, linux-arch, linux-s390, x86, linux-mips,
linux-xtensa, arnd, jannh, linux-m68k, viro, linux-arm-kernel,
fweimer, linux-parisc, linux-api, oleg, linux-alpha,
linux-fsdevel, linuxppc-dev
In-Reply-To: <20200602204219.186620-1-christian.brauner@ubuntu.com>
This wires up the close_range() syscall into all arches at once.
Suggested-by: Arnd Bergmann <arnd@arndb.de>
Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
Reviewed-by: Oleg Nesterov <oleg@redhat.com>
Acked-by: Arnd Bergmann <arnd@arndb.de>
Acked-by: Michael Ellerman <mpe@ellerman.id.au> (powerpc)
Cc: Jann Horn <jannh@google.com>
Cc: David Howells <dhowells@redhat.com>
Cc: Dmitry V. Levin <ldv@altlinux.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Florian Weimer <fweimer@redhat.com>
Cc: linux-api@vger.kernel.org
Cc: linux-alpha@vger.kernel.org
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-ia64@vger.kernel.org
Cc: linux-m68k@lists.linux-m68k.org
Cc: linux-mips@vger.kernel.org
Cc: linux-parisc@vger.kernel.org
Cc: linuxppc-dev@lists.ozlabs.org
Cc: linux-s390@vger.kernel.org
Cc: linux-sh@vger.kernel.org
Cc: sparclinux@vger.kernel.org
Cc: linux-xtensa@linux-xtensa.org
Cc: linux-arch@vger.kernel.org
Cc: x86@kernel.org
---
/* v2 */
not present
/* v3 */
not present
/* v4 */
introduced
- Arnd Bergmann <arnd@arndb.de>:
- split into two patches:
1. add close_range()
2. add syscall to all arches at once
- bump __NR_compat_syscalls in arch/arm64/include/asm/unistd.h
/* v5 */
unchanged
---
arch/alpha/kernel/syscalls/syscall.tbl | 1 +
arch/arm/tools/syscall.tbl | 1 +
arch/arm64/include/asm/unistd.h | 2 +-
arch/arm64/include/asm/unistd32.h | 2 ++
arch/ia64/kernel/syscalls/syscall.tbl | 1 +
arch/m68k/kernel/syscalls/syscall.tbl | 1 +
arch/microblaze/kernel/syscalls/syscall.tbl | 1 +
arch/mips/kernel/syscalls/syscall_n32.tbl | 1 +
arch/mips/kernel/syscalls/syscall_n64.tbl | 1 +
arch/mips/kernel/syscalls/syscall_o32.tbl | 1 +
arch/parisc/kernel/syscalls/syscall.tbl | 1 +
arch/powerpc/kernel/syscalls/syscall.tbl | 1 +
arch/s390/kernel/syscalls/syscall.tbl | 1 +
arch/sh/kernel/syscalls/syscall.tbl | 1 +
arch/sparc/kernel/syscalls/syscall.tbl | 1 +
arch/x86/entry/syscalls/syscall_32.tbl | 1 +
arch/x86/entry/syscalls/syscall_64.tbl | 1 +
arch/xtensa/kernel/syscalls/syscall.tbl | 1 +
include/uapi/asm-generic/unistd.h | 4 +++-
19 files changed, 22 insertions(+), 2 deletions(-)
diff --git a/arch/alpha/kernel/syscalls/syscall.tbl b/arch/alpha/kernel/syscalls/syscall.tbl
index 36d42da7466a..67ef02ead4da 100644
--- a/arch/alpha/kernel/syscalls/syscall.tbl
+++ b/arch/alpha/kernel/syscalls/syscall.tbl
@@ -475,5 +475,6 @@
543 common fspick sys_fspick
544 common pidfd_open sys_pidfd_open
# 545 reserved for clone3
+546 common close_range sys_close_range
547 common openat2 sys_openat2
548 common pidfd_getfd sys_pidfd_getfd
diff --git a/arch/arm/tools/syscall.tbl b/arch/arm/tools/syscall.tbl
index 4d1cf74a2caa..13c5652137fb 100644
--- a/arch/arm/tools/syscall.tbl
+++ b/arch/arm/tools/syscall.tbl
@@ -449,5 +449,6 @@
433 common fspick sys_fspick
434 common pidfd_open sys_pidfd_open
435 common clone3 sys_clone3
+436 common close_range sys_close_range
437 common openat2 sys_openat2
438 common pidfd_getfd sys_pidfd_getfd
diff --git a/arch/arm64/include/asm/unistd.h b/arch/arm64/include/asm/unistd.h
index 803039d504de..3b859596840d 100644
--- a/arch/arm64/include/asm/unistd.h
+++ b/arch/arm64/include/asm/unistd.h
@@ -38,7 +38,7 @@
#define __ARM_NR_compat_set_tls (__ARM_NR_COMPAT_BASE + 5)
#define __ARM_NR_COMPAT_END (__ARM_NR_COMPAT_BASE + 0x800)
-#define __NR_compat_syscalls 439
+#define __NR_compat_syscalls 440
#endif
#define __ARCH_WANT_SYS_CLONE
diff --git a/arch/arm64/include/asm/unistd32.h b/arch/arm64/include/asm/unistd32.h
index c1c61635f89c..902bfb136002 100644
--- a/arch/arm64/include/asm/unistd32.h
+++ b/arch/arm64/include/asm/unistd32.h
@@ -879,6 +879,8 @@ __SYSCALL(__NR_fspick, sys_fspick)
__SYSCALL(__NR_pidfd_open, sys_pidfd_open)
#define __NR_clone3 435
__SYSCALL(__NR_clone3, sys_clone3)
+#define __NR_close_range 436
+__SYSCALL(__NR_close_range, sys_close_range)
#define __NR_openat2 437
__SYSCALL(__NR_openat2, sys_openat2)
#define __NR_pidfd_getfd 438
diff --git a/arch/ia64/kernel/syscalls/syscall.tbl b/arch/ia64/kernel/syscalls/syscall.tbl
index 042911e670b8..df2e14da6a29 100644
--- a/arch/ia64/kernel/syscalls/syscall.tbl
+++ b/arch/ia64/kernel/syscalls/syscall.tbl
@@ -356,5 +356,6 @@
433 common fspick sys_fspick
434 common pidfd_open sys_pidfd_open
# 435 reserved for clone3
+436 common close_range sys_close_range
437 common openat2 sys_openat2
438 common pidfd_getfd sys_pidfd_getfd
diff --git a/arch/m68k/kernel/syscalls/syscall.tbl b/arch/m68k/kernel/syscalls/syscall.tbl
index f4f49fcb76d0..553b8858e667 100644
--- a/arch/m68k/kernel/syscalls/syscall.tbl
+++ b/arch/m68k/kernel/syscalls/syscall.tbl
@@ -435,5 +435,6 @@
433 common fspick sys_fspick
434 common pidfd_open sys_pidfd_open
435 common clone3 __sys_clone3
+436 common close_range sys_close_range
437 common openat2 sys_openat2
438 common pidfd_getfd sys_pidfd_getfd
diff --git a/arch/microblaze/kernel/syscalls/syscall.tbl b/arch/microblaze/kernel/syscalls/syscall.tbl
index 4c67b11f9c9e..4467e2211d3c 100644
--- a/arch/microblaze/kernel/syscalls/syscall.tbl
+++ b/arch/microblaze/kernel/syscalls/syscall.tbl
@@ -441,5 +441,6 @@
433 common fspick sys_fspick
434 common pidfd_open sys_pidfd_open
435 common clone3 sys_clone3
+436 common close_range sys_close_range
437 common openat2 sys_openat2
438 common pidfd_getfd sys_pidfd_getfd
diff --git a/arch/mips/kernel/syscalls/syscall_n32.tbl b/arch/mips/kernel/syscalls/syscall_n32.tbl
index 1f9e8ad636cc..82ad4cce163a 100644
--- a/arch/mips/kernel/syscalls/syscall_n32.tbl
+++ b/arch/mips/kernel/syscalls/syscall_n32.tbl
@@ -374,5 +374,6 @@
433 n32 fspick sys_fspick
434 n32 pidfd_open sys_pidfd_open
435 n32 clone3 __sys_clone3
+436 n32 close_range sys_close_range
437 n32 openat2 sys_openat2
438 n32 pidfd_getfd sys_pidfd_getfd
diff --git a/arch/mips/kernel/syscalls/syscall_n64.tbl b/arch/mips/kernel/syscalls/syscall_n64.tbl
index c0b9d802dbf6..232934c26d07 100644
--- a/arch/mips/kernel/syscalls/syscall_n64.tbl
+++ b/arch/mips/kernel/syscalls/syscall_n64.tbl
@@ -350,5 +350,6 @@
433 n64 fspick sys_fspick
434 n64 pidfd_open sys_pidfd_open
435 n64 clone3 __sys_clone3
+436 n64 close_range sys_close_range
437 n64 openat2 sys_openat2
438 n64 pidfd_getfd sys_pidfd_getfd
diff --git a/arch/mips/kernel/syscalls/syscall_o32.tbl b/arch/mips/kernel/syscalls/syscall_o32.tbl
index ac586774c980..d63000c8e769 100644
--- a/arch/mips/kernel/syscalls/syscall_o32.tbl
+++ b/arch/mips/kernel/syscalls/syscall_o32.tbl
@@ -423,5 +423,6 @@
433 o32 fspick sys_fspick
434 o32 pidfd_open sys_pidfd_open
435 o32 clone3 __sys_clone3
+436 o32 close_range sys_close_range
437 o32 openat2 sys_openat2
438 o32 pidfd_getfd sys_pidfd_getfd
diff --git a/arch/parisc/kernel/syscalls/syscall.tbl b/arch/parisc/kernel/syscalls/syscall.tbl
index 52a15f5cd130..8612458afda6 100644
--- a/arch/parisc/kernel/syscalls/syscall.tbl
+++ b/arch/parisc/kernel/syscalls/syscall.tbl
@@ -433,5 +433,6 @@
433 common fspick sys_fspick
434 common pidfd_open sys_pidfd_open
435 common clone3 sys_clone3_wrapper
+436 common close_range sys_close_range
437 common openat2 sys_openat2
438 common pidfd_getfd sys_pidfd_getfd
diff --git a/arch/powerpc/kernel/syscalls/syscall.tbl b/arch/powerpc/kernel/syscalls/syscall.tbl
index 220ae11555f2..ac92f5d7279d 100644
--- a/arch/powerpc/kernel/syscalls/syscall.tbl
+++ b/arch/powerpc/kernel/syscalls/syscall.tbl
@@ -525,5 +525,6 @@
435 32 clone3 ppc_clone3 sys_clone3
435 64 clone3 sys_clone3
435 spu clone3 sys_ni_syscall
+436 common close_range sys_close_range
437 common openat2 sys_openat2
438 common pidfd_getfd sys_pidfd_getfd
diff --git a/arch/s390/kernel/syscalls/syscall.tbl b/arch/s390/kernel/syscalls/syscall.tbl
index bd7bd3581a0f..371bb1f2bfc3 100644
--- a/arch/s390/kernel/syscalls/syscall.tbl
+++ b/arch/s390/kernel/syscalls/syscall.tbl
@@ -438,5 +438,6 @@
433 common fspick sys_fspick sys_fspick
434 common pidfd_open sys_pidfd_open sys_pidfd_open
435 common clone3 sys_clone3 sys_clone3
+436 common close_range sys_close_range sys_close_range
437 common openat2 sys_openat2 sys_openat2
438 common pidfd_getfd sys_pidfd_getfd sys_pidfd_getfd
diff --git a/arch/sh/kernel/syscalls/syscall.tbl b/arch/sh/kernel/syscalls/syscall.tbl
index c7a30fcd135f..4db428e7acdd 100644
--- a/arch/sh/kernel/syscalls/syscall.tbl
+++ b/arch/sh/kernel/syscalls/syscall.tbl
@@ -438,5 +438,6 @@
433 common fspick sys_fspick
434 common pidfd_open sys_pidfd_open
# 435 reserved for clone3
+436 common close_range sys_close_range
437 common openat2 sys_openat2
438 common pidfd_getfd sys_pidfd_getfd
diff --git a/arch/sparc/kernel/syscalls/syscall.tbl b/arch/sparc/kernel/syscalls/syscall.tbl
index f13615ecdecc..c9233f79a11b 100644
--- a/arch/sparc/kernel/syscalls/syscall.tbl
+++ b/arch/sparc/kernel/syscalls/syscall.tbl
@@ -481,5 +481,6 @@
433 common fspick sys_fspick
434 common pidfd_open sys_pidfd_open
# 435 reserved for clone3
+436 common close_range sys_close_range
437 common openat2 sys_openat2
438 common pidfd_getfd sys_pidfd_getfd
diff --git a/arch/x86/entry/syscalls/syscall_32.tbl b/arch/x86/entry/syscalls/syscall_32.tbl
index 54581ac671b4..49ea7190351a 100644
--- a/arch/x86/entry/syscalls/syscall_32.tbl
+++ b/arch/x86/entry/syscalls/syscall_32.tbl
@@ -440,5 +440,6 @@
433 i386 fspick sys_fspick
434 i386 pidfd_open sys_pidfd_open
435 i386 clone3 sys_clone3
+436 i386 close_range sys_close_range
437 i386 openat2 sys_openat2
438 i386 pidfd_getfd sys_pidfd_getfd
diff --git a/arch/x86/entry/syscalls/syscall_64.tbl b/arch/x86/entry/syscalls/syscall_64.tbl
index 37b844f839bc..c2b50b16a24c 100644
--- a/arch/x86/entry/syscalls/syscall_64.tbl
+++ b/arch/x86/entry/syscalls/syscall_64.tbl
@@ -357,6 +357,7 @@
433 common fspick sys_fspick
434 common pidfd_open sys_pidfd_open
435 common clone3 sys_clone3
+436 common close_range sys_close_range
437 common openat2 sys_openat2
438 common pidfd_getfd sys_pidfd_getfd
diff --git a/arch/xtensa/kernel/syscalls/syscall.tbl b/arch/xtensa/kernel/syscalls/syscall.tbl
index 85a9ab1bc04d..d5dd7e506893 100644
--- a/arch/xtensa/kernel/syscalls/syscall.tbl
+++ b/arch/xtensa/kernel/syscalls/syscall.tbl
@@ -406,5 +406,6 @@
433 common fspick sys_fspick
434 common pidfd_open sys_pidfd_open
435 common clone3 sys_clone3
+436 common close_range sys_close_range
437 common openat2 sys_openat2
438 common pidfd_getfd sys_pidfd_getfd
diff --git a/include/uapi/asm-generic/unistd.h b/include/uapi/asm-generic/unistd.h
index 3a3201e4618e..ed4e7c2a557f 100644
--- a/include/uapi/asm-generic/unistd.h
+++ b/include/uapi/asm-generic/unistd.h
@@ -850,6 +850,8 @@ __SYSCALL(__NR_pidfd_open, sys_pidfd_open)
#define __NR_clone3 435
__SYSCALL(__NR_clone3, sys_clone3)
#endif
+#define __NR_close_range 436
+__SYSCALL(__NR_close_range, sys_close_range)
#define __NR_openat2 437
__SYSCALL(__NR_openat2, sys_openat2)
@@ -857,7 +859,7 @@ __SYSCALL(__NR_openat2, sys_openat2)
__SYSCALL(__NR_pidfd_getfd, sys_pidfd_getfd)
#undef __NR_syscalls
-#define __NR_syscalls 439
+#define __NR_syscalls 440
/*
* 32 bit systems traditionally used different
--
2.26.2
^ permalink raw reply related
* Re: [PATCH 1/4] powerpc: Add a ppc_inst_as_str() helper
From: Segher Boessenkool @ 2020-06-02 19:41 UTC (permalink / raw)
To: Jordan Niethe; +Cc: linuxppc-dev, alistair
In-Reply-To: <20200602052728.18227-1-jniethe5@gmail.com>
On Tue, Jun 02, 2020 at 03:27:25PM +1000, Jordan Niethe wrote:
> There are quite a few places where instructions are printed, this is
> done using a '%x' format specifier. With the introduction of prefixed
> instructions, this does not work well. Currently in these places,
> ppc_inst_val() is used for the value for %x so only the first word of
> prefixed instructions are printed.
>
> When the instructions are word instructions, only a single word should
> be printed. For prefixed instructions both the prefix and suffix should
> be printed. To accommodate both of these situations, instead of a '%x'
> specifier use '%s' and introduce a helper, __ppc_inst_as_str() which
> returns a char *. The char * __ppc_inst_as_str() returns is buffer that
> is passed to it by the caller.
>
> It is cumbersome to require every caller of __ppc_inst_as_str() to now
> declare a buffer. To make it more convenient to use __ppc_inst_as_str(),
> wrap it in a macro that uses a compound statement to allocate a buffer
> on the caller's stack before calling it.
>
> Signed-off-by: Jordan Niethe <jniethe5@gmail.com>
Acked-by: Segher Boessenkool <segher@kernel.crashing.org>
> +#define PPC_INST_STR_LEN sizeof("0x00000000 0x00000000")
sizeof is not a function or anything function-like; it's just an
operator, like (unary) "+" or "-" or dereference "*". The parentheses
are completely redundant here. And it kind of matters, as written a
reader might think that a string object is actually constructed here.
But, so very many people do this, I'm not going to fight this fight ;-)
Segher
^ permalink raw reply
* Re: [PATCH 11/14] x86/entry: Clarify irq_{enter,exit}_rcu()
From: Qian Cai @ 2020-06-02 18:47 UTC (permalink / raw)
To: Peter Zijlstra
Cc: daniel.thompson, andrew.cooper3, bigeasy, x86, linux-kernel,
sean.j.christopherson, luto, Lai Jiangshan, rostedt, a.darwish,
tglx, linuxppc-dev
In-Reply-To: <20200602150511.GH706478@hirez.programming.kicks-ass.net>
On Tue, Jun 02, 2020 at 05:05:11PM +0200, Peter Zijlstra wrote:
> On Tue, Jun 02, 2020 at 10:42:35AM -0400, Qian Cai wrote:
>
> > Reverted this commit fixed the POWER9 boot warning,
>
> ARGH, I'm an idiot. Please try this instead:
>
>
> diff --git a/kernel/softirq.c b/kernel/softirq.c
> index a3eb6eba8c41..c4201b7f42b1 100644
> --- a/kernel/softirq.c
> +++ b/kernel/softirq.c
> @@ -438,7 +438,7 @@ void irq_exit_rcu(void)
> */
> void irq_exit(void)
> {
> - irq_exit_rcu();
> + __irq_exit_rcu();
> rcu_irq_exit();
> /* must be last! */
> lockdep_hardirq_exit();
This works fine.
^ permalink raw reply
* Re: [RESEND PATCH v9 3/5] powerpc/papr_scm: Fetch nvdimm health information from PHYP
From: Ira Weiny @ 2020-06-02 18:45 UTC (permalink / raw)
To: Vaibhav Jain
Cc: Santosh Sivaraj, linux-nvdimm, linux-kernel, Steven Rostedt,
Oliver O'Halloran, Aneesh Kumar K . V, Dan Williams,
linuxppc-dev
In-Reply-To: <20200602101438.73929-4-vaibhav@linux.ibm.com>
On Tue, Jun 02, 2020 at 03:44:36PM +0530, Vaibhav Jain wrote:
> Implement support for fetching nvdimm health information via
> H_SCM_HEALTH hcall as documented in Ref[1]. The hcall returns a pair
> of 64-bit bitmap, bitwise-and of which is then stored in
> 'struct papr_scm_priv' and subsequently partially exposed to
> user-space via newly introduced dimm specific attribute
> 'papr/flags'. Since the hcall is costly, the health information is
> cached and only re-queried, 60s after the previous successful hcall.
>
> The patch also adds a documentation text describing flags reported by
> the the new sysfs attribute 'papr/flags' is also introduced at
> Documentation/ABI/testing/sysfs-bus-papr-pmem.
>
> [1] commit 58b278f568f0 ("powerpc: Provide initial documentation for
> PAPR hcalls")
>
> Cc: "Aneesh Kumar K . V" <aneesh.kumar@linux.ibm.com>
> Cc: Dan Williams <dan.j.williams@intel.com>
> Cc: Michael Ellerman <mpe@ellerman.id.au>
> Cc: Ira Weiny <ira.weiny@intel.com>
> Reviewed-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
> Signed-off-by: Vaibhav Jain <vaibhav@linux.ibm.com>
> ---
> Changelog:
>
> Resend:
> * Added ack from Aneesh.
>
> v8..v9:
> * Rename some variables and defines to reduce usage of term SCM
> replacing it with PMEM [Dan Williams, Aneesh]
> * s/PAPR_SCM_DIMM/PAPR_PMEM/g
> * s/papr_scm_nd_attributes/papr_nd_attributes/g
> * s/papr_scm_nd_attribute_group/papr_nd_attribute_group/g
> * s/papr_scm_dimm_attr_groups/papr_nd_attribute_groups/g
> * Renamed file sysfs-bus-papr-scm to sysfs-bus-papr-pmem
>
> v7..v8:
> * Update type of variable 'rc' in __drc_pmem_query_health() and
> drc_pmem_query_health() to long and int respectively. [ Ira ]
> * Updated the patch description to s/64 bit Big Endian Number/64-bit
> bitmap/ [ Ira, Aneesh ].
>
> Resend:
> * None
>
> v6..v7 :
> * Used the exported buf_seq_printf() function to generate content for
> 'papr/flags'
> * Moved the PAPR_SCM_DIMM_* bit-flags macro definitions to papr_scm.c
> and removed the papr_scm.h file [Mpe]
> * Some minor consistency issued in sysfs-bus-papr-scm
> documentation. [Mpe]
> * s/dimm_mutex/health_mutex/g [Mpe]
> * Split drc_pmem_query_health() into two function one of which takes
> care of caching and locking. [Mpe]
> * Fixed a local copy creation of dimm health information using
> READ_ONCE(). [Mpe]
>
> v5..v6 :
> * Change the flags sysfs attribute from 'papr_flags' to 'papr/flags'
> [Dan Williams]
> * Include documentation for 'papr/flags' attr [Dan Williams]
> * Change flag 'save_fail' to 'flush_fail' [Dan Williams]
> * Caching of health bitmap to reduce expensive hcalls [Dan Williams]
> * Removed usage of PPC_BIT from 'papr-scm.h' header [Mpe]
> * Replaced two __be64 integers from papr_scm_priv to a single u64
> integer [Mpe]
> * Updated patch description to reflect the changes made in this
> version.
> * Removed avoidable usage of 'papr_scm_priv.dimm_mutex' from
> flags_show() [Dan Williams]
>
> v4..v5 :
> * None
>
> v3..v4 :
> * None
>
> v2..v3 :
> * Removed PAPR_SCM_DIMM_HEALTH_NON_CRITICAL as a condition for
> NVDIMM unarmed [Aneesh]
>
> v1..v2 :
> * New patch in the series.
> ---
> Documentation/ABI/testing/sysfs-bus-papr-pmem | 27 +++
> arch/powerpc/platforms/pseries/papr_scm.c | 169 +++++++++++++++++-
> 2 files changed, 194 insertions(+), 2 deletions(-)
> create mode 100644 Documentation/ABI/testing/sysfs-bus-papr-pmem
>
> diff --git a/Documentation/ABI/testing/sysfs-bus-papr-pmem b/Documentation/ABI/testing/sysfs-bus-papr-pmem
> new file mode 100644
> index 000000000000..5b10d036a8d4
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-bus-papr-pmem
> @@ -0,0 +1,27 @@
> +What: /sys/bus/nd/devices/nmemX/papr/flags
> +Date: Apr, 2020
> +KernelVersion: v5.8
> +Contact: linuxppc-dev <linuxppc-dev@lists.ozlabs.org>, linux-nvdimm@lists.01.org,
> +Description:
> + (RO) Report flags indicating various states of a
> + papr-pmem NVDIMM device. Each flag maps to a one or
> + more bits set in the dimm-health-bitmap retrieved in
> + response to H_SCM_HEALTH hcall. The details of the bit
> + flags returned in response to this hcall is available
> + at 'Documentation/powerpc/papr_hcalls.rst' . Below are
> + the flags reported in this sysfs file:
> +
> + * "not_armed" : Indicates that NVDIMM contents will not
> + survive a power cycle.
> + * "flush_fail" : Indicates that NVDIMM contents
> + couldn't be flushed during last
> + shut-down event.
> + * "restore_fail": Indicates that NVDIMM contents
> + couldn't be restored during NVDIMM
> + initialization.
> + * "encrypted" : NVDIMM contents are encrypted.
> + * "smart_notify": There is health event for the NVDIMM.
> + * "scrubbed" : Indicating that contents of the
> + NVDIMM have been scrubbed.
> + * "locked" : Indicating that NVDIMM contents cant
> + be modified until next power cycle.
> diff --git a/arch/powerpc/platforms/pseries/papr_scm.c b/arch/powerpc/platforms/pseries/papr_scm.c
> index f35592423380..149431594839 100644
> --- a/arch/powerpc/platforms/pseries/papr_scm.c
> +++ b/arch/powerpc/platforms/pseries/papr_scm.c
> @@ -12,6 +12,7 @@
> #include <linux/libnvdimm.h>
> #include <linux/platform_device.h>
> #include <linux/delay.h>
> +#include <linux/seq_buf.h>
>
> #include <asm/plpar_wrappers.h>
>
> @@ -22,6 +23,44 @@
> (1ul << ND_CMD_GET_CONFIG_DATA) | \
> (1ul << ND_CMD_SET_CONFIG_DATA))
>
> +/* DIMM health bitmap bitmap indicators */
> +/* SCM device is unable to persist memory contents */
> +#define PAPR_PMEM_UNARMED (1ULL << (63 - 0))
> +/* SCM device failed to persist memory contents */
> +#define PAPR_PMEM_SHUTDOWN_DIRTY (1ULL << (63 - 1))
> +/* SCM device contents are persisted from previous IPL */
> +#define PAPR_PMEM_SHUTDOWN_CLEAN (1ULL << (63 - 2))
> +/* SCM device contents are not persisted from previous IPL */
> +#define PAPR_PMEM_EMPTY (1ULL << (63 - 3))
> +/* SCM device memory life remaining is critically low */
> +#define PAPR_PMEM_HEALTH_CRITICAL (1ULL << (63 - 4))
> +/* SCM device will be garded off next IPL due to failure */
> +#define PAPR_PMEM_HEALTH_FATAL (1ULL << (63 - 5))
> +/* SCM contents cannot persist due to current platform health status */
> +#define PAPR_PMEM_HEALTH_UNHEALTHY (1ULL << (63 - 6))
> +/* SCM device is unable to persist memory contents in certain conditions */
> +#define PAPR_PMEM_HEALTH_NON_CRITICAL (1ULL << (63 - 7))
> +/* SCM device is encrypted */
> +#define PAPR_PMEM_ENCRYPTED (1ULL << (63 - 8))
> +/* SCM device has been scrubbed and locked */
> +#define PAPR_PMEM_SCRUBBED_AND_LOCKED (1ULL << (63 - 9))
> +
> +/* Bits status indicators for health bitmap indicating unarmed dimm */
> +#define PAPR_PMEM_UNARMED_MASK (PAPR_PMEM_UNARMED | \
> + PAPR_PMEM_HEALTH_UNHEALTHY)
> +
> +/* Bits status indicators for health bitmap indicating unflushed dimm */
> +#define PAPR_PMEM_BAD_SHUTDOWN_MASK (PAPR_PMEM_SHUTDOWN_DIRTY)
> +
> +/* Bits status indicators for health bitmap indicating unrestored dimm */
> +#define PAPR_PMEM_BAD_RESTORE_MASK (PAPR_PMEM_EMPTY)
> +
> +/* Bit status indicators for smart event notification */
> +#define PAPR_PMEM_SMART_EVENT_MASK (PAPR_PMEM_HEALTH_CRITICAL | \
> + PAPR_PMEM_HEALTH_FATAL | \
> + PAPR_PMEM_HEALTH_UNHEALTHY)
> +
> +/* private struct associated with each region */
> struct papr_scm_priv {
> struct platform_device *pdev;
> struct device_node *dn;
> @@ -39,6 +78,15 @@ struct papr_scm_priv {
> struct resource res;
> struct nd_region *region;
> struct nd_interleave_set nd_set;
> +
> + /* Protect dimm health data from concurrent read/writes */
> + struct mutex health_mutex;
I question if this really needs protection. But I don't think it hurts.
Reviewed-by: Ira Weiny <ira.weiny@intel.com>
> +
> + /* Last time the health information of the dimm was updated */
> + unsigned long lasthealth_jiffies;
> +
> + /* Health information for the dimm */
> + u64 health_bitmap;
> };
>
> static int drc_pmem_bind(struct papr_scm_priv *p)
> @@ -144,6 +192,62 @@ static int drc_pmem_query_n_bind(struct papr_scm_priv *p)
> return drc_pmem_bind(p);
> }
>
> +/*
> + * Issue hcall to retrieve dimm health info and populate papr_scm_priv with the
> + * health information.
> + */
> +static int __drc_pmem_query_health(struct papr_scm_priv *p)
> +{
> + unsigned long ret[PLPAR_HCALL_BUFSIZE];
> + long rc;
> +
> + /* issue the hcall */
> + rc = plpar_hcall(H_SCM_HEALTH, ret, p->drc_index);
> + if (rc != H_SUCCESS) {
> + dev_err(&p->pdev->dev,
> + "Failed to query health information, Err:%ld\n", rc);
> + rc = -ENXIO;
> + goto out;
> + }
> +
> + p->lasthealth_jiffies = jiffies;
> + p->health_bitmap = ret[0] & ret[1];
> +
> + dev_dbg(&p->pdev->dev,
> + "Queried dimm health info. Bitmap:0x%016lx Mask:0x%016lx\n",
> + ret[0], ret[1]);
> +out:
> + return rc;
> +}
> +
> +/* Min interval in seconds for assuming stable dimm health */
> +#define MIN_HEALTH_QUERY_INTERVAL 60
> +
> +/* Query cached health info and if needed call drc_pmem_query_health */
> +static int drc_pmem_query_health(struct papr_scm_priv *p)
> +{
> + unsigned long cache_timeout;
> + int rc;
> +
> + /* Protect concurrent modifications to papr_scm_priv */
> + rc = mutex_lock_interruptible(&p->health_mutex);
> + if (rc)
> + return rc;
> +
> + /* Jiffies offset for which the health data is assumed to be same */
> + cache_timeout = p->lasthealth_jiffies +
> + msecs_to_jiffies(MIN_HEALTH_QUERY_INTERVAL * 1000);
> +
> + /* Fetch new health info is its older than MIN_HEALTH_QUERY_INTERVAL */
> + if (time_after(jiffies, cache_timeout))
> + rc = __drc_pmem_query_health(p);
> + else
> + /* Assume cached health data is valid */
> + rc = 0;
> +
> + mutex_unlock(&p->health_mutex);
> + return rc;
> +}
>
> static int papr_scm_meta_get(struct papr_scm_priv *p,
> struct nd_cmd_get_config_data_hdr *hdr)
> @@ -286,6 +390,64 @@ static int papr_scm_ndctl(struct nvdimm_bus_descriptor *nd_desc,
> return 0;
> }
>
> +static ssize_t flags_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct nvdimm *dimm = to_nvdimm(dev);
> + struct papr_scm_priv *p = nvdimm_provider_data(dimm);
> + struct seq_buf s;
> + u64 health;
> + int rc;
> +
> + rc = drc_pmem_query_health(p);
> + if (rc)
> + return rc;
> +
> + /* Copy health_bitmap locally, check masks & update out buffer */
> + health = READ_ONCE(p->health_bitmap);
> +
> + seq_buf_init(&s, buf, PAGE_SIZE);
> + if (health & PAPR_PMEM_UNARMED_MASK)
> + seq_buf_printf(&s, "not_armed ");
> +
> + if (health & PAPR_PMEM_BAD_SHUTDOWN_MASK)
> + seq_buf_printf(&s, "flush_fail ");
> +
> + if (health & PAPR_PMEM_BAD_RESTORE_MASK)
> + seq_buf_printf(&s, "restore_fail ");
> +
> + if (health & PAPR_PMEM_ENCRYPTED)
> + seq_buf_printf(&s, "encrypted ");
> +
> + if (health & PAPR_PMEM_SMART_EVENT_MASK)
> + seq_buf_printf(&s, "smart_notify ");
> +
> + if (health & PAPR_PMEM_SCRUBBED_AND_LOCKED)
> + seq_buf_printf(&s, "scrubbed locked ");
> +
> + if (seq_buf_used(&s))
> + seq_buf_printf(&s, "\n");
> +
> + return seq_buf_used(&s);
> +}
> +DEVICE_ATTR_RO(flags);
> +
> +/* papr_scm specific dimm attributes */
> +static struct attribute *papr_nd_attributes[] = {
> + &dev_attr_flags.attr,
> + NULL,
> +};
> +
> +static struct attribute_group papr_nd_attribute_group = {
> + .name = "papr",
> + .attrs = papr_nd_attributes,
> +};
> +
> +static const struct attribute_group *papr_nd_attr_groups[] = {
> + &papr_nd_attribute_group,
> + NULL,
> +};
> +
> static int papr_scm_nvdimm_init(struct papr_scm_priv *p)
> {
> struct device *dev = &p->pdev->dev;
> @@ -312,8 +474,8 @@ static int papr_scm_nvdimm_init(struct papr_scm_priv *p)
> dimm_flags = 0;
> set_bit(NDD_LABELING, &dimm_flags);
>
> - p->nvdimm = nvdimm_create(p->bus, p, NULL, dimm_flags,
> - PAPR_SCM_DIMM_CMD_MASK, 0, NULL);
> + p->nvdimm = nvdimm_create(p->bus, p, papr_nd_attr_groups,
> + dimm_flags, PAPR_SCM_DIMM_CMD_MASK, 0, NULL);
> if (!p->nvdimm) {
> dev_err(dev, "Error creating DIMM object for %pOF\n", p->dn);
> goto err;
> @@ -399,6 +561,9 @@ static int papr_scm_probe(struct platform_device *pdev)
> if (!p)
> return -ENOMEM;
>
> + /* Initialize the dimm mutex */
> + mutex_init(&p->health_mutex);
> +
> /* optional DT properties */
> of_property_read_u32(dn, "ibm,metadata-size", &metadata_size);
>
> --
> 2.26.2
>
^ permalink raw reply
* RE: [RFC PATCH 1/2] libnvdimm: Add prctl control for disabling synchronous fault support.
From: Williams, Dan J @ 2020-06-02 17:59 UTC (permalink / raw)
To: Jan Kara
Cc: linux-nvdimm, Aneesh Kumar K.V, jack@suse.de, Jeff Moyer,
Oliver O'Halloran, Michal Suchánek, linuxppc-dev
In-Reply-To: <20200601095049.GB3960@quack2.suse.cz>
[ forgive formatting, a series of unfortunate events has me using Outlook for the moment ]
> From: Jan Kara <jack@suse.cz>
> > > > These flags are device properties that affect the kernel and
> > > > userspace's handling of persistence.
> > > >
> > >
> > > That will not handle the scenario with multiple applications using
> > > the same fsdax mount point where one is updated to use the new
> > > instruction and the other is not.
> >
> > Right, it needs to be a global setting / flag day to switch from one
> > regime to another. Per-process control is a recipe for disaster.
>
> First I'd like to mention that hopefully the concern is mostly theoretical since
> as Aneesh wrote above, real persistent memory never shipped for PPC and
> so there are very few apps (if any) using the old way to ensure cache
> flushing.
>
> But I'd like to understand why do you think per-process control is a recipe for
> disaster? Because from my POV the sysfs interface you propose is actually
> difficult to use in practice. As a distributor, you have hard time picking the
> default because you have a choice between picking safe option which is
> going to confuse users because of failing MAP_SYNC and unsafe option
> where everyone will be happy until someone looses data because of some
> ancient application using wrong instructions to persist data. Poor experience
> for users in either way. And when distro defaults to "safe option", then the
> burden is on the sysadmin to toggle the switch but how is he supposed to
> decide when that is safe? First he has to understand what the problem
> actually is, then he has to audit all the applications using pmem whether they
> use the new instruction - which is IMO a lot of effort if you have a couple of
> applications and practically infeasible if you have more of them.
> So IMO the burden should be *on the application* to declare that it is aware
> of the new instructions to flush pmem on the platform and only to such
> application the kernel should give the trust to use MAP_SYNC mappings.
The "disaster" in my mind is this need to globally change the ABI for persistence semantics for all of Linux because one CPU wants a do over. What does a generic "MAP_SYNC_ENABLE" knob even mean to the existing deployed base of persistent memory applications? Yes, sysfs is awkward, but it's trying to provide some relief without imposing unexplainable semantics on everyone else. I think a comprehensive (overengineered) solution would involve not introducing another "I know what I'm doing" flag to the interface, but maybe requiring applications to call a pmem sync API in something like a vsyscall. Or, also overengineered, some binary translation / interpretation to actively detect and kill applications that deploy the old instructions. Something horrid like on first write fault to a MAP_SYNC try to look ahead in the binary for the correct sync sequence and kill the application otherwise. That would at least provide some enforcement and safety without requiring other architectures to consider what MAP_SYNC_ENABLE means to them.
^ permalink raw reply
* Re: ppc64le and 32-bit LE userland compatibility
From: Segher Boessenkool @ 2020-06-02 17:47 UTC (permalink / raw)
To: Joseph Myers
Cc: libc-alpha, eery, Daniel Kolesa, musl, Will Springer,
Palmer Dabbelt via binutils, via libc-dev, linuxppc-dev
In-Reply-To: <alpine.DEB.2.21.2006021342440.24059@digraph.polyomino.org.uk>
On Tue, Jun 02, 2020 at 01:50:32PM +0000, Joseph Myers wrote:
> On Mon, 1 Jun 2020, Segher Boessenkool wrote:
>
> > > The supported glibc ABIs are listed at
> > > <https://sourceware.org/glibc/wiki/ABIList>.
> >
> > powerpcle-linux already does work somewhat, and that should also he
> > worth something, official or not ;-)
> >
> > (It has worked for very many years already... That is, I have built it
> > a lot, I have no idea about running a full distro that way).
>
> Greg McGary's patches as referenced at
> <https://sourceware.org/legacy-ml/libc-hacker/2006-11/msg00013.html> were
> never merged, and I don't know how big the changes to .S files were or if
> they were ever posted. Many files were subsequently fixed as part of
> bringing up support for powerpc64le, but without actual 32-bit LE testing
> as part of each release cycle there's not much evidence of correctness for
> LE of code not used for powerpc64le.
Yeah sorry, I meant I have built the toolchain parts a lot, not libc.
GCC and binutils. I reckon there is more work to do in libc, certainly
for configuration and similar.
Segher
^ permalink raw reply
* Re: [PATCH] cxl: Fix kobject memory leak in cxl_sysfs_afu_new_cr()
From: Markus Elfring @ 2020-06-02 17:20 UTC (permalink / raw)
To: Wang Hai, linuxppc-dev
Cc: Andrew Donnellan, Arnd Bergmann, Greg Kroah-Hartman,
kernel-janitors, linux-kernel, Ian Munsie, Frederic Barrat
> Fix it by adding a call to kobject_put() in the error path of
> kobject_init_and_add().
Thanks for another completion of the exception handling.
Would an other patch subject be a bit nicer?
…
> +++ b/drivers/misc/cxl/sysfs.c
> @@ -624,7 +624,7 @@ static struct afu_config_record *cxl_sysfs_afu_new_cr(struct cxl_afu *afu, int c
> rc = kobject_init_and_add(&cr->kobj, &afu_config_record_type,
> &afu->dev.kobj, "cr%i", cr->cr);
> if (rc)
> - goto err;
> + goto err1;
…
Can an other label be more reasonable here?
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/coding-style.rst?id=f359287765c04711ff54fbd11645271d8e5ff763#n465
Regards,
Markus
^ permalink raw reply
* Re: [PATCH] cxl: Fix kobject memleak
From: Frederic Barrat @ 2020-06-02 16:21 UTC (permalink / raw)
To: Wang Hai, ajd, arnd, gregkh; +Cc: linuxppc-dev, imunsie, linux-kernel
In-Reply-To: <20200602120733.5943-1-wanghai38@huawei.com>
Le 02/06/2020 à 14:07, Wang Hai a écrit :
> Currently the error return path from kobject_init_and_add() is not
> followed by a call to kobject_put() - which means we are leaking
> the kobject.
>
> Fix it by adding a call to kobject_put() in the error path of
> kobject_init_and_add().
>
> Fixes: b087e6190ddc ("cxl: Export optional AFU configuration record in sysfs")
> Reported-by: Hulk Robot <hulkci@huawei.com>
> Signed-off-by: Wang Hai <wanghai38@huawei.com>
Indeed, a call to kobject_put() is needed when the init fails.
Thanks!
Acked-by: Frederic Barrat <fbarrat@linux.ibm.com>
> ---
> drivers/misc/cxl/sysfs.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/misc/cxl/sysfs.c b/drivers/misc/cxl/sysfs.c
> index f0263d1..d97a243 100644
> --- a/drivers/misc/cxl/sysfs.c
> +++ b/drivers/misc/cxl/sysfs.c
> @@ -624,7 +624,7 @@ static struct afu_config_record *cxl_sysfs_afu_new_cr(struct cxl_afu *afu, int c
> rc = kobject_init_and_add(&cr->kobj, &afu_config_record_type,
> &afu->dev.kobj, "cr%i", cr->cr);
> if (rc)
> - goto err;
> + goto err1;
>
> rc = sysfs_create_bin_file(&cr->kobj, &cr->config_attr);
> if (rc)
>
^ permalink raw reply
* Re: [PATCH v2] cxl: Remove dead Kconfig option
From: Frederic Barrat @ 2020-06-02 16:10 UTC (permalink / raw)
To: Andrew Donnellan, linuxppc-dev
In-Reply-To: <20200602070545.11942-1-ajd@linux.ibm.com>
Le 02/06/2020 à 09:05, Andrew Donnellan a écrit :
> The CXL_AFU_DRIVER_OPS Kconfig option was added to coordinate merging of
> new features. It no longer serves any purpose, so remove it.
>
> Signed-off-by: Andrew Donnellan <ajd@linux.ibm.com>
Acked-by: Frederic Barrat <fbarrat@linux.ibm.com>
>
> ---
> v1->v2:
> - keep CXL_LIB for now to avoid breaking a driver that is currently out of
> tree
> ---
> drivers/misc/cxl/Kconfig | 4 ----
> 1 file changed, 4 deletions(-)
>
> diff --git a/drivers/misc/cxl/Kconfig b/drivers/misc/cxl/Kconfig
> index 39eec9031487..984114f7cee9 100644
> --- a/drivers/misc/cxl/Kconfig
> +++ b/drivers/misc/cxl/Kconfig
> @@ -7,9 +7,6 @@ config CXL_BASE
> bool
> select PPC_COPRO_BASE
>
> -config CXL_AFU_DRIVER_OPS
> - bool
> -
> config CXL_LIB
> bool
>
> @@ -17,7 +14,6 @@ config CXL
> tristate "Support for IBM Coherent Accelerators (CXL)"
> depends on PPC_POWERNV && PCI_MSI && EEH
> select CXL_BASE
> - select CXL_AFU_DRIVER_OPS
> select CXL_LIB
> default m
> help
>
^ permalink raw reply
* Re: ppc64le and 32-bit LE userland compatibility
From: Michal Suchánek @ 2020-06-02 15:56 UTC (permalink / raw)
To: Daniel Kolesa
Cc: libc-alpha, eery, musl, Will Springer,
Palmer Dabbelt via binutils, via libc-dev, linuxppc-dev,
Joseph Myers
In-Reply-To: <454e0d68-d69e-43fc-9a8c-0461dd5817a9@www.fastmail.com>
On Tue, Jun 02, 2020 at 05:40:39PM +0200, Daniel Kolesa wrote:
>
>
> On Tue, Jun 2, 2020, at 17:27, Michal Suchánek wrote:
> > On Tue, Jun 02, 2020 at 05:13:25PM +0200, Daniel Kolesa wrote:
> > > On Tue, Jun 2, 2020, at 16:23, Michal Suchánek wrote:
> > > > On Tue, Jun 02, 2020 at 01:40:23PM +0000, Joseph Myers wrote:
> > > > > On Tue, 2 Jun 2020, Daniel Kolesa wrote:
> > > > >
> > > > > > not be limited to being just userspace under ppc64le, but should be
> > > > > > runnable on a native kernel as well, which should not be limited to any
> > > > > > particular baseline other than just PowerPC.
> > > > >
> > > > > This is a fairly unusual approach to bringing up a new ABI. Since new
> > > > > ABIs are more likely to be used on new systems rather than switching ABI
> > > > > on an existing installation, and since it can take quite some time for all
> > > > > the software support for a new ABI to become widely available in
> > > > > distributions, people developing new ABIs are likely to think about what
> > > > > new systems are going to be relevant in a few years' time when working out
> > > > > the minimum hardware requirements for the new ABI. (The POWER8 minimum
> > > > > for powerpc64le fits in with that, for example.)
> > > > That means that you cannot run ppc64le on FSL embedded CPUs (which lack
> > > > the vector instructions in LE mode). Which may be fine with you but
> > > > other people may want to support these. Can't really say if that's good
> > > > idea or not but I don't foresee them going away in a few years, either.
> > >
> > > well, ppc64le already cannot be run on those, as far as I know (I don't think it's possible to build ppc64le userland without VSX in any configuration)
> >
> > What hardware are you targetting then? I did not notice anything
> > specific mentioned in the thread.
> >
> > Naturally on POWER the first cpu that has LE support is POWER8 so you
> > can count on all other POWER8 features to be present. With other
> > architecture variants the situation is different.
>
> This is not true; nearly every 32-bit PowerPC CPU has LE support (all the way back to 6xx), these would be the native-hardware targets for the port (would need kernel support implemented, but it's technically possible).
I find dealing with memory management issues on 32bit architectures a
pain. There is never enough address space.
>
> As far as 64-bit CPUs go, POWER7 is the first one that could in practice run the current ppc64le configuration, but in glibc it's limited to POWER8 and in gcc the default for powerpc64le is also POWER8 (however, it is perfectly possible to configure gcc for POWER7 and use musl libc with it).
That's interesting. I guess I was tricked but the glibc limitation.
Thanks
Michal
^ permalink raw reply
* Re: ppc64le and 32-bit LE userland compatibility
From: Daniel Kolesa @ 2020-06-02 15:40 UTC (permalink / raw)
To: Michal Suchánek
Cc: libc-alpha, eery, musl, Will Springer,
Palmer Dabbelt via binutils, via libc-dev, linuxppc-dev,
Joseph Myers
In-Reply-To: <20200602152724.GU25173@kitsune.suse.cz>
On Tue, Jun 2, 2020, at 17:27, Michal Suchánek wrote:
> On Tue, Jun 02, 2020 at 05:13:25PM +0200, Daniel Kolesa wrote:
> > On Tue, Jun 2, 2020, at 16:23, Michal Suchánek wrote:
> > > On Tue, Jun 02, 2020 at 01:40:23PM +0000, Joseph Myers wrote:
> > > > On Tue, 2 Jun 2020, Daniel Kolesa wrote:
> > > >
> > > > > not be limited to being just userspace under ppc64le, but should be
> > > > > runnable on a native kernel as well, which should not be limited to any
> > > > > particular baseline other than just PowerPC.
> > > >
> > > > This is a fairly unusual approach to bringing up a new ABI. Since new
> > > > ABIs are more likely to be used on new systems rather than switching ABI
> > > > on an existing installation, and since it can take quite some time for all
> > > > the software support for a new ABI to become widely available in
> > > > distributions, people developing new ABIs are likely to think about what
> > > > new systems are going to be relevant in a few years' time when working out
> > > > the minimum hardware requirements for the new ABI. (The POWER8 minimum
> > > > for powerpc64le fits in with that, for example.)
> > > That means that you cannot run ppc64le on FSL embedded CPUs (which lack
> > > the vector instructions in LE mode). Which may be fine with you but
> > > other people may want to support these. Can't really say if that's good
> > > idea or not but I don't foresee them going away in a few years, either.
> >
> > well, ppc64le already cannot be run on those, as far as I know (I don't think it's possible to build ppc64le userland without VSX in any configuration)
>
> What hardware are you targetting then? I did not notice anything
> specific mentioned in the thread.
>
> Naturally on POWER the first cpu that has LE support is POWER8 so you
> can count on all other POWER8 features to be present. With other
> architecture variants the situation is different.
This is not true; nearly every 32-bit PowerPC CPU has LE support (all the way back to 6xx), these would be the native-hardware targets for the port (would need kernel support implemented, but it's technically possible).
As far as 64-bit CPUs go, POWER7 is the first one that could in practice run the current ppc64le configuration, but in glibc it's limited to POWER8 and in gcc the default for powerpc64le is also POWER8 (however, it is perfectly possible to configure gcc for POWER7 and use musl libc with it).
>
> Thanks
>
> Michal
>
^ permalink raw reply
* Re: ppc64le and 32-bit LE userland compatibility
From: Michal Suchánek @ 2020-06-02 15:27 UTC (permalink / raw)
To: Daniel Kolesa
Cc: libc-alpha, eery, musl, Will Springer,
Palmer Dabbelt via binutils, via libc-dev, linuxppc-dev,
Joseph Myers
In-Reply-To: <3aeb6dfe-ae23-42f9-ac23-16be6b54a850@www.fastmail.com>
On Tue, Jun 02, 2020 at 05:13:25PM +0200, Daniel Kolesa wrote:
> On Tue, Jun 2, 2020, at 16:23, Michal Suchánek wrote:
> > On Tue, Jun 02, 2020 at 01:40:23PM +0000, Joseph Myers wrote:
> > > On Tue, 2 Jun 2020, Daniel Kolesa wrote:
> > >
> > > > not be limited to being just userspace under ppc64le, but should be
> > > > runnable on a native kernel as well, which should not be limited to any
> > > > particular baseline other than just PowerPC.
> > >
> > > This is a fairly unusual approach to bringing up a new ABI. Since new
> > > ABIs are more likely to be used on new systems rather than switching ABI
> > > on an existing installation, and since it can take quite some time for all
> > > the software support for a new ABI to become widely available in
> > > distributions, people developing new ABIs are likely to think about what
> > > new systems are going to be relevant in a few years' time when working out
> > > the minimum hardware requirements for the new ABI. (The POWER8 minimum
> > > for powerpc64le fits in with that, for example.)
> > That means that you cannot run ppc64le on FSL embedded CPUs (which lack
> > the vector instructions in LE mode). Which may be fine with you but
> > other people may want to support these. Can't really say if that's good
> > idea or not but I don't foresee them going away in a few years, either.
>
> well, ppc64le already cannot be run on those, as far as I know (I don't think it's possible to build ppc64le userland without VSX in any configuration)
What hardware are you targetting then? I did not notice anything
specific mentioned in the thread.
Naturally on POWER the first cpu that has LE support is POWER8 so you
can count on all other POWER8 features to be present. With other
architecture variants the situation is different.
Thanks
Michal
^ permalink raw reply
* Re: ppc64le and 32-bit LE userland compatibility
From: Daniel Kolesa @ 2020-06-02 15:13 UTC (permalink / raw)
To: Michal Suchánek, Joseph Myers
Cc: libc-alpha, eery, musl, Will Springer,
Palmer Dabbelt via binutils, via libc-dev, linuxppc-dev
In-Reply-To: <20200602142337.GS25173@kitsune.suse.cz>
On Tue, Jun 2, 2020, at 16:23, Michal Suchánek wrote:
> On Tue, Jun 02, 2020 at 01:40:23PM +0000, Joseph Myers wrote:
> > On Tue, 2 Jun 2020, Daniel Kolesa wrote:
> >
> > > not be limited to being just userspace under ppc64le, but should be
> > > runnable on a native kernel as well, which should not be limited to any
> > > particular baseline other than just PowerPC.
> >
> > This is a fairly unusual approach to bringing up a new ABI. Since new
> > ABIs are more likely to be used on new systems rather than switching ABI
> > on an existing installation, and since it can take quite some time for all
> > the software support for a new ABI to become widely available in
> > distributions, people developing new ABIs are likely to think about what
> > new systems are going to be relevant in a few years' time when working out
> > the minimum hardware requirements for the new ABI. (The POWER8 minimum
> > for powerpc64le fits in with that, for example.)
> That means that you cannot run ppc64le on FSL embedded CPUs (which lack
> the vector instructions in LE mode). Which may be fine with you but
> other people may want to support these. Can't really say if that's good
> idea or not but I don't foresee them going away in a few years, either.
well, ppc64le already cannot be run on those, as far as I know (I don't think it's possible to build ppc64le userland without VSX in any configuration)
>
> Thanks
>
> Michal
>
Daniel
^ permalink raw reply
* Re: [PATCH 11/14] x86/entry: Clarify irq_{enter,exit}_rcu()
From: Peter Zijlstra @ 2020-06-02 15:05 UTC (permalink / raw)
To: Qian Cai
Cc: daniel.thompson, andrew.cooper3, bigeasy, x86, linux-kernel,
sean.j.christopherson, luto, Lai Jiangshan, rostedt, a.darwish,
tglx, linuxppc-dev
In-Reply-To: <20200602144235.GA1129@lca.pw>
On Tue, Jun 02, 2020 at 10:42:35AM -0400, Qian Cai wrote:
> Reverted this commit fixed the POWER9 boot warning,
ARGH, I'm an idiot. Please try this instead:
diff --git a/kernel/softirq.c b/kernel/softirq.c
index a3eb6eba8c41..c4201b7f42b1 100644
--- a/kernel/softirq.c
+++ b/kernel/softirq.c
@@ -438,7 +438,7 @@ void irq_exit_rcu(void)
*/
void irq_exit(void)
{
- irq_exit_rcu();
+ __irq_exit_rcu();
rcu_irq_exit();
/* must be last! */
lockdep_hardirq_exit();
^ permalink raw reply related
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