Re: [PATCH v2 3/5] pci: altera: Add Altera PCIe MSI driver

[Ley Foon Tan]

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 linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/