Re: [PATCH 1/1] PM / devfreq: Replace devfreq->dev.parent as dev in devfreq_add_device
Hi myungjoo, kyungmin and cw: Would you please help to review this patch? Thanks a lot. pierre Kuo 於 2020年12月16日 週三 上午10:26寫道： > > In devfreq_add_device, replace devfreq->dev.parent > as dev to keep code simple. > > Signed-off-by: pierre Kuo > --- > drivers/devfreq/devfreq.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c > index 6aa10de792b3..94cc25fd68da 100644 > --- a/drivers/devfreq/devfreq.c > +++ b/drivers/devfreq/devfreq.c > @@ -893,13 +893,13 @@ struct devfreq *devfreq_add_device(struct device *dev, > goto err_devfreq; > > devfreq->nb_min.notifier_call = qos_min_notifier_call; > - err = dev_pm_qos_add_notifier(devfreq->dev.parent, >nb_min, > + err = dev_pm_qos_add_notifier(dev, >nb_min, > DEV_PM_QOS_MIN_FREQUENCY); > if (err) > goto err_devfreq; > > devfreq->nb_max.notifier_call = qos_max_notifier_call; > - err = dev_pm_qos_add_notifier(devfreq->dev.parent, >nb_max, > + err = dev_pm_qos_add_notifier(dev, >nb_max, > DEV_PM_QOS_MAX_FREQUENCY); > if (err) > goto err_devfreq; > -- > 2.17.1 >
[PATCH 1/1] PM / devfreq: Replace devfreq->dev.parent as dev in devfreq_add_device
In devfreq_add_device, replace devfreq->dev.parent as dev to keep code simple. Signed-off-by: pierre Kuo --- drivers/devfreq/devfreq.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c index 6aa10de792b3..94cc25fd68da 100644 --- a/drivers/devfreq/devfreq.c +++ b/drivers/devfreq/devfreq.c @@ -893,13 +893,13 @@ struct devfreq *devfreq_add_device(struct device *dev, goto err_devfreq; devfreq->nb_min.notifier_call = qos_min_notifier_call; - err = dev_pm_qos_add_notifier(devfreq->dev.parent, >nb_min, + err = dev_pm_qos_add_notifier(dev, >nb_min, DEV_PM_QOS_MIN_FREQUENCY); if (err) goto err_devfreq; devfreq->nb_max.notifier_call = qos_max_notifier_call; - err = dev_pm_qos_add_notifier(devfreq->dev.parent, >nb_max, + err = dev_pm_qos_add_notifier(dev, >nb_max, DEV_PM_QOS_MAX_FREQUENCY); if (err) goto err_devfreq; -- 2.17.1
Re: [PATCH 2/2] driver core: platform: provide devm_platform_iounremap_resource
hi Greg:
> Why are you adding new functions but not actually calling them anywhere?
Below patch introduce a single helper, devm_platform_ioremap_resource,
which combines
platform_get_resource() and devm_ioremap_resource(). But there is no
single helper to release
those resources in driver removing stage.
https://lore.kernel.org/lkml/20190215152507.31066-2-b...@bgdev.pl/
That means driver owner still need to call below (*) and (**) for
releasing resource.
Therefore, this patch adds a single release helper that can be paired with
devm_platform_ioremap_resource.
Appreciate ur kind help,
foo_probe(pdev)
{
iomem = devm_platform_ioremap_resource(pdev, 0);
}
foo_remove(pdev)
{
devm_iounmap(iomem); (*)
devm_release_mem_region(dev, res->start, size); (**)
}
Re: [PATCH 2/2] driver core: platform: provide devm_platform_iounremap_resource
hi Greg:
> Please resend, I can't take patches off of a random web site.
> Now lore.kernel.org I could take them from :)
Please refer to the attachments and links on lore.kernel.org.
https://lore.kernel.org/lkml/20200920113808.3-1-vichy@gmail.com
https://lore.kernel.org/lkml/20200920113808.3-2-vichy@gmail.com
Appreciate your help,
From b141d537904b71b802770d9c0fc3787b98c5cf71 Mon Sep 17 00:00:00 2001
From: pierre Kuo
Date: Tue, 18 Aug 2020 23:05:00 +0800
Subject: [PATCH 1/2] lib: devres: provide devm_iounremap_resource()
Driver doesn't have a single helper function to release memroy
allocated by devm_ioremap_resource(). That mean it needs respectively
to call devm_release_mem_region() and devm_iounmap() for memory release.
This patch creates a helper, devm_iounremap_resource(), to combine above
operations.
Signed-off-by: pierre Kuo
---
include/linux/device.h | 2 ++
lib/devres.c | 25 +
2 files changed, 27 insertions(+)
diff --git a/include/linux/device.h b/include/linux/device.h
index 9e6ea8931a52..33ec7e54c1a9 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -240,6 +240,8 @@ void devm_free_pages(struct device *dev, unsigned long addr);
void __iomem *devm_ioremap_resource(struct device *dev,
const struct resource *res);
+void devm_iounremap_resource(struct device *dev,
+ const struct resource *res, void __iomem *addr);
void __iomem *devm_ioremap_resource_wc(struct device *dev,
const struct resource *res);
diff --git a/lib/devres.c b/lib/devres.c
index ebb1573d9ae3..cdda0cd0a263 100644
--- a/lib/devres.c
+++ b/lib/devres.c
@@ -113,6 +113,31 @@ void devm_iounmap(struct device *dev, void __iomem *addr)
}
EXPORT_SYMBOL(devm_iounmap);
+/**
+ * devm_iounremap_resource() - release mem region, and unremap address
+ * @dev: generic device to handle the resource for
+ * @res: resource of mem region to be release
+ * @addr: address to unmap
+ *
+ * Release memory region and unmap address.
+ */
+void devm_iounremap_resource(struct device *dev,
+ const struct resource *res, void __iomem *addr)
+{
+ resource_size_t size;
+
+ BUG_ON(!dev);
+ if (!res || resource_type(res) != IORESOURCE_MEM) {
+ dev_err(dev, "invalid resource\n");
+ return;
+ }
+
+ size = resource_size(res);
+ devm_release_mem_region(dev, res->start, size);
+ devm_iounmap(dev, addr);
+}
+EXPORT_SYMBOL(devm_iounremap_resource);
+
static void __iomem *
__devm_ioremap_resource(struct device *dev, const struct resource *res,
enum devm_ioremap_type type)
--
2.17.1
From 33afa315c3c941b303e9b3152552010ad266ebbf Mon Sep 17 00:00:00 2001
From: pierre Kuo
Date: Wed, 19 Aug 2020 15:57:05 +0800
Subject: [PATCH 2/2] driver core: platform: provide
devm_platform_iounremap_resource
Combine platform_get_resource() and devm_iounremap_resource() to release
the iomem allocated by devm_platform_get_and_ioremap_resource().
Signed-off-by: pierre Kuo
---
drivers/base/platform.c | 24
include/linux/platform_device.h | 4
2 files changed, 28 insertions(+)
diff --git a/drivers/base/platform.c b/drivers/base/platform.c
index e5d8a0503b4f..e2655c00873f 100644
--- a/drivers/base/platform.c
+++ b/drivers/base/platform.c
@@ -84,6 +84,30 @@ devm_platform_get_and_ioremap_resource(struct platform_device *pdev,
}
EXPORT_SYMBOL_GPL(devm_platform_get_and_ioremap_resource);
+/**
+ * devm_platform_iounremap_resource - call devm_iounremap_resource() for a
+ * platform device with memory that addr points to.
+ *
+ * @pdev: platform device to use both for memory resource lookup as well as
+ *resource management
+ * @index: resource index
+ * @addr: address to be unmap.
+ */
+void
+devm_platform_iounremap_resource(struct platform_device *pdev,
+ unsigned int index, void __iomem *addr)
+{
+ struct resource *r;
+
+ r = platform_get_resource(pdev, IORESOURCE_MEM, index);
+ if (!r)
+ dev_err(>dev,
+ "MEM resource index %d not found\n", index);
+ else
+ devm_iounremap_resource(>dev, r, addr);
+}
+EXPORT_SYMBOL_GPL(devm_platform_iounremap_resource);
+
/**
* devm_platform_ioremap_resource - call devm_ioremap_resource() for a platform
*device
diff --git a/include/linux/platform_device.h b/include/linux/platform_device.h
index 77a2aada106d..75da15937679 100644
--- a/include/linux/platform_device.h
+++ b/include/linux/platform_device.h
@@ -67,6 +67,10 @@ devm_platform_ioremap_resource_wc(struct platform_device *pdev,
extern void __iomem *
devm_platform_ioremap_resource_byname(struct platform_device *pdev,
const char *name);
+extern void
+devm_platform_iounremap_resource(struct platform_device *pdev,
+ unsigned int index,
+ void __iomem *addr);
extern int platform_get_irq(struct platform_device *, unsigned int);
extern int platform_get_irq_optional(struct platform_device *, unsigned int);
extern int platform_irq_count(struct platform_device *);
--
2.17.1
Re: [PATCH 2/2] driver core: platform: provide devm_platform_iounremap_resource
Hi Greg and Rafael:
Would you please help to review these 2 patches?
https://lkml.org/lkml/2020/9/20/112
https://lkml.org/lkml/2020/9/20/113
Appreciate ur help in advance.
>
> Combine platform_get_resource() and devm_iounremap_resource() to release
> the iomem allocated by devm_platform_get_and_ioremap_resource().
>
> Signed-off-by: pierre Kuo
> ---
> drivers/base/platform.c | 24
> include/linux/platform_device.h | 4
> 2 files changed, 28 insertions(+)
>
> diff --git a/drivers/base/platform.c b/drivers/base/platform.c
> index e5d8a0503b4f..e2655c00873f 100644
> --- a/drivers/base/platform.c
> +++ b/drivers/base/platform.c
> @@ -84,6 +84,30 @@ devm_platform_get_and_ioremap_resource(struct
> platform_device *pdev,
> }
> EXPORT_SYMBOL_GPL(devm_platform_get_and_ioremap_resource);
>
> +/**
> + * devm_platform_iounremap_resource - call devm_iounremap_resource() for a
> + * platform device with memory that addr
> points to.
> + *
> + * @pdev: platform device to use both for memory resource lookup as well as
> + *resource management
> + * @index: resource index
> + * @addr: address to be unmap.
> + */
> +void
> +devm_platform_iounremap_resource(struct platform_device *pdev,
> +unsigned int index, void __iomem *addr)
> +{
> + struct resource *r;
> +
> + r = platform_get_resource(pdev, IORESOURCE_MEM, index);
> + if (!r)
> + dev_err(>dev,
> + "MEM resource index %d not found\n", index);
> + else
> + devm_iounremap_resource(>dev, r, addr);
> +}
> +EXPORT_SYMBOL_GPL(devm_platform_iounremap_resource);
> +
> /**
> * devm_platform_ioremap_resource - call devm_ioremap_resource() for a
> platform
> * device
> diff --git a/include/linux/platform_device.h b/include/linux/platform_device.h
> index 77a2aada106d..75da15937679 100644
> --- a/include/linux/platform_device.h
> +++ b/include/linux/platform_device.h
> @@ -67,6 +67,10 @@ devm_platform_ioremap_resource_wc(struct platform_device
> *pdev,
> extern void __iomem *
> devm_platform_ioremap_resource_byname(struct platform_device *pdev,
> const char *name);
> +extern void
> +devm_platform_iounremap_resource(struct platform_device *pdev,
> +unsigned int index,
> +void __iomem *addr);
> extern int platform_get_irq(struct platform_device *, unsigned int);
> extern int platform_get_irq_optional(struct platform_device *, unsigned int);
> extern int platform_irq_count(struct platform_device *);
> --
> 2.17.1
>
[PATCH 1/2] lib: devres: provide devm_iounremap_resource()
Driver doesn't have a single helper function to release memroy
allocated by devm_ioremap_resource(). That mean it needs respectively
to call devm_release_mem_region() and devm_iounmap() for memory release.
This patch creates a helper, devm_iounremap_resource(), to combine above
operations.
Signed-off-by: pierre Kuo
---
include/linux/device.h | 2 ++
lib/devres.c | 25 +
2 files changed, 27 insertions(+)
diff --git a/include/linux/device.h b/include/linux/device.h
index 9e6ea8931a52..33ec7e54c1a9 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -240,6 +240,8 @@ void devm_free_pages(struct device *dev, unsigned long
addr);
void __iomem *devm_ioremap_resource(struct device *dev,
const struct resource *res);
+void devm_iounremap_resource(struct device *dev,
+const struct resource *res, void __iomem *addr);
void __iomem *devm_ioremap_resource_wc(struct device *dev,
const struct resource *res);
diff --git a/lib/devres.c b/lib/devres.c
index ebb1573d9ae3..cdda0cd0a263 100644
--- a/lib/devres.c
+++ b/lib/devres.c
@@ -113,6 +113,31 @@ void devm_iounmap(struct device *dev, void __iomem *addr)
}
EXPORT_SYMBOL(devm_iounmap);
+/**
+ * devm_iounremap_resource() - release mem region, and unremap address
+ * @dev: generic device to handle the resource for
+ * @res: resource of mem region to be release
+ * @addr: address to unmap
+ *
+ * Release memory region and unmap address.
+ */
+void devm_iounremap_resource(struct device *dev,
+const struct resource *res, void __iomem *addr)
+{
+ resource_size_t size;
+
+ BUG_ON(!dev);
+ if (!res || resource_type(res) != IORESOURCE_MEM) {
+ dev_err(dev, "invalid resource\n");
+ return;
+ }
+
+ size = resource_size(res);
+ devm_release_mem_region(dev, res->start, size);
+ devm_iounmap(dev, addr);
+}
+EXPORT_SYMBOL(devm_iounremap_resource);
+
static void __iomem *
__devm_ioremap_resource(struct device *dev, const struct resource *res,
enum devm_ioremap_type type)
--
2.17.1
[PATCH 2/2] driver core: platform: provide devm_platform_iounremap_resource
Combine platform_get_resource() and devm_iounremap_resource() to release
the iomem allocated by devm_platform_get_and_ioremap_resource().
Signed-off-by: pierre Kuo
---
drivers/base/platform.c | 24
include/linux/platform_device.h | 4
2 files changed, 28 insertions(+)
diff --git a/drivers/base/platform.c b/drivers/base/platform.c
index e5d8a0503b4f..e2655c00873f 100644
--- a/drivers/base/platform.c
+++ b/drivers/base/platform.c
@@ -84,6 +84,30 @@ devm_platform_get_and_ioremap_resource(struct
platform_device *pdev,
}
EXPORT_SYMBOL_GPL(devm_platform_get_and_ioremap_resource);
+/**
+ * devm_platform_iounremap_resource - call devm_iounremap_resource() for a
+ * platform device with memory that addr
points to.
+ *
+ * @pdev: platform device to use both for memory resource lookup as well as
+ *resource management
+ * @index: resource index
+ * @addr: address to be unmap.
+ */
+void
+devm_platform_iounremap_resource(struct platform_device *pdev,
+unsigned int index, void __iomem *addr)
+{
+ struct resource *r;
+
+ r = platform_get_resource(pdev, IORESOURCE_MEM, index);
+ if (!r)
+ dev_err(>dev,
+ "MEM resource index %d not found\n", index);
+ else
+ devm_iounremap_resource(>dev, r, addr);
+}
+EXPORT_SYMBOL_GPL(devm_platform_iounremap_resource);
+
/**
* devm_platform_ioremap_resource - call devm_ioremap_resource() for a platform
* device
diff --git a/include/linux/platform_device.h b/include/linux/platform_device.h
index 77a2aada106d..75da15937679 100644
--- a/include/linux/platform_device.h
+++ b/include/linux/platform_device.h
@@ -67,6 +67,10 @@ devm_platform_ioremap_resource_wc(struct platform_device
*pdev,
extern void __iomem *
devm_platform_ioremap_resource_byname(struct platform_device *pdev,
const char *name);
+extern void
+devm_platform_iounremap_resource(struct platform_device *pdev,
+unsigned int index,
+void __iomem *addr);
extern int platform_get_irq(struct platform_device *, unsigned int);
extern int platform_get_irq_optional(struct platform_device *, unsigned int);
extern int platform_irq_count(struct platform_device *);
--
2.17.1
Re: [PATCH v3 1/2] kaslr: shift linear region randomization ahead of memory_limit
hi Ard:
> > The following is schematic diagram of the program before and after the
> > modification.
> >
> > Before:
> > if (memstart_addr + linear_region_size < memblock_end_of_DRAM()) {} --(a)
> > if (memory_limit != PHYS_ADDR_MAX) {} --(b)
> > if (IS_ENABLED(CONFIG_BLK_DEV_INITRD) && phys_initrd_size) {} --(c)
> > if (IS_ENABLED(CONFIG_RANDOMIZE_BASE)) {} --(d)*
> >
> > After:
> > if (memstart_addr + linear_region_size < memblock_end_of_DRAM()) {} --(a)
> > if (IS_ENABLED(CONFIG_RANDOMIZE_BASE)) {} --(d)*
> > if (memory_limit != PHYS_ADDR_MAX) {} --(b)
> > if (IS_ENABLED(CONFIG_BLK_DEV_INITRD) && phys_initrd_size) {} --(c)
> >
> > After grouping modification of memstart_address by moving linear region
> > randomization ahead of memory_init, driver can safely using macro,
> > __phys_to_virt, in (b) or (c), if necessary.
> >
>
> Why is this an advantage?
1st, by putting (d) right behind (a), driver can safely uses macro,
__phys_to_virt that depends on memstart_address, in (b) and (c) if necessary.
That means:
(a)
(d)
- finish modification of memstart_address
(b) --> can safely use __phys_to_virt
(c) --> can safely use __phys_to_virt
2nd, it can make current driver more concise.
Letting (d) right behind (a), as below v3 patch shows,
https://lkml.org/lkml/2019/4/8/683
it can put initrd_start/initrd_end to be calculated only when linear
mapping check
pass and concise the driver by eliminating
"if (IS_ENABLED(CONFIG_BLK_DEV_INITRD) && phys_initrd_size) " as patch shows.
Thanks for your message.
Re: [PATCH 1/1] of: reserved_mem: fix reserve memory leak
hi Rob: > As no one else seems to have any comments, I've applied it. Sorry for bothering you. Since I haven't see this patch on below up stream repository, "git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git" if there is anything wrong about the patch, please let me know. Appreciate ur kind help,
Re: [PATCH v3 1/2] kaslr: shift linear region randomization ahead of memory_limit
hi will and all:
>
> The following is schematic diagram of the program before and after the
> modification.
>
> Before:
> if (memstart_addr + linear_region_size < memblock_end_of_DRAM()) {} --(a)
> if (memory_limit != PHYS_ADDR_MAX) {} --(b)
> if (IS_ENABLED(CONFIG_BLK_DEV_INITRD) && phys_initrd_size) {} --(c)
> if (IS_ENABLED(CONFIG_RANDOMIZE_BASE)) {} --(d)*
>
> After:
> if (memstart_addr + linear_region_size < memblock_end_of_DRAM()) {} --(a)
> if (IS_ENABLED(CONFIG_RANDOMIZE_BASE)) {} --(d)*
> if (memory_limit != PHYS_ADDR_MAX) {} --(b)
> if (IS_ENABLED(CONFIG_BLK_DEV_INITRD) && phys_initrd_size) {} --(c)
>
> After grouping modification of memstart_address by moving linear region
> randomization ahead of memory_init, driver can safely using macro,
> __phys_to_virt, in (b) or (c), if necessary.
>
> Signed-off-by: pierre Kuo
> ---
> Changes in v2:
> - add Fixes tag
>
> Changes in v3:
> - adding patch of shifting linear region randomization ahead of
> memory_limit
>
> arch/arm64/mm/init.c | 33 +
> 1 file changed, 17 insertions(+), 16 deletions(-)
>
> diff --git a/arch/arm64/mm/init.c b/arch/arm64/mm/init.c
> index 7205a9085b4d..5142020fc146 100644
> --- a/arch/arm64/mm/init.c
> +++ b/arch/arm64/mm/init.c
> @@ -389,6 +389,23 @@ void __init arm64_memblock_init(void)
> memblock_remove(0, memstart_addr);
> }
>
> + if (IS_ENABLED(CONFIG_RANDOMIZE_BASE)) {
> + extern u16 memstart_offset_seed;
> + u64 range = linear_region_size -
> + (memblock_end_of_DRAM() -
> memblock_start_of_DRAM());
> +
> + /*
> +* If the size of the linear region exceeds, by a sufficient
> +* margin, the size of the region that the available physical
> +* memory spans, randomize the linear region as well.
> +*/
> + if (memstart_offset_seed > 0 && range >=
> ARM64_MEMSTART_ALIGN) {
> + range /= ARM64_MEMSTART_ALIGN;
> + memstart_addr -= ARM64_MEMSTART_ALIGN *
> +((range * memstart_offset_seed) >>
> 16);
> + }
> + }
> +
> /*
> * Apply the memory limit if it was set. Since the kernel may be
> loaded
> * high up in memory, add back the kernel region that must be
> accessible
> @@ -428,22 +445,6 @@ void __init arm64_memblock_init(void)
> }
> }
>
> - if (IS_ENABLED(CONFIG_RANDOMIZE_BASE)) {
> - extern u16 memstart_offset_seed;
> - u64 range = linear_region_size -
> - (memblock_end_of_DRAM() -
> memblock_start_of_DRAM());
> -
> - /*
> -* If the size of the linear region exceeds, by a sufficient
> -* margin, the size of the region that the available physical
> -* memory spans, randomize the linear region as well.
> -*/
> - if (memstart_offset_seed > 0 && range >=
> ARM64_MEMSTART_ALIGN) {
> - range /= ARM64_MEMSTART_ALIGN;
> - memstart_addr -= ARM64_MEMSTART_ALIGN *
> -((range * memstart_offset_seed) >>
> 16);
> - }
> - }
>
> /*
> * Register the kernel text, kernel data, initrd, and initial
Would you mind to give some comment and suggestion for these v3 patches?
https://lkml.org/lkml/2019/4/8/682
https://lkml.org/lkml/2019/4/8/683
Sincerely appreciate your kind help,
Re: [PATCH 1/1] of: reserved_mem: fix reserve memory leak
hi Rob, Marek and Frank: > > > > In this patch, we un-reserving memory ONLY if explicit compatible matching > > fail. > > That mean driver found something wrong while matching and let OS know. > > (But reserved-memory without compatible property will not be affected.) > > > > So per ur explaination, there would be cases that driver reported > > matching memory fail, > > but the memory is still needed by another processor? > Would you mind to give some comment and suggestion for this patch? Sincerely appreciate ur kind help.
Re: [PATCH 1/1] of: reserved_mem: fix reserve memory leak
hi Rob, Marek and Frank: > > In this patch, we un-reserving memory ONLY if explicit compatible matching > > fail. > > That mean driver found something wrong while matching and let OS know. > > (But reserved-memory without compatible property will not be affected.) > > > > So per ur explaination, there would be cases that driver reported > > matching memory fail, > > but the memory is still needed by another processor? Would you mind to give some comment and suggestion for this patch? Sincerely appreciate ur kind help,
[PATCH v3 2/2] initrd: move initrd_start calculate within linear mapping range check
in the previous case, initrd_start and initrd_end can be successfully
returned either (base < memblock_start_of_DRAM()) or (base + size >
memblock_start_of_DRAM() + linear_region_size).
That means even linear mapping range check fail for initrd_start and
initrd_end, it still can get virtual address. Here we put
initrd_start/initrd_end to be calculated only when linear mapping check
pass.
Fixes: c756c592e442 ("arm64: Utilize phys_initrd_start/phys_initrd_size")
Reviewed-by: Steven Price
Signed-off-by: pierre Kuo
---
Changes in v2:
- add Fixes tag
Changes in v3:
- adding patch of shifting linear region randomization ahead of
memory_limit
arch/arm64/mm/init.c | 8 +++-
1 file changed, 3 insertions(+), 5 deletions(-)
diff --git a/arch/arm64/mm/init.c b/arch/arm64/mm/init.c
index 5142020fc146..566761da5719 100644
--- a/arch/arm64/mm/init.c
+++ b/arch/arm64/mm/init.c
@@ -442,6 +442,9 @@ void __init arm64_memblock_init(void)
memblock_remove(base, size); /* clear MEMBLOCK_ flags */
memblock_add(base, size);
memblock_reserve(base, size);
+ /* the generic initrd code expects virtual addresses */
+ initrd_start = __phys_to_virt(phys_initrd_start);
+ initrd_end = initrd_start + phys_initrd_size;
}
}
@@ -451,11 +454,6 @@ void __init arm64_memblock_init(void)
* pagetables with memblock.
*/
memblock_reserve(__pa_symbol(_text), _end - _text);
- if (IS_ENABLED(CONFIG_BLK_DEV_INITRD) && phys_initrd_size) {
- /* the generic initrd code expects virtual addresses */
- initrd_start = __phys_to_virt(phys_initrd_start);
- initrd_end = initrd_start + phys_initrd_size;
- }
early_init_fdt_scan_reserved_mem();
--
2.17.1
[PATCH v3 1/2] kaslr: shift linear region randomization ahead of memory_limit
The following is schematic diagram of the program before and after the
modification.
Before:
if (memstart_addr + linear_region_size < memblock_end_of_DRAM()) {} --(a)
if (memory_limit != PHYS_ADDR_MAX) {} --(b)
if (IS_ENABLED(CONFIG_BLK_DEV_INITRD) && phys_initrd_size) {} --(c)
if (IS_ENABLED(CONFIG_RANDOMIZE_BASE)) {} --(d)*
After:
if (memstart_addr + linear_region_size < memblock_end_of_DRAM()) {} --(a)
if (IS_ENABLED(CONFIG_RANDOMIZE_BASE)) {} --(d)*
if (memory_limit != PHYS_ADDR_MAX) {} --(b)
if (IS_ENABLED(CONFIG_BLK_DEV_INITRD) && phys_initrd_size) {} --(c)
After grouping modification of memstart_address by moving linear region
randomization ahead of memory_init, driver can safely using macro,
__phys_to_virt, in (b) or (c), if necessary.
Signed-off-by: pierre Kuo
---
Changes in v2:
- add Fixes tag
Changes in v3:
- adding patch of shifting linear region randomization ahead of
memory_limit
arch/arm64/mm/init.c | 33 +
1 file changed, 17 insertions(+), 16 deletions(-)
diff --git a/arch/arm64/mm/init.c b/arch/arm64/mm/init.c
index 7205a9085b4d..5142020fc146 100644
--- a/arch/arm64/mm/init.c
+++ b/arch/arm64/mm/init.c
@@ -389,6 +389,23 @@ void __init arm64_memblock_init(void)
memblock_remove(0, memstart_addr);
}
+ if (IS_ENABLED(CONFIG_RANDOMIZE_BASE)) {
+ extern u16 memstart_offset_seed;
+ u64 range = linear_region_size -
+ (memblock_end_of_DRAM() - memblock_start_of_DRAM());
+
+ /*
+* If the size of the linear region exceeds, by a sufficient
+* margin, the size of the region that the available physical
+* memory spans, randomize the linear region as well.
+*/
+ if (memstart_offset_seed > 0 && range >= ARM64_MEMSTART_ALIGN) {
+ range /= ARM64_MEMSTART_ALIGN;
+ memstart_addr -= ARM64_MEMSTART_ALIGN *
+((range * memstart_offset_seed) >> 16);
+ }
+ }
+
/*
* Apply the memory limit if it was set. Since the kernel may be loaded
* high up in memory, add back the kernel region that must be accessible
@@ -428,22 +445,6 @@ void __init arm64_memblock_init(void)
}
}
- if (IS_ENABLED(CONFIG_RANDOMIZE_BASE)) {
- extern u16 memstart_offset_seed;
- u64 range = linear_region_size -
- (memblock_end_of_DRAM() - memblock_start_of_DRAM());
-
- /*
-* If the size of the linear region exceeds, by a sufficient
-* margin, the size of the region that the available physical
-* memory spans, randomize the linear region as well.
-*/
- if (memstart_offset_seed > 0 && range >= ARM64_MEMSTART_ALIGN) {
- range /= ARM64_MEMSTART_ALIGN;
- memstart_addr -= ARM64_MEMSTART_ALIGN *
-((range * memstart_offset_seed) >> 16);
- }
- }
/*
* Register the kernel text, kernel data, initrd, and initial
--
2.17.1
Re: [PATCH v2 1/1] initrd: move initrd_start calculate within linear mapping range check
hi Will: > > Maybe, but I don't think we've seen a patch which accomplishes that. I think > I'll go ahead and commit the basic one-liner, then we can always improve it > afterwards if somebody sends a patch. It's not like this is a fastpath. Sorry for not showing the patches I try to explain to sir. I will attach v3 patches for ur reference. Thanks for ur kind help,
Re: [PATCH v2 1/1] initrd: move initrd_start calculate within linear mapping range check
hi Will:
>
> [+Ard in case I'm missing something]
>
> On Mon, Apr 01, 2019 at 10:59:53PM +0800, pierre kuo wrote:
> > > > With CONFIG_RANDOMIZE_BASE we can get a further change to memstart_addr
> > > > after the place where you moved the initrd_{start,end} setting, which
> > > > would result in a different value for __phys_to_virt(phys_initrd_start).
> > >
> > > I found what you mean, since __phys_to_virt will use PHYS_OFFSET
> > > (memstart_addr) for calculating.
> > > How about moving CONFIG_RANDOMIZE_BASE part of code ahead of
> > > CONFIG_BLK_DEV_INITRD checking?
> > >
> > > That means below (d) moving ahead of (c)
> > > prvious:
> > > if (memstart_addr + linear_region_size < memblock_end_of_DRAM()) {} ---(a)
> > > if (memory_limit != PHYS_ADDR_MAX) {}---(b)
> > > if (IS_ENABLED(CONFIG_BLK_DEV_INITRD) && phys_initrd_size) {} ---(c)
> > > if (IS_ENABLED(CONFIG_RANDOMIZE_BASE)) {} ---(d)
> > >
> > > now:
> > > if (memstart_addr + linear_region_size < memblock_end_of_DRAM()) { ---(a)
> > > if (memory_limit != PHYS_ADDR_MAX) {} (b)
> > > if (IS_ENABLED(CONFIG_RANDOMIZE_BASE)) {} --(d)
> > > if (IS_ENABLED(CONFIG_BLK_DEV_INITRD) && phys_initrd_size) {} ---(c)
> > >
> >
> > After tracing the kernel code,
> > is it even possible to move CONFIG_RANDOMIZE_BASE part of code ahead
> > of memory_limit?
> >
> > that mean the flow may look like below:
> > now2:
> > if (memstart_addr + linear_region_size < memblock_end_of_DRAM()) {} ---(a)
> > if (IS_ENABLED(CONFIG_RANDOMIZE_BASE)) {} ---(d)
> > if (memory_limit != PHYS_ADDR_MAX) {}---(b)
> > if (IS_ENABLED(CONFIG_BLK_DEV_INITRD) && phys_initrd_size) {} ---(c)
> >
> > in (b), the result of __pa_symbol(_text), memory_limit,
> > memblock_mem_limit_remove_map and memblock_add
> > are not depended on memsart_addr.
> > So the now2 flow can grouping modification of memstart_address, put
> > (a) and (d) together.
>
> I'm afraid that you've lost me with this.
welcome for ur kind suggestion ^^
>Why isn't it just as simple as
> the diff below?
> diff --git a/arch/arm64/mm/init.c b/arch/arm64/mm/init.c
> index c29b17b520cd..ec3487c94b10 100644
> --- a/arch/arm64/mm/init.c
> +++ b/arch/arm64/mm/init.c
> @@ -377,7 +377,7 @@ void __init arm64_memblock_init(void)
> base + size > memblock_start_of_DRAM() +
>linear_region_size,
> "initrd not fully accessible via the linear mapping
> -- please check your bootloader ...\n")) {
> - initrd_start = 0;
> + phys_initrd_size = 0;
> } else {
> memblock_remove(base, size); /* clear MEMBLOCK_ flags
> */
> memblock_add(base, size);
This patch will also fix the issue, but it still needs 2 "if
comparisions" for getting initrd_start/initrd_end.
By possible grouping modification of memstart_address, and put
initrd_start/initrd_end to be calculated only when linear mapping check
pass. Maybe (just if) can let the code be more concise.
Sincerely appreciate all of yours great comment.
Re: [PATCH v2 1/1] initrd: move initrd_start calculate within linear mapping range check
hi Catalin:
> > With CONFIG_RANDOMIZE_BASE we can get a further change to memstart_addr
> > after the place where you moved the initrd_{start,end} setting, which
> > would result in a different value for __phys_to_virt(phys_initrd_start).
>
> I found what you mean, since __phys_to_virt will use PHYS_OFFSET
> (memstart_addr) for calculating.
> How about moving CONFIG_RANDOMIZE_BASE part of code ahead of
> CONFIG_BLK_DEV_INITRD checking?
>
> That means below (d) moving ahead of (c)
> prvious:
> if (memstart_addr + linear_region_size < memblock_end_of_DRAM()) {} ---(a)
> if (memory_limit != PHYS_ADDR_MAX) {}---(b)
> if (IS_ENABLED(CONFIG_BLK_DEV_INITRD) && phys_initrd_size) {} ---(c)
> if (IS_ENABLED(CONFIG_RANDOMIZE_BASE)) {} ---(d)
>
> now:
> if (memstart_addr + linear_region_size < memblock_end_of_DRAM()) { ---(a)
> if (memory_limit != PHYS_ADDR_MAX) {} (b)
> if (IS_ENABLED(CONFIG_RANDOMIZE_BASE)) {} --(d)
> if (IS_ENABLED(CONFIG_BLK_DEV_INITRD) && phys_initrd_size) {} ---(c)
>
After tracing the kernel code,
is it even possible to move CONFIG_RANDOMIZE_BASE part of code ahead
of memory_limit?
that mean the flow may look like below:
now2:
if (memstart_addr + linear_region_size < memblock_end_of_DRAM()) {} ---(a)
if (IS_ENABLED(CONFIG_RANDOMIZE_BASE)) {} ---(d)
if (memory_limit != PHYS_ADDR_MAX) {}---(b)
if (IS_ENABLED(CONFIG_BLK_DEV_INITRD) && phys_initrd_size) {} ---(c)
in (b), the result of __pa_symbol(_text), memory_limit,
memblock_mem_limit_remove_map and memblock_add
are not depended on memsart_addr.
So the now2 flow can grouping modification of memstart_address, put
(a) and (d) together.
Sincerely Appreciate your comment.
Re: [PATCH v2 1/1] initrd: move initrd_start calculate within linear mapping range check
hi Catalin:
> On Thu, Mar 14, 2019 at 11:20:47AM +0800, pierre Kuo wrote:
> > in the previous case, initrd_start and initrd_end can be successfully
> > returned either (base < memblock_start_of_DRAM()) or (base + size >
> > memblock_start_of_DRAM() + linear_region_size).
> >
> > That means even linear mapping range check fail for initrd_start and
> > initrd_end, it still can get virtual address. Here we put
> > initrd_start/initrd_end to be calculated only when linear mapping check
> > pass.
> >
> > Fixes: c756c592e442 ("arm64: Utilize phys_initrd_start/phys_initrd_size")
>
> For future versions, please also cc the author of the original commit
> you are fixing.
Got it and thanks for ur warm reminder ^^
> >
> > arch/arm64/mm/init.c | 8 +++-
> > 1 file changed, 3 insertions(+), 5 deletions(-)
> >
> > diff --git a/arch/arm64/mm/init.c b/arch/arm64/mm/init.c
> > index 7205a9085b4d..1adf418de685 100644
> > --- a/arch/arm64/mm/init.c
> > +++ b/arch/arm64/mm/init.c
> > @@ -425,6 +425,9 @@ void __init arm64_memblock_init(void)
> > memblock_remove(base, size); /* clear MEMBLOCK_ flags
> > */
> > memblock_add(base, size);
> > memblock_reserve(base, size);
> > + /* the generic initrd code expects virtual addresses
> > */
> > + initrd_start = __phys_to_virt(phys_initrd_start);
> > + initrd_end = initrd_start + phys_initrd_size;
> > }
> > }
> >
> > @@ -450,11 +453,6 @@ void __init arm64_memblock_init(void)
> >* pagetables with memblock.
> >*/
> > memblock_reserve(__pa_symbol(_text), _end - _text);
> > - if (IS_ENABLED(CONFIG_BLK_DEV_INITRD) && phys_initrd_size) {
> > - /* the generic initrd code expects virtual addresses */
> > - initrd_start = __phys_to_virt(phys_initrd_start);
> > - initrd_end = initrd_start + phys_initrd_size;
> > - }
>
> With CONFIG_RANDOMIZE_BASE we can get a further change to memstart_addr
> after the place where you moved the initrd_{start,end} setting, which
> would result in a different value for __phys_to_virt(phys_initrd_start).
I found what you mean, since __phys_to_virt will use PHYS_OFFSET
(memstart_addr) for calculating.
How about moving CONFIG_RANDOMIZE_BASE part of code ahead of
CONFIG_BLK_DEV_INITRD checking?
That means below (d) moving ahead of (c)
prvious:
if (memstart_addr + linear_region_size < memblock_end_of_DRAM()) {} ---(a)
if (memory_limit != PHYS_ADDR_MAX) {}---(b)
if (IS_ENABLED(CONFIG_BLK_DEV_INITRD) && phys_initrd_size) {} ---(c)
if (IS_ENABLED(CONFIG_RANDOMIZE_BASE)) {} ---(d)
now:
if (memstart_addr + linear_region_size < memblock_end_of_DRAM()) { ---(a)
if (memory_limit != PHYS_ADDR_MAX) {} (b)
if (IS_ENABLED(CONFIG_RANDOMIZE_BASE)) {} --(d)
if (IS_ENABLED(CONFIG_BLK_DEV_INITRD) && phys_initrd_size) {} ---(c)
Appreciate your kind advice.
Re: [PATCH v2 1/1] initrd: move initrd_start calculate within linear mapping range check
Hi Steven, Catalin and Will:
>
> in the previous case, initrd_start and initrd_end can be successfully
> returned either (base < memblock_start_of_DRAM()) or (base + size >
> memblock_start_of_DRAM() + linear_region_size).
>
> That means even linear mapping range check fail for initrd_start and
> initrd_end, it still can get virtual address. Here we put
> initrd_start/initrd_end to be calculated only when linear mapping check
> pass.
>
> Fixes: c756c592e442 ("arm64: Utilize phys_initrd_start/phys_initrd_size")
> Reviewed-by: Steven Price
> Signed-off-by: pierre Kuo
> ---
> Changes in v2:
> - add Fixes tag
>
Would you mind to give some comment and suggestion for this v2 patch?
If there is anything that are not noticed, please let me know.
Sincerely appreciate ur kind help.
[PATCH v2 1/1] initrd: move initrd_start calculate within linear mapping range check
in the previous case, initrd_start and initrd_end can be successfully
returned either (base < memblock_start_of_DRAM()) or (base + size >
memblock_start_of_DRAM() + linear_region_size).
That means even linear mapping range check fail for initrd_start and
initrd_end, it still can get virtual address. Here we put
initrd_start/initrd_end to be calculated only when linear mapping check
pass.
Fixes: c756c592e442 ("arm64: Utilize phys_initrd_start/phys_initrd_size")
Reviewed-by: Steven Price
Signed-off-by: pierre Kuo
---
Changes in v2:
- add Fixes tag
arch/arm64/mm/init.c | 8 +++-
1 file changed, 3 insertions(+), 5 deletions(-)
diff --git a/arch/arm64/mm/init.c b/arch/arm64/mm/init.c
index 7205a9085b4d..1adf418de685 100644
--- a/arch/arm64/mm/init.c
+++ b/arch/arm64/mm/init.c
@@ -425,6 +425,9 @@ void __init arm64_memblock_init(void)
memblock_remove(base, size); /* clear MEMBLOCK_ flags */
memblock_add(base, size);
memblock_reserve(base, size);
+ /* the generic initrd code expects virtual addresses */
+ initrd_start = __phys_to_virt(phys_initrd_start);
+ initrd_end = initrd_start + phys_initrd_size;
}
}
@@ -450,11 +453,6 @@ void __init arm64_memblock_init(void)
* pagetables with memblock.
*/
memblock_reserve(__pa_symbol(_text), _end - _text);
- if (IS_ENABLED(CONFIG_BLK_DEV_INITRD) && phys_initrd_size) {
- /* the generic initrd code expects virtual addresses */
- initrd_start = __phys_to_virt(phys_initrd_start);
- initrd_end = initrd_start + phys_initrd_size;
- }
early_init_fdt_scan_reserved_mem();
--
2.17.1
[PATCH 1/1] initrd: move initrd_start calculate within linear mapping range check
in the previous case, initrd_start and initrd_end can be successfully
returned even (base < memblock_start_of_DRAM()) or (base + size >
memblock_start_of_DRAM() + linear_region_size).
That means even linear mapping range check fail for initrd_start and
initrd_end, it still can get virtual address. Here we put
initrd_start/initrd_end to be calculated only when linear mapping check
pass.
Signed-off-by: pierre Kuo
---
arch/arm64/mm/init.c | 8 +++-
1 file changed, 3 insertions(+), 5 deletions(-)
diff --git a/arch/arm64/mm/init.c b/arch/arm64/mm/init.c
index 7205a9085b4d..1adf418de685 100644
--- a/arch/arm64/mm/init.c
+++ b/arch/arm64/mm/init.c
@@ -425,6 +425,9 @@ void __init arm64_memblock_init(void)
memblock_remove(base, size); /* clear MEMBLOCK_ flags */
memblock_add(base, size);
memblock_reserve(base, size);
+ /* the generic initrd code expects virtual addresses */
+ initrd_start = __phys_to_virt(phys_initrd_start);
+ initrd_end = initrd_start + phys_initrd_size;
}
}
@@ -450,11 +453,6 @@ void __init arm64_memblock_init(void)
* pagetables with memblock.
*/
memblock_reserve(__pa_symbol(_text), _end - _text);
- if (IS_ENABLED(CONFIG_BLK_DEV_INITRD) && phys_initrd_size) {
- /* the generic initrd code expects virtual addresses */
- initrd_start = __phys_to_virt(phys_initrd_start);
- initrd_end = initrd_start + phys_initrd_size;
- }
early_init_fdt_scan_reserved_mem();
--
2.17.1
Re: [PATCH 1/1] of: reserved_mem: fix reserve memory leak
hi Rob, Marek and Frank: > > > The __reserved_mem_init_node will call region specific reserved memory > > > init codes, but once all compatibled init codes failed, the memory region > > > will left in memory.reserved and cause leakage. > > > > > > Take cma reserve memory DTS for example, if user declare 1MB size, > > > which is not align to (PAGE_SIZE << max(MAX_ORDER - 1, > > > pageblock_order)), rmem_cma_setup will return -EINVAL. > > > Meanwhile, rmem_dma_setup will also return -EINVAL since "reusable" > > > property is not set. If finally there is no reserved memory init pick up > > > this memory, kernel will left the 1MB leak in memory.reserved. > > > > > > This patch will remove this kind of memory from memory.reserved, only > > > when __reserved_mem_init_node return neither 0 nor -ENOENT. > > > > I'm not sure that un-reserving memory on error is the correct > > behavior. It may be fine for something like CMA, but if it is some > > shared memory used by another processor in the system not reserving it > > would probably never be correct. > > In this patch, we un-reserving memory ONLY if explicit compatible matching > fail. > That mean driver found something wrong while matching and let OS know. > (But reserved-memory without compatible property will not be affected.) > > So per ur explaination, there would be cases that driver reported > matching memory fail, > but the memory is still needed by another processor? Would you mind to give some comment and suggestion for this patch? Sincerely appreciate ur kind help.
Re: [PATCH 1/1] of: reserved_mem: fix reserve memory leak
> You should Cc the author(s) of this code. I've added Marek. Thanks ^^ The cc lists I got were from get_maintainer.pl, no matter running with of_reserved_mem.c or patch file. (with file) $ perl scripts/get_maintainer.pl --separator , --norolestats drivers/of/of_reserved_mem.c Rob Herring ,Frank Rowand ,devicet...@vger.kernel.org,linux-kernel@vger.kernel.org (with patch) $ perl scripts/get_maintainer.pl --separator , --norolestats 0001-of-reserved_mem-fix-reserve-memory-leak.patch Rob Herring ,Frank Rowand ,devicet...@vger.kernel.org,linux-kernel@vger.kernel.org > > The __reserved_mem_init_node will call region specific reserved memory > > init codes, but once all compatibled init codes failed, the memory region > > will left in memory.reserved and cause leakage. > > > > Take cma reserve memory DTS for example, if user declare 1MB size, > > which is not align to (PAGE_SIZE << max(MAX_ORDER - 1, > > pageblock_order)), rmem_cma_setup will return -EINVAL. > > Meanwhile, rmem_dma_setup will also return -EINVAL since "reusable" > > property is not set. If finally there is no reserved memory init pick up > > this memory, kernel will left the 1MB leak in memory.reserved. > > > > This patch will remove this kind of memory from memory.reserved, only > > when __reserved_mem_init_node return neither 0 nor -ENOENT. > > I'm not sure that un-reserving memory on error is the correct > behavior. It may be fine for something like CMA, but if it is some > shared memory used by another processor in the system not reserving it > would probably never be correct. In this patch, we un-reserving memory ONLY if explicit compatible matching fail. That mean driver found something wrong while matching and let OS know. (But reserved-memory without compatible property will not be affected.) So per ur explaination, there would be cases that driver reported matching memory fail, but the memory is still needed by another processor? Appreciate ur kind help,
[PATCH 1/1] of: reserved_mem: fix reserve memory leak
The __reserved_mem_init_node will call region specific reserved memory
init codes, but once all compatibled init codes failed, the memory region
will left in memory.reserved and cause leakage.
Take cma reserve memory DTS for example, if user declare 1MB size,
which is not align to (PAGE_SIZE << max(MAX_ORDER - 1,
pageblock_order)), rmem_cma_setup will return -EINVAL.
Meanwhile, rmem_dma_setup will also return -EINVAL since "reusable"
property is not set. If finally there is no reserved memory init pick up
this memory, kernel will left the 1MB leak in memory.reserved.
This patch will remove this kind of memory from memory.reserved, only
when __reserved_mem_init_node return neither 0 nor -ENOENT.
Signed-off-by: pierre Kuo
---
drivers/of/of_reserved_mem.c | 22 +-
1 file changed, 17 insertions(+), 5 deletions(-)
diff --git a/drivers/of/of_reserved_mem.c b/drivers/of/of_reserved_mem.c
index 1977ee0adcb1..d3bde057ec46 100644
--- a/drivers/of/of_reserved_mem.c
+++ b/drivers/of/of_reserved_mem.c
@@ -181,6 +181,7 @@ static int __init __reserved_mem_init_node(struct
reserved_mem *rmem)
{
extern const struct of_device_id __reservedmem_of_table[];
const struct of_device_id *i;
+ int ret = -ENOENT;
for (i = __reservedmem_of_table; i < &__rmem_of_table_sentinel; i++) {
reservedmem_of_init_fn initfn = i->data;
@@ -189,13 +190,14 @@ static int __init __reserved_mem_init_node(struct
reserved_mem *rmem)
if (!of_flat_dt_is_compatible(rmem->fdt_node, compat))
continue;
- if (initfn(rmem) == 0) {
+ ret = initfn(rmem);
+ if (ret == 0) {
pr_info("initialized node %s, compatible id %s\n",
rmem->name, compat);
- return 0;
+ break;
}
}
- return -ENOENT;
+ return ret;
}
static int __init __rmem_cmp(const void *a, const void *b)
@@ -255,7 +257,9 @@ void __init fdt_init_reserved_mem(void)
int len;
const __be32 *prop;
int err = 0;
+ int nomap;
+ nomap = of_get_flat_dt_prop(node, "no-map", NULL) != NULL;
prop = of_get_flat_dt_prop(node, "phandle", );
if (!prop)
prop = of_get_flat_dt_prop(node, "linux,phandle", );
@@ -265,8 +269,16 @@ void __init fdt_init_reserved_mem(void)
if (rmem->size == 0)
err = __reserved_mem_alloc_size(node, rmem->name,
>base, >size);
- if (err == 0)
- __reserved_mem_init_node(rmem);
+ if (err == 0) {
+ err = __reserved_mem_init_node(rmem);
+ if (err != 0 && err != -ENOENT) {
+ pr_info("node %s compatible matching fail\n",
+ rmem->name);
+ memblock_free(rmem->base, rmem->size);
+ if (nomap)
+ memblock_add(rmem->base, rmem->size);
+ }
+ }
}
}
--
2.17.1
Re: [RFC V2] printk: add warning while drop partial text in msg
hi Petr and Sergey:
> I wonder what is the motivation for the extra buffering. Did you
> have troubles with direct printk() calls? For example, because
> of performance, mixed messages, deadlocks?
Yes, when using direct printk() calls, we suffer performance and mix
message issues.
(Since originally we try to collect the status of hardware and driver
at different places,
then use a work queue periodically for printk() the results of above
collections or
also do some simple analysis and calculation before printk().)
>
> Note that any buffering is potentially dangerous. You might miss
> the messages if the system dies before they are flushed. Also
> you might lose messages if the buffer is too small.
> You might go around this by flushing the buffer line by line.
Yes, you are right.
Except flushing the buffer line by line, we try to shrink the buffer size
to the number that plus prefix words will not over the size of
textbuf[], say 992 Bytes.
(that mean "size of buffer + 18*(number of "\n") < 992 Bytes")
> Well, it might get quite complicated, see printk_safe_flush_buffer().
BTW, after checking printk_safe_flush_buffer() and related functions,
we have one question:
a) Why in printk_safe_log_store(), we need to use atomic_ operation in it?
printk_safe_log_store() will be called in either one of below case
i) PRINTK_NMI_CONTEXT_MASK
ii) PRINTK_SAFE_CONTEXT_MASK
and each of them will bring each CPU's own nmi_print_seq or
safe_print_seq separately.
why it still need atomic_xxx operations like below *** show?
kernel/printk/printk_safe.c
-->static __printf(2, 0) int printk_safe_log_store(struct
printk_safe_seq_buf *s,
const char *fmt, va_list args)
{
int add;
size_t len;
again:
len = atomic_read(>len); **
> Another question is if the buffering makes sense then.
We explained our initial thought above.
Appreciate you and Sergey's great comment and help,
Re: [RFC V2] printk: add warning while drop partial text in msg
hi Petr and Sergey:
> I wonder what is the motivation for the extra buffering. Did you
> have troubles with direct printk() calls? For example, because
> of performance, mixed messages, deadlocks?
Yes, when using direct printk() calls, we suffer performance and mix
message issues.
(Since originally we try to collect the status of hardware and driver
at different places,
then use a work queue periodically for printk() the results of above
collections or
also do some simple analysis and calculation before printk().)
>
> Note that any buffering is potentially dangerous. You might miss
> the messages if the system dies before they are flushed. Also
> you might lose messages if the buffer is too small.
> You might go around this by flushing the buffer line by line.
Yes, you are right.
Except flushing the buffer line by line, we try to shrink the buffer size
to the number that plus prefix words will not over the size of
textbuf[], say 992 Bytes.
(that mean "size of buffer + 18*(number of "\n") < 992 Bytes")
> Well, it might get quite complicated, see printk_safe_flush_buffer().
BTW, after checking printk_safe_flush_buffer() and related functions,
we have one question:
a) Why in printk_safe_log_store(), we need to use atomic_ operation in it?
printk_safe_log_store() will be called in either one of below case
i) PRINTK_NMI_CONTEXT_MASK
ii) PRINTK_SAFE_CONTEXT_MASK
and each of them will bring each CPU's own nmi_print_seq or
safe_print_seq separately.
why it still need atomic_xxx operations like below *** show?
kernel/printk/printk_safe.c
-->static __printf(2, 0) int printk_safe_log_store(struct
printk_safe_seq_buf *s,
const char *fmt, va_list args)
{
int add;
size_t len;
again:
len = atomic_read(>len); **
> Another question is if the buffering makes sense then.
We explained our initial thought above.
Appreciate you and Sergey's great comment and help,
Re: [RFC V2] printk: add warning while drop partial text in msg
hi:
> There are several possible solutions:
>
> + We could update vprintk_emit() to detect all newlines and
> call log_store() for each part. But this would be a waste
> of the space.
>
> + We could increase the size provided by syslog_printk().
> But this is ugly.
>
> + We could go back to the original idea and print a warning
> about shrunken message when the first record is not fully
> stored by msg_print_text().
>
> I think that the last solution is the best. Note that the
> original patch was wrong because it warned in any
> msg_print_text() and not only the first one.
Would you mind to let me know more about the wrong places you mean
about the patch?
(since I cannot quite understand the "warned in any msg_print_text()
and not only the first one" mean)
In Aug. 11, https://lkml.org/lkml/2017/8/10/707,
Only if below a) and b) are both true, the patch will try to put
warning message at the end of output buffer.
a) sizeof((the next phrase cut by "\n" token ) + prefix composed by
(level | facility | timestamp)) > left length of output buffer
b) (the left length of output buffer) > strlen ("");
so If any one of above a) or b) is not satisfied, there will be no
warning message.
Meanwhile, console_seq, clear_seq and syslog_seq will still increase
for next msg_print_text() run.
So the warning message will not always shows.
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index fc47863..fcd1dd4 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -557,6 +557,7 @@ static u32 msg_used_size(u16 text_len, u16
dict_len, u32 *pad_len)
*/
#define MAX_LOG_TAKE_PART 4
static const char trunc_msg[] = "";
+static const char drop_msg[] = "";
static u32 truncate_msg(u16 *text_len, u16 *trunc_msg_len,
u16 *dict_len, u32 *pad_len)
@@ -1264,8 +1265,14 @@ static size_t msg_print_text(const struct
printk_log *msg, bool syslog, char *bu
if (buf) {
if (print_prefix(msg, syslog, NULL) +
- text_len + 1 >= size - len)
+ text_len + 1 >= size - len) {
+ /* below adding warning message
+* related information into output buffer
+*/
+ if ((size - len) > strlen(drop_msg))
+ memcpy(buf + len, drop_msg,
strlen(drop_msg));
break;
+ }
>Another question is that printk() is used a wrong way here.
>I will comment this in another mail in this thread.
please see my answer in another mail.
Re: [RFC V2] printk: add warning while drop partial text in msg
hi:
> There are several possible solutions:
>
> + We could update vprintk_emit() to detect all newlines and
> call log_store() for each part. But this would be a waste
> of the space.
>
> + We could increase the size provided by syslog_printk().
> But this is ugly.
>
> + We could go back to the original idea and print a warning
> about shrunken message when the first record is not fully
> stored by msg_print_text().
>
> I think that the last solution is the best. Note that the
> original patch was wrong because it warned in any
> msg_print_text() and not only the first one.
Would you mind to let me know more about the wrong places you mean
about the patch?
(since I cannot quite understand the "warned in any msg_print_text()
and not only the first one" mean)
In Aug. 11, https://lkml.org/lkml/2017/8/10/707,
Only if below a) and b) are both true, the patch will try to put
warning message at the end of output buffer.
a) sizeof((the next phrase cut by "\n" token ) + prefix composed by
(level | facility | timestamp)) > left length of output buffer
b) (the left length of output buffer) > strlen ("");
so If any one of above a) or b) is not satisfied, there will be no
warning message.
Meanwhile, console_seq, clear_seq and syslog_seq will still increase
for next msg_print_text() run.
So the warning message will not always shows.
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index fc47863..fcd1dd4 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -557,6 +557,7 @@ static u32 msg_used_size(u16 text_len, u16
dict_len, u32 *pad_len)
*/
#define MAX_LOG_TAKE_PART 4
static const char trunc_msg[] = "";
+static const char drop_msg[] = "";
static u32 truncate_msg(u16 *text_len, u16 *trunc_msg_len,
u16 *dict_len, u32 *pad_len)
@@ -1264,8 +1265,14 @@ static size_t msg_print_text(const struct
printk_log *msg, bool syslog, char *bu
if (buf) {
if (print_prefix(msg, syslog, NULL) +
- text_len + 1 >= size - len)
+ text_len + 1 >= size - len) {
+ /* below adding warning message
+* related information into output buffer
+*/
+ if ((size - len) > strlen(drop_msg))
+ memcpy(buf + len, drop_msg,
strlen(drop_msg));
break;
+ }
>Another question is that printk() is used a wrong way here.
>I will comment this in another mail in this thread.
please see my answer in another mail.
Re: [RFC V2] printk: add warning while drop partial text in msg
hi:
> printk_deferred("%s", local_string[2048]) makes no sense anyway,
> since we limit the message size to 1024 - HEADER chars in
> vprintk_emit() // see static char textbuf[LOG_LINE_MAX].
In local_string[2048], it will all be 0 from [590] to [2047].
And vprintk_emit() will cut message from [LOG_LINE_MAX(992)] to [2047].
so, from [0] to [589], whole messages are saved in msg->text as
expected, from "this is the 0 line\n" to "this is the 29 line\n"
No truncating happen in this stage.
(sorry for declaring local_string[] as 2048 makes people confused.)
Below is what msg->text saved:
560 + 30 = 590
^^ ^^^
||
|--> 30 "\n"
|
--> 560 chars without "\n" from "this is the 0 line" to "this is the 29 line"
Below is complete message size output from msg_print_text:
18*30 + 590 = 540 + 590 = 1130 Bytes
^ ^^^
| |
| --> 590 chars of msg->text
|
-->30 lines with each 18 chars in the prefix(15 for timesatmap and 3
for levle and facility)
Below is the constituents that msg_print_text sent to buf[1024]
(the size of buf, 1024, pass to msg_print_text is < 1130)
18*27 + 530= 486 + 530= 1016 Bytes
^ ^^^
| |
| -->max chars found by memchr with "\n" as copying unit
to meet the upper bound, 1024.
| (from "this is the 0 line" to "this is the 26 line")
|
--> 27 lines with each 18 character in the prefix
so characters left in msg->text not putting to the buffer are 590 -
530 = 60 chars.
(Truncate the message from "this is the 27 line\n" to "this is the 29 line\n".)
We use the example in this mail since we try to collect the message at
different places in our driver.
And batch to printk for saving individual output time and group
message together.
> I'm not quite following what were you trying to prove, sorry.
>does any function in the upstream kernel printk()-s buffers
>larger than LOG_LINE_MAX? which one?
You are correct.
The upstream kernel printk()-s buffers are indeed smaller LOG_LINE_MAX
and not with multi "\n" like this example did.
> we are straggling to resolve the _existing_ printk issues, so
> _theoretical_ and never seen problems are not on my radar.
Got it and really appreciate your kind explanation.
Re: [RFC V2] printk: add warning while drop partial text in msg
hi:
> printk_deferred("%s", local_string[2048]) makes no sense anyway,
> since we limit the message size to 1024 - HEADER chars in
> vprintk_emit() // see static char textbuf[LOG_LINE_MAX].
In local_string[2048], it will all be 0 from [590] to [2047].
And vprintk_emit() will cut message from [LOG_LINE_MAX(992)] to [2047].
so, from [0] to [589], whole messages are saved in msg->text as
expected, from "this is the 0 line\n" to "this is the 29 line\n"
No truncating happen in this stage.
(sorry for declaring local_string[] as 2048 makes people confused.)
Below is what msg->text saved:
560 + 30 = 590
^^ ^^^
||
|--> 30 "\n"
|
--> 560 chars without "\n" from "this is the 0 line" to "this is the 29 line"
Below is complete message size output from msg_print_text:
18*30 + 590 = 540 + 590 = 1130 Bytes
^ ^^^
| |
| --> 590 chars of msg->text
|
-->30 lines with each 18 chars in the prefix(15 for timesatmap and 3
for levle and facility)
Below is the constituents that msg_print_text sent to buf[1024]
(the size of buf, 1024, pass to msg_print_text is < 1130)
18*27 + 530= 486 + 530= 1016 Bytes
^ ^^^
| |
| -->max chars found by memchr with "\n" as copying unit
to meet the upper bound, 1024.
| (from "this is the 0 line" to "this is the 26 line")
|
--> 27 lines with each 18 character in the prefix
so characters left in msg->text not putting to the buffer are 590 -
530 = 60 chars.
(Truncate the message from "this is the 27 line\n" to "this is the 29 line\n".)
We use the example in this mail since we try to collect the message at
different places in our driver.
And batch to printk for saving individual output time and group
message together.
> I'm not quite following what were you trying to prove, sorry.
>does any function in the upstream kernel printk()-s buffers
>larger than LOG_LINE_MAX? which one?
You are correct.
The upstream kernel printk()-s buffers are indeed smaller LOG_LINE_MAX
and not with multi "\n" like this example did.
> we are straggling to resolve the _existing_ printk issues, so
> _theoretical_ and never seen problems are not on my radar.
Got it and really appreciate your kind explanation.
Re: [RFC V2] printk: add warning while drop partial text in msg
hi Sergey and Petr
> Hi,
> On (08/11/17 00:55), pierre kuo wrote:
> [..]
>> And people will be hard to find out some part of message is left behind.
>> (since the tail of original message is elegantly dropped by "\n")
>> That is the reason I try to add such warning in msg_print_text.
>
> have you ever seen it (the truncation) in real life?
The experimental steps are list as follows.
Feel free to give your comments.
Prerequisite:
a) kernel version:
commit: a80099a152d0 ("Merge tag 'xfs-4.13-merge-6' of
git://git.kernel.org/pub/scm/fs/xfs/xfs-linux")
1. Add below patch in log_store to tell the content and length of log
that saved in log_text(msg) for below step #2 .
@@ -629,6 +629,11 @@ static int log_store(int facility, int level,
msg->len = size;
/* insert message */
+ if (msg->text_len > 512) {
+ trace_printk("%s\n", log_text(msg));
+ trace_printk("msg->text_len %d\n", msg->text_len);
+ }
+
log_next_idx += msg->len;
log_next_seq++;
2. Use below kernel thread sample for adding the string to msg.
int per_cpu_thread_fn(void* data)
{
unsigned int index = 0;
unsigned int len = 0;
char* local_string = kzalloc(2048, GFP_KERNEL);
do {
len += sprintf((local_string + len), "this is the %d line\n", index++);
}while(len < 576);
printk_deferred("%s", local_string);
return 0;
}
3. After running above #2, here is trace output from #1
(from the output, total "29 lines" of local_string has successfully
saved in log_buf)
# cat /sys/kernel/debug/tracing/trace;
# tracer: nop
#
# _-=> irqs-off
# / _=> need-resched
#| / _---=> hardirq/softirq
#|| / _--=> preempt-depth
#||| / delay
# TASK-PID CPU# TIMESTAMP FUNCTION
# | | | | |
per_cpu_thread-81[000] d..126.555745: log_store: this is the 0 line
this is the 1 line
this is the 2 line
this is the 3 line
this is the 4 line
this is the 5 line
this is the 6 line
this is the 7 line
this is the 8 line
this is the 9 line
this is the 10 line
this is the 11 line
this is the 12 line
this is the 13 line
this is the 14 line
this is the 15 line
this is the 16 line
this is the 17 line
this is the 18 line
this is the 19 line
this is the 20 line
this is the 21 line
this is the 22 line
this is the 23 line
this is the 24 line
this is the 25 line
this is the 26 line
this is the 27 line
this is the 28 line
this is the 29 line
per_cpu_thread-81[000] d..126.555753: log_store: msg->text_len 589
4. Write a user mode programs with buffer size 2MB, triple size bigger
than the text length in msg we saved in above #2, and repeatedly
calling SYSLOG_ACTION_READ for getting the log.
Then the log we got will _NOT_ show over than "this is the 26 line" as
below, that mean line#27 ~ line#29 are missing.
(the source is attached as "simple_log.tar.bz2")
<4>[ 39.467710] this is the 0 line
<4>[ 39.467710] this is the 1 line
<4>[ 39.467710] this is the 2 line
<4>[ 39.467710] this is the 3 line
<4>[ 39.467710] this is the 4 line
<4>[ 39.467710] this is the 5 line
<4>[ 39.467710] this is the 6 line
<4>[ 39.467710] this is the 7 line
<4>[ 39.467710] this is the 8 line
<4>[ 39.467710] this is the 9 line
<4>[ 39.467710] this is the 10 line
<4>[ 39.467710] this is the 11 line
<4>[ 39.467710] this is the 12 line
<4>[ 39.467710] this is the 13 line
<4>[ 39.467710] this is the 14 line
<4>[ 39.467710] this is the 15 line
<4>[ 39.467710] this is the 16 line
<4>[ 39.467710] this is the 17 line
<4>[ 39.467710] this is the 18 line
<4>[ 39.467710] this is the 19 line
<4>[ 39.467710] this is the 20 line
<4>[ 39.467710] this is the 21 line
<4>[ 39.467710] this is the 22 line
<4>[ 39.467710] this is the 23 line
<4>[ 39.467710] this is the 24 line
<4>[ 39.467710] this is the 25 line
<4>[ 39.467710] this is the 26 line
simple_log.tar.bz2
Description: BZip2 compressed data
Re: [RFC V2] printk: add warning while drop partial text in msg
hi Sergey and Petr
> Hi,
> On (08/11/17 00:55), pierre kuo wrote:
> [..]
>> And people will be hard to find out some part of message is left behind.
>> (since the tail of original message is elegantly dropped by "\n")
>> That is the reason I try to add such warning in msg_print_text.
>
> have you ever seen it (the truncation) in real life?
The experimental steps are list as follows.
Feel free to give your comments.
Prerequisite:
a) kernel version:
commit: a80099a152d0 ("Merge tag 'xfs-4.13-merge-6' of
git://git.kernel.org/pub/scm/fs/xfs/xfs-linux")
1. Add below patch in log_store to tell the content and length of log
that saved in log_text(msg) for below step #2 .
@@ -629,6 +629,11 @@ static int log_store(int facility, int level,
msg->len = size;
/* insert message */
+ if (msg->text_len > 512) {
+ trace_printk("%s\n", log_text(msg));
+ trace_printk("msg->text_len %d\n", msg->text_len);
+ }
+
log_next_idx += msg->len;
log_next_seq++;
2. Use below kernel thread sample for adding the string to msg.
int per_cpu_thread_fn(void* data)
{
unsigned int index = 0;
unsigned int len = 0;
char* local_string = kzalloc(2048, GFP_KERNEL);
do {
len += sprintf((local_string + len), "this is the %d line\n", index++);
}while(len < 576);
printk_deferred("%s", local_string);
return 0;
}
3. After running above #2, here is trace output from #1
(from the output, total "29 lines" of local_string has successfully
saved in log_buf)
# cat /sys/kernel/debug/tracing/trace;
# tracer: nop
#
# _-=> irqs-off
# / _=> need-resched
#| / _---=> hardirq/softirq
#|| / _--=> preempt-depth
#||| / delay
# TASK-PID CPU# TIMESTAMP FUNCTION
# | | | | |
per_cpu_thread-81[000] d..126.555745: log_store: this is the 0 line
this is the 1 line
this is the 2 line
this is the 3 line
this is the 4 line
this is the 5 line
this is the 6 line
this is the 7 line
this is the 8 line
this is the 9 line
this is the 10 line
this is the 11 line
this is the 12 line
this is the 13 line
this is the 14 line
this is the 15 line
this is the 16 line
this is the 17 line
this is the 18 line
this is the 19 line
this is the 20 line
this is the 21 line
this is the 22 line
this is the 23 line
this is the 24 line
this is the 25 line
this is the 26 line
this is the 27 line
this is the 28 line
this is the 29 line
per_cpu_thread-81[000] d..126.555753: log_store: msg->text_len 589
4. Write a user mode programs with buffer size 2MB, triple size bigger
than the text length in msg we saved in above #2, and repeatedly
calling SYSLOG_ACTION_READ for getting the log.
Then the log we got will _NOT_ show over than "this is the 26 line" as
below, that mean line#27 ~ line#29 are missing.
(the source is attached as "simple_log.tar.bz2")
<4>[ 39.467710] this is the 0 line
<4>[ 39.467710] this is the 1 line
<4>[ 39.467710] this is the 2 line
<4>[ 39.467710] this is the 3 line
<4>[ 39.467710] this is the 4 line
<4>[ 39.467710] this is the 5 line
<4>[ 39.467710] this is the 6 line
<4>[ 39.467710] this is the 7 line
<4>[ 39.467710] this is the 8 line
<4>[ 39.467710] this is the 9 line
<4>[ 39.467710] this is the 10 line
<4>[ 39.467710] this is the 11 line
<4>[ 39.467710] this is the 12 line
<4>[ 39.467710] this is the 13 line
<4>[ 39.467710] this is the 14 line
<4>[ 39.467710] this is the 15 line
<4>[ 39.467710] this is the 16 line
<4>[ 39.467710] this is the 17 line
<4>[ 39.467710] this is the 18 line
<4>[ 39.467710] this is the 19 line
<4>[ 39.467710] this is the 20 line
<4>[ 39.467710] this is the 21 line
<4>[ 39.467710] this is the 22 line
<4>[ 39.467710] this is the 23 line
<4>[ 39.467710] this is the 24 line
<4>[ 39.467710] this is the 25 line
<4>[ 39.467710] this is the 26 line
simple_log.tar.bz2
Description: BZip2 compressed data
Re: [RFC V2] printk: add warning while drop partial text in msg
hi Sergey:
(Please ignore previous mail, I apologize for pressing send button too early :)
>> this is not the only place that can truncate the message.
>> vprintk_emit() can do so as well /* vscnprintf() */. but
>> I think we don't care that much. a user likely will notice
>> truncated messages. we report lost messages, because this
>> is a completely different sort of problem.
Usually people will more easily find message truncated from semantics
by vscnprintf, since it brute force truncate input message by the
upper limit of output buffer.
In msg_print_text, it use memchr to find "\n" as copying unit while
memcping msg->text to output buffer.
And people will be hard to find out some part of message is left behind.
(since the tail of original message is elegantly dropped by "\n")
That is the reason I try to add such warning in msg_print_text.
> your log_store(), invoked from msg_print_text(), will append the error
> message to the logbuf (tail), possibly far-far-far away from console_idx.
> so your "characters dropped" warning will appear much later.
Got ur point and thanks for your advising.
>> len += print_prefix(msg, syslog, buf + len);
>> memcpy(buf + len, text, text_len);
>
>
> but more importantly, msg_print_text() is called from several places. and
> can even be called from a user space, potentially triggering the same
> "" error log_store() over and over again, wiping out
> the actually important kernel messages. which is
> a) not nice
> and
> b) can be used to deliberately "hide" something really important.
>
how about we directly adding warning message in buffer instead of
log_store like below?
Appreciate your review and advising.
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index fc47863..fcd1dd4 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -557,6 +557,7 @@ static u32 msg_used_size(u16 text_len, u16
dict_len, u32 *pad_len)
*/
#define MAX_LOG_TAKE_PART 4
static const char trunc_msg[] = "";
+static const char drop_msg[] = "";
static u32 truncate_msg(u16 *text_len, u16 *trunc_msg_len,
u16 *dict_len, u32 *pad_len)
@@ -1264,8 +1265,14 @@ static size_t msg_print_text(const struct
printk_log *msg, bool syslog, char *bu
if (buf) {
if (print_prefix(msg, syslog, NULL) +
- text_len + 1 >= size - len)
+ text_len + 1 >= size - len) {
+ /* below adding warning message
+* related information into output buffer
+*/
+ if ((size - len) > strlen(drop_msg))
+ memcpy(buf + len, drop_msg,
strlen(drop_msg));
break;
+ }
len += print_prefix(msg, syslog, buf + len);
memcpy(buf + len, text, text_len);
Re: [RFC V2] printk: add warning while drop partial text in msg
hi Sergey:
(Please ignore previous mail, I apologize for pressing send button too early :)
>> this is not the only place that can truncate the message.
>> vprintk_emit() can do so as well /* vscnprintf() */. but
>> I think we don't care that much. a user likely will notice
>> truncated messages. we report lost messages, because this
>> is a completely different sort of problem.
Usually people will more easily find message truncated from semantics
by vscnprintf, since it brute force truncate input message by the
upper limit of output buffer.
In msg_print_text, it use memchr to find "\n" as copying unit while
memcping msg->text to output buffer.
And people will be hard to find out some part of message is left behind.
(since the tail of original message is elegantly dropped by "\n")
That is the reason I try to add such warning in msg_print_text.
> your log_store(), invoked from msg_print_text(), will append the error
> message to the logbuf (tail), possibly far-far-far away from console_idx.
> so your "characters dropped" warning will appear much later.
Got ur point and thanks for your advising.
>> len += print_prefix(msg, syslog, buf + len);
>> memcpy(buf + len, text, text_len);
>
>
> but more importantly, msg_print_text() is called from several places. and
> can even be called from a user space, potentially triggering the same
> "" error log_store() over and over again, wiping out
> the actually important kernel messages. which is
> a) not nice
> and
> b) can be used to deliberately "hide" something really important.
>
how about we directly adding warning message in buffer instead of
log_store like below?
Appreciate your review and advising.
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index fc47863..fcd1dd4 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -557,6 +557,7 @@ static u32 msg_used_size(u16 text_len, u16
dict_len, u32 *pad_len)
*/
#define MAX_LOG_TAKE_PART 4
static const char trunc_msg[] = "";
+static const char drop_msg[] = "";
static u32 truncate_msg(u16 *text_len, u16 *trunc_msg_len,
u16 *dict_len, u32 *pad_len)
@@ -1264,8 +1265,14 @@ static size_t msg_print_text(const struct
printk_log *msg, bool syslog, char *bu
if (buf) {
if (print_prefix(msg, syslog, NULL) +
- text_len + 1 >= size - len)
+ text_len + 1 >= size - len) {
+ /* below adding warning message
+* related information into output buffer
+*/
+ if ((size - len) > strlen(drop_msg))
+ memcpy(buf + len, drop_msg,
strlen(drop_msg));
break;
+ }
len += print_prefix(msg, syslog, buf + len);
memcpy(buf + len, text, text_len);
Re: [RFC V2] printk: add warning while drop partial text in msg
hi Sergey
> this is not the only place that can truncate the message.
> vprintk_emit() can do so as well /* vscnprintf() */. but
> I think we don't care that much. a user likely will notice
> truncated messages. we report lost messages, because this
> is a completely different sort of problem.
Usually people will more easily find message truncated from semantics
by vscnprintf,
since it brute force truncate input message by the upper limit of output buffer.
In msg_print_text, it use memchr to find "\n" as copying unit while
memcping msg->text to output buffer.
People will more difficultly to find out some part of message is left behind.
That is the reason I try to add such warning in msg_print_text.
>
>
> [..]
>> @@ -1264,8 +1270,23 @@ static size_t msg_print_text(const struct printk_log
>> *msg, bool syslog, char *bu
>>
>> if (buf) {
>> if (print_prefix(msg, syslog, NULL) +
>> - text_len + 1 >= size - len)
>> + text_len + 1 >= size - len) {
>> + /* below stores dropped characters
>> + * related information in next msg
>> + */
>> + size_t drop_len;
>> +
>> + drop_len = scnprintf(drop_msg,
>> + MAX_DROP_MSG_LENGTH,
>> + "<%u characters dropped>",
>> + (next) ?
>> + (unsigned int)(text_size + next -
>> text) :
>> + (unsigned int)text_size);
>> + drop_msg[drop_len] = 0;
>> + log_store(msg->facility, msg->level,
>> msg->flags,
>> + 0, NULL, 0, drop_msg,
>> strlen(drop_msg));
>> break;
>> + }
>
> this change, most likely, will confuse people. because msg_print_text() is
> called on a message that is being currently processed, which is not
> necessarily the last message in the logbuf. for example, see console_unlock().
> we do something like this:
>
> while (console_seq != log_next_seq) {
> msg = log_from_idx(console_idx);
> msg_print_text(msg);
> console_idx = log_next(console_idx);
> console_seq++;
> }
>
> your log_store(), invoked from msg_print_text(), will append the error
> message to the logbuf (tail), possibly far-far-far away from console_idx.
> so your "characters dropped" warning will appear much later.
>
>
>> len += print_prefix(msg, syslog, buf + len);
>> memcpy(buf + len, text, text_len);
>
>
> but more importantly, msg_print_text() is called from several places. and
> can even be called from a user space, potentially triggering the same
> "" error log_store() over and over again, wiping out
> the actually important kernel messages. which is
> a) not nice
> and
> b) can be used to deliberately "hide" something really important.
>
>
> so, no. sorry, I don't like this change.
>
> -ss
Re: [RFC V2] printk: add warning while drop partial text in msg
hi Sergey
> this is not the only place that can truncate the message.
> vprintk_emit() can do so as well /* vscnprintf() */. but
> I think we don't care that much. a user likely will notice
> truncated messages. we report lost messages, because this
> is a completely different sort of problem.
Usually people will more easily find message truncated from semantics
by vscnprintf,
since it brute force truncate input message by the upper limit of output buffer.
In msg_print_text, it use memchr to find "\n" as copying unit while
memcping msg->text to output buffer.
People will more difficultly to find out some part of message is left behind.
That is the reason I try to add such warning in msg_print_text.
>
>
> [..]
>> @@ -1264,8 +1270,23 @@ static size_t msg_print_text(const struct printk_log
>> *msg, bool syslog, char *bu
>>
>> if (buf) {
>> if (print_prefix(msg, syslog, NULL) +
>> - text_len + 1 >= size - len)
>> + text_len + 1 >= size - len) {
>> + /* below stores dropped characters
>> + * related information in next msg
>> + */
>> + size_t drop_len;
>> +
>> + drop_len = scnprintf(drop_msg,
>> + MAX_DROP_MSG_LENGTH,
>> + "<%u characters dropped>",
>> + (next) ?
>> + (unsigned int)(text_size + next -
>> text) :
>> + (unsigned int)text_size);
>> + drop_msg[drop_len] = 0;
>> + log_store(msg->facility, msg->level,
>> msg->flags,
>> + 0, NULL, 0, drop_msg,
>> strlen(drop_msg));
>> break;
>> + }
>
> this change, most likely, will confuse people. because msg_print_text() is
> called on a message that is being currently processed, which is not
> necessarily the last message in the logbuf. for example, see console_unlock().
> we do something like this:
>
> while (console_seq != log_next_seq) {
> msg = log_from_idx(console_idx);
> msg_print_text(msg);
> console_idx = log_next(console_idx);
> console_seq++;
> }
>
> your log_store(), invoked from msg_print_text(), will append the error
> message to the logbuf (tail), possibly far-far-far away from console_idx.
> so your "characters dropped" warning will appear much later.
>
>
>> len += print_prefix(msg, syslog, buf + len);
>> memcpy(buf + len, text, text_len);
>
>
> but more importantly, msg_print_text() is called from several places. and
> can even be called from a user space, potentially triggering the same
> "" error log_store() over and over again, wiping out
> the actually important kernel messages. which is
> a) not nice
> and
> b) can be used to deliberately "hide" something really important.
>
>
> so, no. sorry, I don't like this change.
>
> -ss
[RFC V2] printk: add warning while drop partial text in msg
If the buffer pass to msg_print_text is not big enough to put both all
prefixes and log_text(msg), kernel will quietly break.
That means the user may not have the chance to know whether the
log_text(msg) is fully printed into buffer or not.
In this patch, once above case happened, we try to calculate how many
characters of log_text(msg) are dropped and add warning for debugging
purpose.
Signed-off-by: pierre Kuo <vichy@gmail.com>
---
Changes since v2:
* fix typo in commit message from "waring for debugging purpose" to "warning
for debugging purpose"
kernel/printk/printk.c | 23 ++-
1 file changed, 22 insertions(+), 1 deletion(-)
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index fc47863..6cbb3699 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -558,6 +558,12 @@ static u32 msg_used_size(u16 text_len, u16 dict_len, u32
*pad_len)
#define MAX_LOG_TAKE_PART 4
static const char trunc_msg[] = "";
+/*
+ * Define max drop msg length that we put in next msg
+ */
+#define MAX_DROP_MSG_LENGTH 32
+static char drop_msg[MAX_DROP_MSG_LENGTH];
+
static u32 truncate_msg(u16 *text_len, u16 *trunc_msg_len,
u16 *dict_len, u32 *pad_len)
{
@@ -1264,8 +1270,23 @@ static size_t msg_print_text(const struct printk_log
*msg, bool syslog, char *bu
if (buf) {
if (print_prefix(msg, syslog, NULL) +
- text_len + 1 >= size - len)
+ text_len + 1 >= size - len) {
+ /* below stores dropped characters
+* related information in next msg
+*/
+ size_t drop_len;
+
+ drop_len = scnprintf(drop_msg,
+ MAX_DROP_MSG_LENGTH,
+ "<%u characters dropped>",
+ (next) ?
+ (unsigned int)(text_size + next - text)
:
+ (unsigned int)text_size);
+ drop_msg[drop_len] = 0;
+ log_store(msg->facility, msg->level, msg->flags,
+ 0, NULL, 0, drop_msg, strlen(drop_msg));
break;
+ }
len += print_prefix(msg, syslog, buf + len);
memcpy(buf + len, text, text_len);
--
1.9.1
[RFC V2] printk: add warning while drop partial text in msg
If the buffer pass to msg_print_text is not big enough to put both all
prefixes and log_text(msg), kernel will quietly break.
That means the user may not have the chance to know whether the
log_text(msg) is fully printed into buffer or not.
In this patch, once above case happened, we try to calculate how many
characters of log_text(msg) are dropped and add warning for debugging
purpose.
Signed-off-by: pierre Kuo
---
Changes since v2:
* fix typo in commit message from "waring for debugging purpose" to "warning
for debugging purpose"
kernel/printk/printk.c | 23 ++-
1 file changed, 22 insertions(+), 1 deletion(-)
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index fc47863..6cbb3699 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -558,6 +558,12 @@ static u32 msg_used_size(u16 text_len, u16 dict_len, u32
*pad_len)
#define MAX_LOG_TAKE_PART 4
static const char trunc_msg[] = "";
+/*
+ * Define max drop msg length that we put in next msg
+ */
+#define MAX_DROP_MSG_LENGTH 32
+static char drop_msg[MAX_DROP_MSG_LENGTH];
+
static u32 truncate_msg(u16 *text_len, u16 *trunc_msg_len,
u16 *dict_len, u32 *pad_len)
{
@@ -1264,8 +1270,23 @@ static size_t msg_print_text(const struct printk_log
*msg, bool syslog, char *bu
if (buf) {
if (print_prefix(msg, syslog, NULL) +
- text_len + 1 >= size - len)
+ text_len + 1 >= size - len) {
+ /* below stores dropped characters
+* related information in next msg
+*/
+ size_t drop_len;
+
+ drop_len = scnprintf(drop_msg,
+ MAX_DROP_MSG_LENGTH,
+ "<%u characters dropped>",
+ (next) ?
+ (unsigned int)(text_size + next - text)
:
+ (unsigned int)text_size);
+ drop_msg[drop_len] = 0;
+ log_store(msg->facility, msg->level, msg->flags,
+ 0, NULL, 0, drop_msg, strlen(drop_msg));
break;
+ }
len += print_prefix(msg, syslog, buf + len);
memcpy(buf + len, text, text_len);
--
1.9.1
[RFC] printk: add warning while drop partial text in msg
If the buffer pass to msg_print_text is not big enough to put both all
prefixes and log_text(msg), kernel will quietly break.
That means the user may not have the chance to know whether the
log_text(msg) is fully printed into buffer or not.
In this patch, once above case happened, we try to calculate how many
characters of log_text(msg) are dropped and add waring for debugging
purpose.
Signed-off-by: pierre Kuo <vichy@gmail.com>
---
kernel/printk/printk.c | 23 ++-
1 file changed, 22 insertions(+), 1 deletion(-)
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index fc47863..6cbb3699 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -558,6 +558,12 @@ static u32 msg_used_size(u16 text_len, u16 dict_len, u32
*pad_len)
#define MAX_LOG_TAKE_PART 4
static const char trunc_msg[] = "";
+/*
+ * Define max drop msg length that we put in next msg
+ */
+#define MAX_DROP_MSG_LENGTH 32
+static char drop_msg[MAX_DROP_MSG_LENGTH];
+
static u32 truncate_msg(u16 *text_len, u16 *trunc_msg_len,
u16 *dict_len, u32 *pad_len)
{
@@ -1264,8 +1270,23 @@ static size_t msg_print_text(const struct printk_log
*msg, bool syslog, char *bu
if (buf) {
if (print_prefix(msg, syslog, NULL) +
- text_len + 1 >= size - len)
+ text_len + 1 >= size - len) {
+ /* below stores dropped characters
+* related information in next msg
+*/
+ size_t drop_len;
+
+ drop_len = scnprintf(drop_msg,
+ MAX_DROP_MSG_LENGTH,
+ "<%u characters dropped>",
+ (next) ?
+ (unsigned int)(text_size + next - text)
:
+ (unsigned int)text_size);
+ drop_msg[drop_len] = 0;
+ log_store(msg->facility, msg->level, msg->flags,
+ 0, NULL, 0, drop_msg, strlen(drop_msg));
break;
+ }
len += print_prefix(msg, syslog, buf + len);
memcpy(buf + len, text, text_len);
--
1.9.1
[RFC] printk: add warning while drop partial text in msg
If the buffer pass to msg_print_text is not big enough to put both all
prefixes and log_text(msg), kernel will quietly break.
That means the user may not have the chance to know whether the
log_text(msg) is fully printed into buffer or not.
In this patch, once above case happened, we try to calculate how many
characters of log_text(msg) are dropped and add waring for debugging
purpose.
Signed-off-by: pierre Kuo
---
kernel/printk/printk.c | 23 ++-
1 file changed, 22 insertions(+), 1 deletion(-)
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index fc47863..6cbb3699 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -558,6 +558,12 @@ static u32 msg_used_size(u16 text_len, u16 dict_len, u32
*pad_len)
#define MAX_LOG_TAKE_PART 4
static const char trunc_msg[] = "";
+/*
+ * Define max drop msg length that we put in next msg
+ */
+#define MAX_DROP_MSG_LENGTH 32
+static char drop_msg[MAX_DROP_MSG_LENGTH];
+
static u32 truncate_msg(u16 *text_len, u16 *trunc_msg_len,
u16 *dict_len, u32 *pad_len)
{
@@ -1264,8 +1270,23 @@ static size_t msg_print_text(const struct printk_log
*msg, bool syslog, char *bu
if (buf) {
if (print_prefix(msg, syslog, NULL) +
- text_len + 1 >= size - len)
+ text_len + 1 >= size - len) {
+ /* below stores dropped characters
+* related information in next msg
+*/
+ size_t drop_len;
+
+ drop_len = scnprintf(drop_msg,
+ MAX_DROP_MSG_LENGTH,
+ "<%u characters dropped>",
+ (next) ?
+ (unsigned int)(text_size + next - text)
:
+ (unsigned int)text_size);
+ drop_msg[drop_len] = 0;
+ log_store(msg->facility, msg->level, msg->flags,
+ 0, NULL, 0, drop_msg, strlen(drop_msg));
break;
+ }
len += print_prefix(msg, syslog, buf + len);
memcpy(buf + len, text, text_len);
--
1.9.1
Re: [PATCH] printk: modify console_unlock with printk-safe macros
hi Sergey and Andy:
> On (07/15/17 18:36), Pierre Kuo wrote:
> [..]
>> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
>> index fc47863..21557cc 100644
>> --- a/kernel/printk/printk.c
>> +++ b/kernel/printk/printk.c
>> @@ -2194,8 +2194,7 @@ void console_unlock(void)
>> size_t ext_len = 0;
>> size_t len;
>>
>> - printk_safe_enter_irqsave(flags);
>> - raw_spin_lock(_lock);
>> + logbuf_lock_irqsave(flags);
>> if (seen_seq != log_next_seq) {
>> wake_klogd = true;
>> seen_seq = log_next_seq;
>> @@ -2267,8 +2266,7 @@ void console_unlock(void)
>>*/
>> raw_spin_lock(_lock);
>> retry = console_seq != log_next_seq;
>> - raw_spin_unlock(_lock);
>> - printk_safe_exit_irqrestore(flags);
>> + logbuf_unlock_irqrestore(flags);
>>
>> if (retry && console_trylock())
>> goto again;
>
> I did it that particular way for a reason - console_unlock() does a
> bunch of tricks: unlocking logbuf in the middle of printing loop,
> breaking out of loop with local IRQs disabled, re-taking the logbuf
> after the loop still will local IRQs disabled, etc. etc. I didn't
> want to (and still don't) mix-in logbuf macros; we do things that
> macros don't cover anyway. sorry, I don't agree that the patch
> improves readability.
Got ur points and appreciate for your illustration. ^^
Thanks a lot,
Re: [PATCH] printk: modify console_unlock with printk-safe macros
hi Sergey and Andy:
> On (07/15/17 18:36), Pierre Kuo wrote:
> [..]
>> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
>> index fc47863..21557cc 100644
>> --- a/kernel/printk/printk.c
>> +++ b/kernel/printk/printk.c
>> @@ -2194,8 +2194,7 @@ void console_unlock(void)
>> size_t ext_len = 0;
>> size_t len;
>>
>> - printk_safe_enter_irqsave(flags);
>> - raw_spin_lock(_lock);
>> + logbuf_lock_irqsave(flags);
>> if (seen_seq != log_next_seq) {
>> wake_klogd = true;
>> seen_seq = log_next_seq;
>> @@ -2267,8 +2266,7 @@ void console_unlock(void)
>>*/
>> raw_spin_lock(_lock);
>> retry = console_seq != log_next_seq;
>> - raw_spin_unlock(_lock);
>> - printk_safe_exit_irqrestore(flags);
>> + logbuf_unlock_irqrestore(flags);
>>
>> if (retry && console_trylock())
>> goto again;
>
> I did it that particular way for a reason - console_unlock() does a
> bunch of tricks: unlocking logbuf in the middle of printing loop,
> breaking out of loop with local IRQs disabled, re-taking the logbuf
> after the loop still will local IRQs disabled, etc. etc. I didn't
> want to (and still don't) mix-in logbuf macros; we do things that
> macros don't cover anyway. sorry, I don't agree that the patch
> improves readability.
Got ur points and appreciate for your illustration. ^^
Thanks a lot,
[PATCH] printk: modify console_unlock with printk-safe macros
From: pierre Kuo <vichy@gmail.com>
In commit de6fcbdb68b2 ("printk: convert the rest to printk-safe"), we
create 4 helper macros to make printk-safe usage easier.
Here we modify some part of console_unlock with above marcros.
Signed-off-by: Pierre Kuo <vichy@gmail.com>
---
kernel/printk/printk.c |6 ++
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index fc47863..21557cc 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -2194,8 +2194,7 @@ void console_unlock(void)
size_t ext_len = 0;
size_t len;
- printk_safe_enter_irqsave(flags);
- raw_spin_lock(_lock);
+ logbuf_lock_irqsave(flags);
if (seen_seq != log_next_seq) {
wake_klogd = true;
seen_seq = log_next_seq;
@@ -2267,8 +2266,7 @@ void console_unlock(void)
*/
raw_spin_lock(_lock);
retry = console_seq != log_next_seq;
- raw_spin_unlock(_lock);
- printk_safe_exit_irqrestore(flags);
+ logbuf_unlock_irqrestore(flags);
if (retry && console_trylock())
goto again;
--
1.7.9.5
[PATCH] printk: modify console_unlock with printk-safe macros
From: pierre Kuo
In commit de6fcbdb68b2 ("printk: convert the rest to printk-safe"), we
create 4 helper macros to make printk-safe usage easier.
Here we modify some part of console_unlock with above marcros.
Signed-off-by: Pierre Kuo
---
kernel/printk/printk.c |6 ++
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index fc47863..21557cc 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -2194,8 +2194,7 @@ void console_unlock(void)
size_t ext_len = 0;
size_t len;
- printk_safe_enter_irqsave(flags);
- raw_spin_lock(_lock);
+ logbuf_lock_irqsave(flags);
if (seen_seq != log_next_seq) {
wake_klogd = true;
seen_seq = log_next_seq;
@@ -2267,8 +2266,7 @@ void console_unlock(void)
*/
raw_spin_lock(_lock);
retry = console_seq != log_next_seq;
- raw_spin_unlock(_lock);
- printk_safe_exit_irqrestore(flags);
+ logbuf_unlock_irqrestore(flags);
if (retry && console_trylock())
goto again;
--
1.7.9.5
[PATCH v2] printk: Modify operators of printed_len and text_len
With commit ("printk: report lost messages in printk
safe/nmi contexts") and commit <8b1742c9c207> ("printk: remove zap_locks()
function"), it seems we can remove initialization, "=0", of text_len and
directly assign result of log_output to printed_len.
Signed-off-by: Pierre Kuo <vichy@gmail.com>
Reviewed-by: Sergey Senozhatsky <sergey.senozhat...@gmail.com>
---
Changes since v2:
* Per Petr's friendly reminder, add description of ("printk:
report lost messages in printk
safe/nmi contexts") and remove "=0", the initialization, of text_len.
kernel/printk/printk.c |6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index fc47863..229fbdcb 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -1698,10 +1698,10 @@ asmlinkage int vprintk_emit(int facility, int level,
{
static char textbuf[LOG_LINE_MAX];
char *text = textbuf;
- size_t text_len = 0;
+ size_t text_len;
enum log_flags lflags = 0;
unsigned long flags;
- int printed_len = 0;
+ int printed_len;
bool in_sched = false;
if (level == LOGLEVEL_SCHED) {
@@ -1754,7 +1754,7 @@ asmlinkage int vprintk_emit(int facility, int level,
if (dict)
lflags |= LOG_PREFIX|LOG_NEWLINE;
- printed_len += log_output(facility, level, lflags, dict, dictlen, text,
text_len);
+ printed_len = log_output(facility, level, lflags, dict, dictlen, text,
text_len);
logbuf_unlock_irqrestore(flags);
--
1.7.9.5
[PATCH v2] printk: Modify operators of printed_len and text_len
With commit ("printk: report lost messages in printk
safe/nmi contexts") and commit <8b1742c9c207> ("printk: remove zap_locks()
function"), it seems we can remove initialization, "=0", of text_len and
directly assign result of log_output to printed_len.
Signed-off-by: Pierre Kuo
Reviewed-by: Sergey Senozhatsky
---
Changes since v2:
* Per Petr's friendly reminder, add description of ("printk:
report lost messages in printk
safe/nmi contexts") and remove "=0", the initialization, of text_len.
kernel/printk/printk.c |6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index fc47863..229fbdcb 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -1698,10 +1698,10 @@ asmlinkage int vprintk_emit(int facility, int level,
{
static char textbuf[LOG_LINE_MAX];
char *text = textbuf;
- size_t text_len = 0;
+ size_t text_len;
enum log_flags lflags = 0;
unsigned long flags;
- int printed_len = 0;
+ int printed_len;
bool in_sched = false;
if (level == LOGLEVEL_SCHED) {
@@ -1754,7 +1754,7 @@ asmlinkage int vprintk_emit(int facility, int level,
if (dict)
lflags |= LOG_PREFIX|LOG_NEWLINE;
- printed_len += log_output(facility, level, lflags, dict, dictlen, text,
text_len);
+ printed_len = log_output(facility, level, lflags, dict, dictlen, text,
text_len);
logbuf_unlock_irqrestore(flags);
--
1.7.9.5
Re: [PATCH] printk: Modify operators of printed_len
hi Petr
> I just noticed that the same applies also to text_len
> variable. Well, it was caused by another commit ddb9baa822265b55
> ("printk: report lost messages in printk safe/nmi contexts").
> Could you please send a patch for this as well?
sure and it is my pleasure.
>
> This seems to be your first patch sent to the kernel mailing list.
Yes :-)
> There is a standard format how to reference older commits. It is
> 'commit <12+ chars of sha1> ("")', see my comment above
> for an example.
>
> A good practice is to run ./scripts/checkpatch.pl before
> you send the patch. Well, you need to use a common sense and ignore
> false positives or hints that make a particular patch less readable
> in the end.
>
> Also it is handy to bump the version of the patch when it is
> updated, e.g. use [PATCH v2] in the subject. People also
> summarize changes against the previous version(s) below
> the --- line. Well, this is more useful when there is a longer
> delay between the versions and the changes are more complicated.
Really appreciate hints you provided and I will send the v2 patch soon.
Best Regards.
Re: [PATCH] printk: Modify operators of printed_len
hi Petr
> I just noticed that the same applies also to text_len
> variable. Well, it was caused by another commit ddb9baa822265b55
> ("printk: report lost messages in printk safe/nmi contexts").
> Could you please send a patch for this as well?
sure and it is my pleasure.
>
> This seems to be your first patch sent to the kernel mailing list.
Yes :-)
> There is a standard format how to reference older commits. It is
> 'commit <12+ chars of sha1> ("")', see my comment above
> for an example.
>
> A good practice is to run ./scripts/checkpatch.pl before
> you send the patch. Well, you need to use a common sense and ignore
> false positives or hints that make a particular patch less readable
> in the end.
>
> Also it is handy to bump the version of the patch when it is
> updated, e.g. use [PATCH v2] in the subject. People also
> summarize changes against the previous version(s) below
> the --- line. Well, this is more useful when there is a longer
> delay between the versions and the changes are more complicated.
Really appreciate hints you provided and I will send the v2 patch soon.
Best Regards.
[PATCH] printk: Modify operators of printed_len
In 8b1742c9c207, we remove printk-recursion detection code in
vprintk_emit(), where it is the first place that printed_len calculated.
After removing above detection, it seems we can directly assign the
result of log_output to printed_len.
Signed-off-by: Pierre Kuo <vichy@gmail.com>
---
kernel/printk/printk.c |4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index fc47863..a2a8cac 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -1701,7 +1701,7 @@ asmlinkage int vprintk_emit(int facility, int level,
size_t text_len = 0;
enum log_flags lflags = 0;
unsigned long flags;
- int printed_len = 0;
+ int printed_len;
bool in_sched = false;
if (level == LOGLEVEL_SCHED) {
@@ -1754,7 +1754,7 @@ asmlinkage int vprintk_emit(int facility, int level,
if (dict)
lflags |= LOG_PREFIX|LOG_NEWLINE;
- printed_len += log_output(facility, level, lflags, dict, dictlen, text,
text_len);
+ printed_len = log_output(facility, level, lflags, dict, dictlen, text,
text_len);
logbuf_unlock_irqrestore(flags);
--
1.7.9.5
[PATCH] printk: Modify operators of printed_len
In 8b1742c9c207, we remove printk-recursion detection code in
vprintk_emit(), where it is the first place that printed_len calculated.
After removing above detection, it seems we can directly assign the
result of log_output to printed_len.
Signed-off-by: Pierre Kuo
---
kernel/printk/printk.c |4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index fc47863..a2a8cac 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -1701,7 +1701,7 @@ asmlinkage int vprintk_emit(int facility, int level,
size_t text_len = 0;
enum log_flags lflags = 0;
unsigned long flags;
- int printed_len = 0;
+ int printed_len;
bool in_sched = false;
if (level == LOGLEVEL_SCHED) {
@@ -1754,7 +1754,7 @@ asmlinkage int vprintk_emit(int facility, int level,
if (dict)
lflags |= LOG_PREFIX|LOG_NEWLINE;
- printed_len += log_output(facility, level, lflags, dict, dictlen, text,
text_len);
+ printed_len = log_output(facility, level, lflags, dict, dictlen, text,
text_len);
logbuf_unlock_irqrestore(flags);
--
1.7.9.5
Re: [PATCH] printk: Modify operators of printed_len
hi Joe >> > [] >> > > diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c >> > >> > [] >> > > @@ -1754,7 +1754,7 @@ asmlinkage int vprintk_emit(int facility, int >> > > level, >> > > if (dict) >> > > lflags |= LOG_PREFIX|LOG_NEWLINE; >> > > >> > > - printed_len += log_output(facility, level, lflags, dict, dictlen, >> > > text, text_len); >> > > + printed_len = log_output(facility, level, lflags, dict, dictlen, >> > > text, text_len); >> > >> > If this is appropriate, this should also remove the >> > initialization of printed_len and perhaps rename it too. >> >> I cannot quite understand the reason why need to rename. >> printed_len seems meet the meaning we expect for here. > > Verbosity. To me, len would be adequate. > > Anyway, the real point was the declaration of printed_len could > remove the " = 0" as it's now only set once. Got it and I will resend the patch again. Appreciate your kind advice.
Re: [PATCH] printk: Modify operators of printed_len
hi Joe >> > [] >> > > diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c >> > >> > [] >> > > @@ -1754,7 +1754,7 @@ asmlinkage int vprintk_emit(int facility, int >> > > level, >> > > if (dict) >> > > lflags |= LOG_PREFIX|LOG_NEWLINE; >> > > >> > > - printed_len += log_output(facility, level, lflags, dict, dictlen, >> > > text, text_len); >> > > + printed_len = log_output(facility, level, lflags, dict, dictlen, >> > > text, text_len); >> > >> > If this is appropriate, this should also remove the >> > initialization of printed_len and perhaps rename it too. >> >> I cannot quite understand the reason why need to rename. >> printed_len seems meet the meaning we expect for here. > > Verbosity. To me, len would be adequate. > > Anyway, the real point was the declaration of printed_len could > remove the " = 0" as it's now only set once. Got it and I will resend the patch again. Appreciate your kind advice.
Re: [PATCH] printk: Modify operators of printed_len
hi Joe: 2017-07-08 1:12 GMT+08:00 Joe Perches <j...@perches.com>: > On Sat, 2017-07-08 at 00:30 +0800, Pierre Kuo wrote: >> In 8b1742c9c207, we remove printk-recursion detection code in >> vprintk_emit(), where it is the first place that printed_len calculated. >> After removing above detection, it seems we can directly assign the >> result of log_output to printed_len. > [] >> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c > [] >> @@ -1754,7 +1754,7 @@ asmlinkage int vprintk_emit(int facility, int level, >> if (dict) >> lflags |= LOG_PREFIX|LOG_NEWLINE; >> >> - printed_len += log_output(facility, level, lflags, dict, dictlen, >> text, text_len); >> + printed_len = log_output(facility, level, lflags, dict, dictlen, text, >> text_len); > > If this is appropriate, this should also remove the > initialization of printed_len and perhaps rename it too. I cannot quite understand the reason why need to rename. printed_len seems meet the meaning we expect for here. thanks for your friendly comment.
Re: [PATCH] printk: Modify operators of printed_len
hi Joe: 2017-07-08 1:12 GMT+08:00 Joe Perches : > On Sat, 2017-07-08 at 00:30 +0800, Pierre Kuo wrote: >> In 8b1742c9c207, we remove printk-recursion detection code in >> vprintk_emit(), where it is the first place that printed_len calculated. >> After removing above detection, it seems we can directly assign the >> result of log_output to printed_len. > [] >> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c > [] >> @@ -1754,7 +1754,7 @@ asmlinkage int vprintk_emit(int facility, int level, >> if (dict) >> lflags |= LOG_PREFIX|LOG_NEWLINE; >> >> - printed_len += log_output(facility, level, lflags, dict, dictlen, >> text, text_len); >> + printed_len = log_output(facility, level, lflags, dict, dictlen, text, >> text_len); > > If this is appropriate, this should also remove the > initialization of printed_len and perhaps rename it too. I cannot quite understand the reason why need to rename. printed_len seems meet the meaning we expect for here. thanks for your friendly comment.
[PATCH] printk: Modify operators of printed_len
In 8b1742c9c207, we remove printk-recursion detection code in vprintk_emit(), where it is the first place that printed_len calculated. After removing above detection, it seems we can directly assign the result of log_output to printed_len. Signed-off-by: Pierre Kuo <vichy@gmail.com> --- kernel/printk/printk.c |2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c index fc47863..16f3a61 100644 --- a/kernel/printk/printk.c +++ b/kernel/printk/printk.c @@ -1754,7 +1754,7 @@ asmlinkage int vprintk_emit(int facility, int level, if (dict) lflags |= LOG_PREFIX|LOG_NEWLINE; - printed_len += log_output(facility, level, lflags, dict, dictlen, text, text_len); + printed_len = log_output(facility, level, lflags, dict, dictlen, text, text_len); logbuf_unlock_irqrestore(flags); -- 1.7.9.5
[PATCH] printk: Modify operators of printed_len
In 8b1742c9c207, we remove printk-recursion detection code in vprintk_emit(), where it is the first place that printed_len calculated. After removing above detection, it seems we can directly assign the result of log_output to printed_len. Signed-off-by: Pierre Kuo --- kernel/printk/printk.c |2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c index fc47863..16f3a61 100644 --- a/kernel/printk/printk.c +++ b/kernel/printk/printk.c @@ -1754,7 +1754,7 @@ asmlinkage int vprintk_emit(int facility, int level, if (dict) lflags |= LOG_PREFIX|LOG_NEWLINE; - printed_len += log_output(facility, level, lflags, dict, dictlen, text, text_len); + printed_len = log_output(facility, level, lflags, dict, dictlen, text, text_len); logbuf_unlock_irqrestore(flags); -- 1.7.9.5
