* [PATCH v2 0/7] libfs: group and simplify linux fs code
From: Emanuele Giuseppe Esposito @ 2020-04-21 13:51 UTC (permalink / raw)
To: linux-fsdevel
Cc: Rafael J. Wysocki, David Airlie, dri-devel, Christoph Hellwig,
Andrew Donnellan, Emanuele Giuseppe Esposito, linux-scsi,
James Morris, Serge E. Hallyn, Daniel Vetter, Arnd Bergmann,
James E.J. Bottomley, Maarten Lankhorst, Maxime Ripard,
Manoj N. Kumar, Alexander Viro, Matthew R. Ochs, Uma Krishnan,
John Johansen, Martin K. Petersen, Greg Kroah-Hartman,
linux-kernel, linux-security-module, Thomas Zimmermann,
Frederic Barrat, Paolo Bonzini, linuxppc-dev, Joel Becker
libfs.c has many functions that are useful to implement dentry and inode
operations, but not many at the filesystem level. As a result, code to
create files and inodes has a lot of duplication, to the point that
tracefs has copied several hundred lines from debugfs.
The main two libfs.c functions for filesystems are simple_pin_fs and
simple_release_fs, which hide a somewhat complicated locking sequence
that is needed to serialize vfs_kern_mount and mntget. In this series,
my aim is to add functions that create dentries and inodes of various
kinds (either anonymous inodes, or directory/file/symlink). These
functions take the code that was duplicated across debugfs and tracefs
and move it to libfs.c.
In order to limit the number of arguments to the new functions, the
series first creates a data type that is passed to both
simple_pin_fs/simple_release_fs and the new creation functions. The new
struct, introduced in patch 2, simply groups the "mount" and "count"
arguments to simple_pin_fs and simple_release_fs.
Patches 1-4 are preparations to introduce the new simple_fs struct and
new functions that are useful in the remainder of the series. Patch 5
introduces the dentry and inode creation functions. Patch 6-7 can then
adopt them in debugfs and tracefs.
Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
v1->v2: rename simple_new_inode in new_inode_current_time,
more detailed explanations, put all common code in fs/libfs.c
Emanuele Giuseppe Esposito (7):
apparmor: just use vfs_kern_mount to make .null
libfs: wrap simple_pin_fs/simple_release_fs arguments in a struct
libfs: introduce new_inode_current_time
libfs: add alloc_anon_inode wrapper
libfs: add file creation functions
debugfs: switch to simplefs inode creation API
tracefs: switch to simplefs inode creation API
drivers/gpu/drm/drm_drv.c | 11 +-
drivers/misc/cxl/api.c | 13 +-
drivers/scsi/cxlflash/ocxl_hw.c | 14 +-
fs/binfmt_misc.c | 9 +-
fs/configfs/mount.c | 10 +-
fs/debugfs/inode.c | 158 +++--------------
fs/libfs.c | 299 ++++++++++++++++++++++++++++++--
fs/tracefs/inode.c | 96 ++--------
include/linux/fs.h | 31 +++-
security/apparmor/apparmorfs.c | 38 ++--
security/inode.c | 11 +-
11 files changed, 399 insertions(+), 291 deletions(-)
--
2.25.2
^ permalink raw reply
* Re: [PATCH] KVM: PPC: Book3S HV: read ibm,secure-memory nodes
From: Oliver O'Halloran @ 2020-04-21 13:43 UTC (permalink / raw)
To: Michael Ellerman
Cc: Alexey Kardashevskiy, Linux Kernel Mailing List, kvm-ppc,
Paul Mackerras, Laurent Dufour, linuxppc-dev
In-Reply-To: <87k129gdx8.fsf@mpe.ellerman.id.au>
On Tue, Apr 21, 2020 at 11:37 PM Michael Ellerman <mpe@ellerman.id.au> wrote:
>
> Hi Laurent,
>
> Laurent Dufour <ldufour@linux.ibm.com> writes:
> > The newly introduced ibm,secure-memory nodes supersede the
> > ibm,uv-firmware's property secure-memory-ranges.
>
> Is either documented in a device tree binding document anywhere?
>
> cheers
>
> > Firmware will no more expose the secure-memory-ranges property so first
> > read the new one and if not found rollback to the older one.
There's some in Ryan's UV support series for skiboot:
https://patchwork.ozlabs.org/project/skiboot/patch/20200227204023.22125-2-grimm@linux.ibm.com/
...which is also marked RFC. Cool.
Oliver
^ permalink raw reply
* Re: [PATCH] KVM: PPC: Book3S HV: read ibm,secure-memory nodes
From: Michael Ellerman @ 2020-04-21 13:34 UTC (permalink / raw)
To: Laurent Dufour, kvm-ppc, linuxppc-dev
Cc: Alexey Kardashevskiy, paulus, linux-kernel
In-Reply-To: <20200416162715.45846-1-ldufour@linux.ibm.com>
Hi Laurent,
Laurent Dufour <ldufour@linux.ibm.com> writes:
> The newly introduced ibm,secure-memory nodes supersede the
> ibm,uv-firmware's property secure-memory-ranges.
Is either documented in a device tree binding document anywhere?
cheers
> Firmware will no more expose the secure-memory-ranges property so first
> read the new one and if not found rollback to the older one.
>
> Signed-off-by: Laurent Dufour <ldufour@linux.ibm.com>
> ---
> arch/powerpc/kvm/book3s_hv_uvmem.c | 14 ++++++++++++++
> 1 file changed, 14 insertions(+)
>
> diff --git a/arch/powerpc/kvm/book3s_hv_uvmem.c b/arch/powerpc/kvm/book3s_hv_uvmem.c
> index 53b88cae3e73..ad950f8996e0 100644
> --- a/arch/powerpc/kvm/book3s_hv_uvmem.c
> +++ b/arch/powerpc/kvm/book3s_hv_uvmem.c
> @@ -735,6 +735,20 @@ static u64 kvmppc_get_secmem_size(void)
> const __be32 *prop;
> u64 size = 0;
>
> + /*
> + * First try the new ibm,secure-memory nodes which supersede the
> + * secure-memory-ranges property.
> + * If we found somes, no need to read the deprecated one.
> + */
> + for_each_compatible_node(np, NULL, "ibm,secure-memory") {
> + prop = of_get_property(np, "reg", &len);
> + if (!prop)
> + continue;
> + size += of_read_number(prop + 2, 2);
> + }
> + if (size)
> + return size;
> +
> np = of_find_compatible_node(NULL, NULL, "ibm,uv-firmware");
> if (!np)
> goto out;
> --
> 2.26.1
^ permalink raw reply
* Re: [PATCH v5 0/5] Track and expose idle PURR and SPURR ticks
From: Nathan Lynch @ 2020-04-21 13:30 UTC (permalink / raw)
To: Gautham R. Shenoy
Cc: Gautham R. Shenoy, Tyrel Datwyler, linux-kernel, Kamalesh Babulal,
Naveen N. Rao, Vaidyanathan Srinivasan, linuxppc-dev
In-Reply-To: <1586249263-14048-1-git-send-email-ego@linux.vnet.ibm.com>
"Gautham R. Shenoy" <ego@linux.vnet.ibm.com> writes:
> This is the fifth version of the patches to track and expose idle PURR
> and SPURR ticks. These patches are required by tools such as lparstat
> to compute system utilization for capacity planning purposes.
>
> The previous versions can be found here:
> v4: https://lkml.org/lkml/2020/3/27/323
> v3: https://lkml.org/lkml/2020/3/11/331
> v2: https://lkml.org/lkml/2020/2/21/21
> v1: https://lore.kernel.org/patchwork/cover/1159341/
>
> They changes from v4 are:
>
> - As suggested by Naveen, moved the functions read_this_idle_purr()
> and read_this_idle_spurr() from Patch 2 and Patch 3 respectively
> to Patch 4 where it is invoked.
>
> - Dropped Patch 6 which cached the values of purr, spurr,
> idle_purr, idle_spurr in order to minimize the number of IPIs
> sent.
>
> - Updated the dates for the idle_purr, idle_spurr in the
> Documentation Patch 5.
LGTM
Acked-by: Nathan Lynch <nathanl@linux.ibm.com>
Thanks.
^ permalink raw reply
* Re: [PATCH 1/3] kexec: Prevent removal of memory in use by a loaded kexec image
From: David Hildenbrand @ 2020-04-21 13:29 UTC (permalink / raw)
To: Baoquan He, Andrew Morton
Cc: piliu, Anshuman Khandual, Catalin Marinas, Bhupesh Sharma,
linuxppc-dev, kexec, Russell King - ARM Linux admin, linux-mm,
James Morse, Eric W. Biederman, Will Deacon, linux-arm-kernel
In-Reply-To: <2525cc9c-3566-6275-105b-7f4af8f980bc@redhat.com>
>> ACPI SRAT is embeded into efi, need read out the rsdp pointer. If we don't
>> pass the efi, it won't get the SRAT table correctly, if I remember
>> correctly. Yeah, I remeber kvm guest can get memory hotplugged with
>> ACPI only, this won't happen on bare metal though. Need check carefully.
>> I have been using kvm guest with uefi firmwire recently.
>
> Yeah, I can imagine that bare metal is different. kvm only uses ACPI.
>
> I'm also asking because of virtio-mem. Memory added via virtio-mem is
> not part of any efi tables or whatsoever. So I assume the kexec kernel
> will not detect it automatically (good!), instead load the virtio-mem
> driver and let it add memory back to the system.
>
> I should probably play with kexec and virtio-mem once I have some spare
> cycles ... to find out what's broken and needs to be addressed :)
FWIW, I just gave virtio-mem and kexec/kdump a try.
a) kdump seems to work. Memory added by virtio-mem is getting dumped.
The kexec kernel only uses memory in the crash region. The virtio-mem
driver properly bails out due to is_kdump_kernel().
b) "kexec -s -l" seems to work fine. For now, the kernel does not seem
to get placed on virtio-mem memory (pure luck due to the left-to-right
search). Memory added by virtio-mem is not getting added to the e820
map. Once the virtio-mem driver comes back up in the kexec kernel, the
right memory is readded.
c) "kexec -c -l" does not work properly. All memory added by virtio-mem
is added to the e820 map, which is wrong. Memory that should not be
touched will be touched by the kexec kernel. I assume kexec-tools just
goes ahead and adds anything it can find in /proc/iomem (or
/sys/firmware/memmap/) to the e820 map of the new kernel.
Due to c), I assume all hotplugged memory (e.g., ACPI DIMMs) is
similarly added to the e820 map and, therefore, won't be able to be
onlined MOVABLE easily.
At least for virtio-mem, I would either have to
a) Not support "kexec -c -l". A viable option if we would be planning on
not supporting it either way in the long term. I could block this
in-kernel somehow eventually.
b) Teach kexec-tools to leave virtio-mem added memory alone. E.g., by
indicating it in /proc/iomem in a special way ("System RAM
(hotplugged)"/"System RAM (virtio-mem)").
Baoquan, any opinion on that?
--
Thanks,
David / dhildenb
^ permalink raw reply
* Re: [PATCH 1/2] powerpc: Add base support for ISA v3.1
From: Michael Ellerman @ 2020-04-21 12:39 UTC (permalink / raw)
To: Segher Boessenkool, Alistair Popple; +Cc: mikey, linuxppc-dev, npiggin
In-Reply-To: <20200403153208.GI26902@gate.crashing.org>
Segher Boessenkool <segher@kernel.crashing.org> writes:
> Hi!
>
> On Fri, Apr 03, 2020 at 03:10:54PM +1100, Alistair Popple wrote:
>> +#define PCR_ARCH_300 0x10 /* Architecture 3.00 */
>
> It's called 3.0, not 3.00?
It should actually be 3.0B shouldn't it?
cheers
^ permalink raw reply
* Re: crash in cpuidle_enter_state with 5.7-rc1
From: Michal Suchánek @ 2020-04-21 12:37 UTC (permalink / raw)
To: Michael Ellerman; +Cc: linuxppc-dev
In-Reply-To: <87sggxghb3.fsf@mpe.ellerman.id.au>
On Tue, Apr 21, 2020 at 10:21:52PM +1000, Michael Ellerman wrote:
> Michal Suchánek <msuchanek@suse.de> writes:
> > On Mon, Apr 20, 2020 at 08:50:30AM +0200, Michal Suchánek wrote:
> >> On Mon, Apr 20, 2020 at 04:15:39PM +1000, Michael Ellerman wrote:
> >> > Michal Suchánek <msuchanek@suse.de> writes:
> > ...
> >> >
> >> >
> >> > And I've just hit it with your config on a machine here, but the crash
> >> > is different:
> >> That does not look like it.
> >> You don't have this part in the stack trace:
> >> > [ 1.234899] [c000000007597420] [0000000000000000] 0x0
> >> > [ 1.234908] [c000000007597720] [0000000000000a6d] 0xa6d
> >> > [ 1.234919] [c000000007597a20] [0000000000000000] 0x0
> >> > [ 1.234931] [c000000007597d20] [0000000000000004] 0x4
> >> which is somewhat random but at least on such line is always present in
> >> the traces I get. Also I always get crash in cpuidle_enter_state
> > ..
> >> > I'm going to guess it's STRICT_KERNEL_RWX that's at fault.
> >> I can try without that as well.
> >
> > Can't reproduce without STRICT_KERNEL_RWX either.
>
> I've reproduced something similar all the way back to v5.5, though it
> seems harder to hit - sometimes 5 boots will succeed before one fails.
I only tried 3 times because I do not have automation in place to
capture these early crashes. I suppose I could tell the kernel to not
reboot on panic and try rebooting several times.
>
> Are you testing on top of PowerVM or KVM?
PowerVM.
Thanks
Michal
^ permalink raw reply
* Re: crash in cpuidle_enter_state with 5.7-rc1
From: Michael Ellerman @ 2020-04-21 12:21 UTC (permalink / raw)
To: Michal Suchánek; +Cc: linuxppc-dev
In-Reply-To: <20200420194132.GM25468@kitsune.suse.cz>
Michal Suchánek <msuchanek@suse.de> writes:
> On Mon, Apr 20, 2020 at 08:50:30AM +0200, Michal Suchánek wrote:
>> On Mon, Apr 20, 2020 at 04:15:39PM +1000, Michael Ellerman wrote:
>> > Michal Suchánek <msuchanek@suse.de> writes:
> ...
>> >
>> >
>> > And I've just hit it with your config on a machine here, but the crash
>> > is different:
>> That does not look like it.
>> You don't have this part in the stack trace:
>> > [ 1.234899] [c000000007597420] [0000000000000000] 0x0
>> > [ 1.234908] [c000000007597720] [0000000000000a6d] 0xa6d
>> > [ 1.234919] [c000000007597a20] [0000000000000000] 0x0
>> > [ 1.234931] [c000000007597d20] [0000000000000004] 0x4
>> which is somewhat random but at least on such line is always present in
>> the traces I get. Also I always get crash in cpuidle_enter_state
> ..
>> > I'm going to guess it's STRICT_KERNEL_RWX that's at fault.
>> I can try without that as well.
>
> Can't reproduce without STRICT_KERNEL_RWX either.
I've reproduced something similar all the way back to v5.5, though it
seems harder to hit - sometimes 5 boots will succeed before one fails.
Are you testing on top of PowerVM or KVM?
cheers
^ permalink raw reply
* Re: [PATCH 2/8] fs: extract simple_pin/release_fs to separate files
From: Emanuele Giuseppe Esposito @ 2020-04-21 11:26 UTC (permalink / raw)
To: Frederic Barrat, linux-nfs
Cc: Song Liu, linux-usb, bpf, Rafael J. Wysocki, David Airlie,
Heiko Carstens, Alexei Starovoitov, dri-devel, J. Bruce Fields,
Joseph Qi, Hugh Dickins, Paul Mackerras, John Johansen, netdev,
linux-s390, Christoph Hellwig, Andrew Donnellan, Matthew Garrett,
linux-efi, Arnd Bergmann, Daniel Borkmann, Christian Borntraeger,
linux-rdma, Mark Fasheh, Anton Vorontsov, John Fastabend,
James Morris, Ard Biesheuvel, Jason Gunthorpe, Doug Ledford,
oprofile-list, Yonghong Song, Ian Kent, Andrii Nakryiko,
Alexey Dobriyan, Serge E. Hallyn, Robert Richter,
Thomas Zimmermann, Vasily Gorbik, Tony Luck, Kees Cook,
James E.J. Bottomley, autofs, Maarten Lankhorst, Uma Krishnan,
Maxime Ripard, Manoj N. Kumar, Alexander Viro, Jakub Kicinski,
KP Singh, Trond Myklebust, Matthew R. Ochs, David S. Miller,
Felipe Balbi, Mike Marciniszyn, Iurii Zaikin, linux-scsi,
Martin K. Petersen, linux-mm, Greg Kroah-Hartman,
Dennis Dalessandro, Miklos Szeredi, linux-security-module,
linux-kernel, Anna Schumaker, Luis Chamberlain, Chuck Lever,
Jeremy Kerr, Daniel Vetter, Colin Cross, linux-fsdevel,
Paolo Bonzini, Andrew Morton, Mike Kravetz, linuxppc-dev,
Martin KaFai Lau, ocfs2-devel, Joel Becker
In-Reply-To: <7abfdd3b-dbbe-52ab-df53-95e9f86d39cd@linux.ibm.com>
On 4/21/20 1:19 PM, Frederic Barrat wrote:
>
>
>> diff --git a/drivers/misc/cxl/Kconfig b/drivers/misc/cxl/Kconfig
>> index 39eec9031487..a62795079d9c 100644
>> --- a/drivers/misc/cxl/Kconfig
>> +++ b/drivers/misc/cxl/Kconfig
>> @@ -19,6 +19,7 @@ config CXL
>> select CXL_BASE
>> select CXL_AFU_DRIVER_OPS
>> select CXL_LIB
>> + select SIMPLEFS
>> default m
>> help
>> Select this option to enable driver support for IBM Coherent
>> diff --git a/drivers/misc/cxl/api.c b/drivers/misc/cxl/api.c
>> index b493de962153..0b8f8de7475a 100644
>> --- a/drivers/misc/cxl/api.c
>> +++ b/drivers/misc/cxl/api.c
>> @@ -9,6 +9,7 @@
>> #include <misc/cxl.h>
>> #include <linux/module.h>
>> #include <linux/mount.h>
>> +#include <linux/simplefs.h>
>> #include <linux/pseudo_fs.h>
>> #include <linux/sched/mm.h>
>> #include <linux/mmu_context.h>
>> diff --git a/drivers/misc/ocxl/Kconfig b/drivers/misc/ocxl/Kconfig
>> index 2d2266c1439e..ddd9245fff3d 100644
>> --- a/drivers/misc/ocxl/Kconfig
>> +++ b/drivers/misc/ocxl/Kconfig
>> @@ -12,6 +12,7 @@ config OCXL
>> depends on PPC_POWERNV && PCI && EEH
>> select OCXL_BASE
>> select HOTPLUG_PCI_POWERNV
>> + select SIMPLEFS
>
>
> It's not clear to me the Kconfig updated is needed for the ocxl driver.
> I think it's only needed for the cxl driver.
I am going to get rid of the separate simplefs.c file and related
Kconfig entry and put everything in fs/libfs.c, so this file (together
with many others touched in this patch) won't be modified in v2.
Thanks,
Emanuele
^ permalink raw reply
* Re: [PATCH 2/8] fs: extract simple_pin/release_fs to separate files
From: Frederic Barrat @ 2020-04-21 11:19 UTC (permalink / raw)
To: Emanuele Giuseppe Esposito, linux-nfs
Cc: Song Liu, linux-usb, bpf, Rafael J. Wysocki, David Airlie,
Heiko Carstens, Alexei Starovoitov, dri-devel, J. Bruce Fields,
Joseph Qi, Hugh Dickins, Paul Mackerras, John Johansen, netdev,
linux-s390, Christoph Hellwig, Andrew Donnellan, Matthew Garrett,
linux-efi, Arnd Bergmann, Daniel Borkmann, Christian Borntraeger,
linux-rdma, Mark Fasheh, Anton Vorontsov, John Fastabend,
James Morris, Ard Biesheuvel, Jason Gunthorpe, Doug Ledford,
oprofile-list, Yonghong Song, Ian Kent, Andrii Nakryiko,
Alexey Dobriyan, Serge E. Hallyn, Robert Richter,
Thomas Zimmermann, Vasily Gorbik, Tony Luck, Kees Cook,
James E.J. Bottomley, autofs, Maarten Lankhorst, Uma Krishnan,
Maxime Ripard, Manoj N. Kumar, Alexander Viro, Jakub Kicinski,
KP Singh, Trond Myklebust, Matthew R. Ochs, David S. Miller,
Felipe Balbi, Mike Marciniszyn, Iurii Zaikin, linux-scsi,
Martin K. Petersen, linux-mm, Greg Kroah-Hartman,
Dennis Dalessandro, Miklos Szeredi, linux-security-module,
linux-kernel, Anna Schumaker, Luis Chamberlain, Chuck Lever,
Jeremy Kerr, Daniel Vetter, Colin Cross, linux-fsdevel,
Paolo Bonzini, Andrew Morton, Mike Kravetz, linuxppc-dev,
Martin KaFai Lau, ocfs2-devel, Joel Becker
In-Reply-To: <20200414124304.4470-3-eesposit@redhat.com>
> diff --git a/drivers/misc/cxl/Kconfig b/drivers/misc/cxl/Kconfig
> index 39eec9031487..a62795079d9c 100644
> --- a/drivers/misc/cxl/Kconfig
> +++ b/drivers/misc/cxl/Kconfig
> @@ -19,6 +19,7 @@ config CXL
> select CXL_BASE
> select CXL_AFU_DRIVER_OPS
> select CXL_LIB
> + select SIMPLEFS
> default m
> help
> Select this option to enable driver support for IBM Coherent
> diff --git a/drivers/misc/cxl/api.c b/drivers/misc/cxl/api.c
> index b493de962153..0b8f8de7475a 100644
> --- a/drivers/misc/cxl/api.c
> +++ b/drivers/misc/cxl/api.c
> @@ -9,6 +9,7 @@
> #include <misc/cxl.h>
> #include <linux/module.h>
> #include <linux/mount.h>
> +#include <linux/simplefs.h>
> #include <linux/pseudo_fs.h>
> #include <linux/sched/mm.h>
> #include <linux/mmu_context.h>
> diff --git a/drivers/misc/ocxl/Kconfig b/drivers/misc/ocxl/Kconfig
> index 2d2266c1439e..ddd9245fff3d 100644
> --- a/drivers/misc/ocxl/Kconfig
> +++ b/drivers/misc/ocxl/Kconfig
> @@ -12,6 +12,7 @@ config OCXL
> depends on PPC_POWERNV && PCI && EEH
> select OCXL_BASE
> select HOTPLUG_PCI_POWERNV
> + select SIMPLEFS
It's not clear to me the Kconfig updated is needed for the ocxl driver.
I think it's only needed for the cxl driver.
Fred
^ permalink raw reply
* Re: [PATCH AUTOSEL 5.5 71/75] powerpc/powernv/ioda: Fix ref count for devices with their own PE
From: Frederic Barrat @ 2020-04-21 11:07 UTC (permalink / raw)
To: Sasha Levin, linux-kernel, stable; +Cc: linuxppc-dev, Andrew Donnellan
In-Reply-To: <20200418140910.8280-71-sashal@kernel.org>
Le 18/04/2020 à 16:09, Sasha Levin a écrit :
> From: Frederic Barrat <fbarrat@linux.ibm.com>
>
> [ Upstream commit 05dd7da76986937fb288b4213b1fa10dbe0d1b33 ]
Like for other stable kernels, patches 71, 72, 73 of this series are not
desirable for stable.
Fred
> The pci_dn structure used to store a pointer to the struct pci_dev, so
> taking a reference on the device was required. However, the pci_dev
> pointer was later removed from the pci_dn structure, but the reference
> was kept for the npu device.
> See commit 902bdc57451c ("powerpc/powernv/idoa: Remove unnecessary
> pcidev from pci_dn").
>
> We don't need to take a reference on the device when assigning the PE
> as the struct pnv_ioda_pe is cleaned up at the same time as
> the (physical) device is released. Doing so prevents the device from
> being released, which is a problem for opencapi devices, since we want
> to be able to remove them through PCI hotplug.
>
> Now the ugly part: nvlink npu devices are not meant to be
> released. Because of the above, we've always leaked a reference and
> simply removing it now is dangerous and would likely require more
> work. There's currently no release device callback for nvlink devices
> for example. So to be safe, this patch leaks a reference on the npu
> device, but only for nvlink and not opencapi.
>
> Signed-off-by: Frederic Barrat <fbarrat@linux.ibm.com>
> Reviewed-by: Andrew Donnellan <ajd@linux.ibm.com>
> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
> Link: https://lore.kernel.org/r/20191121134918.7155-2-fbarrat@linux.ibm.com
> Signed-off-by: Sasha Levin <sashal@kernel.org>
> ---
> arch/powerpc/platforms/powernv/pci-ioda.c | 19 ++++++++++++-------
> 1 file changed, 12 insertions(+), 7 deletions(-)
>
> diff --git a/arch/powerpc/platforms/powernv/pci-ioda.c b/arch/powerpc/platforms/powernv/pci-ioda.c
> index 67e4628dd5274..b4afabe20744a 100644
> --- a/arch/powerpc/platforms/powernv/pci-ioda.c
> +++ b/arch/powerpc/platforms/powernv/pci-ioda.c
> @@ -1062,14 +1062,13 @@ static struct pnv_ioda_pe *pnv_ioda_setup_dev_PE(struct pci_dev *dev)
> return NULL;
> }
>
> - /* NOTE: We get only one ref to the pci_dev for the pdn, not for the
> - * pointer in the PE data structure, both should be destroyed at the
> - * same time. However, this needs to be looked at more closely again
> - * once we actually start removing things (Hotplug, SR-IOV, ...)
> + /* NOTE: We don't get a reference for the pointer in the PE
> + * data structure, both the device and PE structures should be
> + * destroyed at the same time. However, removing nvlink
> + * devices will need some work.
> *
> * At some point we want to remove the PDN completely anyways
> */
> - pci_dev_get(dev);
> pdn->pe_number = pe->pe_number;
> pe->flags = PNV_IODA_PE_DEV;
> pe->pdev = dev;
> @@ -1084,7 +1083,6 @@ static struct pnv_ioda_pe *pnv_ioda_setup_dev_PE(struct pci_dev *dev)
> pnv_ioda_free_pe(pe);
> pdn->pe_number = IODA_INVALID_PE;
> pe->pdev = NULL;
> - pci_dev_put(dev);
> return NULL;
> }
>
> @@ -1205,6 +1203,14 @@ static struct pnv_ioda_pe *pnv_ioda_setup_npu_PE(struct pci_dev *npu_pdev)
> struct pci_controller *hose = pci_bus_to_host(npu_pdev->bus);
> struct pnv_phb *phb = hose->private_data;
>
> + /*
> + * Intentionally leak a reference on the npu device (for
> + * nvlink only; this is not an opencapi path) to make sure it
> + * never goes away, as it's been the case all along and some
> + * work is needed otherwise.
> + */
> + pci_dev_get(npu_pdev);
> +
> /*
> * Due to a hardware errata PE#0 on the NPU is reserved for
> * error handling. This means we only have three PEs remaining
> @@ -1228,7 +1234,6 @@ static struct pnv_ioda_pe *pnv_ioda_setup_npu_PE(struct pci_dev *npu_pdev)
> */
> dev_info(&npu_pdev->dev,
> "Associating to existing PE %x\n", pe_num);
> - pci_dev_get(npu_pdev);
> npu_pdn = pci_get_pdn(npu_pdev);
> rid = npu_pdev->bus->number << 8 | npu_pdn->devfn;
> npu_pdn->pe_number = pe_num;
>
^ permalink raw reply
* Re: [PATCH AUTOSEL 4.19 41/47] powerpc/powernv/ioda: Fix ref count for devices with their own PE
From: Frederic Barrat @ 2020-04-21 11:06 UTC (permalink / raw)
To: Sasha Levin, linux-kernel, stable; +Cc: linuxppc-dev, Andrew Donnellan
In-Reply-To: <20200418144227.9802-41-sashal@kernel.org>
Le 18/04/2020 à 16:42, Sasha Levin a écrit :
> From: Frederic Barrat <fbarrat@linux.ibm.com>
>
> [ Upstream commit 05dd7da76986937fb288b4213b1fa10dbe0d1b33 ]
Like for kernel 5.4, the patches 41, 42, 43 of this series are not
desirable for stable.
Fred
> The pci_dn structure used to store a pointer to the struct pci_dev, so
> taking a reference on the device was required. However, the pci_dev
> pointer was later removed from the pci_dn structure, but the reference
> was kept for the npu device.
> See commit 902bdc57451c ("powerpc/powernv/idoa: Remove unnecessary
> pcidev from pci_dn").
>
> We don't need to take a reference on the device when assigning the PE
> as the struct pnv_ioda_pe is cleaned up at the same time as
> the (physical) device is released. Doing so prevents the device from
> being released, which is a problem for opencapi devices, since we want
> to be able to remove them through PCI hotplug.
>
> Now the ugly part: nvlink npu devices are not meant to be
> released. Because of the above, we've always leaked a reference and
> simply removing it now is dangerous and would likely require more
> work. There's currently no release device callback for nvlink devices
> for example. So to be safe, this patch leaks a reference on the npu
> device, but only for nvlink and not opencapi.
>
> Signed-off-by: Frederic Barrat <fbarrat@linux.ibm.com>
> Reviewed-by: Andrew Donnellan <ajd@linux.ibm.com>
> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
> Link: https://lore.kernel.org/r/20191121134918.7155-2-fbarrat@linux.ibm.com
> Signed-off-by: Sasha Levin <sashal@kernel.org>
> ---
> arch/powerpc/platforms/powernv/pci-ioda.c | 19 ++++++++++++-------
> 1 file changed, 12 insertions(+), 7 deletions(-)
>
> diff --git a/arch/powerpc/platforms/powernv/pci-ioda.c b/arch/powerpc/platforms/powernv/pci-ioda.c
> index ecd211c5f24a5..19cd6affdd5fb 100644
> --- a/arch/powerpc/platforms/powernv/pci-ioda.c
> +++ b/arch/powerpc/platforms/powernv/pci-ioda.c
> @@ -1071,14 +1071,13 @@ static struct pnv_ioda_pe *pnv_ioda_setup_dev_PE(struct pci_dev *dev)
> return NULL;
> }
>
> - /* NOTE: We get only one ref to the pci_dev for the pdn, not for the
> - * pointer in the PE data structure, both should be destroyed at the
> - * same time. However, this needs to be looked at more closely again
> - * once we actually start removing things (Hotplug, SR-IOV, ...)
> + /* NOTE: We don't get a reference for the pointer in the PE
> + * data structure, both the device and PE structures should be
> + * destroyed at the same time. However, removing nvlink
> + * devices will need some work.
> *
> * At some point we want to remove the PDN completely anyways
> */
> - pci_dev_get(dev);
> pdn->pe_number = pe->pe_number;
> pe->flags = PNV_IODA_PE_DEV;
> pe->pdev = dev;
> @@ -1093,7 +1092,6 @@ static struct pnv_ioda_pe *pnv_ioda_setup_dev_PE(struct pci_dev *dev)
> pnv_ioda_free_pe(pe);
> pdn->pe_number = IODA_INVALID_PE;
> pe->pdev = NULL;
> - pci_dev_put(dev);
> return NULL;
> }
>
> @@ -1213,6 +1211,14 @@ static struct pnv_ioda_pe *pnv_ioda_setup_npu_PE(struct pci_dev *npu_pdev)
> struct pci_controller *hose = pci_bus_to_host(npu_pdev->bus);
> struct pnv_phb *phb = hose->private_data;
>
> + /*
> + * Intentionally leak a reference on the npu device (for
> + * nvlink only; this is not an opencapi path) to make sure it
> + * never goes away, as it's been the case all along and some
> + * work is needed otherwise.
> + */
> + pci_dev_get(npu_pdev);
> +
> /*
> * Due to a hardware errata PE#0 on the NPU is reserved for
> * error handling. This means we only have three PEs remaining
> @@ -1236,7 +1242,6 @@ static struct pnv_ioda_pe *pnv_ioda_setup_npu_PE(struct pci_dev *npu_pdev)
> */
> dev_info(&npu_pdev->dev,
> "Associating to existing PE %x\n", pe_num);
> - pci_dev_get(npu_pdev);
> npu_pdn = pci_get_pdn(npu_pdev);
> rid = npu_pdev->bus->number << 8 | npu_pdn->devfn;
> npu_pdn->pe_number = pe_num;
>
^ permalink raw reply
* Re: [PATCH AUTOSEL 5.4 71/78] ocxl: Add PCI hotplug dependency to Kconfig
From: Frederic Barrat @ 2020-04-21 11:05 UTC (permalink / raw)
To: Sasha Levin, linux-kernel, stable
Cc: linuxppc-dev, Andrew Donnellan, Alastair D'Silva
In-Reply-To: <20200418144047.9013-71-sashal@kernel.org>
Le 18/04/2020 à 16:40, Sasha Levin a écrit :
> From: Frederic Barrat <fbarrat@linux.ibm.com>
>
> [ Upstream commit 49ce94b8677c7d7a15c4d7cbbb9ff1cd8387827b ]
This shouldn't be backported to stable.
Fred
> The PCI hotplug framework is used to update the devices when a new
> image is written to the FPGA.
>
> Reviewed-by: Alastair D'Silva <alastair@d-silva.org>
> Reviewed-by: Andrew Donnellan <ajd@linux.ibm.com>
> Signed-off-by: Frederic Barrat <fbarrat@linux.ibm.com>
> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
> Link: https://lore.kernel.org/r/20191121134918.7155-12-fbarrat@linux.ibm.com
> Signed-off-by: Sasha Levin <sashal@kernel.org>
> ---
> drivers/misc/ocxl/Kconfig | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/misc/ocxl/Kconfig b/drivers/misc/ocxl/Kconfig
> index 1916fa65f2f2a..2d2266c1439ef 100644
> --- a/drivers/misc/ocxl/Kconfig
> +++ b/drivers/misc/ocxl/Kconfig
> @@ -11,6 +11,7 @@ config OCXL
> tristate "OpenCAPI coherent accelerator support"
> depends on PPC_POWERNV && PCI && EEH
> select OCXL_BASE
> + select HOTPLUG_PCI_POWERNV
> default m
> help
> Select this option to enable the ocxl driver for Open
>
^ permalink raw reply
* Re: [PATCH AUTOSEL 5.4 70/78] pci/hotplug/pnv-php: Remove erroneous warning
From: Frederic Barrat @ 2020-04-21 11:03 UTC (permalink / raw)
To: Sasha Levin, linux-kernel, stable
Cc: linux-pci, linuxppc-dev, Andrew Donnellan, Alastair D'Silva
In-Reply-To: <20200418144047.9013-70-sashal@kernel.org>
Le 18/04/2020 à 16:40, Sasha Levin a écrit :
> From: Frederic Barrat <fbarrat@linux.ibm.com>
>
> [ Upstream commit 658ab186dd22060408d94f5c5a6d02d809baba44 ]
This doesn't need to be backported to stable.
Fred
> On powernv, when removing a device through hotplug, the following
> warning is logged:
>
> Invalid refcount <.> on <...>
>
> It may be incorrect, the refcount may be set to a higher value than 1
> and be valid. of_detach_node() can drop more than one reference. As it
> doesn't seem trivial to assert the correct value, let's remove the
> warning.
>
> Reviewed-by: Alastair D'Silva <alastair@d-silva.org>
> Reviewed-by: Andrew Donnellan <ajd@linux.ibm.com>
> Signed-off-by: Frederic Barrat <fbarrat@linux.ibm.com>
> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
> Link: https://lore.kernel.org/r/20191121134918.7155-7-fbarrat@linux.ibm.com
> Signed-off-by: Sasha Levin <sashal@kernel.org>
> ---
> drivers/pci/hotplug/pnv_php.c | 6 ------
> 1 file changed, 6 deletions(-)
>
> diff --git a/drivers/pci/hotplug/pnv_php.c b/drivers/pci/hotplug/pnv_php.c
> index d7b2b47bc33eb..6037983c6e46b 100644
> --- a/drivers/pci/hotplug/pnv_php.c
> +++ b/drivers/pci/hotplug/pnv_php.c
> @@ -151,17 +151,11 @@ static void pnv_php_rmv_pdns(struct device_node *dn)
> static void pnv_php_detach_device_nodes(struct device_node *parent)
> {
> struct device_node *dn;
> - int refcount;
>
> for_each_child_of_node(parent, dn) {
> pnv_php_detach_device_nodes(dn);
>
> of_node_put(dn);
> - refcount = kref_read(&dn->kobj.kref);
> - if (refcount != 1)
> - pr_warn("Invalid refcount %d on <%pOF>\n",
> - refcount, dn);
> -
> of_detach_node(dn);
> }
> }
>
^ permalink raw reply
* Re: [PATCH AUTOSEL 5.4 69/78] powerpc/powernv/ioda: Fix ref count for devices with their own PE
From: Frederic Barrat @ 2020-04-21 11:02 UTC (permalink / raw)
To: Sasha Levin, linux-kernel, stable; +Cc: linuxppc-dev, Andrew Donnellan
In-Reply-To: <20200418144047.9013-69-sashal@kernel.org>
Le 18/04/2020 à 16:40, Sasha Levin a écrit :
> From: Frederic Barrat <fbarrat@linux.ibm.com>
>
> [ Upstream commit 05dd7da76986937fb288b4213b1fa10dbe0d1b33 ]
This shouldn't be backported to stable.
Fred
> The pci_dn structure used to store a pointer to the struct pci_dev, so
> taking a reference on the device was required. However, the pci_dev
> pointer was later removed from the pci_dn structure, but the reference
> was kept for the npu device.
> See commit 902bdc57451c ("powerpc/powernv/idoa: Remove unnecessary
> pcidev from pci_dn").
>
> We don't need to take a reference on the device when assigning the PE
> as the struct pnv_ioda_pe is cleaned up at the same time as
> the (physical) device is released. Doing so prevents the device from
> being released, which is a problem for opencapi devices, since we want
> to be able to remove them through PCI hotplug.
>
> Now the ugly part: nvlink npu devices are not meant to be
> released. Because of the above, we've always leaked a reference and
> simply removing it now is dangerous and would likely require more
> work. There's currently no release device callback for nvlink devices
> for example. So to be safe, this patch leaks a reference on the npu
> device, but only for nvlink and not opencapi.
>
> Signed-off-by: Frederic Barrat <fbarrat@linux.ibm.com>
> Reviewed-by: Andrew Donnellan <ajd@linux.ibm.com>
> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
> Link: https://lore.kernel.org/r/20191121134918.7155-2-fbarrat@linux.ibm.com
> Signed-off-by: Sasha Levin <sashal@kernel.org>
> ---
> arch/powerpc/platforms/powernv/pci-ioda.c | 19 ++++++++++++-------
> 1 file changed, 12 insertions(+), 7 deletions(-)
>
> diff --git a/arch/powerpc/platforms/powernv/pci-ioda.c b/arch/powerpc/platforms/powernv/pci-ioda.c
> index 058223233088e..e9cda7e316a50 100644
> --- a/arch/powerpc/platforms/powernv/pci-ioda.c
> +++ b/arch/powerpc/platforms/powernv/pci-ioda.c
> @@ -1062,14 +1062,13 @@ static struct pnv_ioda_pe *pnv_ioda_setup_dev_PE(struct pci_dev *dev)
> return NULL;
> }
>
> - /* NOTE: We get only one ref to the pci_dev for the pdn, not for the
> - * pointer in the PE data structure, both should be destroyed at the
> - * same time. However, this needs to be looked at more closely again
> - * once we actually start removing things (Hotplug, SR-IOV, ...)
> + /* NOTE: We don't get a reference for the pointer in the PE
> + * data structure, both the device and PE structures should be
> + * destroyed at the same time. However, removing nvlink
> + * devices will need some work.
> *
> * At some point we want to remove the PDN completely anyways
> */
> - pci_dev_get(dev);
> pdn->pe_number = pe->pe_number;
> pe->flags = PNV_IODA_PE_DEV;
> pe->pdev = dev;
> @@ -1084,7 +1083,6 @@ static struct pnv_ioda_pe *pnv_ioda_setup_dev_PE(struct pci_dev *dev)
> pnv_ioda_free_pe(pe);
> pdn->pe_number = IODA_INVALID_PE;
> pe->pdev = NULL;
> - pci_dev_put(dev);
> return NULL;
> }
>
> @@ -1205,6 +1203,14 @@ static struct pnv_ioda_pe *pnv_ioda_setup_npu_PE(struct pci_dev *npu_pdev)
> struct pci_controller *hose = pci_bus_to_host(npu_pdev->bus);
> struct pnv_phb *phb = hose->private_data;
>
> + /*
> + * Intentionally leak a reference on the npu device (for
> + * nvlink only; this is not an opencapi path) to make sure it
> + * never goes away, as it's been the case all along and some
> + * work is needed otherwise.
> + */
> + pci_dev_get(npu_pdev);
> +
> /*
> * Due to a hardware errata PE#0 on the NPU is reserved for
> * error handling. This means we only have three PEs remaining
> @@ -1228,7 +1234,6 @@ static struct pnv_ioda_pe *pnv_ioda_setup_npu_PE(struct pci_dev *npu_pdev)
> */
> dev_info(&npu_pdev->dev,
> "Associating to existing PE %x\n", pe_num);
> - pci_dev_get(npu_pdev);
> npu_pdn = pci_get_pdn(npu_pdev);
> rid = npu_pdev->bus->number << 8 | npu_pdn->devfn;
> npu_pdn->pe_number = pe_num;
>
^ permalink raw reply
* Re: [PATCH] ASoC: fsl_esai: Remove expensive print in irq handler
From: Shengjiu Wang @ 2020-04-21 10:51 UTC (permalink / raw)
To: Mark Brown
Cc: Linux-ALSA, Timur Tabi, Xiubo Li, Fabio Estevam, Shengjiu Wang,
Takashi Iwai, linux-kernel, Nicolin Chen, linuxppc-dev
In-Reply-To: <20200421095139.GA4540@sirena.org.uk>
Hi
On Tue, Apr 21, 2020 at 5:53 PM Mark Brown <broonie@kernel.org> wrote:
>
> On Tue, Apr 21, 2020 at 04:41:23PM +0800, Shengjiu Wang wrote:
> > Use dev_dbg instead of dev_err in irq handler, the dev_err
> > is expensive, we don't need the message to be printed everytime,
> > which is almost a debug option.
>
> > if (esr & ESAI_ESR_RFF_MASK)
> > - dev_warn(&pdev->dev, "isr: Receiving overrun\n");
> > + dev_dbg(&pdev->dev, "isr: Receiving overrun\n");
> >
> > if (esr & ESAI_ESR_TFE_MASK)
> > - dev_warn(&pdev->dev, "isr: Transmission underrun\n");
> > + dev_dbg(&pdev->dev, "isr: Transmission underrun\n");
>
> These are error messages which would suggest a problem that'd lead to
> data corruption, it seems bad not to try to flag that to the user -
> surely we've got bigger problems than performance if this happens?
> Perhaps convert to a ratelimited print if the issue is that when errors
> happen they happen a lot?
Thanks for review, I will rethink about this.
best regards
wang shengjiu
^ permalink raw reply
* Re: [PATCH v2,RESEND] misc: new driver sram_uapi for user level SRAM access
From: 王文虎 @ 2020-04-21 10:03 UTC (permalink / raw)
To: Greg KH
Cc: robh, arnd, Randy Dunlap, linux-kernel, Scott Wood, kernel,
linuxppc-dev
In-Reply-To: <20200421093427.GC725219@kroah.com>
>On Tue, Apr 21, 2020 at 05:09:47PM +0800, 王文虎 wrote:
>> Hi, Greg, Arnd,
>>
>> Thank you for your comments first, and then really very very very sorry
>> for driving Greg to sigh and I hope there would be chance to share Moutai
>> (rather than whisky, we drink it much, a kind of Baijiu), after the virus.
>>
>> Back to the comments, I'd like to do a bit of documentation or explanation first,
>> which should have been done early or else there would not be so much to explain:
>> 1. What I have been trying to do is to access the Freescale Cache-SRAM device form
>> user level;
>> 2. I implemented it using UIO, which was thought of non-proper;
>
>I still think that using uio is the best way to do this, and never said
>it was "non-proper". All we got bogged down in was the DT
>representation of stuff from what I remember. That should be worked
>through.
>
>thanks,
>
>greg k-h
Surely, but so how would things go? Scott said not fit for him. And he was
gonna to write a new patch(Oh, that is what I have been doing.....,and I really
donot think he need to do it)
This is the final version using UIO, and even Christophe had Reviewed-by,
https://lore.kernel.org/patchwork/patch/1226225/
If no ending reaches, I have to make a step forward to keep working with
the misc device version.
Thanks, and regards,
Wenhu
^ permalink raw reply
* Re: [musl] Powerpc Linux 'scv' system call ABI proposal take 2
From: Florian Weimer @ 2020-04-21 9:57 UTC (permalink / raw)
To: Nicholas Piggin
Cc: libc-dev, Rich Felker, Nicholas Piggin via Libc-alpha,
linuxppc-dev, musl
In-Reply-To: <20200420211751.GF23945@port70.net>
* Szabolcs Nagy:
> * Nicholas Piggin <npiggin@gmail.com> [2020-04-20 12:08:36 +1000]:
>> Excerpts from Rich Felker's message of April 20, 2020 11:29 am:
>> > Also, allowing patching of executable pages is generally frowned upon
>> > these days because W^X is a desirable hardening property.
>>
>> Right, it would want be write-protected after being patched.
>
> "frowned upon" means that users may have to update
> their security policy setting in pax, selinux, apparmor,
> seccomp bpf filters and who knows what else that may
> monitor and flag W&X mprotect.
>
> libc update can break systems if the new libc does W&X.
It's possible to map over pre-compiled alternative implementations,
though. Basically, we would do the patching and build time and store
the results in the file.
It works best if the variance is concentrated on a few pages, and
there are very few alternatives. For example, having two syscall APIs
and supporting threading and no-threading versions would need four
code versions in total, which is likely excessive.
^ permalink raw reply
* Re: [PATCH] ASoC: fsl_esai: Remove expensive print in irq handler
From: Mark Brown @ 2020-04-21 9:51 UTC (permalink / raw)
To: Shengjiu Wang
Cc: alsa-devel, timur, Xiubo.Lee, linuxppc-dev, tiwai, perex,
nicoleotsuka, festevam, linux-kernel
In-Reply-To: <1587458483-2166-1-git-send-email-shengjiu.wang@nxp.com>
[-- Attachment #1: Type: text/plain, Size: 844 bytes --]
On Tue, Apr 21, 2020 at 04:41:23PM +0800, Shengjiu Wang wrote:
> Use dev_dbg instead of dev_err in irq handler, the dev_err
> is expensive, we don't need the message to be printed everytime,
> which is almost a debug option.
> if (esr & ESAI_ESR_RFF_MASK)
> - dev_warn(&pdev->dev, "isr: Receiving overrun\n");
> + dev_dbg(&pdev->dev, "isr: Receiving overrun\n");
>
> if (esr & ESAI_ESR_TFE_MASK)
> - dev_warn(&pdev->dev, "isr: Transmission underrun\n");
> + dev_dbg(&pdev->dev, "isr: Transmission underrun\n");
These are error messages which would suggest a problem that'd lead to
data corruption, it seems bad not to try to flag that to the user -
surely we've got bigger problems than performance if this happens?
Perhaps convert to a ratelimited print if the issue is that when errors
happen they happen a lot?
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* Re: [PATCH 03/21] mm: remove CONFIG_HAVE_MEMBLOCK_NODE_MAP option
From: Baoquan He @ 2020-04-21 9:45 UTC (permalink / raw)
To: Mike Rapoport
Cc: Rich Felker, linux-ia64, linux-doc, Catalin Marinas,
Heiko Carstens, Michal Hocko, James E.J. Bottomley, Max Filippov,
Guo Ren, linux-csky, linux-parisc, sparclinux, linux-hexagon,
linux-riscv, Greg Ungerer, linux-arch, linux-s390, linux-snps-arc,
linux-c6x-dev, Brian Cain, Jonathan Corbet, linux-sh,
Helge Deller, x86, Russell King, Ley Foon Tan, Mike Rapoport,
Geert Uytterhoeven, linux-arm-kernel, Mark Salter, Matt Turner,
linux-mips, uclinux-h8-devel, linux-xtensa, linux-alpha, linux-um,
linux-m68k, Tony Luck, Greentime Hu, Paul Walmsley,
Stafford Horne, Guan Xuetao, Hoan Tran, Michal Simek,
Thomas Bogendoerfer, Yoshinori Sato, Nick Hu, linux-mm,
Vineet Gupta, linux-kernel, openrisc, Richard Weinberger,
Andrew Morton, linuxppc-dev, David S. Miller
In-Reply-To: <20200421090908.GC14260@kernel.org>
On 04/21/20 at 12:09pm, Mike Rapoport wrote:
> > > diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> > > index fc0aad0bc1f5..e67dc501576a 100644
> > > --- a/mm/memory_hotplug.c
> > > +++ b/mm/memory_hotplug.c
> > > @@ -1372,11 +1372,7 @@ check_pages_isolated_cb(unsigned long start_pfn, unsigned long nr_pages,
> > >
> > > static int __init cmdline_parse_movable_node(char *p)
> > > {
> > > -#ifdef CONFIG_HAVE_MEMBLOCK_NODE_MAP
> > > movable_node_enabled = true;
> > > -#else
> > > - pr_warn("movable_node parameter depends on CONFIG_HAVE_MEMBLOCK_NODE_MAP to work properly\n");
> > > -#endif
> >
> > Wondering if this change will impact anything. Before, those ARCHes with
> > CONFIG_HAVE_MEMBLOCK_NODE_MAP support movable_node. With this patch
> > applied, those ARCHes which don't support CONFIG_HAVE_MEMBLOCK_NODE_MAP
> > can also have 'movable_node' specified in kernel cmdline.
> >
> > > return 0;
> > > }
> > > early_param("movable_node", cmdline_parse_movable_node);
> > > diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> > > index 1ac775bfc9cf..4530e9cfd9f7 100644
> > > --- a/mm/page_alloc.c
> > > +++ b/mm/page_alloc.c
> > > @@ -335,7 +335,6 @@ static unsigned long nr_kernel_pages __initdata;
> > > static unsigned long nr_all_pages __initdata;
> > > static unsigned long dma_reserve __initdata;
> > >
> > > -#ifdef CONFIG_HAVE_MEMBLOCK_NODE_MAP
> > > static unsigned long arch_zone_lowest_possible_pfn[MAX_NR_ZONES] __initdata;
> > > static unsigned long arch_zone_highest_possible_pfn[MAX_NR_ZONES] __initdata;
> > > static unsigned long required_kernelcore __initdata;
> >
> > Does it mean those ARCHes which don't support
> > CONFIG_HAVE_MEMBLOCK_NODE_MAP before, will have 'kernelcore=' and
> > 'movablecore=' now, and will have MOVABLE zone?
>
> I hesitated a lot about whether to hide the kernelcore/movablecore and
> related code behind an #ifdef.
> In the end I've decided to keep the code compiled unconditionally as it
> is anyway __init and no sane person would pass "kernelcore=" to the
> kernel on a UMA system.
I see. Then maybe can do something if someone complains about it
in the future, e.g warn out with a message in
cmdline_parse_movable_node(), cmdline_parse_kernelcore().
>
> > > @@ -348,7 +347,6 @@ static bool mirrored_kernelcore __meminitdata;
> > > /* movable_zone is the "real" zone pages in ZONE_MOVABLE are taken from */
> > > int movable_zone;
> > > EXPORT_SYMBOL(movable_zone);
> > > -#endif /* CONFIG_HAVE_MEMBLOCK_NODE_MAP */
> > >
> > > #if MAX_NUMNODES > 1
> > > unsigned int nr_node_ids __read_mostly = MAX_NUMNODES;
> > > @@ -1499,8 +1497,7 @@ void __free_pages_core(struct page *page, unsigned int order)
> > > __free_pages(page, order);
> > > }
> > >
> > > -#if defined(CONFIG_HAVE_ARCH_EARLY_PFN_TO_NID) || \
> > > - defined(CONFIG_HAVE_MEMBLOCK_NODE_MAP)
> > > +#ifdef CONFIG_NEED_MULTIPLE_NODES
> > >
> > > static struct mminit_pfnnid_cache early_pfnnid_cache __meminitdata;
> > >
> > > @@ -1542,7 +1539,7 @@ int __meminit early_pfn_to_nid(unsigned long pfn)
> > >
> > > return nid;
> > > }
> > > -#endif
> > > +#endif /* CONFIG_NEED_MULTIPLE_NODES */
> > >
> > > #ifdef CONFIG_NODES_SPAN_OTHER_NODES
> > > /* Only safe to use early in boot when initialisation is single-threaded */
> > > @@ -5924,7 +5921,6 @@ void __ref build_all_zonelists(pg_data_t *pgdat)
> > > static bool __meminit
> > > overlap_memmap_init(unsigned long zone, unsigned long *pfn)
> > > {
> > > -#ifdef CONFIG_HAVE_MEMBLOCK_NODE_MAP
> > > static struct memblock_region *r;
> > >
> > > if (mirrored_kernelcore && zone == ZONE_MOVABLE) {
> > > @@ -5940,7 +5936,6 @@ overlap_memmap_init(unsigned long zone, unsigned long *pfn)
> > > return true;
> > > }
> > > }
> > > -#endif
> > > return false;
> > > }
> > >
> > > @@ -6573,8 +6568,7 @@ static unsigned long __init zone_absent_pages_in_node(int nid,
> > > return nr_absent;
> > > }
> > >
> > > -#else /* CONFIG_HAVE_MEMBLOCK_NODE_MAP */
> > > -static inline unsigned long __init zone_spanned_pages_in_node(int nid,
> > > +static inline unsigned long __init compat_zone_spanned_pages_in_node(int nid,
> >
> > Is it compact zone which has continuous memory region, and the
> > compat here is typo? Or it's compatible zone? The name seems a little
> > confusing, or I miss something.
>
> It's 'compat' from 'compatibility'. This is kinda "the old way" and the
> version that was defined when CONFIG_HAVE_MEMBLOCK_NODE_MAP=y is the
> "new way", so I picked 'compat' for backwards compatibility.
> Anyway, it will go away later in pacth 19.
Got it, thanks for telling.
>
> > > unsigned long zone_type,
> > > unsigned long node_start_pfn,
> > > unsigned long node_end_pfn,
> > > @@ -6593,7 +6587,7 @@ static inline unsigned long __init zone_spanned_pages_in_node(int nid,
> > > return zones_size[zone_type];
> > > }
> > >
> > > -static inline unsigned long __init zone_absent_pages_in_node(int nid,
> > > +static inline unsigned long __init compat_zone_absent_pages_in_node(int nid,
> > > unsigned long zone_type,
> > > unsigned long node_start_pfn,
> > > unsigned long node_end_pfn,
> > > @@ -6605,13 +6599,12 @@ static inline unsigned long __init zone_absent_pages_in_node(int nid,
> > > return zholes_size[zone_type];
> > > }
> > >
> > > -#endif /* CONFIG_HAVE_MEMBLOCK_NODE_MAP */
> > > -
> > > static void __init calculate_node_totalpages(struct pglist_data *pgdat,
> > > unsigned long node_start_pfn,
> > > unsigned long node_end_pfn,
> > > unsigned long *zones_size,
> > > - unsigned long *zholes_size)
> > > + unsigned long *zholes_size,
> > > + bool compat)
> > > {
> > > unsigned long realtotalpages = 0, totalpages = 0;
> > > enum zone_type i;
> > > @@ -6619,17 +6612,38 @@ static void __init calculate_node_totalpages(struct pglist_data *pgdat,
> > > for (i = 0; i < MAX_NR_ZONES; i++) {
> > > struct zone *zone = pgdat->node_zones + i;
> > > unsigned long zone_start_pfn, zone_end_pfn;
> > > + unsigned long spanned, absent;
> > > unsigned long size, real_size;
> > >
> > > - size = zone_spanned_pages_in_node(pgdat->node_id, i,
> > > - node_start_pfn,
> > > - node_end_pfn,
> > > - &zone_start_pfn,
> > > - &zone_end_pfn,
> > > - zones_size);
> > > - real_size = size - zone_absent_pages_in_node(pgdat->node_id, i,
> > > - node_start_pfn, node_end_pfn,
> > > - zholes_size);
> > > + if (compat) {
> > > + spanned = compat_zone_spanned_pages_in_node(
> > > + pgdat->node_id, i,
> > > + node_start_pfn,
> > > + node_end_pfn,
> > > + &zone_start_pfn,
> > > + &zone_end_pfn,
> > > + zones_size);
> > > + absent = compat_zone_absent_pages_in_node(
> > > + pgdat->node_id, i,
> > > + node_start_pfn,
> > > + node_end_pfn,
> > > + zholes_size);
> > > + } else {
> > > + spanned = zone_spanned_pages_in_node(pgdat->node_id, i,
> > > + node_start_pfn,
> > > + node_end_pfn,
> > > + &zone_start_pfn,
> > > + &zone_end_pfn,
> > > + zones_size);
> > > + absent = zone_absent_pages_in_node(pgdat->node_id, i,
> > > + node_start_pfn,
> > > + node_end_pfn,
> > > + zholes_size);
> > > + }
> > > +
> > > + size = spanned;
> > > + real_size = size - absent;
> > > +
> > > if (size)
> > > zone->zone_start_pfn = zone_start_pfn;
> > > else
> > > @@ -6929,10 +6943,8 @@ static void __ref alloc_node_mem_map(struct pglist_data *pgdat)
> > > */
> > > if (pgdat == NODE_DATA(0)) {
> > > mem_map = NODE_DATA(0)->node_mem_map;
> > > -#if defined(CONFIG_HAVE_MEMBLOCK_NODE_MAP) || defined(CONFIG_FLATMEM)
> > > if (page_to_pfn(mem_map) != pgdat->node_start_pfn)
> > > mem_map -= offset;
> > > -#endif /* CONFIG_HAVE_MEMBLOCK_NODE_MAP */
> > > }
> > > #endif
> > > }
> > > @@ -6949,9 +6961,10 @@ static inline void pgdat_set_deferred_range(pg_data_t *pgdat)
> > > static inline void pgdat_set_deferred_range(pg_data_t *pgdat) {}
> > > #endif
> > >
> > > -void __init free_area_init_node(int nid, unsigned long *zones_size,
> > > - unsigned long node_start_pfn,
> > > - unsigned long *zholes_size)
> > > +static void __init __free_area_init_node(int nid, unsigned long *zones_size,
> > > + unsigned long node_start_pfn,
> > > + unsigned long *zholes_size,
> > > + bool compat)
> > > {
> > > pg_data_t *pgdat = NODE_DATA(nid);
> > > unsigned long start_pfn = 0;
> > > @@ -6963,16 +6976,16 @@ void __init free_area_init_node(int nid, unsigned long *zones_size,
> > > pgdat->node_id = nid;
> > > pgdat->node_start_pfn = node_start_pfn;
> > > pgdat->per_cpu_nodestats = NULL;
> > > -#ifdef CONFIG_HAVE_MEMBLOCK_NODE_MAP
> > > - get_pfn_range_for_nid(nid, &start_pfn, &end_pfn);
> > > - pr_info("Initmem setup node %d [mem %#018Lx-%#018Lx]\n", nid,
> > > - (u64)start_pfn << PAGE_SHIFT,
> > > - end_pfn ? ((u64)end_pfn << PAGE_SHIFT) - 1 : 0);
> > > -#else
> > > - start_pfn = node_start_pfn;
> > > -#endif
> > > + if (!compat) {
> > > + get_pfn_range_for_nid(nid, &start_pfn, &end_pfn);
> > > + pr_info("Initmem setup node %d [mem %#018Lx-%#018Lx]\n", nid,
> > > + (u64)start_pfn << PAGE_SHIFT,
> > > + end_pfn ? ((u64)end_pfn << PAGE_SHIFT) - 1 : 0);
> > > + } else {
> > > + start_pfn = node_start_pfn;
> > > + }
> > > calculate_node_totalpages(pgdat, start_pfn, end_pfn,
> > > - zones_size, zholes_size);
> > > + zones_size, zholes_size, compat);
> > >
> > > alloc_node_mem_map(pgdat);
> > > pgdat_set_deferred_range(pgdat);
> > > @@ -6980,6 +6993,14 @@ void __init free_area_init_node(int nid, unsigned long *zones_size,
> > > free_area_init_core(pgdat);
> > > }
> > >
> > > +void __init free_area_init_node(int nid, unsigned long *zones_size,
> > > + unsigned long node_start_pfn,
> > > + unsigned long *zholes_size)
> > > +{
> > > + __free_area_init_node(nid, zones_size, node_start_pfn, zholes_size,
> > > + true);
> > > +}
> > > +
> > > #if !defined(CONFIG_FLAT_NODE_MEM_MAP)
> > > /*
> > > * Initialize all valid struct pages in the range [spfn, epfn) and mark them
> > > @@ -7063,8 +7084,6 @@ static inline void __init init_unavailable_mem(void)
> > > }
> > > #endif /* !CONFIG_FLAT_NODE_MEM_MAP */
> > >
> > > -#ifdef CONFIG_HAVE_MEMBLOCK_NODE_MAP
> > > -
> > > #if MAX_NUMNODES > 1
> > > /*
> > > * Figure out the number of possible node ids.
> > > @@ -7493,8 +7512,8 @@ void __init free_area_init_nodes(unsigned long *max_zone_pfn)
> > > init_unavailable_mem();
> > > for_each_online_node(nid) {
> > > pg_data_t *pgdat = NODE_DATA(nid);
> > > - free_area_init_node(nid, NULL,
> > > - find_min_pfn_for_node(nid), NULL);
> > > + __free_area_init_node(nid, NULL,
> > > + find_min_pfn_for_node(nid), NULL, false);
> > >
> > > /* Any memory on that node */
> > > if (pgdat->node_present_pages)
> > > @@ -7559,8 +7578,6 @@ static int __init cmdline_parse_movablecore(char *p)
> > > early_param("kernelcore", cmdline_parse_kernelcore);
> > > early_param("movablecore", cmdline_parse_movablecore);
> > >
> > > -#endif /* CONFIG_HAVE_MEMBLOCK_NODE_MAP */
> > > -
> > > void adjust_managed_page_count(struct page *page, long count)
> > > {
> > > atomic_long_add(count, &page_zone(page)->managed_pages);
> > > --
> > > 2.25.1
> > >
> >
>
> --
> Sincerely yours,
> Mike.
>
^ permalink raw reply
* Re: [PATCH] i2c: powermac: Simplify reading the "reg" and "i2c-address" property
From: Wolfram Sang @ 2020-04-21 9:37 UTC (permalink / raw)
To: Aishwarya R, Erhard F.
Cc: Kate Stewart, linux-kernel, Richard Fontana, Paul Mackerras,
linux-i2c, Greg Kroah-Hartman, Thomas Gleixner, linuxppc-dev
In-Reply-To: <20200415131916.23555-1-aishwaryarj100@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 750 bytes --]
On Wed, Apr 15, 2020 at 06:49:14PM +0530, Aishwarya R wrote:
> >> Use of_property_read_u32 to read the "reg" and "i2c-address" property
> >> instead of using of_get_property to check the return values.
> >>
> >> Signed-off-by: Aishwarya R <aishwaryarj100@gmail.com>
>
> > This is quite a fragile driver. Have you tested it on HW?
>
> This change is not tested with the Hardware.
> But of_property_read_u32 is better here than generic of_get_property.
> This make sure that value read properly independent of system endianess.
This driver is only used on PPC_BE. And it is *very* fragile. The gain
is not enough for me to accept it without testing. Maybe Erhard (CCed)
is interested. If not, you may find someone on the ppc lists.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH v2, RESEND] misc: new driver sram_uapi for user level SRAM access
From: Greg KH @ 2020-04-21 9:34 UTC (permalink / raw)
To: 王文虎
Cc: robh, arnd, Randy Dunlap, linux-kernel, Scott Wood, kernel,
linuxppc-dev
In-Reply-To: <AB6A-gBhCLyx5bgBRPuIiqp1.3.1587460187094.Hmail.wenhu.wang@vivo.com>
On Tue, Apr 21, 2020 at 05:09:47PM +0800, 王文虎 wrote:
> Hi, Greg, Arnd,
>
> Thank you for your comments first, and then really very very very sorry
> for driving Greg to sigh and I hope there would be chance to share Moutai
> (rather than whisky, we drink it much, a kind of Baijiu), after the virus.
>
> Back to the comments, I'd like to do a bit of documentation or explanation first,
> which should have been done early or else there would not be so much to explain:
> 1. What I have been trying to do is to access the Freescale Cache-SRAM device form
> user level;
> 2. I implemented it using UIO, which was thought of non-proper;
I still think that using uio is the best way to do this, and never said
it was "non-proper". All we got bogged down in was the DT
representation of stuff from what I remember. That should be worked
through.
thanks,
greg k-h
^ permalink raw reply
* Re: [PATCH 02/21] mm: make early_pfn_to_nid() and related defintions close to each other
From: Baoquan He @ 2020-04-21 9:33 UTC (permalink / raw)
To: Mike Rapoport
Cc: Rich Felker, linux-ia64, linux-doc, Catalin Marinas,
Heiko Carstens, Michal Hocko, James E.J. Bottomley, Max Filippov,
Guo Ren, linux-csky, linux-parisc, sparclinux, linux-hexagon,
linux-riscv, Greg Ungerer, linux-arch, linux-s390, linux-snps-arc,
linux-c6x-dev, Brian Cain, Jonathan Corbet, linux-sh,
Helge Deller, x86, Russell King, Ley Foon Tan, Mike Rapoport,
Geert Uytterhoeven, linux-arm-kernel, Mark Salter, Matt Turner,
linux-mips, uclinux-h8-devel, linux-xtensa, linux-alpha, linux-um,
linux-m68k, Tony Luck, Greentime Hu, Paul Walmsley,
Stafford Horne, Guan Xuetao, Hoan Tran, Michal Simek,
Thomas Bogendoerfer, Yoshinori Sato, Nick Hu, linux-mm,
Vineet Gupta, linux-kernel, openrisc, Richard Weinberger,
Andrew Morton, linuxppc-dev, David S. Miller
In-Reply-To: <20200421084935.GB14260@kernel.org>
On 04/21/20 at 11:49am, Mike Rapoport wrote:
> On Tue, Apr 21, 2020 at 10:24:35AM +0800, Baoquan He wrote:
> > On 04/12/20 at 10:48pm, Mike Rapoport wrote:
> > > From: Mike Rapoport <rppt@linux.ibm.com>
> > >
> > > The early_pfn_to_nid() and it's helper __early_pfn_to_nid() are spread
> > > around include/linux/mm.h, include/linux/mmzone.h and mm/page_alloc.c.
> > >
> > > Drop unused stub for __early_pfn_to_nid() and move its actual generic
> > > implementation close to its users.
> > >
> > > Signed-off-by: Mike Rapoport <rppt@linux.ibm.com>
> > > ---
> > > include/linux/mm.h | 4 ++--
> > > include/linux/mmzone.h | 9 --------
> > > mm/page_alloc.c | 51 +++++++++++++++++++++---------------------
> > > 3 files changed, 27 insertions(+), 37 deletions(-)
> > >
> > > diff --git a/include/linux/mm.h b/include/linux/mm.h
> > > index 5a323422d783..a404026d14d4 100644
> > > --- a/include/linux/mm.h
> > > +++ b/include/linux/mm.h
> > > @@ -2388,9 +2388,9 @@ extern void sparse_memory_present_with_active_regions(int nid);
> > >
> > > #if !defined(CONFIG_HAVE_MEMBLOCK_NODE_MAP) && \
> > > !defined(CONFIG_HAVE_ARCH_EARLY_PFN_TO_NID)
> > > -static inline int __early_pfn_to_nid(unsigned long pfn,
> > > - struct mminit_pfnnid_cache *state)
> > > +static inline int early_pfn_to_nid(unsigned long pfn)
> > > {
> > > + BUILD_BUG_ON(IS_ENABLED(CONFIG_NUMA));
> > > return 0;
> > > }
> >
> > It's better to make a separate patch to drop __early_pfn_to_nid() here.
>
> Not sure it's really worth it.
> This patch anyway only moves the code around without any actual changes.
OK, it's fine to me.
^ permalink raw reply
* Re: [PATCH v2 5/5] uaccess: Rename user_access_begin/end() to user_full_access_begin/end()
From: Russell King - ARM Linux admin @ 2020-04-21 9:12 UTC (permalink / raw)
To: Al Viro, Will Deacon
Cc: linux-arch, linuxppc-dev, Kees Cook, Dave Airlie, intel-gfx,
Peter Anvin, Linux Kernel Mailing List, Linux-MM, Paul Mackerras,
Daniel Vetter, Andrew Morton, Linus Torvalds
In-Reply-To: <20200421024919.GA23230@ZenIV.linux.org.uk>
On Tue, Apr 21, 2020 at 03:49:19AM +0100, Al Viro wrote:
> The only source I'd been able to find speeks of >= 60 cycles
> (and possibly much more) for non-pipelined coprocessor instructions;
> the list of such does contain loads and stores to a bunch of registers.
> However, the register in question (p15/c3) has only store mentioned there,
> so loads might be cheap; no obvious reasons for those to be slow.
> That's a question to arm folks, I'm afraid... rmk?
I have no information on that; instruction timings are not defined
at architecture level (architecture reference manual), nor do I find
information in the CPU technical reference manual (which would be
specific to the CPU). Instruction timings tend to be implementation
dependent.
I've always consulted Will Deacon when I've needed to know whether
an instruction is expensive or not.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 10.2Mbps down 587kbps up
^ permalink raw reply
* Re: [PATCH v2,RESEND] misc: new driver sram_uapi for user level SRAM access
From: 王文虎 @ 2020-04-21 9:09 UTC (permalink / raw)
To: Greg KH
Cc: robh, arnd, Randy Dunlap, linux-kernel, Scott Wood, kernel,
linuxppc-dev
In-Reply-To: <20200420145128.GA4131449@kroah.com>
Hi, Greg, Arnd,
Thank you for your comments first, and then really very very very sorry
for driving Greg to sigh and I hope there would be chance to share Moutai
(rather than whisky, we drink it much, a kind of Baijiu), after the virus.
Back to the comments, I'd like to do a bit of documentation or explanation first,
which should have been done early or else there would not be so much to explain:
1. What I have been trying to do is to access the Freescale Cache-SRAM device form
user level;
2. I implemented it using UIO, which was thought of non-proper;
3. So I implemented it here to create a misc device as a interface between Kernel
and user space applications, as the comments of Scott suggested;
[Link for 2 & 3: https://lore.kernel.org/patchwork/patch/1225798/]
4. This is exactly a shim of Cache-SRAM hardware level driver, and it would use
apis provided by the hardware driver to do the allocation and free work of
SRAM memory;
5. The file drivers/misc/sram.c actually has done some abstractions for SRAM access,
but it is used as a static way that resources are defined within devicetree;
6. This patch is to reach a way that allocate and release SRAM memory dynamically
which is much more convenient for user space applications, and for another reason,
the Cache-SRAM of Freescale is kind of not compatible with drivers/misc/sram.c;
So I implemented the sram_uapi.c as following:
1. Create a misc device as a interface that support ioctl and mmap for memory access;
2. For that I think this could be used for any SRAM devices that would support dynamic
memory allocation and release, I create sram_api_list as follow:
static DEFINE_MUTEX(sram_api_list_lock);
static LIST_HEAD(sram_api_list);
and different SRAM devices could add their apis to the sram_api_list
which could be used for the allocation from them;
a. int sram_api_register(struct sram_api *sa);
b. int sram_api_unregister(struct sram_api *sa);
c. ioctl case SRAM_UAPI_IOC_SET_SRAM_TYPE;
the register and unregister api are exported for any SRAM drivers to do this,
and maybe user could chose one with ioctl to allocate;
[maybe only one SRAM device available currently and the implementation is redundant]
3. For each fd that opened, a sram_uapi would be created:
struct sram_uapi {
struct list_head res_list;
struct sram_api *sa;
};
The res_list would be a list of resources that allocated attached to it:
struct sram_resource {
struct list_head list;
struct res_info info;
phys_addr_t phys;
void *virt;
struct vm_area_struct *vma;
struct sram_uapi *parent;
};
The sa is a pointer to the apis registered by hardware SRAM driver as descibed earlier,
and when a fd is attached to it, kref it once;
And for the resources, it is accessed by the process only so I did not use a lock here,
and as Greg commented, lock is needed for the fd sharement.
4. The exported apis are currently not reference and I would add another patch series
for Freescale Cache-SRAM to use the api sram_api_register to register its allocation
and free api, as well as sram_api_unregister for unregistering;
Actually, I implemented the api list another way in v1, but was commented by Arnd not a
better way.
[https://lore.kernel.org/patchwork/patch/1226689/]
Then for the comments:
1. Ioctl: I will move the block to uapi, and use static length definition of __be64;
2. Naming: as Greg commented, so hard, and how do you think of
sram_dynamic(as compared to drivers/misc/sram.c)?
3. API list: only one SRAM device? But however, this is a shim, and hardware level
SRAM driver(s) should be enabled to register or init the apis for real allocation;
4. Test: My team and me myself did not cover it, especially newly added kref, and that
is really really so wrong of us, and I will make efforts to test for every logical path
next time, and this would never ever happen. Really sorry for it.
>On Sun, Apr 19, 2020 at 08:05:38PM -0700, Wang Wenhu wrote:
>> A generic User-Kernel interface that allows a misc device created
>> by it to support file-operations of ioctl and mmap to access SRAM
>> memory from user level. Different kinds of SRAM alloction and free
>> APIs could be registered by specific SRAM hardware level driver to
>> the available list and then be chosen by users to allocate and map
>> SRAM memory from user level.
>>
>> It is extremely helpful for the user space applications that require
>> high performance memory accesses, such as embedded networking devices
>> that would process data in user space, and PowerPC e500 is a case.
>>
>> Signed-off-by: Wang Wenhu <wenhu.wang@vivo.com>
>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>> Cc: Arnd Bergmann <arnd@arndb.de>
>> Cc: Christophe Leroy <christophe.leroy@c-s.fr>
>> Cc: Scott Wood <oss@buserror.net>
>> Cc: Michael Ellerman <mpe@ellerman.id.au>
>> Cc: Randy Dunlap <rdunlap@infradead.org>
>> Cc: linuxppc-dev@lists.ozlabs.org
>> ---
>> Changes since v1: addressed comments from Arnd
>> * Changed the ioctl cmd definitions using _IO micros
>> * Export interfaces for HW-SRAM drivers to register apis to available list
>> * Modified allocation alignment to PAGE_SIZE
>> * Use phys_addr_t as type of SRAM resource size and offset
>> * Support compat_ioctl
>> * Misc device name:sram
>>
>> Note: From this on, the SRAM_UAPI driver is independent to any hardware
>> drivers, so I would only commit the patch itself as v2, while the v1 of
>> it was wrapped together with patches for Freescale L2-Cache-SRAM device.
>> Then after, I'd create patches for Freescale L2-Cache-SRAM device as
>> another series.
>>
>> Link for v1:
>> * https://lore.kernel.org/lkml/20200418162157.50428-5-wenhu.wang@vivo.com/
>
>Why was this a RESEND? What was wrong with the original v2 version?
>
>Anyway, minor review comments:
>
>> ---
>> drivers/misc/Kconfig | 11 ++
>> drivers/misc/Makefile | 1 +
>> drivers/misc/sram_uapi.c | 352 ++++++++++++++++++++++++++++++++++++++
>> include/linux/sram_uapi.h | 28 +++
>> 4 files changed, 392 insertions(+)
>> create mode 100644 drivers/misc/sram_uapi.c
>> create mode 100644 include/linux/sram_uapi.h
>>
>> diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
>> index 99e151475d8f..b19c8b24f18e 100644
>> --- a/drivers/misc/Kconfig
>> +++ b/drivers/misc/Kconfig
>> @@ -465,6 +465,17 @@ config PVPANIC
>> a paravirtualized device provided by QEMU; it lets a virtual machine
>> (guest) communicate panic events to the host.
>>
>> +config SRAM_UAPI
>> + bool "Generic SRAM User Level API driver"
>> + help
>> + This driver allows you to create a misc device which could be used
>> + as an interface to allocate SRAM memory from user level.
>> +
>> + It is extremely helpful for some user space applications that require
>> + high performance memory accesses.
>> +
>> + If unsure, say N.
>
>Naming is hard, but shouldn't this just be "sram" and drop the whole
>"UAPI" stuff everywhere? That doesn't make much sense as drivers are
>just interfaces to userspace...
>
>
>> +
>> source "drivers/misc/c2port/Kconfig"
>> source "drivers/misc/eeprom/Kconfig"
>> source "drivers/misc/cb710/Kconfig"
>> diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
>> index 9abf2923d831..794447ca07ca 100644
>> --- a/drivers/misc/Makefile
>> +++ b/drivers/misc/Makefile
>> @@ -46,6 +46,7 @@ obj-$(CONFIG_VMWARE_VMCI) += vmw_vmci/
>> obj-$(CONFIG_LATTICE_ECP3_CONFIG) += lattice-ecp3-config.o
>> obj-$(CONFIG_SRAM) += sram.o
>> obj-$(CONFIG_SRAM_EXEC) += sram-exec.o
>> +obj-$(CONFIG_SRAM_UAPI) += sram_uapi.o
>> obj-y += mic/
>> obj-$(CONFIG_GENWQE) += genwqe/
>> obj-$(CONFIG_ECHO) += echo/
>> diff --git a/drivers/misc/sram_uapi.c b/drivers/misc/sram_uapi.c
>> new file mode 100644
>> index 000000000000..66c7b56b635f
>> --- /dev/null
>> +++ b/drivers/misc/sram_uapi.c
>> @@ -0,0 +1,352 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (C) 2020 Vivo Communication Technology Co. Ltd.
>> + * Copyright (C) 2020 Wang Wenhu <wenhu.wang@vivo.com>
>> + * All rights reserved.
>> + */
>> +
>> +#include <linux/module.h>
>> +#include <linux/kernel.h>
>> +#include <linux/slab.h>
>> +#include <linux/mm.h>
>> +#include <linux/fs.h>
>> +#include <linux/miscdevice.h>
>> +#include <linux/uaccess.h>
>> +#include <linux/sram_uapi.h>
>> +
>> +#define DRIVER_NAME "sram_uapi"
>
>If you really need this, just use KBUILD_MODNAME instead. But I don't
>think you need it, as most drivers do not.
>
Yes, no need here.
>
>> +
>> +struct res_info {
>> + phys_addr_t offset;
>> + phys_addr_t size;
>> +};
>> +
>> +struct sram_resource {
>> + struct list_head list;
>> + struct res_info info;
>> + phys_addr_t phys;
>> + void *virt;
>> + struct vm_area_struct *vma;
>> + struct sram_uapi *parent;
>> +};
>> +
>> +struct sram_uapi {
>> + struct list_head res_list;
>> + struct sram_api *sa;
>> +};
>> +
>> +static DEFINE_MUTEX(sram_api_list_lock);
>> +static LIST_HEAD(sram_api_list);
>> +
>> +long sram_api_register(struct sram_api *sa)
>
>Why are all of these functions global?
>
>And shouldn't this return an 'int'?
>
Should be int, and as explained upper, this is to be called by
hardware driver like Freescale Cache-SRAM to register their
allocation and free apis to the sram_api_list.
>> +{
>> + struct sram_api *cur;
>> +
>> + if (!sa || !sa->name || !sa->sram_alloc || !sa->sram_free)
>> + return -EINVAL;
>
>How can any of these be true? You control all the callers of this
>function.
Allocation and free apis are really needed, while name maybe is just
a superfluous choice(which may be added to attrs), I will drop it.
>
>> +
>> + mutex_lock(&sram_api_list_lock);
>> + list_for_each_entry(cur, &sram_api_list, list) {
>> + if (cur->type == sa->type) {
>> + pr_err("error sram %s type %d exists\n", sa->name,
>> + sa->type);
>> + mutex_unlock(&sram_api_list_lock);
>> + return -EEXIST;
>> + }
>> + }
>> +
>> + kref_init(&sa->kref);
>
>So the caller has to allocate the space and set some things up, but not
>everything? That's a mess, why not just do it either all at once with
>an "allocate" like function, or just do it all in one function. Why is
>this a separate function anyway?
>
Should be done by caller.
>> + list_add_tail(&sa->list, &sram_api_list);
>> + pr_info("sram %s type %d registered\n", sa->name, sa->type);
>
>Don't leave debugging comments in the driver, if all goes well a driver
>should be totally silent. Only log errors.
>
>> +
>> + mutex_unlock(&sram_api_list_lock);
>> +
>> + return 0;
>> +};
>> +EXPORT_SYMBOL(sram_api_register);
>
>Exported for what? This is a stand-alone driver, nothing else uses
>these symbols.
>
To be added to the hardware drivers, and currently Freescale Cache-SRAM.
(So I should have done all these within one series?)
>> +
>> +long sram_api_unregister(struct sram_api *sa)
>
>Why long?
int everywhere(rather than uapi, I was influenced by ioctl return type).
>
>> +{
>> + struct sram_api *cur, *tmp;
>> + long ret = -ENODEV;
>> +
>> + if (!sa || !sa->name || !sa->sram_alloc || !sa->sram_free)
>> + return -EINVAL;
>
>Again, how can any of this be true?
>
>> +
>> + mutex_lock(&sram_api_list_lock);
>> + list_for_each_entry_safe(cur, tmp, &sram_api_list, list) {
>> + if (cur->type == sa->type && !strcmp(cur->name, sa->name)) {
>> + if (kref_read(&cur->kref)) {
>> + pr_err("error sram %s type %d is busy\n",
>> + sa->name, sa->type);
>> + ret = -EBUSY;
>> + } else {
>> + list_del(&cur->list);
>> + pr_info("sram %s type %d unregistered\n",
>> + sa->name, sa->type);
>> + ret = 0;
>> + }
>> + break;
>> + }
>> + }
>> + mutex_unlock(&sram_api_list_lock);
>> +
>> + return ret;
>> +};
>> +EXPORT_SYMBOL(sram_api_unregister);
>> +
>> +static struct sram_api *get_sram_api_from_type(__u32 type)
>> +{
>> + struct sram_api *cur;
>> +
>> + mutex_lock(&sram_api_list_lock);
>> + list_for_each_entry(cur, &sram_api_list, list) {
>> + if (cur->type == type) {
>> + kref_get(&cur->kref);
>> + mutex_unlock(&sram_api_list_lock);
>> + return cur;
>
>Document that the reference count is incremented, or someone will forget
>that.
>
I will.
>
>> + }
>> + }
>> + mutex_unlock(&sram_api_list_lock);
>> +
>> + return NULL;
>> +}
>> +
>> +static void sram_uapi_res_insert(struct sram_uapi *uapi,
>> + struct sram_resource *res)
>> +{
>> + struct sram_resource *cur, *tmp;
>> + struct list_head *head = &uapi->res_list;
>> +
>> + list_for_each_entry_safe(cur, tmp, head, list) {
>> + if (&tmp->list != head &&
>> + (cur->info.offset + cur->info.size + res->info.size <=
>> + tmp->info.offset)) {
>> + res->info.offset = cur->info.offset + cur->info.size;
>
>This feels really odd, what are you doing here with this loop to try to
>find a space and then put it in and then change the resource???
>
Like commets from Scott, I used the fd to allocate a lot of memory areas and they are
managed with different offset. So I just need to keep them in a sequence and no overlap.
The parent is useless and I will delete it. Anyway, things should be done before put it in.
I may change it according to the comments from Scott, that a fd only be attached to one
memory area which looks exactly like the thing in drivers/misc/sram.c. And then, this block
is useless and will be dropped.
>
>> + res->parent = uapi;
>> + list_add(&res->list, &cur->list);
>> + return;
>> + }
>> + }
>> +
>> + if (list_empty(head))
>> + res->info.offset = 0;
>> + else {
>> + tmp = list_last_entry(head, struct sram_resource, list);
>> + res->info.offset = tmp->info.offset + tmp->info.size;
>> + }
>> + list_add_tail(&res->list, head);
>
>No locking anywhere?
This is attached to the fd, and may be co-accessed, should be locked for access.
>
>> +}
>> +
>> +static struct sram_resource *sram_uapi_res_delete(struct sram_uapi *uapi,
>> + struct res_info *info)
>> +{
>> + struct sram_resource *res, *tmp;
>> +
>> + list_for_each_entry_safe(res, tmp, &uapi->res_list, list) {
>> + if (res->info.offset == info->offset) {
>
>Why is the offset somehow the "key" for this?
A fd attached to a lot of memory areas, and they are arranged by offset within fd.
>
>> + list_del(&res->list);
>> + res->parent = NULL;
>> + return res;
>> + }
>> + }
>
>No locking anywhere?
>
>> +
>> + return NULL;
>> +}
>> +
>> +static struct sram_resource *sram_uapi_find_res(struct sram_uapi *uapi,
>> + __u32 offset)
>> +{
>> + struct sram_resource *res;
>> +
>> + list_for_each_entry(res, &uapi->res_list, list) {
>> + if (res->info.offset == offset)
>> + return res;
>> + }
>
>No lock??? No reference counting?
As addressed upper, should use lock. And yes, should be reference counted.
>
>> +
>> + return NULL;
>> +}
>> +
>> +static int sram_uapi_open(struct inode *inode, struct file *filp)
>> +{
>> + struct sram_uapi *uapi;
>> +
>> + uapi = kzalloc(sizeof(*uapi), GFP_KERNEL);
>> + if (!uapi)
>> + return -ENOMEM;
>> +
>> + INIT_LIST_HEAD(&uapi->res_list);
>> + filp->private_data = uapi;
>> +
>> + return 0;
>> +}
>> +
>> +static long sram_uapi_ioctl(struct file *filp, unsigned int cmd,
>> + unsigned long arg)
>> +{
>> + struct sram_uapi *uapi = filp->private_data;
>> + struct sram_resource *res;
>> + struct res_info info;
>> + long ret = -ENOIOCTLCMD;
>
>-ENOTTY?
Yes.
>
>> + int size;
>> + __u32 type;
>> +
>> + if (!uapi)
>> + return ret;
>> +
>> + switch (cmd) {
>> + case SRAM_UAPI_IOC_SET_SRAM_TYPE:
>> + if (uapi->sa)
>> + return -EEXIST;
>> +
>> + get_user(type, (const __u32 __user *)arg);
>> + uapi->sa = get_sram_api_from_type(type);
>> + if (uapi->sa)
>> + ret = 0;
>> + else
>> + ret = -ENODEV;
>> +
>> + break;
>
>Why does userspace have to set the type of sram?
>
Currently only one SRAM available on SoC, if as descriped upper there
are more, user fd could select which one it would be attached to.
>
>> +
>> + case SRAM_UAPI_IOC_ALLOC:
>> + if (!uapi->sa)
>> + return -EINVAL;
>> +
>> + res = kzalloc(sizeof(*res), GFP_KERNEL);
>> + if (!res)
>> + return -ENOMEM;
>> +
>> + size = copy_from_user((void *)&res->info,
>> + (const void __user *)arg,
>> + sizeof(res->info));
>> + if (!PAGE_ALIGNED(res->info.size) || !res->info.size)
>> + return -EINVAL;
>> +
>> + res->virt = (void *)uapi->sa->sram_alloc(res->info.size,
>> + &res->phys,
>> + PAGE_SIZE);
>
>Look ma, userspace just allocated as much memory as it asked for!
>
>Remember, all input is evil, check everything.
>
>And, as is obvious now, this isn't really a "driver", it's a shim layer
>for something else. Who sets up sram_alloc()?
>
Yes, a shim, and sram_api_register exported to be called by hardware driver
to register it. Params should be checked and I guess the size checked by
lower alloc api?(-ENOMEM or something be returned by it if not afforded to?)
Actually in v1, I used a array to reference the apis statically, and wrapped
with specific MICROs, and changed it this way according to the comments of Arnd.
Link: https://lore.kernel.org/patchwork/patch/1226689/
>> + if (!res->virt) {
>> + kfree(res);
>> + return -ENOMEM;
>> + }
>> +
>> + sram_uapi_res_insert(uapi, res);
>> + size = copy_to_user((void __user *)arg,
>> + (const void *)&res->info,
>> + sizeof(res->info));
>
>an alloc function copies a bunch of memory to userspace????
Tell the userspace the offset of area within the fd, which is the key
as you mentioned upper, and could be used for SRAM_UAPI_IOC_FREE to release,
and mmap to finally attach it to the vma of user process.
>
>You need to document the heck out of this new interface as it really
>does not make sense to me, sorry.
>
>> +
>> + ret = 0;
>> + break;
>> +
>> + case SRAM_UAPI_IOC_FREE:
>> + if (!uapi->sa)
>> + return -EINVAL;
>> +
>> + size = copy_from_user((void *)&info, (const void __user *)arg,
>> + sizeof(info));
>> +
>> + res = sram_uapi_res_delete(uapi, &info);
>> + if (!res) {
>> + pr_err("error no sram resource found\n");
>> + return -EINVAL;
>> + }
>> +
>> + uapi->sa->sram_free(res->virt);
>> + kfree(res);
>
>Two step delete function with locking at all? Ugh :(
>
As upper, should be locked.
>> +
>> + ret = 0;
>> + break;
>> +
>> + default:
>> + pr_err("error no cmd not supported\n");
>> + break;
>
>Userspace now can spam the kernel log, do not do that.
>
Learned.
>> + }
>> +
>> + return ret;
>> +}
>> +
>> +static int sram_uapi_mmap(struct file *filp, struct vm_area_struct *vma)
>> +{
>> + struct sram_uapi *uapi = filp->private_data;
>> + struct sram_resource *res;
>> +
>> + res = sram_uapi_find_res(uapi, vma->vm_pgoff);
>> + if (!res)
>> + return -EINVAL;
>> +
>> + if (vma->vm_end - vma->vm_start > res->info.size)
>> + return -EINVAL;
>> +
>> + vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot);
>> +
>> + return remap_pfn_range(vma, vma->vm_start,
>> + res->phys >> PAGE_SHIFT,
>> + vma->vm_end - vma->vm_start,
>> + vma->vm_page_prot);
>> +}
>> +
>> +static void sram_uapi_res_release(struct sram_uapi *uapi)
>> +{
>> + struct sram_resource *res, *tmp;
>> +
>> + list_for_each_entry_safe(res, tmp, &uapi->res_list, list) {
>> + list_del(&res->list);
>> + uapi->sa->sram_free(res->virt);
>> + kfree(res);
>> + }
>> +}
>> +
>> +static int sram_uapi_release(struct inode *inodp, struct file *filp)
>> +{
>> + struct sram_uapi *uapi = filp->private_data;
>> +
>> + sram_uapi_res_release(uapi);
>> + if (uapi->sa)
>> + kref_put(&uapi->sa->kref, NULL);
>
>Um, hm, how do I put this nicely.
>
>Wow, this is wrong. So so so so so so wrong.
>
>There's documentation in the kernel tree about how wrong this is. The
>kernel itself will warn you about how wrong this is. Actually, no, it
>will just crash. If this function ever gets called your kernel just
>blew up, so you really really really did not test this code at all.
>
>{sigh}
>
>Someone owes me a new bottle of whisky, this one is about to be empty...
>
>
I will check it again and again and again and again. So so so so Sorry.
And I would catch up with it after another checking done.
>
>
>> +
>> + kfree(uapi);
>> +
>> + return 0;
>> +}
>> +
>> +static const struct file_operations sram_uapi_ops = {
>> + .owner = THIS_MODULE,
>> + .open = sram_uapi_open,
>> + .unlocked_ioctl = sram_uapi_ioctl,
>> + .compat_ioctl = compat_ptr_ioctl,
>> + .mmap = sram_uapi_mmap,
>> + .release = sram_uapi_release,
>> +};
>> +
>> +static struct miscdevice sram_uapi_miscdev = {
>> + MISC_DYNAMIC_MINOR,
>> + "sram",
>> + &sram_uapi_ops,
>
>named identifiers please, this isn't the 1980's.
>
? you mean MISC_DYNAMIC_MINOR?
>> +};
>> +
>> +static int __init sram_uapi_init(void)
>> +{
>> + int ret;
>> +
>> + INIT_LIST_HEAD(&sram_api_list);
>
>Why do you need a static list?
>
>> + mutex_init(&sram_api_list_lock);
>
>Why do you need a static lock?
>
Like we have there SRAM or anything like it that would call exported
symbol sram_api_unregister, this is to keep it safe.
>> +
>> + ret = misc_register(&sram_uapi_miscdev);
>> + if (ret)
>> + pr_err("failed to register sram uapi misc device\n");
>
>No need for the error, don't be noisy.
Got it.
>
>> +
>> + return ret;
>> +}
>> +
>> +static void __exit sram_uapi_exit(void)
>> +{
>> + misc_deregister(&sram_uapi_miscdev);
>> +}
>> +
>> +module_init(sram_uapi_init);
>> +module_exit(sram_uapi_exit);
>
>drop the crazy static list and lock (again, this isn't the 1990's), and
>you can get rid of the module_init/exit stuff too and just use a single
>line to register the misc device.
>
>Meta-comment here, you are trying to create a driver with a new api, but
>you have no real users of this api, so no one knows if this really is
>the correct api at all. That's not ok, especially for one that is
>purporting to be so "generic" as to claim it is a sram driver for
>everyone.
>
>
As mentioned before, maybe I should have added the caller of sram_api_unregister
in Freescale Cache-SRAM driver too in a series.
>
>
>> +
>> +MODULE_AUTHOR("Wang Wenhu <wenhu.wang@vivo.com>");
>> +MODULE_DESCRIPTION("SRAM User API Driver");
>> +MODULE_ALIAS("platform:" DRIVER_NAME);
>> +MODULE_LICENSE("GPL v2");
>> diff --git a/include/linux/sram_uapi.h b/include/linux/sram_uapi.h
>> new file mode 100644
>> index 000000000000..50fbf9dc308f
>> --- /dev/null
>> +++ b/include/linux/sram_uapi.h
>> @@ -0,0 +1,28 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +#ifndef __SRAM_UAPI_H
>> +#define __SRAM_UAPI_H
>> +
>> +/* Set SRAM type to be accessed */
>> +#define SRAM_UAPI_IOC_SET_SRAM_TYPE _IOW('S', 0, __u32)
>> +
>> +/* Allocate resource from SRAM */
>> +#define SRAM_UAPI_IOC_ALLOC _IOWR('S', 1, struct res_info)
>> +
>> +/* Free allocated resource of SRAM */
>> +#define SRAM_UAPI_IOC_FREE _IOW('S', 2, struct res_info)
>> +
>> +struct sram_api {
>> + struct list_head list;
>> + struct kref kref;
>> + __u32 type;
>> + const char *name;
>> +
>> + long (*sram_alloc)(__u32 size, phys_addr_t *phys, __u32 align);
>> + void (*sram_free)(void *ptr);
>> +};
>
>Why is this structure in a uapi header file?
>
>Oh wait, this isn't a uapi header file, this thing doesn't work at all.
What can I say......
>
>ugh, this needs _so_ much work.
I will do it do it and do it.
>
>And again, someone owes me more whisky....
Sorry for that
>
Thanks and regards,
Wenhu
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox