Skip to contentSkip to conversation

'flags' in rsparser.c change from 'register disabled resources' patch

PATCH Superseded Dr. David Alan Gilbert 3 participants 4 messages Jun 9, 2011 – Sep 13, 2011

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

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

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   |_______/
#1 lore raw

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

#2 lore raw

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
diff -u 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 {
Acked-by on v1 1/1
#3 lore raw

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

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.c linux/drivers/pnp/pnpacpi/rsparser.c

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