LinuxPPC-Dev Archive on lore.kernel.org
 help / color / mirror / Atom feed
* Re: [PATCH kernel] prom_init: Fetch flatten device tree from the system firmware
From: Segher Boessenkool @ 2019-06-03 23:42 UTC (permalink / raw)
  To: Alexey Kardashevskiy; +Cc: linuxppc-dev, Suraj Jitindar Singh, David Gibson
In-Reply-To: <7fc6cd5e-ddd6-4028-b4ef-7bdcd6db69d0@ozlabs.ru>

Hi!

On Mon, Jun 03, 2019 at 12:56:26PM +1000, Alexey Kardashevskiy wrote:
> On 03/06/2019 09:23, Segher Boessenkool wrote:
> >> So I go for the simple one and agree with Alexey's idea.
> > 
> > When dealing with a whole device tree you have to know about the various
> > dynamically generated nodes and props, and handle each appropriately.
> 
> The code I am changing fetches the device tree and build an fdt. What is
> that special knowledge in this context you are talking about?

Things like /options are dynamically generated.

I thought you would just be able to reuse some FDT parsing code to
implement my suggested new interface.  Maybe that was too optimistic.


Segher

^ permalink raw reply

* Re: [PATCH v2] scsi: ibmvscsi: Don't use rc uninitialized in ibmvscsi_do_work
From: Tyrel Datwyler @ 2019-06-03 23:35 UTC (permalink / raw)
  To: Nathan Chancellor, Tyrel Datwyler, James E.J. Bottomley,
	Martin K. Petersen
  Cc: clang-built-linux, linuxppc-dev, linux-kernel, linux-scsi
In-Reply-To: <8598d642-82e3-daad-a487-693208e13c90@linux.vnet.ibm.com>

On 06/03/2019 04:34 PM, Tyrel Datwyler wrote:
> On 06/03/2019 04:25 PM, Tyrel Datwyler wrote:
>> On 06/03/2019 03:19 PM, Nathan Chancellor wrote:
>>> clang warns:
>>>
>>> drivers/scsi/ibmvscsi/ibmvscsi.c:2126:7: warning: variable 'rc' is used
>>> uninitialized whenever switch case is taken [-Wsometimes-uninitialized]
>>>         case IBMVSCSI_HOST_ACTION_NONE:
>>>              ^~~~~~~~~~~~~~~~~~~~~~~~~
>>> drivers/scsi/ibmvscsi/ibmvscsi.c:2151:6: note: uninitialized use occurs
>>> here
>>>         if (rc) {
>>>             ^~
>>>
>>> Initialize rc to zero in the case statements that clang mentions so that
>>> the atomic_set and dev_err statement don't trigger for them.
>>>
>>> Fixes: 035a3c4046b5 ("scsi: ibmvscsi: redo driver work thread to use enum action states")
>>> Link: https://github.com/ClangBuiltLinux/linux/issues/502
>>> Suggested-by: Michael Ellerman <mpe@ellerman.id.au>
>>> Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
>>
>> Acked-by: Tyrel Datwyler <tyreld@linux.ibm.com>
>>
> 
> On second thought NACK. See my response to Michael earlier in the thread.
> 
> I think this is the better solution:
> 
> diff --git a/drivers/scsi/ibmvscsi/ibmvscsi.c b/drivers/scsi/ibmvscsi/ibmvscsi.c
> index 727c31dc11a0..c3cf05dd8733 100644
> --- a/drivers/scsi/ibmvscsi/ibmvscsi.c
> +++ b/drivers/scsi/ibmvscsi/ibmvscsi.c
> @@ -2123,8 +2123,8 @@ static void ibmvscsi_do_work(struct ibmvscsi_host_data
> *hostdata)
> 
>         spin_lock_irqsave(hostdata->host->host_lock, flags);
>         switch (hostdata->action) {
> -       case IBMVSCSI_HOST_ACTION_NONE:
>         case IBMVSCSI_HOST_ACTION_UNBLOCK:
> +               rc = 0;
>                 break;
>         case IBMVSCSI_HOST_ACTION_RESET:
>                 spin_unlock_irqrestore(hostdata->host->host_lock, flags);
> @@ -2142,8 +2142,9 @@ static void ibmvscsi_do_work(struct ibmvscsi_host_data
> *hostdata)
>                 if (!rc)
>                         rc = ibmvscsi_send_crq(hostdata, 0xC001000000000000LL, 0);
>                 break;
> +       case IBMVSCSI_HOST_ACTION_NONE:
>         default:
> -               break;

Need a spin_unlock_irqrestore() here before the return.

-Tyrel

> +               return;
>         }
> 
>         hostdata->action = IBMVSCSI_HOST_ACTION_NONE;
> 


^ permalink raw reply

* Re: [PATCH v2] scsi: ibmvscsi: Don't use rc uninitialized in ibmvscsi_do_work
From: Tyrel Datwyler @ 2019-06-03 23:34 UTC (permalink / raw)
  To: Nathan Chancellor, Tyrel Datwyler, James E.J. Bottomley,
	Martin K. Petersen
  Cc: clang-built-linux, linuxppc-dev, linux-kernel, linux-scsi
In-Reply-To: <6fa1dd2e-676f-b12a-5bb6-e86f5c5628fa@linux.vnet.ibm.com>

On 06/03/2019 04:25 PM, Tyrel Datwyler wrote:
> On 06/03/2019 03:19 PM, Nathan Chancellor wrote:
>> clang warns:
>>
>> drivers/scsi/ibmvscsi/ibmvscsi.c:2126:7: warning: variable 'rc' is used
>> uninitialized whenever switch case is taken [-Wsometimes-uninitialized]
>>         case IBMVSCSI_HOST_ACTION_NONE:
>>              ^~~~~~~~~~~~~~~~~~~~~~~~~
>> drivers/scsi/ibmvscsi/ibmvscsi.c:2151:6: note: uninitialized use occurs
>> here
>>         if (rc) {
>>             ^~
>>
>> Initialize rc to zero in the case statements that clang mentions so that
>> the atomic_set and dev_err statement don't trigger for them.
>>
>> Fixes: 035a3c4046b5 ("scsi: ibmvscsi: redo driver work thread to use enum action states")
>> Link: https://github.com/ClangBuiltLinux/linux/issues/502
>> Suggested-by: Michael Ellerman <mpe@ellerman.id.au>
>> Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
> 
> Acked-by: Tyrel Datwyler <tyreld@linux.ibm.com>
> 

On second thought NACK. See my response to Michael earlier in the thread.

I think this is the better solution:

diff --git a/drivers/scsi/ibmvscsi/ibmvscsi.c b/drivers/scsi/ibmvscsi/ibmvscsi.c
index 727c31dc11a0..c3cf05dd8733 100644
--- a/drivers/scsi/ibmvscsi/ibmvscsi.c
+++ b/drivers/scsi/ibmvscsi/ibmvscsi.c
@@ -2123,8 +2123,8 @@ static void ibmvscsi_do_work(struct ibmvscsi_host_data
*hostdata)

        spin_lock_irqsave(hostdata->host->host_lock, flags);
        switch (hostdata->action) {
-       case IBMVSCSI_HOST_ACTION_NONE:
        case IBMVSCSI_HOST_ACTION_UNBLOCK:
+               rc = 0;
                break;
        case IBMVSCSI_HOST_ACTION_RESET:
                spin_unlock_irqrestore(hostdata->host->host_lock, flags);
@@ -2142,8 +2142,9 @@ static void ibmvscsi_do_work(struct ibmvscsi_host_data
*hostdata)
                if (!rc)
                        rc = ibmvscsi_send_crq(hostdata, 0xC001000000000000LL, 0);
                break;
+       case IBMVSCSI_HOST_ACTION_NONE:
        default:
-               break;
+               return;
        }

        hostdata->action = IBMVSCSI_HOST_ACTION_NONE;


^ permalink raw reply related

* Re: [PATCH] scsi: ibmvscsi: Don't use rc uninitialized in ibmvscsi_do_work
From: Tyrel Datwyler @ 2019-06-03 23:30 UTC (permalink / raw)
  To: Michael Ellerman, Nathan Chancellor, Tyrel Datwyler,
	James E.J. Bottomley, Martin K. Petersen
  Cc: clang-built-linux, linuxppc-dev, linux-kernel, linux-scsi
In-Reply-To: <87blzgnvhx.fsf@concordia.ellerman.id.au>

On 06/02/2019 03:15 AM, Michael Ellerman wrote:
> Hi Nathan,
> 
> Nathan Chancellor <natechancellor@gmail.com> writes:
>> clang warns:
>>
>> drivers/scsi/ibmvscsi/ibmvscsi.c:2126:7: warning: variable 'rc' is used
>> uninitialized whenever switch case is taken [-Wsometimes-uninitialized]
>>         case IBMVSCSI_HOST_ACTION_NONE:
>>              ^~~~~~~~~~~~~~~~~~~~~~~~~
>> drivers/scsi/ibmvscsi/ibmvscsi.c:2151:6: note: uninitialized use occurs
>> here
>>         if (rc) {
>>             ^~
>>
>> Initialize rc to zero so that the atomic_set and dev_err statement don't
>> trigger for the cases that just break.
>>
>> Fixes: 035a3c4046b5 ("scsi: ibmvscsi: redo driver work thread to use enum action states")
>> Link: https://github.com/ClangBuiltLinux/linux/issues/502
>> Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
>> ---
>>  drivers/scsi/ibmvscsi/ibmvscsi.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/scsi/ibmvscsi/ibmvscsi.c b/drivers/scsi/ibmvscsi/ibmvscsi.c
>> index 727c31dc11a0..6714d8043e62 100644
>> --- a/drivers/scsi/ibmvscsi/ibmvscsi.c
>> +++ b/drivers/scsi/ibmvscsi/ibmvscsi.c
>> @@ -2118,7 +2118,7 @@ static unsigned long ibmvscsi_get_desired_dma(struct vio_dev *vdev)
>>  static void ibmvscsi_do_work(struct ibmvscsi_host_data *hostdata)
>>  {
>>  	unsigned long flags;
>> -	int rc;
>> +	int rc = 0;
>>  	char *action = "reset";
>>  
>>  	spin_lock_irqsave(hostdata->host->host_lock, flags);
> 
> It's always preferable IMHO to keep any initialisation as localised as
> possible, so that the compiler can continue to warn about uninitialised
> usages elsewhere. In this case that would mean doing the rc = 0 in the
> switch, something like:
> 
> diff --git a/drivers/scsi/ibmvscsi/ibmvscsi.c b/drivers/scsi/ibmvscsi/ibmvscsi.c
> index 727c31dc11a0..7ee5755cf636 100644
> --- a/drivers/scsi/ibmvscsi/ibmvscsi.c
> +++ b/drivers/scsi/ibmvscsi/ibmvscsi.c
> @@ -2123,9 +2123,6 @@ static void ibmvscsi_do_work(struct ibmvscsi_host_data *hostdata)
>  
>         spin_lock_irqsave(hostdata->host->host_lock, flags);
>         switch (hostdata->action) {
> -       case IBMVSCSI_HOST_ACTION_NONE:
> -       case IBMVSCSI_HOST_ACTION_UNBLOCK:
> -               break;
>         case IBMVSCSI_HOST_ACTION_RESET:
>                 spin_unlock_irqrestore(hostdata->host->host_lock, flags);
>                 rc = ibmvscsi_reset_crq_queue(&hostdata->queue, hostdata);
> @@ -2142,7 +2139,10 @@ static void ibmvscsi_do_work(struct ibmvscsi_host_data *hostdata)
>                 if (!rc)
>                         rc = ibmvscsi_send_crq(hostdata, 0xC001000000000000LL, 0);
>                 break;
> +       case IBMVSCSI_HOST_ACTION_NONE:
> +       case IBMVSCSI_HOST_ACTION_UNBLOCK:
>         default:
> +               rc = 0;
>                 break;
>         }
> 
> 
> But then that makes me wonder if that's actually correct?
> 
> If we get an action that we don't recognise should we just throw it away
> like that? (by doing hostdata->action = IBMVSCSI_HOST_ACTION_NONE). Tyrel?

On initial pass I was ok with this, but after thinking on it I think it is more
subtle.

The right approach is to set rc = 0 for HOST_ACTION_UNBLOCK as we want to fall
through. For HOST_ACTION_NONE and default we need to return directly from the
function.

-Tyrel

> 
> cheers
> 


^ permalink raw reply

* Re: [PATCH v2] scsi: ibmvscsi: Don't use rc uninitialized in ibmvscsi_do_work
From: Tyrel Datwyler @ 2019-06-03 23:25 UTC (permalink / raw)
  To: Nathan Chancellor, Tyrel Datwyler, James E.J. Bottomley,
	Martin K. Petersen
  Cc: clang-built-linux, linuxppc-dev, linux-kernel, linux-scsi
In-Reply-To: <20190603221941.65432-1-natechancellor@gmail.com>

On 06/03/2019 03:19 PM, Nathan Chancellor wrote:
> clang warns:
> 
> drivers/scsi/ibmvscsi/ibmvscsi.c:2126:7: warning: variable 'rc' is used
> uninitialized whenever switch case is taken [-Wsometimes-uninitialized]
>         case IBMVSCSI_HOST_ACTION_NONE:
>              ^~~~~~~~~~~~~~~~~~~~~~~~~
> drivers/scsi/ibmvscsi/ibmvscsi.c:2151:6: note: uninitialized use occurs
> here
>         if (rc) {
>             ^~
> 
> Initialize rc to zero in the case statements that clang mentions so that
> the atomic_set and dev_err statement don't trigger for them.
> 
> Fixes: 035a3c4046b5 ("scsi: ibmvscsi: redo driver work thread to use enum action states")
> Link: https://github.com/ClangBuiltLinux/linux/issues/502
> Suggested-by: Michael Ellerman <mpe@ellerman.id.au>
> Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>

Acked-by: Tyrel Datwyler <tyreld@linux.ibm.com>


^ permalink raw reply

* [PATCH v2] scsi: ibmvscsi: Don't use rc uninitialized in ibmvscsi_do_work
From: Nathan Chancellor @ 2019-06-03 22:19 UTC (permalink / raw)
  To: Tyrel Datwyler, James E.J. Bottomley, Martin K. Petersen
  Cc: linux-scsi, linux-kernel, clang-built-linux, Nathan Chancellor,
	linuxppc-dev
In-Reply-To: <20190531185306.41290-1-natechancellor@gmail.com>

clang warns:

drivers/scsi/ibmvscsi/ibmvscsi.c:2126:7: warning: variable 'rc' is used
uninitialized whenever switch case is taken [-Wsometimes-uninitialized]
        case IBMVSCSI_HOST_ACTION_NONE:
             ^~~~~~~~~~~~~~~~~~~~~~~~~
drivers/scsi/ibmvscsi/ibmvscsi.c:2151:6: note: uninitialized use occurs
here
        if (rc) {
            ^~

Initialize rc to zero in the case statements that clang mentions so that
the atomic_set and dev_err statement don't trigger for them.

Fixes: 035a3c4046b5 ("scsi: ibmvscsi: redo driver work thread to use enum action states")
Link: https://github.com/ClangBuiltLinux/linux/issues/502
Suggested-by: Michael Ellerman <mpe@ellerman.id.au>
Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
---

v1 -> v2:

* Initialize rc in the case statements, rather than at the top of the
  function, as suggested by Michael.

 drivers/scsi/ibmvscsi/ibmvscsi.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/scsi/ibmvscsi/ibmvscsi.c b/drivers/scsi/ibmvscsi/ibmvscsi.c
index 65053daef5f7..3b5647d622d9 100644
--- a/drivers/scsi/ibmvscsi/ibmvscsi.c
+++ b/drivers/scsi/ibmvscsi/ibmvscsi.c
@@ -2109,9 +2109,6 @@ static void ibmvscsi_do_work(struct ibmvscsi_host_data *hostdata)
 
 	spin_lock_irqsave(hostdata->host->host_lock, flags);
 	switch (hostdata->action) {
-	case IBMVSCSI_HOST_ACTION_NONE:
-	case IBMVSCSI_HOST_ACTION_UNBLOCK:
-		break;
 	case IBMVSCSI_HOST_ACTION_RESET:
 		spin_unlock_irqrestore(hostdata->host->host_lock, flags);
 		rc = ibmvscsi_reset_crq_queue(&hostdata->queue, hostdata);
@@ -2128,7 +2125,10 @@ static void ibmvscsi_do_work(struct ibmvscsi_host_data *hostdata)
 		if (!rc)
 			rc = ibmvscsi_send_crq(hostdata, 0xC001000000000000LL, 0);
 		break;
+	case IBMVSCSI_HOST_ACTION_NONE:
+	case IBMVSCSI_HOST_ACTION_UNBLOCK:
 	default:
+		rc = 0;
 		break;
 	}
 
-- 
2.22.0.rc3


^ permalink raw reply related

* Re: [PATCH v3 06/11] mm/memory_hotplug: Allow arch_remove_pages() without CONFIG_MEMORY_HOTREMOVE
From: Wei Yang @ 2019-06-03 22:15 UTC (permalink / raw)
  To: David Hildenbrand
  Cc: Oscar Salvador, Rich Felker, linux-ia64, Anshuman Khandual,
	linux-sh, Peter Zijlstra, Dave Hansen, Heiko Carstens, Arun KS,
	Wei Yang, linux-mm, Michal Hocko, Paul Mackerras, H. Peter Anvin,
	Thomas Gleixner, Qian Cai, linux-s390, Yoshinori Sato,
	Rafael J. Wysocki, Mike Rapoport, Ingo Molnar, Fenghua Yu,
	Pavel Tatashin, Vasily Gorbik, Rob Herring, mike.travis@hpe.com,
	Nicholas Piggin, Alex Deucher, Mark Brown, Borislav Petkov,
	Andy Lutomirski, Dan Williams, Chris Wilson, linux-arm-kernel,
	Tony Luck, Baoquan He, Masahiro Yamada, Mathieu Malaterre,
	Greg Kroah-Hartman, Andrew Banman, linux-kernel, Logan Gunthorpe,
	Wei Yang, Martin Schwidefsky, Igor Mammedov, akpm, linuxppc-dev,
	David S. Miller, Kirill A. Shutemov
In-Reply-To: <20190527111152.16324-7-david@redhat.com>

Allow arch_remove_pages() or arch_remove_memory()?

And want to confirm the kernel build on affected arch succeed?

On Mon, May 27, 2019 at 01:11:47PM +0200, David Hildenbrand wrote:
>We want to improve error handling while adding memory by allowing
>to use arch_remove_memory() and __remove_pages() even if
>CONFIG_MEMORY_HOTREMOVE is not set to e.g., implement something like:
>
>	arch_add_memory()
>	rc = do_something();
>	if (rc) {
>		arch_remove_memory();
>	}
>
>We won't get rid of CONFIG_MEMORY_HOTREMOVE for now, as it will require
>quite some dependencies for memory offlining.
>
>Cc: Tony Luck <tony.luck@intel.com>
>Cc: Fenghua Yu <fenghua.yu@intel.com>
>Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
>Cc: Paul Mackerras <paulus@samba.org>
>Cc: Michael Ellerman <mpe@ellerman.id.au>
>Cc: Martin Schwidefsky <schwidefsky@de.ibm.com>
>Cc: Heiko Carstens <heiko.carstens@de.ibm.com>
>Cc: Yoshinori Sato <ysato@users.sourceforge.jp>
>Cc: Rich Felker <dalias@libc.org>
>Cc: Dave Hansen <dave.hansen@linux.intel.com>
>Cc: Andy Lutomirski <luto@kernel.org>
>Cc: Peter Zijlstra <peterz@infradead.org>
>Cc: Thomas Gleixner <tglx@linutronix.de>
>Cc: Ingo Molnar <mingo@redhat.com>
>Cc: Borislav Petkov <bp@alien8.de>
>Cc: "H. Peter Anvin" <hpa@zytor.com>
>Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>Cc: "Rafael J. Wysocki" <rafael@kernel.org>
>Cc: Andrew Morton <akpm@linux-foundation.org>
>Cc: Michal Hocko <mhocko@suse.com>
>Cc: Mike Rapoport <rppt@linux.ibm.com>
>Cc: David Hildenbrand <david@redhat.com>
>Cc: Oscar Salvador <osalvador@suse.com>
>Cc: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>
>Cc: Alex Deucher <alexander.deucher@amd.com>
>Cc: "David S. Miller" <davem@davemloft.net>
>Cc: Mark Brown <broonie@kernel.org>
>Cc: Chris Wilson <chris@chris-wilson.co.uk>
>Cc: Christophe Leroy <christophe.leroy@c-s.fr>
>Cc: Nicholas Piggin <npiggin@gmail.com>
>Cc: Vasily Gorbik <gor@linux.ibm.com>
>Cc: Rob Herring <robh@kernel.org>
>Cc: Masahiro Yamada <yamada.masahiro@socionext.com>
>Cc: "mike.travis@hpe.com" <mike.travis@hpe.com>
>Cc: Andrew Banman <andrew.banman@hpe.com>
>Cc: Pavel Tatashin <pasha.tatashin@soleen.com>
>Cc: Wei Yang <richardw.yang@linux.intel.com>
>Cc: Arun KS <arunks@codeaurora.org>
>Cc: Qian Cai <cai@lca.pw>
>Cc: Mathieu Malaterre <malat@debian.org>
>Cc: Baoquan He <bhe@redhat.com>
>Cc: Logan Gunthorpe <logang@deltatee.com>
>Cc: Anshuman Khandual <anshuman.khandual@arm.com>
>Signed-off-by: David Hildenbrand <david@redhat.com>
>---
> arch/arm64/mm/mmu.c            | 2 --
> arch/ia64/mm/init.c            | 2 --
> arch/powerpc/mm/mem.c          | 2 --
> arch/s390/mm/init.c            | 2 --
> arch/sh/mm/init.c              | 2 --
> arch/x86/mm/init_32.c          | 2 --
> arch/x86/mm/init_64.c          | 2 --
> drivers/base/memory.c          | 2 --
> include/linux/memory.h         | 2 --
> include/linux/memory_hotplug.h | 2 --
> mm/memory_hotplug.c            | 2 --
> mm/sparse.c                    | 6 ------
> 12 files changed, 28 deletions(-)
>
>diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
>index e569a543c384..9ccd7539f2d4 100644
>--- a/arch/arm64/mm/mmu.c
>+++ b/arch/arm64/mm/mmu.c
>@@ -1084,7 +1084,6 @@ int arch_add_memory(int nid, u64 start, u64 size,
> 	return __add_pages(nid, start >> PAGE_SHIFT, size >> PAGE_SHIFT,
> 			   restrictions);
> }
>-#ifdef CONFIG_MEMORY_HOTREMOVE
> void arch_remove_memory(int nid, u64 start, u64 size,
> 			struct vmem_altmap *altmap)
> {
>@@ -1103,4 +1102,3 @@ void arch_remove_memory(int nid, u64 start, u64 size,
> 	__remove_pages(zone, start_pfn, nr_pages, altmap);
> }
> #endif
>-#endif
>diff --git a/arch/ia64/mm/init.c b/arch/ia64/mm/init.c
>index d28e29103bdb..aae75fd7b810 100644
>--- a/arch/ia64/mm/init.c
>+++ b/arch/ia64/mm/init.c
>@@ -681,7 +681,6 @@ int arch_add_memory(int nid, u64 start, u64 size,
> 	return ret;
> }
> 
>-#ifdef CONFIG_MEMORY_HOTREMOVE
> void arch_remove_memory(int nid, u64 start, u64 size,
> 			struct vmem_altmap *altmap)
> {
>@@ -693,4 +692,3 @@ void arch_remove_memory(int nid, u64 start, u64 size,
> 	__remove_pages(zone, start_pfn, nr_pages, altmap);
> }
> #endif
>-#endif
>diff --git a/arch/powerpc/mm/mem.c b/arch/powerpc/mm/mem.c
>index e885fe2aafcc..e4bc2dc3f593 100644
>--- a/arch/powerpc/mm/mem.c
>+++ b/arch/powerpc/mm/mem.c
>@@ -130,7 +130,6 @@ int __ref arch_add_memory(int nid, u64 start, u64 size,
> 	return __add_pages(nid, start_pfn, nr_pages, restrictions);
> }
> 
>-#ifdef CONFIG_MEMORY_HOTREMOVE
> void __ref arch_remove_memory(int nid, u64 start, u64 size,
> 			     struct vmem_altmap *altmap)
> {
>@@ -164,7 +163,6 @@ void __ref arch_remove_memory(int nid, u64 start, u64 size,
> 		pr_warn("Hash collision while resizing HPT\n");
> }
> #endif
>-#endif /* CONFIG_MEMORY_HOTPLUG */
> 
> #ifndef CONFIG_NEED_MULTIPLE_NODES
> void __init mem_topology_setup(void)
>diff --git a/arch/s390/mm/init.c b/arch/s390/mm/init.c
>index 14955e0a9fcf..ffb81fe95c77 100644
>--- a/arch/s390/mm/init.c
>+++ b/arch/s390/mm/init.c
>@@ -239,7 +239,6 @@ int arch_add_memory(int nid, u64 start, u64 size,
> 	return rc;
> }
> 
>-#ifdef CONFIG_MEMORY_HOTREMOVE
> void arch_remove_memory(int nid, u64 start, u64 size,
> 			struct vmem_altmap *altmap)
> {
>@@ -251,5 +250,4 @@ void arch_remove_memory(int nid, u64 start, u64 size,
> 	__remove_pages(zone, start_pfn, nr_pages, altmap);
> 	vmem_remove_mapping(start, size);
> }
>-#endif
> #endif /* CONFIG_MEMORY_HOTPLUG */
>diff --git a/arch/sh/mm/init.c b/arch/sh/mm/init.c
>index 13c6a6bb5fd9..dfdbaa50946e 100644
>--- a/arch/sh/mm/init.c
>+++ b/arch/sh/mm/init.c
>@@ -429,7 +429,6 @@ int memory_add_physaddr_to_nid(u64 addr)
> EXPORT_SYMBOL_GPL(memory_add_physaddr_to_nid);
> #endif
> 
>-#ifdef CONFIG_MEMORY_HOTREMOVE
> void arch_remove_memory(int nid, u64 start, u64 size,
> 			struct vmem_altmap *altmap)
> {
>@@ -440,5 +439,4 @@ void arch_remove_memory(int nid, u64 start, u64 size,
> 	zone = page_zone(pfn_to_page(start_pfn));
> 	__remove_pages(zone, start_pfn, nr_pages, altmap);
> }
>-#endif
> #endif /* CONFIG_MEMORY_HOTPLUG */
>diff --git a/arch/x86/mm/init_32.c b/arch/x86/mm/init_32.c
>index f265a4316179..4068abb9427f 100644
>--- a/arch/x86/mm/init_32.c
>+++ b/arch/x86/mm/init_32.c
>@@ -860,7 +860,6 @@ int arch_add_memory(int nid, u64 start, u64 size,
> 	return __add_pages(nid, start_pfn, nr_pages, restrictions);
> }
> 
>-#ifdef CONFIG_MEMORY_HOTREMOVE
> void arch_remove_memory(int nid, u64 start, u64 size,
> 			struct vmem_altmap *altmap)
> {
>@@ -872,7 +871,6 @@ void arch_remove_memory(int nid, u64 start, u64 size,
> 	__remove_pages(zone, start_pfn, nr_pages, altmap);
> }
> #endif
>-#endif
> 
> int kernel_set_to_readonly __read_mostly;
> 
>diff --git a/arch/x86/mm/init_64.c b/arch/x86/mm/init_64.c
>index 693aaf28d5fe..8335ac6e1112 100644
>--- a/arch/x86/mm/init_64.c
>+++ b/arch/x86/mm/init_64.c
>@@ -1196,7 +1196,6 @@ void __ref vmemmap_free(unsigned long start, unsigned long end,
> 	remove_pagetable(start, end, false, altmap);
> }
> 
>-#ifdef CONFIG_MEMORY_HOTREMOVE
> static void __meminit
> kernel_physical_mapping_remove(unsigned long start, unsigned long end)
> {
>@@ -1221,7 +1220,6 @@ void __ref arch_remove_memory(int nid, u64 start, u64 size,
> 	__remove_pages(zone, start_pfn, nr_pages, altmap);
> 	kernel_physical_mapping_remove(start, start + size);
> }
>-#endif
> #endif /* CONFIG_MEMORY_HOTPLUG */
> 
> static struct kcore_list kcore_vsyscall;
>diff --git a/drivers/base/memory.c b/drivers/base/memory.c
>index f914fa6fe350..ac17c95a5f28 100644
>--- a/drivers/base/memory.c
>+++ b/drivers/base/memory.c
>@@ -727,7 +727,6 @@ int hotplug_memory_register(int nid, struct mem_section *section)
> 	return ret;
> }
> 
>-#ifdef CONFIG_MEMORY_HOTREMOVE
> static void
> unregister_memory(struct memory_block *memory)
> {
>@@ -766,7 +765,6 @@ void unregister_memory_section(struct mem_section *section)
> out_unlock:
> 	mutex_unlock(&mem_sysfs_mutex);
> }
>-#endif /* CONFIG_MEMORY_HOTREMOVE */
> 
> /* return true if the memory block is offlined, otherwise, return false */
> bool is_memblock_offlined(struct memory_block *mem)
>diff --git a/include/linux/memory.h b/include/linux/memory.h
>index e1dc1bb2b787..474c7c60c8f2 100644
>--- a/include/linux/memory.h
>+++ b/include/linux/memory.h
>@@ -112,9 +112,7 @@ extern void unregister_memory_notifier(struct notifier_block *nb);
> extern int register_memory_isolate_notifier(struct notifier_block *nb);
> extern void unregister_memory_isolate_notifier(struct notifier_block *nb);
> int hotplug_memory_register(int nid, struct mem_section *section);
>-#ifdef CONFIG_MEMORY_HOTREMOVE
> extern void unregister_memory_section(struct mem_section *);
>-#endif
> extern int memory_dev_init(void);
> extern int memory_notify(unsigned long val, void *v);
> extern int memory_isolate_notify(unsigned long val, void *v);
>diff --git a/include/linux/memory_hotplug.h b/include/linux/memory_hotplug.h
>index ae892eef8b82..2d4de313926d 100644
>--- a/include/linux/memory_hotplug.h
>+++ b/include/linux/memory_hotplug.h
>@@ -123,12 +123,10 @@ static inline bool movable_node_is_enabled(void)
> 	return movable_node_enabled;
> }
> 
>-#ifdef CONFIG_MEMORY_HOTREMOVE
> extern void arch_remove_memory(int nid, u64 start, u64 size,
> 			       struct vmem_altmap *altmap);
> extern void __remove_pages(struct zone *zone, unsigned long start_pfn,
> 			   unsigned long nr_pages, struct vmem_altmap *altmap);
>-#endif /* CONFIG_MEMORY_HOTREMOVE */
> 
> /*
>  * Do we want sysfs memblock files created. This will allow userspace to online
>diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
>index 762887b2358b..4b9d2974f86c 100644
>--- a/mm/memory_hotplug.c
>+++ b/mm/memory_hotplug.c
>@@ -318,7 +318,6 @@ int __ref __add_pages(int nid, unsigned long phys_start_pfn,
> 	return err;
> }
> 
>-#ifdef CONFIG_MEMORY_HOTREMOVE
> /* find the smallest valid pfn in the range [start_pfn, end_pfn) */
> static unsigned long find_smallest_section_pfn(int nid, struct zone *zone,
> 				     unsigned long start_pfn,
>@@ -582,7 +581,6 @@ void __remove_pages(struct zone *zone, unsigned long phys_start_pfn,
> 
> 	set_zone_contiguous(zone);
> }
>-#endif /* CONFIG_MEMORY_HOTREMOVE */
> 
> int set_online_page_callback(online_page_callback_t callback)
> {
>diff --git a/mm/sparse.c b/mm/sparse.c
>index fd13166949b5..d1d5e05f5b8d 100644
>--- a/mm/sparse.c
>+++ b/mm/sparse.c
>@@ -604,7 +604,6 @@ static void __kfree_section_memmap(struct page *memmap,
> 
> 	vmemmap_free(start, end, altmap);
> }
>-#ifdef CONFIG_MEMORY_HOTREMOVE
> static void free_map_bootmem(struct page *memmap)
> {
> 	unsigned long start = (unsigned long)memmap;
>@@ -612,7 +611,6 @@ static void free_map_bootmem(struct page *memmap)
> 
> 	vmemmap_free(start, end, NULL);
> }
>-#endif /* CONFIG_MEMORY_HOTREMOVE */
> #else
> static struct page *__kmalloc_section_memmap(void)
> {
>@@ -651,7 +649,6 @@ static void __kfree_section_memmap(struct page *memmap,
> 			   get_order(sizeof(struct page) * PAGES_PER_SECTION));
> }
> 
>-#ifdef CONFIG_MEMORY_HOTREMOVE
> static void free_map_bootmem(struct page *memmap)
> {
> 	unsigned long maps_section_nr, removing_section_nr, i;
>@@ -681,7 +678,6 @@ static void free_map_bootmem(struct page *memmap)
> 			put_page_bootmem(page);
> 	}
> }
>-#endif /* CONFIG_MEMORY_HOTREMOVE */
> #endif /* CONFIG_SPARSEMEM_VMEMMAP */
> 
> /**
>@@ -746,7 +742,6 @@ int __meminit sparse_add_one_section(int nid, unsigned long start_pfn,
> 	return ret;
> }
> 
>-#ifdef CONFIG_MEMORY_HOTREMOVE
> #ifdef CONFIG_MEMORY_FAILURE
> static void clear_hwpoisoned_pages(struct page *memmap, int nr_pages)
> {
>@@ -823,5 +818,4 @@ void sparse_remove_one_section(struct zone *zone, struct mem_section *ms,
> 			PAGES_PER_SECTION - map_offset);
> 	free_section_usemap(memmap, usemap, altmap);
> }
>-#endif /* CONFIG_MEMORY_HOTREMOVE */
> #endif /* CONFIG_MEMORY_HOTPLUG */
>-- 
>2.20.1

-- 
Wei Yang
Help you, Help me

^ permalink raw reply

* [PATCH v2] PCI: rpaphp: Avoid a sometimes-uninitialized warning
From: Nathan Chancellor @ 2019-06-03 22:11 UTC (permalink / raw)
  To: Tyrel Datwyler, Benjamin Herrenschmidt, Paul Mackerras,
	Michael Ellerman
  Cc: linux-pci, Nick Desaulniers, linux-kernel, clang-built-linux,
	Bjorn Helgaas, Nathan Chancellor, linuxppc-dev
In-Reply-To: <20190603174323.48251-1-natechancellor@gmail.com>

When building with -Wsometimes-uninitialized, clang warns:

drivers/pci/hotplug/rpaphp_core.c:243:14: warning: variable 'fndit' is
used uninitialized whenever 'for' loop exits because its condition is
false [-Wsometimes-uninitialized]
        for (j = 0; j < entries; j++) {
                    ^~~~~~~~~~~
drivers/pci/hotplug/rpaphp_core.c:256:6: note: uninitialized use occurs
here
        if (fndit)
            ^~~~~
drivers/pci/hotplug/rpaphp_core.c:243:14: note: remove the condition if
it is always true
        for (j = 0; j < entries; j++) {
                    ^~~~~~~~~~~
drivers/pci/hotplug/rpaphp_core.c:233:14: note: initialize the variable
'fndit' to silence this warning
        int j, fndit;
                    ^
                     = 0

fndit is only used to gate a sprintf call, which can be moved into the
loop to simplify the code and eliminate the local variable, which will
fix this warning.

Link: https://github.com/ClangBuiltLinux/linux/issues/504
Fixes: 2fcf3ae508c2 ("hotplug/drc-info: Add code to search ibm,drc-info property")
Suggested-by: Nick Desaulniers <ndesaulniers@google.com>
Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
---

v1 -> v2:

* Eliminate fndit altogether by shuffling the sprintf call into the for
  loop and changing the if conditional, as suggested by Nick.

 drivers/pci/hotplug/rpaphp_core.c | 18 +++++++-----------
 1 file changed, 7 insertions(+), 11 deletions(-)

diff --git a/drivers/pci/hotplug/rpaphp_core.c b/drivers/pci/hotplug/rpaphp_core.c
index bcd5d357ca23..c3899ee1db99 100644
--- a/drivers/pci/hotplug/rpaphp_core.c
+++ b/drivers/pci/hotplug/rpaphp_core.c
@@ -230,7 +230,7 @@ static int rpaphp_check_drc_props_v2(struct device_node *dn, char *drc_name,
 	struct of_drc_info drc;
 	const __be32 *value;
 	char cell_drc_name[MAX_DRC_NAME_LEN];
-	int j, fndit;
+	int j;
 
 	info = of_find_property(dn->parent, "ibm,drc-info", NULL);
 	if (info == NULL)
@@ -245,17 +245,13 @@ static int rpaphp_check_drc_props_v2(struct device_node *dn, char *drc_name,
 
 		/* Should now know end of current entry */
 
-		if (my_index > drc.last_drc_index)
-			continue;
-
-		fndit = 1;
-		break;
+		/* Found it */
+		if (my_index <= drc.last_drc_index) {
+			sprintf(cell_drc_name, "%s%d", drc.drc_name_prefix,
+				my_index);
+			break;
+		}
 	}
-	/* Found it */
-
-	if (fndit)
-		sprintf(cell_drc_name, "%s%d", drc.drc_name_prefix, 
-			my_index);
 
 	if (((drc_name == NULL) ||
 	     (drc_name && !strcmp(drc_name, cell_drc_name))) &&
-- 
2.22.0.rc3


^ permalink raw reply related

* Re: [PATCH] PCI: rpaphp: Avoid a sometimes-uninitialized warning
From: Nathan Chancellor @ 2019-06-03 21:51 UTC (permalink / raw)
  To: Nick Desaulniers
  Cc: Tyrel Datwyler, LKML, clang-built-linux, Paul Mackerras,
	linux-pci, Bjorn Helgaas, linuxppc-dev
In-Reply-To: <CAKwvOdkQzdZezwf51UddFVGQh0mRFMEexr1cMHx=va88T515YQ@mail.gmail.com>

Hi Nick,

On Mon, Jun 03, 2019 at 02:07:50PM -0700, Nick Desaulniers wrote:
> On Mon, Jun 3, 2019 at 10:44 AM Nathan Chancellor
> <natechancellor@gmail.com> wrote:
> > Looking at the loop in a vacuum as clang would, fndit could be
> > uninitialized if entries was ever zero or the if statement was
> > always true within the loop. Regardless of whether or not this
> > warning is a problem in practice, "found" variables should always
> > be initialized to false so that there is no possibility of
> > undefined behavior.
> 
> Thanks for the patch Nathan.  fndit isn't really being used for
> anything other than a print statement outside of the loop.  How about:

Thank you for the review, this seems reasonable. I will send a v2
shortly.

> 
> ```
> diff --git a/drivers/pci/hotplug/rpaphp_core.c
> b/drivers/pci/hotplug/rpaphp_core.c
> index bcd5d357ca23..c3899ee1db99 100644
> --- a/drivers/pci/hotplug/rpaphp_core.c
> +++ b/drivers/pci/hotplug/rpaphp_core.c
> @@ -230,7 +230,7 @@ static int rpaphp_check_drc_props_v2(struct
> device_node *dn, char *drc_name,
>   struct of_drc_info drc;
>   const __be32 *value;
>   char cell_drc_name[MAX_DRC_NAME_LEN];
> - int j, fndit;
> + int j;
> 
>   info = of_find_property(dn->parent, "ibm,drc-info", NULL);
>   if (info == NULL)
> @@ -245,17 +245,13 @@ static int rpaphp_check_drc_props_v2(struct
> device_node *dn, char *drc_name,
> 
>   /* Should now know end of current entry */
> 
> - if (my_index > drc.last_drc_index)
> - continue;
> -
> - fndit = 1;
> - break;
> + /* Found it */
> + if (my_index <= drc.last_drc_index) {
> + sprintf(cell_drc_name, "%s%d", drc.drc_name_prefix,
> + my_index);
> + break;
> + }
>   }
> - /* Found it */
> -
> - if (fndit)
> - sprintf(cell_drc_name, "%s%d", drc.drc_name_prefix,
> - my_index);
> 
>   if (((drc_name == NULL) ||
>        (drc_name && !strcmp(drc_name, cell_drc_name))) &&
> ```
> (not sure my tabs were pasted properly in the above...)

Doesn't look like it but no worries.

Thanks,
Nathan

^ permalink raw reply

* Re: [PATCH v3 05/11] drivers/base/memory: Pass a block_id to init_memory_block()
From: Wei Yang @ 2019-06-03 21:49 UTC (permalink / raw)
  To: David Hildenbrand
  Cc: linux-s390, linux-ia64, linux-sh, Greg Kroah-Hartman,
	Rafael J. Wysocki, linux-kernel, Wei Yang, linux-mm,
	Igor Mammedov, akpm, linuxppc-dev, Dan Williams, linux-arm-kernel
In-Reply-To: <20190527111152.16324-6-david@redhat.com>

On Mon, May 27, 2019 at 01:11:46PM +0200, David Hildenbrand wrote:
>We'll rework hotplug_memory_register() shortly, so it no longer consumes
>pass a section.
>
>Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>Cc: "Rafael J. Wysocki" <rafael@kernel.org>
>Signed-off-by: David Hildenbrand <david@redhat.com>
>---
> drivers/base/memory.c | 15 +++++++--------
> 1 file changed, 7 insertions(+), 8 deletions(-)
>
>diff --git a/drivers/base/memory.c b/drivers/base/memory.c
>index f180427e48f4..f914fa6fe350 100644
>--- a/drivers/base/memory.c
>+++ b/drivers/base/memory.c
>@@ -651,21 +651,18 @@ int register_memory(struct memory_block *memory)
> 	return ret;
> }
> 
>-static int init_memory_block(struct memory_block **memory,
>-			     struct mem_section *section, unsigned long state)
>+static int init_memory_block(struct memory_block **memory, int block_id,
>+			     unsigned long state)
> {
> 	struct memory_block *mem;
> 	unsigned long start_pfn;
>-	int scn_nr;
> 	int ret = 0;
> 
> 	mem = kzalloc(sizeof(*mem), GFP_KERNEL);
> 	if (!mem)
> 		return -ENOMEM;
> 
>-	scn_nr = __section_nr(section);
>-	mem->start_section_nr =
>-			base_memory_block_id(scn_nr) * sections_per_block;
>+	mem->start_section_nr = block_id * sections_per_block;
> 	mem->end_section_nr = mem->start_section_nr + sections_per_block - 1;
> 	mem->state = state;
> 	start_pfn = section_nr_to_pfn(mem->start_section_nr);
>@@ -694,7 +691,8 @@ static int add_memory_block(int base_section_nr)
> 
> 	if (section_count == 0)
> 		return 0;
>-	ret = init_memory_block(&mem, __nr_to_section(section_nr), MEM_ONLINE);
>+	ret = init_memory_block(&mem, base_memory_block_id(base_section_nr),
>+				MEM_ONLINE);

If my understanding is correct, section_nr could be removed too.

> 	if (ret)
> 		return ret;
> 	mem->section_count = section_count;
>@@ -707,6 +705,7 @@ static int add_memory_block(int base_section_nr)
>  */
> int hotplug_memory_register(int nid, struct mem_section *section)
> {
>+	int block_id = base_memory_block_id(__section_nr(section));
> 	int ret = 0;
> 	struct memory_block *mem;
> 
>@@ -717,7 +716,7 @@ int hotplug_memory_register(int nid, struct mem_section *section)
> 		mem->section_count++;
> 		put_device(&mem->dev);
> 	} else {
>-		ret = init_memory_block(&mem, section, MEM_OFFLINE);
>+		ret = init_memory_block(&mem, block_id, MEM_OFFLINE);
> 		if (ret)
> 			goto out;
> 		mem->section_count++;
>-- 
>2.20.1

-- 
Wei Yang
Help you, Help me

^ permalink raw reply

* Re: [PATCH v3 00/11] mm/memory_hotplug: Factor out memory block devicehandling
From: David Hildenbrand @ 2019-06-03 21:40 UTC (permalink / raw)
  To: Wei Yang
  Cc: Mark Rutland, Oscar Salvador, Michal Hocko, linux-ia64, linux-sh,
	Peter Zijlstra, Dave Hansen, Heiko Carstens, Chris Wilson,
	linux-mm, Pavel Tatashin, Rich Felker, Arun KS, Chintan Pandya,
	Ingo Molnar, Paul Mackerras, Qian Cai, linux-s390, H. Peter Anvin,
	Yu Zhao, Baoquan He, Logan Gunthorpe, Rafael J. Wysocki,
	Mike Rapoport, Jun Yao, mike.travis@hpe.com, Ingo Molnar,
	Catalin Marinas, Rob Herring, Fenghua Yu, Pavel Tatashin,
	Vasily Gorbik, Anshuman Khandual, Masahiro Yamada, Will Deacon,
	Robin Murphy, Nicholas Piggin, Martin Schwidefsky, Mark Brown,
	Borislav Petkov, Andy Lutomirski, Jonathan Cameron, Dan Williams,
	Joonsoo Kim, linux-arm-kernel, Oscar Salvador, Tony Luck,
	Yoshinori Sato, Ard Biesheuvel, Mathieu Malaterre,
	Greg Kroah-Hartman, Andrew Banman, linux-kernel, Mike Rapoport,
	Thomas Gleixner, Wei Yang, Alex Deucher, Igor Mammedov, akpm,
	linuxppc-dev, David S. Miller, Kirill A. Shutemov
In-Reply-To: <20190603212146.7hdha6wrlxtkxxxr@master>

On 03.06.19 23:21, Wei Yang wrote:
> IMHO, there is some typo.

Yes, thanks.

> 
> s/devicehandling/device handling/
> 
> On Mon, May 27, 2019 at 01:11:41PM +0200, David Hildenbrand wrote:
>> We only want memory block devices for memory to be onlined/offlined
>> (add/remove from the buddy). This is required so user space can
>> online/offline memory and kdump gets notified about newly onlined memory.
>>
>> Let's factor out creation/removal of memory block devices. This helps
>> to further cleanup arch_add_memory/arch_remove_memory() and to make
>> implementation of new features easier - especially sub-section
>> memory hot add from Dan.
>>
>> Anshuman Khandual is currently working on arch_remove_memory(). I added
>> a temporary solution via "arm64/mm: Add temporary arch_remove_memory()
>> implementation", that is sufficient as a firsts tep in the context of
> 
> s/firsts tep/first step/
> 
>> this series. (we don't cleanup page tables in case anything goes
>> wrong already)
>>
>> Did a quick sanity test with DIMM plug/unplug, making sure all devices
>> and sysfs links properly get added/removed. Compile tested on s390x and
>> x86-64.
>>
>> Based on next/master.
>>
>> Next refactoring on my list will be making sure that remove_memory()
>> will never deal with zones / access "struct pages". Any kind of zone
>> handling will have to be done when offlining system memory / before
>> removing device memory. I am thinking about remove_pfn_range_from_zone()",
>> du undo everything "move_pfn_range_to_zone()" did.
> 
> what is "du undo"? I may not get it.

to undo ;)

-- 

Thanks,

David / dhildenb

^ permalink raw reply

* Re: [PATCH v3 00/11] mm/memory_hotplug: Factor out memory block devicehandling
From: Wei Yang @ 2019-06-03 21:21 UTC (permalink / raw)
  To: David Hildenbrand
  Cc: Mark Rutland, Oscar Salvador, Michal Hocko, linux-ia64, linux-sh,
	Peter Zijlstra, Dave Hansen, Heiko Carstens, Wei Yang, linux-mm,
	Pavel Tatashin, Rich Felker, Arun KS, Chintan Pandya, Ingo Molnar,
	Paul Mackerras, Qian Cai, linux-s390, H. Peter Anvin, Yu Zhao,
	Baoquan He, Logan Gunthorpe, Rafael J. Wysocki, Mike Rapoport,
	Jun Yao, mike.travis@hpe.com, Ingo Molnar, Catalin Marinas,
	Rob Herring, Fenghua Yu, Pavel Tatashin, Vasily Gorbik,
	Anshuman Khandual, Masahiro Yamada, Will Deacon, Robin Murphy,
	Nicholas Piggin, Martin Schwidefsky, Mark Brown, Borislav Petkov,
	Andy Lutomirski, Jonathan Cameron, Dan Williams, Chris Wilson,
	Joonsoo Kim, linux-arm-kernel, Oscar Salvador, Tony Luck,
	Yoshinori Sato, Ard Biesheuvel, Mathieu Malaterre,
	Greg Kroah-Hartman, Andrew Banman, linux-kernel, Mike Rapoport,
	Thomas Gleixner, Wei Yang, Alex Deucher, Igor Mammedov, akpm,
	linuxppc-dev, David S. Miller, Kirill A. Shutemov
In-Reply-To: <20190527111152.16324-1-david@redhat.com>

IMHO, there is some typo.

s/devicehandling/device handling/

On Mon, May 27, 2019 at 01:11:41PM +0200, David Hildenbrand wrote:
>We only want memory block devices for memory to be onlined/offlined
>(add/remove from the buddy). This is required so user space can
>online/offline memory and kdump gets notified about newly onlined memory.
>
>Let's factor out creation/removal of memory block devices. This helps
>to further cleanup arch_add_memory/arch_remove_memory() and to make
>implementation of new features easier - especially sub-section
>memory hot add from Dan.
>
>Anshuman Khandual is currently working on arch_remove_memory(). I added
>a temporary solution via "arm64/mm: Add temporary arch_remove_memory()
>implementation", that is sufficient as a firsts tep in the context of

s/firsts tep/first step/

>this series. (we don't cleanup page tables in case anything goes
>wrong already)
>
>Did a quick sanity test with DIMM plug/unplug, making sure all devices
>and sysfs links properly get added/removed. Compile tested on s390x and
>x86-64.
>
>Based on next/master.
>
>Next refactoring on my list will be making sure that remove_memory()
>will never deal with zones / access "struct pages". Any kind of zone
>handling will have to be done when offlining system memory / before
>removing device memory. I am thinking about remove_pfn_range_from_zone()",
>du undo everything "move_pfn_range_to_zone()" did.

what is "du undo"? I may not get it.

>
>v2 -> v3:
>- Add "s390x/mm: Fail when an altmap is used for arch_add_memory()"
>- Add "arm64/mm: Add temporary arch_remove_memory() implementation"
>- Add "drivers/base/memory: Pass a block_id to init_memory_block()"
>- Various changes to "mm/memory_hotplug: Create memory block devices
>  after arch_add_memory()" and "mm/memory_hotplug: Create memory block
>  devices after arch_add_memory()" due to switching from sections to
>  block_id's.
>
>v1 -> v2:
>- s390x/mm: Implement arch_remove_memory()
>-- remove mapping after "__remove_pages"
>
>David Hildenbrand (11):
>  mm/memory_hotplug: Simplify and fix check_hotplug_memory_range()
>  s390x/mm: Fail when an altmap is used for arch_add_memory()
>  s390x/mm: Implement arch_remove_memory()
>  arm64/mm: Add temporary arch_remove_memory() implementation
>  drivers/base/memory: Pass a block_id to init_memory_block()
>  mm/memory_hotplug: Allow arch_remove_pages() without
>    CONFIG_MEMORY_HOTREMOVE
>  mm/memory_hotplug: Create memory block devices after arch_add_memory()
>  mm/memory_hotplug: Drop MHP_MEMBLOCK_API
>  mm/memory_hotplug: Remove memory block devices before
>    arch_remove_memory()
>  mm/memory_hotplug: Make unregister_memory_block_under_nodes() never
>    fail
>  mm/memory_hotplug: Remove "zone" parameter from
>    sparse_remove_one_section
>
> arch/arm64/mm/mmu.c            |  17 +++++
> arch/ia64/mm/init.c            |   2 -
> arch/powerpc/mm/mem.c          |   2 -
> arch/s390/mm/init.c            |  18 +++--
> arch/sh/mm/init.c              |   2 -
> arch/x86/mm/init_32.c          |   2 -
> arch/x86/mm/init_64.c          |   2 -
> drivers/base/memory.c          | 134 +++++++++++++++++++--------------
> drivers/base/node.c            |  27 +++----
> include/linux/memory.h         |   6 +-
> include/linux/memory_hotplug.h |  12 +--
> include/linux/node.h           |   7 +-
> mm/memory_hotplug.c            |  44 +++++------
> mm/sparse.c                    |  10 +--
> 14 files changed, 140 insertions(+), 145 deletions(-)
>
>-- 
>2.20.1

-- 
Wei Yang
Help you, Help me

^ permalink raw reply

* Re: [PATCH v3 0/6] soc/fsl/qe: cleanups and new DT binding
From: Rasmus Villemoes @ 2019-06-03 19:53 UTC (permalink / raw)
  To: devicetree@vger.kernel.org, Qiang Zhao, Li Yang
  Cc: Mark Rutland, linux-kernel@vger.kernel.org, Scott Wood,
	Rob Herring, linuxppc-dev@lists.ozlabs.org,
	linux-arm-kernel@lists.infradead.org
In-Reply-To: <20190513111442.25724-1-rasmus.villemoes@prevas.dk>

On 13/05/2019 13.14, Rasmus Villemoes wrote:
> This small series consists of some small cleanups and simplifications
> of the QUICC engine driver, and introduces a new DT binding that makes
> it much easier to support other variants of the QUICC engine IP block
> that appears in the wild: There's no reason to expect in general that
> the number of valid SNUMs uniquely determines the set of such, so it's
> better to simply let the device tree specify the values (and,
> implicitly via the array length, also the count).
> 
> Which tree should this go through?

Ping? These patches should be ready to go in, but I don't know who is
supposed to pick them up.

Thanks,
Rasmus

^ permalink raw reply

* [PATCH] powerpc: Enable kernel XZ compression option on PPC_85xx
From: Pawel Dembicki @ 2019-06-03 16:41 UTC (permalink / raw)
  Cc: Christian Lamparter, linux-kernel, Pawel Dembicki, Paul Mackerras,
	linuxppc-dev

Enable kernel XZ compression option on PPC_85xx. Tested with
simpleImage on TP-Link TL-WDR4900 (Freescale P1014 processor).

Suggested-by: Christian Lamparter <chunkeey@gmail.com>
Signed-off-by: Pawel Dembicki <paweldembicki@gmail.com>
---
 arch/powerpc/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index 8c1c636308c8..daf4cb968922 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -196,7 +196,7 @@ config PPC
 	select HAVE_IOREMAP_PROT
 	select HAVE_IRQ_EXIT_ON_IRQ_STACK
 	select HAVE_KERNEL_GZIP
-	select HAVE_KERNEL_XZ			if PPC_BOOK3S || 44x
+	select HAVE_KERNEL_XZ			if PPC_BOOK3S || 44x || PPC_85xx
 	select HAVE_KPROBES
 	select HAVE_KPROBES_ON_FTRACE
 	select HAVE_KRETPROBES
-- 
2.20.1


^ permalink raw reply related

* Re: [PATCH 22/22] docs: fix broken documentation links
From: Mark Brown @ 2019-06-03 15:41 UTC (permalink / raw)
  To: Mauro Carvalho Chehab
  Cc: Andrew Lunn, Andy Lutomirski, Wolfram Sang, Catalin Marinas,
	Linus Walleij, Will Deacon, Pavel Tatashin, Paul Mackerras,
	Alessia Mantegazza, Jakub Wilk, Bartosz Golaszewski,
	Paul E. McKenney, Kevin Hilman, James Morris, linux-acpi,
	Ingo Molnar, xen-devel, Jason Wang, Alexander Popov, Qian Cai,
	Al Viro, Thomas Preston, Thomas Gleixner, Kairui Song, Ding Xiang,
	Greg Kroah-Hartman, Rafael J. Wysocki, linux-kernel, Paul Burton,
	Jiri Kosina, Casey Schaufler, Andrew Morton, Lu Baolu,
	Mark Rutland, Feng Tang, Linux Doc Mailing List, Dave Hansen,
	Mimi Zohar, Kamalesh Babulal, linux-mm, Masahiro Yamada,
	Yannik Sembritzki, Harry Wei, linux-i2c, Shuah Khan,
	Stephen Rothwell, Stefano Stabellini, Alexandre Ghiti, YueHaibing,
	Robert Moore, AKASHI Takahiro, Len Brown, Joerg Roedel,
	linux-arm-msm, linuxppc-dev, Mauro Carvalho Chehab, linux-gpio,
	Claudiu Manoil, Florian Fainelli, Jacek Anaszewski, Bjorn Helgaas,
	linux-amlogic, Boris Ostrovsky, Mika Westerberg, linux-arm-kernel,
	Tony Luck, Sean Christopherson, James Morse, Samuel Mendoza-Jonas,
	linux-pci, Bhupesh Sharma, Jonathan Cameron, platform-driver-x86,
	Quentin Perret, linux-kselftest, Alex Shi, Lorenzo Pieralisi,
	Baoquan He, Jonathan Corbet, Raphael Gault, Joel Stanley,
	Federico Vaga, Darren Hart, linux-edac, Erik Schmauss,
	Serge E. Hallyn, Palmer Dabbelt, Kees Cook,
	Bartlomiej Zolnierkiewicz, Jonathan Neuschäfer,
	SeongJae Park, Borislav Petkov, Sunil Muthuswamy, virtualization,
	devel, Ard Biesheuvel, Liam Girdwood, Sakari Ailus,
	Olof Johansson, Logan Gunthorpe, David S. Miller,
	Kirill A. Shutemov, Sven Van Asbroeck, Michal Hocko, kvm,
	Michael S. Tsirkin, Peter Zijlstra, Thorsten Leemhuis,
	David Howells, David Brown, H. Peter Anvin, devel, Manfred Spraul,
	x86, Russell King, Mike Rapoport, Andy Gross, Dave Young,
	devicetree, Arnaldo Carvalho de Melo, Jerome Glisse, Rob Herring,
	Josh Poimboeuf, Dmitry Vyukov, Luis Chamberlain, Juergen Gross,
	Denis Efremov, netdev, Nicolas Ferre, Changbin Du,
	linux-security-module, Robin Murphy, Andy Shevchenko
In-Reply-To: <f9fecacbe4ce0b2b3aed38d71ae3753f2daf3ce3.1559171394.git.mchehab+samsung@kernel.org>

[-- Attachment #1: Type: text/plain, Size: 231 bytes --]

On Wed, May 29, 2019 at 08:23:53PM -0300, Mauro Carvalho Chehab wrote:
> Mostly due to x86 and acpi conversion, several documentation
> links are still pointing to the old file. Fix them.

Acked-by: Mark Brown <broonie@kernel.org>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply

* Re: [PATCH v3 04/11] arm64/mm: Add temporary arch_remove_memory() implementation
From: Wei Yang @ 2019-06-03 21:41 UTC (permalink / raw)
  To: David Hildenbrand
  Cc: Mark Rutland, linux-s390, linux-ia64, Yu Zhao, Anshuman Khandual,
	linux-sh, Catalin Marinas, Ard Biesheuvel, Will Deacon,
	linux-kernel, Wei Yang, Jun Yao, linux-mm, Chintan Pandya,
	Igor Mammedov, akpm, Mike Rapoport, linuxppc-dev, Dan Williams,
	linux-arm-kernel, Robin Murphy
In-Reply-To: <20190527111152.16324-5-david@redhat.com>

On Mon, May 27, 2019 at 01:11:45PM +0200, David Hildenbrand wrote:
>A proper arch_remove_memory() implementation is on its way, which also
>cleanly removes page tables in arch_add_memory() in case something goes
>wrong.

Would this be better to understand?

    removes page tables created in arch_add_memory

>
>As we want to use arch_remove_memory() in case something goes wrong
>during memory hotplug after arch_add_memory() finished, let's add
>a temporary hack that is sufficient enough until we get a proper
>implementation that cleans up page table entries.
>
>We will remove CONFIG_MEMORY_HOTREMOVE around this code in follow up
>patches.
>
>Cc: Catalin Marinas <catalin.marinas@arm.com>
>Cc: Will Deacon <will.deacon@arm.com>
>Cc: Mark Rutland <mark.rutland@arm.com>
>Cc: Andrew Morton <akpm@linux-foundation.org>
>Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>Cc: Chintan Pandya <cpandya@codeaurora.org>
>Cc: Mike Rapoport <rppt@linux.ibm.com>
>Cc: Jun Yao <yaojun8558363@gmail.com>
>Cc: Yu Zhao <yuzhao@google.com>
>Cc: Robin Murphy <robin.murphy@arm.com>
>Cc: Anshuman Khandual <anshuman.khandual@arm.com>
>Signed-off-by: David Hildenbrand <david@redhat.com>
>---
> arch/arm64/mm/mmu.c | 19 +++++++++++++++++++
> 1 file changed, 19 insertions(+)
>
>diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
>index a1bfc4413982..e569a543c384 100644
>--- a/arch/arm64/mm/mmu.c
>+++ b/arch/arm64/mm/mmu.c
>@@ -1084,4 +1084,23 @@ int arch_add_memory(int nid, u64 start, u64 size,
> 	return __add_pages(nid, start >> PAGE_SHIFT, size >> PAGE_SHIFT,
> 			   restrictions);
> }
>+#ifdef CONFIG_MEMORY_HOTREMOVE
>+void arch_remove_memory(int nid, u64 start, u64 size,
>+			struct vmem_altmap *altmap)
>+{
>+	unsigned long start_pfn = start >> PAGE_SHIFT;
>+	unsigned long nr_pages = size >> PAGE_SHIFT;
>+	struct zone *zone;
>+
>+	/*
>+	 * FIXME: Cleanup page tables (also in arch_add_memory() in case
>+	 * adding fails). Until then, this function should only be used
>+	 * during memory hotplug (adding memory), not for memory
>+	 * unplug. ARCH_ENABLE_MEMORY_HOTREMOVE must not be
>+	 * unlocked yet.
>+	 */
>+	zone = page_zone(pfn_to_page(start_pfn));

Compared with arch_remove_memory in x86. If altmap is not NULL, zone will be
retrieved from page related to altmap. Not sure why this is not the same?

>+	__remove_pages(zone, start_pfn, nr_pages, altmap);
>+}
>+#endif
> #endif
>-- 
>2.20.1

-- 
Wei Yang
Help you, Help me

^ permalink raw reply

* Re: [PATCH kernel] prom_init: Fetch flatten device tree from the system firmware
From: Benjamin Herrenschmidt @ 2019-06-03 21:18 UTC (permalink / raw)
  To: Alexey Kardashevskiy, Segher Boessenkool
  Cc: linuxppc-dev, Suraj Jitindar Singh, David Gibson
In-Reply-To: <7fc6cd5e-ddd6-4028-b4ef-7bdcd6db69d0@ozlabs.ru>

On Mon, 2019-06-03 at 12:56 +1000, Alexey Kardashevskiy wrote:
> 
> > That is all you need if you do not want to use OF at all.
> 
> ? We also need OF drivers to boot grub and the system, and a default
> console for early booting, but yes, we do not want to keep using slof
> that much.
> 
> > If you *do* want to keep having an Open Firmware, what we want or need
> > is a faster way to walk huge device trees.
> 
> Why? We do not need to _walk_ the tree at all (we _have_ to now and
> while walking we do nothing but pack everything together into the fdt
> blob) as slof can easily do this already.

I agree with Alexey. In a sense this can be thought as an extension of
"quiesce", which effectively kills OF.

Yes we could make property fetching faster but mostly by creating a new
bulk interface which is quite a bit of work, a new API, and will in
practice not be used for anything other than creating the FDT. In that
case, nothing will beat in performance having OF create the FDT itself
based on its current tree.

> > > There is no use for the "fetch all properties" cases other than
> > > building an FDT that any of us can think of, and it would create a more
> > > complicated interface than just "fetch an FDT".
> > 
> > It is a simple way to speed up fetching the device tree enormously,
> > without needing big changes to either OF or the clients using it -- not
> > in the code, but importantly also not conceptually: everything works just
> > as before, just a lot faster.
> 
> I can safely presume though that this will never ever be used in
> practice. And it will be still slower, a tiny bit. And require new code
> in both slof and linux.

Correct.

> I can rather see us getting rid of SLOF in the future which in turn will
> require the fdt blob pointer in r5 (or whatever it is, forgot) when the
> guest starts; so "fetch-all-props" won't be used there either.
> 
> > > So I go for the simple one and agree with Alexey's idea.
> > 
> > When dealing with a whole device tree you have to know about the various
> > dynamically generated nodes and props, and handle each appropriately.
> 
> The code I am changing fetches the device tree and build an fdt. What is
> that special knowledge in this context you are talking about?

Ben.



^ permalink raw reply

* Re: [PATCH] PCI: rpaphp: Avoid a sometimes-uninitialized warning
From: Nick Desaulniers @ 2019-06-03 21:07 UTC (permalink / raw)
  To: Nathan Chancellor
  Cc: Tyrel Datwyler, LKML, clang-built-linux, Paul Mackerras,
	linux-pci, Bjorn Helgaas, linuxppc-dev
In-Reply-To: <20190603174323.48251-1-natechancellor@gmail.com>

On Mon, Jun 3, 2019 at 10:44 AM Nathan Chancellor
<natechancellor@gmail.com> wrote:
> Looking at the loop in a vacuum as clang would, fndit could be
> uninitialized if entries was ever zero or the if statement was
> always true within the loop. Regardless of whether or not this
> warning is a problem in practice, "found" variables should always
> be initialized to false so that there is no possibility of
> undefined behavior.

Thanks for the patch Nathan.  fndit isn't really being used for
anything other than a print statement outside of the loop.  How about:

```
diff --git a/drivers/pci/hotplug/rpaphp_core.c
b/drivers/pci/hotplug/rpaphp_core.c
index bcd5d357ca23..c3899ee1db99 100644
--- a/drivers/pci/hotplug/rpaphp_core.c
+++ b/drivers/pci/hotplug/rpaphp_core.c
@@ -230,7 +230,7 @@ static int rpaphp_check_drc_props_v2(struct
device_node *dn, char *drc_name,
  struct of_drc_info drc;
  const __be32 *value;
  char cell_drc_name[MAX_DRC_NAME_LEN];
- int j, fndit;
+ int j;

  info = of_find_property(dn->parent, "ibm,drc-info", NULL);
  if (info == NULL)
@@ -245,17 +245,13 @@ static int rpaphp_check_drc_props_v2(struct
device_node *dn, char *drc_name,

  /* Should now know end of current entry */

- if (my_index > drc.last_drc_index)
- continue;
-
- fndit = 1;
- break;
+ /* Found it */
+ if (my_index <= drc.last_drc_index) {
+ sprintf(cell_drc_name, "%s%d", drc.drc_name_prefix,
+ my_index);
+ break;
+ }
  }
- /* Found it */
-
- if (fndit)
- sprintf(cell_drc_name, "%s%d", drc.drc_name_prefix,
- my_index);

  if (((drc_name == NULL) ||
       (drc_name && !strcmp(drc_name, cell_drc_name))) &&
```
(not sure my tabs were pasted properly in the above...)

-- 
Thanks,
~Nick Desaulniers

^ permalink raw reply related

* RE: [PATCH v3 0/6] soc/fsl/qe: cleanups and new DT binding
From: Leo Li @ 2019-06-03 19:55 UTC (permalink / raw)
  To: Rasmus Villemoes, devicetree@vger.kernel.org, Qiang Zhao
  Cc: Mark Rutland, linux-kernel@vger.kernel.org, Scott Wood,
	Rob Herring, linuxppc-dev@lists.ozlabs.org,
	linux-arm-kernel@lists.infradead.org
In-Reply-To: <e11c1e55-1e11-7ce3-3c0f-0b723ab260aa@prevas.se>



> -----Original Message-----
> From: Rasmus Villemoes <Rasmus.Villemoes@prevas.se>
> Sent: Monday, June 3, 2019 2:54 PM
> To: devicetree@vger.kernel.org; Qiang Zhao <qiang.zhao@nxp.com>; Leo Li
> <leoyang.li@nxp.com>
> Cc: linuxppc-dev@lists.ozlabs.org; linux-arm-kernel@lists.infradead.org;
> linux-kernel@vger.kernel.org; Rob Herring <robh+dt@kernel.org>; Scott
> Wood <oss@buserror.net>; Christophe Leroy <christophe.leroy@c-s.fr>;
> Mark Rutland <mark.rutland@arm.com>; jocke@infinera.com
> <joakim.tjernlund@infinera.com>
> Subject: Re: [PATCH v3 0/6] soc/fsl/qe: cleanups and new DT binding
> 
> On 13/05/2019 13.14, Rasmus Villemoes wrote:
> > This small series consists of some small cleanups and simplifications
> > of the QUICC engine driver, and introduces a new DT binding that makes
> > it much easier to support other variants of the QUICC engine IP block
> > that appears in the wild: There's no reason to expect in general that
> > the number of valid SNUMs uniquely determines the set of such, so it's
> > better to simply let the device tree specify the values (and,
> > implicitly via the array length, also the count).
> >
> > Which tree should this go through?
> 
> Ping? These patches should be ready to go in, but I don't know who is
> supposed to pick them up.

I can pick them up through the soc/fsl tree.

Regards,
Leo

^ permalink raw reply

* Re: [PATCH v3] mm: add account_locked_vm utility function
From: Alex Williamson @ 2019-06-03 19:15 UTC (permalink / raw)
  To: Daniel Jordan
  Cc: Mark Rutland, Davidlohr Bueso, kvm, Alan Tull,
	Alexey Kardashevskiy, linux-fpga, linuxppc-dev, kvm-ppc,
	linux-kernel, linux-mm, Jason Gunthorpe, Moritz Fischer,
	Steve Sistare, akpm, Ira Weiny, Christoph Lameter, Wu Hao
In-Reply-To: <20190529205019.20927-1-daniel.m.jordan@oracle.com>

On Wed, 29 May 2019 16:50:19 -0400
Daniel Jordan <daniel.m.jordan@oracle.com> wrote:

> locked_vm accounting is done roughly the same way in five places, so
> unify them in a helper.
> 
> Include the helper's caller in the debug print to distinguish between
> callsites.
> 
> Error codes stay the same, so user-visible behavior does too.  The one
> exception is that the -EPERM case in tce_account_locked_vm is removed
> because Alexey has never seen it triggered.
> 
> Signed-off-by: Daniel Jordan <daniel.m.jordan@oracle.com>
> Tested-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> Cc: Alan Tull <atull@kernel.org>
> Cc: Alex Williamson <alex.williamson@redhat.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> Cc: Christoph Lameter <cl@linux.com>
> Cc: Christophe Leroy <christophe.leroy@c-s.fr>
> Cc: Davidlohr Bueso <dave@stgolabs.net>
> Cc: Ira Weiny <ira.weiny@intel.com>
> Cc: Jason Gunthorpe <jgg@mellanox.com>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: Michael Ellerman <mpe@ellerman.id.au>
> Cc: Moritz Fischer <mdf@kernel.org>
> Cc: Paul Mackerras <paulus@ozlabs.org>
> Cc: Steve Sistare <steven.sistare@oracle.com>
> Cc: Wu Hao <hao.wu@intel.com>
> Cc: linux-mm@kvack.org
> Cc: kvm@vger.kernel.org
> Cc: kvm-ppc@vger.kernel.org
> Cc: linuxppc-dev@lists.ozlabs.org
> Cc: linux-fpga@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> ---
> v3:
>  - uninline account_locked_vm (Andrew)
>  - fix doc comment (Ira)
>  - retain down_write_killable in vfio type1 (Alex)
>  - leave Alexey's T-b since the code is the same aside from uninlining
>  - sanity tested with vfio type1, sanity-built on ppc
> 
>  arch/powerpc/kvm/book3s_64_vio.c     | 44 ++--------------
>  arch/powerpc/mm/book3s64/iommu_api.c | 41 ++-------------
>  drivers/fpga/dfl-afu-dma-region.c    | 53 ++------------------
>  drivers/vfio/vfio_iommu_spapr_tce.c  | 54 ++------------------
>  drivers/vfio/vfio_iommu_type1.c      | 17 +------
>  include/linux/mm.h                   |  4 ++
>  mm/util.c                            | 75 ++++++++++++++++++++++++++++
>  7 files changed, 98 insertions(+), 190 deletions(-)

I tend to prefer adding a negative rather than converting to absolute
and passing a bool for inc/dec, but it all seems equivalent, so for
vfio parts

Acked-by: Alex Williamson <alex.williamson@redhat.com>

> 
> diff --git a/arch/powerpc/kvm/book3s_64_vio.c b/arch/powerpc/kvm/book3s_64_vio.c
> index 66270e07449a..768b645c7edf 100644
> --- a/arch/powerpc/kvm/book3s_64_vio.c
> +++ b/arch/powerpc/kvm/book3s_64_vio.c
> @@ -30,6 +30,7 @@
>  #include <linux/anon_inodes.h>
>  #include <linux/iommu.h>
>  #include <linux/file.h>
> +#include <linux/mm.h>
>  
>  #include <asm/kvm_ppc.h>
>  #include <asm/kvm_book3s.h>
> @@ -56,43 +57,6 @@ static unsigned long kvmppc_stt_pages(unsigned long tce_pages)
>  	return tce_pages + ALIGN(stt_bytes, PAGE_SIZE) / PAGE_SIZE;
>  }
>  
> -static long kvmppc_account_memlimit(unsigned long stt_pages, bool inc)
> -{
> -	long ret = 0;
> -
> -	if (!current || !current->mm)
> -		return ret; /* process exited */
> -
> -	down_write(&current->mm->mmap_sem);
> -
> -	if (inc) {
> -		unsigned long locked, lock_limit;
> -
> -		locked = current->mm->locked_vm + stt_pages;
> -		lock_limit = rlimit(RLIMIT_MEMLOCK) >> PAGE_SHIFT;
> -		if (locked > lock_limit && !capable(CAP_IPC_LOCK))
> -			ret = -ENOMEM;
> -		else
> -			current->mm->locked_vm += stt_pages;
> -	} else {
> -		if (WARN_ON_ONCE(stt_pages > current->mm->locked_vm))
> -			stt_pages = current->mm->locked_vm;
> -
> -		current->mm->locked_vm -= stt_pages;
> -	}
> -
> -	pr_debug("[%d] RLIMIT_MEMLOCK KVM %c%ld %ld/%ld%s\n", current->pid,
> -			inc ? '+' : '-',
> -			stt_pages << PAGE_SHIFT,
> -			current->mm->locked_vm << PAGE_SHIFT,
> -			rlimit(RLIMIT_MEMLOCK),
> -			ret ? " - exceeded" : "");
> -
> -	up_write(&current->mm->mmap_sem);
> -
> -	return ret;
> -}
> -
>  static void kvm_spapr_tce_iommu_table_free(struct rcu_head *head)
>  {
>  	struct kvmppc_spapr_tce_iommu_table *stit = container_of(head,
> @@ -302,7 +266,7 @@ static int kvm_spapr_tce_release(struct inode *inode, struct file *filp)
>  
>  	kvm_put_kvm(stt->kvm);
>  
> -	kvmppc_account_memlimit(
> +	account_locked_vm(current->mm,
>  		kvmppc_stt_pages(kvmppc_tce_pages(stt->size)), false);
>  	call_rcu(&stt->rcu, release_spapr_tce_table);
>  
> @@ -327,7 +291,7 @@ long kvm_vm_ioctl_create_spapr_tce(struct kvm *kvm,
>  		return -EINVAL;
>  
>  	npages = kvmppc_tce_pages(size);
> -	ret = kvmppc_account_memlimit(kvmppc_stt_pages(npages), true);
> +	ret = account_locked_vm(current->mm, kvmppc_stt_pages(npages), true);
>  	if (ret)
>  		return ret;
>  
> @@ -373,7 +337,7 @@ long kvm_vm_ioctl_create_spapr_tce(struct kvm *kvm,
>  
>  	kfree(stt);
>   fail_acct:
> -	kvmppc_account_memlimit(kvmppc_stt_pages(npages), false);
> +	account_locked_vm(current->mm, kvmppc_stt_pages(npages), false);
>  	return ret;
>  }
>  
> diff --git a/arch/powerpc/mm/book3s64/iommu_api.c b/arch/powerpc/mm/book3s64/iommu_api.c
> index 5c521f3924a5..18d22eec0ebd 100644
> --- a/arch/powerpc/mm/book3s64/iommu_api.c
> +++ b/arch/powerpc/mm/book3s64/iommu_api.c
> @@ -19,6 +19,7 @@
>  #include <linux/hugetlb.h>
>  #include <linux/swap.h>
>  #include <linux/sizes.h>
> +#include <linux/mm.h>
>  #include <asm/mmu_context.h>
>  #include <asm/pte-walk.h>
>  #include <linux/mm_inline.h>
> @@ -51,40 +52,6 @@ struct mm_iommu_table_group_mem_t {
>  	u64 dev_hpa;		/* Device memory base address */
>  };
>  
> -static long mm_iommu_adjust_locked_vm(struct mm_struct *mm,
> -		unsigned long npages, bool incr)
> -{
> -	long ret = 0, locked, lock_limit;
> -
> -	if (!npages)
> -		return 0;
> -
> -	down_write(&mm->mmap_sem);
> -
> -	if (incr) {
> -		locked = mm->locked_vm + npages;
> -		lock_limit = rlimit(RLIMIT_MEMLOCK) >> PAGE_SHIFT;
> -		if (locked > lock_limit && !capable(CAP_IPC_LOCK))
> -			ret = -ENOMEM;
> -		else
> -			mm->locked_vm += npages;
> -	} else {
> -		if (WARN_ON_ONCE(npages > mm->locked_vm))
> -			npages = mm->locked_vm;
> -		mm->locked_vm -= npages;
> -	}
> -
> -	pr_debug("[%d] RLIMIT_MEMLOCK HASH64 %c%ld %ld/%ld\n",
> -			current ? current->pid : 0,
> -			incr ? '+' : '-',
> -			npages << PAGE_SHIFT,
> -			mm->locked_vm << PAGE_SHIFT,
> -			rlimit(RLIMIT_MEMLOCK));
> -	up_write(&mm->mmap_sem);
> -
> -	return ret;
> -}
> -
>  bool mm_iommu_preregistered(struct mm_struct *mm)
>  {
>  	return !list_empty(&mm->context.iommu_group_mem_list);
> @@ -101,7 +68,7 @@ static long mm_iommu_do_alloc(struct mm_struct *mm, unsigned long ua,
>  	unsigned long entry, chunk;
>  
>  	if (dev_hpa == MM_IOMMU_TABLE_INVALID_HPA) {
> -		ret = mm_iommu_adjust_locked_vm(mm, entries, true);
> +		ret = account_locked_vm(mm, entries, true);
>  		if (ret)
>  			return ret;
>  
> @@ -216,7 +183,7 @@ static long mm_iommu_do_alloc(struct mm_struct *mm, unsigned long ua,
>  	kfree(mem);
>  
>  unlock_exit:
> -	mm_iommu_adjust_locked_vm(mm, locked_entries, false);
> +	account_locked_vm(mm, locked_entries, false);
>  
>  	return ret;
>  }
> @@ -316,7 +283,7 @@ long mm_iommu_put(struct mm_struct *mm, struct mm_iommu_table_group_mem_t *mem)
>  unlock_exit:
>  	mutex_unlock(&mem_list_mutex);
>  
> -	mm_iommu_adjust_locked_vm(mm, unlock_entries, false);
> +	account_locked_vm(mm, unlock_entries, false);
>  
>  	return ret;
>  }
> diff --git a/drivers/fpga/dfl-afu-dma-region.c b/drivers/fpga/dfl-afu-dma-region.c
> index c438722bf4e1..0a532c602d8f 100644
> --- a/drivers/fpga/dfl-afu-dma-region.c
> +++ b/drivers/fpga/dfl-afu-dma-region.c
> @@ -12,6 +12,7 @@
>  #include <linux/dma-mapping.h>
>  #include <linux/sched/signal.h>
>  #include <linux/uaccess.h>
> +#include <linux/mm.h>
>  
>  #include "dfl-afu.h"
>  
> @@ -31,52 +32,6 @@ void afu_dma_region_init(struct dfl_feature_platform_data *pdata)
>  	afu->dma_regions = RB_ROOT;
>  }
>  
> -/**
> - * afu_dma_adjust_locked_vm - adjust locked memory
> - * @dev: port device
> - * @npages: number of pages
> - * @incr: increase or decrease locked memory
> - *
> - * Increase or decrease the locked memory size with npages input.
> - *
> - * Return 0 on success.
> - * Return -ENOMEM if locked memory size is over the limit and no CAP_IPC_LOCK.
> - */
> -static int afu_dma_adjust_locked_vm(struct device *dev, long npages, bool incr)
> -{
> -	unsigned long locked, lock_limit;
> -	int ret = 0;
> -
> -	/* the task is exiting. */
> -	if (!current->mm)
> -		return 0;
> -
> -	down_write(&current->mm->mmap_sem);
> -
> -	if (incr) {
> -		locked = current->mm->locked_vm + npages;
> -		lock_limit = rlimit(RLIMIT_MEMLOCK) >> PAGE_SHIFT;
> -
> -		if (locked > lock_limit && !capable(CAP_IPC_LOCK))
> -			ret = -ENOMEM;
> -		else
> -			current->mm->locked_vm += npages;
> -	} else {
> -		if (WARN_ON_ONCE(npages > current->mm->locked_vm))
> -			npages = current->mm->locked_vm;
> -		current->mm->locked_vm -= npages;
> -	}
> -
> -	dev_dbg(dev, "[%d] RLIMIT_MEMLOCK %c%ld %ld/%ld%s\n", current->pid,
> -		incr ? '+' : '-', npages << PAGE_SHIFT,
> -		current->mm->locked_vm << PAGE_SHIFT, rlimit(RLIMIT_MEMLOCK),
> -		ret ? "- exceeded" : "");
> -
> -	up_write(&current->mm->mmap_sem);
> -
> -	return ret;
> -}
> -
>  /**
>   * afu_dma_pin_pages - pin pages of given dma memory region
>   * @pdata: feature device platform data
> @@ -92,7 +47,7 @@ static int afu_dma_pin_pages(struct dfl_feature_platform_data *pdata,
>  	struct device *dev = &pdata->dev->dev;
>  	int ret, pinned;
>  
> -	ret = afu_dma_adjust_locked_vm(dev, npages, true);
> +	ret = account_locked_vm(current->mm, npages, true);
>  	if (ret)
>  		return ret;
>  
> @@ -121,7 +76,7 @@ static int afu_dma_pin_pages(struct dfl_feature_platform_data *pdata,
>  free_pages:
>  	kfree(region->pages);
>  unlock_vm:
> -	afu_dma_adjust_locked_vm(dev, npages, false);
> +	account_locked_vm(current->mm, npages, false);
>  	return ret;
>  }
>  
> @@ -141,7 +96,7 @@ static void afu_dma_unpin_pages(struct dfl_feature_platform_data *pdata,
>  
>  	put_all_pages(region->pages, npages);
>  	kfree(region->pages);
> -	afu_dma_adjust_locked_vm(dev, npages, false);
> +	account_locked_vm(current->mm, npages, false);
>  
>  	dev_dbg(dev, "%ld pages unpinned\n", npages);
>  }
> diff --git a/drivers/vfio/vfio_iommu_spapr_tce.c b/drivers/vfio/vfio_iommu_spapr_tce.c
> index 40ddc0c5f677..d06e8e291924 100644
> --- a/drivers/vfio/vfio_iommu_spapr_tce.c
> +++ b/drivers/vfio/vfio_iommu_spapr_tce.c
> @@ -22,6 +22,7 @@
>  #include <linux/vmalloc.h>
>  #include <linux/sched/mm.h>
>  #include <linux/sched/signal.h>
> +#include <linux/mm.h>
>  
>  #include <asm/iommu.h>
>  #include <asm/tce.h>
> @@ -34,51 +35,6 @@
>  static void tce_iommu_detach_group(void *iommu_data,
>  		struct iommu_group *iommu_group);
>  
> -static long try_increment_locked_vm(struct mm_struct *mm, long npages)
> -{
> -	long ret = 0, locked, lock_limit;
> -
> -	if (WARN_ON_ONCE(!mm))
> -		return -EPERM;
> -
> -	if (!npages)
> -		return 0;
> -
> -	down_write(&mm->mmap_sem);
> -	locked = mm->locked_vm + npages;
> -	lock_limit = rlimit(RLIMIT_MEMLOCK) >> PAGE_SHIFT;
> -	if (locked > lock_limit && !capable(CAP_IPC_LOCK))
> -		ret = -ENOMEM;
> -	else
> -		mm->locked_vm += npages;
> -
> -	pr_debug("[%d] RLIMIT_MEMLOCK +%ld %ld/%ld%s\n", current->pid,
> -			npages << PAGE_SHIFT,
> -			mm->locked_vm << PAGE_SHIFT,
> -			rlimit(RLIMIT_MEMLOCK),
> -			ret ? " - exceeded" : "");
> -
> -	up_write(&mm->mmap_sem);
> -
> -	return ret;
> -}
> -
> -static void decrement_locked_vm(struct mm_struct *mm, long npages)
> -{
> -	if (!mm || !npages)
> -		return;
> -
> -	down_write(&mm->mmap_sem);
> -	if (WARN_ON_ONCE(npages > mm->locked_vm))
> -		npages = mm->locked_vm;
> -	mm->locked_vm -= npages;
> -	pr_debug("[%d] RLIMIT_MEMLOCK -%ld %ld/%ld\n", current->pid,
> -			npages << PAGE_SHIFT,
> -			mm->locked_vm << PAGE_SHIFT,
> -			rlimit(RLIMIT_MEMLOCK));
> -	up_write(&mm->mmap_sem);
> -}
> -
>  /*
>   * VFIO IOMMU fd for SPAPR_TCE IOMMU implementation
>   *
> @@ -336,7 +292,7 @@ static int tce_iommu_enable(struct tce_container *container)
>  		return ret;
>  
>  	locked = table_group->tce32_size >> PAGE_SHIFT;
> -	ret = try_increment_locked_vm(container->mm, locked);
> +	ret = account_locked_vm(container->mm, locked, true);
>  	if (ret)
>  		return ret;
>  
> @@ -355,7 +311,7 @@ static void tce_iommu_disable(struct tce_container *container)
>  	container->enabled = false;
>  
>  	BUG_ON(!container->mm);
> -	decrement_locked_vm(container->mm, container->locked_pages);
> +	account_locked_vm(container->mm, container->locked_pages, false);
>  }
>  
>  static void *tce_iommu_open(unsigned long arg)
> @@ -659,7 +615,7 @@ static long tce_iommu_create_table(struct tce_container *container,
>  	if (!table_size)
>  		return -EINVAL;
>  
> -	ret = try_increment_locked_vm(container->mm, table_size >> PAGE_SHIFT);
> +	ret = account_locked_vm(container->mm, table_size >> PAGE_SHIFT, true);
>  	if (ret)
>  		return ret;
>  
> @@ -678,7 +634,7 @@ static void tce_iommu_free_table(struct tce_container *container,
>  	unsigned long pages = tbl->it_allocated_size >> PAGE_SHIFT;
>  
>  	iommu_tce_table_put(tbl);
> -	decrement_locked_vm(container->mm, pages);
> +	account_locked_vm(container->mm, pages, false);
>  }
>  
>  static long tce_iommu_create_window(struct tce_container *container,
> diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
> index 3ddc375e7063..bf449ace1676 100644
> --- a/drivers/vfio/vfio_iommu_type1.c
> +++ b/drivers/vfio/vfio_iommu_type1.c
> @@ -275,21 +275,8 @@ static int vfio_lock_acct(struct vfio_dma *dma, long npage, bool async)
>  
>  	ret = down_write_killable(&mm->mmap_sem);
>  	if (!ret) {
> -		if (npage > 0) {
> -			if (!dma->lock_cap) {
> -				unsigned long limit;
> -
> -				limit = task_rlimit(dma->task,
> -						RLIMIT_MEMLOCK) >> PAGE_SHIFT;
> -
> -				if (mm->locked_vm + npage > limit)
> -					ret = -ENOMEM;
> -			}
> -		}
> -
> -		if (!ret)
> -			mm->locked_vm += npage;
> -
> +		ret = __account_locked_vm(mm, abs(npage), npage > 0, dma->task,
> +					  dma->lock_cap);
>  		up_write(&mm->mmap_sem);
>  	}
>  
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index 0e8834ac32b7..95510f6fad45 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -1564,6 +1564,10 @@ long get_user_pages_unlocked(unsigned long start, unsigned long nr_pages,
>  int get_user_pages_fast(unsigned long start, int nr_pages,
>  			unsigned int gup_flags, struct page **pages);
>  
> +int account_locked_vm(struct mm_struct *mm, unsigned long pages, bool inc);
> +int __account_locked_vm(struct mm_struct *mm, unsigned long pages, bool inc,
> +			struct task_struct *task, bool bypass_rlim);
> +
>  /* Container for pinned pfns / pages */
>  struct frame_vector {
>  	unsigned int nr_allocated;	/* Number of frames we have space for */
> diff --git a/mm/util.c b/mm/util.c
> index 91682a2090ee..cbbcc035b12b 100644
> --- a/mm/util.c
> +++ b/mm/util.c
> @@ -7,6 +7,7 @@
>  #include <linux/err.h>
>  #include <linux/sched.h>
>  #include <linux/sched/mm.h>
> +#include <linux/sched/signal.h>
>  #include <linux/sched/task_stack.h>
>  #include <linux/security.h>
>  #include <linux/swap.h>
> @@ -347,6 +348,80 @@ int __weak get_user_pages_fast(unsigned long start,
>  }
>  EXPORT_SYMBOL_GPL(get_user_pages_fast);
>  
> +/**
> + * __account_locked_vm - account locked pages to an mm's locked_vm
> + * @mm:          mm to account against
> + * @pages:       number of pages to account
> + * @inc:         %true if @pages should be considered positive, %false if not
> + * @task:        task used to check RLIMIT_MEMLOCK
> + * @bypass_rlim: %true if checking RLIMIT_MEMLOCK should be skipped
> + *
> + * Assumes @task and @mm are valid (i.e. at least one reference on each), and
> + * that mmap_sem is held as writer.
> + *
> + * Return:
> + * * 0       on success
> + * * -ENOMEM if RLIMIT_MEMLOCK would be exceeded.
> + */
> +int __account_locked_vm(struct mm_struct *mm, unsigned long pages, bool inc,
> +			struct task_struct *task, bool bypass_rlim)
> +{
> +	unsigned long locked_vm, limit;
> +	int ret = 0;
> +
> +	lockdep_assert_held_exclusive(&mm->mmap_sem);
> +
> +	locked_vm = mm->locked_vm;
> +	if (inc) {
> +		if (!bypass_rlim) {
> +			limit = task_rlimit(task, RLIMIT_MEMLOCK) >> PAGE_SHIFT;
> +			if (locked_vm + pages > limit)
> +				ret = -ENOMEM;
> +		}
> +		if (!ret)
> +			mm->locked_vm = locked_vm + pages;
> +	} else {
> +		WARN_ON_ONCE(pages > locked_vm);
> +		mm->locked_vm = locked_vm - pages;
> +	}
> +
> +	pr_debug("%s: [%d] caller %ps %c%lu %lu/%lu%s\n", __func__, task->pid,
> +		 (void *)_RET_IP_, (inc) ? '+' : '-', pages << PAGE_SHIFT,
> +		 locked_vm << PAGE_SHIFT, task_rlimit(task, RLIMIT_MEMLOCK),
> +		 ret ? " - exceeded" : "");
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(__account_locked_vm);
> +
> +/**
> + * account_locked_vm - account locked pages to an mm's locked_vm
> + * @mm:          mm to account against, may be NULL
> + * @pages:       number of pages to account
> + * @inc:         %true if @pages should be considered positive, %false if not
> + *
> + * Assumes a non-NULL @mm is valid (i.e. at least one reference on it).
> + *
> + * Return:
> + * * 0       on success, or if mm is NULL
> + * * -ENOMEM if RLIMIT_MEMLOCK would be exceeded.
> + */
> +int account_locked_vm(struct mm_struct *mm, unsigned long pages, bool inc)
> +{
> +	int ret;
> +
> +	if (pages == 0 || !mm)
> +		return 0;
> +
> +	down_write(&mm->mmap_sem);
> +	ret = __account_locked_vm(mm, pages, inc, current,
> +				  capable(CAP_IPC_LOCK));
> +	up_write(&mm->mmap_sem);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(account_locked_vm);
> +
>  unsigned long vm_mmap_pgoff(struct file *file, unsigned long addr,
>  	unsigned long len, unsigned long prot,
>  	unsigned long flag, unsigned long pgoff)
> 
> base-commit: cd6c84d8f0cdc911df435bb075ba22ce3c605b07


^ permalink raw reply

* [PATCH] PCI: rpaphp: Avoid a sometimes-uninitialized warning
From: Nathan Chancellor @ 2019-06-03 17:43 UTC (permalink / raw)
  To: Tyrel Datwyler, Benjamin Herrenschmidt, Paul Mackerras,
	Michael Ellerman
  Cc: linux-pci, linux-kernel, clang-built-linux, Bjorn Helgaas,
	Nathan Chancellor, linuxppc-dev

When building with -Wsometimes-uninitialized, clang warns:

drivers/pci/hotplug/rpaphp_core.c:243:14: warning: variable 'fndit' is
used uninitialized whenever 'for' loop exits because its condition is
false [-Wsometimes-uninitialized]
        for (j = 0; j < entries; j++) {
                    ^~~~~~~~~~~
drivers/pci/hotplug/rpaphp_core.c:256:6: note: uninitialized use occurs
here
        if (fndit)
            ^~~~~
drivers/pci/hotplug/rpaphp_core.c:243:14: note: remove the condition if
it is always true
        for (j = 0; j < entries; j++) {
                    ^~~~~~~~~~~
drivers/pci/hotplug/rpaphp_core.c:233:14: note: initialize the variable
'fndit' to silence this warning
        int j, fndit;
                    ^
                     = 0

Looking at the loop in a vacuum as clang would, fndit could be
uninitialized if entries was ever zero or the if statement was
always true within the loop. Regardless of whether or not this
warning is a problem in practice, "found" variables should always
be initialized to false so that there is no possibility of
undefined behavior.

Link: https://github.com/ClangBuiltLinux/linux/issues/504
Fixes: 2fcf3ae508c2 ("hotplug/drc-info: Add code to search ibm,drc-info property")
Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
---
 drivers/pci/hotplug/rpaphp_core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pci/hotplug/rpaphp_core.c b/drivers/pci/hotplug/rpaphp_core.c
index bcd5d357ca23..07b56bf2886f 100644
--- a/drivers/pci/hotplug/rpaphp_core.c
+++ b/drivers/pci/hotplug/rpaphp_core.c
@@ -230,7 +230,7 @@ static int rpaphp_check_drc_props_v2(struct device_node *dn, char *drc_name,
 	struct of_drc_info drc;
 	const __be32 *value;
 	char cell_drc_name[MAX_DRC_NAME_LEN];
-	int j, fndit;
+	int j, fndit = 0;
 
 	info = of_find_property(dn->parent, "ibm,drc-info", NULL);
 	if (info == NULL)
-- 
2.22.0.rc2


^ permalink raw reply related

* Re: [PATCH 03/16] mm: simplify gup_fast_permitted
From: Linus Torvalds @ 2019-06-03 17:02 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: Rich Felker, Yoshinori Sato, Linux-sh list, James Hogan,
	the arch/x86 maintainers, Khalid Aziz, Nicholas Piggin,
	linux-mips, Linux-MM, Paul Burton, Paul Mackerras,
	Andrey Konovalov, sparclinux, linuxppc-dev, David S. Miller,
	Linux List Kernel Mailing
In-Reply-To: <CAHk-=wg5mww3StP8HqPN4d5eij3KmEayM743v-nDKAMgRe2J6g@mail.gmail.com>

On Mon, Jun 3, 2019 at 9:08 AM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> The new code has no test at all for "nr_pages == 0", afaik.

Note that it really is important to check for that, because right now we do

        if (gup_fast_permitted(start, nr_pages)) {
                local_irq_save(flags);
                gup_pgd_range(start, end, write ? FOLL_WRITE : 0, pages, &nr);
                local_irq_restore(flags);
        }

and that gup_pgd_range() function *depends* on the range being
non-zero, and does

        pgdp = pgd_offset(current->mm, addr);
        do {
                pgd_t pgd = READ_ONCE(*pgdp);
...
        } while (pgdp++, addr = next, addr != end);

Note how a zero range would turn into an infinite range here.

And the only check for 0 was that

        if (nr_pages <= 0)
                return 0;

in get_user_pages_fast() that you removed.

(Admittedly, it would be much better to have that check in
__get_user_pages_fast() itself, because we do have callers that call
the double-underscore version)

Now, I sincerely hope that we don't have anybody that passes in a zero
nr_pages (or a negative one), but we do actually have a comment saying
it's ok.

Note that the check for "if (end < start)" not only does not check for
0, it also doesn't really check for negative. It checks for
_overflow_. Admittedly most negative values would be expected to
overflow, but it's still a very different issue.

Maybe you added the check for negative somewhere else (in another
patch), but I don't see it.

                Linus

^ permalink raw reply

* Re: [PATCH v3 0/6] Prerequisites for NXP LS104xA SMMU enablement
From: Andreas Färber @ 2019-06-03 16:42 UTC (permalink / raw)
  To: Laurentiu Tudor
  Cc: Madalin-cristian Bucur, netdev@vger.kernel.org, Roy Pledge,
	linux-kernel@vger.kernel.org, Leo Li,
	iommu@lists.linux-foundation.org, Camelia Alexandra Groza,
	Mian Yousaf Kaukab, linuxppc-dev@lists.ozlabs.org,
	davem@davemloft.net, linux-arm-kernel@lists.infradead.org
In-Reply-To: <VI1PR04MB5134E4DA6EA052BEBB3C26EFEC190@VI1PR04MB5134.eurprd04.prod.outlook.com>

Am 31.05.19 um 19:32 schrieb Laurentiu Tudor:
>> -----Original Message-----
>> From: Andreas Färber <afaerber@suse.de>
>> Sent: Friday, May 31, 2019 8:04 PM
>>
>> Hello Laurentiu,
>>
>> Am 31.05.19 um 18:46 schrieb Laurentiu Tudor:
>>>> -----Original Message-----
>>>> From: Andreas Färber <afaerber@suse.de>
>>>> Sent: Friday, May 31, 2019 7:15 PM
>>>>
>>>> Hi Laurentiu,
>>>>
>>>> Am 30.05.19 um 16:19 schrieb laurentiu.tudor@nxp.com:
>>>>> This patch series contains several fixes in preparation for SMMU
>>>>> support on NXP LS1043A and LS1046A chips. Once these get picked up,
>>>>> I'll submit the actual SMMU enablement patches consisting in the
>>>>> required device tree changes.
>>>>
>>>> Have you thought through what will happen if this patch ordering is not
>>>> preserved? In particular, a user installing a future U-Boot update with
>>>> the DTB bits but booting a stable kernel without this patch series -
>>>> wouldn't that regress dpaa then for our customers?
>>>>
>>>
>>> These are fixes for issues that popped out after enabling SMMU.
>>> I do not expect them to break anything.
>>
>> That was not my question! You're missing my point: All your patches are
>> lacking a Fixes header in their commit message, for backporting them, to
>> avoid _your DT patches_ breaking the driver on stable branches!
> 
> It does appear that I'm missing your point. For sure, the DT updates solely will
> break the kernel without these fixes but I'm not sure I understand how this
> could happen.

In short, customers rarely run master branch. Kindly have your
colleagues explain stable branches to you in details.

With Fixes header I was referring to the syntax explained here:
https://www.kernel.org/doc/html/latest/process/submitting-patches.html#describe-your-changes
https://www.kernel.org/doc/html/latest/process/submitting-patches.html#using-reported-by-tested-by-reviewed-by-suggested-by-and-fixes

> My plan was to share the kernel dts patches sometime after this series
> makes it through.

That's fine. What I'm warning you is that seemingly your DT patches,
once in one of your LSDK U-Boot releases, will cause a regression for
distros like our SLES 15 SP1 unless these prereq kernel patches get
applied on the respective stable branches. Which will not happen
automatically unless you as patch author take the appropriate action
before they get merged.

Thanks,
Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Mary Higgins, Sri Rasiah
HRB 21284 (AG Nürnberg)

^ permalink raw reply

* Re: [PATCH 03/16] mm: simplify gup_fast_permitted
From: Linus Torvalds @ 2019-06-03 16:08 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: Rich Felker, Yoshinori Sato, Linux-sh list, James Hogan,
	the arch/x86 maintainers, Khalid Aziz, Nicholas Piggin,
	linux-mips, Linux-MM, Paul Burton, Paul Mackerras,
	Andrey Konovalov, sparclinux, linuxppc-dev, David S. Miller,
	Linux List Kernel Mailing
In-Reply-To: <20190603074121.GA22920@lst.de>

On Mon, Jun 3, 2019 at 12:41 AM Christoph Hellwig <hch@lst.de> wrote:
>
> I only removed a duplicate of it.

I don't see any remaining cases.

> The full (old) code in get_user_pages_fast() looks like this:
>
>         if (nr_pages <= 0)
>                 return 0;
>
>         if (unlikely(!access_ok((void __user *)start, len)))
>                 return -EFAULT;
>
>         if (gup_fast_permitted(start, nr_pages)) {

Yes, and that code was correct.

The new code has no test at all for "nr_pages == 0", afaik.

                 Linus

^ permalink raw reply

* Re: [PATCH 01/16] uaccess: add untagged_addr definition for other arches
From: Khalid Aziz @ 2019-06-03 15:16 UTC (permalink / raw)
  To: Christoph Hellwig, Linus Torvalds, Paul Burton, James Hogan,
	Yoshinori Sato, Rich Felker, David S. Miller, Andrey Konovalov
  Cc: Catalin Marinas, linux-sh, x86, linux-mips, Nicholas Piggin,
	linux-kernel, linux-mm, Paul Mackerras, sparclinux, linuxppc-dev
In-Reply-To: <20190601074959.14036-2-hch@lst.de>

On 6/1/19 1:49 AM, Christoph Hellwig wrote:
> From: Andrey Konovalov <andreyknvl@google.com>
> 
> To allow arm64 syscalls to accept tagged pointers from userspace, we must
> untag them when they are passed to the kernel. Since untagging is done in
> generic parts of the kernel, the untagged_addr macro needs to be defined
> for all architectures.
> 
> Define it as a noop for architectures other than arm64.

Could you reword above sentence? We are already starting off with
untagged_addr() not being no-op for arm64 and sparc64. It will expand
further potentially. So something more along the lines of "Define it as
noop for architectures that do not support memory tagging". The first
paragraph in the log can also be rewritten to be not specific to arm64.

--
Khalid

> 
> Acked-by: Catalin Marinas <catalin.marinas@arm.com>
> Signed-off-by: Andrey Konovalov <andreyknvl@google.com>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  include/linux/mm.h | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index 0e8834ac32b7..949d43e9c0b6 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -99,6 +99,10 @@ extern int mmap_rnd_compat_bits __read_mostly;
>  #include <asm/pgtable.h>
>  #include <asm/processor.h>
>  
> +#ifndef untagged_addr
> +#define untagged_addr(addr) (addr)
> +#endif
> +
>  #ifndef __pa_symbol
>  #define __pa_symbol(x)  __pa(RELOC_HIDE((unsigned long)(x), 0))
>  #endif
> 


^ permalink raw reply


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox