Re: [PATCH part2 3/4] acpi: Remove "continue" in macro INVALID_TABLE().

2013-08-12 Thread Tang Chen 

On 08/12/2013 10:21 PM, Tejun Heo wrote:

On Thu, Aug 08, 2013 at 07:09:53AM -0700, Joe Perches wrote:

If you really think that the #define is better, use
something like HW_ERR does and embed that #define
in the pr_err.

#define ACPI_OVERRIDE   "ACPI OVERRIDE: "

pr_err(ACPI_OVERRIDE "Table smaller than ACPI header [%s%s]\n",
   cpio_path, file.name);

It's only used a few times by a single file so
I think it's unnecessary.


I agree with Joe here.  Just doing normal pr_err() should be enough.
You can use pr_fmt() to add headers but given that we aren't talking
about huge number of printks, that probably is an overkill too.


OK, followed.

Thanks.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH part2 3/4] acpi: Remove "continue" in macro INVALID_TABLE().

2013-08-12 Thread Tejun Heo 
On Thu, Aug 08, 2013 at 07:09:53AM -0700, Joe Perches wrote:
> If you really think that the #define is better, use
> something like HW_ERR does and embed that #define
> in the pr_err.
> 
> #define ACPI_OVERRIDE "ACPI OVERRIDE: "
> 
>   pr_err(ACPI_OVERRIDE "Table smaller than ACPI header [%s%s]\n",
>  cpio_path, file.name);
> 
> It's only used a few times by a single file so
> I think it's unnecessary.

I agree with Joe here.  Just doing normal pr_err() should be enough.
You can use pr_fmt() to add headers but given that we aren't talking
about huge number of printks, that probably is an overkill too.

Thanks.

-- 
tejun
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH part2 3/4] acpi: Remove "continue" in macro INVALID_TABLE().

2013-08-08 Thread Joe Perches 
On Thu, 2013-08-08 at 20:18 +0800, Tang Chen wrote:
> Hi Joe,

Hello Tang.

> On 08/08/2013 01:27 PM, Joe Perches wrote:
> > On Thu, 2013-08-08 at 13:03 +0800, Tang Chen wrote:
> >
> >> Change it to the style like other macros:
> >>
> >>   #define INVALID_TABLE(x, path, name)\
> >>   do { pr_err("ACPI OVERRIDE: " x " [%s%s]\n", path, name); } 
> >> while (0)
> >
> > Single statement macros do _not_ need to use
> > "do { foo(); } while (0)"
> > and should be written as
> > "foo()"
> 
> OK, will remove the do {} while (0).
> 
> But I think we'd better keep the macro, or rename it to something
> more meaningful. At least we can use it to avoid adding "ACPI OVERRIDE:"
> prefix every time. Maybe this is why it is defined.

No, it's just silly.

If you really think that the #define is better, use
something like HW_ERR does and embed that #define
in the pr_err.

#define ACPI_OVERRIDE   "ACPI OVERRIDE: "

pr_err(ACPI_OVERRIDE "Table smaller than ACPI header [%s%s]\n",
   cpio_path, file.name);

It's only used a few times by a single file so
I think it's unnecessary.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH part2 3/4] acpi: Remove "continue" in macro INVALID_TABLE().

2013-08-08 Thread Tang Chen 

Hi Joe,


On 08/08/2013 01:27 PM, Joe Perches wrote:

On Thu, 2013-08-08 at 13:03 +0800, Tang Chen wrote:


Change it to the style like other macros:

  #define INVALID_TABLE(x, path, name)\
  do { pr_err("ACPI OVERRIDE: " x " [%s%s]\n", path, name); } while (0)


Single statement macros do _not_ need to use
"do { foo(); } while (0)"
and should be written as
"foo()"


OK, will remove the do {} while (0).

But I think we'd better keep the macro, or rename it to something
more meaningful. At least we can use it to avoid adding "ACPI OVERRIDE:"
prefix every time. Maybe this is why it is defined.

Thanks. :)




diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c

[]

@@ -564,8 +564,8 @@ static const char * const table_sigs[] = {
ACPI_SIG_RSDT, ACPI_SIG_XSDT, ACPI_SIG_SSDT, NULL };

  /* Non-fatal errors: Affected tables/files are ignored */
-#define INVALID_TABLE(x, path, name)   \
-   { pr_err("ACPI OVERRIDE: " x " [%s%s]\n", path, name); continue; }
+#define ACPI_INVALID_TABLE(x, path, name)  
\
+   do { pr_err("ACPI OVERRIDE: " x " [%s%s]\n", path, name); } while (0)


Just remove the silly macro altogether


@@ -593,9 +593,11 @@ void __init acpi_initrd_override(void *data, size_t size)

[]

-   if (file.size<  sizeof(struct acpi_table_header))
-   INVALID_TABLE("Table smaller than ACPI header",
+   if (file.size<  sizeof(struct acpi_table_header)) {
+   ACPI_INVALID_TABLE("Table smaller than ACPI header",
  cpio_path, file.name);


and use the normal style

pr_err("ACPI OVERRIDE: Table smaller than ACPI header 
[%s%s]\n",
   cpio_path, file.name);


@@ -603,15 +605,21 @@ void __init acpi_initrd_override(void *data, size_t size)


etc...



--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH part2 3/4] acpi: Remove "continue" in macro INVALID_TABLE().

2013-08-07 Thread Joe Perches 
On Thu, 2013-08-08 at 13:03 +0800, Tang Chen wrote:

> Change it to the style like other macros:
> 
>  #define INVALID_TABLE(x, path, name)\
>  do { pr_err("ACPI OVERRIDE: " x " [%s%s]\n", path, name); } while (0)

Single statement macros do _not_ need to use 
"do { foo(); } while (0)"
and should be written as
"foo()"

> diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
[]
> @@ -564,8 +564,8 @@ static const char * const table_sigs[] = {
>   ACPI_SIG_RSDT, ACPI_SIG_XSDT, ACPI_SIG_SSDT, NULL };
>  
>  /* Non-fatal errors: Affected tables/files are ignored */
> -#define INVALID_TABLE(x, path, name) \
> - { pr_err("ACPI OVERRIDE: " x " [%s%s]\n", path, name); continue; }
> +#define ACPI_INVALID_TABLE(x, path, name)
> \
> + do { pr_err("ACPI OVERRIDE: " x " [%s%s]\n", path, name); } while (0)

Just remove the silly macro altogether

> @@ -593,9 +593,11 @@ void __init acpi_initrd_override(void *data, size_t size)
[]
> - if (file.size < sizeof(struct acpi_table_header))
> - INVALID_TABLE("Table smaller than ACPI header",
> + if (file.size < sizeof(struct acpi_table_header)) {
> + ACPI_INVALID_TABLE("Table smaller than ACPI header",
> cpio_path, file.name);

and use the normal style

pr_err("ACPI OVERRIDE: Table smaller than ACPI header 
[%s%s]\n",
   cpio_path, file.name);

> @@ -603,15 +605,21 @@ void __init acpi_initrd_override(void *data, size_t 
> size)

etc...

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH part2 3/4] acpi: Remove "continue" in macro INVALID_TABLE().

2013-08-07 Thread Tang Chen 
The macro INVALID_TABLE() is defined like this:

 #define INVALID_TABLE(x, path, name)\
 { pr_err("ACPI OVERRIDE: " x " [%s%s]\n", path, name); continue; }

And it is used like this:

for (...) {
...
if (...)
INVALID_TABLE()
...
}

The "continue" in the macro makes the code hard to understand.
Change it to the style like other macros:

 #define INVALID_TABLE(x, path, name)\
 do { pr_err("ACPI OVERRIDE: " x " [%s%s]\n", path, name); } while (0)

And also, INVALID_TABLE() is used to checkout acpi tables, so rename it to
ACPI_INVALID_TABLE(). This is suggested by Toshi Kani .

So after this patch, this macro should be used like this:

for (...) {
...
if (...) {
ACPI_INVALID_TABLE()
continue;
}
...
}

Add the "continue" wherever the macro is called.
(For now, it is only called in acpi_initrd_override().)

The idea is from Yinghai Lu .

Signed-off-by: Tang Chen 
Signed-off-by: Yinghai Lu 
Acked-by: Tejun Heo 
Acked-by: Rafael J. Wysocki 
Acked-by: Toshi Kani 
Reviewed-by: Zhang Yanfei 
---
 drivers/acpi/osl.c |   28 ++--
 1 files changed, 18 insertions(+), 10 deletions(-)

diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
index 6ab2c35..3b8bab2 100644
--- a/drivers/acpi/osl.c
+++ b/drivers/acpi/osl.c
@@ -564,8 +564,8 @@ static const char * const table_sigs[] = {
ACPI_SIG_RSDT, ACPI_SIG_XSDT, ACPI_SIG_SSDT, NULL };
 
 /* Non-fatal errors: Affected tables/files are ignored */
-#define INVALID_TABLE(x, path, name)   \
-   { pr_err("ACPI OVERRIDE: " x " [%s%s]\n", path, name); continue; }
+#define ACPI_INVALID_TABLE(x, path, name)  
\
+   do { pr_err("ACPI OVERRIDE: " x " [%s%s]\n", path, name); } while (0)
 
 #define ACPI_HEADER_SIZE sizeof(struct acpi_table_header)
 
@@ -593,9 +593,11 @@ void __init acpi_initrd_override(void *data, size_t size)
data += offset;
size -= offset;
 
-   if (file.size < sizeof(struct acpi_table_header))
-   INVALID_TABLE("Table smaller than ACPI header",
+   if (file.size < sizeof(struct acpi_table_header)) {
+   ACPI_INVALID_TABLE("Table smaller than ACPI header",
  cpio_path, file.name);
+   continue;
+   }
 
table = file.data;
 
@@ -603,15 +605,21 @@ void __init acpi_initrd_override(void *data, size_t size)
if (!memcmp(table->signature, table_sigs[sig], 4))
break;
 
-   if (!table_sigs[sig])
-   INVALID_TABLE("Unknown signature",
+   if (!table_sigs[sig]) {
+   ACPI_INVALID_TABLE("Unknown signature",
  cpio_path, file.name);
-   if (file.size != table->length)
-   INVALID_TABLE("File length does not match table length",
+   continue;
+   }
+   if (file.size != table->length) {
+   ACPI_INVALID_TABLE("File length does not match table 
length",
  cpio_path, file.name);
-   if (acpi_table_checksum(file.data, table->length))
-   INVALID_TABLE("Bad table checksum",
+   continue;
+   }
+   if (acpi_table_checksum(file.data, table->length)) {
+   ACPI_INVALID_TABLE("Bad table checksum",
  cpio_path, file.name);
+   continue;
+   }
 
pr_info("%4.4s ACPI table found in initrd [%s%s][0x%x]\n",
table->signature, cpio_path, file.name, table->length);
-- 
1.7.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/