| Message ID | 20181206110132.8701-1-yuval.shaia@oracle.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | IB/rxe: Reuse code which sets port state | expand |
On 2018/12/6 19:01, Yuval Shaia wrote: > Same code is executed in both rxe_param_set_add and rxe_notify > functions. > Make one function and call it from both places. > > Note that the code that checks validity of rxe object is omitted since > both callers already make sure it is valid. > > Signed-off-by: Yuval Shaia <yuval.shaia@oracle.com> > --- > drivers/infiniband/sw/rxe/rxe.h | 1 + > drivers/infiniband/sw/rxe/rxe_net.c | 13 +++++++++---- > drivers/infiniband/sw/rxe/rxe_sysfs.c | 18 +----------------- > 3 files changed, 11 insertions(+), 21 deletions(-) > > diff --git a/drivers/infiniband/sw/rxe/rxe.h b/drivers/infiniband/sw/rxe/rxe.h > index 8f79bd86d033..cd40a3062551 100644 > --- a/drivers/infiniband/sw/rxe/rxe.h > +++ b/drivers/infiniband/sw/rxe/rxe.h > @@ -110,5 +110,6 @@ struct rxe_dev *get_rxe_by_name(const char *name); > > void rxe_port_up(struct rxe_dev *rxe); > void rxe_port_down(struct rxe_dev *rxe); > +void rxe_set_port_state(struct net_device *ndev, struct rxe_dev *rxe); > > #endif /* RXE_H */ > diff --git a/drivers/infiniband/sw/rxe/rxe_net.c b/drivers/infiniband/sw/rxe/rxe_net.c > index b26a8141f3ed..467329f69300 100644 > --- a/drivers/infiniband/sw/rxe/rxe_net.c > +++ b/drivers/infiniband/sw/rxe/rxe_net.c > @@ -625,6 +625,14 @@ void rxe_port_down(struct rxe_dev *rxe) > dev_info(&rxe->ib_dev.dev, "set down\n"); > } > > +void rxe_set_port_state(struct net_device *ndev, struct rxe_dev *rxe) an inline function is better? Zhu Yanjun > +{ > + if (netif_running(ndev) && netif_carrier_ok(ndev)) > + rxe_port_up(rxe); > + else > + rxe_port_down(rxe); > +} > + > static int rxe_notify(struct notifier_block *not_blk, > unsigned long event, > void *arg) > @@ -651,10 +659,7 @@ static int rxe_notify(struct notifier_block *not_blk, > rxe_set_mtu(rxe, ndev->mtu); > break; > case NETDEV_CHANGE: > - if (netif_running(ndev) && netif_carrier_ok(ndev)) > - rxe_port_up(rxe); > - else > - rxe_port_down(rxe); > + rxe_set_port_state(ndev, rxe); > break; > case NETDEV_REBOOT: > case NETDEV_GOING_DOWN: > diff --git a/drivers/infiniband/sw/rxe/rxe_sysfs.c b/drivers/infiniband/sw/rxe/rxe_sysfs.c > index 73a19f808e1b..21c319e22ca5 100644 > --- a/drivers/infiniband/sw/rxe/rxe_sysfs.c > +++ b/drivers/infiniband/sw/rxe/rxe_sysfs.c > @@ -53,22 +53,6 @@ static int sanitize_arg(const char *val, char *intf, int intf_len) > return len; > } > > -static void rxe_set_port_state(struct net_device *ndev) > -{ > - struct rxe_dev *rxe = net_to_rxe(ndev); > - bool is_up = netif_running(ndev) && netif_carrier_ok(ndev); > - > - if (!rxe) > - goto out; > - > - if (is_up) > - rxe_port_up(rxe); > - else > - rxe_port_down(rxe); /* down for unknown state */ > -out: > - return; > -} > - > static int rxe_param_set_add(const char *val, const struct kernel_param *kp) > { > int len; > @@ -104,7 +88,7 @@ static int rxe_param_set_add(const char *val, const struct kernel_param *kp) > goto err; > } > > - rxe_set_port_state(ndev); > + rxe_set_port_state(ndev, rxe); > dev_info(&rxe->ib_dev.dev, "added %s\n", intf); > err: > if (ndev)
On Thu, Dec 06, 2018 at 09:03:56PM +0800, Yanjun Zhu wrote: > > On 2018/12/6 19:01, Yuval Shaia wrote: > > Same code is executed in both rxe_param_set_add and rxe_notify > > functions. > > Make one function and call it from both places. > > > > Note that the code that checks validity of rxe object is omitted since > > both callers already make sure it is valid. > > > > Signed-off-by: Yuval Shaia <yuval.shaia@oracle.com> > > --- > > drivers/infiniband/sw/rxe/rxe.h | 1 + > > drivers/infiniband/sw/rxe/rxe_net.c | 13 +++++++++---- > > drivers/infiniband/sw/rxe/rxe_sysfs.c | 18 +----------------- > > 3 files changed, 11 insertions(+), 21 deletions(-) > > > > diff --git a/drivers/infiniband/sw/rxe/rxe.h b/drivers/infiniband/sw/rxe/rxe.h > > index 8f79bd86d033..cd40a3062551 100644 > > --- a/drivers/infiniband/sw/rxe/rxe.h > > +++ b/drivers/infiniband/sw/rxe/rxe.h > > @@ -110,5 +110,6 @@ struct rxe_dev *get_rxe_by_name(const char *name); > > void rxe_port_up(struct rxe_dev *rxe); > > void rxe_port_down(struct rxe_dev *rxe); > > +void rxe_set_port_state(struct net_device *ndev, struct rxe_dev *rxe); > > #endif /* RXE_H */ > > diff --git a/drivers/infiniband/sw/rxe/rxe_net.c b/drivers/infiniband/sw/rxe/rxe_net.c > > index b26a8141f3ed..467329f69300 100644 > > --- a/drivers/infiniband/sw/rxe/rxe_net.c > > +++ b/drivers/infiniband/sw/rxe/rxe_net.c > > @@ -625,6 +625,14 @@ void rxe_port_down(struct rxe_dev *rxe) > > dev_info(&rxe->ib_dev.dev, "set down\n"); > > } > > +void rxe_set_port_state(struct net_device *ndev, struct rxe_dev *rxe) > > an inline function is better? > > Zhu Yanjun Thanks, i just followed the convention used in rxe_port_up and rxe_port_down plus the fact that original rxe_set_port_state was not. > > > > +{ > > + if (netif_running(ndev) && netif_carrier_ok(ndev)) > > + rxe_port_up(rxe); > > + else > > + rxe_port_down(rxe); > > +} > > + > > static int rxe_notify(struct notifier_block *not_blk, > > unsigned long event, > > void *arg) > > @@ -651,10 +659,7 @@ static int rxe_notify(struct notifier_block *not_blk, > > rxe_set_mtu(rxe, ndev->mtu); > > break; > > case NETDEV_CHANGE: > > - if (netif_running(ndev) && netif_carrier_ok(ndev)) > > - rxe_port_up(rxe); > > - else > > - rxe_port_down(rxe); > > + rxe_set_port_state(ndev, rxe); > > break; > > case NETDEV_REBOOT: > > case NETDEV_GOING_DOWN: > > diff --git a/drivers/infiniband/sw/rxe/rxe_sysfs.c b/drivers/infiniband/sw/rxe/rxe_sysfs.c > > index 73a19f808e1b..21c319e22ca5 100644 > > --- a/drivers/infiniband/sw/rxe/rxe_sysfs.c > > +++ b/drivers/infiniband/sw/rxe/rxe_sysfs.c > > @@ -53,22 +53,6 @@ static int sanitize_arg(const char *val, char *intf, int intf_len) > > return len; > > } > > -static void rxe_set_port_state(struct net_device *ndev) > > -{ > > - struct rxe_dev *rxe = net_to_rxe(ndev); > > - bool is_up = netif_running(ndev) && netif_carrier_ok(ndev); > > - > > - if (!rxe) > > - goto out; > > - > > - if (is_up) > > - rxe_port_up(rxe); > > - else > > - rxe_port_down(rxe); /* down for unknown state */ > > -out: > > - return; > > -} > > - > > static int rxe_param_set_add(const char *val, const struct kernel_param *kp) > > { > > int len; > > @@ -104,7 +88,7 @@ static int rxe_param_set_add(const char *val, const struct kernel_param *kp) > > goto err; > > } > > - rxe_set_port_state(ndev); > > + rxe_set_port_state(ndev, rxe); > > dev_info(&rxe->ib_dev.dev, "added %s\n", intf); > > err: > > if (ndev)
On 12/6/2018 5:01 AM, Yuval Shaia wrote: > Same code is executed in both rxe_param_set_add and rxe_notify > functions. > Make one function and call it from both places. > > Note that the code that checks validity of rxe object is omitted since > both callers already make sure it is valid. > > Signed-off-by: Yuval Shaia <yuval.shaia@oracle.com> Reviewed-by: Steve Wise <swise@opengridcomputing.com> I need this patch as part of my newlink/dellink submission [1]. It fixes a bug in my reference logic by the fact that it removes the call to net_to_rxe(). May I include it in my patch series? That way we don't create a dependency until both merge. [1] https://www.spinics.net/lists/linux-rdma/msg71873.html Thanks, Steve
On Thu, Dec 06, 2018 at 10:13:30AM -0600, Steve Wise wrote: > > On 12/6/2018 5:01 AM, Yuval Shaia wrote: > > Same code is executed in both rxe_param_set_add and rxe_notify > > functions. > > Make one function and call it from both places. > > > > Note that the code that checks validity of rxe object is omitted since > > both callers already make sure it is valid. > > > > Signed-off-by: Yuval Shaia <yuval.shaia@oracle.com> > > Reviewed-by: Steve Wise <swise@opengridcomputing.com> > > I need this patch as part of my newlink/dellink submission [1]. It > fixes a bug in my reference logic by the fact that it removes the call > to net_to_rxe(). May I include it in my patch series? That way we > don't create a dependency until both merge. > > [1] https://www.spinics.net/lists/linux-rdma/msg71873.html Sure, thanks. > > Thanks, > > Steve > >
On Thu, 2018-12-06 at 21:03 +0800, Yanjun Zhu wrote: > On 2018/12/6 19:01, Yuval Shaia wrote: > > +void rxe_set_port_state(struct net_device *ndev, struct rxe_dev *rxe) > > an inline function is better? What makes you think that declaring this function 'inline' would be better? The compiler should be smart enough to figure out that this function can be inlined without declaring it 'inline'. Bart.
On Thu, 2018-12-06 at 13:01 +0200, Yuval Shaia wrote: > Same code is executed in both rxe_param_set_add and rxe_notify > functions. > Make one function and call it from both places. > > Note that the code that checks validity of rxe object is omitted since > both callers already make sure it is valid. Reviewed-by: Bart Van Assche <bvanassche@acm.org>
On 2018/12/7 1:49, Bart Van Assche wrote: > On Thu, 2018-12-06 at 21:03 +0800, Yanjun Zhu wrote: >> On 2018/12/6 19:01, Yuval Shaia wrote: >>> +void rxe_set_port_state(struct net_device *ndev, struct rxe_dev *rxe) >> an inline function is better? > What makes you think that declaring this function 'inline' would be better? > The compiler should be smart enough to figure out that this function can be From what compiler version, the compile is smart enough to figure out inline? Zhu Yanjun > inlined without declaring it 'inline'. > > Bart.
On Thu, Dec 06, 2018 at 10:13:30AM -0600, Steve Wise wrote: > > On 12/6/2018 5:01 AM, Yuval Shaia wrote: > > Same code is executed in both rxe_param_set_add and rxe_notify > > functions. > > Make one function and call it from both places. > > > > Note that the code that checks validity of rxe object is omitted since > > both callers already make sure it is valid. > > > > Signed-off-by: Yuval Shaia <yuval.shaia@oracle.com> > > Reviewed-by: Steve Wise <swise@opengridcomputing.com> > > I need this patch as part of my newlink/dellink submission [1]. It > fixes a bug in my reference logic by the fact that it removes the call > to net_to_rxe(). May I include it in my patch series? That way we > don't create a dependency until both merge. > > [1] https://www.spinics.net/lists/linux-rdma/msg71873.html > > Thanks, > > Steve Please add Bart's r-b > >
On 12/9/2018 4:53 AM, Yuval Shaia wrote: > On Thu, Dec 06, 2018 at 10:13:30AM -0600, Steve Wise wrote: >> On 12/6/2018 5:01 AM, Yuval Shaia wrote: >>> Same code is executed in both rxe_param_set_add and rxe_notify >>> functions. >>> Make one function and call it from both places. >>> >>> Note that the code that checks validity of rxe object is omitted since >>> both callers already make sure it is valid. >>> >>> Signed-off-by: Yuval Shaia <yuval.shaia@oracle.com> >> Reviewed-by: Steve Wise <swise@opengridcomputing.com> >> >> I need this patch as part of my newlink/dellink submission [1]. It >> fixes a bug in my reference logic by the fact that it removes the call >> to net_to_rxe(). May I include it in my patch series? That way we >> don't create a dependency until both merge. >> >> [1] https://www.spinics.net/lists/linux-rdma/msg71873.html >> >> Thanks, >> >> Steve > Please add Bart's r-b Sure.
diff --git a/drivers/infiniband/sw/rxe/rxe.h b/drivers/infiniband/sw/rxe/rxe.h index 8f79bd86d033..cd40a3062551 100644 --- a/drivers/infiniband/sw/rxe/rxe.h +++ b/drivers/infiniband/sw/rxe/rxe.h @@ -110,5 +110,6 @@ struct rxe_dev *get_rxe_by_name(const char *name); void rxe_port_up(struct rxe_dev *rxe); void rxe_port_down(struct rxe_dev *rxe); +void rxe_set_port_state(struct net_device *ndev, struct rxe_dev *rxe); #endif /* RXE_H */ diff --git a/drivers/infiniband/sw/rxe/rxe_net.c b/drivers/infiniband/sw/rxe/rxe_net.c index b26a8141f3ed..467329f69300 100644 --- a/drivers/infiniband/sw/rxe/rxe_net.c +++ b/drivers/infiniband/sw/rxe/rxe_net.c @@ -625,6 +625,14 @@ void rxe_port_down(struct rxe_dev *rxe) dev_info(&rxe->ib_dev.dev, "set down\n"); } +void rxe_set_port_state(struct net_device *ndev, struct rxe_dev *rxe) +{ + if (netif_running(ndev) && netif_carrier_ok(ndev)) + rxe_port_up(rxe); + else + rxe_port_down(rxe); +} + static int rxe_notify(struct notifier_block *not_blk, unsigned long event, void *arg) @@ -651,10 +659,7 @@ static int rxe_notify(struct notifier_block *not_blk, rxe_set_mtu(rxe, ndev->mtu); break; case NETDEV_CHANGE: - if (netif_running(ndev) && netif_carrier_ok(ndev)) - rxe_port_up(rxe); - else - rxe_port_down(rxe); + rxe_set_port_state(ndev, rxe); break; case NETDEV_REBOOT: case NETDEV_GOING_DOWN: diff --git a/drivers/infiniband/sw/rxe/rxe_sysfs.c b/drivers/infiniband/sw/rxe/rxe_sysfs.c index 73a19f808e1b..21c319e22ca5 100644 --- a/drivers/infiniband/sw/rxe/rxe_sysfs.c +++ b/drivers/infiniband/sw/rxe/rxe_sysfs.c @@ -53,22 +53,6 @@ static int sanitize_arg(const char *val, char *intf, int intf_len) return len; } -static void rxe_set_port_state(struct net_device *ndev) -{ - struct rxe_dev *rxe = net_to_rxe(ndev); - bool is_up = netif_running(ndev) && netif_carrier_ok(ndev); - - if (!rxe) - goto out; - - if (is_up) - rxe_port_up(rxe); - else - rxe_port_down(rxe); /* down for unknown state */ -out: - return; -} - static int rxe_param_set_add(const char *val, const struct kernel_param *kp) { int len; @@ -104,7 +88,7 @@ static int rxe_param_set_add(const char *val, const struct kernel_param *kp) goto err; } - rxe_set_port_state(ndev); + rxe_set_port_state(ndev, rxe); dev_info(&rxe->ib_dev.dev, "added %s\n", intf); err: if (ndev)
Same code is executed in both rxe_param_set_add and rxe_notify functions. Make one function and call it from both places. Note that the code that checks validity of rxe object is omitted since both callers already make sure it is valid. Signed-off-by: Yuval Shaia <yuval.shaia@oracle.com> --- drivers/infiniband/sw/rxe/rxe.h | 1 + drivers/infiniband/sw/rxe/rxe_net.c | 13 +++++++++---- drivers/infiniband/sw/rxe/rxe_sysfs.c | 18 +----------------- 3 files changed, 11 insertions(+), 21 deletions(-)