Re: [PATCH net-next 1/1] net: fec: add netif status check before set mac address

[Lucas Stach]

Am Mittwoch, den 09.09.2015, 10:42 +0800 schrieb Fugang Duan:
> There exist one issue by below case that case system hang:
> ifconfig eth0 down
> ifconfig eth0 hw ether 00:10:19:19:81:19
> 
> After eth0 down, all fec clocks are gated off. In the .fec_set_mac_address()
> function, it will set new MAC address to registers, which causes system hang.
> 
> So it needs to add netif status check to avoid registers access when clocks 
> are
> gated off. Until eth0 up the new MAC address are wrote into related registers.
> 
> Signed-off-by: Fugang Duan 
> ---
>  drivers/net/ethernet/freescale/fec_main.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/net/ethernet/freescale/fec_main.c 
> b/drivers/net/ethernet/freescale/fec_main.c
> index 91925e3..cd09dbb 100644
> --- a/drivers/net/ethernet/freescale/fec_main.c
> +++ b/drivers/net/ethernet/freescale/fec_main.c
> @@ -3029,6 +3029,9 @@ fec_set_mac_address(struct net_device *ndev, void *p)
>   memcpy(ndev->dev_addr, addr->sa_data, ndev->addr_len);
>   }
>  
> + if (!netif_running(ndev))
> + return 0;
This deserves a comment in the code as to why it is needed and how it
still works.

Regards,
Lucas
> +
>   writel(ndev->dev_addr[3] | (ndev->dev_addr[2] << 8) |
>   (ndev->dev_addr[1] << 16) | (ndev->dev_addr[0] << 24),
>   fep->hwp + FEC_ADDR_LOW);

-- 
Pengutronix e.K. | Lucas Stach |
Industrial Linux Solutions   | http://www.pengutronix.de/  |

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH net-next 1/1] net: fec: add netif status check before set mac address

[Duan Andy]

From: Lucas Stach <l.st...@pengutronix.de> Sent: Wednesday, September 09, 2015 
5:14 PM
> To: Duan Fugang-B38611
> Cc: da...@davemloft.net; netdev@vger.kernel.org;
> bhutchi...@solarflare.com
> Subject: Re: [PATCH net-next 1/1] net: fec: add netif status check before
> set mac address
> 
> Am Mittwoch, den 09.09.2015, 10:42 +0800 schrieb Fugang Duan:
> > There exist one issue by below case that case system hang:
> > ifconfig eth0 down
> > ifconfig eth0 hw ether 00:10:19:19:81:19
> >
> > After eth0 down, all fec clocks are gated off. In the
> > .fec_set_mac_address() function, it will set new MAC address to
> registers, which causes system hang.
> >
> > So it needs to add netif status check to avoid registers access when
> > clocks are gated off. Until eth0 up the new MAC address are wrote into
> related registers.
> >
> > Signed-off-by: Fugang Duan <b38...@freescale.com>
> > ---
> >  drivers/net/ethernet/freescale/fec_main.c | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/drivers/net/ethernet/freescale/fec_main.c
> > b/drivers/net/ethernet/freescale/fec_main.c
> > index 91925e3..cd09dbb 100644
> > --- a/drivers/net/ethernet/freescale/fec_main.c
> > +++ b/drivers/net/ethernet/freescale/fec_main.c
> > @@ -3029,6 +3029,9 @@ fec_set_mac_address(struct net_device *ndev, void
> *p)
> > memcpy(ndev->dev_addr, addr->sa_data, ndev->addr_len);
> > }
> >
> > +   if (!netif_running(ndev))
> > +   return 0;
> This deserves a comment in the code as to why it is needed and how it
> still works.
> 
> Regards,
> Lucas
> > +

Ok, I will add comment on here in next version.

Thanks.
> > writel(ndev->dev_addr[3] | (ndev->dev_addr[2] << 8) |
> > (ndev->dev_addr[1] << 16) | (ndev->dev_addr[0] << 24),
> > fep->hwp + FEC_ADDR_LOW);
> 
> --
> Pengutronix e.K. | Lucas Stach |
> Industrial Linux Solutions   | http://www.pengutronix.de/  |
N�r��yb�X��ǧv�^�)޺{.n�+���z�^�)w*jg����ݢj/���z�ޖ��2�ޙ&�)ߡ�a�����G���h��j:+v���w��٥

Re: [PATCH net-next 1/1] net: fec: add netif status check before set mac address

[David Miller]

From: Duan Andy <fugang.d...@freescale.com>
Date: Wed, 9 Sep 2015 03:45:48 +

> From: Florian Fainelli <f.faine...@gmail.com> Sent: Wednesday, September 09, 
> 2015 11:38 AM
>> To: Duan Fugang-B38611; da...@davemloft.net
>> Cc: netdev@vger.kernel.org; bhutchi...@solarflare.com
>> Subject: Re: [PATCH net-next 1/1] net: fec: add netif status check before
>> set mac address
>> 
>> Le 09/08/15 19:42, Fugang Duan a écrit :
>> > There exist one issue by below case that case system hang:
>> > ifconfig eth0 down
>> > ifconfig eth0 hw ether 00:10:19:19:81:19
>> >
>> > After eth0 down, all fec clocks are gated off. In the
>> > .fec_set_mac_address() function, it will set new MAC address to
>> registers, which causes system hang.
>> >
>> > So it needs to add netif status check to avoid registers access when
>> > clocks are gated off. Until eth0 up the new MAC address are wrote into
>> related registers.
>> 
>> Since this is a bug fix, do not you intend to target the "net" tree
>> instead of "net-next"?
>> 
> Thanks for your reminding, it is better to enter net tree.
> 
> David, if you apply it, pls put it into net tree. Thanks.

You need to address the feedback given by Lucas first.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH net-next 1/1] net: fec: add netif status check before set mac address

[Fugang Duan]

There exist one issue by below case that case system hang:
ifconfig eth0 down
ifconfig eth0 hw ether 00:10:19:19:81:19

After eth0 down, all fec clocks are gated off. In the .fec_set_mac_address()
function, it will set new MAC address to registers, which causes system hang.

So it needs to add netif status check to avoid registers access when clocks are
gated off. Until eth0 up the new MAC address are wrote into related registers.

Signed-off-by: Fugang Duan 
---
 drivers/net/ethernet/freescale/fec_main.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/net/ethernet/freescale/fec_main.c 
b/drivers/net/ethernet/freescale/fec_main.c
index 91925e3..cd09dbb 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -3029,6 +3029,9 @@ fec_set_mac_address(struct net_device *ndev, void *p)
memcpy(ndev->dev_addr, addr->sa_data, ndev->addr_len);
}
 
+   if (!netif_running(ndev))
+   return 0;
+
writel(ndev->dev_addr[3] | (ndev->dev_addr[2] << 8) |
(ndev->dev_addr[1] << 16) | (ndev->dev_addr[0] << 24),
fep->hwp + FEC_ADDR_LOW);
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH net-next 1/1] net: fec: add netif status check before set mac address

[Florian Fainelli]

Le 09/08/15 19:42, Fugang Duan a écrit :
> There exist one issue by below case that case system hang:
> ifconfig eth0 down
> ifconfig eth0 hw ether 00:10:19:19:81:19
> 
> After eth0 down, all fec clocks are gated off. In the .fec_set_mac_address()
> function, it will set new MAC address to registers, which causes system hang.
> 
> So it needs to add netif status check to avoid registers access when clocks 
> are
> gated off. Until eth0 up the new MAC address are wrote into related registers.

Since this is a bug fix, do not you intend to target the "net" tree
instead of "net-next"?

> 
> Signed-off-by: Fugang Duan 
> ---
>  drivers/net/ethernet/freescale/fec_main.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/net/ethernet/freescale/fec_main.c 
> b/drivers/net/ethernet/freescale/fec_main.c
> index 91925e3..cd09dbb 100644
> --- a/drivers/net/ethernet/freescale/fec_main.c
> +++ b/drivers/net/ethernet/freescale/fec_main.c
> @@ -3029,6 +3029,9 @@ fec_set_mac_address(struct net_device *ndev, void *p)
>   memcpy(ndev->dev_addr, addr->sa_data, ndev->addr_len);
>   }
>  
> + if (!netif_running(ndev))
> + return 0;
> +
>   writel(ndev->dev_addr[3] | (ndev->dev_addr[2] << 8) |
>   (ndev->dev_addr[1] << 16) | (ndev->dev_addr[0] << 24),
>   fep->hwp + FEC_ADDR_LOW);
> 


-- 
Florian
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH net-next 1/1] net: fec: add netif status check before set mac address

[Duan Andy]

From: Florian Fainelli <f.faine...@gmail.com> Sent: Wednesday, September 09, 
2015 11:38 AM
> To: Duan Fugang-B38611; da...@davemloft.net
> Cc: netdev@vger.kernel.org; bhutchi...@solarflare.com
> Subject: Re: [PATCH net-next 1/1] net: fec: add netif status check before
> set mac address
> 
> Le 09/08/15 19:42, Fugang Duan a écrit :
> > There exist one issue by below case that case system hang:
> > ifconfig eth0 down
> > ifconfig eth0 hw ether 00:10:19:19:81:19
> >
> > After eth0 down, all fec clocks are gated off. In the
> > .fec_set_mac_address() function, it will set new MAC address to
> registers, which causes system hang.
> >
> > So it needs to add netif status check to avoid registers access when
> > clocks are gated off. Until eth0 up the new MAC address are wrote into
> related registers.
> 
> Since this is a bug fix, do not you intend to target the "net" tree
> instead of "net-next"?
> 
Thanks for your reminding, it is better to enter net tree.

David, if you apply it, pls put it into net tree. Thanks.

> >
> > Signed-off-by: Fugang Duan <b38...@freescale.com>
> > ---
> >  drivers/net/ethernet/freescale/fec_main.c | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/drivers/net/ethernet/freescale/fec_main.c
> > b/drivers/net/ethernet/freescale/fec_main.c
> > index 91925e3..cd09dbb 100644
> > --- a/drivers/net/ethernet/freescale/fec_main.c
> > +++ b/drivers/net/ethernet/freescale/fec_main.c
> > @@ -3029,6 +3029,9 @@ fec_set_mac_address(struct net_device *ndev, void
> *p)
> > memcpy(ndev->dev_addr, addr->sa_data, ndev->addr_len);
> > }
> >
> > +   if (!netif_running(ndev))
> > +   return 0;
> > +
> > writel(ndev->dev_addr[3] | (ndev->dev_addr[2] << 8) |
> > (ndev->dev_addr[1] << 16) | (ndev->dev_addr[0] << 24),
> > fep->hwp + FEC_ADDR_LOW);
> >
> 
> 
> --
> Florian
N�r��yb�X��ǧv�^�)޺{.n�+���z�^�)w*jg����ݢj/���z�ޖ��2�ޙ&�)ߡ�a�����G���h��j:+v���w��٥