[PATCH 0/4] MIPS: Fix missing proto and passing arg warnings

[PATCH 0/4] MIPS: Fix missing proto and passing arg warnings

[Serge Semin]

After getting my local tree rebased onto the kernel 6.8-rc3 the MIPS32
kernel build procedure produced a couple of warnings which I suggest to
fix in the framework of this series.

A first warning is of the "no previous prototype for `<func>`" type. In
particular my arch-specific code has the mips_cm_l2sync_phys_base() method
re-defined, but even though the function is global it' prototype isn't
declared anywhere. Fix that by adding the method prototype declaration to
the mips/include/asm/mips-cm.h header file. A similar solution was
provided for the methods:
__mips_cm_l2sync_phys_base()
mips_cm_phys_base()
__mips_cm_phys_base()
too (Please see the patches 1/4 and 2/4 notes section for an alternative
suggestion of the way to fix the warning).

One more case of the denoted warning I spotted in the self-extracting
kernel (so called zboot) with the debug printouts enabled. In particular
there are several putc() method re-definitions available in:
arch/mips/boot/compressed/uart-prom.c
arch/mips/boot/compressed/uart-16550.c
arch/mips/boot/compressed/uart-alchemy.c
All of these files lacked the prototype declaration what caused having the
"no previous prototype for ‘putc’" printed on my build with the next
configs enabled:
CONFIG_SYS_SUPPORTS_ZBOOT=y
CONFIG_SYS_SUPPORTS_ZBOOT_UART_PROM=y
CONFIG_ZBOOT_LOAD_ADDRESS=0x85100000
CONFIG_DEBUG_ZBOOT=y

The second warning is of the "passing argument <x> of ‘<func>’ from
incompatible pointer type" type which I discovered in the
drivers/tty/mips_ejtag_fdc.c driver. The problem most likely happened due
to the commit ce7cbd9a6c81 ("tty: mips_ejtag_fdc: use u8 for character
pointers").

That's it for today.) Thanks for review in advance. Any tests are very
welcome.

Cc: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru>
Cc: Stephen Rothwell <sfr@rothwell.id.au>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: linux-mips@vger.kernel.org
Cc: linux-serial@vger.kernel.org
Cc: linux-kernel@vger.kernel.org

Serge Semin (4):
  mips: cm: Add __mips_cm_l2sync_phys_base prototype declaration
  mips: cm: Add CM GCR and L2-sync base address getters declarations
  mips: zboot: Fix "no previous prototype" build warning
  tty: mips_ejtag_fdc: Fix passing incompatible pointer type warning

 arch/mips/boot/compressed/uart-16550.c   |  2 ++
 arch/mips/boot/compressed/uart-alchemy.c |  2 ++
 arch/mips/boot/compressed/uart-prom.c    |  2 ++
 arch/mips/include/asm/mips-cm.h          | 16 ++++++++++++++++
 arch/mips/kernel/mips-cm.c               |  2 +-
 drivers/tty/mips_ejtag_fdc.c             |  2 +-
 6 files changed, 24 insertions(+), 2 deletions(-)

-- 
2.43.0

[PATCH 1/4] mips: cm: Add __mips_cm_l2sync_phys_base prototype declaration

[Serge Semin]

The __mips_cm_l2sync_phys_base() and mips_cm_l2sync_phys_base() couple was
introduced in commit 9f98f3dd0c51 ("MIPS: Add generic CM probe & access
code") where the former method was a weak implementation of the later
function. Such design pattern permitted to re-define the original method
and use the weak implementation in the new function. A similar approach
was introduced in the framework of another arch-specific programmable
interface: mips_cm_phys_base() and __mips_cm_phys_base(). The only
difference is that the underscored method of the later couple was declared
in the "asm/mips-cm.h" header file, but it wasn't done for the CM L2-sync
methods in the subject. Due to the missing the global function declaration
the "missing prototype" warning was spotted in the framework of the commit
9a2036724cd6 ("mips: mark local function static if possible") and fixed
just be re-qualifying the weak method as static. Doing that broke what was
originally implied by having the weak implementation globally defined. Fix
that by dropping the static qualifier and adding the
__mips_cm_l2sync_phys_base() prototype declared in the "asm/mips-cm.h"
header file.

Fixes: 9a2036724cd6 ("mips: mark local function static if possible")
Signed-off-by: Serge Semin <fancer.lancer@gmail.com>

---

Note seeing there is no user of the pattern described above we can convert
it to having just weakly defined methods. Let me know if that would be a
better alternative.
---
 arch/mips/include/asm/mips-cm.h | 14 ++++++++++++++
 arch/mips/kernel/mips-cm.c      |  2 +-
 2 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/arch/mips/include/asm/mips-cm.h b/arch/mips/include/asm/mips-cm.h
index 23c67c0871b1..1f143dfad7a2 100644
--- a/arch/mips/include/asm/mips-cm.h
+++ b/arch/mips/include/asm/mips-cm.h
@@ -33,6 +33,20 @@ extern void __iomem *mips_cm_l2sync_base;
  */
 extern phys_addr_t __mips_cm_phys_base(void);
 
+/**
+ * __mips_cm_l2sync_phys_base - retrieve the physical base address of the CM
+ *                              L2-sync region
+ *
+ * This function returns the physical base address of the Coherence Manager
+ * L2-cache only region. It provides a default implementation which reads the
+ * CMGCRL2OnlySyncBase register where available or returns a 4K region just
+ * behind the CM GCR base address. It may be overridden by platforms which
+ * determine this address in a different way by defining a function with the
+ * same prototype except for the name mips_cm_l2sync_phys_base (without
+ * underscores).
+ */
+extern phys_addr_t __mips_cm_l2sync_phys_base(void);
+
 /*
  * mips_cm_is64 - determine CM register width
  *
diff --git a/arch/mips/kernel/mips-cm.c b/arch/mips/kernel/mips-cm.c
index 84b3affb9de8..3f00788b0871 100644
--- a/arch/mips/kernel/mips-cm.c
+++ b/arch/mips/kernel/mips-cm.c
@@ -201,7 +201,7 @@ phys_addr_t __mips_cm_phys_base(void)
 phys_addr_t mips_cm_phys_base(void)
 	__attribute__((weak, alias("__mips_cm_phys_base")));
 
-static phys_addr_t __mips_cm_l2sync_phys_base(void)
+phys_addr_t __mips_cm_l2sync_phys_base(void)
 {
 	u32 base_reg;
 
-- 
2.43.0

Re: [PATCH 1/4] mips: cm: Add __mips_cm_l2sync_phys_base prototype declaration

[Andy Shevchenko]

Thu, Feb 15, 2024 at 08:17:26PM +0300, Serge Semin kirjoitti:
> The __mips_cm_l2sync_phys_base() and mips_cm_l2sync_phys_base() couple was
> introduced in commit 9f98f3dd0c51 ("MIPS: Add generic CM probe & access
> code") where the former method was a weak implementation of the later
> function. Such design pattern permitted to re-define the original method
> and use the weak implementation in the new function. A similar approach
> was introduced in the framework of another arch-specific programmable
> interface: mips_cm_phys_base() and __mips_cm_phys_base(). The only
> difference is that the underscored method of the later couple was declared
> in the "asm/mips-cm.h" header file, but it wasn't done for the CM L2-sync
> methods in the subject. Due to the missing the global function declaration
> the "missing prototype" warning was spotted in the framework of the commit
> 9a2036724cd6 ("mips: mark local function static if possible") and fixed
> just be re-qualifying the weak method as static. Doing that broke what was
> originally implied by having the weak implementation globally defined. Fix
> that by dropping the static qualifier and adding the
> __mips_cm_l2sync_phys_base() prototype declared in the "asm/mips-cm.h"
> header file.

...

> +/**
> + * __mips_cm_l2sync_phys_base - retrieve the physical base address of the CM
> + *                              L2-sync region
> + *
> + * This function returns the physical base address of the Coherence Manager
> + * L2-cache only region. It provides a default implementation which reads the
> + * CMGCRL2OnlySyncBase register where available or returns a 4K region just
> + * behind the CM GCR base address. It may be overridden by platforms which
> + * determine this address in a different way by defining a function with the
> + * same prototype except for the name mips_cm_l2sync_phys_base (without
> + * underscores).
> + */
> +extern phys_addr_t __mips_cm_l2sync_phys_base(void);

I'm wondering if you run

	scripts/kernel-doc -v -none -Wall ...

against this file. I believe it will complain that you missed Return section.

-- 
With Best Regards,
Andy Shevchenko

[PATCH 2/4] mips: cm: Add CM GCR and L2-sync base address getters declarations

[Serge Semin]

Based on the design pattern utilized in the CM GCR and L2-sync base
address getters implementation the platform-specific code is capable to
re-define the getters and re-use the weakly defined initial versions. But
since the re-definition is supposed to be done in another source file the
interface methods have been globally defined which in its turn causes the
"no previous prototype" warning printed should the re-definition is
finally introduced. Since without the global declarations the pattern can
be considered as incomplete and causing the warning printed, fix it by
providing the respective methods prototype declarations in
"arch/mips/include/asm/mips-cm.h".

Signed-off-by: Serge Semin <fancer.lancer@gmail.com>

---

Note as I mentioned in the previous patch, since the weak implementation
of the getters isn't utilized other than as a default implementation of
the original methods, we can convert the denoted pattern to a simple
__weak attributed methods. Let me know if that would be more preferable.
---
 arch/mips/include/asm/mips-cm.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/mips/include/asm/mips-cm.h b/arch/mips/include/asm/mips-cm.h
index 1f143dfad7a2..6dbe74dc323d 100644
--- a/arch/mips/include/asm/mips-cm.h
+++ b/arch/mips/include/asm/mips-cm.h
@@ -32,6 +32,7 @@ extern void __iomem *mips_cm_l2sync_base;
  * name mips_cm_phys_base (without underscores).
  */
 extern phys_addr_t __mips_cm_phys_base(void);
+extern phys_addr_t mips_cm_phys_base(void);
 
 /**
  * __mips_cm_l2sync_phys_base - retrieve the physical base address of the CM
@@ -46,6 +47,7 @@ extern phys_addr_t __mips_cm_phys_base(void);
  * underscores).
  */
 extern phys_addr_t __mips_cm_l2sync_phys_base(void);
+extern phys_addr_t mips_cm_l2sync_phys_base(void);
 
 /*
  * mips_cm_is64 - determine CM register width
-- 
2.43.0

Re: [PATCH 2/4] mips: cm: Add CM GCR and L2-sync base address getters declarations

[Thomas Bogendoerfer]

On Thu, Feb 15, 2024 at 08:17:27PM +0300, Serge Semin wrote:
> Based on the design pattern utilized in the CM GCR and L2-sync base
> address getters implementation the platform-specific code is capable to
> re-define the getters and re-use the weakly defined initial versions. But
> since the re-definition is supposed to be done in another source file the
> interface methods have been globally defined which in its turn causes the
> "no previous prototype" warning printed should the re-definition is
> finally introduced. Since without the global declarations the pattern can
> be considered as incomplete and causing the warning printed, fix it by
> providing the respective methods prototype declarations in
> "arch/mips/include/asm/mips-cm.h".
> 
> Signed-off-by: Serge Semin <fancer.lancer@gmail.com>
> 
> ---
> 
> Note as I mentioned in the previous patch, since the weak implementation
> of the getters isn't utilized other than as a default implementation of
> the original methods, we can convert the denoted pattern to a simple
> __weak attributed methods. Let me know if that would be more preferable.

how about simply remove __mips_cm_l2sync_phys_base() and do everything
via mips_cm_phys_base(). And at the moment without anyone overriding
mips_cm_phys_base I tend to keep static without __weak. If someone
needs, we can change it. Does this make sense ?

Thomas.

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

Re: [PATCH 2/4] mips: cm: Add CM GCR and L2-sync base address getters declarations

[Serge Semin]

On Tue, Feb 20, 2024 at 06:24:25PM +0100, Thomas Bogendoerfer wrote:
> On Thu, Feb 15, 2024 at 08:17:27PM +0300, Serge Semin wrote:
> > Based on the design pattern utilized in the CM GCR and L2-sync base
> > address getters implementation the platform-specific code is capable to
> > re-define the getters and re-use the weakly defined initial versions. But
> > since the re-definition is supposed to be done in another source file the
> > interface methods have been globally defined which in its turn causes the
> > "no previous prototype" warning printed should the re-definition is
> > finally introduced. Since without the global declarations the pattern can
> > be considered as incomplete and causing the warning printed, fix it by
> > providing the respective methods prototype declarations in
> > "arch/mips/include/asm/mips-cm.h".
> > 
> > Signed-off-by: Serge Semin <fancer.lancer@gmail.com>
> > 
> > ---
> > 
> > Note as I mentioned in the previous patch, since the weak implementation
> > of the getters isn't utilized other than as a default implementation of
> > the original methods, we can convert the denoted pattern to a simple
> > __weak attributed methods. Let me know if that would be more preferable.
> 

> how about simply remove __mips_cm_l2sync_phys_base() and do everything
> via mips_cm_phys_base(). And at the moment without anyone overriding
> mips_cm_phys_base I tend to keep static without __weak. If someone
> needs, we can change it. Does this make sense ?

To be honest my arch code (not submitted yet) do override the
mips_cm_l2sync_phys_base() method. The memory just behind the CM2
region is hardwired for the EEPROM mapping. So the default
__mips_cm_l2sync_phys_base() implementation selecting the L2-sync with
(mips_cm_phys_base() + MIPS_CM_GCR_SIZE) isn't suitable for my
platform.

Note what you suggest will require the CM and CM L2-sync probe code
logic redevelopment. The mips_cm_phys_base() method is currently
dedicated for a single purpose: return the CM GCR physical base address.
So is the mips_cm_l2sync_phys_base() method. Merging the later method
into the former one will cause the mips_cm_phys_base() function
looking less neat. We will also require to have the mips_cm_probe()
method keeping the CM L2-sync physical base addresses around and then
passing it to mips_cm_probe_l2sync() or having the later method
embedded into mips_cm_probe(). All of that won't look as good as the
current implementation.

What about instead of that I'll just convert both mips_cm_phys_base()
and mips_cm_l2sync_phys_base() to being defined with the underscored
methods body and assign the __weak attribute to them?

-Serge(y)

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

Re: [PATCH 2/4] mips: cm: Add CM GCR and L2-sync base address getters declarations

[Thomas Bogendoerfer]

On Wed, Feb 21, 2024 at 09:39:58PM +0300, Serge Semin wrote:
> On Tue, Feb 20, 2024 at 06:24:25PM +0100, Thomas Bogendoerfer wrote:
> > On Thu, Feb 15, 2024 at 08:17:27PM +0300, Serge Semin wrote:
> > > Based on the design pattern utilized in the CM GCR and L2-sync base
> > > address getters implementation the platform-specific code is capable to
> > > re-define the getters and re-use the weakly defined initial versions. But
> > > since the re-definition is supposed to be done in another source file the
> > > interface methods have been globally defined which in its turn causes the
> > > "no previous prototype" warning printed should the re-definition is
> > > finally introduced. Since without the global declarations the pattern can
> > > be considered as incomplete and causing the warning printed, fix it by
> > > providing the respective methods prototype declarations in
> > > "arch/mips/include/asm/mips-cm.h".
> > > 
> > > Signed-off-by: Serge Semin <fancer.lancer@gmail.com>
> > > 
> > > ---
> > > 
> > > Note as I mentioned in the previous patch, since the weak implementation
> > > of the getters isn't utilized other than as a default implementation of
> > > the original methods, we can convert the denoted pattern to a simple
> > > __weak attributed methods. Let me know if that would be more preferable.
> > 
> 
> > how about simply remove __mips_cm_l2sync_phys_base() and do everything
> > via mips_cm_phys_base(). And at the moment without anyone overriding
> > mips_cm_phys_base I tend to keep static without __weak. If someone
> > needs, we can change it. Does this make sense ?
> 
> To be honest my arch code (not submitted yet) do override the
> mips_cm_l2sync_phys_base() method. The memory just behind the CM2

that's fine, I just wanted to know a reason for having it provided as
weak symbol.

> What about instead of that I'll just convert both mips_cm_phys_base()
> and mips_cm_l2sync_phys_base() to being defined with the underscored
> methods body and assign the __weak attribute to them?

works for me ;-) I'll pick patch 3/4 of this series, so no need to
resend them.

Thomas.

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

Re: [PATCH 2/4] mips: cm: Add CM GCR and L2-sync base address getters declarations

[Serge Semin]

On Fri, Feb 23, 2024 at 10:06:33AM +0100, Thomas Bogendoerfer wrote:
> On Wed, Feb 21, 2024 at 09:39:58PM +0300, Serge Semin wrote:
> > On Tue, Feb 20, 2024 at 06:24:25PM +0100, Thomas Bogendoerfer wrote:
> > > On Thu, Feb 15, 2024 at 08:17:27PM +0300, Serge Semin wrote:
> > > > Based on the design pattern utilized in the CM GCR and L2-sync base
> > > > address getters implementation the platform-specific code is capable to
> > > > re-define the getters and re-use the weakly defined initial versions. But
> > > > since the re-definition is supposed to be done in another source file the
> > > > interface methods have been globally defined which in its turn causes the
> > > > "no previous prototype" warning printed should the re-definition is
> > > > finally introduced. Since without the global declarations the pattern can
> > > > be considered as incomplete and causing the warning printed, fix it by
> > > > providing the respective methods prototype declarations in
> > > > "arch/mips/include/asm/mips-cm.h".
> > > > 
> > > > Signed-off-by: Serge Semin <fancer.lancer@gmail.com>
> > > > 
> > > > ---
> > > > 
> > > > Note as I mentioned in the previous patch, since the weak implementation
> > > > of the getters isn't utilized other than as a default implementation of
> > > > the original methods, we can convert the denoted pattern to a simple
> > > > __weak attributed methods. Let me know if that would be more preferable.
> > > 
> > 
> > > how about simply remove __mips_cm_l2sync_phys_base() and do everything
> > > via mips_cm_phys_base(). And at the moment without anyone overriding
> > > mips_cm_phys_base I tend to keep static without __weak. If someone
> > > needs, we can change it. Does this make sense ?
> > 
> > To be honest my arch code (not submitted yet) do override the
> > mips_cm_l2sync_phys_base() method. The memory just behind the CM2
> 
> that's fine, I just wanted to know a reason for having it provided as
> weak symbol.
> 
> > What about instead of that I'll just convert both mips_cm_phys_base()
> > and mips_cm_l2sync_phys_base() to being defined with the underscored
> > methods body and assign the __weak attribute to them?
> 
> works for me ;-) I'll pick patch 3/4 of this series, so no need to
> resend them.

Ok. Thanks. I'll submit the respective patch(es) in a several days.

-Serge(y)

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

[PATCH 3/4] mips: zboot: Fix "no previous prototype" build warning

[Serge Semin]

Without having the putc() prototype forwardly declared the next
build-warning is printed:

arch/mips/boot/compressed/uart-prom.c:6:6: warning: no previous prototype for ‘putc’ [-Wmissing-prototypes]
...

Fix that by adding the "decompress.h" header file with the putc() function
declaration to the source files which have the method redefined.

Signed-off-by: Serge Semin <fancer.lancer@gmail.com>
---
 arch/mips/boot/compressed/uart-16550.c   | 2 ++
 arch/mips/boot/compressed/uart-alchemy.c | 2 ++
 arch/mips/boot/compressed/uart-prom.c    | 2 ++
 3 files changed, 6 insertions(+)

diff --git a/arch/mips/boot/compressed/uart-16550.c b/arch/mips/boot/compressed/uart-16550.c
index 09dcd2c561d9..db618e72a0c4 100644
--- a/arch/mips/boot/compressed/uart-16550.c
+++ b/arch/mips/boot/compressed/uart-16550.c
@@ -8,6 +8,8 @@
 
 #include <asm/addrspace.h>
 
+#include "decompress.h"
+
 #if defined(CONFIG_MACH_LOONGSON64) || defined(CONFIG_MIPS_MALTA)
 #define UART_BASE 0x1fd003f8
 #define PORT(offset) (CKSEG1ADDR(UART_BASE) + (offset))
diff --git a/arch/mips/boot/compressed/uart-alchemy.c b/arch/mips/boot/compressed/uart-alchemy.c
index 8ec63011e7dc..003967c084b3 100644
--- a/arch/mips/boot/compressed/uart-alchemy.c
+++ b/arch/mips/boot/compressed/uart-alchemy.c
@@ -1,6 +1,8 @@
 // SPDX-License-Identifier: GPL-2.0
 #include <asm/mach-au1x00/au1000.h>
 
+#include "decompress.h"
+
 void putc(char c)
 {
 	alchemy_uart_putchar(AU1000_UART0_PHYS_ADDR, c);
diff --git a/arch/mips/boot/compressed/uart-prom.c b/arch/mips/boot/compressed/uart-prom.c
index a8a0a32e05d1..5fa3b9945333 100644
--- a/arch/mips/boot/compressed/uart-prom.c
+++ b/arch/mips/boot/compressed/uart-prom.c
@@ -1,6 +1,8 @@
 // SPDX-License-Identifier: GPL-2.0
 #include <asm/setup.h>
 
+#include "decompress.h"
+
 void putc(char c)
 {
 	prom_putchar(c);
-- 
2.43.0

Re: [PATCH 3/4] mips: zboot: Fix "no previous prototype" build warning

[Thomas Bogendoerfer]

On Thu, Feb 15, 2024 at 08:17:28PM +0300, Serge Semin wrote:
> Without having the putc() prototype forwardly declared the next
> build-warning is printed:
> 
> arch/mips/boot/compressed/uart-prom.c:6:6: warning: no previous prototype for ‘putc’ [-Wmissing-prototypes]
> ...
> 
> Fix that by adding the "decompress.h" header file with the putc() function
> declaration to the source files which have the method redefined.
> 
> Signed-off-by: Serge Semin <fancer.lancer@gmail.com>
> ---
>  arch/mips/boot/compressed/uart-16550.c   | 2 ++
>  arch/mips/boot/compressed/uart-alchemy.c | 2 ++
>  arch/mips/boot/compressed/uart-prom.c    | 2 ++
>  3 files changed, 6 insertions(+)

applied to mips-next.

Thomas.

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

[PATCH 4/4] tty: mips_ejtag_fdc: Fix passing incompatible pointer type warning

[Serge Semin]

mips_ejtag_fdc_encode() method expects having a first argument passed of
the "u8 **" type, meanwhile the driver passes the "const char **" type.
That causes the next build-warning:

drivers/tty/mips_ejtag_fdc.c: In function ‘mips_ejtag_fdc_console_write’:
drivers/tty/mips_ejtag_fdc.c:343:32: error: passing argument 1 of ‘mips_ejtag_fdc_encode’ from incompatible pointer type [-Werror=incompatible-pointer-types]
   word = mips_ejtag_fdc_encode(&buf_ptr, &buf_len, 1);
                                ^
drivers/tty/mips_ejtag_fdc.c:216:24: note: expected ‘const u8 ** {aka const unsigned char **}’ but argument is of type ‘const char **’
 static struct fdc_word mips_ejtag_fdc_encode(const u8 **ptrs,
                        ^~~~~~~~~~~~~~~~~~~~~

Fix it by altering the type of the pointer which is passed to the
mips_ejtag_fdc_encode() method.

Fixes: ce7cbd9a6c81 ("tty: mips_ejtag_fdc: use u8 for character pointers")
Signed-off-by: Serge Semin <fancer.lancer@gmail.com>
---
 drivers/tty/mips_ejtag_fdc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/tty/mips_ejtag_fdc.c b/drivers/tty/mips_ejtag_fdc.c
index aac80b69a069..afbf7738c7c4 100644
--- a/drivers/tty/mips_ejtag_fdc.c
+++ b/drivers/tty/mips_ejtag_fdc.c
@@ -309,7 +309,7 @@ static void mips_ejtag_fdc_console_write(struct console *c, const char *s,
 	unsigned int i, buf_len, cpu;
 	bool done_cr = false;
 	char buf[4];
-	const char *buf_ptr = buf;
+	const u8 *buf_ptr = buf;
 	/* Number of bytes of input data encoded up to each byte in buf */
 	u8 inc[4];
 
-- 
2.43.0

Re: [PATCH 4/4] tty: mips_ejtag_fdc: Fix passing incompatible pointer type warning

[Jiri Slaby]

On 15. 02. 24, 18:17, Serge Semin wrote:
> mips_ejtag_fdc_encode() method expects having a first argument passed of
> the "u8 **" type, meanwhile the driver passes the "const char **" type.
> That causes the next build-warning:
> 
> drivers/tty/mips_ejtag_fdc.c: In function ‘mips_ejtag_fdc_console_write’:
> drivers/tty/mips_ejtag_fdc.c:343:32: error: passing argument 1 of ‘mips_ejtag_fdc_encode’ from incompatible pointer type [-Werror=incompatible-pointer-types]
>     word = mips_ejtag_fdc_encode(&buf_ptr, &buf_len, 1);
>                                  ^
> drivers/tty/mips_ejtag_fdc.c:216:24: note: expected ‘const u8 ** {aka const unsigned char **}’ but argument is of type ‘const char **’
>   static struct fdc_word mips_ejtag_fdc_encode(const u8 **ptrs,
>                          ^~~~~~~~~~~~~~~~~~~~~

It's a correct change. But why the compiler complains, given
KBUILD_CFLAGS += -funsigned-char
?

> Fix it by altering the type of the pointer which is passed to the
> mips_ejtag_fdc_encode() method.
> 
> Fixes: ce7cbd9a6c81 ("tty: mips_ejtag_fdc: use u8 for character pointers")
> Signed-off-by: Serge Semin <fancer.lancer@gmail.com>
> ---
>   drivers/tty/mips_ejtag_fdc.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/tty/mips_ejtag_fdc.c b/drivers/tty/mips_ejtag_fdc.c
> index aac80b69a069..afbf7738c7c4 100644
> --- a/drivers/tty/mips_ejtag_fdc.c
> +++ b/drivers/tty/mips_ejtag_fdc.c
> @@ -309,7 +309,7 @@ static void mips_ejtag_fdc_console_write(struct console *c, const char *s,
>   	unsigned int i, buf_len, cpu;
>   	bool done_cr = false;
>   	char buf[4];
> -	const char *buf_ptr = buf;
> +	const u8 *buf_ptr = buf;
>   	/* Number of bytes of input data encoded up to each byte in buf */
>   	u8 inc[4];
>   

thanks,
-- 
js
suse labs

Re: [PATCH 4/4] tty: mips_ejtag_fdc: Fix passing incompatible pointer type warning

[Serge Semin]

On Fri, Feb 16, 2024 at 06:51:01AM +0100, Jiri Slaby wrote:
> On 15. 02. 24, 18:17, Serge Semin wrote:
> > mips_ejtag_fdc_encode() method expects having a first argument passed of
> > the "u8 **" type, meanwhile the driver passes the "const char **" type.
> > That causes the next build-warning:
> > 
> > drivers/tty/mips_ejtag_fdc.c: In function ‘mips_ejtag_fdc_console_write’:
> > drivers/tty/mips_ejtag_fdc.c:343:32: error: passing argument 1 of ‘mips_ejtag_fdc_encode’ from incompatible pointer type [-Werror=incompatible-pointer-types]
> >     word = mips_ejtag_fdc_encode(&buf_ptr, &buf_len, 1);
> >                                  ^
> > drivers/tty/mips_ejtag_fdc.c:216:24: note: expected ‘const u8 ** {aka const unsigned char **}’ but argument is of type ‘const char **’
> >   static struct fdc_word mips_ejtag_fdc_encode(const u8 **ptrs,
> >                          ^~~~~~~~~~~~~~~~~~~~~
> 
> It's a correct change. But why the compiler complains, given
> KBUILD_CFLAGS += -funsigned-char
> ?

What a tricky question you asked.) It cost me some new gray hairs, but
I guess I figured the correct answer out.

First of all it turns out that "char", "signed char" and "unsigned
char" are three _distant_ types. The "-funsigned-char/-fsigned-char"
flag changes the signedness of the plain "char", but it doesn't make
it matching to any of "signed char" or "unsigned char". Thus the flag
you mentioned couldn't suppress the warning I discovered (a bit later
you'll see that it is unrelated to that flag, but to the fact that the
types are different). Here is a simple test-code illustrating what I
said above:

1: int main(int argc, char *argv[], char *env[])
2: {
3:         char c;
4:         char *cp = &c;
5:         signed char sc;
6:         signed char *scp1 = &c;
7:         signed char *scp2 = ≻
8:         unsigned char uc;
9:         unsigned char *ucp1 = &c;
10:        unsigned char *ucp2 = &uc;
11:        return 0;
12: }

$ gcc -Wall -Wno-unused-variable -funsigned-char tmp_char.c -o tmp_char
tmp_char.c: In function ‘main’:
tmp_char.c:6:29: warning: pointer targets in initialization of ‘signed char *’ from ‘char *’ differ in signedness [-Wpointer-sign]
    6 |         signed char *scp1 = &c;
      |                             ^
tmp_char.c:9:31: warning: pointer targets in initialization of ‘unsigned char *’ from ‘char *’ differ in signedness [-Wpointer-sign]
    9 |         unsigned char *ucp1 = &c;
      |                               ^
$

See, a new warning (not as the one in the patch log) is printed
despite of having the "-funsigned-char" flag specified. A more
detailed discussion around that you can find here:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=28912
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=23087
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=71202

The next question would be: "Why don't we see such warning printed all
over the kernel then?" That's because the "-Wno-pointer-sign" warning
suppressor is enabled in the kernel. It shuts up the sign-mismatching
pointers cast warnings. So no warning will be printed if the test-code
above is compiled as:

$ gcc -Wall -Wno-unused-variable -funsigned-char -Wno-pointer-sign tmp_char.c -o tmp_char
$

But why do we still see a warning as mentioned in the patch log? It
turns out that the "-Wno-pointer-sign" flag only works for the
_simple_ pointer signedness mismatch, but not for the multi-level
pointers. So as long as there is a pointer-to-pointer or
pointer-to-pointer-to-pointer, etc involved, another warning will be
printed. Here is the test-code modified to re-produce the compile
warning cited in the patch log:

1: int main(int argc, char *argv[], char *env[])
2: {
3:         char c;
4:         char *cp = &c;
5:         signed char sc;
6:         signed char *scp1 = &c;
7:         signed char *scp2 = ≻
8:         signed char **scpp = &cp;
9:         unsigned char uc;
10:        unsigned char *ucp1 = &c;
11:        unsigned char *ucp2 = &uc;
12:        unsigned char **ucpp = &cp;
13:        return 0;
14: }

$ gcc -Wall -Wno-unused-variable -funsigned-char -Wno-pointer-sign tmp_char.c -o tmp_char
tmp_char.c: In function ‘main’:
tmp_char.c:8:30: warning: initialization of ‘signed char **’ from incompatible pointer type ‘char **’ [-Wincompatible-pointer-types]
    8 |         signed char **scpp = &cp;
      |                              ^
tmp_char.c:12:32: warning: initialization of ‘unsigned char **’ from incompatible pointer type ‘char **’ [-Wincompatible-pointer-types]
   12 |         unsigned char **ucpp = &cp;
      |                                ^
$

See, the lines 6 and 10 don't cause any warning printed (due to the
"-Wno-pointer-sign" flag), but the new lines 8 and 12 do. This is the
case that simulates what was discovered in the
drivers/tty/mips_ejtag_fdc.c driver and what was fixed in this patch.
I don't know for sure but I guess the compiler considers the
high-level pointers a bit differently than the single-level ones. So
the pointers to different pointer types are considered as
incompatible, which is also relevant for the char-family types since
these are three distant types. Thus that's what the warning about.

> 
> > Fix it by altering the type of the pointer which is passed to the
> > mips_ejtag_fdc_encode() method.
> > 
> > Fixes: ce7cbd9a6c81 ("tty: mips_ejtag_fdc: use u8 for character pointers")
> > Signed-off-by: Serge Semin <fancer.lancer@gmail.com>
> > ---
> >   drivers/tty/mips_ejtag_fdc.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/tty/mips_ejtag_fdc.c b/drivers/tty/mips_ejtag_fdc.c
> > index aac80b69a069..afbf7738c7c4 100644
> > --- a/drivers/tty/mips_ejtag_fdc.c
> > +++ b/drivers/tty/mips_ejtag_fdc.c
> > @@ -309,7 +309,7 @@ static void mips_ejtag_fdc_console_write(struct console *c, const char *s,
> >   	unsigned int i, buf_len, cpu;
> >   	bool done_cr = false;
> >   	char buf[4];
> > -	const char *buf_ptr = buf;
> > +	const u8 *buf_ptr = buf;
> >   	/* Number of bytes of input data encoded up to each byte in buf */
> >   	u8 inc[4];
> 
> thanks,

Please explicitly add your tag if you are ok with the fix.

-Serge(y)

> -- 
> js
> suse labs
> 

Re: [PATCH 4/4] tty: mips_ejtag_fdc: Fix passing incompatible pointer type warning

[Thomas Bogendoerfer]

On Thu, Feb 15, 2024 at 08:17:29PM +0300, Serge Semin wrote:
> mips_ejtag_fdc_encode() method expects having a first argument passed of
> the "u8 **" type, meanwhile the driver passes the "const char **" type.
> That causes the next build-warning:
> 
> drivers/tty/mips_ejtag_fdc.c: In function ‘mips_ejtag_fdc_console_write’:
> drivers/tty/mips_ejtag_fdc.c:343:32: error: passing argument 1 of ‘mips_ejtag_fdc_encode’ from incompatible pointer type [-Werror=incompatible-pointer-types]
>    word = mips_ejtag_fdc_encode(&buf_ptr, &buf_len, 1);
>                                 ^
> drivers/tty/mips_ejtag_fdc.c:216:24: note: expected ‘const u8 ** {aka const unsigned char **}’ but argument is of type ‘const char **’
>  static struct fdc_word mips_ejtag_fdc_encode(const u8 **ptrs,
>                         ^~~~~~~~~~~~~~~~~~~~~
> 
> Fix it by altering the type of the pointer which is passed to the
> mips_ejtag_fdc_encode() method.
> 
> Fixes: ce7cbd9a6c81 ("tty: mips_ejtag_fdc: use u8 for character pointers")
> Signed-off-by: Serge Semin <fancer.lancer@gmail.com>
> ---
>  drivers/tty/mips_ejtag_fdc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/tty/mips_ejtag_fdc.c b/drivers/tty/mips_ejtag_fdc.c
> index aac80b69a069..afbf7738c7c4 100644
> --- a/drivers/tty/mips_ejtag_fdc.c
> +++ b/drivers/tty/mips_ejtag_fdc.c
> @@ -309,7 +309,7 @@ static void mips_ejtag_fdc_console_write(struct console *c, const char *s,
>  	unsigned int i, buf_len, cpu;
>  	bool done_cr = false;
>  	char buf[4];
> -	const char *buf_ptr = buf;
> +	const u8 *buf_ptr = buf;
>  	/* Number of bytes of input data encoded up to each byte in buf */
>  	u8 inc[4];

applied to mips-next.

Thomas.

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