Commit 303c6443 authored by Russell King's avatar Russell King Committed by Russell King

[ARM] clearpage: provide our own clear_user_highpage()

For similar reasons as copy_user_page(), we want to avoid the
additional kmap_atomic if it's unnecessary.
Signed-off-by: default avatarRussell King <rmk+kernel@arm.linux.org.uk>
parent 063b0a42
...@@ -111,7 +111,7 @@ ...@@ -111,7 +111,7 @@
struct page; struct page;
struct cpu_user_fns { struct cpu_user_fns {
void (*cpu_clear_user_page)(void *p, unsigned long user); void (*cpu_clear_user_highpage)(struct page *page, unsigned long vaddr);
void (*cpu_copy_user_highpage)(struct page *to, struct page *from, void (*cpu_copy_user_highpage)(struct page *to, struct page *from,
unsigned long vaddr); unsigned long vaddr);
}; };
...@@ -119,20 +119,21 @@ struct cpu_user_fns { ...@@ -119,20 +119,21 @@ struct cpu_user_fns {
#ifdef MULTI_USER #ifdef MULTI_USER
extern struct cpu_user_fns cpu_user; extern struct cpu_user_fns cpu_user;
#define __cpu_clear_user_page cpu_user.cpu_clear_user_page #define __cpu_clear_user_highpage cpu_user.cpu_clear_user_highpage
#define __cpu_copy_user_highpage cpu_user.cpu_copy_user_highpage #define __cpu_copy_user_highpage cpu_user.cpu_copy_user_highpage
#else #else
#define __cpu_clear_user_page __glue(_USER,_clear_user_page) #define __cpu_clear_user_highpage __glue(_USER,_clear_user_highpage)
#define __cpu_copy_user_highpage __glue(_USER,_copy_user_highpage) #define __cpu_copy_user_highpage __glue(_USER,_copy_user_highpage)
extern void __cpu_clear_user_page(void *p, unsigned long user); extern void __cpu_clear_user_highpage(struct page *page, unsigned long vaddr);
extern void __cpu_copy_user_highpage(struct page *to, struct page *from, extern void __cpu_copy_user_highpage(struct page *to, struct page *from,
unsigned long vaddr); unsigned long vaddr);
#endif #endif
#define clear_user_page(addr,vaddr,pg) __cpu_clear_user_page(addr, vaddr) #define clear_user_highpage(page,vaddr) \
__cpu_clear_user_highpage(page, vaddr)
#define __HAVE_ARCH_COPY_USER_HIGHPAGE #define __HAVE_ARCH_COPY_USER_HIGHPAGE
#define copy_user_highpage(to,from,vaddr,vma) \ #define copy_user_highpage(to,from,vaddr,vma) \
......
...@@ -79,12 +79,11 @@ void feroceon_copy_user_highpage(struct page *to, struct page *from, ...@@ -79,12 +79,11 @@ void feroceon_copy_user_highpage(struct page *to, struct page *from,
kunmap_atomic(kto, KM_USER0); kunmap_atomic(kto, KM_USER0);
} }
void __attribute__((naked)) void feroceon_clear_user_highpage(struct page *page, unsigned long vaddr)
feroceon_clear_user_page(void *kaddr, unsigned long vaddr)
{ {
void *kaddr = kmap_atomic(page, KM_USER0);
asm("\ asm("\
stmfd sp!, {r4-r7, lr} \n\ mov r1, %1 \n\
mov r1, %0 \n\
mov r2, #0 \n\ mov r2, #0 \n\
mov r3, #0 \n\ mov r3, #0 \n\
mov r4, #0 \n\ mov r4, #0 \n\
...@@ -93,19 +92,20 @@ feroceon_clear_user_page(void *kaddr, unsigned long vaddr) ...@@ -93,19 +92,20 @@ feroceon_clear_user_page(void *kaddr, unsigned long vaddr)
mov r7, #0 \n\ mov r7, #0 \n\
mov ip, #0 \n\ mov ip, #0 \n\
mov lr, #0 \n\ mov lr, #0 \n\
1: stmia r0, {r2-r7, ip, lr} \n\ 1: stmia %0, {r2-r7, ip, lr} \n\
subs r1, r1, #1 \n\ subs r1, r1, #1 \n\
mcr p15, 0, r0, c7, c14, 1 @ clean and invalidate D line\n\ mcr p15, 0, %0, c7, c14, 1 @ clean and invalidate D line\n\
add r0, r0, #32 \n\ add r0, r0, #32 \n\
bne 1b \n\ bne 1b \n\
mcr p15, 0, r1, c7, c10, 4 @ drain WB\n\ mcr p15, 0, r1, c7, c10, 4 @ drain WB"
ldmfd sp!, {r4-r7, pc}"
: :
: "I" (PAGE_SIZE / 32)); : "r" (kaddr), "I" (PAGE_SIZE / 32)
: "r1", "r2", "r3", "r4", "r5", "r6", "r7", "ip", "lr");
kunmap_atomic(kaddr, KM_USER0);
} }
struct cpu_user_fns feroceon_user_fns __initdata = { struct cpu_user_fns feroceon_user_fns __initdata = {
.cpu_clear_user_page = feroceon_clear_user_page, .cpu_clear_user_highpage = feroceon_clear_user_highpage,
.cpu_copy_user_highpage = feroceon_copy_user_highpage, .cpu_copy_user_highpage = feroceon_copy_user_highpage,
}; };
...@@ -54,10 +54,10 @@ void v3_copy_user_highpage(struct page *to, struct page *from, ...@@ -54,10 +54,10 @@ void v3_copy_user_highpage(struct page *to, struct page *from,
* *
* FIXME: do we need to handle cache stuff... * FIXME: do we need to handle cache stuff...
*/ */
void __attribute__((naked)) v3_clear_user_page(void *kaddr, unsigned long vaddr) void v3_clear_user_highpage(struct page *page, unsigned long vaddr)
{ {
void *kaddr = kmap_atomic(page, KM_USER0);
asm("\n\ asm("\n\
str lr, [sp, #-4]!\n\
mov r1, %1 @ 1\n\ mov r1, %1 @ 1\n\
mov r2, #0 @ 1\n\ mov r2, #0 @ 1\n\
mov r3, #0 @ 1\n\ mov r3, #0 @ 1\n\
...@@ -68,13 +68,14 @@ void __attribute__((naked)) v3_clear_user_page(void *kaddr, unsigned long vaddr) ...@@ -68,13 +68,14 @@ void __attribute__((naked)) v3_clear_user_page(void *kaddr, unsigned long vaddr)
stmia %0!, {r2, r3, ip, lr} @ 4\n\ stmia %0!, {r2, r3, ip, lr} @ 4\n\
stmia %0!, {r2, r3, ip, lr} @ 4\n\ stmia %0!, {r2, r3, ip, lr} @ 4\n\
subs r1, r1, #1 @ 1\n\ subs r1, r1, #1 @ 1\n\
bne 1b @ 1\n\ bne 1b @ 1"
ldr pc, [sp], #4"
: :
: "r" (kaddr), "I" (PAGE_SIZE / 64)); : "r" (kaddr), "I" (PAGE_SIZE / 64)
: "r1", "r2", "r3", "ip", "lr");
kunmap_atomic(kaddr, KM_USER0);
} }
struct cpu_user_fns v3_user_fns __initdata = { struct cpu_user_fns v3_user_fns __initdata = {
.cpu_clear_user_page = v3_clear_user_page, .cpu_clear_user_highpage = v3_clear_user_highpage,
.cpu_copy_user_highpage = v3_copy_user_highpage, .cpu_copy_user_highpage = v3_copy_user_highpage,
}; };
...@@ -91,30 +91,30 @@ void v4_mc_copy_user_highpage(struct page *from, struct page *to, ...@@ -91,30 +91,30 @@ void v4_mc_copy_user_highpage(struct page *from, struct page *to,
/* /*
* ARMv4 optimised clear_user_page * ARMv4 optimised clear_user_page
*/ */
void __attribute__((naked)) void v4_mc_clear_user_highpage(struct page *page, unsigned long vaddr)
v4_mc_clear_user_page(void *kaddr, unsigned long vaddr)
{ {
asm volatile( void *kaddr = kmap_atomic(page, KM_USER0);
"str lr, [sp, #-4]!\n\ asm volatile("\
mov r1, %0 @ 1\n\ mov r1, %0 @ 1\n\
mov r2, #0 @ 1\n\ mov r2, #0 @ 1\n\
mov r3, #0 @ 1\n\ mov r3, #0 @ 1\n\
mov ip, #0 @ 1\n\ mov ip, #0 @ 1\n\
mov lr, #0 @ 1\n\ mov lr, #0 @ 1\n\
1: mcr p15, 0, r0, c7, c6, 1 @ 1 invalidate D line\n\ 1: mcr p15, 0, %0, c7, c6, 1 @ 1 invalidate D line\n\
stmia r0!, {r2, r3, ip, lr} @ 4\n\ stmia %0!, {r2, r3, ip, lr} @ 4\n\
stmia r0!, {r2, r3, ip, lr} @ 4\n\ stmia %0!, {r2, r3, ip, lr} @ 4\n\
mcr p15, 0, r0, c7, c6, 1 @ 1 invalidate D line\n\ mcr p15, 0, %0, c7, c6, 1 @ 1 invalidate D line\n\
stmia r0!, {r2, r3, ip, lr} @ 4\n\ stmia %0!, {r2, r3, ip, lr} @ 4\n\
stmia r0!, {r2, r3, ip, lr} @ 4\n\ stmia %0!, {r2, r3, ip, lr} @ 4\n\
subs r1, r1, #1 @ 1\n\ subs r1, r1, #1 @ 1\n\
bne 1b @ 1\n\ bne 1b @ 1"
ldr pc, [sp], #4"
: :
: "I" (PAGE_SIZE / 64)); : "r" (kaddr), "I" (PAGE_SIZE / 64)
: "r1", "r2", "r3", "ip", "lr");
kunmap_atomic(kaddr, KM_USER0);
} }
struct cpu_user_fns v4_mc_user_fns __initdata = { struct cpu_user_fns v4_mc_user_fns __initdata = {
.cpu_clear_user_page = v4_mc_clear_user_page, .cpu_clear_user_highpage = v4_mc_clear_user_highpage,
.cpu_copy_user_highpage = v4_mc_copy_user_highpage, .cpu_copy_user_highpage = v4_mc_copy_user_highpage,
}; };
...@@ -64,31 +64,31 @@ void v4wb_copy_user_highpage(struct page *to, struct page *from, ...@@ -64,31 +64,31 @@ void v4wb_copy_user_highpage(struct page *to, struct page *from,
* *
* Same story as above. * Same story as above.
*/ */
void __attribute__((naked)) void v4wb_clear_user_highpage(struct page *page, unsigned long vaddr)
v4wb_clear_user_page(void *kaddr, unsigned long vaddr)
{ {
void *kaddr = kmap_atomic(page, KM_USER0);
asm("\ asm("\
str lr, [sp, #-4]!\n\ mov r1, %1 @ 1\n\
mov r1, %0 @ 1\n\
mov r2, #0 @ 1\n\ mov r2, #0 @ 1\n\
mov r3, #0 @ 1\n\ mov r3, #0 @ 1\n\
mov ip, #0 @ 1\n\ mov ip, #0 @ 1\n\
mov lr, #0 @ 1\n\ mov lr, #0 @ 1\n\
1: mcr p15, 0, r0, c7, c6, 1 @ 1 invalidate D line\n\ 1: mcr p15, 0, %0, c7, c6, 1 @ 1 invalidate D line\n\
stmia r0!, {r2, r3, ip, lr} @ 4\n\ stmia %0!, {r2, r3, ip, lr} @ 4\n\
stmia r0!, {r2, r3, ip, lr} @ 4\n\ stmia %0!, {r2, r3, ip, lr} @ 4\n\
mcr p15, 0, r0, c7, c6, 1 @ 1 invalidate D line\n\ mcr p15, 0, %0, c7, c6, 1 @ 1 invalidate D line\n\
stmia r0!, {r2, r3, ip, lr} @ 4\n\ stmia %0!, {r2, r3, ip, lr} @ 4\n\
stmia r0!, {r2, r3, ip, lr} @ 4\n\ stmia %0!, {r2, r3, ip, lr} @ 4\n\
subs r1, r1, #1 @ 1\n\ subs r1, r1, #1 @ 1\n\
bne 1b @ 1\n\ bne 1b @ 1\n\
mcr p15, 0, r1, c7, c10, 4 @ 1 drain WB\n\ mcr p15, 0, r1, c7, c10, 4 @ 1 drain WB"
ldr pc, [sp], #4"
: :
: "I" (PAGE_SIZE / 64)); : "r" (kaddr), "I" (PAGE_SIZE / 64)
: "r1", "r2", "r3", "ip", "lr");
kunmap_atomic(kaddr, KM_USER0);
} }
struct cpu_user_fns v4wb_user_fns __initdata = { struct cpu_user_fns v4wb_user_fns __initdata = {
.cpu_clear_user_page = v4wb_clear_user_page, .cpu_clear_user_highpage = v4wb_clear_user_highpage,
.cpu_copy_user_highpage = v4wb_copy_user_highpage, .cpu_copy_user_highpage = v4wb_copy_user_highpage,
}; };
...@@ -60,29 +60,29 @@ void v4wt_copy_user_highpage(struct page *to, struct page *from, ...@@ -60,29 +60,29 @@ void v4wt_copy_user_highpage(struct page *to, struct page *from,
* *
* Same story as above. * Same story as above.
*/ */
void __attribute__((naked)) void v4wt_clear_user_highpage(struct page *page, unsigned long vaddr)
v4wt_clear_user_page(void *kaddr, unsigned long vaddr)
{ {
void *kaddr = kmap_atomic(page, KM_USER0);
asm("\ asm("\
str lr, [sp, #-4]!\n\ mov r1, %1 @ 1\n\
mov r1, %0 @ 1\n\
mov r2, #0 @ 1\n\ mov r2, #0 @ 1\n\
mov r3, #0 @ 1\n\ mov r3, #0 @ 1\n\
mov ip, #0 @ 1\n\ mov ip, #0 @ 1\n\
mov lr, #0 @ 1\n\ mov lr, #0 @ 1\n\
1: stmia r0!, {r2, r3, ip, lr} @ 4\n\ 1: stmia %0!, {r2, r3, ip, lr} @ 4\n\
stmia r0!, {r2, r3, ip, lr} @ 4\n\ stmia %0!, {r2, r3, ip, lr} @ 4\n\
stmia r0!, {r2, r3, ip, lr} @ 4\n\ stmia %0!, {r2, r3, ip, lr} @ 4\n\
stmia r0!, {r2, r3, ip, lr} @ 4\n\ stmia %0!, {r2, r3, ip, lr} @ 4\n\
subs r1, r1, #1 @ 1\n\ subs r1, r1, #1 @ 1\n\
bne 1b @ 1\n\ bne 1b @ 1\n\
mcr p15, 0, r2, c7, c7, 0 @ flush ID cache\n\ mcr p15, 0, r2, c7, c7, 0 @ flush ID cache"
ldr pc, [sp], #4"
: :
: "I" (PAGE_SIZE / 64)); : "r" (kaddr), "I" (PAGE_SIZE / 64)
: "r1", "r2", "r3", "ip", "lr");
kunmap_atomic(kaddr, KM_USER0);
} }
struct cpu_user_fns v4wt_user_fns __initdata = { struct cpu_user_fns v4wt_user_fns __initdata = {
.cpu_clear_user_page = v4wt_clear_user_page, .cpu_clear_user_highpage = v4wt_clear_user_highpage,
.cpu_copy_user_highpage = v4wt_copy_user_highpage, .cpu_copy_user_highpage = v4wt_copy_user_highpage,
}; };
...@@ -49,9 +49,11 @@ static void v6_copy_user_highpage_nonaliasing(struct page *to, ...@@ -49,9 +49,11 @@ static void v6_copy_user_highpage_nonaliasing(struct page *to,
* Clear the user page. No aliasing to deal with so we can just * Clear the user page. No aliasing to deal with so we can just
* attack the kernel's existing mapping of this page. * attack the kernel's existing mapping of this page.
*/ */
static void v6_clear_user_page_nonaliasing(void *kaddr, unsigned long vaddr) static void v6_clear_user_highpage_nonaliasing(struct page *page, unsigned long vaddr)
{ {
void *kaddr = kmap_atomic(page, KM_USER0);
clear_page(kaddr); clear_page(kaddr);
kunmap_atomic(kaddr, KM_USER0);
} }
/* /*
...@@ -107,20 +109,13 @@ static void v6_copy_user_highpage_aliasing(struct page *to, ...@@ -107,20 +109,13 @@ static void v6_copy_user_highpage_aliasing(struct page *to,
* so remap the kernel page into the same cache colour as the user * so remap the kernel page into the same cache colour as the user
* page. * page.
*/ */
static void v6_clear_user_page_aliasing(void *kaddr, unsigned long vaddr) static void v6_clear_user_highpage_aliasing(struct page *page, unsigned long vaddr)
{ {
unsigned int offset = CACHE_COLOUR(vaddr); unsigned int offset = CACHE_COLOUR(vaddr);
unsigned long to = to_address + (offset << PAGE_SHIFT); unsigned long to = to_address + (offset << PAGE_SHIFT);
/* /* FIXME: not highmem safe */
* Discard data in the kernel mapping for the new page discard_old_kernel_data(page_address(page));
* FIXME: needs this MCRR to be supported.
*/
__asm__("mcrr p15, 0, %1, %0, c6 @ 0xec401f06"
:
: "r" (kaddr),
"r" ((unsigned long)kaddr + PAGE_SIZE - L1_CACHE_BYTES)
: "cc");
/* /*
* Now clear the page using the same cache colour as * Now clear the page using the same cache colour as
...@@ -128,7 +123,7 @@ static void v6_clear_user_page_aliasing(void *kaddr, unsigned long vaddr) ...@@ -128,7 +123,7 @@ static void v6_clear_user_page_aliasing(void *kaddr, unsigned long vaddr)
*/ */
spin_lock(&v6_lock); spin_lock(&v6_lock);
set_pte_ext(TOP_PTE(to_address) + offset, pfn_pte(__pa(kaddr) >> PAGE_SHIFT, PAGE_KERNEL), 0); set_pte_ext(TOP_PTE(to_address) + offset, pfn_pte(page_to_pfn(page), PAGE_KERNEL), 0);
flush_tlb_kernel_page(to); flush_tlb_kernel_page(to);
clear_page((void *)to); clear_page((void *)to);
...@@ -136,14 +131,14 @@ static void v6_clear_user_page_aliasing(void *kaddr, unsigned long vaddr) ...@@ -136,14 +131,14 @@ static void v6_clear_user_page_aliasing(void *kaddr, unsigned long vaddr)
} }
struct cpu_user_fns v6_user_fns __initdata = { struct cpu_user_fns v6_user_fns __initdata = {
.cpu_clear_user_page = v6_clear_user_page_nonaliasing, .cpu_clear_user_highpage = v6_clear_user_highpage_nonaliasing,
.cpu_copy_user_highpage = v6_copy_user_highpage_nonaliasing, .cpu_copy_user_highpage = v6_copy_user_highpage_nonaliasing,
}; };
static int __init v6_userpage_init(void) static int __init v6_userpage_init(void)
{ {
if (cache_is_vipt_aliasing()) { if (cache_is_vipt_aliasing()) {
cpu_user.cpu_clear_user_page = v6_clear_user_page_aliasing; cpu_user.cpu_clear_user_highpage = v6_clear_user_highpage_aliasing;
cpu_user.cpu_copy_user_highpage = v6_copy_user_highpage_aliasing; cpu_user.cpu_copy_user_highpage = v6_copy_user_highpage_aliasing;
} }
......
...@@ -87,26 +87,27 @@ void xsc3_mc_copy_user_highpage(struct page *to, struct page *from, ...@@ -87,26 +87,27 @@ void xsc3_mc_copy_user_highpage(struct page *to, struct page *from,
* r0 = destination * r0 = destination
* r1 = virtual user address of ultimate destination page * r1 = virtual user address of ultimate destination page
*/ */
void __attribute__((naked)) void xsc3_mc_clear_user_highpage(struct page *page, unsigned long vaddr)
xsc3_mc_clear_user_page(void *kaddr, unsigned long vaddr)
{ {
void *kaddr = kmap_atomic(page, KM_USER0);
asm("\ asm("\
mov r1, %0 \n\ mov r1, %1 \n\
mov r2, #0 \n\ mov r2, #0 \n\
mov r3, #0 \n\ mov r3, #0 \n\
1: mcr p15, 0, r0, c7, c6, 1 @ invalidate line\n\ 1: mcr p15, 0, %0, c7, c6, 1 @ invalidate line\n\
strd r2, [r0], #8 \n\ strd r2, [%0], #8 \n\
strd r2, [r0], #8 \n\ strd r2, [%0], #8 \n\
strd r2, [r0], #8 \n\ strd r2, [%0], #8 \n\
strd r2, [r0], #8 \n\ strd r2, [%0], #8 \n\
subs r1, r1, #1 \n\ subs r1, r1, #1 \n\
bne 1b \n\ bne 1b"
mov pc, lr"
: :
: "I" (PAGE_SIZE / 32)); : "r" (kaddr), "I" (PAGE_SIZE / 32)
: "r1", "r2", "r3");
kunmap_atomic(kaddr, KM_USER0);
} }
struct cpu_user_fns xsc3_mc_user_fns __initdata = { struct cpu_user_fns xsc3_mc_user_fns __initdata = {
.cpu_clear_user_page = xsc3_mc_clear_user_page, .cpu_clear_user_highpage = xsc3_mc_clear_user_highpage,
.cpu_copy_user_highpage = xsc3_mc_copy_user_highpage, .cpu_copy_user_highpage = xsc3_mc_copy_user_highpage,
}; };
...@@ -113,28 +113,30 @@ void xscale_mc_copy_user_highpage(struct page *to, struct page *from, ...@@ -113,28 +113,30 @@ void xscale_mc_copy_user_highpage(struct page *to, struct page *from,
/* /*
* XScale optimised clear_user_page * XScale optimised clear_user_page
*/ */
void __attribute__((naked)) void
xscale_mc_clear_user_page(void *kaddr, unsigned long vaddr) xscale_mc_clear_user_highpage(struct page *page, unsigned long vaddr)
{ {
void *kaddr = kmap_atomic(page, KM_USER0);
asm volatile( asm volatile(
"mov r1, %0 \n\ "mov r1, %1 \n\
mov r2, #0 \n\ mov r2, #0 \n\
mov r3, #0 \n\ mov r3, #0 \n\
1: mov ip, r0 \n\ 1: mov ip, %0 \n\
strd r2, [r0], #8 \n\ strd r2, [%0], #8 \n\
strd r2, [r0], #8 \n\ strd r2, [%0], #8 \n\
strd r2, [r0], #8 \n\ strd r2, [%0], #8 \n\
strd r2, [r0], #8 \n\ strd r2, [%0], #8 \n\
mcr p15, 0, ip, c7, c10, 1 @ clean D line\n\ mcr p15, 0, ip, c7, c10, 1 @ clean D line\n\
subs r1, r1, #1 \n\ subs r1, r1, #1 \n\
mcr p15, 0, ip, c7, c6, 1 @ invalidate D line\n\ mcr p15, 0, ip, c7, c6, 1 @ invalidate D line\n\
bne 1b \n\ bne 1b"
mov pc, lr"
: :
: "I" (PAGE_SIZE / 32)); : "r" (kaddr), "I" (PAGE_SIZE / 32)
: "r1", "r2", "r3", "ip");
kunmap_atomic(kaddr, KM_USER0);
} }
struct cpu_user_fns xscale_mc_user_fns __initdata = { struct cpu_user_fns xscale_mc_user_fns __initdata = {
.cpu_clear_user_page = xscale_mc_clear_user_page, .cpu_clear_user_highpage = xscale_mc_clear_user_highpage,
.cpu_copy_user_highpage = xscale_mc_copy_user_highpage, .cpu_copy_user_highpage = xscale_mc_copy_user_highpage,
}; };
...@@ -33,7 +33,7 @@ EXPORT_SYMBOL(cpu_cache); ...@@ -33,7 +33,7 @@ EXPORT_SYMBOL(cpu_cache);
#ifdef CONFIG_MMU #ifdef CONFIG_MMU
#ifndef MULTI_USER #ifndef MULTI_USER
EXPORT_SYMBOL(__cpu_clear_user_page); EXPORT_SYMBOL(__cpu_clear_user_highpage);
EXPORT_SYMBOL(__cpu_copy_user_highpage); EXPORT_SYMBOL(__cpu_copy_user_highpage);
#else #else
EXPORT_SYMBOL(cpu_user); EXPORT_SYMBOL(cpu_user);
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment