Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752799AbbHCLAS (ORCPT ); Mon, 3 Aug 2015 07:00:18 -0400 Received: from mail-lb0-f178.google.com ([209.85.217.178]:32929 "EHLO mail-lb0-f178.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751770AbbHCLAQ (ORCPT ); Mon, 3 Aug 2015 07:00:16 -0400 MIME-Version: 1.0 In-Reply-To: <55BF4830.9050104@arm.com> References: <1438337715-22594-1-git-send-email-lftan@altera.com> <1438337715-22594-4-git-send-email-lftan@altera.com> <55BB6621.9060505@arm.com> <55BF4830.9050104@arm.com> Date: Mon, 3 Aug 2015 19:00:13 +0800 X-Google-Sender-Auth: rafIrDS4JG8yIhbiQAoJtDPEfMY Message-ID: Subject: Re: [PATCH v2 3/5] pci: altera: Add Altera PCIe MSI driver From: Ley Foon Tan To: Marc Zyngier Cc: Bjorn Helgaas , Russell King , Arnd Bergmann , Dinh Nguyen , "devicetree@vger.kernel.org" , "linux-doc@vger.kernel.org" , "linux-pci@vger.kernel.org" , "linux-kernel@vger.kernel.org" , Rob Herring , "linux-arm-kernel@lists.infradead.org" Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2729 Lines: 83 On Mon, Aug 3, 2015 at 6:53 PM, Marc Zyngier 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/