Re: [PATCH 2/4] ar5523: Add driver header file

[Kalle Valo <kvalo@adurom.com>]

Pontus Fuchs <pontus.fuchs@gmail.com> writes:

> On 2012-10-13 07:41, Kalle Valo wrote:
>> Pontus Fuchs <pontus.fuchs@gmail.com> writes:
>>
>>> +#define ar5523_err(ar, format, arg...) \
>>> +do { \
>>> +	if (!test_bit(AR5523_USB_DISCONNECTED, &ar->flags)) { \
>>> +		dev_err(&(ar)->dev->dev, format, ## arg); \
>>> +	} \
>>> +} while (0)
>> This looks suspicious and might hide something important. It's better to
>> fix the real cause than to workaround it in the error log printer. Do
>> you know why this was added?
>
> Yes. The purpose is to hide all the USB errors that happens when you
> hot-unplug the dongle. There can be quite a few URBs in flight and
> all of them will fail with -ENODEV or -ESHUTDOWN. Instead of checking
> for these errors in all the usb submissions and callbacks I added this
> flag. Do you still a problem with this approach?

IMHO it's bad style to hide that inside a logging macro and might hide
some other bugs. It's better to check the state explicitly in the
callback.

But again this isn't anything important, just something I noticed while
reviewing the code. Do what you think is best.

-- 
Kalle Valo