[PATCH] serial: 8250_lpss: Unconditionally set PCI master for Quark

[PATCH] serial: 8250_lpss: Unconditionally set PCI master for Quark

[Jan Kiszka]

MSI needs it as well.

Should have no practical impact, though, as DMA is always available on
the Quark. But given the few users of pci_alloc_irq_vectors so far, this
incorrect pattern may spread otherwise.

Fixes: 3f3a46951e02 ("serial: 8250_lpss: set PCI master only for private DMA")
Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 drivers/tty/serial/8250/8250_lpss.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/serial/8250/8250_lpss.c b/drivers/tty/serial/8250/8250_lpss.c
index f09f68a..9315197 100644
--- a/drivers/tty/serial/8250/8250_lpss.c
+++ b/drivers/tty/serial/8250/8250_lpss.c
@@ -183,7 +183,6 @@ static void qrk_serial_setup_dma(struct lpss8250 *lpss, struct uart_port *port)
 	if (ret)
 		return;
 
-	pci_set_master(pdev);
 	pci_try_set_mwi(pdev);
 
 	/* Special DMA address for UART */
@@ -216,6 +215,8 @@ static int qrk_serial_setup(struct lpss8250 *lpss, struct uart_port *port)
 	struct pci_dev *pdev = to_pci_dev(port->dev);
 	int ret;
 
+	pci_set_master(pdev);
+
 	ret = pci_alloc_irq_vectors(pdev, 1, 1, PCI_IRQ_ALL_TYPES);
 	if (ret < 0)
 		return ret;
-- 
2.1.4

Re: [PATCH] serial: 8250_lpss: Unconditionally set PCI master for Quark

[Andy Shevchenko]

On Wed, Jan 4, 2017 at 10:48 PM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
> MSI needs it as well.
>
> Should have no practical impact, though, as DMA is always available on
> the Quark. But given the few users of pci_alloc_irq_vectors so far, this
> incorrect pattern may spread otherwise.

This looks valid, but let me review it later, need some sleep.

>
> Fixes: 3f3a46951e02 ("serial: 8250_lpss: set PCI master only for private DMA")
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
>  drivers/tty/serial/8250/8250_lpss.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/tty/serial/8250/8250_lpss.c b/drivers/tty/serial/8250/8250_lpss.c
> index f09f68a..9315197 100644
> --- a/drivers/tty/serial/8250/8250_lpss.c
> +++ b/drivers/tty/serial/8250/8250_lpss.c
> @@ -183,7 +183,6 @@ static void qrk_serial_setup_dma(struct lpss8250 *lpss, struct uart_port *port)
>         if (ret)
>                 return;
>
> -       pci_set_master(pdev);
>         pci_try_set_mwi(pdev);
>
>         /* Special DMA address for UART */
> @@ -216,6 +215,8 @@ static int qrk_serial_setup(struct lpss8250 *lpss, struct uart_port *port)
>         struct pci_dev *pdev = to_pci_dev(port->dev);
>         int ret;
>
> +       pci_set_master(pdev);
> +
>         ret = pci_alloc_irq_vectors(pdev, 1, 1, PCI_IRQ_ALL_TYPES);
>         if (ret < 0)
>                 return ret;
> --
> 2.1.4



-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] serial: 8250_lpss: Unconditionally set PCI master for Quark

[Andy Shevchenko]

On Wed, Jan 4, 2017 at 10:48 PM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
> MSI needs it as well.
>
> Should have no practical impact, though, as DMA is always available on
> the Quark. But given the few users of pci_alloc_irq_vectors so far, this
> incorrect pattern may spread otherwise.
>

One question below.

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

> Fixes: 3f3a46951e02 ("serial: 8250_lpss: set PCI master only for private DMA")
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
>  drivers/tty/serial/8250/8250_lpss.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/tty/serial/8250/8250_lpss.c b/drivers/tty/serial/8250/8250_lpss.c
> index f09f68a..9315197 100644
> --- a/drivers/tty/serial/8250/8250_lpss.c
> +++ b/drivers/tty/serial/8250/8250_lpss.c
> @@ -183,7 +183,6 @@ static void qrk_serial_setup_dma(struct lpss8250 *lpss, struct uart_port *port)
>         if (ret)
>                 return;
>
> -       pci_set_master(pdev);
>         pci_try_set_mwi(pdev);

Does it make sense to move MWI there as well?

>
>         /* Special DMA address for UART */
> @@ -216,6 +215,8 @@ static int qrk_serial_setup(struct lpss8250 *lpss, struct uart_port *port)
>         struct pci_dev *pdev = to_pci_dev(port->dev);
>         int ret;
>
> +       pci_set_master(pdev);
> +
>         ret = pci_alloc_irq_vectors(pdev, 1, 1, PCI_IRQ_ALL_TYPES);
>         if (ret < 0)
>                 return ret;
> --
> 2.1.4



-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] serial: 8250_lpss: Unconditionally set PCI master for Quark

[Andy Shevchenko]

On Thu, Jan 5, 2017 at 1:56 AM, Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Wed, Jan 4, 2017 at 10:48 PM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>> MSI needs it as well.
>>
>> Should have no practical impact, though, as DMA is always available on
>> the Quark. But given the few users of pci_alloc_irq_vectors so far, this
>> incorrect pattern may spread otherwise.
>
> This looks valid, but let me review it later, need some sleep.
>

Ah, Jan, perhaps you would like to comment on this one as well
http://marc.info/?l=linux-serial&m=148334902916347&w=2

?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] serial: 8250_lpss: Unconditionally set PCI master for Quark

[Jan Kiszka]

On 2017-01-05 22:54, Andy Shevchenko wrote:
> On Wed, Jan 4, 2017 at 10:48 PM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>> MSI needs it as well.
>>
>> Should have no practical impact, though, as DMA is always available on
>> the Quark. But given the few users of pci_alloc_irq_vectors so far, this
>> incorrect pattern may spread otherwise.
>>
> 
> One question below.
> 
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> 
>> Fixes: 3f3a46951e02 ("serial: 8250_lpss: set PCI master only for private DMA")
>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>> ---
>>  drivers/tty/serial/8250/8250_lpss.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/tty/serial/8250/8250_lpss.c b/drivers/tty/serial/8250/8250_lpss.c
>> index f09f68a..9315197 100644
>> --- a/drivers/tty/serial/8250/8250_lpss.c
>> +++ b/drivers/tty/serial/8250/8250_lpss.c
>> @@ -183,7 +183,6 @@ static void qrk_serial_setup_dma(struct lpss8250 *lpss, struct uart_port *port)
>>         if (ret)
>>                 return;
>>
>> -       pci_set_master(pdev);
>>         pci_try_set_mwi(pdev);
> 
> Does it make sense to move MWI there as well?

TBH, I didn't come across the need to enable this bit so far,
specifically not for doing MSI transactions. Is MWI used at all for MSI
(spec says that MSI is "using a PCI DWORD memory write transaction")?
That question is better answered by someone more familiar with such PCI
details.

Jan

> 
>>
>>         /* Special DMA address for UART */
>> @@ -216,6 +215,8 @@ static int qrk_serial_setup(struct lpss8250 *lpss, struct uart_port *port)
>>         struct pci_dev *pdev = to_pci_dev(port->dev);
>>         int ret;
>>
>> +       pci_set_master(pdev);
>> +
>>         ret = pci_alloc_irq_vectors(pdev, 1, 1, PCI_IRQ_ALL_TYPES);
>>         if (ret < 0)
>>                 return ret;
>> --
>> 2.1.4
> 
> 
> 

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

Re: [PATCH] serial: 8250_lpss: Unconditionally set PCI master for Quark

[Jan Kiszka]

On 2017-01-05 22:56, Andy Shevchenko wrote:
> On Thu, Jan 5, 2017 at 1:56 AM, Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
>> On Wed, Jan 4, 2017 at 10:48 PM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>>> MSI needs it as well.
>>>
>>> Should have no practical impact, though, as DMA is always available on
>>> the Quark. But given the few users of pci_alloc_irq_vectors so far, this
>>> incorrect pattern may spread otherwise.
>>
>> This looks valid, but let me review it later, need some sleep.
>>
> 
> Ah, Jan, perhaps you would like to comment on this one as well
> http://marc.info/?l=linux-serial&m=148334902916347&w=2
> 
> ?
> 

http://marc.info/?l=linux-serial&m=148334905916354&w=2 enables MSI, but
that's only for Denverton, and there it is unconditional. So this is ok.

Jan

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

Re: [PATCH] serial: 8250_lpss: Unconditionally set PCI master for Quark

[Andy Shevchenko]

On Mon, Jan 9, 2017 at 7:11 PM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
> On 2017-01-05 22:56, Andy Shevchenko wrote:
>> On Thu, Jan 5, 2017 at 1:56 AM, Andy Shevchenko
>> <andy.shevchenko@gmail.com> wrote:
>>
>> Ah, Jan, perhaps you would like to comment on this one as well
>> http://marc.info/?l=linux-serial&m=148334902916347&w=2
>> ?
>>
>
> http://marc.info/?l=linux-serial&m=148334905916354&w=2 enables MSI, but
> that's only for Denverton, and there it is unconditional. So this is ok.

I think there is potential issue, if DMA driver is not compiled or
probed (by whatever reason) we will have MSI enabled without bus
mastering.
And like you said for sake of not copying wrong pattern for new API,
would be good to move it.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] serial: 8250_lpss: Unconditionally set PCI master for Quark

[Jan Kiszka]

On 2017-01-10 00:24, Andy Shevchenko wrote:
> On Mon, Jan 9, 2017 at 7:11 PM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>> On 2017-01-05 22:56, Andy Shevchenko wrote:
>>> On Thu, Jan 5, 2017 at 1:56 AM, Andy Shevchenko
>>> <andy.shevchenko@gmail.com> wrote:
>>>
>>> Ah, Jan, perhaps you would like to comment on this one as well
>>> http://marc.info/?l=linux-serial&m=148334902916347&w=2
>>> ?
>>>
>>
>> http://marc.info/?l=linux-serial&m=148334905916354&w=2 enables MSI, but
>> that's only for Denverton, and there it is unconditional. So this is ok.
> 
> I think there is potential issue, if DMA driver is not compiled or
> probed (by whatever reason) we will have MSI enabled without bus
> mastering.
> And like you said for sake of not copying wrong pattern for new API,
> would be good to move it.

Ah, indeed: the pci_set_master is not yet done unconditionally. Move it
prior to hsu_dma_probe, and everyone should be happy.

Jan

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

Re: [PATCH] serial: 8250_lpss: Unconditionally set PCI master for Quark

[Andy Shevchenko]

On Mon, 2017-01-09 at 18:00 +0100, Jan Kiszka wrote:
> On 2017-01-05 22:54, Andy Shevchenko wrote:
> > On Wed, Jan 4, 2017 at 10:48 PM, Jan Kiszka <jan.kiszka@siemens.com>
> > wrote:
> > > MSI needs it as well.
> > > 
> > > Should have no practical impact, though, as DMA is always
> > > available on
> > > the Quark. But given the few users of pci_alloc_irq_vectors so
> > > far, this
> > > incorrect pattern may spread otherwise.
> > > 
> > 
> > One question below.
> > 
> > Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> > 
> > > Fixes: 3f3a46951e02 ("serial: 8250_lpss: set PCI master only for
> > > private DMA")
> > > Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> > > ---
> > >  drivers/tty/serial/8250/8250_lpss.c | 3 ++-
> > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/tty/serial/8250/8250_lpss.c
> > > b/drivers/tty/serial/8250/8250_lpss.c
> > > index f09f68a..9315197 100644
> > > --- a/drivers/tty/serial/8250/8250_lpss.c
> > > +++ b/drivers/tty/serial/8250/8250_lpss.c
> > > @@ -183,7 +183,6 @@ static void qrk_serial_setup_dma(struct
> > > lpss8250 *lpss, struct uart_port *port)
> > >         if (ret)
> > >                 return;
> > > 
> > > -       pci_set_master(pdev);
> > >         pci_try_set_mwi(pdev);
> > 
> > Does it make sense to move MWI there as well?
> 
> TBH, I didn't come across the need to enable this bit so far,
> specifically not for doing MSI transactions. Is MWI used at all for
> MSI
> (spec says that MSI is "using a PCI DWORD memory write transaction")?

No, it can't. PCIe has no MWI feature at all, but you may have MSI
there.

> That question is better answered by someone more familiar with such
> PCI
> details.

In most (modern) cases trying MWI is no-op.

So, I think you may send v2 of this with my tag without any changes in
the code. Thanks!

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