* 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
* Re: [PATCH 03/21] mm: remove CONFIG_HAVE_MEMBLOCK_NODE_MAP option
From: Mike Rapoport @ 2020-04-21 9:09 UTC (permalink / raw)
To: Baoquan He
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: <20200421042316.GQ4247@MiWiFi-R3L-srv>
On Tue, Apr 21, 2020 at 12:23:16PM +0800, Baoquan He wrote:
> On 04/12/20 at 10:48pm, Mike Rapoport wrote:
> > From: Mike Rapoport <rppt@linux.ibm.com>
> >
> > The CONFIG_HAVE_MEMBLOCK_NODE_MAP is used to differentiate initialization
> > of nodes and zones structures between the systems that have region to node
> > mapping in memblock and those that don't.
> >
> > Currently all the NUMA architectures enable this option and for the
> > non-NUMA systems we can presume that all the memory belongs to node 0 and
> > therefore the compile time configuration option is not required.
> >
> > The remaining few architectures that use DISCONTIGMEM without NUMA are
> > easily updated to use memblock_add_node() instead of memblock_add() and
> > thus have proper correspondence of memblock regions to NUMA nodes.
> >
> > Still, free_area_init_node() must have a backward compatible version
> > because its semantics with and without CONFIG_HAVE_MEMBLOCK_NODE_MAP is
> > different. Once all the architectures will use the new semantics, the
> > entire compatibility layer can be dropped.
> >
> > To avoid addition of extra run time memory to store node id for
> > architectures that keep memblock but have only a single node, the node id
> > field of the memblock_region is guarded by CONFIG_NEED_MULTIPLE_NODES and
> > the corresponding accessors presume that in those cases it is always 0.
> >
> > Signed-off-by: Mike Rapoport <rppt@linux.ibm.com>
> > ---
> ...
> > diff --git a/include/linux/memblock.h b/include/linux/memblock.h
> > index 6bc37a731d27..45abfc54da37 100644
> > --- a/include/linux/memblock.h
> > +++ b/include/linux/memblock.h
> > @@ -50,7 +50,7 @@ struct memblock_region {
> > phys_addr_t base;
> > phys_addr_t size;
> > enum memblock_flags flags;
> > -#ifdef CONFIG_HAVE_MEMBLOCK_NODE_MAP
> > +#ifdef CONFIG_NEED_MULTIPLE_NODES
> > int nid;
> > #endif
> > };
> > @@ -215,7 +215,6 @@ static inline bool memblock_is_nomap(struct memblock_region *m)
> > return m->flags & MEMBLOCK_NOMAP;
> > }
> >
> > -#ifdef CONFIG_HAVE_MEMBLOCK_NODE_MAP
> > int memblock_search_pfn_nid(unsigned long pfn, unsigned long *start_pfn,
> > unsigned long *end_pfn);
> > void __next_mem_pfn_range(int *idx, int nid, unsigned long *out_start_pfn,
> > @@ -234,7 +233,6 @@ void __next_mem_pfn_range(int *idx, int nid, unsigned long *out_start_pfn,
> > #define for_each_mem_pfn_range(i, nid, p_start, p_end, p_nid) \
> > for (i = -1, __next_mem_pfn_range(&i, nid, p_start, p_end, p_nid); \
> > i >= 0; __next_mem_pfn_range(&i, nid, p_start, p_end, p_nid))
> > -#endif /* CONFIG_HAVE_MEMBLOCK_NODE_MAP */
> >
> > #ifdef CONFIG_DEFERRED_STRUCT_PAGE_INIT
> > void __next_mem_pfn_range_in_zone(u64 *idx, struct zone *zone,
> > @@ -310,10 +308,10 @@ void __next_mem_pfn_range_in_zone(u64 *idx, struct zone *zone,
> > for_each_mem_range_rev(i, &memblock.memory, &memblock.reserved, \
> > nid, flags, p_start, p_end, p_nid)
> >
> > -#ifdef CONFIG_HAVE_MEMBLOCK_NODE_MAP
> > int memblock_set_node(phys_addr_t base, phys_addr_t size,
> > struct memblock_type *type, int nid);
> >
> > +#ifdef CONFIG_NEED_MULTIPLE_NODES
> > static inline void memblock_set_region_node(struct memblock_region *r, int nid)
> > {
> > r->nid = nid;
> > @@ -332,7 +330,7 @@ static inline int memblock_get_region_node(const struct memblock_region *r)
> > {
> > return 0;
> > }
> > -#endif /* CONFIG_HAVE_MEMBLOCK_NODE_MAP */
> > +#endif /* CONFIG_NEED_MULTIPLE_NODES */
> >
> > /* Flags for memblock allocation APIs */
> > #define MEMBLOCK_ALLOC_ANYWHERE (~(phys_addr_t)0)
> > diff --git a/include/linux/mm.h b/include/linux/mm.h
> > index a404026d14d4..5903bbbdb336 100644
> > --- a/include/linux/mm.h
> > +++ b/include/linux/mm.h
> > @@ -2344,9 +2344,8 @@ static inline unsigned long get_num_physpages(void)
> > return phys_pages;
> > }
> >
> > -#ifdef CONFIG_HAVE_MEMBLOCK_NODE_MAP
> > /*
> > - * With CONFIG_HAVE_MEMBLOCK_NODE_MAP set, an architecture may initialise its
> > + * Using memblock node mappings, an architecture may initialise its
> > * zones, allocate the backing mem_map and account for memory holes in a more
> > * architecture independent manner. This is a substitute for creating the
> > * zone_sizes[] and zholes_size[] arrays and passing them to
> > @@ -2367,9 +2366,6 @@ static inline unsigned long get_num_physpages(void)
> > * registered physical page range. Similarly
> > * sparse_memory_present_with_active_regions() calls memory_present() for
> > * each range when SPARSEMEM is enabled.
> > - *
> > - * See mm/page_alloc.c for more information on each function exposed by
> > - * CONFIG_HAVE_MEMBLOCK_NODE_MAP.
> > */
> > extern void free_area_init_nodes(unsigned long *max_zone_pfn);
> > unsigned long node_map_pfn_alignment(void);
> > @@ -2384,13 +2380,9 @@ extern void free_bootmem_with_active_regions(int nid,
> > unsigned long max_low_pfn);
> > extern void sparse_memory_present_with_active_regions(int nid);
> >
> > -#endif /* CONFIG_HAVE_MEMBLOCK_NODE_MAP */
> > -
> > -#if !defined(CONFIG_HAVE_MEMBLOCK_NODE_MAP) && \
> > - !defined(CONFIG_HAVE_ARCH_EARLY_PFN_TO_NID)
> > +#ifndef CONFIG_NEED_MULTIPLE_NODES
> > static inline int early_pfn_to_nid(unsigned long pfn)
> > {
> > - BUILD_BUG_ON(IS_ENABLED(CONFIG_NUMA));
> > return 0;
> > }
> > #else
> > diff --git a/include/linux/mmzone.h b/include/linux/mmzone.h
> > index 7b5b6eba402f..ffc2a3d6036b 100644
> > --- a/include/linux/mmzone.h
> > +++ b/include/linux/mmzone.h
> > @@ -874,7 +874,7 @@ extern int movable_zone;
> > #ifdef CONFIG_HIGHMEM
> > static inline int zone_movable_is_highmem(void)
> > {
> > -#ifdef CONFIG_HAVE_MEMBLOCK_NODE_MAP
> > +#ifdef CONFIG_NEED_MULTIPLE_NODES
> > return movable_zone == ZONE_HIGHMEM;
> > #else
> > return (ZONE_MOVABLE - 1) == ZONE_HIGHMEM;
>
> If CONFIG_HIGHMEM is enabled, the above judgement is always true,
> wondering what's the purpose we have to do like this. It's not related
> to this patch though.
The whole is_higmem() implementation looks scary and begs for cleanup :)
> > diff --git a/mm/Kconfig b/mm/Kconfig index
> > c1acc34c1c35..aaa5bdaa1c8a 100644 --- a/mm/Kconfig
> > +++ b/mm/Kconfig
> > @@ -126,9 +126,6 @@ config SPARSEMEM_VMEMMAP
> > pfn_to_page and page_to_pfn operations. This is the most
> > efficient option when sufficient kernel resources are available.
> >
> > -config HAVE_MEMBLOCK_NODE_MAP
> > - bool
> > -
> > config HAVE_MEMBLOCK_PHYS_MAP
> > bool
> >
> > diff --git a/mm/memblock.c b/mm/memblock.c
> > index 43e2fd3006c1..743659d88fc4 100644
> > --- a/mm/memblock.c
> > +++ b/mm/memblock.c
> > @@ -620,7 +620,7 @@ static int __init_memblock memblock_add_range(struct memblock_type *type,
> > * area, insert that portion.
> > */
> > if (rbase > base) {
> > -#ifdef CONFIG_HAVE_MEMBLOCK_NODE_MAP
> > +#ifdef CONFIG_NEED_MULTIPLE_NODES
> > WARN_ON(nid != memblock_get_region_node(rgn));
> > #endif
> > WARN_ON(flags != rgn->flags);
> > @@ -1197,7 +1197,6 @@ void __init_memblock __next_mem_range_rev(u64 *idx, int nid,
> > *idx = ULLONG_MAX;
> > }
> >
> > -#ifdef CONFIG_HAVE_MEMBLOCK_NODE_MAP
> > /*
> > * Common iterator interface used to define for_each_mem_pfn_range().
> > */
> > @@ -1247,6 +1246,7 @@ void __init_memblock __next_mem_pfn_range(int *idx, int nid,
> > int __init_memblock memblock_set_node(phys_addr_t base, phys_addr_t size,
> > struct memblock_type *type, int nid)
> > {
> > +#ifdef CONFIG_NEED_MULTIPLE_NODES
> > int start_rgn, end_rgn;
> > int i, ret;
> >
> > @@ -1258,9 +1258,10 @@ int __init_memblock memblock_set_node(phys_addr_t base, phys_addr_t size,
> > memblock_set_region_node(&type->regions[i], nid);
> >
> > memblock_merge_regions(type);
> > +#endif
> > return 0;
> > }
> > -#endif /* CONFIG_HAVE_MEMBLOCK_NODE_MAP */
> > +
> > #ifdef CONFIG_DEFERRED_STRUCT_PAGE_INIT
> > /**
> > * __next_mem_pfn_range_in_zone - iterator for for_each_*_range_in_zone()
> > @@ -1799,7 +1800,6 @@ bool __init_memblock memblock_is_map_memory(phys_addr_t addr)
> > return !memblock_is_nomap(&memblock.memory.regions[i]);
> > }
> >
> > -#ifdef CONFIG_HAVE_MEMBLOCK_NODE_MAP
> > int __init_memblock memblock_search_pfn_nid(unsigned long pfn,
> > unsigned long *start_pfn, unsigned long *end_pfn)
> > {
> > @@ -1814,7 +1814,6 @@ int __init_memblock memblock_search_pfn_nid(unsigned long pfn,
> >
> > return memblock_get_region_node(&type->regions[mid]);
> > }
> > -#endif
> >
> > /**
> > * memblock_is_region_memory - check if a region is a subset of memory
> > @@ -1905,7 +1904,7 @@ static void __init_memblock memblock_dump(struct memblock_type *type)
> > size = rgn->size;
> > end = base + size - 1;
> > flags = rgn->flags;
> > -#ifdef CONFIG_HAVE_MEMBLOCK_NODE_MAP
> > +#ifdef CONFIG_NEED_MULTIPLE_NODES
> > if (memblock_get_region_node(rgn) != MAX_NUMNODES)
> > snprintf(nid_buf, sizeof(nid_buf), " on node %d",
> > memblock_get_region_node(rgn));
> > 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.
> > @@ -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.
> > 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 02/21] mm: make early_pfn_to_nid() and related defintions close to each other
From: Mike Rapoport @ 2020-04-21 8:49 UTC (permalink / raw)
To: Baoquan He
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: <20200421022435.GP4247@MiWiFi-R3L-srv>
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.
> > #else
> > diff --git a/include/linux/mmzone.h b/include/linux/mmzone.h
> > index 1b9de7d220fb..7b5b6eba402f 100644
> > --- a/include/linux/mmzone.h
> > +++ b/include/linux/mmzone.h
> > @@ -1078,15 +1078,6 @@ static inline struct zoneref *first_zones_zonelist(struct zonelist *zonelist,
> > #include <asm/sparsemem.h>
> > #endif
> >
> > -#if !defined(CONFIG_HAVE_ARCH_EARLY_PFN_TO_NID) && \
> > - !defined(CONFIG_HAVE_MEMBLOCK_NODE_MAP)
> > -static inline unsigned long early_pfn_to_nid(unsigned long pfn)
> > -{
> > - BUILD_BUG_ON(IS_ENABLED(CONFIG_NUMA));
> > - return 0;
> > -}
> > -#endif
> > -
> > #ifdef CONFIG_FLATMEM
> > #define pfn_to_nid(pfn) (0)
> > #endif
> > diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> > index 0d012eda1694..1ac775bfc9cf 100644
> > --- a/mm/page_alloc.c
> > +++ b/mm/page_alloc.c
> > @@ -1504,6 +1504,31 @@ void __free_pages_core(struct page *page, unsigned int order)
>
> #if defined(CONFIG_HAVE_ARCH_EARLY_PFN_TO_NID) || \
> defined(CONFIG_HAVE_MEMBLOCK_NODE_MAP)
>
> This is the upper layer of ifdeffery scope.
> >
> > static struct mminit_pfnnid_cache early_pfnnid_cache __meminitdata;
> >
> > +#ifndef CONFIG_HAVE_ARCH_EARLY_PFN_TO_NID
>
> Moving __early_pfn_to_nid() here makes the upper layer of ifdeferry
> scope a little werid. But seems no better way to optimize it.
It gets a bit better after patch 3 :)
> Otherwise, this patch looks good to me.
>
> Reviewed-by: Baoquan He <bhe@redhat.com>
Thanks!
> > +
> > +/*
> > + * Required by SPARSEMEM. Given a PFN, return what node the PFN is on.
> > + */
> > +int __meminit __early_pfn_to_nid(unsigned long pfn,
> > + struct mminit_pfnnid_cache *state)
> > +{
> > + unsigned long start_pfn, end_pfn;
> > + int nid;
> > +
> > + if (state->last_start <= pfn && pfn < state->last_end)
> > + return state->last_nid;
> > +
> > + nid = memblock_search_pfn_nid(pfn, &start_pfn, &end_pfn);
> > + if (nid != NUMA_NO_NODE) {
> > + state->last_start = start_pfn;
> > + state->last_end = end_pfn;
> > + state->last_nid = nid;
> > + }
> > +
> > + return nid;
> > +}
> > +#endif /* CONFIG_HAVE_ARCH_EARLY_PFN_TO_NID */
> > +
> > int __meminit early_pfn_to_nid(unsigned long pfn)
> > {
> > static DEFINE_SPINLOCK(early_pfn_lock);
> > @@ -6298,32 +6323,6 @@ void __meminit init_currently_empty_zone(struct zone *zone,
> > zone->initialized = 1;
> > }
> >
> > -#ifdef CONFIG_HAVE_MEMBLOCK_NODE_MAP
> > -#ifndef CONFIG_HAVE_ARCH_EARLY_PFN_TO_NID
> > -
> > -/*
> > - * Required by SPARSEMEM. Given a PFN, return what node the PFN is on.
> > - */
> > -int __meminit __early_pfn_to_nid(unsigned long pfn,
> > - struct mminit_pfnnid_cache *state)
> > -{
> > - unsigned long start_pfn, end_pfn;
> > - int nid;
> > -
> > - if (state->last_start <= pfn && pfn < state->last_end)
> > - return state->last_nid;
> > -
> > - nid = memblock_search_pfn_nid(pfn, &start_pfn, &end_pfn);
> > - if (nid != NUMA_NO_NODE) {
> > - state->last_start = start_pfn;
> > - state->last_end = end_pfn;
> > - state->last_nid = nid;
> > - }
> > -
> > - return nid;
> > -}
> > -#endif /* CONFIG_HAVE_ARCH_EARLY_PFN_TO_NID */
> > -
> > /**
> > * free_bootmem_with_active_regions - Call memblock_free_early_nid for each active range
> > * @nid: The node to free memory on. If MAX_NUMNODES, all nodes are freed.
> > --
> > 2.25.1
> >
>
--
Sincerely yours,
Mike.
^ permalink raw reply
* [PATCH] ASoC: fsl_esai: Remove expensive print in irq handler
From: Shengjiu Wang @ 2020-04-21 8:41 UTC (permalink / raw)
To: timur, nicoleotsuka, Xiubo.Lee, festevam, broonie, perex, tiwai,
alsa-devel
Cc: linuxppc-dev, linux-kernel
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.
Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
---
sound/soc/fsl/fsl_esai.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/sound/soc/fsl/fsl_esai.c b/sound/soc/fsl/fsl_esai.c
index c7a49d03463a..453392a9cd68 100644
--- a/sound/soc/fsl/fsl_esai.c
+++ b/sound/soc/fsl/fsl_esai.c
@@ -94,10 +94,10 @@ static irqreturn_t esai_isr(int irq, void *devid)
dev_dbg(&pdev->dev, "isr: Transmission Initialized\n");
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");
if (esr & ESAI_ESR_TLS_MASK)
dev_dbg(&pdev->dev, "isr: Just transmitted the last slot\n");
--
2.21.0
^ permalink raw reply related
* Re: [PATCH 02/21] mm: make early_pfn_to_nid() and related defintions close to each other
From: Mike Rapoport @ 2020-04-21 8:39 UTC (permalink / raw)
To: Baoquan He
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: <20200421033114.GA29658@MiWiFi-R3L-srv>
On Tue, Apr 21, 2020 at 11:31:14AM +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(-)
...
> > int __meminit early_pfn_to_nid(unsigned long pfn)
> > {
> > static DEFINE_SPINLOCK(early_pfn_lock);
> > @@ -6298,32 +6323,6 @@ void __meminit init_currently_empty_zone(struct zone *zone,
> > zone->initialized = 1;
> > }
> >
> > -#ifdef CONFIG_HAVE_MEMBLOCK_NODE_MAP
>
> Here it's apparently removing CONFIG_HAVE_MEMBLOCK_NODE_MAP too early,
> it should be done in patch 3, and its #end is kept there. I just found
> it when I almost became dizzy in reviewing patch 3.
Right, thanks for catching!
> > -#ifndef CONFIG_HAVE_ARCH_EARLY_PFN_TO_NID
> > -
--
Sincerely yours,
Mike.
^ permalink raw reply
* Re: ppc64 early slub caches have zero random value
From: Nicolai Stange @ 2020-04-21 8:39 UTC (permalink / raw)
To: Vlastimil Babka
Cc: Jason A. Donenfeld, Theodore Ts'o, Kees Cook, linux-kernel,
linux-mm, Andy Lutomirski, Michal Suchánek, linuxppc-dev
In-Reply-To: <8c93960b-587e-a576-91b8-666f106f8b60@suse.cz>
Hi
[adding some drivers/char/random folks + LKML to CC]
Vlastimil Babka <vbabka@suse.cz> writes:
> On 4/17/20 6:53 PM, Michal Suchánek wrote:
>> Hello,
>
> Hi, thanks for reproducing on latest upstream!
>
>> instrumenting the kernel with the following patch
>>
>> ---
>> mm/slub.c | 1 +
>> 1 file changed, 1 insertion(+)
>>
>> diff --git a/mm/slub.c b/mm/slub.c
>> index d6787bbe0248..d40995d5f8ff 100644
>> --- a/mm/slub.c
>> +++ b/mm/slub.c
>> @@ -3633,6 +3633,7 @@ static int kmem_cache_open(struct kmem_cache *s, slab_flags_t flags)
>> s->flags = kmem_cache_flags(s->size, flags, s->name, s->ctor);
>> #ifdef CONFIG_SLAB_FREELIST_HARDENED
>> s->random = get_random_long();
>> + pr_notice("Creating cache %s with s->random=%ld\n", s->name, s->random);
>> #endif
>>
>> if (!calculate_sizes(s, -1))
>>
>> I get:
>>
>> [ 0.000000] random: get_random_u64 called from kmem_cache_open+0x3c/0x5b0
> with crng_init=0
>> [ 0.000000] Creating cache kmem_cache_node with s->random=0
>> [ 0.000000] Creating cache kmem_cache with s->random=0
>> [ 0.000000] Creating cache kmalloc-8 with s->random=0
>> [ 0.000000] Creating cache kmalloc-16 with s->random=0
>> [ 0.000000] Creating cache kmalloc-32 with s->random=0
>> [ 0.000000] Creating cache kmalloc-64 with s->random=0
>> [ 0.000000] Creating cache kmalloc-96 with s->random=0
>> [ 0.000000] Creating cache kmalloc-128 with s->random=0
>> [ 0.000000] Creating cache kmalloc-192 with s->random=-682532147323126958
>>
>> The earliest caches created invariably end up with s->random of zero.
>
> It seems that reliably it's the first 8 calls get_random_u64(), which sounds
> more like some off-by-X bug than a genuine lack entropy that would become fixed
> in the meanwhile?
>
>> This is a problem for crash which does not recognize these as randomized
>> and fails to read them. While this can be addressed in crash is it
>> intended to create caches with zero random value in the kernel?
>
> Definitely not. The question is more likely what guarantees we have with
> crng_init=0. Probably we can't expect cryptographically strong randomness, but
> zeroes still do look like a bug to me?
>
>> This is broken at least in the 5.4~5.7 range but it is not clear if this
>> ever worked. All examples of earlier kernels I have at hand use slab mm.
>>
>> Thanks
>>
>> Michal
>>
FWIW, I've seen something similar in a slightly different context,
c.f. [1].
Basically, the issue is that on anything but x86_64 (and perhaps arm64
IIRC), arch_get_random_long() is unavailable and thus, get_random_u64()
falls through to that batched extract_crng() extraction. That is, it
extracts eight random longs from the chacha20 based RNG at once and
batches them up for consumption by the current and subsequent
get_random_u64() invocations. Which is in line with your observation
that get_random_u64() returned zero exactly eight times in a row.
The fact that extract_crng() actually extracted eight successive zero
values surprised me though. But from looking at chacha20_block(), called
from _extract_crng() with the primary_crng instance's state buffer as
input, it seems like a zeroed state buffer gets identity transformed and
that all this fancy shifting and rolling and whatnot in chacha_permute()
would have no effect at all. So I suppose that the primary_crng's state
buffer is still zeroed out at that point during boot.
Thanks,
Nicolai
[1] https://lkml.kernel.org/r/87d08rbbg9.fsf@suse.de
--
SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg), GF: Felix Imendörffer
^ permalink raw reply
* [PATCH v2 2/2] clk: qoriq: add cpufreq platform device
From: Mian Yousaf Kaukab @ 2020-04-21 8:30 UTC (permalink / raw)
To: linux-pm, andy.tang, sboyd, linux-clk, rjw, shawnguo, leoyang.li
Cc: Mian Yousaf Kaukab, viresh.kumar, linuxppc-dev, linux-kernel,
linux-arm-kernel
In-Reply-To: <20200421083000.16740-1-ykaukab@suse.de>
Add a platform device for qoirq-cpufreq driver for the compatible
clockgen blocks.
Reviewed-by: Yuantian Tang <andy.tang@nxp.com>
Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
Signed-off-by: Mian Yousaf Kaukab <ykaukab@suse.de>
---
v2:
+Rafael, Stephen, linux-clk
Add Reviewed-by and Acked-by tags
drivers/clk/clk-qoriq.c | 30 +++++++++++++++++++++++++++---
1 file changed, 27 insertions(+), 3 deletions(-)
diff --git a/drivers/clk/clk-qoriq.c b/drivers/clk/clk-qoriq.c
index d5946f7486d6..374afcab89af 100644
--- a/drivers/clk/clk-qoriq.c
+++ b/drivers/clk/clk-qoriq.c
@@ -95,6 +95,7 @@ struct clockgen {
};
static struct clockgen clockgen;
+static bool add_cpufreq_dev __initdata;
static void cg_out(struct clockgen *cg, u32 val, u32 __iomem *reg)
{
@@ -1019,7 +1020,7 @@ static void __init create_muxes(struct clockgen *cg)
}
}
-static void __init clockgen_init(struct device_node *np);
+static void __init _clockgen_init(struct device_node *np, bool legacy);
/*
* Legacy nodes may get probed before the parent clockgen node.
@@ -1030,7 +1031,7 @@ static void __init clockgen_init(struct device_node *np);
static void __init legacy_init_clockgen(struct device_node *np)
{
if (!clockgen.node)
- clockgen_init(of_get_parent(np));
+ _clockgen_init(of_get_parent(np), true);
}
/* Legacy node */
@@ -1447,7 +1448,7 @@ static bool __init has_erratum_a4510(void)
}
#endif
-static void __init clockgen_init(struct device_node *np)
+static void __init _clockgen_init(struct device_node *np, bool legacy)
{
int i, ret;
bool is_old_ls1021a = false;
@@ -1516,12 +1517,35 @@ static void __init clockgen_init(struct device_node *np)
__func__, np, ret);
}
+ /* Don't create cpufreq device for legacy clockgen blocks */
+ add_cpufreq_dev = !legacy;
+
return;
err:
iounmap(clockgen.regs);
clockgen.regs = NULL;
}
+static void __init clockgen_init(struct device_node *np)
+{
+ _clockgen_init(np, false);
+}
+
+static int __init clockgen_cpufreq_init(void)
+{
+ struct platform_device *pdev;
+
+ if (add_cpufreq_dev) {
+ pdev = platform_device_register_simple("qoriq-cpufreq", -1,
+ NULL, 0);
+ if (IS_ERR(pdev))
+ pr_err("Couldn't register qoriq-cpufreq err=%ld\n",
+ PTR_ERR(pdev));
+ }
+ return 0;
+}
+device_initcall(clockgen_cpufreq_init);
+
CLK_OF_DECLARE(qoriq_clockgen_1, "fsl,qoriq-clockgen-1.0", clockgen_init);
CLK_OF_DECLARE(qoriq_clockgen_2, "fsl,qoriq-clockgen-2.0", clockgen_init);
CLK_OF_DECLARE(qoriq_clockgen_b4420, "fsl,b4420-clockgen", clockgen_init);
--
2.16.4
^ permalink raw reply related
* [PATCH v2 1/2] cpufreq: qoriq: convert to a platform driver
From: Mian Yousaf Kaukab @ 2020-04-21 8:29 UTC (permalink / raw)
To: linux-pm, andy.tang, sboyd, linux-clk, rjw, shawnguo, leoyang.li
Cc: Mian Yousaf Kaukab, viresh.kumar, linuxppc-dev, linux-kernel,
linux-arm-kernel
The driver has to be manually loaded if it is built as a module. It
is neither exporting MODULE_DEVICE_TABLE nor MODULE_ALIAS. Moreover,
no platform-device is created (and thus no uevent is sent) for the
clockgen nodes it depends on.
Convert the module to a platform driver with its own alias. Moreover,
drop whitelisted SOCs. Platform device will be created only for the
compatible platforms.
Reviewed-by: Yuantian Tang <andy.tang@nxp.com>
Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
Signed-off-by: Mian Yousaf Kaukab <ykaukab@suse.de>
---
v2:
+Rafael, Stephen, linux-clk
Add Reviewed-by and Acked-by tags
drivers/cpufreq/qoriq-cpufreq.c | 76 ++++++++++++++++-------------------------
1 file changed, 29 insertions(+), 47 deletions(-)
diff --git a/drivers/cpufreq/qoriq-cpufreq.c b/drivers/cpufreq/qoriq-cpufreq.c
index 8e436dc75c8b..6b6b20da2bcf 100644
--- a/drivers/cpufreq/qoriq-cpufreq.c
+++ b/drivers/cpufreq/qoriq-cpufreq.c
@@ -18,6 +18,7 @@
#include <linux/of.h>
#include <linux/slab.h>
#include <linux/smp.h>
+#include <linux/platform_device.h>
/**
* struct cpu_data
@@ -29,12 +30,6 @@ struct cpu_data {
struct cpufreq_frequency_table *table;
};
-/*
- * Don't use cpufreq on this SoC -- used when the SoC would have otherwise
- * matched a more generic compatible.
- */
-#define SOC_BLACKLIST 1
-
/**
* struct soc_data - SoC specific data
* @flags: SOC_xxx
@@ -264,64 +259,51 @@ static struct cpufreq_driver qoriq_cpufreq_driver = {
.attr = cpufreq_generic_attr,
};
-static const struct soc_data blacklist = {
- .flags = SOC_BLACKLIST,
-};
-
-static const struct of_device_id node_matches[] __initconst = {
+static const struct of_device_id qoriq_cpufreq_blacklist[] = {
/* e6500 cannot use cpufreq due to erratum A-008083 */
- { .compatible = "fsl,b4420-clockgen", &blacklist },
- { .compatible = "fsl,b4860-clockgen", &blacklist },
- { .compatible = "fsl,t2080-clockgen", &blacklist },
- { .compatible = "fsl,t4240-clockgen", &blacklist },
-
- { .compatible = "fsl,ls1012a-clockgen", },
- { .compatible = "fsl,ls1021a-clockgen", },
- { .compatible = "fsl,ls1028a-clockgen", },
- { .compatible = "fsl,ls1043a-clockgen", },
- { .compatible = "fsl,ls1046a-clockgen", },
- { .compatible = "fsl,ls1088a-clockgen", },
- { .compatible = "fsl,ls2080a-clockgen", },
- { .compatible = "fsl,lx2160a-clockgen", },
- { .compatible = "fsl,p4080-clockgen", },
- { .compatible = "fsl,qoriq-clockgen-1.0", },
- { .compatible = "fsl,qoriq-clockgen-2.0", },
+ { .compatible = "fsl,b4420-clockgen", },
+ { .compatible = "fsl,b4860-clockgen", },
+ { .compatible = "fsl,t2080-clockgen", },
+ { .compatible = "fsl,t4240-clockgen", },
{}
};
-static int __init qoriq_cpufreq_init(void)
+static int qoriq_cpufreq_probe(struct platform_device *pdev)
{
int ret;
- struct device_node *np;
- const struct of_device_id *match;
- const struct soc_data *data;
-
- np = of_find_matching_node(NULL, node_matches);
- if (!np)
- return -ENODEV;
-
- match = of_match_node(node_matches, np);
- data = match->data;
-
- of_node_put(np);
+ struct device_node *np;
- if (data && data->flags & SOC_BLACKLIST)
+ np = of_find_matching_node(NULL, qoriq_cpufreq_blacklist);
+ if (np) {
+ dev_info(&pdev->dev, "Disabling due to erratum A-008083");
return -ENODEV;
+ }
ret = cpufreq_register_driver(&qoriq_cpufreq_driver);
- if (!ret)
- pr_info("Freescale QorIQ CPU frequency scaling driver\n");
+ if (ret)
+ return ret;
- return ret;
+ dev_info(&pdev->dev, "Freescale QorIQ CPU frequency scaling driver\n");
+ return 0;
}
-module_init(qoriq_cpufreq_init);
-static void __exit qoriq_cpufreq_exit(void)
+static int qoriq_cpufreq_remove(struct platform_device *pdev)
{
cpufreq_unregister_driver(&qoriq_cpufreq_driver);
+
+ return 0;
}
-module_exit(qoriq_cpufreq_exit);
+static struct platform_driver qoriq_cpufreq_platform_driver = {
+ .driver = {
+ .name = "qoriq-cpufreq",
+ },
+ .probe = qoriq_cpufreq_probe,
+ .remove = qoriq_cpufreq_remove,
+};
+module_platform_driver(qoriq_cpufreq_platform_driver);
+
+MODULE_ALIAS("platform:qoriq-cpufreq");
MODULE_LICENSE("GPL");
MODULE_AUTHOR("Tang Yuantian <Yuantian.Tang@freescale.com>");
MODULE_DESCRIPTION("cpufreq driver for Freescale QorIQ series SoCs");
--
2.16.4
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox