[PATCH 0/2] MIPS: Chore clean ups

[PATCH 0/2] MIPS: Chore clean ups

[Jiaxun Yang]

Hi all,

This is a spin-off from my mips-rust series, to fix some warnings,
misuse of data type.

Thanks

To: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
Cc: linux-kernel@vger.kernel.org

Signed-off-by: Jiaxun Yang <jiaxun.yang@flygoat.com>
---
Jiaxun Yang (2):
      MIPS: ftrace: Mark ftrace_get_parent_ra_addr as static
      MIPS: kprobes: Massage previous delay slot detection

 arch/mips/kernel/ftrace.c  |  4 ++--
 arch/mips/kernel/kprobes.c | 12 ++++++------
 2 files changed, 8 insertions(+), 8 deletions(-)
---
base-commit: 9aaeb87ce1e966169a57f53a02ba05b30880ffb8
change-id: 20240908-mips-chore-25f2c66cc388

Best regards,
-- 
Jiaxun Yang <jiaxun.yang@flygoat.com>

[PATCH 1/2] MIPS: ftrace: Mark ftrace_get_parent_ra_addr as static

[Jiaxun Yang]

This function is only used in ftrace.c.

Mark as static to avoid compiler warning.

Signed-off-by: Jiaxun Yang <jiaxun.yang@flygoat.com>
---
 arch/mips/kernel/ftrace.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/mips/kernel/ftrace.c b/arch/mips/kernel/ftrace.c
index 8c401e42301c..96016eb1c476 100644
--- a/arch/mips/kernel/ftrace.c
+++ b/arch/mips/kernel/ftrace.c
@@ -248,8 +248,8 @@ int ftrace_disable_ftrace_graph_caller(void)
 #define S_R_SP	(0xafb0 << 16)	/* s{d,w} R, offset(sp) */
 #define OFFSET_MASK	0xffff	/* stack offset range: 0 ~ PT_SIZE */
 
-unsigned long ftrace_get_parent_ra_addr(unsigned long self_ra, unsigned long
-		old_parent_ra, unsigned long parent_ra_addr, unsigned long fp)
+static unsigned long ftrace_get_parent_ra_addr(unsigned long self_ra, unsigned long old_parent_ra,
+					       unsigned long parent_ra_addr, unsigned long fp)
 {
 	unsigned long sp, ip, tmp;
 	unsigned int code;

-- 
2.46.0

Re: [PATCH 1/2] MIPS: ftrace: Mark ftrace_get_parent_ra_addr as static

[Maciej W. Rozycki]

On Sun, 8 Sep 2024, Jiaxun Yang wrote:

> diff --git a/arch/mips/kernel/ftrace.c b/arch/mips/kernel/ftrace.c
> index 8c401e42301c..96016eb1c476 100644
> --- a/arch/mips/kernel/ftrace.c
> +++ b/arch/mips/kernel/ftrace.c
> @@ -248,8 +248,8 @@ int ftrace_disable_ftrace_graph_caller(void)
>  #define S_R_SP	(0xafb0 << 16)	/* s{d,w} R, offset(sp) */
>  #define OFFSET_MASK	0xffff	/* stack offset range: 0 ~ PT_SIZE */
>  
> -unsigned long ftrace_get_parent_ra_addr(unsigned long self_ra, unsigned long
> -		old_parent_ra, unsigned long parent_ra_addr, unsigned long fp)
> +static unsigned long ftrace_get_parent_ra_addr(unsigned long self_ra, unsigned long old_parent_ra,
> +					       unsigned long parent_ra_addr, unsigned long fp)

 Overlong lines.

  Maciej

[PATCH 2/2] MIPS: kprobes: Massage previous delay slot detection

[Jiaxun Yang]

Expand the if condition into cascaded ifs to make code
readable.

Also use sizeof(union mips_instruction) instead of
sizeof(mips_instruction) to match the code context.

Signed-off-by: Jiaxun Yang <jiaxun.yang@flygoat.com>
---
 arch/mips/kernel/kprobes.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/arch/mips/kernel/kprobes.c b/arch/mips/kernel/kprobes.c
index dc39f5b3fb83..96139adefad2 100644
--- a/arch/mips/kernel/kprobes.c
+++ b/arch/mips/kernel/kprobes.c
@@ -89,12 +89,12 @@ int arch_prepare_kprobe(struct kprobe *p)
 		goto out;
 	}
 
-	if (copy_from_kernel_nofault(&prev_insn, p->addr - 1,
-			sizeof(mips_instruction)) == 0 &&
-	    insn_has_delayslot(prev_insn)) {
-		pr_notice("Kprobes for branch delayslot are not supported\n");
-		ret = -EINVAL;
-		goto out;
+	if (!copy_from_kernel_nofault(&prev_insn, p->addr - 1, sizeof(union mips_instruction))) {
+		if (insn_has_delayslot(prev_insn)) {
+			pr_notice("Kprobes for branch delayslot are not supported\n");
+			ret = -EINVAL;
+			goto out;
+		}
 	}
 
 	if (__insn_is_compact_branch(insn)) {

-- 
2.46.0

Re: [PATCH 2/2] MIPS: kprobes: Massage previous delay slot detection

[Maciej W. Rozycki]

On Sun, 8 Sep 2024, Jiaxun Yang wrote:

> Expand the if condition into cascaded ifs to make code
> readable.

 Apart from broken formatting what's making original code unreadable?

> Also use sizeof(union mips_instruction) instead of
> sizeof(mips_instruction) to match the code context.

 That has to be a separate change.

> diff --git a/arch/mips/kernel/kprobes.c b/arch/mips/kernel/kprobes.c
> index dc39f5b3fb83..96139adefad2 100644
> --- a/arch/mips/kernel/kprobes.c
> +++ b/arch/mips/kernel/kprobes.c
> @@ -89,12 +89,12 @@ int arch_prepare_kprobe(struct kprobe *p)
>  		goto out;
>  	}
>  
> -	if (copy_from_kernel_nofault(&prev_insn, p->addr - 1,
> -			sizeof(mips_instruction)) == 0 &&
> -	    insn_has_delayslot(prev_insn)) {
> -		pr_notice("Kprobes for branch delayslot are not supported\n");
> -		ret = -EINVAL;
> -		goto out;
> +	if (!copy_from_kernel_nofault(&prev_insn, p->addr - 1, sizeof(union mips_instruction))) {

 Overlong line.

> +		if (insn_has_delayslot(prev_insn)) {
> +			pr_notice("Kprobes for branch delayslot are not supported\n");

 This now overruns 80 columns making code *less* readable.

  Maciej

Re: [PATCH 2/2] MIPS: kprobes: Massage previous delay slot detection

[Jiaxun Yang]

在2024年9月9日九月 下午11:02，Maciej W. Rozycki写道：
> On Sun, 8 Sep 2024, Jiaxun Yang wrote:
>
>> Expand the if condition into cascaded ifs to make code
>> readable.
>
>  Apart from broken formatting what's making original code unreadable?

For me it's confusing because wired layout, cascaded ifs are clearly
easier to format and has clear intention.

>
>> Also use sizeof(union mips_instruction) instead of
>> sizeof(mips_instruction) to match the code context.
>
>  That has to be a separate change.

Given that it's a tiny style change as well, it makes sense to combine
into same patch.

>
>> diff --git a/arch/mips/kernel/kprobes.c b/arch/mips/kernel/kprobes.c
>> index dc39f5b3fb83..96139adefad2 100644
>> --- a/arch/mips/kernel/kprobes.c
>> +++ b/arch/mips/kernel/kprobes.c
>> @@ -89,12 +89,12 @@ int arch_prepare_kprobe(struct kprobe *p)
>>  		goto out;
>>  	}
>>  
>> -	if (copy_from_kernel_nofault(&prev_insn, p->addr - 1,
>> -			sizeof(mips_instruction)) == 0 &&
>> -	    insn_has_delayslot(prev_insn)) {
>> -		pr_notice("Kprobes for branch delayslot are not supported\n");
>> -		ret = -EINVAL;
>> -		goto out;
>> +	if (!copy_from_kernel_nofault(&prev_insn, p->addr - 1, sizeof(union mips_instruction))) {
>
>  Overlong line.

Nowadays, check-patch.pl is happy with 100 column line.

I used 100 column line in many subsystems and never receive any complaint.

>
>> +		if (insn_has_delayslot(prev_insn)) {
>> +			pr_notice("Kprobes for branch delayslot are not supported\n");
>
>  This now overruns 80 columns making code *less* readable.

I don't really agree, we are not in VGA display era any more, see Linus's
arguments on removal of 80 columns [1] and why long line are more readable [2].


[1]: https://lore.kernel.org/lkml/CAHk-=wj3iGQqjpvc+gf6+C29Jo4COj6OQQFzdY0h5qvYKTdCow@mail.gmail.com/
[2]: https://lore.kernel.org/lkml/CAHk-=wjR0H3+2ba0UUWwoYzYBH0GX9yTf5dj2MZyo0xvyzvJnA@mail.gmail.com/

Thanks
>
>   Maciej

-- 
- Jiaxun

Re: [PATCH 2/2] MIPS: kprobes: Massage previous delay slot detection

[Thomas Bogendoerfer]

On Tue, Sep 10, 2024 at 10:43:23AM +0100, Jiaxun Yang wrote:
> 
> 
> 在2024年9月9日九月 下午11:02，Maciej W. Rozycki写道：
> > On Sun, 8 Sep 2024, Jiaxun Yang wrote:
> >
> >> Expand the if condition into cascaded ifs to make code
> >> readable.
> >
> >  Apart from broken formatting what's making original code unreadable?
> 
> For me it's confusing because wired layout, cascaded ifs are clearly
> easier to format and has clear intention.

I prefer the original version, it's just to statements combined with &&,
which isn't scary at all.

Thomas.

-- 
Crap can work. Given enough thrust pigs will fly, but it's not necessarily a
good idea.                                                [ RFC1925, 2.3 ]

Re: [PATCH 2/2] MIPS: kprobes: Massage previous delay slot detection

[Maciej W. Rozycki]

On Tue, 10 Sep 2024, Jiaxun Yang wrote:

> >> +		if (insn_has_delayslot(prev_insn)) {
> >> +			pr_notice("Kprobes for branch delayslot are not supported\n");
> >
> >  This now overruns 80 columns making code *less* readable.
> 
> I don't really agree, we are not in VGA display era any more, see Linus's
> arguments on removal of 80 columns [1] and why long line are more readable [2].

 Human perception hasn't changed though and just that you can squeeze a 
vast number of characters in a line it doesn't mean you can parse them 
comfortably with eyes.  Printed books have a common line length limit too, 
established with centuries of experience.  Some projects such as GDB 
prefer a lower soft limit of 74 characters even (with 80 being the hard 
one), exactly for this reason[1].

 NB even back in 1990s there was an option to use 132-character lines with 
SVGA hardware in text mode, but hardly anybody used that because, well, it 
didn't make text any more readable.  Rather one became lost right away.

 Last but not least Documentation/process/coding-style.rst still mandates
80-column formatting.

References:

[1] <https://sourceware.org/ml/gdb-patches/2014-01/msg00216.html>

  Maciej