Re: [PATCH 2/2] mwifiex: print URB submit failure error after threshold attemtps

Brian Norris [off-list ref] writes:

On Wed, Sep 20, 2017 at 07:30:29AM +0300, Kalle Valo wrote:

quoted

Brian Norris [off-list ref] writes:

quoted

Hi Ganapathi,

On Thu, Sep 14, 2017 at 02:14:24PM +0000, Ganapathi Bhat wrote:

quoted

quoted

On Thu, 2017-08-31 at 01:21 +0530, Ganapathi Bhat wrote:

quoted

quoted

quoted

quoted

Why not use a ratelimit?

Since this is for receive, the packets are from AP side and we cannot
lower the rate from AP. On some low performance systems this change
will be helpful.

I think Joe was referring to things like printk_ratelimited() or
dev_err_ratelimited(). Those automatically ratelimit prints for you,
using a static counter. You'd just need to make a small warpper for
mwifiex_dbg() using __ratelimit().

Those sort of rate limits are significantly different than yours though.
You were looking to avoid printing errors when there are only a few
failures in a row, whereas the existing rate-limiting infrastructure
looks to avoid printing errors if too many happen in a row. Those are
different goals.

Are you saying that this patch is good to take? Or should Ganapathi
submit v2?

If you're asking me...

Yeah, I was asking you because to me this patch looks like an ugly
workaround to a bug. And now that looked patch 1 more closely it feels
the same.

All I was saying was that I don't think Joe's suggestion will help
Ganapathi. I'd expect Ganapathi could confirm/deny that part. (Or Joe
could correct me if my interpretation is wrong.)

Ok.

I'm also not familiar with how we expect dev_alloc_skb() failures to be
handled. If that's a common expected failure mode in low-memory
situations (seems reasonable?) and the driver handles these failure just
fine (completely unreviewed by me, so far; I suspect it doesn't do this
completely correctly), then sure, being less noisy about it as done in
this patch should be fine.

But this is a debug message so it should not bother normal users, right?
I think that having a threshold like this is just hiding problems and
not solving them. The real issue here is that dev_alloc_skb() is failing
and that's what should be solved, not to paper it over by limiting debug
messages. It just means that the real issue will be even more difficult
to detect in the future.

IOW, I don't have concrete feedback for Ganapathi to address, but I'm
not exactly "ack"ing it myself.

I'm not very confident about this patch either, it's not just making any
sense.

-- 
Kalle Valo