[PATCH 1/4] staging: most: Fix coding style problems.

[PATCH 1/4] staging: most: Fix coding style problems.

[Quytelda Kahja]

Makes two very minor changes indicated by checkpatch:
1) Add a newline after components_show() definition.
2) Fix a line over the 80 character limit.

Signed-off-by: Quytelda Kahja <quytelda@tamalin.org>
---
 drivers/staging/most/core.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/most/core.c b/drivers/staging/most/core.c
index 3dda8d81bf0b..18157dd80324 100644
--- a/drivers/staging/most/core.c
+++ b/drivers/staging/most/core.c
@@ -583,6 +583,7 @@ static ssize_t components_show(struct device_driver *drv, char *buf)
 	}
 	return offs;
 }
+
 /**
  * split_string - parses buf and extracts ':' separated substrings.
  *
@@ -1474,7 +1475,9 @@ void most_deregister_interface(struct most_interface *iface)
 	int i;
 	struct most_channel *c;
 
-	pr_info("deregistering device %s (%s)\n", dev_name(&iface->dev), iface->description);
+	pr_info("deregistering device %s (%s)\n",
+		dev_name(&iface->dev),
+		iface->description);
 	for (i = 0; i < iface->num_channels; i++) {
 		c = iface->p->channel[i];
 		if (c->pipe0.comp)
-- 
2.16.2

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 2/4] staging: most: Replace calls to BUG_ON() with WARN_ONCE() and return.

[Quytelda Kahja]

Replace calls to BUG_ON() used to check for NULL pointers with WARN_ONCE()
followed by a return.

Signed-off-by: Quytelda Kahja <quytelda@tamalin.org>
---
 drivers/staging/most/core.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/most/core.c b/drivers/staging/most/core.c
index 18157dd80324..3f65390a6e17 100644
--- a/drivers/staging/most/core.c
+++ b/drivers/staging/most/core.c
@@ -916,7 +916,11 @@ static void arm_mbo(struct mbo *mbo)
 	unsigned long flags;
 	struct most_channel *c;
 
-	BUG_ON((!mbo) || (!mbo->context));
+	if (WARN_ONCE(!mbo || !mbo->context,
+		      "Bad mbo or missing channel reference.\n")) {
+		return;
+	}
+
 	c = mbo->context;
 
 	if (c->is_poisoned) {
@@ -1001,7 +1005,7 @@ static int arm_mbo_chain(struct most_channel *c, int dir,
 void most_submit_mbo(struct mbo *mbo)
 {
 	if (WARN_ONCE(!mbo || !mbo->context,
-		      "bad mbo or missing channel reference\n"))
+		      "Bad mbo or missing channel reference.\n"))
 		return;
 
 	nq_hdm_mbo(mbo);
@@ -1019,7 +1023,10 @@ static void most_write_completion(struct mbo *mbo)
 {
 	struct most_channel *c;
 
-	BUG_ON((!mbo) || (!mbo->context));
+	if (WARN_ONCE(!mbo || !mbo->context,
+		      "Bad mbo or missing channel reference.\n")) {
+		return;
+	}
 
 	c = mbo->context;
 	if (mbo->status == MBO_E_INVAL)
-- 
2.16.2

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 2/4] staging: most: Replace calls to BUG_ON() with WARN_ONCE() and return.

[Greg KH]

On Fri, Feb 23, 2018 at 11:58:33PM -0800, Quytelda Kahja wrote:
> Replace calls to BUG_ON() used to check for NULL pointers with WARN_ONCE()
> followed by a return.

Are you sure this will work?

> 
> Signed-off-by: Quytelda Kahja <quytelda@tamalin.org>
> ---
>  drivers/staging/most/core.c | 13 ++++++++++---
>  1 file changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/staging/most/core.c b/drivers/staging/most/core.c
> index 18157dd80324..3f65390a6e17 100644
> --- a/drivers/staging/most/core.c
> +++ b/drivers/staging/most/core.c
> @@ -916,7 +916,11 @@ static void arm_mbo(struct mbo *mbo)
>  	unsigned long flags;
>  	struct most_channel *c;
>  
> -	BUG_ON((!mbo) || (!mbo->context));
> +	if (WARN_ONCE(!mbo || !mbo->context,
> +		      "Bad mbo or missing channel reference.\n")) {
> +		return;

How is the code supposed to recover from this major problem?

> +	}
> +
>  	c = mbo->context;
>  
>  	if (c->is_poisoned) {
> @@ -1001,7 +1005,7 @@ static int arm_mbo_chain(struct most_channel *c, int dir,
>  void most_submit_mbo(struct mbo *mbo)
>  {
>  	if (WARN_ONCE(!mbo || !mbo->context,
> -		      "bad mbo or missing channel reference\n"))
> +		      "Bad mbo or missing channel reference.\n"))

You did something different here that you did not describe in your
changelog :(

thanks,

greg k-h
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 2/4] staging: most: Replace calls to BUG_ON() with WARN_ONCE() and return.

[Quytelda Kahja]

> Are you sure this will work?
Well, my goal was just to replace the code that could crash the kernel
and let somebody with a better understanding of this particular driver
write the recovery code, if necessary.  It seemed from context that
the BUG_ON() calls were being used like assert() though, so I assumed
there wasn't really much recovery to be made from that problem.  If
you feel this doesn't improve the behavior of the driver, just drop
the patch.

Thank you,
Quytelda Kahja

On Thu, Mar 1, 2018 at 8:21 AM, Greg KH <gregkh@linuxfoundation.org> wrote:
> On Fri, Feb 23, 2018 at 11:58:33PM -0800, Quytelda Kahja wrote:
>> Replace calls to BUG_ON() used to check for NULL pointers with WARN_ONCE()
>> followed by a return.
>
> Are you sure this will work?
>
>>
>> Signed-off-by: Quytelda Kahja <quytelda@tamalin.org>
>> ---
>>  drivers/staging/most/core.c | 13 ++++++++++---
>>  1 file changed, 10 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/staging/most/core.c b/drivers/staging/most/core.c
>> index 18157dd80324..3f65390a6e17 100644
>> --- a/drivers/staging/most/core.c
>> +++ b/drivers/staging/most/core.c
>> @@ -916,7 +916,11 @@ static void arm_mbo(struct mbo *mbo)
>>       unsigned long flags;
>>       struct most_channel *c;
>>
>> -     BUG_ON((!mbo) || (!mbo->context));
>> +     if (WARN_ONCE(!mbo || !mbo->context,
>> +                   "Bad mbo or missing channel reference.\n")) {
>> +             return;
>
> How is the code supposed to recover from this major problem?
>
>> +     }
>> +
>>       c = mbo->context;
>>
>>       if (c->is_poisoned) {
>> @@ -1001,7 +1005,7 @@ static int arm_mbo_chain(struct most_channel *c, int dir,
>>  void most_submit_mbo(struct mbo *mbo)
>>  {
>>       if (WARN_ONCE(!mbo || !mbo->context,
>> -                   "bad mbo or missing channel reference\n"))
>> +                   "Bad mbo or missing channel reference.\n"))
>
> You did something different here that you did not describe in your
> changelog :(
>
> thanks,
>
> greg k-h
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 2/4] staging: most: Replace calls to BUG_ON() with WARN_ONCE() and return.

[Dan Carpenter]

On Tue, Mar 06, 2018 at 01:23:18AM -0800, Quytelda Kahja wrote:
> > Are you sure this will work?
> Well, my goal was just to replace the code that could crash the kernel
> and let somebody with a better understanding of this particular driver
> write the recovery code, if necessary.  It seemed from context that
> the BUG_ON() calls were being used like assert() though, so I assumed
> there wasn't really much recovery to be made from that problem.  If
> you feel this doesn't improve the behavior of the driver, just drop
> the patch.
> 

mbo is always non-NULL just from a quick glance.  I didn't look hard
enough to verify that mbo->context was OK.

It's generally best to just check the callers and delete these.

Say you have a BUG_ON() then that prevents memory corruption (not an
issue here) but it makes debugging hard.  But if you just have a NULL
dereference it probably kills the driver but you get an Oops which you
can debug.  So a NULL dereference is normally better than a BUG_ON().

regards,
dan carpenter

[PATCH] staging: most: Remove unnecessary usage of BUG_ON().

[Quytelda Kahja]

There is no need for the calls to BUG_ON() in this driver, which are
used to check if mbo or mbo->context are NULL; mbo is never NULL, and
if mbo->context is NULL it would have already been dereferenced and
oopsed before reaching the BUG_ON().

Signed-off-by: Quytelda Kahja <quytelda@tamalin.org>
Acked-by: Christian Gromm <christian.gromm@microchip.com>
---
 drivers/staging/most/core.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/staging/most/core.c b/drivers/staging/most/core.c
index 0ab2de5ecf18..3afc25a61643 100644
--- a/drivers/staging/most/core.c
+++ b/drivers/staging/most/core.c
@@ -915,7 +915,6 @@ static void arm_mbo(struct mbo *mbo)
 	unsigned long flags;
 	struct most_channel *c;
 
-	BUG_ON((!mbo) || (!mbo->context));
 	c = mbo->context;
 
 	if (c->is_poisoned) {
@@ -1018,8 +1017,6 @@ static void most_write_completion(struct mbo *mbo)
 {
 	struct most_channel *c;
 
-	BUG_ON((!mbo) || (!mbo->context));
-
 	c = mbo->context;
 	if (mbo->status == MBO_E_INVAL)
 		pr_info("WARN: Tx MBO status: invalid\n");
-- 
2.16.2

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: most: Remove unnecessary usage of BUG_ON().

[Christian Gromm]

On 07.03.2018 02:31, Quytelda Kahja wrote:
> There is no need for the calls to BUG_ON() in this driver, which are
> used to check if mbo or mbo->context are NULL; mbo is never NULL, and
> if mbo->context is NULL it would have already been dereferenced and
> oopsed before reaching the BUG_ON().
> 
> Signed-off-by: Quytelda Kahja <quytelda@tamalin.org>
Acked-by: Christian Gromm <christian.gromm@microchip.com>
> ---
>   drivers/staging/most/core.c | 3 ---
>   1 file changed, 3 deletions(-)
> 
> diff --git a/drivers/staging/most/core.c b/drivers/staging/most/core.c
> index 0ab2de5ecf18..3afc25a61643 100644
> --- a/drivers/staging/most/core.c
> +++ b/drivers/staging/most/core.c
> @@ -915,7 +915,6 @@ static void arm_mbo(struct mbo *mbo)
>   	unsigned long flags;
>   	struct most_channel *c;
>   
> -	BUG_ON((!mbo) || (!mbo->context));
>   	c = mbo->context;
>   
>   	if (c->is_poisoned) {
> @@ -1018,8 +1017,6 @@ static void most_write_completion(struct mbo *mbo)
>   {
>   	struct most_channel *c;
>   
> -	BUG_ON((!mbo) || (!mbo->context));
> -
>   	c = mbo->context;
>   	if (mbo->status == MBO_E_INVAL)
>   		pr_info("WARN: Tx MBO status: invalid\n");
> 

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 3/4] staging: most: Remove unnecessary OOM messages.

[Quytelda Kahja]

It isn't necessary for the driver to log out-of-memory errors, so
these have been removed and the functions simply return -ENOMEM.

Signed-off-by: Quytelda Kahja <quytelda@tamalin.org>
---
 drivers/staging/most/core.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/staging/most/core.c b/drivers/staging/most/core.c
index 3f65390a6e17..b8792d70db8b 100644
--- a/drivers/staging/most/core.c
+++ b/drivers/staging/most/core.c
@@ -1210,7 +1210,6 @@ int most_start_channel(struct most_interface *iface, int id,
 		num_buffer = arm_mbo_chain(c, c->cfg.direction,
 					   most_write_completion);
 	if (unlikely(!num_buffer)) {
-		pr_info("failed to allocate memory\n");
 		ret = -ENOMEM;
 		goto error;
 	}
@@ -1389,7 +1388,6 @@ int most_register_interface(struct most_interface *iface)
 
 	iface->p = kzalloc(sizeof(*iface->p), GFP_KERNEL);
 	if (!iface->p) {
-		pr_info("Failed to allocate interface instance\n");
 		ida_simple_remove(&mdev_id, id);
 		return -ENOMEM;
 	}
-- 
2.16.2

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 4/4] staging: most: Fix missing identifier in function definition argument.

[Quytelda Kahja]

The function pointer 'complete' in 'struct mbo' should use an identifier
for its argument.

Signed-off-by: Quytelda Kahja <quytelda@tamalin.org>
---
 drivers/staging/most/core.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/most/core.h b/drivers/staging/most/core.h
index 74a29163b68a..884bd71fafce 100644
--- a/drivers/staging/most/core.h
+++ b/drivers/staging/most/core.h
@@ -184,7 +184,7 @@ struct mbo {
 	u16 buffer_length;
 	u16 processed_length;
 	enum mbo_status_flags status;
-	void (*complete)(struct mbo *);
+	void (*complete)(struct mbo *mbo);
 };
 
 /**
-- 
2.16.2

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 1/4] staging: most: Fix coding style problems.

[Greg KH]

On Fri, Feb 23, 2018 at 11:58:32PM -0800, Quytelda Kahja wrote:
> Makes two very minor changes indicated by checkpatch:
> 1) Add a newline after components_show() definition.
> 2) Fix a line over the 80 character limit.

Do not do multiple things in the same patch, whenever possible.  Please
break this up into 2 patches.

thanks,

greg k-h
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 1/2] staging: most: Fix a coding style problem.

[Quytelda Kahja]

Use a blank line after components_show() function declaration.

Signed-off-by: Quytelda Kahja <quytelda@tamalin.org>
---
 drivers/staging/most/core.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/staging/most/core.c b/drivers/staging/most/core.c
index 0ab2de5ecf18..67e2d7f29967 100644
--- a/drivers/staging/most/core.c
+++ b/drivers/staging/most/core.c
@@ -583,6 +583,7 @@ static ssize_t components_show(struct device_driver *drv, char *buf)
 	}
 	return offs;
 }
+
 /**
  * split_string - parses buf and extracts ':' separated substrings.
  *
-- 
2.16.2

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 2/2] staging: most: Fix a coding style problem

[Quytelda Kahja]

Indent the parameters for a function call that extends past 80 characters.

Signed-off-by: Quytelda Kahja <quytelda@tamalin.org>
---
 drivers/staging/most/core.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/most/core.c b/drivers/staging/most/core.c
index 67e2d7f29967..8d311970225e 100644
--- a/drivers/staging/most/core.c
+++ b/drivers/staging/most/core.c
@@ -1473,7 +1473,8 @@ void most_deregister_interface(struct most_interface *iface)
 	int i;
 	struct most_channel *c;
 
-	pr_info("deregistering device %s (%s)\n", dev_name(&iface->dev), iface->description);
+	pr_info("deregistering device %s (%s)\n", dev_name(&iface->dev),
+		iface->description);
 	for (i = 0; i < iface->num_channels; i++) {
 		c = iface->p->channel[i];
 		if (c->pipe0.comp)
-- 
2.16.2

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 2/2] staging: most: Fix a coding style problem

[Dan Carpenter]

You've got two subjects which are the same.  That's not allowed, sorry.
You could just say:

[PATCH 1/2] staging: most: Add a blank line
[PATCH 2/2] staging: most: Silence a long line warning

regards,
dan carpenter

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH v2 1/2] staging: most: Add a blank line.

[Quytelda Kahja]

Use a blank line after components_show() function declaration.

Signed-off-by: Quytelda Kahja <quytelda@tamalin.org>
---
 drivers/staging/most/core.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/staging/most/core.c b/drivers/staging/most/core.c
index 0ab2de5ecf18..67e2d7f29967 100644
--- a/drivers/staging/most/core.c
+++ b/drivers/staging/most/core.c
@@ -583,6 +583,7 @@ static ssize_t components_show(struct device_driver *drv, char *buf)
 	}
 	return offs;
 }
+
 /**
  * split_string - parses buf and extracts ':' separated substrings.
  *
-- 
2.16.2

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH v2 2/2] staging: most: Indent function parameter.

[Quytelda Kahja]

Indent the parameters for a function call that extends past 80 characters.

Signed-off-by: Quytelda Kahja <quytelda@tamalin.org>
---
 drivers/staging/most/core.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/most/core.c b/drivers/staging/most/core.c
index 67e2d7f29967..8d311970225e 100644
--- a/drivers/staging/most/core.c
+++ b/drivers/staging/most/core.c
@@ -1473,7 +1473,8 @@ void most_deregister_interface(struct most_interface *iface)
 	int i;
 	struct most_channel *c;
 
-	pr_info("deregistering device %s (%s)\n", dev_name(&iface->dev), iface->description);
+	pr_info("deregistering device %s (%s)\n", dev_name(&iface->dev),
+		iface->description);
 	for (i = 0; i < iface->num_channels; i++) {
 		c = iface->p->channel[i];
 		if (c->pipe0.comp)
-- 
2.16.2

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel