netdev - Re: [PATCH net-next v2 0/4] sfc: support unicast PTP

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 2 Feb 2023 08:08:10 +0100
From:   Íñigo Huguet <ihuguet@...hat.com>
To:     Jakub Kicinski <kuba@...nel.org>
Cc:     ecree.xilinx@...il.com, habetsm.xilinx@...il.com,
        richardcochran@...il.com, davem@...emloft.net, edumazet@...gle.com,
        pabeni@...hat.com, netdev@...r.kernel.org,
        Yalin Li <yalli@...hat.com>, kernel test robot <lkp@...el.com>
Subject: Re: [PATCH net-next v2 0/4] sfc: support unicast PTP

On Wed, Feb 1, 2023 at 8:05 PM Jakub Kicinski <kuba@...nel.org> wrote:
>
> On Wed,  1 Feb 2023 09:08:45 +0100 Íñigo Huguet wrote:
> > v2: fixed missing IS_ERR
> >     added doc of missing fields in efx_ptp_rxfilter
>
> 1. don't repost within 24h, *especially* if you're reposting
> because of compilation problems
>
> https://www.kernel.org/doc/html/next/process/maintainer-netdev.html

Sorry, I wasn't aware of this.

> 2. please don't repost in a thread, it makes it harder for me
> to maintain a review queue

What do you mean? I sent it with `git send-email --in-reply-to`, I
thought this was the canonical way to send a v2 and superseed the
previous version.

> 3. drop the pointless inline in the source file in patch 4
>
> +static inline void efx_ptp_remove_one_filter(struct efx_nic *efx,
> +                                            struct efx_ptp_rxfilter *rxfilter)

This is the second time I get pushback because of this. Could you
please explain the rationale of not allowing it?

I understand that the compiler probably will do the right thing with
or without the `inline`, and more being in the same translation unit.
Actually, I checked the style guide [1] and I thought it was fine like
this: it says that `inline` should not be abused, but it's fine in
cases like this one. Quotes from the guide:
  "Generally, inline functions are preferable to macros resembling functions"
  "A reasonable rule of thumb is to not put inline at functions that
have more than 3 lines of code in them"

I have the feeling that if I had made it as a macro it had been
accepted, but inline not, despite the "prefer inline over macro".

I don't mind changing it, but I'd like to understand it so I can
remember the next time. And if it's such a hard rule that it's
considered a "fail" in the patchwork checks, maybe it should be
documented somewhere.

Thanks!

[1] https://www.kernel.org/doc/html/latest/process/coding-style.html

-- 
Íñigo Huguet