* [PATCH 0/2] Fix two bugs of switchtec module
@ 2019-04-08 14:34 Wesley Sheng
2019-04-08 14:34 ` [PATCH 1/2] switchtec: Fix false maximum supported PCIe function number issue Wesley Sheng
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Wesley Sheng @ 2019-04-08 14:34 UTC (permalink / raw)
To: kurt.schwemmer, logang, bhelgaas, linux-pci, linux-kernel
Cc: wesleyshenggit, wesley.sheng, kelvin.cao
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
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
^ permalink raw reply [flat|nested] 6+ messages in thread* [PATCH 1/2] switchtec: Fix false maximum supported PCIe function number issue 2019-04-08 14:34 [PATCH 0/2] Fix two bugs of switchtec module Wesley Sheng @ 2019-04-08 14:34 ` Wesley Sheng 2019-04-09 22:36 ` Bjorn Helgaas 2019-04-08 14:34 ` [PATCH 2/2] switchtec: Fix unintended mask of MRPC event Wesley Sheng 2019-04-08 16:00 ` [PATCH 0/2] Fix two bugs of switchtec module Logan Gunthorpe 2 siblings, 1 reply; 6+ messages in thread From: Wesley Sheng @ 2019-04-08 14:34 UTC (permalink / raw) To: kurt.schwemmer, logang, bhelgaas, linux-pci, linux-kernel Cc: wesleyshenggit, wesley.sheng, kelvin.cao 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] switchtec: Fix false maximum supported PCIe function number issue 2019-04-08 14:34 ` [PATCH 1/2] switchtec: Fix false maximum supported PCIe function number issue Wesley Sheng @ 2019-04-09 22:36 ` Bjorn Helgaas 2019-04-09 23:32 ` Logan Gunthorpe 0 siblings, 1 reply; 6+ messages in thread From: Bjorn Helgaas @ 2019-04-09 22:36 UTC (permalink / raw) To: Wesley Sheng Cc: kurt.schwemmer, logang, linux-pci, linux-kernel, wesleyshenggit, kelvin.cao Hi Wesley, On Mon, Apr 08, 2019 at 10:34:47PM +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 above is either one paragraph that needs to be rewrapped, or two paragraphs that need a blank line between. What's a PFF? > 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) I'm not an ioctl expert. Is the different struct size enough to distinguish these two, since they're both ioctl number 0x42? If I'm reading the patch right, a user program compiled against the old header will continue working unchanged (with only 48 PFFs) even though the kernel struct name changed from switchtec_ioctl_event_summary to switchtec_ioctl_event_summary_legacy. But if it is merely recompiled against the new header with no other change, it will silently start using 255 PFFs. I guess as long as it uses "sizeof(switchtec_ioctl_event_summary.pff)" or similar, it should work, but if it assumed an array size of 48, it will break. No doubt this is all exactly what you intended, and if I understood ioctls it would be obvious to me. But it would be *more* obviously backwards-compatible if you left the existing ioctl number and structure the same and merely added a new "SWITCHTEC_IOCTL_EVENT_SUMMARY_EXTENDED" with a new number and a new struct. So I'm just asking to be sure. > #define SWITCHTEC_IOCTL_EVENT_CTL \ > _IOWR('W', 0x43, struct switchtec_ioctl_event_ctl) > #define SWITCHTEC_IOCTL_PFF_TO_PORT \ > -- > 2.7.4 > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] switchtec: Fix false maximum supported PCIe function number issue 2019-04-09 22:36 ` Bjorn Helgaas @ 2019-04-09 23:32 ` Logan Gunthorpe 0 siblings, 0 replies; 6+ messages in thread From: Logan Gunthorpe @ 2019-04-09 23:32 UTC (permalink / raw) To: Bjorn Helgaas, Wesley Sheng Cc: kurt.schwemmer, linux-pci, linux-kernel, wesleyshenggit, kelvin.cao On 2019-04-09 4:36 p.m., Bjorn Helgaas wrote: > Hi Wesley, > > On Mon, Apr 08, 2019 at 10:34:47PM +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 above is either one paragraph that needs to be rewrapped, or two > paragraphs that need a blank line between. Wesley can address this. > What's a PFF? 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). Logan ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 2/2] switchtec: Fix unintended mask of MRPC event 2019-04-08 14:34 [PATCH 0/2] Fix two bugs of switchtec module Wesley Sheng 2019-04-08 14:34 ` [PATCH 1/2] switchtec: Fix false maximum supported PCIe function number issue Wesley Sheng @ 2019-04-08 14:34 ` Wesley Sheng 2019-04-08 16:00 ` [PATCH 0/2] Fix two bugs of switchtec module Logan Gunthorpe 2 siblings, 0 replies; 6+ messages in thread From: Wesley Sheng @ 2019-04-08 14:34 UTC (permalink / raw) To: kurt.schwemmer, logang, bhelgaas, linux-pci, linux-kernel Cc: wesleyshenggit, wesley.sheng, kelvin.cao 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 0/2] Fix two bugs of switchtec module 2019-04-08 14:34 [PATCH 0/2] Fix two bugs of switchtec module Wesley Sheng 2019-04-08 14:34 ` [PATCH 1/2] switchtec: Fix false maximum supported PCIe function number issue Wesley Sheng 2019-04-08 14:34 ` [PATCH 2/2] switchtec: Fix unintended mask of MRPC event Wesley Sheng @ 2019-04-08 16:00 ` Logan Gunthorpe 2 siblings, 0 replies; 6+ messages in thread From: Logan Gunthorpe @ 2019-04-08 16:00 UTC (permalink / raw) To: Wesley Sheng, kurt.schwemmer, bhelgaas, linux-pci, linux-kernel Cc: wesleyshenggit, kelvin.cao On 2019-04-08 8:34 a.m., Wesley Sheng wrote: > Wesley Sheng (2): > switchtec: Fix false maximum supported PCIe function number issue > switchtec: Fix unintended mask of MRPC event This series looks good to me: Reviewed-by: Logan Gunthorpe <logang@deltatee.com> Thanks, Logan ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-04-09 23:32 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-04-08 14:34 [PATCH 0/2] Fix two bugs of switchtec module Wesley Sheng 2019-04-08 14:34 ` [PATCH 1/2] switchtec: Fix false maximum supported PCIe function number issue Wesley Sheng 2019-04-09 22:36 ` Bjorn Helgaas 2019-04-09 23:32 ` Logan Gunthorpe 2019-04-08 14:34 ` [PATCH 2/2] switchtec: Fix unintended mask of MRPC event Wesley Sheng 2019-04-08 16:00 ` [PATCH 0/2] Fix two bugs of switchtec module Logan Gunthorpe
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).