Re: [PATCH v2 02/10] ata: ahci: Support per-port interrupts

[Miquel Raynal <miquel.raynal@bootlin.com>]

Hi Hans,

Hans de Goede <hdegoede@redhat.com> wrote on Wed, 6 Mar 2019 16:01:16
+0100:

> >   /**
> >    * ahci_platform_enable_clks - Enable platform clocks
> >    * @hpriv: host private area to store config values
> > @@ -385,6 +394,7 @@ static int ahci_platform_get_regulator(struct ahci_host_priv *hpriv, u32 port,
> >    *    or for non devicetree enabled platforms a single clock
> >    * 4) resets, if flags has AHCI_PLATFORM_GET_RESETS (optional)
> >    * 5) phys (optional)
> > + * 6) interrupt(s)
> >    *
> >    * RETURNS:
> >    * The allocated ahci_host_priv on success, otherwise an ERR_PTR value
> > @@ -396,7 +406,7 @@ struct ahci_host_priv *ahci_platform_get_resources(struct platform_device *pdev,
> >   	struct ahci_host_priv *hpriv;
> >   	struct clk *clk;
> >   	struct device_node *child;
> > -	int i, enabled_ports = 0, rc = -ENOMEM, child_nodes;
> > +	int i, enabled_ports = 0, rc = -ENOMEM, child_nodes, ctrl_irq;
> >   	u32 mask_port_map = 0;  
> >   >   	if (!devres_open_group(dev, NULL, GFP_KERNEL))  
> > @@ -489,10 +499,30 @@ struct ahci_host_priv *ahci_platform_get_resources(struct platform_device *pdev,
> >   		goto err_out;
> >   	}  
> >   > +	hpriv->irqs = devm_kzalloc(dev, sizeof(*hpriv->irqs) * hpriv->nports,  
> > +				   GFP_KERNEL);
> > +	if (!hpriv->irqs) {
> > +		rc = -ENOMEM;
> > +		goto err_out;
> > +	}
> > +
> > +	ctrl_irq = platform_get_irq(pdev, 0);
> > +	if (ctrl_irq < 0) {
> > +		if (ctrl_irq == -EPROBE_DEFER) {
> > +			rc = ctrl_irq;
> > +			goto err_out;
> > +		}
> > +		ctrl_irq = 0;
> > +	}
> > +
> > +	if (ctrl_irq > 0)
> > +		hpriv->irqs[0] = ctrl_irq;
> > +
> >   	if (child_nodes) {
> >   		for_each_child_of_node(dev->of_node, child) {
> >   			u32 port;
> >   			struct platform_device *port_dev __maybe_unused;
> > +			int port_irq;  
> >   >   			if (!of_device_is_available(child))  
> >   				continue;
> > @@ -521,6 +551,18 @@ struct ahci_host_priv *ahci_platform_get_resources(struct platform_device *pdev,
> >   			}
> >   #endif  
> >   > +			if (!ctrl_irq) {  
> > +				port_irq = of_irq_get(child, 0);
> > +				if (!port_irq)
> > +					port_irq = -EINVAL;
> > +				if (port_irq < 0) {
> > +					rc = port_irq;
> > +					goto err_out;
> > +				}
> > +
> > +				hpriv->irqs[port] = port_irq;
> > +			}
> > +
> >   			rc = ahci_platform_get_phy(hpriv, port, dev, child);
> >   			if (rc)
> >   				goto err_out;
> > @@ -548,6 +590,18 @@ struct ahci_host_priv *ahci_platform_get_resources(struct platform_device *pdev,
> >   		if (rc == -EPROBE_DEFER)
> >   			goto err_out;
> >   	}
> > +
> > +	if (!ctrl_irq && !enabled_ports) {
> > +		dev_err(&pdev->dev, "No IRQ defined\n");
> > +		rc = -ENODEV;
> > +		goto err_out;
> > +	}
> > +
> > +	if (enabled_ports > 1) {
> > +		hpriv->flags |= AHCI_HFLAG_MULTI_MSI;
> > +		hpriv->get_irq_vector = ahci_get_per_port_irq_vector;
> > +	}
> > +  
> 
> I believe that the "if (enabled_ports > 1)" here needs to be:
> 
> 	if (!ctrl_irq && enabled_ports > 1) {
> 
> Otherwise existing boards which use a single irq defined at the
> main of_node level for the device and have defined more then 1
> child-node in their DTB will now get AHCI_HFLAG_MULTI_MSI set,
> but they will only have hpriv->irqs[0] set to the ctrl_irq
> leading to returning of 0 as irq from ahci_get_per_port_irq_vector
> for the other ports, which is wrong.
> 
> Otherwise this patch looks good to me, so with this fixed
> (assuming you agree) you may add my:
> 
> Reviewed-by: Hans de Goede <hdegoede@redhat.com>
> 
> to the next version of this patch.

You are completely right, this is a mistake, I will fix it in v3. I
will just wait for the review of the other ahci_mvebu patches.


Thanks,
Miquèl

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel