On 03/08/15 11:37, Ley Foon Tan wrote:

On Fri, Jul 31, 2015 at 8:12 PM, Marc Zyngier [off-list ref] wrote:

quoted

On 31/07/15 11:15, Ley Foon Tan wrote:

quoted

This patch adds Altera PCIe MSI driver. This soft IP supports configurable
number of vectors, which is a dts parameter.

I've reviewed the initial drop of this code; basic courtesy would be to
keep me CCed on the follow-up series.

Will keep you in CC for the following revision. Sorry about this.

quoted

quoted

Signed-off-by: Ley Foon Tan <redacted>
---
 drivers/pci/host/Kconfig           |   7 +
 drivers/pci/host/Makefile          |   1 +
 drivers/pci/host/pcie-altera-msi.c | 324 +++++++++++++++++++++++++++++++++++++
 3 files changed, 332 insertions(+)
 create mode 100644 drivers/pci/host/pcie-altera-msi.c

[...]

quoted

quoted

+
+     msg->address_lo = lower_32_bits(addr);
+     msg->address_hi = upper_32_bits(addr);
+     msg->data = data->hwirq;
+
+     mask = msi_readl(msi, MSI_INTMASK);
+     mask |= 1 << data->hwirq;
+     msi_writel(msi, mask, MSI_INTMASK);

It feels a bit weird to unmask the interrupt when you compose the
message. I'd expect this to be done in the mask/unmask methods.

Do you refer to these 2 callbacks?
.irq_mask = pci_msi_mask_irq,
.irq_unmask = pci_msi_unmask_irq,

How about we move this INTMASK code above to altera_irq_domain_alloc()?
We have unmask code in altera_irq_domain_free() now.

Either you move the mask/unmask code to local irq_mask/irq_unmask
methods (and call pci_msi_(un)mask_irq from there), or you move it
entierely to alloc/free.

Some level of symmetry helps following what is going on in the code.

[...]

quoted

quoted

+static int altera_irq_domain_alloc(struct irq_domain *domain, unsigned int virq,
+                               unsigned int nr_irqs, void *args)
+{
+     struct altera_msi *msi = domain->host_data;
+     int bit;
+
+     mutex_lock(&msi->lock);
+
+     bit = find_first_zero_bit(msi->used, msi->num_of_vectors);
+     if (bit < msi->num_of_vectors)
+             set_bit(bit, msi->used);
+
+     mutex_unlock(&msi->lock);
+
+     if (bit < 0)
+             return bit;

How can "bit" be negative here? find_first_zero_bit returns an unsigned
long... You probably want to change the type of "bit" to reflect that.

Okay, will change to "unsigned long" type.

quoted

quoted

+     else if (bit >= msi->num_of_vectors)

Useless "else" anyway.

I think we still need to check for failing case, if we don't have
available unused bit.
This could be rewritten  as below:

bit = find_first_zero_bit(msi->used, msi->num_of_vectors);
if (bit < msi->num_of_vectors)
      set_bit(bit, msi->used);
else
      return -ENOSPC;

The more logical to write this is:

	if (bit >= msi->num_of_vectors)
		return -ENOSPC;

	set_bit(bit, msi->used);

which gets rid of the error case early and streamlines the normal case.

quoted

quoted

+             return -ENOSPC;
+
+     irq_domain_set_info(domain, virq, bit, &altera_msi_bottom_irq_chip,
+                         domain->host_data, handle_simple_irq,
+                         NULL, NULL);
+     set_irq_flags(virq, IRQF_VALID);
+
+     return 0;
+}

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html