[PATCH 0/7] char: xillybus: Harden driver and improve code quality

[PATCH 0/7] char: xillybus: Harden driver and improve code quality

[Eli Billauer]

This patch set consists of several boundary and sanity checks, gaining
better control of execution flow and fixing minor coding issues. No
difference is expected in the driver's behavior under normal conditions,
except for one change that might improve bandwidth performance
marginally.

Why all of the sudden? These patches are the result of a comprehensive
AI-assisted code review, using Deepseek, Kimi Thinking K2.6, ChatGPT,
and Claude Sonnet 4.6 as plain prompt tools.

I've spent three weeks in what I call a "reverse rubber duck" session,
asking these LLMs to find bugs in the code and following up on different
leads, the vast majority of which turned out to be no more than quack.
Occasionally the points made by AI were valid, and in others, the mere
effort to explain why the remark is wrong to the rubber duck led to
finding real issues.

Almost none of the fixes suggested by AI were even near the actual
corrections in this patch set.

Thanks & regards,
   Eli

Eli Billauer (7):
  char: xillybus: Improve control of execution flow with mutexes
  char: xillybus: Remove duplicate error path code
  char: xillybus: Avoid possible bandwidth inefficiency
  char: xillybus: Use unsigned arithmetic for jiffies differences
  char: xillybus: Integer arithmetic improvements
  char: xillybus: Add defensive sanity checks
  char: xillybus: Ignore and report unsolicited interrupts

 drivers/char/xillybus/xillybus.h       |   3 +
 drivers/char/xillybus/xillybus_class.c |  11 +++
 drivers/char/xillybus/xillybus_class.h |   4 +
 drivers/char/xillybus/xillybus_core.c  | 100 ++++++++++++++++++++++---
 drivers/char/xillybus/xillybus_of.c    |   6 ++
 drivers/char/xillybus/xillyusb.c       |  62 +++++++++++----
 6 files changed, 160 insertions(+), 26 deletions(-)

-- 
2.34.1

[PATCH 1/7] char: xillybus: Improve control of execution flow with mutexes

[Eli Billauer]

This commit addresses two issues by using mutexes:

(1) Add a mutex to protect the fifo_buf_order global variable. The
    purpose of this variable is avoid repeated failed calls to
    __get_free_pages() for allocating FIFO memory, when the chunk
    size was too big. However, if two drivers are initialized at the
    same time, fifo_init() may run in parallel, and fifo_buf_order
    may be reduced too much. This is a far-fetched scenario, now
    completely prevented by fifo_buf_order_mutex.

(2) setup_channels() acquires process_in_mutex to prevent
    process_bulk_in() from accessing the xillyusb_dev struct. With
    correctly working hardware, process_bulk_in() is never called while
    setup_channels() runs, because the device has no reason to send data
    in that phase. The mutex ensures that process_bulk_in() does not
    touch the members that setup_channels() alters.

    There is no similar protection for data flow in the other direction,
    because during the setup process, the only outbound data is the
    BULK endpoint used for commands, and it remains untouched after its
    initial setup.

Signed-off-by: Eli Billauer <eli.billauer@gmail.com>
---
 drivers/char/xillybus/xillyusb.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/char/xillybus/xillyusb.c b/drivers/char/xillybus/xillyusb.c
index 34e7ad3bcab3..a28e6416cb01 100644
--- a/drivers/char/xillybus/xillyusb.c
+++ b/drivers/char/xillybus/xillyusb.c
@@ -50,6 +50,7 @@ MODULE_LICENSE("GPL v2");
 static const char xillyname[] = "xillyusb";
 
 static unsigned int fifo_buf_order;
+static DEFINE_MUTEX(fifo_buf_order_mutex);
 static struct workqueue_struct *wakeup_wq;
 
 #define USB_VENDOR_ID_XILINX		0x03fd
@@ -375,6 +376,8 @@ static int fifo_init(struct xillyfifo *fifo,
 
 	unsigned int log2_fifo_buf_size;
 
+	guard(mutex)(&fifo_buf_order_mutex);
+
 retry:
 	log2_fifo_buf_size = fifo_buf_order + PAGE_SHIFT;
 
@@ -1943,6 +1946,9 @@ static int setup_channels(struct xillyusb_dev *xdev,
 	struct xillyusb_channel *chan, *new_channels;
 	int i;
 
+	/* Don't let process_bulk_in() run while we change the channels */
+	guard(mutex)(&xdev->process_in_mutex);
+
 	chan = kzalloc_objs(*chan, num_channels);
 	if (!chan)
 		return -ENOMEM;
-- 
2.34.1

[PATCH 2/7] char: xillybus: Remove duplicate error path code

[Eli Billauer]

No need for dedicated code for the error path. The removed dedicated
error path code differs only in when the mutex is released, and that
makes no difference in this context.

Signed-off-by: Eli Billauer <eli.billauer@gmail.com>
---
 drivers/char/xillybus/xillyusb.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/drivers/char/xillybus/xillyusb.c b/drivers/char/xillybus/xillyusb.c
index a28e6416cb01..ab5f9159aa17 100644
--- a/drivers/char/xillybus/xillyusb.c
+++ b/drivers/char/xillybus/xillyusb.c
@@ -1419,16 +1419,12 @@ static int xillyusb_open(struct inode *inode, struct file *filp)
 	if (filp->f_mode & FMODE_WRITE)
 		chan->open_for_write = 0;
 
+unmutex_fail:
 	mutex_unlock(&chan->lock);
 
 	kref_put(&xdev->kref, cleanup_dev);
 
 	return rc;
-
-unmutex_fail:
-	kref_put(&xdev->kref, cleanup_dev);
-	mutex_unlock(&chan->lock);
-	return rc;
 }
 
 static ssize_t xillyusb_read(struct file *filp, char __user *userbuf,
-- 
2.34.1

[PATCH 3/7] char: xillybus: Avoid possible bandwidth inefficiency

[Eli Billauer]

The host flow controls the payload data flow from the FPGA by sending
OPCODE_SET_CHECKPOINT messages. Fix the condition for sending such a
message, to correctly handle the case where leap < 0.

The previous expression leap > (fifo->size >> 3) was not intended to
evaluate true when leap is negative. However, due to C's integer
promotion rules, leap (of s32 type) is promoted to unsigned int when
compared with the unsigned fifo->size >> 3 expression. As a result,
negative leap values are interpreted as large positive numbers, causing
the condition to evaluate true unintentionally.

Consequently, the device receives correctly formed checkpoint messages
that encourage it to send data, but too frequently. This may cause the
device to send short data chunks, wasting USB bandwidth.

Signed-off-by: Eli Billauer <eli.billauer@gmail.com>
---
 drivers/char/xillybus/xillyusb.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/char/xillybus/xillyusb.c b/drivers/char/xillybus/xillyusb.c
index ab5f9159aa17..7d2434c02fa7 100644
--- a/drivers/char/xillybus/xillyusb.c
+++ b/drivers/char/xillybus/xillyusb.c
@@ -1507,8 +1507,8 @@ static ssize_t xillyusb_read(struct file *filp, char __user *userbuf,
 		 */
 
 		if (chan->read_data_ok &&
-		    (leap > (fifo->size >> 3) ||
-		     (checkpoint_for_complete && leap > 0))) {
+		    (leap > 0 && (leap > (fifo->size >> 3) ||
+				  checkpoint_for_complete))) {
 			chan->in_current_checkpoint = checkpoint;
 			rc = xillyusb_send_opcode(xdev, chan_num,
 						  OPCODE_SET_CHECKPOINT,
-- 
2.34.1

[PATCH 4/7] char: xillybus: Use unsigned arithmetic for jiffies differences

[Eli Billauer]

Change the type of jiffies-related deadline variables from long to
unsigned long, and remove unnecessary casts when computing time
remaining as deadline - jiffies.

No functional change is expected: although signed overflow is undefined
in the C standard, processors perform the calculation correctly in
practice. Using unsigned arithmetic is nevertheless the proper way to
handle jiffies differences.

Signed-off-by: Eli Billauer <eli.billauer@gmail.com>
---
 drivers/char/xillybus/xillybus_core.c |  5 +++--
 drivers/char/xillybus/xillyusb.c      | 17 ++++++++++-------
 2 files changed, 13 insertions(+), 9 deletions(-)

diff --git a/drivers/char/xillybus/xillybus_core.c b/drivers/char/xillybus/xillybus_core.c
index 952ef149aba1..7acebc1e6050 100644
--- a/drivers/char/xillybus/xillybus_core.c
+++ b/drivers/char/xillybus/xillybus_core.c
@@ -694,7 +694,8 @@ static ssize_t xillybus_read(struct file *filp, char __user *userbuf,
 	unsigned long flags;
 	int bytes_done = 0;
 	int no_time_left = 0;
-	long deadline, left_to_sleep;
+	unsigned long deadline;
+	long left_to_sleep;
 	struct xilly_channel *channel = filp->private_data;
 
 	int empty, reached_eof, exhausted, ready;
@@ -938,7 +939,7 @@ static ssize_t xillybus_read(struct file *filp, char __user *userbuf,
 			return -EINTR;
 		}
 
-		left_to_sleep = deadline - ((long) jiffies);
+		left_to_sleep = deadline - jiffies;
 
 		/*
 		 * If our time is out, skip the waiting. We may miss wr_sleepy
diff --git a/drivers/char/xillybus/xillyusb.c b/drivers/char/xillybus/xillyusb.c
index 7d2434c02fa7..aa08206a18ef 100644
--- a/drivers/char/xillybus/xillyusb.c
+++ b/drivers/char/xillybus/xillyusb.c
@@ -1127,12 +1127,13 @@ static int xillyusb_send_opcode(struct xillyusb_dev *xdev,
  */
 
 static int flush_downstream(struct xillyusb_channel *chan,
-			    long timeout,
+			    unsigned long timeout,
 			    bool interruptible)
 {
 	struct xillyusb_dev *xdev = chan->xdev;
 	int chan_num = chan->chan_idx << 1;
-	long deadline, left_to_sleep;
+	unsigned long deadline;
+	long left_to_sleep;
 	int rc;
 
 	if (chan->flushed)
@@ -1141,7 +1142,8 @@ static int flush_downstream(struct xillyusb_channel *chan,
 	deadline = jiffies + 1 + timeout;
 
 	if (chan->flushing) {
-		long cancel_deadline = jiffies + 1 + XILLY_RESPONSE_TIMEOUT;
+		unsigned long cancel_deadline =
+			jiffies + 1 + XILLY_RESPONSE_TIMEOUT;
 
 		chan->canceled = 0;
 		rc = xillyusb_send_opcode(xdev, chan_num,
@@ -1152,7 +1154,7 @@ static int flush_downstream(struct xillyusb_channel *chan,
 
 		/* Ignoring interrupts. Cancellation must be handled */
 		while (!chan->canceled) {
-			left_to_sleep = cancel_deadline - ((long)jiffies);
+			left_to_sleep = cancel_deadline - jiffies;
 
 			if (left_to_sleep <= 0) {
 				report_io_error(xdev, -EIO);
@@ -1202,7 +1204,7 @@ static int flush_downstream(struct xillyusb_channel *chan,
 	}
 
 	while (chan->flushing) {
-		left_to_sleep = deadline - ((long)jiffies);
+		left_to_sleep = deadline - jiffies;
 
 		if (left_to_sleep <= 0)
 			return -ETIMEDOUT;
@@ -1435,7 +1437,8 @@ static ssize_t xillyusb_read(struct file *filp, char __user *userbuf,
 	struct xillyfifo *fifo = chan->in_fifo;
 	int chan_num = (chan->chan_idx << 1) | 1;
 
-	long deadline, left_to_sleep;
+	unsigned long deadline;
+	long left_to_sleep;
 	int bytes_done = 0;
 	bool sent_set_push = false;
 	int rc;
@@ -1464,7 +1467,7 @@ static ssize_t xillyusb_read(struct file *filp, char __user *userbuf,
 		bytes_done += rc;
 		chan->in_consumed_bytes += rc;
 
-		left_to_sleep = deadline - ((long)jiffies);
+		left_to_sleep = deadline - jiffies;
 
 		/*
 		 * Some 32-bit arithmetic that may wrap. Note that
-- 
2.34.1

[PATCH 5/7] char: xillybus: Integer arithmetic improvements

[Eli Billauer]

Choose unsigned integers instead of signed where natural, and also ensure
overflow wraps as expected.

Simplify an arithmetic expression too.

No functional change is expected, as the relevant variables normally never
reach values where this transition matters.

Signed-off-by: Eli Billauer <eli.billauer@gmail.com>
---
 drivers/char/xillybus/xillybus_core.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/char/xillybus/xillybus_core.c b/drivers/char/xillybus/xillybus_core.c
index 7acebc1e6050..b264578b2572 100644
--- a/drivers/char/xillybus/xillybus_core.c
+++ b/drivers/char/xillybus/xillybus_core.c
@@ -343,7 +343,8 @@ static int xilly_map_single(struct xilly_endpoint *ep,
 static int xilly_get_dma_buffers(struct xilly_endpoint *ep,
 				 struct xilly_alloc_state *s,
 				 struct xilly_buffer **buffers,
-				 int bufnum, int bytebufsize)
+				 unsigned int bufnum,
+				 unsigned int bytebufsize)
 {
 	int i, rc;
 	dma_addr_t dma_addr;
@@ -431,8 +432,8 @@ static int xilly_setupchannels(struct xilly_endpoint *ep,
 	struct device *dev = ep->dev;
 	int i, entry, rc;
 	struct xilly_channel *channel;
-	int channelnum, bufnum, bufsize, format, is_writebuf;
-	int bytebufsize;
+	unsigned int channelnum, bufnum, bufsize, format, is_writebuf;
+	unsigned int bytebufsize;
 	int synchronous, allowpartial, exclusive_open, seekable;
 	int supports_nonempty;
 	int msg_buf_done = 0;
@@ -531,8 +532,7 @@ static int xilly_setupchannels(struct xilly_endpoint *ep,
 			channel->log2_element_size = ((format > 2) ?
 						      2 : format);
 
-			bytebufsize = bufsize *
-				(1 << channel->log2_element_size);
+			bytebufsize = bufsize << channel->log2_element_size;
 
 			buffers = devm_kcalloc(dev, bufnum,
 					       sizeof(struct xilly_buffer *),
@@ -589,7 +589,7 @@ static int xilly_setupchannels(struct xilly_endpoint *ep,
 static int xilly_scan_idt(struct xilly_endpoint *endpoint,
 			  struct xilly_idt_handle *idt_handle)
 {
-	int count = 0;
+	unsigned int count = 0;
 	unsigned char *idt = endpoint->channels[1]->wr_buffers[0]->addr;
 	unsigned char *end_of_idt = idt + endpoint->idtlen - 4;
 	unsigned char *scan;
-- 
2.34.1

[PATCH 6/7] char: xillybus: Add defensive sanity checks

[Eli Billauer]

Add validation checks for values derived from hardware or user input to
prevent incorrect behavior with malformed data.

Signed-off-by: Eli Billauer <eli.billauer@gmail.com>
---
 drivers/char/xillybus/xillybus_class.c | 11 ++++++++
 drivers/char/xillybus/xillybus_class.h |  4 +++
 drivers/char/xillybus/xillybus_core.c  | 37 ++++++++++++++++++++++++--
 drivers/char/xillybus/xillybus_of.c    |  6 +++++
 drivers/char/xillybus/xillyusb.c       | 29 +++++++++++++++++++-
 5 files changed, 84 insertions(+), 3 deletions(-)

diff --git a/drivers/char/xillybus/xillybus_class.c b/drivers/char/xillybus/xillybus_class.c
index 5e8f03b77064..1eecb8ba244a 100644
--- a/drivers/char/xillybus/xillybus_class.c
+++ b/drivers/char/xillybus/xillybus_class.c
@@ -57,6 +57,9 @@ int xillybus_init_chrdev(struct device *dev,
 	size_t namelen;
 	struct xilly_unit *unit, *u;
 
+	if (num_nodes <= 0 || num_nodes > XILLYBUS_MAX_NODES || !idt || !prefix || !dev)
+		return -ENODEV;
+
 	unit = kzalloc_obj(*unit);
 
 	if (!unit)
@@ -68,6 +71,11 @@ int xillybus_init_chrdev(struct device *dev,
 		snprintf(unit->name, UNITNAMELEN, "%s", prefix);
 
 	for (i = 0; enumerate; i++) {
+		if (i > 99) {
+			dev_err(dev, "Failed to obtain unique unit name\n");
+			goto fail_obtain;
+		}
+
 		snprintf(unit->name, UNITNAMELEN, "%s_%02d",
 			 prefix, i);
 
@@ -219,6 +227,9 @@ int xillybus_find_inode(struct inode *inode,
 	int major = imajor(inode);
 	struct xilly_unit *unit = NULL, *iter;
 
+	if (!inode || !private_data || !index)
+		return -ENODEV;
+
 	mutex_lock(&unit_mutex);
 
 	list_for_each_entry(iter, &unit_list, list_entry)
diff --git a/drivers/char/xillybus/xillybus_class.h b/drivers/char/xillybus/xillybus_class.h
index 5dbfdfc95c65..264b9f6d6793 100644
--- a/drivers/char/xillybus/xillybus_class.h
+++ b/drivers/char/xillybus/xillybus_class.h
@@ -8,6 +8,10 @@
 #ifndef __XILLYBUS_CLASS_H
 #define __XILLYBUS_CLASS_H
 
+#define XILLYBUS_MAX_COUNT (((unsigned int) ~0x1ffff) >> 1)
+#define XILLYBUS_MAX_NODES 1024
+#define XILLYBUS_MAX_IDT 1048576
+
 #include <linux/types.h>
 #include <linux/device.h>
 #include <linux/fs.h>
diff --git a/drivers/char/xillybus/xillybus_core.c b/drivers/char/xillybus/xillybus_core.c
index b264578b2572..3436f16092e0 100644
--- a/drivers/char/xillybus/xillybus_core.c
+++ b/drivers/char/xillybus/xillybus_core.c
@@ -351,6 +351,12 @@ static int xilly_get_dma_buffers(struct xilly_endpoint *ep,
 	struct device *dev = ep->dev;
 	struct xilly_buffer *this_buffer = NULL; /* Init to silence warning */
 
+	if (bytebufsize == 0 || bytebufsize > 0x40000000) {
+		dev_err(ep->dev,
+			"Illegal buffer size requested in IDT. Aborting.\n");
+		return -ENODEV;
+	}
+
 	if (buffers) { /* Not the message buffer */
 		this_buffer = devm_kcalloc(dev, bufnum,
 					   sizeof(struct xilly_buffer),
@@ -623,6 +629,12 @@ static int xilly_scan_idt(struct xilly_endpoint *endpoint,
 		return -ENODEV;
 	}
 
+	if (count == 0 || count > XILLYBUS_MAX_NODES) {
+		dev_err(endpoint->dev,
+			"Unreasonable number of channels. Aborting.\n");
+		return -ENODEV;
+	}
+
 	idt_handle->entries = len >> 2;
 	endpoint->num_channels = count;
 
@@ -707,6 +719,9 @@ static ssize_t xillybus_read(struct file *filp, char __user *userbuf,
 	if (channel->endpoint->fatal_error)
 		return -EIO;
 
+	if (count > XILLYBUS_MAX_COUNT)
+		count = XILLYBUS_MAX_COUNT;
+
 	deadline = jiffies + 1 + XILLY_RX_TIMEOUT;
 
 	rc = mutex_lock_interruptible(&channel->wr_mutex);
@@ -725,8 +740,18 @@ static ssize_t xillybus_read(struct file *filp, char __user *userbuf,
 			bufidx = channel->wr_host_buf_idx;
 			bufpos = channel->wr_host_buf_pos;
 			howmany = ((channel->wr_buffers[bufidx]->end_offset
-				    + 1) << channel->log2_element_size)
-				- bufpos;
+				    + 1) << channel->log2_element_size);
+
+			if (howmany > channel->wr_buf_size ||
+			    howmany < bufpos) {
+				dev_err(channel->endpoint->dev,
+					"Illegal buffer fill level from hardware\n");
+				channel->endpoint->fatal_error = 1;
+				spin_unlock_irqrestore(&channel->wr_spinlock, flags);
+				break;
+			}
+
+			howmany	-= bufpos;
 
 			/* Update wr_host_* to its post-operation state */
 			if (howmany > bytes_to_do) {
@@ -1216,6 +1241,9 @@ static ssize_t xillybus_write(struct file *filp, const char __user *userbuf,
 	if (channel->endpoint->fatal_error)
 		return -EIO;
 
+	if (count > XILLYBUS_MAX_COUNT)
+		count = XILLYBUS_MAX_COUNT;
+
 	rc = mutex_lock_interruptible(&channel->rd_mutex);
 	if (rc)
 		return rc;
@@ -1902,6 +1930,11 @@ int xillybus_endpoint_discovery(struct xilly_endpoint *endpoint)
 		return -ENODEV;
 	}
 
+	if (endpoint->idtlen < 4 || endpoint->idtlen > XILLYBUS_MAX_IDT) {
+		dev_err(endpoint->dev, "Invalid IDT length. Aborting.\n");
+		return -ENODEV;
+	}
+
 	/* Enable DMA */
 	iowrite32((u32) (0x0002 | (endpoint->dma_using_dac & 0x0001)),
 		  endpoint->registers + fpga_dma_control_reg);
diff --git a/drivers/char/xillybus/xillybus_of.c b/drivers/char/xillybus/xillybus_of.c
index 1a1e64133315..22dcc387a5e4 100644
--- a/drivers/char/xillybus/xillybus_of.c
+++ b/drivers/char/xillybus/xillybus_of.c
@@ -53,6 +53,12 @@ static int xilly_drv_probe(struct platform_device *op)
 
 	irq = platform_get_irq(op, 0);
 
+	if (irq < 0) {
+		dev_err(endpoint->dev,
+			"Failed to obtain IRQ number. Aborting.\n");
+		return irq;
+	}
+
 	rc = devm_request_irq(dev, irq, xillybus_isr, 0, xillyname, endpoint);
 
 	if (rc) {
diff --git a/drivers/char/xillybus/xillyusb.c b/drivers/char/xillybus/xillyusb.c
index aa08206a18ef..7459ec9295af 100644
--- a/drivers/char/xillybus/xillyusb.c
+++ b/drivers/char/xillybus/xillyusb.c
@@ -396,6 +396,10 @@ static int fifo_init(struct xillyfifo *fifo,
 	fifo->size = fifo->bufnum * fifo->bufsize;
 	fifo->buf_order = buf_order;
 
+	if (!fifo->size || /* Unsigned integer overflow */
+	    fifo->size > 0x40000000) /* Stay clear from signed int issues */
+		return -ENOMEM; /* Reported as greed for memory */
+
 	fifo->mem = kmalloc_array(fifo->bufnum, sizeof(void *), GFP_KERNEL);
 
 	if (!fifo->mem)
@@ -888,6 +892,7 @@ static int process_in_opcode(struct xillyusb_dev *xdev,
 	struct xillyusb_channel *chan;
 	struct device *dev = xdev->dev;
 	int chan_idx = chan_num >> 1;
+	struct xillyfifo *in_fifo;
 
 	if (chan_idx >= xdev->num_channels) {
 		dev_err(dev, "Received illegal channel ID %d from FPGA\n",
@@ -912,7 +917,10 @@ static int process_in_opcode(struct xillyusb_dev *xdev,
 		 */
 		smp_wmb();
 		WRITE_ONCE(chan->read_data_ok, 0);
-		wake_up_interruptible(&chan->in_fifo->waitq);
+
+		in_fifo = READ_ONCE(chan->in_fifo);
+		if (in_fifo)
+			wake_up_interruptible(&in_fifo->waitq);
 		break;
 
 	case OPCODE_REACHED_CHECKPOINT:
@@ -1443,6 +1451,9 @@ static ssize_t xillyusb_read(struct file *filp, char __user *userbuf,
 	bool sent_set_push = false;
 	int rc;
 
+	if (count > XILLYBUS_MAX_COUNT)
+		count = XILLYBUS_MAX_COUNT;
+
 	deadline = jiffies + 1 + XILLY_RX_TIMEOUT;
 
 	rc = mutex_lock_interruptible(&chan->in_mutex);
@@ -1649,6 +1660,9 @@ static ssize_t xillyusb_write(struct file *filp, const char __user *userbuf,
 	struct xillyfifo *fifo = &chan->out_ep->fifo;
 	int rc;
 
+	if (count > XILLYBUS_MAX_COUNT)
+		count = XILLYBUS_MAX_COUNT;
+
 	rc = mutex_lock_interruptible(&chan->out_mutex);
 
 	if (rc)
@@ -2072,6 +2086,13 @@ static int xillyusb_discovery(struct usb_interface *interface)
 	}
 
 	idt_len = READ_ONCE(idt_fifo.fill);
+
+	if (idt_len < 4 || idt_len > XILLYBUS_MAX_IDT) {
+		rc = -ENODEV;
+		dev_err(&interface->dev, "Invalid IDT length. Aborting.\n");
+		goto unfifo;
+	}
+
 	idt = kmalloc(idt_len, GFP_KERNEL);
 
 	if (!idt) {
@@ -2106,6 +2127,12 @@ static int xillyusb_discovery(struct usb_interface *interface)
 		goto unidt;
 	}
 
+	if (num_channels == 0 || num_channels > XILLYBUS_MAX_NODES) {
+		dev_err(&interface->dev, "Unreasonable number of channels. Aborting.\n");
+		rc = -ENODEV;
+		goto unidt;
+	}
+
 	rc = setup_channels(xdev, (void *)idt + 3, num_channels);
 
 	if (rc)
-- 
2.34.1

[PATCH 7/7] char: xillybus: Ignore and report unsolicited interrupts

[Eli Billauer]

During initialization, the hardware should issue interrupts only in
response to requests from the host. Ignore and log unexpected interrupts,
as these indicate misbehaving hardware.

In the same spirit, in xilly_quiesce(), assign endpoint->num_channels = 0
before allowing the ISR, in order to expose whether the hardware
incorrectly sends messages related to data channels during shutdown.

Signed-off-by: Eli Billauer <eli.billauer@gmail.com>
---
 drivers/char/xillybus/xillybus.h      |  3 ++
 drivers/char/xillybus/xillybus_core.c | 46 ++++++++++++++++++++++++++-
 2 files changed, 48 insertions(+), 1 deletion(-)

diff --git a/drivers/char/xillybus/xillybus.h b/drivers/char/xillybus/xillybus.h
index 51de7cbc579e..98c7ac4dd1f9 100644
--- a/drivers/char/xillybus/xillybus.h
+++ b/drivers/char/xillybus/xillybus.h
@@ -94,6 +94,9 @@ struct xilly_endpoint {
 	__iomem void *registers;
 	int fatal_error;
 
+	bool allow_isr;
+	spinlock_t allow_isr_lock;
+
 	struct mutex register_mutex;
 	wait_queue_head_t ep_wait;
 
diff --git a/drivers/char/xillybus/xillybus_core.c b/drivers/char/xillybus/xillybus_core.c
index 3436f16092e0..ea0debe24968 100644
--- a/drivers/char/xillybus/xillybus_core.c
+++ b/drivers/char/xillybus/xillybus_core.c
@@ -72,6 +72,8 @@ static struct workqueue_struct *xillybus_wq;
  *
  * rd_spinlock does the same with rd_*_buf_idx, rd_empty and end_offset.
  *
+ * allow_isr_lock protects allow_isr.
+ *
  * register_mutex is endpoint-specific, and is held when non-atomic
  * register operations are performed. wr_mutex and rd_mutex may be
  * held when register_mutex is taken, but none of the spinlocks. Note that
@@ -84,7 +86,8 @@ static struct workqueue_struct *xillybus_wq;
  * Only interruptible blocking is allowed on mutexes and wait queues.
  *
  * All in all, the locking order goes (with skips allowed, of course):
- * wr_mutex -> rd_mutex -> register_mutex -> wr_spinlock -> rd_spinlock
+ * wr_mutex -> rd_mutex -> register_mutex ->
+ * allow_isr_lock -> wr_spinlock -> rd_spinlock
  */
 
 static void malformed_message(struct xilly_endpoint *endpoint, u32 *buf)
@@ -119,6 +122,13 @@ irqreturn_t xillybus_isr(int irq, void *data)
 	unsigned int msg_channel, msg_bufno, msg_data, msg_dir;
 	struct xilly_channel *channel;
 
+	guard(spinlock)(&ep->allow_isr_lock);
+
+	if (!ep->allow_isr) {
+		dev_err_ratelimited(ep->dev, "Unexpected interrupt! Something is wrong with the hardware.\n");
+		return IRQ_HANDLED;
+	}
+
 	buf = ep->msgbuf_addr;
 	buf_size = ep->msg_buf_size/sizeof(u32);
 
@@ -283,6 +293,19 @@ irqreturn_t xillybus_isr(int irq, void *data)
 }
 EXPORT_SYMBOL(xillybus_isr);
 
+/*
+ * xilly_allow_isr() is similar to enabling / disabling the interrupt,
+ * with the difference that if an interrupt is issued while ep->allow_isr
+ * is false, this is visible in the kernel log.
+ */
+
+static void xilly_allow_isr(struct xilly_endpoint *ep, bool newstate)
+{
+	guard(spinlock_irqsave)(&ep->allow_isr_lock);
+
+	ep->allow_isr = newstate;
+}
+
 /*
  * A few trivial memory management functions.
  * NOTE: These functions are used only on probe and remove, and therefore
@@ -651,6 +674,8 @@ static int xilly_obtain_idt(struct xilly_endpoint *endpoint)
 
 	channel->wr_sleepy = 1;
 
+	xilly_allow_isr(endpoint, true);
+
 	iowrite32(1 |
 		  (3 << 24), /* Opcode 3 for channel 0 = Send IDT */
 		  endpoint->registers + fpga_buf_ctrl_reg);
@@ -659,6 +684,8 @@ static int xilly_obtain_idt(struct xilly_endpoint *endpoint)
 					     (!channel->wr_sleepy),
 					     XILLY_TIMEOUT);
 
+	xilly_allow_isr(endpoint, false);
+
 	if (t <= 0) {
 		dev_err(endpoint->dev, "Failed to obtain IDT. Aborting.\n");
 
@@ -1843,6 +1870,9 @@ struct xilly_endpoint *xillybus_init_endpoint(struct device *dev)
 	endpoint->failed_messages = 0;
 	endpoint->fatal_error = 0;
 
+	endpoint->allow_isr = false;
+	spin_lock_init(&endpoint->allow_isr_lock);
+
 	init_waitqueue_head(&endpoint->ep_wait);
 	mutex_init(&endpoint->register_mutex);
 
@@ -1855,6 +1885,9 @@ static int xilly_quiesce(struct xilly_endpoint *endpoint)
 	long t;
 
 	endpoint->idtlen = -1;
+	endpoint->num_channels = 0;
+
+	xilly_allow_isr(endpoint, true);
 
 	iowrite32((u32) (endpoint->dma_using_dac & 0x0001),
 		  endpoint->registers + fpga_dma_control_reg);
@@ -1862,6 +1895,9 @@ static int xilly_quiesce(struct xilly_endpoint *endpoint)
 	t = wait_event_interruptible_timeout(endpoint->ep_wait,
 					     (endpoint->idtlen >= 0),
 					     XILLY_TIMEOUT);
+
+	xilly_allow_isr(endpoint, false);
+
 	if (t <= 0) {
 		dev_err(endpoint->dev,
 			"Failed to quiesce the device on exit.\n");
@@ -1915,6 +1951,8 @@ int xillybus_endpoint_discovery(struct xilly_endpoint *endpoint)
 
 	endpoint->idtlen = -1;
 
+	xilly_allow_isr(endpoint, true);
+
 	/*
 	 * Set DMA 32/64 bit mode, quiesce the device (?!) and get IDT
 	 * buffer size.
@@ -1925,6 +1963,9 @@ int xillybus_endpoint_discovery(struct xilly_endpoint *endpoint)
 	t = wait_event_interruptible_timeout(endpoint->ep_wait,
 					     (endpoint->idtlen >= 0),
 					     XILLY_TIMEOUT);
+
+	xilly_allow_isr(endpoint, false);
+
 	if (t <= 0) {
 		dev_err(endpoint->dev, "No response from FPGA. Aborting.\n");
 		return -ENODEV;
@@ -1951,6 +1992,7 @@ int xillybus_endpoint_discovery(struct xilly_endpoint *endpoint)
 	if (rc)
 		goto failed_idt;
 
+	/* xilly_obtain_idt() allows and then disallows the ISR */
 	rc = xilly_obtain_idt(endpoint);
 	if (rc)
 		goto failed_idt;
@@ -1969,6 +2011,8 @@ int xillybus_endpoint_discovery(struct xilly_endpoint *endpoint)
 	if (rc)
 		goto failed_idt;
 
+	xilly_allow_isr(endpoint, true);
+
 	rc = xillybus_init_chrdev(dev, &xillybus_fops,
 				  endpoint->owner, endpoint,
 				  idt_handle.names,
-- 
2.34.1