linux-rtc.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v1 1/3] rtc: cmos: Stop using shared IRQ
@ 2020-01-22 14:45 Andy Shevchenko
  2020-01-22 14:45 ` [PATCH v1 2/3] rtc: cmos: Use predefined value for RTC IRQ on legacy x86 Andy Shevchenko
                   ` (3 more replies)
  0 siblings, 4 replies; 9+ messages in thread
From: Andy Shevchenko @ 2020-01-22 14:45 UTC (permalink / raw)
  To: Alessandro Zummo, Alexandre Belloni, linux-rtc, Guilherme G. Piccoli
  Cc: Andy Shevchenko, Hans de Goede

As reported by Guilherme G. Piccoli:

--- 8< --- 8< ---

The rtc-cmos interrupt setting was changed in the commit 079062b28fb4
("rtc: cmos: prevent kernel warning on IRQ flags mismatch") in order
to allow shared interrupts; according to that commit's description,
some machine got kernel warnings due to the interrupt line being shared
between rtc-cmos and other hardware, and rtc-cmos didn't allow IRQ sharing
that time.

After the aforementioned commit though it was observed a huge increase
in lost HPET interrupts in some systems, observed through the following
kernel message:

[...] hpet1: lost 35 rtc interrupts

After investigation, it was narrowed down to the shared interrupts
usage when having the kernel option "irqpoll" enabled. In this case,
all IRQ handlers are called for non-timer interrupts, if such handlers
are setup in shared IRQ lines. The rtc-cmos IRQ handler could be set to
hpet_rtc_interrupt(), which will produce the kernel "lost interrupts"
message after doing work - lots of readl/writel to HPET registers, which
are known to be slow.

Although "irqpoll" is not a default kernel option, it's used in some contexts,
one being the kdump kernel (which is an already "impaired" kernel usually
running with 1 CPU available), so the performance burden could be considerable.
Also, the same issue would happen (in a shorter extent though) when using
"irqfixup" kernel option.

In a quick experiment, a virtual machine with uptime of 2 minutes produced
>300 calls to hpet_rtc_interrupt() when "irqpoll" was set, whereas without
sharing interrupts this number reduced to 1 interrupt. Machines with more
hardware than a VM should generate even more unnecessary HPET interrupts
in this scenario.

--- 8< --- 8< ---

After looking into the rtc-cmos driver history and DSDT table from
the Microsoft Surface 3, we may notice that Hans de Goede submitted
a correct fix (see dependency below). Thus, we simply revert
the culprit commit.

Fixes: 079062b28fb4 ("rtc: cmos: prevent kernel warning on IRQ flags mismatch")
Depends-on: a1e23a42f1bd ("rtc: cmos: Do not assume irq 8 for rtc when there are no legacy irqs")
Reported-by: Guilherme G. Piccoli <gpiccoli@canonical.com>
Cc: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/rtc/rtc-cmos.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/rtc/rtc-cmos.c b/drivers/rtc/rtc-cmos.c
index 033303708c8b..cb28bbdc9e17 100644
--- a/drivers/rtc/rtc-cmos.c
+++ b/drivers/rtc/rtc-cmos.c
@@ -850,7 +850,7 @@ cmos_do_probe(struct device *dev, struct resource *ports, int rtc_irq)
 			rtc_cmos_int_handler = cmos_interrupt;
 
 		retval = request_irq(rtc_irq, rtc_cmos_int_handler,
-				IRQF_SHARED, dev_name(&cmos_rtc.rtc->dev),
+				0, dev_name(&cmos_rtc.rtc->dev),
 				cmos_rtc.rtc);
 		if (retval < 0) {
 			dev_dbg(dev, "IRQ %d is already in use\n", rtc_irq);
-- 
2.24.1


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH v1 2/3] rtc: cmos: Use predefined value for RTC IRQ on legacy x86
  2020-01-22 14:45 [PATCH v1 1/3] rtc: cmos: Stop using shared IRQ Andy Shevchenko
@ 2020-01-22 14:45 ` Andy Shevchenko
  2020-01-22 14:45 ` [PATCH v1 3/3] rtc: cmos: Refactor code by using the new dmi_get_bios_year() helper Andy Shevchenko
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 9+ messages in thread
From: Andy Shevchenko @ 2020-01-22 14:45 UTC (permalink / raw)
  To: Alessandro Zummo, Alexandre Belloni, linux-rtc, Guilherme G. Piccoli
  Cc: Andy Shevchenko, Hans de Goede

When legacy devices are present on x86 machine, the RTC IRQ has
a dedicated pre-defined value. Use it instead of hard coded number.

Cc: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/rtc/rtc-cmos.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/rtc/rtc-cmos.c b/drivers/rtc/rtc-cmos.c
index cb28bbdc9e17..aee55b658f85 100644
--- a/drivers/rtc/rtc-cmos.c
+++ b/drivers/rtc/rtc-cmos.c
@@ -1305,7 +1305,7 @@ static int cmos_pnp_probe(struct pnp_dev *pnp, const struct pnp_device_id *id)
 		 * hardcode it on systems with a legacy PIC.
 		 */
 		if (nr_legacy_irqs())
-			irq = 8;
+			irq = RTC_IRQ;
 #endif
 		return cmos_do_probe(&pnp->dev,
 				pnp_get_resource(pnp, IORESOURCE_IO, 0), irq);
-- 
2.24.1


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH v1 3/3] rtc: cmos: Refactor code by using the new dmi_get_bios_year() helper
  2020-01-22 14:45 [PATCH v1 1/3] rtc: cmos: Stop using shared IRQ Andy Shevchenko
  2020-01-22 14:45 ` [PATCH v1 2/3] rtc: cmos: Use predefined value for RTC IRQ on legacy x86 Andy Shevchenko
@ 2020-01-22 14:45 ` Andy Shevchenko
  2020-01-22 16:57 ` [PATCH v1 1/3] rtc: cmos: Stop using shared IRQ Hans de Goede
  2020-01-22 17:32 ` Guilherme Piccoli
  3 siblings, 0 replies; 9+ messages in thread
From: Andy Shevchenko @ 2020-01-22 14:45 UTC (permalink / raw)
  To: Alessandro Zummo, Alexandre Belloni, linux-rtc, Guilherme G. Piccoli
  Cc: Andy Shevchenko, Hans de Goede

Refactor code by using the new dmi_get_bios_year() helper instead of
open coding its functionality. This also makes logic slightly clearer.

No changes intended.

Cc: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/rtc/rtc-cmos.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/rtc/rtc-cmos.c b/drivers/rtc/rtc-cmos.c
index aee55b658f85..b795fe4cbd2e 100644
--- a/drivers/rtc/rtc-cmos.c
+++ b/drivers/rtc/rtc-cmos.c
@@ -1197,8 +1197,6 @@ static void rtc_wake_off(struct device *dev)
 /* Enable use_acpi_alarm mode for Intel platforms no earlier than 2015 */
 static void use_acpi_alarm_quirks(void)
 {
-	int year;
-
 	if (boot_cpu_data.x86_vendor != X86_VENDOR_INTEL)
 		return;
 
@@ -1208,8 +1206,10 @@ static void use_acpi_alarm_quirks(void)
 	if (!is_hpet_enabled())
 		return;
 
-	if (dmi_get_date(DMI_BIOS_DATE, &year, NULL, NULL) && year >= 2015)
-		use_acpi_alarm = true;
+	if (dmi_get_bios_year() < 2015)
+		return;
+
+	use_acpi_alarm = true;
 }
 #else
 static inline void use_acpi_alarm_quirks(void) { }
-- 
2.24.1


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH v1 1/3] rtc: cmos: Stop using shared IRQ
  2020-01-22 14:45 [PATCH v1 1/3] rtc: cmos: Stop using shared IRQ Andy Shevchenko
  2020-01-22 14:45 ` [PATCH v1 2/3] rtc: cmos: Use predefined value for RTC IRQ on legacy x86 Andy Shevchenko
  2020-01-22 14:45 ` [PATCH v1 3/3] rtc: cmos: Refactor code by using the new dmi_get_bios_year() helper Andy Shevchenko
@ 2020-01-22 16:57 ` Hans de Goede
  2020-01-22 16:58   ` Hans de Goede
  2020-01-22 17:32 ` Guilherme Piccoli
  3 siblings, 1 reply; 9+ messages in thread
From: Hans de Goede @ 2020-01-22 16:57 UTC (permalink / raw)
  To: Andy Shevchenko, Alessandro Zummo, Alexandre Belloni, linux-rtc,
	Guilherme G. Piccoli

Hi,

On 22-01-2020 15:45, Andy Shevchenko wrote:
> As reported by Guilherme G. Piccoli:
> 
> --- 8< --- 8< ---
> 
> The rtc-cmos interrupt setting was changed in the commit 079062b28fb4
> ("rtc: cmos: prevent kernel warning on IRQ flags mismatch") in order
> to allow shared interrupts; according to that commit's description,
> some machine got kernel warnings due to the interrupt line being shared
> between rtc-cmos and other hardware, and rtc-cmos didn't allow IRQ sharing
> that time.
> 
> After the aforementioned commit though it was observed a huge increase
> in lost HPET interrupts in some systems, observed through the following
> kernel message:
> 
> [...] hpet1: lost 35 rtc interrupts
> 
> After investigation, it was narrowed down to the shared interrupts
> usage when having the kernel option "irqpoll" enabled. In this case,
> all IRQ handlers are called for non-timer interrupts, if such handlers
> are setup in shared IRQ lines. The rtc-cmos IRQ handler could be set to
> hpet_rtc_interrupt(), which will produce the kernel "lost interrupts"
> message after doing work - lots of readl/writel to HPET registers, which
> are known to be slow.
> 
> Although "irqpoll" is not a default kernel option, it's used in some contexts,
> one being the kdump kernel (which is an already "impaired" kernel usually
> running with 1 CPU available), so the performance burden could be considerable.
> Also, the same issue would happen (in a shorter extent though) when using
> "irqfixup" kernel option.
> 
> In a quick experiment, a virtual machine with uptime of 2 minutes produced
>> 300 calls to hpet_rtc_interrupt() when "irqpoll" was set, whereas without
> sharing interrupts this number reduced to 1 interrupt. Machines with more
> hardware than a VM should generate even more unnecessary HPET interrupts
> in this scenario.
> 
> --- 8< --- 8< ---
> 
> After looking into the rtc-cmos driver history and DSDT table from
> the Microsoft Surface 3, we may notice that Hans de Goede submitted
> a correct fix (see dependency below). Thus, we simply revert
> the culprit commit.
> 
> Fixes: 079062b28fb4 ("rtc: cmos: prevent kernel warning on IRQ flags mismatch")
> Depends-on: a1e23a42f1bd ("rtc: cmos: Do not assume irq 8 for rtc when there are no legacy irqs")
> Reported-by: Guilherme G. Piccoli <gpiccoli@canonical.com>
> Cc: Hans de Goede <hdegoede@redhat.com>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Entire series looks good to me:

Signed-off-by: Hans de Goede <hdegoede@redhat.com>

Regards,

Hans




> ---
>   drivers/rtc/rtc-cmos.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/rtc/rtc-cmos.c b/drivers/rtc/rtc-cmos.c
> index 033303708c8b..cb28bbdc9e17 100644
> --- a/drivers/rtc/rtc-cmos.c
> +++ b/drivers/rtc/rtc-cmos.c
> @@ -850,7 +850,7 @@ cmos_do_probe(struct device *dev, struct resource *ports, int rtc_irq)
>   			rtc_cmos_int_handler = cmos_interrupt;
>   
>   		retval = request_irq(rtc_irq, rtc_cmos_int_handler,
> -				IRQF_SHARED, dev_name(&cmos_rtc.rtc->dev),
> +				0, dev_name(&cmos_rtc.rtc->dev),
>   				cmos_rtc.rtc);
>   		if (retval < 0) {
>   			dev_dbg(dev, "IRQ %d is already in use\n", rtc_irq);
> 


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v1 1/3] rtc: cmos: Stop using shared IRQ
  2020-01-22 16:57 ` [PATCH v1 1/3] rtc: cmos: Stop using shared IRQ Hans de Goede
@ 2020-01-22 16:58   ` Hans de Goede
  0 siblings, 0 replies; 9+ messages in thread
From: Hans de Goede @ 2020-01-22 16:58 UTC (permalink / raw)
  To: Andy Shevchenko, Alessandro Zummo, Alexandre Belloni, linux-rtc,
	Guilherme G. Piccoli

Hi,

On 22-01-2020 17:57, Hans de Goede wrote:
> Hi,
> 
> On 22-01-2020 15:45, Andy Shevchenko wrote:
>> As reported by Guilherme G. Piccoli:
>>
>> --- 8< --- 8< ---
>>
>> The rtc-cmos interrupt setting was changed in the commit 079062b28fb4
>> ("rtc: cmos: prevent kernel warning on IRQ flags mismatch") in order
>> to allow shared interrupts; according to that commit's description,
>> some machine got kernel warnings due to the interrupt line being shared
>> between rtc-cmos and other hardware, and rtc-cmos didn't allow IRQ sharing
>> that time.
>>
>> After the aforementioned commit though it was observed a huge increase
>> in lost HPET interrupts in some systems, observed through the following
>> kernel message:
>>
>> [...] hpet1: lost 35 rtc interrupts
>>
>> After investigation, it was narrowed down to the shared interrupts
>> usage when having the kernel option "irqpoll" enabled. In this case,
>> all IRQ handlers are called for non-timer interrupts, if such handlers
>> are setup in shared IRQ lines. The rtc-cmos IRQ handler could be set to
>> hpet_rtc_interrupt(), which will produce the kernel "lost interrupts"
>> message after doing work - lots of readl/writel to HPET registers, which
>> are known to be slow.
>>
>> Although "irqpoll" is not a default kernel option, it's used in some contexts,
>> one being the kdump kernel (which is an already "impaired" kernel usually
>> running with 1 CPU available), so the performance burden could be considerable.
>> Also, the same issue would happen (in a shorter extent though) when using
>> "irqfixup" kernel option.
>>
>> In a quick experiment, a virtual machine with uptime of 2 minutes produced
>>> 300 calls to hpet_rtc_interrupt() when "irqpoll" was set, whereas without
>> sharing interrupts this number reduced to 1 interrupt. Machines with more
>> hardware than a VM should generate even more unnecessary HPET interrupts
>> in this scenario.
>>
>> --- 8< --- 8< ---
>>
>> After looking into the rtc-cmos driver history and DSDT table from
>> the Microsoft Surface 3, we may notice that Hans de Goede submitted
>> a correct fix (see dependency below). Thus, we simply revert
>> the culprit commit.
>>
>> Fixes: 079062b28fb4 ("rtc: cmos: prevent kernel warning on IRQ flags mismatch")
>> Depends-on: a1e23a42f1bd ("rtc: cmos: Do not assume irq 8 for rtc when there are no legacy irqs")
>> Reported-by: Guilherme G. Piccoli <gpiccoli@canonical.com>
>> Cc: Hans de Goede <hdegoede@redhat.com>
>> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> 
> Entire series looks good to me:
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

That should have been:

Reviewed-by: Hans de Goede <hdegoede@redhat.com>

Of course, sorry.

Regards,

Hans


>> ---
>>   drivers/rtc/rtc-cmos.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/rtc/rtc-cmos.c b/drivers/rtc/rtc-cmos.c
>> index 033303708c8b..cb28bbdc9e17 100644
>> --- a/drivers/rtc/rtc-cmos.c
>> +++ b/drivers/rtc/rtc-cmos.c
>> @@ -850,7 +850,7 @@ cmos_do_probe(struct device *dev, struct resource *ports, int rtc_irq)
>>               rtc_cmos_int_handler = cmos_interrupt;
>>           retval = request_irq(rtc_irq, rtc_cmos_int_handler,
>> -                IRQF_SHARED, dev_name(&cmos_rtc.rtc->dev),
>> +                0, dev_name(&cmos_rtc.rtc->dev),
>>                   cmos_rtc.rtc);
>>           if (retval < 0) {
>>               dev_dbg(dev, "IRQ %d is already in use\n", rtc_irq);
>>


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v1 1/3] rtc: cmos: Stop using shared IRQ
  2020-01-22 14:45 [PATCH v1 1/3] rtc: cmos: Stop using shared IRQ Andy Shevchenko
                   ` (2 preceding siblings ...)
  2020-01-22 16:57 ` [PATCH v1 1/3] rtc: cmos: Stop using shared IRQ Hans de Goede
@ 2020-01-22 17:32 ` Guilherme Piccoli
  2020-01-22 19:58   ` Andy Shevchenko
  3 siblings, 1 reply; 9+ messages in thread
From: Guilherme Piccoli @ 2020-01-22 17:32 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Alessandro Zummo, Alexandre Belloni, linux-rtc, Hans de Goede

On Wed, Jan 22, 2020 at 11:45 AM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> [...]

Thank you Andy, great and simple series! I've tested that on top of
5.5-rc7 and it's working fine with "irqpoll" enabled.
Feel free to add my:

Tested-by: Guilherme G. Piccoli <gpiccoli@canonical.com>

The only oddity here is about the scissors, I'm not sure how it is
supposed to work on git, but when I git am'ed the patch, the commit
message was only "As reported by Guilherme G. Piccoli:", everything
else was dropped.
Cheers,


Guilherme

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v1 1/3] rtc: cmos: Stop using shared IRQ
  2020-01-22 17:32 ` Guilherme Piccoli
@ 2020-01-22 19:58   ` Andy Shevchenko
  2020-01-22 20:05     ` Guilherme Piccoli
  0 siblings, 1 reply; 9+ messages in thread
From: Andy Shevchenko @ 2020-01-22 19:58 UTC (permalink / raw)
  To: Guilherme Piccoli
  Cc: Alessandro Zummo, Alexandre Belloni, linux-rtc, Hans de Goede

On Wed, Jan 22, 2020 at 02:32:01PM -0300, Guilherme Piccoli wrote:
> On Wed, Jan 22, 2020 at 11:45 AM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > [...]
> 
> Thank you Andy, great and simple series! I've tested that on top of
> 5.5-rc7 and it's working fine with "irqpoll" enabled.
> Feel free to add my:
> 
> Tested-by: Guilherme G. Piccoli <gpiccoli@canonical.com>

Thanks!

> The only oddity here is about the scissors, I'm not sure how it is
> supposed to work on git, but when I git am'ed the patch, the commit
> message was only "As reported by Guilherme G. Piccoli:", everything
> else was dropped.

I didn't read any RFC or document about '--- ' line. But seems either bug in
git-am, or I have to put something else at the beginning of those lines.

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v1 1/3] rtc: cmos: Stop using shared IRQ
  2020-01-22 19:58   ` Andy Shevchenko
@ 2020-01-22 20:05     ` Guilherme Piccoli
  2020-01-23  8:08       ` Andy Shevchenko
  0 siblings, 1 reply; 9+ messages in thread
From: Guilherme Piccoli @ 2020-01-22 20:05 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Alessandro Zummo, Alexandre Belloni, linux-rtc, Hans de Goede

On Wed, Jan 22, 2020 at 4:58 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> [...]
> > The only oddity here is about the scissors, I'm not sure how it is
> > supposed to work on git, but when I git am'ed the patch, the commit
> > message was only "As reported by Guilherme G. Piccoli:", everything
> > else was dropped.
>
> I didn't read any RFC or document about '--- ' line. But seems either bug in
> git-am, or I have to put something else at the beginning of those lines.
>

Interesting! What I usually do is to add comments
to-be-disregarded-on-merge right below "---", before the diff starts.
Git will discard this full block. I guess probably having the "---" on
top makes Git discard everything below it until the diff.
Anyway, hope your commit message don't get messed in the merge heh

Cheers,


Guilherme

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v1 1/3] rtc: cmos: Stop using shared IRQ
  2020-01-22 20:05     ` Guilherme Piccoli
@ 2020-01-23  8:08       ` Andy Shevchenko
  0 siblings, 0 replies; 9+ messages in thread
From: Andy Shevchenko @ 2020-01-23  8:08 UTC (permalink / raw)
  To: Guilherme Piccoli
  Cc: Alessandro Zummo, Alexandre Belloni, linux-rtc, Hans de Goede

On Wed, Jan 22, 2020 at 05:05:15PM -0300, Guilherme Piccoli wrote:
> On Wed, Jan 22, 2020 at 4:58 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > [...]
> > > The only oddity here is about the scissors, I'm not sure how it is
> > > supposed to work on git, but when I git am'ed the patch, the commit
> > > message was only "As reported by Guilherme G. Piccoli:", everything
> > > else was dropped.
> >
> > I didn't read any RFC or document about '--- ' line. But seems either bug in
> > git-am, or I have to put something else at the beginning of those lines.
> >
> 
> Interesting! What I usually do is to add comments
> to-be-disregarded-on-merge right below "---", before the diff starts.
> Git will discard this full block. I guess probably having the "---" on
> top makes Git discard everything below it until the diff.

For the record it's '--- ' (mind the whitespace at the end).

> Anyway, hope your commit message don't get messed in the merge heh

I'll send a new version.

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2020-01-23  8:08 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-01-22 14:45 [PATCH v1 1/3] rtc: cmos: Stop using shared IRQ Andy Shevchenko
2020-01-22 14:45 ` [PATCH v1 2/3] rtc: cmos: Use predefined value for RTC IRQ on legacy x86 Andy Shevchenko
2020-01-22 14:45 ` [PATCH v1 3/3] rtc: cmos: Refactor code by using the new dmi_get_bios_year() helper Andy Shevchenko
2020-01-22 16:57 ` [PATCH v1 1/3] rtc: cmos: Stop using shared IRQ Hans de Goede
2020-01-22 16:58   ` Hans de Goede
2020-01-22 17:32 ` Guilherme Piccoli
2020-01-22 19:58   ` Andy Shevchenko
2020-01-22 20:05     ` Guilherme Piccoli
2020-01-23  8:08       ` Andy Shevchenko

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).