'flags' in rsparser.c change from 'register disabled resources' patch
Subsystems (2)
Hi Witold,
'sparse' is giving me the following set of warnings in the latest
kernel:
CHECK drivers/pnp/pnpacpi/rsparser.cdrivers/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
Signature · 4 lines
-----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 |_______/
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
PATCH 1/1 PNPACPI: Simplify disabled resource registration
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.
drivers/pnp/pnpacpi/rsparser.c +15 -47
@@ -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 {
Re: [PATCH] PNPACPI: Simplify disabled resource registration
Quoted 8 lines · Witold Szczeponik
The attached patch simplifies
29df8d8f8702f0f53c1375015f09f04bc8d023c1. As the "pnp_xxx" structs are not designed to cope withIORESOURCE_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 <[email protected]>
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.
Quoted 177 lines
diff -u
linux/drivers/pnp/pnpacpi/rsparser.clinux/drivers/pnp/pnpacpi/rsparser.cdrivers/pnp/pnpacpi/rsparser.c +15
-48@@ -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 { --
Quoted 4 lines
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to [email protected]
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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html