On 20-1-2017 12:17, Rafał Miłecki wrote:
> On 01/20/2017 11:26 AM, Arend Van Spriel wrote:
>> On 18-1-2017 15:27, Rafał Miłecki wrote:
>>> From: Rafał Miłecki <rafal@xxxxxxxxxx>
>>>
>>> This macro accepts an extra argument of struct brcmf_pub pointer. It
>>> allows referencing struct device and printing error using dev_err. This
>>> is very useful on devices with more than one wireless card suppored by
>>> brcmfmac. Thanks for dev_err it's possible to identify device that error
>>> is related to.
>>>
>>> Signed-off-by: Rafał Miłecki <rafal@xxxxxxxxxx>
>>> ---
>>> I could convert few more brcmf_err calls to this new brcmf_err_pub
>>> but I don't
>>> want to spent too much time on this in case someone has a better idea.
>>>
>>> If you do, comment! :)
>>
>> I do not, but still comment ;-). Like the idea. Was on our list because
>> it is damn convenient when testing on router with multiple devices. I
>> would prefer to replace the existing brcmf_err() rather than introducing
>> a new one and would like to do the same for brcmf_dbg(). However, not
>> all call sites have a struct brcmf_pub instance available. Everywhere
>> before completing brcmf_attach() that is.
>
> I decided to try new macro because:
> 1) I was too lazy to convert all calls at once
> 2) I could stick to brcmf_err when struct brcmf_pub isn't available yet
>
> In theory we could pass NULL to dev_err, I'd result in something like:
> [ 14.746754] (NULL device *): brcmf_c_preinit_dcmds: Firmware version
> = wl0: Sep 18 2015 03:30:01 version 7.35.177.56 (r587209) FWID 01-6cb8e269
>
> Unfortunately AFAIK it's not possible to handle
> brcmf_err(NULL "Foo\n");
> calls while keeping brcmf_err a macro.
>
> I was thinking about something like:
> #define brcmf_err(pub, fmt, ...) \
> do { \
> if (IS_ENABLED(CONFIG_BRCMDBG) || net_ratelimit()) \
> dev_err(pub ? pub->bus_if->dev : NULL, \
> "%s: " fmt, __func__, \
> ##__VA_ARGS__); \
> } while (0)
>
> but this wouldn't compile because for brcmf_err(NULL "Foo\n"); it would
> roll
> out to:
> dev_err(NULL ? NULL->bus_if->dev : NULL, "Foo\n");
>
> I *can* support brcmf_err(NULL "Foo\n"); calls if I change brcmf_err from
> macro to a function (inline probably). Do you think it's a good idea?
I was wondering if brcmf_err(ptr, "bla\n") could determine the pointer
type and accept 'struct device *' and 'struct brcmf_pub *'. I think we
always have a struct device pointer. Only in brcmf_module_init() we do not.
Regards,
Arend
