On 12/8/20 6:53 AM, Xiaoming Ni wrote:
On 2020/12/8 2:59, Vignesh Raghavendra wrote:
Hi Xiaoming,
[...]
drivers/mtd/chips/cfi_cmdset_0002.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)
diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c b/drivers/mtd/chips/cfi_cmdset_0002.c index a1f3e1031c3d..12c3776f093a 100644 --- a/drivers/mtd/chips/cfi_cmdset_0002.c +++ b/drivers/mtd/chips/cfi_cmdset_0002.c @@ -1682,7 +1682,11 @@ static int __xipram do_write_oneword_once(struct map_info *map, set_current_state(TASK_UNINTERRUPTIBLE); add_wait_queue(&chip->wq, &wait); mutex_unlock(&chip->mutex); + if (IS_ENABLED(CONFIG_MTD_XIP)) + local_irq_enable(); schedule(); + if (IS_ENABLED(CONFIG_MTD_XIP)) + local_irq_disable();
The fix really seems strange to me. I will let Vignesh decide but I think we should consider updating/fixing xip_disable instead.
Agree with Miquel. Have you done any testing or is this purely based on code inspection?
I don't have the corresponding device test environment. I found the problem through code review.
Sorry, I am not comfortable applying this patch without proper testing that given the unknowns and legacy nature of the code.
What about comment before xip_disable() function:
/* * No interrupt what so ever can be serviced while the flash isn't in array * mode. This is ensured by the xip_disable() and xip_enable() functions * enclosing any code path where the flash is known not to be in array mode. * And within a XIP disabled code path, only functions marked with __xipram * may be called and nothing else (it's a good thing to inspect generated * assembly to make sure inline functions were actually inlined and that gcc * didn't emit calls to its own support functions). Also configuring MTD CFI * support to a single buswidth and a single interleave is also recommended. */
So, I don't think the fix is as simple as this patch.
+xip_enable(); schedule(); +xip_disable();
Do I need to change it to this?
This just narrows the window, but an IRQ is still possible just after xip_enable() but before schedule().
Regards Vignesh