Re: BUG: INTx is assered unexpectly when unload AHCI driver with MSIx support.

Re: BUG: INTx is assered unexpectly when unload AHCI driver with MSIx support.

[tj]

Hello,

(Can you please flow the text a bit below 80 column when you reply?)

On Wed, Jul 06, 2016 at 02:51:47AM +0000, Pang Raymond wrote:
> Why does the phenomenon appear?
> 	Two factors cause this problem.
> 		1. In MSIx's irq handler
> 		   (ahci_multi_irqs_intr_hard()), we do not clear
> 		   GHC.IS register which presents which ports need
> 		   interrupt service.  static irqreturn_t
> 		   ahci_multi_irqs_intr_hard(int irq, void
> 		   *dev_instance)
> 			{
> 				//  omitting unconcerned codes here
> 				// ...				
> 				// here we just clear PxIS register, not clear GHC.IS
> 				status = readl(port_mmio + PORT_IRQ_STAT);
> 				writel(status, port_mmio + PORT_IRQ_STAT);
> 				// ...
> 			}
> 		2. In PCI subsystem, after Disable MSIx, INTx is
>		   enabled automatically.
> 			void pci_msix_shutdown(struct pci_dev *dev)
> 			{
> 				// omitting unconcerned codes here
> 				// ...				
> 				// after Disable MSIx, enable INTx imediately
> 				pci_msix_clear_and_set_ctrl(dev, PCI_MSIX_FLAGS_ENABLE, 0);
> 				pci_intx_for_msi(dev, 1);
> 				dev->msix_enabled = 0;
> 				pcibios_alloc_irq(dev);
> 			}
>
> When Kernel enables INTx(I mean clearing PCI configure space
> Rx04.bit10), controller will find GHC.IS is not Zero (because driver
> haven't cleared GHC.IS after servicing MSIx interrupt), so it
> believes some SATA ports need interrupt servicing and asserts INTx
> interrupt.

Looks like you debugged the problem.  Care to write up and test a
patch?

Thanks a lot!

-- 
tejun

Re: 答复: BUG: INTx is assered unexpectly when unload AHCI driver with MSIx support.

[tj]

Hello,

On Mon, Jul 11, 2016 at 05:16:00AM +0000, Pang Raymond wrote:
> static irqreturn_t  ahci_multi_irqs_intr_hard(int irq,
> void *dev_instance)
> {
>       //  omitting unconcerned codes here
>       //  ...
>            status = readl(port_mmio + PORT_IRQ_STAT);
>            writel(status, port_mmio + PORT_IRQ_STAT);
> 
>       // add patch code here.
> +         writel(1 << ap->port_no, ap->host->iomap + HOST_IRQ_STAT);
> 
>       // ...

I think it'd be better to avoid adding stuff to the hot path.  This
only matters when the device is shut down, right?  Can't it just be
cleared in the driver cleanup path?

Thanks.

-- 
tejun

Re: BUG: INTx is assered unexpectly when unload AHCI driver with MSIx support.

[tj]

Hello, Pang.

On Fri, Jul 15, 2016 at 12:39:59PM +0000, Pang Raymond wrote:
> Hi Tejun,
> 
> 
> Yes! It only happens when the device is shutdown.
> 
> I think you're right. It's more wiser to add clearing operation to driver
> 
> clean up path.
> 
> So we can add it to ahci_port_stop()
> 
> 
> libahci.c is got from Kernel 4.6.3 stable
> 
> ============================================================
> 
> --- drivers/ata/libahci.c.old 2016-07-15 13:33:47.489620405 +0800
> +++ drivers/ata/libahci.c 2016-07-15 14:01:33.081574586 +0800
> @@ -2392,12 +2392,18 @@
> static void ahci_port_stop(struct ata_port *ap)
> {
>   const char *emsg = NULL;
> + struct ahci_host_priv *hpriv = ap->host->private_data;
> + void __iomem *host_mmio = hpriv->mmio;
>   int rc;
> 
>   /* de-initialize port */
>   rc = ahci_deinit_port(ap, &emsg);
>   if (rc)
>    ata_port_warn(ap, "%s (%d)\n", emsg, rc);
> +
> + /* Clear GHC.IS in case of asserting INTx after disable MSIx and re-enable INTx */
> + writel(1 << ap->port_no, host_mmio + HOST_IRQ_STAT);
> +
> }

Yeah, this looks good to me.  Can you please format the patch
properly, add description and Signed-off-by?

  https://www.kernel.org/doc/Documentation/SubmittingPatches

Thanks.

-- 
tejun