linux-kernel - Re: [PATCH v2 1/5] PCI/switchtec: Error out MRPC execution when MMIO reads fail

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] [day] [month] [year] [list]

Message-ID: <5b554388b1e46a0d2f3f7082a4d4defe55707712.camel@microchip.com>
Date:   Fri, 15 Oct 2021 22:13:36 +0000
From:   <Kelvin.Cao@...rochip.com>
To:     <kw@...ux.com>
CC:     <kurt.schwemmer@...rosemi.com>, <bhelgaas@...gle.com>,
        <kelvincao@...look.com>, <logang@...tatee.com>,
        <linux-kernel@...r.kernel.org>, <linux-pci@...r.kernel.org>
Subject: Re: [PATCH v2 1/5] PCI/switchtec: Error out MRPC execution when MMIO
 reads fail

On Fri, 2021-10-15 at 03:21 +0200, Krzysztof Wilczyński wrote:
> Hi Kelvin,
> 
> Thank you for sending the series over!
> 
> I am terribly sorry for a very late comment, especially since Bjorn
> already
> accepted this series to be included, but allow me for a small
> question
> below.
> 
> [...]
> > @@ -113,6 +127,7 @@ static void stuser_set_state(struct
> > switchtec_user *stuser,
> >               [MRPC_QUEUED] = "QUEUED",
> >               [MRPC_RUNNING] = "RUNNING",
> >               [MRPC_DONE] = "DONE",
> > +             [MRPC_IO_ERROR] = "IO_ERROR",
> 
> Looking at the above, and then looking at stuser_set_state(), which
> contains the following local array definition:
> 
>         const char * const state_names[] = {
>                 [MRPC_IDLE] = "IDLE",
>                 [MRPC_QUEUED] = "QUEUED",
>                 [MRPC_RUNNING] = "RUNNING",
>                 [MRPC_DONE] = "DONE",
>         };
> 
> I was wondering if there might be a small benefit of declaring this
> array
> state_names[], or list of states if you wish, as static so that we
> avoid
> having to allocate space and fill it in with values every time this
> functions runs?
> 
> The function itself if referenced in few places as per:
> 
>   Index File                           Line Content
>       1 drivers/pci/switch/switchtec.c  159 stuser_set_state(stuser,
> MRPC_RUNNING);
>       2 drivers/pci/switch/switchtec.c  178 stuser_set_state(stuser,
> MRPC_QUEUED);
>       3 drivers/pci/switch/switchtec.c  206 stuser_set_state(stuser,
> MRPC_DONE);
>       4 drivers/pci/switch/switchtec.c  567 stuser_set_state(stuser,
> MRPC_IDLE);
> 
> Even though the string representation of the state is ever only
> printed if
> a debug logging is requested, we would allocate and popular this
> array
> every time anyway, regardless of whether we print any debug
> information or
> not.
> 
> What do you think?

Thank you Krzysztof. That will be an improvement. I can probably tweak
it in the next patchset (coming soon). 

Kelvin
> 
>         Krzysztof