| Message ID | 20241008061153.1977930-1-wei.fang@nxp.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 6be063071a457767ee229db13f019c2ec03bfe44 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] net: fec: don't save PTP state if PTP is unsupported | expand |
On 2024. 10. 08. 8:11, Wei Fang wrote: > Some platforms (such as i.MX25 and i.MX27) do not support PTP, so on > these platforms fec_ptp_init() is not called and the related members > in fep are not initialized. However, fec_ptp_save_state() is called > unconditionally, which causes the kernel to panic. Therefore, add a > condition so that fec_ptp_save_state() is not called if PTP is not > supported. > > Fixes: a1477dc87dc4 ("net: fec: Restart PPS after link state change") > Reported-by: Guenter Roeck <linux@roeck-us.net> > Closes: https://lore.kernel.org/lkml/353e41fe-6bb4-4ee9-9980-2da2a9c1c508@roeck-us.net/ > Signed-off-by: Wei Fang <wei.fang@nxp.com> > --- > drivers/net/ethernet/freescale/fec_main.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c > index 60fb54231ead..1b55047c0237 100644 > --- a/drivers/net/ethernet/freescale/fec_main.c > +++ b/drivers/net/ethernet/freescale/fec_main.c > @@ -1077,7 +1077,8 @@ fec_restart(struct net_device *ndev) > u32 rcntl = OPT_FRAME_SIZE | 0x04; > u32 ecntl = FEC_ECR_ETHEREN; > > - fec_ptp_save_state(fep); > + if (fep->bufdesc_ex) > + fec_ptp_save_state(fep); > > /* Whack a reset. We should wait for this. > * For i.MX6SX SOC, enet use AXI bus, we use disable MAC > @@ -1340,7 +1341,8 @@ fec_stop(struct net_device *ndev) > netdev_err(ndev, "Graceful transmit stop did not complete!\n"); > } > > - fec_ptp_save_state(fep); > + if (fep->bufdesc_ex) > + fec_ptp_save_state(fep); > > /* Whack a reset. We should wait for this. > * For i.MX6SX SOC, enet use AXI bus, we use disable MAC Thanks, it seems fec_ptp_restore_state() was properly guarded, but save_state() was not. Sorry for that. Reviewed-by: Csókás, Bence <csokas.bence@prolan.hu>
On Tue, Oct 08, 2024 at 02:11:53PM +0800, Wei Fang wrote: > Some platforms (such as i.MX25 and i.MX27) do not support PTP, so on > these platforms fec_ptp_init() is not called and the related members > in fep are not initialized. However, fec_ptp_save_state() is called > unconditionally, which causes the kernel to panic. Therefore, add a > condition so that fec_ptp_save_state() is not called if PTP is not > supported. > > Fixes: a1477dc87dc4 ("net: fec: Restart PPS after link state change") > Reported-by: Guenter Roeck <linux@roeck-us.net> > Closes: https://lore.kernel.org/lkml/353e41fe-6bb4-4ee9-9980-2da2a9c1c508@roeck-us.net/ > Signed-off-by: Wei Fang <wei.fang@nxp.com> > --- > drivers/net/ethernet/freescale/fec_main.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c > index 60fb54231ead..1b55047c0237 100644 > --- a/drivers/net/ethernet/freescale/fec_main.c > +++ b/drivers/net/ethernet/freescale/fec_main.c > @@ -1077,7 +1077,8 @@ fec_restart(struct net_device *ndev) > u32 rcntl = OPT_FRAME_SIZE | 0x04; > u32 ecntl = FEC_ECR_ETHEREN; > > - fec_ptp_save_state(fep); > + if (fep->bufdesc_ex) > + fec_ptp_save_state(fep); Hi, I am wondering if you considered adding this check to (the top of) fec_ptp_save_state. It seems like it would both lead to a smaller change and be less error-prone to use. > > /* Whack a reset. We should wait for this. > * For i.MX6SX SOC, enet use AXI bus, we use disable MAC > @@ -1340,7 +1341,8 @@ fec_stop(struct net_device *ndev) > netdev_err(ndev, "Graceful transmit stop did not complete!\n"); > } > > - fec_ptp_save_state(fep); > + if (fep->bufdesc_ex) > + fec_ptp_save_state(fep); > > /* Whack a reset. We should wait for this. > * For i.MX6SX SOC, enet use AXI bus, we use disable MAC > -- > 2.34.1 > >
> -----Original Message----- > From: Simon Horman <horms@kernel.org> > Sent: 2024年10月9日 19:55 > To: Wei Fang <wei.fang@nxp.com> > Cc: davem@davemloft.net; edumazet@google.com; kuba@kernel.org; > pabeni@redhat.com; richardcochran@gmail.com; csokas.bence@prolan.hu; > Shenwei Wang <shenwei.wang@nxp.com>; Clark Wang > <xiaoning.wang@nxp.com>; linux@roeck-us.net; imx@lists.linux.dev; > netdev@vger.kernel.org; linux-kernel@vger.kernel.org > Subject: Re: [PATCH net] net: fec: don't save PTP state if PTP is unsupported > > On Tue, Oct 08, 2024 at 02:11:53PM +0800, Wei Fang wrote: > > Some platforms (such as i.MX25 and i.MX27) do not support PTP, so on > > these platforms fec_ptp_init() is not called and the related members > > in fep are not initialized. However, fec_ptp_save_state() is called > > unconditionally, which causes the kernel to panic. Therefore, add a > > condition so that fec_ptp_save_state() is not called if PTP is not > > supported. > > > > Fixes: a1477dc87dc4 ("net: fec: Restart PPS after link state change") > > Reported-by: Guenter Roeck <linux@roeck-us.net> > > Closes: > https://lore.ker/ > nel.org%2Flkml%2F353e41fe-6bb4-4ee9-9980-2da2a9c1c508%40roeck-us.net > %2F&data=05%7C02%7Cwei.fang%40nxp.com%7Cb10cac9ed8cd43284aae08 > dce85930cd%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C63864 > 0716999752935%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLC > JQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C0%7C%7C%7C&sdata=1 > gxwnxNjk91xX7I%2Foco%2F4OhBbxNCryhDMo72O9Jkr2w%3D&reserved=0 > > Signed-off-by: Wei Fang <wei.fang@nxp.com> > > --- > > drivers/net/ethernet/freescale/fec_main.c | 6 ++++-- > > 1 file changed, 4 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/net/ethernet/freescale/fec_main.c > b/drivers/net/ethernet/freescale/fec_main.c > > index 60fb54231ead..1b55047c0237 100644 > > --- a/drivers/net/ethernet/freescale/fec_main.c > > +++ b/drivers/net/ethernet/freescale/fec_main.c > > @@ -1077,7 +1077,8 @@ fec_restart(struct net_device *ndev) > > u32 rcntl = OPT_FRAME_SIZE | 0x04; > > u32 ecntl = FEC_ECR_ETHEREN; > > > > - fec_ptp_save_state(fep); > > + if (fep->bufdesc_ex) > > + fec_ptp_save_state(fep); > > Hi, > > I am wondering if you considered adding this check to (the top of) > fec_ptp_save_state. It seems like it would both lead to a smaller > change and be less error-prone to use. > Yes, I considered this solution, but when I thought about it, fec_ptp_save_state() and fec_ptp_restore_state() are a pair. If the check is added to fec_ptp_save_state(), it is better to add it to fec_ptp_restore_state(). However, considering that this is not related to the current problem, and there are relatively few calls to fec_ptp_restore_state(), I did not do this. If there are more calls to fec_ptp_restore_state()/fec_ptp_restore_state() in the future, I will consider it. Thanks. > > > > /* Whack a reset. We should wait for this. > > * For i.MX6SX SOC, enet use AXI bus, we use disable MAC > > @@ -1340,7 +1341,8 @@ fec_stop(struct net_device *ndev) > > netdev_err(ndev, "Graceful transmit stop did not complete!\n"); > > } > > > > - fec_ptp_save_state(fep); > > + if (fep->bufdesc_ex) > > + fec_ptp_save_state(fep); > > > > /* Whack a reset. We should wait for this. > > * For i.MX6SX SOC, enet use AXI bus, we use disable MAC > > -- > > 2.34.1 > > > >
On Wed, Oct 09, 2024 at 12:43:50PM +0000, Wei Fang wrote: > > -----Original Message----- > > From: Simon Horman <horms@kernel.org> > > Sent: 2024年10月9日 19:55 > > To: Wei Fang <wei.fang@nxp.com> > > Cc: davem@davemloft.net; edumazet@google.com; kuba@kernel.org; > > pabeni@redhat.com; richardcochran@gmail.com; csokas.bence@prolan.hu; > > Shenwei Wang <shenwei.wang@nxp.com>; Clark Wang > > <xiaoning.wang@nxp.com>; linux@roeck-us.net; imx@lists.linux.dev; > > netdev@vger.kernel.org; linux-kernel@vger.kernel.org > > Subject: Re: [PATCH net] net: fec: don't save PTP state if PTP is unsupported > > > > On Tue, Oct 08, 2024 at 02:11:53PM +0800, Wei Fang wrote: > > > Some platforms (such as i.MX25 and i.MX27) do not support PTP, so on > > > these platforms fec_ptp_init() is not called and the related members > > > in fep are not initialized. However, fec_ptp_save_state() is called > > > unconditionally, which causes the kernel to panic. Therefore, add a > > > condition so that fec_ptp_save_state() is not called if PTP is not > > > supported. > > > > > > Fixes: a1477dc87dc4 ("net: fec: Restart PPS after link state change") > > > Reported-by: Guenter Roeck <linux@roeck-us.net> > > > Closes: > > https://lore.ker/ > > nel.org%2Flkml%2F353e41fe-6bb4-4ee9-9980-2da2a9c1c508%40roeck-us.net > > %2F&data=05%7C02%7Cwei.fang%40nxp.com%7Cb10cac9ed8cd43284aae08 > > dce85930cd%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C63864 > > 0716999752935%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLC > > JQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C0%7C%7C%7C&sdata=1 > > gxwnxNjk91xX7I%2Foco%2F4OhBbxNCryhDMo72O9Jkr2w%3D&reserved=0 > > > Signed-off-by: Wei Fang <wei.fang@nxp.com> > > > --- > > > drivers/net/ethernet/freescale/fec_main.c | 6 ++++-- > > > 1 file changed, 4 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/net/ethernet/freescale/fec_main.c > > b/drivers/net/ethernet/freescale/fec_main.c > > > index 60fb54231ead..1b55047c0237 100644 > > > --- a/drivers/net/ethernet/freescale/fec_main.c > > > +++ b/drivers/net/ethernet/freescale/fec_main.c > > > @@ -1077,7 +1077,8 @@ fec_restart(struct net_device *ndev) > > > u32 rcntl = OPT_FRAME_SIZE | 0x04; > > > u32 ecntl = FEC_ECR_ETHEREN; > > > > > > - fec_ptp_save_state(fep); > > > + if (fep->bufdesc_ex) > > > + fec_ptp_save_state(fep); > > > > Hi, > > > > I am wondering if you considered adding this check to (the top of) > > fec_ptp_save_state. It seems like it would both lead to a smaller > > change and be less error-prone to use. > > > > Yes, I considered this solution, but when I thought about it, > fec_ptp_save_state() and fec_ptp_restore_state() are a pair. If > the check is added to fec_ptp_save_state(), it is better to add > it to fec_ptp_restore_state(). However, considering that this is > not related to the current problem, and there are relatively few > calls to fec_ptp_restore_state(), I did not do this. If there are more > calls to fec_ptp_restore_state()/fec_ptp_restore_state() in the > future, I will consider it. Sure, I agree on your point regarding symmetry, which I had not considered when I wrote my previous email. And I agree that the patch is suitable for net in it's current form. Reviewed-by: Simon Horman <horms@kernel.org> ...
On 10/7/24 23:11, Wei Fang wrote: > Some platforms (such as i.MX25 and i.MX27) do not support PTP, so on > these platforms fec_ptp_init() is not called and the related members > in fep are not initialized. However, fec_ptp_save_state() is called > unconditionally, which causes the kernel to panic. Therefore, add a > condition so that fec_ptp_save_state() is not called if PTP is not > supported. > > Fixes: a1477dc87dc4 ("net: fec: Restart PPS after link state change") > Reported-by: Guenter Roeck <linux@roeck-us.net> > Closes: https://lore.kernel.org/lkml/353e41fe-6bb4-4ee9-9980-2da2a9c1c508@roeck-us.net/ > Signed-off-by: Wei Fang <wei.fang@nxp.com> Tested-by: Guenter Roeck <linux@roeck-us.net>
Hello: This patch was applied to netdev/net.git (main) by Jakub Kicinski <kuba@kernel.org>: On Tue, 8 Oct 2024 14:11:53 +0800 you wrote: > Some platforms (such as i.MX25 and i.MX27) do not support PTP, so on > these platforms fec_ptp_init() is not called and the related members > in fep are not initialized. However, fec_ptp_save_state() is called > unconditionally, which causes the kernel to panic. Therefore, add a > condition so that fec_ptp_save_state() is not called if PTP is not > supported. > > [...] Here is the summary with links: - [net] net: fec: don't save PTP state if PTP is unsupported https://git.kernel.org/netdev/net/c/6be063071a45 You are awesome, thank you!
diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c index 60fb54231ead..1b55047c0237 100644 --- a/drivers/net/ethernet/freescale/fec_main.c +++ b/drivers/net/ethernet/freescale/fec_main.c @@ -1077,7 +1077,8 @@ fec_restart(struct net_device *ndev) u32 rcntl = OPT_FRAME_SIZE | 0x04; u32 ecntl = FEC_ECR_ETHEREN; - fec_ptp_save_state(fep); + if (fep->bufdesc_ex) + fec_ptp_save_state(fep); /* Whack a reset. We should wait for this. * For i.MX6SX SOC, enet use AXI bus, we use disable MAC @@ -1340,7 +1341,8 @@ fec_stop(struct net_device *ndev) netdev_err(ndev, "Graceful transmit stop did not complete!\n"); } - fec_ptp_save_state(fep); + if (fep->bufdesc_ex) + fec_ptp_save_state(fep); /* Whack a reset. We should wait for this. * For i.MX6SX SOC, enet use AXI bus, we use disable MAC
Some platforms (such as i.MX25 and i.MX27) do not support PTP, so on these platforms fec_ptp_init() is not called and the related members in fep are not initialized. However, fec_ptp_save_state() is called unconditionally, which causes the kernel to panic. Therefore, add a condition so that fec_ptp_save_state() is not called if PTP is not supported. Fixes: a1477dc87dc4 ("net: fec: Restart PPS after link state change") Reported-by: Guenter Roeck <linux@roeck-us.net> Closes: https://lore.kernel.org/lkml/353e41fe-6bb4-4ee9-9980-2da2a9c1c508@roeck-us.net/ Signed-off-by: Wei Fang <wei.fang@nxp.com> --- drivers/net/ethernet/freescale/fec_main.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)