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

[Logan Gunthorpe <logang@deltatee.com>]

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