[PATCH] serial: 8250_lpss: Release Quark MSI vectors on exit

[PATCH] serial: 8250_lpss: Release Quark MSI vectors on exit

[Jan Kiszka]

No one seems to do this magically in the background, so we have to do
the job in the exit handler that corresponds to the board setup handler.

Fixes: 60a9244a5d14 ("serial: 8250_lpss: enable MSI for Intel Quark")
Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 drivers/tty/serial/8250/8250_lpss.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_lpss.c b/drivers/tty/serial/8250/8250_lpss.c
index 58cbb30..f09f68a 100644
--- a/drivers/tty/serial/8250/8250_lpss.c
+++ b/drivers/tty/serial/8250/8250_lpss.c
@@ -47,7 +47,7 @@ struct lpss8250_board {
 	unsigned long freq;
 	unsigned int base_baud;
 	int (*setup)(struct lpss8250 *, struct uart_port *p);
-	void (*exit)(struct lpss8250 *);
+	void (*exit)(struct lpss8250 *, struct pci_dev *pdev);
 };
 
 struct lpss8250 {
@@ -226,9 +226,10 @@ static int qrk_serial_setup(struct lpss8250 *lpss, struct uart_port *port)
 	return 0;
 }
 
-static void qrk_serial_exit(struct lpss8250 *lpss)
+static void qrk_serial_exit(struct lpss8250 *lpss, struct pci_dev *pdev)
 {
 	qrk_serial_exit_dma(lpss);
+	pci_free_irq_vectors(pdev);
 }
 
 static bool lpss8250_dma_filter(struct dma_chan *chan, void *param)
@@ -324,7 +325,7 @@ static int lpss8250_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 
 err_exit:
 	if (lpss->board->exit)
-		lpss->board->exit(lpss);
+		lpss->board->exit(lpss, pdev);
 	return ret;
 }
 
@@ -333,7 +334,7 @@ static void lpss8250_remove(struct pci_dev *pdev)
 	struct lpss8250 *lpss = pci_get_drvdata(pdev);
 
 	if (lpss->board->exit)
-		lpss->board->exit(lpss);
+		lpss->board->exit(lpss, pdev);
 
 	serial8250_unregister_port(lpss->line);
 }
-- 
2.1.4

Re: [PATCH] serial: 8250_lpss: Release Quark MSI vectors on exit

[Andy Shevchenko]

On Wed, Jan 4, 2017 at 9:36 PM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
> No one seems to do this magically in the background, so we have to do
> the job in the exit handler that corresponds to the board setup handler.
>
> Fixes: 60a9244a5d14 ("serial: 8250_lpss: enable MSI for Intel Quark")
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>

NAK, check the PCI devres code, please.

Christoph, can you amend documentation to make this clear?

I NAKed already third patch related to PCI managed resources (couple
of those regarding to pci_irq_* API)/

> ---
>  drivers/tty/serial/8250/8250_lpss.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/tty/serial/8250/8250_lpss.c b/drivers/tty/serial/8250/8250_lpss.c
> index 58cbb30..f09f68a 100644
> --- a/drivers/tty/serial/8250/8250_lpss.c
> +++ b/drivers/tty/serial/8250/8250_lpss.c
> @@ -47,7 +47,7 @@ struct lpss8250_board {
>         unsigned long freq;
>         unsigned int base_baud;
>         int (*setup)(struct lpss8250 *, struct uart_port *p);
> -       void (*exit)(struct lpss8250 *);
> +       void (*exit)(struct lpss8250 *, struct pci_dev *pdev);
>  };
>
>  struct lpss8250 {
> @@ -226,9 +226,10 @@ static int qrk_serial_setup(struct lpss8250 *lpss, struct uart_port *port)
>         return 0;
>  }
>
> -static void qrk_serial_exit(struct lpss8250 *lpss)
> +static void qrk_serial_exit(struct lpss8250 *lpss, struct pci_dev *pdev)
>  {
>         qrk_serial_exit_dma(lpss);
> +       pci_free_irq_vectors(pdev);
>  }
>
>  static bool lpss8250_dma_filter(struct dma_chan *chan, void *param)
> @@ -324,7 +325,7 @@ static int lpss8250_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>
>  err_exit:
>         if (lpss->board->exit)
> -               lpss->board->exit(lpss);
> +               lpss->board->exit(lpss, pdev);
>         return ret;
>  }
>
> @@ -333,7 +334,7 @@ static void lpss8250_remove(struct pci_dev *pdev)
>         struct lpss8250 *lpss = pci_get_drvdata(pdev);
>
>         if (lpss->board->exit)
> -               lpss->board->exit(lpss);
> +               lpss->board->exit(lpss, pdev);
>
>         serial8250_unregister_port(lpss->line);
>  }
> --
> 2.1.4



-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] serial: 8250_lpss: Release Quark MSI vectors on exit

[Jan Kiszka]

On 2017-01-04 23:19, Andy Shevchenko wrote:
> On Wed, Jan 4, 2017 at 9:36 PM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>> No one seems to do this magically in the background, so we have to do
>> the job in the exit handler that corresponds to the board setup handler.
>>
>> Fixes: 60a9244a5d14 ("serial: 8250_lpss: enable MSI for Intel Quark")
>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> 
> NAK, check the PCI devres code, please.
> 
> Christoph, can you amend documentation to make this clear?
> 
> I NAKed already third patch related to PCI managed resources (couple
> of those regarding to pci_irq_* API)/
> 

Ah, there are resources that are managed without being allocated
explicitly that way. Hmm, not very intuitive. Are MSI / MSI-X vectors
the only such cases?

Thanks,
Jan

-- 
Siemens AG, Corporate Technology, CT RDA ITP SES-DE
Corporate Competence Center Embedded Linux

Re: [PATCH] serial: 8250_lpss: Release Quark MSI vectors on exit

[Andy Shevchenko]

On Thu, Jan 5, 2017 at 12:46 AM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
> On 2017-01-04 23:19, Andy Shevchenko wrote:
>> On Wed, Jan 4, 2017 at 9:36 PM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>>> No one seems to do this magically in the background, so we have to do
>>> the job in the exit handler that corresponds to the board setup handler.
>>>
>>> Fixes: 60a9244a5d14 ("serial: 8250_lpss: enable MSI for Intel Quark")
>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>>
>> NAK, check the PCI devres code, please.
>>
>> Christoph, can you amend documentation to make this clear?
>>
>> I NAKed already third patch related to PCI managed resources (couple
>> of those regarding to pci_irq_* API)/
>>
>
> Ah, there are resources that are managed without being allocated
> explicitly that way. Hmm, not very intuitive. Are MSI / MSI-X vectors
> the only such cases?

pci_request_regions(), etc.
Almost all stuff which is usually used in ->probe() of casual PCI driver.


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] serial: 8250_lpss: Release Quark MSI vectors on exit

[Christoph Hellwig]

On Wed, Jan 04, 2017 at 08:36:51PM +0100, Jan Kiszka wrote:
> No one seems to do this magically in the background, so we have to do
> the job in the exit handler that corresponds to the board setup handler.
> 
> Fixes: 60a9244a5d14 ("serial: 8250_lpss: enable MSI for Intel Quark")
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>

Looks good.   I already pointed out to Andy that we need this anyway.

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH] serial: 8250_lpss: Release Quark MSI vectors on exit

[Christoph Hellwig]

On Wed, Jan 04, 2017 at 11:46:58PM +0100, Jan Kiszka wrote:
> > I NAKed already third patch related to PCI managed resources (couple
> > of those regarding to pci_irq_* API)/
> > 
> 
> Ah, there are resources that are managed without being allocated
> explicitly that way. Hmm, not very intuitive. Are MSI / MSI-X vectors
> the only such cases?

MSI/MSI-X resources are not managed that way and an explicit call to
pci_free_irq_vectors is required from the API standpoint.  It might not
actually free memory in many cases, but it still is a symmetric API.

Re: [PATCH] serial: 8250_lpss: Release Quark MSI vectors on exit

[Jan Kiszka]

On 2017-01-05 06:25, Christoph Hellwig wrote:
> On Wed, Jan 04, 2017 at 11:46:58PM +0100, Jan Kiszka wrote:
>>> I NAKed already third patch related to PCI managed resources (couple
>>> of those regarding to pci_irq_* API)/
>>>
>>
>> Ah, there are resources that are managed without being allocated
>> explicitly that way. Hmm, not very intuitive. Are MSI / MSI-X vectors
>> the only such cases?
> 
> MSI/MSI-X resources are not managed that way and an explicit call to
> pci_free_irq_vectors is required from the API standpoint.  It might not
> actually free memory in many cases, but it still is a symmetric API.
> 

Andy is referring to the following:
- pcim_enable_device registers pci_devres with the devres subsystem
- on device release, devres_release_all is invoked, and that calls
  pcim_release
- the latter will invoke pci_disable_msi and pci_disable_msix if any of
  them is enabled

The user will still call the same pci_msi_enable, pci_alloc_irq_vectors
etc., but they are now "magically" managed with pcim_enable_device being
used. Same for pci_request_region.

Jan

-- 
Siemens AG, Corporate Technology, CT RDA ITP SES-DE
Corporate Competence Center Embedded Linux

Re: [PATCH] serial: 8250_lpss: Release Quark MSI vectors on exit

[Andy Shevchenko]

On Wed, 2017-01-04 at 21:25 -0800, Christoph Hellwig wrote:
> On Wed, Jan 04, 2017 at 08:36:51PM +0100, Jan Kiszka wrote:
> > No one seems to do this magically in the background, so we have to
> > do
> > the job in the exit handler that corresponds to the board setup
> > handler.
> > 
> > Fixes: 60a9244a5d14 ("serial: 8250_lpss: enable MSI for Intel
> > Quark")
> > Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> 
> Looks good.   I already pointed out to Andy that we need this anyway.
> 
> Reviewed-by: Christoph Hellwig <hch@lst.de>

No, we don't.

Jan already pointed out how it works right now.

That's why I'm asking to fix documentation.

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH] serial: 8250_lpss: Release Quark MSI vectors on exit

[Christoph Hellwig]

On Thu, Jan 05, 2017 at 12:19:56AM +0200, Andy Shevchenko wrote:
> NAK, check the PCI devres code, please.

Releasing something through devres that wasn't allocated using a devm_* or
pcim_* function isn't expected, and we should fix that instead.
pci_free_irq_vectors is _currently_ implemented by calling
pci_disable_msi and pci_disable_msix, but there is no guarantee for that
in the API.

Your code works by accident, not by design.  If you want the resources
to be auto-released you need to add a proper pcim_alloc_irq_vectors
API.

Re: [PATCH] serial: 8250_lpss: Release Quark MSI vectors on exit

[Andy Shevchenko]

On Sun, 2017-01-08 at 02:24 -0800, Christoph Hellwig wrote:
> On Thu, Jan 05, 2017 at 12:19:56AM +0200, Andy Shevchenko wrote:
> > NAK, check the PCI devres code, please.

+Cc: Tejun, Bjorn.

> 
> Releasing something through devres that wasn't allocated using a
> devm_* or
> pcim_* function isn't expected, and we should fix that instead.
> pci_free_irq_vectors is _currently_ implemented by calling
> pci_disable_msi and pci_disable_msix, but there is no guarantee for
> that
> in the API.

> Your code works by accident, not by design.  If you want the resources
> to be auto-released you need to add a proper pcim_alloc_irq_vectors
> API.

Though idea sounds sane I disagree this is accidental. The PCI managed
introduction includes among other this one:

+  pcim_enable_device() : after success, all PCI ops become managed

As per commit 9ac7849e35f7 ("devres: device resource management").

Thus, I suppose a new API should follow existing design, or provide a
sane fix. I briefly checked MSI/-X usage and since I'm not so familiar
with the PCI core code, I wouldn't be brave to break it.

Currently I suspect pci_free_irq_vectors() is not friendly with
pcim_release().

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy