Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Fri, 15 Oct 2021 03:21:47 +0200
From:   Krzysztof =?utf-8?Q?Wilczy=C5=84ski?= <kw@linux.com>
To:     kelvin.cao@microchip.com
Cc:     kurt.schwemmer@microsemi.com, logang@deltatee.com,
        bhelgaas@google.com, linux-pci@vger.kernel.org,
        linux-kernel@vger.kernel.org, kelvincao@outlook.com
Subject: Re: [PATCH v2 1/5] PCI/switchtec: Error out MRPC execution when MMIO
 reads fail
Message-ID: <YWjXq/NL6zex4oeR@rocinante>
References: <20211014141859.11444-1-kelvin.cao@microchip.com>
 <20211014141859.11444-2-kelvin.cao@microchip.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <20211014141859.11444-2-kelvin.cao@microchip.com>
Precedence: bulk

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?

	Krzysztof