[PATCH] kfifo: fix warn_unused_result

[PATCH] kfifo: fix warn_unused_result

[Stefani Seibold]

As requested by Andrew Morton:

This patch fix the "ignoring return value of '...', declared with
attribute warn_unused_result" compiler warning in several users of the
new kfifo API.

The patch-set is against current mm tree from 11-Dec-2009

Greetings,
Stefani

Signed-off-by: Stefani Seibold <stefani@seibold.net>
---
 drivers/char/nozomi.c                       |    5 ++--
 drivers/char/sonypi.c                       |   10 ++++----
 drivers/infiniband/hw/cxgb3/cxio_resource.c |   28 ++++++++++++++--------
 drivers/media/video/meye.c                  |   25 ++++++++++++--------
 drivers/net/wireless/libertas/main.c        |    9 ++++---
 drivers/platform/x86/sony-laptop.c          |   10 ++++----
 drivers/scsi/libiscsi.c                     |   18 +++++++++-----
 drivers/scsi/libiscsi_tcp.c                 |   35 +++++++++++++++++-----------
 drivers/scsi/libsrp.c                       |   14 +++++++----
 net/dccp/probe.c                            |    3 +-
 10 files changed, 99 insertions(+), 58 deletions(-)

diff -u -N -r -p mmotm/drivers/char/nozomi.c linux-2.6.32/drivers/char/nozomi.c
--- mmotm/drivers/char/nozomi.c	2009-12-11 08:31:46.670736197 +0100
+++ linux-2.6.32/drivers/char/nozomi.c	2009-12-11 09:25:46.941436203 +0100
@@ -685,8 +685,9 @@ static int nozomi_read_config_table(stru
 		dump_table(dc);
 
 		for (i = PORT_MDM; i < MAX_PORT; i++) {
-			kfifo_alloc(&dc->port[i].fifo_ul,
-				FIFO_BUFFER_SIZE_UL, GFP_ATOMIC);
+			if (kfifo_alloc(&dc->port[i].fifo_ul,
+				FIFO_BUFFER_SIZE_UL, GFP_ATOMIC))
+					BUG();
 			memset(&dc->port[i].ctrl_dl, 0, sizeof(struct ctrl_dl));
 			memset(&dc->port[i].ctrl_ul, 0, sizeof(struct ctrl_ul));
 		}
diff -u -N -r -p mmotm/drivers/char/sonypi.c linux-2.6.32/drivers/char/sonypi.c
--- mmotm/drivers/char/sonypi.c	2009-12-11 08:31:46.687469885 +0100
+++ linux-2.6.32/drivers/char/sonypi.c	2009-12-11 09:26:32.956848176 +0100
@@ -828,9 +828,10 @@ static void sonypi_report_input_event(u8
 	if (kp.dev) {
 		input_report_key(kp.dev, kp.key, 1);
 		input_sync(kp.dev);
-		kfifo_in_locked(&sonypi_device.input_fifo,
+		if (kfifo_in_locked(&sonypi_device.input_fifo,
 			(unsigned char *)&kp, sizeof(kp),
-			&sonypi_device.input_fifo_lock);
+			&sonypi_device.input_fifo_lock) != sizeof(kp))
+				BUG();
 		schedule_work(&sonypi_device.input_work);
 	}
 }
@@ -882,8 +883,9 @@ found:
 		acpi_bus_generate_proc_event(sonypi_acpi_device, 1, event);
 #endif
 
-	kfifo_in_locked(&sonypi_device.fifo, (unsigned char *)&event,
-			sizeof(event), &sonypi_device.fifo_lock);
+	if (kfifo_in_locked(&sonypi_device.fifo, (unsigned char *)&event,
+		sizeof(event), &sonypi_device.fifo_lock) != sizeof(event))
+			BUG();
 	kill_fasync(&sonypi_device.fifo_async, SIGIO, POLL_IN);
 	wake_up_interruptible(&sonypi_device.fifo_proc_list);
 
diff -u -N -r -p mmotm/drivers/infiniband/hw/cxgb3/cxio_resource.c linux-2.6.32/drivers/infiniband/hw/cxgb3/cxio_resource.c
--- mmotm/drivers/infiniband/hw/cxgb3/cxio_resource.c	2009-12-11 08:31:46.872467869 +0100
+++ linux-2.6.32/drivers/infiniband/hw/cxgb3/cxio_resource.c	2009-12-11 09:30:12.686765202 +0100
@@ -59,7 +59,9 @@ static int __cxio_init_resource_fifo(str
 		return -ENOMEM;
 
 	for (i = 0; i < skip_low + skip_high; i++)
-		kfifo_in(fifo, (unsigned char *) &entry, sizeof(u32));
+		if (kfifo_in(fifo, (unsigned char *) &entry,
+			sizeof(u32)) != sizeof(u32))
+				BUG();
 	if (random) {
 		j = 0;
 		random_bytes = random32();
@@ -71,23 +73,28 @@ static int __cxio_init_resource_fifo(str
 				random_bytes = random32();
 			}
 			idx = (random_bytes >> (j * 2)) & 0xF;
-			kfifo_in(fifo,
+			if (kfifo_in(fifo,
 				(unsigned char *) &rarray[idx],
-				sizeof(u32));
+				sizeof(u32)) != sizeof(u32))
+					BUG();
 			rarray[idx] = i;
 			j++;
 		}
 		for (i = 0; i < RANDOM_SIZE; i++)
-			kfifo_in(fifo,
+			if (kfifo_in(fifo,
 				(unsigned char *) &rarray[i],
-				sizeof(u32));
+				sizeof(u32)) != sizeof(u32))
+					BUG();
 	} else
 		for (i = skip_low; i < nr - skip_high; i++)
-			kfifo_in(fifo, (unsigned char *) &i, sizeof(u32));
+			if (kfifo_in(fifo, (unsigned char *) &i,
+				sizeof(u32)) != sizeof(u32))
+					BUG();
 
 	for (i = 0; i < skip_low + skip_high; i++)
-		kfifo_out_locked(fifo, (unsigned char *) &entry,
-				sizeof(u32), fifo_lock);
+		if (kfifo_out_locked(fifo, (unsigned char *) &entry,
+				sizeof(u32), fifo_lock) != sizeof(u32))
+					BUG();
 	return 0;
 }
 
@@ -119,8 +126,9 @@ static int cxio_init_qpid_fifo(struct cx
 
 	for (i = 16; i < T3_MAX_NUM_QP; i++)
 		if (!(i & rdev_p->qpmask))
-			kfifo_in(&rdev_p->rscp->qpid_fifo,
-				    (unsigned char *) &i, sizeof(u32));
+			if (kfifo_in(&rdev_p->rscp->qpid_fifo,
+			    (unsigned char *) &i, sizeof(u32)) != sizeof(u32))
+				BUG();
 	return 0;
 }
 
diff -u -N -r -p mmotm/drivers/media/video/meye.c linux-2.6.32/drivers/media/video/meye.c
--- mmotm/drivers/media/video/meye.c	2009-12-11 08:31:47.526583967 +0100
+++ linux-2.6.32/drivers/media/video/meye.c	2009-12-11 09:33:05.861721921 +0100
@@ -811,8 +811,9 @@ again:
 		meye.grab_buffer[reqnr].state = MEYE_BUF_DONE;
 		do_gettimeofday(&meye.grab_buffer[reqnr].timestamp);
 		meye.grab_buffer[reqnr].sequence = sequence++;
-		kfifo_in_locked(&meye.doneq, (unsigned char *)&reqnr,
-				sizeof(int), &meye.doneq_lock);
+		if (kfifo_in_locked(&meye.doneq, (unsigned char *)&reqnr,
+			sizeof(int), &meye.doneq_lock) != sizeof(int))
+				BUG();
 		wake_up_interruptible(&meye.proc_list);
 	} else {
 		int size;
@@ -832,8 +833,9 @@ again:
 		meye.grab_buffer[reqnr].state = MEYE_BUF_DONE;
 		do_gettimeofday(&meye.grab_buffer[reqnr].timestamp);
 		meye.grab_buffer[reqnr].sequence = sequence++;
-		kfifo_in_locked(&meye.doneq, (unsigned char *)&reqnr,
-				sizeof(int), &meye.doneq_lock);
+		if (kfifo_in_locked(&meye.doneq, (unsigned char *)&reqnr,
+			sizeof(int), &meye.doneq_lock) != sizeof(int))
+				BUG();
 		wake_up_interruptible(&meye.proc_list);
 	}
 	mchip_free_frame();
@@ -935,8 +937,9 @@ static int meyeioc_qbuf_capt(int *nb)
 		mchip_cont_compression_start();
 
 	meye.grab_buffer[*nb].state = MEYE_BUF_USING;
-	kfifo_in_locked(&meye.grabq, (unsigned char *)nb, sizeof(int),
-			 &meye.grabq_lock);
+	if (kfifo_in_locked(&meye.grabq, (unsigned char *)nb, sizeof(int),
+		 &meye.grabq_lock) != sizeof(int))
+			BUG();
 	mutex_unlock(&meye.lock);
 
 	return 0;
@@ -968,8 +971,9 @@ static int meyeioc_sync(struct file *fil
 		/* fall through */
 	case MEYE_BUF_DONE:
 		meye.grab_buffer[*i].state = MEYE_BUF_UNUSED;
-		kfifo_out_locked(&meye.doneq, (unsigned char *)&unused,
-				sizeof(int), &meye.doneq_lock);
+		if (kfifo_out_locked(&meye.doneq, (unsigned char *)&unused,
+			sizeof(int), &meye.doneq_lock) != sizeof(int))
+				BUG();
 	}
 	*i = meye.grab_buffer[*i].size;
 	mutex_unlock(&meye.lock);
@@ -1456,8 +1460,9 @@ static int vidioc_qbuf(struct file *file
 	buf->flags |= V4L2_BUF_FLAG_QUEUED;
 	buf->flags &= ~V4L2_BUF_FLAG_DONE;
 	meye.grab_buffer[buf->index].state = MEYE_BUF_USING;
-	kfifo_in_locked(&meye.grabq, (unsigned char *)&buf->index,
-			sizeof(int), &meye.grabq_lock);
+	if (kfifo_in_locked(&meye.grabq, (unsigned char *)&buf->index,
+		sizeof(int), &meye.grabq_lock) != sizeof(int))
+			BUG();
 	mutex_unlock(&meye.lock);
 
 	return 0;
diff -u -N -r -p mmotm/drivers/net/wireless/libertas/main.c linux-2.6.32/drivers/net/wireless/libertas/main.c
--- mmotm/drivers/net/wireless/libertas/main.c	2009-12-11 08:31:48.346583558 +0100
+++ linux-2.6.32/drivers/net/wireless/libertas/main.c	2009-12-11 09:35:22.549908343 +0100
@@ -514,8 +514,9 @@ static int lbs_thread(void *data)
 		while (kfifo_len(&priv->event_fifo)) {
 			u32 event;
 
-			kfifo_out(&priv->event_fifo, (unsigned char *) &event,
-				sizeof(event));
+			if (kfifo_out(&priv->event_fifo, (unsigned char *)
+				&event, sizeof(event)) != sizeof(event))
+					BUG();
 			spin_unlock_irq(&priv->driver_lock);
 			lbs_process_event(priv, event);
 			spin_lock_irq(&priv->driver_lock);
@@ -1176,7 +1177,9 @@ void lbs_queue_event(struct lbs_private 
 	if (priv->psstate == PS_STATE_SLEEP)
 		priv->psstate = PS_STATE_AWAKE;
 
-	kfifo_in(&priv->event_fifo, (unsigned char *) &event, sizeof(u32));
+	if (kfifo_in(&priv->event_fifo, (unsigned char *) &event,
+		sizeof(u32)) != sizeof(u32))
+			BUG();
 
 	wake_up_interruptible(&priv->waitq);
 
diff -u -N -r -p mmotm/drivers/platform/x86/sony-laptop.c linux-2.6.32/drivers/platform/x86/sony-laptop.c
--- mmotm/drivers/platform/x86/sony-laptop.c	2009-12-11 08:31:48.459583608 +0100
+++ linux-2.6.32/drivers/platform/x86/sony-laptop.c	2009-12-11 09:36:20.365344161 +0100
@@ -363,9 +363,10 @@ static void sony_laptop_report_input_eve
 		/* we emit the scancode so we can always remap the key */
 		input_event(kp.dev, EV_MSC, MSC_SCAN, event);
 		input_sync(kp.dev);
-		kfifo_in_locked(&sony_laptop_input.fifo,
+		if (kfifo_in_locked(&sony_laptop_input.fifo,
 			  (unsigned char *)&kp, sizeof(kp),
-			  &sony_laptop_input.fifo_lock);
+			  &sony_laptop_input.fifo_lock) != sizeof(kp))
+				BUG();
 
 		if (!work_pending(&sony_laptop_release_key_work))
 			queue_work(sony_laptop_input.wq,
@@ -2310,8 +2311,9 @@ static struct miscdevice sonypi_misc_dev
 
 static void sonypi_compat_report_event(u8 event)
 {
-	kfifo_in_locked(&sonypi_compat.fifo, (unsigned char *)&event,
-			sizeof(event), &sonypi_compat.fifo_lock);
+	if (kfifo_in_locked(&sonypi_compat.fifo, (unsigned char *)&event,
+		sizeof(event), &sonypi_compat.fifo_lock) != sizeof(event))
+			BUG();
 	kill_fasync(&sonypi_compat.fifo_async, SIGIO, POLL_IN);
 	wake_up_interruptible(&sonypi_compat.fifo_proc_list);
 }
diff -u -N -r -p mmotm/drivers/scsi/libiscsi.c linux-2.6.32/drivers/scsi/libiscsi.c
--- mmotm/drivers/scsi/libiscsi.c	2009-12-11 08:31:48.989159288 +0100
+++ linux-2.6.32/drivers/scsi/libiscsi.c	2009-12-11 09:21:57.984708390 +0100
@@ -517,7 +517,9 @@ static void iscsi_free_task(struct iscsi
 	if (conn->login_task == task)
 		return;
 
-	kfifo_in(&session->cmdpool.queue, (void*)&task, sizeof(void*));
+	if (kfifo_in(&session->cmdpool.queue, (void*)&task,
+	     sizeof(void*)) != sizeof(void*))
+		BUG();
 
 	if (sc) {
 		task->sc = NULL;
@@ -2469,7 +2471,9 @@ iscsi_pool_init(struct iscsi_pool *q, in
 			q->max = i;
 			goto enomem;
 		}
-		kfifo_in(&q->queue, (void*)&q->pool[i], sizeof(void*));
+		if (kfifo_in(&q->queue, (void*)&q->pool[i],
+		     sizeof(void*)) != sizeof(void*))
+			BUG();
 	}
 
 	if (items) {
@@ -2839,8 +2843,9 @@ iscsi_conn_setup(struct iscsi_cls_sessio
 	return cls_conn;
 
 login_task_data_alloc_fail:
-	kfifo_in(&session->cmdpool.queue, (void*)&conn->login_task,
-		    sizeof(void*));
+	if (kfifo_in(&session->cmdpool.queue, (void*)&conn->login_task,
+	     sizeof(void*)) != sizeof(void*))
+		BUG();
 login_task_alloc_fail:
 	iscsi_destroy_conn(cls_conn);
 	return NULL;
@@ -2902,8 +2907,9 @@ void iscsi_conn_teardown(struct iscsi_cl
 	free_pages((unsigned long) conn->data,
 		   get_order(ISCSI_DEF_MAX_RECV_SEG_LEN));
 	kfree(conn->persistent_address);
-	kfifo_in(&session->cmdpool.queue, (void*)&conn->login_task,
-		    sizeof(void*));
+	if (kfifo_in(&session->cmdpool.queue, (void*)&conn->login_task,
+	     sizeof(void*)) != sizeof(void*))
+		BUG();
 	if (session->leadconn == conn)
 		session->leadconn = NULL;
 	spin_unlock_bh(&session->lock);
diff -u -N -r -p mmotm/drivers/scsi/libiscsi_tcp.c linux-2.6.32/drivers/scsi/libiscsi_tcp.c
--- mmotm/drivers/scsi/libiscsi_tcp.c	2009-12-11 08:31:48.989159288 +0100
+++ linux-2.6.32/drivers/scsi/libiscsi_tcp.c	2009-12-11 09:41:25.819705434 +0100
@@ -446,15 +446,17 @@ void iscsi_tcp_cleanup_task(struct iscsi
 
 	/* flush task's r2t queues */
 	while (kfifo_out(&tcp_task->r2tqueue, (void*)&r2t, sizeof(void*))) {
-		kfifo_in(&tcp_task->r2tpool.queue, (void*)&r2t,
-			    sizeof(void*));
+		if (kfifo_in(&tcp_task->r2tpool.queue, (void*)&r2t,
+			    sizeof(void*)) != sizeof(void*))
+				BUG();
 		ISCSI_DBG_TCP(task->conn, "pending r2t dropped\n");
 	}
 
 	r2t = tcp_task->r2t;
 	if (r2t != NULL) {
-		kfifo_in(&tcp_task->r2tpool.queue, (void*)&r2t,
-			    sizeof(void*));
+		if (kfifo_in(&tcp_task->r2tpool.queue, (void*)&r2t,
+			    sizeof(void*)) != sizeof(void*))
+				BUG();
 		tcp_task->r2t = NULL;
 	}
 }
@@ -554,8 +556,9 @@ static int iscsi_tcp_r2t_rsp(struct iscs
 	if (r2t->data_length == 0) {
 		iscsi_conn_printk(KERN_ERR, conn,
 				  "invalid R2T with zero data len\n");
-		kfifo_in(&tcp_task->r2tpool.queue, (void*)&r2t,
-			    sizeof(void*));
+		if (kfifo_in(&tcp_task->r2tpool.queue, (void*)&r2t,
+			    sizeof(void*)) != sizeof(void*))
+				BUG();
 		return ISCSI_ERR_DATALEN;
 	}
 
@@ -570,8 +573,9 @@ static int iscsi_tcp_r2t_rsp(struct iscs
 				  "invalid R2T with data len %u at offset %u "
 				  "and total length %d\n", r2t->data_length,
 				  r2t->data_offset, scsi_out(task->sc)->length);
-		kfifo_in(&tcp_task->r2tpool.queue, (void*)&r2t,
-			    sizeof(void*));
+		if (kfifo_in(&tcp_task->r2tpool.queue, (void*)&r2t,
+			    sizeof(void*)) != sizeof(void*))
+				BUG();
 		return ISCSI_ERR_DATALEN;
 	}
 
@@ -580,7 +584,9 @@ static int iscsi_tcp_r2t_rsp(struct iscs
 	r2t->sent = 0;
 
 	tcp_task->exp_datasn = r2tsn + 1;
-	kfifo_in(&tcp_task->r2tqueue, (void*)&r2t, sizeof(void*));
+	if (kfifo_in(&tcp_task->r2tqueue, (void*)&r2t, sizeof(void*)) !=
+	     sizeof(void*))
+		BUG();
 	conn->r2t_pdus_cnt++;
 
 	iscsi_requeue_task(task);
@@ -982,16 +988,19 @@ static struct iscsi_r2t_info *iscsi_tcp_
 			if (r2t->data_length <= r2t->sent) {
 				ISCSI_DBG_TCP(task->conn,
 					      "  done with r2t %p\n", r2t);
-				kfifo_in(&tcp_task->r2tpool.queue,
+				if (kfifo_in(&tcp_task->r2tpool.queue,
 					    (void *)&tcp_task->r2t,
-					    sizeof(void *));
+					    sizeof(void *)) != sizeof(void*))
+						BUG();
 				tcp_task->r2t = r2t = NULL;
 			}
 		}
 
 		if (r2t == NULL) {
-			kfifo_out(&tcp_task->r2tqueue,
-				    (void *)&tcp_task->r2t, sizeof(void *));
+			if (kfifo_out(&tcp_task->r2tqueue,
+				    (void *)&tcp_task->r2t,
+				    sizeof(void *)) != sizeof(void*))
+					BUG();
 			r2t = tcp_task->r2t;
 		}
 		spin_unlock_bh(&session->lock);
diff -u -N -r -p mmotm/drivers/scsi/libsrp.c linux-2.6.32/drivers/scsi/libsrp.c
--- mmotm/drivers/scsi/libsrp.c	2009-12-11 08:31:48.992509364 +0100
+++ linux-2.6.32/drivers/scsi/libsrp.c	2009-12-11 09:42:52.551466710 +0100
@@ -61,7 +61,9 @@ static int srp_iu_pool_alloc(struct srp_
 	kfifo_init(&q->queue, (void *) q->pool, max * sizeof(void *));
 
 	for (i = 0, iue = q->items; i < max; i++) {
-		kfifo_in(&q->queue, (void *) &iue, sizeof(void *));
+		if (kfifo_in(&q->queue, (void *) &iue,
+		     sizeof(void *)) != sizeof(void*))
+			BUG();
 		iue->sbuf = ring[i];
 		iue++;
 	}
@@ -163,8 +165,9 @@ struct iu_entry *srp_iu_get(struct srp_t
 {
 	struct iu_entry *iue = NULL;
 
-	kfifo_out_locked(&target->iu_queue.queue, (void *) &iue,
-			sizeof(void *), &target->iu_queue.lock);
+	if (kfifo_out_locked(&target->iu_queue.queue, (void *) &iue,
+		sizeof(void *), &target->iu_queue.lock) != sizeof(void*))
+			BUG();
 	if (!iue)
 		return iue;
 	iue->target = target;
@@ -176,8 +179,9 @@ EXPORT_SYMBOL_GPL(srp_iu_get);
 
 void srp_iu_put(struct iu_entry *iue)
 {
-	kfifo_in_locked(&iue->target->iu_queue.queue, (void *) &iue,
-			sizeof(void *), &iue->target->iu_queue.lock);
+	if (kfifo_in_locked(&iue->target->iu_queue.queue, (void *) &iue,
+		sizeof(void *), &iue->target->iu_queue.lock) != sizeof(void*))
+			BUG();
 }
 EXPORT_SYMBOL_GPL(srp_iu_put);
 
diff -u -N -r -p mmotm/net/dccp/probe.c linux-2.6.32/net/dccp/probe.c
--- mmotm/net/dccp/probe.c	2009-12-11 08:31:51.810583482 +0100
+++ linux-2.6.32/net/dccp/probe.c	2009-12-11 09:43:20.478026812 +0100
@@ -67,7 +67,8 @@ static void printl(const char *fmt, ...)
 	len += vscnprintf(tbuf+len, sizeof(tbuf)-len, fmt, args);
 	va_end(args);
 
-	kfifo_in_locked(&dccpw.fifo, tbuf, len, &dccpw.lock);
+	if (kfifo_in_locked(&dccpw.fifo, tbuf, len, &dccpw.lock) != len)
+		BUG();
 	wake_up(&dccpw.wait);
 }
 

Re: [PATCH] kfifo: fix warn_unused_result

[Andrew Morton]

On Fri, 11 Dec 2009 10:18:28 +0100 Stefani Seibold <stefani@seibold.net> wrote:

> As requested by Andrew Morton:
> 
> This patch fix the "ignoring return value of '...', declared with
> attribute warn_unused_result" compiler warning in several users of the
> new kfifo API.
> 
> The patch-set is against current mm tree from 11-Dec-2009
> 
> ...
>
> --- mmotm/drivers/char/nozomi.c	2009-12-11 08:31:46.670736197 +0100
> +++ linux-2.6.32/drivers/char/nozomi.c	2009-12-11 09:25:46.941436203 +0100
> @@ -685,8 +685,9 @@ static int nozomi_read_config_table(stru
>  		dump_table(dc);
>  
>  		for (i = PORT_MDM; i < MAX_PORT; i++) {
> -			kfifo_alloc(&dc->port[i].fifo_ul,
> -				FIFO_BUFFER_SIZE_UL, GFP_ATOMIC);
> +			if (kfifo_alloc(&dc->port[i].fifo_ul,
> +				FIFO_BUFFER_SIZE_UL, GFP_ATOMIC))
> +					BUG();

No, we can't do this.  GFP_ATOMIC allocations are unreliable and can
fail.  The calling code *has* to detect the failure and then take some
recovery action.

It would be better to leave the warning in place, rather than to add
this runtime landmine.

>  		input_sync(kp.dev);
> -		kfifo_in_locked(&sonypi_device.input_fifo,
> +		if (kfifo_in_locked(&sonypi_device.input_fifo,
>  			(unsigned char *)&kp, sizeof(kp),
> -			&sonypi_device.input_fifo_lock);
> +			&sonypi_device.input_fifo_lock) != sizeof(kp))
> +				BUG();

The rest of the patch seems to be adding BUG()s if kfifo_in() fails. 
All over the place.

If that's the appropriate way to handle failure for these callsites
then it would be neater to do this in the callee.  ie, add a new

unsigned int kfifo_in_nonpartial(struct kfifo *fifo,
                        const unsigned char *from, unsigned int len)
{
	unsigned int ret = kfifo_in(fifo, from, len);

	BUG_ON(ret != len);
	return ret;
}

Re: [PATCH] kfifo: fix warn_unused_result

[Alan Cox]

> The rest of the patch seems to be adding BUG()s if kfifo_in() fails. 
> All over the place.
> 
> If that's the appropriate way to handle failure for these callsites
> then it would be neater to do this in the callee.  ie, add a new

BUG_ON is a bit extreme for most of those if they fail. WARN_ONCE perhaps
given the usual consequences are minor (eg pointer jumping)

Re: [PATCH] kfifo: fix warn_unused_result

[Stefani Seibold]

Am Sonntag, den 13.12.2009, 14:29 -0800 schrieb Andrew Morton:
> On Fri, 11 Dec 2009 10:18:28 +0100 Stefani Seibold <stefani@seibold.net> wrote:
> 
> > As requested by Andrew Morton:
> > 
> > This patch fix the "ignoring return value of '...', declared with
> > attribute warn_unused_result" compiler warning in several users of the
> > new kfifo API.
> > 
> > The patch-set is against current mm tree from 11-Dec-2009
> > 
> > ...
> >
> > --- mmotm/drivers/char/nozomi.c	2009-12-11 08:31:46.670736197 +0100
> > +++ linux-2.6.32/drivers/char/nozomi.c	2009-12-11 09:25:46.941436203 +0100
> > @@ -685,8 +685,9 @@ static int nozomi_read_config_table(stru
> >  		dump_table(dc);
> >  
> >  		for (i = PORT_MDM; i < MAX_PORT; i++) {
> > -			kfifo_alloc(&dc->port[i].fifo_ul,
> > -				FIFO_BUFFER_SIZE_UL, GFP_ATOMIC);
> > +			if (kfifo_alloc(&dc->port[i].fifo_ul,
> > +				FIFO_BUFFER_SIZE_UL, GFP_ATOMIC))
> > +					BUG();
> 
> No, we can't do this.  GFP_ATOMIC allocations are unreliable and can
> fail.  The calling code *has* to detect the failure and then take some
> recovery action.
> 
> It would be better to leave the warning in place, rather than to add
> this runtime landmine.
> 

The problem is that the old code did not provide an error check. So i
don't think it is a land mine, because this drivers tries to allocate a
some kfifo inside an interrupt. But i think i am able to understand the
code and fix it.

> >  		input_sync(kp.dev);
> > -		kfifo_in_locked(&sonypi_device.input_fifo,
> > +		if (kfifo_in_locked(&sonypi_device.input_fifo,
> >  			(unsigned char *)&kp, sizeof(kp),
> > -			&sonypi_device.input_fifo_lock);
> > +			&sonypi_device.input_fifo_lock) != sizeof(kp))
> > +				BUG();
> 
> The rest of the patch seems to be adding BUG()s if kfifo_in() fails. 
> All over the place.
> 
> If that's the appropriate way to handle failure for these callsites
> then it would be neater to do this in the callee.  ie, add a new
> 
> unsigned int kfifo_in_nonpartial(struct kfifo *fifo,
>                         const unsigned char *from, unsigned int len)
> {
> 	unsigned int ret = kfifo_in(fifo, from, len);
> 
> 	BUG_ON(ret != len);
> 	return ret;
> }
> 

No, i don't like to idea to introduce a new API call, because i must
also introduce a 

kfifo_out_nonpartial()
kfifo_in_nonpartial_locked()
and
kfifo_out_nonpartial_locked()

I don't like this _locked functions, it is a design break and only
introduced for compatibility reasons.

This will also go way if i get the okay for the new kqueue API.

If it is okay, i will remove the __must_check from kfifo_in and
kfifo_in_locked. But the kfifo_out and kfifo_out_locked check must be
performed and if it fails, it is a real BUG.

[PATCH] kfifo: fix warn_unused_result

[Stefani Seibold]

As requested by Andrew Morton:

This patch fix the "ignoring return value of '...', declared with
attribute warn_unused_result" compiler warning in several users of the
new kfifo API.
 
It removes the __must_check attribute from kfifo_in() and
kfifo_in_locked() which must not necessary performed.

Fix the allocation bug in the nozomi driver file, by moving out the
kfifo_alloc from the interrupt handler into the probe function.

Fix the kfifo_out() and kfifo_out_locked() users to handle a unexpected
end of fifo. 

The patch-set is against current mm tree from 11-Dec-2009

Greetings,
Stefani

Signed-off-by: Stefani Seibold <stefani@seibold.net>
---
 drivers/char/nozomi.c                       |   31 ++++++++++++++++++++++++----
 drivers/infiniband/hw/cxgb3/cxio_resource.c |    5 ++--
 drivers/media/video/meye.c                  |    5 ++--
 drivers/net/wireless/libertas/main.c        |    6 +++--
 drivers/scsi/libiscsi_tcp.c                 |    9 ++++++--
 drivers/scsi/libsrp.c                       |    7 ++++--
 include/linux/kfifo.h                       |    4 +--
 7 files changed, 51 insertions(+), 16 deletions(-)

diff -u -N -r mmotm.orig/include/linux/kfifo.h mmotm.new/include/linux/kfifo.h
--- mmotm.orig/include/linux/kfifo.h	2009-12-14 04:28:36.392623524 +0100
+++ mmotm.new/include/linux/kfifo.h	2009-12-14 06:15:53.889911612 +0100
@@ -110,7 +110,7 @@
 extern __must_check int kfifo_alloc(struct kfifo *fifo, unsigned int size,
 			gfp_t gfp_mask);
 extern void kfifo_free(struct kfifo *fifo);
-extern __must_check unsigned int kfifo_in(struct kfifo *fifo,
+extern unsigned int kfifo_in(struct kfifo *fifo,
 				const unsigned char *from, unsigned int len);
 extern __must_check unsigned int kfifo_out(struct kfifo *fifo,
 				unsigned char *to, unsigned int len);
@@ -194,7 +194,7 @@
  * the FIFO depending on the free space, and returns the number of
  * bytes copied.
  */
-static inline __must_check unsigned int kfifo_in_locked(struct kfifo *fifo,
+static inline unsigned int kfifo_in_locked(struct kfifo *fifo,
 		const unsigned char *from, unsigned int n, spinlock_t *lock)
 {
 	unsigned long flags;
diff -u -N -r mmotm.orig/drivers/char/nozomi.c mmotm.new/drivers/char/nozomi.c
--- mmotm.orig/drivers/char/nozomi.c	2009-12-14 04:56:54.757061915 +0100
+++ mmotm.new/drivers/char/nozomi.c	2009-12-14 06:15:53.887965205 +0100
@@ -685,8 +685,6 @@
 		dump_table(dc);
 
 		for (i = PORT_MDM; i < MAX_PORT; i++) {
-			kfifo_alloc(&dc->port[i].fifo_ul,
-				FIFO_BUFFER_SIZE_UL, GFP_ATOMIC);
 			memset(&dc->port[i].ctrl_dl, 0, sizeof(struct ctrl_dl));
 			memset(&dc->port[i].ctrl_ul, 0, sizeof(struct ctrl_ul));
 		}
@@ -1433,6 +1431,16 @@
 		goto err_free_sbuf;
 	}
 
+	for (i = PORT_MDM; i < MAX_PORT; i++) {
+		if (kfifo_alloc(&dc->port[i].fifo_ul,
+		      FIFO_BUFFER_SIZE_UL, GFP_ATOMIC)) {
+			dev_err(&pdev->dev,
+					"Could not allocate kfifo buffer\n");
+			ret = -ENOMEM;
+			goto err_free_kfifo;
+		}
+	}
+
 	spin_lock_init(&dc->spin_mutex);
 
 	nozomi_setup_private_data(dc);
@@ -1445,7 +1453,7 @@
 			NOZOMI_NAME, dc);
 	if (unlikely(ret)) {
 		dev_err(&pdev->dev, "can't request irq %d\n", pdev->irq);
-		goto err_free_sbuf;
+		goto err_free_kfifo;
 	}
 
 	DBG1("base_addr: %p", dc->base_addr);
@@ -1464,13 +1472,28 @@
 	dc->state = NOZOMI_STATE_ENABLED;
 
 	for (i = 0; i < MAX_PORT; i++) {
+		struct device *tty_dev;
+
 		mutex_init(&dc->port[i].tty_sem);
 		tty_port_init(&dc->port[i].port);
-		tty_register_device(ntty_driver, dc->index_start + i,
+		tty_dev = tty_register_device(ntty_driver, dc->index_start + i,
 							&pdev->dev);
+
+		if (IS_ERR(tty_dev)) {
+			ret = PTR_ERR(tty_dev);
+			dev_err(&pdev->dev, "Could not allocate tty?\n");
+			goto err_free_tty;
+		}
 	}
+
 	return 0;
 
+err_free_tty:
+	for (i = dc->index_start; i < dc->index_start + MAX_PORT; ++i)
+		tty_unregister_device(ntty_driver, i);
+err_free_kfifo:
+	for (i = 0; i < MAX_PORT; i++)
+		kfifo_free(&dc->port[i].fifo_ul);
 err_free_sbuf:
 	kfree(dc->send_buf);
 	iounmap(dc->base_addr);
diff -u -N -r mmotm.orig/drivers/infiniband/hw/cxgb3/cxio_resource.c mmotm.new/drivers/infiniband/hw/cxgb3/cxio_resource.c
--- mmotm.orig/drivers/infiniband/hw/cxgb3/cxio_resource.c	2009-12-14 05:48:19.409593867 +0100
+++ mmotm.new/drivers/infiniband/hw/cxgb3/cxio_resource.c	2009-12-14 06:15:53.887047560 +0100
@@ -86,8 +86,9 @@
 			kfifo_in(fifo, (unsigned char *) &i, sizeof(u32));
 
 	for (i = 0; i < skip_low + skip_high; i++)
-		kfifo_out_locked(fifo, (unsigned char *) &entry,
-				sizeof(u32), fifo_lock);
+		if (kfifo_out_locked(fifo, (unsigned char *) &entry,
+				sizeof(u32), fifo_lock) != sizeof(u32))
+					break;
 	return 0;
 }
 
diff -u -N -r mmotm.orig/drivers/media/video/meye.c mmotm.new/drivers/media/video/meye.c
--- mmotm.orig/drivers/media/video/meye.c	2009-12-14 05:50:04.409843744 +0100
+++ mmotm.new/drivers/media/video/meye.c	2009-12-14 06:15:53.885580195 +0100
@@ -968,8 +968,9 @@
 		/* fall through */
 	case MEYE_BUF_DONE:
 		meye.grab_buffer[*i].state = MEYE_BUF_UNUSED;
-		kfifo_out_locked(&meye.doneq, (unsigned char *)&unused,
-				sizeof(int), &meye.doneq_lock);
+		if (kfifo_out_locked(&meye.doneq, (unsigned char *)&unused,
+				sizeof(int), &meye.doneq_lock) != sizeof(int))
+					break;
 	}
 	*i = meye.grab_buffer[*i].size;
 	mutex_unlock(&meye.lock);
diff -u -N -r mmotm.orig/drivers/net/wireless/libertas/main.c mmotm.new/drivers/net/wireless/libertas/main.c
--- mmotm.orig/drivers/net/wireless/libertas/main.c	2009-12-14 05:50:59.173511999 +0100
+++ mmotm.new/drivers/net/wireless/libertas/main.c	2009-12-14 06:15:53.889002627 +0100
@@ -514,8 +514,10 @@
 		while (kfifo_len(&priv->event_fifo)) {
 			u32 event;
 
-			kfifo_out(&priv->event_fifo, (unsigned char *) &event,
-				sizeof(event));
+			if (kfifo_out(&priv->event_fifo,
+				(unsigned char *) &event, sizeof(event)) !=
+				sizeof(event))
+					break;
 			spin_unlock_irq(&priv->driver_lock);
 			lbs_process_event(priv, event);
 			spin_lock_irq(&priv->driver_lock);
diff -u -N -r mmotm.orig/drivers/scsi/libiscsi_tcp.c mmotm.new/drivers/scsi/libiscsi_tcp.c
--- mmotm.orig/drivers/scsi/libiscsi_tcp.c	2009-12-14 05:51:36.459482982 +0100
+++ mmotm.new/drivers/scsi/libiscsi_tcp.c	2009-12-14 06:15:53.883593559 +0100
@@ -990,8 +990,13 @@
 		}
 
 		if (r2t == NULL) {
-			kfifo_out(&tcp_task->r2tqueue,
-				    (void *)&tcp_task->r2t, sizeof(void *));
+			if (kfifo_out(&tcp_task->r2tqueue,
+			    (void *)&tcp_task->r2t, sizeof(void *)) !=
+			    sizeof(void *)) {
+				WARN_ONCE(1, "unexpected fifo state");
+				r2t = NULL;
+			}
+					
 			r2t = tcp_task->r2t;
 		}
 		spin_unlock_bh(&session->lock);
diff -u -N -r mmotm.orig/drivers/scsi/libsrp.c mmotm.new/drivers/scsi/libsrp.c
--- mmotm.orig/drivers/scsi/libsrp.c	2009-12-14 05:51:53.838843981 +0100
+++ mmotm.new/drivers/scsi/libsrp.c	2009-12-14 06:15:53.885580195 +0100
@@ -163,8 +163,11 @@
 {
 	struct iu_entry *iue = NULL;
 
-	kfifo_out_locked(&target->iu_queue.queue, (void *) &iue,
-			sizeof(void *), &target->iu_queue.lock);
+	if (kfifo_out_locked(&target->iu_queue.queue, (void *) &iue,
+		sizeof(void *), &target->iu_queue.lock) != sizeof(void *)) {
+			WARN_ONCE(1, "unexpected fifo state");
+			return NULL;
+	}
 	if (!iue)
 		return iue;
 	iue->target = target;

Re: [PATCH] kfifo: fix warn_unused_result

[Andrew Morton]

On Mon, 14 Dec 2009 05:36:39 +0100 Stefani Seibold <stefani@seibold.net> wrote:

> If it is okay, i will remove the __must_check from kfifo_in and
> kfifo_in_locked.

That works, I suppose.