| Message ID | 20231214114600.2451162-20-claudiu.beznea.uj@bp.renesas.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: ravb: Add suspend to RAM and runtime PM support for RZ/G3S | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/apply | fail | Patch does not apply to net-next |
On 12/14/23 2:45 PM, Claudiu wrote: > From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > > Do not allow setting promiscuous mode if the interface is down. In case > runtime PM is enabled, and while interface is down, the IP will be in reset > mode (as for some platforms disabling/enabling the clocks will switch the > IP to standby mode which will lead to losing registers' content). Register. Have this issue actually occurred for you? > Commit prepares for the addition of runtime PM. > > Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> [...] > diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c > index 1995cf7ff084..633346b6cd7c 100644 > --- a/drivers/net/ethernet/renesas/ravb_main.c > +++ b/drivers/net/ethernet/renesas/ravb_main.c > @@ -2164,6 +2164,9 @@ static void ravb_set_rx_mode(struct net_device *ndev) > struct ravb_private *priv = netdev_priv(ndev); > unsigned long flags; > > + if (!netif_running(ndev)) Seems racy as well... > + return; Hm, sh_eth.c doesn't have such check -- perhaps should be fixed as well... [...] MBR, Sergey
On 16.12.2023 22:16, Sergey Shtylyov wrote: > On 12/14/23 2:45 PM, Claudiu wrote: > >> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> >> >> Do not allow setting promiscuous mode if the interface is down. In case >> runtime PM is enabled, and while interface is down, the IP will be in reset >> mode (as for some platforms disabling/enabling the clocks will switch the >> IP to standby mode which will lead to losing registers' content). > > Register. > Have this issue actually occurred for you? > >> Commit prepares for the addition of runtime PM. >> >> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > [...] > >> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c >> index 1995cf7ff084..633346b6cd7c 100644 >> --- a/drivers/net/ethernet/renesas/ravb_main.c >> +++ b/drivers/net/ethernet/renesas/ravb_main.c >> @@ -2164,6 +2164,9 @@ static void ravb_set_rx_mode(struct net_device *ndev) >> struct ravb_private *priv = netdev_priv(ndev); >> unsigned long flags; >> >> + if (!netif_running(ndev)) > > Seems racy as well... It is also called with rtnl_mutex locked at least though __dev_change_flags(). > >> + return; > > Hm, sh_eth.c doesn't have such check -- perhaps should be fixed > as well... > > [...] > > MBR, Sergey
On 12/17/23 5:02 PM, claudiu beznea wrote: [...] >>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> >>> >>> Do not allow setting promiscuous mode if the interface is down. In case >>> runtime PM is enabled, and while interface is down, the IP will be in reset >>> mode (as for some platforms disabling/enabling the clocks will switch the >>> IP to standby mode which will lead to losing registers' content). >> >> Register. >> Have this issue actually occurred for you? >> >>> Commit prepares for the addition of runtime PM. >>> >>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> >> [...] >> >>> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c >>> index 1995cf7ff084..633346b6cd7c 100644 >>> --- a/drivers/net/ethernet/renesas/ravb_main.c >>> +++ b/drivers/net/ethernet/renesas/ravb_main.c >>> @@ -2164,6 +2164,9 @@ static void ravb_set_rx_mode(struct net_device *ndev) >>> struct ravb_private *priv = netdev_priv(ndev); >>> unsigned long flags; >>> >>> + if (!netif_running(ndev)) >> >> Seems racy as well... > > It is also called with rtnl_mutex locked at least though __dev_change_flags(). I'm tired of chasing the call tree for you. :-) Since I'm hoping we'll agree on the ndo_get_stats() case, it would seem safer to use an is_opened flag here as well, like sh_eth.c does. [...] MBR, Sergey
diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c index 1995cf7ff084..633346b6cd7c 100644 --- a/drivers/net/ethernet/renesas/ravb_main.c +++ b/drivers/net/ethernet/renesas/ravb_main.c @@ -2164,6 +2164,9 @@ static void ravb_set_rx_mode(struct net_device *ndev) struct ravb_private *priv = netdev_priv(ndev); unsigned long flags; + if (!netif_running(ndev)) + return; + spin_lock_irqsave(&priv->lock, flags); ravb_modify(ndev, ECMR, ECMR_PRM, ndev->flags & IFF_PROMISC ? ECMR_PRM : 0);