[PATCH 1/3] char: lp: ensure that index has not exceeded LP_NO

[PATCH 1/3] char: lp: ensure that index has not exceeded LP_NO

[Shreenidhi Shedi]

After finishing the loop, index value can be equal to LP_NO and lp_table
array is of size LP_NO, so this can end up in accessing an out of bound
address in lp_register function.

Signed-off-by: Shreenidhi Shedi <sshedi@vmware.com>
---
 drivers/char/lp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/char/lp.c b/drivers/char/lp.c
index bd95aba1f..e61060f3c 100644
--- a/drivers/char/lp.c
+++ b/drivers/char/lp.c
@@ -971,7 +971,7 @@ static void lp_attach(struct parport *port)
 			if (port_num[i] == -1)
 				break;
 
-		if (!lp_register(i, port))
+		if (i < LP_NO && !lp_register(i, port))
 			lp_count++;
 		break;
 
-- 
2.36.1

[PATCH 2/3] char: lp: remove redundant initialization of err

[Shreenidhi Shedi]

err is getting assigned with an appropriate value before returning,
hence this initialization is unnecessary.

Signed-off-by: Shreenidhi Shedi <sshedi@vmware.com>
---
 drivers/char/lp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/char/lp.c b/drivers/char/lp.c
index e61060f3c..5e8bd8d6c 100644
--- a/drivers/char/lp.c
+++ b/drivers/char/lp.c
@@ -1018,7 +1018,7 @@ static struct parport_driver lp_driver = {
 
 static int __init lp_init(void)
 {
-	int i, err = 0;
+	int i, err;
 
 	if (parport_nr[0] == LP_PARPORT_OFF)
 		return 0;
-- 
2.36.1

Re: [PATCH 2/3] char: lp: remove redundant initialization of err

[Greg KH]

On Sun, May 15, 2022 at 01:24:54PM +0530, Shreenidhi Shedi wrote:
> err is getting assigned with an appropriate value before returning,
> hence this initialization is unnecessary.
> 
> Signed-off-by: Shreenidhi Shedi <sshedi@vmware.com>
> ---
>  drivers/char/lp.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/char/lp.c b/drivers/char/lp.c
> index e61060f3c..5e8bd8d6c 100644
> --- a/drivers/char/lp.c
> +++ b/drivers/char/lp.c
> @@ -1018,7 +1018,7 @@ static struct parport_driver lp_driver = {
>  
>  static int __init lp_init(void)
>  {
> -	int i, err = 0;
> +	int i, err;
>  
>  	if (parport_nr[0] == LP_PARPORT_OFF)
>  		return 0;
> -- 
> 2.36.1
> 

Hi,

This is the friendly patch-bot of Greg Kroah-Hartman.  You have sent him
a patch that has triggered this response.  He used to manually respond
to these common problems, but in order to save his sanity (he kept
writing the same thing over and over, yet to different people), I was
created.  Hopefully you will not take offence and will fix the problem
in your patch and resubmit it so that it can be accepted into the Linux
kernel tree.

You are receiving this message because of the following common error(s)
as indicated below:

- It looks like you did not use your "real" name for the patch on either
  the Signed-off-by: line, or the From: line (both of which have to
  match).  Please read the kernel file, Documentation/SubmittingPatches
  for how to do this correctly.

- This looks like a new version of a previously submitted patch, but you
  did not list below the --- line any changes from the previous version.
  Please read the section entitled "The canonical patch format" in the
  kernel file, Documentation/SubmittingPatches for what needs to be done
  here to properly describe this.

If you wish to discuss this problem further, or you have questions about
how to resolve this issue, please feel free to respond to this email and
Greg will reply once he has dug out from the pending patches received
from other developers.

thanks,

greg k-h's patch email bot

[PATCH 3/3] char: lp: fix most of checkspec.pl warnings & errors

[Shreenidhi Shedi]

This commit fixes almost all styling issues reported by checkpatch.pl
script.

Signed-off-by: Shreenidhi Shedi <sshedi@vmware.com>
---
 drivers/char/lp.c | 338 ++++++++++++++++++++++++----------------------
 1 file changed, 177 insertions(+), 161 deletions(-)

diff --git a/drivers/char/lp.c b/drivers/char/lp.c
index 5e8bd8d6c..2efebdfac 100644
--- a/drivers/char/lp.c
+++ b/drivers/char/lp.c
@@ -16,7 +16,7 @@
  * Support for parport by Philip Blundell <philb@gnu.org>
  * Parport sharing hacking by Andrea Arcangeli
  * Fixed kernel_(to/from)_user memory copy to check for errors
- * 				by Riccardo Facchetti <fizban@tin.it>
+ *				by Riccardo Facchetti <fizban@tin.it>
  * 22-JAN-1998  Added support for devfs  Richard Gooch <rgooch@atnf.csiro.au>
  * Redesigned interrupt handling for handle printers with buggy handshake
  *				by Andrea Arcangeli, 11 May 1998
@@ -30,7 +30,7 @@
  * CAREFUL will block a bit after in lp_check_status().
  *				Andrea Arcangeli, 15 Oct 1998
  * Obsoleted and removed all the lowlevel stuff implemented in the last
- * month to use the IEEE1284 functions (that handle the _new_ compatibilty
+ * month to use the IEEE1284 functions (that handle the _new_ compatibility
  * mode fine).
  */
 
@@ -144,7 +144,7 @@ static DEFINE_MUTEX(lp_mutex);
 static struct lp_struct lp_table[LP_NO];
 static int port_num[LP_NO];
 
-static unsigned int lp_count = 0;
+static unsigned int lp_count;
 static struct class *lp_class;
 
 #ifdef CONFIG_LP_CONSOLE
@@ -161,35 +161,31 @@ static struct parport *console_registered;
 
 #define r_dtr(x)	(parport_read_data(lp_table[(x)].dev->port))
 #define r_str(x)	(parport_read_status(lp_table[(x)].dev->port))
-#define w_ctr(x,y)	do { parport_write_control(lp_table[(x)].dev->port, (y)); } while (0)
-#define w_dtr(x,y)	do { parport_write_data(lp_table[(x)].dev->port, (y)); } while (0)
+#define w_ctr(x, y)	(parport_write_control(lp_table[(x)].dev->port, (y)))
+#define w_dtr(x, y)	(parport_write_data(lp_table[(x)].dev->port, (y)))
 
 /* Claim the parport or block trying unless we've already claimed it */
 static void lp_claim_parport_or_block(struct lp_struct *this_lp)
 {
-	if (!test_and_set_bit(LP_PARPORT_CLAIMED, &this_lp->bits)) {
+	if (!test_and_set_bit(LP_PARPORT_CLAIMED, &this_lp->bits))
 		parport_claim_or_block(this_lp->dev);
-	}
 }
 
 /* Claim the parport or block trying unless we've already claimed it */
 static void lp_release_parport(struct lp_struct *this_lp)
 {
-	if (test_and_clear_bit(LP_PARPORT_CLAIMED, &this_lp->bits)) {
+	if (test_and_clear_bit(LP_PARPORT_CLAIMED, &this_lp->bits))
 		parport_release(this_lp->dev);
-	}
 }
 
-
-
 static int lp_preempt(void *handle)
 {
 	struct lp_struct *this_lp = (struct lp_struct *)handle;
+
 	set_bit(LP_PREEMPT_REQUEST, &this_lp->bits);
 	return 1;
 }
 
-
 /*
  * Try to negotiate to a new mode; if unsuccessful negotiate to
  * compatibility mode.  Return the mode we ended up in.
@@ -207,12 +203,14 @@ static int lp_negotiate(struct parport *port, int mode)
 static int lp_reset(int minor)
 {
 	int retval;
+
 	lp_claim_parport_or_block(&lp_table[minor]);
 	w_ctr(minor, LP_PSELECP);
 	udelay(LP_DELAY);
 	w_ctr(minor, LP_PSELECP | LP_PINITP);
 	retval = r_str(minor);
 	lp_release_parport(&lp_table[minor]);
+
 	return retval;
 }
 
@@ -241,30 +239,34 @@ static int lp_check_status(int minor)
 	int error = 0;
 	unsigned int last = lp_table[minor].last_error;
 	unsigned char status = r_str(minor);
+
 	if ((status & LP_PERRORP) && !(LP_F(minor) & LP_CAREFUL))
 		/* No error. */
 		last = 0;
 	else if ((status & LP_POUTPA)) {
 		if (last != LP_POUTPA) {
 			last = LP_POUTPA;
-			printk(KERN_INFO "lp%d out of paper\n", minor);
+			pr_info("lp%d out of paper\n", minor);
 		}
 		error = -ENOSPC;
 	} else if (!(status & LP_PSELECD)) {
 		if (last != LP_PSELECD) {
 			last = LP_PSELECD;
-			printk(KERN_INFO "lp%d off-line\n", minor);
+			pr_info("lp%d off-line\n", minor);
 		}
 		error = -EIO;
 	} else if (!(status & LP_PERRORP)) {
 		if (last != LP_PERRORP) {
 			last = LP_PERRORP;
-			printk(KERN_INFO "lp%d on fire\n", minor);
+			pr_info("lp%d on fire\n", minor);
 		}
 		error = -EIO;
 	} else {
-		last = 0; /* Come here if LP_CAREFUL is set and no
-			     errors are reported. */
+		/*
+		 * Come here if LP_CAREFUL is set and no
+		 * errors are reported.
+		 */
+		last = 0;
 	}
 
 	lp_table[minor].last_error = last;
@@ -280,9 +282,8 @@ static int lp_wait_ready(int minor, int nonblock)
 	int error = 0;
 
 	/* If we're not in compatibility mode, we're ready now! */
-	if (lp_table[minor].current_mode != IEEE1284_MODE_COMPAT) {
+	if (lp_table[minor].current_mode != IEEE1284_MODE_COMPAT)
 		return 0;
-	}
 
 	do {
 		error = lp_check_status(minor);
@@ -338,68 +339,69 @@ static ssize_t lp_write(struct file *file, const char __user *buf,
 			    (nonblock ? PARPORT_INACTIVITY_O_NONBLOCK
 			     : lp_table[minor].timeout));
 
-	if ((retv = lp_wait_ready(minor, nonblock)) == 0)
-	do {
-		/* Write the data. */
-		written = parport_write(port, kbuf, copy_size);
-		if (written > 0) {
-			copy_size -= written;
-			count -= written;
-			buf  += written;
-			retv += written;
-		}
+	retv = lp_wait_ready(minor, nonblock);
+	if (retv == 0)
+		do {
+			/* Write the data. */
+			written = parport_write(port, kbuf, copy_size);
+			if (written > 0) {
+				copy_size -= written;
+				count -= written;
+				buf  += written;
+				retv += written;
+			}
 
-		if (signal_pending(current)) {
-			if (retv == 0)
-				retv = -EINTR;
+			if (signal_pending(current)) {
+				if (retv == 0)
+					retv = -EINTR;
 
-			break;
-		}
+				break;
+			}
 
-		if (copy_size > 0) {
-			/* incomplete write -> check error ! */
-			int error;
+			if (copy_size > 0) {
+				/* incomplete write -> check error ! */
+				int error;
 
-			parport_negotiate(lp_table[minor].dev->port,
-					  IEEE1284_MODE_COMPAT);
-			lp_table[minor].current_mode = IEEE1284_MODE_COMPAT;
+				parport_negotiate(lp_table[minor].dev->port,
+						  IEEE1284_MODE_COMPAT);
+				lp_table[minor].current_mode = IEEE1284_MODE_COMPAT;
 
-			error = lp_wait_ready(minor, nonblock);
+				error = lp_wait_ready(minor, nonblock);
 
-			if (error) {
-				if (retv == 0)
-					retv = error;
-				break;
-			} else if (nonblock) {
-				if (retv == 0)
-					retv = -EAGAIN;
-				break;
-			}
+				if (error) {
+					if (retv == 0)
+						retv = error;
+					break;
+				} else if (nonblock) {
+					if (retv == 0)
+						retv = -EAGAIN;
+					break;
+				}
 
-			parport_yield_blocking(lp_table[minor].dev);
-			lp_table[minor].current_mode
-			  = lp_negotiate(port,
-					 lp_table[minor].best_mode);
+				parport_yield_blocking(lp_table[minor].dev);
+				lp_table[minor].current_mode
+				  = lp_negotiate(port,
+						 lp_table[minor].best_mode);
 
-		} else if (need_resched())
-			schedule();
+			} else if (need_resched())
+				schedule();
 
-		if (count) {
-			copy_size = count;
-			if (copy_size > LP_BUFFER_SIZE)
-				copy_size = LP_BUFFER_SIZE;
+			if (count) {
+				copy_size = count;
+				if (copy_size > LP_BUFFER_SIZE)
+					copy_size = LP_BUFFER_SIZE;
 
-			if (copy_from_user(kbuf, buf, copy_size)) {
-				if (retv == 0)
-					retv = -EFAULT;
-				break;
+				if (copy_from_user(kbuf, buf, copy_size)) {
+					if (retv == 0)
+						retv = -EFAULT;
+					break;
+				}
 			}
-		}
-	} while (count > 0);
+		} while (count > 0);
 
 	if (test_and_clear_bit(LP_PREEMPT_REQUEST,
 			       &lp_table[minor].bits)) {
-		printk(KERN_INFO "lp%d releasing parport\n", minor);
+		pr_info("lp%d releasing parport\n", minor);
 		parport_negotiate(lp_table[minor].dev->port,
 				  IEEE1284_MODE_COMPAT);
 		lp_table[minor].current_mode = IEEE1284_MODE_COMPAT;
@@ -418,7 +420,8 @@ static ssize_t lp_read(struct file *file, char __user *buf,
 		       size_t count, loff_t *ppos)
 {
 	DEFINE_WAIT(wait);
-	unsigned int minor=iminor(file_inode(file));
+
+	unsigned int minor = iminor(file_inode(file));
 	struct parport *port = lp_table[minor].dev->port;
 	ssize_t retval = 0;
 	char *kbuf = lp_table[minor].lp_buffer;
@@ -511,28 +514,32 @@ static int lp_open(struct inode *inode, struct file *file)
 		ret = -EBUSY;
 		goto out;
 	}
-	/* If ABORTOPEN is set and the printer is offline or out of paper,
-	   we may still want to open it to perform ioctl()s.  Therefore we
-	   have commandeered O_NONBLOCK, even though it is being used in
-	   a non-standard manner.  This is strictly a Linux hack, and
-	   should most likely only ever be used by the tunelp application. */
+	/*
+	 * If ABORTOPEN is set and the printer is offline or out of paper,
+	 * we may still want to open it to perform ioctl()s. Therefore we
+	 * have commandeered O_NONBLOCK, even though it is being used in
+	 * a non-standard manner.  This is strictly a Linux hack, and
+	 * should most likely only ever be used by the tunelp application.
+	 */
 	if ((LP_F(minor) & LP_ABORTOPEN) && !(file->f_flags & O_NONBLOCK)) {
 		int status;
+
 		lp_claim_parport_or_block(&lp_table[minor]);
 		status = r_str(minor);
 		lp_release_parport(&lp_table[minor]);
+
 		if (status & LP_POUTPA) {
-			printk(KERN_INFO "lp%d out of paper\n", minor);
+			pr_info("lp%d out of paper\n", minor);
 			LP_F(minor) &= ~LP_BUSY;
 			ret = -ENOSPC;
 			goto out;
 		} else if (!(status & LP_PSELECD)) {
-			printk(KERN_INFO "lp%d off-line\n", minor);
+			pr_info("lp%d off-line\n", minor);
 			LP_F(minor) &= ~LP_BUSY;
 			ret = -EIO;
 			goto out;
 		} else if (!(status & LP_PERRORP)) {
-			printk(KERN_ERR "lp%d printer error\n", minor);
+			pr_err("lp%d printer error\n", minor);
 			LP_F(minor) &= ~LP_BUSY;
 			ret = -EIO;
 			goto out;
@@ -591,75 +598,74 @@ static int lp_do_ioctl(unsigned int minor, unsigned int cmd,
 	if ((LP_F(minor) & LP_EXIST) == 0)
 		return -ENODEV;
 	switch ( cmd ) {
-		case LPTIME:
-			if (arg > UINT_MAX / HZ)
-				return -EINVAL;
-			LP_TIME(minor) = arg * HZ/100;
-			break;
-		case LPCHAR:
-			LP_CHAR(minor) = arg;
-			break;
-		case LPABORT:
-			if (arg)
-				LP_F(minor) |= LP_ABORT;
-			else
-				LP_F(minor) &= ~LP_ABORT;
-			break;
-		case LPABORTOPEN:
-			if (arg)
-				LP_F(minor) |= LP_ABORTOPEN;
-			else
-				LP_F(minor) &= ~LP_ABORTOPEN;
-			break;
-		case LPCAREFUL:
-			if (arg)
-				LP_F(minor) |= LP_CAREFUL;
-			else
-				LP_F(minor) &= ~LP_CAREFUL;
-			break;
-		case LPWAIT:
-			LP_WAIT(minor) = arg;
-			break;
-		case LPSETIRQ:
+	case LPTIME:
+		if (arg > UINT_MAX / HZ)
 			return -EINVAL;
-			break;
-		case LPGETIRQ:
-			if (copy_to_user(argp, &LP_IRQ(minor),
-					sizeof(int)))
-				return -EFAULT;
-			break;
-		case LPGETSTATUS:
-			if (mutex_lock_interruptible(&lp_table[minor].port_mutex))
-				return -EINTR;
-			lp_claim_parport_or_block(&lp_table[minor]);
-			status = r_str(minor);
-			lp_release_parport(&lp_table[minor]);
-			mutex_unlock(&lp_table[minor].port_mutex);
-
-			if (copy_to_user(argp, &status, sizeof(int)))
-				return -EFAULT;
-			break;
-		case LPRESET:
-			lp_reset(minor);
-			break;
+		LP_TIME(minor) = arg * HZ/100;
+		break;
+	case LPCHAR:
+		LP_CHAR(minor) = arg;
+		break;
+	case LPABORT:
+		if (arg)
+			LP_F(minor) |= LP_ABORT;
+		else
+			LP_F(minor) &= ~LP_ABORT;
+		break;
+	case LPABORTOPEN:
+		if (arg)
+			LP_F(minor) |= LP_ABORTOPEN;
+		else
+			LP_F(minor) &= ~LP_ABORTOPEN;
+		break;
+	case LPCAREFUL:
+		if (arg)
+			LP_F(minor) |= LP_CAREFUL;
+		else
+			LP_F(minor) &= ~LP_CAREFUL;
+		break;
+	case LPWAIT:
+		LP_WAIT(minor) = arg;
+		break;
+	case LPSETIRQ:
+		return -EINVAL;
+	case LPGETIRQ:
+		if (copy_to_user(argp, &LP_IRQ(minor),
+				sizeof(int)))
+			return -EFAULT;
+		break;
+	case LPGETSTATUS:
+		if (mutex_lock_interruptible(&lp_table[minor].port_mutex))
+			return -EINTR;
+		lp_claim_parport_or_block(&lp_table[minor]);
+		status = r_str(minor);
+		lp_release_parport(&lp_table[minor]);
+		mutex_unlock(&lp_table[minor].port_mutex);
+
+		if (copy_to_user(argp, &status, sizeof(int)))
+			return -EFAULT;
+		break;
+	case LPRESET:
+		lp_reset(minor);
+		break;
 #ifdef LP_STATS
-		case LPGETSTATS:
-			if (copy_to_user(argp, &LP_STAT(minor),
-					sizeof(struct lp_stats)))
-				return -EFAULT;
-			if (capable(CAP_SYS_ADMIN))
-				memset(&LP_STAT(minor), 0,
-						sizeof(struct lp_stats));
-			break;
+	case LPGETSTATS:
+		if (copy_to_user(argp, &LP_STAT(minor),
+				sizeof(struct lp_stats)))
+			return -EFAULT;
+		if (capable(CAP_SYS_ADMIN))
+			memset(&LP_STAT(minor), 0,
+					sizeof(struct lp_stats));
+		break;
 #endif
-		case LPGETFLAGS:
-			status = LP_F(minor);
-			if (copy_to_user(argp, &status, sizeof(int)))
-				return -EFAULT;
-			break;
+	case LPGETFLAGS:
+		status = LP_F(minor);
+		if (copy_to_user(argp, &status, sizeof(int)))
+			return -EFAULT;
+		break;
 
-		default:
-			retval = -EINVAL;
+	default:
+		retval = -EINVAL;
 	}
 	return retval;
 }
@@ -689,9 +695,9 @@ static int lp_set_timeout(unsigned int minor, s64 tv_sec, long tv_usec)
 		to_jiffies += tv_sec * (long) HZ;
 	}
 
-	if (to_jiffies <= 0) {
+	if (to_jiffies <= 0)
 		return -EINVAL;
-	}
+
 	lp_table[minor].timeout = to_jiffies;
 	return 0;
 }
@@ -803,15 +809,17 @@ static const struct file_operations lp_fops = {
 
 #define CONSOLE_LP 0
 
-/* If the printer is out of paper, we can either lose the messages or
+/*
+ * If the printer is out of paper, we can either lose the messages or
  * stall until the printer is happy again.  Define CONSOLE_LP_STRICT
- * non-zero to get the latter behaviour. */
+ * non-zero to get the latter behaviour.
+ */
 #define CONSOLE_LP_STRICT 1
 
 /* The console must be locked when we get here. */
 
 static void lp_console_write(struct console *co, const char *s,
-			     unsigned count)
+			     unsigned int count)
 {
 	struct pardevice *dev = lp_table[CONSOLE_LP].dev;
 	struct parport *port = dev->port;
@@ -830,6 +838,7 @@ static void lp_console_write(struct console *co, const char *s,
 		/* Write the data, converting LF->CRLF as we go. */
 		ssize_t canwrite = count;
 		char *lf = memchr(s, '\n', count);
+
 		if (lf)
 			canwrite = lf - s;
 
@@ -890,24 +899,28 @@ static int __init lp_setup(char *str)
 			/* disable driver on "lp=" or "lp=0" */
 			parport_nr[0] = LP_PARPORT_OFF;
 		} else {
-			printk(KERN_WARNING "warning: 'lp=0x%x' is deprecated, ignored\n", x);
+			pr_warn("warning: 'lp=0x%x' is deprecated, ignored\n", x);
 			return 0;
 		}
 	} else if (!strncmp(str, "parport", 7)) {
-		int n = simple_strtoul(str+7, NULL, 10);
+		int err;
+		unsigned long n = 0;
+
+		err = kstrtoul(str+7, 10, &n);
+		if (err)
+			return err;
+
 		if (parport_ptr < LP_NO)
 			parport_nr[parport_ptr++] = n;
 		else
-			printk(KERN_INFO "lp: too many ports, %s ignored.\n",
-			       str);
+			pr_info("lp: too many ports, %s ignored.\n", str);
 	} else if (!strcmp(str, "auto")) {
 		parport_nr[0] = LP_PARPORT_AUTO;
 	} else if (!strcmp(str, "none")) {
 		if (parport_ptr < LP_NO)
 			parport_nr[parport_ptr++] = LP_PARPORT_NONE;
 		else
-			printk(KERN_INFO "lp: too many ports, %s ignored.\n",
-			       str);
+			pr_info("lp: too many ports, %s ignored.\n", str);
 	} else if (!strcmp(str, "reset")) {
 		reset = true;
 	}
@@ -934,7 +947,7 @@ static int lp_register(int nr, struct parport *port)
 	device_create(lp_class, port->dev, MKDEV(LP_MAJOR, nr), NULL,
 		      "lp%d", nr);
 
-	printk(KERN_INFO "lp%d: using %s (%s).\n", nr, port->name,
+	pr_info("lp%d: using %s (%s).\n", nr, port->name,
 	       (port->irq == PARPORT_IRQ_NONE)?"polling":"interrupt-driven");
 
 #ifdef CONFIG_LP_CONSOLE
@@ -942,9 +955,9 @@ static int lp_register(int nr, struct parport *port)
 		if (port->modes & PARPORT_MODE_SAFEININT) {
 			register_console(&lpcons);
 			console_registered = port;
-			printk(KERN_INFO "lp%d: console ready\n", CONSOLE_LP);
+			pr_info("lp%d: console ready\n", CONSOLE_LP);
 		} else
-			printk(KERN_ERR "lp%d: cannot run console on %s\n",
+			pr_err("lp%d: cannot run console on %s\n",
 			       CONSOLE_LP, port->name);
 	}
 #endif
@@ -964,7 +977,7 @@ static void lp_attach(struct parport *port)
 		    port->probe_info[0].class != PARPORT_CLASS_PRINTER)
 			return;
 		if (lp_count == LP_NO) {
-			printk(KERN_INFO "lp: ignoring parallel port (max. %d)\n",LP_NO);
+			pr_info("lp: ignoring parallel port (max. %d)\n", LP_NO);
 			return;
 		}
 		for (i = 0; i < LP_NO; i++)
@@ -1044,7 +1057,7 @@ static int __init lp_init(void)
 	}
 
 	if (register_chrdev(LP_MAJOR, "lp", &lp_fops)) {
-		printk(KERN_ERR "lp: unable to get major %d\n", LP_MAJOR);
+		pr_err("lp: unable to get major %d\n", LP_MAJOR);
 		return -EIO;
 	}
 
@@ -1055,16 +1068,16 @@ static int __init lp_init(void)
 	}
 
 	if (parport_register_driver(&lp_driver)) {
-		printk(KERN_ERR "lp: unable to register with parport\n");
+		pr_err("lp: unable to register with parport\n");
 		err = -EIO;
 		goto out_class;
 	}
 
 	if (!lp_count) {
-		printk(KERN_INFO "lp: driver loaded but no devices found\n");
+		pr_info("lp: driver loaded but no devices found\n");
 #ifndef CONFIG_PARPORT_1284
 		if (parport_nr[0] == LP_PARPORT_AUTO)
-			printk(KERN_INFO "lp: (is IEEE 1284 support enabled?)\n");
+			pr_info("lp: (is IEEE 1284 support enabled?)\n");
 #endif
 	}
 
@@ -1085,16 +1098,19 @@ static int __init lp_init_module(void)
 			parport_nr[0] = LP_PARPORT_AUTO;
 		else {
 			int n;
+
 			for (n = 0; n < LP_NO && parport[n]; n++) {
 				if (!strncmp(parport[n], "none", 4))
 					parport_nr[n] = LP_PARPORT_NONE;
 				else {
 					char *ep;
+					/* Don't update to kstrtoul, ep needs to be updated */
 					unsigned long r = simple_strtoul(parport[n], &ep, 0);
+
 					if (ep != parport[n])
 						parport_nr[n] = r;
 					else {
-						printk(KERN_ERR "lp: bad port specifier `%s'\n", parport[n]);
+						pr_err("lp: bad port specifier `%s'\n", parport[n]);
 						return -ENODEV;
 					}
 				}
-- 
2.36.1

Re: [PATCH 3/3] char: lp: fix most of checkspec.pl warnings & errors

[Greg KH]

On Sun, May 15, 2022 at 01:24:55PM +0530, Shreenidhi Shedi wrote:
> This commit fixes almost all styling issues reported by checkpatch.pl
> script.
> 
> Signed-off-by: Shreenidhi Shedi <sshedi@vmware.com>
> ---
>  drivers/char/lp.c | 338 ++++++++++++++++++++++++----------------------
>  1 file changed, 177 insertions(+), 161 deletions(-)
> 
> diff --git a/drivers/char/lp.c b/drivers/char/lp.c
> index 5e8bd8d6c..2efebdfac 100644
> --- a/drivers/char/lp.c
> +++ b/drivers/char/lp.c
> @@ -16,7 +16,7 @@
>   * Support for parport by Philip Blundell <philb@gnu.org>
>   * Parport sharing hacking by Andrea Arcangeli
>   * Fixed kernel_(to/from)_user memory copy to check for errors
> - * 				by Riccardo Facchetti <fizban@tin.it>
> + *				by Riccardo Facchetti <fizban@tin.it>
>   * 22-JAN-1998  Added support for devfs  Richard Gooch <rgooch@atnf.csiro.au>
>   * Redesigned interrupt handling for handle printers with buggy handshake
>   *				by Andrea Arcangeli, 11 May 1998
> @@ -30,7 +30,7 @@
>   * CAREFUL will block a bit after in lp_check_status().
>   *				Andrea Arcangeli, 15 Oct 1998
>   * Obsoleted and removed all the lowlevel stuff implemented in the last
> - * month to use the IEEE1284 functions (that handle the _new_ compatibilty
> + * month to use the IEEE1284 functions (that handle the _new_ compatibility
>   * mode fine).
>   */
>  
> @@ -144,7 +144,7 @@ static DEFINE_MUTEX(lp_mutex);
>  static struct lp_struct lp_table[LP_NO];
>  static int port_num[LP_NO];
>  
> -static unsigned int lp_count = 0;
> +static unsigned int lp_count;
>  static struct class *lp_class;
>  
>  #ifdef CONFIG_LP_CONSOLE
> @@ -161,35 +161,31 @@ static struct parport *console_registered;
>  
>  #define r_dtr(x)	(parport_read_data(lp_table[(x)].dev->port))
>  #define r_str(x)	(parport_read_status(lp_table[(x)].dev->port))
> -#define w_ctr(x,y)	do { parport_write_control(lp_table[(x)].dev->port, (y)); } while (0)
> -#define w_dtr(x,y)	do { parport_write_data(lp_table[(x)].dev->port, (y)); } while (0)
> +#define w_ctr(x, y)	(parport_write_control(lp_table[(x)].dev->port, (y)))
> +#define w_dtr(x, y)	(parport_write_data(lp_table[(x)].dev->port, (y)))
>  
>  /* Claim the parport or block trying unless we've already claimed it */
>  static void lp_claim_parport_or_block(struct lp_struct *this_lp)
>  {
> -	if (!test_and_set_bit(LP_PARPORT_CLAIMED, &this_lp->bits)) {
> +	if (!test_and_set_bit(LP_PARPORT_CLAIMED, &this_lp->bits))
>  		parport_claim_or_block(this_lp->dev);
> -	}
>  }
>  
>  /* Claim the parport or block trying unless we've already claimed it */
>  static void lp_release_parport(struct lp_struct *this_lp)
>  {
> -	if (test_and_clear_bit(LP_PARPORT_CLAIMED, &this_lp->bits)) {
> +	if (test_and_clear_bit(LP_PARPORT_CLAIMED, &this_lp->bits))
>  		parport_release(this_lp->dev);
> -	}
>  }
>  
> -
> -
>  static int lp_preempt(void *handle)
>  {
>  	struct lp_struct *this_lp = (struct lp_struct *)handle;
> +
>  	set_bit(LP_PREEMPT_REQUEST, &this_lp->bits);
>  	return 1;
>  }
>  
> -
>  /*
>   * Try to negotiate to a new mode; if unsuccessful negotiate to
>   * compatibility mode.  Return the mode we ended up in.
> @@ -207,12 +203,14 @@ static int lp_negotiate(struct parport *port, int mode)
>  static int lp_reset(int minor)
>  {
>  	int retval;
> +
>  	lp_claim_parport_or_block(&lp_table[minor]);
>  	w_ctr(minor, LP_PSELECP);
>  	udelay(LP_DELAY);
>  	w_ctr(minor, LP_PSELECP | LP_PINITP);
>  	retval = r_str(minor);
>  	lp_release_parport(&lp_table[minor]);
> +
>  	return retval;
>  }
>  
> @@ -241,30 +239,34 @@ static int lp_check_status(int minor)
>  	int error = 0;
>  	unsigned int last = lp_table[minor].last_error;
>  	unsigned char status = r_str(minor);
> +
>  	if ((status & LP_PERRORP) && !(LP_F(minor) & LP_CAREFUL))
>  		/* No error. */
>  		last = 0;
>  	else if ((status & LP_POUTPA)) {
>  		if (last != LP_POUTPA) {
>  			last = LP_POUTPA;
> -			printk(KERN_INFO "lp%d out of paper\n", minor);
> +			pr_info("lp%d out of paper\n", minor);
>  		}
>  		error = -ENOSPC;
>  	} else if (!(status & LP_PSELECD)) {
>  		if (last != LP_PSELECD) {
>  			last = LP_PSELECD;
> -			printk(KERN_INFO "lp%d off-line\n", minor);
> +			pr_info("lp%d off-line\n", minor);
>  		}
>  		error = -EIO;
>  	} else if (!(status & LP_PERRORP)) {
>  		if (last != LP_PERRORP) {
>  			last = LP_PERRORP;
> -			printk(KERN_INFO "lp%d on fire\n", minor);
> +			pr_info("lp%d on fire\n", minor);
>  		}
>  		error = -EIO;
>  	} else {
> -		last = 0; /* Come here if LP_CAREFUL is set and no
> -			     errors are reported. */
> +		/*
> +		 * Come here if LP_CAREFUL is set and no
> +		 * errors are reported.
> +		 */
> +		last = 0;
>  	}
>  
>  	lp_table[minor].last_error = last;
> @@ -280,9 +282,8 @@ static int lp_wait_ready(int minor, int nonblock)
>  	int error = 0;
>  
>  	/* If we're not in compatibility mode, we're ready now! */
> -	if (lp_table[minor].current_mode != IEEE1284_MODE_COMPAT) {
> +	if (lp_table[minor].current_mode != IEEE1284_MODE_COMPAT)
>  		return 0;
> -	}
>  
>  	do {
>  		error = lp_check_status(minor);
> @@ -338,68 +339,69 @@ static ssize_t lp_write(struct file *file, const char __user *buf,
>  			    (nonblock ? PARPORT_INACTIVITY_O_NONBLOCK
>  			     : lp_table[minor].timeout));
>  
> -	if ((retv = lp_wait_ready(minor, nonblock)) == 0)
> -	do {
> -		/* Write the data. */
> -		written = parport_write(port, kbuf, copy_size);
> -		if (written > 0) {
> -			copy_size -= written;
> -			count -= written;
> -			buf  += written;
> -			retv += written;
> -		}
> +	retv = lp_wait_ready(minor, nonblock);
> +	if (retv == 0)
> +		do {
> +			/* Write the data. */
> +			written = parport_write(port, kbuf, copy_size);
> +			if (written > 0) {
> +				copy_size -= written;
> +				count -= written;
> +				buf  += written;
> +				retv += written;
> +			}
>  
> -		if (signal_pending(current)) {
> -			if (retv == 0)
> -				retv = -EINTR;
> +			if (signal_pending(current)) {
> +				if (retv == 0)
> +					retv = -EINTR;
>  
> -			break;
> -		}
> +				break;
> +			}
>  
> -		if (copy_size > 0) {
> -			/* incomplete write -> check error ! */
> -			int error;
> +			if (copy_size > 0) {
> +				/* incomplete write -> check error ! */
> +				int error;
>  
> -			parport_negotiate(lp_table[minor].dev->port,
> -					  IEEE1284_MODE_COMPAT);
> -			lp_table[minor].current_mode = IEEE1284_MODE_COMPAT;
> +				parport_negotiate(lp_table[minor].dev->port,
> +						  IEEE1284_MODE_COMPAT);
> +				lp_table[minor].current_mode = IEEE1284_MODE_COMPAT;
>  
> -			error = lp_wait_ready(minor, nonblock);
> +				error = lp_wait_ready(minor, nonblock);
>  
> -			if (error) {
> -				if (retv == 0)
> -					retv = error;
> -				break;
> -			} else if (nonblock) {
> -				if (retv == 0)
> -					retv = -EAGAIN;
> -				break;
> -			}
> +				if (error) {
> +					if (retv == 0)
> +						retv = error;
> +					break;
> +				} else if (nonblock) {
> +					if (retv == 0)
> +						retv = -EAGAIN;
> +					break;
> +				}
>  
> -			parport_yield_blocking(lp_table[minor].dev);
> -			lp_table[minor].current_mode
> -			  = lp_negotiate(port,
> -					 lp_table[minor].best_mode);
> +				parport_yield_blocking(lp_table[minor].dev);
> +				lp_table[minor].current_mode
> +				  = lp_negotiate(port,
> +						 lp_table[minor].best_mode);
>  
> -		} else if (need_resched())
> -			schedule();
> +			} else if (need_resched())
> +				schedule();
>  
> -		if (count) {
> -			copy_size = count;
> -			if (copy_size > LP_BUFFER_SIZE)
> -				copy_size = LP_BUFFER_SIZE;
> +			if (count) {
> +				copy_size = count;
> +				if (copy_size > LP_BUFFER_SIZE)
> +					copy_size = LP_BUFFER_SIZE;
>  
> -			if (copy_from_user(kbuf, buf, copy_size)) {
> -				if (retv == 0)
> -					retv = -EFAULT;
> -				break;
> +				if (copy_from_user(kbuf, buf, copy_size)) {
> +					if (retv == 0)
> +						retv = -EFAULT;
> +					break;
> +				}
>  			}
> -		}
> -	} while (count > 0);
> +		} while (count > 0);
>  
>  	if (test_and_clear_bit(LP_PREEMPT_REQUEST,
>  			       &lp_table[minor].bits)) {
> -		printk(KERN_INFO "lp%d releasing parport\n", minor);
> +		pr_info("lp%d releasing parport\n", minor);
>  		parport_negotiate(lp_table[minor].dev->port,
>  				  IEEE1284_MODE_COMPAT);
>  		lp_table[minor].current_mode = IEEE1284_MODE_COMPAT;
> @@ -418,7 +420,8 @@ static ssize_t lp_read(struct file *file, char __user *buf,
>  		       size_t count, loff_t *ppos)
>  {
>  	DEFINE_WAIT(wait);
> -	unsigned int minor=iminor(file_inode(file));
> +
> +	unsigned int minor = iminor(file_inode(file));
>  	struct parport *port = lp_table[minor].dev->port;
>  	ssize_t retval = 0;
>  	char *kbuf = lp_table[minor].lp_buffer;
> @@ -511,28 +514,32 @@ static int lp_open(struct inode *inode, struct file *file)
>  		ret = -EBUSY;
>  		goto out;
>  	}
> -	/* If ABORTOPEN is set and the printer is offline or out of paper,
> -	   we may still want to open it to perform ioctl()s.  Therefore we
> -	   have commandeered O_NONBLOCK, even though it is being used in
> -	   a non-standard manner.  This is strictly a Linux hack, and
> -	   should most likely only ever be used by the tunelp application. */
> +	/*
> +	 * If ABORTOPEN is set and the printer is offline or out of paper,
> +	 * we may still want to open it to perform ioctl()s. Therefore we
> +	 * have commandeered O_NONBLOCK, even though it is being used in
> +	 * a non-standard manner.  This is strictly a Linux hack, and
> +	 * should most likely only ever be used by the tunelp application.
> +	 */
>  	if ((LP_F(minor) & LP_ABORTOPEN) && !(file->f_flags & O_NONBLOCK)) {
>  		int status;
> +
>  		lp_claim_parport_or_block(&lp_table[minor]);
>  		status = r_str(minor);
>  		lp_release_parport(&lp_table[minor]);
> +
>  		if (status & LP_POUTPA) {
> -			printk(KERN_INFO "lp%d out of paper\n", minor);
> +			pr_info("lp%d out of paper\n", minor);
>  			LP_F(minor) &= ~LP_BUSY;
>  			ret = -ENOSPC;
>  			goto out;
>  		} else if (!(status & LP_PSELECD)) {
> -			printk(KERN_INFO "lp%d off-line\n", minor);
> +			pr_info("lp%d off-line\n", minor);
>  			LP_F(minor) &= ~LP_BUSY;
>  			ret = -EIO;
>  			goto out;
>  		} else if (!(status & LP_PERRORP)) {
> -			printk(KERN_ERR "lp%d printer error\n", minor);
> +			pr_err("lp%d printer error\n", minor);
>  			LP_F(minor) &= ~LP_BUSY;
>  			ret = -EIO;
>  			goto out;
> @@ -591,75 +598,74 @@ static int lp_do_ioctl(unsigned int minor, unsigned int cmd,
>  	if ((LP_F(minor) & LP_EXIST) == 0)
>  		return -ENODEV;
>  	switch ( cmd ) {
> -		case LPTIME:
> -			if (arg > UINT_MAX / HZ)
> -				return -EINVAL;
> -			LP_TIME(minor) = arg * HZ/100;
> -			break;
> -		case LPCHAR:
> -			LP_CHAR(minor) = arg;
> -			break;
> -		case LPABORT:
> -			if (arg)
> -				LP_F(minor) |= LP_ABORT;
> -			else
> -				LP_F(minor) &= ~LP_ABORT;
> -			break;
> -		case LPABORTOPEN:
> -			if (arg)
> -				LP_F(minor) |= LP_ABORTOPEN;
> -			else
> -				LP_F(minor) &= ~LP_ABORTOPEN;
> -			break;
> -		case LPCAREFUL:
> -			if (arg)
> -				LP_F(minor) |= LP_CAREFUL;
> -			else
> -				LP_F(minor) &= ~LP_CAREFUL;
> -			break;
> -		case LPWAIT:
> -			LP_WAIT(minor) = arg;
> -			break;
> -		case LPSETIRQ:
> +	case LPTIME:
> +		if (arg > UINT_MAX / HZ)
>  			return -EINVAL;
> -			break;
> -		case LPGETIRQ:
> -			if (copy_to_user(argp, &LP_IRQ(minor),
> -					sizeof(int)))
> -				return -EFAULT;
> -			break;
> -		case LPGETSTATUS:
> -			if (mutex_lock_interruptible(&lp_table[minor].port_mutex))
> -				return -EINTR;
> -			lp_claim_parport_or_block(&lp_table[minor]);
> -			status = r_str(minor);
> -			lp_release_parport(&lp_table[minor]);
> -			mutex_unlock(&lp_table[minor].port_mutex);
> -
> -			if (copy_to_user(argp, &status, sizeof(int)))
> -				return -EFAULT;
> -			break;
> -		case LPRESET:
> -			lp_reset(minor);
> -			break;
> +		LP_TIME(minor) = arg * HZ/100;
> +		break;
> +	case LPCHAR:
> +		LP_CHAR(minor) = arg;
> +		break;
> +	case LPABORT:
> +		if (arg)
> +			LP_F(minor) |= LP_ABORT;
> +		else
> +			LP_F(minor) &= ~LP_ABORT;
> +		break;
> +	case LPABORTOPEN:
> +		if (arg)
> +			LP_F(minor) |= LP_ABORTOPEN;
> +		else
> +			LP_F(minor) &= ~LP_ABORTOPEN;
> +		break;
> +	case LPCAREFUL:
> +		if (arg)
> +			LP_F(minor) |= LP_CAREFUL;
> +		else
> +			LP_F(minor) &= ~LP_CAREFUL;
> +		break;
> +	case LPWAIT:
> +		LP_WAIT(minor) = arg;
> +		break;
> +	case LPSETIRQ:
> +		return -EINVAL;
> +	case LPGETIRQ:
> +		if (copy_to_user(argp, &LP_IRQ(minor),
> +				sizeof(int)))
> +			return -EFAULT;
> +		break;
> +	case LPGETSTATUS:
> +		if (mutex_lock_interruptible(&lp_table[minor].port_mutex))
> +			return -EINTR;
> +		lp_claim_parport_or_block(&lp_table[minor]);
> +		status = r_str(minor);
> +		lp_release_parport(&lp_table[minor]);
> +		mutex_unlock(&lp_table[minor].port_mutex);
> +
> +		if (copy_to_user(argp, &status, sizeof(int)))
> +			return -EFAULT;
> +		break;
> +	case LPRESET:
> +		lp_reset(minor);
> +		break;
>  #ifdef LP_STATS
> -		case LPGETSTATS:
> -			if (copy_to_user(argp, &LP_STAT(minor),
> -					sizeof(struct lp_stats)))
> -				return -EFAULT;
> -			if (capable(CAP_SYS_ADMIN))
> -				memset(&LP_STAT(minor), 0,
> -						sizeof(struct lp_stats));
> -			break;
> +	case LPGETSTATS:
> +		if (copy_to_user(argp, &LP_STAT(minor),
> +				sizeof(struct lp_stats)))
> +			return -EFAULT;
> +		if (capable(CAP_SYS_ADMIN))
> +			memset(&LP_STAT(minor), 0,
> +					sizeof(struct lp_stats));
> +		break;
>  #endif
> -		case LPGETFLAGS:
> -			status = LP_F(minor);
> -			if (copy_to_user(argp, &status, sizeof(int)))
> -				return -EFAULT;
> -			break;
> +	case LPGETFLAGS:
> +		status = LP_F(minor);
> +		if (copy_to_user(argp, &status, sizeof(int)))
> +			return -EFAULT;
> +		break;
>  
> -		default:
> -			retval = -EINVAL;
> +	default:
> +		retval = -EINVAL;
>  	}
>  	return retval;
>  }
> @@ -689,9 +695,9 @@ static int lp_set_timeout(unsigned int minor, s64 tv_sec, long tv_usec)
>  		to_jiffies += tv_sec * (long) HZ;
>  	}
>  
> -	if (to_jiffies <= 0) {
> +	if (to_jiffies <= 0)
>  		return -EINVAL;
> -	}
> +
>  	lp_table[minor].timeout = to_jiffies;
>  	return 0;
>  }
> @@ -803,15 +809,17 @@ static const struct file_operations lp_fops = {
>  
>  #define CONSOLE_LP 0
>  
> -/* If the printer is out of paper, we can either lose the messages or
> +/*
> + * If the printer is out of paper, we can either lose the messages or
>   * stall until the printer is happy again.  Define CONSOLE_LP_STRICT
> - * non-zero to get the latter behaviour. */
> + * non-zero to get the latter behaviour.
> + */
>  #define CONSOLE_LP_STRICT 1
>  
>  /* The console must be locked when we get here. */
>  
>  static void lp_console_write(struct console *co, const char *s,
> -			     unsigned count)
> +			     unsigned int count)
>  {
>  	struct pardevice *dev = lp_table[CONSOLE_LP].dev;
>  	struct parport *port = dev->port;
> @@ -830,6 +838,7 @@ static void lp_console_write(struct console *co, const char *s,
>  		/* Write the data, converting LF->CRLF as we go. */
>  		ssize_t canwrite = count;
>  		char *lf = memchr(s, '\n', count);
> +
>  		if (lf)
>  			canwrite = lf - s;
>  
> @@ -890,24 +899,28 @@ static int __init lp_setup(char *str)
>  			/* disable driver on "lp=" or "lp=0" */
>  			parport_nr[0] = LP_PARPORT_OFF;
>  		} else {
> -			printk(KERN_WARNING "warning: 'lp=0x%x' is deprecated, ignored\n", x);
> +			pr_warn("warning: 'lp=0x%x' is deprecated, ignored\n", x);
>  			return 0;
>  		}
>  	} else if (!strncmp(str, "parport", 7)) {
> -		int n = simple_strtoul(str+7, NULL, 10);
> +		int err;
> +		unsigned long n = 0;
> +
> +		err = kstrtoul(str+7, 10, &n);
> +		if (err)
> +			return err;
> +
>  		if (parport_ptr < LP_NO)
>  			parport_nr[parport_ptr++] = n;
>  		else
> -			printk(KERN_INFO "lp: too many ports, %s ignored.\n",
> -			       str);
> +			pr_info("lp: too many ports, %s ignored.\n", str);
>  	} else if (!strcmp(str, "auto")) {
>  		parport_nr[0] = LP_PARPORT_AUTO;
>  	} else if (!strcmp(str, "none")) {
>  		if (parport_ptr < LP_NO)
>  			parport_nr[parport_ptr++] = LP_PARPORT_NONE;
>  		else
> -			printk(KERN_INFO "lp: too many ports, %s ignored.\n",
> -			       str);
> +			pr_info("lp: too many ports, %s ignored.\n", str);
>  	} else if (!strcmp(str, "reset")) {
>  		reset = true;
>  	}
> @@ -934,7 +947,7 @@ static int lp_register(int nr, struct parport *port)
>  	device_create(lp_class, port->dev, MKDEV(LP_MAJOR, nr), NULL,
>  		      "lp%d", nr);
>  
> -	printk(KERN_INFO "lp%d: using %s (%s).\n", nr, port->name,
> +	pr_info("lp%d: using %s (%s).\n", nr, port->name,
>  	       (port->irq == PARPORT_IRQ_NONE)?"polling":"interrupt-driven");
>  
>  #ifdef CONFIG_LP_CONSOLE
> @@ -942,9 +955,9 @@ static int lp_register(int nr, struct parport *port)
>  		if (port->modes & PARPORT_MODE_SAFEININT) {
>  			register_console(&lpcons);
>  			console_registered = port;
> -			printk(KERN_INFO "lp%d: console ready\n", CONSOLE_LP);
> +			pr_info("lp%d: console ready\n", CONSOLE_LP);
>  		} else
> -			printk(KERN_ERR "lp%d: cannot run console on %s\n",
> +			pr_err("lp%d: cannot run console on %s\n",
>  			       CONSOLE_LP, port->name);
>  	}
>  #endif
> @@ -964,7 +977,7 @@ static void lp_attach(struct parport *port)
>  		    port->probe_info[0].class != PARPORT_CLASS_PRINTER)
>  			return;
>  		if (lp_count == LP_NO) {
> -			printk(KERN_INFO "lp: ignoring parallel port (max. %d)\n",LP_NO);
> +			pr_info("lp: ignoring parallel port (max. %d)\n", LP_NO);
>  			return;
>  		}
>  		for (i = 0; i < LP_NO; i++)
> @@ -1044,7 +1057,7 @@ static int __init lp_init(void)
>  	}
>  
>  	if (register_chrdev(LP_MAJOR, "lp", &lp_fops)) {
> -		printk(KERN_ERR "lp: unable to get major %d\n", LP_MAJOR);
> +		pr_err("lp: unable to get major %d\n", LP_MAJOR);
>  		return -EIO;
>  	}
>  
> @@ -1055,16 +1068,16 @@ static int __init lp_init(void)
>  	}
>  
>  	if (parport_register_driver(&lp_driver)) {
> -		printk(KERN_ERR "lp: unable to register with parport\n");
> +		pr_err("lp: unable to register with parport\n");
>  		err = -EIO;
>  		goto out_class;
>  	}
>  
>  	if (!lp_count) {
> -		printk(KERN_INFO "lp: driver loaded but no devices found\n");
> +		pr_info("lp: driver loaded but no devices found\n");
>  #ifndef CONFIG_PARPORT_1284
>  		if (parport_nr[0] == LP_PARPORT_AUTO)
> -			printk(KERN_INFO "lp: (is IEEE 1284 support enabled?)\n");
> +			pr_info("lp: (is IEEE 1284 support enabled?)\n");
>  #endif
>  	}
>  
> @@ -1085,16 +1098,19 @@ static int __init lp_init_module(void)
>  			parport_nr[0] = LP_PARPORT_AUTO;
>  		else {
>  			int n;
> +
>  			for (n = 0; n < LP_NO && parport[n]; n++) {
>  				if (!strncmp(parport[n], "none", 4))
>  					parport_nr[n] = LP_PARPORT_NONE;
>  				else {
>  					char *ep;
> +					/* Don't update to kstrtoul, ep needs to be updated */
>  					unsigned long r = simple_strtoul(parport[n], &ep, 0);
> +
>  					if (ep != parport[n])
>  						parport_nr[n] = r;
>  					else {
> -						printk(KERN_ERR "lp: bad port specifier `%s'\n", parport[n]);
> +						pr_err("lp: bad port specifier `%s'\n", parport[n]);
>  						return -ENODEV;
>  					}
>  				}
> -- 
> 2.36.1
> 


Hi,

This is the friendly patch-bot of Greg Kroah-Hartman.  You have sent him
a patch that has triggered this response.  He used to manually respond
to these common problems, but in order to save his sanity (he kept
writing the same thing over and over, yet to different people), I was
created.  Hopefully you will not take offence and will fix the problem
in your patch and resubmit it so that it can be accepted into the Linux
kernel tree.

You are receiving this message because of the following common error(s)
as indicated below:

- Your patch did many different things all at once, making it difficult
  to review.  All Linux kernel patches need to only do one thing at a
  time.  If you need to do multiple things (such as clean up all coding
  style issues in a file/driver), do it in a sequence of patches, each
  one doing only one thing.  This will make it easier to review the
  patches to ensure that they are correct, and to help alleviate any
  merge issues that larger patches can cause.

- It looks like you did not use your "real" name for the patch on either
  the Signed-off-by: line, or the From: line (both of which have to
  match).  Please read the kernel file, Documentation/SubmittingPatches
  for how to do this correctly.

- This looks like a new version of a previously submitted patch, but you
  did not list below the --- line any changes from the previous version.
  Please read the section entitled "The canonical patch format" in the
  kernel file, Documentation/SubmittingPatches for what needs to be done
  here to properly describe this.


If you wish to discuss this problem further, or you have questions about
how to resolve this issue, please feel free to respond to this email and
Greg will reply once he has dug out from the pending patches received
from other developers.

thanks,

greg k-h's patch email bot

Re: [PATCH 1/3] char: lp: ensure that index has not exceeded LP_NO

[Greg KH]

On Sun, May 15, 2022 at 01:24:53PM +0530, Shreenidhi Shedi wrote:
> After finishing the loop, index value can be equal to LP_NO and lp_table
> array is of size LP_NO, so this can end up in accessing an out of bound
> address in lp_register function.
> 
> Signed-off-by: Shreenidhi Shedi <sshedi@vmware.com>
> ---
>  drivers/char/lp.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/char/lp.c b/drivers/char/lp.c
> index bd95aba1f..e61060f3c 100644
> --- a/drivers/char/lp.c
> +++ b/drivers/char/lp.c
> @@ -971,7 +971,7 @@ static void lp_attach(struct parport *port)
>  			if (port_num[i] == -1)
>  				break;
>  
> -		if (!lp_register(i, port))
> +		if (i < LP_NO && !lp_register(i, port))
>  			lp_count++;
>  		break;
>  
> -- 
> 2.36.1
> 

Hi,

This is the friendly patch-bot of Greg Kroah-Hartman.  You have sent him
a patch that has triggered this response.  He used to manually respond
to these common problems, but in order to save his sanity (he kept
writing the same thing over and over, yet to different people), I was
created.  Hopefully you will not take offence and will fix the problem
in your patch and resubmit it so that it can be accepted into the Linux
kernel tree.

You are receiving this message because of the following common error(s)
as indicated below:

- This looks like a new version of a previously submitted patch, but you
  did not list below the --- line any changes from the previous version.
  Please read the section entitled "The canonical patch format" in the
  kernel file, Documentation/SubmittingPatches for what needs to be done
  here to properly describe this.

If you wish to discuss this problem further, or you have questions about
how to resolve this issue, please feel free to respond to this email and
Greg will reply once he has dug out from the pending patches received
from other developers.

thanks,

greg k-h's patch email bot

Re: [PATCH 1/3] char: lp: ensure that index has not exceeded LP_NO

[Shreenidhi Shedi]

On 15/05/22 1:24 pm, Shreenidhi Shedi wrote:
> After finishing the loop, index value can be equal to LP_NO and lp_table
> array is of size LP_NO, so this can end up in accessing an out of bound
> address in lp_register function.
> 
> Signed-off-by: Shreenidhi Shedi <sshedi@vmware.com>
> ---
>  drivers/char/lp.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/char/lp.c b/drivers/char/lp.c
> index bd95aba1f..e61060f3c 100644
> --- a/drivers/char/lp.c
> +++ b/drivers/char/lp.c
> @@ -971,7 +971,7 @@ static void lp_attach(struct parport *port)
>  			if (port_num[i] == -1)
>  				break;
>  
> -		if (!lp_register(i, port))
> +		if (i < LP_NO && !lp_register(i, port))
>  			lp_count++;
>  		break;
>  

Hi Greg and Arnd Bergmann,

Please review the above changes. Ignore second patch.

--
Shedi

Re: [PATCH 1/3] char: lp: ensure that index has not exceeded LP_NO

[Shreenidhi Shedi]

On 03/06/22 5:27 pm, Shreenidhi Shedi wrote:
> On 15/05/22 1:24 pm, Shreenidhi Shedi wrote:
>> After finishing the loop, index value can be equal to LP_NO and lp_table
>> array is of size LP_NO, so this can end up in accessing an out of bound
>> address in lp_register function.
>>
>> Signed-off-by: Shreenidhi Shedi <sshedi@vmware.com>
>> ---
>>  drivers/char/lp.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/char/lp.c b/drivers/char/lp.c
>> index bd95aba1f..e61060f3c 100644
>> --- a/drivers/char/lp.c
>> +++ b/drivers/char/lp.c
>> @@ -971,7 +971,7 @@ static void lp_attach(struct parport *port)
>>  			if (port_num[i] == -1)
>>  				break;
>>  
>> -		if (!lp_register(i, port))
>> +		if (i < LP_NO && !lp_register(i, port))
>>  			lp_count++;
>>  		break;
>>  
> 
> Hi Greg and Arnd Bergmann,
> 
> Please review the above changes. Ignore second patch.
> 
> --
> Shedi

Sorry, not the second patch. Ignore the 3rd patch which fixes checkspec.pl warnings.

--
Shedi

Re: [PATCH 1/3] char: lp: ensure that index has not exceeded LP_NO

[Greg KH]

On Fri, Jun 03, 2022 at 05:32:25PM +0530, Shreenidhi Shedi wrote:
> On 03/06/22 5:27 pm, Shreenidhi Shedi wrote:
> > On 15/05/22 1:24 pm, Shreenidhi Shedi wrote:
> >> After finishing the loop, index value can be equal to LP_NO and lp_table
> >> array is of size LP_NO, so this can end up in accessing an out of bound
> >> address in lp_register function.
> >>
> >> Signed-off-by: Shreenidhi Shedi <sshedi@vmware.com>
> >> ---
> >>  drivers/char/lp.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/char/lp.c b/drivers/char/lp.c
> >> index bd95aba1f..e61060f3c 100644
> >> --- a/drivers/char/lp.c
> >> +++ b/drivers/char/lp.c
> >> @@ -971,7 +971,7 @@ static void lp_attach(struct parport *port)
> >>  			if (port_num[i] == -1)
> >>  				break;
> >>  
> >> -		if (!lp_register(i, port))
> >> +		if (i < LP_NO && !lp_register(i, port))
> >>  			lp_count++;
> >>  		break;
> >>  
> > 
> > Hi Greg and Arnd Bergmann,
> > 
> > Please review the above changes. Ignore second patch.
> > 
> > --
> > Shedi
> 
> Sorry, not the second patch. Ignore the 3rd patch which fixes checkspec.pl warnings.

I have no way to do that, please fix up and resend the correct series,
our tools can not pick and choose from a series easily.

Also remember this is the middle of the merge window and we can not do
anything at the moment.

greg k-h

Re: [PATCH 1/3] char: lp: ensure that index has not exceeded LP_NO

[Shreenidhi Shedi]

On 03/06/22 6:14 pm, Greg KH wrote:
> On Fri, Jun 03, 2022 at 05:32:25PM +0530, Shreenidhi Shedi wrote:
>> On 03/06/22 5:27 pm, Shreenidhi Shedi wrote:
>>> On 15/05/22 1:24 pm, Shreenidhi Shedi wrote:
>>>> After finishing the loop, index value can be equal to LP_NO and lp_table
>>>> array is of size LP_NO, so this can end up in accessing an out of bound
>>>> address in lp_register function.
>>>>
>>>> Signed-off-by: Shreenidhi Shedi <sshedi@vmware.com>
>>>> ---
>>>>  drivers/char/lp.c | 2 +-
>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/char/lp.c b/drivers/char/lp.c
>>>> index bd95aba1f..e61060f3c 100644
>>>> --- a/drivers/char/lp.c
>>>> +++ b/drivers/char/lp.c
>>>> @@ -971,7 +971,7 @@ static void lp_attach(struct parport *port)
>>>>  			if (port_num[i] == -1)
>>>>  				break;
>>>>  
>>>> -		if (!lp_register(i, port))
>>>> +		if (i < LP_NO && !lp_register(i, port))
>>>>  			lp_count++;
>>>>  		break;
>>>>  
>>>
>>> Hi Greg and Arnd Bergmann,
>>>
>>> Please review the above changes. Ignore second patch.
>>>
>>> --
>>> Shedi
>>
>> Sorry, not the second patch. Ignore the 3rd patch which fixes checkspec.pl warnings.
> 
> I have no way to do that, please fix up and resend the correct series,
> our tools can not pick and choose from a series easily.
> 
> Also remember this is the middle of the merge window and we can not do
> anything at the moment.
> 
> greg k-h

No problem, I will send a new patch series with only intended fixes.
It's okay, take the patch when the time is right. Thanks for your prompt response and your time.
 
--
Shedi