* '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: '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
* 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).