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;
MIME-Version: 1.0
In-Reply-To: <20180619152524.6080-1-vasilyev@ispras.ru>
References: <20180619074248.ssu5kza6wdwhzdqi@mwanda> <20180619152524.6080-1-vasilyev@ispras.ru>
From:   Andy Shevchenko <andy.shevchenko@gmail.com>
Date:   Tue, 19 Jun 2018 20:13:02 +0300
Message-ID: <CAHp75VfktnjL+G0vjHNHr=8mbDXu+4BDogsnxrp_2Bo4em=r2Q@mail.gmail.com>
Subject: Re: [PATCH v4] staging: rts5208: add error handling into rtsx_probe
To:     Anton Vasilyev <vasilyev@ispras.ru>
Cc:     Dan Carpenter <dan.carpenter@oracle.com>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Sinan Kaya <okaya@codeaurora.org>,
        Johannes Thumshirn <jthumshirn@suse.de>,
        Gaurav Pathak <gauravpathak129@gmail.com>,
        Hannes Reinecke <hare@suse.de>, devel@driverdev.osuosl.org,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        ldv-project@linuxtesting.org
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Tue, Jun 19, 2018 at 6:25 PM, Anton Vasilyev <vasilyev@ispras.ru> wrote:
> If rtsx_probe() fails to allocate dev->chip, then release_everything()
> will crash on uninitialized dev->cmnd_ready complete

Period is missed at the end.

>
> Patch adds error handling into rtsx_probe.

an error

>
> Found by Linux Driver Verification project (linuxtesting.org).
>

Reviewed-by: Andy Shevchenko <andy.shevhchenko@gmail.com>

Couple of comments below though.

> Signed-off-by: Anton Vasilyev <vasilyev@ispras.ru>
> ---
> v4: rename labels baced on Dan Carpenter's recommendation
> v3: fix subject and commit message
> v2: Add error handling into rtsx_probe based on Dan Carpenter's comment.
> I do not have corresponding hardware, so patch was tested by compilation only.
>
> I faced with inaccuracy at rtsx_remove() and original rtsx_probe():
> there is quiesce_and_remove_host() call with scsi_remove_host() inside,
> whereas release_everything() calls scsi_host_put() after this
> scsi_remove_host() call. This is strange for me.
> Also I do not know is it require to check result value of
> rtsx_init_chip() call on rtsx_probe().
> ---
>  drivers/staging/rts5208/rtsx.c | 34 ++++++++++++++++++++++++----------
>  1 file changed, 24 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/staging/rts5208/rtsx.c b/drivers/staging/rts5208/rtsx.c
> index 70e0b8623110..233026ee5dd4 100644
> --- a/drivers/staging/rts5208/rtsx.c
> +++ b/drivers/staging/rts5208/rtsx.c
> @@ -879,7 +879,7 @@ static int rtsx_probe(struct pci_dev *pci,
>         if (!dev->remap_addr) {
>                 dev_err(&pci->dev, "ioremap error\n");
>                 err = -ENXIO;
> -               goto errout;
> +               goto err_chip_free;
>         }
>
>         /*
> @@ -894,7 +894,7 @@ static int rtsx_probe(struct pci_dev *pci,
>         if (!dev->rtsx_resv_buf) {
>                 dev_err(&pci->dev, "alloc dma buffer fail\n");
>                 err = -ENXIO;
> -               goto errout;
> +               goto err_addr_unmap;
>         }
>         dev->chip->host_cmds_ptr = dev->rtsx_resv_buf;
>         dev->chip->host_cmds_addr = dev->rtsx_resv_buf_addr;
> @@ -915,7 +915,7 @@ static int rtsx_probe(struct pci_dev *pci,
>
>         if (rtsx_acquire_irq(dev) < 0) {
>                 err = -EBUSY;
> -               goto errout;
> +               goto err_disable_msi;
>         }
>
>         pci_set_master(pci);
> @@ -935,14 +935,14 @@ static int rtsx_probe(struct pci_dev *pci,
>         if (IS_ERR(th)) {
>                 dev_err(&pci->dev, "Unable to start control thread\n");
>                 err = PTR_ERR(th);
> -               goto errout;
> +               goto err_rtsx_release;
>         }
>         dev->ctl_thread = th;
>
>         err = scsi_add_host(host, &pci->dev);
>         if (err) {
>                 dev_err(&pci->dev, "Unable to add the scsi host\n");
> -               goto errout;
> +               goto err_complete_control_thread;
>         }
>
>         /* Start up the thread for delayed SCSI-device scanning */
> @@ -950,18 +950,16 @@ static int rtsx_probe(struct pci_dev *pci,
>         if (IS_ERR(th)) {
>                 dev_err(&pci->dev, "Unable to start the device-scanning thread\n");
>                 complete(&dev->scanning_done);
> -               quiesce_and_remove_host(dev);
>                 err = PTR_ERR(th);
> -               goto errout;
> +               goto err_stop_host;
>         }
>
>         /* Start up the thread for polling thread */
>         th = kthread_run(rtsx_polling_thread, dev, "rtsx-polling");
>         if (IS_ERR(th)) {
>                 dev_err(&pci->dev, "Unable to start the device-polling thread\n");
> -               quiesce_and_remove_host(dev);
>                 err = PTR_ERR(th);
> -               goto errout;
> +               goto err_stop_host;
>         }
>         dev->polling_thread = th;
>
> @@ -970,9 +968,25 @@ static int rtsx_probe(struct pci_dev *pci,
>         return 0;
>
>         /* We come here if there are any problems */
> +err_stop_host:
> +       quiesce_and_remove_host(dev);
> +err_complete_control_thread:
> +       complete(&dev->cmnd_ready);
> +       wait_for_completion(&dev->control_exit);
> +err_rtsx_release:
> +       free_irq(dev->irq, (void *)dev);
> +       rtsx_release_chip(dev->chip);
> +err_disable_msi:

> +       dev->chip->host_cmds_ptr = NULL;
> +       dev->chip->host_sg_tbl_ptr = NULL;

This seems superfluous, you are freeing entire struct few calls after.

> +       if (dev->chip->msi_en)
> +               pci_disable_msi(dev->pci);

This might be material for another improvement, PCI core tracks
MSI/MSI-X enable state.
So, the conditional with the local variable looks superfluous as well
(in some, rare, cases we indeed need something like this to basically
distinguish MSI from MSI-X, though I don't think it's a case here)

> +err_addr_unmap:
> +       iounmap(dev->remap_addr);
> +err_chip_free:
> +       kfree(dev->chip);
>  errout:
>         dev_err(&pci->dev, "%s failed\n", __func__);
> -       release_everything(dev);
>
>         return err;
>  }
> --
> 2.17.1
>


-- 
With Best Regards,
Andy Shevchenko