* Re: 'flags' in rsparser.c change from 'register disabled resources' patch
2011-08-22 0:59 'flags' in rsparser.c change from 'register disabled resources' patch Dr. David Alan Gilbert
@ 2011-06-09 18:58 ` Witold Szczeponik
2011-06-09 19:03 ` [PATCH] PNPACPI: Simplify disabled resource registration Witold Szczeponik
0 siblings, 1 reply; 4+ messages in thread
From: Witold Szczeponik @ 2011-06-09 18:58 UTC (permalink / raw)
To: Dr. David Alan Gilbert; +Cc: linux-kernel, linux-acpi
Hi David,
your observation is correct. In fact, the patch can be simplified by
not setting the IORESOURCE_DISABLED flag at all, which is functionally
equivalent to the current state where the compiler removes this value
because it cannot be assigned to a char. A quick compare of the
generated code of the current version (with IORESOURCE_DISABLED
included) and with IORESOURCE_DISABLED not used at all yields pretty
much the same code.
I realized the problem only days after I submitted my patch.
Apparently, a follow-up patch which takes care of the
IORESOURCE_DISABLED flags did not make it into the mainline kernel.
I'll resend it as a response to this post shortly. In addition, in
https://lkml.org/lkml/2011/7/31/50 I submitted a much simpler version of
the patch which better exposes the change it introduces but requires
29df8d8f8702f0f53c1375015f09f04bc8d023c1 to be reverted.
--- Witold
^ permalink raw reply [flat|nested] 4+ messages in thread
* [PATCH] PNPACPI: Simplify disabled resource registration
2011-06-09 18:58 ` Witold Szczeponik
@ 2011-06-09 19:03 ` Witold Szczeponik
2011-09-13 16:25 ` Bjorn Helgaas
0 siblings, 1 reply; 4+ messages in thread
From: Witold Szczeponik @ 2011-06-09 19:03 UTC (permalink / raw)
To: Dr. David Alan Gilbert; +Cc: linux-kernel, linux-acpi
The attached patch simplifies 29df8d8f8702f0f53c1375015f09f04bc8d023c1. As
the "pnp_xxx" structs are not designed to cope with IORESOURCE_DISABLED, and
hence no code can test for this value, setting this value is actually a "no op"
and can be skipped altogether. It is sufficient to remove the checks for
"empty" resources and continue processing.
Signed-off-by: Witold Szczeponik <Witold.Szczeponik@gmx.net>
diff -u linux/drivers/pnp/pnpacpi/rsparser.c linux/drivers/pnp/pnpacpi/rsparser.c
--- linux/drivers/pnp/pnpacpi/rsparser.c
+++ linux/drivers/pnp/pnpacpi/rsparser.c
@@ -509,15 +509,12 @@
struct acpi_resource_dma *p)
{
int i;
- unsigned char map = 0, flags = 0;
-
- if (p->channel_count == 0)
- flags |= IORESOURCE_DISABLED;
+ unsigned char map = 0, flags;
for (i = 0; i < p->channel_count; i++)
map |= 1 << p->channels[i];
- flags |= dma_flags(dev, p->type, p->bus_master, p->transfer);
+ flags = dma_flags(dev, p->type, p->bus_master, p->transfer);
pnp_register_dma_resource(dev, option_flags, map, flags);
}
@@ -527,17 +524,14 @@
{
int i;
pnp_irq_mask_t map;
- unsigned char flags = 0;
-
- if (p->interrupt_count == 0)
- flags |= IORESOURCE_DISABLED;
+ unsigned char flags;
bitmap_zero(map.bits, PNP_IRQ_NR);
for (i = 0; i < p->interrupt_count; i++)
if (p->interrupts[i])
__set_bit(p->interrupts[i], map.bits);
- flags |= irq_flags(p->triggering, p->polarity, p->sharable);
+ flags = irq_flags(p->triggering, p->polarity, p->sharable);
pnp_register_irq_resource(dev, option_flags, &map, flags);
}
@@ -547,10 +541,7 @@
{
int i;
pnp_irq_mask_t map;
- unsigned char flags = 0;
-
- if (p->interrupt_count == 0)
- flags |= IORESOURCE_DISABLED;
+ unsigned char flags;
bitmap_zero(map.bits, PNP_IRQ_NR);
for (i = 0; i < p->interrupt_count; i++) {
@@ -564,7 +555,7 @@
}
}
- flags |= irq_flags(p->triggering, p->polarity, p->sharable);
+ flags = irq_flags(p->triggering, p->polarity, p->sharable);
pnp_register_irq_resource(dev, option_flags, &map, flags);
}
@@ -574,11 +565,8 @@
{
unsigned char flags = 0;
- if (io->address_length == 0)
- flags |= IORESOURCE_DISABLED;
-
if (io->io_decode == ACPI_DECODE_16)
- flags |= IORESOURCE_IO_16BIT_ADDR;
+ flags = IORESOURCE_IO_16BIT_ADDR;
pnp_register_port_resource(dev, option_flags, io->minimum, io->maximum,
io->alignment, io->address_length, flags);
}
@@ -587,13 +575,8 @@
unsigned int option_flags,
struct acpi_resource_fixed_io *io)
{
- unsigned char flags = 0;
-
- if (io->address_length == 0)
- flags |= IORESOURCE_DISABLED;
-
pnp_register_port_resource(dev, option_flags, io->address, io->address,
- 0, io->address_length, flags | IORESOURCE_IO_FIXED);
+ 0, io->address_length, IORESOURCE_IO_FIXED);
}
static __init void pnpacpi_parse_mem24_option(struct pnp_dev *dev,
@@ -602,11 +585,8 @@
{
unsigned char flags = 0;
- if (p->address_length == 0)
- flags |= IORESOURCE_DISABLED;
-
if (p->write_protect == ACPI_READ_WRITE_MEMORY)
- flags |= IORESOURCE_MEM_WRITEABLE;
+ flags = IORESOURCE_MEM_WRITEABLE;
pnp_register_mem_resource(dev, option_flags, p->minimum, p->maximum,
p->alignment, p->address_length, flags);
}
@@ -617,11 +597,8 @@
{
unsigned char flags = 0;
- if (p->address_length == 0)
- flags |= IORESOURCE_DISABLED;
-
if (p->write_protect == ACPI_READ_WRITE_MEMORY)
- flags |= IORESOURCE_MEM_WRITEABLE;
+ flags = IORESOURCE_MEM_WRITEABLE;
pnp_register_mem_resource(dev, option_flags, p->minimum, p->maximum,
p->alignment, p->address_length, flags);
}
@@ -632,11 +609,8 @@
{
unsigned char flags = 0;
- if (p->address_length == 0)
- flags |= IORESOURCE_DISABLED;
-
if (p->write_protect == ACPI_READ_WRITE_MEMORY)
- flags |= IORESOURCE_MEM_WRITEABLE;
+ flags = IORESOURCE_MEM_WRITEABLE;
pnp_register_mem_resource(dev, option_flags, p->address, p->address,
0, p->address_length, flags);
}
@@ -656,19 +630,16 @@
return;
}
- if (p->address_length == 0)
- flags |= IORESOURCE_DISABLED;
-
if (p->resource_type == ACPI_MEMORY_RANGE) {
if (p->info.mem.write_protect == ACPI_READ_WRITE_MEMORY)
- flags |= IORESOURCE_MEM_WRITEABLE;
+ flags = IORESOURCE_MEM_WRITEABLE;
pnp_register_mem_resource(dev, option_flags, p->minimum,
p->minimum, 0, p->address_length,
flags);
} else if (p->resource_type == ACPI_IO_RANGE)
pnp_register_port_resource(dev, option_flags, p->minimum,
p->minimum, 0, p->address_length,
- flags | IORESOURCE_IO_FIXED);
+ IORESOURCE_IO_FIXED);
}
static __init void pnpacpi_parse_ext_address_option(struct pnp_dev *dev,
@@ -678,19 +649,16 @@
struct acpi_resource_extended_address64 *p = &r->data.ext_address64;
unsigned char flags = 0;
- if (p->address_length == 0)
- flags |= IORESOURCE_DISABLED;
-
if (p->resource_type == ACPI_MEMORY_RANGE) {
if (p->info.mem.write_protect == ACPI_READ_WRITE_MEMORY)
- flags |= IORESOURCE_MEM_WRITEABLE;
+ flags = IORESOURCE_MEM_WRITEABLE;
pnp_register_mem_resource(dev, option_flags, p->minimum,
p->minimum, 0, p->address_length,
flags);
} else if (p->resource_type == ACPI_IO_RANGE)
pnp_register_port_resource(dev, option_flags, p->minimum,
p->minimum, 0, p->address_length,
- flags | IORESOURCE_IO_FIXED);
+ IORESOURCE_IO_FIXED);
}
struct acpipnp_parse_option_s {
^ permalink raw reply [flat|nested] 4+ messages in thread
* 'flags' in rsparser.c change from 'register disabled resources' patch
@ 2011-08-22 0:59 Dr. David Alan Gilbert
2011-06-09 18:58 ` Witold Szczeponik
0 siblings, 1 reply; 4+ messages in thread
From: Dr. David Alan Gilbert @ 2011-08-22 0:59 UTC (permalink / raw)
To: Witold.Szczeponik; +Cc: linux-kernel, linux-acpi
Hi Witold,
'sparse' is giving me the following set of warnings in the latest
kernel:
CHECK drivers/pnp/pnpacpi/rsparser.c
drivers/pnp/pnpacpi/rsparser.c:515:26: warning: cast truncates bits from constant value (10000000 becomes 0)
drivers/pnp/pnpacpi/rsparser.c:533:26: warning: cast truncates bits from constant value (10000000 becomes 0)
drivers/pnp/pnpacpi/rsparser.c:553:26: warning: cast truncates bits from constant value (10000000 becomes 0)
drivers/pnp/pnpacpi/rsparser.c:578:26: warning: cast truncates bits from constant value (10000000 becomes 0)
drivers/pnp/pnpacpi/rsparser.c:593:26: warning: cast truncates bits from constant value (10000000 becomes 0)
drivers/pnp/pnpacpi/rsparser.c:606:26: warning: cast truncates bits from constant value (10000000 becomes 0)
drivers/pnp/pnpacpi/rsparser.c:621:26: warning: cast truncates bits from constant value (10000000 becomes 0)
drivers/pnp/pnpacpi/rsparser.c:636:26: warning: cast truncates bits from constant value (10000000 becomes 0)
drivers/pnp/pnpacpi/rsparser.c:660:26: warning: cast truncates bits from constant value (10000000 becomes 0)
drivers/pnp/pnpacpi/rsparser.c:682:26: warning: cast truncates bits from constant value (10000000 becomes 0)
these all seem to come from the 'flags' variables you added
as part of patch 29df8d8f being char's and IORESOURCE_DISABLED
being 0x10000000 so lines like:
+ unsigned char flags = 0;
+ flags |= IORESOURCE_DISABLED;
shouldn't work.
Dave
--
-----Open up your eyes, open up your mind, open up your code -------
/ Dr. David Alan Gilbert | Running GNU/Linux | Happy \
\ gro.gilbert @ treblig.org | | In Hex /
\ _________________________|_____ http://www.treblig.org |_______/
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] PNPACPI: Simplify disabled resource registration
2011-06-09 19:03 ` [PATCH] PNPACPI: Simplify disabled resource registration Witold Szczeponik
@ 2011-09-13 16:25 ` Bjorn Helgaas
0 siblings, 0 replies; 4+ messages in thread
From: Bjorn Helgaas @ 2011-09-13 16:25 UTC (permalink / raw)
To: Witold Szczeponik; +Cc: Dr. David Alan Gilbert, linux-kernel, linux-acpi
On Thu, Jun 9, 2011 at 1:03 PM, Witold Szczeponik
<Witold.Szczeponik@gmx.net> wrote:
> The attached patch simplifies 29df8d8f8702f0f53c1375015f09f04bc8d023c1. As
> the "pnp_xxx" structs are not designed to cope with IORESOURCE_DISABLED, and
> hence no code can test for this value, setting this value is actually a "no op"
> and can be skipped altogether. It is sufficient to remove the checks for
> "empty" resources and continue processing.
>
>
> Signed-off-by: Witold Szczeponik <Witold.Szczeponik@gmx.net>
Acked-by: Bjorn Helgaas <bhelgaas@google.com>
I think keeping all the resources is the right thing because we need
them for _SRS templates, as you mentioned.
Len normally takes PNP patches through his ACPI tree.
>
>
> diff -u linux/drivers/pnp/pnpacpi/rsparser.c linux/drivers/pnp/pnpacpi/rsparser.c
> --- linux/drivers/pnp/pnpacpi/rsparser.c
> +++ linux/drivers/pnp/pnpacpi/rsparser.c
> @@ -509,15 +509,12 @@
> struct acpi_resource_dma *p)
> {
> int i;
> - unsigned char map = 0, flags = 0;
> -
> - if (p->channel_count == 0)
> - flags |= IORESOURCE_DISABLED;
> + unsigned char map = 0, flags;
>
> for (i = 0; i < p->channel_count; i++)
> map |= 1 << p->channels[i];
>
> - flags |= dma_flags(dev, p->type, p->bus_master, p->transfer);
> + flags = dma_flags(dev, p->type, p->bus_master, p->transfer);
> pnp_register_dma_resource(dev, option_flags, map, flags);
> }
>
> @@ -527,17 +524,14 @@
> {
> int i;
> pnp_irq_mask_t map;
> - unsigned char flags = 0;
> -
> - if (p->interrupt_count == 0)
> - flags |= IORESOURCE_DISABLED;
> + unsigned char flags;
>
> bitmap_zero(map.bits, PNP_IRQ_NR);
> for (i = 0; i < p->interrupt_count; i++)
> if (p->interrupts[i])
> __set_bit(p->interrupts[i], map.bits);
>
> - flags |= irq_flags(p->triggering, p->polarity, p->sharable);
> + flags = irq_flags(p->triggering, p->polarity, p->sharable);
> pnp_register_irq_resource(dev, option_flags, &map, flags);
> }
>
> @@ -547,10 +541,7 @@
> {
> int i;
> pnp_irq_mask_t map;
> - unsigned char flags = 0;
> -
> - if (p->interrupt_count == 0)
> - flags |= IORESOURCE_DISABLED;
> + unsigned char flags;
>
> bitmap_zero(map.bits, PNP_IRQ_NR);
> for (i = 0; i < p->interrupt_count; i++) {
> @@ -564,7 +555,7 @@
> }
> }
>
> - flags |= irq_flags(p->triggering, p->polarity, p->sharable);
> + flags = irq_flags(p->triggering, p->polarity, p->sharable);
> pnp_register_irq_resource(dev, option_flags, &map, flags);
> }
>
> @@ -574,11 +565,8 @@
> {
> unsigned char flags = 0;
>
> - if (io->address_length == 0)
> - flags |= IORESOURCE_DISABLED;
> -
> if (io->io_decode == ACPI_DECODE_16)
> - flags |= IORESOURCE_IO_16BIT_ADDR;
> + flags = IORESOURCE_IO_16BIT_ADDR;
> pnp_register_port_resource(dev, option_flags, io->minimum, io->maximum,
> io->alignment, io->address_length, flags);
> }
> @@ -587,13 +575,8 @@
> unsigned int option_flags,
> struct acpi_resource_fixed_io *io)
> {
> - unsigned char flags = 0;
> -
> - if (io->address_length == 0)
> - flags |= IORESOURCE_DISABLED;
> -
> pnp_register_port_resource(dev, option_flags, io->address, io->address,
> - 0, io->address_length, flags | IORESOURCE_IO_FIXED);
> + 0, io->address_length, IORESOURCE_IO_FIXED);
> }
>
> static __init void pnpacpi_parse_mem24_option(struct pnp_dev *dev,
> @@ -602,11 +585,8 @@
> {
> unsigned char flags = 0;
>
> - if (p->address_length == 0)
> - flags |= IORESOURCE_DISABLED;
> -
> if (p->write_protect == ACPI_READ_WRITE_MEMORY)
> - flags |= IORESOURCE_MEM_WRITEABLE;
> + flags = IORESOURCE_MEM_WRITEABLE;
> pnp_register_mem_resource(dev, option_flags, p->minimum, p->maximum,
> p->alignment, p->address_length, flags);
> }
> @@ -617,11 +597,8 @@
> {
> unsigned char flags = 0;
>
> - if (p->address_length == 0)
> - flags |= IORESOURCE_DISABLED;
> -
> if (p->write_protect == ACPI_READ_WRITE_MEMORY)
> - flags |= IORESOURCE_MEM_WRITEABLE;
> + flags = IORESOURCE_MEM_WRITEABLE;
> pnp_register_mem_resource(dev, option_flags, p->minimum, p->maximum,
> p->alignment, p->address_length, flags);
> }
> @@ -632,11 +609,8 @@
> {
> unsigned char flags = 0;
>
> - if (p->address_length == 0)
> - flags |= IORESOURCE_DISABLED;
> -
> if (p->write_protect == ACPI_READ_WRITE_MEMORY)
> - flags |= IORESOURCE_MEM_WRITEABLE;
> + flags = IORESOURCE_MEM_WRITEABLE;
> pnp_register_mem_resource(dev, option_flags, p->address, p->address,
> 0, p->address_length, flags);
> }
> @@ -656,19 +630,16 @@
> return;
> }
>
> - if (p->address_length == 0)
> - flags |= IORESOURCE_DISABLED;
> -
> if (p->resource_type == ACPI_MEMORY_RANGE) {
> if (p->info.mem.write_protect == ACPI_READ_WRITE_MEMORY)
> - flags |= IORESOURCE_MEM_WRITEABLE;
> + flags = IORESOURCE_MEM_WRITEABLE;
> pnp_register_mem_resource(dev, option_flags, p->minimum,
> p->minimum, 0, p->address_length,
> flags);
> } else if (p->resource_type == ACPI_IO_RANGE)
> pnp_register_port_resource(dev, option_flags, p->minimum,
> p->minimum, 0, p->address_length,
> - flags | IORESOURCE_IO_FIXED);
> + IORESOURCE_IO_FIXED);
> }
>
> static __init void pnpacpi_parse_ext_address_option(struct pnp_dev *dev,
> @@ -678,19 +649,16 @@
> struct acpi_resource_extended_address64 *p = &r->data.ext_address64;
> unsigned char flags = 0;
>
> - if (p->address_length == 0)
> - flags |= IORESOURCE_DISABLED;
> -
> if (p->resource_type == ACPI_MEMORY_RANGE) {
> if (p->info.mem.write_protect == ACPI_READ_WRITE_MEMORY)
> - flags |= IORESOURCE_MEM_WRITEABLE;
> + flags = IORESOURCE_MEM_WRITEABLE;
> pnp_register_mem_resource(dev, option_flags, p->minimum,
> p->minimum, 0, p->address_length,
> flags);
> } else if (p->resource_type == ACPI_IO_RANGE)
> pnp_register_port_resource(dev, option_flags, p->minimum,
> p->minimum, 0, p->address_length,
> - flags | IORESOURCE_IO_FIXED);
> + IORESOURCE_IO_FIXED);
> }
>
> struct acpipnp_parse_option_s {
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2011-09-13 16:25 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-08-22 0:59 'flags' in rsparser.c change from 'register disabled resources' patch Dr. David Alan Gilbert
2011-06-09 18:58 ` Witold Szczeponik
2011-06-09 19:03 ` [PATCH] PNPACPI: Simplify disabled resource registration Witold Szczeponik
2011-09-13 16:25 ` Bjorn Helgaas
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).