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

2015-09-09 Thread Duan Andy 
From: Lucas Stach  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 
> > ---
> >  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

2015-09-09 Thread David Miller 
From: Duan Andy 
Date: Wed, 9 Sep 2015 03:45:48 +

> From: Florian Fainelli  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

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

2015-09-09 Thread 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

2015-09-08 Thread Duan Andy 
From: Florian Fainelli  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 
> > ---
> >  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��٥

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

2015-09-08 Thread 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