| Message ID | 1466673741-23115-3-git-send-email-mkl@pengutronix.de |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
Hello. On 6/23/2016 12:22 PM, Marc Kleine-Budde wrote: > From: Oliver Hartkopp <socketcan@hartkopp.net> > > For 'real' hardware CAN devices the netlink interface is used to set CAN > specific communication parameters. Real CAN hardware can not be created nor > removed with the ip tool ... > > This patch adds a private dellink function for the CAN device driver interface > that does just nothing. > > It's a follow up to commit 993e6f2fd ("can: fix oops caused by wrong rtnl > newlink usage") but for dellink. > > Reported-by: ajneu <ajneu1@gmail.com> > Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net> > Cc: <stable@vger.kernel.org> > Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de> > --- > drivers/net/can/dev.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c > index 348dd5001fa4..ad535a854e5c 100644 > --- a/drivers/net/can/dev.c > +++ b/drivers/net/can/dev.c > @@ -1011,6 +1011,11 @@ static int can_newlink(struct net *src_net, struct net_device *dev, > return -EOPNOTSUPP; > } > > +static void can_dellink(struct net_device *dev, struct list_head *head) > +{ > + return; Why? > +} > + > static struct rtnl_link_ops can_link_ops __read_mostly = { > .kind = "can", > .maxtype = IFLA_CAN_MAX, [...] MBR, Sergei
On 06/23/2016 02:55 PM, Sergei Shtylyov wrote: > Hello. > > On 6/23/2016 12:22 PM, Marc Kleine-Budde wrote: > >> From: Oliver Hartkopp <socketcan@hartkopp.net> >> >> For 'real' hardware CAN devices the netlink interface is used to set CAN >> specific communication parameters. Real CAN hardware can not be >> created nor >> removed with the ip tool ... >> >> This patch adds a private dellink function for the CAN device driver >> interface >> that does just nothing. >> >> It's a follow up to commit 993e6f2fd ("can: fix oops caused by wrong rtnl >> newlink usage") but for dellink. >> >> Reported-by: ajneu <ajneu1@gmail.com> >> Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net> >> Cc: <stable@vger.kernel.org> >> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de> >> --- >> drivers/net/can/dev.c | 6 ++++++ >> 1 file changed, 6 insertions(+) >> >> diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c >> index 348dd5001fa4..ad535a854e5c 100644 >> --- a/drivers/net/can/dev.c >> +++ b/drivers/net/can/dev.c >> @@ -1011,6 +1011,11 @@ static int can_newlink(struct net *src_net, >> struct net_device *dev, >> return -EOPNOTSUPP; >> } >> >> +static void can_dellink(struct net_device *dev, struct list_head *head) >> +{ >> + return; > > Why? > http://marc.info/?l=linux-can&m=146651600421205&w=2 The same reason as for commit 993e6f2fd. Regards, Oliver >> +} >> + >> static struct rtnl_link_ops can_link_ops __read_mostly = { >> .kind = "can", >> .maxtype = IFLA_CAN_MAX, > [...] > > MBR, Sergei > > -- > To unsubscribe from this list: send the line "unsubscribe linux-can" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
On 6/23/2016 4:01 PM, Oliver Hartkopp wrote: >>> From: Oliver Hartkopp <socketcan@hartkopp.net> >>> >>> For 'real' hardware CAN devices the netlink interface is used to set CAN >>> specific communication parameters. Real CAN hardware can not be >>> created nor >>> removed with the ip tool ... >>> >>> This patch adds a private dellink function for the CAN device driver >>> interface >>> that does just nothing. >>> >>> It's a follow up to commit 993e6f2fd ("can: fix oops caused by wrong rtnl >>> newlink usage") but for dellink. >>> >>> Reported-by: ajneu <ajneu1@gmail.com> >>> Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net> >>> Cc: <stable@vger.kernel.org> >>> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de> >>> --- >>> drivers/net/can/dev.c | 6 ++++++ >>> 1 file changed, 6 insertions(+) >>> >>> diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c >>> index 348dd5001fa4..ad535a854e5c 100644 >>> --- a/drivers/net/can/dev.c >>> +++ b/drivers/net/can/dev.c >>> @@ -1011,6 +1011,11 @@ static int can_newlink(struct net *src_net, >>> struct net_device *dev, >>> return -EOPNOTSUPP; >>> } >>> >>> +static void can_dellink(struct net_device *dev, struct list_head *head) >>> +{ >>> + return; >> >> Why? >> > > http://marc.info/?l=linux-can&m=146651600421205&w=2 > > The same reason as for commit 993e6f2fd. I was asking just about the useless *return* statement... > Regards, > Oliver MBR, Sergei
On 06/23/2016 03:09 PM, Sergei Shtylyov wrote: >>>> +static void can_dellink(struct net_device *dev, struct list_head >>>> *head) >>>> +{ >>>> + return; >>> >>> Why? >>> >> >> http://marc.info/?l=linux-can&m=146651600421205&w=2 >> >> The same reason as for commit 993e6f2fd. > > I was asking just about the useless *return* statement... > Ah! I did some investigation before whether using 'return' in empty void functions or not. static void can_dellink(struct net_device *dev, struct list_head *head); and static void can_dellink(struct net_device *dev, struct list_head *head) { return; } do the same job, right? But the first one looks like a forward declaration and you would try to find the 'implementing' function then. Of course you can write less code and both implementations are correct - but this representation makes it pretty clear that here's nothing to do :-) Regards, Oliver
> static void can_dellink(struct net_device *dev, struct list_head *head); > > and > > static void can_dellink(struct net_device *dev, struct list_head *head) > { > return; > } Wouldn't the canonical form be this: static void can_dellink(struct net_device *dev, struct list_head *head) { } - the curly braces make sure this isn't a forward definition - but no useless return either But then again, this "return" is only cosmetical. No compiler will generate any code from it.
On 06/28/2016 09:36 AM, Holger Schurig wrote: >> static void can_dellink(struct net_device *dev, struct list_head *head); >> >> and >> >> static void can_dellink(struct net_device *dev, struct list_head *head) >> { >> return; >> } > > Wouldn't the canonical form be this: > > static void can_dellink(struct net_device *dev, struct list_head *head) > { > } > > > - the curly braces make sure this isn't a forward definition > - but no useless return either > > > But then again, this "return" is only cosmetical. Yes it is just coding style. > No compiler will > generate any code from it. ACK. If you check ~/linux$ git grep \{\ return\; there are many occurrences of empty void functions having a 'return' inside the curly braces. I think static void can_dellink( ... ){} would have made it too. Now can_dellink() just locks similar to can_newlink() some lines above. Regards, Oliver
diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c index 348dd5001fa4..ad535a854e5c 100644 --- a/drivers/net/can/dev.c +++ b/drivers/net/can/dev.c @@ -1011,6 +1011,11 @@ static int can_newlink(struct net *src_net, struct net_device *dev, return -EOPNOTSUPP; } +static void can_dellink(struct net_device *dev, struct list_head *head) +{ + return; +} + static struct rtnl_link_ops can_link_ops __read_mostly = { .kind = "can", .maxtype = IFLA_CAN_MAX, @@ -1019,6 +1024,7 @@ static struct rtnl_link_ops can_link_ops __read_mostly = { .validate = can_validate, .newlink = can_newlink, .changelink = can_changelink, + .dellink = can_dellink, .get_size = can_get_size, .fill_info = can_fill_info, .get_xstats_size = can_get_xstats_size,