On 14-Mar-19 17:31, Jason Gunthorpe wrote:
> On Thu, Mar 14, 2019 at 05:09:22PM +0200, Gal Pressman wrote:
>>>> +#define efa_dbg(_dev, format, ...) \
>>>> + dev_dbg(_dev, "(pid %d) %s: " format, current->pid, \
>>>> + __func__, ##__VA_ARGS__)
>>>> +#define efa_info(_dev, format, ...) \
>>>> + dev_info(_dev, "(pid %d) %s: " format, current->pid, \
>>>> + __func__, ##__VA_ARGS__)
>>>> +#define efa_warn(_dev, format, ...) \
>>>> + dev_warn(_dev, "(pid %d) %s: " format, current->pid, \
>>>> + __func__, ##__VA_ARGS__)
>>>> +#define efa_err(_dev, format, ...) \
>>>> + dev_err(_dev, "(pid %d) %s: " format, current->pid, \
>>>> + __func__, ##__VA_ARGS__)
>>>> +#define efa_err_rl(_dev, format, ...) \
>>>> + dev_err_ratelimited(_dev, "(pid %d) %s: " format, current->pid, \
>>>> + __func__, ##__VA_ARGS__)
>>>
>>> Every time when I see such debug prints, it makes me wonder if they
>>> actually needed. Anyway "current->pid" will print wrong output for any
>>> kernel threads. I know that you are not supporting kverbs, but still
>>> don't think that it is right thing to print.
>>
>> What's the reason pid is wrong for kernel threads?
>> I found it quite useful to see the process id while debugging, at least for
>> userspace applications. Is there anything other we can use instead of
>> current->pid that would work for both?
>
> Again, I'd really like it if the three new drivers could get together
> and have core code that does this stuff sensibly and
> consistently. netdev has stuff like this already
>
> If pid logging makes sense here then it does for all..
I'm fine with that, is this an acceptable format for the subsystem? Should I
remove the pid?
>
>>>> +
>>>> +enum {
>>>> + EFA_DEVICE_RUNNING_BIT,
>>>
>>> Doesn't RDMA/core manage the state of device running/not running?
>>
>> This bit is protecting the driver from handling interrupts before the device
>> probe is finished (after request_irq is called). This could happen if the device
>> sends a keep-alive AENQ message before probe finish for example.
>
> I was looking at this as well with some suspicion..
>
> Generally I expect request_irq to happen only once the device is fully
> ready to take IRQs.
>
> What reason is there to request earlier and oddly mask the IRQ with a
> test_bit? Seems really weird.
The bit was added to be on the safe side with the async notifications, it can
probably be removed though.
Will check.
