On 20 January 2017 at 12:37, Arend Van Spriel
<arend.vanspr...@broadcom.com> wrote:
> 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 <ra...@milecki.pl>
>>>>
>>>> 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 <ra...@milecki.pl>
>>>> ---
>>>> 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.
Is this possible with a macro? Could you point me to some example for
something like this?
Also is this acceptable for upstream code?
--
Rafał
