Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
To:     Bjorn Helgaas <helgaas@kernel.org>,
        Wesley Sheng <wesley.sheng@microchip.com>
Cc:     kurt.schwemmer@microsemi.com, linux-pci@vger.kernel.org,
        linux-kernel@vger.kernel.org, wesleyshenggit@sina.com
References: <1555339302-31829-1-git-send-email-wesley.sheng@microchip.com>
 <1555339302-31829-2-git-send-email-wesley.sheng@microchip.com>
 <20190417224858.GW126710@google.com>
From:   Logan Gunthorpe <logang@deltatee.com>
Message-ID: <b3318670-f7f4-5f2e-751a-e028d4caf0b9@deltatee.com>
Date:   Thu, 18 Apr 2019 09:49:56 -0600
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101
 Thunderbird/60.6.1
MIME-Version: 1.0
In-Reply-To: <20190417224858.GW126710@google.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Subject: Re: [PATCH v2 1/2] switchtec: Fix false maximum supported PCIe
 function number issue
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk


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