| Message ID | 20201128122819.32187696@canb.auug.org.au (mailing list archive) |
|---|---|
| State | Rejected, archived |
| Headers | show |
| Series | powerpc: fix the allyesconfig build | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/apply_patch | success | Successfully applied on branch powerpc/merge (2551450071df2dabd7134e548ac209688ac6741d) |
| snowpatch_ozlabs/build-ppc64le | success | Build succeeded |
| snowpatch_ozlabs/build-ppc64be | success | Build succeeded |
| snowpatch_ozlabs/build-ppc64e | success | Build succeeded |
| snowpatch_ozlabs/build-pmac32 | success | Build succeeded |
| snowpatch_ozlabs/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 20 lines checked |
| snowpatch_ozlabs/needsstable | success | Patch has no Fixes tags |
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 {
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; }; }; > . >
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).
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?
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. > . >
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
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.
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
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?