On Mon, Aug 3, 2015 at 6:53 PM, Marc Zyngier <marc.zyngier@xxxxxxx> wrote:
> On 03/08/15 11:37, Ley Foon Tan wrote:
>>>> + 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.
>
> [...]
>
>>>> +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.
>>>
>>>> + 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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
