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