Re: [PATCH RFC V2 2/6] hwmon: Add support for RPi voltage sensor

[Robin Murphy]

On 22/05/18 20:31, Stefan Wahren wrote:
[...]

+static int rpi_hwmon_probe(struct platform_device *pdev)
+{
+   struct device *dev = >dev;
+   struct rpi_hwmon_data *data;
+   int ret;
+
+   data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
+   if (!data)
+   return -ENOMEM;
+
+   data->fw = platform_get_drvdata(to_platform_device(dev->parent));
+   if (!data->fw)
+   return -EPROBE_DEFER;
+


I am a bit at loss here (and sorry I didn't bring this up before).
How would this ever be possible, given that the driver is registered
from the firmware driver ?


Do you refer to the (wrong) return code, the assumption that the parent must be 
a platform driver or a possible race?



The return code is one thing. My question was how the driver would ever be 
instantiated
with platform_get_drvdata(to_platform_device(dev->parent)) == NULL (but 
dev->parent != NULL),
so I referred to the race. But, sure, a second question would be how that would 
indicate
that the parent is not instantiated yet (which by itself seems like an odd 
question).


This shouldn't happen and worth a log error. In patch #3 the registration is 
called after the complete private data of the firmware driver is initialized. 
Did i missed something?

But i must confess that i didn't test all builtin/module combinations.


The point is that, by construction, a "raspberrypi-hwmon" device will 
only ever be created for this driver to bind to if the firmware device 
is both fully initialised and known to support the GET_THROTTLED call 
already. Thus trying to check those again from the hwmon driver is at 
best pointless, and at worst misleading. If somebody *does* manage to 
bind this driver to some random inappropriate device, you've still got 
no guarantee that dev->parent is valid or that 
dev_get_drvdata(dev->parent)) won't return something non-NULL that isn't 
a struct rpi_firmware pointer, at which point you're liable to pass the 
paranoid check yet still crash anyway.


IOW, you can't reasonably defend against incorrect operation, and under 
correct operation there's nothing to defend against, so either way it's 
pretty futile to waste effort trying.


Robin.
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 2/2] ThunderX2: Add Cavium ThunderX2 SoC UNCORE PMU driver

[Robin Murphy]

Hi Kim,

On 04/05/18 01:30, Kim Phillips wrote:

On Tue, 1 May 2018 12:54:05 +0100
Will Deacon  wrote:


Hi Kim,


Hi Will, thanks for responding.


On Fri, Apr 27, 2018 at 11:56:25AM -0500, Kim Phillips wrote:

On Fri, 27 Apr 2018 17:09:14 +0100
Will Deacon  wrote:

On Fri, Apr 27, 2018 at 10:46:29AM -0500, Kim Phillips wrote:

On Fri, 27 Apr 2018 15:37:20 +0100
Will Deacon  wrote:

For anything under drivers/perf/, I'd prefer not to have these prints
and instead see efforts to improve error reporting via the perf system
call interface.


We'd all prefer that, and for all PMU drivers, why should ones under
drivers/perf be treated differently?


Because they're the ones I maintain...


You represent a minority on your opinion on this matter though.


I'm not sure that's true


I haven't seen another arch/PMU driver maintainer actively oppose it
other than you (and Mark).


-- you tend to be the only one raising this issue;


If you're basing that on list activity, I wouldn't, since most Arm perf
users don't subscribe to LKML.  I'm trying to represent current and
future Arm perf users that shouldn't be expected to hang out here on
LKML and review PMU drivers.  Couldn't tell you why PMU driver authors
themselves don't chime in, but will note that some drivers in
drivers/perf do print things from their event_init functions.


As a PMU driver author who *has* already invested not-inconsiderable 
time and effort in trying to explain to you the technical issues from a 
non-maintainer perspective (and is about to do so again), I can't help 
but feel rather slighted by that comment.



Also, you have been cc'd on off-list email threads where SPE users had
to debug the SPE driver's event_init() manually.  There are other SPE
users I know that have had to go through the same painstaking process of
debugging the SPE driver.  Last but not least, there's me, and I'd sure
appreciate more verbose PMU drivers, based on my real-life performance
monitoring experience(s).


I think most people don't actually care one way or the other. If you know of


Like I said, I think you're limiting your audience to LKML subscribers,
who are more amenable to being convinced they need to debug their
kernel.


others who care about it too then I suggest you work together as a group to
solve the problem in a way which is amenable to the upstream maintainers.


I know a few off-list people that would like a more user-friendly Arm
perf experience, but we're not qualified to solve the larger problem
that perf has had since its inception, and I think it's a little unfair
for you to suggest that, esp. given you're one of the maintainers, and
there's Linus on top of you.


Then you won't have to worry about fixing it personally. You could even
propose a topic for plumbers if there is enough interest in a general
framework for propagating error messages to userspace.


I'm not worried about fixing it personally or not.  I'm worried about
our perf users' experience given your objection to using the vehicle
already in use on other architectures and PMU drivers.

If you - or anyone else for that matter - know of a way that'll get us
past this, by all means, say something.

If it helps, I recently asked acme if we could borrow bits from struct
perf_event and got no response.


As you are already aware, I've personally tried to fix this problem -
that has existed since before the introduction of the perf tool (I
consider it a syscall-independent enhanced error interface), multiple
times, and failed.


Why is that my problem? Try harder?


It's your problem because we're here reviewing a patch that happens to
fall under your maintainership.  I'll be the first person to tell you
I'm obviously incompetent and haven't been able to come up with a
solution that is acceptable for everyone up to and including Linus
Torvalds.  I'm just noticing a chronic usability problem that can be
easily alleviated in the context of this patch review.


I don't see how incompetence has anything to do with it and, like most


Well you told me to try harder, and I'm trying to let you know that I
can try harder - ad infinitum - and still not get it, so telling me to
try harder isn't going to necessarily resolve the issue.


people (lucky gits...), I doubt Torvalds cares too deeply about PMU driver
internals. No arguments on the usability problem, but this ain't the fix


Like acme, I doubt Linus runs perf on real Arm h/w, so yes, I don't
think they care that much, but if they did try to do it one day, I'd
like to that they'd be able to have dmesg contain that extra error
information for them.

As you know, David Howells wrote a supplemental error syscall facility
[1], that Linus nacked (off list no less).  Does Linus care about what
David Howells was developing it for?  I don't know, but he does have
enough experience to know whether a certain approach to a problem is
sustainable in his kernel or not.


you're 

Re: [PATCH v3 06/10] drivers/peci: Add a PECI adapter driver for Aspeed AST24xx/AST25xx

[Robin Murphy]

Just a drive-by nit:

On 10/04/18 19:32, Jae Hyun Yoo wrote:
[...]

+#define PECI_CTRL_SAMPLING_MASK GENMASK(19, 16)
+#define PECI_CTRL_SAMPLING(x)   (((x) << 16) & PECI_CTRL_SAMPLING_MASK)
+#define PECI_CTRL_SAMPLING_GET(x)   (((x) & PECI_CTRL_SAMPLING_MASK) >> 16)


FWIW,  already provides functionality like this, so it 
might be worth taking a look at FIELD_{GET,PREP}() to save all these 
local definitions.


Robin.
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 2/2] mm: remove odd HAVE_PTE_SPECIAL

[Robin Murphy]

On 10/04/18 16:25, Laurent Dufour wrote:

Remove the additional define HAVE_PTE_SPECIAL and rely directly on
CONFIG_ARCH_HAS_PTE_SPECIAL.

There is no functional change introduced by this patch

Signed-off-by: Laurent Dufour 
---
  mm/memory.c | 23 ++-
  1 file changed, 10 insertions(+), 13 deletions(-)

diff --git a/mm/memory.c b/mm/memory.c
index 96910c625daa..53b6344a90d2 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -817,19 +817,13 @@ static void print_bad_pte(struct vm_area_struct *vma, 
unsigned long addr,
   * PFNMAP mappings in order to support COWable mappings.
   *
   */
-#ifdef CONFIG_ARCH_HAS_PTE_SPECIAL
-# define HAVE_PTE_SPECIAL 1
-#else
-# define HAVE_PTE_SPECIAL 0
-#endif
  struct page *_vm_normal_page(struct vm_area_struct *vma, unsigned long addr,
 pte_t pte, bool with_public_device)
  {
unsigned long pfn = pte_pfn(pte);
  
-	if (HAVE_PTE_SPECIAL) {

-   if (likely(!pte_special(pte)))
-   goto check_pfn;
+#ifdef CONFIG_ARCH_HAS_PTE_SPECIAL


Nit: Couldn't you use IS_ENABLED(CONFIG_ARCH_HAS_PTE_SPECIAL) within the 
existing code structure to avoid having to add these #ifdefs?


Robin.


+   if (unlikely(pte_special(pte))) {
if (vma->vm_ops && vma->vm_ops->find_special_page)
return vma->vm_ops->find_special_page(vma, addr);
if (vma->vm_flags & (VM_PFNMAP | VM_MIXEDMAP))
@@ -862,7 +856,7 @@ struct page *_vm_normal_page(struct vm_area_struct *vma, 
unsigned long addr,
return NULL;
}
  
-	/* !HAVE_PTE_SPECIAL case follows: */

+#else  /* CONFIG_ARCH_HAS_PTE_SPECIAL */
  
  	if (unlikely(vma->vm_flags & (VM_PFNMAP|VM_MIXEDMAP))) {

if (vma->vm_flags & VM_MIXEDMAP) {
@@ -881,7 +875,8 @@ struct page *_vm_normal_page(struct vm_area_struct *vma, 
unsigned long addr,
  
  	if (is_zero_pfn(pfn))

return NULL;
-check_pfn:
+#endif /* CONFIG_ARCH_HAS_PTE_SPECIAL */
+
if (unlikely(pfn > highest_memmap_pfn)) {
print_bad_pte(vma, addr, pte, NULL);
return NULL;
@@ -891,7 +886,7 @@ struct page *_vm_normal_page(struct vm_area_struct *vma, 
unsigned long addr,
 * NOTE! We still have PageReserved() pages in the page tables.
 * eg. VDSO mappings can cause them to exist.
 */
-out:
+out: __maybe_unused
return pfn_to_page(pfn);
  }
  
@@ -904,7 +899,7 @@ struct page *vm_normal_page_pmd(struct vm_area_struct *vma, unsigned long addr,

/*
 * There is no pmd_special() but there may be special pmds, e.g.
 * in a direct-access (dax) mapping, so let's just replicate the
-* !HAVE_PTE_SPECIAL case from vm_normal_page() here.
+* !CONFIG_ARCH_HAS_PTE_SPECIAL case from vm_normal_page() here.
 */
if (unlikely(vma->vm_flags & (VM_PFNMAP|VM_MIXEDMAP))) {
if (vma->vm_flags & VM_MIXEDMAP) {
@@ -1926,6 +1921,7 @@ static int __vm_insert_mixed(struct vm_area_struct *vma, 
unsigned long addr,
  
  	track_pfn_insert(vma, , pfn);
  
+#ifndef CONFIG_ARCH_HAS_PTE_SPECIAL

/*
 * If we don't have pte special, then we have to use the pfn_valid()
 * based VM_MIXEDMAP scheme (see vm_normal_page), and thus we *must*
@@ -1933,7 +1929,7 @@ static int __vm_insert_mixed(struct vm_area_struct *vma, 
unsigned long addr,
 * than insert_pfn).  If a zero_pfn were inserted into a VM_MIXEDMAP
 * without pte special, it would there be refcounted as a normal page.
 */
-   if (!HAVE_PTE_SPECIAL && !pfn_t_devmap(pfn) && pfn_t_valid(pfn)) {
+   if (!pfn_t_devmap(pfn) && pfn_t_valid(pfn)) {
struct page *page;
  
  		/*

@@ -1944,6 +1940,7 @@ static int __vm_insert_mixed(struct vm_area_struct *vma, 
unsigned long addr,
page = pfn_to_page(pfn_t_to_pfn(pfn));
return insert_page(vma, addr, page, pgprot);
}
+#endif
return insert_pfn(vma, addr, pfn, pgprot, mkwrite);
  }
  


--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 2/4] soc: qcom: Add GENI based QUP Wrapper driver

[Robin Murphy]

On 08/03/18 06:46, Karthik Ramasubramanian wrote:



On 3/6/2018 2:56 PM, Stephen Boyd wrote:

Quoting Karthik Ramasubramanian (2018-03-02 16:58:23)


+   return iova;
+}
+EXPORT_SYMBOL(geni_se_tx_dma_prep);
+
+/**
+ * geni_se_rx_dma_prep() - Prepare the Serial Engine for RX DMA 
transfer
+ * @se:    Pointer to the concerned Serial 
Engine.

+ * @buf:   Pointer to the RX buffer.
+ * @len:   Length of the RX buffer.
+ *
+ * This function is used to prepare the buffers for DMA RX.
+ *
+ * Return: Mapped DMA Address of the buffer on success, NULL on 
failure.

+ */
+dma_addr_t geni_se_rx_dma_prep(struct geni_se *se, void *buf, 
size_t len)

+{
+   dma_addr_t iova;
+   struct geni_wrapper *wrapper = se->wrapper;
+   u32 val;
+
+   iova = dma_map_single(wrapper->dev, buf, len, 
DMA_FROM_DEVICE);

+   if (dma_mapping_error(wrapper->dev, iova))
+   return (dma_addr_t)NULL;


Can't return a dma_mapping_error address to the caller and have them
figure it out?

Earlier we used to return the DMA_ERROR_CODE which has been removed
recently in arm64 architecture. If we return the dma_mapping_error, then
the caller also needs the device which encountered the mapping error.
The serial interface drivers can use their parent currently to resolve
the mapping error. Once the wrapper starts mapping using IOMMU context
bank, then the serial interface drivers do not know which device to use
to know if there is an error.

Having said that, the dma_ops suggestion might help with handling this
situation. I will look into it further.


Ok, thanks.


+{
+   struct geni_wrapper *wrapper = se->wrapper;
+
+   if (iova)
+   dma_unmap_single(wrapper->dev, iova, len, 
DMA_FROM_DEVICE);

+}
+EXPORT_SYMBOL(geni_se_rx_dma_unprep);


Instead of having the functions exported, could we set the dma_ops on
all child devices of the wrapper that this driver populates and then
implement the DMA ops for those devices here? I assume that there's
never another DMA master between the wrapper and the serial engine, 
so I

think it would work.

This suggestion looks like it will work.


It would be a good idea to check with some other people on the dma_ops
suggestion. Maybe add the DMA mapping subsystem folks to help out here

I have added the DMA mapping subsystem folks to help out here.

To present an overview, we have a wrapper controller which is composed 
of several serial engines. The serial engines are programmed with UART, 
I2C or SPI protocol and support DMA transfer. When the serial engines 
perform DMA transfer, the wrapper controller device is used to perform 
the mapping. The reason wrapper device is used is because of IOMMU and 
there is only one IOMMU context bank to perform the translation for the 
entire wrapper controller. So the wrapper controller exports map and 
unmap functions to the individual protocol drivers.


There is a suggestion to make the parent wrapper controller implement 
the dma_map_ops, instead of exported map/unmap functions and populate 
those dma_map_ops on all the children serial engines. Can you please 
provide your inputs regarding this suggestion?


Implementing dma_map_ops inside a driver for real hardware is almost 
always the wrong thing to do.


Based on what I could infer about the hardware from looking through the 
whole series in the linux-arm-msm archive, this is probably more like a 
multi-channel DMA controller where each "channel" has a configurable 
serial interface on the other end, as opposed to an actual bus where the 
serial engines are individually distinct AHB masters routed through the 
wrapper. If that's true, then using the QUP platform device for DMA API 
calls is the appropriate thing to do. Personally I'd be inclined not to 
abstract the dma_{map,unmap} calls at all, and just have the protocol 
drivers make them directly using dev->parent/wrapper->dev/whatever, but 
if you do want to abstract those then just give the abstraction a saner 
interface, i.e. pass the DMA handle by reference and return a regular 
int for error/success status.


Robin.
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] tty: pl011: Work around QDF2400 E44 stuck BUSY bit

[Robin Murphy]

On 08/02/17 00:57, Christopher Covington wrote:
> The Qualcomm Datacenter Technologies QDF2400 family of SoCs contains a
> custom (non-PrimeCell) implementation of the SBSA UART. Occasionally the
> BUSY bit in the Flag Register gets stuck as 1, erratum 44 for both 2432v1
> and 2400v1 SoCs. Checking that the Transmit FIFO Empty (TXFE) bit is 0,
> instead of checking that the BUSY bit is 1, works around the issue. To
> facilitate this substitution when UART AMBA Port (UAP) data is available,
> introduce vendor-specific inversion of Feature Register bits. To keep the
> change small, this patch only works around the full console case, where UAP
> data is available, and does not handle the erratum for earlycon, as the UAP
> data is not available then.
> 
> Signed-off-by: Christopher Covington 
> Acked-by: Russell King 
> ---
> Changes between the previous RFC [1] and this PATCH:
> * don't use arch/arm64/kernel/cpu_errata.c at Will's request
> * separate out earlycon case to separate patch
> * rework earlycon case to not depend on UAP as suggested by Timur
> 
> Because they need a newly-defined MIDR values, the patches are currently
> based on:
> https://git.kernel.org/cgit/linux/kernel/git/arm64/linux.git/log/?h=for-next/core
> 
> I'm not confident that I know the best route for these two patches. Should
> I request Catalin and Will to take them via arm64 as the essential MIDR
> additions are their purview?
> 
> Thanks Russell for the ack. Compared to the RFC, I've made minor changes to
> what is now patch 1/2 and am making an educated guess that the ack sticks
> (but if not please let me know). Patch 2/2 is significantly revised from
> the RFC so I've not included the ack on it.
> 
> 1. https://patchwork.codeaurora.org/patch/163281/
> ---
>  Documentation/arm64/silicon-errata.txt |  2 ++
>  arch/arm64/include/asm/cputype.h   |  2 ++
>  drivers/tty/serial/Kconfig | 12 ++
>  drivers/tty/serial/amba-pl011.c| 40 
> +++---
>  4 files changed, 53 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/arm64/silicon-errata.txt 
> b/Documentation/arm64/silicon-errata.txt
> index 50da8391e9dd..0993ebb3e86b 100644
> --- a/Documentation/arm64/silicon-errata.txt
> +++ b/Documentation/arm64/silicon-errata.txt
> @@ -65,3 +65,5 @@ stable kernels.
>  | Freescale/NXP  | LS2080A/LS1043A | A-008585| FSL_ERRATUM_A008585   
>   |
>  | Qualcomm Tech. | Falkor v1   | E1003   | 
> QCOM_FALKOR_ERRATUM_1003|
>  | Qualcomm Tech. | Falkor v1   | E1009   | 
> QCOM_FALKOR_ERRATUM_1009|
> +| Qualcomm Tech. | QDF2432v1 UART  | SoC E44 | 
> QCOM_QDF2400_ERRATUM_44 |
> +| Qualcomm Tech. | QDF2400v1 UART  | SoC E44 | 
> QCOM_QDF2400_ERRATUM_44 |
> diff --git a/arch/arm64/include/asm/cputype.h 
> b/arch/arm64/include/asm/cputype.h
> index fc502713ab37..cb399c7fe6ec 100644
> --- a/arch/arm64/include/asm/cputype.h
> +++ b/arch/arm64/include/asm/cputype.h
> @@ -88,12 +88,14 @@
>  
>  #define BRCM_CPU_PART_VULCAN 0x516
>  
> +#define QCOM_CPU_PART_KRYO_V10x281
>  #define QCOM_CPU_PART_FALKOR_V1  0x800
>  
>  #define MIDR_CORTEX_A53 MIDR_CPU_MODEL(ARM_CPU_IMP_ARM, 
> ARM_CPU_PART_CORTEX_A53)
>  #define MIDR_CORTEX_A57 MIDR_CPU_MODEL(ARM_CPU_IMP_ARM, 
> ARM_CPU_PART_CORTEX_A57)
>  #define MIDR_THUNDERXMIDR_CPU_MODEL(ARM_CPU_IMP_CAVIUM, 
> CAVIUM_CPU_PART_THUNDERX)
>  #define MIDR_THUNDERX_81XX MIDR_CPU_MODEL(ARM_CPU_IMP_CAVIUM, 
> CAVIUM_CPU_PART_THUNDERX_81XX)
> +#define MIDR_QCOM_KRYO_V1 MIDR_CPU_MODEL(ARM_CPU_IMP_QCOM, 
> QCOM_CPU_PART_KRYO_V1)
>  #define MIDR_QCOM_FALKOR_V1 MIDR_CPU_MODEL(ARM_CPU_IMP_QCOM, 
> QCOM_CPU_PART_FALKOR_V1)
>  
>  #ifndef __ASSEMBLY__
> diff --git a/drivers/tty/serial/Kconfig b/drivers/tty/serial/Kconfig
> index e9cf5b67f1b7..4cde8f48a540 100644
> --- a/drivers/tty/serial/Kconfig
> +++ b/drivers/tty/serial/Kconfig
> @@ -73,6 +73,18 @@ config SERIAL_AMBA_PL011_CONSOLE
> your boot loader (lilo or loadlin) about how to pass options to the
> kernel at boot time.)
>  
> +config QCOM_QDF2400_ERRATUM_44
> + bool "Work around QDF2400 SoC E44 stuck BUSY bit"
> + depends on SERIAL_AMBA_PL011_CONSOLE=y
> + default y
> + help
> +   The BUSY bit in the Flag Register of the UART on the QDF2432v1 and
> +   QDF2400v1 SoCs may get stuck as 1, resulting in a hung serial console.
> +   Say Y here to work around the issue by checking TXFE == 0 instead of
> +   BUSY == 1 on affected systems.
> +
> +   If unsure, say Y.

Is there a reason anyone would ever want to turn this off? AFAICS you
save a few dozen bytes in return for a kernel image which you know won't
work properly on some hardware. That doesn't seem particularly
worthwhile, and it's not like the PL011 driver isn't already ripe with
unconditional vendor-specific data.

> +
>  config SERIAL_EARLYCON_ARM_SEMIHOST
>   bool 

Re: [PATCH 2/2] security: Change name of CONFIG_DEBUG_SET_MODULE_RONX

[Robin Murphy]

Hi Laura,

On 19/01/17 01:29, Laura Abbott wrote:
> 
> Despite the word 'debug' in CONFIG_DEBUG_SET_MODULE_RONX, this kernel
> option provides key security features that are to be expected on a
> modern system. Change the name to CONFIG_HARDENED_MODULE_MAPPINGS which
> more accurately describes what this option is intended to do.
> 
> Signed-off-by: Laura Abbott 
> ---

[...]

> diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> index 09aff28..ef852e4 100644
> --- a/arch/arm/Kconfig
> +++ b/arch/arm/Kconfig
> @@ -8,6 +8,7 @@ config ARM
>   select ARCH_HAVE_CUSTOM_GPIO_H
>   select ARCH_HAS_GCOV_PROFILE_ALL
>   select ARCH_HAS_HARDENED_MAPPINGS if MMU && !XIP_KERNEL
> + select ARCH_HAS_HARDENED_MODULE_MAPPINGS if MMU
>   select ARCH_MIGHT_HAVE_PC_PARPORT
>   select ARCH_SUPPORTS_ATOMIC_RMW
>   select ARCH_USE_BUILTIN_BSWAP
> diff --git a/arch/arm/Kconfig.debug b/arch/arm/Kconfig.debug
> index d83f7c3..426d271 100644
> --- a/arch/arm/Kconfig.debug
> +++ b/arch/arm/Kconfig.debug
> @@ -1738,17 +1738,6 @@ config PID_IN_CONTEXTIDR
> additional instructions during context switch. Say Y here only if you
> are planning to use hardware trace tools with this kernel.
>  
> -config DEBUG_SET_MODULE_RONX
> - bool "Set loadable kernel module data as NX and text as RO"
> - depends on MODULES && MMU
> - ---help---
> -   This option helps catch unintended modifications to loadable
> -   kernel module's text and read-only data. It also prevents execution
> -   of module data. Such protection may interfere with run-time code
> -   patching and dynamic kernel tracing - and they might also protect
> -   against certain classes of kernel exploits.
> -   If in doubt, say "N".
> -
>  source "drivers/hwtracing/coresight/Kconfig"
>  
>  endmenu

[...]

> --- a/arch/arm64/Kconfig
> +++ b/arch/arm64/Kconfig
> @@ -12,6 +12,7 @@ config ARM64
>   select ARCH_HAS_GCOV_PROFILE_ALL
>   select ARCH_HAS_GIGANTIC_PAGE
>   select ARCH_HAS_HARDENED_MAPPINGS
> + select ARCH_HAS_HARDENED_MODULE_MAPPINGS
>   select ARCH_HAS_KCOV
>   select ARCH_HAS_SG_CHAIN
>   select ARCH_HAS_TICK_BROADCAST if GENERIC_CLOCKEVENTS_BROADCAST
> diff --git a/arch/arm64/Kconfig.debug b/arch/arm64/Kconfig.debug
> index a26d27f..1eebe1f 100644
> --- a/arch/arm64/Kconfig.debug
> +++ b/arch/arm64/Kconfig.debug
> @@ -71,17 +71,6 @@ config DEBUG_WX
>  
> If in doubt, say "Y".
>  
> -config DEBUG_SET_MODULE_RONX
> - bool "Set loadable kernel module data as NX and text as RO"
> - depends on MODULES
> - default y
> - help
> -   Is this is set, kernel module text and rodata will be made read-only.
> -   This is to help catch accidental or malicious attempts to change the
> -   kernel's executable code.
> -
> -   If in doubt, say Y.
> -
>  config DEBUG_ALIGN_RODATA
>   depends on ARCH_HAS_HARDENED_MAPPINGS
>   bool "Align linker sections up to SECTION_SIZE"

[...]

> --- a/arch/s390/Kconfig
> +++ b/arch/s390/Kconfig
> @@ -69,6 +69,7 @@ config S390
>   select ARCH_HAS_GCOV_PROFILE_ALL
>   select ARCH_HAS_GIGANTIC_PAGE
>   select ARCH_HAS_HARDENED_MAPPINGS
> + select ARCH_HAS_HARDENED_MODULE_MAPPINGS
>   select ARCH_HAS_KCOV
>   select ARCH_HAS_SG_CHAIN
>   select ARCH_HAS_UBSAN_SANITIZE_ALL
> diff --git a/arch/s390/Kconfig.debug b/arch/s390/Kconfig.debug
> index 26c5d5be..57f8ea9 100644
> --- a/arch/s390/Kconfig.debug
> +++ b/arch/s390/Kconfig.debug
> @@ -17,7 +17,4 @@ config S390_PTDUMP
> kernel.
> If in doubt, say "N"
>  
> -config DEBUG_SET_MODULE_RONX
> - def_bool y
> - depends on MODULES
>  endmenu
> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> index 9d80cd8..38ce850 100644
> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -51,6 +51,7 @@ config X86
>   select ARCH_HAS_FAST_MULTIPLIER
>   select ARCH_HAS_GCOV_PROFILE_ALL
>   select ARCH_HAS_HARDENED_MAPPINGS
> + select ARCH_HAS_HARDENED_MODULE_MAPPINGS
>   select ARCH_HAS_KCOVif X86_64
>   select ARCH_HAS_MMIO_FLUSH
>   select ARCH_HAS_PMEM_APIif X86_64
> diff --git a/arch/x86/Kconfig.debug b/arch/x86/Kconfig.debug
> index 67eec55..69cdd0b 100644
> --- a/arch/x86/Kconfig.debug
> +++ b/arch/x86/Kconfig.debug
> @@ -109,17 +109,6 @@ config DEBUG_WX
>  
> If in doubt, say "Y".
>  
> -config DEBUG_SET_MODULE_RONX
> - bool "Set loadable kernel module data as NX and text as RO"
> - depends on MODULES
> - ---help---
> -   This option helps catch unintended modifications to loadable
> -   kernel module's text and read-only data. It also prevents execution
> -   of module data. Such protection may interfere with run-time code
> -   patching and dynamic kernel tracing - and they might also protect
> -   against certain classes of kernel exploits.
> -   If in doubt, say "N".
> -
>  config 

Re: [PATCH 2/2] swiotlb: Add swiotlb=nobounce debug option

[Robin Murphy]

Hi Geert,

On 07/11/16 15:41, Geert Uytterhoeven wrote:
> Hi Robin,
> 
> On Tue, Nov 1, 2016 at 12:46 PM, Robin Murphy <robin.mur...@arm.com> wrote:
>>>>> To aid debugging and catch devices not supporting DMA to memory outside
>>>>> the 32-bit address space, add a kernel command line option
>>>>> "swiotlb=nobounce", which disables the use of bounce buffers.
>>>>> If specified, trying to map memory that cannot be used with DMA will
>>>>> fail, and a warning will be printed (rate-limited).
>>>>
>>>> This rationale seems questionable - how useful is non-deterministic
>>>> behaviour for debugging really? What you end up with is DMA sometimes
>>>> working or sometimes not depending on whether allocations happen to
>>>> naturally fall below 4GB or not. In my experience, that in itself can be
>>>> a pain in the arse to debug.
>>>
>>> It immediately triggered for me, though:
>>>
>>> rcar-dmac e730.dma-controller: Cannot do DMA to address
>>> 0x00067a9b7000
>>> ravb e680.ethernet: Cannot do DMA to address 0x00067aa07780
>>>
>>>> Most of the things you might then do to make things more deterministic
>>>> again (like making the default DMA mask tiny or hacking out all the
>>>> system's 32-bit addressable RAM) are also generally sufficient to make
>>>> DMA fail earlier and make this option moot anyway. What's the specific
>>>> use case motivating this?
>>>
>>> My use case is finding which drivers and DMA engines do not support 64-bit
>>> memory. There's more info in my series "[PATCH/RFC 0/5] arm64: r8a7796: 
>>> 64-bit
>>> Memory and Ethernet Prototype"
>>> (https://www.mail-archive.com/linux-renesas-soc@vger.kernel.org/msg08393.html)
>>
>> Thanks for the context. I've done very similar things in the past, and
>> my first instinct would be to change the default DMA mask in
>> of_dma_configure() to something which can't reach RAM (e.g. <30 bits),
>> then instrument dma_set_mask() to catch cleverer drivers. That's a
>> straightforward way to get 100% coverage - the problem with simply
>> disabling bounce buffering is that whilst statistically it almost
>> certainly will catch >95% of cases, there will always be some that it
>> won't; if some driver only ever does a single dma_alloc_coherent() early
>> enough that allocations are still fairly deterministic, and always
>> happens to get a 32-bit address on that platform, it's likely to slip
>> through the net.
>>
>> I'm not against the idea of SWIOTLB growing a runtime-disable option,
>> I'm just not sure what situation it's actually the best solution for.
> 
> If I set the DMA mask to a small value, DMA is never used, and SWIOTLB
> always falls back to bounce buffers (and DMAing from the small pool)?

Not quite - I meant setting the default mask to a value small enough
that any attempt to allocate or map within it (whether bounced or
otherwise) cannot possibly succeed. Of course, if you have RAM right
down to address 0 this becomes trickier - I'm not entirely certain that
going to extremes (DMA_BIT_MASK(1), say) wouldn't end up going wrong
somewhere for misleadingly unrelated reasons - but I got the impression
from the mainline DT that RAM on your platform starts at 0x4000,
hence the 30-bit suggestion.

At that point, you can instantly tell for certain that any driver for
which DMA starts failing is relying on the default mask and definitely
needs looking at for 64-bit support, and anything that doesn't just
needs a simple check of what it's passing to dma_set_*mask*().

Alternatively, the really bulletproof option is to get the firmware to
load the kernel into the >4GB area(s) of RAM (or hack dram_base in
handle_kernel_image() in arch/arm64/kernel/efi-stub.c) and simply remove
any 32-bit-addressable areas from the DT entirely (assuming the EFI
memory map isn't hard-coded). Then SWIOTLB becomes moot as there's
nowhere it can even allocate a usable bounce buffer.

> That's the inverse of what I want to achieve: I want to avoid using the
> bounce feature, to make sure the DMA engine is always used with whatever
> kind of memory.

As I said, it's that "always" that's the problem - there is already a
no-op path through the SWIOTLB code if the buffer happens to lie within
the device's DMA mask to start with, so just making SWIOTLB a no-op does
nothing to reveal anything sufficiently lucky to always hit that path on
current platforms.

Robin.

> 
> Gr{oetje,eeting}s,
> 
> Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- 
> ge...@linux-m68k.org
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like 
> that.
> -- Linus Torvalds
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] swiotlb: Add swiotlb=nobounce debug option

[Robin Murphy]

On 31/10/16 18:20, Geert Uytterhoeven wrote:
> Hi Robin,
> 
> On Mon, Oct 31, 2016 at 6:41 PM, Robin Murphy <robin.mur...@arm.com> wrote:
>> On 31/10/16 15:45, Geert Uytterhoeven wrote:
>>> On architectures like arm64, swiotlb is tied intimately to the core
>>> architecture DMA support. In addition, ZONE_DMA cannot be disabled.
>>
>> To be fair, that only takes a single-character change in
>> arch/arm64/Kconfig - in fact, I'm amused to see my stupid patch to fix
>> the build if you do just that (86a5906e4d1d) has just had its birthday ;)
> 
> Unfortunately it's not that simple. Using a small patch (based on Mark 
> Salter's
> "arm64: make CONFIG_ZONE_DMA user settable"), it appears to work. However:
>   - With CONFIG_ZONE_DMA=n and memory present over 4G, swiotlb_init() is
> not called.
> This will lead to a NULL pointer dereference later, when
> dma_map_single() calls into an unitialized SWIOTLB subsystem through
> swiotlb_tbl_map_single().
>   - With CONFIG_ZONE_DMA=n and no memory present over 4G, swiotlb_init()
> is also not called, but RAVB works fine.
> Disabling CONFIG_SWIOTLB is non-trivial, as the arm64 DMA core always
> uses swiotlb_dma_ops, and its operations depend a lot on SWIOTLB
> helpers.
> 
> So that's why I went for this option.

OK, that's new to me - I guess this behaviour was introduced by
b67a8b29df7e ("arm64: mm: only initialize swiotlb when necessary").
Regardless of this patch, that check probably wants fixing to still do
the appropriate thing if arm64_dma_phys_limit is above 4GB (or just
depend on ZONE_DMA). Disabling ZONE_DMA for development doesn't seem
that unreasonable a thing to do, especially if there are ready-made
patches floating around already, so having it crash the kernel in ways
it didn't before isn't ideal.

>>> To aid debugging and catch devices not supporting DMA to memory outside
>>> the 32-bit address space, add a kernel command line option
>>> "swiotlb=nobounce", which disables the use of bounce buffers.
>>> If specified, trying to map memory that cannot be used with DMA will
>>> fail, and a warning will be printed (rate-limited).
>>
>> This rationale seems questionable - how useful is non-deterministic
>> behaviour for debugging really? What you end up with is DMA sometimes
>> working or sometimes not depending on whether allocations happen to
>> naturally fall below 4GB or not. In my experience, that in itself can be
>> a pain in the arse to debug.
> 
> It immediately triggered for me, though:
> 
> rcar-dmac e730.dma-controller: Cannot do DMA to address
> 0x00067a9b7000
> ravb e680.ethernet: Cannot do DMA to address 0x00067aa07780
> 
>> Most of the things you might then do to make things more deterministic
>> again (like making the default DMA mask tiny or hacking out all the
>> system's 32-bit addressable RAM) are also generally sufficient to make
>> DMA fail earlier and make this option moot anyway. What's the specific
>> use case motivating this?
> 
> My use case is finding which drivers and DMA engines do not support 64-bit
> memory. There's more info in my series "[PATCH/RFC 0/5] arm64: r8a7796: 64-bit
> Memory and Ethernet Prototype"
> (https://www.mail-archive.com/linux-renesas-soc@vger.kernel.org/msg08393.html)

Thanks for the context. I've done very similar things in the past, and
my first instinct would be to change the default DMA mask in
of_dma_configure() to something which can't reach RAM (e.g. <30 bits),
then instrument dma_set_mask() to catch cleverer drivers. That's a
straightforward way to get 100% coverage - the problem with simply
disabling bounce buffering is that whilst statistically it almost
certainly will catch >95% of cases, there will always be some that it
won't; if some driver only ever does a single dma_alloc_coherent() early
enough that allocations are still fairly deterministic, and always
happens to get a 32-bit address on that platform, it's likely to slip
through the net.

I'm not against the idea of SWIOTLB growing a runtime-disable option,
I'm just not sure what situation it's actually the best solution for.

Robin.

> 
> Gr{oetje,eeting}s,
> 
> Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- 
> ge...@linux-m68k.org
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like 
> that.
> -- Linus Torvalds
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] swiotlb: Add swiotlb=nobounce debug option

[Robin Murphy]

Hi Geert,

On 31/10/16 15:45, Geert Uytterhoeven wrote:
> On architectures like arm64, swiotlb is tied intimately to the core
> architecture DMA support. In addition, ZONE_DMA cannot be disabled.

To be fair, that only takes a single-character change in
arch/arm64/Kconfig - in fact, I'm amused to see my stupid patch to fix
the build if you do just that (86a5906e4d1d) has just had its birthday ;)

> To aid debugging and catch devices not supporting DMA to memory outside
> the 32-bit address space, add a kernel command line option
> "swiotlb=nobounce", which disables the use of bounce buffers.
> If specified, trying to map memory that cannot be used with DMA will
> fail, and a warning will be printed (rate-limited).

This rationale seems questionable - how useful is non-deterministic
behaviour for debugging really? What you end up with is DMA sometimes
working or sometimes not depending on whether allocations happen to
naturally fall below 4GB or not. In my experience, that in itself can be
a pain in the arse to debug.

Most of the things you might then do to make things more deterministic
again (like making the default DMA mask tiny or hacking out all the
system's 32-bit addressable RAM) are also generally sufficient to make
DMA fail earlier and make this option moot anyway. What's the specific
use case motivating this?

Robin.

> Note that io_tlb_nslabs is set to 1, which is the minimal supported
> value.
> 
> Signed-off-by: Geert Uytterhoeven 
> ---
>  Documentation/kernel-parameters.txt |  3 ++-
>  lib/swiotlb.c   | 19 +--
>  2 files changed, 19 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/kernel-parameters.txt 
> b/Documentation/kernel-parameters.txt
> index 37babf91f2cb6de2..38556cdceabaf087 100644
> --- a/Documentation/kernel-parameters.txt
> +++ b/Documentation/kernel-parameters.txt
> @@ -3998,10 +3998,11 @@ bytes respectively. Such letter suffixes can also be 
> entirely omitted.
>   it if 0 is given (See 
> Documentation/cgroup-v1/memory.txt)
>  
>   swiotlb=[ARM,IA-64,PPC,MIPS,X86]
> - Format: {  | force }
> + Format: {  | force | nobounce }
>-- Number of I/O TLB slabs
>   force -- force using of bounce buffers even if they
>wouldn't be automatically used by the kernel
> + nobounce -- Never use bounce buffers (for debugging)
>  
>   switches=   [HW,M68k]
>  
> diff --git a/lib/swiotlb.c b/lib/swiotlb.c
> index 6ce764410ae475cc..4550e6b516c2a4c0 100644
> --- a/lib/swiotlb.c
> +++ b/lib/swiotlb.c
> @@ -54,6 +54,7 @@
>  #define IO_TLB_MIN_SLABS ((1<<20) >> IO_TLB_SHIFT)
>  
>  int swiotlb_force;
> +static int swiotlb_nobounce;
>  
>  /*
>   * Used to do a quick range check in swiotlb_tbl_unmap_single and
> @@ -106,8 +107,12 @@
>   }
>   if (*str == ',')
>   ++str;
> - if (!strcmp(str, "force"))
> + if (!strcmp(str, "force")) {
>   swiotlb_force = 1;
> + } else if (!strcmp(str, "nobounce")) {
> + swiotlb_nobounce = 1;
> + io_tlb_nslabs = 1;
> + }
>  
>   return 0;
>  }
> @@ -541,8 +546,15 @@ phys_addr_t swiotlb_tbl_map_single(struct device *hwdev,
>  map_single(struct device *hwdev, phys_addr_t phys, size_t size,
>  enum dma_data_direction dir)
>  {
> - dma_addr_t start_dma_addr = phys_to_dma(hwdev, io_tlb_start);
> + dma_addr_t start_dma_addr;
> +
> + if (swiotlb_nobounce) {
> + dev_warn_ratelimited(hwdev, "Cannot do DMA to address %pa\n",
> +  );
> + return SWIOTLB_MAP_ERROR;
> + }
>  
> + start_dma_addr = phys_to_dma(hwdev, io_tlb_start);
>   return swiotlb_tbl_map_single(hwdev, start_dma_addr, phys, size, dir);
>  }
>  
> @@ -707,6 +719,9 @@ void swiotlb_tbl_sync_single(struct device *hwdev, 
> phys_addr_t tlb_addr,
>  swiotlb_full(struct device *dev, size_t size, enum dma_data_direction dir,
>int do_panic)
>  {
> + if (swiotlb_nobounce)
> + return;
> +
>   /*
>* Ran out of IOMMU space for this operation. This is very bad.
>* Unfortunately the drivers cannot handle this operation properly.
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 4/6] common: DMA-mapping: add DMA_ATTR_PRIVILEGED_EXECUTABLE attribute

[Robin Murphy]

On 09/07/16 03:09, Mitchel Humpherys wrote:
> This patch adds the DMA_ATTR_PRIVILEGED_EXECUTABLE attribute to the
> DMA-mapping subsystem.

DMA_ATTR_PRIVILEGED. We can worry about the (much bigger) executable vs.
NX issue some other time.

> Some architectures require that writable mappings also be non-executable at
> lesser-privileged levels of execution.  This attribute is used to indicate

I don't think "architectures" is really an appropriate fit here - for
what we're addressing, maybe something like "Some advanced peripherals
such as remote processors and GPUs perform accesses to DMA buffers in
both privileged "supervisor" and unprivileged "user" modes."

> to the DMA-mapping subsystem that it should do whatever is necessary to
> ensure that the buffer is executable at an elevated privilege level (by
> making it read-only at the lesser-privileged levels, for example).

and correspondingly "...the buffer is fully accessible at the elevated
privilege level (and ideally inaccessible or at least read-only at the
lesser-privileged levels)". What do you think?

I'm not sure what the latest status of Krzysztof's dma_attrs
coversion[1] is, but if you weren't aware of it this is just a heads-up
that there might need to be some coordination there.

What would be really cool if we could somehow remember which buffers
were allocated as privileged and refuse to expose them to userspace in
dma_mmap_attrs(), but I don't a feasible way to implement that without
impractically massive changes.

Robin.

[1]:http://thread.gmane.org/gmane.comp.freedesktop.xorg.drivers.freedreno/164

> Cc: linux-doc@vger.kernel.org
> Signed-off-by: Mitchel Humpherys 
> ---
>  Documentation/DMA-attributes.txt | 9 +
>  include/linux/dma-attrs.h| 1 +
>  2 files changed, 10 insertions(+)
> 
> diff --git a/Documentation/DMA-attributes.txt 
> b/Documentation/DMA-attributes.txt
> index e8cf9cf873b3..6a22d4307008 100644
> --- a/Documentation/DMA-attributes.txt
> +++ b/Documentation/DMA-attributes.txt
> @@ -126,3 +126,12 @@ means that we won't try quite as hard to get them.
>  
>  NOTE: At the moment DMA_ATTR_ALLOC_SINGLE_PAGES is only implemented on ARM,
>  though ARM64 patches will likely be posted soon.
> +
> +DMA_ATTR_PRIVILEGED_EXECUTABLE
> +--
> +
> +Some architectures require that writable mappings also be non-executable at
> +lesser-privileged levels of execution.  This attribute is used to indicate
> +to the DMA-mapping subsystem that it should do whatever is necessary to
> +ensure that the buffer is executable at an elevated privilege level (by
> +making it read-only at the lesser-privileged levels, for example).
> diff --git a/include/linux/dma-attrs.h b/include/linux/dma-attrs.h
> index 5246239a4953..8cf4dff6185b 100644
> --- a/include/linux/dma-attrs.h
> +++ b/include/linux/dma-attrs.h
> @@ -19,6 +19,7 @@ enum dma_attr {
>   DMA_ATTR_SKIP_CPU_SYNC,
>   DMA_ATTR_FORCE_CONTIGUOUS,
>   DMA_ATTR_ALLOC_SINGLE_PAGES,
> + DMA_ATTR_PRIVILEGED_EXECUTABLE,
>   DMA_ATTR_MAX,
>  };
>  
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] arm64:swiotlb:Enable only when Input size through command line

[Robin Murphy]

Hi Konrad,

On 24/06/16 11:46, Konrad Rzeszutek Wilk wrote:
> On Fri, Jun 24, 2016 at 10:57:29AM +0800, Jisheng Zhang wrote:
>> Dear Konrad,
>>
>> On Thu, 23 Jun 2016 12:06:10 -0400 Konrad Rzeszutek Wilk wrote:
>>
>>> On June 23, 2016 10:30:34 AM EDT, Catalin Marinas  
>>> wrote:
 On Thu, Jun 23, 2016 at 05:43:40PM +0530, Manjeet Pawar wrote:  
> From: Rohit Thapliyal 
>
> swiotlb default size of 64M is too big as
> default value therefore it is made configurable
> through command line through swiotlb_size parameter.
> swiotlb allocation shall be done only when the
> swiotlb size is given through command line.
> Otherwise no swiotlb is allocated.  

 I already queued this patch:

 http://lkml.kernel.org/g/1465372426-4077-1-git-send-email-jszh...@marvell.com

 If you have any objections to it, please reply there.  
>>>
>>>
>>> I do (sorry about duplicate email, the other got rejected by mailing lists).
>>>
>>> Why not expand the swiotlb= parameter instead of introducing a new one?
>>
>> Do you mean pass "swiotlb=" for those platforms(most probably, arm64 with 
>> less
>> than 4GB DDR) which don't need swiotlb? I'm afraid this is not convenient, 
>> and
> 
> Why not just have a function that checks the amount of memory? x86 has
> that - if it finds that the machine has less than 4GB it will not setup
> SWIOTLB?
> 
>> users even don't notice swiotlb parameter. From another side, pass 
>> "swiotlb=0"
>> will make the swiotlb reserve 64MB instead, so how can we achieve zero 
>> reserved
>> memory for swiotlb through "swiotlb=" parameter?
> 
> Obviously make the function understand that 0 is to turn it off.
>>
>> PS: my patch didn't introduce new boot parameter.
> 
> swiotlb_sz ?

Note that Jisheng's patch is the one Catalin linked to, *not* this one,
and more or less does exactly what you describe.

Robin.

>>
>> I'm not sure I got your meaning, so could you please comment my patch
>> directly?
>>
>> Thanks,
>> Jisheng
>>
>>>
>>> Also, why not use the swiotlb by itself? That does the job as well?
>>>
>>>
>>> ___
>>> linux-arm-kernel mailing list
>>> linux-arm-ker...@lists.infradead.org
>>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>>
> 
> ___
> linux-arm-kernel mailing list
> linux-arm-ker...@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] arm64:swiotlb:Enable only when Input size through command line

[Robin Murphy]

On 23/06/16 13:13, Manjeet Pawar wrote:

From: Rohit Thapliyal 

swiotlb default size of 64M is too big as
default value therefore it is made configurable
through command line through swiotlb_size parameter.
swiotlb allocation shall be done only when the
swiotlb size is given through command line.
Otherwise no swiotlb is allocated.


So all platforms with most memory physically above 4GB (which is quite a 
lot of them) are suddenly broken unless they go and muck about with 
their bootloader?


If anyone's got to muck about with their bootloader, why can't it be the 
memory-constrained platforms just passing "swiotlb=1" instead?


Robin.


Signed-off-by: Rohit Thapliyal 
Signed-off-by: Manjeet Pawar 
Reviewed-by: Akhilesh Kumar 
Reviewed-by: Ajeet Kumar Yadav 
---
  Documentation/kernel-parameters.txt |3 +++
  arch/arm64/mm/init.c|3 ++-
  include/linux/swiotlb.h |1 +
  lib/swiotlb.c   |   33 +
  4 files changed, 35 insertions(+), 5 deletions(-)

diff --git a/Documentation/kernel-parameters.txt 
b/Documentation/kernel-parameters.txt
index 82b42c9..12b680f 100644
--- a/Documentation/kernel-parameters.txt
+++ b/Documentation/kernel-parameters.txt
@@ -3875,6 +3875,9 @@ bytes respectively. Such letter suffixes can also be 
entirely omitted.
force -- force using of bounce buffers even if they
 wouldn't be automatically used by the kernel

+   swiotlb_sz= [KNL] enter swiotlb size.
+   Sets the swiotlb size for eg. swiotlb_sz=64M
+
switches=   [HW,M68k]

sysfs.deprecated=0|1 [KNL]
diff --git a/arch/arm64/mm/init.c b/arch/arm64/mm/init.c
index d45f862..89c6b39 100644
--- a/arch/arm64/mm/init.c
+++ b/arch/arm64/mm/init.c
@@ -403,7 +403,8 @@ static void __init free_unused_memmap(void)
   */
  void __init mem_init(void)
  {
-   swiotlb_init(1);
+   if (swiotlb_enabled)
+   swiotlb_init(1);

set_max_mapnr(pfn_to_page(max_pfn) - mem_map);

diff --git a/include/linux/swiotlb.h b/include/linux/swiotlb.h
index 017fced..c7eb146 100644
--- a/include/linux/swiotlb.h
+++ b/include/linux/swiotlb.h
@@ -11,6 +11,7 @@ struct page;
  struct scatterlist;

  extern int swiotlb_force;
+extern int swiotlb_enabled;

  /*
   * Maximum allowable number of contiguous slabs to map,
diff --git a/lib/swiotlb.c b/lib/swiotlb.c
index 76f29ec..e89296a 100644
--- a/lib/swiotlb.c
+++ b/lib/swiotlb.c
@@ -54,6 +54,7 @@
  #define IO_TLB_MIN_SLABS ((1<<20) >> IO_TLB_SHIFT)

  int swiotlb_force;
+int swiotlb_enabled;

  /*
   * Used to do a quick range check in swiotlb_tbl_unmap_single and
@@ -96,6 +97,9 @@ static DEFINE_SPINLOCK(io_tlb_lock);

  static int late_alloc;

+unsigned long swiotlb_sz;
+unsigned int swiotlb_sz_shift;
+
  static int __init
  setup_io_tlb_npages(char *str)
  {
@@ -112,6 +116,24 @@ setup_io_tlb_npages(char *str)
return 0;
  }
  early_param("swiotlb", setup_io_tlb_npages);
+
+static int __init
+setup_io_tlb_size(char *str)
+{
+   int len = strlen(str);
+
+   if (str[len-1] == 'M')
+   swiotlb_sz_shift = 20;
+   else if (str[len-1] == 'K')
+   swiotlb_sz_shift = 10;
+   str[len-1] = '\0';
+   if (isdigit(*str))
+   swiotlb_sz = kstrtoul(str, , 0);
+
+   swiotlb_enabled = 1;
+   return 0;
+}
+early_param("swiotlb_sz", setup_io_tlb_size);
  /* make io_tlb_overflow tunable too? */

  unsigned long swiotlb_nr_tbl(void)
@@ -120,8 +142,9 @@ unsigned long swiotlb_nr_tbl(void)
  }
  EXPORT_SYMBOL_GPL(swiotlb_nr_tbl);

-/* default to 64MB */
-#define IO_TLB_DEFAULT_SIZE (64UL<<20)
+/* Pass from command line as swiotlb_sz=64M (for eg.)*/
+#define IO_TLB_DEFAULT_SIZE (swiotlb_sz<> 20, vstart, vend - 1);
+   bytes >> swiotlb_sz_shift,
+   swiotlb_sz_shift == 20 ? 'M' : 'K',
+   vstart, vend - 1);
  }

  int __init swiotlb_init_with_tbl(char *tlb, unsigned long nslabs, int verbose)



--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html