Re: [PATCH 6/9] nvme-apple: Add initial Apple SoC NVMe driver

[Arnd Bergmann <arnd@arndb.de>]

On Sat, Apr 2, 2022 at 3:34 PM Sven Peter <sven@svenpeter.dev> wrote:
> On Tue, Mar 22, 2022, at 14:38, Arnd Bergmann wrote:
> > On Mon, Mar 21, 2022 at 5:50 PM Sven Peter <sven@svenpeter.dev> wrote:

> >> +bad_sgl:
> >> +       WARN(DO_ONCE(apple_nvme_print_sgl, iod->sg, iod->nents),
> >> +            "Invalid SGL for payload:%d nents:%d\n", blk_rq_payload_bytes(req),
> >> +            iod->nents);
> >
> > I think you mean WARN_ONCE() here?
>
> This is taken from pci.c which used to use WARN_ONCE but was replaced in
> d08774738446e77734777adcf5d1045237b4475a with this construction here.
> The commit message mentions
>
>     The WARN_ONCE macro returns true if the condition is true, not if the
>     warn was raised, so we're printing the scatter list every time it's
>     invalid. This is excessive and makes debugging harder, so this patch
>     prints it just once.
>

Ok, makes sense. If we get more of these in the kernel, we may want to
add a helper that makes this more obvious, but it appears that for now
these two are the only ones.

It could also be expressed by moving the WARN_ONCE() into the
condition above the 'goto', but I don't see a reason to change the other
driver and it's better to keep the two consistent.

> Agreed, I've actually tried replacing all non-relaxed ones with the normal
> accessors (even those inside the hot path) and didn't see any performance
> difference.
> I can use the normal ones here and I'll consider using the non-relaxed ones
> in the hot path together with a comment why they are safe in those places.

Sounds good. If you find any instances in the hot path that have a
corresponding version in the pci.c file, it would be good to keep these
two in sync, hopefully making the non-apple case faster as well.

        Arnd