Re: [PATCH 3/5] switchtec: A temporary variable should be used for the flags of switchtec_ioctl_event_ctl

From: Logan Gunthorpe <logang@deltatee.com>
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
Subject: Re: [PATCH 3/5] switchtec: A temporary variable should be used for the flags of switchtec_ioctl_event_ctl
Date: Wed, 12 Dec 2018 15:52:48 -0700	[thread overview]
Message-ID: <0e8da614-dd78-5e41-df25-ec1b037ef4d7@deltatee.com> (raw)
In-Reply-To: <20181212224315.GK99796@google.com>

On 2018-12-12 3:43 p.m., Bjorn Helgaas wrote:
> On Mon, Dec 10, 2018 at 05:12:22PM +0800, Wesley Sheng wrote:
>> From: Joey Zhang <joey.zhang@microchip.com>
>>
>> For nr_idxs is larger than 1 switchtec_ioctl_event_ctl event flags will be
>> used by each event indexes. In current implementation the event flags are
>> overwritten by first call of the function event_ctl().
>>
>> Preserve the event flag value with a temporary variable.
>>
>> Fixes: 52eabba5bcdb ("switchtec: Add IOCTLs to the Switchtec driver")
>> Signed-off-by: Joey Zhang <joey.zhang@microchip.com>
>> Signed-off-by: Wesley Sheng <wesley.sheng@microchip.com>
>> Reviewed-by: Logan Gunthorpe <logang@deltatee.com>
>> ---
>>  drivers/pci/switch/switchtec.c | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/drivers/pci/switch/switchtec.c b/drivers/pci/switch/switchtec.c
>> index 480107e..a908670 100644
>> --- a/drivers/pci/switch/switchtec.c
>> +++ b/drivers/pci/switch/switchtec.c
>> @@ -796,6 +796,7 @@ static int ioctl_event_ctl(struct switchtec_dev *stdev,
>>  {
>>  	int ret;
>>  	int nr_idxs;
>> +	unsigned int event_flags;
>>  	struct switchtec_ioctl_event_ctl ctl;
>>  
>>  	if (copy_from_user(&ctl, uctl, sizeof(ctl)))
>> @@ -817,7 +818,9 @@ static int ioctl_event_ctl(struct switchtec_dev *stdev,
>>  		else
>>  			return -EINVAL;
>>  
>> +		event_flags = ctl.flags;
>>  		for (ctl.index = 0; ctl.index < nr_idxs; ctl.index++) {
>> +			ctl.flags = event_flags;
>>  			ret = event_ctl(stdev, &ctl);
> 
> event_ctl() overwrites several other things, in addition to ctl.flags:
> 
>   ctl.data[]
>   ctl.occurred
>   ctl.count
> 
> Is that what you intend?  It looks like only the values from the *last*
> call of event_ctl() will be copied back to the user buffer.

Yeah, it's just SWITCHTEC_IOCTL_EVENT_IDX_ALL is perhaps a strange abuse
of the interface. The intention being that if you are querying
information about an event you'd use it's specific index. If you are
trying to set flags you can set them for all event of a specific type at
once using IDX_ALL.

Looking at it now it looks pretty ugly (and I'm not sure what I was
thinking when I wrote it). But it's what we have and this patch fixes a
bug where we aren't actually enabling/disabling all events when that's
what the user is asking for.

Logan