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 <[email protected]>
---
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
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 <[email protected]>
---
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
Replace calls to BUG_ON() used to check for NULL pointers with WARN_ONCE()
followed by a return.
Signed-off-by: Quytelda Kahja <[email protected]>
---
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
The function pointer 'complete' in 'struct mbo' should use an identifier
for its argument.
Signed-off-by: Quytelda Kahja <[email protected]>
---
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
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
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 <[email protected]>
> ---
> 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
Use a blank line after components_show() function declaration.
Signed-off-by: Quytelda Kahja <[email protected]>
---
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
Indent the parameters for a function call that extends past 80 characters.
Signed-off-by: Quytelda Kahja <[email protected]>
---
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
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
Use a blank line after components_show() function declaration.
Signed-off-by: Quytelda Kahja <[email protected]>
---
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
> 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 <[email protected]> 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 <[email protected]>
>> ---
>> 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
Indent the parameters for a function call that extends past 80 characters.
Signed-off-by: Quytelda Kahja <[email protected]>
---
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
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
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 <[email protected]>
---
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
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 <[email protected]>
Acked-by: Christian Gromm <[email protected]>
> ---
> 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");
>