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

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

Posted by Juergen Borleis 3 years, 6 months ago

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);
+	}
+#else
+	/* coldfire */
+	static u32 *reg_list = fec_enet_register_offset;
+	static const u32 reg_cnt = ARRAY_SIZE(fec_enet_register_offset);
+#endif
 	ret = pm_runtime_resume_and_get(dev);
 	if (ret < 0)
 		return;
@@ -2415,8 +2459,8 @@ static void fec_enet_get_regs(struct net_device *ndev,
 
 	memset(buf, 0, regs->len);
 
-	for (i = 0; i < ARRAY_SIZE(fec_enet_register_offset); i++) {
-		off = fec_enet_register_offset[i];
+	for (i = 0; i < reg_cnt; i++) {
+		off = reg_list[i];
 
 		if ((off == FEC_R_BOUND || off == FEC_R_FSTART) &&
 		    !(fep->quirks & FEC_QUIRK_HAS_FRREG))
-- 
2.30.2

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

Posted by Andrew Lunn 3 years, 6 months ago

> +/* 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;

Seeing this, i wonder if the i.MX6ul needs its own register version,
so that ethtool(1) knows what registers are valid?

   Andrew

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

Posted by Juergen Borleis 3 years, 5 months ago

Am Dienstag, dem 20.09.2022 um 14:46 +0200 schrieb Andrew Lunn:
> > +/* 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;
> 
> Seeing this, i wonder if the i.MX6ul needs its own register version,
> so that ethtool(1) knows what registers are valid?

I don't think so. The register layout is the same in both SoCs, e.g. all
existing registers are at the same offsets on i.MX6 and i.MX6UL. And due to the
memset() call, the few missing registers on i.MX6UL are all reported as 0.

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    |

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

Posted by Marco Felsch 3 years, 6 months ago

On 22-09-20, Andrew Lunn wrote:
> > +/* 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;
> 
> Seeing this, i wonder if the i.MX6ul needs its own register version,
> so that ethtool(1) knows what registers are valid?

Regarding the uAPI (uapi/linux/ethtool.h):
8<-------------------------------------------------
 * @version: Dump format version.  This is driver-specific and may
 *      distinguish different chips/revisions.  Drivers must use new
 *      version numbers whenever the dump format changes in an
 *      incompatible way.
8<-------------------------------------------------
I would say yes.

Regards,
  Marco

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

Posted by Juergen Borleis 3 years, 5 months ago

Am Dienstag, dem 20.09.2022 um 14:50 +0200 schrieb Marco Felsch:
> On 22-09-20, Andrew Lunn wrote:
> > > +/* 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;
> > 
> > Seeing this, i wonder if the i.MX6ul needs its own register version,
> > so that ethtool(1) knows what registers are valid?
> 
> Regarding the uAPI (uapi/linux/ethtool.h):
> 8<-------------------------------------------------
>  * @version: Dump format version.  This is driver-specific and may
>  *      distinguish different chips/revisions.  Drivers must use new
>  *      version numbers whenever the dump format changes in an
>  *      incompatible way.
> 8<-------------------------------------------------
> I would say yes.

But there is no format change. Only a value change. Where the i.MX6 may report a
value, the i.MX6UL just reports a zero.

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    |

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

Posted by Marc Kleine-Budde 3 years, 6 months ago

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()?

> +#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?

>  	ret = pm_runtime_resume_and_get(dev);
>  	if (ret < 0)
>  		return;
> @@ -2415,8 +2459,8 @@ static void fec_enet_get_regs(struct net_device *ndev,
>  
>  	memset(buf, 0, regs->len);
>  
> -	for (i = 0; i < ARRAY_SIZE(fec_enet_register_offset); i++) {
> -		off = fec_enet_register_offset[i];
> +	for (i = 0; i < reg_cnt; i++) {
> +		off = reg_list[i];
>  
>  		if ((off == FEC_R_BOUND || off == FEC_R_FSTART) &&
>  		    !(fep->quirks & FEC_QUIRK_HAS_FRREG))

Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |

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

Posted by Juergen Borleis 3 years, 5 months ago

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    |