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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAFiDJ5_WaNwXMppkhbxVakW2UddN29ypxE9dxhnyfEw5iwvmcQ@mail.gmail.com>
Date:	Mon, 3 Aug 2015 19:00:13 +0800
From:	Ley Foon Tan <lftan@...era.com>
To:	Marc Zyngier <marc.zyngier@....com>
Cc:	Bjorn Helgaas <bhelgaas@...gle.com>,
	Russell King <linux@....linux.org.uk>,
	Arnd Bergmann <arnd@...db.de>,
	Dinh Nguyen <dinguyen@...nsource.altera.com>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	"linux-doc@...r.kernel.org" <linux-doc@...r.kernel.org>,
	"linux-pci@...r.kernel.org" <linux-pci@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	Rob Herring <robh+dt@...nel.org>,
	"linux-arm-kernel@...ts.infradead.org" 
	<linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH v2 3/5] pci: altera: Add Altera PCIe MSI driver

On Mon, Aug 3, 2015 at 6:53 PM, Marc Zyngier <marc.zyngier@....com> 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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/