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..
> >> +
> >> +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.
Jason
