Messages in this thread | | | From | Logan Gunthorpe <> | Date | Thu, 18 Apr 2019 09:49:56 -0600 | Subject | Re: [PATCH v2 1/2] switchtec: Fix false maximum supported PCIe function number issue |
| |
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
| |