LinuxLists.cc - [PATCH v2 0/2] PCI: switchtec: Trivial cleanups

2022-12-16 16:39:57

Subject: [PATCH v2 0/2] PCI: switchtec: Trivial cleanups

From: Bjorn Helgaas <[email protected]>

Simplify switchtec_dma_mrpc_isr() slightly and return the right
copy_to_user() error code from switchtec_dev_read().

Bjorn Helgaas (2):
PCI: switchtec: Simplify switchtec_dma_mrpc_isr()
PCI: switchtec: Return -EFAULT for copy_to_user() errors

drivers/pci/switch/switchtec.c | 12 +++++-------
1 file changed, 5 insertions(+), 7 deletions(-)

Changes between v1 and v2:
- Return -EFAULT for copy_to_user() errors.
--
2.25.1

2022-12-16 16:45:41

by Bjorn Helgaas

[permalink] [raw]

Subject: [PATCH v2 2/2] PCI: switchtec: Return -EFAULT for copy_to_user() errors

From: Bjorn Helgaas <[email protected]>

switchtec_dev_read() didn't handle copy_to_user() errors correctly: it
assigned "rc = -EFAULT", but actually returned either "size", -ENXIO, or
-EBADMSG instead.

Update the failure cases to unlock mrpc_mutex and return -EFAULT directly.

Fixes: 080b47def5e5 ("MicroSemi Switchtec management interface driver")
Signed-off-by: Bjorn Helgaas <[email protected]>
---
drivers/pci/switch/switchtec.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/pci/switch/switchtec.c b/drivers/pci/switch/switchtec.c
index d7ae84070e29..0ac9d4488210 100644
--- a/drivers/pci/switch/switchtec.c
+++ b/drivers/pci/switch/switchtec.c
@@ -606,16 +606,16 @@ static ssize_t switchtec_dev_read(struct file *filp, char __user *data,
rc = copy_to_user(data, &stuser->return_code,
sizeof(stuser->return_code));
if (rc) {
- rc = -EFAULT;
- goto out;
+ mutex_unlock(&stdev->mrpc_mutex);
+ return -EFAULT;
}

data += sizeof(stuser->return_code);
rc = copy_to_user(data, &stuser->data,
size - sizeof(stuser->return_code));
if (rc) {
- rc = -EFAULT;
- goto out;
+ mutex_unlock(&stdev->mrpc_mutex);
+ return -EFAULT;
}

stuser_set_state(stuser, MRPC_IDLE);
--
2.25.1

2022-12-16 16:45:58

by Logan Gunthorpe

[permalink] [raw]

Subject: Re: [PATCH v2 2/2] PCI: switchtec: Return -EFAULT for copy_to_user() errors

On 2022-12-16 09:21, Bjorn Helgaas wrote:
> From: Bjorn Helgaas <[email protected]>
>
> switchtec_dev_read() didn't handle copy_to_user() errors correctly: it
> assigned "rc = -EFAULT", but actually returned either "size", -ENXIO, or
> -EBADMSG instead.
>
> Update the failure cases to unlock mrpc_mutex and return -EFAULT directly.
>
> Fixes: 080b47def5e5 ("MicroSemi Switchtec management interface driver")
> Signed-off-by: Bjorn Helgaas <[email protected]>

Looks good to me thanks!

Reviewed-by: Logan Gunthorpe <[email protected]>

Logan

2022-12-16 16:49:25

by Bjorn Helgaas

[permalink] [raw]

Subject: [PATCH v2 1/2] PCI: switchtec: Simplify switchtec_dma_mrpc_isr()

From: Bjorn Helgaas <[email protected]>

The "ret" variable in switchtec_dma_mrpc_isr() is superfluous. Remove it
and just return the value. No functional change intended.

Signed-off-by: Bjorn Helgaas <[email protected]>
Reviewed-by: Logan Gunthorpe <[email protected]>
---
drivers/pci/switch/switchtec.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/pci/switch/switchtec.c b/drivers/pci/switch/switchtec.c
index 75be4fe22509..d7ae84070e29 100644
--- a/drivers/pci/switch/switchtec.c
+++ b/drivers/pci/switch/switchtec.c
@@ -1480,15 +1480,13 @@ static irqreturn_t switchtec_event_isr(int irq, void *dev)
static irqreturn_t switchtec_dma_mrpc_isr(int irq, void *dev)
{
struct switchtec_dev *stdev = dev;
- irqreturn_t ret = IRQ_NONE;

iowrite32(SWITCHTEC_EVENT_CLEAR |
SWITCHTEC_EVENT_EN_IRQ,
&stdev->mmio_part_cfg->mrpc_comp_hdr);
schedule_work(&stdev->mrpc_work);

- ret = IRQ_HANDLED;
- return ret;
+ return IRQ_HANDLED;
}

static int switchtec_init_isr(struct switchtec_dev *stdev)
--
2.25.1

2022-12-16 18:40:26

by kernel test robot

[permalink] [raw]

Subject: Re: [PATCH v2 2/2] PCI: switchtec: Return -EFAULT for copy_to_user() errors

Hi Bjorn,

I love your patch! Perhaps
[auto build test WARNING [also build test WARNING [If your patch is applied And when submitting patch, compiler: gcc-11 (Debian reproduce (this is a W=1 build):
# something to improve:
on helgaas-pci/next]
on helgaas-pci/for-linus linus/master v6.1 next-20221216]
to the wrong git tree, kindly drop us a note.
we suggest to use '--base' as documented in
docs/git-format-patch#_base_tree_information]">https://git-scm.com/docs/git-format-patch#_base_tree_information]
ub.com/intel-lab-lkp/linux/commits/Bjorn-Helgaas/PCI-switchtec-Trivial-cleanups/20221217-002341">https://github.com/intel-lab-lkp/linux/commits/Bjorn-Helgaas/PCI-switchtec-Trivial-cleanups/20221217-002341
kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git">https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git next
://lore.kernel.org/r/20221216162126.207863-3-helgaas%40kernel.org">https://lore.kernel.org/r/20221216162126.207863-3-helgaas%40kernel.org
v2 2/2] PCI: switchtec: Return -EFAULT for copy_to_user() errors
11.3.0-8) 11.3.0
thub.com/intel-lab-lkp/linux/commit/a598bd934af2ccce340a59da1d10a3959b085dd0">https://github.com/intel-lab-lkp/linux/commit/a598bd934af2ccce340a59da1d10a3959b085dd0
href="https://github.com/intel-lab-lkp/linux">https://github.com/intel-lab-lkp/linux
linux-review Bjorn-Helgaas/PCI-switchtec-Trivial-cleanups/20221217-002341
a598bd934af2ccce340a59da1d10a3959b085dd0
&& cp config build_dir/.config
ARCH=x86_64 olddefconfig
ARCH=x86_64 SHELL=/bin/bash drivers/pci/
kindly add following tag where applicable
test robot <[email protected]>
prefixed by >>):
In function 'switchtec_dev_read':
itchtec.c:623:1: warning: label 'out' defined but not used [-Wunused-label]
tch/switchtec.c
2017-03-06 557
2017-03-06 558 static ssize_t switchtec_dev_read(struct file *filp, char __user *data,
2017-03-06 559 size_t size, loff_t *off)
2017-03-06 560 {
2017-03-06 561 struct switchtec_user *stuser = filp->private_data;
2017-03-06 562 struct switchtec_dev *stdev = stuser->stdev;
2017-03-06 563 int rc;
2017-03-06 564
2017-03-06 565 if (size < sizeof(stuser->cmd) ||
2017-03-06 566 size > sizeof(stuser->cmd) + sizeof(stuser->data))
2017-03-06 567 return -EINVAL;
2017-03-06 568
2017-03-06 569 rc = lock_mutex_and_test_alive(stdev);
2017-03-06 570 if (rc)
2017-03-06 571 return rc;
2017-03-06 572
2017-03-06 573 if (stuser->state == MRPC_IDLE) {
2017-03-06 574 mutex_unlock(&stdev->mrpc_mutex);
2017-03-06 575 return -EBADE;
2017-03-06 576 }
2017-03-06 577
2017-03-06 578 stuser->read_len = size - sizeof(stuser->return_code);
2017-03-06 579
2017-03-06 580 mutex_unlock(&stdev->mrpc_mutex);
2017-03-06 581
2017-03-06 582 if (filp->f_flags & O_NONBLOCK) {
Andrzej Siewior 2020-03-21 583 if (!stuser->cmd_done)
2017-03-06 584 return -EAGAIN;
2017-03-06 585 } else {
Andrzej Siewior 2020-03-21 586 rc = wait_event_interruptible(stuser->cmd_comp,
Andrzej Siewior 2020-03-21 587 stuser->cmd_done);
2017-03-06 588 if (rc < 0)
2017-03-06 589 return rc;
2017-03-06 590 }
2017-03-06 591
2017-03-06 592 rc = lock_mutex_and_test_alive(stdev);
2017-03-06 593 if (rc)
2017-03-06 594 return rc;
2017-03-06 595
2021-10-14 596 if (stuser->state == MRPC_IO_ERROR) {
2021-10-14 597 mutex_unlock(&stdev->mrpc_mutex);
2021-10-14 598 return -EIO;
2021-10-14 599 }
2021-10-14 600
2017-03-06 601 if (stuser->state != MRPC_DONE) {
2017-03-06 602 mutex_unlock(&stdev->mrpc_mutex);
2017-03-06 603 return -EBADE;
2017-03-06 604 }
2017-03-06 605
2017-03-06 606 rc = copy_to_user(data, &stuser->return_code,
2017-03-06 607 sizeof(stuser->return_code));
2017-03-06 608 if (rc) {
2022-12-16 609 mutex_unlock(&stdev->mrpc_mutex);
2022-12-16 610 return -EFAULT;
2017-03-06 611 }
2017-03-06 612
2017-03-06 613 data += sizeof(stuser->return_code);
2017-03-06 614 rc = copy_to_user(data, &stuser->data,
2017-03-06 615 size - sizeof(stuser->return_code));
2017-03-06 616 if (rc) {
2022-12-16 617 mutex_unlock(&stdev->mrpc_mutex);
2022-12-16 618 return -EFAULT;
2017-03-06 619 }
2017-03-06 620
2017-03-06 621 stuser_set_state(stuser, MRPC_IDLE);
2017-03-06 622
2017-03-06 @623 out:
2017-03-06 624 mutex_unlock(&stdev->mrpc_mutex);
2017-03-06 625
2021-10-14 626 if (stuser->status == SWITCHTEC_MRPC_STATUS_DONE ||
2021-10-14 627 stuser->status == SWITCHTEC_MRPC_STATUS_ERROR)
2017-03-06 628 return size;
2017-03-06 629 else if (stuser->status == SWITCHTEC_MRPC_STATUS_INTERRUPTED)
2017-03-06 630 return -ENXIO;
2017-03-06 631 else
2017-03-06 632 return -EBADMSG;
2017-03-06 633 }
2017-03-06 634
https://01.org/lkp

Attachments:

(No filename) (8.00 kB)
config (297.60 kB)
Download all attachments

2022-12-16 19:50:57

by kernel test robot

[permalink] [raw]

Subject: Re: [PATCH v2 2/2] PCI: switchtec: Return -EFAULT for copy_to_user() errors

Hi Bjorn,

I love your patch! Perhaps something to improve:

[auto build test WARNING on helgaas-pci/next]
[also build test WARNING on linus/master v6.1 next-20221216]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url: https://github.com/intel-lab-lkp/linux/commits/Bjorn-Helgaas/PCI-switchtec-Trivial-cleanups/20221217-002341
base: https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git next
patch link: https://lore.kernel.org/r/20221216162126.207863-3-helgaas%40kernel.org
patch subject: [PATCH v2 2/2] PCI: switchtec: Return -EFAULT for copy_to_user() errors
config: i386-randconfig-a004
compiler: clang version 14.0.6 (https://github.com/llvm/llvm-project f28c006a5895fc0e329fe15fead81e37457cb1d1)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/a598bd934af2ccce340a59da1d10a3959b085dd0
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Bjorn-Helgaas/PCI-switchtec-Trivial-cleanups/20221217-002341
git checkout a598bd934af2ccce340a59da1d10a3959b085dd0
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=i386 olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=i386 SHELL=/bin/bash drivers/pci/switch/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <[email protected]>

All warnings (new ones prefixed by >>):

>> drivers/pci/switch/switchtec.c:623:1: warning: unused label 'out' [-Wunused-label]
out:
^~~~
1 warning generated.

vim +/out +623 drivers/pci/switch/switchtec.c

080b47def5e5e2 Logan Gunthorpe 2017-03-06 557
080b47def5e5e2 Logan Gunthorpe 2017-03-06 558 static ssize_t switchtec_dev_read(struct file *filp, char __user *data,
080b47def5e5e2 Logan Gunthorpe 2017-03-06 559 size_t size, loff_t *off)
080b47def5e5e2 Logan Gunthorpe 2017-03-06 560 {
080b47def5e5e2 Logan Gunthorpe 2017-03-06 561 struct switchtec_user *stuser = filp->private_data;
080b47def5e5e2 Logan Gunthorpe 2017-03-06 562 struct switchtec_dev *stdev = stuser->stdev;
080b47def5e5e2 Logan Gunthorpe 2017-03-06 563 int rc;
080b47def5e5e2 Logan Gunthorpe 2017-03-06 564
080b47def5e5e2 Logan Gunthorpe 2017-03-06 565 if (size < sizeof(stuser->cmd) ||
080b47def5e5e2 Logan Gunthorpe 2017-03-06 566 size > sizeof(stuser->cmd) + sizeof(stuser->data))
080b47def5e5e2 Logan Gunthorpe 2017-03-06 567 return -EINVAL;
080b47def5e5e2 Logan Gunthorpe 2017-03-06 568
080b47def5e5e2 Logan Gunthorpe 2017-03-06 569 rc = lock_mutex_and_test_alive(stdev);
080b47def5e5e2 Logan Gunthorpe 2017-03-06 570 if (rc)
080b47def5e5e2 Logan Gunthorpe 2017-03-06 571 return rc;
080b47def5e5e2 Logan Gunthorpe 2017-03-06 572
080b47def5e5e2 Logan Gunthorpe 2017-03-06 573 if (stuser->state == MRPC_IDLE) {
080b47def5e5e2 Logan Gunthorpe 2017-03-06 574 mutex_unlock(&stdev->mrpc_mutex);
080b47def5e5e2 Logan Gunthorpe 2017-03-06 575 return -EBADE;
080b47def5e5e2 Logan Gunthorpe 2017-03-06 576 }
080b47def5e5e2 Logan Gunthorpe 2017-03-06 577
080b47def5e5e2 Logan Gunthorpe 2017-03-06 578 stuser->read_len = size - sizeof(stuser->return_code);
080b47def5e5e2 Logan Gunthorpe 2017-03-06 579
080b47def5e5e2 Logan Gunthorpe 2017-03-06 580 mutex_unlock(&stdev->mrpc_mutex);
080b47def5e5e2 Logan Gunthorpe 2017-03-06 581
080b47def5e5e2 Logan Gunthorpe 2017-03-06 582 if (filp->f_flags & O_NONBLOCK) {
deaa0a8a74d865 Sebastian Andrzej Siewior 2020-03-21 583 if (!stuser->cmd_done)
080b47def5e5e2 Logan Gunthorpe 2017-03-06 584 return -EAGAIN;
080b47def5e5e2 Logan Gunthorpe 2017-03-06 585 } else {
deaa0a8a74d865 Sebastian Andrzej Siewior 2020-03-21 586 rc = wait_event_interruptible(stuser->cmd_comp,
deaa0a8a74d865 Sebastian Andrzej Siewior 2020-03-21 587 stuser->cmd_done);
080b47def5e5e2 Logan Gunthorpe 2017-03-06 588 if (rc < 0)
080b47def5e5e2 Logan Gunthorpe 2017-03-06 589 return rc;
080b47def5e5e2 Logan Gunthorpe 2017-03-06 590 }
080b47def5e5e2 Logan Gunthorpe 2017-03-06 591
080b47def5e5e2 Logan Gunthorpe 2017-03-06 592 rc = lock_mutex_and_test_alive(stdev);
080b47def5e5e2 Logan Gunthorpe 2017-03-06 593 if (rc)
080b47def5e5e2 Logan Gunthorpe 2017-03-06 594 return rc;
080b47def5e5e2 Logan Gunthorpe 2017-03-06 595
1a323bd071dd81 Kelvin Cao 2021-10-14 596 if (stuser->state == MRPC_IO_ERROR) {
1a323bd071dd81 Kelvin Cao 2021-10-14 597 mutex_unlock(&stdev->mrpc_mutex);
1a323bd071dd81 Kelvin Cao 2021-10-14 598 return -EIO;
1a323bd071dd81 Kelvin Cao 2021-10-14 599 }
1a323bd071dd81 Kelvin Cao 2021-10-14 600
080b47def5e5e2 Logan Gunthorpe 2017-03-06 601 if (stuser->state != MRPC_DONE) {
080b47def5e5e2 Logan Gunthorpe 2017-03-06 602 mutex_unlock(&stdev->mrpc_mutex);
080b47def5e5e2 Logan Gunthorpe 2017-03-06 603 return -EBADE;
080b47def5e5e2 Logan Gunthorpe 2017-03-06 604 }
080b47def5e5e2 Logan Gunthorpe 2017-03-06 605
080b47def5e5e2 Logan Gunthorpe 2017-03-06 606 rc = copy_to_user(data, &stuser->return_code,
080b47def5e5e2 Logan Gunthorpe 2017-03-06 607 sizeof(stuser->return_code));
080b47def5e5e2 Logan Gunthorpe 2017-03-06 608 if (rc) {
a598bd934af2cc Bjorn Helgaas 2022-12-16 609 mutex_unlock(&stdev->mrpc_mutex);
a598bd934af2cc Bjorn Helgaas 2022-12-16 610 return -EFAULT;
080b47def5e5e2 Logan Gunthorpe 2017-03-06 611 }
080b47def5e5e2 Logan Gunthorpe 2017-03-06 612
080b47def5e5e2 Logan Gunthorpe 2017-03-06 613 data += sizeof(stuser->return_code);
080b47def5e5e2 Logan Gunthorpe 2017-03-06 614 rc = copy_to_user(data, &stuser->data,
080b47def5e5e2 Logan Gunthorpe 2017-03-06 615 size - sizeof(stuser->return_code));
080b47def5e5e2 Logan Gunthorpe 2017-03-06 616 if (rc) {
a598bd934af2cc Bjorn Helgaas 2022-12-16 617 mutex_unlock(&stdev->mrpc_mutex);
a598bd934af2cc Bjorn Helgaas 2022-12-16 618 return -EFAULT;
080b47def5e5e2 Logan Gunthorpe 2017-03-06 619 }
080b47def5e5e2 Logan Gunthorpe 2017-03-06 620
080b47def5e5e2 Logan Gunthorpe 2017-03-06 621 stuser_set_state(stuser, MRPC_IDLE);
080b47def5e5e2 Logan Gunthorpe 2017-03-06 622
080b47def5e5e2 Logan Gunthorpe 2017-03-06 @623 out:
080b47def5e5e2 Logan Gunthorpe 2017-03-06 624 mutex_unlock(&stdev->mrpc_mutex);
080b47def5e5e2 Logan Gunthorpe 2017-03-06 625
551ec658b69807 Kelvin Cao 2021-10-14 626 if (stuser->status == SWITCHTEC_MRPC_STATUS_DONE ||
551ec658b69807 Kelvin Cao 2021-10-14 627 stuser->status == SWITCHTEC_MRPC_STATUS_ERROR)
080b47def5e5e2 Logan Gunthorpe 2017-03-06 628 return size;
080b47def5e5e2 Logan Gunthorpe 2017-03-06 629 else if (stuser->status == SWITCHTEC_MRPC_STATUS_INTERRUPTED)
080b47def5e5e2 Logan Gunthorpe 2017-03-06 630 return -ENXIO;
080b47def5e5e2 Logan Gunthorpe 2017-03-06 631 else
080b47def5e5e2 Logan Gunthorpe 2017-03-06 632 return -EBADMSG;
080b47def5e5e2 Logan Gunthorpe 2017-03-06 633 }
080b47def5e5e2 Logan Gunthorpe 2017-03-06 634

--
0-DAY CI Kernel Test Service
https://01.org/lkp

Attachments:

(No filename) (8.22 kB)
config (132.79 kB)
Download all attachments

2022-12-30 11:40:11

by Lorenzo Pieralisi

[permalink] [raw]

Subject: Re: [PATCH v2 0/2] PCI: switchtec: Trivial cleanups

On Fri, 16 Dec 2022 10:21:24 -0600, Bjorn Helgaas wrote:
> From: Bjorn Helgaas <[email protected]>
>
> Simplify switchtec_dma_mrpc_isr() slightly and return the right
> copy_to_user() error code from switchtec_dev_read().
>
> Bjorn Helgaas (2):
> PCI: switchtec: Simplify switchtec_dma_mrpc_isr()
> PCI: switchtec: Return -EFAULT for copy_to_user() errors
>
> [...]

Applied to pci/switchtec, thanks!

[1/2] PCI: switchtec: Simplify switchtec_dma_mrpc_isr()
https://git.kernel.org/lpieralisi/pci/c/1a810b9bb681
[2/2] PCI: switchtec: Return -EFAULT for copy_to_user() errors
https://git.kernel.org/lpieralisi/pci/c/fbc855bce49e

Thanks,
Lorenzo