Re: [PATCH] net: fec: limit register access on i.MX6UL

[Juergen Borleis <jbe@pengutronix.de>]

Hi Marc,

Am Dienstag, dem 20.09.2022 um 13:56 +0200 wrote Marc Kleine-Budde:
> On 20.09.2022 11:51:06, Juergen Borleis wrote:
> > Using 'ethtool -d […]' on an i.MX6UL leads to a kernel crash:
> > 
> >    Unhandled fault: external abort on non-linefetch (0x1008) at […]
> > 
> > due to this SoC has less registers in its FEC implementation compared to
> > other i.MX6 variants. Thus, a run-time decision is required to avoid access
> > to non-existing registers.
> > 
> > Signed-off-by: Juergen Borleis <jbe@pengutronix.de>
> > ---
> >  drivers/net/ethernet/freescale/fec_main.c | 50 +++++++++++++++++++++--
> >  1 file changed, 47 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/net/ethernet/freescale/fec_main.c
> > b/drivers/net/ethernet/freescale/fec_main.c
> > index 6152f6d..ab620b4 100644
> > --- a/drivers/net/ethernet/freescale/fec_main.c
> > +++ b/drivers/net/ethernet/freescale/fec_main.c
> > @@ -2382,6 +2382,31 @@ static u32 fec_enet_register_offset[] = {
> >         IEEE_R_DROP, IEEE_R_FRAME_OK, IEEE_R_CRC, IEEE_R_ALIGN,
> > IEEE_R_MACERR,
> >         IEEE_R_FDXFC, IEEE_R_OCTETS_OK
> >  };
> > +/* for i.MX6ul */
> > +static u32 fec_enet_register_offset_6ul[] = {
> > +       FEC_IEVENT, FEC_IMASK, FEC_R_DES_ACTIVE_0, FEC_X_DES_ACTIVE_0,
> > +       FEC_ECNTRL, FEC_MII_DATA, FEC_MII_SPEED, FEC_MIB_CTRLSTAT,
> > FEC_R_CNTRL,
> > +       FEC_X_CNTRL, FEC_ADDR_LOW, FEC_ADDR_HIGH, FEC_OPD, FEC_TXIC0,
> > FEC_RXIC0,
> > +       FEC_HASH_TABLE_HIGH, FEC_HASH_TABLE_LOW, FEC_GRP_HASH_TABLE_HIGH,
> > +       FEC_GRP_HASH_TABLE_LOW, FEC_X_WMRK, FEC_R_DES_START_0,
> > +       FEC_X_DES_START_0, FEC_R_BUFF_SIZE_0, FEC_R_FIFO_RSFL,
> > FEC_R_FIFO_RSEM,
> > +       FEC_R_FIFO_RAEM, FEC_R_FIFO_RAFL, FEC_RACC,
> > +       RMON_T_DROP, RMON_T_PACKETS, RMON_T_BC_PKT, RMON_T_MC_PKT,
> > +       RMON_T_CRC_ALIGN, RMON_T_UNDERSIZE, RMON_T_OVERSIZE, RMON_T_FRAG,
> > +       RMON_T_JAB, RMON_T_COL, RMON_T_P64, RMON_T_P65TO127,
> > RMON_T_P128TO255,
> > +       RMON_T_P256TO511, RMON_T_P512TO1023, RMON_T_P1024TO2047,
> > +       RMON_T_P_GTE2048, RMON_T_OCTETS,
> > +       IEEE_T_DROP, IEEE_T_FRAME_OK, IEEE_T_1COL, IEEE_T_MCOL, IEEE_T_DEF,
> > +       IEEE_T_LCOL, IEEE_T_EXCOL, IEEE_T_MACERR, IEEE_T_CSERR, IEEE_T_SQE,
> > +       IEEE_T_FDXFC, IEEE_T_OCTETS_OK,
> > +       RMON_R_PACKETS, RMON_R_BC_PKT, RMON_R_MC_PKT, RMON_R_CRC_ALIGN,
> > +       RMON_R_UNDERSIZE, RMON_R_OVERSIZE, RMON_R_FRAG, RMON_R_JAB,
> > +       RMON_R_RESVD_O, RMON_R_P64, RMON_R_P65TO127, RMON_R_P128TO255,
> > +       RMON_R_P256TO511, RMON_R_P512TO1023, RMON_R_P1024TO2047,
> > +       RMON_R_P_GTE2048, RMON_R_OCTETS,
> > +       IEEE_R_DROP, IEEE_R_FRAME_OK, IEEE_R_CRC, IEEE_R_ALIGN,
> > IEEE_R_MACERR,
> > +       IEEE_R_FDXFC, IEEE_R_OCTETS_OK
> > +};
> >  #else
> >  static __u32 fec_enet_register_version = 1;
> >  static u32 fec_enet_register_offset[] = {
> > @@ -2406,7 +2431,26 @@ static void fec_enet_get_regs(struct net_device
> > *ndev,
> >         u32 *buf = (u32 *)regbuf;
> >         u32 i, off;
> >         int ret;
> > -
> > +#if defined(CONFIG_M523x) || defined(CONFIG_M527x) || defined(CONFIG_M528x)
> > || \
> > +       defined(CONFIG_M520x) || defined(CONFIG_M532x) ||
> > defined(CONFIG_ARM) || \
> > +       defined(CONFIG_ARM64) || defined(CONFIG_COMPILE_TEST)
> > +       struct platform_device_id *dev_info =
> > +                       (struct platform_device_id *)fep->pdev->id_entry;
> > +       u32 *reg_list;
> > +       u32 reg_cnt;
> > +
> > +       if (strcmp(dev_info->name, "imx6ul-fec")) {
> > +               reg_list = fec_enet_register_offset;
> > +               reg_cnt = ARRAY_SIZE(fec_enet_register_offset);
> > +       } else {
> > +               reg_list = fec_enet_register_offset_6ul;
> > +               reg_cnt = ARRAY_SIZE(fec_enet_register_offset_6ul);
> > +       }
> 
> What about using of_machine_is_compatible()?

Good point. Thought this call requires an oftree handle. Will change it in v2.

> > +#else
> > +       /* coldfire */
> > +       static u32 *reg_list = fec_enet_register_offset;
> > +       static const u32 reg_cnt = ARRAY_SIZE(fec_enet_register_offset);
> > +#endif
> 
> Why do you need the ifdef?

The coldfire variant of this part is already implemented in this way.

> […]

jb

-- 
Pengutronix e.K.                       | Juergen Borleis             |
Steuerwalder Str. 21                   | https://www.pengutronix.de/ |
31137 Hildesheim, Germany              | Phone: +49-5121-206917-128  |
Amtsgericht Hildesheim, HRA 2686       | Fax:   +49-5121-206917-9    |