[PATCH] avoid crashing the kernel

[PATCH] avoid crashing the kernel

[Michael Estner]

To avoid chrashing the kernel I use WARN_ON instead.

Signed-off-by: Michael Estner <michaelestner@web.de>
---
 drivers/staging/most/i2c/i2c.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/most/i2c/i2c.c b/drivers/staging/most/i2c/i2c.c
index 7042f10887bb..e1edd892f9fd 100644
--- a/drivers/staging/most/i2c/i2c.c
+++ b/drivers/staging/most/i2c/i2c.c
@@ -68,7 +68,7 @@ static int configure_channel(struct most_interface *most_iface,
 	struct hdm_i2c *dev = to_hdm(most_iface);
 	unsigned int delay, pr;

-	BUG_ON(ch_idx < 0 || ch_idx >= NUM_CHANNELS);
+	WARN_ON(ch_idx < 0 || ch_idx >= NUM_CHANNELS);

 	if (channel_config->data_type != MOST_CH_CONTROL) {
 		pr_err("bad data type for channel %d\n", ch_idx);
@@ -122,7 +122,7 @@ static int enqueue(struct most_interface *most_iface,
 	struct hdm_i2c *dev = to_hdm(most_iface);
 	int ret;

-	BUG_ON(ch_idx < 0 || ch_idx >= NUM_CHANNELS);
+	WARN_ON(ch_idx < 0 || ch_idx >= NUM_CHANNELS);

 	if (ch_idx == CH_RX) {
 		/* RX */
@@ -167,7 +167,7 @@ static int poison_channel(struct most_interface *most_iface,
 	struct hdm_i2c *dev = to_hdm(most_iface);
 	struct mbo *mbo;

-	BUG_ON(ch_idx < 0 || ch_idx >= NUM_CHANNELS);
+	WARN_ON(ch_idx < 0 || ch_idx >= NUM_CHANNELS);

 	if (ch_idx == CH_RX) {
 		if (!polling_rate)
--
2.25.1

Re: [PATCH] avoid crashing the kernel

[Greg Kroah-Hartman]

On Sat, Sep 25, 2021 at 10:04:30PM +0200, Michael Estner wrote:
> To avoid chrashing the kernel I use WARN_ON instead.
> 
> Signed-off-by: Michael Estner <michaelestner@web.de>
> ---
>  drivers/staging/most/i2c/i2c.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)

Hi,

First off, thanks for the changes, but they need a bit more work.

Your subject line should match the others done for this file, so you
need a "staging: most:" prefix like others.  To see this better, do:
	git log --oneline the_file_i_am_touching.c


Also take a look at the kernel documentation for how to write a good
changelog text.  Your wording above needs some work...

> diff --git a/drivers/staging/most/i2c/i2c.c b/drivers/staging/most/i2c/i2c.c
> index 7042f10887bb..e1edd892f9fd 100644
> --- a/drivers/staging/most/i2c/i2c.c
> +++ b/drivers/staging/most/i2c/i2c.c
> @@ -68,7 +68,7 @@ static int configure_channel(struct most_interface *most_iface,
>  	struct hdm_i2c *dev = to_hdm(most_iface);
>  	unsigned int delay, pr;
> 
> -	BUG_ON(ch_idx < 0 || ch_idx >= NUM_CHANNELS);
> +	WARN_ON(ch_idx < 0 || ch_idx >= NUM_CHANNELS);

You really aren't changing much here, for systems running with "panic on
warn", right?

To solve this correctly you should either:
	- determine that these are impossible to hit and remove the test
	  entirely
	- determine that these are possible to hit, and turn them into a
	  real test and handle the error properly.

thanks,

greg k-h

Re: [PATCH] avoid crashing the kernel

[Fabio M. De Francesco]

On Saturday, September 25, 2021 10:04:30 PM CEST Michael Estner wrote:
> To avoid chrashing the kernel I use WARN_ON instead.

"[] I use WARN_ON instead". Instead of what? 

Changelogs must be self-contained. I had to look at the diff to complete that 
sentence in my mind.

Please don't ask reviewers to go back and forth commit messages and code in 
order to get the whole picture.

However, my suggestion is only about the formal aspects of proper patch 
construction. The actual top priority is to properly solve this problem using 
the tips provided by Greg K-H with his review.
Thanks,

Fabio

P.S.: "chrashing" => "crashing".

> 
> Signed-off-by: Michael Estner <michaelestner@web.de>
> ---
>  drivers/staging/most/i2c/i2c.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/staging/most/i2c/i2c.c b/drivers/staging/most/i2c/
i2c.c
> index 7042f10887bb..e1edd892f9fd 100644
> --- a/drivers/staging/most/i2c/i2c.c
> +++ b/drivers/staging/most/i2c/i2c.c
> @@ -68,7 +68,7 @@ static int configure_channel(struct most_interface 
*most_iface,
>  	struct hdm_i2c *dev = to_hdm(most_iface);
>  	unsigned int delay, pr;
> 
> -	BUG_ON(ch_idx < 0 || ch_idx >= NUM_CHANNELS);
> +	WARN_ON(ch_idx < 0 || ch_idx >= NUM_CHANNELS);

Re: [PATCH] avoid crashing the kernel

[Dan Carpenter]

Missing patch prefix.

On Sat, Sep 25, 2021 at 10:04:30PM +0200, Michael Estner wrote:
> To avoid chrashing the kernel I use WARN_ON instead.
> 

This is not actually the difference at all.  You are doing something
without really understand it.  If we wanted to, we could do this
automically with a Perl script but it really is better if someone can
look at it and think about it a bit.

regards,
dan carpenter