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

[<Kelvin.Cao@microchip.com>]

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