[PATCH v2 0/2] Fix two bugs of switchtec module

[PATCH v2 0/2] Fix two bugs of switchtec module

[Wesley Sheng]

Hi, Everyone,

This patch series fix two bugs of switchtec module.

The first is introduced by device spec definition issue: the maximum
supported PCIe function number by hardware should be 255 instead of 
the false number of 48. Rectify it in driver and for backward 
compatible, a new ioctl and corresponding data structure are created,
while keep the deprecated one.

The second is MRPC event unintentionally masked at corner case.
Fix this bug by skipping the mask operation for MRPC event in event ISR
like what we already do for LINK event.

Regard,
Wesley

--

Changed since v1:
  - rewrapped the commit message of [PATCH 1/2] into one paragraph

--

Wesley Sheng (2):
  switchtec: Fix false maximum supported PCIe function number issue
  switchtec: Fix unintended mask of MRPC event

 drivers/pci/switch/switchtec.c       | 42 +++++++++++++++++++++++++-----------
 include/linux/switchtec.h            |  2 +-
 include/uapi/linux/switchtec_ioctl.h | 13 ++++++++++-
 3 files changed, 42 insertions(+), 15 deletions(-)

-- 
2.7.4

[PATCH v2 1/2] switchtec: Fix false maximum supported PCIe function number issue

[Wesley Sheng]

The hardware supports up to 255 PFFs and the driver only supports 48, so
this patch updates the driver to support them all. To be backward
compatible, a new ioctl and corresponding data structure are created,
while keep the deprecated one.

Signed-off-by: Wesley Sheng <wesley.sheng@microchip.com>
---
 drivers/pci/switch/switchtec.c       | 39 +++++++++++++++++++++++++-----------
 include/linux/switchtec.h            |  2 +-
 include/uapi/linux/switchtec_ioctl.h | 13 +++++++++++-
 3 files changed, 40 insertions(+), 14 deletions(-)

diff --git a/drivers/pci/switch/switchtec.c b/drivers/pci/switch/switchtec.c
index e22766c..7df9a69 100644
--- a/drivers/pci/switch/switchtec.c
+++ b/drivers/pci/switch/switchtec.c
@@ -658,19 +658,25 @@ static int ioctl_flash_part_info(struct switchtec_dev *stdev,
 
 static int ioctl_event_summary(struct switchtec_dev *stdev,
 	struct switchtec_user *stuser,
-	struct switchtec_ioctl_event_summary __user *usum)
+	struct switchtec_ioctl_event_summary __user *usum,
+	size_t size)
 {
-	struct switchtec_ioctl_event_summary s = {0};
+	struct switchtec_ioctl_event_summary *s;
 	int i;
 	u32 reg;
+	int ret = 0;
 
-	s.global = ioread32(&stdev->mmio_sw_event->global_summary);
-	s.part_bitmap = ioread32(&stdev->mmio_sw_event->part_event_bitmap);
-	s.local_part = ioread32(&stdev->mmio_part_cfg->part_event_summary);
+	s = kzalloc(sizeof(*s), GFP_KERNEL);
+	if (!s)
+		return -ENOMEM;
+
+	s->global = ioread32(&stdev->mmio_sw_event->global_summary);
+	s->part_bitmap = ioread32(&stdev->mmio_sw_event->part_event_bitmap);
+	s->local_part = ioread32(&stdev->mmio_part_cfg->part_event_summary);
 
 	for (i = 0; i < stdev->partition_count; i++) {
 		reg = ioread32(&stdev->mmio_part_cfg_all[i].part_event_summary);
-		s.part[i] = reg;
+		s->part[i] = reg;
 	}
 
 	for (i = 0; i < SWITCHTEC_MAX_PFF_CSR; i++) {
@@ -679,15 +685,19 @@ static int ioctl_event_summary(struct switchtec_dev *stdev,
 			break;
 
 		reg = ioread32(&stdev->mmio_pff_csr[i].pff_event_summary);
-		s.pff[i] = reg;
+		s->pff[i] = reg;
 	}
 
-	if (copy_to_user(usum, &s, sizeof(s)))
-		return -EFAULT;
+	if (copy_to_user(usum, s, size)) {
+		ret = -EFAULT;
+		goto error_case;
+	}
 
 	stuser->event_cnt = atomic_read(&stdev->event_cnt);
 
-	return 0;
+error_case:
+	kfree(s);
+	return ret;
 }
 
 static u32 __iomem *global_ev_reg(struct switchtec_dev *stdev,
@@ -977,8 +987,9 @@ static long switchtec_dev_ioctl(struct file *filp, unsigned int cmd,
 	case SWITCHTEC_IOCTL_FLASH_PART_INFO:
 		rc = ioctl_flash_part_info(stdev, argp);
 		break;
-	case SWITCHTEC_IOCTL_EVENT_SUMMARY:
-		rc = ioctl_event_summary(stdev, stuser, argp);
+	case SWITCHTEC_IOCTL_EVENT_SUMMARY_LEGACY:
+		rc = ioctl_event_summary(stdev, stuser, argp,
+					 sizeof(struct switchtec_ioctl_event_summary_legacy));
 		break;
 	case SWITCHTEC_IOCTL_EVENT_CTL:
 		rc = ioctl_event_ctl(stdev, argp);
@@ -989,6 +1000,10 @@ static long switchtec_dev_ioctl(struct file *filp, unsigned int cmd,
 	case SWITCHTEC_IOCTL_PORT_TO_PFF:
 		rc = ioctl_port_to_pff(stdev, argp);
 		break;
+	case SWITCHTEC_IOCTL_EVENT_SUMMARY:
+		rc = ioctl_event_summary(stdev, stuser, argp,
+					 sizeof(struct switchtec_ioctl_event_summary));
+		break;
 	default:
 		rc = -ENOTTY;
 		break;
diff --git a/include/linux/switchtec.h b/include/linux/switchtec.h
index 52a079b..0cfc34a 100644
--- a/include/linux/switchtec.h
+++ b/include/linux/switchtec.h
@@ -20,7 +20,7 @@
 #include <linux/cdev.h>
 
 #define SWITCHTEC_MRPC_PAYLOAD_SIZE 1024
-#define SWITCHTEC_MAX_PFF_CSR 48
+#define SWITCHTEC_MAX_PFF_CSR 255
 
 #define SWITCHTEC_EVENT_OCCURRED BIT(0)
 #define SWITCHTEC_EVENT_CLEAR    BIT(0)
diff --git a/include/uapi/linux/switchtec_ioctl.h b/include/uapi/linux/switchtec_ioctl.h
index 4f4daf8..c912b5a 100644
--- a/include/uapi/linux/switchtec_ioctl.h
+++ b/include/uapi/linux/switchtec_ioctl.h
@@ -50,7 +50,7 @@ struct switchtec_ioctl_flash_part_info {
 	__u32 active;
 };
 
-struct switchtec_ioctl_event_summary {
+struct switchtec_ioctl_event_summary_legacy {
 	__u64 global;
 	__u64 part_bitmap;
 	__u32 local_part;
@@ -59,6 +59,15 @@ struct switchtec_ioctl_event_summary {
 	__u32 pff[48];
 };
 
+struct switchtec_ioctl_event_summary {
+	__u64 global;
+	__u64 part_bitmap;
+	__u32 local_part;
+	__u32 padding;
+	__u32 part[48];
+	__u32 pff[255];
+};
+
 #define SWITCHTEC_IOCTL_EVENT_STACK_ERROR		0
 #define SWITCHTEC_IOCTL_EVENT_PPU_ERROR			1
 #define SWITCHTEC_IOCTL_EVENT_ISP_ERROR			2
@@ -127,6 +136,8 @@ struct switchtec_ioctl_pff_port {
 	_IOWR('W', 0x41, struct switchtec_ioctl_flash_part_info)
 #define SWITCHTEC_IOCTL_EVENT_SUMMARY \
 	_IOR('W', 0x42, struct switchtec_ioctl_event_summary)
+#define SWITCHTEC_IOCTL_EVENT_SUMMARY_LEGACY \
+	_IOR('W', 0x42, struct switchtec_ioctl_event_summary_legacy)
 #define SWITCHTEC_IOCTL_EVENT_CTL \
 	_IOWR('W', 0x43, struct switchtec_ioctl_event_ctl)
 #define SWITCHTEC_IOCTL_PFF_TO_PORT \
-- 
2.7.4

[PATCH v2 2/2] switchtec: Fix unintended mask of MRPC event

[Wesley Sheng]

When running application tool switchtec-user's `firmware update` and
`event wait` commands concurrently, sometimes the firmware update
speed reduced evidently.

It is because when the MRPC event happened right after MRPC event
occurrence check but before event mask loop reach to its header
register in event ISR, the MRPC event would be masked unintentionally.
Since there's no chance to enable it again except for a module reload,
all the following MRPC execution completion check will be deferred to
timeout.

Fix this bug by skipping the mask operation for MRPC event in event
ISR, same as what we already do for LINK event.

Fixes: 52eabba5bcdb ("switchtec: Add IOCTLs to the Switchtec driver")
Signed-off-by: Wesley Sheng <wesley.sheng@microchip.com>
---
 drivers/pci/switch/switchtec.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/switch/switchtec.c b/drivers/pci/switch/switchtec.c
index 7df9a69..30f6e08 100644
--- a/drivers/pci/switch/switchtec.c
+++ b/drivers/pci/switch/switchtec.c
@@ -1177,7 +1177,8 @@ static int mask_event(struct switchtec_dev *stdev, int eid, int idx)
 	if (!(hdr & SWITCHTEC_EVENT_OCCURRED && hdr & SWITCHTEC_EVENT_EN_IRQ))
 		return 0;
 
-	if (eid == SWITCHTEC_IOCTL_EVENT_LINK_STATE)
+	if (eid == SWITCHTEC_IOCTL_EVENT_LINK_STATE ||
+	    eid == SWITCHTEC_IOCTL_EVENT_MRPC_COMP)
 		return 0;
 
 	dev_dbg(&stdev->dev, "%s: %d %d %x\n", __func__, eid, idx, hdr);
-- 
2.7.4

Re: [PATCH v2 0/2] Fix two bugs of switchtec module

[Logan Gunthorpe]

On 2019-04-15 8:41 a.m., Wesley Sheng wrote:
> Hi, Everyone,
> 
> This patch series fix two bugs of switchtec module.
> 
> The first is introduced by device spec definition issue: the maximum
> supported PCIe function number by hardware should be 255 instead of 
> the false number of 48. Rectify it in driver and for backward 
> compatible, a new ioctl and corresponding data structure are created,
> while keep the deprecated one.
> 
> The second is MRPC event unintentionally masked at corner case.
> Fix this bug by skipping the mask operation for MRPC event in event ISR
> like what we already do for LINK event.
> 
> Regard,
> Wesley
> 
> --
> 
> Changed since v1:
>   - rewrapped the commit message of [PATCH 1/2] into one paragraph

Probably should have collected my Reviewed-by tag into the patches as
well. I still approve this series:

Reviewed-by: Logan Gunthorpe <logang@deltatee.com>

Thanks,

Logan

Re: [PATCH v2 1/2] switchtec: Fix false maximum supported PCIe function number issue

[Bjorn Helgaas]

On Mon, Apr 15, 2019 at 10:41:41PM +0800, Wesley Sheng wrote:
> The hardware supports up to 255 PFFs and the driver only supports 48, so
> this patch updates the driver to support them all. To be backward
> compatible, a new ioctl and corresponding data structure are created,
> while keep the deprecated one.

The commit log needs to include some kind of detail about what "PFF"
is.  Logan provided the following, which looks like a good starting
point:

  PFF is really a concept internal to the Switchtec device. It stands
  for PCIe Function Framework. Essentially, there is a bank of
  registers for every PCIe Function (aka endpoint) in the switch. When
  I originally wrote the driver, I assumed incorrectly there would
  only ever be one PFF per port and the maximum number of ports for
  Switchtec parts is 48. In fact, the hardware supports up to 255 and
  there are typically two PFFs per upstream port (one for the port
  itself and one for the management endpoint).

I had a couple minor questions below that aren't exactly related to
this patch.  If you did decide to address them (in a separate patch,
of course), you could expand the commit log for this patch with the
PFF info.  If the questions below don't need anything, I can
incorporate something about PFF myself.

> ---
>  drivers/pci/switch/switchtec.c       | 39 +++++++++++++++++++++++++-----------
>  include/linux/switchtec.h            |  2 +-
>  include/uapi/linux/switchtec_ioctl.h | 13 +++++++++++-
>  3 files changed, 40 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/pci/switch/switchtec.c b/drivers/pci/switch/switchtec.c
> index e22766c..7df9a69 100644
> --- a/drivers/pci/switch/switchtec.c
> +++ b/drivers/pci/switch/switchtec.c
> @@ -658,19 +658,25 @@ static int ioctl_flash_part_info(struct switchtec_dev *stdev,
>  
>  static int ioctl_event_summary(struct switchtec_dev *stdev,
>  	struct switchtec_user *stuser,
> -	struct switchtec_ioctl_event_summary __user *usum)
> +	struct switchtec_ioctl_event_summary __user *usum,
> +	size_t size)
>  {
> -	struct switchtec_ioctl_event_summary s = {0};
> +	struct switchtec_ioctl_event_summary *s;
>  	int i;
>  	u32 reg;
> +	int ret = 0;
>  
> -	s.global = ioread32(&stdev->mmio_sw_event->global_summary);
> -	s.part_bitmap = ioread32(&stdev->mmio_sw_event->part_event_bitmap);
> -	s.local_part = ioread32(&stdev->mmio_part_cfg->part_event_summary);
> +	s = kzalloc(sizeof(*s), GFP_KERNEL);
> +	if (!s)
> +		return -ENOMEM;
> +
> +	s->global = ioread32(&stdev->mmio_sw_event->global_summary);
> +	s->part_bitmap = ioread32(&stdev->mmio_sw_event->part_event_bitmap);
> +	s->local_part = ioread32(&stdev->mmio_part_cfg->part_event_summary);
>  
>  	for (i = 0; i < stdev->partition_count; i++) {
>  		reg = ioread32(&stdev->mmio_part_cfg_all[i].part_event_summary);
> -		s.part[i] = reg;
> +		s->part[i] = reg;
>  	}
>  
>  	for (i = 0; i < SWITCHTEC_MAX_PFF_CSR; i++) {

Should this be "i < stdev->pff_csr_count", as in
check_link_state_events(), enable_link_state_events() and
mask_all_events()?  If so, I assume the read and check of vendor_id
would be unnecessary?

The last loop in init_pff() currently checks against
SWITCHTEC_MAX_PFF_CSR:

        for (i = 0; i < ARRAY_SIZE(pcfg->dsp_pff_inst_id); i++) {
                reg = ioread32(&pcfg->dsp_pff_inst_id[i]);
                if (reg < SWITCHTEC_MAX_PFF_CSR)
                        stdev->pff_local[reg] = 1;
        }

Should it check "reg < stdev->pff_csr_count" instead?  It looks like
mask_all_events(), the only reader of pff_local[], only looks up to
pff_csr_count anyway.

> @@ -679,15 +685,19 @@ static int ioctl_event_summary(struct switchtec_dev *stdev,
>  			break;
>  
>  		reg = ioread32(&stdev->mmio_pff_csr[i].pff_event_summary);
> -		s.pff[i] = reg;
> +		s->pff[i] = reg;
>  	}
>  
> -	if (copy_to_user(usum, &s, sizeof(s)))
> -		return -EFAULT;
> +	if (copy_to_user(usum, s, size)) {
> +		ret = -EFAULT;
> +		goto error_case;
> +	}
>  
>  	stuser->event_cnt = atomic_read(&stdev->event_cnt);
>  
> -	return 0;
> +error_case:
> +	kfree(s);
> +	return ret;
>  }
>  
>  static u32 __iomem *global_ev_reg(struct switchtec_dev *stdev,
> @@ -977,8 +987,9 @@ static long switchtec_dev_ioctl(struct file *filp, unsigned int cmd,
>  	case SWITCHTEC_IOCTL_FLASH_PART_INFO:
>  		rc = ioctl_flash_part_info(stdev, argp);
>  		break;
> -	case SWITCHTEC_IOCTL_EVENT_SUMMARY:
> -		rc = ioctl_event_summary(stdev, stuser, argp);
> +	case SWITCHTEC_IOCTL_EVENT_SUMMARY_LEGACY:
> +		rc = ioctl_event_summary(stdev, stuser, argp,
> +					 sizeof(struct switchtec_ioctl_event_summary_legacy));
>  		break;
>  	case SWITCHTEC_IOCTL_EVENT_CTL:
>  		rc = ioctl_event_ctl(stdev, argp);
> @@ -989,6 +1000,10 @@ static long switchtec_dev_ioctl(struct file *filp, unsigned int cmd,
>  	case SWITCHTEC_IOCTL_PORT_TO_PFF:
>  		rc = ioctl_port_to_pff(stdev, argp);
>  		break;
> +	case SWITCHTEC_IOCTL_EVENT_SUMMARY:
> +		rc = ioctl_event_summary(stdev, stuser, argp,
> +					 sizeof(struct switchtec_ioctl_event_summary));
> +		break;
>  	default:
>  		rc = -ENOTTY;
>  		break;
> diff --git a/include/linux/switchtec.h b/include/linux/switchtec.h
> index 52a079b..0cfc34a 100644
> --- a/include/linux/switchtec.h
> +++ b/include/linux/switchtec.h
> @@ -20,7 +20,7 @@
>  #include <linux/cdev.h>
>  
>  #define SWITCHTEC_MRPC_PAYLOAD_SIZE 1024
> -#define SWITCHTEC_MAX_PFF_CSR 48
> +#define SWITCHTEC_MAX_PFF_CSR 255
>  
>  #define SWITCHTEC_EVENT_OCCURRED BIT(0)
>  #define SWITCHTEC_EVENT_CLEAR    BIT(0)
> diff --git a/include/uapi/linux/switchtec_ioctl.h b/include/uapi/linux/switchtec_ioctl.h
> index 4f4daf8..c912b5a 100644
> --- a/include/uapi/linux/switchtec_ioctl.h
> +++ b/include/uapi/linux/switchtec_ioctl.h
> @@ -50,7 +50,7 @@ struct switchtec_ioctl_flash_part_info {
>  	__u32 active;
>  };
>  
> -struct switchtec_ioctl_event_summary {
> +struct switchtec_ioctl_event_summary_legacy {
>  	__u64 global;
>  	__u64 part_bitmap;
>  	__u32 local_part;
> @@ -59,6 +59,15 @@ struct switchtec_ioctl_event_summary {
>  	__u32 pff[48];
>  };
>  
> +struct switchtec_ioctl_event_summary {
> +	__u64 global;
> +	__u64 part_bitmap;
> +	__u32 local_part;
> +	__u32 padding;
> +	__u32 part[48];
> +	__u32 pff[255];
> +};
> +
>  #define SWITCHTEC_IOCTL_EVENT_STACK_ERROR		0
>  #define SWITCHTEC_IOCTL_EVENT_PPU_ERROR			1
>  #define SWITCHTEC_IOCTL_EVENT_ISP_ERROR			2
> @@ -127,6 +136,8 @@ struct switchtec_ioctl_pff_port {
>  	_IOWR('W', 0x41, struct switchtec_ioctl_flash_part_info)
>  #define SWITCHTEC_IOCTL_EVENT_SUMMARY \
>  	_IOR('W', 0x42, struct switchtec_ioctl_event_summary)
> +#define SWITCHTEC_IOCTL_EVENT_SUMMARY_LEGACY \
> +	_IOR('W', 0x42, struct switchtec_ioctl_event_summary_legacy)
>  #define SWITCHTEC_IOCTL_EVENT_CTL \
>  	_IOWR('W', 0x43, struct switchtec_ioctl_event_ctl)
>  #define SWITCHTEC_IOCTL_PFF_TO_PORT \
> -- 
> 2.7.4
> 

Re: [PATCH v2 1/2] switchtec: Fix false maximum supported PCIe function number issue

[Bjorn Helgaas]

On Fri, Apr 19, 2019 at 12:56:21AM +0800, wesleywesley wrote:
> On Wed, Apr 17, 2019 at 05:48:58PM -0500, Bjorn Helgaas wrote:
> > On Mon, Apr 15, 2019 at 10:41:41PM +0800, Wesley Sheng wrote:
> > > The hardware supports up to 255 PFFs and the driver only supports 48, so
> > > this patch updates the driver to support them all. To be backward
> > > compatible, a new ioctl and corresponding data structure are created,
> > > while keep the deprecated one.
> > 
> > The commit log needs to include some kind of detail about what "PFF"
> > is.  Logan provided the following, which looks like a good starting
> > point:
> > 
> >   PFF is really a concept internal to the Switchtec device. It stands
> >   for PCIe Function Framework. Essentially, there is a bank of
> >   registers for every PCIe Function (aka endpoint) in the switch. When
> >   I originally wrote the driver, I assumed incorrectly there would
> >   only ever be one PFF per port and the maximum number of ports for
> >   Switchtec parts is 48. In fact, the hardware supports up to 255 and
> >   there are typically two PFFs per upstream port (one for the port
> >   itself and one for the management endpoint).
> > 
> > I had a couple minor questions below that aren't exactly related to
> > this patch.  If you did decide to address them (in a separate patch,
> > of course), you could expand the commit log for this patch with the
> > PFF info.  If the questions below don't need anything, I can
> > incorporate something about PFF myself.
> 
> Hi, Bjorn,
> 
> See my comments below your questions.
> They are good catches, but I think it is not a bug we should fix it urgently.
> What about take it as an improvement of code, and we will submit it in the
> next upstream session. Before that, we would like to test with this change.

Totally agreed, I'll merge the current stuff today and we can get on with
this.

> > > ---
> > >  drivers/pci/switch/switchtec.c       | 39 +++++++++++++++++++++++++-----------
> > >  include/linux/switchtec.h            |  2 +-
> > >  include/uapi/linux/switchtec_ioctl.h | 13 +++++++++++-
> > >  3 files changed, 40 insertions(+), 14 deletions(-)
> > > 
> > > diff --git a/drivers/pci/switch/switchtec.c b/drivers/pci/switch/switchtec.c
> > > index e22766c..7df9a69 100644
> > > --- a/drivers/pci/switch/switchtec.c
> > > +++ b/drivers/pci/switch/switchtec.c
> > > @@ -658,19 +658,25 @@ static int ioctl_flash_part_info(struct switchtec_dev *stdev,
> > >  
> > >  static int ioctl_event_summary(struct switchtec_dev *stdev,
> > >  	struct switchtec_user *stuser,
> > > -	struct switchtec_ioctl_event_summary __user *usum)
> > > +	struct switchtec_ioctl_event_summary __user *usum,
> > > +	size_t size)
> > >  {
> > > -	struct switchtec_ioctl_event_summary s = {0};
> > > +	struct switchtec_ioctl_event_summary *s;
> > >  	int i;
> > >  	u32 reg;
> > > +	int ret = 0;
> > >  
> > > -	s.global = ioread32(&stdev->mmio_sw_event->global_summary);
> > > -	s.part_bitmap = ioread32(&stdev->mmio_sw_event->part_event_bitmap);
> > > -	s.local_part = ioread32(&stdev->mmio_part_cfg->part_event_summary);
> > > +	s = kzalloc(sizeof(*s), GFP_KERNEL);
> > > +	if (!s)
> > > +		return -ENOMEM;
> > > +
> > > +	s->global = ioread32(&stdev->mmio_sw_event->global_summary);
> > > +	s->part_bitmap = ioread32(&stdev->mmio_sw_event->part_event_bitmap);
> > > +	s->local_part = ioread32(&stdev->mmio_part_cfg->part_event_summary);
> > >  
> > >  	for (i = 0; i < stdev->partition_count; i++) {
> > >  		reg = ioread32(&stdev->mmio_part_cfg_all[i].part_event_summary);
> > > -		s.part[i] = reg;
> > > +		s->part[i] = reg;
> > >  	}
> > >  
> > >  	for (i = 0; i < SWITCHTEC_MAX_PFF_CSR; i++) {
> > 
> > Should this be "i < stdev->pff_csr_count", as in
> > check_link_state_events(), enable_link_state_events() and
> > mask_all_events()?  If so, I assume the read and check of vendor_id
> > would be unnecessary?
> 
> Yes, I think it is a good catch.
> Once the configuration file of Switchtec is choosed, the logical ports bound to 
> USPs/DSPs physical port, unbound logical ports, and vEPs (NT/Management/NT only),
> their corresponding PFF's index are fixed and the total number are invariable.
> > 
> > The last loop in init_pff() currently checks against
> > SWITCHTEC_MAX_PFF_CSR:
> > 
> >         for (i = 0; i < ARRAY_SIZE(pcfg->dsp_pff_inst_id); i++) {
> >                 reg = ioread32(&pcfg->dsp_pff_inst_id[i]);
> >                 if (reg < SWITCHTEC_MAX_PFF_CSR)
> >                         stdev->pff_local[reg] = 1;
> >         }
> > 
> > Should it check "reg < stdev->pff_csr_count" instead?  It looks like
> > mask_all_events(), the only reader of pff_local[], only looks up to
> > pff_csr_count anyway.
> > 
> Yes, I agree.
> Actually, all the SWITCHTEC_MAX_PFF_CSR in init_pff() could be replaced by it.
> > > @@ -679,15 +685,19 @@ static int ioctl_event_summary(struct switchtec_dev *stdev,
> > >  			break;
> > >  
> > >  		reg = ioread32(&stdev->mmio_pff_csr[i].pff_event_summary);
> > > -		s.pff[i] = reg;
> > > +		s->pff[i] = reg;
> > >  	}
> > >  
> > > -	if (copy_to_user(usum, &s, sizeof(s)))
> > > -		return -EFAULT;
> > > +	if (copy_to_user(usum, s, size)) {
> > > +		ret = -EFAULT;
> > > +		goto error_case;
> > > +	}
> > >  
> > >  	stuser->event_cnt = atomic_read(&stdev->event_cnt);
> > >  
> > > -	return 0;
> > > +error_case:
> > > +	kfree(s);
> > > +	return ret;
> > >  }
> > >  
> > >  static u32 __iomem *global_ev_reg(struct switchtec_dev *stdev,
> > > @@ -977,8 +987,9 @@ static long switchtec_dev_ioctl(struct file *filp, unsigned int cmd,
> > >  	case SWITCHTEC_IOCTL_FLASH_PART_INFO:
> > >  		rc = ioctl_flash_part_info(stdev, argp);
> > >  		break;
> > > -	case SWITCHTEC_IOCTL_EVENT_SUMMARY:
> > > -		rc = ioctl_event_summary(stdev, stuser, argp);
> > > +	case SWITCHTEC_IOCTL_EVENT_SUMMARY_LEGACY:
> > > +		rc = ioctl_event_summary(stdev, stuser, argp,
> > > +					 sizeof(struct switchtec_ioctl_event_summary_legacy));
> > >  		break;
> > >  	case SWITCHTEC_IOCTL_EVENT_CTL:
> > >  		rc = ioctl_event_ctl(stdev, argp);
> > > @@ -989,6 +1000,10 @@ static long switchtec_dev_ioctl(struct file *filp, unsigned int cmd,
> > >  	case SWITCHTEC_IOCTL_PORT_TO_PFF:
> > >  		rc = ioctl_port_to_pff(stdev, argp);
> > >  		break;
> > > +	case SWITCHTEC_IOCTL_EVENT_SUMMARY:
> > > +		rc = ioctl_event_summary(stdev, stuser, argp,
> > > +					 sizeof(struct switchtec_ioctl_event_summary));
> > > +		break;
> > >  	default:
> > >  		rc = -ENOTTY;
> > >  		break;
> > > diff --git a/include/linux/switchtec.h b/include/linux/switchtec.h
> > > index 52a079b..0cfc34a 100644
> > > --- a/include/linux/switchtec.h
> > > +++ b/include/linux/switchtec.h
> > > @@ -20,7 +20,7 @@
> > >  #include <linux/cdev.h>
> > >  
> > >  #define SWITCHTEC_MRPC_PAYLOAD_SIZE 1024
> > > -#define SWITCHTEC_MAX_PFF_CSR 48
> > > +#define SWITCHTEC_MAX_PFF_CSR 255
> > >  
> > >  #define SWITCHTEC_EVENT_OCCURRED BIT(0)
> > >  #define SWITCHTEC_EVENT_CLEAR    BIT(0)
> > > diff --git a/include/uapi/linux/switchtec_ioctl.h b/include/uapi/linux/switchtec_ioctl.h
> > > index 4f4daf8..c912b5a 100644
> > > --- a/include/uapi/linux/switchtec_ioctl.h
> > > +++ b/include/uapi/linux/switchtec_ioctl.h
> > > @@ -50,7 +50,7 @@ struct switchtec_ioctl_flash_part_info {
> > >  	__u32 active;
> > >  };
> > >  
> > > -struct switchtec_ioctl_event_summary {
> > > +struct switchtec_ioctl_event_summary_legacy {
> > >  	__u64 global;
> > >  	__u64 part_bitmap;
> > >  	__u32 local_part;
> > > @@ -59,6 +59,15 @@ struct switchtec_ioctl_event_summary {
> > >  	__u32 pff[48];
> > >  };
> > >  
> > > +struct switchtec_ioctl_event_summary {
> > > +	__u64 global;
> > > +	__u64 part_bitmap;
> > > +	__u32 local_part;
> > > +	__u32 padding;
> > > +	__u32 part[48];
> > > +	__u32 pff[255];
> > > +};
> > > +
> > >  #define SWITCHTEC_IOCTL_EVENT_STACK_ERROR		0
> > >  #define SWITCHTEC_IOCTL_EVENT_PPU_ERROR			1
> > >  #define SWITCHTEC_IOCTL_EVENT_ISP_ERROR			2
> > > @@ -127,6 +136,8 @@ struct switchtec_ioctl_pff_port {
> > >  	_IOWR('W', 0x41, struct switchtec_ioctl_flash_part_info)
> > >  #define SWITCHTEC_IOCTL_EVENT_SUMMARY \
> > >  	_IOR('W', 0x42, struct switchtec_ioctl_event_summary)
> > > +#define SWITCHTEC_IOCTL_EVENT_SUMMARY_LEGACY \
> > > +	_IOR('W', 0x42, struct switchtec_ioctl_event_summary_legacy)
> > >  #define SWITCHTEC_IOCTL_EVENT_CTL \
> > >  	_IOWR('W', 0x43, struct switchtec_ioctl_event_ctl)
> > >  #define SWITCHTEC_IOCTL_PFF_TO_PORT \
> > > -- 
> > > 2.7.4
> > > 
> > 
> 

Re: [PATCH v2 2/2] switchtec: Fix unintended mask of MRPC event

[Bjorn Helgaas]

On Mon, Apr 15, 2019 at 10:41:42PM +0800, Wesley Sheng wrote:
> When running application tool switchtec-user's `firmware update` and
> `event wait` commands concurrently, sometimes the firmware update
> speed reduced evidently.
> 
> It is because when the MRPC event happened right after MRPC event
> occurrence check but before event mask loop reach to its header
> register in event ISR, the MRPC event would be masked unintentionally.
> Since there's no chance to enable it again except for a module reload,
> all the following MRPC execution completion check will be deferred to
> timeout.
> 
> Fix this bug by skipping the mask operation for MRPC event in event
> ISR, same as what we already do for LINK event.
> 
> Fixes: 52eabba5bcdb ("switchtec: Add IOCTLs to the Switchtec driver")
> Signed-off-by: Wesley Sheng <wesley.sheng@microchip.com>
> ---
>  drivers/pci/switch/switchtec.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/switch/switchtec.c b/drivers/pci/switch/switchtec.c
> index 7df9a69..30f6e08 100644
> --- a/drivers/pci/switch/switchtec.c
> +++ b/drivers/pci/switch/switchtec.c
> @@ -1177,7 +1177,8 @@ static int mask_event(struct switchtec_dev *stdev, int eid, int idx)
>  	if (!(hdr & SWITCHTEC_EVENT_OCCURRED && hdr & SWITCHTEC_EVENT_EN_IRQ))
>  		return 0;
>  
> -	if (eid == SWITCHTEC_IOCTL_EVENT_LINK_STATE)
> +	if (eid == SWITCHTEC_IOCTL_EVENT_LINK_STATE ||
> +	    eid == SWITCHTEC_IOCTL_EVENT_MRPC_COMP)
>  		return 0;

I'm OK with this, but I do wonder why this check is in mask_event()
instead of in switchtec_event_isr().  AFAICT it doesn't depend on
anything in the mask_all_events() -> mask_event() path, and doing it
in switchtec_event_isr() would avoid the CSR read in mask_event().

But maybe it logically belongs here.  I'll merge it as-is for now.

>  	dev_dbg(&stdev->dev, "%s: %d %d %x\n", __func__, eid, idx, hdr);
> -- 
> 2.7.4
> 

Re: [PATCH v2 0/2] Fix two bugs of switchtec module

[Bjorn Helgaas]

On Mon, Apr 15, 2019 at 10:41:40PM +0800, Wesley Sheng wrote:
> Hi, Everyone,
> 
> This patch series fix two bugs of switchtec module.
> 
> The first is introduced by device spec definition issue: the maximum
> supported PCIe function number by hardware should be 255 instead of 
> the false number of 48. Rectify it in driver and for backward 
> compatible, a new ioctl and corresponding data structure are created,
> while keep the deprecated one.
> 
> The second is MRPC event unintentionally masked at corner case.
> Fix this bug by skipping the mask operation for MRPC event in event ISR
> like what we already do for LINK event.
> 
> Regard,
> Wesley
> 
> --
> 
> Changed since v1:
>   - rewrapped the commit message of [PATCH 1/2] into one paragraph
> 
> --
> 
> Wesley Sheng (2):
>   switchtec: Fix false maximum supported PCIe function number issue
>   switchtec: Fix unintended mask of MRPC event
> 
>  drivers/pci/switch/switchtec.c       | 42 +++++++++++++++++++++++++-----------
>  include/linux/switchtec.h            |  2 +-
>  include/uapi/linux/switchtec_ioctl.h | 13 ++++++++++-
>  3 files changed, 42 insertions(+), 15 deletions(-)

Applied to pci/switchtec for v5.2, thanks!

Re: [PATCH v2 1/2] switchtec: Fix false maximum supported PCIe function number issue

[Logan Gunthorpe]

On 2019-04-17 4:48 p.m., Bjorn Helgaas wrote:
>> ---
>>   drivers/pci/switch/switchtec.c       | 39 +++++++++++++++++++++++++-----------
>>   include/linux/switchtec.h            |  2 +-
>>   include/uapi/linux/switchtec_ioctl.h | 13 +++++++++++-
>>   3 files changed, 40 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/pci/switch/switchtec.c b/drivers/pci/switch/switchtec.c
>> index e22766c..7df9a69 100644
>> --- a/drivers/pci/switch/switchtec.c
>> +++ b/drivers/pci/switch/switchtec.c
>> @@ -658,19 +658,25 @@ static int ioctl_flash_part_info(struct switchtec_dev *stdev,
>>   
>>   static int ioctl_event_summary(struct switchtec_dev *stdev,
>>   	struct switchtec_user *stuser,
>> -	struct switchtec_ioctl_event_summary __user *usum)
>> +	struct switchtec_ioctl_event_summary __user *usum,
>> +	size_t size)
>>   {
>> -	struct switchtec_ioctl_event_summary s = {0};
>> +	struct switchtec_ioctl_event_summary *s;
>>   	int i;
>>   	u32 reg;
>> +	int ret = 0;
>>   
>> -	s.global = ioread32(&stdev->mmio_sw_event->global_summary);
>> -	s.part_bitmap = ioread32(&stdev->mmio_sw_event->part_event_bitmap);
>> -	s.local_part = ioread32(&stdev->mmio_part_cfg->part_event_summary);
>> +	s = kzalloc(sizeof(*s), GFP_KERNEL);
>> +	if (!s)
>> +		return -ENOMEM;
>> +
>> +	s->global = ioread32(&stdev->mmio_sw_event->global_summary);
>> +	s->part_bitmap = ioread32(&stdev->mmio_sw_event->part_event_bitmap);
>> +	s->local_part = ioread32(&stdev->mmio_part_cfg->part_event_summary);
>>   
>>   	for (i = 0; i < stdev->partition_count; i++) {
>>   		reg = ioread32(&stdev->mmio_part_cfg_all[i].part_event_summary);
>> -		s.part[i] = reg;
>> +		s->part[i] = reg;
>>   	}
>>   
>>   	for (i = 0; i < SWITCHTEC_MAX_PFF_CSR; i++) {
> 
> Should this be "i < stdev->pff_csr_count", as in
> check_link_state_events(), enable_link_state_events() and
> mask_all_events()?  If so, I assume the read and check of vendor_id
> would be unnecessary?

Yes, nice catch. I think that would be a good simplification.

> The last loop in init_pff() currently checks against
> SWITCHTEC_MAX_PFF_CSR:
> 
>          for (i = 0; i < ARRAY_SIZE(pcfg->dsp_pff_inst_id); i++) {
>                  reg = ioread32(&pcfg->dsp_pff_inst_id[i]);
>                  if (reg < SWITCHTEC_MAX_PFF_CSR)
>                          stdev->pff_local[reg] = 1;
>          }
> 
> Should it check "reg < stdev->pff_csr_count" instead?  It looks like
> mask_all_events(), the only reader of pff_local[], only looks up to
> pff_csr_count anyway.

Yeah, I could go either way. The hardware would have to be broken if reg 
was between pff_csr_count and SWITCHTEC_MAX_PFF_CSR. This is mostly to 
catch 0xFF which indicates it's unset (if I recall correctly).

Logan