Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
In-Reply-To: <20180301162130.GB15087@kroah.com>
References: <20180224075835.11618-1-quytelda@tamalin.org> <20180224075835.11618-2-quytelda@tamalin.org>
 <20180301162130.GB15087@kroah.com>
From:   Quytelda Kahja <quytelda@tamalin.org>
Date:   Tue, 6 Mar 2018 01:23:18 -0800
Message-ID: <CAFLvi20e+D4984vypj7nVECFxh_AKVwGWTqCqfyFYJ9AV=Rvdg@mail.gmail.com>
Subject: Re: [PATCH 2/4] staging: most: Replace calls to BUG_ON() with
 WARN_ONCE() and return.
To:     Greg KH <gregkh@linuxfoundation.org>
Cc:     christian.gromm@microchip.com, devel@driverdev.osuosl.org,
        Michael.Fabry@microchip.com, chris@engineersdelight.de,
        linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

> 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