IrDA 2.6.28 bug fix

RFC PATCH In review Samuel Ortiz 3 participants 13 messages Dec 15, 2008 – Dec 17, 2008 1 message not in the archive

Subsystems (2)

Networking Network drivers

lore

Share & tools

lore

Samuel Ortiz

author

2008-12-15 02:03 UTC #0 lore raw

cover IrDA 2.6.28 bug fix

Hi Dave,

This is a 9 patches series for IrDA, against your net-2.6 tree. This is an attempt to fix kernel.bugzilla.org bug #11795, where we noticed skb->cb could be altered once submitted to dev_queue_xmit. Since IrDA is using this callback to pass per-skb information, we are now stuffing it in front of skb->data, after allocating the right headroom.

Another solution would be to play with the IrDA physical header and skb_pull our skbs whenever we are physically transmitting the data.

Cheers,
Samuel.

Samuel Ortiz

author

2008-12-15 02:03 UTC #1 lore raw

RFC PATCH 1/9 irda: Introduce irda_alloc_skb

We want to have extra space for the irda_skb_cb structure in front of the actual IrDA data.
It's currently sitting in skb->cb, but that pointer can be altered after hitting dev_queue_xmit. So, an additional skb header is a safer place.

Signed-off-by Samuel Ortiz <[email protected]>

---
 include/net/irda/irda.h         |    1 +
 include/net/irda/irda_device.h  |    6 +++
 net/irda/af_irda.c              |    4 +-
 net/irda/ircomm/ircomm_lmp.c    |    4 +-
 net/irda/ircomm/ircomm_param.c  |    2 +-
 net/irda/ircomm/ircomm_tty.c    |    6 ++--
 net/irda/irda_device.c          |   12 ++++++
 net/irda/iriap.c                |   10 +++---
 net/irda/iriap_event.c          |    2 +-
 net/irda/irlan/irlan_common.c   |   77 +++++++++++++++++++++------------------
 net/irda/irlan/irlan_provider.c |   18 ++++++----
 net/irda/irlap.c                |    2 +-
 net/irda/irlap_frame.c          |   28 ++++++++-------
 net/irda/irlmp.c                |    2 +-
 net/irda/irttp.c                |   14 ++++----
 15 files changed, 110 insertions(+), 78 deletions(-)

include/net/irda/irda.h +1 -0

index 7e58206..ce510aa 100644
@@ -132,4 +132,5 @@ extern int irlap_driver_rcv(struct sk_buff *skb, struct net_device *dev,
 			    struct packet_type *ptype,
 			    struct net_device *orig_dev);
 
+extern struct sk_buff *irda_alloc_skb(unsigned int size, gfp_t priority);
 #endif /* NET_IRDA_H */

include/net/irda/irda_device.h +6 -0

index 3025ae1..305eb05 100644
@@ -230,6 +230,12 @@ struct net_device *alloc_irdadev(int sizeof_priv);
 
 void irda_setup_dma(int channel, dma_addr_t buffer, int count, int mode);
 
+
+static inline struct irda_skb_cb *irda_get_skb_cb(struct sk_buff *skb)
+{
+	return (struct irda_skb_cb *)(skb->data - sizeof(struct irda_skb_cb));
+}
+
 /*
  * Function irda_get_mtt (skb)
  *

net/irda/af_irda.c +2 -2

index 3eb5bcc..61def98 100644
@@ -304,8 +304,8 @@ static void irda_connect_response(struct irda_sock *self)
 
 	IRDA_DEBUG(2, "%s()\n", __func__);
 
-	skb = alloc_skb(TTP_MAX_HEADER + TTP_SAR_HEADER,
-			GFP_ATOMIC);
+	skb = irda_alloc_skb(TTP_MAX_HEADER + TTP_SAR_HEADER,
+			     GFP_ATOMIC);
 	if (skb == NULL) {
 		IRDA_DEBUG(0, "%s() Unable to allocate sk_buff!\n",
 			   __func__);

net/irda/ircomm/ircomm_lmp.c +2 -2

index 67c99d2..407bad3 100644
@@ -80,7 +80,7 @@ static int ircomm_lmp_connect_response(struct ircomm_cb *self,
 
 	/* Any userdata supplied? */
 	if (userdata == NULL) {
-		tx_skb = alloc_skb(LMP_MAX_HEADER, GFP_ATOMIC);
+		tx_skb = irda_alloc_skb(LMP_MAX_HEADER, GFP_ATOMIC);
 		if (!tx_skb)
 			return -ENOMEM;

@@ -114,7 +114,7 @@ static int ircomm_lmp_disconnect_request(struct ircomm_cb *self,
 	IRDA_DEBUG(0, "%s()\n", __func__ );
 
 	if (!userdata) {
-		tx_skb = alloc_skb(LMP_MAX_HEADER, GFP_ATOMIC);
+		tx_skb = irda_alloc_skb(LMP_MAX_HEADER, GFP_ATOMIC);
 		if (!tx_skb)
 			return -ENOMEM;

net/irda/ircomm/ircomm_param.c +1 -1

index d57aefd..e62979b 100644
@@ -120,7 +120,7 @@ int ircomm_param_request(struct ircomm_tty_cb *self, __u8 pi, int flush)
 
 	skb = self->ctrl_skb;
 	if (!skb) {
-		skb = alloc_skb(256, GFP_ATOMIC);
+		skb = irda_alloc_skb(256, GFP_ATOMIC);
 		if (!skb) {
 			spin_unlock_irqrestore(&self->spinlock, flags);
 			return -ENOMEM;

net/irda/ircomm/ircomm_tty.c +3 -3

index e4e2cae..fa62490 100644
@@ -752,9 +752,9 @@ static int ircomm_tty_write(struct tty_struct *tty,
 			}
 		} else {
 			/* Prepare a full sized frame */
-			skb = alloc_skb(self->max_data_size+
-					self->max_header_size,
-					GFP_ATOMIC);
+			skb = irda_alloc_skb(self->max_data_size+
+					     self->max_header_size,
+					     GFP_ATOMIC);
 			if (!skb) {
 				spin_unlock_irqrestore(&self->spinlock, flags);
 				return -ENOBUFS;

net/irda/irda_device.c +12 -0

index ea319e3..5271015 100644
@@ -296,6 +296,18 @@ struct net_device *alloc_irdadev(int sizeof_priv)
 }
 EXPORT_SYMBOL(alloc_irdadev);
 
+struct sk_buff *irda_alloc_skb(unsigned int size, gfp_t priority)
+{
+	struct sk_buff *skb;
+
+	skb = alloc_skb(size + sizeof(struct irda_skb_cb), priority);
+	if (likely(skb))
+		skb_reserve(skb, sizeof(struct irda_skb_cb));
+
+	return skb;
+}
+EXPORT_SYMBOL(irda_alloc_skb);
+
 #ifdef CONFIG_ISA_DMA_API
 /*
  * Function setup_dma (idev, buffer, count, mode)

net/irda/iriap.c +5 -5

index 4a105dc..dbd91bf 100644
@@ -345,7 +345,7 @@ static void iriap_disconnect_request(struct iriap_cb *self)
 	IRDA_ASSERT(self != NULL, return;);
 	IRDA_ASSERT(self->magic == IAS_MAGIC, return;);
 
-	tx_skb = alloc_skb(LMP_MAX_HEADER, GFP_ATOMIC);
+	tx_skb = irda_alloc_skb(LMP_MAX_HEADER, GFP_ATOMIC);
 	if (tx_skb == NULL) {
 		IRDA_DEBUG(0,
 			   "%s(), Could not allocate an sk_buff of length %d\n",

@@ -397,7 +397,7 @@ int iriap_getvaluebyclass_request(struct iriap_cb *self,
 	attr_len = strlen(attr);	/* Up to IAS_MAX_ATTRIBNAME = 60 */
 
 	skb_len = self->max_header_size+2+name_len+1+attr_len+4;
-	tx_skb = alloc_skb(skb_len, GFP_ATOMIC);
+	tx_skb = irda_alloc_skb(skb_len, GFP_ATOMIC);
 	if (!tx_skb)
 		return -ENOMEM;

@@ -565,8 +565,8 @@ static void iriap_getvaluebyclass_response(struct iriap_cb *self,
 	 *  value. We add 32 bytes because of the 6 bytes for the frame and
 	 *  max 5 bytes for the value coding.
 	 */
-	tx_skb = alloc_skb(value->len + self->max_header_size + 32,
-			   GFP_ATOMIC);
+	tx_skb = irda_alloc_skb(value->len + self->max_header_size + 32,
+				GFP_ATOMIC);
 	if (!tx_skb)
 		return;

@@ -704,7 +704,7 @@ void iriap_send_ack(struct iriap_cb *self)
 	IRDA_ASSERT(self != NULL, return;);
 	IRDA_ASSERT(self->magic == IAS_MAGIC, return;);
 
-	tx_skb = alloc_skb(LMP_MAX_HEADER + 1, GFP_ATOMIC);
+	tx_skb = irda_alloc_skb(LMP_MAX_HEADER + 1, GFP_ATOMIC);
 	if (!tx_skb)
 		return;

net/irda/iriap_event.c +1 -1

index a301cbd..87cd337 100644
@@ -365,7 +365,7 @@ static void state_r_disconnect(struct iriap_cb *self, IRIAP_EVENT event,
 
 	switch (event) {
 	case IAP_LM_CONNECT_INDICATION:
-		tx_skb = alloc_skb(LMP_MAX_HEADER, GFP_ATOMIC);
+		tx_skb = irda_alloc_skb(LMP_MAX_HEADER, GFP_ATOMIC);
 		if (tx_skb == NULL) {
 			IRDA_WARNING("%s: unable to malloc!\n", __func__);
 			return;

net/irda/irlan/irlan_common.c +42 ~~-35~~

125 lines not on this page · raw message

net/irda/irlan/irlan_provider.c +11 -7

index 3f81f81..4d8ce7f 100644
@@ -296,13 +296,17 @@ void irlan_provider_send_reply(struct irlan_cb *self, int command,
 	IRDA_ASSERT(self != NULL, return;);
 	IRDA_ASSERT(self->magic == IRLAN_MAGIC, return;);
 
-	skb = alloc_skb(IRLAN_MAX_HEADER + IRLAN_CMD_HEADER +
-			/* Bigger param length comes from CMD_GET_MEDIA_CHAR */
-			IRLAN_STRING_PARAMETER_LEN("FILTER_TYPE", "DIRECTED") +
-			IRLAN_STRING_PARAMETER_LEN("FILTER_TYPE", "BORADCAST") +
-			IRLAN_STRING_PARAMETER_LEN("FILTER_TYPE", "MULTICAST") +
-			IRLAN_STRING_PARAMETER_LEN("ACCESS_TYPE", "HOSTED"),
-			GFP_ATOMIC);
+	skb = irda_alloc_skb(IRLAN_MAX_HEADER + IRLAN_CMD_HEADER +
+			     /* Bigger param length from CMD_GET_MEDIA_CHAR */
+			     IRLAN_STRING_PARAMETER_LEN("FILTER_TYPE",
+							"DIRECTED") +
+			     IRLAN_STRING_PARAMETER_LEN("FILTER_TYPE",
+							"BORADCAST") +
+			     IRLAN_STRING_PARAMETER_LEN("FILTER_TYPE",
+							"MULTICAST") +
+			     IRLAN_STRING_PARAMETER_LEN("ACCESS_TYPE",
+							"HOSTED"),
+			     GFP_ATOMIC);
 
 	if (!skb)
 		return;

net/irda/irlap.c +1 -1

index e4965b7..4139e7a 100644
@@ -881,7 +881,7 @@ static void irlap_change_speed(struct irlap_cb *self, __u32 speed, int now)
 	/* Change speed now, or just piggyback speed on frames */
 	if (now) {
 		/* Send down empty frame to trigger speed change */
-		skb = alloc_skb(0, GFP_ATOMIC);
+		skb = irda_alloc_skb(0, GFP_ATOMIC);
 		if (skb)
 			irlap_queue_xmit(self, skb);
 	}

net/irda/irlap_frame.c +15 ~~-13~~

index f17b65a..bf41ebf 100644
@@ -126,9 +126,9 @@ void irlap_send_snrm_frame(struct irlap_cb *self, struct qos_info *qos)
 	IRDA_ASSERT(self->magic == LAP_MAGIC, return;);
 
 	/* Allocate frame */
-	tx_skb = alloc_skb(sizeof(struct snrm_frame) +
-			   IRLAP_NEGOCIATION_PARAMS_LEN,
-			   GFP_ATOMIC);
+	tx_skb = irda_alloc_skb(sizeof(struct snrm_frame) +
+				IRLAP_NEGOCIATION_PARAMS_LEN,
+				GFP_ATOMIC);
 	if (!tx_skb)
 		return;

@@ -221,9 +221,9 @@ void irlap_send_ua_response_frame(struct irlap_cb *self, struct qos_info *qos)
 	IRDA_ASSERT(self->magic == LAP_MAGIC, return;);
 
 	/* Allocate frame */
-	tx_skb = alloc_skb(sizeof(struct ua_frame) +
-			   IRLAP_NEGOCIATION_PARAMS_LEN,
-			   GFP_ATOMIC);
+	tx_skb = irda_alloc_skb(sizeof(struct ua_frame) +
+				IRLAP_NEGOCIATION_PARAMS_LEN,
+				GFP_ATOMIC);
 	if (!tx_skb)
 		return;

@@ -263,7 +263,7 @@ void irlap_send_dm_frame( struct irlap_cb *self)
 	IRDA_ASSERT(self != NULL, return;);
 	IRDA_ASSERT(self->magic == LAP_MAGIC, return;);
 
-	tx_skb = alloc_skb(sizeof(struct dm_frame), GFP_ATOMIC);
+	tx_skb = irda_alloc_skb(sizeof(struct dm_frame), GFP_ATOMIC);
 	if (!tx_skb)
 		return;

@@ -295,7 +295,7 @@ void irlap_send_disc_frame(struct irlap_cb *self)
 	IRDA_ASSERT(self != NULL, return;);
 	IRDA_ASSERT(self->magic == LAP_MAGIC, return;);
 
-	tx_skb = alloc_skb(sizeof(struct disc_frame), GFP_ATOMIC);
+	tx_skb = irda_alloc_skb(sizeof(struct disc_frame), GFP_ATOMIC);
 	if (!tx_skb)
 		return;

@@ -328,8 +328,9 @@ void irlap_send_discovery_xid_frame(struct irlap_cb *self, int S, __u8 s,
 	IRDA_ASSERT(self->magic == LAP_MAGIC, return;);
 	IRDA_ASSERT(discovery != NULL, return;);
 
-	tx_skb = alloc_skb(sizeof(struct xid_frame) + IRLAP_DISCOVERY_INFO_LEN,
-			   GFP_ATOMIC);
+	tx_skb = irda_alloc_skb(sizeof(struct xid_frame) +
+				IRLAP_DISCOVERY_INFO_LEN,
+				GFP_ATOMIC);
 	if (!tx_skb)
 		return;

@@ -589,7 +590,7 @@ void irlap_send_rr_frame(struct irlap_cb *self, int command)
 	struct sk_buff *tx_skb;
 	struct rr_frame *frame;
 
-	tx_skb = alloc_skb(sizeof(struct rr_frame), GFP_ATOMIC);
+	tx_skb = irda_alloc_skb(sizeof(struct rr_frame), GFP_ATOMIC);
 	if (!tx_skb)
 		return;

@@ -614,7 +615,7 @@ void irlap_send_rd_frame(struct irlap_cb *self)
 	struct sk_buff *tx_skb;
 	struct rd_frame *frame;
 
-	tx_skb = alloc_skb(sizeof(struct rd_frame), GFP_ATOMIC);
+	tx_skb = irda_alloc_skb(sizeof(struct rd_frame), GFP_ATOMIC);
 	if (!tx_skb)
 		return;

@@ -1231,7 +1232,8 @@ void irlap_send_test_frame(struct irlap_cb *self, __u8 caddr, __u32 daddr,
 	struct test_frame *frame;
 	__u8 *info;
 
-	tx_skb = alloc_skb(cmd->len + sizeof(struct test_frame), GFP_ATOMIC);
+	tx_skb = irda_alloc_skb(cmd->len + sizeof(struct test_frame),
+				GFP_ATOMIC);
 	if (!tx_skb)
 		return;

net/irda/irlmp.c +1 -1

index 7bf5b91..18188f2 100644
@@ -396,7 +396,7 @@ int irlmp_connect_request(struct lsap_cb *self, __u8 dlsap_sel,
 
 	/* Any userdata? */
 	if (tx_skb == NULL) {
-		tx_skb = alloc_skb(LMP_MAX_HEADER, GFP_ATOMIC);
+		tx_skb = irda_alloc_skb(LMP_MAX_HEADER, GFP_ATOMIC);
 		if (!tx_skb)
 			return -ENOMEM;

net/irda/irttp.c +7 -7

index 74e439e..4b913de 100644
@@ -306,8 +306,8 @@ static inline void irttp_fragment_skb(struct tsap_cb *self,
 		IRDA_DEBUG(2, "%s(), fragmenting ...\n", __func__);
 
 		/* Make new segment */
-		frag = alloc_skb(self->max_seg_size+self->max_header_size,
-				 GFP_ATOMIC);
+		frag = irda_alloc_skb(self->max_seg_size+self->max_header_size,
+				      GFP_ATOMIC);
 		if (!frag)
 			return;

@@ -817,7 +817,7 @@ static inline void irttp_give_credit(struct tsap_cb *self)
 		   self->send_credit, self->avail_credit, self->remote_credit);
 
 	/* Give credit to peer */
-	tx_skb = alloc_skb(TTP_MAX_HEADER, GFP_ATOMIC);
+	tx_skb = irda_alloc_skb(TTP_MAX_HEADER, GFP_ATOMIC);
 	if (!tx_skb)
 		return;

@@ -1106,8 +1106,8 @@ int irttp_connect_request(struct tsap_cb *self, __u8 dtsap_sel,
 
 	/* Any userdata supplied? */
 	if (userdata == NULL) {
-		tx_skb = alloc_skb(TTP_MAX_HEADER + TTP_SAR_HEADER,
-				   GFP_ATOMIC);
+		tx_skb = irda_alloc_skb(TTP_MAX_HEADER + TTP_SAR_HEADER,
+					GFP_ATOMIC);
 		if (!tx_skb)
 			return -ENOMEM;

@@ -1355,7 +1355,7 @@ int irttp_connect_response(struct tsap_cb *self, __u32 max_sdu_size,
 
 	/* Any userdata supplied? */
 	if (userdata == NULL) {
-		tx_skb = alloc_skb(TTP_MAX_HEADER + TTP_SAR_HEADER,
+		tx_skb = irda_alloc_skb(TTP_MAX_HEADER + TTP_SAR_HEADER,
 				   GFP_ATOMIC);
 		if (!tx_skb)
 			return -ENOMEM;

@@ -1553,7 +1553,7 @@ int irttp_disconnect_request(struct tsap_cb *self, struct sk_buff *userdata,
 
 	if (!userdata) {
 		struct sk_buff *tx_skb;
-		tx_skb = alloc_skb(LMP_MAX_HEADER, GFP_ATOMIC);
+		tx_skb = irda_alloc_skb(LMP_MAX_HEADER, GFP_ATOMIC);
 		if (!tx_skb)
 			return -ENOMEM;

Signature · 5 lines

1.6.0.4.766.g6fc4a.dirty

-- 
Intel Open Source Technology Centre
http://oss.intel.com/

Samuel Ortiz

author

2008-12-15 02:03 UTC #2 lore raw

RFC PATCH 2/9 irda: stack should call irda_get_skb_cb()

In reply to Samuel Ortiz

Instead of accessing the irda_skb_cb directly, the stack callers should use the irda_get_skb_cb() routine.

Signed-off-by Samuel Ortiz <[email protected]>

---
 include/net/irda/irda_device.h |    8 ++++----
 net/irda/ircomm/ircomm_lmp.c   |    4 ++--
 net/irda/irlap_frame.c         |    4 +++-
 net/irda/wrapper.c             |    4 +++-
 4 files changed, 12 insertions(+), 8 deletions(-)

include/net/irda/irda_device.h +4 -4

index 305eb05..3bbf695 100644
@@ -244,7 +244,7 @@ static inline struct irda_skb_cb *irda_get_skb_cb(struct sk_buff *skb)
  */
 static inline __u16 irda_get_mtt(const struct sk_buff *skb)
 {
-	const struct irda_skb_cb *cb = (const struct irda_skb_cb *) skb->cb;
+	const struct irda_skb_cb *cb = (const struct irda_skb_cb *) skb->head;
 	return (cb->magic == LAP_MAGIC) ? cb->mtt : 10000;
 }

@@ -257,7 +257,7 @@ static inline __u16 irda_get_mtt(const struct sk_buff *skb)
  */
 static inline __u32 irda_get_next_speed(const struct sk_buff *skb)
 {
-	const struct irda_skb_cb *cb = (const struct irda_skb_cb *) skb->cb;
+	const struct irda_skb_cb *cb = (const struct irda_skb_cb *) skb->head;
 	return (cb->magic == LAP_MAGIC) ? cb->next_speed : -1;
 }

@@ -270,7 +270,7 @@ static inline __u32 irda_get_next_speed(const struct sk_buff *skb)
  */
 static inline __u16 irda_get_xbofs(const struct sk_buff *skb)
 {
-	const struct irda_skb_cb *cb = (const struct irda_skb_cb *) skb->cb;
+	const struct irda_skb_cb *cb = (const struct irda_skb_cb *) skb->head;
 	return (cb->magic == LAP_MAGIC) ? cb->xbofs : 10;
 }

@@ -283,7 +283,7 @@ static inline __u16 irda_get_xbofs(const struct sk_buff *skb)
  */
 static inline __u16 irda_get_next_xbofs(const struct sk_buff *skb)
 {
-	const struct irda_skb_cb *cb = (const struct irda_skb_cb *) skb->cb;
+	const struct irda_skb_cb *cb = (const struct irda_skb_cb *) skb->head;
 	return (cb->magic == LAP_MAGIC) ? cb->next_xbofs : -1;
 }
 #endif /* IRDA_DEVICE_H */

net/irda/ircomm/ircomm_lmp.c +2 -2

index 407bad3..c463bec 100644
@@ -146,7 +146,7 @@ static void ircomm_lmp_flow_control(struct sk_buff *skb)
 
 	IRDA_ASSERT(skb != NULL, return;);
 
-	cb = (struct irda_skb_cb *) skb->cb;
+	cb = irda_get_skb_cb(skb);
 
 	IRDA_DEBUG(2, "%s()\n", __func__ );

@@ -187,7 +187,7 @@ static int ircomm_lmp_data_request(struct ircomm_cb *self,
 
 	IRDA_ASSERT(skb != NULL, return -1;);
 
-	cb = (struct irda_skb_cb *) skb->cb;
+	cb = irda_get_skb_cb(skb);
 
 	cb->line = self->line;

net/irda/irlap_frame.c +3 -1

index bf41ebf..8760af6 100644
@@ -56,7 +56,9 @@ static void irlap_send_i_frame(struct irlap_cb *self, struct sk_buff *skb,
 static inline void irlap_insert_info(struct irlap_cb *self,
 				     struct sk_buff *skb)
 {
-	struct irda_skb_cb *cb = (struct irda_skb_cb *) skb->cb;
+	struct irda_skb_cb *cb;
+
+	cb = irda_get_skb_cb(skb);
 
 	/*
 	 * Insert MTT (min. turn time) and speed into skb, so that the

net/irda/wrapper.c +3 -1

index fd0995b..b13cb3b 100644
@@ -82,7 +82,7 @@ static inline int stuff_byte(__u8 byte, __u8 *buf)
  */
 int async_wrap_skb(struct sk_buff *skb, __u8 *tx_buff, int buffsize)
 {
-	struct irda_skb_cb *cb = (struct irda_skb_cb *) skb->cb;
+	struct irda_skb_cb *cb;
 	int xbofs;
 	int i;
 	int n;

@@ -91,6 +91,8 @@ int async_wrap_skb(struct sk_buff *skb, __u8 *tx_buff, int buffsize)
 		__u8 bytes[2];
 	} fcs;
 
+	cb = irda_get_skb_cb(skb);
+
 	/* Initialize variables */
 	fcs.value = INIT_FCS;
 	n = 0;

Signature · 5 lines

1.6.0.4.766.g6fc4a.dirty

-- 
Intel Open Source Technology Centre
http://oss.intel.com/

Samuel Ortiz

author

2008-12-15 02:03 UTC #4 lore raw

RFC PATCH 4/9 irda: reserve irda_skb on sock_alloc_send_skb() skbs

In reply to Samuel Ortiz

Those skbs are on the TX path, and need to have the irda_skb_cb space allocated as well.

Signed-off-by Samuel Ortiz <[email protected]>

---
 net/irda/af_irda.c |   17 +++++++++++------
 1 files changed, 11 insertions(+), 6 deletions(-)

net/irda/af_irda.c +11 -6

index 61def98..8b0ac27 100644
@@ -59,6 +59,7 @@
 #include <net/tcp_states.h>
 
 #include <net/irda/af_irda.h>
+#include <net/irda/irda_device.h>
 
 static int irda_create(struct net *net, struct socket *sock, int protocol);

@@ -1288,12 +1289,14 @@ static int irda_sendmsg(struct kiocb *iocb, struct socket *sock,
 		len = self->max_data_size;
 	}
 
-	skb = sock_alloc_send_skb(sk, len + self->max_header_size + 16,
+	skb = sock_alloc_send_skb(sk, len + self->max_header_size + 16
+				  + sizeof(struct irda_skb_cb),
 				  msg->msg_flags & MSG_DONTWAIT, &err);
 	if (!skb)
 		goto out_err;
 
-	skb_reserve(skb, self->max_header_size + 16);
+	skb_reserve(skb, self->max_header_size + 16
+		    + sizeof(struct irda_skb_cb));
 	skb_reset_transport_header(skb);
 	skb_put(skb, len);
 	err = memcpy_fromiovec(skb_transport_header(skb), msg->msg_iov, len);

@@ -1534,12 +1537,13 @@ static int irda_sendmsg_dgram(struct kiocb *iocb, struct socket *sock,
 		len = self->max_data_size;
 	}
 
-	skb = sock_alloc_send_skb(sk, len + self->max_header_size,
+	skb = sock_alloc_send_skb(sk, len + self->max_header_size
+				  + sizeof(struct irda_skb_cb),
 				  msg->msg_flags & MSG_DONTWAIT, &err);
 	if (!skb)
 		return -ENOBUFS;
 
-	skb_reserve(skb, self->max_header_size);
+	skb_reserve(skb, self->max_header_size + sizeof(struct irda_skb_cb));
 	skb_reset_transport_header(skb);
 
 	IRDA_DEBUG(4, "%s(), appending user data\n", __func__);

@@ -1629,12 +1633,13 @@ static int irda_sendmsg_ultra(struct kiocb *iocb, struct socket *sock,
 		len = self->max_data_size;
 	}
 
-	skb = sock_alloc_send_skb(sk, len + self->max_header_size,
+	skb = sock_alloc_send_skb(sk, len + self->max_header_size
+				  + sizeof(struct irda_skb_cb),
 				  msg->msg_flags & MSG_DONTWAIT, &err);
 	if (!skb)
 		return -ENOBUFS;
 
-	skb_reserve(skb, self->max_header_size);
+	skb_reserve(skb, self->max_header_size + sizeof(struct irda_skb_cb));
 	skb_reset_transport_header(skb);
 
 	IRDA_DEBUG(4, "%s(), appending user data\n", __func__);

Signature · 5 lines

1.6.0.4.766.g6fc4a.dirty

-- 
Intel Open Source Technology Centre
http://oss.intel.com/

Samuel Ortiz

author

2008-12-15 02:03 UTC #5 lore raw

RFC PATCH 5/9 irda: Introduce irda_dev_alloc_skb

In reply to Samuel Ortiz

Some RX skbs are cloned and then sent back to the transmitter. In order to avoid reallocation on that code path, we get more space from the beginning on the RX path.

Signed-off-by Samuel Ortiz <[email protected]>

---
 include/net/irda/irda.h |    1 +
 net/irda/irda_device.c  |   12 ++++++++++++
 2 files changed, 13 insertions(+), 0 deletions(-)

include/net/irda/irda.h +1 -0

index ce510aa..213d4c0 100644
@@ -133,4 +133,5 @@ extern int irlap_driver_rcv(struct sk_buff *skb, struct net_device *dev,
 			    struct net_device *orig_dev);
 
 extern struct sk_buff *irda_alloc_skb(unsigned int size, gfp_t priority);
+extern struct sk_buff *irda_dev_alloc_skb(unsigned int size);
 #endif /* NET_IRDA_H */

net/irda/irda_device.c +12 -0

index 5271015..62b991c 100644
@@ -308,6 +308,18 @@ struct sk_buff *irda_alloc_skb(unsigned int size, gfp_t priority)
 }
 EXPORT_SYMBOL(irda_alloc_skb);
 
+struct sk_buff *irda_dev_alloc_skb(unsigned int size)
+{
+	struct sk_buff *skb;
+
+	skb = dev_alloc_skb(size + sizeof(struct irda_skb_cb));
+	if (likely(skb))
+		skb_reserve(skb, sizeof(struct irda_skb_cb));
+
+	return skb;
+}
+EXPORT_SYMBOL(irda_dev_alloc_skb);
+
 #ifdef CONFIG_ISA_DMA_API
 /*
  * Function setup_dma (idev, buffer, count, mode)

Signature · 5 lines

1.6.0.4.766.g6fc4a.dirty

-- 
Intel Open Source Technology Centre
http://oss.intel.com/

Samuel Ortiz

author

2008-12-15 02:03 UTC #6 lore raw

RFC PATCH 6/9 irda: Drivers should use irda_dev_alloc_skb() on the RX path

In reply to Samuel Ortiz

Signed-off-by Samuel Ortiz <[email protected]>

---
 drivers/net/irda/ali-ircc.c     |    2 +-
 drivers/net/irda/donauboe.c     |    2 +-
 drivers/net/irda/irda-usb.c     |   12 ++++++------
 drivers/net/irda/kingsun-sir.c  |    2 +-
 drivers/net/irda/ks959-sir.c    |    2 +-
 drivers/net/irda/ksdazzle-sir.c |    2 +-
 drivers/net/irda/mcs7780.c      |    6 +++---
 drivers/net/irda/nsc-ircc.c     |    2 +-
 drivers/net/irda/sir_dev.c      |    4 +++-
 drivers/net/irda/smsc-ircc2.c   |    2 +-
 drivers/net/irda/stir4200.c     |    6 +++---
 drivers/net/irda/via-ircc.c     |    8 ++++----
 drivers/net/irda/vlsi_ir.c      |    2 +-
 drivers/net/irda/w83977af_ir.c  |    2 +-
 14 files changed, 28 insertions(+), 26 deletions(-)

drivers/net/irda/ali-ircc.c +1 -1

index 2ff1818..517e6b2 100644
@@ -1904,7 +1904,7 @@ static int  ali_ircc_dma_receive_complete(struct ali_ircc_cb *self)
 			 */
 			do_gettimeofday(&self->stamp);
 
-			skb = dev_alloc_skb(len+1);
+			skb = irda_dev_alloc_skb(len+1);
 			if (skb == NULL)  
 			{
 				IRDA_WARNING("%s(), memory squeeze, "

drivers/net/irda/donauboe.c +1 -1

index 1f0f9ee..b4f954e 100644
@@ -1265,7 +1265,7 @@ dumpbufs(self->rx_bufs[self->rxs],len,'<');
 
               if (len)
                 {
-                  skb = dev_alloc_skb (len + 1);
+		  skb = irda_dev_alloc_skb(len + 1);
                   if (skb)
                     {
                       skb_reserve (skb, 1);

drivers/net/irda/irda-usb.c +6 -6

index f5c13af..0bfdcdf 100644
@@ -890,12 +890,12 @@ static void irda_usb_receive(struct urb *urb)
 
 	/* Allocate a new skb */
 	if (self->capability & IUC_STIR421X)
-		newskb = dev_alloc_skb(docopy ? urb->actual_length :
-				       IRDA_SKB_MAX_MTU +
-				       USB_IRDA_STIR421X_HEADER);
+		newskb = irda_dev_alloc_skb(docopy ? urb->actual_length :
+					    IRDA_SKB_MAX_MTU +
+					    USB_IRDA_STIR421X_HEADER);
 	else
-		newskb = dev_alloc_skb(docopy ? urb->actual_length :
-				       IRDA_SKB_MAX_MTU);
+		newskb = irda_dev_alloc_skb(docopy ? urb->actual_length :
+					    IRDA_SKB_MAX_MTU);
 
 	if (!newskb)  {
 		self->stats.rx_dropped++;

@@ -1235,7 +1235,7 @@ static int irda_usb_net_open(struct net_device *netdev)
 	/* Now that we can pass data to IrLAP, allow the USB layer
 	 * to send us some data... */
 	for (i = 0; i < IU_MAX_ACTIVE_RX_URBS; i++) {
-		struct sk_buff *skb = dev_alloc_skb(IRDA_SKB_MAX_MTU);
+		struct sk_buff *skb = irda_dev_alloc_skb(IRDA_SKB_MAX_MTU);
 		if (!skb) {
 			/* If this ever happen, we are in deep s***.
 			 * Basically, we can't start the Rx path... */

drivers/net/irda/kingsun-sir.c +1 -1

index e1429fc..47ba0a9 100644
@@ -267,7 +267,7 @@ static int kingsun_net_open(struct net_device *netdev)
 	kingsun->rx_buff.in_frame = FALSE;
 	kingsun->rx_buff.state = OUTSIDE_FRAME;
 	kingsun->rx_buff.truesize = IRDA_SKB_MAX_MTU;
-	kingsun->rx_buff.skb = dev_alloc_skb(IRDA_SKB_MAX_MTU);
+	kingsun->rx_buff.skb = irda_dev_alloc_skb(IRDA_SKB_MAX_MTU);
 	if (!kingsun->rx_buff.skb)
 		goto free_mem;

drivers/net/irda/ks959-sir.c +1 -1

index 2e67ae0..97f49ed 100644
@@ -507,7 +507,7 @@ static int ks959_net_open(struct net_device *netdev)
 	kingsun->rx_unwrap_buff.in_frame = FALSE;
 	kingsun->rx_unwrap_buff.state = OUTSIDE_FRAME;
 	kingsun->rx_unwrap_buff.truesize = IRDA_SKB_MAX_MTU;
-	kingsun->rx_unwrap_buff.skb = dev_alloc_skb(IRDA_SKB_MAX_MTU);
+	kingsun->rx_unwrap_buff.skb = irda_dev_alloc_skb(IRDA_SKB_MAX_MTU);
 	if (!kingsun->rx_unwrap_buff.skb)
 		goto free_mem;

drivers/net/irda/ksdazzle-sir.c +1 -1

index 3843b5f..e747c59 100644
@@ -401,7 +401,7 @@ static int ksdazzle_net_open(struct net_device *netdev)
 	kingsun->rx_unwrap_buff.in_frame = FALSE;
 	kingsun->rx_unwrap_buff.state = OUTSIDE_FRAME;
 	kingsun->rx_unwrap_buff.truesize = IRDA_SKB_MAX_MTU;
-	kingsun->rx_unwrap_buff.skb = dev_alloc_skb(IRDA_SKB_MAX_MTU);
+	kingsun->rx_unwrap_buff.skb = irda_dev_alloc_skb(IRDA_SKB_MAX_MTU);
 	if (!kingsun->rx_unwrap_buff.skb)
 		goto free_mem;

drivers/net/irda/mcs7780.c +3 -3

index ad92d3f..9776f41 100644
@@ -418,7 +418,7 @@ static void mcs_unwrap_mir(struct mcs_cb *mcs, __u8 *buf, int len)
 		return;
 	}
 
-	skb = dev_alloc_skb(new_len + 1);
+	skb = irda_dev_alloc_skb(new_len + 1);
 	if(unlikely(!skb)) {
 		++mcs->stats.rx_dropped;
 		return;

@@ -471,7 +471,7 @@ static void mcs_unwrap_fir(struct mcs_cb *mcs, __u8 *buf, int len)
 		return;
 	}
 
-	skb = dev_alloc_skb(new_len + 1);
+	skb = irda_dev_alloc_skb(new_len + 1);
 	if(unlikely(!skb)) {
 		++mcs->stats.rx_dropped;
 		return;

@@ -718,7 +718,7 @@ static int mcs_net_open(struct net_device *netdev)
 	/* Initialize for SIR/FIR to copy data directly into skb.  */
 	mcs->receiving = 0;
 	mcs->rx_buff.truesize = IRDA_SKB_MAX_MTU;
-	mcs->rx_buff.skb = dev_alloc_skb(IRDA_SKB_MAX_MTU);
+	mcs->rx_buff.skb = irda_dev_alloc_skb(IRDA_SKB_MAX_MTU);
 	if (!mcs->rx_buff.skb)
 		goto error1;

drivers/net/irda/nsc-ircc.c +1 -1

index 8583d95..05f53ca 100644
@@ -1858,7 +1858,7 @@ static int nsc_ircc_dma_receive_complete(struct nsc_ircc_cb *self, int iobase)
 			 */
 			do_gettimeofday(&self->stamp);
 
-			skb = dev_alloc_skb(len+1);
+			skb = irda_dev_alloc_skb(len+1);
 			if (skb == NULL)  {
 				IRDA_WARNING("%s(), memory squeeze, "
 					     "dropping frame.\n",

drivers/net/irda/sir_dev.c +3 -1

index 3f32909..3ca64e8 100644
@@ -760,7 +760,9 @@ static int sirdev_alloc_buffers(struct sir_dev *dev)
 	dev->rx_buff.truesize = IRDA_SKB_MAX_MTU; 
 
 	/* Bootstrap ZeroCopy Rx */
-	dev->rx_buff.skb = __dev_alloc_skb(dev->rx_buff.truesize, GFP_KERNEL);
+	dev->rx_buff.skb = __dev_alloc_skb(dev->rx_buff.truesize +
+					   sizeof(struct irda_skb_cb),
+					   GFP_KERNEL);
 	if (dev->rx_buff.skb == NULL)
 		return -ENOMEM;
 	skb_reserve(dev->rx_buff.skb, 1);

drivers/net/irda/smsc-ircc2.c +1 -1

index b5360fe..f13c79e 100644
@@ -1450,7 +1450,7 @@ static void smsc_ircc_dma_receive_complete(struct smsc_ircc_cb *self)
 	}
 	IRDA_DEBUG(2, "%s: msgcnt = %d, len=%d\n", __func__, msgcnt, len);
 
-	skb = dev_alloc_skb(len + 1);
+	skb = irda_dev_alloc_skb(len + 1);
 	if (!skb) {
 		IRDA_WARNING("%s(), memory squeeze, dropping frame.\n",
 			     __func__);

drivers/net/irda/stir4200.c +3 -3

index 3575804..f0edc00 100644
@@ -338,7 +338,7 @@ static void fir_eof(struct stir_cb *stir)
 
 	/* if frame is short then just copy it */
 	if (len < IRDA_RX_COPY_THRESHOLD) {
-		nskb = dev_alloc_skb(len + 1);
+		nskb = irda_dev_alloc_skb(len + 1);
 		if (unlikely(!nskb)) {
 			++stir->stats.rx_dropped;
 			return;

@@ -347,7 +347,7 @@ static void fir_eof(struct stir_cb *stir)
 		skb = nskb;
 		skb_copy_to_linear_data(nskb, rx_buff->data, len);
 	} else {
-		nskb = dev_alloc_skb(rx_buff->truesize);
+		nskb = irda_dev_alloc_skb(rx_buff->truesize);
 		if (unlikely(!nskb)) {
 			++stir->stats.rx_dropped;
 			return;

@@ -871,7 +871,7 @@ static int stir_net_open(struct net_device *netdev)
 	/* Initialize for SIR/FIR to copy data directly into skb.  */
 	stir->receiving = 0;
 	stir->rx_buff.truesize = IRDA_SKB_MAX_MTU;
-	stir->rx_buff.skb = dev_alloc_skb(IRDA_SKB_MAX_MTU);
+	stir->rx_buff.skb = irda_dev_alloc_skb(IRDA_SKB_MAX_MTU);
 	if (!stir->rx_buff.skb) 
 		goto err_out1;

drivers/net/irda/via-ircc.c +4 -4

index 84e609e..8784215 100644
@@ -1101,7 +1101,7 @@ static int via_ircc_dma_receive_complete(struct via_ircc_cb *self,
 
 	if (self->io.speed < 4000000) {	//Speed below FIR
 		len = GetRecvByte(iobase, self);
-		skb = dev_alloc_skb(len + 1);
+		skb = irda_dev_alloc_skb(len + 1);
 		if (skb == NULL)
 			return FALSE;
 		// Make sure IP header gets aligned

@@ -1173,7 +1173,7 @@ F01_E */
 		st_fifo->head++;
 		st_fifo->len--;
 
-		skb = dev_alloc_skb(len + 1 - 4);
+		skb = irda_dev_alloc_skb(len + 1 - 4);
 		/*
 		 * if frame size,data ptr,or skb ptr are wrong ,the get next
 		 * entry.

@@ -1224,7 +1224,7 @@ static int upload_rxdata(struct via_ircc_cb *self, int iobase)
 		return FALSE;
 	}
 
-	skb = dev_alloc_skb(len + 1);
+	skb = irda_dev_alloc_skb(len + 1);
 	if (skb == NULL) {
 		self->stats.rx_dropped++;
 		return FALSE;

@@ -1288,7 +1288,7 @@ static int RxTimerHandler(struct via_ircc_cb *self, int iobase)
 			st_fifo->head++;
 			st_fifo->len--;
 
-			skb = dev_alloc_skb(len + 1 - 4);
+			skb = irda_dev_alloc_skb(len + 1 - 4);
 			/*
 			 * if frame size, data ptr, or skb ptr are wrong,
 			 * then get next entry.

drivers/net/irda/vlsi_ir.c +1 -1

index 9c926d2..162872e 100644
@@ -622,7 +622,7 @@ static void vlsi_fill_rx(struct vlsi_ring *r)
 			break;
 		}
 		if (!rd->skb) {
-			rd->skb = dev_alloc_skb(IRLAP_SKB_ALLOCSIZE);
+			rd->skb = irda_dev_alloc_skb(IRLAP_SKB_ALLOCSIZE);
 			if (rd->skb) {
 				skb_reserve(rd->skb,1);
 				rd->skb->protocol = htons(ETH_P_IRDA);

drivers/net/irda/w83977af_ir.c +1 -1

index 002a6d7..a3a44c7 100644
@@ -889,7 +889,7 @@ int w83977af_dma_receive_complete(struct w83977af_ir *self)
 #endif
 			}
 						
-			skb = dev_alloc_skb(len+1);
+			skb = irda_dev_alloc_skb(len+1);
 			if (skb == NULL)  {
 				printk(KERN_INFO
 				       "%s(), memory squeeze, dropping frame.\n", __func__);

Signature · 5 lines

1.6.0.4.766.g6fc4a.dirty

-- 
Intel Open Source Technology Centre
http://oss.intel.com/

Samuel Ortiz

author

2008-12-15 02:03 UTC #7 lore raw

RFC PATCH 7/9 irda: Stack RX path callers should use irda_dev_alloc_skb

In reply to Samuel Ortiz

Signed-off-by Samuel Ortiz <[email protected]>

---
 net/irda/irttp.c   |    2 +-
 net/irda/wrapper.c |    3 ++-
 2 files changed, 3 insertions(+), 2 deletions(-)

net/irda/irttp.c +1 -1

index 4b913de..6f28186 100644
@@ -241,7 +241,7 @@ static struct sk_buff *irttp_reassemble_skb(struct tsap_cb *self)
 	IRDA_DEBUG(2, "%s(), self->rx_sdu_size=%d\n", __func__,
 		   self->rx_sdu_size);
 
-	skb = dev_alloc_skb(TTP_HEADER + self->rx_sdu_size);
+	skb = irda_dev_alloc_skb(TTP_HEADER + self->rx_sdu_size);
 	if (!skb)
 		return NULL;

net/irda/wrapper.c +2 -1

index b13cb3b..e1ab6d1 100644
@@ -225,7 +225,8 @@ async_bump(struct net_device *dev,
 		  (rx_buff->len < IRDA_RX_COPY_THRESHOLD));
 
 	/* Allocate a new skb */
-	newskb = dev_alloc_skb(docopy ? rx_buff->len + 1 : rx_buff->truesize);
+	newskb = irda_dev_alloc_skb(docopy ?
+				    rx_buff->len + 1 : rx_buff->truesize);
 	if (!newskb)  {
 		stats->rx_dropped++;
 		/* We could deliver the current skb if doing ZeroCopy Rx,

Signature · 5 lines

1.6.0.4.766.g6fc4a.dirty

-- 
Intel Open Source Technology Centre
http://oss.intel.com/

Samuel Ortiz

author

2008-12-15 02:03 UTC #8 lore raw

RFC PATCH 8/9 irda: Add a WARN_ON when our head room is too small

In reply to Samuel Ortiz

Signed-off-by Samuel Ortiz <[email protected]>

---
 include/net/irda/irda_device.h |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

include/net/irda/irda_device.h +1 -0

index 3bbf695..28c690c 100644
@@ -233,6 +233,7 @@ void irda_setup_dma(int channel, dma_addr_t buffer, int count, int mode);
 
 static inline struct irda_skb_cb *irda_get_skb_cb(struct sk_buff *skb)
 {
+	WARN_ON(skb_headroom(skb) < sizeof(struct irda_skb_cb));
 	return (struct irda_skb_cb *)(skb->data - sizeof(struct irda_skb_cb));
 }

Signature · 5 lines

1.6.0.4.766.g6fc4a.dirty

-- 
Intel Open Source Technology Centre
http://oss.intel.com/

Samuel Ortiz

author

2008-12-15 02:03 UTC #9 lore raw

RFC PATCH 9/9 irda: Fix irda_skb_cb size

In reply to Samuel Ortiz

Since we're stuffing an irda_skb_cb structure in front of the skb->data, having sizeof(irda_skb_cb) as a multiple of 4 would be a lot nicer.

Signed-off-by Samuel Ortiz <[email protected]>

---
 include/net/irda/irda_device.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

include/net/irda/irda_device.h +1 -1

index 28c690c..71c07ce 100644
@@ -146,7 +146,7 @@ struct irda_skb_cb {
 	void    *context;    /* May be used by drivers */
 	void    (*destructor)(struct sk_buff *skb); /* Used for flow control */
 	__u16   xbofs_delay; /* Number of xbofs used for generating the mtt */
-	__u8    line;        /* Used by IrCOMM in IrLPT mode */
+	__u32   line;        /* Used by IrCOMM in IrLPT mode */
 };
 
 /* Chip specific info */

Signature · 5 lines

1.6.0.4.766.g6fc4a.dirty

-- 
Intel Open Source Technology Centre
http://oss.intel.com/

Samuel Ortiz

author

2008-12-15 02:03 UTC #3 lore raw

RFC PATCH 3/9 irda: IrDA drivers should call irda_get_skb_cb()

In reply to Samuel Ortiz

Instead of accessing irda_skb_cb directly, irda drivers should call irda_get_skb_cb().

Signed-off-by Samuel Ortiz <[email protected]>

---
 drivers/net/irda/donauboe.c |    9 +--------
 drivers/net/irda/irda-usb.c |   19 ++++++++++++++-----
 2 files changed, 15 insertions(+), 13 deletions(-)

drivers/net/irda/donauboe.c +1 -8

index 69d16b3..1f0f9ee 100644
@@ -977,20 +977,13 @@ toshoboe_hard_xmit (struct sk_buff *skb, struct net_device *dev)
   __s32 speed;
   int mtt, len, ctl;
   unsigned long flags;
-  struct irda_skb_cb *cb = (struct irda_skb_cb *) skb->cb;
 
   self = (struct toshoboe_cb *) dev->priv;
 
   IRDA_ASSERT (self != NULL, return 0; );
 
   IRDA_DEBUG (1, "%s.tx:%x(%x)%x\n", __func__
-      ,skb->len,self->txpending,INB (OBOE_ENABLEH));
-  if (!cb->magic) {
-      IRDA_DEBUG (2, "%s.Not IrLAP:%x\n", __func__, cb->magic);
-#ifdef DUMP_PACKETS
-      _dumpbufs(skb->data,skb->len,'>');
-#endif
-    }
+	      , skb->len, self->txpending, INB(OBOE_ENABLEH));
 
   /* change speed pending, wait for its execution */
   if (self->new_speed)

drivers/net/irda/irda-usb.c +14 -5

index b5d6b9a..f5c13af 100644
@@ -385,6 +385,7 @@ static void speed_bulk_callback(struct urb *urb)
 static int irda_usb_hard_xmit(struct sk_buff *skb, struct net_device *netdev)
 {
 	struct irda_usb_cb *self = netdev->priv;
+	struct irda_skb_cb *cb;
 	struct urb *urb = self->tx_urb;
 	unsigned long flags;
 	s32 speed;

@@ -464,7 +465,8 @@ static int irda_usb_hard_xmit(struct sk_buff *skb, struct net_device *netdev)
 	}
 
 	/* FIXME: Make macro out of this one */
-	((struct irda_skb_cb *)skb->cb)->context = self;
+	cb = irda_get_skb_cb(skb);
+	cb->context = self;
 
 	usb_fill_bulk_urb(urb, self->usbdev,
 		      usb_sndbulkpipe(self->usbdev, self->bulk_out_ep),

@@ -554,10 +556,16 @@ static void write_bulk_callback(struct urb *urb)
 {
 	unsigned long flags;
 	struct sk_buff *skb = urb->context;
-	struct irda_usb_cb *self = ((struct irda_skb_cb *) skb->cb)->context;
+	struct irda_skb_cb *cb;
+	struct irda_usb_cb *self;
 	
 	IRDA_DEBUG(2, "%s()\n", __func__);
 
+	cb = irda_get_skb_cb(skb);
+	IRDA_ASSERT(cb != NULL, return;);
+
+	self = cb->context;
+
 	/* We should always have a context */
 	IRDA_ASSERT(self != NULL, return;);
 	/* We should always be called for the speed URB */

@@ -770,7 +778,7 @@ static void irda_usb_submit(struct irda_usb_cb *self, struct sk_buff *skb, struc
 	IRDA_ASSERT(urb != NULL, return;);
 
 	/* Save ourselves in the skb */
-	cb = (struct irda_skb_cb *) skb->cb;
+	cb = irda_get_skb_cb(skb);
 	cb->context = self;
 
 	/* Reinitialize URB */

@@ -810,7 +818,7 @@ static void irda_usb_receive(struct urb *urb)
 	IRDA_DEBUG(2, "%s(), len=%d\n", __func__, urb->actual_length);
 	
 	/* Find ourselves */
-	cb = (struct irda_skb_cb *) skb->cb;
+	cb = irda_get_skb_cb(skb);
 	IRDA_ASSERT(cb != NULL, return;);
 	self = (struct irda_usb_cb *) cb->context;
 	IRDA_ASSERT(self != NULL, return;);

@@ -970,7 +978,8 @@ static void irda_usb_rx_defer_expired(unsigned long data)
 	IRDA_DEBUG(2, "%s()\n", __func__);
 
 	/* Find ourselves */
-	cb = (struct irda_skb_cb *) skb->cb;
+	cb = irda_get_skb_cb(skb);
+
 	IRDA_ASSERT(cb != NULL, return;);
 	self = (struct irda_usb_cb *) cb->context;
 	IRDA_ASSERT(self != NULL, return;);

Signature · 5 lines

1.6.0.4.766.g6fc4a.dirty

-- 
Intel Open Source Technology Centre
http://oss.intel.com/

David Miller

2008-12-15 07:08 UTC #11 lore raw

In reply to a message not in the archive

From: Samuel Ortiz <[email protected]>
Date: Mon, 15 Dec 2008 02:57:29 +0100

Quoted 8 lines

This is a 9 patches series for IrDA, against your net-2.6 tree. This is an attempt to fix kernel.bugzilla.org bug #11795, where we noticed skb->cb could be altered once submitted to dev_queue_xmit. Since IrDA is using this callback to pass per-skb information, we are now stuffing it in front of skb->data, after allocating the right headroom.
Another solution would be to play with the IrDA physical header and skb_pull our skbs whenever we are physically transmitting the data.

Hi Sam.

I appreciate you working on this.

But I there are two issues here:

1) There is no way I can put a series of 9 invasive patches like these so late into 2.6.28

If we need to fix it in 2.6.28 it'd need to be a 5 to 10 line change at most, even if hackish, before I could seriously consider it.

2) Just like you can't claim ownership to skb->cb after dev_queue_xmit(), you just as equally can't claim ownership to areas in front of skb->data either.

I'm pretty sure we discussed how #2 wouldn't work for this problem.

I understand you need a way to pass information, but you're trying to do it using things the IRDA (nor any other) stack does not own across a dev_queue_xmit() invocation.

Furthermore, device layer schemes that try to use some shared part of the skb for their communication is frail, and a good way to see how frail it is is to consider how encapsulation of such device types within themselves might be made to work.

You can't do it with skb->cb[] private storage, and you doubly can't do it by sneaking things in front of skb->data because that's where the encapsulated protocol headers would go.

------------------------------------------------------------------------------
SF.Net email is Sponsored by MIX09, March 18-20, 2009 in Las Vegas, Nevada. The future of the web can't happen without you. Join us at MIX09 to help pave the way to the Next Web now. Learn more and register at
http://ad.doubleclick.net/clk;208669438;13503038;i?http://2009.visitmix.com/

Samuel Ortiz

author

2008-12-15 12:35 UTC #12 lore raw

Hi Dave,

Quoted 16 lines · David Miller

From: Samuel Ortiz <[email protected]>
Date: Mon, 15 Dec 2008 02:57:29 +0100
Quoted 8 lines
This is a 9 patches series for IrDA, against your net-2.6 tree. This is an attempt to fix kernel.bugzilla.org bug #11795, where we noticed skb->cb could be altered once submitted to dev_queue_xmit. Since IrDA is using this callback to pass per-skb information, we are now stuffing it in front of skb->data, after allocating the right headroom.
Another solution would be to play with the IrDA physical header and skb_pull our skbs whenever we are physically transmitting the data.
Hi Sam.
I appreciate you working on this.

Quoted 8 lines

But I there are two issues here:
1) There is no way I can put a series of 9 invasive patches like these so late into 2.6.28
If we need to fix it in 2.6.28 it'd need to be a 5 to 10 line change at most, even if hackish, before I could seriously consider it.

Ok, I understand. Sorry for not being faster at tackling that bug.
Regarding hackish fixes, would something like commit
f7cd168645dda3e9067f24fabbfa787f9a237488 (see
http://git.kernel.org/?p=linux/kernel/git/davem/net-2.6.git;a=commit;h=f7cd168645dda3e9067f24fabbfa787f9a237488 ) be acceptable to you as a short term solution ?

Quoted 10 lines

2) Just like you can't claim ownership to skb->cb after dev_queue_xmit(), you just as equally can't claim ownership to areas in front of skb->data either.
I'm pretty sure we discussed how #2 wouldn't work for this problem.
I understand you need a way to pass information, but you're trying to do it using things the IRDA (nor any other) stack does not own across a dev_queue_xmit() invocation.

Quoted 8 lines

Furthermore, device layer schemes that try to use some shared part of the skb for their communication is frail, and a good way to see how frail it is is to consider how encapsulation of such device types within themselves might be made to work.
You can't do it with skb->cb[] private storage, and you doubly can't do it by sneaking things in front of skb->data because that's where the encapsulated protocol headers would go.

Ok, so I guess I really have to work on passing that meta information along with the actual data. I tried to avoid this as a lot of code on the IrDA stack is accessing the IrLAP payload directly. I guess this is the right opportunity to add proper IrDA accessors, and then stuff the IrDA cb in the actual data header.
That's obviously -next material though...

Thanks a lot for the feedback.

Cheers,
Samuel.

David Miller

maintainer

2008-12-17 07:59 UTC #13 lore raw

From: Samuel Ortiz <[email protected]>
Date: Mon, 15 Dec 2008 13:31:44 +0100

Quoted 4 lines

Regarding hackish fixes, would something like commit
f7cd168645dda3e9067f24fabbfa787f9a237488 (see
http://git.kernel.org/?p=linux/kernel/git/davem/net-2.6.git;a=commit;h=f7cd168645dda3e9067f24fabbfa787f9a237488 ) be acceptable to you as a short term solution ?

Most definitely, yes.