[RFC v3 3/6] can: make struct proto const

can_ioctl is the only reason for struct proto to be non-const.
script/check-patch.pl suggests struct proto be const.
This patch performs the necessary change.

Signed-off-by: Kurt Van Dijck <kurt.van.dijck-/BeEPy95v10@public.gmane.org>
---

diff --git a/include/linux/can/core.h b/include/linux/can/core.h
index 430c446..0767cc6 100644
--- a/include/linux/can/core.h
+++ b/include/linux/can/core.h

@@ -39,7 +39,7 @@
 struct can_proto {
 	int              type;
 	int              protocol;
-	struct proto_ops *ops;
+	const struct proto_ops *ops;
 	struct proto     *prot;
 
 	const struct rtnl_af_ops *rtnl_link_ops;

@@ -78,6 +78,8 @@ struct rtgencanmsg {
 
 extern int  can_proto_register(const struct can_proto *cp);
 extern void can_proto_unregister(const struct can_proto *cp);
+extern int can_sock_ioctl(struct socket *sock, unsigned int cmd,
+		unsigned long arg);
 
 extern int  can_rx_register(struct net_device *dev, canid_t can_id,
 			    canid_t mask,

diff --git a/net/can/af_can.c b/net/can/af_can.c
index db59c6e..c1f8c05 100644
--- a/net/can/af_can.c
+++ b/net/can/af_can.c

@@ -139,7 +139,7 @@ static inline void can_put_proto(const struct can_proto *cp)
  * af_can socket functions
  */
 
-static int can_ioctl(struct socket *sock, unsigned int cmd, unsigned long arg)
+int can_sock_ioctl(struct socket *sock, unsigned int cmd, unsigned long arg)
 {
 	struct sock *sk = sock->sk;
 

@@ -152,6 +152,7 @@ static int can_ioctl(struct socket *sock, unsigned int cmd, unsigned long arg)
 		return -ENOIOCTLCMD;
 	}
 }
+EXPORT_SYMBOL(can_sock_ioctl);
 
 static void can_sock_destruct(struct sock *sk)
 {

@@ -720,10 +721,6 @@ int can_proto_register(const struct can_proto *cp)
 		err = -EBUSY;
 	} else {
 		proto_tab[proto] = cp;
-
-		/* use generic ioctl function if not defined by module */
-		if (!cp->ops->ioctl)
-			cp->ops->ioctl = can_ioctl;
 	}
 	spin_unlock(&proto_tab_lock);
 

diff --git a/net/can/bcm.c b/net/can/bcm.c
index ac1961d..fd89542 100644
--- a/net/can/bcm.c
+++ b/net/can/bcm.c

@@ -1569,7 +1569,7 @@ static int bcm_recvmsg(struct kiocb *iocb, struct socket *sock,
 	return size;
 }
 
-static struct proto_ops bcm_ops __read_mostly = {
+static const struct proto_ops bcm_ops = {
 	.family        = PF_CAN,
 	.release       = bcm_release,
 	.bind          = sock_no_bind,

@@ -1578,7 +1578,7 @@ static struct proto_ops bcm_ops __read_mostly = {
 	.accept        = sock_no_accept,
 	.getname       = sock_no_getname,
 	.poll          = datagram_poll,
-	.ioctl         = NULL,		/* use can_ioctl() from af_can.c */
+	.ioctl         = can_sock_ioctl,
 	.listen        = sock_no_listen,
 	.shutdown      = sock_no_shutdown,
 	.setsockopt    = sock_no_setsockopt,

diff --git a/net/can/raw.c b/net/can/raw.c
index 9ad3dfc..8581596 100644
--- a/net/can/raw.c
+++ b/net/can/raw.c

@@ -742,7 +742,7 @@ static int raw_recvmsg(struct kiocb *iocb, struct socket *sock,
 	return size;
 }
 
-static struct proto_ops raw_ops __read_mostly = {
+static const struct proto_ops raw_ops = {
 	.family        = PF_CAN,
 	.release       = raw_release,
 	.bind          = raw_bind,

@@ -751,7 +751,7 @@ static struct proto_ops raw_ops __read_mostly = {
 	.accept        = sock_no_accept,
 	.getname       = raw_getname,
 	.poll          = datagram_poll,
-	.ioctl         = NULL,		/* use can_ioctl() from af_can.c */
+	.ioctl         = can_sock_ioctl,
 	.listen        = sock_no_listen,
 	.shutdown      = sock_no_shutdown,
 	.setsockopt    = raw_setsockopt,

Le lundi 14 mars 2011 à 14:47 +0100, Kurt Van Dijck a écrit :

Hmm, you actually fixed a race / bug, since we installed in proto_tab[]
a pointer to a not yet initted ops structure.

I suggest you change patch title and changelog to properly describe the
bug fix.

On Mon, Mar 14, 2011 at 03:09:15PM +0100, Eric Dumazet wrote:

Le lundi 14 mars 2011 à 14:47 +0100, Kurt Van Dijck a écrit :

quoted

can_ioctl is the only reason for struct proto to be non-const.
script/check-patch.pl suggests struct proto be const.
This patch performs the necessary change.

 static void can_sock_destruct(struct sock *sk)
 {

@@ -720,10 +721,6 @@ int can_proto_register(const struct can_proto *cp)
 		err = -EBUSY;
 	} else {
 		proto_tab[proto] = cp;
-
-		/* use generic ioctl function if not defined by module */
-		if (!cp->ops->ioctl)
-			cp->ops->ioctl = can_ioctl;

Hmm, you actually fixed a race / bug, since we installed in proto_tab[]
a pointer to a not yet initted ops structure.

I see your point.
The reason I modified this was, as described, checkpatch.pl was complaining.
Anyway, this sequence was protected by a spinlock 'proto_tab_lock'.
I don't think it was a race.

Did I miss something?
Kurt
_______________________________________________
Socketcan-core mailing list
Socketcan-core@lists.berlios.de
https://lists.berlios.de/mailman/listinfo/socketcan-core

Le lundi 14 mars 2011 à 16:02 +0100, Kurt Van Dijck a écrit :

I see your point.
The reason I modified this was, as described, checkpatch.pl was complaining.
Anyway, this sequence was protected by a spinlock 'proto_tab_lock'.
I don't think it was a race.

Did I miss something?

As soon as proto_tab[proto] = cp; is done, another thread on another cpu
can read the pointer and follow it.

Hmm, I missed can_create() also uses the spinlock protection, so you're
probably right.

It seems a bit overkill :(
phonet uses RCU for example.

On Mon, Mar 14, 2011 at 05:42:23PM +0100, Eric Dumazet wrote:

Le lundi 14 mars 2011 à 16:02 +0100, Kurt Van Dijck a écrit :

quoted

I see your point.
The reason I modified this was, as described, checkpatch.pl was complaining.
Anyway, this sequence was protected by a spinlock 'proto_tab_lock'.
I don't think it was a race.

Did I miss something?

As soon as proto_tab[proto] = cp; is done, another thread on another cpu
can read the pointer and follow it.

Hmm, I missed can_create() also uses the spinlock protection, so you're
probably right.

It seems a bit overkill :(
phonet uses RCU for example.

I'll keep that in mind.
Switching CAN protos to RCU is far beyond to goal of this patch :-)

Thanks,
Kurt
_______________________________________________
Socketcan-core mailing list
Socketcan-core@lists.berlios.de
https://lists.berlios.de/mailman/listinfo/socketcan-core

On 14.03.2011 18:17, Kurt Van Dijck wrote:

On Mon, Mar 14, 2011 at 05:42:23PM +0100, Eric Dumazet wrote:

quoted

Le lundi 14 mars 2011 à 16:02 +0100, Kurt Van Dijck a écrit :

quoted

I see your point.
The reason I modified this was, as described, checkpatch.pl was complaining.
Anyway, this sequence was protected by a spinlock 'proto_tab_lock'.
I don't think it was a race.

Did I miss something?

As soon as proto_tab[proto] = cp; is done, another thread on another cpu
can read the pointer and follow it.

Hmm, I missed can_create() also uses the spinlock protection, so you're
probably right.

It seems a bit overkill :(
phonet uses RCU for example.

Hello Eric,

the RCU code in phonet is worth to consider. But loading and removing of
protocols is far away from being a hot path ... what would be the advantages
to move to RCU here?

I'll keep that in mind.
Switching CAN protos to RCU is far beyond to goal of this patch :-)

Anyway making the struct proto a const (and set the default defines in the CAN
protocols) is a nice cleanup. Let's wait for the feedback about the RCU from
Eric ... then this patch could be put into mainline independently from the
j1939 protocol.

Thanks,
Oliver

Le mardi 15 mars 2011 à 22:28 +0100, Oliver Hartkopp a écrit :

Hello Eric,

the RCU code in phonet is worth to consider. But loading and removing of
protocols is far away from being a hot path ... what would be the advantages
to move to RCU here?

can_create() could avoid taking the spinlock.

I have no idea if its a critical path, I guess not.