Re: [PATCH 3/3] mtd: phram: Fix error return code in phram_setup()
On 2021/04/08 20:46, Miquel Raynal wrote:
Hi Yu,
Yu Kuai wrote on Thu, 8 Apr 2021 19:15:14 +0800:
Fix to return a negative error code from the error handling
case instead of 0, as done elsewhere in this function.
Reported-by: Hulk Robot
Signed-off-by: Yu Kuai
---
drivers/mtd/devices/phram.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/mtd/devices/phram.c b/drivers/mtd/devices/phram.c
index 5b04ae6c3057..6ed6c51fac69 100644
--- a/drivers/mtd/devices/phram.c
+++ b/drivers/mtd/devices/phram.c
@@ -270,6 +270,7 @@ static int phram_setup(const char *val)
if (len == 0 || erasesize == 0 || erasesize > len
|| erasesize > UINT_MAX || rem) {
parse_err("illegal erasesize or len\n");
+ ret = -EINVAL;
goto error;
}
It looks like you're doing the opposite of what you say.
Hi,
sorry about that, I misunderstood 'fix to'.
Thanks
Yu Kuai
Thanks,
Miquèl
.
Re: [PATCH 2/2] powerpc: make 'boot_text_mapped' static
On 2021/04/08 13:04, Christophe Leroy wrote:
Le 08/04/2021 à 03:18, Yu Kuai a écrit :
The sparse tool complains as follow:
arch/powerpc/kernel/btext.c:48:5: warning:
symbol 'boot_text_mapped' was not declared. Should it be static?
This symbol is not used outside of btext.c, so this commit make
it static.
Signed-off-by: Yu Kuai
---
arch/powerpc/kernel/btext.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/powerpc/kernel/btext.c b/arch/powerpc/kernel/btext.c
index 359d0f4ca532..8df9230be6fa 100644
--- a/arch/powerpc/kernel/btext.c
+++ b/arch/powerpc/kernel/btext.c
@@ -45,7 +45,7 @@ unsigned long disp_BAT[2] __initdata = {0, 0};
static unsigned char vga_font[cmapsz];
-int boot_text_mapped __force_data = 0;
+static int boot_text_mapped __force_data;
Are you sure the initialisation to 0 can be removed ? Usually
initialisation to 0 is not needed because not initialised variables go
in the BSS section which is zeroed at startup. But here the variable is
flagged with __force_data so it is not going in the BSS section.
Hi,
I removed the initialisation to 0 because checkpatch complained about
it, I do not familiar with '__force_data', thanks for pointing it out.
Thanks,
Yu Kuai
extern void rmci_on(void);
extern void rmci_off(void);
.
Re: [PATCH 1/2] powerpc: remove set but not used variable 'force_printk_to_btext'
On 2021/04/08 13:01, Christophe Leroy wrote:
Le 08/04/2021 à 03:18, Yu Kuai a écrit :
Fixes gcc '-Wunused-but-set-variable' warning:
arch/powerpc/kernel/btext.c:49:12: error: 'force_printk_to_btext'
defined but not used.
You don't get this error as it is now.
You will get this error only if you make it 'static', which is what you
did in your first patch based on the 'sparse' report.
When removing a non static variable, you should explain that you can
remove it after you have verifier that it is nowhere used, neither in
that file nor in any other one.
Hi,
I do use 'git grep force_printk_to_btext' to confirm that
'force_printk_to_btext' is not used anywhere. Maybe it's better to
metion it in commit message?
Thanks
Yu Kuai
It is never used, and so can be removed.
Signed-off-by: Yu Kuai
---
arch/powerpc/kernel/btext.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/arch/powerpc/kernel/btext.c b/arch/powerpc/kernel/btext.c
index 803c2a45b22a..359d0f4ca532 100644
--- a/arch/powerpc/kernel/btext.c
+++ b/arch/powerpc/kernel/btext.c
@@ -46,7 +46,6 @@ unsigned long disp_BAT[2] __initdata = {0, 0};
static unsigned char vga_font[cmapsz];
int boot_text_mapped __force_data = 0;
-int force_printk_to_btext = 0;
extern void rmci_on(void);
extern void rmci_off(void);
.
Re: [PATCH] powerpc: Make some symbols static
On 2021/04/08 0:57, kernel test robot wrote: Hi Yu, Thank you for the patch! Yet something to improve: [auto build test ERROR on powerpc/next] [also build test ERROR on v5.12-rc6 next-20210407] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch] url: https://github.com/0day-ci/linux/commits/Yu-Kuai/powerpc-Make-some-symbols-static/20210407-205258 base: https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git next config: powerpc64-defconfig (attached as .config) compiler: powerpc64-linux-gcc (GCC) 9.3.0 reproduce (this is a W=1 build): wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # https://github.com/0day-ci/linux/commit/7c0f3f68006b9b42ce944b02a2059128cc5826ec git remote add linux-review https://github.com/0day-ci/linux git fetch --no-tags linux-review Yu-Kuai/powerpc-Make-some-symbols-static/20210407-205258 git checkout 7c0f3f68006b9b42ce944b02a2059128cc5826ec # save the attached .config to linux build tree COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=powerpc64 If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot All errors (new ones prefixed by >>): arch/powerpc/kernel/btext.c:49:12: error: 'force_printk_to_btext' defined but not used [-Werror=unused-variable] 49 | static int force_printk_to_btext; |^ cc1: all warnings being treated as errors vim +/force_printk_to_btext +49 arch/powerpc/kernel/btext.c 47 48 static int boot_text_mapped __force_data; > 49 static int force_printk_to_btext; 50 Will remove this variable in another patch. Thanks Yu Kuai --- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-...@lists.01.org
Re: [PATCH 1/3] blk-mq: allow hardware queue to get more tag while sharing a tag set
Hi On 2020/12/29 9:15, Ming Lei wrote: Just wondering why you try to set 128 via sysfs for all disks? If you do that, you should know the potential result given the whole tags queue depth is just 128. It's just a extreme example to show the unexpected result of "always return true from hctx_may_queue()". Thanks, Yu Kuai
Re: [PATCH 1/3] blk-mq: allow hardware queue to get more tag while sharing a tag set
Hi On 2020/12/28 16:28, Ming Lei wrote: Another candidate solution may be to always return true from hctx_may_queue() for this kind of queue because queue_depth has provided fair allocation for each LUN, and looks not necessary to do that again. If always return true from hctx_may_queue() in this case, for example, we set queue_depth to 128(if can't, the biggger, the better) for all disks, and test with numjobs=64. The result should be one disk with high iops, and the rest very low. So I think it's better to ensure the max tags a disk can get in this case. Thanks! Yu Kuai
Re: [PATCH 1/3] blk-mq: allow hardware queue to get more tag while sharing a tag set
Hi, On 2020/12/27 19:58, Ming Lei wrote: Hi Yu Kuai, On Sat, Dec 26, 2020 at 06:28:06PM +0800, Yu Kuai wrote: When sharing a tag set, if most disks are issuing small amount of IO, and only a few is issuing a large amount of IO. Current approach is to limit the max amount of tags a disk can get equally to the average of total tags. Thus the few heavy load disk can't get enough tags while many tags are still free in the tag set. Yeah, current approach just allocates same share for each active queue which is evaluated in each timeout period. That said you are trying to improve the following case: - heavy IO on one or several disks, and the average share for these disks become bottleneck of IO performance - small amount IO on other disks attached to the same host, and all IOs are submitted to disk in <30 second period. Just wondering if you may share the workload you are trying to optimize, or it is just one improvement in theory? And what is the disk(hdd, ssd or nvme) and host? And how many disks in your setting? And how deep the tagset depth is? The details of the environment that we found the problem are as follows: total driver tags: 128 number of disks: 13 (network drive, and they form a dm-multipath) default queue_depth: 32 disk performance: when test with 4k randread and single thread, iops is 300. And can up to 4000 with 32 thread. test cmd: fio -ioengine=psync -numjobs=32 ... We found that mpath will issue sg_io periodically(about 15s)，which lead to active_queues setting to 13 for about 5s in every 15s. By the way, I'm not sure this is a common scenario, however, sq don't have such problem, Thanks Yu Kuai
Re: [RFC PATCH] blk-cgroup: prevent rcu_sched detected stalls warnings in blkg_destroy_all()
On 2020/11/25 20:32, Tejun Heo wrote:
Hello,
Thanks for the fix. A couple comments below.
On Sat, Nov 21, 2020 at 04:34:20PM +0800, Yu Kuai wrote:
+#define BLKG_DESTROY_BATH 4096
I think you meant BLKG_DESTROY_BATCH.
static void blkg_destroy_all(struct request_queue *q)
{
struct blkcg_gq *blkg, *n;
+ int count = BLKG_DESTROY_BATH;
But might as well just write 4096 here.
spin_lock_irq(>queue_lock);
list_for_each_entry_safe(blkg, n, >blkg_list, q_node) {
struct blkcg *blkcg = blkg->blkcg;
+ /*
+* If the list is too long, the loop can took a long time,
+* thus relese the lock for a while when a batch of blkcg
+* were destroyed.
+*/
+ if (!(--count)) {
+ count = BLKG_DESTROY_BATH;
+ spin_unlock_irq(>queue_lock);
+ cond_resched();
+ spin_lock_irq(>queue_lock);
You can't continue iteration after dropping both locks. You'd have to jump
out of loop and start list_for_each_entry_safe() again.
Thanks for your review, it's right. On the other hand
blkcg_activate_policy() and blkcg_deactivate_policy() might have the
same issue. My idea is that inserting a bookmark to the list, and
restard from here.
By the way, I found that blk_throtl_update_limit_valid() is called from
throtl_pd_offline(). If CONFIG_BLK_DEV_THROTTLING_LOW is off, lower
limit will always be zero, therefor a lot of time will be wasted to
iterate descendants to find a nonzero lower limit.
Do you think it's ok to do such modification:
diff --git a/block/blk-throttle.c b/block/blk-throttle.c
index b771c4299982..d52cac9f3a7c 100644
--- a/block/blk-throttle.c
+++ b/block/blk-throttle.c
@@ -587,6 +587,7 @@ static void throtl_pd_online(struct blkg_policy_data
*pd)
tg_update_has_rules(tg);
}
+#ifdef CONFIG_BLK_DEV_THROTTLING_LOW
static void blk_throtl_update_limit_valid(struct throtl_data *td)
{
struct cgroup_subsys_state *pos_css;
@@ -607,6 +608,11 @@ static void blk_throtl_update_limit_valid(struct
throtl_data *td)
td->limit_valid[LIMIT_LOW] = low_valid;
}
+#else
+static inline void blk_throtl_update_limit_valid(struct throtl_data *td)
+{
+}
+#endif
static void throtl_upgrade_state(struct throtl_data *td);
static void throtl_pd_offline(struct blkg_policy_data *pd)
Thanks!
Yu Kuai
Re: [PATCH V3] memory: tegra: add missing put_device() call in error path of tegra_emc_probe()
On 2020/11/10 23:21, Krzysztof Kozlowski wrote:
On Tue, Nov 10, 2020 at 09:33:11AM +0800, Yu Kuai wrote:
The reference to device obtained with of_find_device_by_node() should
be dropped. Thus add jump target to fix the exception handling for this
function implementation.
Fixes: 73a7f0a90641("memory: tegra: Add EMC (external memory controller)
driver")
Signed-off-by: Yu Kuai
---
drivers/memory/tegra/tegra124-emc.c | 21 +++--
1 file changed, 15 insertions(+), 6 deletions(-)
I think you missed my previous comment about the issue being fixed
already. Are you sure you rebased this on top of latest next?
Hi,
It's true the issue was fixed.
Thanks,
Yu Kuai
Best regards,
Krzysztof
.
Re: [PATCH] mtd: rawnand: ingenic: remove redundant get_device() in ingenic_ecc_get()
ping..
On 2020/10/31 18:54, Yu Kuai wrote:
of_find_device_by_node() already takes a reference to the device, and
ingenic_ecc_release() will drop the reference. So, the get_device() in
ingenic_ecc_get() is redundand.
Fixes: 15de8c6efd0e("mtd: rawnand: ingenic: Separate top-level and SoC specific
code")
Signed-off-by: Yu Kuai
---
drivers/mtd/nand/raw/ingenic/ingenic_ecc.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/mtd/nand/raw/ingenic/ingenic_ecc.c
b/drivers/mtd/nand/raw/ingenic/ingenic_ecc.c
index 8e22cd6ec71f..efe0ffe4f1ab 100644
--- a/drivers/mtd/nand/raw/ingenic/ingenic_ecc.c
+++ b/drivers/mtd/nand/raw/ingenic/ingenic_ecc.c
@@ -71,8 +71,6 @@ static struct ingenic_ecc *ingenic_ecc_get(struct device_node
*np)
if (!pdev || !platform_get_drvdata(pdev))
return ERR_PTR(-EPROBE_DEFER);
- get_device(>dev);
-
ecc = platform_get_drvdata(pdev);
clk_prepare_enable(ecc->clk);
Re: [PATCH V3] fsl/fman: add missing put_devcie() call in fman_port_probe()
在 2020/11/08 6:09, Jakub Kicinski 写道:
On Sat, 7 Nov 2020 17:09:25 +0800 Yu Kuai wrote:
if of_find_device_by_node() succeed, fman_port_probe() doesn't have a
corresponding put_device(). Thus add jump target to fix the exception
handling for this function implementation.
Fixes: 0572054617f3 ("fsl/fman: fix dereference null return value")
Signed-off-by: Yu Kuai
@@ -1792,20 +1792,20 @@ static int fman_port_probe(struct platform_device
*of_dev)
if (!fm_node) {
dev_err(port->dev, "%s: of_get_parent() failed\n", __func__);
err = -ENODEV;
- goto return_err;
+ goto free_port;
And now you no longer put port_node if jumping from here...
Sincerely apologize for that stupid mistake...
Also does the reference to put_device() not have to be released when
this function succeeds?
I'm not sure about that, since fman_port_driver doesn't define other
interface, maybe it reasonable to release it here.
}
@@ -1896,7 +1895,9 @@ static int fman_port_probe(struct platform_device *of_dev)
return 0;
-return_err:
+put_device:
+ put_device(_pdev->dev);
+put_node:
of_node_put(port_node);
free_port:
kfree(port);
.
Re: [PATCH] fsl/fman: add missing put_devcie() call in fman_port_probe()
On 2020/11/03 9:30, Jakub Kicinski wrote:
On Sat, 31 Oct 2020 18:54:18 +0800 Yu Kuai wrote:
if of_find_device_by_node() succeed, fman_port_probe() doesn't have a
corresponding put_device(). Thus add jump target to fix the exception
handling for this function implementation.
Fixes: 0572054617f3 ("fsl/fman: fix dereference null return value")
Signed-off-by: Yu Kuai
diff --git a/drivers/net/ethernet/freescale/fman/fman_port.c
b/drivers/net/ethernet/freescale/fman/fman_port.c
index d9baac0dbc7d..576ce6df3fce 100644
--- a/drivers/net/ethernet/freescale/fman/fman_port.c
+++ b/drivers/net/ethernet/freescale/fman/fman_port.c
@@ -1799,13 +1799,13 @@ static int fman_port_probe(struct platform_device
*of_dev)
of_node_put(fm_node);
if (!fm_pdev) {
err = -EINVAL;
- goto return_err;
+ goto put_device;
}
@@ -1898,6 +1898,8 @@ static int fman_port_probe(struct platform_device *of_dev)
return_err:
of_node_put(port_node);
+put_device:
+ put_device(_pdev->dev);
free_port:
kfree(port);
return err;
This does not look right. You're jumping to put_device() when fm_pdev
is NULL?
Hi,
oops, it's a silly mistake. Will fix it in V2 patch.
Thanks,
Yu Kuai
The order of error handling should be the reverse of the order of
execution of the function.
.
Re: [PATCH] iommu/rockchip: check return value of of_find_device_by_node() in rk_iommu_of_xlate()
On 2020/10/29 21:51, Robin Murphy wrote: On 2020-10-29 13:19, yukuai (C) wrote: On 2020/10/29 18:08, Robin Murphy wrote: On 2020-10-29 09:22, Yu Kuai wrote: If of_find_device_by_node() failed in rk_iommu_of_xlate(), null pointer dereference will be triggered. Thus return error code if of_find_device_by_node() failed. How can that happen? (Given that ".suppress_bind_attrs = true") Robin. I'm not sure if that could happen... My thought is that it's better to do such checking to aviod any possible problem. ->of_xlate() is only invoked on the specific set of ops returned by iommu_ops_from_fwnode(). In turn, iommu_ops_from_fwnode() will only return those ops if the driver has successfully probed and called iommu_register_device() with the relevant DT node. For the driver to have been able to probe at all, a platform device associated with that DT node must have been created, and therefore of_find_device_by_node() cannot fail. If there ever were some problem serious enough to break that fundamental assumption, then I *want* these drivers to crash right here, with a nice clear stack trace to start debugging from. So no, I firmly disagree that adding redundant code, which will never do anything except attempt to paper over catastrophic memory corruption, is "better". Sorry :) Sounds reasonable, thanks for your explanation Yu Kuai
Re: [PATCH] iommu/rockchip: check return value of of_find_device_by_node() in rk_iommu_of_xlate()
On 2020/10/29 18:08, Robin Murphy wrote: On 2020-10-29 09:22, Yu Kuai wrote: If of_find_device_by_node() failed in rk_iommu_of_xlate(), null pointer dereference will be triggered. Thus return error code if of_find_device_by_node() failed. How can that happen? (Given that ".suppress_bind_attrs = true") Robin. I'm not sure if that could happen... My thought is that it's better to do such checking to aviod any possible problem. Thanks! Yu Kuai
Re: [PATCH] iommu/qcom: add missing put_device() call in qcom_iommu_of_xlate()
On 2020/09/29 7:08, Will Deacon wrote: On Mon, Sep 21, 2020 at 09:45:57PM +0100, Will Deacon wrote: On Tue, Sep 22, 2020 at 03:13:53AM +0800, kernel test robot wrote: Thank you for the patch! Perhaps something to improve: [auto build test WARNING on iommu/next] [also build test WARNING on linus/master v5.9-rc6 next-20200921] [cannot apply to robclark/msm-next] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch] url: https://github.com/0day-ci/linux/commits/Yu-Kuai/iommu-qcom-add-missing-put_device-call-in-qcom_iommu_of_xlate/20200918-091341 base: https://git.kernel.org/pub/scm/linux/kernel/git/joro/iommu.git next config: arm64-randconfig-r023-20200920 (attached as .config) compiler: clang version 12.0.0 (https://github.com/llvm/llvm-project 4e8c028158b56d9c2142a62464e8e0686bde3584) reproduce (this is a W=1 build): wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # install arm64 cross compiling tool for clang build # apt-get install binutils-aarch64-linux-gnu # save the attached .config to linux build tree COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=arm64 If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot All warnings (new ones prefixed by >>): drivers/iommu/arm/arm-smmu/qcom_iommu.c:601:4: warning: misleading indentation; statement is not part of the previous 'if' [-Wmisleading-indentation] return -EINVAL; ^ drivers/iommu/arm/arm-smmu/qcom_iommu.c:599:3: note: previous statement is here if (WARN_ON(qcom_iommu != dev_iommu_priv_get(dev))) Oh, this looks like a nasty bug. Seems we're missing some braces. Yu Kuai: please could you send a v2 of this? Hi, Will Thanks for your notice, will send a V2 soon. Yu Kuai
Re: [RFC PATCH V4] iomap: add support to track dirty state of sub pages
On 2020/09/11 20:05, Matthew Wilcox wrote: @@ -683,7 +736,7 @@ static size_t __iomap_write_end(struct inode *inode, loff_t pos, size_t len, if (unlikely(copied < len && !PageUptodate(page))) return 0; iomap_set_range_uptodate(page, offset_in_page(pos), len); - iomap_set_page_dirty(page); + iomap_set_range_dirty(page, offset_in_page(pos), len); I_think_ the call to set_range_uptodate here is now unnecessary. The blocks should already have been set uptodate in write_begin. But please check! Hi, Matthew Sorry it took me so long to get back to this. I found that set_range_uptodate() might be skipped in write_begin() in this case: if (!(flags & IOMAP_WRITE_F_UNSHARE) && ┊ (from <= poff || from >= poff + plen) && ┊ (to <= poff || to >= poff + plen)) continue; And iomap_adjust_read_range() can set 'poff' greater than 'from' and 'poff + len' less than 'to'. Thanks Yu Kuai
Re: [RFC PATCH V4] iomap: add support to track dirty state of sub pages
Hi,
Sorry that after copy and paste, the content of the patch somehow
changed and looks strange.
Best regards,
Yu Kuai
On 2020/09/11 16:27, yukuai (C) wrote:
On 2020/08/21 20:44, Matthew Wilcox wrote:
On Fri, Aug 21, 2020 at 08:33:06PM +0800, Yu Kuai wrote:
changes from v3: - add IOMAP_STATE_ARRAY_SIZE - replace set_bit /
clear_bit with bitmap_set / bitmap_clear - move
iomap_set_page_dirty() out of 'iop->state_lock' - merge
iomap_set/clear_range_dirty() and iomap_iop_clear/clear_range_dirty()
I'm still working on the iomap parts of the THP series (fixing up
invalidatepage right now), but here are some of the relevant bits
(patch series to follow)
Hi, Matthew
Since your THP iomap patches were reviewed, I made some modifications
based on these patches.
Best regards,
Yu Kuai
---
fs/iomap/buffered-io.c | 92 +-
1 file changed, 74 insertions(+), 18 deletions(-)
diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c
index edf5eea56cf5..bc7f57748be8 100644
--- a/fs/iomap/buffered-io.c
+++ b/fs/iomap/buffered-io.c
@@ -23,13 +23,17 @@
/*
* Structure allocated for each page or THP when block size < page size
- * to track sub-page uptodate status and I/O completions.
+ * to track sub-page status and I/O completions.
*/
struct iomap_page {
atomic_t read_bytes_pending;
atomic_t write_bytes_pending;
- spinlock_t uptodate_lock;
- unsigned long uptodate[];
+ spinlock_t state_lock;
+ /*
+ * The first half bits are used to track sub-page uptodate status,
+ * the second half bits are for dirty status.
+ */
+ unsigned long state[];
};
static inline struct iomap_page *to_iomap_page(struct page *page)
@@ -57,9 +61,9 @@ iomap_page_create(struct inode *inode, struct page *page)
if (iop || nr_blocks <= 1)
return iop;
- iop = kzalloc(struct_size(iop, uptodate, BITS_TO_LONGS(nr_blocks)),
+ iop = kzalloc(struct_size(iop, state, BITS_TO_LONGS(2 * nr_blocks)),
GFP_NOFS | __GFP_NOFAIL);
- spin_lock_init(>uptodate_lock);
+ spin_lock_init(>state_lock);
attach_page_private(page, iop);
return iop;
}
@@ -74,7 +78,7 @@ iomap_page_release(struct page *page)
return;
WARN_ON_ONCE(atomic_read(>read_bytes_pending));
WARN_ON_ONCE(atomic_read(>write_bytes_pending));
- WARN_ON_ONCE(bitmap_full(iop->uptodate, nr_blocks) !=
+ WARN_ON_ONCE(bitmap_full(iop->state, nr_blocks) !=
PageUptodate(page));
kfree(iop);
}
@@ -105,7 +109,7 @@ iomap_adjust_read_range(struct inode *inode, struct
iomap_page *iop,
/* move forward for each leading block marked uptodate */
for (i = first; i <= last; i++) {
- if (!test_bit(i, iop->uptodate))
+ if (!test_bit(i, iop->state))
break;
*pos += block_size;
poff += block_size;
@@ -115,7 +119,7 @@ iomap_adjust_read_range(struct inode *inode, struct
iomap_page *iop,
/* truncate len if we find any trailing uptodate block(s) */
for ( ; i <= last; i++) {
- if (test_bit(i, iop->uptodate)) {
+ if (test_bit(i, iop->state)) {
plen -= (last - i + 1) * block_size;
last = i - 1;
break;
@@ -139,6 +143,55 @@ iomap_adjust_read_range(struct inode *inode, struct
iomap_page *iop,
*lenp = plen;
}
+static void
+iomap_set_range_dirty(struct page *page, unsigned int off,
+ unsigned int len)
+{
+ struct inode *inode = page->mapping->host;
+ unsigned int blocks_per_page = i_blocks_per_page(inode, page);
+ unsigned int first = (off >> inode->i_blkbits) + blocks_per_page;
+ unsigned int last = ((off + len - 1) >> inode->i_blkbits) +
blocks_per_page;
+ unsigned long flags;
+ struct iomap_page *iop;
+
+ if (PageError(page))
+ return;
+
+ if (len)
+ iomap_set_page_dirty(page);
+
+ if (!page_has_private(page))
+ return;
+
+ iop = to_iomap_page(page);
+ spin_lock_irqsave(>state_lock, flags);
+ bitmap_set(iop->state, first, last - first + 1);
+ spin_unlock_irqrestore(>state_lock, flags);
+}
+
+static void
+iomap_clear_range_dirty(struct page *page, unsigned int off,
+ unsigned int len)
+{
+ struct inode *inode = page->mapping->host;
+ unsigned int blocks_per_page = i_blocks_per_page(inode, page);
+ unsigned int first = (off >> inode->i_blkbits) + blocks_per_page;
+ unsigned int last = ((off + len - 1) >> inode->i_blkbits) +
blocks_per_page;
+ unsigned long flags;
+ struct iomap_page *iop;
+
+ if (PageError(page))
+ return;
+
+ if (!page_has_private(page))
+ return;
+
+ iop = to_iomap_page(page);
+ spin_lock_irqsave(>state_lock, flags);
+ bitmap_clear(
Re: [RFC PATCH V4] iomap: add support to track dirty state of sub pages
On 2020/08/21 20:44, Matthew Wilcox wrote:
On Fri, Aug 21, 2020 at 08:33:06PM +0800, Yu Kuai wrote:
changes from v3: - add IOMAP_STATE_ARRAY_SIZE - replace set_bit /
clear_bit with bitmap_set / bitmap_clear - move
iomap_set_page_dirty() out of 'iop->state_lock' - merge
iomap_set/clear_range_dirty() and
iomap_iop_clear/clear_range_dirty()
I'm still working on the iomap parts of the THP series (fixing up
invalidatepage right now), but here are some of the relevant bits
(patch series to follow)
Hi, Matthew
Since your THP iomap patches were reviewed, I made some modifications
based on these patches.
Best regards,
Yu Kuai
---
fs/iomap/buffered-io.c | 92 +-
1 file changed, 74 insertions(+), 18 deletions(-)
diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c
index edf5eea56cf5..bc7f57748be8 100644
--- a/fs/iomap/buffered-io.c
+++ b/fs/iomap/buffered-io.c
@@ -23,13 +23,17 @@
/*
* Structure allocated for each page or THP when block size < page size
- * to track sub-page uptodate status and I/O completions.
+ * to track sub-page status and I/O completions.
*/
struct iomap_page {
atomic_tread_bytes_pending;
atomic_twrite_bytes_pending;
- spinlock_t uptodate_lock;
- unsigned long uptodate[];
+ spinlock_t state_lock;
+ /*
+* The first half bits are used to track sub-page uptodate status,
+* the second half bits are for dirty status.
+*/
+ unsigned long state[];
};
static inline struct iomap_page *to_iomap_page(struct page *page)
@@ -57,9 +61,9 @@ iomap_page_create(struct inode *inode, struct page *page)
if (iop || nr_blocks <= 1)
return iop;
- iop = kzalloc(struct_size(iop, uptodate, BITS_TO_LONGS(nr_blocks)),
+ iop = kzalloc(struct_size(iop, state, BITS_TO_LONGS(2 * nr_blocks)),
GFP_NOFS | __GFP_NOFAIL);
- spin_lock_init(>uptodate_lock);
+ spin_lock_init(>state_lock);
attach_page_private(page, iop);
return iop;
}
@@ -74,7 +78,7 @@ iomap_page_release(struct page *page)
return;
WARN_ON_ONCE(atomic_read(>read_bytes_pending));
WARN_ON_ONCE(atomic_read(>write_bytes_pending));
- WARN_ON_ONCE(bitmap_full(iop->uptodate, nr_blocks) !=
+ WARN_ON_ONCE(bitmap_full(iop->state, nr_blocks) !=
PageUptodate(page));
kfree(iop);
}
@@ -105,7 +109,7 @@ iomap_adjust_read_range(struct inode *inode, struct
iomap_page *iop,
/* move forward for each leading block marked uptodate */
for (i = first; i <= last; i++) {
- if (!test_bit(i, iop->uptodate))
+ if (!test_bit(i, iop->state))
break;
*pos += block_size;
poff += block_size;
@@ -115,7 +119,7 @@ iomap_adjust_read_range(struct inode *inode, struct
iomap_page *iop,
/* truncate len if we find any trailing uptodate block(s) */
for ( ; i <= last; i++) {
- if (test_bit(i, iop->uptodate)) {
+ if (test_bit(i, iop->state)) {
plen -= (last - i + 1) * block_size;
last = i - 1;
break;
@@ -139,6 +143,55 @@ iomap_adjust_read_range(struct inode *inode, struct
iomap_page *iop,
*lenp = plen;
}
+static void
+iomap_set_range_dirty(struct page *page, unsigned int off,
+ unsigned int len)
+{
+ struct inode *inode = page->mapping->host;
+ unsigned int blocks_per_page = i_blocks_per_page(inode, page);
+ unsigned int first = (off >> inode->i_blkbits) + blocks_per_page;
+ unsigned int last = ((off + len - 1) >> inode->i_blkbits) +
blocks_per_page;
+ unsigned long flags;
+ struct iomap_page *iop;
+
+ if (PageError(page))
+ return;
+
+ if (len)
+ iomap_set_page_dirty(page);
+
+ if (!page_has_private(page))
+ return;
+
+ iop = to_iomap_page(page);
+ spin_lock_irqsave(>state_lock, flags);
+ bitmap_set(iop->state, first, last - first + 1);
+ spin_unlock_irqrestore(>state_lock, flags);
+}
+
+static void
+iomap_clear_range_dirty(struct page *page, unsigned int off,
+ unsigned int len)
+{
+ struct inode *inode = page->mapping->host;
+ unsigned int blocks_per_page = i_blocks_per_page(inode, page);
+ unsigned int first = (off >> inode->i_blkbits) + blocks_per_page;
+ unsigned int last = ((off + len - 1) >> inode->i_blkbits) +
blocks_per_page;
+ unsigned long flags;
+ struct iomap_page *iop;
+
+ if (PageError(page))
+ return;
+
+ if (!page_has_private(page))
+ return;
+
+ iop =
Re: [PATCH] drm/mediatek: add missing put_device() call in mtk_ddp_comp_init()
On 2020/09/08 6:56, Chun-Kuang Hu wrote: Hi Yu Kuai: Yu Kuai 於 2020年9月5日 週六 下午4:31寫道： if of_find_device_by_node() succeed, mtk_ddp_comp_init() doesn't have a corresponding put_device(). Thus add put_device() to fix the exception handling for this function implementation. This patch looks good to me, but I find another thing related to this. mtk_ddp_comp_init() is called in a loop in mtk_drm_probe(), when this component init fail, I think we should uninitialize previous successive init component and put their device. Would you like to make this patch more complete? Hi, Of course, thank you for your review. Best regards, Yu Kuai
Re: [PATCH] drm/sun4i: add missing put_device() call in sun8i_r40_tcon_tv_set_mux()
On 2020/08/25 21:38, Maxime Ripard wrote:
Hi,
On Tue, Aug 25, 2020 at 07:44:03PM +0800, Yu Kuai wrote:
If sun8i_r40_tcon_tv_set_mux() succeed, at_dma_xlate() doesn't have a
corresponding put_device(). Thus add put_device() to fix the exception
handling for this function implementation.
Fixes: 0305189afb32 ("drm/sun4i: tcon: Add support for R40 TCON")
Signed-off-by: Yu Kuai
That doesn't sound right, we're not using at_dma_xlate at all in that
driver?
Hi!
sry about that, should be sun8i_r40_tcon_tv_set_mux(), same as the
title said.
Best regards,
Yu Kuai
Re: [PATCH 1/2] ASoC: fsl: imx-es8328: add missing kfree() call in imx_es8328_probe()
On 2020/08/25 20:11, Mark Brown wrote:
On Tue, Aug 25, 2020 at 08:05:30PM +0800, Yu Kuai wrote:
If memory allocation for 'data' or 'comp' succeed, imx_es8328_probe()
doesn't have corresponding kfree() in exception handling. Thus add
kfree() for this function implementation.
@@ -151,7 +151,7 @@ static int imx_es8328_probe(struct platform_device *pdev)
comp = devm_kzalloc(dev, 3 * sizeof(*comp), GFP_KERNEL);
if (!comp) {
The allocation is being done using devm_ which means no explicit kfree()
is needed, the allocation will be automatically unwound when the device
is unbound.
Hi,
Thanks for pointing it out, I'll remove this patch.
Best regards,
Yu Kuai
Re: [RFC PATCH V3] iomap: add support to track dirty state of sub pages
On 2020/8/19 20:56, Gao Xiang wrote:
On Wed, Aug 19, 2020 at 08:05:42PM +0800, Yu Kuai wrote:
...
+static void
+iomap_iop_set_range_dirty(struct page *page, unsigned int off,
+ unsigned int len)
+{
+ struct iomap_page *iop = to_iomap_page(page);
+ struct inode *inode = page->mapping->host;
+ unsigned int first = DIRTY_BITS(off >> inode->i_blkbits);
+ unsigned int last = DIRTY_BITS((off + len - 1) >> inode->i_blkbits);
+ unsigned long flags;
+ unsigned int i;
+
+ spin_lock_irqsave(>state_lock, flags);
+ for (i = first; i <= last; i++)
+ set_bit(i, iop->state);
+
+ if (last >= first)
+ iomap_set_page_dirty(page);
set_page_dirty() in the atomic context?
Hi,
You'are right, this shouldn't be inside spin_lock.
+
+ spin_unlock_irqrestore(>state_lock, flags);
+}
+
+static void
+iomap_set_range_dirty(struct page *page, unsigned int off,
+ unsigned int len)
+{
+ if (PageError(page))
+ return;
+
+ if (page_has_private(page))
+ iomap_iop_set_range_dirty(page, off, len);
I vaguely remembered iomap doesn't always set up PagePrivate.
If so, maybe I should move iomap_set_page_dirty() to
ioamp_set_range_dirty().
Thanks,
Yu Kuai
@@ -705,7 +770,7 @@ __iomap_write_end(struct inode *inode, loff_t pos, unsigned
len,
if (unlikely(copied < len && !PageUptodate(page)))
return 0;
iomap_set_range_uptodate(page, offset_in_page(pos), len);
- iomap_set_page_dirty(page);
+ iomap_set_range_dirty(page, offset_in_page(pos), len);
return copied;
}
so here could be suspectable, but I might be wrong here since
I just take a quick look.
Thanks,
Gao Xiang
.
Re: [RFC PATCH] iomap: add support to track dirty state of sub pages
On 2020/7/30 11:19, Matthew Wilcox wrote: Maybe let the discussion on removing the ->uptodate array finish before posting another patch for review? Hi, Matthew! Of course, I missed the discussion thread before sending this path. And thanks for your suggestions. Best regards, Yu Kuai
Re: [RFC PATCH] iomap: add support to track dirty state of sub pages
On 2020/7/30 10:27, Gao Xiang wrote:
Hi Kuai,
On Thu, Jul 30, 2020 at 09:19:01AM +0800, Yu Kuai wrote:
commit 9dc55f1389f9 ("iomap: add support for sub-pagesize buffered I/O
without buffer heads") replace the per-block structure buffer_head with
the per-page structure iomap_page. However, iomap_page can't track the
dirty state of sub pages, which will cause performance issue since sub
pages will be writeback even if they are not dirty.
For example, if block size is 4k and page size is 64k:
dd if=/dev/zero of=testfile bs=4k count=16 oflag=sync
With buffer_head implementation, the above dd cmd will writeback 4k in
each round. However, with iomap_page implementation, the range of
writeback in each round is from the start of the page to the end offset
we just wrote.
Thus add support to track dirty state for sub pages in iomap_page.
AFAIK, the current focus is also on the numbers in the original
discussion thread, so it'd be better to show some numbers with
large PAGE_SIZE on this with some workloads as well.
https://lore.kernel.org/r/20200729230503.ga2...@dread.disaster.area
Hi, Xiang!
The problem was found by iozone to test 4k sequintail write in my
case, thanks for pointing out the discussion thread. I'll test it if
this patch have any effect on that situation.
Thanks,
Yu Kuai
e.g. My guess is if the dirty blocks in the page are highly fragmented, maybe
it'd be better to writeback the whole page in an IO rather than individual
blocks.
At a very quick glance, the approach looks good to me.
Thanks,
Gao Xiang
Signed-off-by: Yu Kuai
---
fs/iomap/buffered-io.c | 51 +-
1 file changed, 50 insertions(+), 1 deletion(-)
diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c
index bcfc288dba3f..ac2676146b98 100644
--- a/fs/iomap/buffered-io.c
+++ b/fs/iomap/buffered-io.c
@@ -29,7 +29,9 @@ struct iomap_page {
atomic_tread_count;
atomic_twrite_count;
spinlock_t uptodate_lock;
+ spinlock_t dirty_lock;
DECLARE_BITMAP(uptodate, PAGE_SIZE / 512);
+ DECLARE_BITMAP(dirty, PAGE_SIZE / 512);
};
static inline struct iomap_page *to_iomap_page(struct page *page)
@@ -53,7 +55,9 @@ iomap_page_create(struct inode *inode, struct page *page)
atomic_set(>read_count, 0);
atomic_set(>write_count, 0);
spin_lock_init(>uptodate_lock);
+ spin_lock_init(>dirty_lock);
bitmap_zero(iop->uptodate, PAGE_SIZE / SECTOR_SIZE);
+ bitmap_zero(iop->dirty, PAGE_SIZE / SECTOR_SIZE);
/*
* migrate_page_move_mapping() assumes that pages with private data have
@@ -135,6 +139,44 @@ iomap_adjust_read_range(struct inode *inode, struct
iomap_page *iop,
*lenp = plen;
}
+static void
+iomap_iop_set_or_clear_range_dirty(
+ struct page *page,
+ unsigned int off,
+ unsigned int len,
+ bool is_set)
+{
+ struct iomap_page *iop = to_iomap_page(page);
+ struct inode *inode = page->mapping->host;
+ unsigned int first = off >> inode->i_blkbits;
+ unsigned int last = (off + len - 1) >> inode->i_blkbits;
+ unsigned long flags;
+ unsigned int i;
+
+ spin_lock_irqsave(>dirty_lock, flags);
+ for (i = first; i <= last; i++)
+ if (is_set)
+ set_bit(i, iop->dirty);
+ else
+ clear_bit(i, iop->dirty);
+ spin_unlock_irqrestore(>dirty_lock, flags);
+}
+
+static void
+iomap_set_or_clear_range_dirty(
+ struct page *page,
+ unsigned int off,
+ unsigned int len,
+ bool is_set)
+{
+ if (PageError(page))
+ return;
+
+ if (page_has_private(page))
+ iomap_iop_set_or_clear_range_dirty(
+ page, off, len, is_set);
3> +}
+
static void
iomap_iop_set_range_uptodate(struct page *page, unsigned off, unsigned len)
{
@@ -705,6 +747,8 @@ __iomap_write_end(struct inode *inode, loff_t pos, unsigned
len,
if (unlikely(copied < len && !PageUptodate(page)))
return 0;
iomap_set_range_uptodate(page, offset_in_page(pos), len);
+ iomap_set_or_clear_range_dirty(
+ page, offset_in_page(pos), len, true);
iomap_set_page_dirty(page);
return copied;
}
@@ -1030,6 +1074,8 @@ iomap_page_mkwrite_actor(struct inode *inode, loff_t pos,
loff_t length,
WARN_ON_ONCE(!PageUptodate(page));
iomap_page_create(inode, page);
set_page_dirty(page);
+ iomap_set_or_clear_range_dirty(
+ page, offset_in_page(pos), length, true);
}
return length;
@@ -1386,7 +1432,8 @@ iomap_writepage_map(struct iomap_writepage_ctx *wpc,
for (i = 0, file_offset = page_offset(page);
i < (PAGE_SIZE >> inode->i_blkbits) && file_offset < end_offset;
i++, file_offset +=
Re: [PATCH] ARM: at91: pm: add missing put_device() call in at91_pm_sram_init()
On 2020/7/18 6:55, Alexandre Belloni wrote: A better fix would have been to also factorize imx_suspend_alloc_ocram, imx6q_suspend_init, socfpga_setup_ocram_self_refresh and at91_pm_sram_init as they were all copied from pm-imx6.c imx_suspend_alloc_ocram and imx6q_suspend_init are done areadly, socfpga_setup_ocram_self_refresh and at91_pm_sram_init still need to fix. Thanks for pointing out! Yu Kuai
Re: [PATCH] ARM: at91: pm: add missing put_device() call in at91_pm_sram_init()
On 2020/7/3 4:09, Alexandre Belloni wrote:
Hi,
On 04/06/2020 20:33:01+0800, yu kuai wrote:
if of_find_device_by_node() succeed, at91_pm_sram_init() doesn't have
a corresponding put_device(). Thus add a jump target to fix the exception
handling for this function implementation.
Fixes: d2e467905596 ("ARM: at91: pm: use the mmio-sram pool to access SRAM")
Signed-off-by: yu kuai
---
arch/arm/mach-at91/pm.c | 11 ---
1 file changed, 8 insertions(+), 3 deletions(-)
diff --git a/arch/arm/mach-at91/pm.c b/arch/arm/mach-at91/pm.c
index 074bde64064e..2aab043441e8 100644
--- a/arch/arm/mach-at91/pm.c
+++ b/arch/arm/mach-at91/pm.c
@@ -592,13 +592,13 @@ static void __init at91_pm_sram_init(void)
sram_pool = gen_pool_get(>dev, NULL);
Isn't the best solution to simply have put_device hereHi, Alexandre !
I think put_device() is supposed to be called in the exception handling
path.
if (!sram_pool) {
pr_warn("%s: sram pool unavailable!\n", __func__);
- return;
+ goto out_put_device;
}
sram_base = gen_pool_alloc(sram_pool, at91_pm_suspend_in_sram_sz);
if (!sram_base) {
pr_warn("%s: unable to alloc sram!\n", __func__);
- return;
+ goto out_put_device;
}
sram_pbase = gen_pool_virt_to_phys(sram_pool, sram_base);
@@ -606,12 +606,17 @@ static void __init at91_pm_sram_init(void)
at91_pm_suspend_in_sram_sz, false);
if (!at91_suspend_sram_fn) {
pr_warn("SRAM: Could not map\n");
- return;
+ goto out_put_device;
}
/* Copy the pm suspend handler to SRAM */
at91_suspend_sram_fn = fncpy(at91_suspend_sram_fn,
_pm_suspend_in_sram, at91_pm_suspend_in_sram_sz);
If nothing is wrong, maybe put_device shounld't be called?
Thanks!
Yu Kuai
+ return;
+
+out_put_device:
+ put_device(>dev);
+ return;
}
static bool __init at91_is_pm_mode_active(int pm_mode)
--
2.25.4
Re: [PATCH] ARM: socfpga: add missing put_device() call in socfpga_setup_ocram_self_refresh()
ping?
On 2020/6/4 21:10, yu kuai wrote:
if of_find_device_by_node() succeed, socfpga_setup_ocram_self_refresh()
doesn't have a corresponding put_device(). Thus add a jump target to fix
the exception handling for this function implementation.
Fixes: 44fd8c7d4005 ("ARM: socfpga: support suspend to ram")
Signed-off-by: yu kuai
---
arch/arm/mach-socfpga/pm.c | 11 ---
1 file changed, 8 insertions(+), 3 deletions(-)
diff --git a/arch/arm/mach-socfpga/pm.c b/arch/arm/mach-socfpga/pm.c
index 6ed887cf8dc9..7267f5da15a4 100644
--- a/arch/arm/mach-socfpga/pm.c
+++ b/arch/arm/mach-socfpga/pm.c
@@ -49,14 +49,14 @@ static int socfpga_setup_ocram_self_refresh(void)
if (!ocram_pool) {
pr_warn("%s: ocram pool unavailable!\n", __func__);
ret = -ENODEV;
- goto put_node;
+ goto put_device;
}
ocram_base = gen_pool_alloc(ocram_pool, socfpga_sdram_self_refresh_sz);
if (!ocram_base) {
pr_warn("%s: unable to alloc ocram!\n", __func__);
ret = -ENOMEM;
- goto put_node;
+ goto put_device;
}
ocram_pbase = gen_pool_virt_to_phys(ocram_pool, ocram_base);
@@ -67,7 +67,7 @@ static int socfpga_setup_ocram_self_refresh(void)
if (!suspend_ocram_base) {
pr_warn("%s: __arm_ioremap_exec failed!\n", __func__);
ret = -ENOMEM;
- goto put_node;
+ goto put_device;
}
/* Copy the code that puts DDR in self refresh to ocram */
@@ -80,7 +80,12 @@ static int socfpga_setup_ocram_self_refresh(void)
"could not copy function to ocram");
if (!socfpga_sdram_self_refresh_in_ocram)
ret = -EFAULT;
+
+ if (!ret)
+ goto put_node;
+put_device:
+ put_device(>dev);
put_node:
of_node_put(np);
Re: [PATCH] ARM: at91: pm: add missing put_device() call in at91_pm_sram_init()
ping?
On 2020/6/4 20:33, yu kuai wrote:
if of_find_device_by_node() succeed, at91_pm_sram_init() doesn't have
a corresponding put_device(). Thus add a jump target to fix the exception
handling for this function implementation.
Fixes: d2e467905596 ("ARM: at91: pm: use the mmio-sram pool to access SRAM")
Signed-off-by: yu kuai
---
arch/arm/mach-at91/pm.c | 11 ---
1 file changed, 8 insertions(+), 3 deletions(-)
diff --git a/arch/arm/mach-at91/pm.c b/arch/arm/mach-at91/pm.c
index 074bde64064e..2aab043441e8 100644
--- a/arch/arm/mach-at91/pm.c
+++ b/arch/arm/mach-at91/pm.c
@@ -592,13 +592,13 @@ static void __init at91_pm_sram_init(void)
sram_pool = gen_pool_get(>dev, NULL);
if (!sram_pool) {
pr_warn("%s: sram pool unavailable!\n", __func__);
- return;
+ goto out_put_device;
}
sram_base = gen_pool_alloc(sram_pool, at91_pm_suspend_in_sram_sz);
if (!sram_base) {
pr_warn("%s: unable to alloc sram!\n", __func__);
- return;
+ goto out_put_device;
}
sram_pbase = gen_pool_virt_to_phys(sram_pool, sram_base);
@@ -606,12 +606,17 @@ static void __init at91_pm_sram_init(void)
at91_pm_suspend_in_sram_sz, false);
if (!at91_suspend_sram_fn) {
pr_warn("SRAM: Could not map\n");
- return;
+ goto out_put_device;
}
/* Copy the pm suspend handler to SRAM */
at91_suspend_sram_fn = fncpy(at91_suspend_sram_fn,
_pm_suspend_in_sram, at91_pm_suspend_in_sram_sz);
+ return;
+
+out_put_device:
+ put_device(>dev);
+ return;
}
static bool __init at91_is_pm_mode_active(int pm_mode)
Re: [PATCH] ARM: imx6: add missing put_device() call in imx6q_suspend_init()
On 2020/6/23 20:11, Shawn Guo wrote: On Thu, Jun 04, 2020 at 08:54:49PM +0800, yu kuai wrote: if of_find_device_by_node() succeed, imx6q_suspend_init() doesn't have a corresponding put_device(). Thus add a jump target to fix the exception handling for this function implementation. Signed-off-by: yu kuai Applied, thanks. Hi, Shawn How about this patch: https://lkml.org/lkml/2020/6/4/428 The patch fix the similar issure. Thanks! Yu Kuai
Re: [PATCH] xfs: fix use-after-free on CIL context on shutdown
On 2020/6/11 10:45, Dave Chinner wrote:
From: Dave Chinner
xlog_wait() on the CIL context can reference a freed context if the
waiter doesn't get scheduled before the CIL context is freed. This
can happen when a task is on the hard throttle and the CIL push
aborts due to a shutdown. This was detected by generic/019:
thread 1thread 2
__xfs_trans_commit
xfs_log_commit_cil
xlog_wait
schedule
xlog_cil_push_work
wake_up_all
xlog_cil_committed
kmem_free
remove_wait_queue
spin_lock_irqsave --> UAF
Fix it by moving the wait queue to the CIL rather than keeping it in
in the CIL context that gets freed on push completion. Because the
wait queue is now independent of the CIL context and we might have
multiple contexts in flight at once, only wake the waiters on the
push throttle when the context we are pushing is over the hard
throttle size threshold.
Hi, Dave,
How do you think about the following fix:
1. use autoremove_wake_func(), and remove remove_wait_queue() to
avoid UAF.
2. add finish_wait().
@@ -576,12 +576,13 @@ xlog_wait(
__releases(lock)
{
DECLARE_WAITQUEUE(wait, current);
+ wait.func = autoremove_wake_function;
add_wait_queue_exclusive(wq, );
__set_current_state(TASK_UNINTERRUPTIBLE);
spin_unlock(lock);
schedule();
- remove_wait_queue(wq, );
+ finish_wait(wq, );
}
Best regards!
Yu Kuai
Re: [RFC PATCH] fix use after free in xlog_wait()
On 2020/6/11 10:28, Dave Chinner wrote Actually, it's a lot simpler: thread1 thread2 __xfs_trans_commit xfs_log_commit_cil xlog_wait schedule xlog_cil_push_work wake_up_all xlog_cil_committed kmem_free remove_wait_queue spin_lock_irqsave --> UAF It's ture in this case, however, I got another result when I tried to reporduce it, which seems 'ctx' can be freed in a different path: [ 64.975996] run fstests generic/019 at 2020-06-10 16:13:44 [ 69.126208] xfs filesystem being mounted at /tmp/scratch supports timestamps until 2038 (0x7fff) [ 99.166846] XFS (sdb): log I/O error -5 [ 99.170111] XFS (sdb): Log I/O Error Detected. Shutting down filesystem [ 99.171071] XFS (sdb): Please unmount the filesystem and rectify the problem(s) [ 99.179569] [ cut here ] [ 99.180432] WARNING: CPU: 7 PID: 2705 at fs/iomap/buffered-io.c:1030 iomap_page_mkwrite_actor+0x17d/0x1b0 [ 99.181273] == [ 99.181758] Modules linked in: [ 99.182806] BUG: KASAN: use-after-free in do_raw_spin_trylock+0x67/0x180 [ 99.183255] CPU: 7 PID: 2705 Comm: fio Not tainted 5.7.0-next-20200602+ #29 [ 99.184166] Read of size 4 at addr 888115f28868 by task fio/2704 [ 99.184171] [ 99.185142] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS ?-20190727_073836-buildvm-ppc64le-16.ppc.fedoraproject.org-3.fc31 04/01/2014 [ 99.185995] CPU: 6 PID: 2704 Comm: fio Not tainted 5.7.0-next-20200602+ #29 [ 99.186227] RIP: 0010:iomap_page_mkwrite_actor+0x17d/0x1b0 [ 99.188199] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS ?-20190727_073836-buildvm-ppc64le-16.ppc.fedoraproject.org-3.fc31 04/01/2014 [ 99.189178] Code: 89 ef e8 a6 d4 c7 ff e9 3f ff ff ff e8 fc 64 ad ff 89 da 31 f6 48 89 ef e8 b0 1e f2 ff 49 89 dc e9 26 ff ff ff e8 e3 64 ad ff <0f> 0b eb be e8 da 64 ad ff 4d 8d 67 ffe [ 99.189899] Call Trace: [ 99.191748] RSP: :88810599fa18 EFLAGS: 00010293 [ 99.194218] dump_stack+0xf6/0x16e [ 99.194574] RAX: 888106df3680 RBX: 1000 RCX: 94338cad [ 99.195301] ? do_raw_spin_trylock+0x67/0x180 [ 99.195777] RDX: RSI: RDI: 0001 [ 99.195786] RBP: ea0002544800 R08: 888106df3680 R09: f940004a8901 [ 99.196764] ? do_raw_spin_trylock+0x67/0x180 [ 99.196778] print_address_description.constprop.0.cold+0xd3/0x415 [ 99.197393] R10: ea0002544807 R11: f940004a8900 R12: [ 99.198380] ? do_raw_spin_trylock+0x67/0x180 [ 99.199349] R13: R14: 8880b6ee7280 R15: ea00025447c8 [ 99.199939] kasan_report.cold+0x1f/0x37 [ 99.199949] ? __switch_to+0x510/0xef0 [ 99.200812] FS: 7ff7d8562740() GS:88811a40() knlGS: [ 99.201755] ? do_raw_spin_trylock+0x67/0x180 [ 99.201765] check_memory_region+0x14e/0x1b0 [ 99.202378] CS: 0010 DS: ES: CR0: 80050033 [ 99.203319] do_raw_spin_trylock+0x67/0x180 [ 99.203858] CR2: 7ff7ae3e4ff0 CR3: 0001028be000 CR4: 06e0 [ 99.204374] ? do_raw_spin_lock+0x290/0x290 [ 99.205470] DR0: DR1: DR2: [ 99.206069] _raw_spin_lock_irqsave+0x44/0x80 [ 99.206641] DR3: DR6: fffe0ff0 DR7: 0400 [ 99.206647] Call Trace: [ 99.207420] ? remove_wait_queue+0x22/0x190 [ 99.208014] iomap_apply+0x2d7/0xc00 [ 99.208949] remove_wait_queue+0x22/0x190 [ 99.209539] ? iomap_page_create+0x350/0x350 [ 99.210510] xfs_log_commit_cil+0x1c7e/0x2940 [ 99.25] ? trace_event_raw_event_iomap_apply+0x430/0x430 [ 99.212073] ? xlog_cil_empty+0x90/0x90 [ 99.212421] ? down_write_trylock+0x2f0/0x2f0 [ 99.212979] ? check_flags.part.0+0x430/0x430 [ 99.213486] ? update_time+0xc0/0xc0 [ 99.214025] ? wake_up_q+0x140/0x140 [ 99.214610] iomap_page_mkwrite+0x26a/0x3b0 [ 99.215201] ? xlog_ticket_alloc+0x3e0/0x3e0 [ 99.215966] ? iomap_page_create+0x350/0x350 [ 99.216487] ? __kasan_kmalloc.constprop.0+0xc2/0xd0 [ 99.217097] __xfs_filemap_fault.constprop.0+0x32f/0x4e0 [ 99.217675] __xfs_trans_commit+0x2b3/0xf20 [ 99.218206] do_page_mkwrite+0x1b1/0x470 [ 99.218681] ? xfs_trans_unreserve_and_mod_sb+0xab0/0xab0 [ 99.219259] do_wp_page+0x9e7/0x1b10 [ 99.219838] ? xfs_isilocked+0x8c/0x2f0 [ 99.220439] ? finish_mkwrite_fault+0x4b0/0x4b0 [ 99.221102] ? xfs_trans_log_inode+0x1b2/0x480 [ 99.221817] ? do_user_addr_fault+0x2fd/0xd42 [ 99.222382] xfs_vn_update_time+0x40a/0x730 [ 99.222922] handle_mm_fault+0x1c9f/0x3600 [ 99.223634] ? xfs_setattr_mode.isra.0+0xb0/0xb0 [ 99.224140] ? __pmd_alloc+0x390/0x390 [ 99.224653] ? current_time+0xad/0x110 [
Re: [PATCH] ARM: imx6: add missing put_device() call in imx6q_suspend_init()
On 2020/6/5 3:07, Markus Elfring wrote: if of_find_device_by_node() succeed, imx6q_suspend_init() doesn't have a corresponding put_device(). Thus add a jump target to fix the exception handling for this function implementation. Do you find a previous update suggestion useful? ARM: imx6: Add missing put_device() call in imx6q_suspend_init() https://lore.kernel.org/linux-arm-kernel/5acd7308-f6e1-4b1e-c744-bb2e5fdca...@web.de/ https://lore.kernel.org/patchwork/patch/1151158/ https://lkml.org/lkml/2019/11/9/125 Hi, Markus It is useful indeed. Although I didn't run cocci script, since it can produce too many false result which is hard to filter out. BTW, I see you haver done the work already, I guess I won't send any patches related to 'missing put_device after of_find_device_by_node()'. Any idea why these pathes didn't get applied ? Best regards, Yu Kuai
Re: [PATCH V2] pinctrl: sirf: add missing put_device() call in sirfsoc_gpio_probe()
On 2020/6/3 9:35, yu kuai wrote:
A coccicheck run provided information like the following:
drivers/pinctrl/sirf/pinctrl-sirf.c:798:2-8: ERROR: missing put_device;
call of_find_device_by_node on line 792, but without a corresponding
object release within this function.
Generated by: scripts/coccinelle/free/put_device.cocci
Thus add a jump target to fix the exception handling for this
function implementation.
Fixes: 5130216265f6 ("PINCTRL: SiRF: add GPIO and GPIO irq support in CSR
SiRFprimaII")
Signed-off-by: yu kuai
---
drivers/pinctrl/sirf/pinctrl-sirf.c | 20 ++--
1 file changed, 14 insertions(+), 6 deletions(-)
Sorry about the missing change log:
Changes in V2:
change the variant of commit message suggested by Markus.
Best Regards,
Yu Kuai
Re: [PATCH] pinctrl: sirf: Add missing put_device() call in sirfsoc_gpio_probe()
On 2020/6/3 2:56, Markus Elfring wrote: in sirfsoc_gpio_probe(), if of_find_device_by_node() succeed, put_device() is missing in the error handling patch. How do you think about another wording variant? A coccicheck run provided information like the following. drivers/pinctrl/sirf/pinctrl-sirf.c:798:2-8: ERROR: missing put_device; call of_find_device_by_node on line 792, but without a corresponding object release within this function. Generated by: scripts/coccinelle/free/put_device.cocci Thus add a jump target to fix the exception handling for this function implementation. Would you like to add the tag “Fixes” to the commit message? Will do, thanks for your advise! Yu Kuai
