[3/5] switchtec: A temporary variable should be used for the flags of switchtec_ioctl_event_ctl
diff mbox series

Message ID 1544433144-7563-4-git-send-email-wesley.sheng@microchip.com
State In Next
Commit e4a7dca5de625018b29417ecc39dc5037d9a5a36
Headers show
Series
  • Switchtec MRPC DMA mode support
Related show

Commit Message

Wesley Sheng Dec. 10, 2018, 9:12 a.m. UTC
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(+)

Comments

Bjorn Helgaas Dec. 12, 2018, 10:43 p.m. UTC | #1
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.

>  			if (ret < 0)
>  				return ret;
> -- 
> 2.7.4
>
Logan Gunthorpe Dec. 12, 2018, 10:52 p.m. UTC | #2
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

Patch
diff mbox series

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);
 			if (ret < 0)
 				return ret;