[PATCH v2 0/5] Switchtec Fixes and Improvements

[PATCH v2 0/5] Switchtec Fixes and Improvements

[kelvin.cao]

From: Kelvin Cao <kelvin.cao@microchip.com>

Hi,

This patchset is mainly for improving the commit message of [PATCH 1/5]
in v1[1] to elaborate the root cause, together with a function renaming
and some other tweaks.

This patchset is based on v5.15-rc5.

Thanks,
Kelvin

[1] https://lore.kernel.org/lkml/20210924110842.6323-1-kelvin.cao@microchip.com/

Changes since v1:
- Rebase on 5.15-rc5
- Tweak some comment spacing (as suggested by Bjorn)
- Update commit message to elaborate the root cause of MRPC execution
  hanging (as suggested by Bjorn)
- Rename function check_access() to is_firmware_running()
  (as suggested by Logan) so the function name suggests the meaning of
  the return values (as suggested by Bjorn)
- Add comment to function is_firmware_running() (as suggested by Logan)


Kelvin Cao (4):
  PCI/switchtec: Error out MRPC execution when MMIO reads fail
  PCI/switchtec: Fix a MRPC error status handling issue
  PCI/switchtec: Update the way of getting management VEP instance ID
  PCI/switchtec: Replace ENOTSUPP with EOPNOTSUPP

Logan Gunthorpe (1):
  PCI/switchtec: Add check of event support

 drivers/pci/switch/switchtec.c | 95 ++++++++++++++++++++++++++++------
 include/linux/switchtec.h      |  1 +
 2 files changed, 79 insertions(+), 17 deletions(-)

-- 
2.25.1

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

[kelvin.cao]

From: Kelvin Cao <kelvin.cao@microchip.com>

A firmware hard reset may be initiated by various mechanisms including a
UART interface, TWI sideband interface from BMC, MRPC command from
userspace, etc. The switchtec management driver is unaware of these
resets.

The reset clears PCI state including the BARs and Memory Space Enable
bits, so the device no longer responds to the MMIO accesses the driver
uses to operate it.

MMIO reads to the device will fail with a PCIe error. When the root
complex handles that error, it typically fabricates ~0 data to complete
the CPU read.

Check for this sort of error by reading the device ID from MMIO space.
This ID can never be ~0, so if we see that value, it probably means the
PCIe Memory Read failed and we should return an error indication to the
application using the switchtec driver.

Signed-off-by: Kelvin Cao <kelvin.cao@microchip.com>
---
 drivers/pci/switch/switchtec.c | 67 ++++++++++++++++++++++++++++++----
 1 file changed, 60 insertions(+), 7 deletions(-)

diff --git a/drivers/pci/switch/switchtec.c b/drivers/pci/switch/switchtec.c
index 0b301f8be9ed..e5bb2ac0e7bb 100644
--- a/drivers/pci/switch/switchtec.c
+++ b/drivers/pci/switch/switchtec.c
@@ -45,6 +45,7 @@ enum mrpc_state {
 	MRPC_QUEUED,
 	MRPC_RUNNING,
 	MRPC_DONE,
+	MRPC_IO_ERROR,
 };
 
 struct switchtec_user {
@@ -66,6 +67,19 @@ struct switchtec_user {
 	int event_cnt;
 };
 
+/*
+ * The MMIO reads to the device_id register should always return the device ID
+ * of the device, otherwise the firmware is probably stuck or unreachable
+ * due to a firmware reset which clears PCI state including the BARs and Memory
+ * Space Enable bits.
+ */
+static int is_firmware_running(struct switchtec_dev *stdev)
+{
+	u32 device = ioread32(&stdev->mmio_sys_info->device_id);
+
+	return stdev->pdev->device == device;
+}
+
 static struct switchtec_user *stuser_create(struct switchtec_dev *stdev)
 {
 	struct switchtec_user *stuser;
@@ -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",
 	};
 
 	stuser->state = state;
@@ -184,9 +199,26 @@ static int mrpc_queue_cmd(struct switchtec_user *stuser)
 	return 0;
 }
 
+static void mrpc_cleanup_cmd(struct switchtec_dev *stdev)
+{
+	/* requires the mrpc_mutex to already be held when called */
+
+	struct switchtec_user *stuser = list_entry(stdev->mrpc_queue.next,
+						   struct switchtec_user, list);
+
+	stuser->cmd_done = true;
+	wake_up_interruptible(&stuser->cmd_comp);
+	list_del_init(&stuser->list);
+	stuser_put(stuser);
+	stdev->mrpc_busy = 0;
+
+	mrpc_cmd_submit(stdev);
+}
+
 static void mrpc_complete_cmd(struct switchtec_dev *stdev)
 {
 	/* requires the mrpc_mutex to already be held when called */
+
 	struct switchtec_user *stuser;
 
 	if (list_empty(&stdev->mrpc_queue))
@@ -223,13 +255,7 @@ static void mrpc_complete_cmd(struct switchtec_dev *stdev)
 		memcpy_fromio(stuser->data, &stdev->mmio_mrpc->output_data,
 			      stuser->read_len);
 out:
-	stuser->cmd_done = true;
-	wake_up_interruptible(&stuser->cmd_comp);
-	list_del_init(&stuser->list);
-	stuser_put(stuser);
-	stdev->mrpc_busy = 0;
-
-	mrpc_cmd_submit(stdev);
+	mrpc_cleanup_cmd(stdev);
 }
 
 static void mrpc_event_work(struct work_struct *work)
@@ -246,6 +272,23 @@ static void mrpc_event_work(struct work_struct *work)
 	mutex_unlock(&stdev->mrpc_mutex);
 }
 
+static void mrpc_error_complete_cmd(struct switchtec_dev *stdev)
+{
+	/* requires the mrpc_mutex to already be held when called */
+
+	struct switchtec_user *stuser;
+
+	if (list_empty(&stdev->mrpc_queue))
+		return;
+
+	stuser = list_entry(stdev->mrpc_queue.next,
+			    struct switchtec_user, list);
+
+	stuser_set_state(stuser, MRPC_IO_ERROR);
+
+	mrpc_cleanup_cmd(stdev);
+}
+
 static void mrpc_timeout_work(struct work_struct *work)
 {
 	struct switchtec_dev *stdev;
@@ -257,6 +300,11 @@ static void mrpc_timeout_work(struct work_struct *work)
 
 	mutex_lock(&stdev->mrpc_mutex);
 
+	if (!is_firmware_running(stdev)) {
+		mrpc_error_complete_cmd(stdev);
+		goto out;
+	}
+
 	if (stdev->dma_mrpc)
 		status = stdev->dma_mrpc->status;
 	else
@@ -544,6 +592,11 @@ static ssize_t switchtec_dev_read(struct file *filp, char __user *data,
 	if (rc)
 		return rc;
 
+	if (stuser->state == MRPC_IO_ERROR) {
+		mutex_unlock(&stdev->mrpc_mutex);
+		return -EIO;
+	}
+
 	if (stuser->state != MRPC_DONE) {
 		mutex_unlock(&stdev->mrpc_mutex);
 		return -EBADE;
-- 
2.25.1

[PATCH v2 2/5] PCI/switchtec: Fix a MRPC error status handling issue

[kelvin.cao]

From: Kelvin Cao <kelvin.cao@microchip.com>

If an error is encountered when executing a MRPC command, the firmware
will set the status register to SWITCHTEC_MRPC_STATUS_ERROR and return
the error code in the return value register.

Add handling of SWITCHTEC_MRPC_STATUS_ERROR on status register when
completing a MRPC command.

Signed-off-by: Kelvin Cao <kelvin.cao@microchip.com>
---
 drivers/pci/switch/switchtec.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/switch/switchtec.c b/drivers/pci/switch/switchtec.c
index e5bb2ac0e7bb..5c300ff3921d 100644
--- a/drivers/pci/switch/switchtec.c
+++ b/drivers/pci/switch/switchtec.c
@@ -238,7 +238,8 @@ static void mrpc_complete_cmd(struct switchtec_dev *stdev)
 	stuser_set_state(stuser, MRPC_DONE);
 	stuser->return_code = 0;
 
-	if (stuser->status != SWITCHTEC_MRPC_STATUS_DONE)
+	if (stuser->status != SWITCHTEC_MRPC_STATUS_DONE &&
+	    stuser->status != SWITCHTEC_MRPC_STATUS_ERROR)
 		goto out;
 
 	if (stdev->dma_mrpc)
@@ -622,7 +623,8 @@ static ssize_t switchtec_dev_read(struct file *filp, char __user *data,
 out:
 	mutex_unlock(&stdev->mrpc_mutex);
 
-	if (stuser->status == SWITCHTEC_MRPC_STATUS_DONE)
+	if (stuser->status == SWITCHTEC_MRPC_STATUS_DONE ||
+	    stuser->status == SWITCHTEC_MRPC_STATUS_ERROR)
 		return size;
 	else if (stuser->status == SWITCHTEC_MRPC_STATUS_INTERRUPTED)
 		return -ENXIO;
-- 
2.25.1

[PATCH v2 3/5] PCI/switchtec: Update the way of getting management VEP instance ID

[kelvin.cao]

From: Kelvin Cao <kelvin.cao@microchip.com>

Gen4 firmware adds DMA VEP and NVMe VEP support in VEP (virtual EP)
instance ID register in addtion to management EP, update the way of
getting management VEP instance ID.

Signed-off-by: Kelvin Cao <kelvin.cao@microchip.com>
---
 drivers/pci/switch/switchtec.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/switch/switchtec.c b/drivers/pci/switch/switchtec.c
index 5c300ff3921d..97a93c9b4629 100644
--- a/drivers/pci/switch/switchtec.c
+++ b/drivers/pci/switch/switchtec.c
@@ -1133,7 +1133,7 @@ static int ioctl_pff_to_port(struct switchtec_dev *stdev,
 			break;
 		}
 
-		reg = ioread32(&pcfg->vep_pff_inst_id);
+		reg = ioread32(&pcfg->vep_pff_inst_id) & 0xFF;
 		if (reg == p.pff) {
 			p.port = SWITCHTEC_IOCTL_PFF_VEP;
 			break;
@@ -1179,7 +1179,7 @@ static int ioctl_port_to_pff(struct switchtec_dev *stdev,
 		p.pff = ioread32(&pcfg->usp_pff_inst_id);
 		break;
 	case SWITCHTEC_IOCTL_PFF_VEP:
-		p.pff = ioread32(&pcfg->vep_pff_inst_id);
+		p.pff = ioread32(&pcfg->vep_pff_inst_id) & 0xFF;
 		break;
 	default:
 		if (p.port > ARRAY_SIZE(pcfg->dsp_pff_inst_id))
@@ -1553,7 +1553,7 @@ static void init_pff(struct switchtec_dev *stdev)
 	if (reg < stdev->pff_csr_count)
 		stdev->pff_local[reg] = 1;
 
-	reg = ioread32(&pcfg->vep_pff_inst_id);
+	reg = ioread32(&pcfg->vep_pff_inst_id) & 0xFF;
 	if (reg < stdev->pff_csr_count)
 		stdev->pff_local[reg] = 1;
 
-- 
2.25.1

[PATCH v2 4/5] PCI/switchtec: Replace ENOTSUPP with EOPNOTSUPP

[kelvin.cao]

From: Kelvin Cao <kelvin.cao@microchip.com>

ENOTSUPP is not a SUSV4 error code, and the following checkpatch.pl
warning will be given for new patches which still use ENOTSUPP.

    WARNING: ENOTSUPP is not a SUSV4 error code, prefer EOPNOTSUPP

See the link below for the discussion.

  https://lore.kernel.org/netdev/20200511165319.2251678-1-kuba@kernel.org/

Replace ENOTSUPP with EOPNOTSUPP to align with future patches which will
be using EOPNOTSUPP.

Signed-off-by: Kelvin Cao <kelvin.cao@microchip.com>
---
 drivers/pci/switch/switchtec.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/pci/switch/switchtec.c b/drivers/pci/switch/switchtec.c
index 97a93c9b4629..236cb40cc7c5 100644
--- a/drivers/pci/switch/switchtec.c
+++ b/drivers/pci/switch/switchtec.c
@@ -376,7 +376,7 @@ static ssize_t field ## _show(struct device *dev, \
 		return io_string_show(buf, &si->gen4.field, \
 				      sizeof(si->gen4.field)); \
 	else \
-		return -ENOTSUPP; \
+		return -EOPNOTSUPP; \
 } \
 \
 static DEVICE_ATTR_RO(field)
@@ -668,7 +668,7 @@ static int ioctl_flash_info(struct switchtec_dev *stdev,
 		info.flash_length = ioread32(&fi->gen4.flash_length);
 		info.num_partitions = SWITCHTEC_NUM_PARTITIONS_GEN4;
 	} else {
-		return -ENOTSUPP;
+		return -EOPNOTSUPP;
 	}
 
 	if (copy_to_user(uinfo, &info, sizeof(info)))
@@ -876,7 +876,7 @@ static int ioctl_flash_part_info(struct switchtec_dev *stdev,
 		if (ret)
 			return ret;
 	} else {
-		return -ENOTSUPP;
+		return -EOPNOTSUPP;
 	}
 
 	if (copy_to_user(uinfo, &info, sizeof(info)))
@@ -1611,7 +1611,7 @@ static int switchtec_init_pci(struct switchtec_dev *stdev,
 	else if (stdev->gen == SWITCHTEC_GEN4)
 		part_id = &stdev->mmio_sys_info->gen4.partition_id;
 	else
-		return -ENOTSUPP;
+		return -EOPNOTSUPP;
 
 	stdev->partition = ioread8(part_id);
 	stdev->partition_count = ioread8(&stdev->mmio_ntb->partition_count);
-- 
2.25.1

[PATCH v2 5/5] PCI/switchtec: Add check of event support

[kelvin.cao]

From: Logan Gunthorpe <logang@deltatee.com>

Not all events are supported by every gen/variant of the Switchtec
firmware. To solve this, since Gen4, a new bit in each event header
is introduced to indicate if an event is supported by the firmware.

Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
Signed-off-by: Kelvin Cao <kelvin.cao@microchip.com>
---
 drivers/pci/switch/switchtec.c | 8 +++++++-
 include/linux/switchtec.h      | 1 +
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/switch/switchtec.c b/drivers/pci/switch/switchtec.c
index 236cb40cc7c5..38c2b036fb8e 100644
--- a/drivers/pci/switch/switchtec.c
+++ b/drivers/pci/switch/switchtec.c
@@ -1024,6 +1024,9 @@ static int event_ctl(struct switchtec_dev *stdev,
 		return PTR_ERR(reg);
 
 	hdr = ioread32(reg);
+	if (hdr & SWITCHTEC_EVENT_NOT_SUPP)
+		return -EOPNOTSUPP;
+
 	for (i = 0; i < ARRAY_SIZE(ctl->data); i++)
 		ctl->data[i] = ioread32(&reg[i + 1]);
 
@@ -1096,7 +1099,7 @@ static int ioctl_event_ctl(struct switchtec_dev *stdev,
 		for (ctl.index = 0; ctl.index < nr_idxs; ctl.index++) {
 			ctl.flags = event_flags;
 			ret = event_ctl(stdev, &ctl);
-			if (ret < 0)
+			if (ret < 0 && ret != -EOPNOTSUPP)
 				return ret;
 		}
 	} else {
@@ -1403,6 +1406,9 @@ static int mask_event(struct switchtec_dev *stdev, int eid, int idx)
 	hdr_reg = event_regs[eid].map_reg(stdev, off, idx);
 	hdr = ioread32(hdr_reg);
 
+	if (hdr & SWITCHTEC_EVENT_NOT_SUPP)
+		return 0;
+
 	if (!(hdr & SWITCHTEC_EVENT_OCCURRED && hdr & SWITCHTEC_EVENT_EN_IRQ))
 		return 0;
 
diff --git a/include/linux/switchtec.h b/include/linux/switchtec.h
index 082f1d51957a..be24056ac00f 100644
--- a/include/linux/switchtec.h
+++ b/include/linux/switchtec.h
@@ -19,6 +19,7 @@
 #define SWITCHTEC_EVENT_EN_CLI   BIT(2)
 #define SWITCHTEC_EVENT_EN_IRQ   BIT(3)
 #define SWITCHTEC_EVENT_FATAL    BIT(4)
+#define SWITCHTEC_EVENT_NOT_SUPP BIT(31)
 
 #define SWITCHTEC_DMA_MRPC_EN	BIT(0)
 
-- 
2.25.1

Re: [PATCH v2 0/5] Switchtec Fixes and Improvements

[Bjorn Helgaas]

On Thu, Oct 14, 2021 at 02:18:54PM +0000, kelvin.cao@microchip.com wrote:
> From: Kelvin Cao <kelvin.cao@microchip.com>
> 
> Hi,
> 
> This patchset is mainly for improving the commit message of [PATCH 1/5]
> in v1[1] to elaborate the root cause, together with a function renaming
> and some other tweaks.
> 
> This patchset is based on v5.15-rc5.

Applied, replacing the previous set, thanks!

> [1] https://lore.kernel.org/lkml/20210924110842.6323-1-kelvin.cao@microchip.com/
> 
> Changes since v1:
> - Rebase on 5.15-rc5
> - Tweak some comment spacing (as suggested by Bjorn)
> - Update commit message to elaborate the root cause of MRPC execution
>   hanging (as suggested by Bjorn)
> - Rename function check_access() to is_firmware_running()
>   (as suggested by Logan) so the function name suggests the meaning of
>   the return values (as suggested by Bjorn)
> - Add comment to function is_firmware_running() (as suggested by Logan)
> 
> 
> Kelvin Cao (4):
>   PCI/switchtec: Error out MRPC execution when MMIO reads fail
>   PCI/switchtec: Fix a MRPC error status handling issue
>   PCI/switchtec: Update the way of getting management VEP instance ID
>   PCI/switchtec: Replace ENOTSUPP with EOPNOTSUPP
> 
> Logan Gunthorpe (1):
>   PCI/switchtec: Add check of event support
> 
>  drivers/pci/switch/switchtec.c | 95 ++++++++++++++++++++++++++++------
>  include/linux/switchtec.h      |  1 +
>  2 files changed, 79 insertions(+), 17 deletions(-)
> 
> -- 
> 2.25.1
> 

Re: [PATCH v2 0/5] Switchtec Fixes and Improvements

[Kelvin.Cao]

On Thu, 2021-10-14 at 09:47 -0500, Bjorn Helgaas wrote:
> On Thu, Oct 14, 2021 at 02:18:54PM +0000, kelvin.cao@microchip.com
> wrote:
> > From: Kelvin Cao <kelvin.cao@microchip.com>
> > 
> > Hi,
> > 
> > This patchset is mainly for improving the commit message of [PATCH
> > 1/5]
> > in v1[1] to elaborate the root cause, together with a function
> > renaming
> > and some other tweaks.
> > 
> > This patchset is based on v5.15-rc5.
> 
> Applied, replacing the previous set, thanks!

Thank you Bjorn!
> 
> > [1] 
> > https://lore.kernel.org/lkml/20210924110842.6323-1-kelvin.cao@microchip.com/
> > 
> > Changes since v1:
> > - Rebase on 5.15-rc5
> > - Tweak some comment spacing (as suggested by Bjorn)
> > - Update commit message to elaborate the root cause of MRPC
> > execution
> >   hanging (as suggested by Bjorn)
> > - Rename function check_access() to is_firmware_running()
> >   (as suggested by Logan) so the function name suggests the meaning
> > of
> >   the return values (as suggested by Bjorn)
> > - Add comment to function is_firmware_running() (as suggested by
> > Logan)
> > 
> > 
> > Kelvin Cao (4):
> >   PCI/switchtec: Error out MRPC execution when MMIO reads fail
> >   PCI/switchtec: Fix a MRPC error status handling issue
> >   PCI/switchtec: Update the way of getting management VEP instance
> > ID
> >   PCI/switchtec: Replace ENOTSUPP with EOPNOTSUPP
> > 
> > Logan Gunthorpe (1):
> >   PCI/switchtec: Add check of event support
> > 
> >  drivers/pci/switch/switchtec.c | 95 ++++++++++++++++++++++++++++
> > ------
> >  include/linux/switchtec.h      |  1 +
> >  2 files changed, 79 insertions(+), 17 deletions(-)
> > 
> > --
> > 2.25.1
> > 

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

[Krzysztof Wilczyński]

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

Re: [PATCH v2 4/5] PCI/switchtec: Replace ENOTSUPP with EOPNOTSUPP

[Krzysztof Wilczyński]

[+cc Jakub for visibility]

Hi,

> ENOTSUPP is not a SUSV4 error code, and the following checkpatch.pl
> warning will be given for new patches which still use ENOTSUPP.
> 
>     WARNING: ENOTSUPP is not a SUSV4 error code, prefer EOPNOTSUPP
> 
> See the link below for the discussion.
> 
>   https://lore.kernel.org/netdev/20200511165319.2251678-1-kuba@kernel.org/

This makes me wonder whether we should fix other occurrences of ENOTSUPP we
have in the PCI tree, as per:

  Index File                                       Line Content
      0 drivers/pci/msi.c                          1304 *  -ENOTSUPP otherwise
      1 drivers/pci/msi.c                          1316 return -ENOTSUPP;
      2 drivers/pci/pcie/dpc.c                      355 return -ENOTSUPP;
      3 drivers/pci/setup-res.c                     421 return -ENOTSUPP;
      4 drivers/pci/setup-res.c                     433 return -ENOTSUPP;
      5 drivers/pci/pci.c                          3600 * Returns -ENOTSUPP if resizable BARs are not supported at all.
      6 drivers/pci/pci.c                          3610 return -ENOTSUPP;
      7 drivers/pci/switch/switchtec.c              330 return -ENOTSUPP; \
      8 drivers/pci/switch/switchtec.c              616 return -ENOTSUPP;
      9 drivers/pci/switch/switchtec.c              824 return -ENOTSUPP;
     10 drivers/pci/switch/switchtec.c             1559 return -ENOTSUPP;
     11 drivers/pci/controller/dwc/pcie-tegra194.c 2244 return -ENOTSUPP;

What do you think Bjorn? Jakub?

	Krzysztof

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

[Kelvin.Cao]

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