Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1299346 > unrolled thread

Re: [PATCH V9 1/2] ACPI, PCI, irq: remove interrupt count restriction

Started byAndy Shevchenko <andy.shevchenko@gmail.com>
First post2015-12-30 14:30 +0100
Last post2016-01-04 14:40 +0100
Articles 7 — 3 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH V9 1/2] ACPI, PCI, irq: remove interrupt count restriction Andy Shevchenko <andy.shevchenko@gmail.com> - 2015-12-30 14:30 +0100
    Re: [PATCH V9 1/2] ACPI, PCI, irq: remove interrupt count restriction Sinan Kaya <okaya@codeaurora.org> - 2015-12-30 20:20 +0100
      Re: [PATCH V9 1/2] ACPI, PCI, irq: remove interrupt count restriction Andy Shevchenko <andy.shevchenko@gmail.com> - 2015-12-30 21:00 +0100
        Re: [PATCH V9 1/2] ACPI, PCI, irq: remove interrupt count restriction "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2016-01-01 01:20 +0100
          Re: [PATCH V9 1/2] ACPI, PCI, irq: remove interrupt count restriction Andy Shevchenko <andy.shevchenko@gmail.com> - 2016-01-04 10:10 +0100
            Re: [PATCH V9 1/2] ACPI, PCI, irq: remove interrupt count restriction "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2016-01-04 14:30 +0100
              Re: [PATCH V9 1/2] ACPI, PCI, irq: remove interrupt count restriction Andy Shevchenko <andy.shevchenko@gmail.com> - 2016-01-04 14:40 +0100

#1299346 — Re: [PATCH V9 1/2] ACPI, PCI, irq: remove interrupt count restriction

FromAndy Shevchenko <andy.shevchenko@gmail.com>
Date2015-12-30 14:30 +0100
SubjectRe: [PATCH V9 1/2] ACPI, PCI, irq: remove interrupt count restriction
Message-ID<qLoZb-79S-5@gated-at.bofh.it>
On Wed, Dec 30, 2015 at 3:23 PM, Sinan Kaya <okaya@codeaurora.org> wrote:
> On 12/9/2015 12:14 PM, Christopher Covington wrote:
>>> On 12/9/2015 11:59 AM, Andy Shevchenko wrote:

>>>>> +       if (trigger != ACPI_MADT_TRIGGER_LEVEL ||
>>>>>> +           polarity != ACPI_MADT_POLARITY_ACTIVE_LOW)
>>>>>> +               penalty = PIRQ_PENALTY_ISA_ALWAYS;
>>>>>> +       else
>>>>>> +               penalty = PIRQ_PENALTY_PCI_USING;
>>>>>> +
>>>>>> +       acpi_irq_add_penalty(irq, penalty);

>>>> Why not to change in place? I think a common sense rule is not to
>>>> change something existing if it doesn't add any significant value.

>>>> -               acpi_irq_penalty[irq] += PIRQ_PENALTY_PCI_USING;
>>>> +              acpi_irq_add_penalty(irq, PIRQ_PENALTY_PCI_USING);
>>
>> I think Andy was suggesting that you make the change without introducing
>> the penalty variable.

> Is Chris' interpretation correct?

Yep, I meant not to use an additional variable.

> BTW, I suggest you spend some time around checkpatch for contributions. I could
> have caught most of the issues you are generally concerned before submitting a patch.

Is it a question?

-- 
With Best Regards,
Andy Shevchenko
--
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/

[toc] | [next] | [standalone]

#1299462

FromSinan Kaya <okaya@codeaurora.org>
Date2015-12-30 20:20 +0100
Message-ID<qLurT-2du-1@gated-at.bofh.it>
In reply to#1299346
On 12/30/2015 8:28 AM, Andy Shevchenko wrote:
> Yep, I meant not to use an additional variable.
> 
>> > BTW, I suggest you spend some time around checkpatch for contributions. I could
>> > have caught most of the issues you are generally concerned before submitting a patch.
> Is it a question?

It is a request not a question. I hate wasting your time and my time with things that I could
have fixed before submitting a patch.

I ran the checkpatch and it said I'm good to go. But, obviously I'm not. 

-- 
Sinan Kaya
Qualcomm Technologies, Inc. on behalf of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project
--
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/

[toc] | [prev] | [next] | [standalone]

#1299478

FromAndy Shevchenko <andy.shevchenko@gmail.com>
Date2015-12-30 21:00 +0100
Message-ID<qLv4C-2sX-19@gated-at.bofh.it>
In reply to#1299462
On Wed, Dec 30, 2015 at 9:17 PM, Sinan Kaya <okaya@codeaurora.org> wrote:
> On 12/30/2015 8:28 AM, Andy Shevchenko wrote:
>> Yep, I meant not to use an additional variable.
>>
>>> > BTW, I suggest you spend some time around checkpatch for contributions. I could
>>> > have caught most of the issues you are generally concerned before submitting a patch.
>> Is it a question?
>
> It is a request not a question. I hate wasting your time and my time with things that I could
> have fixed before submitting a patch.
>
> I ran the checkpatch and it said I'm good to go. But, obviously I'm not.

Hmm… checkpatch.pl is just a small helper to fix style issues. Here is
just a common sense rule, or kind of Occam's razor: no need to have
more variables then needed if it doesn't improve something really
significantly.

-- 
With Best Regards,
Andy Shevchenko
--
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/

[toc] | [prev] | [next] | [standalone]

#1299808

From"Rafael J. Wysocki" <rjw@rjwysocki.net>
Date2016-01-01 01:20 +0100
Message-ID<qLVBL-2We-1@gated-at.bofh.it>
In reply to#1299478
On Wednesday, December 30, 2015 09:55:35 PM Andy Shevchenko wrote:
> On Wed, Dec 30, 2015 at 9:17 PM, Sinan Kaya <okaya@codeaurora.org> wrote:
> > On 12/30/2015 8:28 AM, Andy Shevchenko wrote:
> >> Yep, I meant not to use an additional variable.
> >>
> >>> > BTW, I suggest you spend some time around checkpatch for contributions. I could
> >>> > have caught most of the issues you are generally concerned before submitting a patch.
> >> Is it a question?
> >
> > It is a request not a question. I hate wasting your time and my time with things that I could
> > have fixed before submitting a patch.
> >
> > I ran the checkpatch and it said I'm good to go. But, obviously I'm not.
> 
> Hmm… checkpatch.pl is just a small helper to fix style issues. Here is
> just a common sense rule, or kind of Occam's razor: no need to have
> more variables then needed if it doesn't improve something really
> significantly.

That said, compilers optimize things anyway, so using an extra local variable
shouldn't matter for the resulting machine code.

Thanks,
Rafael

--
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/

[toc] | [prev] | [next] | [standalone]

#1300563

FromAndy Shevchenko <andy.shevchenko@gmail.com>
Date2016-01-04 10:10 +0100
Message-ID<qN9jk-1sI-13@gated-at.bofh.it>
In reply to#1299808
On Fri, Jan 1, 2016 at 2:48 AM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> On Wednesday, December 30, 2015 09:55:35 PM Andy Shevchenko wrote:
>> On Wed, Dec 30, 2015 at 9:17 PM, Sinan Kaya <okaya@codeaurora.org> wrote:
>> > On 12/30/2015 8:28 AM, Andy Shevchenko wrote:
>> >> Yep, I meant not to use an additional variable.
>> >>
>> >>> > BTW, I suggest you spend some time around checkpatch for contributions. I could
>> >>> > have caught most of the issues you are generally concerned before submitting a patch.
>> >> Is it a question?
>> >
>> > It is a request not a question. I hate wasting your time and my time with things that I could
>> > have fixed before submitting a patch.
>> >
>> > I ran the checkpatch and it said I'm good to go. But, obviously I'm not.
>>
>> Hmm… checkpatch.pl is just a small helper to fix style issues. Here is
>> just a common sense rule, or kind of Occam's razor: no need to have
>> more variables then needed if it doesn't improve something really
>> significantly.
>
> That said, compilers optimize things anyway, so using an extra local variable
> shouldn't matter for the resulting machine code.

I'm not totally against that, but is the additional variable helpful here?


-- 
With Best Regards,
Andy Shevchenko
--
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/

[toc] | [prev] | [next] | [standalone]

#1300748

From"Rafael J. Wysocki" <rjw@rjwysocki.net>
Date2016-01-04 14:30 +0100
Message-ID<qNdmW-40K-17@gated-at.bofh.it>
In reply to#1300563
On Monday, January 04, 2016 11:01:05 AM Andy Shevchenko wrote:
> On Fri, Jan 1, 2016 at 2:48 AM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > On Wednesday, December 30, 2015 09:55:35 PM Andy Shevchenko wrote:
> >> On Wed, Dec 30, 2015 at 9:17 PM, Sinan Kaya <okaya@codeaurora.org> wrote:
> >> > On 12/30/2015 8:28 AM, Andy Shevchenko wrote:
> >> >> Yep, I meant not to use an additional variable.
> >> >>
> >> >>> > BTW, I suggest you spend some time around checkpatch for contributions. I could
> >> >>> > have caught most of the issues you are generally concerned before submitting a patch.
> >> >> Is it a question?
> >> >
> >> > It is a request not a question. I hate wasting your time and my time with things that I could
> >> > have fixed before submitting a patch.
> >> >
> >> > I ran the checkpatch and it said I'm good to go. But, obviously I'm not.
> >>
> >> Hmm… checkpatch.pl is just a small helper to fix style issues. Here is
> >> just a common sense rule, or kind of Occam's razor: no need to have
> >> more variables then needed if it doesn't improve something really
> >> significantly.
> >
> > That said, compilers optimize things anyway, so using an extra local variable
> > shouldn't matter for the resulting machine code.
> 
> I'm not totally against that, but is the additional variable helpful here?

Well, I guess you can argue both ways.

Surely, the same result can be achieved with fewer lines of code if that's
what you mean, so what about the following change on top of the $subject patch?

---
From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Subject: [PATCH] ACPI / PCI: Simplify acpi_penalize_isa_irq()

acpi_penalize_isa_irq() can be written in fewer lines of code,
so do that.  No functional change.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/acpi/pci_link.c |   14 +++-----------
 1 file changed, 3 insertions(+), 11 deletions(-)

Index: linux-pm/drivers/acpi/pci_link.c
===================================================================
--- linux-pm.orig/drivers/acpi/pci_link.c
+++ linux-pm/drivers/acpi/pci_link.c
@@ -877,17 +877,9 @@ static int __init acpi_irq_penalty_updat
  */
 void acpi_penalize_isa_irq(int irq, int active)
 {
-	int penalty;
-
-	if (irq < 0)
-		return;
-
-	if (active)
-		penalty = PIRQ_PENALTY_ISA_USED;
-	else
-		penalty = PIRQ_PENALTY_PCI_USING;
-
-	acpi_irq_add_penalty(irq, penalty);
+	if (irq >= 0)
+		acpi_irq_add_penalty(irq, active ?
+			PIRQ_PENALTY_ISA_USED : PIRQ_PENALTY_PCI_USING);
 }
 
 bool acpi_isa_irq_available(int irq)

--
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/

[toc] | [prev] | [next] | [standalone]

#1300751

FromAndy Shevchenko <andy.shevchenko@gmail.com>
Date2016-01-04 14:40 +0100
Message-ID<qNdwC-45f-25@gated-at.bofh.it>
In reply to#1300748
On Mon, Jan 4, 2016 at 3:50 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> On Monday, January 04, 2016 11:01:05 AM Andy Shevchenko wrote:
>> On Fri, Jan 1, 2016 at 2:48 AM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>> > On Wednesday, December 30, 2015 09:55:35 PM Andy Shevchenko wrote:
>> >> On Wed, Dec 30, 2015 at 9:17 PM, Sinan Kaya <okaya@codeaurora.org> wrote:
>> >> > On 12/30/2015 8:28 AM, Andy Shevchenko wrote:
>> >> >> Yep, I meant not to use an additional variable.
>> >> >>
>> >> >>> > BTW, I suggest you spend some time around checkpatch for contributions. I could
>> >> >>> > have caught most of the issues you are generally concerned before submitting a patch.
>> >> >> Is it a question?
>> >> >
>> >> > It is a request not a question. I hate wasting your time and my time with things that I could
>> >> > have fixed before submitting a patch.
>> >> >
>> >> > I ran the checkpatch and it said I'm good to go. But, obviously I'm not.
>> >>
>> >> Hmm… checkpatch.pl is just a small helper to fix style issues. Here is
>> >> just a common sense rule, or kind of Occam's razor: no need to have
>> >> more variables then needed if it doesn't improve something really
>> >> significantly.
>> >
>> > That said, compilers optimize things anyway, so using an extra local variable
>> > shouldn't matter for the resulting machine code.
>>
>> I'm not totally against that, but is the additional variable helpful here?
>
> Well, I guess you can argue both ways.
>
> Surely, the same result can be achieved with fewer lines of code if that's
> what you mean, so what about the following change on top of the $subject patch?
>
> ---
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Subject: [PATCH] ACPI / PCI: Simplify acpi_penalize_isa_irq()
>
> acpi_penalize_isa_irq() can be written in fewer lines of code,
> so do that.  No functional change.
>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>  drivers/acpi/pci_link.c |   14 +++-----------
>  1 file changed, 3 insertions(+), 11 deletions(-)
>
> Index: linux-pm/drivers/acpi/pci_link.c
> ===================================================================
> --- linux-pm.orig/drivers/acpi/pci_link.c
> +++ linux-pm/drivers/acpi/pci_link.c
> @@ -877,17 +877,9 @@ static int __init acpi_irq_penalty_updat
>   */
>  void acpi_penalize_isa_irq(int irq, int active)
>  {
> -       int penalty;
> -
> -       if (irq < 0)
> -               return;
> -
> -       if (active)
> -               penalty = PIRQ_PENALTY_ISA_USED;
> -       else
> -               penalty = PIRQ_PENALTY_PCI_USING;
> -
> -       acpi_irq_add_penalty(irq, penalty);
> +       if (irq >= 0)
> +               acpi_irq_add_penalty(irq, active ?
> +                       PIRQ_PENALTY_ISA_USED : PIRQ_PENALTY_PCI_USING);

Works for me as well!

>  }
>
>  bool acpi_isa_irq_available(int irq)
>



-- 
With Best Regards,
Andy Shevchenko
--
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/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web