Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1551146 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [PATCH] serial: 8250_lpss: Release Quark MSI vectors on exit Jan Kiszka <jan.kiszka@siemens.com> - 2017-01-04 21:00 +0100
    Re: [PATCH] serial: 8250_lpss: Release Quark MSI vectors on exit Andy Shevchenko <andy.shevchenko@gmail.com> - 2017-01-04 23:30 +0100
      Re: [PATCH] serial: 8250_lpss: Release Quark MSI vectors on exit Jan Kiszka <jan.kiszka@siemens.com> - 2017-01-04 23:50 +0100
        Re: [PATCH] serial: 8250_lpss: Release Quark MSI vectors on exit Andy Shevchenko <andy.shevchenko@gmail.com> - 2017-01-05 00:00 +0100
        Re: [PATCH] serial: 8250_lpss: Release Quark MSI vectors on exit Christoph Hellwig <hch@lst.de> - 2017-01-05 06:30 +0100
          Re: [PATCH] serial: 8250_lpss: Release Quark MSI vectors on exit Jan Kiszka <jan.kiszka@siemens.com> - 2017-01-05 08:10 +0100
      Re: [PATCH] serial: 8250_lpss: Release Quark MSI vectors on exit Christoph Hellwig <hch@infradead.org> - 2017-01-08 11:30 +0100
        Re: [PATCH] serial: 8250_lpss: Release Quark MSI vectors on exit Andy Shevchenko <andriy.shevchenko@linux.intel.com> - 2017-01-08 17:10 +0100
    Re: [PATCH] serial: 8250_lpss: Release Quark MSI vectors on exit Christoph Hellwig <hch@infradead.org> - 2017-01-05 06:30 +0100
      Re: [PATCH] serial: 8250_lpss: Release Quark MSI vectors on exit Andy Shevchenko <andriy.shevchenko@linux.intel.com> - 2017-01-05 10:10 +0100

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

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

[toc] | [next] | [standalone]

#1551449

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

[toc] | [prev] | [next] | [standalone]

#1551467

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

[toc] | [prev] | [next] | [standalone]

#1551476

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

[toc] | [prev] | [next] | [standalone]

#1551636

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.

[toc] | [prev] | [next] | [standalone]

#1551680

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

[toc] | [prev] | [next] | [standalone]

#1553830

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.

[toc] | [prev] | [next] | [standalone]

#1553900

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

[toc] | [prev] | [next] | [standalone]

#1551637

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>

[toc] | [prev] | [next] | [standalone]

#1551777

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web