On Mon, Aug 3, 2015 at 6:53 PM, Marc Zyngier [off-list ref] wrote:

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

quoted

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.

Okay, will move it to alloc/free.

[...]

quoted

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.

Yes, will change to this way.

Thanks.

Regards
Ley Foon