[PATCH] powerpc: fix the allyesconfig build

[PATCH] powerpc: fix the allyesconfig build

[Stephen Rothwell]

[-- Attachment #1: Type: text/plain, Size: 2571 bytes --]

There are 2 drivers that have arrays of packed structures that contain
pointers that end up at unaligned offsets.  These produce warnings in
the PowerPC allyesconfig build like this:

WARNING: 148 bad relocations
c00000000e56510b R_PPC64_UADDR64   .rodata+0x0000000001c72378
c00000000e565126 R_PPC64_UADDR64   .rodata+0x0000000001c723c0

They are not drivers that are used on PowerPC (I assume), so mark them
to not be built on PPC64 when CONFIG_RELOCATABLE is enabled.

Cc: Geert Uytterhoeven <geert+renesas@glider.be>
Cc: Michael Turquette <mturquette@baylibre.com>
Cc: Stephen Boyd <sboyd@kernel.org>
Cc: Yisen Zhuang <yisen.zhuang@huawei.com>
Cc: Salil Mehta <salil.mehta@huawei.com>
Cc: David S. Miller <davem@davemloft.net>
Cc: Jakub Kicinski <kuba@kernel.org>
Cc: Nicholas Piggin  <npiggin@gmail.com>
Cc: Daniel Axtens <dja@axtens.net>
Cc: Joel Stanley <joel@jms.id.au>
Signed-off-by: Stephen Rothwell <sfr@canb.auug.org.au>
---
 drivers/clk/renesas/Kconfig            | 4 ++++
 drivers/net/ethernet/hisilicon/Kconfig | 4 ++++
 2 files changed, 8 insertions(+)

It might be easiest to put this through the PowerPC (fixes?) tree, if
people woulf just sned ACKs, please?

diff --git a/drivers/clk/renesas/Kconfig b/drivers/clk/renesas/Kconfig
index 18915d668a30..53f24a0cad7a 100644
--- a/drivers/clk/renesas/Kconfig
+++ b/drivers/clk/renesas/Kconfig
@@ -151,6 +151,10 @@ config CLK_R8A779A0
 	select CLK_RENESAS_CPG_MSSR
 
 config CLK_R9A06G032
+	# PPC64 RELOCATABLE kernels cannot handle relocations to
+	# unaligned locations that are produced by the array of
+	# packed structures in this driver.
+	depends on !(PPC64 && RELOCATABLE)
 	bool "Renesas R9A06G032 clock driver"
 	help
 	  This is a driver for R9A06G032 clocks
diff --git a/drivers/net/ethernet/hisilicon/Kconfig b/drivers/net/ethernet/hisilicon/Kconfig
index 44f9279cdde1..bf47e504b365 100644
--- a/drivers/net/ethernet/hisilicon/Kconfig
+++ b/drivers/net/ethernet/hisilicon/Kconfig
@@ -102,6 +102,10 @@ config HNS3_HCLGE
 	tristate "Hisilicon HNS3 HCLGE Acceleration Engine & Compatibility Layer Support"
 	default m
 	depends on PCI_MSI
+	# PPC64 RELOCATABLE kernels cannot handle relocations to
+	# unaligned locations that are produced by the array of
+	# packed structures in this driver.
+	depends on !(PPC64 && RELOCATABLE)
 	help
 	  This selects the HNS3_HCLGE network acceleration engine & its hardware
 	  compatibility layer. The engine would be used in Hisilicon hip08 family of
-- 
2.29.2

-- 
Cheers,
Stephen Rothwell

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] powerpc: fix the allyesconfig build

[Jakub Kicinski]

On Sat, 28 Nov 2020 12:28:19 +1100 Stephen Rothwell wrote:
> There are 2 drivers that have arrays of packed structures that contain
> pointers that end up at unaligned offsets.  These produce warnings in
> the PowerPC allyesconfig build like this:
> 
> WARNING: 148 bad relocations
> c00000000e56510b R_PPC64_UADDR64   .rodata+0x0000000001c72378
> c00000000e565126 R_PPC64_UADDR64   .rodata+0x0000000001c723c0
> 
> They are not drivers that are used on PowerPC (I assume), so mark them
> to not be built on PPC64 when CONFIG_RELOCATABLE is enabled.

😳😳

What's the offending structure in hisilicon? I'd rather have a look
packing structs with pointers in 'em sounds questionable.

I only see these two:

$ git grep packed drivers/net/ethernet/hisilicon/
drivers/net/ethernet/hisilicon/hns/hnae.h:struct __packed hnae_desc {
drivers/net/ethernet/hisilicon/hns3/hns3_enet.h:struct __packed hns3_desc {

Re: [PATCH] powerpc: fix the allyesconfig build

[Yunsheng Lin]

On 2020/11/28 9:56, Jakub Kicinski wrote:
> On Sat, 28 Nov 2020 12:28:19 +1100 Stephen Rothwell wrote:
>> There are 2 drivers that have arrays of packed structures that contain
>> pointers that end up at unaligned offsets.  These produce warnings in
>> the PowerPC allyesconfig build like this:
>>
>> WARNING: 148 bad relocations
>> c00000000e56510b R_PPC64_UADDR64   .rodata+0x0000000001c72378
>> c00000000e565126 R_PPC64_UADDR64   .rodata+0x0000000001c723c0
>>
>> They are not drivers that are used on PowerPC (I assume), so mark them
>> to not be built on PPC64 when CONFIG_RELOCATABLE is enabled.
> 
> 😳😳
> 
> What's the offending structure in hisilicon? I'd rather have a look
> packing structs with pointers in 'em sounds questionable.
> 
> I only see these two:
> 
> $ git grep packed drivers/net/ethernet/hisilicon/
> drivers/net/ethernet/hisilicon/hns/hnae.h:struct __packed hnae_desc {
> drivers/net/ethernet/hisilicon/hns3/hns3_enet.h:struct __packed hns3_desc {

I assmue "struct __packed hnae_desc" is the offending structure, because
flag_ipoffset field is defined as __le32 and is not 32 bit aligned.

struct __packed hnae_desc {
	__le64 addr;							//0
	union {
		struct {						//64
			union {
				__le16 asid_bufnum_pid;
				__le16 asid;
			};
			__le16 send_size;				//92
			union {
				__le32 flag_ipoffset;			//*108*
				struct {
					__u8 bn_pid;
					__u8 ra_ri_cs_fe_vld;
					__u8 ip_offset;
					__u8 tse_vlan_snap_v6_sctp_nth;
				};
			};
			__le16 mss;
			__u8 l4_len;
			__u8 reserved1;
			__le16 paylen;
			__u8 vmid;
			__u8 qid;
			__le32 reserved2[2];
		} tx;

		struct {
			__le32 ipoff_bnum_pid_flag;
			__le16 pkt_len;
			__le16 size;
			union {
				__le32 vlan_pri_asid;
				struct {
					__le16 asid;
					__le16 vlan_cfi_pri;
				};
			};
			__le32 rss_hash;
			__le32 reserved_1[2];
		} rx;
	};
};

> .
> 

Re: [PATCH] powerpc: fix the allyesconfig build

[Stephen Rothwell]

[-- Attachment #1: Type: text/plain, Size: 836 bytes --]

Hi Jakub,

On Fri, 27 Nov 2020 17:56:42 -0800 Jakub Kicinski <kuba@kernel.org> wrote:
>
> What's the offending structure in hisilicon? I'd rather have a look
> packing structs with pointers in 'em sounds questionable.
> 
> I only see these two:
> 
> $ git grep packed drivers/net/ethernet/hisilicon/
> drivers/net/ethernet/hisilicon/hns/hnae.h:struct __packed hnae_desc {
> drivers/net/ethernet/hisilicon/hns3/hns3_enet.h:struct __packed hns3_desc {

struct hclge_dbg_reg_type_info which is 28 bytes long due to the
included struct struct hclge_dbg_reg_common_msg (which is 12 bytes
long).  They are surrounded by #pragma pack(1)/pack().

This forces the 2 pointers in each second array element of
hclge_dbg_reg_info[] to be 4 byte aligned (where pointers are 8 bytes
long on PPC64).
-- 
Cheers,
Stephen Rothwell

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 484 bytes --]

Re: [PATCH] powerpc: fix the allyesconfig build

[Jakub Kicinski]

On Sat, 28 Nov 2020 16:20:54 +1100 Stephen Rothwell wrote:
> On Fri, 27 Nov 2020 17:56:42 -0800 Jakub Kicinski <kuba@kernel.org> wrote:
> >
> > What's the offending structure in hisilicon? I'd rather have a look
> > packing structs with pointers in 'em sounds questionable.
> > 
> > I only see these two:
> > 
> > $ git grep packed drivers/net/ethernet/hisilicon/
> > drivers/net/ethernet/hisilicon/hns/hnae.h:struct __packed hnae_desc {
> > drivers/net/ethernet/hisilicon/hns3/hns3_enet.h:struct __packed hns3_desc {  
> 
> struct hclge_dbg_reg_type_info which is 28 bytes long due to the
> included struct struct hclge_dbg_reg_common_msg (which is 12 bytes
> long).  They are surrounded by #pragma pack(1)/pack().
> 
> This forces the 2 pointers in each second array element of
> hclge_dbg_reg_info[] to be 4 byte aligned (where pointers are 8 bytes
> long on PPC64).

Ah! Thanks, I don't see a reason for these to be packed. 
Looks  like an accident, there is no reason to pack anything 
past struct hclge_dbg_reg_common_msg AFAICT.

Huawei folks, would you mind sending a fix if the analysis is correct?

Re: [PATCH] powerpc: fix the allyesconfig build

[Yunsheng Lin]

On 2020/11/29 3:36, Jakub Kicinski wrote:
> On Sat, 28 Nov 2020 16:20:54 +1100 Stephen Rothwell wrote:
>> On Fri, 27 Nov 2020 17:56:42 -0800 Jakub Kicinski <kuba@kernel.org> wrote:
>>>
>>> What's the offending structure in hisilicon? I'd rather have a look
>>> packing structs with pointers in 'em sounds questionable.
>>>
>>> I only see these two:
>>>
>>> $ git grep packed drivers/net/ethernet/hisilicon/
>>> drivers/net/ethernet/hisilicon/hns/hnae.h:struct __packed hnae_desc {
>>> drivers/net/ethernet/hisilicon/hns3/hns3_enet.h:struct __packed hns3_desc {  
>>
>> struct hclge_dbg_reg_type_info which is 28 bytes long due to the
>> included struct struct hclge_dbg_reg_common_msg (which is 12 bytes
>> long).  They are surrounded by #pragma pack(1)/pack().
>>
>> This forces the 2 pointers in each second array element of
>> hclge_dbg_reg_info[] to be 4 byte aligned (where pointers are 8 bytes
>> long on PPC64).
> 
> Ah! Thanks, I don't see a reason for these to be packed. 
> Looks  like an accident, there is no reason to pack anything 
> past struct hclge_dbg_reg_common_msg AFAICT.
> 
> Huawei folks, would you mind sending a fix if the analysis is correct?

Yes, will send a patch to fix that. Thanks for the analysis.

> .
> 

Re: [PATCH] powerpc: fix the allyesconfig build

[Geert Uytterhoeven]

Hi Stephen,

On Sat, Nov 28, 2020 at 2:28 AM Stephen Rothwell <sfr@canb.auug.org.au> wrote:
> There are 2 drivers that have arrays of packed structures that contain
> pointers that end up at unaligned offsets.  These produce warnings in
> the PowerPC allyesconfig build like this:
>
> WARNING: 148 bad relocations
> c00000000e56510b R_PPC64_UADDR64   .rodata+0x0000000001c72378
> c00000000e565126 R_PPC64_UADDR64   .rodata+0x0000000001c723c0
>
> They are not drivers that are used on PowerPC (I assume), so mark them
> to not be built on PPC64 when CONFIG_RELOCATABLE is enabled.
>
> Cc: Geert Uytterhoeven <geert+renesas@glider.be>
> Cc: Michael Turquette <mturquette@baylibre.com>
> Cc: Stephen Boyd <sboyd@kernel.org>
> Cc: Yisen Zhuang <yisen.zhuang@huawei.com>
> Cc: Salil Mehta <salil.mehta@huawei.com>
> Cc: David S. Miller <davem@davemloft.net>
> Cc: Jakub Kicinski <kuba@kernel.org>
> Cc: Nicholas Piggin  <npiggin@gmail.com>
> Cc: Daniel Axtens <dja@axtens.net>
> Cc: Joel Stanley <joel@jms.id.au>
> Signed-off-by: Stephen Rothwell <sfr@canb.auug.org.au>

Thanks for your patch!

> --- a/drivers/clk/renesas/Kconfig
> +++ b/drivers/clk/renesas/Kconfig
> @@ -151,6 +151,10 @@ config CLK_R8A779A0
>         select CLK_RENESAS_CPG_MSSR
>
>  config CLK_R9A06G032
> +       # PPC64 RELOCATABLE kernels cannot handle relocations to
> +       # unaligned locations that are produced by the array of
> +       # packed structures in this driver.
> +       depends on !(PPC64 && RELOCATABLE)
>         bool "Renesas R9A06G032 clock driver"
>         help
>           This is a driver for R9A06G032 clocks

I prefer to fix this in the driver instead.  The space saving by packing the
structure is minimal.
I've sent a patch
https://lore.kernel.org/r/20201130085743.1656317-1-geert+renesas@glider.be
(when lore is back)

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] powerpc: fix the allyesconfig build

[Stephen Rothwell]

[-- Attachment #1: Type: text/plain, Size: 482 bytes --]

Hi Geert,

On Mon, 30 Nov 2020 09:58:23 +0100 Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>
> Thanks for your patch!

No worries, it has been a small irritant to me for quite a while.

> I prefer to fix this in the driver instead.  The space saving by packing the
> structure is minimal.
> I've sent a patch
> https://lore.kernel.org/r/20201130085743.1656317-1-geert+renesas@glider.be
> (when lore is back)

Absolutely, thanks.

-- 
Cheers,
Stephen Rothwell

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]